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

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

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