Re: [PATCH 2/2] iwlwifi: dbg: Mark ucode tlv data as const
Takashi Iwai wrote: > The ucode TLV data may be read-only and should be treated as const > pointers, but currently a few code forcibly cast to the writable > pointer unnecessarily. This gave developers a wrong impression as if > it can be modified, resulting in crashing regressions already a couple > of times. > > This patch adds the const prefix to those cast pointers, so that such > attempt can be caught more easily in future. > > Signed-off-by: Takashi Iwai > Acked-by: Luca Coelho Patch applied to iwlwifi-next.git, thanks. 71b6254a6c98 iwlwifi: dbg: Mark ucode tlv data as const
Re: [PATCH v2 00/29] [Set 1,2,3] Rid W=1 warnings in Wireless
On Tue, 2020-10-06 at 10:10 +0300, Kalle Valo wrote:
> Lee Jones writes:
>
> > On Tue, 06 Oct 2020, Kalle Valo wrote:
> >
> > > Lee Jones writes:
> > >
> > > > On Thu, 10 Sep 2020, Lee Jones wrote:
> > > >
> > > > > This is a rebased/re-worked set of patches which have been
> > > > > previously posted to the mailing list(s).
> > > > >
> > > > > This set is part of a larger effort attempting to clean-up W=1
> > > > > kernel builds, which are currently overwhelmingly riddled with
> > > > > niggly little warnings.
> > > > >
> > > > > There are quite a few W=1 warnings in the Wireless. My plan
> > > > > is to work through all of them over the next few weeks.
> > > > > Hopefully it won't be too long before drivers/net/wireless
> > > > > builds clean with W=1 enabled.
> > > > >
> > > > > Lee Jones (29):
> > > > > iwlwifi: dvm: Demote non-compliant kernel-doc headers
> > > > > iwlwifi: rs: Demote non-compliant kernel-doc headers
> > > > > iwlwifi: dvm: tx: Demote non-compliant kernel-doc headers
> > > > > iwlwifi: dvm: lib: Demote non-compliant kernel-doc headers
> > > > > iwlwifi: calib: Demote seemingly unintentional kerneldoc header
> > > > > wil6210: Fix a couple of formatting issues in 'wil6210_debugfs_init'
> > > > > iwlwifi: dvm: sta: Demote a bunch of nonconformant kernel-doc
> > > > > headers
> > > > > iwlwifi: mvm: ops: Remove unused static struct 'iwl_mvm_debug_names'
> > > > > iwlwifi: dvm: Demote a couple of nonconformant kernel-doc headers
> > > > > iwlwifi: mvm: utils: Fix some doc-rot
> > > > > iwlwifi: dvm: scan: Demote a few nonconformant kernel-doc headers
> > > > > iwlwifi: dvm: rxon: Demote non-conformant kernel-doc headers
> > > > > iwlwifi: mvm: tx: Demote misuse of kernel-doc headers
> > > > > iwlwifi: dvm: devices: Fix function documentation formatting issues
> > > > > iwlwifi: iwl-drv: Provide descriptions debugfs dentries
> > > > > wil6210: wmi: Fix formatting and demote non-conforming function
> > > > > headers
> > > > > wil6210: interrupt: Demote comment header which is clearly not
> > > > > kernel-doc
> > > > > wil6210: txrx: Demote obvious abuse of kernel-doc
> > > > > wil6210: txrx_edma: Demote comments which are clearly not kernel-doc
> > > > > wil6210: pmc: Demote a few nonconformant kernel-doc function headers
> > > > > wil6210: wil_platform: Demote kernel-doc header to standard comment
> > > > > block
> > > > > wil6210: wmi: Correct misnamed function parameter 'ptr_'
> > > > > ath6kl: wmi: Remove unused variable 'rate'
> > > > > ath9k: ar9002_initvals: Remove unused array
> > > > > 'ar9280PciePhy_clkreq_off_L1_9280'
> > > > > ath9k: ar9001_initvals: Remove unused array 'ar5416Bank6_9100'
> > > > > ath9k: ar5008_initvals: Remove unused table entirely
> > > > > ath9k: ar5008_initvals: Move ar5416Bank{0,1,2,3,7} to where they are
> > > > > used
> > > > > brcmsmac: phytbl_lcn: Remove unused array 'dot11lcn_gain_tbl_rev1'
> > > > > brcmsmac: phy_lcn: Remove unused variable
> > > > > 'lcnphy_rx_iqcomp_table_rev0'
> > > >
> > > > What's happening with all of these iwlwifi patches?
> > > >
> > > > Looks like they are still not applied.
> > >
> > > Luca (CCed) takes iwlwifi patches to his iwlwifi tree.
> >
> > Thanks Kalle.
> >
> > Luca,
> >
> > Do you know why these patches have not been applied yet? Do you
> > plan on applying them this week? -rc1 is not due for release for
> > nearly 3 weeks now that Linus tagged an -rc8.
>
> I can also take Lee's patches directly to wireless-drivers-next, if
> that's easier for Luca.
Hi,
Yes, please take these patches directly. It simplifies things.
Thanks!
--
Cheers,
Luca.
Re: [PATCH] iwlwifi: mvm: Remove unused inline function iwl_mvm_tid_to_ac_queue
YueHaibing wrote:
> commit cfbc6c4c5b91 ("iwlwifi: mvm: support mac80211 TXQs model")
> left behind this, remove it.
>
> Signed-off-by: YueHaibing
Patch applied to iwlwifi-next.git, thanks.
f12694634153 iwlwifi: mvm: Remove unused inline function iwl_mvm_tid_to_ac_queue
Re: [PATCH 6/9] net: wireless: intel: fix wiki website url
Flavio Suligoi wrote: > In some Intel files, the wiki url is still the old > "wireless.kernel.org" instead of the new > "wireless.wiki.kernel.org" > > Signed-off-by: Flavio Suligoi Patch applied to iwlwifi-next.git, thanks. e00c6d8d491b net: wireless: intel: fix wiki website url
Re: [PATCH] iwlwifi: remove redundant assignment to variable bufsz
On Thu, 2019-08-01 at 17:44 +0100, Colin King wrote:
> From: Colin Ian King
>
> The variable bufsz is being initialized with a value that is never
> read and it is being updated later with a new value. The
> initialization is redundant and can be removed.
>
> Addresses-Coverity: ("Unused value")
> Signed-off-by: Colin Ian King
> ---
Thanks! I applied this to our internal tree and it will reach the
mainline following our usual upstreaming process.
--
Cheers,
Luca.
Re: [PATCH] iwlwifi: trans: fix killer series loadded incorrect firmware
On Mon, 2019-05-13 at 21:33 +0800, Cyrus Lien wrote:
> Killer series loadded IWL_22000_HR_B_FW_PRE prefixed firmware instead
> IWL_CC_A_FW_PRE prefixed firmware.
>
> Add killer series to the check logic as iwl_ax200_cfg_cc.
>
> Signed-off-by: Cyrus Lien
> ---
> drivers/net/wireless/intel/iwlwifi/pcie/trans.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/intel/iwlwifi/pcie/trans.c
> b/drivers/net/wireless/intel/iwlwifi/pcie/trans.c
> index 79c1dc05f948..576c2186b6bf 100644
> --- a/drivers/net/wireless/intel/iwlwifi/pcie/trans.c
> +++ b/drivers/net/wireless/intel/iwlwifi/pcie/trans.c
> @@ -3565,7 +3565,9 @@ struct iwl_trans *iwl_trans_pcie_alloc(struct
> pci_dev *pdev,
> }
> } else if (CSR_HW_RF_ID_TYPE_CHIP_ID(trans->hw_rf_id) ==
> CSR_HW_RF_ID_TYPE_CHIP_ID(CSR_HW_RF_ID_TYPE_HR) &&
> -(trans->cfg != &iwl_ax200_cfg_cc ||
> +((trans->cfg != &iwl_ax200_cfg_cc &&
> + trans->cfg != &killer1650x_2ax_cfg &&
> + trans->cfg != &killer1650w_2ax_cfg) ||
> trans->hw_rev == CSR_HW_REV_TYPE_QNJ_B0)) {
> u32 hw_status;
>
Thanks for your patch, Cyrus! We already have an identical patch in our
internal tree and it will reach the mainline soon.
--
Cheers,
Luca.
Re: [PATCH] net: wireless: iwlwifi: Fix double-free problems in iwl_req_fw_callback()
On Sat, 2019-05-04 at 17:33 +0800, Jia-Ju Bai wrote: > In the error handling code of iwl_req_fw_callback(), > iwl_dealloc_ucode() > is called to free data. In iwl_drv_stop(), iwl_dealloc_ucode() is > called > again, which can cause double-free problems. > > To fix this bug, the call to iwl_dealloc_ucode() in > iwl_req_fw_callback() is deleted. > > This bug is found by a runtime fuzzing tool named FIZZER written by > us. > > Signed-off-by: Jia-Ju Bai > --- > drivers/net/wireless/intel/iwlwifi/iwl-drv.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/drivers/net/wireless/intel/iwlwifi/iwl-drv.c > b/drivers/net/wireless/intel/iwlwifi/iwl-drv.c > index 689a65b11cc3..4fd1737d768b 100644 > --- a/drivers/net/wireless/intel/iwlwifi/iwl-drv.c > +++ b/drivers/net/wireless/intel/iwlwifi/iwl-drv.c > @@ -1579,7 +1579,6 @@ static void iwl_req_fw_callback(const struct > firmware *ucode_raw, void *context) > goto free; > > out_free_fw: > - iwl_dealloc_ucode(drv); > release_firmware(ucode_raw); > out_unbind: > complete(&drv->request_firmware_complete); Thanks! Applied to our internal tree and will reach the mainline following our normal upstreaming process. -- Cheers, Luca.
Re: [PATCH] iwlwifi: fix 64-bit division
On Tue, 2019-03-05 at 13:11 +0200, Kalle Valo wrote:
> Arnd Bergmann writes:
>
> > do_div() expects unsigned operands and otherwise triggers a warning
> > like:
> >
> > drivers/net/wireless/intel/iwlwifi/mvm/ftm-initiator.c:465:2:
> > error: comparison of distinct pointer types ('typeof ((rtt_avg)) *'
> > (aka 'long long *') and 'uint64_t *' (aka 'unsigned long long *'))
> > [-Werror,-Wcompare-distinct-pointer-types]
> > do_div(rtt_avg, );
> > ^
> > include/asm-generic/div64.h:222:28: note: expanded from macro
> > 'do_div'
> > (void)(((typeof((n)) *)0) == ((uint64_t *)0)); \
> >~~ ^ ~~~
> > 1 error generated.
> >
> > Change the do_div() to the simpler div_s64() that can handle
> > negative inputs correctly.
> >
> > Fixes: 937b10c0de68 ("iwlwifi: mvm: add debug prints for FTM")
> > Signed-off-by: Arnd Bergmann
>
> Luca, can I take this directly?
Yeah, I guess to make things simpler, and since you're planning to send
fixes to 5.1 already anyway, you can just take this one. I'll assign
it to you in patchwork.
Arnd, this way you'll get it earlier. ;)
--
Cheers,
Luca.
Re: [PATCH] iwlwifi: mvm: Use div64_s64 instead of do_div in iwl_mvm_debug_range_resp
On Tue, 2019-02-19 at 11:05 -0800, Nick Desaulniers wrote:
> On Tue, Feb 19, 2019 at 10:21 AM Nathan Chancellor
> wrote:
> > Clang warns:
> >
> > drivers/net/wireless/intel/iwlwifi/mvm/ftm-initiator.c:465:2:
> > warning:
> > comparison of distinct pointer types ('typeof ((rtt_avg)) *' (aka
> > 'long
> > long *') and 'uint64_t *' (aka 'unsigned long long *'))
> > [-Wcompare-distinct-pointer-types]
> > do_div(rtt_avg, );
> > ^
> > include/asm-generic/div64.h:222:28: note: expanded from macro
> > 'do_div'
> > (void)(((typeof((n)) *)0) == ((uint64_t *)0)); \
> >~~ ^ ~~~
> > 1 warning generated.
> >
> > do_div expects an unsigned dividend. Use div64_s64, which expects a
> > signed dividend.
>
> Eh, IIRC, signed vs unsigned division has implications for rounding
> towards zero or not, but I doubt that the round trip time average
> (RTT
> avg) should ever be negative. General rule of thumb for C is to keep
> arithmetic signed (even when working with non zero values), so rather
> than make the literal () a unsigned long, I agree with your
> change
> to keep the division signed as well. Thanks for the fix.
> Reviewed-by: Nick Desaulniers
Thanks for the patch and for the review. I've applied this to our
internal tree and it will be sent upstreaming following our normal
upstreaming process.
--
Cheers,
Luca.
Re: [PATCH] net: wireless: prefix header search paths with $(srctree)/
On Mon, 2019-01-28 at 10:21 +0200, Kalle Valo wrote:
> Masahiro Yamada writes:
>
> > Currently, the Kbuild core manipulates header search paths in a
> > crazy
> > way [1].
> >
> > To fix this mess, I want all Makefiles to add explicit $(srctree)/
> > to
> > the search paths in the srctree. Some Makefiles are already written
> > in
> > that way, but not all. The goal of this work is to make the
> > notation
> > consistent, and finally get rid of the gross hacks.
> >
> > Having whitespaces after -I does not matter since commit
> > 48f6e3cf5bc6
> > ("kbuild: do not drop -I without parameter").
> >
> > I also removed one header search path in:
> >
> > drivers/net/wireless/broadcom/brcm80211/brcmutil/Makefile
> >
> > I was able to compile without it.
> >
> > [1]: https://patchwork.kernel.org/patch/9632347/
> >
> > Signed-off-by: Masahiro Yamada
> > ---
> >
> > drivers/net/wireless/broadcom/brcm80211/brcmfmac/Makefile | 4 ++--
> > drivers/net/wireless/broadcom/brcm80211/brcmsmac/Makefile | 6 +++-
> > --
> > drivers/net/wireless/broadcom/brcm80211/brcmutil/Makefile | 4 +---
> > drivers/net/wireless/intel/iwlwifi/dvm/Makefile | 2 +-
> > drivers/net/wireless/intel/iwlwifi/mvm/Makefile | 2 +-
> > drivers/net/wireless/realtek/rtl818x/rtl8180/Makefile | 2 +-
> > drivers/net/wireless/realtek/rtl818x/rtl8187/Makefile | 2 +-
> > 7 files changed, 10 insertions(+), 12 deletions(-)
>
> Luca, is it ok if I take this to wireless-drivers-next even though it
> touches iwlwifi makefiles?
Sure, it's much easier like that.
Acked-by: Luca Coelho
--
Cheers,
Luca.
Re: [PATCH] iwlwifi: fix false-positive maybe-uninitialized warning
On Mon, 2018-12-10 at 21:39 +0100, Arnd Bergmann wrote: > With CONFIG_NO_AUTO_INLINE, we run into a silly warning when > gcc fails to remember that n_profiles is constant across > the function call to iwl_mvm_sar_set_profile: > > drivers/net/wireless/intel/iwlwifi/mvm/fw.c: In function > 'iwl_mvm_sar_get_ewrd_table': > drivers/net/wireless/intel/iwlwifi/mvm/fw.c:746:9: error: 'ret' may > be used uninitialized in this function [-Werror=maybe-uninitialized] > > Marking that function 'inline' avoids the warning. > > Signed-off-by: Arnd Bergmann > --- Thanks! This has been applied in our internal tree and will reach the mainline following our normal upstreaming process. -- Cheers, Luca.
Re: linux-next: build warnings after merge of the wireless-drivers-next tree
On Wed, 2018-12-19 at 08:31 +, Grumbach, Emmanuel wrote:
> > Stephen Rothwell writes:
> >
> > > On Fri, 30 Nov 2018 12:05:55 +1100 Stephen Rothwell
> > wrote:
> > > > After merging the wireless-drivers-next tree, today's linux-next
> > > > build
> > > > (x86_64 allmodconfig) produced these warnings:
> > > >
> > > > drivers/net/wireless/intel/iwlwifi/iwl-drv.c: In function
> > 'iwl_parse_tlv_firmware':
> > > > drivers/net/wireless/intel/iwlwifi/iwl-drv.c:1098:7: warning: this
> > statement may fall through [-Wimplicit-fallthrough=]
> > > > if (iwlwifi_mod_params.enable_ini)
> > > >^
> > > > drivers/net/wireless/intel/iwlwifi/iwl-drv.c:1100:3: note: here
> > > >default:
> > > >^~~
> > > > drivers/net/wireless/intel/iwlwifi/iwl-dbg-tlv.c: In function
> > 'iwl_parse_fw_dbg_tlv':
> > > > drivers/net/wireless/intel/iwlwifi/iwl-dbg-tlv.c:203:4: warning: this
> > statement may fall through [-Wimplicit-fallthrough=]
> > > > iwl_fw_dbg_copy_tlv(trans, tlv, true);
> > > > ^
> > > > drivers/net/wireless/intel/iwlwifi/iwl-dbg-tlv.c:204:3: note: here
> > > >default:
> > > >^~~
> > > >
> > > > Introduced by commit
> > > >
> > > > f14cda6f3b31 ("iwlwifi: trans: parse and store debug ini TLVs")
> > > >
> > > > These are noted because I use -Wimplict-fallthrough
> > > >
> > > > The warnings can be suppressed by adding a comment like
> > > > /* fall through */
> > > > at the appropriate place to indicate that the fallthough is intended.
> > >
> > > I am still seeing these warnings (but in the net-next tree now) and I
> > > do not see a fix patch in the wireless-drivers-next tree.
> >
> > Luca did submit a patch[1] for cfg80211 to fix those but I don't see any
> > patches for iwlwifi (even in the one pending pull request he sent), not sure
> > what happened. I know that Luca is already on holidays but adding
> > Emmanuel, maybe he can help here?
> >
>
> Just sent a patch.
Those patches were still queued for upstreaming and I didn't think they
were that urgent, so I didn't pick them.
--
Luca.
Re: [PATCH 07/33] iwlwifi: mvm: use match_string() helper
On Mon, 2018-05-21 at 19:57 +0800, Yisheng Xie wrote:
> match_string() returns the index of an array for a matching string,
> which can be used intead of open coded variant.
>
> Cc: Kalle Valo
> Cc: Intel Linux Wireless
> Cc: Johannes Berg
> Cc: Emmanuel Grumbach
> Cc: linux-wirel...@vger.kernel.org
> Cc: netdev@vger.kernel.org
> Signed-off-by: Yisheng Xie
> ---
> drivers/net/wireless/intel/iwlwifi/mvm/debugfs.c | 11 +++
> 1 file changed, 3 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/debugfs.c
> b/drivers/net/wireless/intel/iwlwifi/mvm/debugfs.c
> index 0e6401c..e8249a6 100644
> --- a/drivers/net/wireless/intel/iwlwifi/mvm/debugfs.c
> +++ b/drivers/net/wireless/intel/iwlwifi/mvm/debugfs.c
> @@ -671,14 +671,9 @@ static ssize_t iwl_dbgfs_bt_cmd_read(struct file
> *file, char __user *user_buf,
> };
> int ret, bt_force_ant_mode;
>
> - for (bt_force_ant_mode = 0;
> - bt_force_ant_mode < ARRAY_SIZE(modes_str);
> - bt_force_ant_mode++) {
> - if (!strcmp(buf, modes_str[bt_force_ant_mode]))
> - break;
> - }
> -
> - if (bt_force_ant_mode >= ARRAY_SIZE(modes_str))
> + bt_force_ant_mode = match_string(modes_str,
> + ARRAY_SIZE(modes_str),
> buf);
> + if (bt_force_ant_mode < 0)
> return -EINVAL;
>
> ret = 0;
Looks fine, I'll push this to our internal tree for review and take a
closer look at what the match_string() function does exactly.
Thanks for the patch.
--
Cheers,
Luca.
Re: [PATCH][next] iwlwifi: mvm: remove division by size of sizeof(struct ieee80211_wmm_rule)
On Wed, 2018-04-11 at 14:05 +0100, Colin King wrote:
> From: Colin Ian King
>
> The subtraction of two struct ieee80211_wmm_rule pointers leaves a
> result
> that is automatically scaled down by the size of the size of pointed-
> to
> type, hence the division by sizeof(struct ieee80211_wmm_rule) is
> bogus and should be removed.
>
> Detected by CoverityScan, CID#146 ("Extra sizeof expression")
>
> Fixes: 77e30e10ee28 ("iwlwifi: mvm: query regdb for wmm rule if
> needed")
> Signed-off-by: Colin Ian King
> ---
Thanks, Colin! I've pushed this to our internal tree for review and if
everything goes fine it will land upstream following our normal
upstreaming process.
--
Cheers,
Luca.
Re: [PATCH 13/17] iwlwifi: mvm: Convert timers to use timer_setup()
On Mon, 2017-11-06 at 11:45 -0800, Kees Cook wrote: > On Sun, Oct 29, 2017 at 5:28 AM, Luca Coelho wrote: > > From: Kees Cook > > > > In preparation for unconditionally passing the struct timer_list > > pointer to > > all timer callbacks, switch to using the new timer_setup() and > > from_timer() > > to pass the timer pointer explicitly. > > > > The RCU lifetime on baid_data is unclear, so this adds a direct > > copy of the > > rcu_ptr passed to the original callback. It may be possible to > > improve this > > to just use baid_data->mvm->baid_map[baid_data->baid] instead. > > > > Cc: Johannes Berg > > Cc: Emmanuel Grumbach > > Cc: Luca Coelho > > Cc: Intel Linux Wireless > > Cc: Kalle Valo > > Cc: Sara Sharon > > Cc: linux-wirel...@vger.kernel.org > > Cc: netdev@vger.kernel.org > > Signed-off-by: Kees Cook > > Signed-off-by: Luca Coelho > > --- > > drivers/net/wireless/intel/iwlwifi/mvm/mvm.h | 3 ++- > > drivers/net/wireless/intel/iwlwifi/mvm/rxmq.c | 4 ++-- > > drivers/net/wireless/intel/iwlwifi/mvm/sta.c | 18 + > > - > > 3 files changed, 13 insertions(+), 12 deletions(-) > > Hi, > > Thanks for taking this! I had a question on timing: is this expected > to land for 4.15? If not, I would like to take this via the timers > tree, since it is one of the few remaining conversions. Hi Kees, Yes, this should land for 4.15. Kalle just pulled my pull-request (which includes this) to wireless-drivers-next. He told me he'll send a pull-request for 4.15 during this week and hopefully Dave will pull from him too. I'll let you know if something doesn't go as planned. -- Cheers, Luca.
[PATCH 13/17] iwlwifi: mvm: Convert timers to use timer_setup()
From: Kees Cook
In preparation for unconditionally passing the struct timer_list pointer to
all timer callbacks, switch to using the new timer_setup() and from_timer()
to pass the timer pointer explicitly.
The RCU lifetime on baid_data is unclear, so this adds a direct copy of the
rcu_ptr passed to the original callback. It may be possible to improve this
to just use baid_data->mvm->baid_map[baid_data->baid] instead.
Cc: Johannes Berg
Cc: Emmanuel Grumbach
Cc: Luca Coelho
Cc: Intel Linux Wireless
Cc: Kalle Valo
Cc: Sara Sharon
Cc: linux-wirel...@vger.kernel.org
Cc: netdev@vger.kernel.org
Signed-off-by: Kees Cook
Signed-off-by: Luca Coelho
---
drivers/net/wireless/intel/iwlwifi/mvm/mvm.h | 3 ++-
drivers/net/wireless/intel/iwlwifi/mvm/rxmq.c | 4 ++--
drivers/net/wireless/intel/iwlwifi/mvm/sta.c | 18 +-
3 files changed, 13 insertions(+), 12 deletions(-)
diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/mvm.h
b/drivers/net/wireless/intel/iwlwifi/mvm/mvm.h
index 2f3d5bef4b9e..0e18c5066f04 100644
--- a/drivers/net/wireless/intel/iwlwifi/mvm/mvm.h
+++ b/drivers/net/wireless/intel/iwlwifi/mvm/mvm.h
@@ -652,6 +652,7 @@ struct iwl_mvm_baid_data {
u16 entries_per_queue;
unsigned long last_rx;
struct timer_list session_timer;
+ struct iwl_mvm_baid_data __rcu **rcu_ptr;
struct iwl_mvm *mvm;
struct iwl_mvm_reorder_buffer reorder_buf[IWL_MAX_RX_HW_QUEUES];
struct iwl_mvm_reorder_buf_entry entries[];
@@ -1853,7 +1854,7 @@ void iwl_mvm_tdls_ch_switch_work(struct work_struct
*work);
void iwl_mvm_sync_rx_queues_internal(struct iwl_mvm *mvm,
struct iwl_mvm_internal_rxq_notif *notif,
u32 size);
-void iwl_mvm_reorder_timer_expired(unsigned long data);
+void iwl_mvm_reorder_timer_expired(struct timer_list *t);
struct ieee80211_vif *iwl_mvm_get_bss_vif(struct iwl_mvm *mvm);
bool iwl_mvm_is_vif_assoc(struct iwl_mvm *mvm);
diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/rxmq.c
b/drivers/net/wireless/intel/iwlwifi/mvm/rxmq.c
index 9852a4d62337..343bdc4266cd 100644
--- a/drivers/net/wireless/intel/iwlwifi/mvm/rxmq.c
+++ b/drivers/net/wireless/intel/iwlwifi/mvm/rxmq.c
@@ -460,9 +460,9 @@ static void iwl_mvm_release_frames(struct iwl_mvm *mvm,
}
}
-void iwl_mvm_reorder_timer_expired(unsigned long data)
+void iwl_mvm_reorder_timer_expired(struct timer_list *t)
{
- struct iwl_mvm_reorder_buffer *buf = (void *)data;
+ struct iwl_mvm_reorder_buffer *buf = from_timer(buf, t, reorder_timer);
struct iwl_mvm_baid_data *baid_data =
iwl_mvm_baid_data_from_reorder_buf(buf);
struct iwl_mvm_reorder_buf_entry *entries =
diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/sta.c
b/drivers/net/wireless/intel/iwlwifi/mvm/sta.c
index 039efcd2735d..c19f98489d4e 100644
--- a/drivers/net/wireless/intel/iwlwifi/mvm/sta.c
+++ b/drivers/net/wireless/intel/iwlwifi/mvm/sta.c
@@ -252,9 +252,11 @@ int iwl_mvm_sta_send_to_fw(struct iwl_mvm *mvm, struct
ieee80211_sta *sta,
return ret;
}
-static void iwl_mvm_rx_agg_session_expired(unsigned long data)
+static void iwl_mvm_rx_agg_session_expired(struct timer_list *t)
{
- struct iwl_mvm_baid_data __rcu **rcu_ptr = (void *)data;
+ struct iwl_mvm_baid_data *data =
+ from_timer(data, t, session_timer);
+ struct iwl_mvm_baid_data __rcu **rcu_ptr = data->rcu_ptr;
struct iwl_mvm_baid_data *ba_data;
struct ieee80211_sta *sta;
struct iwl_mvm_sta *mvm_sta;
@@ -2160,10 +2162,8 @@ static void iwl_mvm_init_reorder_buffer(struct iwl_mvm
*mvm,
reorder_buf->head_sn = ssn;
reorder_buf->buf_size = buf_size;
/* rx reorder timer */
- reorder_buf->reorder_timer.function =
- iwl_mvm_reorder_timer_expired;
- reorder_buf->reorder_timer.data = (unsigned long)reorder_buf;
- init_timer(&reorder_buf->reorder_timer);
+ timer_setup(&reorder_buf->reorder_timer,
+ iwl_mvm_reorder_timer_expired, 0);
spin_lock_init(&reorder_buf->lock);
reorder_buf->mvm = mvm;
reorder_buf->queue = i;
@@ -2286,9 +2286,9 @@ int iwl_mvm_sta_rx_agg(struct iwl_mvm *mvm, struct
ieee80211_sta *sta,
baid_data->baid = baid;
baid_data->timeout = timeout;
baid_data->last_rx = jiffies;
- setup_timer(&baid_data->session_timer,
- iwl_mvm_rx_agg_session_expired,
- (unsigned long)&mvm->baid_map[baid]);
+ baid_data->rcu_ptr = &mvm->baid_map[baid];
+ timer_setup(&baid_data->session_timer,
+ iwl_mvm_rx_agg_sess
Re: [PATCH] drivers/wireless: iwlwifi/mvm: Convert timers to use timer_setup()
On Tue, 2017-10-24 at 02:29 -0700, Kees Cook wrote: > In preparation for unconditionally passing the struct timer_list > pointer to > all timer callbacks, switch to using the new timer_setup() and > from_timer() > to pass the timer pointer explicitly. > > The RCU lifetime on baid_data is unclear, so this adds a direct copy > of the > rcu_ptr passed to the original callback. It may be possible to > improve this > to just use baid_data->mvm->baid_map[baid_data->baid] instead. > > Cc: Johannes Berg > Cc: Emmanuel Grumbach > Cc: Luca Coelho > Cc: Intel Linux Wireless > Cc: Kalle Valo > Cc: Sara Sharon > Cc: linux-wirel...@vger.kernel.org > Cc: netdev@vger.kernel.org > Signed-off-by: Kees Cook > --- Thanks, Kees. I'm taking this for review on our internal tree. If all our checks pass, I'll apply it and it will reach the mainline following our usual upstreaming process. -- Cheers, Luca.
[PATCH] iwlwifi: mvm: only send LEDS_CMD when the FW supports it
From: Luca Coelho
The LEDS_CMD command is only supported in some newer FW versions
(e.g. iwlwifi-8000C-31.ucode), so we can't send it to older versions
(such as iwlwifi-8000C-27.ucode).
To fix this, check for a new bit in the FW capabilities TLV that tells
when the command is supported.
Note that the current version of -31.ucode in linux-firmware.git
(31.532993.0) does not have this capability bit set, so the LED won't
work, even though this version should support it. But we will update
this firmware soon, so it won't be a problem anymore.
Fixes: 7089ae634c50 ("iwlwifi: mvm: use firmware LED command where applicable")
Reported-by: Linus Torvalds
Signed-off-by: Luca Coelho
---
drivers/net/wireless/intel/iwlwifi/fw/file.h | 1 +
drivers/net/wireless/intel/iwlwifi/mvm/led.c | 3 ++-
2 files changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/net/wireless/intel/iwlwifi/fw/file.h
b/drivers/net/wireless/intel/iwlwifi/fw/file.h
index 887f6d8fc8a7..279248cd9cfb 100644
--- a/drivers/net/wireless/intel/iwlwifi/fw/file.h
+++ b/drivers/net/wireless/intel/iwlwifi/fw/file.h
@@ -378,6 +378,7 @@ enum iwl_ucode_tlv_capa {
IWL_UCODE_TLV_CAPA_EXTEND_SHARED_MEM_CFG= (__force
iwl_ucode_tlv_capa_t)80,
IWL_UCODE_TLV_CAPA_LQM_SUPPORT = (__force
iwl_ucode_tlv_capa_t)81,
IWL_UCODE_TLV_CAPA_TX_POWER_ACK = (__force
iwl_ucode_tlv_capa_t)84,
+ IWL_UCODE_TLV_CAPA_LED_CMD_SUPPORT = (__force
iwl_ucode_tlv_capa_t)86,
IWL_UCODE_TLV_CAPA_MLME_OFFLOAD = (__force
iwl_ucode_tlv_capa_t)96,
NUM_IWL_UCODE_TLV_CAPA
diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/led.c
b/drivers/net/wireless/intel/iwlwifi/mvm/led.c
index 005e2e7278a5..b27269504a62 100644
--- a/drivers/net/wireless/intel/iwlwifi/mvm/led.c
+++ b/drivers/net/wireless/intel/iwlwifi/mvm/led.c
@@ -92,7 +92,8 @@ static void iwl_mvm_send_led_fw_cmd(struct iwl_mvm *mvm, bool
on)
static void iwl_mvm_led_set(struct iwl_mvm *mvm, bool on)
{
- if (mvm->cfg->device_family >= IWL_DEVICE_FAMILY_8000) {
+ if (fw_has_capa(&mvm->fw->ucode_capa,
+ IWL_UCODE_TLV_CAPA_LED_CMD_SUPPORT)) {
iwl_mvm_send_led_fw_cmd(mvm, on);
return;
}
--
2.14.1
Re: [GIT] Networking
On Thu, 2017-09-07 at 05:04 +, Coelho, Luciano wrote:
> On Wed, 2017-09-06 at 21:57 -0700, Linus Torvalds wrote:
> > On Wed, Sep 6, 2017 at 9:11 PM, Coelho, Luciano
> > wrote:
> > >
> > > This seems to be a problem with backwards-compatibility with FW version
> > > 27. We are now in version 31[1] and upgrading will probably fix that.
> >
> > I can confirm that fw version 31 works.
>
> Great, so I know for sure that this is a backwards-compatibility issue
> with the FW API.
>
>
> > > But obviously the driver should not fail miserably like this with
> > > version 27, because it claims to support it still.
> >
> > Not just "claims to support it", but if it's what is shipped with a
> > fairly recent distro like an up-to-date version of F26, I would really
> > hope that the driver can still work with it.
>
> I totally agree, we support a bunch of older versions for that exact
> reason. We just don't really test all the supported versions very
> often. We should probably change that.
>
> I'll make sure it still works with version 27.
Okay, I found the offending patch:
commit 7089ae634c50544b29b31faf1a751e8765c8de3b
Author: Johannes Berg
AuthorDate: Wed Jun 28 16:19:49 2017 +0200
Commit: Luca Coelho
CommitDate: Wed Aug 9 09:15:32 2017 +0300
iwlwifi: mvm: use firmware LED command where applicable
On devices starting from 8000 series, the host can no longer toggle
the LED through the CSR_LED_REG register, but must do it via the
firmware instead. Add support for this. Note that this means that
the LED cannot be turned on while the firmware is off, so using an
arbitrary LED trigger may not work as expected.
Fixes: 503ab8c56ca0 ("iwlwifi: Add 8000 HW family support")
Signed-off-by: Johannes Berg
Signed-off-by: Luca Coelho
Reverting it solves the problem. We introduced a new command to control
the LED lights and assumed it was available in older FW versions as
well, which turned out not to be the case.
This patch is not very important (unless you really like blinking lights
-- maybe I'll change my mind when the holidays approach :P). so it is
fine if you just want to revert it for now.
In any case, I'll send a patch fixing this problem soon.
--
Cheers,
Luca.
Re: [PATCH] iwlwifi: Demote messages about fw flags size to info
Hi João Paulo, On Tue, 2017-08-01 at 15:58 -0700, João Paulo Rechi Vita wrote: > Hello Luca, > > On Mon, Jul 24, 2017 at 4:01 AM, Coelho, Luciano > wrote: > > On Fri, 2017-07-21 at 07:51 -0700, João Paulo Rechi Vita wrote: > > (...) > > > > Currently these messages are presented to the user during boot if there > > > is no bootsplash covering the console, sometimes even if the boot splash > > > is enabled but has not started yet by the time this message is shown. > > > > > I should have provided another piece of information here: all of this > happens even when booting with the 'quiet' kernel parameter. Oh, okay, that's annoying. > > This specific case is harmless, but I'd rather keep this message as an > > error, because in other situations it could lead to unexpected > > behavioir, so I prefer to keep it very visible. > > > > > > I see your point, and I understand the purpose of these messages. I'm > wondering if perhaps having them at the warning level would give them > enough visibility, while still keeping a clean boot process to the end > user. If so, I can send an updated patch. > > Thanks for your reply and for pointing to the fix for the root cause > of that message. Sure, I agree it's better to make it use IWL_WARN(), which will generate a dev_warn() instead of a dev_err(). -- Cheers, Luca.
Re: [PATCH v2] iwlwifi: mvm: Fix a memory leak in an error handling path in 'iwl_mvm_sar_get_wgds_table()'
On Fri, 2017-07-14 at 12:06 +0200, Christophe JAILLET wrote:
> We should free 'wgds.pointer' here as done a few lines above in another
> error handling path.
> It was allocated within 'acpi_evaluate_object()'.
>
> Signed-off-by: Christophe JAILLET
> ---
> v2: rebase after 7fe90e0e3d60 ("iwlwifi: mvm: refactor geo init")
Thanks, Christophe!
I've pushed this to our internal tree and it will eventually reach the
mainline via our normal process.
> Moreovern a comment in '/drivers/acpi/acpica/utalloc.c' states that:
> /* [...] Note: The caller should use acpi_os_free to free this
> * buffer created via ACPI_ALLOCATE_BUFFER.
> */
>
> So, at the end of this function:
> out_free:
> kfree(wgds.pointer);
> should maybe be:
> out_free:
> acpi_os_free(wgds.pointer);
>
> If correct, several places should be fixed accordingly.
Thanks for pointing out! I'm about to do some refactoring in this code
and I'll make sure I check the proper way to free the acpi buffer when
doing so.
--
Cheers,
Luca.
Re: [PATCH] iwlwifi: mvm: add const to thermal_cooling_device_ops structure
On Wed, 2017-06-28 at 14:49 +0300, Luca Coelho wrote: > On Wed, 2017-06-21 at 14:10 +0530, Bhumika Goyal wrote: > > Declare thermal_cooling_device_ops structure as const as it is only passed > > as an argument to the function thermal_cooling_device_register and this > > argument is of type const. So, declare the structure as const. > > > > Signed-off-by: Bhumika Goyal > > --- > > Thanks, we're reviewing this internally. It looks fine, but I need to > assess whether this will have any impacts in our backports project > before we can apply it. This has been applied in our internal tree and will eventually land in the mainline. Thanks! -- Cheers, Luca.
Re: [PATCH] iwlwifi: mvm: fix iwl_mvm_sar_find_wifi_pkg corner case
On Tue, 2017-06-27 at 17:24 +0200, Arnd Bergmann wrote:
> gcc warns about what it thinks is an uninitialized variable
> access:
>
> drivers/net/wireless/intel/iwlwifi/mvm/fw.c: In function
> 'iwl_mvm_sar_find_wifi_pkg.isra.14':
> drivers/net/wireless/intel/iwlwifi/mvm/fw.c:1102:5: error: 'wifi_pkg' may be
> used uninitialized in this function [-Werror=maybe-uninitialized]
>
> That problem cannot really happen, as we check data->package.count
> to ensure that the loop is entered at least once.
> However, something that can indeed happen is returning an incorrect
> wifi_pkg pointer in case none of the elements are what we are looking
> for.
>
> This modifies the loop again, to only return a correct object, and
> to shut up that warning.
>
> Fixes: c386dacb4ed6 ("iwlwifi: mvm: refactor SAR init to prepare for dynamic
> SAR")
> Signed-off-by: Arnd Bergmann
> ---
Thanks, Arnd!
I've pushed this to our internal tree and it will eventually reach the
mainline, via our normal upstreaming process.
--
Cheers,
Luca.
Re: [PATCH] iwlwifi: mvm: add const to thermal_cooling_device_ops structure
On Wed, 2017-06-21 at 14:10 +0530, Bhumika Goyal wrote: > Declare thermal_cooling_device_ops structure as const as it is only passed > as an argument to the function thermal_cooling_device_register and this > argument is of type const. So, declare the structure as const. > > Signed-off-by: Bhumika Goyal > --- Thanks, we're reviewing this internally. It looks fine, but I need to assess whether this will have any impacts in our backports project before we can apply it. -- Cheers, Luca.
Re: [PATCH] net: wireless: intel: iwlwifi: dvm: remove unused defines
On Wed, 2017-06-07 at 01:20 +0200, Seraphime Kirkovski wrote: > Those constants have been unused for quite some time now. > > Signed-off-by: Seraphime Kirkovski > --- > I've compile-tested it. Thanks. I've applied it to our internal tree and it will reach the mainline at some point. -- Cheers, Luca.
Re: drivers/net/wireless/intel/iwlwifi/pcie/trans.c: 2 * suspicious code ?
On Fri, 2017-01-06 at 17:47 +, David Binderman wrote: > Hello there, Hi David, > 1. > > drivers/net/wireless/intel/iwlwifi/pcie/trans.c:2039:14: warning: decrement > of a boolean expression [-Wbool-operation] > > Source code is > >txq->block--; > > Maybe someone got a bool and a int mixed up ? > > 2. > > drivers/net/wireless/intel/iwlwifi/pcie/trans.c:2045:14: warning: increment > of a boolean expression [-Wbool-operation] > > Duplicate a few lines further down. Emmanuel has fixed this in our internal tree and I'll be sending it out together with our normal upstreaming process. Thanks for reporting! -- Cheers, Luca.
Re: [PATCH 5/8] linux: drop __bitwise__ everywhere
On Thu, 2016-12-15 at 07:15 +0200, Michael S. Tsirkin wrote: > __bitwise__ used to mean "yes, please enable sparse checks > unconditionally", but now that we dropped __CHECK_ENDIAN__ > __bitwise is exactly the same. > There aren't many users, replace it by __bitwise everywhere. > > Signed-off-by: Michael S. Tsirkin > --- > arch/arm/plat-samsung/include/plat/gpio-cfg.h| 2 +- > drivers/md/dm-cache-block-types.h| 6 +++--- > drivers/net/ethernet/sun/sunhme.h| 2 +- > drivers/net/wireless/intel/iwlwifi/iwl-fw-file.h | 4 ++-- For drivers/net/wireless/intel/iwlwifi/iwl-fw-file.h: Acked-by: Luca Coelho -- Luca.
Re: [PATCH 8/8] Makefile: drop -D__CHECK_ENDIAN__ from cflags
On Thu, 2016-12-15 at 07:15 +0200, Michael S. Tsirkin wrote: > That's the default now, no need for makefiles to set it. > > Signed-off-by: Michael S. Tsirkin > --- > drivers/bluetooth/Makefile| 2 -- > drivers/net/can/Makefile | 1 - > drivers/net/ethernet/altera/Makefile | 1 - > drivers/net/ethernet/atheros/alx/Makefile | 1 - > drivers/net/ethernet/freescale/Makefile | 2 -- > drivers/net/wireless/ath/Makefile | 2 -- > drivers/net/wireless/ath/wil6210/Makefile | 2 -- > drivers/net/wireless/broadcom/brcm80211/brcmfmac/Makefile | 2 -- > drivers/net/wireless/broadcom/brcm80211/brcmsmac/Makefile | 1 - > drivers/net/wireless/intel/iwlegacy/Makefile | 2 -- > drivers/net/wireless/intel/iwlwifi/Makefile | 2 +- > drivers/net/wireless/intel/iwlwifi/dvm/Makefile | 2 +- > drivers/net/wireless/intel/iwlwifi/mvm/Makefile | 2 +- For the drivers/net/wireless/intel/iwlwifi/ part: Acked-by: Luca Coelho -- Luca.
Re: Intel Wireless 7260 Microcode SW error detected
Hi Tibor, On Sun, 2016-10-16 at 20:08 +0200, Billes Tibor wrote: > I have Lenovo laptop with an Intel Wireless 7260 wifi module and the > latest stable kernel 4.8.1. I was playing a movie from an sshfs mounted > file system, when the movie just froze and my network stopped working. > Dmesg said 'Microcode SW error detected'. Below is my full `dmesg -r` > output. I probably will not be able to reproduce this behaviour, because > it did not happen in the past, and it seems to work fine after reboot. Can you file a bug in bugzilla so we can track this? It seems to be a firmware problem that we'll have to investigate. We have instructions on how to report here: https://wireless.wiki.kernel.org/en/users/drivers/iwlwifi/debugging > Is there anything more that I can do to help you debug this? It would be nice if you could enable firmware debugging so that, if this happens again, we'll have the appropriate FW logs to investigate it further. There are instructions on how to enable it in the "Firmware Debugging" section of that wiki page. Please make sure you read and understand the privacy aspects of submitting firmware debugging data, as explained here: https://wireless.wiki.kernel.org/en/users/drivers/iwlwifi/debugging#privacy_aspects > <3>[37750.190201] iwlwifi :08:00.0: Microcode SW error detected. > Restarting 0x200. > <3>[37750.190206] iwlwifi :08:00.0: CSR values: > <3>[37750.190207] iwlwifi :08:00.0: (2nd byte of CSR_INT_COALESCING > is CSR_INT_PERIODIC_REG) > <3>[37750.190212] iwlwifi :08:00.0: CSR_HW_IF_CONFIG_REG: 0X40489204 > <3>[37750.190226] iwlwifi :08:00.0: CSR_INT_COALESCING: 0X8040 > <3>[37750.190239] iwlwifi :08:00.0: CSR_INT: 0X > <3>[37750.190253] iwlwifi :08:00.0: CSR_INT_MASK: 0X > <3>[37750.190267] iwlwifi :08:00.0: CSR_FH_INT_STATUS: 0X > <3>[37750.190281] iwlwifi :08:00.0: CSR_GPIO_IN: 0X > <3>[37750.190294] iwlwifi :08:00.0: CSR_RESET: 0X > <3>[37750.190308] iwlwifi :08:00.0: CSR_GP_CNTRL: 0X080403c5 > <3>[37750.190322] iwlwifi :08:00.0: CSR_HW_REV: 0X0144 > <3>[37750.190336] iwlwifi :08:00.0: CSR_EEPROM_REG: 0X > <3>[37750.190350] iwlwifi :08:00.0: CSR_EEPROM_GP: 0X8000 > <3>[37750.190363] iwlwifi :08:00.0: CSR_OTP_GP_REG: 0X803a > <3>[37750.190377] iwlwifi :08:00.0: CSR_GIO_REG: 0X001f0042 > <3>[37750.190391] iwlwifi :08:00.0: CSR_GP_UCODE_REG: 0X > <3>[37750.190405] iwlwifi :08:00.0: CSR_GP_DRIVER_REG: 0X > <3>[37750.190419] iwlwifi :08:00.0: CSR_UCODE_DRV_GP1: 0X > <3>[37750.190432] iwlwifi :08:00.0: CSR_UCODE_DRV_GP2: 0X > <3>[37750.190446] iwlwifi :08:00.0: CSR_LED_REG: 0X0060 > <3>[37750.190460] iwlwifi :08:00.0: CSR_DRAM_INT_TBL_REG: 0X881530a3 > <3>[37750.190474] iwlwifi :08:00.0: CSR_GIO_CHICKEN_BITS: 0X27800200 > <3>[37750.190488] iwlwifi :08:00.0: CSR_ANA_PLL_CFG: 0Xd5d5 > <3>[37750.190501] iwlwifi :08:00.0: CSR_MONITOR_STATUS_REG: 0X3d0801bd > <3>[37750.190515] iwlwifi :08:00.0: CSR_HW_REV_WA_REG: 0X0001001a > <3>[37750.190529] iwlwifi :08:00.0: CSR_DBG_HPET_MEM_REG: 0X0010 > <3>[37750.190530] iwlwifi :08:00.0: FH register values: > <3>[37750.190552] iwlwifi :08:00.0: FH_RSCSR_CHNL0_STTS_WPTR_REG: > 0X15005f00 > <3>[37750.190566] iwlwifi :08:00.0: FH_RSCSR_CHNL0_RBDCB_BASE_REG: > 0X015005e0 > <3>[37750.190580] iwlwifi :08:00.0: FH_RSCSR_CHNL0_WPTR: 0X00e8 > <3>[37750.190593] iwlwifi :08:00.0: FH_MEM_RCSR_CHNL0_CONFIG_REG: > 0X80801114 > <3>[37750.190607] iwlwifi :08:00.0: FH_MEM_RSSR_SHARED_CTRL_REG: > 0X00fc > <3>[37750.190621] iwlwifi :08:00.0: FH_MEM_RSSR_RX_STATUS_REG: > 0X0703 > <3>[37750.190635] iwlwifi :08:00.0: > FH_MEM_RSSR_RX_ENABLE_ERR_IRQ2DRV: 0X > <3>[37750.190648] iwlwifi :08:00.0: FH_TSSR_TX_STATUS_REG: 0X07ff0001 > <3>[37750.190662] iwlwifi :08:00.0: FH_TSSR_TX_ERROR_REG: 0X > <3>[37750.190790] iwlwifi :08:00.0: Start IWL Error Log Dump: > <3>[37750.190791] iwlwifi :08:00.0: Status: 0x, count: 6 > <3>[37750.190793] iwlwifi :08:00.0: Loaded firmware version: 16.242414.0 I recommend that you update your firmware to the latest version (iwlwifi-7260-17.ucode) published in linux-firmware.git: http://git.kernel.org/cgit/linux/kernel/git/firmware/linux-firmware.git While I'm not sure the issue you saw is solved by that, we do have lots of other fixes in it that are worth the effort. <3>[37750.190794] iwlwifi :08:00.0: 0x307C | ADVANCED_SYSASSERT I hadn't seen this for a while. This is something related to the TX queues and could have many different causes... Thanks for reporting! -- Cheers, Luca.
Re: linux-4.9-rc1/drivers/net/wireless/intel/iwlwifi/mvm/debugfs.c:1561: poor error checking ?
Hi David,
On Mon, 2016-10-17 at 07:40 +, David Binderman wrote:
> Hello there,
>
> linux-4.9-rc1/drivers/net/wireless/intel/iwlwifi/mvm/debugfs.c:1561]: (style)
> Checking if unsigned variable 'len' is less than zero.
>
> Source code is
>
> len = min((size_t)le32_to_cpu(rsp->len) << 2,
> iwl_rx_packet_payload_len(hcmd.resp_pkt) - sizeof(*rsp));
> len = min(len - delta, count);
> if (len < 0) {
> ret = -EFAULT;
> goto out;
> }
>
> Suggest improve error checking.
Thanks for reporting! A fix for this is already queued in our internal
tree and will be sent upstream soon.
--
Cheers,
Luca.
Re: [PATCH] iwlwifi: pcie: fix SPLC structure parsing
On Thu, 2016-10-13 at 14:55 +0200, Paul Bolle wrote: > On Thu, 2016-10-13 at 15:44 +0300, Luca Coelho wrote: > > Even though there is apparently something wrong with this part of the > > ACPI table on you laptop, since it doesn't match our specifications. > > In any case, it's mostly harmless. > > > Would a correct implementation by Dell have any benefits for the users > of these laptops? In other words: should I bother somehow contacting > Dell and point them to this discussion in order to have them fix this? This value provides a way for the OEM to fine tune the power budget of the device. This is (usually) used to prevent parts of the platform from getting too hot. We have a certain default that is good enough for most cases. If Dell didn't want to set proper limits for this device, it probably means that it is not a concern. Dell does set these values correctly for other platforms. So, I guess it's up to you if you want to clarify this with them. This could be some default "blank" values when they don't want to change them. -- Luca.
Re: [PATCH] iwlwifi: pcie: fix SPLC structure parsing
On Thu, 2016-10-13 at 08:56 -0500, Chris Rorvick wrote: > Hi Luca, > > > On Thu, 2016-10-13 at 13:21 +0300, Luca Coelho wrote: > > Could you please give this a spin? I have tested it with some handmade > > ACPI tables in QEMU and it seems to work fine now. > > > Tested-by: Chris Rorvick > > I think the debug output looks as expected, see below for the first 20 > lines or so. And, more importantly, everything seems to be working! > :-) Yes, you got exactly what was expected. Thanks for testing! -- Luca.
Re: [PATCH] iwlwifi: pcie: fix SPLC structure parsing
On Thu, 2016-10-13 at 14:36 +0200, Paul Bolle wrote: > On Thu, 2016-10-13 at 14:30 +0300, Luca Coelho wrote: > > I forgot to say... could you load the iwlwifi module with debug=0x01 > > (module parameter), so we can see the messages the driver is printing > > when it doesn't find a proper structure? > > > That makes a lot of noise! Here's the first 100 lines or so, that > apparently are generated directly after modprobing iwlwifi. > > After these 100 lines there's a ten second gap (I guess it took me ten > seconds to actually use the wifi). I assume you don't care about that > part of the debug messages. > > Have fun! Thanks, Paul! Yeah, this is the "INFO" debugging level and it is a sort of fallback bucket when there is nothing more specific. Sorry for that. > <7>[ 767.691342] iwlwifi :3a:00.0: U iwl_pcie_prepare_card_hw > iwl_trans_prepare_card_hw enter > <7>[ 767.691362] iwlwifi :3a:00.0: U iwl_pcie_set_hw_ready hardware ready > <7>[ 767.692127] iwlwifi :3a:00.0: U iwl_request_firmware attempting to > load firmware 'iwlwifi-8000C-24.ucode' > <7>[ 767.692322] iwlwifi :3a:00.0: U splc_get_pwr_limit No element for > the WiFi domain returned by the SPLC method. This is the line I was looking for. :) So everything looks fine now. Even though there is apparently something wrong with this part of the ACPI table on you laptop, since it doesn't match our specifications. In any case, it's mostly harmless. Thanks for the help! -- Cheers, Luca.
Re: [PATCH] iwlwifi: pcie: fix SPLC structure parsing
On Thu, 2016-10-13 at 13:27 +0200, Paul Bolle wrote: > Luca, > > On Thu, 2016-10-13 at 13:21 +0300, Luca Coelho wrote: > > Could you please give this a spin? I have tested it with some handmade > > ACPI tables in QEMU and it seems to work fine now. > > > Tested-by: Paul Bolle > > Not that this test was worth a lot: it builds cleanly (on top of > 4.8.1), the error at boot is gone, obviously, and wifi still works (as > you're reading a message that was sent out over wifi). And I haven't > even tested this on another machine than my XPS 13 (9350). Thanks for testing! I forgot to say... could you load the iwlwifi module with debug=0x01 (module parameter), so we can see the messages the driver is printing when it doesn't find a proper structure? -- Luca.
[PATCH] iwlwifi: pcie: fix SPLC structure parsing
From: Luca Coelho
The SPLC data parsing is too restrictive and was not trying find the
correct element for WiFi. This causes problems with some BIOSes where
the SPLC method exists, but doesn't have a WiFi entry on the first
element of the list. The domain type values are also incorrect
according to the specification.
Fix this by complying with the actual specification.
Additionally, replace all occurrences of SPLX to SPLC, since SPLX is
only a structure internal to the ACPI tables, and may not even exist.
Fixes: bcb079a14d75 ("iwlwifi: pcie: retrieve and parse ACPI power limitations")
Reported-by: Chris Rorvick
Signed-off-by: Luca Coelho
---
Paul, Chris,
Could you please give this a spin? I have tested it with some handmade
ACPI tables in QEMU and it seems to work fine now.
Thanks!
drivers/net/wireless/intel/iwlwifi/pcie/drv.c | 79 ---
1 file changed, 48 insertions(+), 31 deletions(-)
diff --git a/drivers/net/wireless/intel/iwlwifi/pcie/drv.c
b/drivers/net/wireless/intel/iwlwifi/pcie/drv.c
index 001be40..da4810f 100644
--- a/drivers/net/wireless/intel/iwlwifi/pcie/drv.c
+++ b/drivers/net/wireless/intel/iwlwifi/pcie/drv.c
@@ -541,48 +541,64 @@ static const struct pci_device_id iwl_hw_card_ids[] = {
MODULE_DEVICE_TABLE(pci, iwl_hw_card_ids);
#ifdef CONFIG_ACPI
-#define SPL_METHOD "SPLC"
-#define SPL_DOMAINTYPE_MODULE BIT(0)
-#define SPL_DOMAINTYPE_WIFIBIT(1)
-#define SPL_DOMAINTYPE_WIGIG BIT(2)
-#define SPL_DOMAINTYPE_RFEMBIT(3)
+#define ACPI_SPLC_METHOD "SPLC"
+#define ACPI_SPLC_DOMAIN_WIFI (0x07)
-static u64 splx_get_pwr_limit(struct iwl_trans *trans, union acpi_object *splx)
+static u64 splc_get_pwr_limit(struct iwl_trans *trans, union acpi_object *splc)
{
- union acpi_object *limits, *domain_type, *power_limit;
-
- if (splx->type != ACPI_TYPE_PACKAGE ||
- splx->package.count != 2 ||
- splx->package.elements[0].type != ACPI_TYPE_INTEGER ||
- splx->package.elements[0].integer.value != 0) {
- IWL_ERR(trans, "Unsupported splx structure\n");
+ union acpi_object *data_pkg, *dflt_pwr_limit;
+ int i;
+
+ /* We need at least two elemets, one for the revision and one
+* for the data itself. Also check that the revision is
+* supported (currently only revision 0).
+ */
+ if (splc->type != ACPI_TYPE_PACKAGE ||
+ splc->package.count < 2 ||
+ splc->package.elements[0].type != ACPI_TYPE_INTEGER ||
+ splc->package.elements[0].integer.value != 0) {
+ IWL_DEBUG_INFO(trans,
+ "Unsupported structure returned by the SPLC
method. Ignoring.\n");
return 0;
}
- limits = &splx->package.elements[1];
- if (limits->type != ACPI_TYPE_PACKAGE ||
- limits->package.count < 2 ||
- limits->package.elements[0].type != ACPI_TYPE_INTEGER ||
- limits->package.elements[1].type != ACPI_TYPE_INTEGER) {
- IWL_ERR(trans, "Invalid limits element\n");
- return 0;
+ /* loop through all the packages to find the one for WiFi */
+ for (i = 1; i < splc->package.count; i++) {
+ union acpi_object *domain;
+
+ data_pkg = &splc->package.elements[i];
+
+ /* Skip anything that is not a package with the right
+* amount of elements (i.e. at least 2 integers).
+*/
+ if (data_pkg->type != ACPI_TYPE_PACKAGE ||
+ data_pkg->package.count < 2 ||
+ data_pkg->package.elements[0].type != ACPI_TYPE_INTEGER ||
+ data_pkg->package.elements[1].type != ACPI_TYPE_INTEGER)
+ continue;
+
+ domain = &data_pkg->package.elements[0];
+ if (domain->integer.value == ACPI_SPLC_DOMAIN_WIFI)
+ break;
+
+ data_pkg = NULL;
}
- domain_type = &limits->package.elements[0];
- power_limit = &limits->package.elements[1];
- if (!(domain_type->integer.value & SPL_DOMAINTYPE_WIFI)) {
- IWL_DEBUG_INFO(trans, "WiFi power is not limited\n");
+ if (!data_pkg) {
+ IWL_DEBUG_INFO(trans,
+ "No element for the WiFi domain returned by the
SPLC method.\n");
return 0;
}
- return power_limit->integer.value;
+ dflt_pwr_limit = &data_pkg->package.elements[1];
+ return dflt_pwr_limit->integer.value;
}
static void set_dflt_pwr_limit(struct iwl_trans *trans, struct pci_dev *pdev)
{
acpi_handle pxsx_handle;
acpi_handle handle;
- struct acpi_buffer splx = {ACPI_ALLOCATE_BU
Re: [PATCH] iwlwifi: pcie: reduce "unsupported splx" to a warning
On Wed, 2016-10-12 at 12:50 -0500, Chris Rorvick wrote: > Hi Luca, > > FYI, It seems that Google does not like your email as I'm not > receiving any of your messages in gmail. Some responses below: That's odd. It works for me. Replied privately to try to sort this out. > On Wed, 2016-10-12 at 15:24 +0300, Luca Coelho wrote: > > Hi Chris, > > On Tue, 2016-10-11 at 09:09 -0500, Chris Rorvick wrote: > > > On Tue, Oct 11, 2016 at 5:11 AM, Paul Bolle wrote: > > > > > This is not coming from the NIC itself, but from the platform's ACPI > > > > > tables. Can you tell us which platform you are using? > > > > > > > > > Interesting. I'm running a Dell XPS 13 9350. I replaced the > > > factory-provided Broadcom card with an AC 8260. I can update the > > > commit log to reflect this. > > > > > > Okay, so this makes sense. Those entries are probably formatted for > > the Broadcom card, which the iwlwifi driver obviously doesn't > > understand. The best we can do, as I already said, is to ignore values > > we don't understand. > > > This may already be apparent, but Dell sells two versions of the 9350: > one with the Broadcom adapter and one with the AC 8260. I just > happened to find the former version at a deep discount at Costco so > decided to chance it. Turns out the Broadcom card is not so good even > with new kernels so I upgraded. Anyway, since Paul is seeing the same > issue I don't think the values are intended to be Broadcom-specific. Right, this is not for Broadcom. I found out that this is something Intel specifies as part of the entire platform's ACPI recommendations. > On Wed, 2016-10-12 at 17:21 +0300, Luca Coelho wrote: > > And, the values in the SPLX structs are being changed here, to DOM1, > > LIM1, TIM1 etc., before being returned. Â This also matches your > > description that, at runtime, you got something different than the pure > > dump. Â If you follow these DOM*, LIM*, TIM* symbols, you'll probably > > end up getting the values you observed at runtime. > > > Probably not important, but it seems that there is some additional > indirection. The only values I'm seeing associated with those symbols > are 8 and 16: > > $ grep -e 'DOM[0-9]' -e 'LIM[0-9]' -e 'TIM[0-9]' dsdt.dsl | grep -v Store > DOM1, 8, > LIM1, 16, > TIM1, 16, > DOM2, 8, > LIM2, 16, > TIM2, 16, > DOM3, 8, > LIM3, 16, > TIM3, 16, Yeah, there are often many levels of indirection. These settings can be also tied to the configuration that is reachable by the user in the BIOS setup, so you never know. The easiest way is probably to run the ASL with acpiexec and execute the method... > > I'll send you a patch for testing soon. > > > I will keep an eye on the list archive, thanks! I'll ping you from my Intel address and provide you with a link to the patch, to make it easier for you. ;) -- Cheers, Luca.
Re: [PATCH] iwlwifi: pcie: reduce "unsupported splx" to a warning
On Wed, 2016-10-12 at 14:36 +0200, Paul Bolle wrote:
> On Wed, 2016-10-12 at 15:24 +0300, Luca Coelho wrote:
> > Okay... Actually this is a structure in the BIOS and the actual method
> > we call is SPLC. The SPLC method may return one item from this table,
> > or something entirely different, possible one of the three values
> > depending on a configuration option or so.
> >
> > Can you to find and send me the actual SPLC method that we call, from
> > your BIOS?
>
>
> It seems Chris and I basically have identical setups, so I'll answer.
Thanks! Yeah, I implied any of you two. ;)
> There are 20 SPLC methods in the BIOS. The first reads
> Method (SPLC, 0, Serialized)
> {
> DerefOf (SPLX [One]) [Zero] = DOM1 /* \DOM1 */
> DerefOf (SPLX [One]) [One] = LIM1 /* \LIM1 */
> DerefOf (SPLX [One]) [0x02] = TIM1 /* \TIM1 */
> DerefOf (SPLX [0x02]) [Zero] = DOM2 /* \DOM2 */
> DerefOf (SPLX [0x02]) [One] = LIM2 /* \LIM2 */
> DerefOf (SPLX [0x02]) [0x02] = TIM2 /* \TIM2 */
> DerefOf (SPLX [0x03]) [Zero] = DOM3 /* \DOM3 */
> DerefOf (SPLX [0x03]) [One] = LIM3 /* \LIM3 */
> DerefOf (SPLX [0x03]) [0x02] = TIM3 /* \TIM3 */
> Return (SPLX) /* \_SB_.PCI0.RP01.PXSX.SPLX */
> }
>
> The only difference is in the last comment. Ie, RP01 is increased until
> it reaches RP20. (The machine has 20 PCI devices according to lspci. I
> have no clue how to match that RPxx number to the 20 devices showing up
> in lspci, sorry.)
No problem, these BIOSes are usually quite cryptic. :) But what you're
saying makes sense. They have added the SPLC method to all PCI root-
ports (which is what RP stands for here).
And, the values in the SPLX structs are being changed here, to DOM1,
LIM1, TIM1 etc., before being returned. This also matches your
description that, at runtime, you got something different than the pure
dump. If you follow these DOM*, LIM*, TIM* symbols, you'll probably
end up getting the values you observed at runtime.
Basically this tells me that indeed 3 "structs" are being returned (as
your dumps already showed). And, according to the specs that I found
(which unfortunately are confidential, so I can't share) this is
correct and the driver code is broken.
I'll send you a patch for testing soon.
Thanks for all the help!
--
Cheers,
Luca.
Re: [PATCH] iwlwifi: pcie: reduce "unsupported splx" to a warning
On Tue, 2016-10-11 at 23:32 -0500, Chris Rorvick wrote:
> On Tue, Oct 11, 2016 at 5:11 AM, Paul Bolle wrote:
> > For what it's worth, on my machine I have twenty (!) SPLX entries, all
> > reading:
> > Name (SPLX, Package (0x04)
> > {
> > Zero,
> > Package (0x03)
> > {
> > 0x8000,
> > 0x8000,
> > 0x8000
> > },
> >
> > Package (0x03)
> > {
> >0x8000,
> >0x8000,
> >0x8000
> > },
> >
> > Package (0x03)
> > {
> > 0x8000,
> > 0x8000,
> > 0x8000
> > }
> > })
>
>
> I actually see exactly the same on my Dell XPS 13 (9350) when I use
> acpidump, etc. I typed the entry I included in the commit log by hand
> based on what the driver gets back from the SPLC method (I added a
> function to dump the returned object.)
Okay... Actually this is a structure in the BIOS and the actual method
we call is SPLC. The SPLC method may return one item from this table,
or something entirely different, possible one of the three values
depending on a configuration option or so.
Can you to find and send me the actual SPLC method that we call, from
your BIOS?
--
Cheers,
Luca.
Re: [PATCH] iwlwifi: pcie: reduce "unsupported splx" to a warning
Hi Chris, On Tue, 2016-10-11 at 09:09 -0500, Chris Rorvick wrote: > On Tue, Oct 11, 2016 at 5:11 AM, Paul Bolle wrote: > > > This is not coming from the NIC itself, but from the platform's ACPI > > > tables. Can you tell us which platform you are using? > > > Interesting. I'm running a Dell XPS 13 9350. I replaced the > factory-provided Broadcom card with an AC 8260. I can update the > commit log to reflect this. Okay, so this makes sense. Those entries are probably formatted for the Broadcom card, which the iwlwifi driver obviously doesn't understand. The best we can do, as I already said, is to ignore values we don't understand. I will also check what is the correct procedure in such cases, because it is possible, in theory, that the format *matches* but applies only to another device. > > > If this is really bothering you, I guess I could apply this patch for > > > now. But as I said, this is not solving the actual problem. > > > > > > Bikeshedding: I think IWL_INFO() is more appropriate, as info doesn't > > imply one needs to act on this message, while warn does imply that > > action is needed. > > > Agreed. I still think making this a warning is appropriate, but it > seems pretty clear this is not an error. This has nothing to do with > how much it bothers me. An error tells the user something needs to be > fixed, but in this case the interface is working fine. Making it a > warning with an improved message will result in fewer people wasting > their time. Yes, so I'll try to stop wasting people's timing by trying to do the correct thing without bothering the user at all. :) Thanks for pointing this all out! -- Cheers, Luca.
Re: [PATCH] iwlwifi: pcie: reduce "unsupported splx" to a warning
Hi Paul,
On Tue, 2016-10-11 at 12:11 +0200, Paul Bolle wrote:
> On Mon, 2016-10-10 at 17:02 +0300, Luca Coelho wrote:
> > On Mon, 2016-10-10 at 02:19 -0500, Chris Rorvick wrote:
> > This is not coming from the NIC itself, but from the platform's ACPI
> > tables. Can you tell us which platform you are using?
>
>
> On my machine I'm seeing the same error as Chris. So what exactly do
> you mean with "platform" here?
By "platform" I meant the PC you are using. The ACPI table is created
by the OEM, so different PCs have different tables.
>
> > > Name (SPLX, Package (0x04)
> > > {
> > > Zero,
> > > Package (0x03)
> > > {
> > > 0,
> > > 1200,
> > > 1000
> > > },
> > > Package (0x03)
> > > {
> > > 0,
> > > 1200,
> > > 1000
> > > },
> > > Package (0x03)
> > > {
> > > 0,
> > > 1200,
> > > 1000
> > > }
> > > })
> >
> >
> > This is not the structure that we are expecting. We expect this:
> >
> > Name (SPLX, Package (0x02)
> > {
> > Zero,
> > Package (0x03)
> > {
> > 0x07,
> > ,
> >
> > }
> > })
> >
> > ...as you correctly pointed out. The data in the structure you have is
> > not for WiFi (actually I don't think 0 is a valid value, but I'll
> > double-check).
>
>
> For what it's worth, on my machine I have twenty (!) SPLX entries, all
> reading:
> Name (SPLX, Package (0x04)
> {
> Zero,
> Package (0x03)
> {
> 0x8000,
> 0x8000,
> 0x8000
> },
>
> Package (0x03)
> {
>0x8000,
>0x8000,
>0x8000
> },
>
> Package (0x03)
> {
> 0x8000,
> 0x8000,
> 0x8000
> }
> })
Thanks. So this is another case where the first value doesn't match
what we are expecting and we should just ignore that.
> > There are other things that look a bit inconsistent in this code...
> > I'll try to find the official ACPI table definitions for this entries
> > to make sure it's correct.
>
>
> When I looked into this error, some time ago, I searched around a bit
> for documentation on this splx stuff. Sadly, commit bcb079a14d75
> ("iwlwifi: pcie: retrieve and parse ACPI power limitations") provides
> very few clues and my searches turned up nothing useful. So a pointer
> or two would be really appreciated.
Yeah, I looked into that commit too and there's not much there. I'll
try to find the documentation and, if I can, I'll share it with you.
> > > --- a/drivers/net/wireless/intel/iwlwifi/pcie/drv.c
> > > +++ b/drivers/net/wireless/intel/iwlwifi/pcie/drv.c
> > > @@ -540,7 +540,7 @@ static u64 splx_get_pwr_limit(struct iwl_trans
> > > *trans, union acpi_object *splx)
> > > splx->package.count != 2 ||
> > > splx->package.elements[0].type != ACPI_TYPE_INTEGER ||
> > > splx->package.elements[0].integer.value != 0) {
> > > - IWL_ERR(trans, "Unsupported splx structure\n");
> > > + IWL_WARN(trans, "Unsupported splx structure, not limiting WiFi
> > > power\n");
> > > return 0;
> > > }
> >
> >
> > If this is really bothering you, I guess I could apply this patch for
> > now. But as I said, this is not solving the actual problem.
>
>
> Bikeshedding: I think IWL_INFO() is more appropriate, as info doesn't
> imply one needs to act on this message, while warn does imply that
> action is needed.
Right, but in fact, the code considers that if the SPLX method exists,
it must return a value iwlwifi can understand, thus the error. That
assumption is wrong, so we should just ignore entries that don't match
and continue without printing anything out (as would happen if the splx
method were not even there).
In any case, I will rework this code, so I'd prefer if we skip this
patch entirely.
--
Cheers,
Luca.
Re: [PATCH] iwlwifi: pcie: reduce "unsupported splx" to a warning
Hi,
On Mon, 2016-10-10 at 02:19 -0500, Chris Rorvick wrote:
> Commit bcb079a14d75 ("iwlwifi: pcie: retrieve and parse ACPI power
> limitations") looks for a specific structure in the ACPI tables for
> setting the default power limit. The data returned for at least some
> dual band chipsets is not recognized, though. For example, the AC 8260
> reports the following:
This is not coming from the NIC itself, but from the platform's ACPI
tables. Can you tell us which platform you are using?
> Name (SPLX, Package (0x04)
> {
> Zero,
> Package (0x03)
> {
> 0,
> 1200,
> 1000
> },
> Package (0x03)
> {
> 0,
> 1200,
> 1000
> },
> Package (0x03)
> {
> 0,
> 1200,
> 1000
> }
> })
This is not the structure that we are expecting. We expect this:
Name (SPLX, Package (0x02)
{
Zero,
Package (0x03)
{
0x07,
,
}
})
...as you correctly pointed out. The data in the structure you have is
not for WiFi (actually I don't think 0 is a valid value, but I'll
double-check).
> The current logic expects exactly two elements in the outer package,
> causing the above to be ignored and the power limit unset.
>
> Despite the interface being fully functional after initialization, the
> above condition is reported as an error. Knock the message down to a
> warning and provide better context for understanding its consequence.
Reducing this to a warning is an easy way to reduce the verbosity of
the problem, but I think the correct thing to do would be to accept
multiple entries and ignore the ones that don't have the WIFI marker.
And only type-check the WIFI ones.
There are other things that look a bit inconsistent in this code...
I'll try to find the official ACPI table definitions for this entries
to make sure it's correct.
> Signed-off-by: Chris Rorvick
> ---
> drivers/net/wireless/intel/iwlwifi/pcie/drv.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/intel/iwlwifi/pcie/drv.c
> b/drivers/net/wireless/intel/iwlwifi/pcie/drv.c
> index 78cf9a7..19b531f 100644
> --- a/drivers/net/wireless/intel/iwlwifi/pcie/drv.c
> +++ b/drivers/net/wireless/intel/iwlwifi/pcie/drv.c
> @@ -540,7 +540,7 @@ static u64 splx_get_pwr_limit(struct iwl_trans *trans,
> union acpi_object *splx)
> splx->package.count != 2 ||
> splx->package.elements[0].type != ACPI_TYPE_INTEGER ||
> splx->package.elements[0].integer.value != 0) {
> - IWL_ERR(trans, "Unsupported splx structure\n");
> + IWL_WARN(trans, "Unsupported splx structure, not limiting WiFi
> power\n");
> return 0;
> }
If this is really bothering you, I guess I could apply this patch for
now. But as I said, this is not solving the actual problem.
--
Cheers,
Luca.
Re: [PATCH RESEND] iwlwifi, Do not implement thermal zone unless ucode is loaded
On Wed, 2016-07-13 at 09:50 +0300, Kalle Valo wrote: > Prarit Bhargava writes: > > > > We implement thermal zone because we do support it, but the > > > problem is > > > that we need the firmware to be loaded for that. So you can argue > > > that > > > we should register *later* when the firmware is loaded. But this > > > is > > > really not helping all that much because the firmware can also be > > > stopped at any time. So you'd want us to register / unregister > > > the > > > thermal zone anytime the firmware is loaded / unloaded? > > > > You might have to do that. I think that if the firmware enables a > > feature then > > the act of loading the firmware should run the code that enables > > the feature. > > IMO of course. > > But I suspect that the iwlwifi firmware is loaded during interface up > (and unloaded during interface down) and in that case > register/unregister would be happening all the time. That doesn't > sound > like a good idea. I would rather try to fix the thermal interface to > handle the cases when the measurement is not available. I totally agree with Emmanuel and Kalle. We should not change this. It is a design decision to return an error when the interface is down, this is very common with other subsystems as well. The userspace should be able to handle errors and report something like "unavailable" when this kind of error is returned. I'm not sure EIO is the best we can have, but for me that's exactly what it is. The thermal zone *is* there, but cannot be accessed because the firmware is not available. I'm okay to change it to EBUSY, if that would help userspace, but I think that's a bit misleading. The device is not busy, on the contrary, it's not even running at all. Furthermore, I don't think this is "breaking userspace" in the sense of being a regression. The userspace API has always been implemented with the possibility of returning errors. It's not a good design if a single device returning an error causes all the other devices to also fail. -- Cheers, Luca.
Re: [PATCH] iwlwifi: add missing type declaration
On Mon, 2016-07-11 at 22:49 +0200, Arnd Bergmann wrote:
> The iwl-debug.h header relies in implicit inclusion of linux/device.h
> and
> we get a lot of warnings without that:
>
> drivers/net/wireless/intel/iwlwifi/iwl-debug.h:44:23: error: 'struct
> device' declared inside parameter list will not be visible outside of
> this definition or declaration [-Werror]
> void __iwl_err(struct device *dev, bool rfkill_prefix, bool
> only_trace,
> ^~
> In file included from drivers/net/wireless/intel/iwlwifi/iwl-eeprom-
> read.h:66:0,
> from drivers/net/wireless/intel/iwlwifi/iwl-eeprom-
> read.c:68:
> drivers/net/wireless/intel/iwlwifi/iwl-trans.h: In function
> 'iwl_trans_tx':
> drivers/net/wireless/intel/iwlwifi/iwl-trans.h:1030:348: error:
> passing argument 1 of '__iwl_err' from incompatible pointer type [-
> Werror=incompatible-pointer-types]
> IWL_ERR(trans, "%s bad state = %d\n", __func__, trans->state);
>
>
>
>
>
> ^
> In file included from drivers/net/wireless/intel/iwlwifi/iwl-eeprom-
> read.c:67:0:
> drivers/net/wireless/intel/iwlwifi/iwl-debug.h:44:6: note: expected
> 'struct device *' but argument is of type 'struct device *'
> void __iwl_err(struct device *dev, bool rfkill_prefix, bool
> only_trace,
> ^
>
> The easiest workaround is to just declare 'struct device' before its
> first use,
> rather than including the entire header file.
>
> Signed-off-by: Arnd Bergmann
> Fixes: 21cb3222fe56 ("iwlwifi: decouple PCIe transport from
> mac80211")
> ---
Acked-by: Luca Coelho
Agree with Kalle that he will take this directly to wireless-drivers-
next.
--
Cheers,
Luca.
Re: [PATCH] iwlwifi: Remove unused array 'iwlagn_loose_lookup'
On Fri, 2016-06-03 at 14:39 -0700, Guenter Roeck wrote: > gcc-6 reports the following error if -Werror=unused-const-variable > is enabled. > > drivers/net/wireless/intel/iwlwifi/dvm/lib.c:210:21: error: > 'iwlagn_loose_lookup' defined but not used > > Signed-off-by: Guenter Roeck > --- Thanks! I'm queuing this via our internal trees (adding "dvm: " to the subject, to indicate this is related to the dvm module). -- Cheers, Luca.
[PATCH] iwlwifi: mvm: fix merge damage in tx.c
From: Luca Coelho
During the merge in commit 909b27f70643 ("Merge
git://git.kernel.org/pub/scm/linux/kernel/git/davem/net"), there was a
small merge damage where one instance of info was not converted into
skb_info. Fix this.
Signed-off-by: Luca Coelho
---
drivers/net/wireless/intel/iwlwifi/mvm/tx.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/tx.c
b/drivers/net/wireless/intel/iwlwifi/mvm/tx.c
index 8802109..c53aa0f 100644
--- a/drivers/net/wireless/intel/iwlwifi/mvm/tx.c
+++ b/drivers/net/wireless/intel/iwlwifi/mvm/tx.c
@@ -211,6 +211,7 @@ void iwl_mvm_set_tx_cmd(struct iwl_mvm *mvm, struct sk_buff
*skb,
struct iwl_tx_cmd *tx_cmd,
struct ieee80211_tx_info *info, u8 sta_id)
{
+ struct ieee80211_tx_info *skb_info = IEEE80211_SKB_CB(skb);
struct ieee80211_hdr *hdr = (void *)skb->data;
__le16 fc = hdr->frame_control;
u32 tx_flags = le32_to_cpu(tx_cmd->tx_flags);
@@ -294,7 +295,7 @@ void iwl_mvm_set_tx_cmd(struct iwl_mvm *mvm, struct sk_buff
*skb,
tx_cmd->tx_flags = cpu_to_le32(tx_flags);
/* Total # bytes to be transmitted */
tx_cmd->len = cpu_to_le16((u16)skb->len +
- (uintptr_t)info->driver_data[0]);
+ (uintptr_t)skb_info->driver_data[0]);
tx_cmd->life_time = cpu_to_le32(TX_CMD_LIFE_TIME_INFINITE);
tx_cmd->sta_id = sta_id;
--
2.8.1
Re: iwlwifi: mvm: add reorder buffer per queue
On Fri, 2016-05-13 at 11:54 +0300, Dan Carpenter wrote: > Hello Sara Sharon, > > The patch b915c10174fb: "iwlwifi: mvm: add reorder buffer per queue" > from Mar 23, 2016, leads to the following static checker warnings: > > drivers/net/wireless/intel/iwlwifi/mvm/rxmq.c:912 > iwl_mvm_rx_mpdu_mq() > error: potential NULL dereference 'sta'. > > drivers/net/wireless/intel/iwlwifi/mvm/rxmq.c:912 > iwl_mvm_rx_mpdu_mq() > error: we previously assumed 'sta' could be null (see line 796) Thanks for the analysis and report, Dan! I have queued a fix for this through our internal tree. -- Cheers, Luca.
