[PATCH net-next 00/10] net*: Convert to platform remove callback returning void

[PATCH net-next 00/10] net*: Convert to platform remove callback returning void

[Uwe Kleine-König]

Hello,

this series converts the platform drivers below drivers/net that are not
covered in the two other series converting drivers/net/ethernet and
drivers/net/wireless. I put them all in a single series even though they
are not maintained together. I thought that to be better than sending
them out individually, I hope you agree.

See commit 5c5a7680e67b ("platform: Provide a remove callback that
returns no value") for an extended explanation and the eventual goal.
The TL;DR; is to make it harder for driver authors to leak resources
without noticing.

The first patch is a fix, but I don't think it's worth to add that to
stable, it was broken since v5.7-rc1 and nobody seems to have hit the
problem.

Best regards
Uwe

Uwe Kleine-König (10):
  net: ipa: Don't error out in .remove()
  net: ipa: Convert to platform remove callback returning void
  net: fjes: Convert to platform remove callback returning void
  net: pcs: rzn1-miic: Convert to platform remove callback returning
    void
  net: sfp: Convert to platform remove callback returning void
  net: wan/fsl_ucc_hdlc: Convert to platform remove callback returning
    void
  net: wan/ixp4xx_hss: Convert to platform remove callback returning
    void
  net: wwan: qcom_bam_dmux: Convert to platform remove callback
    returning void
  ieee802154: fakelb: Convert to platform remove callback returning void
  ieee802154: hwsim: Convert to platform remove callback returning void

 drivers/net/fjes/fjes_main.c             |  6 ++----
 drivers/net/ieee802154/fakelb.c          |  5 ++---
 drivers/net/ieee802154/mac802154_hwsim.c |  6 ++----
 drivers/net/ipa/ipa_main.c               | 20 +++++---------------
 drivers/net/pcs/pcs-rzn1-miic.c          |  6 ++----
 drivers/net/phy/sfp.c                    |  6 ++----
 drivers/net/wan/fsl_ucc_hdlc.c           |  6 ++----
 drivers/net/wan/ixp4xx_hss.c             |  5 ++---
 drivers/net/wwan/qcom_bam_dmux.c         |  6 ++----
 9 files changed, 21 insertions(+), 45 deletions(-)

base-commit: eff99d8edbed7918317331ebd1e365d8e955d65e
-- 
2.42.0

[PATCH net-next 01/10] net: ipa: Don't error out in .remove()

[Uwe Kleine-König]

Returning early from .remove() with an error code still results in the
driver unbinding the device. So the driver core ignores the returned error
code and the resources that were not freed are never catched up. In
combination with devm this also often results in use-after-free bugs.

Here even if the modem cannot be stopped, resources must be freed. So
replace the early error return by an error message an continue to clean up.

This prepares changing ipa_remove() to return void.

Fixes: cdf2e9419dd9 ("soc: qcom: ipa: main code")
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/net/ipa/ipa_main.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ipa/ipa_main.c b/drivers/net/ipa/ipa_main.c
index da853353a5c7..60e4f590f5de 100644
--- a/drivers/net/ipa/ipa_main.c
+++ b/drivers/net/ipa/ipa_main.c
@@ -960,7 +960,8 @@ static int ipa_remove(struct platform_device *pdev)
 			ret = ipa_modem_stop(ipa);
 		}
 		if (ret)
-			return ret;
+			dev_err(dev, "Failed to stop modem (%pe)\n",
+				ERR_PTR(ret));
 
 		ipa_teardown(ipa);
 	}
-- 
2.42.0

[PATCH net-next 02/10] net: ipa: 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 ignored (apart
from emitting a warning) 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. Eventually after all drivers
are converted, .remove_new() will be renamed to .remove().

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/ipa/ipa_main.c | 17 +++--------------
 1 file changed, 3 insertions(+), 14 deletions(-)

diff --git a/drivers/net/ipa/ipa_main.c b/drivers/net/ipa/ipa_main.c
index 60e4f590f5de..2c769b85a2cd 100644
--- a/drivers/net/ipa/ipa_main.c
+++ b/drivers/net/ipa/ipa_main.c
@@ -936,7 +936,7 @@ static int ipa_probe(struct platform_device *pdev)
 	return ret;
 }
 
-static int ipa_remove(struct platform_device *pdev)
+static void ipa_remove(struct platform_device *pdev)
 {
 	struct ipa *ipa = dev_get_drvdata(&pdev->dev);
 	struct ipa_power *power = ipa->power;
@@ -979,17 +979,6 @@ static int ipa_remove(struct platform_device *pdev)
 	ipa_power_exit(power);
 
 	dev_info(dev, "IPA driver removed");
-
-	return 0;
-}
-
-static void ipa_shutdown(struct platform_device *pdev)
-{
-	int ret;
-
-	ret = ipa_remove(pdev);
-	if (ret)
-		dev_err(&pdev->dev, "shutdown: remove returned %d\n", ret);
 }
 
 static const struct attribute_group *ipa_attribute_groups[] = {
@@ -1002,8 +991,8 @@ static const struct attribute_group *ipa_attribute_groups[] = {
 
 static struct platform_driver ipa_driver = {
 	.probe		= ipa_probe,
-	.remove		= ipa_remove,
-	.shutdown	= ipa_shutdown,
+	.remove_new	= ipa_remove,
+	.shutdown	= ipa_remove,
 	.driver	= {
 		.name		= "ipa",
 		.pm		= &ipa_pm_ops,
-- 
2.42.0

[PATCH net-next 03/10] net: fjes: 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 ignored (apart
from emitting a warning) 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. Eventually after all drivers
are converted, .remove_new() will be renamed to .remove().

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/fjes/fjes_main.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/net/fjes/fjes_main.c b/drivers/net/fjes/fjes_main.c
index cd8cf08477ec..5fbe33a09bb0 100644
--- a/drivers/net/fjes/fjes_main.c
+++ b/drivers/net/fjes/fjes_main.c
@@ -1438,7 +1438,7 @@ static int fjes_probe(struct platform_device *plat_dev)
 }
 
 /* fjes_remove - Device Removal Routine */
-static int fjes_remove(struct platform_device *plat_dev)
+static void fjes_remove(struct platform_device *plat_dev)
 {
 	struct net_device *netdev = dev_get_drvdata(&plat_dev->dev);
 	struct fjes_adapter *adapter = netdev_priv(netdev);
@@ -1462,8 +1462,6 @@ static int fjes_remove(struct platform_device *plat_dev)
 	netif_napi_del(&adapter->napi);
 
 	free_netdev(netdev);
-
-	return 0;
 }
 
 static struct platform_driver fjes_driver = {
@@ -1471,7 +1469,7 @@ static struct platform_driver fjes_driver = {
 		.name = DRV_NAME,
 	},
 	.probe = fjes_probe,
-	.remove = fjes_remove,
+	.remove_new = fjes_remove,
 };
 
 static acpi_status
-- 
2.42.0

[PATCH net-next 04/10] net: pcs: rzn1-miic: 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 ignored (apart
from emitting a warning) 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. Eventually after all drivers
are converted, .remove_new() will be renamed to .remove().

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/pcs/pcs-rzn1-miic.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/net/pcs/pcs-rzn1-miic.c b/drivers/net/pcs/pcs-rzn1-miic.c
index 97139c07130f..d93f84fbb1fd 100644
--- a/drivers/net/pcs/pcs-rzn1-miic.c
+++ b/drivers/net/pcs/pcs-rzn1-miic.c
@@ -505,11 +505,9 @@ static int miic_probe(struct platform_device *pdev)
 	return ret;
 }
 
-static int miic_remove(struct platform_device *pdev)
+static void miic_remove(struct platform_device *pdev)
 {
 	pm_runtime_put(&pdev->dev);
-
-	return 0;
 }
 
 static const struct of_device_id miic_of_mtable[] = {
@@ -525,7 +523,7 @@ static struct platform_driver miic_driver = {
 		.of_match_table = miic_of_mtable,
 	},
 	.probe = miic_probe,
-	.remove = miic_remove,
+	.remove_new = miic_remove,
 };
 module_platform_driver(miic_driver);
 
-- 
2.42.0

[PATCH net-next 05/10] net: sfp: 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 ignored (apart
from emitting a warning) 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. Eventually after all drivers
are converted, .remove_new() will be renamed to .remove().

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/phy/sfp.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c
index 5468bd209fab..859b4749e8e2 100644
--- a/drivers/net/phy/sfp.c
+++ b/drivers/net/phy/sfp.c
@@ -3096,7 +3096,7 @@ static int sfp_probe(struct platform_device *pdev)
 	return 0;
 }
 
-static int sfp_remove(struct platform_device *pdev)
+static void sfp_remove(struct platform_device *pdev)
 {
 	struct sfp *sfp = platform_get_drvdata(pdev);
 
@@ -3106,8 +3106,6 @@ static int sfp_remove(struct platform_device *pdev)
 	rtnl_lock();
 	sfp_sm_event(sfp, SFP_E_REMOVE);
 	rtnl_unlock();
-
-	return 0;
 }
 
 static void sfp_shutdown(struct platform_device *pdev)
@@ -3128,7 +3126,7 @@ static void sfp_shutdown(struct platform_device *pdev)
 
 static struct platform_driver sfp_driver = {
 	.probe = sfp_probe,
-	.remove = sfp_remove,
+	.remove_new = sfp_remove,
 	.shutdown = sfp_shutdown,
 	.driver = {
 		.name = "sfp",
-- 
2.42.0

[PATCH net-next 06/10] net: wan/fsl_ucc_hdlc: 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 ignored (apart
from emitting a warning) 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. Eventually after all drivers
are converted, .remove_new() will be renamed to .remove().

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/wan/fsl_ucc_hdlc.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wan/fsl_ucc_hdlc.c b/drivers/net/wan/fsl_ucc_hdlc.c
index fd50bb313b92..605e70f7baac 100644
--- a/drivers/net/wan/fsl_ucc_hdlc.c
+++ b/drivers/net/wan/fsl_ucc_hdlc.c
@@ -1259,7 +1259,7 @@ static int ucc_hdlc_probe(struct platform_device *pdev)
 	return ret;
 }
 
-static int ucc_hdlc_remove(struct platform_device *pdev)
+static void ucc_hdlc_remove(struct platform_device *pdev)
 {
 	struct ucc_hdlc_private *priv = dev_get_drvdata(&pdev->dev);
 
@@ -1277,8 +1277,6 @@ static int ucc_hdlc_remove(struct platform_device *pdev)
 	kfree(priv);
 
 	dev_info(&pdev->dev, "UCC based hdlc module removed\n");
-
-	return 0;
 }
 
 static const struct of_device_id fsl_ucc_hdlc_of_match[] = {
@@ -1292,7 +1290,7 @@ MODULE_DEVICE_TABLE(of, fsl_ucc_hdlc_of_match);
 
 static struct platform_driver ucc_hdlc_driver = {
 	.probe	= ucc_hdlc_probe,
-	.remove	= ucc_hdlc_remove,
+	.remove_new = ucc_hdlc_remove,
 	.driver	= {
 		.name		= DRV_NAME,
 		.pm		= HDLC_PM_OPS,
-- 
2.42.0

[PATCH net-next 07/10] net: wan/ixp4xx_hss: 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 ignored (apart
from emitting a warning) 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. Eventually after all drivers
are converted, .remove_new() will be renamed to .remove().

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/wan/ixp4xx_hss.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wan/ixp4xx_hss.c b/drivers/net/wan/ixp4xx_hss.c
index b09f4c235142..931c5ca79ea5 100644
--- a/drivers/net/wan/ixp4xx_hss.c
+++ b/drivers/net/wan/ixp4xx_hss.c
@@ -1522,20 +1522,19 @@ static int ixp4xx_hss_probe(struct platform_device *pdev)
 	return err;
 }
 
-static int ixp4xx_hss_remove(struct platform_device *pdev)
+static void ixp4xx_hss_remove(struct platform_device *pdev)
 {
 	struct port *port = platform_get_drvdata(pdev);
 
 	unregister_hdlc_device(port->netdev);
 	free_netdev(port->netdev);
 	npe_release(port->npe);
-	return 0;
 }
 
 static struct platform_driver ixp4xx_hss_driver = {
 	.driver.name	= DRV_NAME,
 	.probe		= ixp4xx_hss_probe,
-	.remove		= ixp4xx_hss_remove,
+	.remove_new	= ixp4xx_hss_remove,
 };
 module_platform_driver(ixp4xx_hss_driver);
 
-- 
2.42.0

[PATCH net-next 08/10] net: wwan: qcom_bam_dmux: 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 ignored (apart
from emitting a warning) 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. Eventually after all drivers
are converted, .remove_new() will be renamed to .remove().

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/wwan/qcom_bam_dmux.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wwan/qcom_bam_dmux.c b/drivers/net/wwan/qcom_bam_dmux.c
index 17d46f4d2913..26ca719fa0de 100644
--- a/drivers/net/wwan/qcom_bam_dmux.c
+++ b/drivers/net/wwan/qcom_bam_dmux.c
@@ -846,7 +846,7 @@ static int bam_dmux_probe(struct platform_device *pdev)
 	return 0;
 }
 
-static int bam_dmux_remove(struct platform_device *pdev)
+static void bam_dmux_remove(struct platform_device *pdev)
 {
 	struct bam_dmux *dmux = platform_get_drvdata(pdev);
 	struct device *dev = dmux->dev;
@@ -877,8 +877,6 @@ static int bam_dmux_remove(struct platform_device *pdev)
 	disable_irq(dmux->pc_irq);
 	bam_dmux_power_off(dmux);
 	bam_dmux_free_skbs(dmux->tx_skbs, DMA_TO_DEVICE);
-
-	return 0;
 }
 
 static const struct dev_pm_ops bam_dmux_pm_ops = {
@@ -893,7 +891,7 @@ MODULE_DEVICE_TABLE(of, bam_dmux_of_match);
 
 static struct platform_driver bam_dmux_driver = {
 	.probe = bam_dmux_probe,
-	.remove = bam_dmux_remove,
+	.remove_new = bam_dmux_remove,
 	.driver = {
 		.name = "bam-dmux",
 		.pm = &bam_dmux_pm_ops,
-- 
2.42.0

[PATCH net-next 09/10] ieee802154: fakelb: 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 ignored (apart
from emitting a warning) 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. Eventually after all drivers
are converted, .remove_new() will be renamed to .remove().

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/ieee802154/fakelb.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ieee802154/fakelb.c b/drivers/net/ieee802154/fakelb.c
index 523d13ee02bf..35e55f198e05 100644
--- a/drivers/net/ieee802154/fakelb.c
+++ b/drivers/net/ieee802154/fakelb.c
@@ -221,7 +221,7 @@ static int fakelb_probe(struct platform_device *pdev)
 	return err;
 }
 
-static int fakelb_remove(struct platform_device *pdev)
+static void fakelb_remove(struct platform_device *pdev)
 {
 	struct fakelb_phy *phy, *tmp;
 
@@ -229,14 +229,13 @@ static int fakelb_remove(struct platform_device *pdev)
 	list_for_each_entry_safe(phy, tmp, &fakelb_phys, list)
 		fakelb_del(phy);
 	mutex_unlock(&fakelb_phys_lock);
-	return 0;
 }
 
 static struct platform_device *ieee802154fake_dev;
 
 static struct platform_driver ieee802154fake_driver = {
 	.probe = fakelb_probe,
-	.remove = fakelb_remove,
+	.remove_new = fakelb_remove,
 	.driver = {
 			.name = "ieee802154fakelb",
 	},
-- 
2.42.0

[PATCH net-next 10/10] ieee802154: hwsim: 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 ignored (apart
from emitting a warning) 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. Eventually after all drivers
are converted, .remove_new() will be renamed to .remove().

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/ieee802154/mac802154_hwsim.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ieee802154/mac802154_hwsim.c b/drivers/net/ieee802154/mac802154_hwsim.c
index 31cba9aa7636..2c2483bbe780 100644
--- a/drivers/net/ieee802154/mac802154_hwsim.c
+++ b/drivers/net/ieee802154/mac802154_hwsim.c
@@ -1035,7 +1035,7 @@ static int hwsim_probe(struct platform_device *pdev)
 	return err;
 }
 
-static int hwsim_remove(struct platform_device *pdev)
+static void hwsim_remove(struct platform_device *pdev)
 {
 	struct hwsim_phy *phy, *tmp;
 
@@ -1043,13 +1043,11 @@ static int hwsim_remove(struct platform_device *pdev)
 	list_for_each_entry_safe(phy, tmp, &hwsim_phys, list)
 		hwsim_del(phy);
 	mutex_unlock(&hwsim_phys_lock);
-
-	return 0;
 }
 
 static struct platform_driver mac802154hwsim_driver = {
 	.probe = hwsim_probe,
-	.remove = hwsim_remove,
+	.remove_new = hwsim_remove,
 	.driver = {
 			.name = "mac802154_hwsim",
 	},
-- 
2.42.0

Re: [PATCH net-next 04/10] net: pcs: rzn1-miic: Convert to platform remove callback returning void

[Geert Uytterhoeven]

On Fri, Nov 17, 2023 at 11:00 AM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> 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 ignored (apart
> from emitting a warning) 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. Eventually after all drivers
> are converted, .remove_new() will be renamed to .remove().
>
> 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>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH net-next 09/10] ieee802154: fakelb: Convert to platform remove callback returning void

[Stefan Schmidt]

Hello.

On 17.11.23 10:59, 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 ignored (apart
> from emitting a warning) 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. Eventually after all drivers
> are converted, .remove_new() will be renamed to .remove().
> 
> 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/ieee802154/fakelb.c | 5 ++---
>   1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ieee802154/fakelb.c b/drivers/net/ieee802154/fakelb.c
> index 523d13ee02bf..35e55f198e05 100644
> --- a/drivers/net/ieee802154/fakelb.c
> +++ b/drivers/net/ieee802154/fakelb.c
> @@ -221,7 +221,7 @@ static int fakelb_probe(struct platform_device *pdev)
>   	return err;
>   }
>   
> -static int fakelb_remove(struct platform_device *pdev)
> +static void fakelb_remove(struct platform_device *pdev)
>   {
>   	struct fakelb_phy *phy, *tmp;
>   
> @@ -229,14 +229,13 @@ static int fakelb_remove(struct platform_device *pdev)
>   	list_for_each_entry_safe(phy, tmp, &fakelb_phys, list)
>   		fakelb_del(phy);
>   	mutex_unlock(&fakelb_phys_lock);
> -	return 0;
>   }
>   
>   static struct platform_device *ieee802154fake_dev;
>   
>   static struct platform_driver ieee802154fake_driver = {
>   	.probe = fakelb_probe,
> -	.remove = fakelb_remove,
> +	.remove_new = fakelb_remove,
>   	.driver = {
>   			.name = "ieee802154fakelb",
>   	},


Acked-by: Stefan Schmidt <stefan@datenfreihafen.org>

regards
Stefan Schmidt

Re: [PATCH net-next 10/10] ieee802154: hwsim: Convert to platform remove callback returning void

[Stefan Schmidt]

Hello.

On 17.11.23 10:59, 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 ignored (apart
> from emitting a warning) 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. Eventually after all drivers
> are converted, .remove_new() will be renamed to .remove().
> 
> 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/ieee802154/mac802154_hwsim.c | 6 ++----
>   1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ieee802154/mac802154_hwsim.c b/drivers/net/ieee802154/mac802154_hwsim.c
> index 31cba9aa7636..2c2483bbe780 100644
> --- a/drivers/net/ieee802154/mac802154_hwsim.c
> +++ b/drivers/net/ieee802154/mac802154_hwsim.c
> @@ -1035,7 +1035,7 @@ static int hwsim_probe(struct platform_device *pdev)
>   	return err;
>   }
>   
> -static int hwsim_remove(struct platform_device *pdev)
> +static void hwsim_remove(struct platform_device *pdev)
>   {
>   	struct hwsim_phy *phy, *tmp;
>   
> @@ -1043,13 +1043,11 @@ static int hwsim_remove(struct platform_device *pdev)
>   	list_for_each_entry_safe(phy, tmp, &hwsim_phys, list)
>   		hwsim_del(phy);
>   	mutex_unlock(&hwsim_phys_lock);
> -
> -	return 0;
>   }
>   
>   static struct platform_driver mac802154hwsim_driver = {
>   	.probe = hwsim_probe,
> -	.remove = hwsim_remove,
> +	.remove_new = hwsim_remove,
>   	.driver = {
>   			.name = "mac802154_hwsim",
>   	},


Acked-by: Stefan Schmidt <stefan@datenfreihafen.org>

regards
Stefan Schmidt

Re: [PATCH net-next 05/10] net: sfp: Convert to platform remove callback returning void

[Russell King (Oracle)]

On Fri, Nov 17, 2023 at 10:59:28AM +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 ignored (apart
> from emitting a warning) 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. Eventually after all drivers
> are converted, .remove_new() will be renamed to .remove().
> 
> 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>

Reviewed-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Thanks!

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next 01/10] net: ipa: Don't error out in .remove()

[Alex Elder]

On 11/17/23 3:59 AM, Uwe Kleine-König wrote:
> Returning early from .remove() with an error code still results in the
> driver unbinding the device. So the driver core ignores the returned error
> code and the resources that were not freed are never catched up. In
> combination with devm this also often results in use-after-free bugs.
> 
> Here even if the modem cannot be stopped, resources must be freed. So
> replace the early error return by an error message an continue to clean up.
> 
> This prepares changing ipa_remove() to return void.
> 
> Fixes: cdf2e9419dd9 ("soc: qcom: ipa: main code")

Is this really a bug fix?  This code was doing the right
thing even if the caller was not.

> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>   drivers/net/ipa/ipa_main.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ipa/ipa_main.c b/drivers/net/ipa/ipa_main.c
> index da853353a5c7..60e4f590f5de 100644
> --- a/drivers/net/ipa/ipa_main.c
> +++ b/drivers/net/ipa/ipa_main.c
> @@ -960,7 +960,8 @@ static int ipa_remove(struct platform_device *pdev)
>   			ret = ipa_modem_stop(ipa);
>   		}
>   		if (ret)
> -			return ret;
> +			dev_err(dev, "Failed to stop modem (%pe)\n",
> +				ERR_PTR(ret));

I think this is not correct, or rather, I think it is less
correct than returning early.

What's happening here is we're trying to stop the modem.
It is an external entity that might have some in-flight
activity that could include "owning" some buffers provided
by Linux, to be filled with received data.  There's a
chance that cleaning up (with the call to ipa_teardown())
can do the right thing, but I'm not going to sign off on
this until I've looked at that in closer detail.

This is something that *could* happen but is not *expected*
to happen.  We expect stopping the modem to succeed so if
it doesn't, something's wrong and it's not 100% clear how
to properly handle it.

For now...  you know a little more about my hesitation, but
please wait to commit this change until I've had a chance
to spend more time reviewing.

					-Alex

>   
>   		ipa_teardown(ipa);
>   	}

Re: [PATCH net-next 02/10] net: ipa: Convert to platform remove callback returning void

[Alex Elder]

On 11/17/23 3:59 AM, 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 ignored (apart
> from emitting a warning) 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. Eventually after all drivers
> are converted, .remove_new() will be renamed to .remove().
> 
> 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>

To be clear, this patch can't proceed until the previous
one is resolved.  Once it is, this should be fine.

Sorry for not just doing it now.  I like what you're doing,
I just don't have time to spend at the moment for the
review this will require.

					-Alex


> ---
>   drivers/net/ipa/ipa_main.c | 17 +++--------------
>   1 file changed, 3 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/net/ipa/ipa_main.c b/drivers/net/ipa/ipa_main.c
> index 60e4f590f5de..2c769b85a2cd 100644
> --- a/drivers/net/ipa/ipa_main.c
> +++ b/drivers/net/ipa/ipa_main.c
> @@ -936,7 +936,7 @@ static int ipa_probe(struct platform_device *pdev)
>   	return ret;
>   }
>   
> -static int ipa_remove(struct platform_device *pdev)
> +static void ipa_remove(struct platform_device *pdev)
>   {
>   	struct ipa *ipa = dev_get_drvdata(&pdev->dev);
>   	struct ipa_power *power = ipa->power;
> @@ -979,17 +979,6 @@ static int ipa_remove(struct platform_device *pdev)
>   	ipa_power_exit(power);
>   
>   	dev_info(dev, "IPA driver removed");
> -
> -	return 0;
> -}
> -
> -static void ipa_shutdown(struct platform_device *pdev)
> -{
> -	int ret;
> -
> -	ret = ipa_remove(pdev);
> -	if (ret)
> -		dev_err(&pdev->dev, "shutdown: remove returned %d\n", ret);
>   }
>   
>   static const struct attribute_group *ipa_attribute_groups[] = {
> @@ -1002,8 +991,8 @@ static const struct attribute_group *ipa_attribute_groups[] = {
>   
>   static struct platform_driver ipa_driver = {
>   	.probe		= ipa_probe,
> -	.remove		= ipa_remove,
> -	.shutdown	= ipa_shutdown,
> +	.remove_new	= ipa_remove,
> +	.shutdown	= ipa_remove,
>   	.driver	= {
>   		.name		= "ipa",
>   		.pm		= &ipa_pm_ops,

Re: [PATCH net-next 01/10] net: ipa: Don't error out in .remove()

[Uwe Kleine-König]

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

Hello Alex,

On Fri, Nov 17, 2023 at 08:16:02AM -0600, Alex Elder wrote:
> On 11/17/23 3:59 AM, Uwe Kleine-König wrote:
> > Returning early from .remove() with an error code still results in the
> > driver unbinding the device. So the driver core ignores the returned error
> > code and the resources that were not freed are never catched up. In
> > combination with devm this also often results in use-after-free bugs.
> > 
> > Here even if the modem cannot be stopped, resources must be freed. So
> > replace the early error return by an error message an continue to clean up.
> > 
> > This prepares changing ipa_remove() to return void.
> > 
> > Fixes: cdf2e9419dd9 ("soc: qcom: ipa: main code")
> 
> Is this really a bug fix?  This code was doing the right
> thing even if the caller was not.

Yes, since cdf2e9419dd9 the driver is leaking resources if
ipa_modem_stop() fails. I call that a bug.

> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > ---
> >   drivers/net/ipa/ipa_main.c | 3 ++-
> >   1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/net/ipa/ipa_main.c b/drivers/net/ipa/ipa_main.c
> > index da853353a5c7..60e4f590f5de 100644
> > --- a/drivers/net/ipa/ipa_main.c
> > +++ b/drivers/net/ipa/ipa_main.c
> > @@ -960,7 +960,8 @@ static int ipa_remove(struct platform_device *pdev)
> >   			ret = ipa_modem_stop(ipa);
> >   		}
> >   		if (ret)
> > -			return ret;
> > +			dev_err(dev, "Failed to stop modem (%pe)\n",
> > +				ERR_PTR(ret));
> 
> I think this is not correct, or rather, I think it is less
> correct than returning early.
> 
> What's happening here is we're trying to stop the modem.
> It is an external entity that might have some in-flight
> activity that could include "owning" some buffers provided
> by Linux, to be filled with received data.  There's a
> chance that cleaning up (with the call to ipa_teardown())
> can do the right thing, but I'm not going to sign off on
> this until I've looked at that in closer detail.
> 
> This is something that *could* happen but is not *expected*
> to happen.  We expect stopping the modem to succeed so if
> it doesn't, something's wrong and it's not 100% clear how
> to properly handle it.

Returning early is wrong for sure. You skip for example the free_irq
step, so the irq stays active, it might trigger and use the unbound
device. And as the device is unbound, .remove() is never retried and the
irq (and all the other resources) are never freed.

Take the time you need to review the changes. If you don't want to
accept the change now, I'd like to apply the following change:

diff --git a/drivers/net/ipa/ipa_main.c b/drivers/net/ipa/ipa_main.c
index da853353a5c7..f77decf0b1e4 100644
--- a/drivers/net/ipa/ipa_main.c
+++ b/drivers/net/ipa/ipa_main.c
@@ -959,8 +959,11 @@ static int ipa_remove(struct platform_device *pdev)
 			usleep_range(USEC_PER_MSEC, 2 * USEC_PER_MSEC);
 			ret = ipa_modem_stop(ipa);
 		}
-		if (ret)
-			return ret;
+		if (ret) {
+			dev_err(dev, "Failed to stop modem (%pe)\n",
+				ERR_PTR(ret));
+			return 0;
+		}
 
 		ipa_teardown(ipa);
 	}

instead. This introduces no change in behaviour apart from improving the
error message and allows me to continue with my quest to make .remove
return void.

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 01/10] net: ipa: Don't error out in .remove()

[Alex Elder]

On 11/17/23 8:45 AM, Uwe Kleine-König wrote:
>>> Fixes: cdf2e9419dd9 ("soc: qcom: ipa: main code")
>> Is this really a bug fix?  This code was doing the right
>> thing even if the caller was not.
> Yes, since cdf2e9419dd9 the driver is leaking resources if
> ipa_modem_stop() fails. I call that a bug.

I understand that.  But the alternative is that we free
those resources and allow the hardware to (eventually)
complete an in-flight operation that touches one of those
resources, which is a use-after-free (rather than a leak),
and I call that a bug too.

The function was returning an error to the caller to
indicate the request failed.  I'm comfortable with
accepting that and just issuing a warning and returning
no error.

The reason I wanted more time to review was that I want to
walk through that code path again and decide which of the
bugs I'd rather keep--and I think it would be the resource
leak.

It's also possible the cleanup function can preclude any
later completion from causing a problem, which would be
the best.  I just don't remember without looking at it
again closely.

					-Alex

Re: [PATCH net-next 01/10] net: ipa: Don't error out in .remove()

[Alex Elder]

On 11/17/23 3:59 AM, Uwe Kleine-König wrote:
> Returning early from .remove() with an error code still results in the
> driver unbinding the device. So the driver core ignores the returned error
> code and the resources that were not freed are never catched up. In
> combination with devm this also often results in use-after-free bugs.
> 
> Here even if the modem cannot be stopped, resources must be freed. So
> replace the early error return by an error message an continue to clean up.
> 
> This prepares changing ipa_remove() to return void.
> 
> Fixes: cdf2e9419dd9 ("soc: qcom: ipa: main code")
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>   drivers/net/ipa/ipa_main.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ipa/ipa_main.c b/drivers/net/ipa/ipa_main.c
> index da853353a5c7..60e4f590f5de 100644
> --- a/drivers/net/ipa/ipa_main.c
> +++ b/drivers/net/ipa/ipa_main.c
> @@ -960,7 +960,8 @@ static int ipa_remove(struct platform_device *pdev)
>   			ret = ipa_modem_stop(ipa);

This ipa_modem_stop() call is the second one in this function,
which is called if the first attempt returned an error.  The
only error that's returned is -EBUSY, which occurs if a request
to stop the modem arrives at a time when we're in transition.
That is, either we are in the process of starting it up, or
we are in the process of stopping it.  In either case, this
transitional state should come to an end quickly.  This second
call happens after a short delay, giving a chance for the start
or stop that's underway to complete.

If the *second* call returns an error, it's assumed we're stuck
in one of the two transitional states, in which case something is
wrong with the hardware (we've issued a request that did not
complete, for example).  And in that case, we have no way of
knowing whether the hardware will come alive and do something
with a resource that's been allocated for it.

I think I'd rather live with whatever resource leaks occur in
such an unlikely case, rather than free them without knowing
what the (broken) hardware is going to do.

>   		}
>   		if (ret)
> -			return ret;
> +			dev_err(dev, "Failed to stop modem (%pe)\n",
> +				ERR_PTR(ret));

By the above reasoning, I'd rather your patch result in the code
looking like this:

	if (ret) {
		dev_err(dev, "remove: error %d stopping modem\n", ret);
		return ret;		/* XXX Later: just return; */
	}

The message is more consistent with the way other messages in the
driver are written.  If %pe is preferred I'd rather make that change
comprehensively throughout the driver rather than bit-by-bit.

Thanks.

					-Alex

>   		ipa_teardown(ipa);
>   	}

Re: [PATCH net-next 10/10] ieee802154: hwsim: Convert to platform remove callback returning void

[Miquel Raynal]

On Fri, 2023-11-17 at 09:59:33 UTC, =?utf-8?q?Uwe_Kleine-K=C3=B6nig?= 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 ignored (apart
> from emitting a warning) 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. Eventually after all drivers
> are converted, .remove_new() will be renamed to .remove().
> 
> 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>
> Acked-by: Stefan Schmidt <stefan@datenfreihafen.org>

Applied to https://git.kernel.org/pub/scm/linux/kernel/git/wpan/wpan-next.git staging, thanks.

Miquel

Re: [PATCH net-next 09/10] ieee802154: fakelb: Convert to platform remove callback returning void

[Miquel Raynal]

On Fri, 2023-11-17 at 09:59:32 UTC, =?utf-8?q?Uwe_Kleine-K=C3=B6nig?= 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 ignored (apart
> from emitting a warning) 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. Eventually after all drivers
> are converted, .remove_new() will be renamed to .remove().
> 
> 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>
> Acked-by: Stefan Schmidt <stefan@datenfreihafen.org>

Applied to https://git.kernel.org/pub/scm/linux/kernel/git/wpan/wpan-next.git staging, thanks.

Miquel

Re: [PATCH net-next 08/10] net: wwan: qcom_bam_dmux: Convert to platform remove callback returning void

[Sergey Ryazanov]

On 17.11.2023 11:59, 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 ignored (apart
> from emitting a warning) 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. Eventually after all drivers
> are converted, .remove_new() will be renamed to .remove().
> 
> 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>

Reviewed-by: Sergey Ryazanov <ryazanov.s.a@gmail.com>