[PATCH 0/1] Use of /sys/bus/pci/devices/…/index for non-SMBIOS platforms

[PATCH 0/1] Use of /sys/bus/pci/devices/…/index for non-SMBIOS platforms

[Niklas Schnelle]

Hi Narendra, Hi All,

According to Documentation/ABI/testing/sysfs-bus-pci you are responsible
for the index device attribute that is used by systemd to create network
interface names.

Now we would like to reuse this attribute for firmware provided PCI
device index numbers on the s390 architecture which doesn't have
SMBIOS/DMI nor ACPI. All code changes are within our architecture
specific code but I'd like to get some Acks for this reuse. I've sent an
RFC version of this patch on 15th of March with the subject:

   s390/pci: expose a PCI device's UID as its index

but got no response. Would it be okay to re-use this attribute for
essentially the same purpose but with index numbers provided by
a different platform mechanism? I think this would be cleaner than
further proliferation of /sys/bus/pci/devices/<dev>/xyz_index
attributes and allows re-use of the existing userspace infrastructure.

Thanks,
Niklas Schnelle

Niklas Schnelle (1):
  s390/pci: expose a PCI device's UID as its index

 Documentation/ABI/testing/sysfs-bus-pci | 11 +++++---
 arch/s390/pci/pci_sysfs.c               | 35 +++++++++++++++++++++++++
 2 files changed, 42 insertions(+), 4 deletions(-)

-- 
2.25.1

[PATCH 1/1] s390/pci: expose a PCI device's UID as its index

[Niklas Schnelle]

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 <schnelle@linux.ibm.com>
Acked-by: Viktor Mihajlovski <mihajlov@linux.ibm.com>
---
 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 <narendra_k@dell.com>, linux-bugs@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 */
+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,
+};
+#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

Re: [PATCH 0/1] Use of /sys/bus/pci/devices/…/index for non-SMBIOS platforms

[Leon Romanovsky]

On Mon, Apr 12, 2021 at 03:59:04PM +0200, Niklas Schnelle wrote:
> Hi Narendra, Hi All,
> 
> According to Documentation/ABI/testing/sysfs-bus-pci you are responsible
> for the index device attribute that is used by systemd to create network
> interface names.
> 
> Now we would like to reuse this attribute for firmware provided PCI
> device index numbers on the s390 architecture which doesn't have
> SMBIOS/DMI nor ACPI. All code changes are within our architecture
> specific code but I'd like to get some Acks for this reuse. I've sent an
> RFC version of this patch on 15th of March with the subject:
> 
>    s390/pci: expose a PCI device's UID as its index
> 
> but got no response. Would it be okay to re-use this attribute for
> essentially the same purpose but with index numbers provided by
> a different platform mechanism? I think this would be cleaner than
> further proliferation of /sys/bus/pci/devices/<dev>/xyz_index
> attributes and allows re-use of the existing userspace infrastructure.

I'm missing an explanation that this change is safe for systemd and
they don't have some hard-coded assumption about the meaning of existing
index on s390.

Thanks

Re: [PATCH 0/1] Use of /sys/bus/pci/devices/…/index for non-SMBIOS platforms

[Niklas Schnelle]

On Tue, 2021-04-13 at 08:39 +0300, Leon Romanovsky wrote:
> On Mon, Apr 12, 2021 at 03:59:04PM +0200, Niklas Schnelle wrote:
> > Hi Narendra, Hi All,
> > 
> > According to Documentation/ABI/testing/sysfs-bus-pci you are responsible
> > for the index device attribute that is used by systemd to create network
> > interface names.
> > 
> > Now we would like to reuse this attribute for firmware provided PCI
> > device index numbers on the s390 architecture which doesn't have
> > SMBIOS/DMI nor ACPI. All code changes are within our architecture
> > specific code but I'd like to get some Acks for this reuse. I've sent an
> > RFC version of this patch on 15th of March with the subject:
> > 
> >    s390/pci: expose a PCI device's UID as its index
> > 
> > but got no response. Would it be okay to re-use this attribute for
> > essentially the same purpose but with index numbers provided by
> > a different platform mechanism? I think this would be cleaner than
> > further proliferation of /sys/bus/pci/devices/<dev>/xyz_index
> > attributes and allows re-use of the existing userspace infrastructure.
> 
> I'm missing an explanation that this change is safe for systemd and
> they don't have some hard-coded assumption about the meaning of existing
> index on s390.
> 
> Thanks


Sure, good point. So first off yes this change does create new index
based names also on existing systemd versions, this is known and
intended and we'll certainly closely collaborate with any distributions
wishing to backport this change.

As for being otherwise safe or having unintended consequences, Viktor
(see R-b) and I recently got the following PR merged in that exact area
of systemd to fix how hotplug slot derived interface names are
generated:
https://github.com/systemd/systemd/pull/19017
In working on that we did also analyse the use of the index attribute
for hidden assumptions and tested with this attribute added. Arguably,
as the nature of that PR shows we haven't had a perfect track record of
keeping this monitored but will in the future as PCI based NICs become
increasingly important for our platform. We also have special NIC
naming logic in the same area for our channel based platform specific
NICs which was also contributed by Viktor.

Thanks,
Niklas

Re: [PATCH 0/1] Use of /sys/bus/pci/devices/…/index for non-SMBIOS platforms

[Leon Romanovsky]

On Tue, Apr 13, 2021 at 08:57:19AM +0200, Niklas Schnelle wrote:
> On Tue, 2021-04-13 at 08:39 +0300, Leon Romanovsky wrote:
> > On Mon, Apr 12, 2021 at 03:59:04PM +0200, Niklas Schnelle wrote:
> > > Hi Narendra, Hi All,
> > > 
> > > According to Documentation/ABI/testing/sysfs-bus-pci you are responsible
> > > for the index device attribute that is used by systemd to create network
> > > interface names.
> > > 
> > > Now we would like to reuse this attribute for firmware provided PCI
> > > device index numbers on the s390 architecture which doesn't have
> > > SMBIOS/DMI nor ACPI. All code changes are within our architecture
> > > specific code but I'd like to get some Acks for this reuse. I've sent an
> > > RFC version of this patch on 15th of March with the subject:
> > > 
> > >    s390/pci: expose a PCI device's UID as its index
> > > 
> > > but got no response. Would it be okay to re-use this attribute for
> > > essentially the same purpose but with index numbers provided by
> > > a different platform mechanism? I think this would be cleaner than
> > > further proliferation of /sys/bus/pci/devices/<dev>/xyz_index
> > > attributes and allows re-use of the existing userspace infrastructure.
> > 
> > I'm missing an explanation that this change is safe for systemd and
> > they don't have some hard-coded assumption about the meaning of existing
> > index on s390.
> > 
> > Thanks
> 
> 
> Sure, good point. So first off yes this change does create new index
> based names also on existing systemd versions, this is known and
> intended and we'll certainly closely collaborate with any distributions
> wishing to backport this change.
> 
> As for being otherwise safe or having unintended consequences, Viktor
> (see R-b) and I recently got the following PR merged in that exact area
> of systemd to fix how hotplug slot derived interface names are
> generated:
> https://github.com/systemd/systemd/pull/19017
> In working on that we did also analyse the use of the index attribute
> for hidden assumptions and tested with this attribute added. Arguably,
> as the nature of that PR shows we haven't had a perfect track record of
> keeping this monitored but will in the future as PCI based NICs become
> increasingly important for our platform. We also have special NIC
> naming logic in the same area for our channel based platform specific
> NICs which was also contributed by Viktor.

Thanks, this PR is exciting to read, very warm words were said about
kernel developers :). Can you please summarize that will be the breakage
in old systemd if this index will be overloaded?

Thanks

> 
> Thanks,
> Niklas
> 

Re: [PATCH 0/1] Use of /sys/bus/pci/devices/…/index for non-SMBIOS platforms

[Niklas Schnelle]

On Tue, 2021-04-13 at 10:32 +0300, Leon Romanovsky wrote:
> On Tue, Apr 13, 2021 at 08:57:19AM +0200, Niklas Schnelle wrote:
> > On Tue, 2021-04-13 at 08:39 +0300, Leon Romanovsky wrote:
> > > On Mon, Apr 12, 2021 at 03:59:04PM +0200, Niklas Schnelle wrote:
> > > > Hi Narendra, Hi All,
> > > > 
> > > > According to Documentation/ABI/testing/sysfs-bus-pci you are responsible
> > > > for the index device attribute that is used by systemd to create network
> > > > interface names.
> > > > 
> > > > Now we would like to reuse this attribute for firmware provided PCI
> > > > device index numbers on the s390 architecture which doesn't have
> > > > SMBIOS/DMI nor ACPI. All code changes are within our architecture
> > > > specific code but I'd like to get some Acks for this reuse. I've sent an
> > > > RFC version of this patch on 15th of March with the subject:
> > > > 
> > > >    s390/pci: expose a PCI device's UID as its index
> > > > 
> > > > but got no response. Would it be okay to re-use this attribute for
> > > > essentially the same purpose but with index numbers provided by
> > > > a different platform mechanism? I think this would be cleaner than
> > > > further proliferation of /sys/bus/pci/devices/<dev>/xyz_index
> > > > attributes and allows re-use of the existing userspace infrastructure.
> > > 
> > > I'm missing an explanation that this change is safe for systemd and
> > > they don't have some hard-coded assumption about the meaning of existing
> > > index on s390.
> > > 
> > > Thanks
> > 
> > Sure, good point. So first off yes this change does create new index
> > based names also on existing systemd versions, this is known and
> > intended and we'll certainly closely collaborate with any distributions
> > wishing to backport this change.
> > 
> > As for being otherwise safe or having unintended consequences, Viktor
> > (see R-b) and I recently got the following PR merged in that exact area
> > of systemd to fix how hotplug slot derived interface names are
> > generated:
> > https://github.com/systemd/systemd/pull/19017
> > In working on that we did also analyse the use of the index attribute
> > for hidden assumptions and tested with this attribute added. Arguably,
> > as the nature of that PR shows we haven't had a perfect track record of
> > keeping this monitored but will in the future as PCI based NICs become
> > increasingly important for our platform. We also have special NIC
> > naming logic in the same area for our channel based platform specific
> > NICs which was also contributed by Viktor.
> 
> Thanks, this PR is exciting to read, very warm words were said about
> kernel developers :). Can you please summarize that will be the breakage
> in old systemd if this index will be overloaded?
> 
> Thanks

;-) yeah, maybe us working closely across kernel and systemd will help
improve relations.

In principle the same happens on existing and new systemd i.e. we get
new eno<UID_in_decimal>… interface names. Due to the way naming
priorities work this will become the dominant interface name but thanks
to altname support in current kernels we expect all old settings,
routing rules etc. to remain working with the old name.

This only happens however if the Linux instance is running with active
UID Uniqueness Checking, this is a Hypervisor/Platform setting that
enforces that no PCI device can be attached if one with the same UID is
already attached. So far this setting was hidden inside the kernel but
I recently committed a change to expose it to userspace here:
https://git.kernel.org/pub/scm/linux/kernel/git/s390/linux.git/commit/?h=features&id=408f2c9c15682fc21b645fdec1f726492e235c4b

That said, we're also still open for other approaches for example as
floated by Lennart in this comment:
https://github.com/systemd/systemd/pull/18829#discussion_r584794766
But this of course also depends on whether the kernel community is open
to reusing the index attribute or has other preferences.

> 
> > Thanks,
> > Niklas
> > 

RE: [PATCH 0/1] Use of /sys/bus/pci/devices/…/index for non-SMBIOS platforms

[K, Narendra]

> -----Original Message-----
> From: Niklas Schnelle <schnelle@linux.ibm.com>
> Sent: Monday, April 12, 2021 7:29 PM
> To: K, Narendra
> Cc: Viktor Mihajlovski; Stefan Raspl; Peter Oberparleiter; linux-
> netdev@vger.kernel.org; linux-pci@vger.kernel.org; linux-
> kernel@vger.kernel.org; linux-s390@vger.kernel.org
> Subject: [PATCH 0/1] Use of /sys/bus/pci/devices/…/index for non-SMBIOS
> platforms
> 
> 
> [EXTERNAL EMAIL]
> 
> Hi Narendra, Hi All,
> 
> According to Documentation/ABI/testing/sysfs-bus-pci you are responsible for
> the index device attribute that is used by systemd to create network interface
> names.
> 
> Now we would like to reuse this attribute for firmware provided PCI device
> index numbers on the s390 architecture which doesn't have SMBIOS/DMI nor
> ACPI. All code changes are within our architecture specific code but I'd like to
> get some Acks for this reuse. I've sent an RFC version of this patch on 15th of
> March with the subject:
> 
>    s390/pci: expose a PCI device's UID as its index
> 
> but got no response. 

Hi Niklas,

Sorry, I missed responding to the RFC sent earlier.  

> Would it be okay to re-use this attribute for essentially
> the same purpose but with index numbers provided by a different platform
> mechanism? 

I will go through the request above and provide comments in two or three days.
 
With regards,
Narendra K

Re: [PATCH 1/1] s390/pci: expose a PCI device's UID as its index

[Bjorn Helgaas]

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 <schnelle@linux.ibm.com>
> Acked-by: Viktor Mihajlovski <mihajlov@linux.ibm.com>

This seems like a nice solution to me.

Acked-by: Bjorn Helgaas <bhelgaas@google.com>

> ---
>  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 <narendra_k@dell.com>, linux-bugs@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).

> +};
> +#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
> 

Re: [PATCH 1/1] s390/pci: expose a PCI device's UID as its index

[Niklas Schnelle]

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 <schnelle@linux.ibm.com>
> > Acked-by: Viktor Mihajlovski <mihajlov@linux.ibm.com>
> 
> This seems like a nice solution to me.
> 
> Acked-by: Bjorn Helgaas <bhelgaas@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 <narendra_k@dell.com>, linux-bugs@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
> > 

RE: [PATCH 1/1] s390/pci: expose a PCI device's UID as its index

[K, Narendra]

> -----Original Message-----
> From: Niklas Schnelle <schnelle@linux.ibm.com>
> Sent: Thursday, April 15, 2021 12:55 PM
> To: Bjorn Helgaas
> Cc: K, Narendra; Viktor Mihajlovski; Stefan Raspl; Peter Oberparleiter; linux-
> netdev@vger.kernel.org; linux-pci@vger.kernel.org; linux-
> kernel@vger.kernel.org; linux-s390@vger.kernel.org
> Subject: Re: [PATCH 1/1] s390/pci: expose a PCI device's UID as its index
> 
> 
> [EXTERNAL EMAIL]
> 
> 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 <schnelle@linux.ibm.com>
> > > Acked-by: Viktor Mihajlovski <mihajlov@linux.ibm.com>
> >
> > This seems like a nice solution to me.
> >
> > Acked-by: Bjorn Helgaas <bhelgaas@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.

Hi Niklas,

It seems like the UID and the index are not equivalent in their meaning.

The index SMBIOS type 41 device type instance indicates that 

1. The device is an onboard device.
2. A specific order of the onboard devices.

The UID described seems to indicate that the PCI device is unique within the Linux instance (sufficient for naming). 
But it does not indicate that PCI device is onboard and any order. 

As all devices with UID will be named as eno<UID> (Ethernet onboard), names are not in sync with how each PCI function is exposed on a PCI slot (appears
closer to SMBIOS type 9 record) as described below.

https://github.com/systemd/systemd/pull/19017
https://github.com/systemd/systemd/commit/a496a238e8ee66ce25ad13a3f46549b2e2e979fc

In summary, it seems like the eno<UID> names on s390 will be unique names, but may not match the meaning of the index.

Also,

a) Will UID remain unique/same as initially exposed across reboots ? 

b) Is the reuse of index related to the 'slots' based naming scheme described below ?   

https://github.com/systemd/systemd/pull/19017
https://github.com/systemd/systemd/commit/a496a238e8ee66ce25ad13a3f46549b2e2e979fc

c) If the slot based naming is fixed with the naming scheme from changes below, any thoughts on why is reuse of index needed ? 

https://github.com/systemd/systemd/pull/19017
https://github.com/systemd/systemd/commit/a496a238e8ee66ce25ad13a3f46549b2e2e979fc  

With regards,
Narendra K

Re: [PATCH 1/1] s390/pci: expose a PCI device's UID as its index

[Viktor Mihajlovski]

On 4/16/21 7:58 PM, K, Narendra wrote:
>> -----Original Message-----
>> From: Niklas Schnelle <schnelle@linux.ibm.com>
>> Sent: Thursday, April 15, 2021 12:55 PM
>> To: Bjorn Helgaas
>> Cc: K, Narendra; Viktor Mihajlovski; Stefan Raspl; Peter Oberparleiter; linux-
>> netdev@vger.kernel.org; linux-pci@vger.kernel.org; linux-
>> kernel@vger.kernel.org; linux-s390@vger.kernel.org
>> Subject: Re: [PATCH 1/1] s390/pci: expose a PCI device's UID as its index
>>
>>
>> [EXTERNAL EMAIL]
>>
>> 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 <schnelle@linux.ibm.com>
>>>> Acked-by: Viktor Mihajlovski <mihajlov@linux.ibm.com>
>>>
>>> This seems like a nice solution to me.
>>>
>>> Acked-by: Bjorn Helgaas <bhelgaas@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.
> 
> Hi Niklas,
> 
> It seems like the UID and the index are not equivalent in their meaning.
> 
Hi Narendra,

the reasoning behind the wish to reuse index is that we think it's
similar in the sense that it provides a stable, firmware-reported
interface identifier.
Some background: s390 is a highly virtualized platform. There's
no traditional bare metal mode of operation, neither for the computer
system nor the I/O subsystem.
The equivalent to a standalone system is a logical partition (LPAR)
which in essence is a kind of virtual machine. LPARs access I/O devices
in a virtualized fashion as well. E.g., for PCI devices the I/O
subsystem is responsible for the management of PCI hardware and
will provide the PCI functions to the LPARs.
This is where UID and function_id come into play. Each PCI function will
carry a function_id attribute which defines it uniquely within the
entire s390 system. If a UID is configured for the function, it must
be unique within an LPAR, but doesn't need to be unique system-wide.
For instance, the admistrator may want to ensure that for every
LPAR the primary ethernet interface has the same identifier, to
allow miigration of Linux instances accross LPARs.
This can't be achieved with a slot-based name.
> The index SMBIOS type 41 device type instance indicates that
> 
> 1. The device is an onboard device.
> 2. A specific order of the onboard devices.
> 
> The UID described seems to indicate that the PCI device is unique within the Linux instance (sufficient for naming).
> But it does not indicate that PCI device is onboard and any order.
> 
> As all devices with UID will be named as eno<UID> (Ethernet onboard), names are not in sync with how each PCI function is exposed on a PCI slot (appears
> closer to SMBIOS type 9 record) as described below.
> 
> https://github.com/systemd/systemd/pull/19017
> https://github.com/systemd/systemd/commit/a496a238e8ee66ce25ad13a3f46549b2e2e979fc
> 
> In summary, it seems like the eno<UID> names on s390 will be unique names, but may not match the meaning of the index.
> 
> Also,
> 
> a) Will UID remain unique/same as initially exposed across reboots ?
Yes, the UID is the primary identifier for a PCI function and part of
the LPAR configuration. Even hotplug will not change the UID.
> 
> b) Is the reuse of index related to the 'slots' based naming scheme described below ?
> 
> https://github.com/systemd/systemd/pull/19017
> https://github.com/systemd/systemd/commit/a496a238e8ee66ce25ad13a3f46549b2e2e979fc
> 
No, this is independent. The commit above fixes the slot name parsing.
> c) If the slot based naming is fixed with the naming scheme from changes below, any thoughts on why is reuse of index needed ?
> 
> https://github.com/systemd/systemd/pull/19017
> https://github.com/systemd/systemd/commit/a496a238e8ee66ce25ad13a3f46549b2e2e979fc
As I wrote above, the slot describes the PCI function at the system
level, whereas the uid/index does it in the context off the LPAR.
>  > With regards,
> Narendra K
> 

-- 
Kind Regards,
    Viktor

RE: [PATCH 1/1] s390/pci: expose a PCI device's UID as its index

[K, Narendra]

> -----Original Message-----
> From: Viktor Mihajlovski <mihajlov@linux.ibm.com>
> Sent: Saturday, April 17, 2021 4:18 PM
> To: K, Narendra; Niklas Schnelle; Bjorn Helgaas
> Cc: Stefan Raspl; Peter Oberparleiter; linux-netdev@vger.kernel.org; linux-
> pci@vger.kernel.org; linux-kernel@vger.kernel.org; linux-
> s390@vger.kernel.org
> Subject: Re: [PATCH 1/1] s390/pci: expose a PCI device's UID as its index
> 
> 
> [EXTERNAL EMAIL]
> 
> 
> 
> On 4/16/21 7:58 PM, K, Narendra wrote:
> >> -----Original Message-----
> >> From: Niklas Schnelle <schnelle@linux.ibm.com>
> >> Sent: Thursday, April 15, 2021 12:55 PM
> >> To: Bjorn Helgaas
> >> Cc: K, Narendra; Viktor Mihajlovski; Stefan Raspl; Peter
> >> Oberparleiter; linux- netdev@vger.kernel.org;
> >> linux-pci@vger.kernel.org; linux- kernel@vger.kernel.org;
> >> linux-s390@vger.kernel.org
> >> Subject: Re: [PATCH 1/1] s390/pci: expose a PCI device's UID as its
> >> index
> >>
> >>
> >> [EXTERNAL EMAIL]
> >>
> >> 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 <schnelle@linux.ibm.com>
> >>>> Acked-by: Viktor Mihajlovski <mihajlov@linux.ibm.com>
> >>>
> >>> This seems like a nice solution to me.
> >>>
> >>> Acked-by: Bjorn Helgaas <bhelgaas@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.
> >
> > Hi Niklas,
> >
> > It seems like the UID and the index are not equivalent in their meaning.
> >
> Hi Narendra,
> 
> the reasoning behind the wish to reuse index is that we think it's similar in the
> sense that it provides a stable, firmware-reported interface identifier.
> Some background: s390 is a highly virtualized platform. There's no traditional
> bare metal mode of operation, neither for the computer system nor the I/O
> subsystem.
> The equivalent to a standalone system is a logical partition (LPAR) which in
> essence is a kind of virtual machine. LPARs access I/O devices in a virtualized
> fashion as well. E.g., for PCI devices the I/O subsystem is responsible for the
> management of PCI hardware and will provide the PCI functions to the LPARs.
> This is where UID and function_id come into play. Each PCI function will carry a
> function_id attribute which defines it uniquely within the entire s390 system. If
> a UID is configured for the function, it must be unique within an LPAR, but
> doesn't need to be unique system-wide.
> For instance, the admistrator may want to ensure that for every LPAR the
> primary ethernet interface has the same identifier, to allow miigration of Linux
> instances accross LPARs.
> This can't be achieved with a slot-based name.
> > The index SMBIOS type 41 device type instance indicates that
> >
> > 1. The device is an onboard device.
> > 2. A specific order of the onboard devices.
> >
> > The UID described seems to indicate that the PCI device is unique within the
> Linux instance (sufficient for naming).
> > But it does not indicate that PCI device is onboard and any order.
> >
> > As all devices with UID will be named as eno<UID> (Ethernet onboard),
> > names are not in sync with how each PCI function is exposed on a PCI slot
> (appears closer to SMBIOS type 9 record) as described below.
> >
> > https://urldefense.com/v3/__https://github.com/systemd/systemd/pull/19
> > 017__;!!LpKI!zDeT5hnnMp8tFNAzwNWtW3-
> C6w7p4FBLldAu705T3EPicJZNI7TewsdZa
> > LDBMQ$ [github[.]com]
> >
> https://urldefense.com/v3/__https://github.com/systemd/systemd/commit/
> >
> a496a238e8ee66ce25ad13a3f46549b2e2e979fc__;!!LpKI!zDeT5hnnMp8tFNAz
> wNWt
> > W3-C6w7p4FBLldAu705T3EPicJZNI7TewsfDTU8TaQ$ [github[.]com]
> >
> > In summary, it seems like the eno<UID> names on s390 will be unique
> names, but may not match the meaning of the index.
> >
> > Also,
> >
> > a) Will UID remain unique/same as initially exposed across reboots ?
> Yes, the UID is the primary identifier for a PCI function and part of the LPAR
> configuration. Even hotplug will not change the UID.
> >
> > b) Is the reuse of index related to the 'slots' based naming scheme described
> below ?
> >
> > https://urldefense.com/v3/__https://github.com/systemd/systemd/pull/19
> > 017__;!!LpKI!zDeT5hnnMp8tFNAzwNWtW3-
> C6w7p4FBLldAu705T3EPicJZNI7TewsdZa
> > LDBMQ$ [github[.]com]
> >
> https://urldefense.com/v3/__https://github.com/systemd/systemd/commit/
> >
> a496a238e8ee66ce25ad13a3f46549b2e2e979fc__;!!LpKI!zDeT5hnnMp8tFNAz
> wNWt
> > W3-C6w7p4FBLldAu705T3EPicJZNI7TewsfDTU8TaQ$ [github[.]com]
> >
> No, this is independent. The commit above fixes the slot name parsing.
> > c) If the slot based naming is fixed with the naming scheme from changes
> below, any thoughts on why is reuse of index needed ?
> >
> > https://urldefense.com/v3/__https://github.com/systemd/systemd/pull/19
> > 017__;!!LpKI!zDeT5hnnMp8tFNAzwNWtW3-
> C6w7p4FBLldAu705T3EPicJZNI7TewsdZa
> > LDBMQ$ [github[.]com]
> >
> https://urldefense.com/v3/__https://github.com/systemd/systemd/commit/
> >
> a496a238e8ee66ce25ad13a3f46549b2e2e979fc__;!!LpKI!zDeT5hnnMp8tFNAz
> wNWt
> > W3-C6w7p4FBLldAu705T3EPicJZNI7TewsfDTU8TaQ$ [github[.]com]
> As I wrote above, the slot describes the PCI function at the system level,
> whereas the uid/index does it in the context off the LPAR.

Hi Viktor,

Thank you for the context and clarification. 

I am not familiar with s390 and have not reviewed the patch. Please find the ack for reuse of  '/sys/bus/pci/devices/.../index'  sysfs attribute. 

Acked-by: Narendra K <narendra_k@dell.com>

With regards,
Narendra K