Re: [RESEND PATCH v7 1/1] PCI/sysfs: Expose PCIe device serial number

["Krzysztof Wilczyński" <kw@linux.com>]

Hello,

> > I can see that the PCI r3.0 (conventional PCI) spec doesn't include
> > the Device Serial Number Capability and the PCIe spec does include it,
> > but this seems like it would fit better in the pci_dev_dev_attrs[],
> > and the visibility check would be parallel to the
> > dev_attr_boot_vga.attr check there.
> 
> I'm not sure I agree. The pci_dev_dev_attrs apply to all pci devices,
> but DSN only exists in PCIe Extended Capability space. Conventional pci
> config requests couldn't even describe it, so seems okay to fence it off
> using the PCI-Express attribute group that already has that visibility
> barrier.

PCI-X 2.0 added Extended Configuration Space[1].  Perhaps why Bjorn had
different attributes group in mind here.

> I also don't like Krzysztof's suggestion to make it visible even if we
> know you can't read it.

I simply wanted to keep this new attribute and its behaviour aligned with
the existing ones.  Which we keep visible for historic reasons.

I don't like the special case it becomes within pcie_dev_attrs_are_visible(),
but it can't be helped without introducing an entirely new group for this
alone which would be an overkill, indeed. 

That said, no strong feelings about it.  I wish we could clean the existing
ones up a bit, though.

> The exisiting attributes that behave that way shouldn't do that, IMO.

A lot of then, as Jonathan also reminded us, predate kernfs' visibility
feature, sadly.  We probably won't change the existing ones, indeed, to
avoid potentially breaking something for some users.

This is why I was curious why do we need such an attribute to be added,
as when these new objects for PCI are added and gain users, then there
are often here to stay, with broken behaviour or not, sadly.  We don't
have any sensible way of properly deprecating things in this area.

> It's a waste of resources to provide a handle just to say the capability
> doesn't exist when the handle could just not exist instead.

I haven't checked how the kernfs side looks like, admittedly, but I think
whether an attribute is visible or not, it does not unload and/or de-allocate
any space for the accompanying kernfs object...  So, the resources saving
here might not be in any way significant.

1. https://en.m.wikipedia.org/wiki/PCI_configuration_space

Thank you,

	Krzysztof