[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <8b1c6606-cffe-41d5-82b2-a8fe65686e62@amd.com>
Date: Tue, 25 Mar 2025 14:21:48 +0000
From: Alejandro Lucero Palau <alucerop@....com>
To: Ben Cheatham <benjamin.cheatham@....com>, alejandro.lucero-palau@....com
Cc: linux-cxl@...r.kernel.org, netdev@...r.kernel.org,
dan.j.williams@...el.com, edward.cree@....com, davem@...emloft.net,
kuba@...nel.org, pabeni@...hat.com, edumazet@...gle.com, dave.jiang@...el.com
Subject: Re: [PATCH v11 04/23] cxl: move register/capability check to driver
Hi Ben,
I forgot to reply to this one. Apologies.
My comments below.
On 3/11/25 20:05, Ben Cheatham wrote:
> On 3/10/25 4:03 PM, alejandro.lucero-palau@....com wrote:
>> From: Alejandro Lucero <alucerop@....com>
>>
>> Type3 has some mandatory capabilities which are optional for Type2.
>>
>> In order to support same register/capability discovery code for both
>> types, avoid any assumption about what capabilities should be there, and
>> export the capabilities found for the caller doing the capabilities
>> check based on the expected ones.
>>
>> Signed-off-by: Alejandro Lucero <alucerop@....com>
>> ---
>> drivers/cxl/core/pci.c | 4 ++--
>> drivers/cxl/core/port.c | 8 ++++----
>> drivers/cxl/core/regs.c | 37 +++++++++++++++++++++----------------
>> drivers/cxl/cxl.h | 6 +++---
>> drivers/cxl/cxlpci.h | 2 +-
>> drivers/cxl/pci.c | 31 ++++++++++++++++++++++++++++---
>> include/cxl/cxl.h | 20 ++++++++++++++++++++
>> 7 files changed, 79 insertions(+), 29 deletions(-)
>>
>> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
>> index 0b8dc34b8300..05399292209a 100644
>> --- a/drivers/cxl/core/pci.c
>> +++ b/drivers/cxl/core/pci.c
>> @@ -1061,7 +1061,7 @@ static int cxl_rcrb_get_comp_regs(struct pci_dev *pdev,
>> }
>>
>> int cxl_pci_setup_regs(struct pci_dev *pdev, enum cxl_regloc_type type,
>> - struct cxl_register_map *map)
>> + struct cxl_register_map *map, unsigned long *caps)
>> {
>> int rc;
>>
>> @@ -1091,7 +1091,7 @@ int cxl_pci_setup_regs(struct pci_dev *pdev, enum cxl_regloc_type type,
>> return rc;
>> }
>>
>> - return cxl_setup_regs(map);
>> + return cxl_setup_regs(map, caps);
>> }
>> EXPORT_SYMBOL_NS_GPL(cxl_pci_setup_regs, "CXL");
>>
>> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
>> index 6a44b6dad3c7..ede36f7168ed 100644
>> --- a/drivers/cxl/core/port.c
>> +++ b/drivers/cxl/core/port.c
>> @@ -755,7 +755,7 @@ static struct cxl_port *cxl_port_alloc(struct device *uport_dev,
>> }
>>
>> static int cxl_setup_comp_regs(struct device *host, struct cxl_register_map *map,
>> - resource_size_t component_reg_phys)
>> + resource_size_t component_reg_phys, unsigned long *caps)
>> {
>> *map = (struct cxl_register_map) {
>> .host = host,
>> @@ -769,7 +769,7 @@ static int cxl_setup_comp_regs(struct device *host, struct cxl_register_map *map
>> map->reg_type = CXL_REGLOC_RBI_COMPONENT;
>> map->max_size = CXL_COMPONENT_REG_BLOCK_SIZE;
>>
>> - return cxl_setup_regs(map);
>> + return cxl_setup_regs(map, caps);
>> }
>>
>> static int cxl_port_setup_regs(struct cxl_port *port,
>> @@ -778,7 +778,7 @@ static int cxl_port_setup_regs(struct cxl_port *port,
>> if (dev_is_platform(port->uport_dev))
>> return 0;
>> return cxl_setup_comp_regs(&port->dev, &port->reg_map,
>> - component_reg_phys);
>> + component_reg_phys, NULL);
>> }
>>
>> static int cxl_dport_setup_regs(struct device *host, struct cxl_dport *dport,
>> @@ -795,7 +795,7 @@ static int cxl_dport_setup_regs(struct device *host, struct cxl_dport *dport,
>> * NULL.
>> */
>> rc = cxl_setup_comp_regs(dport->dport_dev, &dport->reg_map,
>> - component_reg_phys);
>> + component_reg_phys, NULL);
> Nit here, but if you just pass in a unsigned long here, and in cxl_port_setup_regs() above, instead of NULL
> you can get rid of the null pointer checks in the register probe functions.
>
Not sure this kind of tricks are usual, but I do not like it. It avoids
the check, but it can confuse the reader, and the checks are not so many.
<snip>
>>
>> + /*
>> + * Checking mandatory caps are there as, at least, a subset of those
>> + * found.
>> + */
>> + if (!bitmap_subset(expected, found, CXL_MAX_CAPS)) {
>> + dev_err(&pdev->dev,
>> + "Expected mandatory capabilities not found: (%pb - %pb)\n",
> I think this will just print the bitmaps, so it would probably be good to highlight which is
> the mandatory capabilites map and which is the found. Maybe something like:
>
> "Found capabilities (%pb) are missing mandatory capabilities (%pb)\n"
>
I can do that.
>> + expected, found);
>> + return -ENXIO;
>> + }
>> +
>> rc = cxl_pci_type3_init_mailbox(cxlds);
>> if (rc)
>> return rc;
>> diff --git a/include/cxl/cxl.h b/include/cxl/cxl.h
>> index 5c6481136f93..02b73c82e5d8 100644
>> --- a/include/cxl/cxl.h
>> +++ b/include/cxl/cxl.h
>> @@ -25,6 +25,26 @@ enum cxl_devtype {
>>
>> struct device;
>>
>> +
>> +/* Capabilities as defined for:
>> + *
>> + * Component Registers (Table 8-22 CXL 3.1 specification)
>> + * Device Registers (8.2.8.2.1 CXL 3.1 specification)
> Update to 3.2 spec?
Sure.
Thanks!
Powered by blists - more mailing lists