Re: [PATCH v3 09/13] PCI/IDE: Report available IDE streams

[<dan.j.williams@intel.com>]

Jonathan Cameron wrote:
> On Thu, 15 May 2025 22:47:28 -0700
> Dan Williams <dan.j.williams@intel.com> wrote:
> 
> > The limited number of link-encryption (IDE) streams that a given set of
> > host bridges supports is a platform specific detail. Provide
> > pci_ide_init_nr_streams() as a generic facility for either platform TSM
> > drivers, or PCI core native IDE, to report the number available streams.
> > After invoking pci_ide_init_nr_streams() an "available_secure_streams"
> > attribute appears in PCI host bridge sysfs to convey that count.
> > 
> > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > Cc: Lukas Wunner <lukas@wunner.de>
> > Cc: Samuel Ortiz <sameo@rivosinc.com>
> > Cc: Alexey Kardashevskiy <aik@amd.com>
> > Cc: Xu Yilun <yilun.xu@linux.intel.com>
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> Trivial stuff inline.
> 
> > ---
> >  .../ABI/testing/sysfs-devices-pci-host-bridge | 13 ++++
> >  drivers/pci/ide.c                             | 59 +++++++++++++++++++
> >  drivers/pci/pci.h                             |  3 +
> >  drivers/pci/probe.c                           | 12 +++-
> >  include/linux/pci.h                           |  8 +++
> >  5 files changed, 94 insertions(+), 1 deletion(-)
> > 
> > diff --git a/Documentation/ABI/testing/sysfs-devices-pci-host-bridge b/Documentation/ABI/testing/sysfs-devices-pci-host-bridge
> > index d592b68c7333..382866a21703 100644
> > --- a/Documentation/ABI/testing/sysfs-devices-pci-host-bridge
> > +++ b/Documentation/ABI/testing/sysfs-devices-pci-host-bridge
> > @@ -36,3 +36,16 @@ Description:
> >  		stream resources shared by the Root Ports in a host bridge.  See
> >  		/sys/devices/pciDDDD:BB entry for details about the DDDD:BB
> >  		format.
> > +
> > +What:		pciDDDD:BB/available_secure_streams
> > +Date:		December, 2024
> > +Contact:	linux-pci@vger.kernel.org
> > +Description:
> > +		(RO) When a host bridge has Root Ports that support PCIe IDE
> > +		(link encryption and integrity protection) there may be a
> > +		limited number of streams that can be used for establishing new
> > +		secure links. This attribute decrements upon secure link setup,
> > +		and increments upon secure link teardown. The in-use stream
> > +		count is determined by counting stream symlinks.  See
> > +		/sys/devices/pciDDDD:BB entry for details about the DDDD:BB
> > +		format.
> 
> Is this name specific enough given mix of link and selective streams, both of which are
> limited?  Nice to use generic terms but this one feels too generic!

I will update the description to call out that these are Selective IDE
Stream resources, but keep the generic name of the attribute.

If Linux ever grows Link IDE support that can come in with a different
name. Especially because the security properties of Link IDE are weaker
in the presence of switches. I.e. you need to trust a switch to
decrypt/re-crypt.

So if Link IDE support ever arrives it can call its related attribute
"available_local_secure_streams" or something similar as "Selective" and
"Link" are not saying much to an admin that has not ready the PCIe
specification.

> > diff --git a/drivers/pci/ide.c b/drivers/pci/ide.c
> > index a529926647f4..b7561ac03405 100644
> > --- a/drivers/pci/ide.c
> > +++ b/drivers/pci/ide.c
> 
> > +static struct attribute *pci_ide_attrs[] = {
> > +	&dev_attr_available_secure_streams.attr,
> > +	NULL,
> 
> As below. No trailing comma here.
> 
> > +};
> 
> 
> 
> > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > index 56704e851224..93be55321537 100644
> > --- a/drivers/pci/probe.c
> > +++ b/drivers/pci/probe.c
> > @@ -640,6 +640,16 @@ static void pci_release_host_bridge_dev(struct device *dev)
> >  	kfree(bridge);
> >  }
> >  
> > +static const struct attribute_group *pci_host_bridge_groups[] = {
> > +	PCI_IDE_ATTR_GROUP,
> > +	NULL,
> 
> One of my favourite comments :)  No comma on terminators. Let's
> not make it easy to accidentally put something after them.

Fixed. ...might be worth a checkpatch update for this, my Perl-fu is
rusty though.

> 
> > +};
> > +
> > +static const struct device_type pci_host_bridge_type = {
> > +	.groups = pci_host_bridge_groups,
> > +	.release = pci_release_host_bridge_dev,
> 
> I've no problem with this as a clean up but you could have just
> used the bridge->dev.groups instead I think? If you are clearing
> that out for some other use alter, mention that in the patch description.

Sure, I will mention it.