| Message ID | 169657718224.1491153.11083952431204965077.stgit@dwillia2-xfh.jf.intel.com |
|---|---|
| State | Accepted |
| Commit | 2627c995c15dc375f4b5a591d782a14b1c0e3e7d |
| Headers | show |
| Series | cxl/mem: Fix shutdown order | expand |
On Fri, 06 Oct 2023, Dan Williams wrote: >If dev_err_probe() is to be used it should at least be used consistently >within the same function. It is also worth questioning whether >every potential -ENOMEM needs an explicit error message. > >Remove the cxl_setup_fw_upload() error prints for what are rare / >hardware-independent failures. > Reviewed-by: Davidlohr Bueso <dave@stgolabs.net>
Dan Williams wrote: > If dev_err_probe() is to be used it should at least be used consistently > within the same function. It is also worth questioning whether > every potential -ENOMEM needs an explicit error message. > > Remove the cxl_setup_fw_upload() error prints for what are rare / > hardware-independent failures. > > Signed-off-by: Dan Williams <dan.j.williams@intel.com> Seems ok. Reviewed-by: Ira Weiny <ira.weiny@intel.com>
On Fri, 06 Oct 2023 00:26:22 -0700 Dan Williams <dan.j.williams@intel.com> wrote: > If dev_err_probe() is to be used it should at least be used consistently > within the same function. It is also worth questioning whether > every potential -ENOMEM needs an explicit error message. > > Remove the cxl_setup_fw_upload() error prints for what are rare / > hardware-independent failures. > > Signed-off-by: Dan Williams <dan.j.williams@intel.com> Vanishingly unlikely this would ever fail under anything approach a normal situation, so fair enough to drop the error message. Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > --- > drivers/cxl/core/memdev.c | 13 ++----------- > 1 file changed, 2 insertions(+), 11 deletions(-) > > diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c > index 2a7a07f6d165..6efe4e2a2cf5 100644 > --- a/drivers/cxl/core/memdev.c > +++ b/drivers/cxl/core/memdev.c > @@ -970,7 +970,6 @@ int cxl_memdev_setup_fw_upload(struct cxl_memdev_state *mds) > struct cxl_dev_state *cxlds = &mds->cxlds; > struct device *dev = &cxlds->cxlmd->dev; > struct fw_upload *fwl; > - int rc; > > if (!test_bit(CXL_MEM_COMMAND_ID_GET_FW_INFO, mds->enabled_cmds)) > return 0; > @@ -978,17 +977,9 @@ int cxl_memdev_setup_fw_upload(struct cxl_memdev_state *mds) > fwl = firmware_upload_register(THIS_MODULE, dev, dev_name(dev), > &cxl_memdev_fw_ops, mds); > if (IS_ERR(fwl)) > - return dev_err_probe(dev, PTR_ERR(fwl), > - "Failed to register firmware loader\n"); > - > - rc = devm_add_action_or_reset(cxlds->dev, devm_cxl_remove_fw_upload, > - fwl); > - if (rc) > - dev_err(dev, > - "Failed to add firmware loader remove action: %d\n", > - rc); > + return PTR_ERR(fwl); > > - return rc; > + return devm_add_action_or_reset(cxlds->dev, devm_cxl_remove_fw_upload, fwl); > } > EXPORT_SYMBOL_NS_GPL(cxl_memdev_setup_fw_upload, CXL); > > >
On 10/6/23 00:26, Dan Williams wrote: > If dev_err_probe() is to be used it should at least be used consistently > within the same function. It is also worth questioning whether > every potential -ENOMEM needs an explicit error message. > > Remove the cxl_setup_fw_upload() error prints for what are rare / > hardware-independent failures. > > Signed-off-by: Dan Williams <dan.j.williams@intel.com> Reviewed-by: Dave Jiang <dave.jiang@intel.com> > --- > drivers/cxl/core/memdev.c | 13 ++----------- > 1 file changed, 2 insertions(+), 11 deletions(-) > > diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c > index 2a7a07f6d165..6efe4e2a2cf5 100644 > --- a/drivers/cxl/core/memdev.c > +++ b/drivers/cxl/core/memdev.c > @@ -970,7 +970,6 @@ int cxl_memdev_setup_fw_upload(struct cxl_memdev_state *mds) > struct cxl_dev_state *cxlds = &mds->cxlds; > struct device *dev = &cxlds->cxlmd->dev; > struct fw_upload *fwl; > - int rc; > > if (!test_bit(CXL_MEM_COMMAND_ID_GET_FW_INFO, mds->enabled_cmds)) > return 0; > @@ -978,17 +977,9 @@ int cxl_memdev_setup_fw_upload(struct cxl_memdev_state *mds) > fwl = firmware_upload_register(THIS_MODULE, dev, dev_name(dev), > &cxl_memdev_fw_ops, mds); > if (IS_ERR(fwl)) > - return dev_err_probe(dev, PTR_ERR(fwl), > - "Failed to register firmware loader\n"); > - > - rc = devm_add_action_or_reset(cxlds->dev, devm_cxl_remove_fw_upload, > - fwl); > - if (rc) > - dev_err(dev, > - "Failed to add firmware loader remove action: %d\n", > - rc); > + return PTR_ERR(fwl); > > - return rc; > + return devm_add_action_or_reset(cxlds->dev, devm_cxl_remove_fw_upload, fwl); > } > EXPORT_SYMBOL_NS_GPL(cxl_memdev_setup_fw_upload, CXL); > > >
diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c index 2a7a07f6d165..6efe4e2a2cf5 100644 --- a/drivers/cxl/core/memdev.c +++ b/drivers/cxl/core/memdev.c @@ -970,7 +970,6 @@ int cxl_memdev_setup_fw_upload(struct cxl_memdev_state *mds) struct cxl_dev_state *cxlds = &mds->cxlds; struct device *dev = &cxlds->cxlmd->dev; struct fw_upload *fwl; - int rc; if (!test_bit(CXL_MEM_COMMAND_ID_GET_FW_INFO, mds->enabled_cmds)) return 0; @@ -978,17 +977,9 @@ int cxl_memdev_setup_fw_upload(struct cxl_memdev_state *mds) fwl = firmware_upload_register(THIS_MODULE, dev, dev_name(dev), &cxl_memdev_fw_ops, mds); if (IS_ERR(fwl)) - return dev_err_probe(dev, PTR_ERR(fwl), - "Failed to register firmware loader\n"); - - rc = devm_add_action_or_reset(cxlds->dev, devm_cxl_remove_fw_upload, - fwl); - if (rc) - dev_err(dev, - "Failed to add firmware loader remove action: %d\n", - rc); + return PTR_ERR(fwl); - return rc; + return devm_add_action_or_reset(cxlds->dev, devm_cxl_remove_fw_upload, fwl); } EXPORT_SYMBOL_NS_GPL(cxl_memdev_setup_fw_upload, CXL);
If dev_err_probe() is to be used it should at least be used consistently within the same function. It is also worth questioning whether every potential -ENOMEM needs an explicit error message. Remove the cxl_setup_fw_upload() error prints for what are rare / hardware-independent failures. Signed-off-by: Dan Williams <dan.j.williams@intel.com> --- drivers/cxl/core/memdev.c | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-)