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

[Jonathan Cameron <Jonathan.Cameron@huawei.com>]

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!

> 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.

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

> +};
> +
>  static void pci_init_host_bridge(struct pci_host_bridge *bridge)
>  {
>  	INIT_LIST_HEAD(&bridge->windows);
> @@ -659,6 +669,7 @@ static void pci_init_host_bridge(struct pci_host_bridge *bridge)
>  	bridge->native_dpc = 1;
>  	bridge->domain_nr = PCI_DOMAIN_NR_NOT_SET;
>  	bridge->native_cxl_error = 1;
> +	bridge->dev.type = &pci_host_bridge_type;
>  
>  	device_initialize(&bridge->dev);
>  }
> @@ -672,7 +683,6 @@ struct pci_host_bridge *pci_alloc_host_bridge(size_t priv)
>  		return NULL;
>  
>  	pci_init_host_bridge(bridge);
> -	bridge->dev.release = pci_release_host_bridge_dev;
>  
>  	return bridge;
>  }