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

[Gerd Bayer <gbayer@linux.ibm.com>]

On Wed, 2025-10-15 at 15:42 +0200, Niklas Schnelle wrote:
> 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.
> 
> The first identifier, the FID, 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 create the same configuration based on FIDs on two
> different LPARs of the same machine, and difficult to reuse across
> machines.
> 
> Such matching LPAR configurations are useful, though, allowing
> standardized setups and booting a Linux installation on different LPARs.
> To this end the UID, or user-defined identifier, was introduced. While
> it is only guaranteed to be unique within an LPAR and only if indicated
> by firmware, it allows users to replicate PCI device 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 the slot name 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.

Hi Niklas,

I like this addition a lot. Also, Lukas' method to add arch-specific
attributes to sysfs. Is there a reason why you didn't apply that
mechanism 1-to-1?

> 
> 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 felt like there is probably too little commonality on
> format and usage patterns

Sorry for my ignorance but how is the hotplug slot driver defining an
"index" or how is used?


> v2->v3:
> - Rebase on v6.18-rc1 and resolve conflict with recent s390 PCI sysfs change
> - Link to v2: https://lore.kernel.org/r/20251008-uid_slot-v2-1-ef22cef27741@linux.ibm.com
> ---
>  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(-)
> 

  [ ... snip ... ]

> diff --git a/drivers/pci/slot.c b/drivers/pci/slot.c
> index 50fb3eb595fe65e271b6b339d43c9677c61b1e45..b09e7852c33ed4957432ac73b36d181ecd8283a1 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,
> +};
> +
> +static const struct attribute_group *pci_slot_default_groups[] = {
> +	&pci_slot_default_group,
> +#ifdef ARCH_PCI_SLOT_GROUPS
> +	ARCH_PCI_SLOT_GROUPS,
> +#endif
> +	NULL,
> +};
>  

With the following diff you could avoid the #ifdef directive in the
middle of the definition of the attribute_group - shamelessly stolen
from Lukas' patch


diff --git a/arch/s390/include/asm/pci.h b/arch/s390/include/asm/pci.h
index 8e5e0f995e91..1d8105c4d2a6 100644
--- a/arch/s390/include/asm/pci.h
+++ b/arch/s390/include/asm/pci.h
@@ -208,7 +208,7 @@ extern const struct attribute_group
zpci_ident_attr_group;
 
 extern const struct attribute_group zpci_slot_attr_group;
 
-#define ARCH_PCI_SLOT_GROUPS (&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;
diff --git a/drivers/pci/slot.c b/drivers/pci/slot.c
index b09e7852c33e..9ba7fc0066bf 100644
--- a/drivers/pci/slot.c
+++ b/drivers/pci/slot.c
@@ -11,6 +11,10 @@
 #include <linux/err.h>
 #include "pci.h"
 
+#ifndef ARCH_PCI_SLOT_GROUPS
+#define ARCH_PCI_SLOT_GROUPS
+#endif
+
 struct kset *pci_slots_kset;
 EXPORT_SYMBOL_GPL(pci_slots_kset);
 
@@ -103,9 +107,7 @@ static const struct attribute_group
pci_slot_default_group = {
 
 static const struct attribute_group *pci_slot_default_groups[] = {
        &pci_slot_default_group,
-#ifdef ARCH_PCI_SLOT_GROUPS
-       ARCH_PCI_SLOT_GROUPS,
-#endif
+       ARCH_PCI_SLOT_GROUPS
        NULL,
 };

Thanks,
Gerd