[PATCH] net: ravb: Fix registered interrupt names

[PATCH] net: ravb: Fix registered interrupt names

[Geert Uytterhoeven]

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(-)

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

Re: [PATCH] net: ravb: Fix registered interrupt names

[Niklas Söderlund]

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

Re: [PATCH] net: ravb: Fix registered interrupt names

[Sergey Shtylyov]

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

Re: [PATCH] net: ravb: Fix registered interrupt names

[Geert Uytterhoeven]

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

Re: [PATCH] net: ravb: Fix registered interrupt names

[claudiu beznea]

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;
>  }

Re: [PATCH] net: ravb: Fix registered interrupt names

[Andrew Lunn]

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

Re: [PATCH] net: ravb: Fix registered interrupt names

[Jakub Kicinski]

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.

Re: [PATCH] net: ravb: Fix registered interrupt names

[patchwork-bot+netdevbpf]

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