Re: [PATCH 00/16] AMD NB and SMN rework

[Bjorn Helgaas <helgaas@kernel.org>]

On Thu, Oct 24, 2024 at 03:08:41PM -0500, Mario Limonciello wrote:
> On 10/24/2024 12:46, Bjorn Helgaas wrote:
> > On Thu, Oct 24, 2024 at 12:01:59PM -0400, Yazen Ghannam wrote:
> > > On Wed, Oct 23, 2024 at 12:59:28PM -0500, Bjorn Helgaas wrote:
> > > > On Wed, Oct 23, 2024 at 05:21:34PM +0000, Yazen Ghannam wrote:
> ...

> > > > The use of pci_get_slot() and pci_get_domain_bus_and_slot() is not
> > > > ideal since all those pci_get_*() interfaces are kind of ugly in my
> > > > opinion, and using them means we have to encode topology details in
> > > > the kernel.  But this still seems like a big improvement.
> > > 
> > > Thanks for the feedback. Hopefully, we'll come to some improved
> > > solution. :)
> > > 
> > > Can you please elaborate on your concern? Is it about saying "thing X is
> > > always at SBDF A:B:C.D" or something else?
> > 
> > "Thing X is always at SBDF A:B:C.D" is one big reason.  "A:B:C.D" says
> > nothing about the actual functionality of the device.  A PCI
> > Vendor/Device ID or a PNP ID identifies the device programming model
> > independent of its geographical location.  Inferring the functionality
> > and programming model from the location is a maintenance issue because
> > hardware may change the address.
> > 
> > PCI bus numbers are under software control, so in general it's not
> > safe to rely on them, although in this case these devices are probably
> > on root buses where the bus number is either fixed or determined by
> > BIOS configuration of the host bridge.
> > 
> > I don't like the pci_get_*() functions because they break the driver
> > model.  The usual .probe() model binds a device to a driver, which
> > essentially means the driver owns the device and its resources, and
> > the driver and doesn't have to worry about other code interfering.
> 
> Are you suggesting that perhaps we should be introducing amd_smn (patch 10)
> as a PCI driver that binds "to the root device" instead?

I don't know any of the specifics, so I can't really opine on that.

The PCI specs envision that a Vendor/Device ID defines the programming
model of the device, and you would only use a new Device ID when that
programming model changes.

Of course, vendors like to define a new set of Device IDs for every
new chipset even when no driver changes are required, so even if a new
SMN works exactly the same as in previous chipsets, you're probably
back to having to add a new Device ID for every new chipset.

The Subsystem Vendor ID and Subsystem ID exist to solve a similar
problem (sort of in reverse).  If AMD could allocate a Subsystem ID
for this SMN programming model and use that same ID in every chipset,
you could make a pci_driver.id_table entry that would match them all,
e.g.,

  .vendor = PCI_VENDOR_ID_AMD,
  .device = PCI_ANY_ID,
  .subvendor = PCI_VENDOR_ID_AMD,
  .subdevice = PCI_SUBSYSTEM_AMD_SMN,

(pci_device_id.subdevice is misnamed; the spec calls it "Subsystem ID")

> There are some areas that do discovery (for example amd_node_get_root() in
> patch 6/16).

Sort of.  amd_node_get_root() and amd_node_get_func() both just grub
through all the devices that the PCI core has enumerated and return
the one that has the right geographical address.

There's no binding to a driver, so another driver could come along and
bind to the same device, and then you have a potential conflict.

You also give up all the standard driver model infrastructure for
hotplug, power management, etc.  Granted, you probably don't care
about those things here.

Bjorn