On 10/24/2024 16:06, Bjorn Helgaas wrote:
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.
Yeah; this I believe is why we're here today and trying to find
something more manageable (IE this series).
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")
Isn't the subsystem ID based typically upon the platform it's running
on? For example I seem to recall on Dell systems it's used the value
that was in the SBMIOS ProductSKU field here (IoW not something AMD
would control).
I mean I guess maybe we could do a:
.vendor = PCI_VENDOR_ID_AMD,
.device = PCI_ANY_ID,
.class = PCI_CLASS_BRIDGE_HOST << 8
And then in probe() figure out if it's the right one, but that's still
pretty ugly, eh?
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.
Right; I agree none of that matters here.
