| Message ID | cde67b68adf115b3cf0b44c32334ae00b2fbb321.1713944647.git.geert+renesas@glider.be (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | 0c81ea5a8e231fa120e3f76aa9ea99fa3950cc59 |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | net: ravb: Fix registered interrupt names | expand |
Hi Geert, Thanks for fixing this! I noticed this as well but was dead-set needing to finding a way to preserve ehtX names, and wrote down I needed to to a large rework here to allow for this. Using the device name is more clever! On 2024-04-24 09:45:21 +0200, Geert Uytterhoeven wrote: > As interrupts are now requested from ravb_probe(), before calling > register_netdev(), ndev->name still contains the template "eth%d", > leading to funny names in /proc/interrupts. E.g. on R-Car E3: > > 89: 0 0 GICv2 93 Level eth%d:ch22:multi > 90: 0 3 GICv2 95 Level eth%d:ch24:emac > 91: 0 23484 GICv2 71 Level eth%d:ch0:rx_be > 92: 0 0 GICv2 72 Level eth%d:ch1:rx_nc > 93: 0 13735 GICv2 89 Level eth%d:ch18:tx_be > 94: 0 0 GICv2 90 Level eth%d:ch19:tx_nc > > Worse, on platforms with multiple RAVB instances (e.g. R-Car V4H), all > interrupts have similar names. > > Fix this by using the device name instead, like is done in several other > drivers: > > 89: 0 0 GICv2 93 Level e6800000.ethernet:ch22:multi > 90: 0 1 GICv2 95 Level e6800000.ethernet:ch24:emac > 91: 0 28578 GICv2 71 Level e6800000.ethernet:ch0:rx_be > 92: 0 0 GICv2 72 Level e6800000.ethernet:ch1:rx_nc > 93: 0 14044 GICv2 89 Level e6800000.ethernet:ch18:tx_be > 94: 0 0 GICv2 90 Level e6800000.ethernet:ch19:tx_nc > > Rename the local variable dev_name, as it shadows the dev_name() > function, and pre-initialize it, to simplify the code. > > Fixes: 32f012b8c01ca9fd ("net: ravb: Move getting/requesting IRQs in the probe() method") > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > --- > drivers/net/ethernet/renesas/ravb_main.c | 11 +++++------ > 1 file changed, 5 insertions(+), 6 deletions(-) > > diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c > index b621ddd4539cf517..384ddad65aaf641a 100644 > --- a/drivers/net/ethernet/renesas/ravb_main.c > +++ b/drivers/net/ethernet/renesas/ravb_main.c > @@ -2729,19 +2729,18 @@ static int ravb_setup_irq(struct ravb_private *priv, const char *irq_name, > struct platform_device *pdev = priv->pdev; > struct net_device *ndev = priv->ndev; > struct device *dev = &pdev->dev; > - const char *dev_name; > + const char *devname = dev_name(dev); > unsigned long flags; > int error, irq_num; > > if (irq_name) { > - dev_name = devm_kasprintf(dev, GFP_KERNEL, "%s:%s", ndev->name, ch); > - if (!dev_name) > + devname = devm_kasprintf(dev, GFP_KERNEL, "%s:%s", devname, ch); > + if (!devname) > return -ENOMEM; > > irq_num = platform_get_irq_byname(pdev, irq_name); > flags = 0; > } else { > - dev_name = ndev->name; > irq_num = platform_get_irq(pdev, 0); > flags = IRQF_SHARED; > } > @@ -2751,9 +2750,9 @@ static int ravb_setup_irq(struct ravb_private *priv, const char *irq_name, > if (irq) > *irq = irq_num; > > - error = devm_request_irq(dev, irq_num, handler, flags, dev_name, ndev); > + error = devm_request_irq(dev, irq_num, handler, flags, devname, ndev); > if (error) > - netdev_err(ndev, "cannot request IRQ %s\n", dev_name); > + netdev_err(ndev, "cannot request IRQ %s\n", devname); > > return error; > } > -- > 2.34.1 >
On 4/24/24 10:45 AM, Geert Uytterhoeven wrote: > As interrupts are now requested from ravb_probe(), before calling > register_netdev(), ndev->name still contains the template "eth%d", > leading to funny names in /proc/interrupts. E.g. on R-Car E3: > > 89: 0 0 GICv2 93 Level eth%d:ch22:multi > 90: 0 3 GICv2 95 Level eth%d:ch24:emac > 91: 0 23484 GICv2 71 Level eth%d:ch0:rx_be > 92: 0 0 GICv2 72 Level eth%d:ch1:rx_nc > 93: 0 13735 GICv2 89 Level eth%d:ch18:tx_be > 94: 0 0 GICv2 90 Level eth%d:ch19:tx_nc > > Worse, on platforms with multiple RAVB instances (e.g. R-Car V4H), all > interrupts have similar names. > > Fix this by using the device name instead, like is done in several other > drivers: > > 89: 0 0 GICv2 93 Level e6800000.ethernet:ch22:multi > 90: 0 1 GICv2 95 Level e6800000.ethernet:ch24:emac > 91: 0 28578 GICv2 71 Level e6800000.ethernet:ch0:rx_be > 92: 0 0 GICv2 72 Level e6800000.ethernet:ch1:rx_nc > 93: 0 14044 GICv2 89 Level e6800000.ethernet:ch18:tx_be > 94: 0 0 GICv2 90 Level e6800000.ethernet:ch19:tx_nc Ugh! Sorry about missing this one... > Rename the local variable dev_name, as it shadows the dev_name() > function, and pre-initialize it, to simplify the code. Why not call it just name instead? > Fixes: 32f012b8c01ca9fd ("net: ravb: Move getting/requesting IRQs in the probe() method") > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru> [...] MBR, Sergey
Hi Sergey, On Wed, Apr 24, 2024 at 11:10 AM Sergey Shtylyov <s.shtylyov@omp.ru> wrote: > On 4/24/24 10:45 AM, Geert Uytterhoeven wrote: > > As interrupts are now requested from ravb_probe(), before calling > > register_netdev(), ndev->name still contains the template "eth%d", > > leading to funny names in /proc/interrupts. E.g. on R-Car E3: > > > > 89: 0 0 GICv2 93 Level eth%d:ch22:multi > > 90: 0 3 GICv2 95 Level eth%d:ch24:emac > > 91: 0 23484 GICv2 71 Level eth%d:ch0:rx_be > > 92: 0 0 GICv2 72 Level eth%d:ch1:rx_nc > > 93: 0 13735 GICv2 89 Level eth%d:ch18:tx_be > > 94: 0 0 GICv2 90 Level eth%d:ch19:tx_nc > > > > Worse, on platforms with multiple RAVB instances (e.g. R-Car V4H), all > > interrupts have similar names. > > > > Fix this by using the device name instead, like is done in several other > > drivers: > > > > 89: 0 0 GICv2 93 Level e6800000.ethernet:ch22:multi > > 90: 0 1 GICv2 95 Level e6800000.ethernet:ch24:emac > > 91: 0 28578 GICv2 71 Level e6800000.ethernet:ch0:rx_be > > 92: 0 0 GICv2 72 Level e6800000.ethernet:ch1:rx_nc > > 93: 0 14044 GICv2 89 Level e6800000.ethernet:ch18:tx_be > > 94: 0 0 GICv2 90 Level e6800000.ethernet:ch19:tx_nc > > Ugh! Sorry about missing this one... > > > Rename the local variable dev_name, as it shadows the dev_name() > > function, and pre-initialize it, to simplify the code. > > Why not call it just name instead? Because the function has a parameter named "irq_name". "devname" also matches the corresponding parameter of devm_request_irq(). > > > Fixes: 32f012b8c01ca9fd ("net: ravb: Move getting/requesting IRQs in the probe() method") > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > > Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru> Thanks! Gr{oetje,eeting}s, Geert
Hi, Geert, On 24.04.2024 10:45, Geert Uytterhoeven wrote: > As interrupts are now requested from ravb_probe(), before calling > register_netdev(), ndev->name still contains the template "eth%d", > leading to funny names in /proc/interrupts. E.g. on R-Car E3: > > 89: 0 0 GICv2 93 Level eth%d:ch22:multi > 90: 0 3 GICv2 95 Level eth%d:ch24:emac > 91: 0 23484 GICv2 71 Level eth%d:ch0:rx_be > 92: 0 0 GICv2 72 Level eth%d:ch1:rx_nc > 93: 0 13735 GICv2 89 Level eth%d:ch18:tx_be > 94: 0 0 GICv2 90 Level eth%d:ch19:tx_nc I failed to notice it, sorry about that. > > Worse, on platforms with multiple RAVB instances (e.g. R-Car V4H), all > interrupts have similar names. > > Fix this by using the device name instead, like is done in several other > drivers: > > 89: 0 0 GICv2 93 Level e6800000.ethernet:ch22:multi > 90: 0 1 GICv2 95 Level e6800000.ethernet:ch24:emac > 91: 0 28578 GICv2 71 Level e6800000.ethernet:ch0:rx_be > 92: 0 0 GICv2 72 Level e6800000.ethernet:ch1:rx_nc > 93: 0 14044 GICv2 89 Level e6800000.ethernet:ch18:tx_be > 94: 0 0 GICv2 90 Level e6800000.ethernet:ch19:tx_nc > > Rename the local variable dev_name, as it shadows the dev_name() > function, and pre-initialize it, to simplify the code. > > Fixes: 32f012b8c01ca9fd ("net: ravb: Move getting/requesting IRQs in the probe() method") > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> Thank you for taking care of it. Reviewed-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> Tested-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> # on RZ/G3S > --- > drivers/net/ethernet/renesas/ravb_main.c | 11 +++++------ > 1 file changed, 5 insertions(+), 6 deletions(-) > > diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c > index b621ddd4539cf517..384ddad65aaf641a 100644 > --- a/drivers/net/ethernet/renesas/ravb_main.c > +++ b/drivers/net/ethernet/renesas/ravb_main.c > @@ -2729,19 +2729,18 @@ static int ravb_setup_irq(struct ravb_private *priv, const char *irq_name, > struct platform_device *pdev = priv->pdev; > struct net_device *ndev = priv->ndev; > struct device *dev = &pdev->dev; > - const char *dev_name; > + const char *devname = dev_name(dev); > unsigned long flags; > int error, irq_num; > > if (irq_name) { > - dev_name = devm_kasprintf(dev, GFP_KERNEL, "%s:%s", ndev->name, ch); > - if (!dev_name) > + devname = devm_kasprintf(dev, GFP_KERNEL, "%s:%s", devname, ch); > + if (!devname) > return -ENOMEM; > > irq_num = platform_get_irq_byname(pdev, irq_name); > flags = 0; > } else { > - dev_name = ndev->name; > irq_num = platform_get_irq(pdev, 0); > flags = IRQF_SHARED; > } > @@ -2751,9 +2750,9 @@ static int ravb_setup_irq(struct ravb_private *priv, const char *irq_name, > if (irq) > *irq = irq_num; > > - error = devm_request_irq(dev, irq_num, handler, flags, dev_name, ndev); > + error = devm_request_irq(dev, irq_num, handler, flags, devname, ndev); > if (error) > - netdev_err(ndev, "cannot request IRQ %s\n", dev_name); > + netdev_err(ndev, "cannot request IRQ %s\n", devname); > > return error; > }
On Wed, Apr 24, 2024 at 09:45:21AM +0200, Geert Uytterhoeven wrote: > As interrupts are now requested from ravb_probe(), before calling > register_netdev(), ndev->name still contains the template "eth%d", > leading to funny names in /proc/interrupts. E.g. on R-Car E3: > > 89: 0 0 GICv2 93 Level eth%d:ch22:multi > 90: 0 3 GICv2 95 Level eth%d:ch24:emac > 91: 0 23484 GICv2 71 Level eth%d:ch0:rx_be > 92: 0 0 GICv2 72 Level eth%d:ch1:rx_nc > 93: 0 13735 GICv2 89 Level eth%d:ch18:tx_be > 94: 0 0 GICv2 90 Level eth%d:ch19:tx_nc > > Worse, on platforms with multiple RAVB instances (e.g. R-Car V4H), all > interrupts have similar names. > > Fix this by using the device name instead, like is done in several other > drivers: > > 89: 0 0 GICv2 93 Level e6800000.ethernet:ch22:multi > 90: 0 1 GICv2 95 Level e6800000.ethernet:ch24:emac > 91: 0 28578 GICv2 71 Level e6800000.ethernet:ch0:rx_be > 92: 0 0 GICv2 72 Level e6800000.ethernet:ch1:rx_nc > 93: 0 14044 GICv2 89 Level e6800000.ethernet:ch18:tx_be > 94: 0 0 GICv2 90 Level e6800000.ethernet:ch19:tx_nc > > Rename the local variable dev_name, as it shadows the dev_name() > function, and pre-initialize it, to simplify the code. Another option is to call dev_alloc_name() soon after alloc_netdev(), to give the device its name earlier than register_netdev(). Andrew
On Thu, 25 Apr 2024 00:45:35 +0200 Andrew Lunn wrote: > > Rename the local variable dev_name, as it shadows the dev_name() > > function, and pre-initialize it, to simplify the code. > > Another option is to call dev_alloc_name() soon after alloc_netdev(), > to give the device its name earlier than register_netdev(). Maybe we shouldn't be advertising that option too broadly. One has to hold rtnl for that to work. Mostly old and staging drivers seem to do this. Name are not stable. If other identifiers are available, that's a better option, IMHO.
Hello: This patch was applied to netdev/net.git (main) by Jakub Kicinski <kuba@kernel.org>: On Wed, 24 Apr 2024 09:45:21 +0200 you wrote: > As interrupts are now requested from ravb_probe(), before calling > register_netdev(), ndev->name still contains the template "eth%d", > leading to funny names in /proc/interrupts. E.g. on R-Car E3: > > 89: 0 0 GICv2 93 Level eth%d:ch22:multi > 90: 0 3 GICv2 95 Level eth%d:ch24:emac > 91: 0 23484 GICv2 71 Level eth%d:ch0:rx_be > 92: 0 0 GICv2 72 Level eth%d:ch1:rx_nc > 93: 0 13735 GICv2 89 Level eth%d:ch18:tx_be > 94: 0 0 GICv2 90 Level eth%d:ch19:tx_nc > > [...] Here is the summary with links: - net: ravb: Fix registered interrupt names https://git.kernel.org/netdev/net/c/0c81ea5a8e23 You are awesome, thank you!
diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c index b621ddd4539cf517..384ddad65aaf641a 100644 --- a/drivers/net/ethernet/renesas/ravb_main.c +++ b/drivers/net/ethernet/renesas/ravb_main.c @@ -2729,19 +2729,18 @@ static int ravb_setup_irq(struct ravb_private *priv, const char *irq_name, struct platform_device *pdev = priv->pdev; struct net_device *ndev = priv->ndev; struct device *dev = &pdev->dev; - const char *dev_name; + const char *devname = dev_name(dev); unsigned long flags; int error, irq_num; if (irq_name) { - dev_name = devm_kasprintf(dev, GFP_KERNEL, "%s:%s", ndev->name, ch); - if (!dev_name) + devname = devm_kasprintf(dev, GFP_KERNEL, "%s:%s", devname, ch); + if (!devname) return -ENOMEM; irq_num = platform_get_irq_byname(pdev, irq_name); flags = 0; } else { - dev_name = ndev->name; irq_num = platform_get_irq(pdev, 0); flags = IRQF_SHARED; } @@ -2751,9 +2750,9 @@ static int ravb_setup_irq(struct ravb_private *priv, const char *irq_name, if (irq) *irq = irq_num; - error = devm_request_irq(dev, irq_num, handler, flags, dev_name, ndev); + error = devm_request_irq(dev, irq_num, handler, flags, devname, ndev); if (error) - netdev_err(ndev, "cannot request IRQ %s\n", dev_name); + netdev_err(ndev, "cannot request IRQ %s\n", devname); return error; }
As interrupts are now requested from ravb_probe(), before calling register_netdev(), ndev->name still contains the template "eth%d", leading to funny names in /proc/interrupts. E.g. on R-Car E3: 89: 0 0 GICv2 93 Level eth%d:ch22:multi 90: 0 3 GICv2 95 Level eth%d:ch24:emac 91: 0 23484 GICv2 71 Level eth%d:ch0:rx_be 92: 0 0 GICv2 72 Level eth%d:ch1:rx_nc 93: 0 13735 GICv2 89 Level eth%d:ch18:tx_be 94: 0 0 GICv2 90 Level eth%d:ch19:tx_nc Worse, on platforms with multiple RAVB instances (e.g. R-Car V4H), all interrupts have similar names. Fix this by using the device name instead, like is done in several other drivers: 89: 0 0 GICv2 93 Level e6800000.ethernet:ch22:multi 90: 0 1 GICv2 95 Level e6800000.ethernet:ch24:emac 91: 0 28578 GICv2 71 Level e6800000.ethernet:ch0:rx_be 92: 0 0 GICv2 72 Level e6800000.ethernet:ch1:rx_nc 93: 0 14044 GICv2 89 Level e6800000.ethernet:ch18:tx_be 94: 0 0 GICv2 90 Level e6800000.ethernet:ch19:tx_nc Rename the local variable dev_name, as it shadows the dev_name() function, and pre-initialize it, to simplify the code. Fixes: 32f012b8c01ca9fd ("net: ravb: Move getting/requesting IRQs in the probe() method") Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> --- drivers/net/ethernet/renesas/ravb_main.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-)