diff mbox series

[07/18] cxl/region: Move region-position validation to a helper

Message ID 167564538779.847146.8356062886811511706.stgit@dwillia2-xfh.jf.intel.com (mailing list archive)
State Handled Elsewhere, archived
Headers show
Series CXL RAM and the 'Soft Reserved' => 'System RAM' default | expand

Commit Message

Dan Williams Feb. 6, 2023, 1:03 a.m. UTC
In preparation for region autodiscovery, that needs all devices
discovered before their relative position in the region can be
determined, consolidate all position dependent validation in a helper.

Recall that in the on-demand region creation flow the end-user picks the
position of a given endpoint decoder in a region. In the autodiscovery
case the position of an endpoint decoder can only be determined after
all other endpoint decoders that claim to decode the region's address
range have been enumerated and attached. So, in the autodiscovery case
endpoint decoders may be attached before their relative position is
known. Once all decoders arrive, then positions can be determined and
validated with cxl_region_validate_position() the same as user initiated
on-demand creation.

Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/cxl/core/region.c |  119 +++++++++++++++++++++++++++++----------------
 1 file changed, 76 insertions(+), 43 deletions(-)

Comments

Ira Weiny Feb. 6, 2023, 5:44 p.m. UTC | #1
Dan Williams wrote:
> In preparation for region autodiscovery, that needs all devices
> discovered before their relative position in the region can be
> determined, consolidate all position dependent validation in a helper.
> 
> Recall that in the on-demand region creation flow the end-user picks the
> position of a given endpoint decoder in a region. In the autodiscovery
> case the position of an endpoint decoder can only be determined after
> all other endpoint decoders that claim to decode the region's address
> range have been enumerated and attached. So, in the autodiscovery case
> endpoint decoders may be attached before their relative position is
> known. Once all decoders arrive, then positions can be determined and
> validated with cxl_region_validate_position() the same as user initiated
> on-demand creation.
> 
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>  drivers/cxl/core/region.c |  119 +++++++++++++++++++++++++++++----------------
>  1 file changed, 76 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 97eafdd75675..c82d3b6f3d1f 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -1207,35 +1207,13 @@ static int cxl_region_setup_targets(struct cxl_region *cxlr)
>  	return 0;
>  }
>  

[snip]

> @@ -1274,6 +1252,71 @@ static int cxl_region_attach(struct cxl_region *cxlr,
>  		}
>  	}
>  
> +	return 0;
> +}
> +
> +static int cxl_region_attach_position(struct cxl_region *cxlr,
> +				      struct cxl_root_decoder *cxlrd,
> +				      struct cxl_endpoint_decoder *cxled,
> +				      const struct cxl_dport *dport, int pos)
> +{
> +	struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
> +	struct cxl_port *iter;
> +	int rc;
> +
> +	if (cxlrd->calc_hb(cxlrd, pos) != dport) {
> +		dev_dbg(&cxlr->dev, "%s:%s invalid target position for %s\n",
> +			dev_name(&cxlmd->dev), dev_name(&cxled->cxld.dev),
> +			dev_name(&cxlrd->cxlsd.cxld.dev));
> +		return -ENXIO;
> +	}

I think I know the answer but I'm curious why this check is not part of
validating the position?  Is it because this is validating the position
relative to the hostbridge which is not strictly part of the region
validation?

Ira
Dan Williams Feb. 6, 2023, 7:15 p.m. UTC | #2
Ira Weiny wrote:
> Dan Williams wrote:
> > In preparation for region autodiscovery, that needs all devices
> > discovered before their relative position in the region can be
> > determined, consolidate all position dependent validation in a helper.
> > 
> > Recall that in the on-demand region creation flow the end-user picks the
> > position of a given endpoint decoder in a region. In the autodiscovery
> > case the position of an endpoint decoder can only be determined after
> > all other endpoint decoders that claim to decode the region's address
> > range have been enumerated and attached. So, in the autodiscovery case
> > endpoint decoders may be attached before their relative position is
> > known. Once all decoders arrive, then positions can be determined and
> > validated with cxl_region_validate_position() the same as user initiated
> > on-demand creation.
> > 
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> > ---
> >  drivers/cxl/core/region.c |  119 +++++++++++++++++++++++++++++----------------
> >  1 file changed, 76 insertions(+), 43 deletions(-)
> > 
> > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> > index 97eafdd75675..c82d3b6f3d1f 100644
> > --- a/drivers/cxl/core/region.c
> > +++ b/drivers/cxl/core/region.c
> > @@ -1207,35 +1207,13 @@ static int cxl_region_setup_targets(struct cxl_region *cxlr)
> >  	return 0;
> >  }
> >  
> 
> [snip]
> 
> > @@ -1274,6 +1252,71 @@ static int cxl_region_attach(struct cxl_region *cxlr,
> >  		}
> >  	}
> >  
> > +	return 0;
> > +}
> > +
> > +static int cxl_region_attach_position(struct cxl_region *cxlr,
> > +				      struct cxl_root_decoder *cxlrd,
> > +				      struct cxl_endpoint_decoder *cxled,
> > +				      const struct cxl_dport *dport, int pos)
> > +{
> > +	struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
> > +	struct cxl_port *iter;
> > +	int rc;
> > +
> > +	if (cxlrd->calc_hb(cxlrd, pos) != dport) {
> > +		dev_dbg(&cxlr->dev, "%s:%s invalid target position for %s\n",
> > +			dev_name(&cxlmd->dev), dev_name(&cxled->cxld.dev),
> > +			dev_name(&cxlrd->cxlsd.cxld.dev));
> > +		return -ENXIO;
> > +	}
> 
> I think I know the answer but I'm curious why this check is not part of
> validating the position?  Is it because this is validating the position
> relative to the hostbridge which is not strictly part of the region
> validation?

cxl_region_validate_position() is just doing the basic checks like
preventing assigning 2 decoders to the same position, or assigning a
device twice to the same region.

The checks in cxl_region_attach_position() and cxl_port_attach_region()
are more about whether that device can be in that position relative to
the hardware topology.

I think this split makes more sense in the follow on patch where you see
that cxl_region_attach_position() is done after sorting while
cxl_region_attach_auto() replaces cxl_region_validate_position() since
the latter is validating user input and the former is reacting to what
platform-firmware already successfully programmed.
Jonathan Cameron Feb. 8, 2023, 12:30 p.m. UTC | #3
On Sun, 05 Feb 2023 17:03:07 -0800
Dan Williams <dan.j.williams@intel.com> wrote:

> In preparation for region autodiscovery, that needs all devices
> discovered before their relative position in the region can be
> determined, consolidate all position dependent validation in a helper.
> 
> Recall that in the on-demand region creation flow the end-user picks the
> position of a given endpoint decoder in a region. In the autodiscovery
> case the position of an endpoint decoder can only be determined after
> all other endpoint decoders that claim to decode the region's address
> range have been enumerated and attached. So, in the autodiscovery case
> endpoint decoders may be attached before their relative position is
> known. Once all decoders arrive, then positions can be determined and
> validated with cxl_region_validate_position() the same as user initiated
> on-demand creation.
> 
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
Hi Dan,

A few comments inline, but mostly reflect the original code rather than
the refactoring you have done in this patch.

Jonathan


> +static int cxl_region_attach(struct cxl_region *cxlr,
> +			     struct cxl_endpoint_decoder *cxled, int pos)
> +{
> +	struct cxl_root_decoder *cxlrd = to_cxl_root_decoder(cxlr->dev.parent);
> +	struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
> +	struct cxl_region_params *p = &cxlr->params;
> +	struct cxl_port *ep_port, *root_port;
> +	struct cxl_dport *dport;
> +	int rc = -ENXIO;
> +
> +	if (cxled->mode != cxlr->mode) {
> +		dev_dbg(&cxlr->dev, "%s region mode: %d mismatch: %d\n",
> +			dev_name(&cxled->cxld.dev), cxlr->mode, cxled->mode);
> +		return -EINVAL;
> +	}
> +
> +	if (cxled->mode == CXL_DECODER_DEAD) {
> +		dev_dbg(&cxlr->dev, "%s dead\n", dev_name(&cxled->cxld.dev));
> +		return -ENODEV;
> +	}
> +
> +	/* all full of members, or interleave config not established? */
> +	if (p->state > CXL_CONFIG_INTERLEAVE_ACTIVE) {
> +		dev_dbg(&cxlr->dev, "region already active\n");
> +		return -EBUSY;
> +	} else if (p->state < CXL_CONFIG_INTERLEAVE_ACTIVE) {
> +		dev_dbg(&cxlr->dev, "interleave config missing\n");
> +		return -ENXIO;
> +	}
> +
>  	ep_port = cxled_to_port(cxled);
>  	root_port = cxlrd_to_port(cxlrd);
>  	dport = cxl_find_dport_by_dev(root_port, ep_port->host_bridge);
> @@ -1284,13 +1327,6 @@ static int cxl_region_attach(struct cxl_region *cxlr,
>  		return -ENXIO;
>  	}
>  
> -	if (cxlrd->calc_hb(cxlrd, pos) != dport) {
> -		dev_dbg(&cxlr->dev, "%s:%s invalid target position for %s\n",
> -			dev_name(&cxlmd->dev), dev_name(&cxled->cxld.dev),
> -			dev_name(&cxlrd->cxlsd.cxld.dev));
> -		return -ENXIO;
> -	}
> -

In an ideal world, this would have been nice as two patches.
One that reorders the various checks so that they are in the order
after you have factored things out (easy to review for correctness)
then one that factored it out.

>  	if (cxled->cxld.target_type != cxlr->type) {
>  		dev_dbg(&cxlr->dev, "%s:%s type mismatch: %d vs %d\n",
>  			dev_name(&cxlmd->dev), dev_name(&cxled->cxld.dev),
> @@ -1314,12 +1350,13 @@ static int cxl_region_attach(struct cxl_region *cxlr,
>  		return -EINVAL;
>  	}
>  
> -	for (iter = ep_port; !is_cxl_root(iter);
> -	     iter = to_cxl_port(iter->dev.parent)) {
> -		rc = cxl_port_attach_region(iter, cxlr, cxled, pos);
> -		if (rc)
> -			goto err;
> -	}
> +	rc = cxl_region_validate_position(cxlr, cxled, pos);
> +	if (rc)
> +		return rc;
> +
> +	rc = cxl_region_attach_position(cxlr, cxlrd, cxled, dport, pos);
> +	if (rc)
> +		return rc;
>  
>  	p->targets[pos] = cxled;
>  	cxled->pos = pos;

More something about original code than the refactoring...

I'm not keen on the side effects that aren't unwound in the error paths.

p->targets[pos] and cxled->pos are left set.  Probably never matters
but not elegant or as easy to reason about as it would be if they
were cleared in error cases.  In particular there is a check on
whether p->targets[pos] is set that will result in a dev_dbg even
though setting it up actually failed.


> @@ -1343,10 +1380,6 @@ static int cxl_region_attach(struct cxl_region *cxlr,
>  
>  err_decrement:
>  	p->nr_targets--;
> -err:
> -	for (iter = ep_port; !is_cxl_root(iter);
> -	     iter = to_cxl_port(iter->dev.parent))
> -		cxl_port_detach_region(iter, cxlr, cxled);
>  	return rc;
>  }
>  
>
Dan Williams Feb. 9, 2023, 4:09 a.m. UTC | #4
Jonathan Cameron wrote:
> On Sun, 05 Feb 2023 17:03:07 -0800
> Dan Williams <dan.j.williams@intel.com> wrote:
> 
> > In preparation for region autodiscovery, that needs all devices
> > discovered before their relative position in the region can be
> > determined, consolidate all position dependent validation in a helper.
> > 
> > Recall that in the on-demand region creation flow the end-user picks the
> > position of a given endpoint decoder in a region. In the autodiscovery
> > case the position of an endpoint decoder can only be determined after
> > all other endpoint decoders that claim to decode the region's address
> > range have been enumerated and attached. So, in the autodiscovery case
> > endpoint decoders may be attached before their relative position is
> > known. Once all decoders arrive, then positions can be determined and
> > validated with cxl_region_validate_position() the same as user initiated
> > on-demand creation.
> > 
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> Hi Dan,
> 
> A few comments inline, but mostly reflect the original code rather than
> the refactoring you have done in this patch.
> 
> Jonathan
> 
> 
> > +static int cxl_region_attach(struct cxl_region *cxlr,
> > +			     struct cxl_endpoint_decoder *cxled, int pos)
> > +{
> > +	struct cxl_root_decoder *cxlrd = to_cxl_root_decoder(cxlr->dev.parent);
> > +	struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
> > +	struct cxl_region_params *p = &cxlr->params;
> > +	struct cxl_port *ep_port, *root_port;
> > +	struct cxl_dport *dport;
> > +	int rc = -ENXIO;
> > +
> > +	if (cxled->mode != cxlr->mode) {
> > +		dev_dbg(&cxlr->dev, "%s region mode: %d mismatch: %d\n",
> > +			dev_name(&cxled->cxld.dev), cxlr->mode, cxled->mode);
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (cxled->mode == CXL_DECODER_DEAD) {
> > +		dev_dbg(&cxlr->dev, "%s dead\n", dev_name(&cxled->cxld.dev));
> > +		return -ENODEV;
> > +	}
> > +
> > +	/* all full of members, or interleave config not established? */
> > +	if (p->state > CXL_CONFIG_INTERLEAVE_ACTIVE) {
> > +		dev_dbg(&cxlr->dev, "region already active\n");
> > +		return -EBUSY;
> > +	} else if (p->state < CXL_CONFIG_INTERLEAVE_ACTIVE) {
> > +		dev_dbg(&cxlr->dev, "interleave config missing\n");
> > +		return -ENXIO;
> > +	}
> > +
> >  	ep_port = cxled_to_port(cxled);
> >  	root_port = cxlrd_to_port(cxlrd);
> >  	dport = cxl_find_dport_by_dev(root_port, ep_port->host_bridge);
> > @@ -1284,13 +1327,6 @@ static int cxl_region_attach(struct cxl_region *cxlr,
> >  		return -ENXIO;
> >  	}
> >  
> > -	if (cxlrd->calc_hb(cxlrd, pos) != dport) {
> > -		dev_dbg(&cxlr->dev, "%s:%s invalid target position for %s\n",
> > -			dev_name(&cxlmd->dev), dev_name(&cxled->cxld.dev),
> > -			dev_name(&cxlrd->cxlsd.cxld.dev));
> > -		return -ENXIO;
> > -	}
> > -
> 
> In an ideal world, this would have been nice as two patches.
> One that reorders the various checks so that they are in the order
> after you have factored things out (easy to review for correctness)
> then one that factored it out.
> 
> >  	if (cxled->cxld.target_type != cxlr->type) {
> >  		dev_dbg(&cxlr->dev, "%s:%s type mismatch: %d vs %d\n",
> >  			dev_name(&cxlmd->dev), dev_name(&cxled->cxld.dev),
> > @@ -1314,12 +1350,13 @@ static int cxl_region_attach(struct cxl_region *cxlr,
> >  		return -EINVAL;
> >  	}
> >  
> > -	for (iter = ep_port; !is_cxl_root(iter);
> > -	     iter = to_cxl_port(iter->dev.parent)) {
> > -		rc = cxl_port_attach_region(iter, cxlr, cxled, pos);
> > -		if (rc)
> > -			goto err;
> > -	}
> > +	rc = cxl_region_validate_position(cxlr, cxled, pos);
> > +	if (rc)
> > +		return rc;
> > +
> > +	rc = cxl_region_attach_position(cxlr, cxlrd, cxled, dport, pos);
> > +	if (rc)
> > +		return rc;
> >  
> >  	p->targets[pos] = cxled;
> >  	cxled->pos = pos;
> 
> More something about original code than the refactoring...
> 
> I'm not keen on the side effects that aren't unwound in the error paths.
> 
> p->targets[pos] and cxled->pos are left set.  Probably never matters
> but not elegant or as easy to reason about as it would be if they
> were cleared in error cases.  In particular there is a check on
> whether p->targets[pos] is set that will result in a dev_dbg even
> though setting it up actually failed.

I'll clean that up with a lead-in fixup.
Dan Williams Feb. 9, 2023, 4:26 a.m. UTC | #5
Jonathan Cameron wrote:
[..]
> > @@ -1284,13 +1327,6 @@ static int cxl_region_attach(struct cxl_region *cxlr,
> >  		return -ENXIO;
> >  	}
> >  
> > -	if (cxlrd->calc_hb(cxlrd, pos) != dport) {
> > -		dev_dbg(&cxlr->dev, "%s:%s invalid target position for %s\n",
> > -			dev_name(&cxlmd->dev), dev_name(&cxled->cxld.dev),
> > -			dev_name(&cxlrd->cxlsd.cxld.dev));
> > -		return -ENXIO;
> > -	}
> > -
> 
> In an ideal world, this would have been nice as two patches.
> One that reorders the various checks so that they are in the order
> after you have factored things out (easy to review for correctness)
> then one that factored it out.

I played with this a bit and the only way I could see to make the diff
come out significantly nicer would be to use a forward declaration to
move the new helpers below cxl_region_attach().
Jonathan Cameron Feb. 9, 2023, 11:07 a.m. UTC | #6
On Wed, 8 Feb 2023 20:26:26 -0800
Dan Williams <dan.j.williams@intel.com> wrote:

> Jonathan Cameron wrote:
> [..]
> > > @@ -1284,13 +1327,6 @@ static int cxl_region_attach(struct cxl_region *cxlr,
> > >  		return -ENXIO;
> > >  	}
> > >  
> > > -	if (cxlrd->calc_hb(cxlrd, pos) != dport) {
> > > -		dev_dbg(&cxlr->dev, "%s:%s invalid target position for %s\n",
> > > -			dev_name(&cxlmd->dev), dev_name(&cxled->cxld.dev),
> > > -			dev_name(&cxlrd->cxlsd.cxld.dev));
> > > -		return -ENXIO;
> > > -	}
> > > -  
> > 
> > In an ideal world, this would have been nice as two patches.
> > One that reorders the various checks so that they are in the order
> > after you have factored things out (easy to review for correctness)
> > then one that factored it out.  
> 
> I played with this a bit and the only way I could see to make the diff
> come out significantly nicer would be to use a forward declaration to
> move the new helpers below cxl_region_attach().

Don't bother then!  Unless you've already done it.

In the ideal world diff would magically present everything in the most
human readable form :)  What are all these AI folk working on that we
don't already have this!

Jonathan
Verma, Vishal L Feb. 9, 2023, 7:45 p.m. UTC | #7
On Sun, 2023-02-05 at 17:03 -0800, Dan Williams wrote:
> In preparation for region autodiscovery, that needs all devices
> discovered before their relative position in the region can be
> determined, consolidate all position dependent validation in a helper.
> 
> Recall that in the on-demand region creation flow the end-user picks the
> position of a given endpoint decoder in a region. In the autodiscovery
> case the position of an endpoint decoder can only be determined after
> all other endpoint decoders that claim to decode the region's address
> range have been enumerated and attached. So, in the autodiscovery case
> endpoint decoders may be attached before their relative position is
> known. Once all decoders arrive, then positions can be determined and
> validated with cxl_region_validate_position() the same as user initiated
> on-demand creation.
> 
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>  drivers/cxl/core/region.c |  119 +++++++++++++++++++++++++++++----------------
>  1 file changed, 76 insertions(+), 43 deletions(-)

Looks good,

Reviewed-by: Vishal Verma <vishal.l.verma@intel.com>

> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 97eafdd75675..c82d3b6f3d1f 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -1207,35 +1207,13 @@ static int cxl_region_setup_targets(struct cxl_region *cxlr)
>         return 0;
>  }
>  
> -static int cxl_region_attach(struct cxl_region *cxlr,
> -                            struct cxl_endpoint_decoder *cxled, int pos)
> +static int cxl_region_validate_position(struct cxl_region *cxlr,
> +                                       struct cxl_endpoint_decoder *cxled,
> +                                       int pos)
>  {
> -       struct cxl_root_decoder *cxlrd = to_cxl_root_decoder(cxlr->dev.parent);
>         struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
> -       struct cxl_port *ep_port, *root_port, *iter;
>         struct cxl_region_params *p = &cxlr->params;
> -       struct cxl_dport *dport;
> -       int i, rc = -ENXIO;
> -
> -       if (cxled->mode != cxlr->mode) {
> -               dev_dbg(&cxlr->dev, "%s region mode: %d mismatch: %d\n",
> -                       dev_name(&cxled->cxld.dev), cxlr->mode, cxled->mode);
> -               return -EINVAL;
> -       }
> -
> -       if (cxled->mode == CXL_DECODER_DEAD) {
> -               dev_dbg(&cxlr->dev, "%s dead\n", dev_name(&cxled->cxld.dev));
> -               return -ENODEV;
> -       }
> -
> -       /* all full of members, or interleave config not established? */
> -       if (p->state > CXL_CONFIG_INTERLEAVE_ACTIVE) {
> -               dev_dbg(&cxlr->dev, "region already active\n");
> -               return -EBUSY;
> -       } else if (p->state < CXL_CONFIG_INTERLEAVE_ACTIVE) {
> -               dev_dbg(&cxlr->dev, "interleave config missing\n");
> -               return -ENXIO;
> -       }
> +       int i;
>  
>         if (pos < 0 || pos >= p->interleave_ways) {
>                 dev_dbg(&cxlr->dev, "position %d out of range %d\n", pos,
> @@ -1274,6 +1252,71 @@ static int cxl_region_attach(struct cxl_region *cxlr,
>                 }
>         }
>  
> +       return 0;
> +}
> +
> +static int cxl_region_attach_position(struct cxl_region *cxlr,
> +                                     struct cxl_root_decoder *cxlrd,
> +                                     struct cxl_endpoint_decoder *cxled,
> +                                     const struct cxl_dport *dport, int pos)
> +{
> +       struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
> +       struct cxl_port *iter;
> +       int rc;
> +
> +       if (cxlrd->calc_hb(cxlrd, pos) != dport) {
> +               dev_dbg(&cxlr->dev, "%s:%s invalid target position for %s\n",
> +                       dev_name(&cxlmd->dev), dev_name(&cxled->cxld.dev),
> +                       dev_name(&cxlrd->cxlsd.cxld.dev));
> +               return -ENXIO;
> +       }
> +
> +       for (iter = cxled_to_port(cxled); !is_cxl_root(iter);
> +            iter = to_cxl_port(iter->dev.parent)) {
> +               rc = cxl_port_attach_region(iter, cxlr, cxled, pos);
> +               if (rc)
> +                       goto err;
> +       }
> +
> +       return 0;
> +
> +err:
> +       for (iter = cxled_to_port(cxled); !is_cxl_root(iter);
> +            iter = to_cxl_port(iter->dev.parent))
> +               cxl_port_detach_region(iter, cxlr, cxled);
> +       return rc;
> +}
> +
> +static int cxl_region_attach(struct cxl_region *cxlr,
> +                            struct cxl_endpoint_decoder *cxled, int pos)
> +{
> +       struct cxl_root_decoder *cxlrd = to_cxl_root_decoder(cxlr->dev.parent);
> +       struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
> +       struct cxl_region_params *p = &cxlr->params;
> +       struct cxl_port *ep_port, *root_port;
> +       struct cxl_dport *dport;
> +       int rc = -ENXIO;
> +
> +       if (cxled->mode != cxlr->mode) {
> +               dev_dbg(&cxlr->dev, "%s region mode: %d mismatch: %d\n",
> +                       dev_name(&cxled->cxld.dev), cxlr->mode, cxled->mode);
> +               return -EINVAL;
> +       }
> +
> +       if (cxled->mode == CXL_DECODER_DEAD) {
> +               dev_dbg(&cxlr->dev, "%s dead\n", dev_name(&cxled->cxld.dev));
> +               return -ENODEV;
> +       }
> +
> +       /* all full of members, or interleave config not established? */
> +       if (p->state > CXL_CONFIG_INTERLEAVE_ACTIVE) {
> +               dev_dbg(&cxlr->dev, "region already active\n");
> +               return -EBUSY;
> +       } else if (p->state < CXL_CONFIG_INTERLEAVE_ACTIVE) {
> +               dev_dbg(&cxlr->dev, "interleave config missing\n");
> +               return -ENXIO;
> +       }
> +
>         ep_port = cxled_to_port(cxled);
>         root_port = cxlrd_to_port(cxlrd);
>         dport = cxl_find_dport_by_dev(root_port, ep_port->host_bridge);
> @@ -1284,13 +1327,6 @@ static int cxl_region_attach(struct cxl_region *cxlr,
>                 return -ENXIO;
>         }
>  
> -       if (cxlrd->calc_hb(cxlrd, pos) != dport) {
> -               dev_dbg(&cxlr->dev, "%s:%s invalid target position for %s\n",
> -                       dev_name(&cxlmd->dev), dev_name(&cxled->cxld.dev),
> -                       dev_name(&cxlrd->cxlsd.cxld.dev));
> -               return -ENXIO;
> -       }
> -
>         if (cxled->cxld.target_type != cxlr->type) {
>                 dev_dbg(&cxlr->dev, "%s:%s type mismatch: %d vs %d\n",
>                         dev_name(&cxlmd->dev), dev_name(&cxled->cxld.dev),
> @@ -1314,12 +1350,13 @@ static int cxl_region_attach(struct cxl_region *cxlr,
>                 return -EINVAL;
>         }
>  
> -       for (iter = ep_port; !is_cxl_root(iter);
> -            iter = to_cxl_port(iter->dev.parent)) {
> -               rc = cxl_port_attach_region(iter, cxlr, cxled, pos);
> -               if (rc)
> -                       goto err;
> -       }
> +       rc = cxl_region_validate_position(cxlr, cxled, pos);
> +       if (rc)
> +               return rc;
> +
> +       rc = cxl_region_attach_position(cxlr, cxlrd, cxled, dport, pos);
> +       if (rc)
> +               return rc;
>  
>         p->targets[pos] = cxled;
>         cxled->pos = pos;
> @@ -1343,10 +1380,6 @@ static int cxl_region_attach(struct cxl_region *cxlr,
>  
>  err_decrement:
>         p->nr_targets--;
> -err:
> -       for (iter = ep_port; !is_cxl_root(iter);
> -            iter = to_cxl_port(iter->dev.parent))
> -               cxl_port_detach_region(iter, cxlr, cxled);
>         return rc;
>  }
>  
>
Dan Williams Feb. 9, 2023, 8:52 p.m. UTC | #8
Jonathan Cameron wrote:
> On Wed, 8 Feb 2023 20:26:26 -0800
> Dan Williams <dan.j.williams@intel.com> wrote:
> 
> > Jonathan Cameron wrote:
> > [..]
> > > > @@ -1284,13 +1327,6 @@ static int cxl_region_attach(struct cxl_region *cxlr,
> > > >  		return -ENXIO;
> > > >  	}
> > > >  
> > > > -	if (cxlrd->calc_hb(cxlrd, pos) != dport) {
> > > > -		dev_dbg(&cxlr->dev, "%s:%s invalid target position for %s\n",
> > > > -			dev_name(&cxlmd->dev), dev_name(&cxled->cxld.dev),
> > > > -			dev_name(&cxlrd->cxlsd.cxld.dev));
> > > > -		return -ENXIO;
> > > > -	}
> > > > -  
> > > 
> > > In an ideal world, this would have been nice as two patches.
> > > One that reorders the various checks so that they are in the order
> > > after you have factored things out (easy to review for correctness)
> > > then one that factored it out.  
> > 
> > I played with this a bit and the only way I could see to make the diff
> > come out significantly nicer would be to use a forward declaration to
> > move the new helpers below cxl_region_attach().
> 
> Don't bother then!  Unless you've already done it.

No worries, abandoned.

> In the ideal world diff would magically present everything in the most
> human readable form :)  What are all these AI folk working on that we
> don't already have this!

+1 to this.
diff mbox series

Patch

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 97eafdd75675..c82d3b6f3d1f 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -1207,35 +1207,13 @@  static int cxl_region_setup_targets(struct cxl_region *cxlr)
 	return 0;
 }
 
-static int cxl_region_attach(struct cxl_region *cxlr,
-			     struct cxl_endpoint_decoder *cxled, int pos)
+static int cxl_region_validate_position(struct cxl_region *cxlr,
+					struct cxl_endpoint_decoder *cxled,
+					int pos)
 {
-	struct cxl_root_decoder *cxlrd = to_cxl_root_decoder(cxlr->dev.parent);
 	struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
-	struct cxl_port *ep_port, *root_port, *iter;
 	struct cxl_region_params *p = &cxlr->params;
-	struct cxl_dport *dport;
-	int i, rc = -ENXIO;
-
-	if (cxled->mode != cxlr->mode) {
-		dev_dbg(&cxlr->dev, "%s region mode: %d mismatch: %d\n",
-			dev_name(&cxled->cxld.dev), cxlr->mode, cxled->mode);
-		return -EINVAL;
-	}
-
-	if (cxled->mode == CXL_DECODER_DEAD) {
-		dev_dbg(&cxlr->dev, "%s dead\n", dev_name(&cxled->cxld.dev));
-		return -ENODEV;
-	}
-
-	/* all full of members, or interleave config not established? */
-	if (p->state > CXL_CONFIG_INTERLEAVE_ACTIVE) {
-		dev_dbg(&cxlr->dev, "region already active\n");
-		return -EBUSY;
-	} else if (p->state < CXL_CONFIG_INTERLEAVE_ACTIVE) {
-		dev_dbg(&cxlr->dev, "interleave config missing\n");
-		return -ENXIO;
-	}
+	int i;
 
 	if (pos < 0 || pos >= p->interleave_ways) {
 		dev_dbg(&cxlr->dev, "position %d out of range %d\n", pos,
@@ -1274,6 +1252,71 @@  static int cxl_region_attach(struct cxl_region *cxlr,
 		}
 	}
 
+	return 0;
+}
+
+static int cxl_region_attach_position(struct cxl_region *cxlr,
+				      struct cxl_root_decoder *cxlrd,
+				      struct cxl_endpoint_decoder *cxled,
+				      const struct cxl_dport *dport, int pos)
+{
+	struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
+	struct cxl_port *iter;
+	int rc;
+
+	if (cxlrd->calc_hb(cxlrd, pos) != dport) {
+		dev_dbg(&cxlr->dev, "%s:%s invalid target position for %s\n",
+			dev_name(&cxlmd->dev), dev_name(&cxled->cxld.dev),
+			dev_name(&cxlrd->cxlsd.cxld.dev));
+		return -ENXIO;
+	}
+
+	for (iter = cxled_to_port(cxled); !is_cxl_root(iter);
+	     iter = to_cxl_port(iter->dev.parent)) {
+		rc = cxl_port_attach_region(iter, cxlr, cxled, pos);
+		if (rc)
+			goto err;
+	}
+
+	return 0;
+
+err:
+	for (iter = cxled_to_port(cxled); !is_cxl_root(iter);
+	     iter = to_cxl_port(iter->dev.parent))
+		cxl_port_detach_region(iter, cxlr, cxled);
+	return rc;
+}
+
+static int cxl_region_attach(struct cxl_region *cxlr,
+			     struct cxl_endpoint_decoder *cxled, int pos)
+{
+	struct cxl_root_decoder *cxlrd = to_cxl_root_decoder(cxlr->dev.parent);
+	struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
+	struct cxl_region_params *p = &cxlr->params;
+	struct cxl_port *ep_port, *root_port;
+	struct cxl_dport *dport;
+	int rc = -ENXIO;
+
+	if (cxled->mode != cxlr->mode) {
+		dev_dbg(&cxlr->dev, "%s region mode: %d mismatch: %d\n",
+			dev_name(&cxled->cxld.dev), cxlr->mode, cxled->mode);
+		return -EINVAL;
+	}
+
+	if (cxled->mode == CXL_DECODER_DEAD) {
+		dev_dbg(&cxlr->dev, "%s dead\n", dev_name(&cxled->cxld.dev));
+		return -ENODEV;
+	}
+
+	/* all full of members, or interleave config not established? */
+	if (p->state > CXL_CONFIG_INTERLEAVE_ACTIVE) {
+		dev_dbg(&cxlr->dev, "region already active\n");
+		return -EBUSY;
+	} else if (p->state < CXL_CONFIG_INTERLEAVE_ACTIVE) {
+		dev_dbg(&cxlr->dev, "interleave config missing\n");
+		return -ENXIO;
+	}
+
 	ep_port = cxled_to_port(cxled);
 	root_port = cxlrd_to_port(cxlrd);
 	dport = cxl_find_dport_by_dev(root_port, ep_port->host_bridge);
@@ -1284,13 +1327,6 @@  static int cxl_region_attach(struct cxl_region *cxlr,
 		return -ENXIO;
 	}
 
-	if (cxlrd->calc_hb(cxlrd, pos) != dport) {
-		dev_dbg(&cxlr->dev, "%s:%s invalid target position for %s\n",
-			dev_name(&cxlmd->dev), dev_name(&cxled->cxld.dev),
-			dev_name(&cxlrd->cxlsd.cxld.dev));
-		return -ENXIO;
-	}
-
 	if (cxled->cxld.target_type != cxlr->type) {
 		dev_dbg(&cxlr->dev, "%s:%s type mismatch: %d vs %d\n",
 			dev_name(&cxlmd->dev), dev_name(&cxled->cxld.dev),
@@ -1314,12 +1350,13 @@  static int cxl_region_attach(struct cxl_region *cxlr,
 		return -EINVAL;
 	}
 
-	for (iter = ep_port; !is_cxl_root(iter);
-	     iter = to_cxl_port(iter->dev.parent)) {
-		rc = cxl_port_attach_region(iter, cxlr, cxled, pos);
-		if (rc)
-			goto err;
-	}
+	rc = cxl_region_validate_position(cxlr, cxled, pos);
+	if (rc)
+		return rc;
+
+	rc = cxl_region_attach_position(cxlr, cxlrd, cxled, dport, pos);
+	if (rc)
+		return rc;
 
 	p->targets[pos] = cxled;
 	cxled->pos = pos;
@@ -1343,10 +1380,6 @@  static int cxl_region_attach(struct cxl_region *cxlr,
 
 err_decrement:
 	p->nr_targets--;
-err:
-	for (iter = ep_port; !is_cxl_root(iter);
-	     iter = to_cxl_port(iter->dev.parent))
-		cxl_port_detach_region(iter, cxlr, cxled);
 	return rc;
 }