On Sat, Sep 11, 2021 at 10:15 AM Ben Widawsky <[email protected]> wrote: > > On 21-09-10 11:36:05, Dan Williams wrote: > > On Fri, Sep 10, 2021 at 3:34 AM Jonathan Cameron > > <[email protected]> wrote: > > > > > > On Wed, 8 Sep 2021 22:13:26 -0700 > > > Dan Williams <[email protected]> wrote: > > > > > > > The kbuild robot reports: > > > > > > > > drivers/cxl/core/bus.c:516:1: warning: stack frame size (1032) > > > > exceeds > > > > limit (1024) in function 'devm_cxl_add_decoder' > > > > > > > > It is also the case the devm_cxl_add_decoder() is unwieldy to use for > > > > all the different decoder types. Fix the stack usage by splitting the > > > > creation into alloc and add steps. This also allows for context > > > > specific construction before adding. > > > > > > > > With the split the caller is responsible for registering a devm callback > > > > to trigger device_unregister() for the decoder rather than it being > > > > implicit in the decoder registration. I.e. the routine that calls alloc > > > > is responsible for calling put_device() if the "add" operation fails. > > > > > > > > Reported-by: kernel test robot <[email protected]> > > > > Reported-by: Nathan Chancellor <[email protected]> > > > > Reported-by: Dan Carpenter <[email protected]> > > > > Signed-off-by: Dan Williams <[email protected]> > > > > > > A few minor things inline. This one was definitely a case where diff > > > wasn't being helpful in how it chose to format things! > > > > > > I haven't taken the time to figure out if the device_lock() changes > > > make complete sense as I don't understand the intent. > > > I think they should be called out in the patch description as they > > > seem a little non obvious. > > > > > > Jonathan > > > > > > > --- > > > > drivers/cxl/acpi.c | 84 +++++++++++++++++++++++++---------- > > > > drivers/cxl/core/bus.c | 114 > > > > ++++++++++++++--------------------------------- > > > > drivers/cxl/core/core.h | 5 -- > > > > drivers/cxl/core/pmem.c | 7 ++- > > > > drivers/cxl/cxl.h | 16 +++---- > > > > 5 files changed, 106 insertions(+), 120 deletions(-) > > > > > > > > diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c > > > > index 9d881eacdae5..654a80547526 100644 > > > > --- a/drivers/cxl/acpi.c > > > > +++ b/drivers/cxl/acpi.c > > > > @@ -82,7 +82,6 @@ static void cxl_add_cfmws_decoders(struct device *dev, > > > > struct cxl_decoder *cxld; > > > > acpi_size len, cur = 0; > > > > void *cedt_subtable; > > > > - unsigned long flags; > > > > int rc; > > > > > > > > len = acpi_cedt->length - sizeof(*acpi_cedt); > > > > @@ -119,24 +118,36 @@ static void cxl_add_cfmws_decoders(struct device > > > > *dev, > > > > for (i = 0; i < CFMWS_INTERLEAVE_WAYS(cfmws); i++) > > > > target_map[i] = cfmws->interleave_targets[i]; > > > > > > > > - flags = cfmws_to_decoder_flags(cfmws->restrictions); > > > > - cxld = devm_cxl_add_decoder(dev, root_port, > > > > - CFMWS_INTERLEAVE_WAYS(cfmws), > > > > - cfmws->base_hpa, > > > > cfmws->window_size, > > > > - CFMWS_INTERLEAVE_WAYS(cfmws), > > > > - > > > > CFMWS_INTERLEAVE_GRANULARITY(cfmws), > > > > - CXL_DECODER_EXPANDER, > > > > - flags, target_map); > > > > - > > > > - if (IS_ERR(cxld)) { > > > > + cxld = cxl_decoder_alloc(root_port, > > > > + CFMWS_INTERLEAVE_WAYS(cfmws)); > > > > + if (IS_ERR(cxld)) > > > > + goto next; > > > > + > > > > + cxld->flags = cfmws_to_decoder_flags(cfmws->restrictions); > > > > + cxld->target_type = CXL_DECODER_EXPANDER; > > > > + cxld->range = (struct range) { > > > > + .start = cfmws->base_hpa, > > > > + .end = cfmws->base_hpa + cfmws->window_size - 1, > > > > + }; > > > > + cxld->interleave_ways = CFMWS_INTERLEAVE_WAYS(cfmws); > > > > + cxld->interleave_granularity = > > > > + CFMWS_INTERLEAVE_GRANULARITY(cfmws); > > > > + > > > > + rc = cxl_decoder_add(dev, cxld, target_map); > > > > + if (rc) > > > > + put_device(&cxld->dev); > > > > + else > > > > + rc = cxl_decoder_autoremove(dev, cxld); > > > > + if (rc) { > > > > dev_err(dev, "Failed to add decoder for > > > > %#llx-%#llx\n", > > > > cfmws->base_hpa, cfmws->base_hpa + > > > > cfmws->window_size - 1); > > > > - } else { > > > > - dev_dbg(dev, "add: %s range %#llx-%#llx\n", > > > > - dev_name(&cxld->dev), cfmws->base_hpa, > > > > - cfmws->base_hpa + cfmws->window_size - > > > > 1); > > > > + goto next; > > > > } > > > > + dev_dbg(dev, "add: %s range %#llx-%#llx\n", > > > > + dev_name(&cxld->dev), cfmws->base_hpa, > > > > + cfmws->base_hpa + cfmws->window_size - 1); > > > > +next: > > > > cur += c->length; > > > > } > > > > } > > > > @@ -268,6 +279,7 @@ static int add_host_bridge_uport(struct device > > > > *match, void *arg) > > > > struct acpi_device *bridge = to_cxl_host_bridge(host, match); > > > > struct acpi_pci_root *pci_root; > > > > struct cxl_walk_context ctx; > > > > + int single_port_map[1], rc; > > > > struct cxl_decoder *cxld; > > > > struct cxl_dport *dport; > > > > struct cxl_port *port; > > > > @@ -303,22 +315,46 @@ static int add_host_bridge_uport(struct device > > > > *match, void *arg) > > > > return -ENODEV; > > > > if (ctx.error) > > > > return ctx.error; > > > > + if (ctx.count > 1) > > > > + return 0; > > > > > > > > /* TODO: Scan CHBCR for HDM Decoder resources */ > > > > > > > > /* > > > > - * In the single-port host-bridge case there are no HDM decoders > > > > - * in the CHBCR and a 1:1 passthrough decode is implied. > > > > + * Per the CXL specification (8.2.5.12 CXL HDM Decoder Capability > > > > + * Structure) single ported host-bridges need not publish a > > > > decoder > > > > + * capability when a passthrough decode can be assumed, i.e. all > > > > + * transactions that the uport sees are claimed and passed to the > > > > single > > > > + * dport. Default the range a 0-base 0-length until the first CXL > > > > region > > > > + * is activated. > > > > */ > > > > - if (ctx.count == 1) { > > > > - cxld = devm_cxl_add_passthrough_decoder(host, port); > > > > - if (IS_ERR(cxld)) > > > > - return PTR_ERR(cxld); > > > > + cxld = cxl_decoder_alloc(port, 1); > > > > + if (IS_ERR(cxld)) > > > > + return PTR_ERR(cxld); > > > > + > > > > + cxld->interleave_ways = 1; > > > > + cxld->interleave_granularity = PAGE_SIZE; > > > > + cxld->target_type = CXL_DECODER_EXPANDER; > > > > + cxld->range = (struct range) { > > > > + .start = 0, > > > > + .end = -1, > > > > + }; > > > > > > > > - dev_dbg(host, "add: %s\n", dev_name(&cxld->dev)); > > > > - } > > > > > > This was previously done without taking the device lock... (see below) > > > > > > > + device_lock(&port->dev); > > > > + dport = list_first_entry(&port->dports, typeof(*dport), list); > > > > + device_unlock(&port->dev); > > > > > > > > - return 0; > > > > + single_port_map[0] = dport->port_id; > > > > + > > > > + rc = cxl_decoder_add(host, cxld, single_port_map); > > > > + if (rc) > > > > + put_device(&cxld->dev); > > > > + else > > > > + rc = cxl_decoder_autoremove(host, cxld); > > > > + > > > > + if (rc == 0) > > > > + dev_dbg(host, "add: %s\n", dev_name(&cxld->dev)); > > > > + return rc; > > > > } > > > > > > > > static int add_host_bridge_dport(struct device *match, void *arg) > > > > diff --git a/drivers/cxl/core/bus.c b/drivers/cxl/core/bus.c > > > > index 176bede30c55..be787685b13e 100644 > > > > --- a/drivers/cxl/core/bus.c > > > > +++ b/drivers/cxl/core/bus.c > > > > @@ -455,16 +455,18 @@ EXPORT_SYMBOL_GPL(cxl_add_dport); > > > > > > > > static int decoder_populate_targets(struct device *host, > > > > struct cxl_decoder *cxld, > > > > - struct cxl_port *port, int > > > > *target_map, > > > > - int nr_targets) > > > > + struct cxl_port *port, int > > > > *target_map) > > > > { > > > > int rc = 0, i; > > > > > > > > + if (list_empty(&port->dports)) > > > > + return -EINVAL; > > > > + > > > > > > The code before this patch did this under the device_lock() Perhaps call > > > out the fact there is no need to do that if we don't need to? > > > > Nope, bug, good catch. > > > > While you're changing this might I request you make this change so I can drop > my > patch: > > commit fe46c7a3e30129c649e17a4c555699e816cf04e7 > Author: Ben Widawsky <[email protected]> > Date: 16 hours ago > > core/bus: Don't error for targetless decoders > > Decoders may not have any targets, endpoints are the best example of > this. In order to prevent having to special case, it's easiest to just > not return an error when no target map is specified as those endpoints > also won't have dports. > > Signed-off-by: Ben Widawsky <[email protected]> > > diff --git a/drivers/cxl/core/bus.c b/drivers/cxl/core/bus.c > index c13b6c4d4135..b98b3e343a3d 100644 > --- a/drivers/cxl/core/bus.c > +++ b/drivers/cxl/core/bus.c > @@ -547,12 +547,12 @@ static int decoder_populate_targets(struct device *host, > { > int rc = 0, i; > > - if (list_empty(&port->dports)) > - return -EINVAL; > - > if (!target_map) > return 0; > > + if (list_empty(&port->dports)) > + return -EINVAL; > + > device_lock(&port->dev); > for (i = 0; i < cxld->nr_targets; i++) { > struct cxl_dport *dport = find_dport(port, target_map[i]); >
Yes, I will fold that fix in when I address the locking around the list_empty check.
