| Message ID | iwlwifi.20200403112332.0ed2f71aee7f.I3a4af6b03b87a6bc18db9b1ff9a812f397bee1fc@changeid (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Luca Coelho |
| Headers | show |
| Series | iwlwifi: fixes intended for v5.7 2020-04-03 | expand |
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), 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. 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 >
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. > 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 > >
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 > > >
On Fri, 2020-04-03 at 13:10 -0400, Mark Asselstine 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? No, it should've said "queue 31" since they're numbered 0-based ... > All these devices have 31 queues > according to (.num_of_queues = 31). Well, they were supposed to have 32, but there's some issue with the last one. I don't really even remember what's up with it, but we just never use it. > 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. I don't see any real difference to your fix? Your fix marks them as used before, whereas mine just avoids looking at them. johannes
On Tue, 2020-04-14 at 13:29 +0200, Johannes Berg wrote: > On Fri, 2020-04-03 at 13:10 -0400, Mark Asselstine 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? > > No, it should've said "queue 31" since they're numbered 0-based ... I will fix this in the commit message and send v2 of the entire series today. -- Cheers, Luca.
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;