| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <c847e042-cc66-462e-a857-d1d9e500a081@omp.ru>
Date: Sun, 17 Nov 2024 20:14:11 +0300
From: Sergey Shtylyov <s.shtylyov@....ru>
To: Rosen Penev <rosenp@...il.com>, <netdev@...r.kernel.org>
CC: Chandrasekar Ramakrishnan <rcsekar@...sung.com>, Marc Kleine-Budde
<mkl@...gutronix.de>, Vincent Mailhol <mailhol.vincent@...adoo.fr>, Andrew
Lunn <andrew+netdev@...n.ch>, "David S. Miller" <davem@...emloft.net>, Eric
Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni
<pabeni@...hat.com>, Kurt Kanzenbach <kurt@...utronix.de>, Vladimir Oltean
<olteanv@...il.com>, Chris Snook <chris.snook@...il.com>, Marcin Wojtas
<marcin.s.wojtas@...il.com>, Russell King <linux@...linux.org.uk>, Horatiu
Vultur <horatiu.vultur@...rochip.com>, "maintainer:MICROCHIP LAN966X ETHERNET
DRIVER" <UNGLinuxDriver@...rochip.com>, Yoshihiro Shimoda
<yoshihiro.shimoda.uh@...esas.com>, Niklas Söderlund
<niklas.soderlund@...natech.se>, Doug Berger <opendmb@...il.com>, Florian
Fainelli <florian.fainelli@...adcom.com>, Broadcom internal kernel review
list <bcm-kernel-feedback-list@...adcom.com>, Heiner Kallweit
<hkallweit1@...il.com>, Richard Cochran <richardcochran@...il.com>, "open
list:MCAN MMIO DEVICE DRIVER" <linux-can@...r.kernel.org>, open list
<linux-kernel@...r.kernel.org>, "open list:RENESAS ETHERNET SWITCH DRIVER"
<linux-renesas-soc@...r.kernel.org>
Subject: Re: [PATCHv2 net-next] net: modernize ioremap in probe
On 11/11/24 11:02 PM, Rosen Penev wrote:
> I changed resource acquisition to be performed in a single step. Possible
> because devm is used here.
Have you tested it? Asking because switching to devm_platform_ioremap_resource_byname()
and devm_platform_get_and_ioremap_resource() seems to add devm_request_mem_region() call
into the picture...
I'm also not sure the single patch per drivers/net/ would be enough, but that's for the
maintainers to decide...
> Signed-off-by: Rosen Penev <rosenp@...il.com>
[...]
> 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");
The new code here should behave equivalently to the old, so seems OK.
> 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");
Here as well...
[...]
> 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;
This change also seems to look sane...
[...]
> 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
[...]
> @@ -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);
>
This one looks OKish too...
> 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;
Looks OKish too...
> diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c
> index d072f394eecb..07d1f1504a97 100644
> --- a/drivers/net/ethernet/renesas/sh_eth.c
> +++ b/drivers/net/ethernet/renesas/sh_eth.c
> @@ -3351,31 +3351,12 @@ static int sh_eth_drv_probe(struct platform_device *pdev)
>
> if (mdp->cd->tsu) {
> int port = pdev->id < 0 ? 0 : pdev->id % 2;
> - struct resource *rtsu;
>
> - rtsu = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> - if (!rtsu) {
> - dev_err(&pdev->dev, "no TSU resource\n");
> - ret = -ENODEV;
> - goto out_release;
> - }
> - /* We can only request the TSU region for the first port
> - * of the two sharing this TSU for the probe to succeed...
> - */
> - if (port == 0 &&
> - !devm_request_mem_region(&pdev->dev, rtsu->start,
> - resource_size(rtsu),
> - dev_name(&pdev->dev))) {
> - dev_err(&pdev->dev, "can't request TSU resource.\n");
> - ret = -EBUSY;
> - goto out_release;
> - }
> /* ioremap the TSU registers */
> - mdp->tsu_addr = devm_ioremap(&pdev->dev, rtsu->start,
> - resource_size(rtsu));
> - if (!mdp->tsu_addr) {
> + mdp->tsu_addr = devm_platform_ioremap_resource(pdev, 1);
> + if (IS_ERR(mdp->tsu_addr)) {
> dev_err(&pdev->dev, "TSU region ioremap() failed.\n");
> - ret = -ENOMEM;
> + ret = PTR_ERR(mdp->tsu_addr);
> goto out_release;
> }
> mdp->port = port;
No, this one won't fly since you're removing the port == 0 check... :-/
This code looks so strange on purpose... :-)
[...]
> diff --git a/drivers/net/mdio/mdio-ipq4019.c b/drivers/net/mdio/mdio-ipq4019.c
> index dd3ed2d6430b..725e5c13d212 100644
> --- a/drivers/net/mdio/mdio-ipq4019.c
> +++ b/drivers/net/mdio/mdio-ipq4019.c
> @@ -256,7 +256,7 @@ static int ipq_mdio_reset(struct mii_bus *bus)
> /* To indicate CMN_PLL that ethernet_ldo has been ready if platform resource 1
> * is specified in the device tree.
> */
> - if (priv->eth_ldo_rdy) {
> + if (!IS_ERR(priv->eth_ldo_rdy)) {
What's that? :-/
Ah, devm_ioremap_resource() returns error ptr too, so this looks like a fix for
the existing code?
> val = readl(priv->eth_ldo_rdy);
> val |= BIT(0);
> writel(val, priv->eth_ldo_rdy);
[...]
> @@ -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;
Other than that looks OKish...
[...]
> diff --git a/drivers/net/mdio/mdio-octeon.c b/drivers/net/mdio/mdio-octeon.c
> index 2beb83154d39..cb53dccbde1a 100644
> --- a/drivers/net/mdio/mdio-octeon.c
> +++ b/drivers/net/mdio/mdio-octeon.c
> @@ -17,37 +17,20 @@ static int octeon_mdiobus_probe(struct platform_device *pdev)
> {
> struct cavium_mdiobus *bus;
> struct mii_bus *mii_bus;
> - struct resource *res_mem;
> - resource_size_t mdio_phys;
> - resource_size_t regsize;
> union cvmx_smix_en smi_en;
> - int err = -ENOENT;
> + int err;
>
> mii_bus = devm_mdiobus_alloc_size(&pdev->dev, sizeof(*bus));
> if (!mii_bus)
> return -ENOMEM;
>
> - res_mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> - if (res_mem == NULL) {
> - dev_err(&pdev->dev, "found no memory resource\n");
> - return -ENXIO;
> - }
> -
> bus = mii_bus->priv;
> bus->mii_bus = mii_bus;
> - mdio_phys = res_mem->start;
> - regsize = resource_size(res_mem);
>
> - if (!devm_request_mem_region(&pdev->dev, mdio_phys, regsize,
> - res_mem->name)) {
> - dev_err(&pdev->dev, "request_mem_region failed\n");
> - return -ENXIO;
> - }
> -
> - bus->register_base = devm_ioremap(&pdev->dev, mdio_phys, regsize);
> - if (!bus->register_base) {
> + bus->register_base = devm_platform_ioremap_resource(pdev, 0);
> + if (IS_ERR(bus->register_base)) {
> dev_err(&pdev->dev, "dev_ioremap failed\n");
> - return -ENOMEM;
> + return PTR_ERR(bus->register_base);
> }
>
> smi_en.u64 = 0;
Seems OKish too...
MBR, Sergey
Powered by blists - more mailing lists