Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <f0a2cb7ea606f1a284d4c23cbf983da2954ce9b6.1598420968.git.mchehab+huawei@kernel.org>
 <CALLGbRL+duiHFd3w7hcD=u47k+JM5rLpOkMrRpW0aQm=oTfUnA@mail.gmail.com> <20200827194225.281eb7dc@coco.lan>
In-Reply-To: <20200827194225.281eb7dc@coco.lan>
From:   Steve deRosier <derosier@gmail.com>
Date:   Thu, 27 Aug 2020 13:36:28 -0700
Message-ID: <CALLGbRLsQpdtrcV9ydz4KJ4A9uaj4P1EhbF0_yMxcdLvOmnY9Q@mail.gmail.com>
Subject: Re: [PATCH] Revert "wlcore: Adding suppoprt for IGTK key in wlcore driver"
To:     Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
Cc:     Kalle Valo <kvalo@codeaurora.org>, linuxarm@huawei.com,
        mauro.chehab@huawei.com, John Stultz <john.stultz@linaro.org>,
        Manivannan Sadhasivam <mani@kernel.org>,
        "David S. Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>, Maital Hahn <maitalm@ti.com>,
        "Gustavo A. R. Silva" <gustavo@embeddedor.com>,
        Raz Bouganim <r-bouganim@ti.com>,
        Tony Lindgren <tony@atomide.com>,
        Dinghao Liu <dinghao.liu@zju.edu.cn>,
        Johannes Berg <johannes.berg@intel.com>,
        Fuqian Huang <huangfq.daxian@gmail.com>,
        linux-wireless <linux-wireless@vger.kernel.org>,
        Network Development <netdev@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Mauro,

On Thu, Aug 27, 2020 at 10:42 AM Mauro Carvalho Chehab
<mchehab+huawei@kernel.org> wrote:
>
> Em Thu, 27 Aug 2020 08:48:30 -0700
> Steve deRosier <derosier@gmail.com> escreveu:
>
> > On Tue, Aug 25, 2020 at 10:49 PM Mauro Carvalho Chehab
> > <mchehab+huawei@kernel.org> wrote:
> > >
> > > This patch causes a regression betwen Kernel 5.7 and 5.8 at wlcore:
> > > with it applied, WiFi stops working, and the Kernel starts printing
> > > this message every second:
> > >
> > >    wlcore: PHY firmware version: Rev 8.2.0.0.242
> > >    wlcore: firmware booted (Rev 8.9.0.0.79)
> > >    wlcore: ERROR command execute failure 14
> >
> > Only if NO firmware for the device in question supports the `KEY_IGTK`
> > value, then this revert is appropriate. Otherwise, it likely isn't.
>
> Yeah, that's what I suspect too: some specific firmware is required
> for KEY_IGTK to work.
>
> >  My suspicion is that the feature that `KEY_IGTK` is enabling is
> > specific to a newer firmware that Mauro hasn't upgraded to. What the
> > OP should do is find the updated firmware and give it a try.
>
> I didn't try checking if linux-firmware tree has a newer version on
> it. I'm using Debian Bullseye on this device. So, I suspect that
> it may have a relatively new firmware.
>
> Btw, that's also the version that came together with Fedora 32:
>
>         $ strings /lib/firmware/ti-connectivity/wl18xx-fw-4.bin |grep FRev
>         FRev 8.9.0.0.79
>         FRev 8.2.0.0.242
>
> Looking at:
>         https://git.ti.com/cgit/wilink8-wlan/wl18xx_fw/
>
> It sounds that there's a newer version released this year:
>
>         2020-05-28      Updated to FW 8.9.0.0.81
>         2018-07-29      Updated to FW 8.9.0.0.79
>
> However, it doesn't reached linux-firmware upstream yet:
>
>         $ git log --pretty=oneline ti-connectivity/wl18xx-fw-4.bin
>         3a5103fc3c29 wl18xx: update firmware file 8.9.0.0.79
>         65b1c68c63f9 wl18xx: update firmware file 8.9.0.0.76
>         dbb85a5154a5 wl18xx: update firmware file
>         69a250dd556b wl18xx: update firmware file
>         dbe3f134bb69 wl18xx: update firmware file, remove conf file
>         dab4b79b3fbc wl18xx: add version 4 of the wl18xx firmware
>
> > AND - since there's some firmware the feature doesn't work with, the
> > driver should be fixed to detect the running firmware version and not
> > do things that the firmware doesn't support.  AND the firmware writer
> > should also make it so the firmware doesn't barf on bad input and
> > instead rejects it politely.
>
> Agreed. The main issue here seems to be that the current patch
> assumes that this feature is available. A proper approach would
> be to check if this feature is available before trying to use it.
>
> Now, I dunno if version 8.9.0.0.81 has what's required for it to
> work - or if KEY_IGTK require some custom firmware version.
>
> If it works with such version, one way would be to add a check
> for this specific version, disabling KEY_IGTK otherwise.
>
> Also, someone from TI should be sending the newer version to
> be added at linux-firmware.
>
> I'll try to do a test maybe tomorrow.
>

I think we're totally agreed on all of the above points.
Fundamentally: the orig patch should've been coded defensively and
tested properly since clearly it causes certain firmwares to break.
Be nice if TI would both update the firmware and also update the
driver to detect the relevant version for features.  I don't know
about this one, but I do know the QCA firmwares (and others) have a
set of feature flags that are detected by the drivers to determine
what is supported.

I look forward to hearing the results of your test.  This whole thing
has gotten me interested. I'd be tempted to pull out the relevant dev
boards and play with them myself, but IIRC they got sent back to a
previous employer and I don't have access to them anymore.


> > But I will say I'm making an educated guess; while I have played with
> > the TI devices in the past, it was years ago and I won't claim to be
> > an expert. I also am unable to fix it myself at this time.
> >
> > I'd just rather see it fixed properly instead of a knee-jerk reaction
> > of reverting it simply because the OP doesn't have current firmware.
>
> > And let's revisit the discussion of having a kernel splat because an
> > unrelated piece of code fails yet the driver does exactly what it is
> > supposed to do. We shouldn't be dumping registers and stack-trace when
> > the code that crashed has nothing to do with the registers and
> > stack-trace outputted. It is a false positive.  A simple printk WARN
> > or ERROR should output notifying us that the chip firmware has crashed
> > and why.  IMHO.
>
> Thanks,
> Mauro

Thanks,
- Steve