[PATCH net-next 0/9] net: freescale: Convert to platform remove callback returning void

[PATCH net-next 0/9] net: freescale: Convert to platform remove callback returning void

[Uwe Kleine-König]

Hello,

this patch set converts the platform drivers below
drivers/net/ethernet/freescale to the .remove_new() callback. Compared to the
traditional .remove() this one returns void. This is a good thing because the
driver core (mostly) ignores the return value and still removes the device
binding. This is part of a bigger effort to convert all 2000+ platform
drivers to this new callback to eventually change .remove() itself to
return void.

The first two patches here are preparation, the following patches
actually convert the drivers.

Best regards
Uwe

Uwe Kleine-König (9):
  net: dpaa: Improve error reporting
  net: fec: Don't return early on error in .remove()
  net: dpaa: Convert to platform remove callback returning void
  net: fec: Convert to platform remove callback returning void
  net: fman: Convert to platform remove callback returning void
  net: fs_enet: Convert to platform remove callback returning void
  net: fsl_pq_mdio: Convert to platform remove callback returning void
  net: gianfar: Convert to platform remove callback returning void
  net: ucc_geth: Convert to platform remove callback returning void

 drivers/net/ethernet/freescale/dpaa/dpaa_eth.c        |  8 ++++----
 drivers/net/ethernet/freescale/fec_main.c             | 11 ++++-------
 drivers/net/ethernet/freescale/fec_mpc52xx.c          |  6 ++----
 drivers/net/ethernet/freescale/fec_mpc52xx_phy.c      |  6 ++----
 drivers/net/ethernet/freescale/fman/mac.c             |  5 ++---
 drivers/net/ethernet/freescale/fs_enet/fs_enet-main.c |  5 ++---
 drivers/net/ethernet/freescale/fs_enet/mii-bitbang.c  |  6 ++----
 drivers/net/ethernet/freescale/fs_enet/mii-fec.c      |  6 ++----
 drivers/net/ethernet/freescale/fsl_pq_mdio.c          |  6 ++----
 drivers/net/ethernet/freescale/gianfar.c              |  6 ++----
 drivers/net/ethernet/freescale/ucc_geth.c             |  6 ++----
 11 files changed, 26 insertions(+), 45 deletions(-)

base-commit: fe15c26ee26efa11741a7b632e9f23b01aca4cc6
-- 
2.39.1

[PATCH net-next 1/9] net: dpaa: Improve error reporting

[Uwe Kleine-König]

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.

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 9318a2554056..97cad3750096 100644
--- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
+++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
@@ -3520,6 +3520,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(&pdev->dev, "Failed to free FQs on remove\n");
 
 	qman_delete_cgr_safe(&priv->ingress_cgr);
 	qman_release_cgrid(priv->ingress_cgr.cgrid);
@@ -3532,7 +3534,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.1

RE: [PATCH net-next 1/9] net: dpaa: Improve error reporting

[Madalin Bucur]

> -----Original Message-----
> From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Sent: 13 March 2023 12:37
> To: Madalin Bucur <madalin.bucur@nxp.com>; David S. Miller
> <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>; Jakub Kicinski
> <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>; Russell King
> <linux@armlinux.org.uk>
> Cc: netdev@vger.kernel.org; kernel@pengutronix.de
> Subject: [PATCH net-next 1/9] net: dpaa: Improve error reporting
> 
> 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.
> 
> 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 9318a2554056..97cad3750096 100644
> --- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> +++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> @@ -3520,6 +3520,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(&pdev->dev, "Failed to free FQs on remove\n");

You have a bit before dev = &pdev->dev; so you can write just as well:
+		dev_err(dev, "Failed to free FQs on remove\n");

With or without this minor nit pick fixed,

Acked-by: Madalin Bucur <madalin.bucur@oss.nxp.com>

> 
>  	qman_delete_cgr_safe(&priv->ingress_cgr);
>  	qman_release_cgrid(priv->ingress_cgr.cgrid);
> @@ -3532,7 +3534,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.1

[PATCH net-next 2/9] net: fec: Don't return early on error in .remove()

[Uwe Kleine-König]

If waking up the device in .remove() fails, exiting early results in
strange state: The platform device will be unbound but not all resources
are freed. E.g. the network device continues to exist without an parent.

Instead of an early error return, only skip the cleanup that was already
done by suspend and release the remaining resources.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/net/ethernet/freescale/fec_main.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index c73e25f8995e..31d1dc5e9196 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -4465,15 +4465,13 @@ fec_drv_remove(struct platform_device *pdev)
 	struct device_node *np = pdev->dev.of_node;
 	int ret;
 
-	ret = pm_runtime_resume_and_get(&pdev->dev);
-	if (ret < 0)
-		return ret;
+	ret = pm_runtime_get_sync(&pdev->dev);
 
 	cancel_work_sync(&fep->tx_timeout_work);
 	fec_ptp_stop(pdev);
 	unregister_netdev(ndev);
 	fec_enet_mii_remove(fep);
-	if (fep->reg_phy)
+	if (ret >= 0 && fep->reg_phy)
 		regulator_disable(fep->reg_phy);
 
 	if (of_phy_is_fixed_link(np))
-- 
2.39.1

Re: [PATCH net-next 2/9] net: fec: Don't return early on error in .remove()

[Andrew Lunn]

On Mon, Mar 13, 2023 at 11:36:46AM +0100, Uwe Kleine-König wrote:
> If waking up the device in .remove() fails, exiting early results in
> strange state: The platform device will be unbound but not all resources
> are freed. E.g. the network device continues to exist without an parent.
> 
> Instead of an early error return, only skip the cleanup that was already
> done by suspend and release the remaining resources.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/net/ethernet/freescale/fec_main.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
> index c73e25f8995e..31d1dc5e9196 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -4465,15 +4465,13 @@ fec_drv_remove(struct platform_device *pdev)
>  	struct device_node *np = pdev->dev.of_node;
>  	int ret;
>  
> -	ret = pm_runtime_resume_and_get(&pdev->dev);
> -	if (ret < 0)
> -		return ret;
> +	ret = pm_runtime_get_sync(&pdev->dev);
>  
>  	cancel_work_sync(&fep->tx_timeout_work);
>  	fec_ptp_stop(pdev);
>  	unregister_netdev(ndev);
>  	fec_enet_mii_remove(fep);
> -	if (fep->reg_phy)
> +	if (ret >= 0 && fep->reg_phy)
>  		regulator_disable(fep->reg_phy);
>  
>  	if (of_phy_is_fixed_link(np))

I'm not sure this is correct. My experience with the FEC is that if
the device is run time suspended, access to the hardware does not
work. In the case i was debugging, MDIO bus reads/writes time out. I
think IO reads and writes turn into NOPs, but i don't actually know.

So if pm_runtime_resume_and_get() fails, fec_ptp_stop() probably does
not work if it touches the hardware. I guess fec_enet_mii_remove()
unregisters any PHYs, which could cause MDIO bus access to shut down
the PHYs, so i expect that also does not work. regulator_disable()
probably does actually work because that is a different hardware block
unaffected by the suspend.

So i think you need to decide:

exit immediately if resume fails, leaving dangling PHYs, netdev,
regulator etc

Keep going, but maybe everything is going to grind to a halt soon
afterwards when accessing the hardware.

You seem to prefer keep going, so i would also suggest you disable the
regulator.

	   Andrew

Re: [PATCH net-next 2/9] net: fec: Don't return early on error in .remove()

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 3510 bytes --]

Hello Andrew,

On Mon, Mar 13, 2023 at 04:07:12PM +0100, Andrew Lunn wrote:
> On Mon, Mar 13, 2023 at 11:36:46AM +0100, Uwe Kleine-König wrote:
> > If waking up the device in .remove() fails, exiting early results in
> > strange state: The platform device will be unbound but not all resources
> > are freed. E.g. the network device continues to exist without an parent.
> > 
> > Instead of an early error return, only skip the cleanup that was already
> > done by suspend and release the remaining resources.
> > 
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > ---
> >  drivers/net/ethernet/freescale/fec_main.c | 6 ++----
> >  1 file changed, 2 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
> > index c73e25f8995e..31d1dc5e9196 100644
> > --- a/drivers/net/ethernet/freescale/fec_main.c
> > +++ b/drivers/net/ethernet/freescale/fec_main.c
> > @@ -4465,15 +4465,13 @@ fec_drv_remove(struct platform_device *pdev)
> >  	struct device_node *np = pdev->dev.of_node;
> >  	int ret;
> >  
> > -	ret = pm_runtime_resume_and_get(&pdev->dev);
> > -	if (ret < 0)
> > -		return ret;
> > +	ret = pm_runtime_get_sync(&pdev->dev);
> >  
> >  	cancel_work_sync(&fep->tx_timeout_work);
> >  	fec_ptp_stop(pdev);
> >  	unregister_netdev(ndev);
> >  	fec_enet_mii_remove(fep);
> > -	if (fep->reg_phy)
> > +	if (ret >= 0 && fep->reg_phy)
> >  		regulator_disable(fep->reg_phy);
> >  
> >  	if (of_phy_is_fixed_link(np))
> 
> I'm not sure this is correct. My experience with the FEC is that if
> the device is run time suspended, access to the hardware does not
> work. In the case i was debugging, MDIO bus reads/writes time out. I
> think IO reads and writes turn into NOPs, but i don't actually know.
> 
> So if pm_runtime_resume_and_get() fails, fec_ptp_stop() probably does
> not work if it touches the hardware.

While creating the patch I checked, and just from looking at the code
I'd say it doesn't access hardware.

> I guess fec_enet_mii_remove()
> unregisters any PHYs, which could cause MDIO bus access to shut down
> the PHYs, so i expect that also does not work.

fec_enet_mii_remove only calls mdiobus_unregister + mdiobus_free which
should be software only?!

> regulator_disable() probably does actually work because that is a
> different hardware block unaffected by the suspend.

fec_suspend() calls

        if (fep->reg_phy && !(fep->wol_flag & FEC_WOL_FLAG_ENABLE))
                regulator_disable(fep->reg_phy);

and if resume failed I have to assume that fec_resume() didn't come
around reenabling it. So not disabling the regulator in .remove() is
correct.

> So i think you need to decide:
> 
> exit immediately if resume fails, leaving dangling PHYs, netdev,
> regulator etc

I think keeping netdev is very prone to surprises. You'd still have eth0
(or however your device is called), it might even work somewhat, or it
might oops because devm_platform_ioremap_resource is undone.
 
> Keep going, but maybe everything is going to grind to a halt soon
> afterwards when accessing the hardware.
> 
> You seem to prefer keep going, so i would also suggest you disable the
> regulator.

(Described above why I didn't.)

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH net-next 2/9] net: fec: Don't return early on error in .remove()

[Andrew Lunn]

> > > -	ret = pm_runtime_resume_and_get(&pdev->dev);
> > > -	if (ret < 0)
> > > -		return ret;
> > regulator_disable() probably does actually work because that is a
> > different hardware block unaffected by the suspend.
 
> fec_suspend() calls
> 
>         if (fep->reg_phy && !(fep->wol_flag & FEC_WOL_FLAG_ENABLE))
>                 regulator_disable(fep->reg_phy);

There are two different types of suspend here.

pm_runtime_resume_and_get() is about runtime suspend. It calls
fec_runtime_suspend() which just turns some clocks off/on.

fec_suspend() is for system sleep, where the whole system is put to
sleep, except what is needed to trigger a wake up, such as Wake on
LAN. The regulator is being used to power the PHY, so you obviously
don't want to turn the PHY off when doing WoL.

But if you are unloading the FEC, WoL is not going to work, so you
should turn the PHY off. And turning the PHY off should not have any
dependencies on first turning on FEC clocks in
pm_runtime_resume_and_get().

    Andrew

Re: [PATCH net-next 2/9] net: fec: Don't return early on error in .remove()

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1163 bytes --]

On Tue, Mar 14, 2023 at 01:30:35AM +0100, Andrew Lunn wrote:
> > > > -	ret = pm_runtime_resume_and_get(&pdev->dev);
> > > > -	if (ret < 0)
> > > > -		return ret;
> > > regulator_disable() probably does actually work because that is a
> > > different hardware block unaffected by the suspend.
>  
> > fec_suspend() calls
> > 
> >         if (fep->reg_phy && !(fep->wol_flag & FEC_WOL_FLAG_ENABLE))
> >                 regulator_disable(fep->reg_phy);
> 
> There are two different types of suspend here.
> 
> pm_runtime_resume_and_get() is about runtime suspend. It calls
> fec_runtime_suspend() which just turns some clocks off/on.

Oh indeed.

I won't resend this series in this cycle with this issue fixed.
Converting all other drivers to .remove_new() will take quite some time
I guess so there is no pressure. If you apply patches 1, 3, 5 - 9 anyhow
that would be great. (Patch 4 depends on this one.) I'll address the fec
driver at a later point in time.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH net-next 3/9] net: dpaa: Convert to platform remove callback returning void

[Uwe Kleine-König]

The .remove() callback for a platform driver returns an int which makes
many driver authors wrongly assume it's possible to do error handling by
returning an error code. However the value returned is (mostly) ignored
and this typically results in resource leaks. To improve here there is a
quest to make the remove callback return void. In the first step of this
quest all drivers are converted to .remove_new() which already returns
void.

Trivially convert this driver from always returning zero in the remove
callback to the void returning variant.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/net/ethernet/freescale/dpaa/dpaa_eth.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
index 97cad3750096..477db267b95f 100644
--- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
+++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
@@ -3501,7 +3501,7 @@ static int dpaa_eth_probe(struct platform_device *pdev)
 	return err;
 }
 
-static int dpaa_remove(struct platform_device *pdev)
+static void dpaa_remove(struct platform_device *pdev)
 {
 	struct net_device *net_dev;
 	struct dpaa_priv *priv;
@@ -3533,8 +3533,6 @@ static int dpaa_remove(struct platform_device *pdev)
 	dpaa_bps_free(priv);
 
 	free_netdev(net_dev);
-
-	return 0;
 }
 
 static const struct platform_device_id dpaa_devtype[] = {
@@ -3552,7 +3550,7 @@ static struct platform_driver dpaa_driver = {
 	},
 	.id_table = dpaa_devtype,
 	.probe = dpaa_eth_probe,
-	.remove = dpaa_remove
+	.remove_new = dpaa_remove
 };
 
 static int __init dpaa_load(void)
-- 
2.39.1

[PATCH net-next 4/9] net: fec: Convert to platform remove callback returning void

[Uwe Kleine-König]

The .remove() callback for a platform driver returns an int which makes
many driver authors wrongly assume it's possible to do error handling by
returning an error code. However the value returned is (mostly) ignored
and this typically results in resource leaks. To improve here there is a
quest to make the remove callback return void. In the first step of this
quest all drivers are converted to .remove_new() which already returns
void.

Trivially convert this driver from always returning zero in the remove
callback to the void returning variant.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/net/ethernet/freescale/fec_main.c        | 5 ++---
 drivers/net/ethernet/freescale/fec_mpc52xx.c     | 6 ++----
 drivers/net/ethernet/freescale/fec_mpc52xx_phy.c | 6 ++----
 3 files changed, 6 insertions(+), 11 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 31d1dc5e9196..2725feb6fc02 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -4457,7 +4457,7 @@ fec_probe(struct platform_device *pdev)
 	return ret;
 }
 
-static int
+static void
 fec_drv_remove(struct platform_device *pdev)
 {
 	struct net_device *ndev = platform_get_drvdata(pdev);
@@ -4484,7 +4484,6 @@ fec_drv_remove(struct platform_device *pdev)
 	pm_runtime_disable(&pdev->dev);
 
 	free_netdev(ndev);
-	return 0;
 }
 
 static int __maybe_unused fec_suspend(struct device *dev)
@@ -4640,7 +4639,7 @@ static struct platform_driver fec_driver = {
 	},
 	.id_table = fec_devtype,
 	.probe	= fec_probe,
-	.remove	= fec_drv_remove,
+	.remove_new = fec_drv_remove,
 };
 
 module_platform_driver(fec_driver);
diff --git a/drivers/net/ethernet/freescale/fec_mpc52xx.c b/drivers/net/ethernet/freescale/fec_mpc52xx.c
index a7f4c3c29f3e..b61fa27e1592 100644
--- a/drivers/net/ethernet/freescale/fec_mpc52xx.c
+++ b/drivers/net/ethernet/freescale/fec_mpc52xx.c
@@ -974,7 +974,7 @@ static int mpc52xx_fec_probe(struct platform_device *op)
 	return rv;
 }
 
-static int
+static void
 mpc52xx_fec_remove(struct platform_device *op)
 {
 	struct net_device *ndev;
@@ -998,8 +998,6 @@ mpc52xx_fec_remove(struct platform_device *op)
 	release_mem_region(ndev->base_addr, sizeof(struct mpc52xx_fec));
 
 	free_netdev(ndev);
-
-	return 0;
 }
 
 #ifdef CONFIG_PM
@@ -1042,7 +1040,7 @@ static struct platform_driver mpc52xx_fec_driver = {
 		.of_match_table = mpc52xx_fec_match,
 	},
 	.probe		= mpc52xx_fec_probe,
-	.remove		= mpc52xx_fec_remove,
+	.remove_new	= mpc52xx_fec_remove,
 #ifdef CONFIG_PM
 	.suspend	= mpc52xx_fec_of_suspend,
 	.resume		= mpc52xx_fec_of_resume,
diff --git a/drivers/net/ethernet/freescale/fec_mpc52xx_phy.c b/drivers/net/ethernet/freescale/fec_mpc52xx_phy.c
index 95f778cce98c..61d3776d6750 100644
--- a/drivers/net/ethernet/freescale/fec_mpc52xx_phy.c
+++ b/drivers/net/ethernet/freescale/fec_mpc52xx_phy.c
@@ -117,7 +117,7 @@ static int mpc52xx_fec_mdio_probe(struct platform_device *of)
 	return err;
 }
 
-static int mpc52xx_fec_mdio_remove(struct platform_device *of)
+static void mpc52xx_fec_mdio_remove(struct platform_device *of)
 {
 	struct mii_bus *bus = platform_get_drvdata(of);
 	struct mpc52xx_fec_mdio_priv *priv = bus->priv;
@@ -126,8 +126,6 @@ static int mpc52xx_fec_mdio_remove(struct platform_device *of)
 	iounmap(priv->regs);
 	kfree(priv);
 	mdiobus_free(bus);
-
-	return 0;
 }
 
 static const struct of_device_id mpc52xx_fec_mdio_match[] = {
@@ -145,7 +143,7 @@ struct platform_driver mpc52xx_fec_mdio_driver = {
 		.of_match_table = mpc52xx_fec_mdio_match,
 	},
 	.probe = mpc52xx_fec_mdio_probe,
-	.remove = mpc52xx_fec_mdio_remove,
+	.remove_new = mpc52xx_fec_mdio_remove,
 };
 
 /* let fec driver call it, since this has to be registered before it */
-- 
2.39.1

Re: [PATCH net-next 4/9] net: fec: Convert to platform remove callback returning void

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1110 bytes --]

Hello,

On Mon, Mar 13, 2023 at 11:36:48AM +0100, Uwe Kleine-König wrote:
> The .remove() callback for a platform driver returns an int which makes
> many driver authors wrongly assume it's possible to do error handling by
> returning an error code. However the value returned is (mostly) ignored
> and this typically results in resource leaks. To improve here there is a
> quest to make the remove callback return void. In the first step of this
> quest all drivers are converted to .remove_new() which already returns
> void.
> 
> Trivially convert this driver from always returning zero in the remove
> callback to the void returning variant.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

FTR: This patch depends on patch 2 of this series which has issues. So
please drop this patch, too. Taking the other 7 patches should be fine
(unless some more issues are discovered of course).

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH net-next 4/9] net: fec: Convert to platform remove callback returning void

[Jakub Kicinski]

On Tue, 14 Mar 2023 23:15:08 +0100 Uwe Kleine-König wrote:
> FTR: This patch depends on patch 2 of this series which has issues. So
> please drop this patch, too. Taking the other 7 patches should be fine
> (unless some more issues are discovered of course).

Could you post a v2 with just the right patches in it?
Would be quicker for us to handle and we're drowning in patches ATM :(

Re: [PATCH net-next 4/9] net: fec: Convert to platform remove callback returning void

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1055 bytes --]

Hello Jakub,

On Tue, Mar 14, 2023 at 10:28:21PM -0700, Jakub Kicinski wrote:
> On Tue, 14 Mar 2023 23:15:08 +0100 Uwe Kleine-König wrote:
> > FTR: This patch depends on patch 2 of this series which has issues. So
> > please drop this patch, too. Taking the other 7 patches should be fine
> > (unless some more issues are discovered of course).
> 
> Could you post a v2 with just the right patches in it?
> Would be quicker for us to handle and we're drowning in patches ATM :(

That approximately matches my plan. I didn't intend to resend the
patches that were not criticised if you pick them up. I have still 2000+
patches of this type in my queue and intend to care for the rejects when
I'm through sending them all once. When I rebase then I can easily see
which drivers need some more care. I expect that won't happen in the
current development cycle.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH net-next 5/9] net: fman: Convert to platform remove callback returning void

[Uwe Kleine-König]

The .remove() callback for a platform driver returns an int which makes
many driver authors wrongly assume it's possible to do error handling by
returning an error code. However the value returned is (mostly) ignored
and this typically results in resource leaks. To improve here there is a
quest to make the remove callback return void. In the first step of this
quest all drivers are converted to .remove_new() which already returns
void.

Trivially convert this driver from always returning zero in the remove
callback to the void returning variant.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/net/ethernet/freescale/fman/mac.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fman/mac.c b/drivers/net/ethernet/freescale/fman/mac.c
index 43665806c590..c5045891d694 100644
--- a/drivers/net/ethernet/freescale/fman/mac.c
+++ b/drivers/net/ethernet/freescale/fman/mac.c
@@ -331,12 +331,11 @@ static int mac_probe(struct platform_device *_of_dev)
 	return err;
 }
 
-static int mac_remove(struct platform_device *pdev)
+static void mac_remove(struct platform_device *pdev)
 {
 	struct mac_device *mac_dev = platform_get_drvdata(pdev);
 
 	platform_device_unregister(mac_dev->priv->eth_dev);
-	return 0;
 }
 
 static struct platform_driver mac_driver = {
@@ -345,7 +344,7 @@ static struct platform_driver mac_driver = {
 		.of_match_table	= mac_match,
 	},
 	.probe		= mac_probe,
-	.remove		= mac_remove,
+	.remove_new	= mac_remove,
 };
 
 builtin_platform_driver(mac_driver);
-- 
2.39.1

[PATCH net-next 6/9] net: fs_enet: Convert to platform remove callback returning void

[Uwe Kleine-König]

The .remove() callback for a platform driver returns an int which makes
many driver authors wrongly assume it's possible to do error handling by
returning an error code. However the value returned is (mostly) ignored
and this typically results in resource leaks. To improve here there is a
quest to make the remove callback return void. In the first step of this
quest all drivers are converted to .remove_new() which already returns
void.

Trivially convert this driver from always returning zero in the remove
callback to the void returning variant.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/net/ethernet/freescale/fs_enet/fs_enet-main.c | 5 ++---
 drivers/net/ethernet/freescale/fs_enet/mii-bitbang.c  | 6 ++----
 drivers/net/ethernet/freescale/fs_enet/mii-fec.c      | 6 ++----
 3 files changed, 6 insertions(+), 11 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fs_enet/fs_enet-main.c b/drivers/net/ethernet/freescale/fs_enet/fs_enet-main.c
index 8844a9a04fcf..f9f5b28cc72e 100644
--- a/drivers/net/ethernet/freescale/fs_enet/fs_enet-main.c
+++ b/drivers/net/ethernet/freescale/fs_enet/fs_enet-main.c
@@ -1051,7 +1051,7 @@ static int fs_enet_probe(struct platform_device *ofdev)
 	return ret;
 }
 
-static int fs_enet_remove(struct platform_device *ofdev)
+static void fs_enet_remove(struct platform_device *ofdev)
 {
 	struct net_device *ndev = platform_get_drvdata(ofdev);
 	struct fs_enet_private *fep = netdev_priv(ndev);
@@ -1066,7 +1066,6 @@ static int fs_enet_remove(struct platform_device *ofdev)
 	if (of_phy_is_fixed_link(ofdev->dev.of_node))
 		of_phy_deregister_fixed_link(ofdev->dev.of_node);
 	free_netdev(ndev);
-	return 0;
 }
 
 static const struct of_device_id fs_enet_match[] = {
@@ -1113,7 +1112,7 @@ static struct platform_driver fs_enet_driver = {
 		.of_match_table = fs_enet_match,
 	},
 	.probe = fs_enet_probe,
-	.remove = fs_enet_remove,
+	.remove_new = fs_enet_remove,
 };
 
 #ifdef CONFIG_NET_POLL_CONTROLLER
diff --git a/drivers/net/ethernet/freescale/fs_enet/mii-bitbang.c b/drivers/net/ethernet/freescale/fs_enet/mii-bitbang.c
index 21de56345503..91a69fc2f7c2 100644
--- a/drivers/net/ethernet/freescale/fs_enet/mii-bitbang.c
+++ b/drivers/net/ethernet/freescale/fs_enet/mii-bitbang.c
@@ -192,7 +192,7 @@ static int fs_enet_mdio_probe(struct platform_device *ofdev)
 	return ret;
 }
 
-static int fs_enet_mdio_remove(struct platform_device *ofdev)
+static void fs_enet_mdio_remove(struct platform_device *ofdev)
 {
 	struct mii_bus *bus = platform_get_drvdata(ofdev);
 	struct bb_info *bitbang = bus->priv;
@@ -201,8 +201,6 @@ static int fs_enet_mdio_remove(struct platform_device *ofdev)
 	free_mdio_bitbang(bus);
 	iounmap(bitbang->dir);
 	kfree(bitbang);
-
-	return 0;
 }
 
 static const struct of_device_id fs_enet_mdio_bb_match[] = {
@@ -219,7 +217,7 @@ static struct platform_driver fs_enet_bb_mdio_driver = {
 		.of_match_table = fs_enet_mdio_bb_match,
 	},
 	.probe = fs_enet_mdio_probe,
-	.remove = fs_enet_mdio_remove,
+	.remove_new = fs_enet_mdio_remove,
 };
 
 module_platform_driver(fs_enet_bb_mdio_driver);
diff --git a/drivers/net/ethernet/freescale/fs_enet/mii-fec.c b/drivers/net/ethernet/freescale/fs_enet/mii-fec.c
index d37d7a19a759..94bd76c6cf9e 100644
--- a/drivers/net/ethernet/freescale/fs_enet/mii-fec.c
+++ b/drivers/net/ethernet/freescale/fs_enet/mii-fec.c
@@ -187,7 +187,7 @@ static int fs_enet_mdio_probe(struct platform_device *ofdev)
 	return ret;
 }
 
-static int fs_enet_mdio_remove(struct platform_device *ofdev)
+static void fs_enet_mdio_remove(struct platform_device *ofdev)
 {
 	struct mii_bus *bus = platform_get_drvdata(ofdev);
 	struct fec_info *fec = bus->priv;
@@ -196,8 +196,6 @@ static int fs_enet_mdio_remove(struct platform_device *ofdev)
 	iounmap(fec->fecp);
 	kfree(fec);
 	mdiobus_free(bus);
-
-	return 0;
 }
 
 static const struct of_device_id fs_enet_mdio_fec_match[] = {
@@ -220,7 +218,7 @@ static struct platform_driver fs_enet_fec_mdio_driver = {
 		.of_match_table = fs_enet_mdio_fec_match,
 	},
 	.probe = fs_enet_mdio_probe,
-	.remove = fs_enet_mdio_remove,
+	.remove_new = fs_enet_mdio_remove,
 };
 
 module_platform_driver(fs_enet_fec_mdio_driver);
-- 
2.39.1

[PATCH net-next 7/9] net: fsl_pq_mdio: Convert to platform remove callback returning void

[Uwe Kleine-König]

The .remove() callback for a platform driver returns an int which makes
many driver authors wrongly assume it's possible to do error handling by
returning an error code. However the value returned is (mostly) ignored
and this typically results in resource leaks. To improve here there is a
quest to make the remove callback return void. In the first step of this
quest all drivers are converted to .remove_new() which already returns
void.

Trivially convert this driver from always returning zero in the remove
callback to the void returning variant.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/net/ethernet/freescale/fsl_pq_mdio.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fsl_pq_mdio.c b/drivers/net/ethernet/freescale/fsl_pq_mdio.c
index 9d58d8334467..01d594886f52 100644
--- a/drivers/net/ethernet/freescale/fsl_pq_mdio.c
+++ b/drivers/net/ethernet/freescale/fsl_pq_mdio.c
@@ -511,7 +511,7 @@ static int fsl_pq_mdio_probe(struct platform_device *pdev)
 }
 
 
-static int fsl_pq_mdio_remove(struct platform_device *pdev)
+static void fsl_pq_mdio_remove(struct platform_device *pdev)
 {
 	struct device *device = &pdev->dev;
 	struct mii_bus *bus = dev_get_drvdata(device);
@@ -521,8 +521,6 @@ static int fsl_pq_mdio_remove(struct platform_device *pdev)
 
 	iounmap(priv->map);
 	mdiobus_free(bus);
-
-	return 0;
 }
 
 static struct platform_driver fsl_pq_mdio_driver = {
@@ -531,7 +529,7 @@ static struct platform_driver fsl_pq_mdio_driver = {
 		.of_match_table = fsl_pq_mdio_match,
 	},
 	.probe = fsl_pq_mdio_probe,
-	.remove = fsl_pq_mdio_remove,
+	.remove_new = fsl_pq_mdio_remove,
 };
 
 module_platform_driver(fsl_pq_mdio_driver);
-- 
2.39.1

[PATCH net-next 8/9] net: gianfar: Convert to platform remove callback returning void

[Uwe Kleine-König]

The .remove() callback for a platform driver returns an int which makes
many driver authors wrongly assume it's possible to do error handling by
returning an error code. However the value returned is (mostly) ignored
and this typically results in resource leaks. To improve here there is a
quest to make the remove callback return void. In the first step of this
quest all drivers are converted to .remove_new() which already returns
void.

Trivially convert this driver from always returning zero in the remove
callback to the void returning variant.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/net/ethernet/freescale/gianfar.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c
index b2def295523a..2f578ad7788d 100644
--- a/drivers/net/ethernet/freescale/gianfar.c
+++ b/drivers/net/ethernet/freescale/gianfar.c
@@ -3364,7 +3364,7 @@ static int gfar_probe(struct platform_device *ofdev)
 	return err;
 }
 
-static int gfar_remove(struct platform_device *ofdev)
+static void gfar_remove(struct platform_device *ofdev)
 {
 	struct gfar_private *priv = platform_get_drvdata(ofdev);
 	struct device_node *np = ofdev->dev.of_node;
@@ -3381,8 +3381,6 @@ static int gfar_remove(struct platform_device *ofdev)
 	gfar_free_rx_queues(priv);
 	gfar_free_tx_queues(priv);
 	free_gfar_dev(priv);
-
-	return 0;
 }
 
 #ifdef CONFIG_PM
@@ -3642,7 +3640,7 @@ static struct platform_driver gfar_driver = {
 		.of_match_table = gfar_match,
 	},
 	.probe = gfar_probe,
-	.remove = gfar_remove,
+	.remove_new = gfar_remove,
 };
 
 module_platform_driver(gfar_driver);
-- 
2.39.1

[PATCH net-next 9/9] net: ucc_geth: Convert to platform remove callback returning void

[Uwe Kleine-König]

The .remove() callback for a platform driver returns an int which makes
many driver authors wrongly assume it's possible to do error handling by
returning an error code. However the value returned is (mostly) ignored
and this typically results in resource leaks. To improve here there is a
quest to make the remove callback return void. In the first step of this
quest all drivers are converted to .remove_new() which already returns
void.

Trivially convert this driver from always returning zero in the remove
callback to the void returning variant.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/net/ethernet/freescale/ucc_geth.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/freescale/ucc_geth.c b/drivers/net/ethernet/freescale/ucc_geth.c
index 7a4cb4f07c32..2b3a15f24e7c 100644
--- a/drivers/net/ethernet/freescale/ucc_geth.c
+++ b/drivers/net/ethernet/freescale/ucc_geth.c
@@ -3753,7 +3753,7 @@ static int ucc_geth_probe(struct platform_device* ofdev)
 	return err;
 }
 
-static int ucc_geth_remove(struct platform_device* ofdev)
+static void ucc_geth_remove(struct platform_device* ofdev)
 {
 	struct net_device *dev = platform_get_drvdata(ofdev);
 	struct ucc_geth_private *ugeth = netdev_priv(dev);
@@ -3767,8 +3767,6 @@ static int ucc_geth_remove(struct platform_device* ofdev)
 	of_node_put(ugeth->ug_info->phy_node);
 	kfree(ugeth->ug_info);
 	free_netdev(dev);
-
-	return 0;
 }
 
 static const struct of_device_id ucc_geth_match[] = {
@@ -3787,7 +3785,7 @@ static struct platform_driver ucc_geth_driver = {
 		.of_match_table = ucc_geth_match,
 	},
 	.probe		= ucc_geth_probe,
-	.remove		= ucc_geth_remove,
+	.remove_new	= ucc_geth_remove,
 	.suspend	= ucc_geth_suspend,
 	.resume		= ucc_geth_resume,
 };
-- 
2.39.1

RE: [PATCH net-next 0/9] net: freescale: Convert to platform remove callback returning void

[Madalin Bucur]

> -----Original Message-----
> From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Sent: 13 March 2023 12:37
> To: Madalin Bucur <madalin.bucur@nxp.com>; David S. Miller
> <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>; Jakub Kicinski
> <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>; Russell King
> <linux@armlinux.org.uk>; Wei Fang <wei.fang@nxp.com>; Wolfram Sang
> <wsa@kernel.org>; Chris Packham <chris.packham@alliedtelesis.co.nz>; Andy
> Shevchenko <andriy.shevchenko@linux.intel.com>; Damien Le Moal
> <damien.lemoal@opensource.wdc.com>; Christophe Leroy
> <christophe.leroy@csgroup.eu>; Michael Ellerman <mpe@ellerman.id.au>;
> Mark Brown <broonie@kernel.org>; Marc Kleine-Budde <mkl@pengutronix.de>;
> Pantelis Antoniou <pantelis.antoniou@gmail.com>; Claudiu Manoil
> <claudiu.manoil@nxp.com>; Leo Li <leoyang.li@nxp.com>
> Cc: netdev@vger.kernel.org; kernel@pengutronix.de; Shenwei Wang
> <shenwei.wang@nxp.com>; Clark Wang <xiaoning.wang@nxp.com>; dl-linux-imx
> <linux-imx@nxp.com>; linuxppc-dev@lists.ozlabs.org
> Subject: [PATCH net-next 0/9] net: freescale: Convert to platform remove
> callback returning void
> 
> Hello,
> 
> this patch set converts the platform drivers below
> drivers/net/ethernet/freescale to the .remove_new() callback. Compared to
> the
> traditional .remove() this one returns void. This is a good thing because
> the
> driver core (mostly) ignores the return value and still removes the
> device
> binding. This is part of a bigger effort to convert all 2000+ platform
> drivers to this new callback to eventually change .remove() itself to
> return void.
> 
> The first two patches here are preparation, the following patches
> actually convert the drivers.
> 
> Best regards
> Uwe
> 
> Uwe Kleine-König (9):
>   net: dpaa: Improve error reporting
>   net: fec: Don't return early on error in .remove()
>   net: dpaa: Convert to platform remove callback returning void
>   net: fec: Convert to platform remove callback returning void
>   net: fman: Convert to platform remove callback returning void
>   net: fs_enet: Convert to platform remove callback returning void
>   net: fsl_pq_mdio: Convert to platform remove callback returning void
>   net: gianfar: Convert to platform remove callback returning void
>   net: ucc_geth: Convert to platform remove callback returning void
> 
>  drivers/net/ethernet/freescale/dpaa/dpaa_eth.c        |  8 ++++----
>  drivers/net/ethernet/freescale/fec_main.c             | 11 ++++-------
>  drivers/net/ethernet/freescale/fec_mpc52xx.c          |  6 ++----
>  drivers/net/ethernet/freescale/fec_mpc52xx_phy.c      |  6 ++----
>  drivers/net/ethernet/freescale/fman/mac.c             |  5 ++---
>  drivers/net/ethernet/freescale/fs_enet/fs_enet-main.c |  5 ++---
>  drivers/net/ethernet/freescale/fs_enet/mii-bitbang.c  |  6 ++----
>  drivers/net/ethernet/freescale/fs_enet/mii-fec.c      |  6 ++----
>  drivers/net/ethernet/freescale/fsl_pq_mdio.c          |  6 ++----
>  drivers/net/ethernet/freescale/gianfar.c              |  6 ++----
>  drivers/net/ethernet/freescale/ucc_geth.c             |  6 ++----
>  11 files changed, 26 insertions(+), 45 deletions(-)
> 
> base-commit: fe15c26ee26efa11741a7b632e9f23b01aca4cc6
> --
> 2.39.1

For the FMan and DPAA drivers,

Acked-by: Madalin Bucur <madalin.bucur@oss.nxp.com>

Re: [PATCH net-next 0/9] net: freescale: Convert to platform remove callback returning void

[Michal Kubiak]

On Mon, Mar 13, 2023 at 11:36:44AM +0100, Uwe Kleine-König wrote:
> Hello,
> 
> this patch set converts the platform drivers below
> drivers/net/ethernet/freescale to the .remove_new() callback. Compared to the
> traditional .remove() this one returns void. This is a good thing because the
> driver core (mostly) ignores the return value and still removes the device
> binding. This is part of a bigger effort to convert all 2000+ platform
> drivers to this new callback to eventually change .remove() itself to
> return void.
> 
> The first two patches here are preparation, the following patches
> actually convert the drivers.
> 
> Best regards
> Uwe
> 

For entire series:
Reviewed-by: Michal Kubiak <michal.kubiak@intel.com>

> Uwe Kleine-König (9):
>   net: dpaa: Improve error reporting
>   net: fec: Don't return early on error in .remove()
>   net: dpaa: Convert to platform remove callback returning void
>   net: fec: Convert to platform remove callback returning void
>   net: fman: Convert to platform remove callback returning void
>   net: fs_enet: Convert to platform remove callback returning void
>   net: fsl_pq_mdio: Convert to platform remove callback returning void
>   net: gianfar: Convert to platform remove callback returning void
>   net: ucc_geth: Convert to platform remove callback returning void
> 
>  drivers/net/ethernet/freescale/dpaa/dpaa_eth.c        |  8 ++++----
>  drivers/net/ethernet/freescale/fec_main.c             | 11 ++++-------
>  drivers/net/ethernet/freescale/fec_mpc52xx.c          |  6 ++----
>  drivers/net/ethernet/freescale/fec_mpc52xx_phy.c      |  6 ++----
>  drivers/net/ethernet/freescale/fman/mac.c             |  5 ++---
>  drivers/net/ethernet/freescale/fs_enet/fs_enet-main.c |  5 ++---
>  drivers/net/ethernet/freescale/fs_enet/mii-bitbang.c  |  6 ++----
>  drivers/net/ethernet/freescale/fs_enet/mii-fec.c      |  6 ++----
>  drivers/net/ethernet/freescale/fsl_pq_mdio.c          |  6 ++----
>  drivers/net/ethernet/freescale/gianfar.c              |  6 ++----
>  drivers/net/ethernet/freescale/ucc_geth.c             |  6 ++----
>  11 files changed, 26 insertions(+), 45 deletions(-)
> 
> base-commit: fe15c26ee26efa11741a7b632e9f23b01aca4cc6
> -- 
> 2.39.1
>