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

Commit Message

Bowman, Terry Aug. 25, 2023, 11:31 p.m. UTC

From: Robert Richter <[e-mail]>

The component registers of a component may not exist or are not
needed. 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
CXL_RESOURCE_NONE.

Signed-off-by: Robert Richter <[e-mail]>
Signed-off-by: Terry Bowman <[e-mail]>
---
 drivers/cxl/core/port.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

Comments

Jonathan Cameron Aug. 29, 2023, 1:38 p.m. UTC | #1

On Fri, 25 Aug 2023 18:31:57 -0500
Terry Bowman <[e-mail]> wrote:

> From: Robert Richter <[e-mail]>

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.

> 
> Signed-off-by: Robert Richter <[e-mail]>
> Signed-off-by: Terry Bowman <[e-mail]>
Otherwise seems sensible to me with one comment below.

Reviewed-by: Jonathan Cameron <[e-mail]>

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

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

Robert Richter Aug. 31, 2023, 12:22 p.m. UTC | #2

On 29.08.23 14:38:51, Jonathan Cameron wrote:
> On Fri, 25 Aug 2023 18:31:57 -0500
> Terry Bowman <[e-mail]> wrote:
> 
> > From: Robert Richter <[e-mail]>
> 
> 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 <[e-mail]>
> > Signed-off-by: Terry Bowman <[e-mail]>
> Otherwise seems sensible to me with one comment below.
> 
> Reviewed-by: Jonathan Cameron <[e-mail]>
> 
> > ---
> >  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);
> >  }
> >  
>

Jonathan Cameron Sept. 1, 2023, 9:06 a.m. UTC | #3

On Thu, 31 Aug 2023 14:22:35 +0200
Robert Richter <[e-mail]> wrote:

> On 29.08.23 14:38:51, Jonathan Cameron wrote:
> > On Fri, 25 Aug 2023 18:31:57 -0500
> > Terry Bowman <[e-mail]> wrote:
> >   
> > > From: Robert Richter <[e-mail]>  
> > 
> > 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 <[e-mail]>
> > > Signed-off-by: Terry Bowman <[e-mail]>  
> > Otherwise seems sensible to me with one comment below.
> > 
> > Reviewed-by: Jonathan Cameron <[e-mail]>
> >   
> > > ---
> > >  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);
> > >  }
> > >    
> >

Patch

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