netdev - Re: [PATCH v2] net: dsa: rtl8366rb: Fix compilation problem

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CACRpkdY4WtDGFxTu+Ke_g+YfzL7vY3BGktzUpUAzJuuDgbBtSw@mail.gmail.com>
Date: Thu, 20 Feb 2025 20:16:26 +0100
From: Linus Walleij <linus.walleij@...aro.org>
To: Vladimir Oltean <olteanv@...il.com>
Cc: Alvin Šipraga <alsi@...g-olufsen.dk>, 
	Andrew Lunn <andrew@...n.ch>, "David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, 
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, 
	Luiz Angelo Daros de Luca <luizluca@...il.com>, netdev@...r.kernel.org, 
	kernel test robot <lkp@...el.com>
Subject: Re: [PATCH v2] net: dsa: rtl8366rb: Fix compilation problem

On Thu, Feb 20, 2025 at 7:08 PM Vladimir Oltean <olteanv@...il.com> wrote:
> On Thu, Feb 20, 2025 at 05:02:21PM +0100, Linus Walleij wrote:
> > diff --git a/drivers/net/dsa/realtek/Makefile b/drivers/net/dsa/realtek/Makefile
> > index 35491dc20d6d6ed54855cbf62a0107b13b2a8fda..2fb362bbbc183584317b4bc7792ee638c40fa6a1 100644
> > --- a/drivers/net/dsa/realtek/Makefile
> > +++ b/drivers/net/dsa/realtek/Makefile
> > @@ -12,4 +12,7 @@ endif
> >
> >  obj-$(CONFIG_NET_DSA_REALTEK_RTL8366RB) += rtl8366.o
> >  rtl8366-objs                                 := rtl8366-core.o rtl8366rb.o
> > +ifdef CONFIG_LEDS_CLASS
> > +rtl8366-objs                                 += rtl8366rb-leds.o
> > +endif
> >  obj-$(CONFIG_NET_DSA_REALTEK_RTL8365MB) += rtl8365mb.o
>
> I was expecting to see even more than this. What happens if CONFIG_LEDS_CLASS
> is a module but CONFIG_NET_DSA_REALTEK_RTL8366RB is built-in? Built-in
> code can't call symbols exported by modules, but I don't see that
> restriction imposed, that CONFIG_NET_DSA_REALTEK_RTL8366RB should also
> be a module.

Well spotted (as usual).

I sent a v3 using a new Kconfig symbol and some tricks I learned
to get the dependencies right, have a look!

> > +     init_data.devicename = kasprintf(GFP_KERNEL, "Realtek-%d:0%d:%d",
> > +                                      dp->ds->index, dp->index, led_group);
> > +     if (!init_data.devicename)
> > +             return -ENOMEM;
> > +
> > +     ret = devm_led_classdev_register_ext(priv->dev, &led->cdev, &init_data);
> > +     if (ret) {
> > +             dev_warn(priv->dev, "Failed to init LED %d for port %d",
> > +                      led_group, dp->index);
> > +             return ret;
> > +     }
> > +
> > +     return 0;
> > +}
>
> I know this is just moving the code around, but I was looking at it anyway.

Let's fix what you see with follow-up patches once the critical compile
bug is fixed, it's always good to fix stuff.

> Doesn't init_data.devicename need to be kfree()d after devm_led_classdev_register_ext(),
> regardless of whether it succeeds or fails?

I believe so, I made a patch.

> IMHO, the code could use further simplification if "realtek,disable-leds" were
> honored only with the LED subsystem enabled. I understand the property exists
> prior to that, but it can be ignored if convenient. It seems reasonable to
> leave LEDs as they are if CONFIG_LEDS_CLASS is disabled. But let me know
> if you disagree, it's not a strong opinion.

I added it to the router for a reason: I found that the LEDs were enabled
on the D-Link DIR-685 (boot on default), despite this device does not
even have any physical LEDs mounted.

So it's there to save some (well, probably not much but not non-existent)
leak current in the silicon and pads from the LED driver stages being
activated despite they are unused. If they are even blinking, it's even
more leak current for blinking the non-LEDs, running timers and what
not.

That's why the binding looks like so:

  realtek,disable-leds:
    type: boolean
    description: |
      if the LED drivers are not used in the hardware design,
      this will disable them so they are not turned on
      and wasting power.

This is maybe a bit perfectionist I know...

> Also, I'm not sure there's any reason to set RTL8366RB_LED_BLINKRATE_56MS from
> within the common code instead of from rtl8366rb-leds.c, since the only
> thing that the common code does is disable the LEDs anyway (so why would
> the blink rate matter). But that's again unrelated to this specific change,
> and something which can be handled later in net-next.

I made a patch for this as well, will send them out as soon as the
compilation issue is fixed in mainline.

Thanks for your attention to detail!

Yours,
Linus Walleij