Re: [PATCH v3 01/20] cxl: add type2 device basic support

Jonathan Cameron
2024-09-16T12:24:08Z

On Mon, 16 Sep 2024 13:03:10 +0100
Alejandro Lucero Palau <alucerop@amd.com> wrote:

> On 9/13/24 17:41, Jonathan Cameron wrote:

> >> Add SFC ethernet network driver as the client.
> > Minor thing (And others may disagree) but I'd split this to be nice
> > to others who might want to backport the type2 support but not
> > the sfc changes (as they are supporting some other hardware).
>
>
> Should I then send incremental sfc changes as well as the API is
> introduced or just a final patch with all of it?

Given aim is to justify each step for this first user I think
incremental sfc changes do make sense.

>
>
> >> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
> >> Co-developed-by: Dan Williams <dan.j.williams@intel.com>
> >
> >> +int cxl_set_resource(struct cxl_dev_state *cxlds, struct resource res,
> >> + enum cxl_resource type)
> >> +{
> >> + switch (type) {
> >> + case CXL_ACCEL_RES_DPA:
> >> + cxlds->dpa_res = res;
> >> + return 0;
> >> + case CXL_ACCEL_RES_RAM:
> >> + cxlds->ram_res = res;
> >> + return 0;
> >> + case CXL_ACCEL_RES_PMEM:
> >> + cxlds->pmem_res = res;
> >> + return 0;
> >> + default:
> >> + dev_err(cxlds->dev, "unknown resource type (%u)\n", type);
> > It's an enum, do we need the default? Hence do we need the return value?
> >
>
> I think it does not harm and helps with extending the enum without
> silently failing if all the places where it is used are not properly
> updated.

It won't silently fail. The various build bots love to point out unhandled
cases :) Adding the default means that you'll only see the problem
in runtime testing rather than at build time.

>
>
> >> + return -EINVAL;
> >> + }
> >> +}
> >> +EXPORT_SYMBOL_NS_GPL(cxl_set_resource, CXL);
> >> +
> >> static int cxl_memdev_release_file(struct inode *inode, struct file *file)
> >> {
> >> struct cxl_memdev *cxlmd =
> >> + if (!dvsec)
> >> dev_warn(&pdev->dev,
> >> "Device DVSEC not present, skip CXL.mem init\n");
> >> + else
> >> + cxl_set_dvsec(cxlds, dvsec);
> > Set it unconditionally perhaps. If it's NULL that's fine and then it corresponds
> > directly to the previous
>
>
> OK. I guess keeping the dev_warn. Right?

Absolutely.

> >> diff --git a/drivers/net/ethernet/sfc/efx.c b/drivers/net/ethernet/sfc/efx.c
> >> index 6f1a01ded7d4..3a7406aa950c 100644
> >> --- a/drivers/net/ethernet/sfc/efx.c
> >> +++ b/drivers/net/ethernet/sfc/efx.c

> >> @@ -1109,6 +1113,15 @@ static int efx_pci_probe(struct pci_dev *pci_dev,
> >> if (rc)
> >> goto fail2;
> >>
> >> + /* A successful cxl initialization implies a CXL region created to be
> >> + * used for PIO buffers. If there is no CXL support, or initialization
> >> + * fails, efx_cxl_pio_initialised wll be false and legacy PIO buffers
> >> + * defined at specific PCI BAR regions will be used.
> >> + */
> >> + rc = efx_cxl_init(efx);
> >> + if (rc)
> >> + pci_err(pci_dev, "CXL initialization failed with error %d\n", rc);
> > If you are carrying on anyway is pci_info() more appropriate?
> > Personally I dislike muddling on in error cases, but understand
> > it can be useful on occasion at the cost of more complex flows.
> >
> >
>
> Not sure. Note this is for the case something went wrong when the device
> has CXL support.
>
> It is not fatal, but it is an error.

Fair enough. I don't care that much about this.
>
>
> >> +
> >> rc = efx_pci_probe_post_io(efx);
> >> if (rc) {
> >> /* On failure, retry once immediately.
> >> diff --git a/drivers/net/ethernet/sfc/efx_cxl.c b/drivers/net/ethernet/sfc/efx_cxl.c
> >> new file mode 100644
> >> index 000000000000..bba36cbbab22
> >> --- /dev/null
> >> +++ b/drivers/net/ethernet/sfc/efx_cxl.c

> > //maybe also cxlds as then you can use __free() to handle the
> > //cleanup paths for both allowing early returns instead
> > //of gotos.
>
>
> Maybe, but using __free is discouraged in network code: 1.6.5 at
>
> https://www.kernel.org/doc/html/latest/process/maintainer-netdev.html

Fair enough. I've not been keeping up with networking maintainer
preferences recently.

Jonathan