Received-SPF: pass (google.com: domain of linux-wireless-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>
In-Reply-To: <f0a2cb7ea606f1a284d4c23cbf983da2954ce9b6.1598420968.git.mchehab+huawei@kernel.org>
From:   Steve deRosier <derosier@gmail.com>
Date:   Thu, 27 Aug 2020 08:48:30 -0700
Message-ID: <CALLGbRL+duiHFd3w7hcD=u47k+JM5rLpOkMrRpW0aQm=oTfUnA@mail.gmail.com>
Subject: Re: [PATCH] Revert "wlcore: Adding suppoprt for IGTK key in wlcore driver"
To:     Mauro Carvalho Chehab <mchehab+huawei@kernel.org>,
        Kalle Valo <kvalo@codeaurora.org>
Cc:     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-wireless-owner@vger.kernel.org
Precedence: bulk

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.
 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.

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.

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.

- Steve