diff mbox series

[13/23] cxl/core: Move target population locking to caller

Message ID 20211120000250.1663391-14-ben.widawsky@intel.com
State Superseded
Headers show
Series Add drivers for CXL ports and mem devices | expand

Commit Message

Ben Widawsky Nov. 20, 2021, 12:02 a.m. UTC
In preparation for a port driver that enumerates a descendant port +
decoder hierarchy, arrange for an unlocked version of cxl_decoder_add().
Otherwise a port-driver that adds a child decoder will deadlock on the
device_lock() in ->probe().

Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>

---

Changes since RFCv2:
- Reword commit message (Dan)
- Move decoder API changes into this patch (Dan)
---
 drivers/cxl/core/bus.c | 59 +++++++++++++++++++++++++++++++-----------
 drivers/cxl/cxl.h      |  1 +
 2 files changed, 45 insertions(+), 15 deletions(-)

Comments

Jonathan Cameron Nov. 22, 2021, 4:33 p.m. UTC | #1
On Fri, 19 Nov 2021 16:02:40 -0800
Ben Widawsky <ben.widawsky@intel.com> wrote:

> In preparation for a port driver that enumerates a descendant port +
> decoder hierarchy, arrange for an unlocked version of cxl_decoder_add().
> Otherwise a port-driver that adds a child decoder will deadlock on the
> device_lock() in ->probe().
> 

I think this description should call out that the lock was originally taken
for a much shorter time in decoder_populate_targets() but is moved
up one layer.

One other query inline.  Seems like we the WARN_ON stuff is a bit
over paranoid given what's visible in this patch.  If there is a
good reason for that, then add something to the patch description to
justify it.
 
> Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
> 
> ---
> 
> Changes since RFCv2:
> - Reword commit message (Dan)
> - Move decoder API changes into this patch (Dan)
> ---
>  drivers/cxl/core/bus.c | 59 +++++++++++++++++++++++++++++++-----------
>  drivers/cxl/cxl.h      |  1 +
>  2 files changed, 45 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/cxl/core/bus.c b/drivers/cxl/core/bus.c
> index 16b15f54fb62..cd6fe7823c69 100644
> --- a/drivers/cxl/core/bus.c
> +++ b/drivers/cxl/core/bus.c
> @@ -487,28 +487,22 @@ static int decoder_populate_targets(struct cxl_decoder *cxld,
>  {
>  	int rc = 0, i;
>  
> +	device_lock_assert(&port->dev);
> +
>  	if (!target_map)
>  		return 0;
>  
> -	device_lock(&port->dev);
> -	if (list_empty(&port->dports)) {
> -		rc = -EINVAL;
> -		goto out_unlock;
> -	}
> +	if (list_empty(&port->dports))
> +		return -EINVAL;
>  
>  	for (i = 0; i < cxld->nr_targets; i++) {
>  		struct cxl_dport *dport = find_dport(port, target_map[i]);
>  
> -		if (!dport) {
> -			rc = -ENXIO;
> -			goto out_unlock;
> -		}
> +		if (!dport)
> +			return -ENXIO;
>  		cxld->target[i] = dport;
>  	}
>  
> -out_unlock:
> -	device_unlock(&port->dev);
> -
>  	return rc;
>  }
>  
> @@ -571,7 +565,7 @@ struct cxl_decoder *cxl_decoder_alloc(struct cxl_port *port,
>  EXPORT_SYMBOL_NS_GPL(cxl_decoder_alloc, CXL);
>  
>  /**
> - * cxl_decoder_add - Add a decoder with targets
> + * cxl_decoder_add_locked - Add a decoder with targets
>   * @cxld: The cxl decoder allocated by cxl_decoder_alloc()
>   * @target_map: A list of downstream ports that this decoder can direct memory
>   *              traffic to. These numbers should correspond with the port number
> @@ -581,12 +575,14 @@ EXPORT_SYMBOL_NS_GPL(cxl_decoder_alloc, CXL);
>   * is an endpoint device. A more awkward example is a hostbridge whose root
>   * ports get hot added (technically possible, though unlikely).
>   *
> - * Context: Process context. Takes and releases the cxld's device lock.
> + * This is the locked variant of cxl_decoder_add().
> + *
> + * Context: Process context. Expects the cxld's device lock to be held.
>   *
>   * Return: Negative error code if the decoder wasn't properly configured; else
>   *	   returns 0.
>   */
> -int cxl_decoder_add(struct cxl_decoder *cxld, int *target_map)
> +int cxl_decoder_add_locked(struct cxl_decoder *cxld, int *target_map)
>  {
>  	struct cxl_port *port;
>  	struct device *dev;
> @@ -619,6 +615,39 @@ int cxl_decoder_add(struct cxl_decoder *cxld, int *target_map)
>  
>  	return device_add(dev);
>  }
> +EXPORT_SYMBOL_NS_GPL(cxl_decoder_add_locked, CXL);
> +
> +/**
> + * cxl_decoder_add - Add a decoder with targets
> + * @cxld: The cxl decoder allocated by cxl_decoder_alloc()
> + * @target_map: A list of downstream ports that this decoder can direct memory
> + *              traffic to. These numbers should correspond with the port number
> + *              in the PCIe Link Capabilities structure.
> + *
> + * This is the unlocked variant of cxl_decoder_add_locked().
> + * See cxl_decoder_add_locked().
> + *
> + * Context: Process context. Takes and releases the cxld's device lock.
> + */
> +int cxl_decoder_add(struct cxl_decoder *cxld, int *target_map)
> +{
> +	struct cxl_port *port;
> +	int rc;
> +
> +	if (WARN_ON_ONCE(!cxld))
> +		return -EINVAL;

Why do we now need these protections but didn't before?


> +
> +	if (WARN_ON_ONCE(IS_ERR(cxld)))
> +		return PTR_ERR(cxld);
> +
> +	port = to_cxl_port(cxld->dev.parent);
> +
> +	device_lock(&port->dev);
> +	rc = cxl_decoder_add_locked(cxld, target_map);
> +	device_unlock(&port->dev);
> +
> +	return rc;
> +}
>  EXPORT_SYMBOL_NS_GPL(cxl_decoder_add, CXL);
>  
>  static void cxld_unregister(void *dev)
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index b66ed8f241c6..2c5627fa8a34 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -290,6 +290,7 @@ struct cxl_decoder *to_cxl_decoder(struct device *dev);
>  bool is_root_decoder(struct device *dev);
>  struct cxl_decoder *cxl_decoder_alloc(struct cxl_port *port,
>  				      unsigned int nr_targets);
> +int cxl_decoder_add_locked(struct cxl_decoder *cxld, int *target_map);
>  int cxl_decoder_add(struct cxl_decoder *cxld, int *target_map);
>  int cxl_decoder_autoremove(struct device *host, struct cxl_decoder *cxld);
>
Ben Widawsky Nov. 22, 2021, 9:58 p.m. UTC | #2
On 21-11-22 16:33:02, Jonathan Cameron wrote:
> On Fri, 19 Nov 2021 16:02:40 -0800
> Ben Widawsky <ben.widawsky@intel.com> wrote:
> 
> > In preparation for a port driver that enumerates a descendant port +
> > decoder hierarchy, arrange for an unlocked version of cxl_decoder_add().
> > Otherwise a port-driver that adds a child decoder will deadlock on the
> > device_lock() in ->probe().
> > 
> 
> I think this description should call out that the lock was originally taken
> for a much shorter time in decoder_populate_targets() but is moved
> up one layer.

Sounds good.

> 
> One other query inline.  Seems like we the WARN_ON stuff is a bit
> over paranoid given what's visible in this patch.  If there is a
> good reason for that, then add something to the patch description to
> justify it.
>  
> > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
> > 
> > ---
> > 
> > Changes since RFCv2:
> > - Reword commit message (Dan)
> > - Move decoder API changes into this patch (Dan)
> > ---
> >  drivers/cxl/core/bus.c | 59 +++++++++++++++++++++++++++++++-----------
> >  drivers/cxl/cxl.h      |  1 +
> >  2 files changed, 45 insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/cxl/core/bus.c b/drivers/cxl/core/bus.c
> > index 16b15f54fb62..cd6fe7823c69 100644
> > --- a/drivers/cxl/core/bus.c
> > +++ b/drivers/cxl/core/bus.c
> > @@ -487,28 +487,22 @@ static int decoder_populate_targets(struct cxl_decoder *cxld,
> >  {
> >  	int rc = 0, i;
> >  
> > +	device_lock_assert(&port->dev);
> > +
> >  	if (!target_map)
> >  		return 0;
> >  
> > -	device_lock(&port->dev);
> > -	if (list_empty(&port->dports)) {
> > -		rc = -EINVAL;
> > -		goto out_unlock;
> > -	}
> > +	if (list_empty(&port->dports))
> > +		return -EINVAL;
> >  
> >  	for (i = 0; i < cxld->nr_targets; i++) {
> >  		struct cxl_dport *dport = find_dport(port, target_map[i]);
> >  
> > -		if (!dport) {
> > -			rc = -ENXIO;
> > -			goto out_unlock;
> > -		}
> > +		if (!dport)
> > +			return -ENXIO;
> >  		cxld->target[i] = dport;
> >  	}
> >  
> > -out_unlock:
> > -	device_unlock(&port->dev);
> > -
> >  	return rc;
> >  }
> >  
> > @@ -571,7 +565,7 @@ struct cxl_decoder *cxl_decoder_alloc(struct cxl_port *port,
> >  EXPORT_SYMBOL_NS_GPL(cxl_decoder_alloc, CXL);
> >  
> >  /**
> > - * cxl_decoder_add - Add a decoder with targets
> > + * cxl_decoder_add_locked - Add a decoder with targets
> >   * @cxld: The cxl decoder allocated by cxl_decoder_alloc()
> >   * @target_map: A list of downstream ports that this decoder can direct memory
> >   *              traffic to. These numbers should correspond with the port number
> > @@ -581,12 +575,14 @@ EXPORT_SYMBOL_NS_GPL(cxl_decoder_alloc, CXL);
> >   * is an endpoint device. A more awkward example is a hostbridge whose root
> >   * ports get hot added (technically possible, though unlikely).
> >   *
> > - * Context: Process context. Takes and releases the cxld's device lock.
> > + * This is the locked variant of cxl_decoder_add().
> > + *
> > + * Context: Process context. Expects the cxld's device lock to be held.
> >   *
> >   * Return: Negative error code if the decoder wasn't properly configured; else
> >   *	   returns 0.
> >   */
> > -int cxl_decoder_add(struct cxl_decoder *cxld, int *target_map)
> > +int cxl_decoder_add_locked(struct cxl_decoder *cxld, int *target_map)
> >  {
> >  	struct cxl_port *port;
> >  	struct device *dev;
> > @@ -619,6 +615,39 @@ int cxl_decoder_add(struct cxl_decoder *cxld, int *target_map)
> >  
> >  	return device_add(dev);
> >  }
> > +EXPORT_SYMBOL_NS_GPL(cxl_decoder_add_locked, CXL);
> > +
> > +/**
> > + * cxl_decoder_add - Add a decoder with targets
> > + * @cxld: The cxl decoder allocated by cxl_decoder_alloc()
> > + * @target_map: A list of downstream ports that this decoder can direct memory
> > + *              traffic to. These numbers should correspond with the port number
> > + *              in the PCIe Link Capabilities structure.
> > + *
> > + * This is the unlocked variant of cxl_decoder_add_locked().
> > + * See cxl_decoder_add_locked().
> > + *
> > + * Context: Process context. Takes and releases the cxld's device lock.
> > + */
> > +int cxl_decoder_add(struct cxl_decoder *cxld, int *target_map)
> > +{
> > +	struct cxl_port *port;
> > +	int rc;
> > +
> > +	if (WARN_ON_ONCE(!cxld))
> > +		return -EINVAL;
> 
> Why do we now need these protections but didn't before?

I don't quite understand what you're trying to point out.

Prior to this patch, cxl_decoder_add() checks:
- !cxld
- IS_ERR(cxld)
- cxld->interleave_ways != 0

After this patch, cxl_decoder_add() checks:
- !cxld
- IS_ERR(cxld)
- (and then calls cxl_decoder_add_locked())

And cxl_decoder_add_locked() checks:
- !cxld
- IS_ERR(cxld)
- cxld->interleave_ways != 0

Ultimately we want to check all 3, and since cxl_decoder_add() calls
cxl_decoder_add_locked(), we're good there. The problem is to get from a cxld to
a port, you need to make sure you have a valid cxld, and the API previously
allowed !cxld and IS_ERR(cxld). So there are duplicative checks if you call
cxl_decoder_add(), but other than that I don't see any new protections.

> 
> 
> > +
> > +	if (WARN_ON_ONCE(IS_ERR(cxld)))
> > +		return PTR_ERR(cxld);
> > +
> > +	port = to_cxl_port(cxld->dev.parent);
> > +
> > +	device_lock(&port->dev);
> > +	rc = cxl_decoder_add_locked(cxld, target_map);
> > +	device_unlock(&port->dev);
> > +
> > +	return rc;
> > +}
> >  EXPORT_SYMBOL_NS_GPL(cxl_decoder_add, CXL);
> >  
> >  static void cxld_unregister(void *dev)
> > diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> > index b66ed8f241c6..2c5627fa8a34 100644
> > --- a/drivers/cxl/cxl.h
> > +++ b/drivers/cxl/cxl.h
> > @@ -290,6 +290,7 @@ struct cxl_decoder *to_cxl_decoder(struct device *dev);
> >  bool is_root_decoder(struct device *dev);
> >  struct cxl_decoder *cxl_decoder_alloc(struct cxl_port *port,
> >  				      unsigned int nr_targets);
> > +int cxl_decoder_add_locked(struct cxl_decoder *cxld, int *target_map);
> >  int cxl_decoder_add(struct cxl_decoder *cxld, int *target_map);
> >  int cxl_decoder_autoremove(struct device *host, struct cxl_decoder *cxld);
> >  
>
Jonathan Cameron Nov. 23, 2021, 11:05 a.m. UTC | #3
On Mon, 22 Nov 2021 13:58:01 -0800
Ben Widawsky <ben.widawsky@intel.com> wrote:

> On 21-11-22 16:33:02, Jonathan Cameron wrote:
> > On Fri, 19 Nov 2021 16:02:40 -0800
> > Ben Widawsky <ben.widawsky@intel.com> wrote:
> >   
> > > In preparation for a port driver that enumerates a descendant port +
> > > decoder hierarchy, arrange for an unlocked version of cxl_decoder_add().
> > > Otherwise a port-driver that adds a child decoder will deadlock on the
> > > device_lock() in ->probe().
> > >   
> > 
> > I think this description should call out that the lock was originally taken
> > for a much shorter time in decoder_populate_targets() but is moved
> > up one layer.  
> 
> Sounds good.
With that added and below discussion resolved.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> 
> > 
> > One other query inline.  Seems like we the WARN_ON stuff is a bit
> > over paranoid given what's visible in this patch.  If there is a
> > good reason for that, then add something to the patch description to
> > justify it.
> >    
> > > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
> > > 
> > > ---
> > > 
> > > Changes since RFCv2:
> > > - Reword commit message (Dan)
> > > - Move decoder API changes into this patch (Dan)
> > > ---
> > >  drivers/cxl/core/bus.c | 59 +++++++++++++++++++++++++++++++-----------
> > >  drivers/cxl/cxl.h      |  1 +
> > >  2 files changed, 45 insertions(+), 15 deletions(-)
> > > 
> > > diff --git a/drivers/cxl/core/bus.c b/drivers/cxl/core/bus.c
> > > index 16b15f54fb62..cd6fe7823c69 100644
> > > --- a/drivers/cxl/core/bus.c
> > > +++ b/drivers/cxl/core/bus.c
> > > @@ -487,28 +487,22 @@ static int decoder_populate_targets(struct cxl_decoder *cxld,
> > >  {
> > >  	int rc = 0, i;
> > >  
> > > +	device_lock_assert(&port->dev);
> > > +
> > >  	if (!target_map)
> > >  		return 0;
> > >  
> > > -	device_lock(&port->dev);
> > > -	if (list_empty(&port->dports)) {
> > > -		rc = -EINVAL;
> > > -		goto out_unlock;
> > > -	}
> > > +	if (list_empty(&port->dports))
> > > +		return -EINVAL;
> > >  
> > >  	for (i = 0; i < cxld->nr_targets; i++) {
> > >  		struct cxl_dport *dport = find_dport(port, target_map[i]);
> > >  
> > > -		if (!dport) {
> > > -			rc = -ENXIO;
> > > -			goto out_unlock;
> > > -		}
> > > +		if (!dport)
> > > +			return -ENXIO;
> > >  		cxld->target[i] = dport;
> > >  	}
> > >  
> > > -out_unlock:
> > > -	device_unlock(&port->dev);
> > > -
> > >  	return rc;
> > >  }
> > >  
> > > @@ -571,7 +565,7 @@ struct cxl_decoder *cxl_decoder_alloc(struct cxl_port *port,
> > >  EXPORT_SYMBOL_NS_GPL(cxl_decoder_alloc, CXL);
> > >  
> > >  /**
> > > - * cxl_decoder_add - Add a decoder with targets
> > > + * cxl_decoder_add_locked - Add a decoder with targets
> > >   * @cxld: The cxl decoder allocated by cxl_decoder_alloc()
> > >   * @target_map: A list of downstream ports that this decoder can direct memory
> > >   *              traffic to. These numbers should correspond with the port number
> > > @@ -581,12 +575,14 @@ EXPORT_SYMBOL_NS_GPL(cxl_decoder_alloc, CXL);
> > >   * is an endpoint device. A more awkward example is a hostbridge whose root
> > >   * ports get hot added (technically possible, though unlikely).
> > >   *
> > > - * Context: Process context. Takes and releases the cxld's device lock.
> > > + * This is the locked variant of cxl_decoder_add().
> > > + *
> > > + * Context: Process context. Expects the cxld's device lock to be held.
> > >   *
> > >   * Return: Negative error code if the decoder wasn't properly configured; else
> > >   *	   returns 0.
> > >   */
> > > -int cxl_decoder_add(struct cxl_decoder *cxld, int *target_map)
> > > +int cxl_decoder_add_locked(struct cxl_decoder *cxld, int *target_map)
> > >  {
> > >  	struct cxl_port *port;
> > >  	struct device *dev;
> > > @@ -619,6 +615,39 @@ int cxl_decoder_add(struct cxl_decoder *cxld, int *target_map)
> > >  
> > >  	return device_add(dev);
> > >  }
> > > +EXPORT_SYMBOL_NS_GPL(cxl_decoder_add_locked, CXL);
> > > +
> > > +/**
> > > + * cxl_decoder_add - Add a decoder with targets
> > > + * @cxld: The cxl decoder allocated by cxl_decoder_alloc()
> > > + * @target_map: A list of downstream ports that this decoder can direct memory
> > > + *              traffic to. These numbers should correspond with the port number
> > > + *              in the PCIe Link Capabilities structure.
> > > + *
> > > + * This is the unlocked variant of cxl_decoder_add_locked().
> > > + * See cxl_decoder_add_locked().
> > > + *
> > > + * Context: Process context. Takes and releases the cxld's device lock.
> > > + */
> > > +int cxl_decoder_add(struct cxl_decoder *cxld, int *target_map)
> > > +{
> > > +	struct cxl_port *port;
> > > +	int rc;
> > > +
> > > +	if (WARN_ON_ONCE(!cxld))
> > > +		return -EINVAL;  
> > 
> > Why do we now need these protections but didn't before?  
> 
> I don't quite understand what you're trying to point out.
> 
> Prior to this patch, cxl_decoder_add() checks:
> - !cxld
> - IS_ERR(cxld)
> - cxld->interleave_ways != 0
> 
> After this patch, cxl_decoder_add() checks:
> - !cxld
> - IS_ERR(cxld)
> - (and then calls cxl_decoder_add_locked())
> 
> And cxl_decoder_add_locked() checks:
> - !cxld
> - IS_ERR(cxld)
> - cxld->interleave_ways != 0
> 
> Ultimately we want to check all 3, and since cxl_decoder_add() calls
> cxl_decoder_add_locked(), we're good there. The problem is to get from a cxld to
> a port, you need to make sure you have a valid cxld, and the API previously
> allowed !cxld and IS_ERR(cxld). So there are duplicative checks if you call
> cxl_decoder_add(), but other than that I don't see any new protections.

Ah. It was the duplication that I didn't follow.

Fair enough

J
> 
> > 
> >   
> > > +
> > > +	if (WARN_ON_ONCE(IS_ERR(cxld)))
> > > +		return PTR_ERR(cxld);
> > > +
> > > +	port = to_cxl_port(cxld->dev.parent);
> > > +
> > > +	device_lock(&port->dev);
> > > +	rc = cxl_decoder_add_locked(cxld, target_map);
> > > +	device_unlock(&port->dev);
> > > +
> > > +	return rc;
> > > +}
> > >  EXPORT_SYMBOL_NS_GPL(cxl_decoder_add, CXL);
> > >  
> > >  static void cxld_unregister(void *dev)
> > > diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> > > index b66ed8f241c6..2c5627fa8a34 100644
> > > --- a/drivers/cxl/cxl.h
> > > +++ b/drivers/cxl/cxl.h
> > > @@ -290,6 +290,7 @@ struct cxl_decoder *to_cxl_decoder(struct device *dev);
> > >  bool is_root_decoder(struct device *dev);
> > >  struct cxl_decoder *cxl_decoder_alloc(struct cxl_port *port,
> > >  				      unsigned int nr_targets);
> > > +int cxl_decoder_add_locked(struct cxl_decoder *cxld, int *target_map);
> > >  int cxl_decoder_add(struct cxl_decoder *cxld, int *target_map);
> > >  int cxl_decoder_autoremove(struct device *host, struct cxl_decoder *cxld);
> > >    
> >
Dan Williams Nov. 25, 2021, 12:34 a.m. UTC | #4
On Fri, Nov 19, 2021 at 4:03 PM Ben Widawsky <ben.widawsky@intel.com> wrote:
>
> In preparation for a port driver that enumerates a descendant port +
> decoder hierarchy, arrange for an unlocked version of cxl_decoder_add().
> Otherwise a port-driver that adds a child decoder will deadlock on the
> device_lock() in ->probe().
>
> Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
>
> ---
>
> Changes since RFCv2:
> - Reword commit message (Dan)
> - Move decoder API changes into this patch (Dan)
> ---
>  drivers/cxl/core/bus.c | 59 +++++++++++++++++++++++++++++++-----------
>  drivers/cxl/cxl.h      |  1 +
>  2 files changed, 45 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/cxl/core/bus.c b/drivers/cxl/core/bus.c
> index 16b15f54fb62..cd6fe7823c69 100644
> --- a/drivers/cxl/core/bus.c
> +++ b/drivers/cxl/core/bus.c
> @@ -487,28 +487,22 @@ static int decoder_populate_targets(struct cxl_decoder *cxld,
>  {
>         int rc = 0, i;
>
> +       device_lock_assert(&port->dev);
> +
>         if (!target_map)
>                 return 0;
>
> -       device_lock(&port->dev);
> -       if (list_empty(&port->dports)) {
> -               rc = -EINVAL;
> -               goto out_unlock;
> -       }
> +       if (list_empty(&port->dports))
> +               return -EINVAL;
>
>         for (i = 0; i < cxld->nr_targets; i++) {
>                 struct cxl_dport *dport = find_dport(port, target_map[i]);
>
> -               if (!dport) {
> -                       rc = -ENXIO;
> -                       goto out_unlock;
> -               }
> +               if (!dport)
> +                       return -ENXIO;
>                 cxld->target[i] = dport;
>         }
>
> -out_unlock:
> -       device_unlock(&port->dev);
> -
>         return rc;
>  }
>
> @@ -571,7 +565,7 @@ struct cxl_decoder *cxl_decoder_alloc(struct cxl_port *port,
>  EXPORT_SYMBOL_NS_GPL(cxl_decoder_alloc, CXL);
>
>  /**
> - * cxl_decoder_add - Add a decoder with targets
> + * cxl_decoder_add_locked - Add a decoder with targets
>   * @cxld: The cxl decoder allocated by cxl_decoder_alloc()
>   * @target_map: A list of downstream ports that this decoder can direct memory
>   *              traffic to. These numbers should correspond with the port number
> @@ -581,12 +575,14 @@ EXPORT_SYMBOL_NS_GPL(cxl_decoder_alloc, CXL);
>   * is an endpoint device. A more awkward example is a hostbridge whose root
>   * ports get hot added (technically possible, though unlikely).
>   *
> - * Context: Process context. Takes and releases the cxld's device lock.
> + * This is the locked variant of cxl_decoder_add().
> + *
> + * Context: Process context. Expects the cxld's device lock to be held.
>   *
>   * Return: Negative error code if the decoder wasn't properly configured; else
>   *        returns 0.
>   */
> -int cxl_decoder_add(struct cxl_decoder *cxld, int *target_map)
> +int cxl_decoder_add_locked(struct cxl_decoder *cxld, int *target_map)
>  {
>         struct cxl_port *port;
>         struct device *dev;
> @@ -619,6 +615,39 @@ int cxl_decoder_add(struct cxl_decoder *cxld, int *target_map)
>
>         return device_add(dev);
>  }
> +EXPORT_SYMBOL_NS_GPL(cxl_decoder_add_locked, CXL);
> +
> +/**
> + * cxl_decoder_add - Add a decoder with targets
> + * @cxld: The cxl decoder allocated by cxl_decoder_alloc()
> + * @target_map: A list of downstream ports that this decoder can direct memory
> + *              traffic to. These numbers should correspond with the port number
> + *              in the PCIe Link Capabilities structure.
> + *
> + * This is the unlocked variant of cxl_decoder_add_locked().
> + * See cxl_decoder_add_locked().
> + *
> + * Context: Process context. Takes and releases the cxld's device lock.

No, it takes the port's lock to walk its dport list.

Otherwise, looks good to me:

Reviewed-by: Dan Williams <dan.j.williams@intel.com>
diff mbox series

Patch

diff --git a/drivers/cxl/core/bus.c b/drivers/cxl/core/bus.c
index 16b15f54fb62..cd6fe7823c69 100644
--- a/drivers/cxl/core/bus.c
+++ b/drivers/cxl/core/bus.c
@@ -487,28 +487,22 @@  static int decoder_populate_targets(struct cxl_decoder *cxld,
 {
 	int rc = 0, i;
 
+	device_lock_assert(&port->dev);
+
 	if (!target_map)
 		return 0;
 
-	device_lock(&port->dev);
-	if (list_empty(&port->dports)) {
-		rc = -EINVAL;
-		goto out_unlock;
-	}
+	if (list_empty(&port->dports))
+		return -EINVAL;
 
 	for (i = 0; i < cxld->nr_targets; i++) {
 		struct cxl_dport *dport = find_dport(port, target_map[i]);
 
-		if (!dport) {
-			rc = -ENXIO;
-			goto out_unlock;
-		}
+		if (!dport)
+			return -ENXIO;
 		cxld->target[i] = dport;
 	}
 
-out_unlock:
-	device_unlock(&port->dev);
-
 	return rc;
 }
 
@@ -571,7 +565,7 @@  struct cxl_decoder *cxl_decoder_alloc(struct cxl_port *port,
 EXPORT_SYMBOL_NS_GPL(cxl_decoder_alloc, CXL);
 
 /**
- * cxl_decoder_add - Add a decoder with targets
+ * cxl_decoder_add_locked - Add a decoder with targets
  * @cxld: The cxl decoder allocated by cxl_decoder_alloc()
  * @target_map: A list of downstream ports that this decoder can direct memory
  *              traffic to. These numbers should correspond with the port number
@@ -581,12 +575,14 @@  EXPORT_SYMBOL_NS_GPL(cxl_decoder_alloc, CXL);
  * is an endpoint device. A more awkward example is a hostbridge whose root
  * ports get hot added (technically possible, though unlikely).
  *
- * Context: Process context. Takes and releases the cxld's device lock.
+ * This is the locked variant of cxl_decoder_add().
+ *
+ * Context: Process context. Expects the cxld's device lock to be held.
  *
  * Return: Negative error code if the decoder wasn't properly configured; else
  *	   returns 0.
  */
-int cxl_decoder_add(struct cxl_decoder *cxld, int *target_map)
+int cxl_decoder_add_locked(struct cxl_decoder *cxld, int *target_map)
 {
 	struct cxl_port *port;
 	struct device *dev;
@@ -619,6 +615,39 @@  int cxl_decoder_add(struct cxl_decoder *cxld, int *target_map)
 
 	return device_add(dev);
 }
+EXPORT_SYMBOL_NS_GPL(cxl_decoder_add_locked, CXL);
+
+/**
+ * cxl_decoder_add - Add a decoder with targets
+ * @cxld: The cxl decoder allocated by cxl_decoder_alloc()
+ * @target_map: A list of downstream ports that this decoder can direct memory
+ *              traffic to. These numbers should correspond with the port number
+ *              in the PCIe Link Capabilities structure.
+ *
+ * This is the unlocked variant of cxl_decoder_add_locked().
+ * See cxl_decoder_add_locked().
+ *
+ * Context: Process context. Takes and releases the cxld's device lock.
+ */
+int cxl_decoder_add(struct cxl_decoder *cxld, int *target_map)
+{
+	struct cxl_port *port;
+	int rc;
+
+	if (WARN_ON_ONCE(!cxld))
+		return -EINVAL;
+
+	if (WARN_ON_ONCE(IS_ERR(cxld)))
+		return PTR_ERR(cxld);
+
+	port = to_cxl_port(cxld->dev.parent);
+
+	device_lock(&port->dev);
+	rc = cxl_decoder_add_locked(cxld, target_map);
+	device_unlock(&port->dev);
+
+	return rc;
+}
 EXPORT_SYMBOL_NS_GPL(cxl_decoder_add, CXL);
 
 static void cxld_unregister(void *dev)
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index b66ed8f241c6..2c5627fa8a34 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -290,6 +290,7 @@  struct cxl_decoder *to_cxl_decoder(struct device *dev);
 bool is_root_decoder(struct device *dev);
 struct cxl_decoder *cxl_decoder_alloc(struct cxl_port *port,
 				      unsigned int nr_targets);
+int cxl_decoder_add_locked(struct cxl_decoder *cxld, int *target_map);
 int cxl_decoder_add(struct cxl_decoder *cxld, int *target_map);
 int cxl_decoder_autoremove(struct device *host, struct cxl_decoder *cxld);