Date:   Mon, 25 Jan 2021 16:04:42 +0100
From:   Krzysztof Kozlowski <krzk@...nel.org>
To:     Arnd Bergmann <arnd@...nel.org>
Cc:     QCA ath9k Development <ath9k-devel@....qualcomm.com>,
        Kalle Valo <kvalo@...eaurora.org>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Johannes Berg <johannes@...solutions.net>,
        Arnd Bergmann <arnd@...db.de>,
        Masahiro Yamada <masahiroy@...nel.org>,
        Flavio Suligoi <f.suligoi@...m.it>,
        linux-wireless <linux-wireless@...r.kernel.org>,
        Networking <netdev@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] ath9k: fix build error with LEDS_CLASS=m

On Mon, 25 Jan 2021 at 15:38, Arnd Bergmann <arnd@...nel.org> wrote:
>
> On Mon, Jan 25, 2021 at 2:27 PM Krzysztof Kozlowski <krzk@...nel.org> wrote:
> > On Mon, 25 Jan 2021 at 14:09, Arnd Bergmann <arnd@...nel.org> wrote:
> > > On Mon, Jan 25, 2021 at 12:40 PM Krzysztof Kozlowski <krzk@...nel.org> wrote:
> > > > On Mon, 25 Jan 2021 at 12:36, Arnd Bergmann <arnd@...nel.org> wrote:
> > > > But we do not want to have this dependency (selecting MAC80211_LEDS).
> > > > I fixed this problem here:
> > > > https://lore.kernel.org/lkml/20201227143034.1134829-1-krzk@kernel.org/
> > > > Maybe let's take this approach?
> > >
> > > Generally speaking, I don't like to have a device driver specific Kconfig
> > > setting 'select' a subsystem', for two reasons:
> > >
> > > - you suddenly get asked for tons of new LED specific options when
> > >   enabling seemingly benign options
> > >
> > > - Mixing 'depends on' and 'select' leads to bugs with circular
> > >   dependencies that usually require turning some other 'select'
> > >   into 'depends on'.
> > >
> > > The problem with LEDS_CLASS in particular is that there is a mix of drivers
> > > using one vs the other roughly 50:50.
> >
> > Yes, you are right, I also don't like it. However it was like this
> > before my commit so I am not introducing a new issue. The point is
> > that in your choice the MAC80211_LEDS will be selected if LEDS_CLASS
> > is present, which is exactly what I was trying to fix/remove. My WiFi
> > dongle does not have a LED and it causes a periodic (every second)
> > event. However I still have LEDS_CLASS for other LEDS in the system.
>
> What is the effect of this lost event every second? If it causes some
> runtime warning or other problem, then neither of our fixes would
> solve it completely, because someone with a distro kernel would
> see the same issue when they have the symbol enabled but no
> physical LED in the device.

I meant that having MAC80211_LEDS selected causes the ath9k driver to
toggle on/off the WiFi LED. Every second, regardless whether it's
doing something or not. In my setup, I have problems with a WiFi
dongle somehow crashing (WiFi disappears, nothing comes from the
dongle... maybe it's Atheros FW, maybe some HW problem) and I found
this LED on/off slightly increases the chances of this dongle-crash.
That was the actual reason behind my commits.

Second reason is that I don't want to send USB commands every second
when the device is idle. It unnecessarily consumes power on my
low-power device.

Of course another solution is to just disable the trigger via sysfs
LED API. It would also work but my patch allows entire code to be
compiled-out (which was conditional in ath9k already).

Therefore the patch I sent allows the ath9k LED option to be fully
choosable. Someone wants every-second-LED-blink, sure, enable
ATH9K_LEDS and you have it. Someone wants to reduce the kernel size,
don't enable ATH9K_LEDS.

Best regards,
Krzysztof

Powered by blists - more mailing lists