Re: [PATCH] PCI: s390: Expose the UID as an arch specific PCI slot attribute

[Ramesh Errabolu <ramesh@linux.ibm.com>]

On 9/26/2025 1:36 PM, Niklas Schnelle wrote:
> On Fri, 2025-09-26 at 11:34 -0500, Ramesh Errabolu wrote:
>> On 9/24/2025 8:14 AM, Niklas Schnelle wrote:
>>
>>> On s390, an individual PCI function can generally be identified by two
>>> IDs, depending on the scope and the platform configuration.
>> It would help to name the two IDs - FID and ???
> How about:
> "On s390, an individual PCI function can generally be identified by two
> identifiers, the FID and the UID. Which identifier is used depends on
> the scope and the platform configuration."
>
> And then reword the below without "so-called".
That will help a lot
>>> The first ID is the so-called FID, which is always available and
>>> identifies a PCI device uniquely within a machine. The FID may be
>>> virtualized by hypervisors, but on the LPAR level, the machine scope
>>> makes it impossible to reuse the same configuration based on FIDs on two
>>> different LPARs.
>>>
>>> Such matching LPAR configurations are useful, though, allowing
>>> standardized setups and booting a Linux installation on different
>>> machines. To allow this, a second user-defined identifier called UID was
>>> introduced. It is only guaranteed to be unique within an LPAR and only
>>> if the platform indicates so via the UID Checking flag.
>> The paragraph as I read is not clear. Your intention is to highlight the
>> need for UID to allow standardized setups.
> Yes, that was my intention. Also here is where the second ID is
> introduced so I'll reword this a bit if the name is already mentioned
> in the first paragraph.
Will await your next update
>>> On s390, which uses a machine hypervisor, a per PCI function hotplug
>>> model is used. The shortcoming with the UID then is, that it is not
>>> visible to the user without first attaching the PCI function and
>>> accessing the "uid" device attribute. The FID, on the other hand, is
>>> used as slot number and is thus known even with the PCI function in
>>> standby.
>>>
>>> Remedy this shortcoming by providing the UID as an attribute on the slot
>>> allowing the user to identify a PCI function based on the UID without
>>> having to first attach it. Do this via a macro mechanism analogous to
>>> what was introduced by commit 265baca69a07 ("s390/pci: Stop usurping
>>> pdev->dev.groups") for the PCI device attributes.
>>>
>>> Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
>>> ---
>>> Note: I considered adding the UID as a generic "index" via the hotplug
>>> slot driver but opted for a minimal solution to open the discussion. In
>>> particular my concern with a generic attribute is that it would be hard
>>> to find a format that fits everyone. For example on PCI devices we also
>>> use the "index" attribute for UIDs analogous to SMBIOS but having it in
>>> decimal is odd on s390 where these are usual in hexadecimal.
>>> ---
>>>    arch/s390/include/asm/pci.h |  4 ++++
>>>    arch/s390/pci/pci_sysfs.c   | 20 ++++++++++++++++++++
>>>    drivers/pci/slot.c          | 13 ++++++++++++-
>>>    3 files changed, 36 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch/s390/include/asm/pci.h b/arch/s390/include/asm/pci.h
>>> index 41f900f693d92522ff729829e446b581977ef3ff..23eed78d9dce72ef466679f31c78aca52ba00f99 100644
>>> --- a/arch/s390/include/asm/pci.h
>>> +++ b/arch/s390/include/asm/pci.h
>>> @@ -207,6 +207,10 @@ extern const struct attribute_group zpci_ident_attr_group;
>>>    			    &pfip_attr_group,		 \
>>>    			    &zpci_ident_attr_group,
>>>    
>>> +extern const struct attribute_group zpci_slot_attr_group;
>>> +
>>> +#define ARCH_PCI_SLOT_GROUPS (&zpci_slot_attr_group)
>>> +
>>>    extern unsigned int s390_pci_force_floating __initdata;
>>>    extern unsigned int s390_pci_no_rid;
>>>    
>> Will this not lead to linking error when the patch is built on non-s390
>> architecture. You could refer to zpci_slot_attr_group using a
>> CONFIG_..... and discard the #define ARCH_PCI_SLOT_GROUPS. I didn't find
>> a relevant CONFIG_... that could be used.
> This code is in arch/s390/ it will not be build on non-s390. For the
> non s390 case ARCH_PCI_SLOT_GROUPS will be undefined and the #ifdef in
> slot.c makes sure we're not trying to insert ARCH_PCI_SLOT_GROUPs in
> the array as it is not defined.
You are right, I completely overlooked it. My comment is incorrect.
>
> Thanks,
>
> Niklas