| Message ID | 20240813070552.3353530-3-ming4.li@intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/3] cxl/port: Use __free() to drop put_device() for cxl_port | expand |
Li Ming wrote: > The "goto error" pattern is not recommended, it can be removed via > refactoring. In __devm_cxl_add_port(), there is a 'goto' to call > put_device() for the error cases between device_initialize() and > device_add() to dereference the 'struct device' of a new cxl_port. > The refactoring is introducing a new function called cxl_port_add() > which is used to add the 'struct device' of a new cxl_port to > device hierarchy, moving the functions needing the help of above > 'goto' into cxl_port_add(), and using a scope-based resource management > __free() to drop the open coded put_device() and 'goto' for the error > cases. > > Suggested-by: Dan Williams <dan.j.williams@intel.com> > Signed-off-by: Li Ming <ming4.li@intel.com> > --- > drivers/cxl/core/port.c | 59 ++++++++++++++++++++++++----------------- > 1 file changed, 35 insertions(+), 24 deletions(-) > > diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c > index 53e2593daa95..a886b16b2610 100644 > --- a/drivers/cxl/core/port.c > +++ b/drivers/cxl/core/port.c > @@ -828,27 +828,20 @@ static void cxl_debugfs_create_dport_dir(struct cxl_dport *dport) > &cxl_einj_inject_fops); > } > > -static struct cxl_port *__devm_cxl_add_port(struct device *host, > - struct device *uport_dev, > - resource_size_t component_reg_phys, > - struct cxl_dport *parent_dport) > +static int cxl_port_add(struct cxl_port *port, > + resource_size_t component_reg_phys, > + struct cxl_dport *parent_dport) > { > - struct cxl_port *port; > - struct device *dev; > + struct device *dev __free(put_device) = &port->dev; > int rc; > > - port = cxl_port_alloc(uport_dev, parent_dport); > - if (IS_ERR(port)) > - return port; > - > - dev = &port->dev; > - if (is_cxl_memdev(uport_dev)) { > - struct cxl_memdev *cxlmd = to_cxl_memdev(uport_dev); > + if (is_cxl_memdev(port->uport_dev)) { > + struct cxl_memdev *cxlmd = to_cxl_memdev(port->uport_dev); > struct cxl_dev_state *cxlds = cxlmd->cxlds; > > rc = dev_set_name(dev, "endpoint%d", port->id); > if (rc) > - goto err; > + return rc; > > /* > * The endpoint driver already enumerated the component and RAS > @@ -861,19 +854,41 @@ static struct cxl_port *__devm_cxl_add_port(struct device *host, > } else if (parent_dport) { > rc = dev_set_name(dev, "port%d", port->id); > if (rc) > - goto err; > + return rc; > > rc = cxl_port_setup_regs(port, component_reg_phys); > if (rc) > - goto err; > - } else > + return rc; > + } else { > rc = dev_set_name(dev, "root%d", port->id); > - if (rc) > - goto err; > + if (rc) > + return rc; > + } > > rc = device_add(dev); > if (rc) > - goto err; > + return rc; > + > + /* Inhibit the cleanup function invoked */ > + dev = NULL; I'm tempted to say we should use no_free_ptr() here. But I don't think there is any magic we need in there. So. Reviewed-by: Ira Weiny <ira.weiny@intel.com> > + return 0; > +} > + > +static struct cxl_port *__devm_cxl_add_port(struct device *host, > + struct device *uport_dev, > + resource_size_t component_reg_phys, > + struct cxl_dport *parent_dport) > +{ > + struct cxl_port *port; > + int rc; > + > + port = cxl_port_alloc(uport_dev, parent_dport); > + if (IS_ERR(port)) > + return port; > + > + rc = cxl_port_add(port, component_reg_phys, parent_dport); > + if (rc) > + return ERR_PTR(rc); > > rc = devm_add_action_or_reset(host, unregister_port, port); > if (rc) > @@ -891,10 +906,6 @@ static struct cxl_port *__devm_cxl_add_port(struct device *host, > port->pci_latency = cxl_pci_get_latency(to_pci_dev(uport_dev)); > > return port; > - > -err: > - put_device(dev); > - return ERR_PTR(rc); > } > > /** > -- > 2.40.1 >
On 8/22/2024 6:01 AM, Ira Weiny wrote: > Li Ming wrote: >> The "goto error" pattern is not recommended, it can be removed via >> refactoring. In __devm_cxl_add_port(), there is a 'goto' to call >> put_device() for the error cases between device_initialize() and >> device_add() to dereference the 'struct device' of a new cxl_port. >> The refactoring is introducing a new function called cxl_port_add() >> which is used to add the 'struct device' of a new cxl_port to >> device hierarchy, moving the functions needing the help of above >> 'goto' into cxl_port_add(), and using a scope-based resource management >> __free() to drop the open coded put_device() and 'goto' for the error >> cases. >> >> Suggested-by: Dan Williams <dan.j.williams@intel.com> >> Signed-off-by: Li Ming <ming4.li@intel.com> >> --- >> drivers/cxl/core/port.c | 59 ++++++++++++++++++++++++----------------- >> 1 file changed, 35 insertions(+), 24 deletions(-) >> >> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c >> index 53e2593daa95..a886b16b2610 100644 >> --- a/drivers/cxl/core/port.c >> +++ b/drivers/cxl/core/port.c >> @@ -828,27 +828,20 @@ static void cxl_debugfs_create_dport_dir(struct cxl_dport *dport) >> &cxl_einj_inject_fops); >> } >> >> -static struct cxl_port *__devm_cxl_add_port(struct device *host, >> - struct device *uport_dev, >> - resource_size_t component_reg_phys, >> - struct cxl_dport *parent_dport) >> +static int cxl_port_add(struct cxl_port *port, >> + resource_size_t component_reg_phys, >> + struct cxl_dport *parent_dport) >> { >> - struct cxl_port *port; >> - struct device *dev; >> + struct device *dev __free(put_device) = &port->dev; >> int rc; >> >> - port = cxl_port_alloc(uport_dev, parent_dport); >> - if (IS_ERR(port)) >> - return port; >> - >> - dev = &port->dev; >> - if (is_cxl_memdev(uport_dev)) { >> - struct cxl_memdev *cxlmd = to_cxl_memdev(uport_dev); >> + if (is_cxl_memdev(port->uport_dev)) { >> + struct cxl_memdev *cxlmd = to_cxl_memdev(port->uport_dev); >> struct cxl_dev_state *cxlds = cxlmd->cxlds; >> >> rc = dev_set_name(dev, "endpoint%d", port->id); >> if (rc) >> - goto err; >> + return rc; >> >> /* >> * The endpoint driver already enumerated the component and RAS >> @@ -861,19 +854,41 @@ static struct cxl_port *__devm_cxl_add_port(struct device *host, >> } else if (parent_dport) { >> rc = dev_set_name(dev, "port%d", port->id); >> if (rc) >> - goto err; >> + return rc; >> >> rc = cxl_port_setup_regs(port, component_reg_phys); >> if (rc) >> - goto err; >> - } else >> + return rc; >> + } else { >> rc = dev_set_name(dev, "root%d", port->id); >> - if (rc) >> - goto err; >> + if (rc) >> + return rc; >> + } >> >> rc = device_add(dev); >> if (rc) >> - goto err; >> + return rc; >> + >> + /* Inhibit the cleanup function invoked */ >> + dev = NULL; > I'm tempted to say we should use no_free_ptr() here. But I don't think > there is any magic we need in there. Yes, I also think using no_free_ptr() is better. but I have to add a extra local variable to store the return value of no_free_ptr(), otherwise there should be a compilation warning. @@ -870,7 +870,7 @@ static int cxl_port_add(struct cxl_port *port, return rc; /* Inhibit the cleanup function invoked */ - dev = NULL; + no_free_ptr(dev); return 0; } compilation warning: drivers/cxl/core/port.c: In function ‘cxl_port_add’: ./include/linux/cleanup.h:76:22: warning: ignoring return value of ‘__must_check_fn’ declared with attribute ‘warn_unused_result’ [-Wunused-result] 76 | ((typeof(p)) __must_check_fn(__get_and_null_ptr(p))) | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ drivers/cxl/core/port.c:873:9: note: in expansion of macro ‘no_free_ptr’ 873 | no_free_ptr(dev); | ^~~~~~~~~~~ LD [M] drivers/cxl/core/cxl_core.o MODPOST Module.symvers > So. > > Reviewed-by: Ira Weiny <ira.weiny@intel.com> > >> + return 0; >> +} >> + >> +static struct cxl_port *__devm_cxl_add_port(struct device *host, >> + struct device *uport_dev, >> + resource_size_t component_reg_phys, >> + struct cxl_dport *parent_dport) >> +{ >> + struct cxl_port *port; >> + int rc; >> + >> + port = cxl_port_alloc(uport_dev, parent_dport); >> + if (IS_ERR(port)) >> + return port; >> + >> + rc = cxl_port_add(port, component_reg_phys, parent_dport); >> + if (rc) >> + return ERR_PTR(rc); >> >> rc = devm_add_action_or_reset(host, unregister_port, port); >> if (rc) >> @@ -891,10 +906,6 @@ static struct cxl_port *__devm_cxl_add_port(struct device *host, >> port->pci_latency = cxl_pci_get_latency(to_pci_dev(uport_dev)); >> >> return port; >> - >> -err: >> - put_device(dev); >> - return ERR_PTR(rc); >> } >> >> /** >> -- >> 2.40.1 >>
diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c index 53e2593daa95..a886b16b2610 100644 --- a/drivers/cxl/core/port.c +++ b/drivers/cxl/core/port.c @@ -828,27 +828,20 @@ static void cxl_debugfs_create_dport_dir(struct cxl_dport *dport) &cxl_einj_inject_fops); } -static struct cxl_port *__devm_cxl_add_port(struct device *host, - struct device *uport_dev, - resource_size_t component_reg_phys, - struct cxl_dport *parent_dport) +static int cxl_port_add(struct cxl_port *port, + resource_size_t component_reg_phys, + struct cxl_dport *parent_dport) { - struct cxl_port *port; - struct device *dev; + struct device *dev __free(put_device) = &port->dev; int rc; - port = cxl_port_alloc(uport_dev, parent_dport); - if (IS_ERR(port)) - return port; - - dev = &port->dev; - if (is_cxl_memdev(uport_dev)) { - struct cxl_memdev *cxlmd = to_cxl_memdev(uport_dev); + if (is_cxl_memdev(port->uport_dev)) { + struct cxl_memdev *cxlmd = to_cxl_memdev(port->uport_dev); struct cxl_dev_state *cxlds = cxlmd->cxlds; rc = dev_set_name(dev, "endpoint%d", port->id); if (rc) - goto err; + return rc; /* * The endpoint driver already enumerated the component and RAS @@ -861,19 +854,41 @@ static struct cxl_port *__devm_cxl_add_port(struct device *host, } else if (parent_dport) { rc = dev_set_name(dev, "port%d", port->id); if (rc) - goto err; + return rc; rc = cxl_port_setup_regs(port, component_reg_phys); if (rc) - goto err; - } else + return rc; + } else { rc = dev_set_name(dev, "root%d", port->id); - if (rc) - goto err; + if (rc) + return rc; + } rc = device_add(dev); if (rc) - goto err; + return rc; + + /* Inhibit the cleanup function invoked */ + dev = NULL; + return 0; +} + +static struct cxl_port *__devm_cxl_add_port(struct device *host, + struct device *uport_dev, + resource_size_t component_reg_phys, + struct cxl_dport *parent_dport) +{ + struct cxl_port *port; + int rc; + + port = cxl_port_alloc(uport_dev, parent_dport); + if (IS_ERR(port)) + return port; + + rc = cxl_port_add(port, component_reg_phys, parent_dport); + if (rc) + return ERR_PTR(rc); rc = devm_add_action_or_reset(host, unregister_port, port); if (rc) @@ -891,10 +906,6 @@ static struct cxl_port *__devm_cxl_add_port(struct device *host, port->pci_latency = cxl_pci_get_latency(to_pci_dev(uport_dev)); return port; - -err: - put_device(dev); - return ERR_PTR(rc); } /**
The "goto error" pattern is not recommended, it can be removed via refactoring. In __devm_cxl_add_port(), there is a 'goto' to call put_device() for the error cases between device_initialize() and device_add() to dereference the 'struct device' of a new cxl_port. The refactoring is introducing a new function called cxl_port_add() which is used to add the 'struct device' of a new cxl_port to device hierarchy, moving the functions needing the help of above 'goto' into cxl_port_add(), and using a scope-based resource management __free() to drop the open coded put_device() and 'goto' for the error cases. Suggested-by: Dan Williams <dan.j.williams@intel.com> Signed-off-by: Li Ming <ming4.li@intel.com> --- drivers/cxl/core/port.c | 59 ++++++++++++++++++++++++----------------- 1 file changed, 35 insertions(+), 24 deletions(-)