Re: [PATCH v5 6/7] cxl: Add cxl_reset sysfs interface for PCI devices

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

On Fri, 6 Mar 2026 09:23:21 +0000
<smadhavan@nvidia.com> wrote:

> From: Srirangan Madhavan <smadhavan@nvidia.com>
> 
> Add a "cxl_reset" sysfs attribute to PCI devices that support CXL
> Reset (CXL r3.2 section 8.1.3.1). The attribute is visible only on
> devices with both CXL.cache and CXL.mem capabilities and the CXL
> Reset Capable bit set in the DVSEC.
> 
> Writing "1" to the attribute triggers the full CXL reset flow via
> cxl_do_reset(). The interface is decoupled from memdev creation:
> when a CXL memdev exists, memory offlining and cache flush are
> performed; otherwise reset proceeds without the memory management.
> 
> The sysfs attribute is managed entirely by the CXL module using
> sysfs_create_group() / sysfs_remove_group() rather than the PCI
> core's static attribute groups. This avoids cross-module symbol
> dependencies between the PCI core (always built-in) and CXL_BUS
> (potentially modular).

The side effect being the races that tend to come with dynamic creation
of sysfs. Not sure we can avoid that though.

> 
> At module init, existing PCI devices are scanned and a PCI bus
> notifier handles hot-plug/unplug. kernfs_drain() makes sure that
> any in-flight store() completes before sysfs_remove_group() returns,
> preventing use-after-free during module unload.
> 
> Signed-off-by: Srirangan Madhavan <smadhavan@nvidia.com>

A few trivial things inline.

> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> index c758b3f1b3f9..3a53d4314f24 100644
> --- a/drivers/cxl/core/pci.c
> +++ b/drivers/cxl/core/pci.c
> @@ -1293,3 +1293,116 @@ static int cxl_do_reset(struct pci_dev *pdev)
> 
>  	return rc;
>  }
> +
> +/*
> + * CXL reset sysfs attribute management.
> + *
> + * The cxl_reset attribute is added to PCI devices that advertise CXL Reset
> + * capability. Managed entirely by the CXL module via subsys_interface on
> + * pci_bus_type, avoiding cross-module symbol dependencies between the PCI
> + * core (built-in) and CXL (potentially modular).
> + *
> + * subsys_interface handles existing devices at register time and hot-plug
> + * add/remove automatically. On unregister, remove_dev runs for all tracked
> + * devices under bus core serialization.
> + */
> +
> +static bool pci_cxl_reset_capable(struct pci_dev *pdev)
> +{
> +	int dvsec;
> +	u16 cap;
> +
> +	dvsec = pci_find_dvsec_capability(pdev, PCI_VENDOR_ID_CXL,
> +					  PCI_DVSEC_CXL_DEVICE);
> +	if (!dvsec)
> +		return false;
> +
> +	if (pci_read_config_word(pdev, dvsec + PCI_DVSEC_CXL_CAP, &cap))
> +		return false;
> +
> +	if (!(cap & PCI_DVSEC_CXL_CACHE_CAPABLE) ||
> +	    !(cap & PCI_DVSEC_CXL_MEM_CAPABLE))

Whilst it's a nonsensical setup to have a CXL device with no
CXL features, is there a reason we need this explicit check?

> +		return false;
> +
> +	return !!(cap & PCI_DVSEC_CXL_RST_CAPABLE);

Technically the !! not needed as the cast will deal with it.
If you want to force a 0/1 I'd prefer FIELD_GET()

> +}
> +
> +static ssize_t cxl_reset_store(struct device *dev,
> +			       struct device_attribute *attr,
> +			       const char *buf, size_t count)
> +{
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +	int rc;
> +
> +	if (!sysfs_streq(buf, "1"))
> +		return -EINVAL;
> +
> +	rc = cxl_do_reset(pdev);
> +	return rc ? rc : count;
> +}
> +static DEVICE_ATTR_WO(cxl_reset);
> +
> +static umode_t cxl_reset_attr_is_visible(struct kobject *kobj,
> +					  struct attribute *a, int n)
> +{
> +	struct pci_dev *pdev = to_pci_dev(kobj_to_dev(kobj));
> +
> +	if (!pci_cxl_reset_capable(pdev))
> +		return 0;
> +
> +	return a->mode;
> +}
> +
> +static struct attribute *cxl_reset_attrs[] = {
> +	&dev_attr_cxl_reset.attr,
> +	NULL,

No comma on a terminating entry. We don't want to make it easy to
add stuff after this!

> +};