| Message ID | 20241203222750.153272-1-rosenp@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | [PATCHv4,net-next] net: modernize ioremap in probe | expand |
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.
> 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
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.
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!
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;
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(-)