* [PATCH] cxl/mem: Hide RCD attributes when pre-requisites are missing
@ 2024-12-10 20:37 Dan Williams
2024-12-11 5:44 ` Li Ming
2024-12-24 16:11 ` Jonathan Cameron
0 siblings, 2 replies; 4+ messages in thread
From: Dan Williams @ 2024-12-10 20:37 UTC (permalink / raw)
To: dave.jiang; +Cc: Li Ming, alison.schofield, linux-cxl
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>
---
drivers/cxl/core/memdev.c | 14 +++++++++-----
drivers/cxl/core/port.c | 3 +++
drivers/cxl/cxlmem.h | 7 +++++--
drivers/cxl/mem.c | 8 ++++++++
drivers/cxl/pci.c | 21 +++++++++++++++++++--
tools/testing/cxl/test/mem.c | 2 +-
6 files changed, 45 insertions(+), 10 deletions(-)
diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
index 84fefb76dafa..7801bf64aafa 100644
--- a/drivers/cxl/core/memdev.c
+++ b/drivers/cxl/core/memdev.c
@@ -616,8 +616,10 @@ static void detach_memdev(struct work_struct *work)
static struct lock_class_key cxl_memdev_key;
-static struct cxl_memdev *cxl_memdev_alloc(struct cxl_dev_state *cxlds,
- const struct file_operations *fops)
+static struct cxl_memdev *
+cxl_memdev_alloc(struct cxl_dev_state *cxlds,
+ const struct file_operations *fops,
+ const struct attribute_group **mem_groups)
{
struct cxl_memdev *cxlmd;
struct device *dev;
@@ -633,6 +635,7 @@ static struct cxl_memdev *cxl_memdev_alloc(struct cxl_dev_state *cxlds,
goto err;
cxlmd->id = rc;
cxlmd->depth = -1;
+ cxlmd->mem_groups = mem_groups;
dev = &cxlmd->dev;
device_initialize(dev);
@@ -1017,15 +1020,16 @@ static const struct file_operations cxl_memdev_fops = {
.llseek = noop_llseek,
};
-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)
{
struct cxl_memdev *cxlmd;
struct device *dev;
struct cdev *cdev;
int rc;
- cxlmd = cxl_memdev_alloc(cxlds, &cxl_memdev_fops);
+ cxlmd = cxl_memdev_alloc(cxlds, &cxl_memdev_fops, mem_groups);
if (IS_ERR(cxlmd))
return cxlmd;
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");
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);
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");
+ return rc;
+ }
+
rc = cxl_endpoint_autoremove(cxlmd, endpoint);
if (rc)
return rc;
diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index b2cb81f6d9e7..7ad2fc42210e 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -885,8 +885,25 @@ static umode_t cxl_rcd_visible(struct kobject *kobj, struct attribute *a, int n)
struct device *dev = kobj_to_dev(kobj);
struct pci_dev *pdev = to_pci_dev(dev);
- if (is_cxl_restricted(pdev))
+ if (is_cxl_restricted(pdev)) {
+ struct cxl_dev_state *cxlds = dev_get_drvdata(dev);
+ struct cxl_memdev *cxlmd = cxlds->cxlmd;
+ struct cxl_dport *dport;
+
+ if (!cxlmd->endpoint)
+ return 0;
+
+ struct cxl_port *root __free(put_cxl_port) =
+ cxl_mem_find_port(cxlmd, &dport);
+
+ if (!root)
+ return 0;
+
+ if (!dport->regs.rcd_pcie_cap)
+ return 0;
+
return a->mode;
+ }
return 0;
}
@@ -993,7 +1010,7 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
if (rc)
return rc;
- cxlmd = devm_cxl_add_memdev(&pdev->dev, cxlds);
+ cxlmd = devm_cxl_add_memdev(&pdev->dev, cxlds, cxl_rcd_groups);
if (IS_ERR(cxlmd))
return PTR_ERR(cxlmd);
diff --git a/tools/testing/cxl/test/mem.c b/tools/testing/cxl/test/mem.c
index 71916e0e1546..cd294c9476ed 100644
--- a/tools/testing/cxl/test/mem.c
+++ b/tools/testing/cxl/test/mem.c
@@ -1543,7 +1543,7 @@ static int cxl_mock_mem_probe(struct platform_device *pdev)
cxl_mock_add_event_logs(&mdata->mes);
- cxlmd = devm_cxl_add_memdev(&pdev->dev, cxlds);
+ cxlmd = devm_cxl_add_memdev(&pdev->dev, cxlds, NULL);
if (IS_ERR(cxlmd))
return PTR_ERR(cxlmd);
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] cxl/mem: Hide RCD attributes when pre-requisites are missing
2024-12-10 20:37 [PATCH] cxl/mem: Hide RCD attributes when pre-requisites are missing Dan Williams
@ 2024-12-11 5:44 ` Li Ming
2024-12-11 5:52 ` Li Ming
2024-12-24 16:11 ` Jonathan Cameron
1 sibling, 1 reply; 4+ messages in thread
From: Li Ming @ 2024-12-11 5:44 UTC (permalink / raw)
To: Dan Williams, dave.jiang; +Cc: alison.schofield, linux-cxl
On 12/11/2024 4:37 AM, Dan Williams 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>
> ---
> drivers/cxl/core/memdev.c | 14 +++++++++-----
> drivers/cxl/core/port.c | 3 +++
> drivers/cxl/cxlmem.h | 7 +++++--
> drivers/cxl/mem.c | 8 ++++++++
> drivers/cxl/pci.c | 21 +++++++++++++++++++--
> tools/testing/cxl/test/mem.c | 2 +-
> 6 files changed, 45 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
> index 84fefb76dafa..7801bf64aafa 100644
> --- a/drivers/cxl/core/memdev.c
> +++ b/drivers/cxl/core/memdev.c
> @@ -616,8 +616,10 @@ static void detach_memdev(struct work_struct *work)
>
> static struct lock_class_key cxl_memdev_key;
>
> -static struct cxl_memdev *cxl_memdev_alloc(struct cxl_dev_state *cxlds,
> - const struct file_operations *fops)
> +static struct cxl_memdev *
> +cxl_memdev_alloc(struct cxl_dev_state *cxlds,
> + const struct file_operations *fops,
> + const struct attribute_group **mem_groups)
> {
> struct cxl_memdev *cxlmd;
> struct device *dev;
> @@ -633,6 +635,7 @@ static struct cxl_memdev *cxl_memdev_alloc(struct cxl_dev_state *cxlds,
> goto err;
> cxlmd->id = rc;
> cxlmd->depth = -1;
> + cxlmd->mem_groups = mem_groups;
>
> dev = &cxlmd->dev;
> device_initialize(dev);
> @@ -1017,15 +1020,16 @@ static const struct file_operations cxl_memdev_fops = {
> .llseek = noop_llseek,
> };
>
> -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)
> {
> struct cxl_memdev *cxlmd;
> struct device *dev;
> struct cdev *cdev;
> int rc;
>
> - cxlmd = cxl_memdev_alloc(cxlds, &cxl_memdev_fops);
> + cxlmd = cxl_memdev_alloc(cxlds, &cxl_memdev_fops, mem_groups);
> if (IS_ERR(cxlmd))
> return cxlmd;
>
> 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");
> 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);
> 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");
> + return rc;
> + }
Hi Dan,
I think the error log in devm_cxl_add_endpoint() should not be "CXL.mem sysfs attributes removed", "Failed to attach CXL.mem sysfs attributes" should be better?
Other looks good to me.
Reviewed-by: Li Ming <ming.li@zohomail.com>
Besides, RCEC address is stored in dport, but rcd upstream port pci express capability is a rcd capability, not a rch dport capability. Is it better to move rcd_pcie_cap into cxlds->regs? If yes, I can work on that.
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] cxl/mem: Hide RCD attributes when pre-requisites are missing
2024-12-11 5:44 ` Li Ming
@ 2024-12-11 5:52 ` Li Ming
0 siblings, 0 replies; 4+ messages in thread
From: Li Ming @ 2024-12-11 5:52 UTC (permalink / raw)
To: Dan Williams, dave.jiang; +Cc: alison.schofield, linux-cxl
On 12/11/2024 1:44 PM, Li Ming wrote:
> On 12/11/2024 4:37 AM, Dan Williams 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>
>> ---
>> drivers/cxl/core/memdev.c | 14 +++++++++-----
>> drivers/cxl/core/port.c | 3 +++
>> drivers/cxl/cxlmem.h | 7 +++++--
>> drivers/cxl/mem.c | 8 ++++++++
>> drivers/cxl/pci.c | 21 +++++++++++++++++++--
>> tools/testing/cxl/test/mem.c | 2 +-
>> 6 files changed, 45 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
>> index 84fefb76dafa..7801bf64aafa 100644
>> --- a/drivers/cxl/core/memdev.c
>> +++ b/drivers/cxl/core/memdev.c
>> @@ -616,8 +616,10 @@ static void detach_memdev(struct work_struct *work)
>>
>> static struct lock_class_key cxl_memdev_key;
>>
>> -static struct cxl_memdev *cxl_memdev_alloc(struct cxl_dev_state *cxlds,
>> - const struct file_operations *fops)
>> +static struct cxl_memdev *
>> +cxl_memdev_alloc(struct cxl_dev_state *cxlds,
>> + const struct file_operations *fops,
>> + const struct attribute_group **mem_groups)
>> {
>> struct cxl_memdev *cxlmd;
>> struct device *dev;
>> @@ -633,6 +635,7 @@ static struct cxl_memdev *cxl_memdev_alloc(struct cxl_dev_state *cxlds,
>> goto err;
>> cxlmd->id = rc;
>> cxlmd->depth = -1;
>> + cxlmd->mem_groups = mem_groups;
>>
>> dev = &cxlmd->dev;
>> device_initialize(dev);
>> @@ -1017,15 +1020,16 @@ static const struct file_operations cxl_memdev_fops = {
>> .llseek = noop_llseek,
>> };
>>
>> -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)
>> {
>> struct cxl_memdev *cxlmd;
>> struct device *dev;
>> struct cdev *cdev;
>> int rc;
>>
>> - cxlmd = cxl_memdev_alloc(cxlds, &cxl_memdev_fops);
>> + cxlmd = cxl_memdev_alloc(cxlds, &cxl_memdev_fops, mem_groups);
>> if (IS_ERR(cxlmd))
>> return cxlmd;
>>
>> 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");
>> 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);
>> 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");
>> + return rc;
>> + }
> Hi Dan,
>
>
> I think the error log in devm_cxl_add_endpoint() should not be "CXL.mem sysfs attributes removed", "Failed to attach CXL.mem sysfs attributes" should be better?
Sorry, I think that I misunderstood it, it is used to update the visibility, "CXL.mem sysfs attributes removed" is good, sorry about that.
>
> Other looks good to me.
>
> Reviewed-by: Li Ming <ming.li@zohomail.com>
>
>
> Besides, RCEC address is stored in dport, but rcd upstream port pci express capability is a rcd capability, not a rch dport capability. Is it better to move rcd_pcie_cap into cxlds->regs? If yes, I can work on that.
>
>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] cxl/mem: Hide RCD attributes when pre-requisites are missing
2024-12-10 20:37 [PATCH] cxl/mem: Hide RCD attributes when pre-requisites are missing Dan Williams
2024-12-11 5:44 ` Li Ming
@ 2024-12-24 16:11 ` Jonathan Cameron
1 sibling, 0 replies; 4+ messages in thread
From: Jonathan Cameron @ 2024-12-24 16:11 UTC (permalink / raw)
To: Dan Williams; +Cc: dave.jiang, Li Ming, alison.schofield, linux-cxl
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;
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2024-12-24 16:11 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-12-10 20:37 [PATCH] cxl/mem: Hide RCD attributes when pre-requisites are missing Dan Williams
2024-12-11 5:44 ` Li Ming
2024-12-11 5:52 ` Li Ming
2024-12-24 16:11 ` Jonathan Cameron
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox