Re: [PATCH v6 8/9] cxl/memdev: Add cxl_reset sysfs attribute

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The probe logic incorrectly restricts the 'cxl_reset' attribute to Type 2 CXL devices, completely excluding standard Type 3 memory expanders.
- [High] The reset probe logic searches for the CXL Device DVSEC on the current PCI function instead of Function 0, causing the capability probe to erroneously fail on multi-function devices.
--

commit 0a3bf8b7cbd748c710f06177b6aba985e72cc401
Author: Srirangan Madhavan <smadhavan@nvidia.com>

cxl/memdev: Add cxl_reset sysfs attribute

This commit exposes the CXL reset functionality via a write-only sysfs
attribute on the CXL memdev device. The attribute is only made visible
when the underlying PCI parent device advertises CXL Reset capability.

> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> index 486c447e98f3f..09f016544d245 100644
> --- a/drivers/cxl/core/pci.c
> +++ b/drivers/cxl/core/pci.c

[ ... ]

> @@ -1207,6 +1207,22 @@ static bool cxl_reset_has_cache_or_mem(struct pci_dev *pdev)
>  	return cap & (PCI_DVSEC_CXL_CACHE_CAPABLE | PCI_DVSEC_CXL_MEM_CAPABLE);
>  }
>  
> +static bool cxl_reset_is_type2(struct pci_dev *pdev)
> +{
> +	u16 dvsec, 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;
> +
> +	return (cap & PCI_DVSEC_CXL_CACHE_CAPABLE) &&
> +	       (cap & PCI_DVSEC_CXL_MEM_CAPABLE);

[Severity: High]
Does this logic unintentionally exclude standard Type 3 memory expanders?

Standard Type 3 devices only advertise the PCI_DVSEC_CXL_MEM_CAPABLE bit.
Because this function requires both the cache and mem bits to be set, it
will fail for Type 3 memdevs, preventing the cxl_reset attribute from
becoming visible.

> +}
> +

[ ... ]

> @@ -1966,3 +1982,87 @@ static int __maybe_unused cxl_do_reset(struct pci_dev *pdev, bool mem_clear)

[ ... ]

> +static bool cxl_memdev_probe_reset_capable(struct cxl_memdev *cxlmd)
> +{
> +	struct device *dev = cxlmd->dev.parent;
> +	struct pci_dev *pdev, *fn0;
> +	int dvsec;
> +	u16 cap;
> +
> +	if (!dev || !dev_is_pci(dev))
> +		return false;
> +
> +	pdev = to_pci_dev(dev);
> +	if (!cxl_reset_is_type2(pdev))
> +		return false;
> +
> +	fn0 = cxl_reset_get_fn0(pdev);

[Severity: High]
Can this capability probe erroneously fail on multi-function devices?

The code checks for the CXL Device DVSEC on the current PCI function
(via cxl_reset_is_type2(pdev)) before acquiring the Function 0 pointer.

Since the CXL specification requires the CXL Device DVSEC to be implemented
on Function 0, calling pci_find_dvsec_capability on pdev directly will fail
if the memdev is bound to a function greater than 0.

Could this check be moved to happen after getting fn0?

> +	if (!fn0)
> +		return false;
> +

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260528083154.137979-1-smadhavan@nvidia.com?part=8