On Fri, Nov 19, 2021 at 4:03 PM Ben Widawsky <ben.widawsky@xxxxxxxxx> 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@xxxxxxxxx>
>
> ---
>
> 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@xxxxxxxxx>
