| Message ID | 20230606162829.166226-2-u.kleine-koenig@pengutronix.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: freescale: Convert to platform remove callback returning void | expand |
| Context | Check | Description |
|---|---|---|
| netdev/series_format | success | Posting correctly formatted |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 8 this patch: 8 |
| netdev/cc_maintainers | success | CCed 7 of 7 maintainers |
| netdev/build_clang | success | Errors and warnings before: 8 this patch: 8 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/deprecated_api | success | None detected |
| netdev/check_selftest | success | No net selftest shell script |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 8 this patch: 8 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 16 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
On Tue, Jun 06, 2023 at 06:28:22PM +0200, Uwe Kleine-König wrote: > Instead of the generic error message emitted by the driver core when a > remove callback returns an error code ("remove callback returned a > non-zero value. This will be ignored."), emit a message describing the > actual problem and return zero to suppress the generic message. > > Note that apart from suppressing the generic error message there are no > side effects by changing the return value to zero. This prepares > changing the remove callback to return void. > > Acked-by: Madalin Bucur <madalin.bucur@oss.nxp.com> > Reviewed-by: Michal Kubiak <michal.kubiak@intel.com> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > --- > drivers/net/ethernet/freescale/dpaa/dpaa_eth.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c > index 431f8917dc39..6226c03cfca0 100644 > --- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c > +++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c > @@ -3516,6 +3516,8 @@ static int dpaa_remove(struct platform_device *pdev) > phylink_destroy(priv->mac_dev->phylink); > > err = dpaa_fq_free(dev, &priv->dpaa_fq_list); > + if (err) > + dev_err(dev, "Failed to free FQs on remove\n"); so 'err' is now redundant - if you don't have any value in printing this out then you could remove this variable altogether. > > qman_delete_cgr_safe(&priv->ingress_cgr); > qman_release_cgrid(priv->ingress_cgr.cgrid); > @@ -3528,7 +3530,7 @@ static int dpaa_remove(struct platform_device *pdev) > > free_netdev(net_dev); > > - return err; > + return 0; > } > > static const struct platform_device_id dpaa_devtype[] = { > -- > 2.39.2 > >
On Tue, Jun 06, 2023 at 07:03:31PM +0200, Maciej Fijalkowski wrote: > On Tue, Jun 06, 2023 at 06:28:22PM +0200, Uwe Kleine-König wrote: > > Instead of the generic error message emitted by the driver core when a > > remove callback returns an error code ("remove callback returned a > > non-zero value. This will be ignored."), emit a message describing the > > actual problem and return zero to suppress the generic message. > > > > Note that apart from suppressing the generic error message there are no > > side effects by changing the return value to zero. This prepares > > changing the remove callback to return void. > > > > Acked-by: Madalin Bucur <madalin.bucur@oss.nxp.com> > > Reviewed-by: Michal Kubiak <michal.kubiak@intel.com> > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > --- > > drivers/net/ethernet/freescale/dpaa/dpaa_eth.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c > > index 431f8917dc39..6226c03cfca0 100644 > > --- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c > > +++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c > > @@ -3516,6 +3516,8 @@ static int dpaa_remove(struct platform_device *pdev) > > phylink_destroy(priv->mac_dev->phylink); > > > > err = dpaa_fq_free(dev, &priv->dpaa_fq_list); > > + if (err) > > + dev_err(dev, "Failed to free FQs on remove\n"); > > so 'err' is now redundant - if you don't have any value in printing this > out then you could remove this variable altogether. Good hint, err can have different values here (at least -EINVAL and -EBUSY), so printing it has some values. I'll wait a bit for more feedback to this series and then prepare a v3 with dev_err(dev, "Failed to free FQs on remove (%pE)\n", err); Best regards Uwe
On Wed, Jun 07, 2023 at 08:42:35AM +0200, Uwe Kleine-König wrote: > On Tue, Jun 06, 2023 at 07:03:31PM +0200, Maciej Fijalkowski wrote: > > On Tue, Jun 06, 2023 at 06:28:22PM +0200, Uwe Kleine-König wrote: > > > Instead of the generic error message emitted by the driver core when a > > > remove callback returns an error code ("remove callback returned a > > > non-zero value. This will be ignored."), emit a message describing the > > > actual problem and return zero to suppress the generic message. > > > > > > Note that apart from suppressing the generic error message there are no > > > side effects by changing the return value to zero. This prepares > > > changing the remove callback to return void. > > > > > > Acked-by: Madalin Bucur <madalin.bucur@oss.nxp.com> > > > Reviewed-by: Michal Kubiak <michal.kubiak@intel.com> > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > > --- > > > drivers/net/ethernet/freescale/dpaa/dpaa_eth.c | 4 +++- > > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c > > > index 431f8917dc39..6226c03cfca0 100644 > > > --- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c > > > +++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c > > > @@ -3516,6 +3516,8 @@ static int dpaa_remove(struct platform_device *pdev) > > > phylink_destroy(priv->mac_dev->phylink); > > > > > > err = dpaa_fq_free(dev, &priv->dpaa_fq_list); > > > + if (err) > > > + dev_err(dev, "Failed to free FQs on remove\n"); > > > > so 'err' is now redundant - if you don't have any value in printing this > > out then you could remove this variable altogether. > > Good hint, err can have different values here (at least -EINVAL and > -EBUSY), so printing it has some values. I'll wait a bit for more > feedback to this series and then prepare a v3 with > > dev_err(dev, "Failed to free FQs on remove (%pE)\n", err); ERR_PTR(err) %p formats always take a pointer.
diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c index 431f8917dc39..6226c03cfca0 100644 --- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c +++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c @@ -3516,6 +3516,8 @@ static int dpaa_remove(struct platform_device *pdev) phylink_destroy(priv->mac_dev->phylink); err = dpaa_fq_free(dev, &priv->dpaa_fq_list); + if (err) + dev_err(dev, "Failed to free FQs on remove\n"); qman_delete_cgr_safe(&priv->ingress_cgr); qman_release_cgrid(priv->ingress_cgr.cgrid); @@ -3528,7 +3530,7 @@ static int dpaa_remove(struct platform_device *pdev) free_netdev(net_dev); - return err; + return 0; } static const struct platform_device_id dpaa_devtype[] = {
Instead of the generic error message emitted by the driver core when a remove callback returns an error code ("remove callback returned a non-zero value. This will be ignored."), emit a message describing the actual problem and return zero to suppress the generic message. Note that apart from suppressing the generic error message there are no side effects by changing the return value to zero. This prepares changing the remove callback to return void. Acked-by: Madalin Bucur <madalin.bucur@oss.nxp.com> Reviewed-by: Michal Kubiak <michal.kubiak@intel.com> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> --- drivers/net/ethernet/freescale/dpaa/dpaa_eth.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)