Re: [PATCH v2 3/5] cxl/mem: Map registers based on capabilities

[Ira Weiny]

On Tue, May 25, 2021 at 10:52:14AM +0100, Jonathan Cameron wrote:
> On Fri, 21 May 2021 17:11:52 -0700
> <ira.weiny@xxxxxxxxx> wrote:
> 
> > From: Ira Weiny <ira.weiny@xxxxxxxxx>
> > 
> > The information required to map registers based on capabilities is
> > contained within the bars themselves.  This means the bar must be mapped
> > to read the information needed and then unmapped to map the individual
> > parts of the BAR based on capabilities.
> > 
> > Change cxl_setup_device_regs() to return a new cxl_register_map, change
> > the name to cxl_probe_device_regs().  Allocate and place
> > cxl_register_maps on a list while processing all of the specified
> > register blocks.
> > 
> > After probing all the register blocks go back and map smaller registers
> > blocks based on their capabilities and dispose of the cxl_register_maps.
> > 
> > NOTE: pci_iomap() is not managed automatically via pcim_enable_device()
> > so be careful to call pci_iounmap() correctly.
> > 
> > Signed-off-by: Ira Weiny <ira.weiny@xxxxxxxxx>
> A couple of really minor queries inline, but otherwise looks good to me.
> 
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>

Thanks!

> 
> > 
> > diff --git a/drivers/cxl/core.c b/drivers/cxl/core.c
> > index 38979c97158d..add66a6ec875 100644
> > --- a/drivers/cxl/core.c
> > +++ b/drivers/cxl/core.c
> > @@ -3,6 +3,7 @@
> >  #include <linux/io-64-nonatomic-lo-hi.h>
> >  #include <linux/device.h>
> >  #include <linux/module.h>
> > +#include <linux/pci.h>
> >  #include "cxl.h"
> >  
> >  /**
> > @@ -12,19 +13,13 @@
> >   * point for cross-device interleave coordination through cxl ports.
> >   */
> >  
> > -/**
> > - * cxl_setup_device_regs() - Detect CXL Device register blocks
> > - * @dev: Host device of the @base mapping
> > - * @base: Mapping of CXL 2.0 8.2.8 CXL Device Register Interface
> > - * @regs: Base pointers for device register blocks (see CXL_DEVICE_REGS())
> > - */
> 
> Nice to keep docs given this is an exported function.

I can write something better but the above does not add much IMO.  The
parameter explanations are unnecessary IMO.

> >  
> > @@ -1030,30 +1091,38 @@ static int cxl_mem_setup_regs(struct cxl_mem *cxlm)
> >  		dev_dbg(dev, "Found register block in bar %u @ 0x%llx of type %u\n",
> >  			bar, offset, reg_type);
> >  
> > -		if (reg_type == CXL_REGLOC_RBI_MEMDEV) {
> > -			base = cxl_mem_map_regblock(cxlm, bar, offset);
> > -			if (!base)
> > -				return -ENOMEM;
> > -			break;
> > +		base = cxl_mem_map_regblock(cxlm, bar, offset);
> > +		if (!base) {
> > +			ret = -ENOMEM;
> > +			goto free_maps;
> >  		}
> > -	}
> >  
> > -	if (i == regblocks) {
> > -		dev_err(dev, "Missing register locator for device registers\n");
> > -		return -ENXIO;
> 
> Do we have or need an equivalent of this check somewhere else?

Yes agreed!  I moved the check to cxl_probe_regs which returns -ENXIO if the
register sets expected are not found.  A check is also added to RBI_COMPONENT
register type in the following patch.  This was moved mainly because each
register type is going to know better what it needs for proper operation.
cxl_probe_device_regs() really can't know that after this series.

Ira