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

[Niklas Schnelle <schnelle@linux.ibm.com>]

On Fri, 2025-09-26 at 16:27 +0200, Benjamin Block wrote:
> > 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,
> > 
--- snip --
> > +extern const struct attribute_group zpci_slot_attr_group;
> > +
> > +#define ARCH_PCI_SLOT_GROUPS (&zpci_slot_attr_group)
> 
> I don't know the history exactly, but this can't be easily extended like the
> other group above `ARCH_PCI_DEV_GROUPS`.
> 
>     (&zpci_slot_attr_group,  \
>      &zpci_slot_attr_group_b)
> 
> Won't compile. The way `ARCH_PCI_DEV_GROUPS` does it, attaching a different
> group is just adding a new line.

Without the parenthesis it should. I only added them because otherwise
checkpatch complains and it's still valid for a single item to have
parenthesis.

> 
> > 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);
> >  
> > --- snip ---
> > +{
> > +	struct zpci_dev *zdev = container_of(slot->hotplug, struct zpci_dev,
> > +					     hotplug_slot);
> > +
> > +	return sysfs_emit(buf, "0x%x\n", zdev->uid);
> 
> Do we need a special case for when `uid` is 0? That would imply the uid is
> invalid, right? Would we want to return an error in that case (-EINVAL, or
> smth)? 
> 
> Also, do we want to use the same format as in `zpci_setup_bus_resources()`
> with the 4 leading 0's (`0x%04x`)?

I chose to match the "uid" device attribute here ("zpci_attr(uid,
"0x%x\n", uid)" in the beginning of the same file). This doesn't
special case UID 0. You are right that this is an invalid UID though.
It also still exposes the UID even if it is not guaranteed to be
unique. But we'll make that setting known to user-space separately.
I feel like knowing the UIDs can still be helpful even when they are
not unique, for example to check that they've been set correctly from
within Linux before enabling UID Checking.

> 
> > +}
> > +
> > +static struct pci_slot_attribute zpci_slot_attr_uid =
> > +	__ATTR(uid, 0444, zpci_uid_slot_show, NULL);
> 
> __ATTR_RO()?