| Message ID | 20241117212711.13612-1-rosenp@gmail.com (mailing list archive) |
|---|---|
| State | Deferred |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [PATCHv3,net-next] net: modernize ioremap in probe | expand |
Hello Rosen, Thanks for your work. On 2024-11-17 13:27:11 -0800, Rosen Penev wrote: > diff --git a/drivers/net/ethernet/renesas/rtsn.c > b/drivers/net/ethernet/renesas/rtsn.c > index 6b3f7fca8d15..bfe08facc707 100644 > --- a/drivers/net/ethernet/renesas/rtsn.c > +++ b/drivers/net/ethernet/renesas/rtsn.c > @@ -1297,14 +1297,8 @@ 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; You have a similar construct using platform_get_resource_byname() a few lines above this one. Please convert both uses, or none, mixing them is just confusing IMHO.
On Sun, Nov 17, 2024 at 2:38 PM Niklas Söderlund <niklas.soderlund@ragnatech.se> wrote: > > Hello Rosen, > > Thanks for your work. > > On 2024-11-17 13:27:11 -0800, Rosen Penev wrote: > > > diff --git a/drivers/net/ethernet/renesas/rtsn.c > > b/drivers/net/ethernet/renesas/rtsn.c > > index 6b3f7fca8d15..bfe08facc707 100644 > > --- a/drivers/net/ethernet/renesas/rtsn.c > > +++ b/drivers/net/ethernet/renesas/rtsn.c > > @@ -1297,14 +1297,8 @@ 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; > > You have a similar construct using platform_get_resource_byname() a few > lines above this one. Please convert both uses, or none, mixing them is > just confusing IMHO. that cannot be converted. devm_platform_ioremap_resource_byname has no res parameter, which is a problem as there's this lovely line below it. ndev->base_addr = res->start; > > -- > Kind Regards, > Niklas Söderlund
On 11/18/24 12:27 AM, Rosen Penev wrote: > Convert platform_get_resource_bynam + devm_ioremap_resource to > devm_platform_ioremap_resource_byname. > > Convert platform_get_resource + devm_ioremap_resource to > devm_platform_ioremap_resource. > > resource aquisition and ioremap can be performed in one step. > > Signed-off-by: Rosen Penev <rosenp@gmail.com> > --- > 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 | 18 +++--------------- > drivers/net/ethernet/atheros/ag71xx.c | 13 +++++-------- > drivers/net/ethernet/broadcom/bcm63xx_enet.c | 6 ++---- > .../net/ethernet/marvell/mvpp2/mvpp2_main.c | 14 +++++--------- > drivers/net/ethernet/renesas/rswitch.c | 9 +-------- > drivers/net/ethernet/renesas/rtsn.c | 10 ++-------- > drivers/net/mdio/mdio-ipq4019.c | 5 +---- > 7 files changed, 19 insertions(+), 56 deletions(-) > > diff --git a/drivers/net/dsa/hirschmann/hellcreek.c b/drivers/net/dsa/hirschmann/hellcreek.c > index 283ec5a6e23c..940c4fa6a924 100644 > --- a/drivers/net/dsa/hirschmann/hellcreek.c > +++ b/drivers/net/dsa/hirschmann/hellcreek.c [...] > @@ -1982,23 +1981,12 @@ 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"); You have full 100 columns now, so doing this with 2 lines doesn't seem necessary -- checkpatch.pl shouldn't complain... [...] Other than that, looks saner now... :-) MBR, Sergey
On 2024-11-17 15:07:53 -0800, Rosen Penev wrote: > On Sun, Nov 17, 2024 at 2:38 PM Niklas Söderlund > <niklas.soderlund@ragnatech.se> wrote: > > > > Hello Rosen, > > > > Thanks for your work. > > > > On 2024-11-17 13:27:11 -0800, Rosen Penev wrote: > > > > > diff --git a/drivers/net/ethernet/renesas/rtsn.c > > > b/drivers/net/ethernet/renesas/rtsn.c > > > index 6b3f7fca8d15..bfe08facc707 100644 > > > --- a/drivers/net/ethernet/renesas/rtsn.c > > > +++ b/drivers/net/ethernet/renesas/rtsn.c > > > @@ -1297,14 +1297,8 @@ 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; > > > > You have a similar construct using platform_get_resource_byname() a few > > lines above this one. Please convert both uses, or none, mixing them is > > just confusing IMHO. > that cannot be converted. > > devm_platform_ioremap_resource_byname has no res parameter, which is a > problem as there's this lovely line below it. > > ndev->base_addr = res->start; I see, maybe we can refactor that too? I see not all drivers set base_addr, and some even set it to the remapped memory returned by devm_platform_ioremap_resource_byname() or such. The documentation for this field is not crystal clear for me and it is marked with a FIXME in the definition. struct net_device { ... /* * I/O specific fields * FIXME: Merge these and struct ifmap into one */ unsigned long mem_end; unsigned long mem_start; unsigned long base_addr; ... > > > > -- > > Kind Regards, > > Niklas Söderlund
On Tue, Nov 19, 2024 at 09:39:16PM +0100, Niklas Söderlund wrote: > On 2024-11-17 15:07:53 -0800, Rosen Penev wrote: > > devm_platform_ioremap_resource_byname has no res parameter, which is a > > problem as there's this lovely line below it. > > > > ndev->base_addr = res->start; > > I see, maybe we can refactor that too? I see not all drivers set > base_addr, and some even set it to the remapped memory returned by > devm_platform_ioremap_resource_byname() or such. base_addr carries with it an issue that setting it on every driver is likely not a good idea. Namely, that it's "unsigned long", it's reported to userspace, and on PAE systems, unsigned long is 32-bit but the device address may be >32-bit. I haven't checked the user APIs, whether that restricts it to 32-bit on 32-bit systems. In any case, whether base_addr is set or not is probably best left as-is and not have some "we must always / never set base_addr" rule applied to it.
On Tue, Nov 19, 2024 at 12:02 PM Sergey Shtylyov <s.shtylyov@omp.ru> wrote: > > On 11/18/24 12:27 AM, Rosen Penev wrote: > > > Convert platform_get_resource_bynam + devm_ioremap_resource to > > devm_platform_ioremap_resource_byname. > > > > Convert platform_get_resource + devm_ioremap_resource to > > devm_platform_ioremap_resource. > > > > resource aquisition and ioremap can be performed in one step. > > > > Signed-off-by: Rosen Penev <rosenp@gmail.com> > > --- > > 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 | 18 +++--------------- > > drivers/net/ethernet/atheros/ag71xx.c | 13 +++++-------- > > drivers/net/ethernet/broadcom/bcm63xx_enet.c | 6 ++---- > > .../net/ethernet/marvell/mvpp2/mvpp2_main.c | 14 +++++--------- > > drivers/net/ethernet/renesas/rswitch.c | 9 +-------- > > drivers/net/ethernet/renesas/rtsn.c | 10 ++-------- > > drivers/net/mdio/mdio-ipq4019.c | 5 +---- > > 7 files changed, 19 insertions(+), 56 deletions(-) > > > > diff --git a/drivers/net/dsa/hirschmann/hellcreek.c b/drivers/net/dsa/hirschmann/hellcreek.c > > index 283ec5a6e23c..940c4fa6a924 100644 > > --- a/drivers/net/dsa/hirschmann/hellcreek.c > > +++ b/drivers/net/dsa/hirschmann/hellcreek.c > [...] > > @@ -1982,23 +1981,12 @@ 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"); > > You have full 100 columns now, so doing this with 2 lines doesn't seem necessary -- > checkpatch.pl shouldn't complain... Looks like that's bdc48fa11e46f according to git blame. Reading the commit message, 80 is still prefered. The reason it's done here is because of .clang-format, which still lists 80. > > [...] > > Other than that, looks saner now... :-) > > MBR, Sergey >
diff --git a/drivers/net/dsa/hirschmann/hellcreek.c b/drivers/net/dsa/hirschmann/hellcreek.c index 283ec5a6e23c..940c4fa6a924 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,12 @@ 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..faf853edc0db 100644 --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c @@ -7425,21 +7425,17 @@ 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)) + dev_warn(&pdev->dev, + "DT is too old, Flow control not supported\n"); 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..bfe08facc707 100644 --- a/drivers/net/ethernet/renesas/rtsn.c +++ b/drivers/net/ethernet/renesas/rtsn.c @@ -1297,14 +1297,8 @@ 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; diff --git a/drivers/net/mdio/mdio-ipq4019.c b/drivers/net/mdio/mdio-ipq4019.c index 859302b0d38c..50afef293649 100644 --- a/drivers/net/mdio/mdio-ipq4019.c +++ b/drivers/net/mdio/mdio-ipq4019.c @@ -327,7 +327,6 @@ static int ipq4019_mdio_probe(struct platform_device *pdev) { struct ipq4019_mdio_data *priv; struct mii_bus *bus; - struct resource *res; int ret; bus = devm_mdiobus_alloc_size(&pdev->dev, sizeof(*priv)); @@ -351,9 +350,7 @@ static int ipq4019_mdio_probe(struct platform_device *pdev) /* The platform resource is provided on the chipset IPQ5018 */ /* This resource is optional */ - res = platform_get_resource(pdev, IORESOURCE_MEM, 1); - if (res) - priv->eth_ldo_rdy = devm_ioremap_resource(&pdev->dev, res); + priv->eth_ldo_rdy = devm_platform_ioremap_resource(pdev, 1); bus->name = "ipq4019_mdio"; bus->read = ipq4019_mdio_read_c22;
Convert platform_get_resource_bynam + devm_ioremap_resource to devm_platform_ioremap_resource_byname. Convert platform_get_resource + devm_ioremap_resource to devm_platform_ioremap_resource. resource aquisition and ioremap can be performed in one step. Signed-off-by: Rosen Penev <rosenp@gmail.com> --- 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 | 18 +++--------------- drivers/net/ethernet/atheros/ag71xx.c | 13 +++++-------- drivers/net/ethernet/broadcom/bcm63xx_enet.c | 6 ++---- .../net/ethernet/marvell/mvpp2/mvpp2_main.c | 14 +++++--------- drivers/net/ethernet/renesas/rswitch.c | 9 +-------- drivers/net/ethernet/renesas/rtsn.c | 10 ++-------- drivers/net/mdio/mdio-ipq4019.c | 5 +---- 7 files changed, 19 insertions(+), 56 deletions(-)