netdev - Re: [PATCH v8 06/27] cxl: add function for type2 cxl regs setup

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <5e25e3a5-cf28-a3b3-0309-d21593eca75b@amd.com>
Date: Mon, 30 Dec 2024 09:01:04 +0000
From: Alejandro Lucero Palau <alucerop@....com>
To: Jonathan Cameron <Jonathan.Cameron@...wei.com>,
 alejandro.lucero-palau@....com
Cc: linux-cxl@...r.kernel.org, netdev@...r.kernel.org,
 dan.j.williams@...el.com, martin.habets@...inx.com, edward.cree@....com,
 davem@...emloft.net, kuba@...nel.org, pabeni@...hat.com,
 edumazet@...gle.com, dave.jiang@...el.com
Subject: Re: [PATCH v8 06/27] cxl: add function for type2 cxl regs setup


On 12/27/24 08:04, Alejandro Lucero Palau wrote:
>
> On 12/24/24 17:22, Jonathan Cameron wrote:
>> On Mon, 16 Dec 2024 16:10:21 +0000
>> alejandro.lucero-palau@....com wrote:
>>
>>> From: Alejandro Lucero <alucerop@....com>
>>>
>>> Create a new function for a type2 device initialising
>>> cxl_dev_state struct regarding cxl regs setup and mapping.
>>>
>>> Signed-off-by: Alejandro Lucero <alucerop@....com>
>>> Reviewed-by: Dave Jiang <dave.jiang@...el.com>
>>> Reviewed-by: Fan Ni <fan.ni@...sung.com>
>> Comments below.
>>
>> J
>>> ---
>>>   drivers/cxl/core/pci.c | 47 
>>> ++++++++++++++++++++++++++++++++++++++++++
>>>   include/cxl/cxl.h      |  2 ++
>>>   2 files changed, 49 insertions(+)
>>>
>>> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
>>> index 3cca3ae438cd..0b578ff14cc3 100644
>>> --- a/drivers/cxl/core/pci.c
>>> +++ b/drivers/cxl/core/pci.c
>>> @@ -1096,6 +1096,53 @@ int cxl_pci_setup_regs(struct pci_dev *pdev, 
>>> enum cxl_regloc_type type,
>>>   }
>>>   EXPORT_SYMBOL_NS_GPL(cxl_pci_setup_regs, "CXL");
>>>   +static int cxl_pci_setup_memdev_regs(struct pci_dev *pdev,
>>> +                     struct cxl_dev_state *cxlds)
>>> +{
>>> +    struct cxl_register_map map;
>>> +    int rc;
>>> +
>>> +    rc = cxl_pci_setup_regs(pdev, CXL_REGLOC_RBI_MEMDEV, &map,
>>> +                cxlds->capabilities);
>>> +    /*
>>> +     * This call returning a non-zero value is not considered an 
>>> error since
>> Error code perhaps rather than non-zero value?
>
>
> Makes sense.
>

I think the right way to solve this is to have a check for rc being 
-ENODEV and returning 0 for that specific case, then returning rc for 
any other error.

I'll add such rationale in the comment as well for v9.


>
>>
>>> +     * these regs are not mandatory for Type2. If they do exist 
>>> then mapping
>>> +     * them should not fail.
>>> +     */
>>> +    if (rc)
>>> +        return 0;
>>> +
>>> +    return cxl_map_device_regs(&map, &cxlds->regs.device_regs);
>>> +}
>>> +
>>> +int cxl_pci_accel_setup_regs(struct pci_dev *pdev, struct 
>>> cxl_dev_state *cxlds)
>>> +{
>>> +    int rc;
>>> +
>>> +    rc = cxl_pci_setup_memdev_regs(pdev, cxlds);
>>> +    if (rc)
>>> +        return rc;
>>> +
>>> +    rc = cxl_pci_setup_regs(pdev, CXL_REGLOC_RBI_COMPONENT,
>>> +                &cxlds->reg_map, cxlds->capabilities);
>>> +    if (rc) {
>>> +        dev_warn(&pdev->dev, "No component registers (%d)\n", rc);
>>> +        return rc;
>>> +    }
>>> +
>>> +    if (!test_bit(CXL_CM_CAP_CAP_ID_RAS, cxlds->capabilities))
>> rc is 0.  I doubt that's the intent - if it is, return 0;
>
>
> Well, if the conditional is true, it is the end of the function, and 
> we know there is no errors,so yes, return 0 will make it.
>
>
>>
>>
>>> +        return rc;
>>> +
>>> +    rc = cxl_map_component_regs(&cxlds->reg_map,
>>> +                    &cxlds->regs.component,
>>> +                    BIT(CXL_CM_CAP_CAP_ID_RAS));
>>> +    if (rc)
>>> +        dev_dbg(&pdev->dev, "Failed to map RAS capability.\n");
>>> +
>>> +    return rc;
>>> +}
>>> +EXPORT_SYMBOL_NS_GPL(cxl_pci_accel_setup_regs, "CXL");
>>> diff --git a/include/cxl/cxl.h b/include/cxl/cxl.h
>>> index 05f06bfd2c29..18fb01adcf19 100644
>>> --- a/include/cxl/cxl.h
>>> +++ b/include/cxl/cxl.h
>>> @@ -5,6 +5,7 @@
>>>   #define __CXL_H
>>>     #include <linux/ioport.h>
>>> +#include <linux/pci.h>
>> Use a forwards def if all you need is
>> struct pci_dev;
>>
>
> I'll do.
>
> Thanks
>
>
>>>     enum cxl_resource {
>>>       CXL_RES_DPA,
>>> @@ -40,4 +41,5 @@ int cxl_set_resource(struct cxl_dev_state *cxlds, 
>>> struct resource res,
>>>   bool cxl_pci_check_caps(struct cxl_dev_state *cxlds,
>>>               unsigned long *expected_caps,
>>>               unsigned long *current_caps);
>>> +int cxl_pci_accel_setup_regs(struct pci_dev *pdev, struct 
>>> cxl_dev_state *cxlds);
>>>   #endif