On Wed, 2021-04-14 at 15:17 -0500, Bjorn Helgaas wrote:
> On Mon, Apr 12, 2021 at 03:59:05PM +0200, Niklas Schnelle wrote:
> > On s390 each PCI device has a user-defined ID (UID) exposed under
> > /sys/bus/pci/devices/<dev>/uid. This ID was designed to serve as the PCI
> > device's primary index and to match the device within Linux to the
> > device configured in the hypervisor. To serve as a primary identifier
> > the UID must be unique within the Linux instance, this is guaranteed by
> > the platform if and only if the UID Uniqueness Checking flag is set
> > within the CLP List PCI Functions response.
> >
> > In this sense the UID serves an analogous function as the SMBIOS
> > instance number or ACPI index exposed as the "index" respectively
> > "acpi_index" device attributes and used by e.g. systemd to set interface
> > names. As s390 does not use and will likely never use ACPI nor SMBIOS
> > there is no conflict and we can just expose the UID under the "index"
> > attribute whenever UID Uniqueness Checking is active and get systemd's
> > interface naming support for free.
> >
> > Signed-off-by: Niklas Schnelle <schne...@linux.ibm.com>
> > Acked-by: Viktor Mihajlovski <mihaj...@linux.ibm.com>
>
> This seems like a nice solution to me.
>
> Acked-by: Bjorn Helgaas <bhelg...@google.com>
Thanks! Yes I agree it's a simple solution that also makes sense from a
design point. I'll wait for Narendra's opinion of course.
>
> > ---
> > Documentation/ABI/testing/sysfs-bus-pci | 11 +++++---
> > arch/s390/pci/pci_sysfs.c | 35 +++++++++++++++++++++++++
> > 2 files changed, 42 insertions(+), 4 deletions(-)
> >
> > diff --git a/Documentation/ABI/testing/sysfs-bus-pci
> > b/Documentation/ABI/testing/sysfs-bus-pci
> > index 25c9c39770c6..1241b6d11a52 100644
> > --- a/Documentation/ABI/testing/sysfs-bus-pci
> > +++ b/Documentation/ABI/testing/sysfs-bus-pci
> > @@ -195,10 +195,13 @@ What: /sys/bus/pci/devices/.../index
> > Date: July 2010
> > Contact: Narendra K <narendr...@dell.com>, linux-b...@dell.com
> > Description:
> > - Reading this attribute will provide the firmware
> > - given instance (SMBIOS type 41 device type instance) of the
> > - PCI device. The attribute will be created only if the firmware
> > - has given an instance number to the PCI device.
> > + Reading this attribute will provide the firmware given instance
> > + number of the PCI device. Depending on the platform this can
> > + be for example the SMBIOS type 41 device type instance or the
> > + user-defined ID (UID) on s390. The attribute will be created
> > + only if the firmware has given an instance number to the PCI
> > + device and that number is guaranteed to uniquely identify the
> > + device in the system.
> > Users:
> > Userspace applications interested in knowing the
> > firmware assigned device type instance of the PCI
> > diff --git a/arch/s390/pci/pci_sysfs.c b/arch/s390/pci/pci_sysfs.c
> > index e14d346dafd6..20dbb2058d51 100644
> > --- a/arch/s390/pci/pci_sysfs.c
> > +++ b/arch/s390/pci/pci_sysfs.c
> > @@ -138,6 +138,38 @@ static ssize_t uid_is_unique_show(struct device *dev,
> > }
> > static DEVICE_ATTR_RO(uid_is_unique);
> >
> > +#ifndef CONFIG_DMI
> > +/* analogous to smbios index */
>
> I think this is smbios_attr_instance, right? Maybe mention that
> specifically to make it easier to match these up.
>
> Looks like smbios_attr_instance and the similar ACPI stuff could use
> some updating to use the current attribute group infrastructure.
>
> > +static ssize_t index_show(struct device *dev,
> > + struct device_attribute *attr, char *buf)
> > +{
> > + struct zpci_dev *zdev = to_zpci(to_pci_dev(dev));
> > + u32 index = ~0;
> > +
> > + if (zpci_unique_uid)
> > + index = zdev->uid;
> > +
> > + return sysfs_emit(buf, "%u\n", index);
> > +}
> > +static DEVICE_ATTR_RO(index);
> > +
> > +static umode_t zpci_unique_uids(struct kobject *kobj,
> > + struct attribute *attr, int n)
> > +{
> > + return zpci_unique_uid ? attr->mode : 0;
> > +}
> > +
> > +static struct attribute *zpci_ident_attrs[] = {
> > + &dev_attr_index.attr,
> > + NULL,
> > +};
> > +
> > +static struct attribute_group zpci_ident_attr_group = {
> > + .attrs = zpci_ident_attrs,
> > + .is_visible = zpci_unique_uids,
>
> It's conventional to name these functions *_is_visible() (another
> convention that smbios_attr_instance and acpi_attr_index probably
> predate).
Thanks, will change. Since he function then references the attribtue
instead of the condition, I'll go with zpci_index_is_visible().
>
> > +};
> > +#endif
> > +
> > static struct bin_attribute *zpci_bin_attrs[] = {
> > &bin_attr_util_string,
> > &bin_attr_report_error,
> > @@ -179,5 +211,8 @@ static struct attribute_group pfip_attr_group = {
> > const struct attribute_group *zpci_attr_groups[] = {
> > &zpci_attr_group,
> > &pfip_attr_group,
> > +#ifndef CONFIG_DMI
> > + &zpci_ident_attr_group,
> > +#endif
> > NULL,
> > };
> > --
> > 2.25.1
> >
