Hi Mauro,

On Thu, Aug 27, 2020 at 10:42 AM Mauro Carvalho Chehab
[off-list ref] wrote:

Em Thu, 27 Aug 2020 08:48:30 -0700
Steve deRosier [off-list ref] escreveu:

quoted

On Tue, Aug 25, 2020 at 10:49 PM Mauro Carvalho Chehab
[off-list ref] wrote:

quoted

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.

quoted

 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

quoted

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.

quoted

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.

quoted

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