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
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 <redacted>
---
drivers/net/ethernet/freescale/fsl_pq_mdio.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
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 <redacted>
---
drivers/net/ethernet/freescale/gianfar.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
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 <redacted>
---
drivers/net/ethernet/freescale/fman/mac.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
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 <redacted>
---
drivers/net/ethernet/freescale/dpaa/dpaa_eth.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
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 <redacted>
---
drivers/net/ethernet/freescale/fec_main.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
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 <redacted>
---
drivers/net/ethernet/freescale/dpaa/dpaa_eth.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
@@ -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);-returnerr;+return0;}staticconststructplatform_device_iddpaa_devtype[]={
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 <redacted>
---
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(-)
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 <redacted>
---
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(-)
@@ -117,7 +117,7 @@ static int mpc52xx_fec_mdio_probe(struct platform_device *of)returnerr;}-staticintmpc52xx_fec_mdio_remove(structplatform_device*of)+staticvoidmpc52xx_fec_mdio_remove(structplatform_device*of){structmii_bus*bus=platform_get_drvdata(of);structmpc52xx_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);--return0;}staticconststructof_device_idmpc52xx_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 */
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 <redacted>
---
drivers/net/ethernet/freescale/ucc_geth.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
-----Original Message-----
From: Uwe Kleine-König <redacted>
Sent: 13 March 2023 12:37
To: Madalin Bucur <madalin.bucur@nxp.com>; David S. Miller
[off-list ref]; Eric Dumazet [off-list ref]; Jakub Kicinski
[off-list ref]; Paolo Abeni [off-list ref]; Russell King
[off-list ref]
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 <redacted>
---
drivers/net/ethernet/freescale/dpaa/dpaa_eth.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
@@ -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 <redacted>
-----Original Message-----
From: Uwe Kleine-König <redacted>
Sent: 13 March 2023 12:37
To: Madalin Bucur <madalin.bucur@nxp.com>; David S. Miller
[off-list ref]; Eric Dumazet [off-list ref]; Jakub Kicinski
[off-list ref]; Paolo Abeni [off-list ref]; Russell King
[off-list ref]; Wei Fang [off-list ref]; Wolfram Sang
[off-list ref]; Chris Packham [off-list ref]; Andy
Shevchenko [off-list ref]; Damien Le Moal
[off-list ref]; Christophe Leroy
[off-list ref]; Michael Ellerman [off-list ref];
Mark Brown [off-list ref]; Marc Kleine-Budde [off-list ref];
Pantelis Antoniou [off-list ref]; Claudiu Manoil
[off-list ref]; Leo Li [off-list ref]
Cc: netdev@vger.kernel.org; kernel@pengutronix.de; Shenwei Wang
[off-list ref]; Clark Wang [off-list ref]; dl-linux-imx
[off-list ref]; 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 <redacted>
From: Andrew Lunn <andrew@lunn.ch> Date: 2023-03-13 15:07:31
On Mon, Mar 13, 2023 at 11:36:46AM +0100, Uwe Kleine-König wrote:
quoted hunk
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 <redacted>
---
drivers/net/ethernet/freescale/fec_main.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
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
From: Michal Kubiak <hidden> Date: 2023-03-13 15:32:06
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 <redacted>
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:
quoted
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 <redacted>
---
drivers/net/ethernet/freescale/fec_main.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
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/ |
From: Andrew Lunn <andrew@lunn.ch> Date: 2023-03-14 00:30:44
quoted
quoted
- 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
On Tue, Mar 14, 2023 at 01:30:35AM +0100, Andrew Lunn wrote:
quoted
quoted
quoted
- 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.
quoted
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/ |
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 <redacted>
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/ |
From: Jakub Kicinski <kuba@kernel.org> Date: 2023-03-15 05:30:04
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 :(
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:
quoted
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/ |