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

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

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 ???
> 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.
>
> 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.


> diff --git a/arch/s390/pci/pci_sysfs.c b/arch/s390/pci/pci_sysfs.c
> index 0ee0924cfab7e5d22468fb197ee78cac679d8c13..997dff3796094680d9a3f0b6eb27a89fa1ed30b2 100644
> --- a/arch/s390/pci/pci_sysfs.c
> +++ b/arch/s390/pci/pci_sysfs.c
> @@ -178,6 +178,17 @@ static ssize_t index_show(struct device *dev,
>   }
>   static DEVICE_ATTR_RO(index);
>   
> +static ssize_t zpci_uid_slot_show(struct pci_slot *slot, char *buf)
> +{
> +	struct zpci_dev *zdev = container_of(slot->hotplug, struct zpci_dev,
> +					     hotplug_slot);
> +
> +	return sysfs_emit(buf, "0x%x\n", zdev->uid);
> +}
> +
> +static struct pci_slot_attribute zpci_slot_attr_uid =
> +	__ATTR(uid, 0444, zpci_uid_slot_show, NULL);
> +
>   static umode_t zpci_index_is_visible(struct kobject *kobj,
>   				     struct attribute *attr, int n)
>   {
> @@ -233,3 +244,12 @@ const struct attribute_group pfip_attr_group = {
>   	.name = "pfip",
>   	.attrs = pfip_attrs,
>   };
> +
> +static struct attribute *zpci_slot_attrs[] = {
> +	&zpci_slot_attr_uid.attr,
> +	NULL,
> +};
> +
> +const struct attribute_group zpci_slot_attr_group = {
> +	.attrs = zpci_slot_attrs,
> +};
> diff --git a/drivers/pci/slot.c b/drivers/pci/slot.c
> index 50fb3eb595fe65e271b6b339d43c9677c61b1e45..7430c7df44e1beef7bcf0531491612734e0dd60c 100644
> --- a/drivers/pci/slot.c
> +++ b/drivers/pci/slot.c
> @@ -96,7 +96,18 @@ static struct attribute *pci_slot_default_attrs[] = {
>   	&pci_slot_attr_cur_speed.attr,
>   	NULL,
>   };
> -ATTRIBUTE_GROUPS(pci_slot_default);
> +
> +static const struct attribute_group pci_slot_default_group = {
> +	.attrs = pci_slot_default_attrs,
> +};
> +
> +const struct attribute_group *pci_slot_default_groups[] = {
> +	&pci_slot_default_group,
> +#ifdef ARCH_PCI_SLOT_GROUPS
> +	ARCH_PCI_SLOT_GROUPS,
> +#endif
> +	NULL,
> +};
>   
>   static const struct kobj_type pci_slot_ktype = {
>   	.sysfs_ops = &pci_slot_sysfs_ops,
>
> ---
> base-commit: cec1e6e5d1ab33403b809f79cd20d6aff124ccfe
> change-id: 20250923-uid_slot-e3559cf5ca30
>
> Best regards,