Re: [PATCH 12/16] PCI: PCIe portdrv: Add cxl_isolation sysfs attributes

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

On Wed, 30 Jul 2025 16:47:14 -0500
Ben Cheatham <Benjamin.Cheatham@amd.com> wrote:

> Add sysfs attributes to enable/disable CXL isolation and transaction
> timeout. The intended use for these attributes is to disable isolation
> and/or timeout as part of device maintenance or hotplug.
> 
> The attributes are added under a new "cxl_isolation" group on the PCIe
> Root Port device.
> 
> Signed-off-by: Ben Cheatham <Benjamin.Cheatham@amd.com>
Comments below apply in quite a few of these functions, I just
picked one example to talk about.

Thanks,

Jonathan

> diff --git a/drivers/pci/pcie/cxl_isolation.c b/drivers/pci/pcie/cxl_isolation.c
> index 5a56a327b599..9d2ad14810e8 100644
> --- a/drivers/pci/pcie/cxl_isolation.c
> +++ b/drivers/pci/pcie/cxl_isolation.c



> +static ssize_t timeout_ctrl_show(struct device *dev,
> +				 struct device_attribute *attr, char *buf)
> +{
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +	struct cxl_port *port;
> +	u32 ctrl;
> +
> +	struct cxl_dport **dport __free(kfree) =
> +		kzalloc(sizeof(*dport), GFP_KERNEL);
> +	if (!dport)
> +		return -ENOMEM;
> +
> +	port = cxl_find_pcie_rp(pdev, dport);
	struct cxl_port *port __free(put_cxl_port) = cxl_find_pcie_rp();

Same for other cases above.


> +	if (!port || !(*dport))

leaks device reference if port is set and dport isn't.

> +		return -ENODEV;
> +
> +	if (!(*dport)->regs.isolation)
leaks device reference.
> +		return -ENXIO;
> +
> +	ctrl = readl((*dport)->regs.isolation + CXL_ISOLATION_CTRL_OFFSET);
> +	put_device(&port->dev);

and no need to do this by hand.

> +
> +	return sysfs_emit(buf, "%lu\n",
> +			  FIELD_GET(CXL_ISOLATION_CTRL_MEM_TIME_ENABLE, ctrl));
> +}
> +DEVICE_ATTR_RW(timeout_ctrl);
> +
> +static struct attribute *isolation_attrs[] = {
> +	&dev_attr_timeout_ctrl.attr,
> +	&dev_attr_isolation_ctrl.attr,
> +	NULL,
My favorite trivial comment :) No need for that trailing comma.
> +};