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

[Robert Richter]

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.
"""

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);
> >  }
> >  
>