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(-)
On s390, an individual PCI function can generally be identified by two
IDs, depending on the scope and the platform configuration.
The first ID is the so-called FID, which 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 reuse the same configuration based on FIDs on two
different LPARs.
Such matching LPAR configurations are useful, though, allowing
standardized setups and booting a Linux installation on different
machines. To allow this, a second user-defined identifier called UID was
introduced. It is only guaranteed to be unique within an LPAR and only
if the platform indicates so via the UID Checking flag.
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 slot number 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.
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 opted for a minimal solution to open the discussion. In
particular my concern with a generic attribute is that it would be hard
to find a format that fits everyone. For example on PCI devices we also
use the "index" attribute for UIDs analogous to SMBIOS but having it in
decimal is odd on s390 where these are usual in hexadecimal.
---
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(-)
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,
+extern const struct attribute_group 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/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);
+static ssize_t zpci_uid_slot_show(struct pci_slot *slot, char *buf)
+{
+ struct zpci_dev *zdev = container_of(slot->hotplug, struct zpci_dev,
+ hotplug_slot);
+
+ return sysfs_emit(buf, "0x%x\n", zdev->uid);
+}
+
+static struct pci_slot_attribute zpci_slot_attr_uid =
+ __ATTR(uid, 0444, zpci_uid_slot_show, NULL);
+
static umode_t zpci_index_is_visible(struct kobject *kobj,
struct attribute *attr, int n)
{
@@ -233,3 +244,12 @@ const struct attribute_group pfip_attr_group = {
.name = "pfip",
.attrs = pfip_attrs,
};
+
+static struct attribute *zpci_slot_attrs[] = {
+ &zpci_slot_attr_uid.attr,
+ NULL,
+};
+
+const struct attribute_group zpci_slot_attr_group = {
+ .attrs = zpci_slot_attrs,
+};
diff --git a/drivers/pci/slot.c b/drivers/pci/slot.c
index 50fb3eb595fe65e271b6b339d43c9677c61b1e45..7430c7df44e1beef7bcf0531491612734e0dd60c 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,
+};
+
+const struct attribute_group *pci_slot_default_groups[] = {
+ &pci_slot_default_group,
+#ifdef ARCH_PCI_SLOT_GROUPS
+ ARCH_PCI_SLOT_GROUPS,
+#endif
+ NULL,
+};
static const struct kobj_type pci_slot_ktype = {
.sysfs_ops = &pci_slot_sysfs_ops,
---
base-commit: cec1e6e5d1ab33403b809f79cd20d6aff124ccfe
change-id: 20250923-uid_slot-e3559cf5ca30
Best regards,
--
Niklas Schnelle
On 9/24/2025 8:14 AM, Niklas Schnelle wrote:
> On s390, an individual PCI function can generally be identified by two
> IDs, depending on the scope and the platform configuration.
It would help to name the two IDs - FID and ???
> The first ID is the so-called FID, which 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 reuse the same configuration based on FIDs on two
> different LPARs.
>
> Such matching LPAR configurations are useful, though, allowing
> standardized setups and booting a Linux installation on different
> machines. To allow this, a second user-defined identifier called UID was
> introduced. It is only guaranteed to be unique within an LPAR and only
> if the platform indicates so via the UID Checking flag.
The paragraph as I read is not clear. Your intention is to highlight the
need for UID to allow standardized 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 slot number 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.
>
> 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 opted for a minimal solution to open the discussion. In
> particular my concern with a generic attribute is that it would be hard
> to find a format that fits everyone. For example on PCI devices we also
> use the "index" attribute for UIDs analogous to SMBIOS but having it in
> decimal is odd on s390 where these are usual in hexadecimal.
> ---
> 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(-)
>
> 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,
>
> +extern const struct attribute_group 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;
>
Will this not lead to linking error when the patch is built on non-s390
architecture. You could refer to zpci_slot_attr_group using a
CONFIG_..... and discard the #define ARCH_PCI_SLOT_GROUPS. I didn't find
a relevant CONFIG_... that could be used.
> 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);
>
> +static ssize_t zpci_uid_slot_show(struct pci_slot *slot, char *buf)
> +{
> + struct zpci_dev *zdev = container_of(slot->hotplug, struct zpci_dev,
> + hotplug_slot);
> +
> + return sysfs_emit(buf, "0x%x\n", zdev->uid);
> +}
> +
> +static struct pci_slot_attribute zpci_slot_attr_uid =
> + __ATTR(uid, 0444, zpci_uid_slot_show, NULL);
> +
> static umode_t zpci_index_is_visible(struct kobject *kobj,
> struct attribute *attr, int n)
> {
> @@ -233,3 +244,12 @@ const struct attribute_group pfip_attr_group = {
> .name = "pfip",
> .attrs = pfip_attrs,
> };
> +
> +static struct attribute *zpci_slot_attrs[] = {
> + &zpci_slot_attr_uid.attr,
> + NULL,
> +};
> +
> +const struct attribute_group zpci_slot_attr_group = {
> + .attrs = zpci_slot_attrs,
> +};
> diff --git a/drivers/pci/slot.c b/drivers/pci/slot.c
> index 50fb3eb595fe65e271b6b339d43c9677c61b1e45..7430c7df44e1beef7bcf0531491612734e0dd60c 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,
> +};
> +
> +const struct attribute_group *pci_slot_default_groups[] = {
> + &pci_slot_default_group,
> +#ifdef ARCH_PCI_SLOT_GROUPS
> + ARCH_PCI_SLOT_GROUPS,
> +#endif
> + NULL,
> +};
>
> static const struct kobj_type pci_slot_ktype = {
> .sysfs_ops = &pci_slot_sysfs_ops,
>
> ---
> base-commit: cec1e6e5d1ab33403b809f79cd20d6aff124ccfe
> change-id: 20250923-uid_slot-e3559cf5ca30
>
> Best regards,
On Fri, 2025-09-26 at 11:34 -0500, Ramesh Errabolu wrote:
> On 9/24/2025 8:14 AM, Niklas Schnelle wrote:
>
> > On s390, an individual PCI function can generally be identified by two
> > IDs, depending on the scope and the platform configuration.
> It would help to name the two IDs - FID and ???
How about:
"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."
And then reword the below without "so-called".
> > The first ID is the so-called FID, which 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 reuse the same configuration based on FIDs on two
> > different LPARs.
> >
> > Such matching LPAR configurations are useful, though, allowing
> > standardized setups and booting a Linux installation on different
> > machines. To allow this, a second user-defined identifier called UID was
> > introduced. It is only guaranteed to be unique within an LPAR and only
> > if the platform indicates so via the UID Checking flag.
> The paragraph as I read is not clear. Your intention is to highlight the
> need for UID to allow standardized setups.
Yes, that was my intention. Also here is where the second ID is
introduced so I'll reword this a bit if the name is already mentioned
in the first paragraph.
> >
> > 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 slot number 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.
> >
> > 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 opted for a minimal solution to open the discussion. In
> > particular my concern with a generic attribute is that it would be hard
> > to find a format that fits everyone. For example on PCI devices we also
> > use the "index" attribute for UIDs analogous to SMBIOS but having it in
> > decimal is odd on s390 where these are usual in hexadecimal.
> > ---
> > 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(-)
> >
> > 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,
> >
> > +extern const struct attribute_group 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;
> >
>
> Will this not lead to linking error when the patch is built on non-s390
> architecture. You could refer to zpci_slot_attr_group using a
> CONFIG_..... and discard the #define ARCH_PCI_SLOT_GROUPS. I didn't find
> a relevant CONFIG_... that could be used.
This code is in arch/s390/ it will not be build on non-s390. For the
non s390 case ARCH_PCI_SLOT_GROUPS will be undefined and the #ifdef in
slot.c makes sure we're not trying to insert ARCH_PCI_SLOT_GROUPs in
the array as it is not defined.
Thanks,
Niklas
On 9/26/2025 1:36 PM, Niklas Schnelle wrote:
> On Fri, 2025-09-26 at 11:34 -0500, Ramesh Errabolu wrote:
>> On 9/24/2025 8:14 AM, Niklas Schnelle wrote:
>>
>>> On s390, an individual PCI function can generally be identified by two
>>> IDs, depending on the scope and the platform configuration.
>> It would help to name the two IDs - FID and ???
> How about:
> "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."
>
> And then reword the below without "so-called".
That will help a lot
>>> The first ID is the so-called FID, which 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 reuse the same configuration based on FIDs on two
>>> different LPARs.
>>>
>>> Such matching LPAR configurations are useful, though, allowing
>>> standardized setups and booting a Linux installation on different
>>> machines. To allow this, a second user-defined identifier called UID was
>>> introduced. It is only guaranteed to be unique within an LPAR and only
>>> if the platform indicates so via the UID Checking flag.
>> The paragraph as I read is not clear. Your intention is to highlight the
>> need for UID to allow standardized setups.
> Yes, that was my intention. Also here is where the second ID is
> introduced so I'll reword this a bit if the name is already mentioned
> in the first paragraph.
Will await your next update
>>> 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 slot number 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.
>>>
>>> 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 opted for a minimal solution to open the discussion. In
>>> particular my concern with a generic attribute is that it would be hard
>>> to find a format that fits everyone. For example on PCI devices we also
>>> use the "index" attribute for UIDs analogous to SMBIOS but having it in
>>> decimal is odd on s390 where these are usual in hexadecimal.
>>> ---
>>> 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(-)
>>>
>>> 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,
>>>
>>> +extern const struct attribute_group 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;
>>>
>> Will this not lead to linking error when the patch is built on non-s390
>> architecture. You could refer to zpci_slot_attr_group using a
>> CONFIG_..... and discard the #define ARCH_PCI_SLOT_GROUPS. I didn't find
>> a relevant CONFIG_... that could be used.
> This code is in arch/s390/ it will not be build on non-s390. For the
> non s390 case ARCH_PCI_SLOT_GROUPS will be undefined and the #ifdef in
> slot.c makes sure we're not trying to insert ARCH_PCI_SLOT_GROUPs in
> the array as it is not defined.
You are right, I completely overlooked it. My comment is incorrect.
>
> Thanks,
>
> Niklas
> 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,
>
> +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.
> 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);
>
> +static ssize_t zpci_uid_slot_show(struct pci_slot *slot, char *buf)
> +{
> + 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`)?
> +}
> +
> +static struct pci_slot_attribute zpci_slot_attr_uid =
> + __ATTR(uid, 0444, zpci_uid_slot_show, NULL);
__ATTR_RO()?
--
Best Regards, Benjamin Block / Linux on IBM Z Kernel Development
IBM Deutschland Research & Development GmbH / https://www.ibm.com/privacy
Vors. Aufs.-R.: Wolfgang Wendt / Geschäftsführung: David Faller
Sitz der Ges.: Böblingen / Registergericht: AmtsG Stuttgart, HRB 243294
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()?
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.
> > > 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.
--
Best Regards, Benjamin Block / Linux on IBM Z Kernel Development
IBM Deutschland Research & Development GmbH / https://www.ibm.com/privacy
Vors. Aufs.-R.: Wolfgang Wendt / Geschäftsführung: David Faller
Sitz der Ges.: Böblingen / Registergericht: AmtsG Stuttgart, HRB 243294
© 2016 - 2025 Red Hat, Inc.