| Message ID | 20211120000250.1663391-14-ben.widawsky@intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add drivers for CXL ports and mem devices | expand |
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); >
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); > > >
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); > > > > >
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 --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);
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(-)