Re: [PATCH for-4.4-stable] ath10k: restore QCA9880-AR1A (v1) detection

[Greg KH <gregkh@linuxfoundation.org>]

On Tue, Dec 03, 2019 at 09:47:38PM +0100, Christian Lamparter wrote:
> On Mon, Dec 2, 2019 at 7:42 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > On Fri, Nov 29, 2019 at 09:53:50PM +0100, Christian Lamparter wrote:
> > > commit f8914a14623a79b73f72b2b1ee4cd9b2cb91b735 upstream
> > > ---
> > > >From f8914a14623a79b73f72b2b1ee4cd9b2cb91b735 Mon Sep 17 00:00:00 2001
> > > From: Christian Lamparter <chunkeey@gmail.com>
> > > Date: Mon, 25 Mar 2019 13:50:19 +0100
> > > Subject: [PATCH 4.4] ath10k: restore QCA9880-AR1A (v1) detection
> > > To: linux-wireless@vger.kernel.org,
> > >     ath10k@lists.infradead.org
> > > Cc: Kalle Valo <kvalo@codeaurora.org>
> > >
> > > This patch restores the old behavior that read
> > > the chip_id on the QCA988x before resetting the
> > > chip. This needs to be done in this order since
> > > the unsupported QCA988x AR1A chips fall off the
> > > bus when resetted. Otherwise the next MMIO Op
> > > after the reset causes a BUS ERROR and panic.
> > >
> > > Cc: stable@vger.kernel.org # 4.4
> > > Fixes: 1a7fecb766c8 ("ath10k: reset chip before reading chip_id in probe")
> > > Signed-off-by: Christian Lamparter <chunkeey@gmail.com>
> > > ---
> > >  drivers/net/wireless/ath/ath10k/pci.c | 36 +++++++++++++++++++--------
> > >  1 file changed, 25 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
> > > --- a/drivers/net/wireless/ath/ath10k/pci.c   2019-09-08 00:07:21.374565470 +0200
> > > +++ b/drivers/net/wireless/ath/ath10k/pci.c   2019-09-08 00:17:15.365912133 +0200
> > > @@ -2988,12 +2988,13 @@ static int ath10k_pci_probe(struct pci_d
> > >       struct ath10k_pci *ar_pci;
> > >       enum ath10k_hw_rev hw_rev;
> > >       u32 chip_id;
> > > -     bool pci_ps;
> > > +     bool pci_ps, is_qca988x = false;
> > >
> > >       switch (pci_dev->device) {
> > >       case QCA988X_2_0_DEVICE_ID:
> > >               hw_rev = ATH10K_HW_QCA988X;
> > >               pci_ps = false;
> > > +             is_qca988x = true;
> > >               break;
> > >       case QCA6164_2_1_DEVICE_ID:
> > >       case QCA6174_2_1_DEVICE_ID:
> > > @@ -3087,6 +3088,19 @@ static int ath10k_pci_probe(struct pci_d
> > >               goto err_deinit_irq;
> > >       }
> > >
> > > +     /* Read CHIP_ID before reset to catch QCA9880-AR1A v1 devices that
> > > +      * fall off the bus during chip_reset. These chips have the same pci
> > > +      * device id as the QCA9880 BR4A or 2R4E. So that's why the check.
> > > +      */
> > > +     if (is_qca988x) {
> > > +             chip_id = ath10k_pci_soc_read32(ar, SOC_CHIP_ID_ADDRESS);
> > > +             if (chip_id != 0xffffffff) {
> > > +                     if (!ath10k_pci_chip_is_supported(pdev->device,
> > > +                                                       chip_id))
> > > +                             goto err_unsupported;
> > > +             }
> > > +     }
> > > +
> > >       ret = ath10k_pci_chip_reset(ar);
> > >       if (ret) {
> > >               ath10k_err(ar, "failed to reset chip: %d\n", ret);
> > > @@ -3099,11 +3113,8 @@ static int ath10k_pci_probe(struct pci_d
> > >               goto err_free_irq;
> > >       }
> > >
> > > -     if (!ath10k_pci_chip_is_supported(pdev->device, chip_id)) {
> > > -             ath10k_err(ar, "device %04x with chip_id %08x isn't supported\n",
> > > -                        pdev->device, chip_id);
> > > -             goto err_free_irq;
> > > -     }
> > > +     if (!ath10k_pci_chip_is_supported(pdev->device, chip_id))
> > > +             goto err_unsupported;
> > >
> > >       ret = ath10k_core_register(ar, chip_id);
> > >       if (ret) {
> > > @@ -3113,6 +3124,10 @@ static int ath10k_pci_probe(struct pci_d
> > >
> > >       return 0;
> > >
> > > +err_unsupported:
> > > +     ath10k_err(ar, "device %04x with chip_id %08x isn't supported\n",
> > > +                pdev->device, bus_params.chip_id);
> >
> > Backports are great, but as I mentioned before, this breaks the build,
> > so we can't take it:
> >
> > drivers/net/wireless/ath/ath10k/pci.c: In function ‘ath10k_pci_probe’:
> > drivers/net/wireless/ath/ath10k/pci.c:3129:20: error: ‘bus_params’ undeclared (first use in this function); did you mean ‘key_params’?
> >  3129 |      pdev->device, bus_params.chip_id);
> >       |                    ^~~~~~~~~~
> >       |                    key_params
> >
> Ok, I see. That "bus_params." there has to go so just "chip_id" remains.
> 
> But, I'm sorry that I caused this havoc.
> For a little bit of background: Getting the QCA9880-AR1A and the board
> it works back, will
> take until next year. Since this card either doesn't fit (it's
> oversized mini-pcie) or flat-out
> doesn't work (it crashes the vast majority of x86/AMD64 system during post).
> So doing tests with the hardware involves going through custom OpenWrt-builds.
> 
> > Please fix this up and resend backports again.
> I guess, I can do that. Question is, given the bug has been around
> since 2015 and the
> response for this patch has been negative (even from those who have
> crashes from it).
> I'm thinking that I'll rather drop this and take the time off by doing
> something else for
> a while.

That's fine, if someone has noticed this, then they just can use a new
kernel version.

thanks,

greg k-h