* Re: [PATCH] net: ravb: Fix registered interrupt names
2024-04-24 7:45 [PATCH] net: ravb: Fix registered interrupt names Geert Uytterhoeven
@ 2024-04-24 7:56 ` Niklas Söderlund
2024-04-24 9:10 ` Sergey Shtylyov
` (3 subsequent siblings)
4 siblings, 0 replies; 8+ messages in thread
From: Niklas Söderlund @ 2024-04-24 7:56 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Claudiu Beznea, Sergey Shtylyov, Paul Barker, David S . Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, netdev,
linux-renesas-soc
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
>
--
Kind Regards,
Niklas Söderlund
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] net: ravb: Fix registered interrupt names
2024-04-24 7:45 [PATCH] net: ravb: Fix registered interrupt names Geert Uytterhoeven
2024-04-24 7:56 ` Niklas Söderlund
@ 2024-04-24 9:10 ` Sergey Shtylyov
2024-04-24 9:19 ` Geert Uytterhoeven
2024-04-24 11:13 ` claudiu beznea
` (2 subsequent siblings)
4 siblings, 1 reply; 8+ messages in thread
From: Sergey Shtylyov @ 2024-04-24 9:10 UTC (permalink / raw)
To: Geert Uytterhoeven, Claudiu Beznea, Paul Barker,
Niklas Söderlund, David S . Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni
Cc: netdev, linux-renesas-soc
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
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] net: ravb: Fix registered interrupt names
2024-04-24 9:10 ` Sergey Shtylyov
@ 2024-04-24 9:19 ` Geert Uytterhoeven
0 siblings, 0 replies; 8+ messages in thread
From: Geert Uytterhoeven @ 2024-04-24 9:19 UTC (permalink / raw)
To: Sergey Shtylyov
Cc: Claudiu Beznea, Paul Barker, Niklas Söderlund,
David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
netdev, linux-renesas-soc
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
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] net: ravb: Fix registered interrupt names
2024-04-24 7:45 [PATCH] net: ravb: Fix registered interrupt names Geert Uytterhoeven
2024-04-24 7:56 ` Niklas Söderlund
2024-04-24 9:10 ` Sergey Shtylyov
@ 2024-04-24 11:13 ` claudiu beznea
2024-04-24 22:45 ` Andrew Lunn
2024-04-25 15:40 ` patchwork-bot+netdevbpf
4 siblings, 0 replies; 8+ messages in thread
From: claudiu beznea @ 2024-04-24 11:13 UTC (permalink / raw)
To: Geert Uytterhoeven, Claudiu Beznea, Sergey Shtylyov, Paul Barker,
Niklas Söderlund, David S . Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni
Cc: netdev, linux-renesas-soc
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;
> }
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] net: ravb: Fix registered interrupt names
2024-04-24 7:45 [PATCH] net: ravb: Fix registered interrupt names Geert Uytterhoeven
` (2 preceding siblings ...)
2024-04-24 11:13 ` claudiu beznea
@ 2024-04-24 22:45 ` Andrew Lunn
2024-04-25 14:16 ` Jakub Kicinski
2024-04-25 15:40 ` patchwork-bot+netdevbpf
4 siblings, 1 reply; 8+ messages in thread
From: Andrew Lunn @ 2024-04-24 22:45 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Claudiu Beznea, Sergey Shtylyov, Paul Barker,
Niklas Söderlund, David S . Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, netdev, linux-renesas-soc
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
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] net: ravb: Fix registered interrupt names
2024-04-24 22:45 ` Andrew Lunn
@ 2024-04-25 14:16 ` Jakub Kicinski
0 siblings, 0 replies; 8+ messages in thread
From: Jakub Kicinski @ 2024-04-25 14:16 UTC (permalink / raw)
To: Andrew Lunn
Cc: Geert Uytterhoeven, Claudiu Beznea, Sergey Shtylyov, Paul Barker,
Niklas Söderlund, David S . Miller, Eric Dumazet,
Paolo Abeni, netdev, linux-renesas-soc
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.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] net: ravb: Fix registered interrupt names
2024-04-24 7:45 [PATCH] net: ravb: Fix registered interrupt names Geert Uytterhoeven
` (3 preceding siblings ...)
2024-04-24 22:45 ` Andrew Lunn
@ 2024-04-25 15:40 ` patchwork-bot+netdevbpf
4 siblings, 0 replies; 8+ messages in thread
From: patchwork-bot+netdevbpf @ 2024-04-25 15:40 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: claudiu.beznea.uj, s.shtylyov, paul.barker.ct,
niklas.soderlund+renesas, davem, edumazet, kuba, pabeni, netdev,
linux-renesas-soc
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!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 8+ messages in thread