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

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

On Tue, 2025-09-30 at 11:09 +0200, Benjamin Block wrote:
> On Fri, Sep 26, 2025 at 08:25:07PM +0200, Niklas Schnelle wrote:
> > On Fri, 2025-09-26 at 16:27 +0200, Benjamin Block wrote:
> > > > +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.
> 
> It's not like checkpatch is the last arbiter of truth here, especially sind we
> already have a case without parenthesis; but I guess if we ever need to extend
> the list, we can remove the parenthesis again.

Yes that was my thought too.

> 
> > > > 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).
> 
> Ah right, fair enough.
> 
> > 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.
> 
> I don't mind the case where the UID is not checked for uniqueness (the naming
> is confusing in any case, since the U doesn't stand for unique), I was just
> wondering whether printing an invalid UID makes sense. I think those are
> distinct cases. 
> It might be easier to 'encode' this knowledge about an UID being "invalid"
> here, rather than 'encoding' it in every single user that might read this
> attribute.
> 
> I guess the same can be said for the old `uid` attribute that is attached to
> the PCI device, but that was introduced by Sebastian a long time ago.

My thought here is that any program that deals with this new uid
attribute would potentially also deal with the existing per-device
attribute. I'd not want to change the existing attribute so then I
think it is best if applications just have to deal with a single "UID
as '0x' prefixed hex with 0 meaning unset" encoding. It's also that
this invalid/unset case does happen as the machine configuration does
allow UIDs to be unset, so it's not really an error case.

Thanks,
Niklas