Re: [RFC PATCH 05/13] pci: pcsc: control the cache via sysfs and kernel params

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

On Fri, 3 Oct 2025 09:00:41 +0000
Evangelos Petrongonas <epetron@amazon.de> wrote:

> Add kernel parameters and runtime control mechanisms for the PCSC
> 
> A new kernel parameter 'pcsc_enabled' allows enabling or disabling
> the cache at boot time. The parameter defaults to disabled.
> 
> A sysfs interface at /sys/bus/pci/pcsc/enabled provides:
> - Read access to query current cache status (1=enabled, 0=disabled)
> - Write access to dynamically enable/disable the cache at runtime
> 
> Signed-off-by: Evangelos Petrongonas <epetron@amazon.de>
> ---
>  Documentation/ABI/testing/sysfs-bus-pci-pcsc  | 20 ++++
>  .../admin-guide/kernel-parameters.txt         |  3 +
>  drivers/pci/pcsc.c                            | 93 ++++++++++++++++++-
>  3 files changed, 114 insertions(+), 2 deletions(-)
>  create mode 100644 Documentation/ABI/testing/sysfs-bus-pci-pcsc
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-pci-pcsc b/Documentation/ABI/testing/sysfs-bus-pci-pcsc
> new file mode 100644
> index 000000000000..ee92bf087816
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-bus-pci-pcsc
> @@ -0,0 +1,20 @@
> +PCI Configuration Space Cache (PCSC)
> +-------------------------------------
> +
> +The PCI Configuration Space Cache (PCSC) is a transparent caching layer
> +that intercepts configuration space operations to reduce hardware access
> +overhead. This subsystem addresses performance bottlenecks in PCI
> +configuration space accesses, particularly in virtualization
> +environments with high-density SR-IOV deployments where repeated
> +enumeration of Virtual Functions creates substantial delays.
> +
> +What:			/sys/bus/pci/pcsc/enabled
> +Date:			September 2025
> +Contact:		Linux PCI developers <linux-pci@vger.kernel.org>
> +Description:
> +				PCI Configuration Space Cache (PCSC) is a subsystem that
> +				caches accesses to the PCI configuration space of PCI
> +				functions. When this file contains the "1", the kernel
> +				is utilizing the cache, while when on "0" the
> +				system bypasses it. This setting can also be controlled
> +parameter.
indent issue on this last line.

Excellent to see someone remembering the ABI docs for once in an RFC!


> diff --git a/drivers/pci/pcsc.c b/drivers/pci/pcsc.c
> index 343f8b03831a..44d842733230 100644
> --- a/drivers/pci/pcsc.c
> +++ b/drivers/pci/pcsc.c
> +static struct kobj_attribute pcsc_enabled_attribute =
> +	__ATTR(enabled, 0644, pcsc_enabled_show, pcsc_enabled_store);
> +
> +static struct attribute *pcsc_attrs[] = {
> +	&pcsc_enabled_attribute.attr,
> +	NULL,
Trivial but no need for that trailing comma after the NULL terminator.
We don't want it to be easy to accidentally add something after that.

> +};
> +
> +static struct attribute_group pcsc_attr_group = {
> +	.attrs = pcsc_attrs,
> +};
> +
> +static struct kobject *pcsc_kobj;
> +
> +static void pcsc_create_sysfs(void)
> +{
> +	struct kset *pci_bus_kset;
> +	int ret;
> +
> +	if (pcsc_kobj)
> +		return; /* Already created */

Why do we need the kobject? Can't we make this a group on the
pci_bus_kset->kobj with a group name of pcsc?

(I see you have a comment on this next bit later in here)
Event better if we can arrange for not to be added after that is
created but just be a group on it in the first place.
That is make it a group in bus_groups of the pci_bus_type alongside
the one with bus_attr_rescan.attr in it.

That should mean you don't need the two tried to set it up that
you have currently.

> +
> +	pci_bus_kset = bus_get_kset(&pci_bus_type);
> +	if (!pci_bus_kset) {
> +		/* PCI bus kset not ready yet, will be retried later */
> +		return;
> +	}
> +

> +
> +/*
> + * The PCI subsystem is initialised later, therefore we need to add
> + * our sysfs entries later. This is done to avoid modifying the sysfs
> + * creation of the core pci driver.
Vs complexity and races, I think I'd rather you did modify that.

> + */
> +late_initcall(pcsc_sysfs_init);