| Message ID | 20220831081603.3415-4-rrichter@amd.com |
|---|---|
| State | Superseded |
| Delegated to: | Dan Williams |
| Headers | show |
| Series | cxl: Add support for Restricted CXL hosts (RCD mode) | expand |
On Wed, 31 Aug 2022 10:15:51 +0200 Robert Richter <rrichter@amd.com> wrote: > CXL ports are added in a couple of code paths using > devm_cxl_add_port(). Debug messages are individually generated, but > are incomplete and inconsistent. Change this by moving its generation > to devm_cxl_add_port(). This unifies the messages and reduces code > duplication. Also, generate messages on failure. > > Signed-off-by: Robert Richter <rrichter@amd.com> This is one for Dan etc as it is mostly a question of how verbose we want the debug prints to be plus preference for caller or callee being responsible for outputting this sort of message. Patch looks good to me if we want to make this sort of change. > --- > drivers/cxl/acpi.c | 2 -- > drivers/cxl/core/port.c | 39 ++++++++++++++++++++++++++++----------- > 2 files changed, 28 insertions(+), 13 deletions(-) > > diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c > index fb649683dd3a..767a91f44221 100644 > --- a/drivers/cxl/acpi.c > +++ b/drivers/cxl/acpi.c > @@ -220,7 +220,6 @@ static int add_host_bridge_uport(struct device *match, void *arg) > port = devm_cxl_add_port(host, match, dport->component_reg_phys, dport); > if (IS_ERR(port)) > return PTR_ERR(port); > - dev_dbg(host, "%s: add: %s\n", dev_name(match), dev_name(&port->dev)); > > return 0; > } > @@ -466,7 +465,6 @@ static int cxl_acpi_probe(struct platform_device *pdev) > root_port = devm_cxl_add_port(host, host, CXL_RESOURCE_NONE, NULL); > if (IS_ERR(root_port)) > return PTR_ERR(root_port); > - dev_dbg(host, "add: %s\n", dev_name(&root_port->dev)); > > rc = bus_for_each_dev(adev->dev.bus, NULL, root_port, > add_host_bridge_dport); > diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c > index bffde862de0b..8604cda88787 100644 > --- a/drivers/cxl/core/port.c > +++ b/drivers/cxl/core/port.c > @@ -666,13 +666,17 @@ struct cxl_port *devm_cxl_add_port(struct device *host, struct device *uport, > resource_size_t component_reg_phys, > struct cxl_dport *parent_dport) > { > - struct cxl_port *port; > + struct cxl_port *port, *parent_port; > struct device *dev; > int rc; > > + parent_port = parent_dport ? parent_dport->port : NULL; > + > port = cxl_port_alloc(uport, component_reg_phys, parent_dport); > - if (IS_ERR(port)) > - return port; > + if (IS_ERR(port)) { > + rc = PTR_ERR(port); > + goto err_out; > + } > > dev = &port->dev; > if (is_cxl_memdev(uport)) > @@ -682,24 +686,39 @@ struct cxl_port *devm_cxl_add_port(struct device *host, struct device *uport, > else > rc = dev_set_name(dev, "root%d", port->id); > if (rc) > - goto err; > + goto err_put; > > rc = device_add(dev); > if (rc) > - goto err; > + goto err_put; > > rc = devm_add_action_or_reset(host, unregister_port, port); > if (rc) > - return ERR_PTR(rc); > + goto err_out; > > rc = devm_cxl_link_uport(host, port); > if (rc) > - return ERR_PTR(rc); > + goto err_out; > > - return port; > + dev_dbg(host, "added %s as%s port of device %s%s%s\n", > + dev_name(&port->dev), > + parent_port ? "" : " root", > + dev_name(uport), > + parent_port ? " to parent port " : "", > + parent_port ? dev_name(&parent_port->dev) : ""); > > -err: > + return port; > +err_put: > put_device(dev); > +err_out: > + dev_dbg(host, "failed to add %s as%s port of device %s%s%s: %d\n", > + dev_name(&port->dev), > + parent_port ? "" : " root", > + dev_name(uport), > + parent_port ? " to parent port " : "", > + parent_port ? dev_name(&parent_port->dev) : "", > + rc); > + > return ERR_PTR(rc); > } > EXPORT_SYMBOL_NS_GPL(devm_cxl_add_port, CXL); > @@ -1140,8 +1159,6 @@ int devm_cxl_add_endpoint(struct cxl_memdev *cxlmd, > if (IS_ERR(endpoint)) > return PTR_ERR(endpoint); > > - dev_dbg(&cxlmd->dev, "add: %s\n", dev_name(&endpoint->dev)); > - > rc = cxl_endpoint_autoremove(cxlmd, endpoint); > if (rc) > return rc;
On 31.08.22 10:59:45, Jonathan Cameron wrote: > On Wed, 31 Aug 2022 10:15:51 +0200 > Robert Richter <rrichter@amd.com> wrote: > > > CXL ports are added in a couple of code paths using > > devm_cxl_add_port(). Debug messages are individually generated, but > > are incomplete and inconsistent. Change this by moving its generation > > to devm_cxl_add_port(). This unifies the messages and reduces code > > duplication. Also, generate messages on failure. > > > > Signed-off-by: Robert Richter <rrichter@amd.com> > > This is one for Dan etc as it is mostly a question of how verbose we want > the debug prints to be plus preference for caller or callee being > responsible for outputting this sort of message. Esp. together with dyndbg this is very useful as you can get a whole picture of the port enablement. -Robert
On 31.08.22 10:59:45, Jonathan Cameron wrote: > On Wed, 31 Aug 2022 10:15:51 +0200 > Robert Richter <rrichter@amd.com> wrote: > > > CXL ports are added in a couple of code paths using > > devm_cxl_add_port(). Debug messages are individually generated, but > > are incomplete and inconsistent. Change this by moving its generation > > to devm_cxl_add_port(). This unifies the messages and reduces code > > duplication. Also, generate messages on failure. > > > > Signed-off-by: Robert Richter <rrichter@amd.com> > > This is one for Dan etc as it is mostly a question of how verbose we want > the debug prints to be plus preference for caller or callee being > responsible for outputting this sort of message. > > Patch looks good to me if we want to make this sort of change. Should I take this as a Reviewed-by? Thanks, -Robert
On Tue, 6 Sep 2022 09:30:37 +0200 Robert Richter <rrichter@amd.com> wrote: > On 31.08.22 10:59:45, Jonathan Cameron wrote: > > On Wed, 31 Aug 2022 10:15:51 +0200 > > Robert Richter <rrichter@amd.com> wrote: > > > > > CXL ports are added in a couple of code paths using > > > devm_cxl_add_port(). Debug messages are individually generated, but > > > are incomplete and inconsistent. Change this by moving its generation > > > to devm_cxl_add_port(). This unifies the messages and reduces code > > > duplication. Also, generate messages on failure. > > > > > > Signed-off-by: Robert Richter <rrichter@amd.com> > > > > This is one for Dan etc as it is mostly a question of how verbose we want > > the debug prints to be plus preference for caller or callee being > > responsible for outputting this sort of message. > > > > Patch looks good to me if we want to make this sort of change. > > Should I take this as a Reviewed-by? Hmm. I guess I could go that far as its a policy decision rather than correctness Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > Thanks, > > -Robert
On Wed, 31 Aug 2022, Robert Richter wrote: >CXL ports are added in a couple of code paths using >devm_cxl_add_port(). Debug messages are individually generated, but >are incomplete and inconsistent. Change this by moving its generation >to devm_cxl_add_port(). This unifies the messages and reduces code >duplication. Also, generate messages on failure. > >Signed-off-by: Robert Richter <rrichter@amd.com> >Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> Reviewed-by: Davidlohr Bueso <dave@stgolabs.net>
Robert Richter wrote: > CXL ports are added in a couple of code paths using > devm_cxl_add_port(). Debug messages are individually generated, but > are incomplete and inconsistent. Change this by moving its generation > to devm_cxl_add_port(). This unifies the messages and reduces code > duplication. Also, generate messages on failure. > > Signed-off-by: Robert Richter <rrichter@amd.com> > --- > drivers/cxl/acpi.c | 2 -- > drivers/cxl/core/port.c | 39 ++++++++++++++++++++++++++++----------- > 2 files changed, 28 insertions(+), 13 deletions(-) > > diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c > index fb649683dd3a..767a91f44221 100644 > --- a/drivers/cxl/acpi.c > +++ b/drivers/cxl/acpi.c > @@ -220,7 +220,6 @@ static int add_host_bridge_uport(struct device *match, void *arg) > port = devm_cxl_add_port(host, match, dport->component_reg_phys, dport); > if (IS_ERR(port)) > return PTR_ERR(port); > - dev_dbg(host, "%s: add: %s\n", dev_name(match), dev_name(&port->dev)); > > return 0; > } > @@ -466,7 +465,6 @@ static int cxl_acpi_probe(struct platform_device *pdev) > root_port = devm_cxl_add_port(host, host, CXL_RESOURCE_NONE, NULL); > if (IS_ERR(root_port)) > return PTR_ERR(root_port); > - dev_dbg(host, "add: %s\n", dev_name(&root_port->dev)); > > rc = bus_for_each_dev(adev->dev.bus, NULL, root_port, > add_host_bridge_dport); > diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c > index bffde862de0b..8604cda88787 100644 > --- a/drivers/cxl/core/port.c > +++ b/drivers/cxl/core/port.c > @@ -666,13 +666,17 @@ struct cxl_port *devm_cxl_add_port(struct device *host, struct device *uport, > resource_size_t component_reg_phys, > struct cxl_dport *parent_dport) > { > - struct cxl_port *port; > + struct cxl_port *port, *parent_port; > struct device *dev; > int rc; > > + parent_port = parent_dport ? parent_dport->port : NULL; > + > port = cxl_port_alloc(uport, component_reg_phys, parent_dport); > - if (IS_ERR(port)) > - return port; > + if (IS_ERR(port)) { > + rc = PTR_ERR(port); > + goto err_out; While I agree this unifies the error messaging I am not a fan of what this does to the readability of the error exits. What about a compromise of renaming devm_cxl_add_port to __devm_cxl_add_port() and add back a new devm_cxl_add_port that does the unified debug messaging?
On 07.09.22 22:53:12, Dan Williams wrote: > Robert Richter wrote: > > + parent_port = parent_dport ? parent_dport->port : NULL; > > + > > port = cxl_port_alloc(uport, component_reg_phys, parent_dport); > > - if (IS_ERR(port)) > > - return port; > > + if (IS_ERR(port)) { > > + rc = PTR_ERR(port); > > + goto err_out; > > While I agree this unifies the error messaging I am not a fan of what > this does to the readability of the error exits. What about a compromise > of renaming devm_cxl_add_port to __devm_cxl_add_port() and add back a > new devm_cxl_add_port that does the unified debug messaging? Ok, will add a wrapper. Thanks, -Robert
diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c index fb649683dd3a..767a91f44221 100644 --- a/drivers/cxl/acpi.c +++ b/drivers/cxl/acpi.c @@ -220,7 +220,6 @@ static int add_host_bridge_uport(struct device *match, void *arg) port = devm_cxl_add_port(host, match, dport->component_reg_phys, dport); if (IS_ERR(port)) return PTR_ERR(port); - dev_dbg(host, "%s: add: %s\n", dev_name(match), dev_name(&port->dev)); return 0; } @@ -466,7 +465,6 @@ static int cxl_acpi_probe(struct platform_device *pdev) root_port = devm_cxl_add_port(host, host, CXL_RESOURCE_NONE, NULL); if (IS_ERR(root_port)) return PTR_ERR(root_port); - dev_dbg(host, "add: %s\n", dev_name(&root_port->dev)); rc = bus_for_each_dev(adev->dev.bus, NULL, root_port, add_host_bridge_dport); diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c index bffde862de0b..8604cda88787 100644 --- a/drivers/cxl/core/port.c +++ b/drivers/cxl/core/port.c @@ -666,13 +666,17 @@ struct cxl_port *devm_cxl_add_port(struct device *host, struct device *uport, resource_size_t component_reg_phys, struct cxl_dport *parent_dport) { - struct cxl_port *port; + struct cxl_port *port, *parent_port; struct device *dev; int rc; + parent_port = parent_dport ? parent_dport->port : NULL; + port = cxl_port_alloc(uport, component_reg_phys, parent_dport); - if (IS_ERR(port)) - return port; + if (IS_ERR(port)) { + rc = PTR_ERR(port); + goto err_out; + } dev = &port->dev; if (is_cxl_memdev(uport)) @@ -682,24 +686,39 @@ struct cxl_port *devm_cxl_add_port(struct device *host, struct device *uport, else rc = dev_set_name(dev, "root%d", port->id); if (rc) - goto err; + goto err_put; rc = device_add(dev); if (rc) - goto err; + goto err_put; rc = devm_add_action_or_reset(host, unregister_port, port); if (rc) - return ERR_PTR(rc); + goto err_out; rc = devm_cxl_link_uport(host, port); if (rc) - return ERR_PTR(rc); + goto err_out; - return port; + dev_dbg(host, "added %s as%s port of device %s%s%s\n", + dev_name(&port->dev), + parent_port ? "" : " root", + dev_name(uport), + parent_port ? " to parent port " : "", + parent_port ? dev_name(&parent_port->dev) : ""); -err: + return port; +err_put: put_device(dev); +err_out: + dev_dbg(host, "failed to add %s as%s port of device %s%s%s: %d\n", + dev_name(&port->dev), + parent_port ? "" : " root", + dev_name(uport), + parent_port ? " to parent port " : "", + parent_port ? dev_name(&parent_port->dev) : "", + rc); + return ERR_PTR(rc); } EXPORT_SYMBOL_NS_GPL(devm_cxl_add_port, CXL); @@ -1140,8 +1159,6 @@ int devm_cxl_add_endpoint(struct cxl_memdev *cxlmd, if (IS_ERR(endpoint)) return PTR_ERR(endpoint); - dev_dbg(&cxlmd->dev, "add: %s\n", dev_name(&endpoint->dev)); - rc = cxl_endpoint_autoremove(cxlmd, endpoint); if (rc) return rc;
CXL ports are added in a couple of code paths using devm_cxl_add_port(). Debug messages are individually generated, but are incomplete and inconsistent. Change this by moving its generation to devm_cxl_add_port(). This unifies the messages and reduces code duplication. Also, generate messages on failure. Signed-off-by: Robert Richter <rrichter@amd.com> --- drivers/cxl/acpi.c | 2 -- drivers/cxl/core/port.c | 39 ++++++++++++++++++++++++++++----------- 2 files changed, 28 insertions(+), 13 deletions(-)