Re: [PATCH v9 01/15] cxl/port: Pre-initialize component register mappings

[Jonathan Cameron]

On Thu, 31 Aug 2023 14:22:35 +0200
Robert Richter <rrichter@xxxxxxx> wrote:

> On 29.08.23 14:38:51, Jonathan Cameron wrote:
> > On Fri, 25 Aug 2023 18:31:57 -0500
> > Terry Bowman <terry.bowman@xxxxxxx> wrote:
> >   
> > > From: Robert Richter <rrichter@xxxxxxx>  
> > 
> > Hi Robert, Terry,
> >   
> > > 
> > > The component registers of a component may not exist or are not
> > > needed.  
> > 
> > How do we now it's not needed in this function?
> > Perhaps "may not exist." is the bit that matters in this sentence.
> >   
> > > The setup may fail for that reason. In some cases the
> > > initialization should continue anyway. Thus, always initialize struct
> > > cxl_register_map with valid values. In case of errors, zero it, set a
> > > value for @dev and make @resource a the valid value using  
> > 
> > make @resource CXL_RESOURCE_NONE.
> > 
> > (not "a the")
> >   
> > > CXL_RESOURCE_NONE.  
> > 
> > It might be worth making it clear that this will (I think) only matter
> > for future usecases and isn't a fix for how this function is used today.  
> 
> I reworded the whole text:
> 
> """
> The component registers of a component may not exist and
> cxl_setup_comp_regs() will fail for that reason. In another case,
> Software may not use and set those registers up. cxl_setup_comp_regs()
> is then called with a base address of CXL_RESOURCE_NONE. Both are
> valid cases, but the function returns without initializing the
> register map.
> 
> Now, a missing component register block is not necessarily a reason to
> fail (feature is optional or its existence checked later). Change
> cxl_setup_comp_regs() to also use components with the component
> register block missing. Thus, always initialize struct
> cxl_register_map with valid values, set @dev and make @resource
> CXL_RESOURCE_NONE.
> 
> The change is in preparation of follow-on patches.
> """

Looks good to me.

J
> 
> I hope that is better now.
> 
> >   
> > > 
> > > Signed-off-by: Robert Richter <rrichter@xxxxxxx>
> > > Signed-off-by: Terry Bowman <terry.bowman@xxxxxxx>  
> > Otherwise seems sensible to me with one comment below.
> > 
> > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
> >   
> > > ---
> > >  drivers/cxl/core/port.c | 11 ++++++-----
> > >  1 file changed, 6 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> > > index 724be8448eb4..2d22e7a5629b 100644
> > > --- a/drivers/cxl/core/port.c
> > > +++ b/drivers/cxl/core/port.c
> > > @@ -693,16 +693,17 @@ static struct cxl_port *cxl_port_alloc(struct device *uport_dev,
> > >  static int cxl_setup_comp_regs(struct device *dev, struct cxl_register_map *map,
> > >  			       resource_size_t component_reg_phys)
> > >  {
> > > -	if (component_reg_phys == CXL_RESOURCE_NONE)
> > > -		return 0;
> > > -
> > >  	*map = (struct cxl_register_map) {
> > >  		.dev = dev,
> > > -		.reg_type = CXL_REGLOC_RBI_COMPONENT,  
> > 
> > Could set this explicitly to CXL_REGLOC_RBI_EMPTY
> > which is what happens anyway, but it isn't obvious that
> > 0 maps to something that indicates this doesn't exist.  
> 
> Will change that.
> 
> Thanks,
> 
> -Robert
> 
> 
> >   
> > >  		.resource = component_reg_phys,
> > > -		.max_size = CXL_COMPONENT_REG_BLOCK_SIZE,
> > >  	};
> > >  
> > > +	if (component_reg_phys == CXL_RESOURCE_NONE)
> > > +		return 0;
> > > +
> > > +	map->reg_type = CXL_REGLOC_RBI_COMPONENT;
> > > +	map->max_size = CXL_COMPONENT_REG_BLOCK_SIZE;
> > > +
> > >  	return cxl_setup_regs(map);
> > >  }
> > >    
> >