[PATCHv4 net-next] net: modernize ioremap in probe

[PATCHv4 net-next] net: modernize ioremap in probe

[Rosen Penev]

resource aquisition and ioremap can be performed in one step.

Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 v4: reformatted to 100 column limit
 v3: reworded commit message again. Also removed devm_ioremap
 conversions. Even though they use normal resource, they are not
 the same.
 v2: fixed compilation errors on PPC and reworded commit message
 drivers/net/dsa/hirschmann/hellcreek.c          | 17 ++---------------
 drivers/net/ethernet/atheros/ag71xx.c           | 13 +++++--------
 drivers/net/ethernet/broadcom/bcm63xx_enet.c    |  6 ++----
 drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c | 11 +++--------
 drivers/net/ethernet/renesas/rswitch.c          |  9 +--------
 drivers/net/ethernet/renesas/rtsn.c             |  9 +--------
 6 files changed, 14 insertions(+), 51 deletions(-)

diff --git a/drivers/net/dsa/hirschmann/hellcreek.c b/drivers/net/dsa/hirschmann/hellcreek.c
index 283ec5a6e23c..3a3e34c1d423 100644
--- a/drivers/net/dsa/hirschmann/hellcreek.c
+++ b/drivers/net/dsa/hirschmann/hellcreek.c
@@ -1932,7 +1932,6 @@ static int hellcreek_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct hellcreek *hellcreek;
-	struct resource *res;
 	int ret, i;
 
 	hellcreek = devm_kzalloc(dev, sizeof(*hellcreek), GFP_KERNEL);
@@ -1982,23 +1981,11 @@ static int hellcreek_probe(struct platform_device *pdev)
 
 	hellcreek->dev = dev;
 
-	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "tsn");
-	if (!res) {
-		dev_err(dev, "No memory region provided!\n");
-		return -ENODEV;
-	}
-
-	hellcreek->base = devm_ioremap_resource(dev, res);
+	hellcreek->base = devm_platform_ioremap_resource_byname(pdev, "tsn");
 	if (IS_ERR(hellcreek->base))
 		return PTR_ERR(hellcreek->base);
 
-	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "ptp");
-	if (!res) {
-		dev_err(dev, "No PTP memory region provided!\n");
-		return -ENODEV;
-	}
-
-	hellcreek->ptp_base = devm_ioremap_resource(dev, res);
+	hellcreek->ptp_base = devm_platform_ioremap_resource_byname(pdev, "ptp");
 	if (IS_ERR(hellcreek->ptp_base))
 		return PTR_ERR(hellcreek->ptp_base);
 
diff --git a/drivers/net/ethernet/atheros/ag71xx.c b/drivers/net/ethernet/atheros/ag71xx.c
index 3d4c3d8698e2..928d27b51b2a 100644
--- a/drivers/net/ethernet/atheros/ag71xx.c
+++ b/drivers/net/ethernet/atheros/ag71xx.c
@@ -1798,15 +1798,16 @@ static int ag71xx_probe(struct platform_device *pdev)
 	if (!ndev)
 		return -ENOMEM;
 
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	if (!res)
-		return -EINVAL;
-
 	dcfg = of_device_get_match_data(&pdev->dev);
 	if (!dcfg)
 		return -EINVAL;
 
 	ag = netdev_priv(ndev);
+
+	ag->mac_base = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
+	if (IS_ERR(ag->mac_base))
+		return PTR_ERR(ag->mac_base);
+
 	ag->mac_idx = -1;
 	for (i = 0; i < ARRAY_SIZE(ar71xx_addr_ar7100); i++) {
 		if (ar71xx_addr_ar7100[i] == res->start)
@@ -1836,10 +1837,6 @@ static int ag71xx_probe(struct platform_device *pdev)
 		return dev_err_probe(&pdev->dev, PTR_ERR(ag->mac_reset),
 				     "missing mac reset");
 
-	ag->mac_base = devm_ioremap_resource(&pdev->dev, res);
-	if (IS_ERR(ag->mac_base))
-		return PTR_ERR(ag->mac_base);
-
 	/* ensure that HW is in manual polling mode before interrupts are
 	 * activated. Otherwise ag71xx_interrupt might call napi_schedule
 	 * before it is initialized by netif_napi_add.
diff --git a/drivers/net/ethernet/broadcom/bcm63xx_enet.c b/drivers/net/ethernet/broadcom/bcm63xx_enet.c
index 65e3a0656a4c..420317abe3d2 100644
--- a/drivers/net/ethernet/broadcom/bcm63xx_enet.c
+++ b/drivers/net/ethernet/broadcom/bcm63xx_enet.c
@@ -2646,16 +2646,14 @@ static int bcm_enetsw_probe(struct platform_device *pdev)
 	struct bcm_enet_priv *priv;
 	struct net_device *dev;
 	struct bcm63xx_enetsw_platform_data *pd;
-	struct resource *res_mem;
 	int ret, irq_rx, irq_tx;
 
 	if (!bcm_enet_shared_base[0])
 		return -EPROBE_DEFER;
 
-	res_mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	irq_rx = platform_get_irq(pdev, 0);
 	irq_tx = platform_get_irq(pdev, 1);
-	if (!res_mem || irq_rx < 0)
+	if (irq_rx < 0)
 		return -ENODEV;
 
 	dev = alloc_etherdev(sizeof(*priv));
@@ -2688,7 +2686,7 @@ static int bcm_enetsw_probe(struct platform_device *pdev)
 	if (ret)
 		goto out;
 
-	priv->base = devm_ioremap_resource(&pdev->dev, res_mem);
+	priv->base = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(priv->base)) {
 		ret = PTR_ERR(priv->base);
 		goto out;
diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
index 571631a30320..af9291574931 100644
--- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
+++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
@@ -7425,21 +7425,16 @@ static int mvpp2_init(struct platform_device *pdev, struct mvpp2 *priv)
 static int mvpp2_get_sram(struct platform_device *pdev,
 			  struct mvpp2 *priv)
 {
-	struct resource *res;
 	void __iomem *base;
 
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 2);
-	if (!res) {
+	base = devm_platform_ioremap_resource(pdev, 2);
+	if (IS_ERR(base)) {
 		if (has_acpi_companion(&pdev->dev))
 			dev_warn(&pdev->dev, "ACPI is too old, Flow control not supported\n");
 		else
 			dev_warn(&pdev->dev, "DT is too old, Flow control not supported\n");
-		return 0;
-	}
-
-	base = devm_ioremap_resource(&pdev->dev, res);
-	if (IS_ERR(base))
 		return PTR_ERR(base);
+	}
 
 	priv->cm3_base = base;
 	return 0;
diff --git a/drivers/net/ethernet/renesas/rswitch.c b/drivers/net/ethernet/renesas/rswitch.c
index 8d18dae4d8fb..8ef52fc46a01 100644
--- a/drivers/net/ethernet/renesas/rswitch.c
+++ b/drivers/net/ethernet/renesas/rswitch.c
@@ -2046,15 +2046,8 @@ static int renesas_eth_sw_probe(struct platform_device *pdev)
 {
 	const struct soc_device_attribute *attr;
 	struct rswitch_private *priv;
-	struct resource *res;
 	int ret;
 
-	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "secure_base");
-	if (!res) {
-		dev_err(&pdev->dev, "invalid resource\n");
-		return -EINVAL;
-	}
-
 	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv)
 		return -ENOMEM;
@@ -2074,7 +2067,7 @@ static int renesas_eth_sw_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, priv);
 	priv->pdev = pdev;
-	priv->addr = devm_ioremap_resource(&pdev->dev, res);
+	priv->addr = devm_platform_ioremap_resource_byname(pdev, "secure_base");
 	if (IS_ERR(priv->addr))
 		return PTR_ERR(priv->addr);
 
diff --git a/drivers/net/ethernet/renesas/rtsn.c b/drivers/net/ethernet/renesas/rtsn.c
index 6b3f7fca8d15..6a2e9a9cef25 100644
--- a/drivers/net/ethernet/renesas/rtsn.c
+++ b/drivers/net/ethernet/renesas/rtsn.c
@@ -1297,14 +1297,7 @@ static int rtsn_probe(struct platform_device *pdev)
 	ndev->netdev_ops = &rtsn_netdev_ops;
 	ndev->ethtool_ops = &rtsn_ethtool_ops;
 
-	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "gptp");
-	if (!res) {
-		dev_err(&pdev->dev, "Can't find gptp resource\n");
-		ret = -EINVAL;
-		goto error_free;
-	}
-
-	priv->ptp_priv->addr = devm_ioremap_resource(&pdev->dev, res);
+	priv->ptp_priv->addr = devm_platform_ioremap_resource_byname(pdev, "gptp");
 	if (IS_ERR(priv->ptp_priv->addr)) {
 		ret = PTR_ERR(priv->ptp_priv->addr);
 		goto error_free;
-- 
2.47.0

Re: [PATCHv4 net-next] net: modernize ioremap in probe

[Russell King (Oracle)]

On Tue, Dec 03, 2024 at 02:27:50PM -0800, Rosen Penev wrote:
> resource aquisition and ioremap can be performed in one step.
...
> diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> index 571631a30320..af9291574931 100644
> --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> @@ -7425,21 +7425,16 @@ static int mvpp2_init(struct platform_device *pdev, struct mvpp2 *priv)
>  static int mvpp2_get_sram(struct platform_device *pdev,
>  			  struct mvpp2 *priv)
>  {
> -	struct resource *res;
>  	void __iomem *base;
>  
> -	res = platform_get_resource(pdev, IORESOURCE_MEM, 2);
> -	if (!res) {
> +	base = devm_platform_ioremap_resource(pdev, 2);
> +	if (IS_ERR(base)) {
>  		if (has_acpi_companion(&pdev->dev))
>  			dev_warn(&pdev->dev, "ACPI is too old, Flow control not supported\n");
>  		else
>  			dev_warn(&pdev->dev, "DT is too old, Flow control not supported\n");
> -		return 0;
> -	}
> -
> -	base = devm_ioremap_resource(&pdev->dev, res);
> -	if (IS_ERR(base))
>  		return PTR_ERR(base);
> +	}

This is not equivalent. This means if ioremap() fails inside
devm_platform_ioremap_resource(), we end up printing a message that
blames the firmware, which is wrong.

It also changes a "resource missing, proceed anyway" situation into
a failure situation.

Please drop this change, "cleaning" this up is introducing bugs.

Thanks.

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

Re: [PATCHv4 net-next] net: modernize ioremap in probe

[Andrew Lunn]

> This is not equivalent. This means if ioremap() fails inside
> devm_platform_ioremap_resource(), we end up printing a message that
> blames the firmware, which is wrong.
> 
> It also changes a "resource missing, proceed anyway" situation into
> a failure situation.
> 
> Please drop this change, "cleaning" this up is introducing bugs.

https://www.kernel.org/doc/html/latest/process/maintainer-netdev.html
says:

  1.6.5. Using device-managed and cleanup.h constructs

  Netdev remains skeptical about promises of all “auto-cleanup” APIs,
  including even devm_ helpers, historically. They are not the
  preferred style of implementation, merely an acceptable one.

  1.6.6. Clean-up patches

  Netdev discourages patches which perform simple clean-ups, which are
  not in the context of other work. For example:

  Addressing checkpatch.pl warnings

  Addressing Local variable ordering issues

  Conversions to device-managed APIs (devm_ helpers)

  This is because it is felt that the churn that such changes produce
  comes at a greater cost than the value of such clean-ups.

As Russell points out, you are breaking things which probably worked
before. "If its not broken, don't fix it". These changes cost reviewer
time trying to find what you have broken, when it would be better
spent other places.

If you have the hardware, and wont to work on the driver to add new
features, then yes, you can do this sort of conversion, because you
should find your own bugs via testing. If you don't have the hardware,
please just leave it alone.

	Andrew

Re: [PATCHv4 net-next] net: modernize ioremap in probe

[Jakub Kicinski]

On Wed, 4 Dec 2024 00:43:13 +0100 Andrew Lunn wrote:
> If you have the hardware, and wont to work on the driver to add new
> features, then yes, you can do this sort of conversion, because you
> should find your own bugs via testing. If you don't have the hardware,
> please just leave it alone.

Which I'm pretty sure I already told Rosen :/

One more complaint. The 15 patch limit is meant as a throttling
mechanism, please do not have more than 15 outstanding patches
at a time.

I hope all the replies you got on this patch give you a clear enough
idea on how valuable the maintainers find this sort of work.

Re: [PATCHv4 net-next] net: modernize ioremap in probe

[Rosen Penev]

On Tue, Dec 3, 2024 at 2:51 PM Russell King (Oracle)
<linux@armlinux.org.uk> wrote:
>
> On Tue, Dec 03, 2024 at 02:27:50PM -0800, Rosen Penev wrote:
> > resource aquisition and ioremap can be performed in one step.
> ...
> > diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> > index 571631a30320..af9291574931 100644
> > --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> > +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> > @@ -7425,21 +7425,16 @@ static int mvpp2_init(struct platform_device *pdev, struct mvpp2 *priv)
> >  static int mvpp2_get_sram(struct platform_device *pdev,
> >                         struct mvpp2 *priv)
> >  {
> > -     struct resource *res;
> >       void __iomem *base;
> >
> > -     res = platform_get_resource(pdev, IORESOURCE_MEM, 2);
> > -     if (!res) {
> > +     base = devm_platform_ioremap_resource(pdev, 2);
> > +     if (IS_ERR(base)) {
> >               if (has_acpi_companion(&pdev->dev))
> >                       dev_warn(&pdev->dev, "ACPI is too old, Flow control not supported\n");
> >               else
> >                       dev_warn(&pdev->dev, "DT is too old, Flow control not supported\n");
> > -             return 0;
> > -     }
> > -
> > -     base = devm_ioremap_resource(&pdev->dev, res);
> > -     if (IS_ERR(base))
> >               return PTR_ERR(base);
> > +     }
>
> This is not equivalent. This means if ioremap() fails inside
> devm_platform_ioremap_resource(), we end up printing a message that
> blames the firmware, which is wrong.
>
> It also changes a "resource missing, proceed anyway" situation into
> a failure situation.
>
> Please drop this change, "cleaning" this up is introducing bugs.
Will do in v5.
>
> Thanks.
>
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!