Re: [PATCH] cxl/mem: Hide RCD attributes when pre-requisites are missing

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

On Tue, 10 Dec 2024 12:37:25 -0800
Dan Williams <dan.j.williams@intel.com> wrote:

> A recent change to avoid a NULL rcd_pcie_cap pointer consumption in
> rcd_pcie_cap_emit() [1] arranged for the problematic sysfs attributes to
> return an error on access. However, if the attributes always fail on
> read they should simply be hidden.
> 
> Add a facility called @mem_groups that follows the same visibility rules
> as the "dev_groups" of the cxl_mem driver. It is expressly for
> PCI-device relative attributes, but that require the device to be
> additionally connected to the CXL port topology.
> 
> Link: http://lore.kernel.org/20241129132825.569237-1-ming.li@zohomail.com
> Cc: Li Ming <ming.li@zohomail.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
Change makes sense. Only real comments are whether the prints
are too noisy.  Maybe demote to dev_dbg?

Jonathan


> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index af92c67bc954..a53f144bbac1 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
> @@ -1397,6 +1397,7 @@ static void delete_endpoint(void *data)
>  {
>  	struct cxl_memdev *cxlmd = data;
>  	struct cxl_port *endpoint = cxlmd->endpoint;
> +	struct device *parent = cxlmd->dev.parent;
>  	struct device *host = endpoint_host(endpoint);
>  
>  	scoped_guard(device, host) {
> @@ -1407,6 +1408,8 @@ static void delete_endpoint(void *data)
>  		}
>  		cxlmd->endpoint = NULL;
>  	}
> +	if (sysfs_update_groups(&parent->kobj, cxlmd->mem_groups))
> +		dev_info(parent, "CXL.mem sysfs attributes removed\n");

dev_info?  That seems noisy and very implementation specific.


>  	put_device(&endpoint->dev);
>  	put_device(host);
>  }
> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> index 2a25d1957ddb..9c5f2544da75 100644
> --- a/drivers/cxl/cxlmem.h
> +++ b/drivers/cxl/cxlmem.h
> @@ -43,6 +43,7 @@
>   * @cxl_nvb: coordinate removal of @cxl_nvd if present
>   * @cxl_nvd: optional bridge to an nvdimm if the device supports pmem
>   * @endpoint: connection to the CXL port topology for this memory device
> + * @mem_groups: host device groups that change visibility based on cxl_mem attach
>   * @id: id number of this memdev instance.
>   * @depth: endpoint port depth
>   */
> @@ -54,6 +55,7 @@ struct cxl_memdev {
>  	struct cxl_nvdimm_bridge *cxl_nvb;
>  	struct cxl_nvdimm *cxl_nvd;
>  	struct cxl_port *endpoint;
> +	const struct attribute_group **mem_groups;
>  	int id;
>  	int depth;
>  };
> @@ -87,8 +89,9 @@ static inline bool is_cxl_endpoint(struct cxl_port *port)
>  	return is_cxl_memdev(port->uport_dev);
>  }
>  
> -struct cxl_memdev *devm_cxl_add_memdev(struct device *host,
> -				       struct cxl_dev_state *cxlds);
> +struct cxl_memdev *
> +devm_cxl_add_memdev(struct device *host, struct cxl_dev_state *cxlds,
> +		    const struct attribute_group **mem_groups);

I'd have been tempted to just go long lines and reduce the churn
caused be the reformat, but not important really.

>  int devm_cxl_sanitize_setup_notifier(struct device *host,
>  				     struct cxl_memdev *cxlmd);
>  struct cxl_memdev_state;
> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
> index a9fd5cd5a0d2..2d2a662c99d4 100644
> --- a/drivers/cxl/mem.c
> +++ b/drivers/cxl/mem.c
> @@ -50,6 +50,7 @@ static int devm_cxl_add_endpoint(struct device *host, struct cxl_memdev *cxlmd,
>  {
>  	struct cxl_port *parent_port = parent_dport->port;
>  	struct cxl_port *endpoint, *iter, *down;
> +	struct device *parent;
>  	int rc;
>  
>  	/*
> @@ -70,6 +71,13 @@ static int devm_cxl_add_endpoint(struct device *host, struct cxl_memdev *cxlmd,
>  	if (IS_ERR(endpoint))
>  		return PTR_ERR(endpoint);
>  
> +	parent = cxlmd->dev.parent;
> +	rc = sysfs_update_groups(&parent->kobj, cxlmd->mem_groups);
> +	if (rc) {
> +		dev_err(parent, "CXL.mem sysfs attributes removed\n");

Likewise, is this too noisy?

> +		return rc;
> +	}
> +
>  	rc = cxl_endpoint_autoremove(cxlmd, endpoint);
>  	if (rc)
>  		return rc;