Received-SPF: pass (google.com: best guess record for domain of linux-wireless-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20200403082955.1126339-1-luca@coelho.fi> <iwlwifi.20200403112332.0ed2f71aee7f.I3a4af6b03b87a6bc18db9b1ff9a812f397bee1fc@changeid>
 <CAPuovEJK8AUBPeBBi=BMx-o+9krdA-_NqCJLByWHV2_q2b42Ow@mail.gmail.com> <CAPuovE+Pmh71BNWdKEBJYYVhuowL3XABMyDtk6kbFTzfSR3M+g@mail.gmail.com>
In-Reply-To: <CAPuovE+Pmh71BNWdKEBJYYVhuowL3XABMyDtk6kbFTzfSR3M+g@mail.gmail.com>
From:   Mark Asselstine <asselsm@gmail.com>
Date:   Sat, 4 Apr 2020 19:17:10 -0400
Message-ID: <CAPuovE+VKNvK=3WtFTWmKTGN0xNZWpq6zDM-PG6m4h1MedQWWw@mail.gmail.com>
Subject: Re: [PATCH v5.7 4/8] iwlwifi: mvm: limit maximum queue appropriately
To:     Luca Coelho <luca@coelho.fi>
Cc:     kvalo@codeaurora.org,
        linux-wireless <linux-wireless@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

On Fri, Apr 3, 2020 at 1:10 PM Mark Asselstine <asselsm@gmail.com> wrote:
>
> On Fri, Apr 3, 2020 at 10:38 AM Mark Asselstine <asselsm@gmail.com> wrote:
> >
> > On Fri, Apr 3, 2020 at 4:32 AM Luca Coelho <luca@coelho.fi> wrote:
> > >
> > > From: Johannes Berg <johannes.berg@intel.com>
> > >
> > > Due to some hardware issues, queue 32 isn't usable on devices that have
> > > 32 queues (7000, 8000, 9000 families),
>
> Is this statement really correct? All these devices have 31 queues
> according to (.num_of_queues = 31). Without a HW specification I can't
> be 100% sure but you should have this information within Intel. From
> the details of my patch and my investigation, this should be nack'd
> along with an explanation as to why my fix is not valid.
>
> Mark
>
> > > which is correctly reflected in
> > > the configuration and TX queue initialization.
> >
> > This will not fix the issue on the 1000, 2000, 5000 and 6000 devices.

Just correcting myself here. These use dvm so are OK, but I think we
still have a problem with the 7000, 8000 and 9000 series with the
change as is.

Mark

> > You need further protection on these as their are only 20
> > (IWLAGN_NUM_QUEUES) queues. I sent out a patch on March 19th with a
> > fix.
> >
> > Mark
> >
> > >
> > > However, the firmware API and queue allocation code assumes that there
> > > are 32 queues, and if something actually attempts to use #31 this leads
> > > to a NULL-pointer dereference since it's not allocated.
> > >
> > > Fix this by limiting to 31 in the IWL_MVM_DQA_MAX_DATA_QUEUE, and also
> > > add some code to catch this earlier in the future, if the configuration
> > > changes perhaps.
> > >
> > > Cc: stable@vger.kernel.org # v4.9+
> > > Signed-off-by: Johannes Berg <johannes.berg@intel.com>
> > > Signed-off-by: Luca Coelho <luciano.coelho@intel.com>
> > > ---
> > >  drivers/net/wireless/intel/iwlwifi/fw/api/txq.h | 6 +++---
> > >  drivers/net/wireless/intel/iwlwifi/mvm/sta.c    | 5 +++++
> > >  2 files changed, 8 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/net/wireless/intel/iwlwifi/fw/api/txq.h b/drivers/net/wireless/intel/iwlwifi/fw/api/txq.h
> > > index 73196cbc7fbe..75d958bab0e3 100644
> > > --- a/drivers/net/wireless/intel/iwlwifi/fw/api/txq.h
> > > +++ b/drivers/net/wireless/intel/iwlwifi/fw/api/txq.h
> > > @@ -8,7 +8,7 @@
> > >   * Copyright(c) 2007 - 2014 Intel Corporation. All rights reserved.
> > >   * Copyright(c) 2013 - 2015 Intel Mobile Communications GmbH
> > >   * Copyright(c) 2016 - 2017 Intel Deutschland GmbH
> > > - * Copyright(c) 2019 Intel Corporation
> > > + * Copyright(c) 2019 - 2020 Intel Corporation
> > >   *
> > >   * This program is free software; you can redistribute it and/or modify
> > >   * it under the terms of version 2 of the GNU General Public License as
> > > @@ -31,7 +31,7 @@
> > >   * Copyright(c) 2005 - 2014 Intel Corporation. All rights reserved.
> > >   * Copyright(c) 2013 - 2015 Intel Mobile Communications GmbH
> > >   * Copyright(c) 2016 - 2017 Intel Deutschland GmbH
> > > - * Copyright(c) 2019 Intel Corporation
> > > + * Copyright(c) 2019 - 2020 Intel Corporation
> > >   * All rights reserved.
> > >   *
> > >   * Redistribution and use in source and binary forms, with or without
> > > @@ -99,7 +99,7 @@ enum iwl_mvm_dqa_txq {
> > >         IWL_MVM_DQA_MAX_MGMT_QUEUE = 8,
> > >         IWL_MVM_DQA_AP_PROBE_RESP_QUEUE = 9,
> > >         IWL_MVM_DQA_MIN_DATA_QUEUE = 10,
> > > -       IWL_MVM_DQA_MAX_DATA_QUEUE = 31,
> > > +       IWL_MVM_DQA_MAX_DATA_QUEUE = 30,
> > >  };
> > >
> > >  enum iwl_mvm_tx_fifo {
> > > diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/sta.c b/drivers/net/wireless/intel/iwlwifi/mvm/sta.c
> > > index 64ef3f3ba23b..251d6fbb1da5 100644
> > > --- a/drivers/net/wireless/intel/iwlwifi/mvm/sta.c
> > > +++ b/drivers/net/wireless/intel/iwlwifi/mvm/sta.c
> > > @@ -722,6 +722,11 @@ static int iwl_mvm_find_free_queue(struct iwl_mvm *mvm, u8 sta_id,
> > >
> > >         lockdep_assert_held(&mvm->mutex);
> > >
> > > +       if (WARN(maxq >= mvm->trans->trans_cfg->base_params->num_of_queues,
> > > +                "max queue %d >= num_of_queues (%d)", maxq,
> > > +                mvm->trans->trans_cfg->base_params->num_of_queues))
> > > +               maxq = mvm->trans->trans_cfg->base_params->num_of_queues - 1;
> > > +
> > >         /* This should not be hit with new TX path */
> > >         if (WARN_ON(iwl_mvm_has_new_tx_api(mvm)))
> > >                 return -ENOSPC;
> > > --
> > > 2.25.1
> > >