Re: [PATCH] iwlwifi: Fix softirq/hardirq disabling in iwl_pcie_enqueue_hcmd()

[Luca Coelho]

On Sat, 2021-03-13 at 19:06 +0200, Kalle Valo wrote:
> Luca Coelho  writes:
> 
> > On Sat, 2021-03-13 at 16:43 +0100, Jiri Kosina wrote:
> > > On Sat, 13 Mar 2021, Kalle Valo wrote:
> > > 
> > > > > > > From: Jiri Kosina 
> > > > > > > 
> > > > > > > It's possible for iwl_pcie_enqueue_hcmd() to be called with hard 
> > > > > > > IRQs 
> > > > > > > disabled (e.g. from LED core). We can't enable BHs in such a 
> > > > > > > situation.
> > > > > > > 
> > > > > > > Turn the unconditional BH-enable/BH-disable code into 
> > > > > > > hardirq-disable/conditional-enable.
> > > > > > > 
> > > > > > > This fixes the warning below.
> > > > > > 
> > > > > > Hi,
> > > > > > 
> > > > > > friendly ping on this one ... 
> > > > > 
> > > > > Luca,
> > > > > 
> > > > > Johannes is telling me that he merged this patch internally, but I 
> > > > > have no 
> > > > > idea what is happening to it ... ?
> > > > > 
> > > > > The reported splat is a clear bug, so it should be fixed one way or 
> > > > > the 
> > > > > other.
> > > > 
> > > > Should I take this to wireless-drivers?
> > > 
> > > I can't speak for the maintainers, but as far as I am concerned, it 
> > > definitely is a 5.12 material, as it fixes real scheduling bug.
> > 
> > Yes, please take this to w-d.  We have a similar patch internally, but
> > there's a backlog and it will take me some time to get to it.  I'll
> > resolve eventual conflicts when time comes.
> 
> Ok, can I have your ack for patchwork?

Sorry, forgot that.

Acked-by: Luca Coelho 

--
Cheers,
Luca.

Re: [PATCH] iwlwifi: Fix softirq/hardirq disabling in iwl_pcie_enqueue_hcmd()

[Luca Coelho]

On Sat, 2021-03-13 at 16:43 +0100, Jiri Kosina wrote:
> On Sat, 13 Mar 2021, Kalle Valo wrote:
> 
> > > > > From: Jiri Kosina 
> > > > > 
> > > > > It's possible for iwl_pcie_enqueue_hcmd() to be called with hard IRQs 
> > > > > disabled (e.g. from LED core). We can't enable BHs in such a 
> > > > > situation.
> > > > > 
> > > > > Turn the unconditional BH-enable/BH-disable code into 
> > > > > hardirq-disable/conditional-enable.
> > > > > 
> > > > > This fixes the warning below.
> > > > 
> > > > Hi,
> > > > 
> > > > friendly ping on this one ... 
> > > 
> > > Luca,
> > > 
> > > Johannes is telling me that he merged this patch internally, but I have 
> > > no 
> > > idea what is happening to it ... ?
> > > 
> > > The reported splat is a clear bug, so it should be fixed one way or the 
> > > other.
> > 
> > Should I take this to wireless-drivers?
> 
> I can't speak for the maintainers, but as far as I am concerned, it 
> definitely is a 5.12 material, as it fixes real scheduling bug.

Yes, please take this to w-d.  We have a similar patch internally, but
there's a backlog and it will take me some time to get to it.  I'll
resolve eventual conflicts when time comes.

Thanks!

--
Cheers,
Luca.

Re: [PATCH v2 00/29] [Set 1,2,3] Rid W=1 warnings in Wireless

[Luca Coelho]

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

[Luca Coelho]

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

[Luca Coelho]

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: Replace zero-length array with flexible-array

[Luca Coelho]

"Gustavo A. R. Silva"  wrote:

> The current codebase makes use of the zero-length array language
> extension to the C90 standard, but the preferred mechanism to declare
> variable-length types such as these ones is a flexible array member[1][2],
> introduced in C99:
> 
> struct foo {
> int stuff;
> struct boo array[];
> };
> 
> By making use of the mechanism above, we will get a compiler warning
> in case the flexible array does not occur last in the structure, which
> will help us prevent some kind of undefined behavior bugs from being
> inadvertently introduced[3] to the codebase from now on.
> 
> Also, notice that, dynamic memory allocations won't be affected by
> this change:
> 
> "Flexible array members have incomplete type, and so the sizeof operator
> may not be applied. As a quirk of the original implementation of
> zero-length arrays, sizeof evaluates to zero."[1]
> 
> sizeof(flexible-array-member) triggers a warning because flexible array
> members have incomplete type[1]. There are some instances of code in
> which the sizeof operator is being incorrectly/erroneously applied to
> zero-length arrays and the result is zero. Such instances may be hiding
> some bugs. So, this work (flexible-array member conversions) will also
> help to get completely rid of those sorts of issues.
> 
> This issue was found with the help of Coccinelle.
> 
> [1] https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html
> [2] https://github.com/KSPP/linux/issues/21
> [3] commit 76497732932f ("cxgb3/l2t: Fix undefined behaviour")
> 
> Signed-off-by: Gustavo A. R. Silva 

Patch applied to iwlwifi-next.git, thanks.

45c21a0e5ba4 iwlwifi: Replace zero-length array with flexible-array

Re: [PATCH] iwlwifi: fix memory leaks in iwl_pcie_ctxt_info_gen3_init

[Luca Coelho]

On Fri, 2019-09-27 at 15:56 -0500, Navid Emamdoost wrote:
> In iwl_pcie_ctxt_info_gen3_init there are cases that the allocated dma
> memory is leaked in case of error.
> DMA memories prph_scratch, prph_info, and ctxt_info_gen3 are allocated
> and initialized to be later assigned to trans_pcie. But in any error case
> before such assignment the allocated memories should be released.
> First of such error cases happens when iwl_pcie_init_fw_sec fails.
> Current implementation correctly releases prph_scratch. But in two
> sunsequent error cases where dma_alloc_coherent may fail, such releases
> are missing. This commit adds release for prph_scratch when allocation
> for prph_info fails, and adds releases for prph_scratch and prph_info
> when allocation for ctxt_info_gen3 fails.
> 
> Fixes: 2ee824026288 ("iwlwifi: pcie: support context information for 22560 
> devices")
> 
> Signed-off-by: Navid Emamdoost 
> ---

Thanks, Navid! I have applied this to our internal tree and it will
reach the mainline following our usual upstreaming process.

--
Cheers,
Luca.

Re: [PATCH] iwlwifi: dbg_ini: fix memory leak in alloc_sgtable

[Luca Coelho]

On Thu, 2019-09-12 at 23:23 -0500, Navid Emamdoost wrote:
> In alloc_sgtable if alloc_page fails, the alocated table should be
> released.
> 
> Signed-off-by: Navid Emamdoost 
> ---
>  drivers/net/wireless/intel/iwlwifi/fw/dbg.c | 1 +
>  1 file changed, 1 insertion(+)

Thanks, Navid! I have applied this to our internal tree and it will
reach the mainline following our usual upstreaming process.

--
Cheers,
Luca.

Re: [PATCH] iwlwifi: fix building without CONFIG_THERMAL

[Luca Coelho]

On Thu, 2019-09-19 at 13:55 +0200, Arnd Bergmann wrote:
> The iwl_mvm_send_temp_report_ths_cmd() function is now called without
> CONFIG_THERMAL, but not defined:
> 
> ERROR: "iwl_mvm_send_temp_report_ths_cmd" 
> [drivers/net/wireless/intel/iwlwifi/mvm/iwlmvm.ko] undefined!
> 
> Move that function out of the #ifdef as well and change it so
> that empty data gets sent even if no thermal device was
> registered.
> 
> Fixes: 242d9c8b9a93 ("iwlwifi: mvm: use FW thermal monitoring regardless of 
> CONFIG_THERMAL")
> Signed-off-by: Arnd Bergmann 
> ---
> No idea if this does what was intended in the commit that introduced
> the link failure, please see for youself.

Thanks for the fix, Arnd! We already sent a fix for this though[1] and
Kalle has already queued it for v5.3.

[1] https://patchwork.kernel.org/patch/11150431/

--
Cheers,
Luca.

Re: [PATCH] iwlwifi: mvm: Move static keyword to the front of declarations

[Luca Coelho]

On Sun, 2019-09-01 at 00:01 +0200, Krzysztof Wilczynski wrote:
> Move the static keyword to the front of declarations of
> he_if_types_ext_capa_sta and he_iftypes_ext_capa, and
> resolve the following compiler warnings that can be seen
> when building with warnings enabled (W=1):
> 
> drivers/net/wireless/intel/iwlwifi/mvm/mac80211.c:427:1: warning:
>   ‘static’ is not at beginning of declaration [-Wold-style-declaration]
> 
> drivers/net/wireless/intel/iwlwifi/mvm/mac80211.c:434:1: warning:
>   ‘static’ is not at beginning of declaration [-Wold-style-declaration]
> 
> Signed-off-by: Krzysztof Wilczynski 
> ---
> Related: https://lore.kernel.org/r/20190827233017.gk9...@google.com
> 
>  drivers/net/wireless/intel/iwlwifi/mvm/mac80211.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/mac80211.c 
> b/drivers/net/wireless/intel/iwlwifi/mvm/mac80211.c
> index d6499763f0dd..937a843fed56 100644
> --- a/drivers/net/wireless/intel/iwlwifi/mvm/mac80211.c
> +++ b/drivers/net/wireless/intel/iwlwifi/mvm/mac80211.c
> @@ -424,14 +424,14 @@ int iwl_mvm_init_fw_regd(struct iwl_mvm *mvm)
>   return ret;
>  }
>  
> -const static u8 he_if_types_ext_capa_sta[] = {
> +static const u8 he_if_types_ext_capa_sta[] = {
>[0] = WLAN_EXT_CAPA1_EXT_CHANNEL_SWITCHING,
>[2] = WLAN_EXT_CAPA3_MULTI_BSSID_SUPPORT,
>[7] = WLAN_EXT_CAPA8_OPMODE_NOTIF,
>[9] = WLAN_EXT_CAPA10_TWT_REQUESTER_SUPPORT,
>  };
>  
> -const static struct wiphy_iftype_ext_capab he_iftypes_ext_capa[] = {
> +static const struct wiphy_iftype_ext_capab he_iftypes_ext_capa[] = {
>   {
>   .iftype = NL80211_IFTYPE_STATION,
>   .extended_capabilities = he_if_types_ext_capa_sta,

Thanks for your patch! Though we already have this change in our
internal tree (submitted by YueHaibing) and it will reach the mainline
soon.

--
Cheers,
Luca.

Re: [PATCH] iwlwifi: remove redundant assignment to variable bufsz

[Luca Coelho]

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: PROBLEM: 5.3.0-rc* causes iwlwifi failure

[Luca Coelho]

On Thu, 2019-08-22 at 09:59 +0100, Chris Clayton wrote:
> Thanks, Stuart.
> 
> On 18/08/2019 11:55, Stuart Little wrote:
> > On Sun, Aug 18, 2019 at 09:17:59AM +0100, Chris Clayton wrote:
> > > 
> > > On 17/08/2019 22:44, Stuart Little wrote:
> > > > After some private coaching from Serge Belyshev on git-revert I can 
> > > > confirm that reverting that commit atop the current tree resolves the 
> > > > issue (the wifi card scans for and finds networks just fine, no dmesg 
> > > > errors reported, etc.).
> > > > 
> > > 
> > > I've reported the "Microcode SW error detected" issue too, but, wrongly, 
> > > only to LKML. I'll point that thread to this
> > > one. I've also been experiencing my network stopping working after 
> > > suspend resume, but haven't got round to reporting
> > > that yet.
> > > 
> > > What was the git magic that you acquired to revert the patch, please?
> > > 
> 
> By following the advice below, I reverted 
> 4fd445a2c855bbcab81fbe06d110e78dbd974a5b and using the resultant kernel I
> haven't seen the "Microcode SW error detected" again. I am, however, still 
> experiencing the problem of my network not
> working after resume from suspend. I've reported it to LKML, so it can be 
> followed there should anyone need/want to.

FWIW, we're tracking the iwlwifi bug here:

https://bugzilla.kernel.org/show_bug.cgi?id=204151

I'm thinking about how to solve this and will probably have a proper
patch by the end of the week.

--
Cheers,
Luca.

Re: Regression with the latest iwlwifi-9260-*-46.ucode

[Luca Coelho]

On Fri, 2019-08-09 at 22:00 +0300, Thomas Backlund wrote:
> Den 06-08-2019 kl. 16:04, skrev Takashi Iwai:
> > On Mon, 05 Aug 2019 14:03:55 +0200,
> > Now we got a feedback from the latest linux-firmware (20190726) and
> > surprising the result was negative.  The dmesg after the cold boot is
> > found at:
> >https://bugzilla.suse.com/show_bug.cgi?id=1142128#c26
> > 
> > The kernel is 5.2.3, so it should be new enough.
> > 
> > If anything else needed (or something missing), let us know.
> > 
> 
> I read on some forum that some commented that the "Add support for SAR 
> South Korea limitation" fix is needed, but it seemed weird...
> 
> Anyway, with theese on top of 5.2.7
> 
> 39bd984c203e86f3  iwlwifi: mvm: don't send GEO_TX_POWER_LIMIT on version 
> < 41
> f5a47fae6aa3eb06  iwlwifi: mvm: fix version check for GEO_TX_POWER_LIMIT 
> support
> 0c3d7282233c7b02  iwlwifi: Add support for SAR South Korea limitation
> 
> 
> We have confirmation from an affected user that its now stable with both 
> older and newer firmwares...
> 
> And we earlier tried with only the:
> 39bd984c203e86f3  iwlwifi: mvm: don't send GEO_TX_POWER_LIMIT on version 
> < 41
> 
> But that did not help (not that I really expected it since its loading 
> version 46 firmwares anyway)
> 
> So my guess is that the newer firmware actually subtly expects to get 
> the behaviour of the:
> 0c3d7282233c7b02  iwlwifi: Add support for SAR South Korea limitation
> 
> Of course that's still guessing and I assume only Intel fw guys can 
> verify that...

Yes, you need the 3 patches.  The first two should solve the
"BAD_COMMAND" issue and the last one fixes the "NMI_INTERRUPT_WDG"
issue.

The first two are already in v5.3-rc4 and in v5.2.9 stable.

I'm going to send the third one to stable now.

--
Cheers,
Luca.

Re: Regression with the latest iwlwifi-9260-*-46.ucode

[Luca Coelho]

On Mon, 2019-08-05 at 13:10 +0300, Luca Coelho wrote:
> On Mon, 2019-08-05 at 12:05 +0200, Takashi Iwai wrote:
> > On Mon, 05 Aug 2019 11:53:33 +0200,
> > Luca Coelho wrote:
> > > On Mon, 2019-08-05 at 12:48 +0300, Luca Coelho wrote:
> > > > On Sun, 2019-07-21 at 18:43 +0200, Takashi Iwai wrote:
> > > > > On Sat, 20 Jul 2019 22:49:33 +0200,
> > > > > Luca Coelho wrote:
> > > > > > On Sat, 2019-07-20 at 22:42 +0200, Takashi Iwai wrote:
> > > > > > > On Fri, 19 Jul 2019 20:07:46 +0200,
> > > > > > > Takashi Iwai wrote:
> > > > > > > > On Fri, 19 Jul 2019 18:36:53 +0200,
> > > > > > > > Luciano Coelho wrote:
> > > > > > > > > Adding Dor.
> > > > > > > > > 
> > > > > > > > > Hi Takashi,
> > > > > > > > > 
> > > > > > > > > Do you have full logs of the crash? We can't see much from 
> > > > > > > > > the log
> > > > > > > > > snippet pasted in the bug report.
> > > > > > > > 
> > > > > > > > OK, I'll ask reporters.  If you have a SUSE/openSUSE bugzilla 
> > > > > > > > account,
> > > > > > > > feel free to join there.
> > > > > > > 
> > > > > > > FYI, the dmesg's have been uploaded to the same bugzilla entry:
> > > > > > >   https://bugzilla.opensuse.org/show_bug.cgi?id=1142128
> > > > > > > 
> > > > > > 
> > > > > > Thanks!
> > > > > > 
> > > > > > BTW, I pushed new firmwares to our firmware tree in git.kernel.org
> > > > > > today.  This is the patch:
> > > > > > 
> > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/iwlwifi/linux-firmware.git/commit/?id=b5f09bb4f816abace0227d0f4e749859364cef6b
> > > > > > 
> > > > > > It would be great if you can try it out and let us know whether the 
> > > > > > problem is gone or not.
> > > > > 
> > > > > I created a test package and asked for testing.
> > > > > The test result seems negative, showing the same error,
> > > > > unfortunately.
> > > > > 
> > > > > The dmesg was uploaded on the bugzilla entry.
> > > > 
> > > > Thanks Takashi! We will look into them as soon as possible (sorry for
> > > > the late reply, I just came back from vacations).
> > > 
> > > Actually, I just noticed that your bugzilla is closed as "RESOLVED
> > > FIXED".  Is this still an issue?
> > 
> > It's "closed" because our package contains the revert.
> > 
> > > This seems like a mismatch between the WiFi and BT firmwares... And
> > > most likely the same issue as this:
> > > 
> > > https://bugzilla.kernel.org/show_bug.cgi?id=202163
> > 
> > OK, so we need the update of the whole linux-firmware.
> > I'll refresh the package and ask for testing.
> 
> I'm not sure the new BT firmware is in linux-firmware yet, since I
> don't handle BT stuff.  I hope it is.

I just double-checked this and got confirmation that the latest BT
firmware in linux-firmware.git is compatible with the latest WiFi
firmware there as well.

The only caveat is that the machine needs to be cold-booted for it to
work

But we are taking steps to make sure this will not happen next time
(i.e. for future hardware), by syncing our BT and WiFi firmware
releases.

--
Cheers,
Luca.

Re: Regression with the latest iwlwifi-9260-*-46.ucode

[Luca Coelho]

On Mon, 2019-08-05 at 12:05 +0200, Takashi Iwai wrote:
> On Mon, 05 Aug 2019 11:53:33 +0200,
> Luca Coelho wrote:
> > On Mon, 2019-08-05 at 12:48 +0300, Luca Coelho wrote:
> > > On Sun, 2019-07-21 at 18:43 +0200, Takashi Iwai wrote:
> > > > On Sat, 20 Jul 2019 22:49:33 +0200,
> > > > Luca Coelho wrote:
> > > > > On Sat, 2019-07-20 at 22:42 +0200, Takashi Iwai wrote:
> > > > > > On Fri, 19 Jul 2019 20:07:46 +0200,
> > > > > > Takashi Iwai wrote:
> > > > > > > On Fri, 19 Jul 2019 18:36:53 +0200,
> > > > > > > Luciano Coelho wrote:
> > > > > > > > Adding Dor.
> > > > > > > > 
> > > > > > > > Hi Takashi,
> > > > > > > > 
> > > > > > > > Do you have full logs of the crash? We can't see much from the 
> > > > > > > > log
> > > > > > > > snippet pasted in the bug report.
> > > > > > > 
> > > > > > > OK, I'll ask reporters.  If you have a SUSE/openSUSE bugzilla 
> > > > > > > account,
> > > > > > > feel free to join there.
> > > > > > 
> > > > > > FYI, the dmesg's have been uploaded to the same bugzilla entry:
> > > > > >   https://bugzilla.opensuse.org/show_bug.cgi?id=1142128
> > > > > > 
> > > > > 
> > > > > Thanks!
> > > > > 
> > > > > BTW, I pushed new firmwares to our firmware tree in git.kernel.org
> > > > > today.  This is the patch:
> > > > > 
> > > > > https://git.kernel.org/pub/scm/linux/kernel/git/iwlwifi/linux-firmware.git/commit/?id=b5f09bb4f816abace0227d0f4e749859364cef6b
> > > > > 
> > > > > It would be great if you can try it out and let us know whether the 
> > > > > problem is gone or not.
> > > > 
> > > > I created a test package and asked for testing.
> > > > The test result seems negative, showing the same error,
> > > > unfortunately.
> > > > 
> > > > The dmesg was uploaded on the bugzilla entry.
> > > 
> > > Thanks Takashi! We will look into them as soon as possible (sorry for
> > > the late reply, I just came back from vacations).
> > 
> > Actually, I just noticed that your bugzilla is closed as "RESOLVED
> > FIXED".  Is this still an issue?
> 
> It's "closed" because our package contains the revert.
> 
> > This seems like a mismatch between the WiFi and BT firmwares... And
> > most likely the same issue as this:
> > 
> > https://bugzilla.kernel.org/show_bug.cgi?id=202163
> 
> OK, so we need the update of the whole linux-firmware.
> I'll refresh the package and ask for testing.

I'm not sure the new BT firmware is in linux-firmware yet, since I
don't handle BT stuff.  I hope it is.

--
Cheers,
Luca.

Re: Regression with the latest iwlwifi-9260-*-46.ucode

[Luca Coelho]

On Mon, 2019-08-05 at 12:48 +0300, Luca Coelho wrote:
> On Sun, 2019-07-21 at 18:43 +0200, Takashi Iwai wrote:
> > On Sat, 20 Jul 2019 22:49:33 +0200,
> > Luca Coelho wrote:
> > > On Sat, 2019-07-20 at 22:42 +0200, Takashi Iwai wrote:
> > > > On Fri, 19 Jul 2019 20:07:46 +0200,
> > > > Takashi Iwai wrote:
> > > > > On Fri, 19 Jul 2019 18:36:53 +0200,
> > > > > Luciano Coelho wrote:
> > > > > > Adding Dor.
> > > > > > 
> > > > > > Hi Takashi,
> > > > > > 
> > > > > > Do you have full logs of the crash? We can't see much from the log
> > > > > > snippet pasted in the bug report.
> > > > > 
> > > > > OK, I'll ask reporters.  If you have a SUSE/openSUSE bugzilla account,
> > > > > feel free to join there.
> > > > 
> > > > FYI, the dmesg's have been uploaded to the same bugzilla entry:
> > > >   https://bugzilla.opensuse.org/show_bug.cgi?id=1142128
> > > > 
> > > 
> > > Thanks!
> > > 
> > > BTW, I pushed new firmwares to our firmware tree in git.kernel.org
> > > today.  This is the patch:
> > > 
> > > https://git.kernel.org/pub/scm/linux/kernel/git/iwlwifi/linux-firmware.git/commit/?id=b5f09bb4f816abace0227d0f4e749859364cef6b
> > > 
> > > It would be great if you can try it out and let us know whether the 
> > > problem is gone or not.
> > 
> > I created a test package and asked for testing.
> > The test result seems negative, showing the same error,
> > unfortunately.
> > 
> > The dmesg was uploaded on the bugzilla entry.
> 
> Thanks Takashi! We will look into them as soon as possible (sorry for
> the late reply, I just came back from vacations).

Actually, I just noticed that your bugzilla is closed as "RESOLVED
FIXED".  Is this still an issue?

This seems like a mismatch between the WiFi and BT firmwares... And
most likely the same issue as this:

https://bugzilla.kernel.org/show_bug.cgi?id=202163


--
Cheers,
Luca.

Re: Regression with the latest iwlwifi-9260-*-46.ucode

[Luca Coelho]

On Sun, 2019-07-21 at 18:43 +0200, Takashi Iwai wrote:
> On Sat, 20 Jul 2019 22:49:33 +0200,
> Luca Coelho wrote:
> > On Sat, 2019-07-20 at 22:42 +0200, Takashi Iwai wrote:
> > > On Fri, 19 Jul 2019 20:07:46 +0200,
> > > Takashi Iwai wrote:
> > > > On Fri, 19 Jul 2019 18:36:53 +0200,
> > > > Luciano Coelho wrote:
> > > > > Adding Dor.
> > > > > 
> > > > > Hi Takashi,
> > > > > 
> > > > > Do you have full logs of the crash? We can't see much from the log
> > > > > snippet pasted in the bug report.
> > > > 
> > > > OK, I'll ask reporters.  If you have a SUSE/openSUSE bugzilla account,
> > > > feel free to join there.
> > > 
> > > FYI, the dmesg's have been uploaded to the same bugzilla entry:
> > >   https://bugzilla.opensuse.org/show_bug.cgi?id=1142128
> > > 
> > 
> > Thanks!
> > 
> > BTW, I pushed new firmwares to our firmware tree in git.kernel.org
> > today.  This is the patch:
> > 
> > https://git.kernel.org/pub/scm/linux/kernel/git/iwlwifi/linux-firmware.git/commit/?id=b5f09bb4f816abace0227d0f4e749859364cef6b
> > 
> > It would be great if you can try it out and let us know whether the problem 
> > is gone or not.
> 
> I created a test package and asked for testing.
> The test result seems negative, showing the same error,
> unfortunately.
> 
> The dmesg was uploaded on the bugzilla entry.

Thanks Takashi! We will look into them as soon as possible (sorry for
the late reply, I just came back from vacations).

--
Cheers,
Luca.

Re: Regression with the latest iwlwifi-9260-*-46.ucode

[Luca Coelho]

On Sat, 2019-07-20 at 22:42 +0200, Takashi Iwai wrote:
> On Fri, 19 Jul 2019 20:07:46 +0200,
> Takashi Iwai wrote:
> > On Fri, 19 Jul 2019 18:36:53 +0200,
> > Luciano Coelho wrote:
> > > Adding Dor.
> > > 
> > > Hi Takashi,
> > > 
> > > Do you have full logs of the crash? We can't see much from the log
> > > snippet pasted in the bug report.
> > 
> > OK, I'll ask reporters.  If you have a SUSE/openSUSE bugzilla account,
> > feel free to join there.
> 
> FYI, the dmesg's have been uploaded to the same bugzilla entry:
>   https://bugzilla.opensuse.org/show_bug.cgi?id=1142128
> 

Thanks!

BTW, I pushed new firmwares to our firmware tree in git.kernel.org
today.  This is the patch:

https://git.kernel.org/pub/scm/linux/kernel/git/iwlwifi/linux-firmware.git/commit/?id=b5f09bb4f816abace0227d0f4e749859364cef6b

It would be great if you can try it out and let us know whether the problem is 
gone or not.

--
Cheers,
Luca.

Re: [PATCH -next] iwlwifi: dbg: work around clang bug by marking debug strings static

[Luca Coelho]

On Tue, 2019-07-16 at 10:28 -0700, Nick Desaulniers wrote:
> On Thu, Jul 11, 2019 at 7:15 PM Joe Perches  wrote:
> > On Thu, 2019-07-11 at 17:17 -0700, Nick Desaulniers wrote:
> > > Commit r353569 in prerelease Clang-9 is producing a linkage failure:
> > > 
> > > ld: drivers/net/wireless/intel/iwlwifi/fw/dbg.o:
> > > in function `_iwl_fw_dbg_apply_point':
> > > dbg.c:(.text+0x827a): undefined reference to `__compiletime_assert_2387'
> > > 
> > > when the following configs are enabled:
> > > - CONFIG_IWLWIFI
> > > - CONFIG_IWLMVM
> > > - CONFIG_KASAN
> > > 
> > > Work around the issue for now by marking the debug strings as `static`,
> > > which they probably should be any ways.
> > > 
> > > Link: https://bugs.llvm.org/show_bug.cgi?id=42580
> > > Link: https://github.com/ClangBuiltLinux/linux/issues/580
> > > Reported-by: Arnd Bergmann 
> > > Reported-by: Nathan Chancellor 
> > > Signed-off-by: Nick Desaulniers 
> > > ---
> > >  drivers/net/wireless/intel/iwlwifi/fw/dbg.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/net/wireless/intel/iwlwifi/fw/dbg.c 
> > > b/drivers/net/wireless/intel/iwlwifi/fw/dbg.c
> > > index e411ac98290d..f8c90ea4e9b4 100644
> > > --- a/drivers/net/wireless/intel/iwlwifi/fw/dbg.c
> > > +++ b/drivers/net/wireless/intel/iwlwifi/fw/dbg.c
> > > @@ -2438,7 +2438,7 @@ static void iwl_fw_dbg_info_apply(struct 
> > > iwl_fw_runtime *fwrt,
> > >  {
> > >   u32 img_name_len = le32_to_cpu(dbg_info->img_name_len);
> > >   u32 dbg_cfg_name_len = le32_to_cpu(dbg_info->dbg_cfg_name_len);
> > > - const char err_str[] =
> > > + static const char err_str[] =
> > >   "WRT: ext=%d. Invalid %s name length %d, expected %d\n";
> > 
> > Better still would be to use the format string directly
> > in both locations instead of trying to deduplicate it
> > via storing it into a separate pointer.
> > 
> > Let the compiler/linker consolidate the format.
> > It's smaller object code, allows format/argument verification,
> > and is simpler for humans to understand.
> 
> Whichever Kalle prefers, I just want my CI green again.

We already changed this in a later internal patch, which will reach the
mainline (-next) soon.  So let's skip this for now.

--
Cheers,
Luca.

Re: [PATCH] iwlwifi: fix warning iwl-trans.h is included more than once

[Luca Coelho]

On Sun, 2019-05-26 at 17:08 +0530, Hariprasad Kelam wrote:
> remove duplication include of iwl-trans.h
> 
> issue identified by includecheck
> 
> Signed-off-by: Hariprasad Kelam 
> ---

Thanks! I have applied this (with small modifications to the commit
message) in our internal tree and it will reach the mainline following
our normal upstreaming process.

--
Cheers,
Luca.

Re: [PATCH][next] iwlwifi: mvm: fix comparison of u32 variable with less than zero

[Luca Coelho]

On Mon, 2019-07-01 at 17:26 +0100, Colin King wrote:
> From: Colin Ian King 
> 
> The comparison of the u32 variable wgds_tbl_idx with less than zero is
> always going to be false because it is unsigned.  Fix this by making
> wgds_tbl_idx a plain signed int.
> 
> Addresses-Coverity: ("Unsigned compared against 0")
> Fixes: 4fd445a2c855 ("iwlwifi: mvm: Add log information about SAR status")
> Signed-off-by: Colin Ian King 
> ---
>  drivers/net/wireless/intel/iwlwifi/mvm/nvm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/nvm.c 
> b/drivers/net/wireless/intel/iwlwifi/mvm/nvm.c
> index 719f793b3487..a9bb43a2f27b 100644
> --- a/drivers/net/wireless/intel/iwlwifi/mvm/nvm.c
> +++ b/drivers/net/wireless/intel/iwlwifi/mvm/nvm.c
> @@ -620,7 +620,7 @@ void iwl_mvm_rx_chub_update_mcc(struct iwl_mvm *mvm,
>   enum iwl_mcc_source src;
>   char mcc[3];
>   struct ieee80211_regdomain *regd;
> - u32 wgds_tbl_idx;
> + int wgds_tbl_idx;
>  
>   lockdep_assert_held(>mutex);

Thanks, Colin!

I applied this to our internal tree and it will reach the mainline
following our normal upstreaming process.

--
Cheers,
Luca.

[PATCH] iwlwifi: don't panic in error path on non-msix systems

[Luca Coelho]

From: Shahar S Matityahu 

The driver uses msix causes-register to handle both msix and non msix
interrupts when performing sync nmi.  On devices that do not support
msix this register is unmapped and accessing it causes a kernel panic.

Solve this by differentiating the two cases and accessing the proper
causes-register in each case.

Reported-by: Michal Hocko 
Signed-off-by: Shahar S Matityahu 
Signed-off-by: Luca Coelho 
---
 .../net/wireless/intel/iwlwifi/pcie/trans.c   | 19 +--
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/drivers/net/wireless/intel/iwlwifi/pcie/trans.c 
b/drivers/net/wireless/intel/iwlwifi/pcie/trans.c
index 79c1dc05f948..c4375b868901 100644
--- a/drivers/net/wireless/intel/iwlwifi/pcie/trans.c
+++ b/drivers/net/wireless/intel/iwlwifi/pcie/trans.c
@@ -3644,20 +3644,27 @@ struct iwl_trans *iwl_trans_pcie_alloc(struct pci_dev 
*pdev,
 
 void iwl_trans_pcie_sync_nmi(struct iwl_trans *trans)
 {
+   struct iwl_trans_pcie *trans_pcie = IWL_TRANS_GET_PCIE_TRANS(trans);
unsigned long timeout = jiffies + IWL_TRANS_NMI_TIMEOUT;
+   u32 inta_addr, sw_err_bit;
+
+   if (trans_pcie->msix_enabled) {
+   inta_addr = CSR_MSIX_HW_INT_CAUSES_AD;
+   sw_err_bit = MSIX_HW_INT_CAUSES_REG_SW_ERR;
+   } else {
+   inta_addr = CSR_INT;
+   sw_err_bit = CSR_INT_BIT_SW_ERR;
+   }
 
iwl_disable_interrupts(trans);
iwl_force_nmi(trans);
while (time_after(timeout, jiffies)) {
-   u32 inta_hw = iwl_read32(trans,
-CSR_MSIX_HW_INT_CAUSES_AD);
+   u32 inta_hw = iwl_read32(trans, inta_addr);
 
/* Error detected by uCode */
-   if (inta_hw & MSIX_HW_INT_CAUSES_REG_SW_ERR) {
+   if (inta_hw & sw_err_bit) {
/* Clear causes register */
-   iwl_write32(trans, CSR_MSIX_HW_INT_CAUSES_AD,
-   inta_hw &
-   MSIX_HW_INT_CAUSES_REG_SW_ERR);
+   iwl_write32(trans, inta_addr, inta_hw & sw_err_bit);
break;
}
 
-- 
2.20.1

Re: [PATCH] iwlwifi: fix 64-bit division

[Luca Coelho]

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

[Luca Coelho]

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: linux-next: build warning after merge of the wireless-drivers-next tree

[Luca Coelho]

On Thu, 2019-01-31 at 10:46 +1100, Stephen Rothwell wrote:
> Hi all,
> 
> After merging the wireless-drivers-next tree, today's linux-next
> build
> (x86_64 allmodconfig) produced this warning:
> 
> drivers/net/wireless/intel/iwlwifi/mvm/rxmq.c:195:13: warning:
> 'iwl_mvm_add_rtap_sniffer_config' defined but not used [-Wunused-
> function]
>  static void iwl_mvm_add_rtap_sniffer_config(struct iwl_mvm *mvm,
>  ^~~
> 
> Introduced by commit
> 
>   9bf13bee2d74 ("iwlwifi: mvm: include configured sniffer AID in
> radiotap")

This was a conflict resolution damage in one of the patches I applied. 
We already have a fix for it[1] and Kalle will apply it to wireless-
drivers-next soon.

Sorry for the trouble, but somehow I didn't see this warning and
kbuildbot also reported successful compilation with it. :(

[1] https://patchwork.kernel.org/patch/10788503/

--
Cheers,
Luca.

Re: [PATCH] net: wireless: prefix header search paths with $(srctree)/

[Luca Coelho]

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

[Luca Coelho]

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

[Luca Coelho]

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

[Luca Coelho]

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: net...@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 07/33] iwlwifi: mvm: use match_string() helper

[Luca Coelho]

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: net...@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)

[Luca Coelho]

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][next] iwlwifi: mvm: remove division by size of sizeof(struct ieee80211_wmm_rule)

[Luca Coelho]

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: UBSAN: Undefined behaviour in drivers/net/wireless/intel/iwlwifi/mvm/utils.c:838:5

[Luca Coelho]

On Thu, 2017-12-21 at 11:49 +0100, Paul Menzel wrote:
> Dear Kalle,
> 
> 
> On 12/21/17 11:38, Kalle Valo wrote:
> > Paul Menzel  writes:
> > 
> > > > http://pastebin.coelho.fi/7b624f474846da52.txt
> > > 
> > > Thank you. The warning is gone now. Thank you. For the next time,
> > > it’d
> > > be great to provide the output of `git format-patch -1`, which
> > > can be
> > > applied with `git am`. The output of `git show` is with my
> > > knowledge
> > > hard to apply.
> > 
> > 'patch -p1 < foo.patch' should work. Of course it omits the commit
> > log
> > but is usually enough for testing purposes.
> 
> That’s what I did in the end. And then I have to do `git commit -a`
> to 
> get the tree in a clean state.
> 
> So `git format-patch -1` and uploading of that, for example with
> some 
> CLI tool, for the win. ;-)

The choice here is: more work for me vs. more work for you. :) But I
guess I can easily do "git format-patch -1 --stdout" and pipe it to my
pastebin script...

--
Luca.

Re: UBSAN: Undefined behaviour in drivers/net/wireless/intel/iwlwifi/mvm/utils.c:838:5

[Luca Coelho]

On Thu, 2017-12-21 at 11:49 +0100, Paul Menzel wrote:
> Dear Kalle,
> 
> 
> On 12/21/17 11:38, Kalle Valo wrote:
> > Paul Menzel  writes:
> > 
> > > > http://pastebin.coelho.fi/7b624f474846da52.txt
> > > 
> > > Thank you. The warning is gone now. Thank you. For the next time,
> > > it’d
> > > be great to provide the output of `git format-patch -1`, which
> > > can be
> > > applied with `git am`. The output of `git show` is with my
> > > knowledge
> > > hard to apply.
> > 
> > 'patch -p1 < foo.patch' should work. Of course it omits the commit
> > log
> > but is usually enough for testing purposes.
> 
> That’s what I did in the end. And then I have to do `git commit -a`
> to 
> get the tree in a clean state.
> 
> So `git format-patch -1` and uploading of that, for example with
> some 
> CLI tool, for the win. ;-)

The choice here is: more work for me vs. more work for you. :) But I
guess I can easily do "git format-patch -1 --stdout" and pipe it to my
pastebin script...

--
Luca.

Re: UBSAN: Undefined behaviour in drivers/net/wireless/intel/iwlwifi/mvm/utils.c:838:5

[Luca Coelho]

On Wed, 2017-12-20 at 12:00 +0100, Paul Menzel wrote:
> Dear Luca,
> 
> 
> Am 18.12.2017 um 19:30 schrieb Luca Coelho:
> > On Wed, 2017-12-13 at 16:32 +0200, Luciano Coelho wrote:
> > > On Wed, 2017-12-13 at 14:25 +0100, Paul Menzel wrote:
> > > > I enabled the undefined behavior sanitizer, and built Linus’ 
> > > > master branch under Ubuntu 17.10 with gcc (Ubuntu 7.2.0-
> > > > 8ubuntu3)
> > > > 7.2.0.
> > > > 
> > > > ``` $ grep UBSAN /boot/config-4.15.0-rc3+ 
> > > > CONFIG_ARCH_HAS_UBSAN_SANITIZE_ALL=y #
> > > > CONFIG_ARCH_WANTS_UBSAN_NO_NULL is not set CONFIG_UBSAN=y 
> > > > CONFIG_UBSAN_SANITIZE_ALL=y # CONFIG_UBSAN_ALIGNMENT is not
> > > > set 
> > > > CONFIG_UBSAN_NULL=y ```
> > > > 
> > > > Starting the system the messages below are printed.
> > > > 
> > > > Starting the system and using the wireless device shows the 
> > > > messages below.
> > > 
> > > Thanks for reporting! This shouldn't cause any problems, but I'll
> > > fix it by checking that the mac80211_queue is not INVALID_QUEUE
> > > (255) which seems to be the trigger for this warning.
> > 
> > Can you try the following patch to see if the problem goes away?
> > 
> > http://pastebin.coelho.fi/7b624f474846da52.txt
> 
> Thank you. The warning is gone now. Thank you.

Thanks for testing! We are actually improving the fix a bit, because it
seems that we should not have gotten INVALID_QUEUE there.  We will have
a two-fold fix (Sari, whom I CCed, is working on the other patch).


>  For the next time, it’d 
> be great to provide the output of `git format-patch -1`, which can
> be 
> applied with `git am`. The output of `git show` is with my knowledge 
> hard to apply.

Ah, sorry about that.  I didn't want to send a full patch here so that
it wouldn't get picked up by patchworks, so I just pastebinned it.

--
Luca.

Re: UBSAN: Undefined behaviour in drivers/net/wireless/intel/iwlwifi/mvm/utils.c:838:5

[Luca Coelho]

On Wed, 2017-12-20 at 12:00 +0100, Paul Menzel wrote:
> Dear Luca,
> 
> 
> Am 18.12.2017 um 19:30 schrieb Luca Coelho:
> > On Wed, 2017-12-13 at 16:32 +0200, Luciano Coelho wrote:
> > > On Wed, 2017-12-13 at 14:25 +0100, Paul Menzel wrote:
> > > > I enabled the undefined behavior sanitizer, and built Linus’ 
> > > > master branch under Ubuntu 17.10 with gcc (Ubuntu 7.2.0-
> > > > 8ubuntu3)
> > > > 7.2.0.
> > > > 
> > > > ``` $ grep UBSAN /boot/config-4.15.0-rc3+ 
> > > > CONFIG_ARCH_HAS_UBSAN_SANITIZE_ALL=y #
> > > > CONFIG_ARCH_WANTS_UBSAN_NO_NULL is not set CONFIG_UBSAN=y 
> > > > CONFIG_UBSAN_SANITIZE_ALL=y # CONFIG_UBSAN_ALIGNMENT is not
> > > > set 
> > > > CONFIG_UBSAN_NULL=y ```
> > > > 
> > > > Starting the system the messages below are printed.
> > > > 
> > > > Starting the system and using the wireless device shows the 
> > > > messages below.
> > > 
> > > Thanks for reporting! This shouldn't cause any problems, but I'll
> > > fix it by checking that the mac80211_queue is not INVALID_QUEUE
> > > (255) which seems to be the trigger for this warning.
> > 
> > Can you try the following patch to see if the problem goes away?
> > 
> > http://pastebin.coelho.fi/7b624f474846da52.txt
> 
> Thank you. The warning is gone now. Thank you.

Thanks for testing! We are actually improving the fix a bit, because it
seems that we should not have gotten INVALID_QUEUE there.  We will have
a two-fold fix (Sari, whom I CCed, is working on the other patch).


>  For the next time, it’d 
> be great to provide the output of `git format-patch -1`, which can
> be 
> applied with `git am`. The output of `git show` is with my knowledge 
> hard to apply.

Ah, sorry about that.  I didn't want to send a full patch here so that
it wouldn't get picked up by patchworks, so I just pastebinned it.

--
Luca.

Re: UBSAN: Undefined behaviour in drivers/net/wireless/intel/iwlwifi/mvm/utils.c:838:5

[Luca Coelho]

On Wed, 2017-12-13 at 16:32 +0200, Luciano Coelho wrote:
> On Wed, 2017-12-13 at 14:25 +0100, Paul Menzel wrote:
> > Dear Linux folks,
> > 
> > 
> > I enabled the undefined behavior sanitizer, and built Linus’
> > master 
> > branch under Ubuntu 17.10 with gcc (Ubuntu 7.2.0-8ubuntu3) 7.2.0.
> > 
> > ```
> > $ grep UBSAN /boot/config-4.15.0-rc3+
> > CONFIG_ARCH_HAS_UBSAN_SANITIZE_ALL=y
> > # CONFIG_ARCH_WANTS_UBSAN_NO_NULL is not set
> > CONFIG_UBSAN=y
> > CONFIG_UBSAN_SANITIZE_ALL=y
> > # CONFIG_UBSAN_ALIGNMENT is not set
> > CONFIG_UBSAN_NULL=y
> > ```
> > 
> > Starting the system the messages below are printed.
> > 
> > Starting the system and using the wireless device shows the
> > messages
> > below.
> 
> Thanks for reporting! This shouldn't cause any problems, but I'll fix
> it by checking that the mac80211_queue is not INVALID_QUEUE (255)
> which seems to be the trigger for this warning.

Can you try the following patch to see if the problem goes away?

http://pastebin.coelho.fi/7b624f474846da52.txt

Thanks!

--
Cheers,
Luca.

Re: UBSAN: Undefined behaviour in drivers/net/wireless/intel/iwlwifi/mvm/utils.c:838:5

[Luca Coelho]

On Wed, 2017-12-13 at 16:32 +0200, Luciano Coelho wrote:
> On Wed, 2017-12-13 at 14:25 +0100, Paul Menzel wrote:
> > Dear Linux folks,
> > 
> > 
> > I enabled the undefined behavior sanitizer, and built Linus’
> > master 
> > branch under Ubuntu 17.10 with gcc (Ubuntu 7.2.0-8ubuntu3) 7.2.0.
> > 
> > ```
> > $ grep UBSAN /boot/config-4.15.0-rc3+
> > CONFIG_ARCH_HAS_UBSAN_SANITIZE_ALL=y
> > # CONFIG_ARCH_WANTS_UBSAN_NO_NULL is not set
> > CONFIG_UBSAN=y
> > CONFIG_UBSAN_SANITIZE_ALL=y
> > # CONFIG_UBSAN_ALIGNMENT is not set
> > CONFIG_UBSAN_NULL=y
> > ```
> > 
> > Starting the system the messages below are printed.
> > 
> > Starting the system and using the wireless device shows the
> > messages
> > below.
> 
> Thanks for reporting! This shouldn't cause any problems, but I'll fix
> it by checking that the mac80211_queue is not INVALID_QUEUE (255)
> which seems to be the trigger for this warning.

Can you try the following patch to see if the problem goes away?

http://pastebin.coelho.fi/7b624f474846da52.txt

Thanks!

--
Cheers,
Luca.

Re: [PATCH] drivers/wireless: iwlwifi/mvm: Convert timers to use timer_setup()

[Luca Coelho]

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 <johannes.b...@intel.com>
> Cc: Emmanuel Grumbach <emmanuel.grumb...@intel.com>
> Cc: Luca Coelho <luciano.coe...@intel.com>
> Cc: Intel Linux Wireless <linuxw...@intel.com>
> Cc: Kalle Valo <kv...@codeaurora.org>
> Cc: Sara Sharon <sara.sha...@intel.com>
> Cc: linux-wirel...@vger.kernel.org
> Cc: net...@vger.kernel.org
> Signed-off-by: Kees Cook <keesc...@chromium.org>
> ---

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.

Re: [PATCH] drivers/wireless: iwlwifi/mvm: Convert timers to use timer_setup()

[Luca Coelho]

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: net...@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.

Re: linux-next: manual merge of the wireless-drivers-next tree with the wireless-drivers tree

[Luca Coelho]

On Thu, 2017-10-12 at 19:59 +0100, Mark Brown wrote:
> On Thu, Oct 12, 2017 at 09:50:51PM +0300, Luca Coelho wrote:
> > On Thu, 2017-10-12 at 19:35 +0100, Mark Brown wrote:
> > > With trees like this that don't coordinate with their fixes
> > > branch
> > > there
> > > are frequently multiple conflicts introduced so I generally
> > > report
> > > things file by file without even looking at the new ones.
> > Sorry for the trouble.  But how do you suggest that we "coordinate
> > our
> > fixes branch"? Merge fixes into the main tree?
> 
> That'd be easiest for me!  It's not of necessity a problem if the
> conflicts are easy enough to resolve if you just let things get
> merged
> in -next, it's just more an observation that that's a thing that
> happens
> and that this is how I cope with it.  Stephen may do things a bit
> differently.

Cool, I'll discuss this with Kalle and make sure I note these potential
conflicts early enough so everyone is aware when they're coming.

Thanks for taking over while Stephen is away! I really appreciate it,
since linux-next is a very important part of our process. :)

--
Luca.

Re: linux-next: manual merge of the wireless-drivers-next tree with the wireless-drivers tree

[Luca Coelho]

On Thu, 2017-10-12 at 19:59 +0100, Mark Brown wrote:
> On Thu, Oct 12, 2017 at 09:50:51PM +0300, Luca Coelho wrote:
> > On Thu, 2017-10-12 at 19:35 +0100, Mark Brown wrote:
> > > With trees like this that don't coordinate with their fixes
> > > branch
> > > there
> > > are frequently multiple conflicts introduced so I generally
> > > report
> > > things file by file without even looking at the new ones.
> > Sorry for the trouble.  But how do you suggest that we "coordinate
> > our
> > fixes branch"? Merge fixes into the main tree?
> 
> That'd be easiest for me!  It's not of necessity a problem if the
> conflicts are easy enough to resolve if you just let things get
> merged
> in -next, it's just more an observation that that's a thing that
> happens
> and that this is how I cope with it.  Stephen may do things a bit
> differently.

Cool, I'll discuss this with Kalle and make sure I note these potential
conflicts early enough so everyone is aware when they're coming.

Thanks for taking over while Stephen is away! I really appreciate it,
since linux-next is a very important part of our process. :)

--
Luca.

Re: linux-next: manual merge of the wireless-drivers-next tree with the wireless-drivers tree

[Luca Coelho]

On Thu, 2017-10-12 at 19:35 +0100, Mark Brown wrote:
> On Thu, Oct 12, 2017 at 09:27:46PM +0300, Luciano Coelho wrote:
> > On Thu, 2017-10-12 at 19:21 +0100, Mark Brown wrote:
> > > I may have confused the trees when I was pasting things in, the
> > > commits
> > > are filled in by hand.
> > 
> > Ah, okay.  But still, if the same patches conflicted twice, why
> > wasn't
> > there only one occurrence with both conflicts at once?
> 
> With trees like this that don't coordinate with their fixes branch
> there
> are frequently multiple conflicts introduced so I generally report
> things file by file without even looking at the new ones.

Sorry for the trouble.  But how do you suggest that we "coordinate our
fixes branch"? Merge fixes into the main tree?

--
Luca.

Re: linux-next: manual merge of the wireless-drivers-next tree with the wireless-drivers tree

[Luca Coelho]

On Thu, 2017-10-12 at 19:35 +0100, Mark Brown wrote:
> On Thu, Oct 12, 2017 at 09:27:46PM +0300, Luciano Coelho wrote:
> > On Thu, 2017-10-12 at 19:21 +0100, Mark Brown wrote:
> > > I may have confused the trees when I was pasting things in, the
> > > commits
> > > are filled in by hand.
> > 
> > Ah, okay.  But still, if the same patches conflicted twice, why
> > wasn't
> > there only one occurrence with both conflicts at once?
> 
> With trees like this that don't coordinate with their fixes branch
> there
> are frequently multiple conflicts introduced so I generally report
> things file by file without even looking at the new ones.

Sorry for the trouble.  But how do you suggest that we "coordinate our
fixes branch"? Merge fixes into the main tree?

--
Luca.

Re: linux-next: manual merge of the wireless-drivers-next tree with the wireless-drivers tree

[Luca Coelho]

On Thu, 2017-10-12 at 18:25 +0100, Mark Brown wrote:
> Hi all,
> 
> Today's linux-next merge of the wireless-drivers-next tree got a
> conflict in:
> 
>   drivers/net/wireless/intel/iwlwifi/iwl-config.h
> 
> between commit:
> 
>dd05f9aab4426f ("iwlwifi: pcie: dynamic Tx command queue size")
> 
> from the wireless-drivers tree and commit:
> 
>44fd09dad5d2b7 ("iwlwifi: nvm: set the correct offsets to 3168
> series")
> 
> from the wireless-drivers-next tree.
> 
> I fixed it up (see below) and can carry the fix as necessary. This
> is now fixed as far as linux-next is concerned, but any non trivial
> conflicts should be mentioned to your upstream maintainer when your
> tree
> is submitted for merging.  You may also want to consider cooperating
> with the maintainer of the conflicting tree to minimise any
> particularly
> complex conflicts.
> 
> diff --cc drivers/net/wireless/intel/iwlwifi/iwl-config.h
> index 71cb1ecde0f7,b9f3b350fe34..
> --- a/drivers/net/wireless/intel/iwlwifi/iwl-config.h
> +++ b/drivers/net/wireless/intel/iwlwifi/iwl-config.h
> @@@ -332,7 -320,9 +332,9 @@@ struct iwl_pwr_tx_backoff 
>* @integrated: discrete or integrated
>* @gen2: a000 and on transport operation
>* @cdb: CDB support
>  - * @ext_nvm: extended NVM format
>  + * @nvm_type: see  iwl_nvm_type
> +  * @tx_cmd_queue_size: size of the cmd queue. If zero, use the same
> value as
> +  *  the regular queues
>*
>* We enable the driver to be backward compatible wrt. hardware
> features.
>* API differences in uCode shouldn't be handled here but through
> TLVs
> @@@ -382,7 -371,9 +384,8 @@@ struct iwl_cfg 
>   use_tfh:1,
>   gen2:1,
>   cdb:1,
>  -ext_nvm:1,

nvm_type seems to be missing from here?

>   dbgc_supported:1;
> + u16 tx_cmd_queue_size;
>   u8 valid_tx_ant;
>   u8 valid_rx_ant;
>   u8 non_shared_ant;

--
Luca.

Re: linux-next: manual merge of the wireless-drivers-next tree with the wireless-drivers tree

[Luca Coelho]

On Thu, 2017-10-12 at 18:25 +0100, Mark Brown wrote:
> Hi all,
> 
> Today's linux-next merge of the wireless-drivers-next tree got a
> conflict in:
> 
>   drivers/net/wireless/intel/iwlwifi/iwl-config.h
> 
> between commit:
> 
>dd05f9aab4426f ("iwlwifi: pcie: dynamic Tx command queue size")
> 
> from the wireless-drivers tree and commit:
> 
>44fd09dad5d2b7 ("iwlwifi: nvm: set the correct offsets to 3168
> series")
> 
> from the wireless-drivers-next tree.
> 
> I fixed it up (see below) and can carry the fix as necessary. This
> is now fixed as far as linux-next is concerned, but any non trivial
> conflicts should be mentioned to your upstream maintainer when your
> tree
> is submitted for merging.  You may also want to consider cooperating
> with the maintainer of the conflicting tree to minimise any
> particularly
> complex conflicts.
> 
> diff --cc drivers/net/wireless/intel/iwlwifi/iwl-config.h
> index 71cb1ecde0f7,b9f3b350fe34..
> --- a/drivers/net/wireless/intel/iwlwifi/iwl-config.h
> +++ b/drivers/net/wireless/intel/iwlwifi/iwl-config.h
> @@@ -332,7 -320,9 +332,9 @@@ struct iwl_pwr_tx_backoff 
>* @integrated: discrete or integrated
>* @gen2: a000 and on transport operation
>* @cdb: CDB support
>  - * @ext_nvm: extended NVM format
>  + * @nvm_type: see  iwl_nvm_type
> +  * @tx_cmd_queue_size: size of the cmd queue. If zero, use the same
> value as
> +  *  the regular queues
>*
>* We enable the driver to be backward compatible wrt. hardware
> features.
>* API differences in uCode shouldn't be handled here but through
> TLVs
> @@@ -382,7 -371,9 +384,8 @@@ struct iwl_cfg 
>   use_tfh:1,
>   gen2:1,
>   cdb:1,
>  -ext_nvm:1,

nvm_type seems to be missing from here?

>   dbgc_supported:1;
> + u16 tx_cmd_queue_size;
>   u8 valid_tx_ant;
>   u8 valid_rx_ant;
>   u8 non_shared_ant;

--
Luca.

Re: 4.14.0-rc3 iwlwifi: Hardware became unavailable during restart

[Luca Coelho]

On Tue, 2017-10-10 at 09:44 +0200, Seraphime Kirkovski wrote:
> Hello,

Hi Seraphime,


> I've got this splat after a couple of suspend-resume cycles on my
> HP-laptop. I haven't had the time to bisect or test other rcs for now.
> Pasting some logs before the actual WARN_ON, as they may be relevant.
> Just after the splat the connection is successfully reestablished.
> 
> [14293.758404] iwlwifi :24:00.0: Error sending REPLY_SCAN_ABORT_CMD: 
> time out after 2000ms.
> [14293.758429] iwlwifi :24:00.0: Current CMD queue read_ptr 67 write_ptr 
> 68
> [14293.758518] iwlwifi :24:00.0: Loaded firmware version: 18.168.6.1

This seems to be a DVM device (iwldvm).  What is the exact device
model? And you have never noticed this problem before?

Unfortunately this is a very old device and we don't support it
actively anymore.  Hopefully this will turn out to be a trivial fix in
the driver side, so we can get it solved for you.

The best way to track this is to report it in
https://bugzilla.kernel.org.

--
Cheers,
Luca.

Re: 4.14.0-rc3 iwlwifi: Hardware became unavailable during restart

[Luca Coelho]

On Tue, 2017-10-10 at 09:44 +0200, Seraphime Kirkovski wrote:
> Hello,

Hi Seraphime,


> I've got this splat after a couple of suspend-resume cycles on my
> HP-laptop. I haven't had the time to bisect or test other rcs for now.
> Pasting some logs before the actual WARN_ON, as they may be relevant.
> Just after the splat the connection is successfully reestablished.
> 
> [14293.758404] iwlwifi :24:00.0: Error sending REPLY_SCAN_ABORT_CMD: 
> time out after 2000ms.
> [14293.758429] iwlwifi :24:00.0: Current CMD queue read_ptr 67 write_ptr 
> 68
> [14293.758518] iwlwifi :24:00.0: Loaded firmware version: 18.168.6.1

This seems to be a DVM device (iwldvm).  What is the exact device
model? And you have never noticed this problem before?

Unfortunately this is a very old device and we don't support it
actively anymore.  Hopefully this will turn out to be a trivial fix in
the driver side, so we can get it solved for you.

The best way to track this is to report it in
https://bugzilla.kernel.org.

--
Cheers,
Luca.

[PATCH] iwlwifi: mvm: only send LEDS_CMD when the FW supports it

[Luca Coelho]

From: Luca Coelho <luciano.coe...@intel.com>

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 <torva...@linux-foundation.org>
Signed-off-by: Luca Coelho <luciano.coe...@intel.com>
---
 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(>fw->ucode_capa,
+   IWL_UCODE_TLV_CAPA_LED_CMD_SUPPORT)) {
iwl_mvm_send_led_fw_cmd(mvm, on);
return;
}
-- 
2.14.1

[PATCH] iwlwifi: mvm: only send LEDS_CMD when the FW supports it

[Luca Coelho]

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

[Luca Coelho]

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
> > <luciano.coe...@intel.com> 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 <johannes.b...@intel.com>
AuthorDate: Wed Jun 28 16:19:49 2017 +0200
Commit: Luca Coelho <luciano.coe...@intel.com>
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 <johannes.b...@intel.com>
Signed-off-by: Luca Coelho <luciano.coe...@intel.com>

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: [GIT] Networking

[Luca Coelho]

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 v2] iwlwifi: pcie: move rx workqueue initialization to iwl_trans_pcie_alloc()

[Luca Coelho]

On Mon, 2017-09-04 at 13:43 +0200, Jiri Kosina wrote:
> On Thu, 24 Aug 2017, Luca Coelho wrote:
> 
> > > looks like a correct fix to me, so feel free to add
> > > 
> > >   Acked-by: Jiri Kosina <jkos...@suse.cz>
> > > 
> > > I'll be able to provide my Tested-by: eventually only in ~10 days.
> > 
> > 
> > Kalle already picked it up in wireless-drivers and this should make it's
> > way to 4.13-rc7 (we hope).
> > 
> > In any case, thanks for reporting and the help debugging it.
> 
> I know it's pretty late for this to be added to the commit, but I don't 
> want this to be left in the void, so for the sake of completness:
> 
>   Tested-by: Jiri Kosina <jkos...@suse.cz>

Thanks, Jiri, for reporting, debugging and testing!

--
Cheers,
Luca.

Re: [PATCH v2] iwlwifi: pcie: move rx workqueue initialization to iwl_trans_pcie_alloc()

[Luca Coelho]

On Mon, 2017-09-04 at 13:43 +0200, Jiri Kosina wrote:
> On Thu, 24 Aug 2017, Luca Coelho wrote:
> 
> > > looks like a correct fix to me, so feel free to add
> > > 
> > >   Acked-by: Jiri Kosina 
> > > 
> > > I'll be able to provide my Tested-by: eventually only in ~10 days.
> > 
> > 
> > Kalle already picked it up in wireless-drivers and this should make it's
> > way to 4.13-rc7 (we hope).
> > 
> > In any case, thanks for reporting and the help debugging it.
> 
> I know it's pretty late for this to be added to the commit, but I don't 
> want this to be left in the void, so for the sake of completness:
> 
>   Tested-by: Jiri Kosina 

Thanks, Jiri, for reporting, debugging and testing!

--
Cheers,
Luca.

Re: [PATCH v2] iwlwifi: pcie: move rx workqueue initialization to iwl_trans_pcie_alloc()

[Luca Coelho]

On Wed, 2017-08-30 at 17:57 +0300, David Weinehall wrote:
> On Tue, Aug 22, 2017 at 10:37:29AM +0300, Luca Coelho wrote:
> > From: Luca Coelho <luciano.coe...@intel.com>
> > 
> > Work queues cannot be allocated when a mutex is held because the mutex
> > may be in use and that would make it sleep.  Doing so generates the
> > following splat with 4.13+:
> > 
> > [   19.513298] ==
> > [   19.513429] WARNING: possible circular locking dependency detected
> > [   19.513557] 4.13.0-rc5+ #6 Not tainted
> > [   19.513638] --
> > [   19.513767] cpuhp/0/12 is trying to acquire lock:
> > [   19.513867]  (>lock){+.+.+.}, at: [] 
> > thermal_zone_get_temp+0x5b/0xb0
> > [   19.514047]
> > [   19.514047] but task is already holding lock:
> > [   19.514166]  (cpuhp_state){+.+.+.}, at: [] 
> > cpuhp_thread_fun+0x3a/0x210
> > [   19.514338]
> > [   19.514338] which lock already depends on the new lock.
> > 
> > This lock dependency already existed with previous kernel versions,
> > but it was not detected until commit 49dfe2a67797 ("cpuhotplug: Link
> > lock stacks for hotplug callbacks") was introduced.
> > 
> > Reported-by: David Weinehall <david.weineh...@intel.com>
> > Reported-by: Jiri Kosina <ji...@kernel.org>
> > Signed-off-by: Luca Coelho <luciano.coe...@intel.com>
> 
> With this patch I no longer get the lockdep warning,
> and the driver seems to work just as well as before.

Great! Thanks for reporting and testing, David!


> Thanks!
> 
> Tested-by: David Weinehall <david.weineh...@linux.intel.com>

Thanks for this too, but unfortunately it's too late to add it, since
the patch is already in net tree.

--
Cheers,
Luca.

Re: [PATCH v2] iwlwifi: pcie: move rx workqueue initialization to iwl_trans_pcie_alloc()

[Luca Coelho]

On Wed, 2017-08-30 at 17:57 +0300, David Weinehall wrote:
> On Tue, Aug 22, 2017 at 10:37:29AM +0300, Luca Coelho wrote:
> > From: Luca Coelho 
> > 
> > Work queues cannot be allocated when a mutex is held because the mutex
> > may be in use and that would make it sleep.  Doing so generates the
> > following splat with 4.13+:
> > 
> > [   19.513298] ==
> > [   19.513429] WARNING: possible circular locking dependency detected
> > [   19.513557] 4.13.0-rc5+ #6 Not tainted
> > [   19.513638] --
> > [   19.513767] cpuhp/0/12 is trying to acquire lock:
> > [   19.513867]  (>lock){+.+.+.}, at: [] 
> > thermal_zone_get_temp+0x5b/0xb0
> > [   19.514047]
> > [   19.514047] but task is already holding lock:
> > [   19.514166]  (cpuhp_state){+.+.+.}, at: [] 
> > cpuhp_thread_fun+0x3a/0x210
> > [   19.514338]
> > [   19.514338] which lock already depends on the new lock.
> > 
> > This lock dependency already existed with previous kernel versions,
> > but it was not detected until commit 49dfe2a67797 ("cpuhotplug: Link
> > lock stacks for hotplug callbacks") was introduced.
> > 
> > Reported-by: David Weinehall 
> > Reported-by: Jiri Kosina 
> > Signed-off-by: Luca Coelho 
> 
> With this patch I no longer get the lockdep warning,
> and the driver seems to work just as well as before.

Great! Thanks for reporting and testing, David!


> Thanks!
> 
> Tested-by: David Weinehall 

Thanks for this too, but unfortunately it's too late to add it, since
the patch is already in net tree.

--
Cheers,
Luca.

Re: [PATCH v2] iwlwifi: pcie: move rx workqueue initialization to iwl_trans_pcie_alloc()

[Luca Coelho]

On Thu, 2017-08-24 at 21:56 +0200, Jiri Kosina wrote:
> On Thu, 24 Aug 2017, Luca Coelho wrote:
> 
> > > Work queues cannot be allocated when a mutex is held because the mutex
> > > may be in use and that would make it sleep.  Doing so generates the
> > > following splat with 4.13+:
> > > 
> > > [   19.513298] ==
> > > [   19.513429] WARNING: possible circular locking dependency detected
> > > [   19.513557] 4.13.0-rc5+ #6 Not tainted
> > > [   19.513638] --
> > > [   19.513767] cpuhp/0/12 is trying to acquire lock:
> > > [   19.513867]  (>lock){+.+.+.}, at: [] 
> > > thermal_zone_get_temp+0x5b/0xb0
> > > [   19.514047]
> > > [   19.514047] but task is already holding lock:
> > > [   19.514166]  (cpuhp_state){+.+.+.}, at: [] 
> > > cpuhp_thread_fun+0x3a/0x210
> > > [   19.514338]
> > > [   19.514338] which lock already depends on the new lock.
> > > 
> > > This lock dependency already existed with previous kernel versions,
> > > but it was not detected until commit 49dfe2a67797 ("cpuhotplug: Link
> > > lock stacks for hotplug callbacks") was introduced.
> > > 
> > > Reported-by: David Weinehall <david.weineh...@intel.com>
> > > Reported-by: Jiri Kosina <ji...@kernel.org>
> > > Signed-off-by: Luca Coelho <luciano.coe...@intel.com>
> > 
> > Jiri, did you have a chance to try this out? I'm about to ask Kalle to
> > merge this so it gets in in time for 4.13-rc7.
> 
> Sorry, I am almost completely offline for one more week (vacation), and 
> will not have access to the affected system before that.

Sounds good! Enjoy! ;)


>  But this indeed 
> looks like a correct fix to me, so feel free to add
> 
>   Acked-by: Jiri Kosina <jkos...@suse.cz>
> 
> I'll be able to provide my Tested-by: eventually only in ~10 days.


Kalle already picked it up in wireless-drivers and this should make it's
way to 4.13-rc7 (we hope).

In any case, thanks for reporting and the help debugging it.

--
Cheers,
Luca.

Re: [PATCH v2] iwlwifi: pcie: move rx workqueue initialization to iwl_trans_pcie_alloc()

[Luca Coelho]

On Thu, 2017-08-24 at 21:56 +0200, Jiri Kosina wrote:
> On Thu, 24 Aug 2017, Luca Coelho wrote:
> 
> > > Work queues cannot be allocated when a mutex is held because the mutex
> > > may be in use and that would make it sleep.  Doing so generates the
> > > following splat with 4.13+:
> > > 
> > > [   19.513298] ==
> > > [   19.513429] WARNING: possible circular locking dependency detected
> > > [   19.513557] 4.13.0-rc5+ #6 Not tainted
> > > [   19.513638] --
> > > [   19.513767] cpuhp/0/12 is trying to acquire lock:
> > > [   19.513867]  (>lock){+.+.+.}, at: [] 
> > > thermal_zone_get_temp+0x5b/0xb0
> > > [   19.514047]
> > > [   19.514047] but task is already holding lock:
> > > [   19.514166]  (cpuhp_state){+.+.+.}, at: [] 
> > > cpuhp_thread_fun+0x3a/0x210
> > > [   19.514338]
> > > [   19.514338] which lock already depends on the new lock.
> > > 
> > > This lock dependency already existed with previous kernel versions,
> > > but it was not detected until commit 49dfe2a67797 ("cpuhotplug: Link
> > > lock stacks for hotplug callbacks") was introduced.
> > > 
> > > Reported-by: David Weinehall 
> > > Reported-by: Jiri Kosina 
> > > Signed-off-by: Luca Coelho 
> > 
> > Jiri, did you have a chance to try this out? I'm about to ask Kalle to
> > merge this so it gets in in time for 4.13-rc7.
> 
> Sorry, I am almost completely offline for one more week (vacation), and 
> will not have access to the affected system before that.

Sounds good! Enjoy! ;)


>  But this indeed 
> looks like a correct fix to me, so feel free to add
> 
>   Acked-by: Jiri Kosina 
> 
> I'll be able to provide my Tested-by: eventually only in ~10 days.


Kalle already picked it up in wireless-drivers and this should make it's
way to 4.13-rc7 (we hope).

In any case, thanks for reporting and the help debugging it.

--
Cheers,
Luca.

Re: [PATCH v2] iwlwifi: pcie: move rx workqueue initialization to iwl_trans_pcie_alloc()

[Luca Coelho]

On Tue, 2017-08-22 at 10:37 +0300, Luca Coelho wrote:
> From: Luca Coelho <luciano.coe...@intel.com>
> 
> Work queues cannot be allocated when a mutex is held because the mutex
> may be in use and that would make it sleep.  Doing so generates the
> following splat with 4.13+:
> 
> [   19.513298] ==
> [   19.513429] WARNING: possible circular locking dependency detected
> [   19.513557] 4.13.0-rc5+ #6 Not tainted
> [   19.513638] --
> [   19.513767] cpuhp/0/12 is trying to acquire lock:
> [   19.513867]  (>lock){+.+.+.}, at: [] 
> thermal_zone_get_temp+0x5b/0xb0
> [   19.514047]
> [   19.514047] but task is already holding lock:
> [   19.514166]  (cpuhp_state){+.+.+.}, at: [] 
> cpuhp_thread_fun+0x3a/0x210
> [   19.514338]
> [   19.514338] which lock already depends on the new lock.
> 
> This lock dependency already existed with previous kernel versions,
> but it was not detected until commit 49dfe2a67797 ("cpuhotplug: Link
> lock stacks for hotplug callbacks") was introduced.
> 
> Reported-by: David Weinehall <david.weineh...@intel.com>
> Reported-by: Jiri Kosina <ji...@kernel.org>
> Signed-off-by: Luca Coelho <luciano.coe...@intel.com>

Jiri, did you have a chance to try this out? I'm about to ask Kalle to
merge this so it gets in in time for 4.13-rc7.

--
Cheers,
Luca.

Re: [PATCH v2] iwlwifi: pcie: move rx workqueue initialization to iwl_trans_pcie_alloc()

[Luca Coelho]

On Tue, 2017-08-22 at 10:37 +0300, Luca Coelho wrote:
> From: Luca Coelho 
> 
> Work queues cannot be allocated when a mutex is held because the mutex
> may be in use and that would make it sleep.  Doing so generates the
> following splat with 4.13+:
> 
> [   19.513298] ==
> [   19.513429] WARNING: possible circular locking dependency detected
> [   19.513557] 4.13.0-rc5+ #6 Not tainted
> [   19.513638] --
> [   19.513767] cpuhp/0/12 is trying to acquire lock:
> [   19.513867]  (>lock){+.+.+.}, at: [] 
> thermal_zone_get_temp+0x5b/0xb0
> [   19.514047]
> [   19.514047] but task is already holding lock:
> [   19.514166]  (cpuhp_state){+.+.+.}, at: [] 
> cpuhp_thread_fun+0x3a/0x210
> [   19.514338]
> [   19.514338] which lock already depends on the new lock.
> 
> This lock dependency already existed with previous kernel versions,
> but it was not detected until commit 49dfe2a67797 ("cpuhotplug: Link
> lock stacks for hotplug callbacks") was introduced.
> 
> Reported-by: David Weinehall 
> Reported-by: Jiri Kosina 
> Signed-off-by: Luca Coelho 

Jiri, did you have a chance to try this out? I'm about to ask Kalle to
merge this so it gets in in time for 4.13-rc7.

--
Cheers,
Luca.

[PATCH v2] iwlwifi: pcie: move rx workqueue initialization to iwl_trans_pcie_alloc()

[Luca Coelho]

From: Luca Coelho <luciano.coe...@intel.com>

Work queues cannot be allocated when a mutex is held because the mutex
may be in use and that would make it sleep.  Doing so generates the
following splat with 4.13+:

[   19.513298] ==
[   19.513429] WARNING: possible circular locking dependency detected
[   19.513557] 4.13.0-rc5+ #6 Not tainted
[   19.513638] --
[   19.513767] cpuhp/0/12 is trying to acquire lock:
[   19.513867]  (>lock){+.+.+.}, at: [] 
thermal_zone_get_temp+0x5b/0xb0
[   19.514047]
[   19.514047] but task is already holding lock:
[   19.514166]  (cpuhp_state){+.+.+.}, at: [] 
cpuhp_thread_fun+0x3a/0x210
[   19.514338]
[   19.514338] which lock already depends on the new lock.

This lock dependency already existed with previous kernel versions,
but it was not detected until commit 49dfe2a67797 ("cpuhotplug: Link
lock stacks for hotplug callbacks") was introduced.

Reported-by: David Weinehall <david.weineh...@intel.com>
Reported-by: Jiri Kosina <ji...@kernel.org>
Signed-off-by: Luca Coelho <luciano.coe...@intel.com>
---
In v2:
   - updated the commit message to a new version, with a grammar fix
 and the actual commit that exposed the problem;

 drivers/net/wireless/intel/iwlwifi/pcie/internal.h |  2 ++
 drivers/net/wireless/intel/iwlwifi/pcie/rx.c   | 10 +-
 drivers/net/wireless/intel/iwlwifi/pcie/trans.c|  9 +
 3 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/drivers/net/wireless/intel/iwlwifi/pcie/internal.h 
b/drivers/net/wireless/intel/iwlwifi/pcie/internal.h
index fa315d84e98e..a1ea9ef97ed9 100644
--- a/drivers/net/wireless/intel/iwlwifi/pcie/internal.h
+++ b/drivers/net/wireless/intel/iwlwifi/pcie/internal.h
@@ -787,6 +787,8 @@ int iwl_pci_fw_enter_d0i3(struct iwl_trans *trans);
 
 void iwl_pcie_enable_rx_wake(struct iwl_trans *trans, bool enable);
 
+void iwl_pcie_rx_allocator_work(struct work_struct *data);
+
 /* common functions that are used by gen2 transport */
 void iwl_pcie_apm_config(struct iwl_trans *trans);
 int iwl_pcie_prepare_card_hw(struct iwl_trans *trans);
diff --git a/drivers/net/wireless/intel/iwlwifi/pcie/rx.c 
b/drivers/net/wireless/intel/iwlwifi/pcie/rx.c
index 351c4423125a..942736d3fa75 100644
--- a/drivers/net/wireless/intel/iwlwifi/pcie/rx.c
+++ b/drivers/net/wireless/intel/iwlwifi/pcie/rx.c
@@ -597,7 +597,7 @@ static void iwl_pcie_rx_allocator_get(struct iwl_trans 
*trans,
rxq->free_count += RX_CLAIM_REQ_ALLOC;
 }
 
-static void iwl_pcie_rx_allocator_work(struct work_struct *data)
+void iwl_pcie_rx_allocator_work(struct work_struct *data)
 {
struct iwl_rb_allocator *rba_p =
container_of(data, struct iwl_rb_allocator, rx_alloc);
@@ -900,10 +900,6 @@ static int _iwl_pcie_rx_init(struct iwl_trans *trans)
return err;
}
def_rxq = trans_pcie->rxq;
-   if (!rba->alloc_wq)
-   rba->alloc_wq = alloc_workqueue("rb_allocator",
-   WQ_HIGHPRI | WQ_UNBOUND, 1);
-   INIT_WORK(>rx_alloc, iwl_pcie_rx_allocator_work);
 
spin_lock(>lock);
atomic_set(>req_pending, 0);
@@ -1017,10 +1013,6 @@ void iwl_pcie_rx_free(struct iwl_trans *trans)
}
 
cancel_work_sync(>rx_alloc);
-   if (rba->alloc_wq) {
-   destroy_workqueue(rba->alloc_wq);
-   rba->alloc_wq = NULL;
-   }
 
iwl_pcie_free_rbs_pool(trans);
 
diff --git a/drivers/net/wireless/intel/iwlwifi/pcie/trans.c 
b/drivers/net/wireless/intel/iwlwifi/pcie/trans.c
index f95eec52508e..3927bbf04f72 100644
--- a/drivers/net/wireless/intel/iwlwifi/pcie/trans.c
+++ b/drivers/net/wireless/intel/iwlwifi/pcie/trans.c
@@ -1786,6 +1786,11 @@ void iwl_trans_pcie_free(struct iwl_trans *trans)
iwl_pcie_tx_free(trans);
iwl_pcie_rx_free(trans);
 
+   if (trans_pcie->rba.alloc_wq) {
+   destroy_workqueue(trans_pcie->rba.alloc_wq);
+   trans_pcie->rba.alloc_wq = NULL;
+   }
+
if (trans_pcie->msix_enabled) {
for (i = 0; i < trans_pcie->alloc_vecs; i++) {
irq_set_affinity_hint(
@@ -3169,6 +3174,10 @@ struct iwl_trans *iwl_trans_pcie_alloc(struct pci_dev 
*pdev,
trans_pcie->inta_mask = CSR_INI_SET_MASK;
 }
 
+   trans_pcie->rba.alloc_wq = alloc_workqueue("rb_allocator",
+  WQ_HIGHPRI | WQ_UNBOUND, 1);
+   INIT_WORK(_pcie->rba.rx_alloc, iwl_pcie_rx_allocator_work);
+
 #ifdef CONFIG_IWLWIFI_PCIE_RTPM
trans->runtime_pm_mode = IWL_PLAT_PM_MODE_D0I3;
 #else
-- 
2.14.1

[PATCH v2] iwlwifi: pcie: move rx workqueue initialization to iwl_trans_pcie_alloc()

[Luca Coelho]

From: Luca Coelho 

Work queues cannot be allocated when a mutex is held because the mutex
may be in use and that would make it sleep.  Doing so generates the
following splat with 4.13+:

[   19.513298] ==
[   19.513429] WARNING: possible circular locking dependency detected
[   19.513557] 4.13.0-rc5+ #6 Not tainted
[   19.513638] --
[   19.513767] cpuhp/0/12 is trying to acquire lock:
[   19.513867]  (>lock){+.+.+.}, at: [] 
thermal_zone_get_temp+0x5b/0xb0
[   19.514047]
[   19.514047] but task is already holding lock:
[   19.514166]  (cpuhp_state){+.+.+.}, at: [] 
cpuhp_thread_fun+0x3a/0x210
[   19.514338]
[   19.514338] which lock already depends on the new lock.

This lock dependency already existed with previous kernel versions,
but it was not detected until commit 49dfe2a67797 ("cpuhotplug: Link
lock stacks for hotplug callbacks") was introduced.

Reported-by: David Weinehall 
Reported-by: Jiri Kosina 
Signed-off-by: Luca Coelho 
---
In v2:
   - updated the commit message to a new version, with a grammar fix
 and the actual commit that exposed the problem;

 drivers/net/wireless/intel/iwlwifi/pcie/internal.h |  2 ++
 drivers/net/wireless/intel/iwlwifi/pcie/rx.c   | 10 +-
 drivers/net/wireless/intel/iwlwifi/pcie/trans.c|  9 +
 3 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/drivers/net/wireless/intel/iwlwifi/pcie/internal.h 
b/drivers/net/wireless/intel/iwlwifi/pcie/internal.h
index fa315d84e98e..a1ea9ef97ed9 100644
--- a/drivers/net/wireless/intel/iwlwifi/pcie/internal.h
+++ b/drivers/net/wireless/intel/iwlwifi/pcie/internal.h
@@ -787,6 +787,8 @@ int iwl_pci_fw_enter_d0i3(struct iwl_trans *trans);
 
 void iwl_pcie_enable_rx_wake(struct iwl_trans *trans, bool enable);
 
+void iwl_pcie_rx_allocator_work(struct work_struct *data);
+
 /* common functions that are used by gen2 transport */
 void iwl_pcie_apm_config(struct iwl_trans *trans);
 int iwl_pcie_prepare_card_hw(struct iwl_trans *trans);
diff --git a/drivers/net/wireless/intel/iwlwifi/pcie/rx.c 
b/drivers/net/wireless/intel/iwlwifi/pcie/rx.c
index 351c4423125a..942736d3fa75 100644
--- a/drivers/net/wireless/intel/iwlwifi/pcie/rx.c
+++ b/drivers/net/wireless/intel/iwlwifi/pcie/rx.c
@@ -597,7 +597,7 @@ static void iwl_pcie_rx_allocator_get(struct iwl_trans 
*trans,
rxq->free_count += RX_CLAIM_REQ_ALLOC;
 }
 
-static void iwl_pcie_rx_allocator_work(struct work_struct *data)
+void iwl_pcie_rx_allocator_work(struct work_struct *data)
 {
struct iwl_rb_allocator *rba_p =
container_of(data, struct iwl_rb_allocator, rx_alloc);
@@ -900,10 +900,6 @@ static int _iwl_pcie_rx_init(struct iwl_trans *trans)
return err;
}
def_rxq = trans_pcie->rxq;
-   if (!rba->alloc_wq)
-   rba->alloc_wq = alloc_workqueue("rb_allocator",
-   WQ_HIGHPRI | WQ_UNBOUND, 1);
-   INIT_WORK(>rx_alloc, iwl_pcie_rx_allocator_work);
 
spin_lock(>lock);
atomic_set(>req_pending, 0);
@@ -1017,10 +1013,6 @@ void iwl_pcie_rx_free(struct iwl_trans *trans)
}
 
cancel_work_sync(>rx_alloc);
-   if (rba->alloc_wq) {
-   destroy_workqueue(rba->alloc_wq);
-   rba->alloc_wq = NULL;
-   }
 
iwl_pcie_free_rbs_pool(trans);
 
diff --git a/drivers/net/wireless/intel/iwlwifi/pcie/trans.c 
b/drivers/net/wireless/intel/iwlwifi/pcie/trans.c
index f95eec52508e..3927bbf04f72 100644
--- a/drivers/net/wireless/intel/iwlwifi/pcie/trans.c
+++ b/drivers/net/wireless/intel/iwlwifi/pcie/trans.c
@@ -1786,6 +1786,11 @@ void iwl_trans_pcie_free(struct iwl_trans *trans)
iwl_pcie_tx_free(trans);
iwl_pcie_rx_free(trans);
 
+   if (trans_pcie->rba.alloc_wq) {
+   destroy_workqueue(trans_pcie->rba.alloc_wq);
+   trans_pcie->rba.alloc_wq = NULL;
+   }
+
if (trans_pcie->msix_enabled) {
for (i = 0; i < trans_pcie->alloc_vecs; i++) {
irq_set_affinity_hint(
@@ -3169,6 +3174,10 @@ struct iwl_trans *iwl_trans_pcie_alloc(struct pci_dev 
*pdev,
trans_pcie->inta_mask = CSR_INI_SET_MASK;
 }
 
+   trans_pcie->rba.alloc_wq = alloc_workqueue("rb_allocator",
+  WQ_HIGHPRI | WQ_UNBOUND, 1);
+   INIT_WORK(_pcie->rba.rx_alloc, iwl_pcie_rx_allocator_work);
+
 #ifdef CONFIG_IWLWIFI_PCIE_RTPM
trans->runtime_pm_mode = IWL_PLAT_PM_MODE_D0I3;
 #else
-- 
2.14.1

[PATCH] iwlwifi: pcie: move rx workqueue initialization to iwl_trans_pcie_alloc()

[Luca Coelho]

From: Luca Coelho <luciano.coe...@intel.com>

Work queues cannot be allocated in when a mutex is held because the
mutex may be in use and that would make it sleep.  Doing so generates
the following splat with 4.13+:

[   19.513298] ==
[   19.513429] WARNING: possible circular locking dependency detected
[   19.513557] 4.13.0-rc5+ #6 Not tainted
[   19.513638] --
[   19.513767] cpuhp/0/12 is trying to acquire lock:
[   19.513867]  (>lock){+.+.+.}, at: [] 
thermal_zone_get_temp+0x5b/0xb0
[   19.514047]
[   19.514047] but task is already holding lock:
[   19.514166]  (cpuhp_state){+.+.+.}, at: [] 
cpuhp_thread_fun+0x3a/0x210
[   19.514338]
[   19.514338] which lock already depends on the new lock.

This lock dependency already existed with previous kernel versions,
but it was not detected until commit ... was introduced.

Reported-by: David Weinehall <david.weineh...@intel.com>
Reported-by: Jiri Kosina <ji...@kernel.org>
Signed-off-by: Luca Coelho <luciano.coe...@intel.com>
---
 drivers/net/wireless/intel/iwlwifi/pcie/internal.h |  2 ++
 drivers/net/wireless/intel/iwlwifi/pcie/rx.c   | 10 +-
 drivers/net/wireless/intel/iwlwifi/pcie/trans.c|  9 +
 3 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/drivers/net/wireless/intel/iwlwifi/pcie/internal.h 
b/drivers/net/wireless/intel/iwlwifi/pcie/internal.h
index fa315d84e98e..a1ea9ef97ed9 100644
--- a/drivers/net/wireless/intel/iwlwifi/pcie/internal.h
+++ b/drivers/net/wireless/intel/iwlwifi/pcie/internal.h
@@ -787,6 +787,8 @@ int iwl_pci_fw_enter_d0i3(struct iwl_trans *trans);
 
 void iwl_pcie_enable_rx_wake(struct iwl_trans *trans, bool enable);
 
+void iwl_pcie_rx_allocator_work(struct work_struct *data);
+
 /* common functions that are used by gen2 transport */
 void iwl_pcie_apm_config(struct iwl_trans *trans);
 int iwl_pcie_prepare_card_hw(struct iwl_trans *trans);
diff --git a/drivers/net/wireless/intel/iwlwifi/pcie/rx.c 
b/drivers/net/wireless/intel/iwlwifi/pcie/rx.c
index 351c4423125a..942736d3fa75 100644
--- a/drivers/net/wireless/intel/iwlwifi/pcie/rx.c
+++ b/drivers/net/wireless/intel/iwlwifi/pcie/rx.c
@@ -597,7 +597,7 @@ static void iwl_pcie_rx_allocator_get(struct iwl_trans 
*trans,
rxq->free_count += RX_CLAIM_REQ_ALLOC;
 }
 
-static void iwl_pcie_rx_allocator_work(struct work_struct *data)
+void iwl_pcie_rx_allocator_work(struct work_struct *data)
 {
struct iwl_rb_allocator *rba_p =
container_of(data, struct iwl_rb_allocator, rx_alloc);
@@ -900,10 +900,6 @@ static int _iwl_pcie_rx_init(struct iwl_trans *trans)
return err;
}
def_rxq = trans_pcie->rxq;
-   if (!rba->alloc_wq)
-   rba->alloc_wq = alloc_workqueue("rb_allocator",
-   WQ_HIGHPRI | WQ_UNBOUND, 1);
-   INIT_WORK(>rx_alloc, iwl_pcie_rx_allocator_work);
 
spin_lock(>lock);
atomic_set(>req_pending, 0);
@@ -1017,10 +1013,6 @@ void iwl_pcie_rx_free(struct iwl_trans *trans)
}
 
cancel_work_sync(>rx_alloc);
-   if (rba->alloc_wq) {
-   destroy_workqueue(rba->alloc_wq);
-   rba->alloc_wq = NULL;
-   }
 
iwl_pcie_free_rbs_pool(trans);
 
diff --git a/drivers/net/wireless/intel/iwlwifi/pcie/trans.c 
b/drivers/net/wireless/intel/iwlwifi/pcie/trans.c
index f95eec52508e..3927bbf04f72 100644
--- a/drivers/net/wireless/intel/iwlwifi/pcie/trans.c
+++ b/drivers/net/wireless/intel/iwlwifi/pcie/trans.c
@@ -1786,6 +1786,11 @@ void iwl_trans_pcie_free(struct iwl_trans *trans)
iwl_pcie_tx_free(trans);
iwl_pcie_rx_free(trans);
 
+   if (trans_pcie->rba.alloc_wq) {
+   destroy_workqueue(trans_pcie->rba.alloc_wq);
+   trans_pcie->rba.alloc_wq = NULL;
+   }
+
if (trans_pcie->msix_enabled) {
for (i = 0; i < trans_pcie->alloc_vecs; i++) {
irq_set_affinity_hint(
@@ -3169,6 +3174,10 @@ struct iwl_trans *iwl_trans_pcie_alloc(struct pci_dev 
*pdev,
trans_pcie->inta_mask = CSR_INI_SET_MASK;
 }
 
+   trans_pcie->rba.alloc_wq = alloc_workqueue("rb_allocator",
+  WQ_HIGHPRI | WQ_UNBOUND, 1);
+   INIT_WORK(_pcie->rba.rx_alloc, iwl_pcie_rx_allocator_work);
+
 #ifdef CONFIG_IWLWIFI_PCIE_RTPM
trans->runtime_pm_mode = IWL_PLAT_PM_MODE_D0I3;
 #else
-- 
2.14.1

[PATCH] iwlwifi: pcie: move rx workqueue initialization to iwl_trans_pcie_alloc()

[Luca Coelho]

From: Luca Coelho 

Work queues cannot be allocated in when a mutex is held because the
mutex may be in use and that would make it sleep.  Doing so generates
the following splat with 4.13+:

[   19.513298] ==
[   19.513429] WARNING: possible circular locking dependency detected
[   19.513557] 4.13.0-rc5+ #6 Not tainted
[   19.513638] --
[   19.513767] cpuhp/0/12 is trying to acquire lock:
[   19.513867]  (>lock){+.+.+.}, at: [] 
thermal_zone_get_temp+0x5b/0xb0
[   19.514047]
[   19.514047] but task is already holding lock:
[   19.514166]  (cpuhp_state){+.+.+.}, at: [] 
cpuhp_thread_fun+0x3a/0x210
[   19.514338]
[   19.514338] which lock already depends on the new lock.

This lock dependency already existed with previous kernel versions,
but it was not detected until commit ... was introduced.

Reported-by: David Weinehall 
Reported-by: Jiri Kosina 
Signed-off-by: Luca Coelho 
---
 drivers/net/wireless/intel/iwlwifi/pcie/internal.h |  2 ++
 drivers/net/wireless/intel/iwlwifi/pcie/rx.c   | 10 +-
 drivers/net/wireless/intel/iwlwifi/pcie/trans.c|  9 +
 3 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/drivers/net/wireless/intel/iwlwifi/pcie/internal.h 
b/drivers/net/wireless/intel/iwlwifi/pcie/internal.h
index fa315d84e98e..a1ea9ef97ed9 100644
--- a/drivers/net/wireless/intel/iwlwifi/pcie/internal.h
+++ b/drivers/net/wireless/intel/iwlwifi/pcie/internal.h
@@ -787,6 +787,8 @@ int iwl_pci_fw_enter_d0i3(struct iwl_trans *trans);
 
 void iwl_pcie_enable_rx_wake(struct iwl_trans *trans, bool enable);
 
+void iwl_pcie_rx_allocator_work(struct work_struct *data);
+
 /* common functions that are used by gen2 transport */
 void iwl_pcie_apm_config(struct iwl_trans *trans);
 int iwl_pcie_prepare_card_hw(struct iwl_trans *trans);
diff --git a/drivers/net/wireless/intel/iwlwifi/pcie/rx.c 
b/drivers/net/wireless/intel/iwlwifi/pcie/rx.c
index 351c4423125a..942736d3fa75 100644
--- a/drivers/net/wireless/intel/iwlwifi/pcie/rx.c
+++ b/drivers/net/wireless/intel/iwlwifi/pcie/rx.c
@@ -597,7 +597,7 @@ static void iwl_pcie_rx_allocator_get(struct iwl_trans 
*trans,
rxq->free_count += RX_CLAIM_REQ_ALLOC;
 }
 
-static void iwl_pcie_rx_allocator_work(struct work_struct *data)
+void iwl_pcie_rx_allocator_work(struct work_struct *data)
 {
struct iwl_rb_allocator *rba_p =
container_of(data, struct iwl_rb_allocator, rx_alloc);
@@ -900,10 +900,6 @@ static int _iwl_pcie_rx_init(struct iwl_trans *trans)
return err;
}
def_rxq = trans_pcie->rxq;
-   if (!rba->alloc_wq)
-   rba->alloc_wq = alloc_workqueue("rb_allocator",
-   WQ_HIGHPRI | WQ_UNBOUND, 1);
-   INIT_WORK(>rx_alloc, iwl_pcie_rx_allocator_work);
 
spin_lock(>lock);
atomic_set(>req_pending, 0);
@@ -1017,10 +1013,6 @@ void iwl_pcie_rx_free(struct iwl_trans *trans)
}
 
cancel_work_sync(>rx_alloc);
-   if (rba->alloc_wq) {
-   destroy_workqueue(rba->alloc_wq);
-   rba->alloc_wq = NULL;
-   }
 
iwl_pcie_free_rbs_pool(trans);
 
diff --git a/drivers/net/wireless/intel/iwlwifi/pcie/trans.c 
b/drivers/net/wireless/intel/iwlwifi/pcie/trans.c
index f95eec52508e..3927bbf04f72 100644
--- a/drivers/net/wireless/intel/iwlwifi/pcie/trans.c
+++ b/drivers/net/wireless/intel/iwlwifi/pcie/trans.c
@@ -1786,6 +1786,11 @@ void iwl_trans_pcie_free(struct iwl_trans *trans)
iwl_pcie_tx_free(trans);
iwl_pcie_rx_free(trans);
 
+   if (trans_pcie->rba.alloc_wq) {
+   destroy_workqueue(trans_pcie->rba.alloc_wq);
+   trans_pcie->rba.alloc_wq = NULL;
+   }
+
if (trans_pcie->msix_enabled) {
for (i = 0; i < trans_pcie->alloc_vecs; i++) {
irq_set_affinity_hint(
@@ -3169,6 +3174,10 @@ struct iwl_trans *iwl_trans_pcie_alloc(struct pci_dev 
*pdev,
trans_pcie->inta_mask = CSR_INI_SET_MASK;
 }
 
+   trans_pcie->rba.alloc_wq = alloc_workqueue("rb_allocator",
+  WQ_HIGHPRI | WQ_UNBOUND, 1);
+   INIT_WORK(_pcie->rba.rx_alloc, iwl_pcie_rx_allocator_work);
+
 #ifdef CONFIG_IWLWIFI_PCIE_RTPM
trans->runtime_pm_mode = IWL_PLAT_PM_MODE_D0I3;
 #else
-- 
2.14.1

Re: [PATCH] iwlwifi: Demote messages about fw flags size to info

[Luca Coelho]

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] iwlwifi: Demote messages about fw flags size to info

[Luca Coelho]

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()'

[Luca Coelho]

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 v2] iwlwifi: mvm: Fix a memory leak in an error handling path in 'iwl_mvm_sar_get_wgds_table()'

[Luca Coelho]

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

[Luca Coelho]

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 <bhumi...@gmail.com>
> > ---
> 
> 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: add const to thermal_cooling_device_ops structure

[Luca Coelho]

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

[Luca Coelho]

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: fix iwl_mvm_sar_find_wifi_pkg corner case

[Luca Coelho]

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

[Luca Coelho]

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] iwlwifi: mvm: add const to thermal_cooling_device_ops structure

[Luca Coelho]

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

[Luca Coelho]

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: [PATCH] net: wireless: intel: iwlwifi: dvm: remove unused defines

[Luca Coelho]

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: [PATCH v6 4/5] iwlwifi: convert to use driver data API

[Luca Coelho]

On Fri, 2017-04-28 at 02:56 +0200, Luis R. Rodriguez wrote:
> On Mon, Apr 10, 2017 at 04:19:12PM +0300, Luca Coelho wrote:
> > On Wed, 2017-03-29 at 20:25 -0700, Luis R. Rodriguez wrote:
> > > The driver data API provides support for looking for firmware
> > > from a specific set of API ranges, so just use that. Since we
> > > free the firmware on the callback immediately after consuming it,
> > > this also takes avantage of that feature.
> > > 
> > > Signed-off-by: Luis R. Rodriguez <mcg...@kernel.org>
> > > ---
> > 
> > Looks fine, with one nitpick.
> > 
> > 
> > >  drivers/net/wireless/intel/iwlwifi/iwl-drv.c | 67 
> > > ++--
> > >  1 file changed, 23 insertions(+), 44 deletions(-)
> > > 
> > > diff --git a/drivers/net/wireless/intel/iwlwifi/iwl-drv.c 
> > > b/drivers/net/wireless/intel/iwlwifi/iwl-drv.c
> > > index be466a074c1d..b6643aa5b344 100644
> > > --- a/drivers/net/wireless/intel/iwlwifi/iwl-drv.c
> > > +++ b/drivers/net/wireless/intel/iwlwifi/iwl-drv.c
> > 
> > [...]
> > 
> > > @@ -1541,11 +1522,9 @@ struct iwl_drv *iwl_drv_start(struct iwl_trans 
> > > *trans)
> > >   }
> > >  #endif
> > >  
> > > - ret = iwl_request_firmware(drv, true);
> > > - if (ret) {
> > > - IWL_ERR(trans, "Couldn't request the fw\n");
> > > + ret = iwl_request_firmware(drv);
> > > + if (ret)
> > >   goto err_fw;
> > > - }
> > 
> > Why remove the error message here?
> 
> The driver data API now has enough semantics even for async requests so that
> an error is either always issued or supressed (optional is true), driver 
> errors
> then are superfluous on error now.
> 
> Let me know if this is OK.

Yeah, that's okay.  We can always add something back if we think it's
needed.

--
Cheers,
Luca.

Re: [PATCH v6 4/5] iwlwifi: convert to use driver data API

[Luca Coelho]

On Fri, 2017-04-28 at 02:56 +0200, Luis R. Rodriguez wrote:
> On Mon, Apr 10, 2017 at 04:19:12PM +0300, Luca Coelho wrote:
> > On Wed, 2017-03-29 at 20:25 -0700, Luis R. Rodriguez wrote:
> > > The driver data API provides support for looking for firmware
> > > from a specific set of API ranges, so just use that. Since we
> > > free the firmware on the callback immediately after consuming it,
> > > this also takes avantage of that feature.
> > > 
> > > Signed-off-by: Luis R. Rodriguez 
> > > ---
> > 
> > Looks fine, with one nitpick.
> > 
> > 
> > >  drivers/net/wireless/intel/iwlwifi/iwl-drv.c | 67 
> > > ++--
> > >  1 file changed, 23 insertions(+), 44 deletions(-)
> > > 
> > > diff --git a/drivers/net/wireless/intel/iwlwifi/iwl-drv.c 
> > > b/drivers/net/wireless/intel/iwlwifi/iwl-drv.c
> > > index be466a074c1d..b6643aa5b344 100644
> > > --- a/drivers/net/wireless/intel/iwlwifi/iwl-drv.c
> > > +++ b/drivers/net/wireless/intel/iwlwifi/iwl-drv.c
> > 
> > [...]
> > 
> > > @@ -1541,11 +1522,9 @@ struct iwl_drv *iwl_drv_start(struct iwl_trans 
> > > *trans)
> > >   }
> > >  #endif
> > >  
> > > - ret = iwl_request_firmware(drv, true);
> > > - if (ret) {
> > > - IWL_ERR(trans, "Couldn't request the fw\n");
> > > + ret = iwl_request_firmware(drv);
> > > + if (ret)
> > >   goto err_fw;
> > > - }
> > 
> > Why remove the error message here?
> 
> The driver data API now has enough semantics even for async requests so that
> an error is either always issued or supressed (optional is true), driver 
> errors
> then are superfluous on error now.
> 
> Let me know if this is OK.

Yeah, that's okay.  We can always add something back if we think it's
needed.

--
Cheers,
Luca.

Re: [PATCH v6 2/5] firmware: add extensible driver data API

[Luca Coelho]

On Thu, 2017-04-27 at 05:16 +0200, Luis R. Rodriguez wrote:
> > > +int driver_data_request_sync(const char *name,
> > > +    const struct driver_data_req_params *req_params,
> > > +    struct device *device)
> > > +{
> > > +   const struct firmware *driver_data;
> > > +   const struct driver_data_reqs *sync_reqs;
> > > +   struct driver_data_params params = {
> > > +   .req_params = *req_params,
> > > +   };
> > > +   int ret;
> > > +
> > > +   if (!device || !req_params || !name || name[0] == '\0')
> > > +   return -EINVAL;
> > > +
> > > +   if (req_params->sync_reqs.mode != DRIVER_DATA_SYNC)
> > > +   return -EINVAL;
> > 
> > Why do you need to check this here? If the caller is calling _sync(),
> > it's because that's what it needs.  This mode value here seems
> > redundant.
> 
> Because we allow one data structure to be used for the specified requirements
> and technically someone can make a mistake and use the wrong macros to set up
> the data structure. This ensures users don't async macros to set up the
> parameters and then use the sync call. Eventually the underlying
> firmware_class.c code does its own conditional checks on this as well.

If this could only happen in a programming error, maybe it's worth a
WARN() then, to make it easier to spot?


> > OTOH, if you do have a reason for this value, then you could use
> > driver_data_request_sync() in this if.
> 
> You mean to allow *one* API call for all. Sure, that's possible, but I think
> its nicer to at least expose async/sync mechanisms on the caller side. The
> sync/async differences seem like a reasonable enough place to split the API.

I don't remember the details of this anymore, but doesn't the
driver_data_sync() function does exactly the same check? I meant that
you could do this:

if(WARN_ON_ONCE(!driver_data_request_sync()))
return -EINVAL;


And yes, I think either a single API call for all or not having the mode
in the struct would be cleaner.  I think there are better ways to
prevent coding errors (since this should only happen if the user code is
implemented incorrectly).

--
Cheers,
Luca.

Re: [PATCH v6 2/5] firmware: add extensible driver data API

[Luca Coelho]

On Thu, 2017-04-27 at 05:16 +0200, Luis R. Rodriguez wrote:
> > > +int driver_data_request_sync(const char *name,
> > > +    const struct driver_data_req_params *req_params,
> > > +    struct device *device)
> > > +{
> > > +   const struct firmware *driver_data;
> > > +   const struct driver_data_reqs *sync_reqs;
> > > +   struct driver_data_params params = {
> > > +   .req_params = *req_params,
> > > +   };
> > > +   int ret;
> > > +
> > > +   if (!device || !req_params || !name || name[0] == '\0')
> > > +   return -EINVAL;
> > > +
> > > +   if (req_params->sync_reqs.mode != DRIVER_DATA_SYNC)
> > > +   return -EINVAL;
> > 
> > Why do you need to check this here? If the caller is calling _sync(),
> > it's because that's what it needs.  This mode value here seems
> > redundant.
> 
> Because we allow one data structure to be used for the specified requirements
> and technically someone can make a mistake and use the wrong macros to set up
> the data structure. This ensures users don't async macros to set up the
> parameters and then use the sync call. Eventually the underlying
> firmware_class.c code does its own conditional checks on this as well.

If this could only happen in a programming error, maybe it's worth a
WARN() then, to make it easier to spot?


> > OTOH, if you do have a reason for this value, then you could use
> > driver_data_request_sync() in this if.
> 
> You mean to allow *one* API call for all. Sure, that's possible, but I think
> its nicer to at least expose async/sync mechanisms on the caller side. The
> sync/async differences seem like a reasonable enough place to split the API.

I don't remember the details of this anymore, but doesn't the
driver_data_sync() function does exactly the same check? I meant that
you could do this:

if(WARN_ON_ONCE(!driver_data_request_sync()))
return -EINVAL;


And yes, I think either a single API call for all or not having the mode
in the struct would be cleaner.  I think there are better ways to
prevent coding errors (since this should only happen if the user code is
implemented incorrectly).

--
Cheers,
Luca.

Re: [PATCH v6 2/5] firmware: add extensible driver data API

[Luca Coelho]

On Thu, 2017-04-27 at 05:16 +0200, Luis R. Rodriguez wrote:
> > > @@ -1460,6 +1471,128 @@ void release_firmware(const struct firmware *fw)
> > >   }
> > >   EXPORT_SYMBOL(release_firmware);
> > >   
> > > +static int _driver_data_request_api(struct driver_data_params *params,
> > > +   struct device *device,
> > > +   const char *name)
> > > +{
> > > +   struct driver_data_priv_params *priv_params = >priv_params;
> > > +   const struct driver_data_req_params *req_params = >req_params;
> > > +   int ret;
> > > +   char *try_name;
> > > +   u8 api_max;
> > > +
> > > +   if (priv_params->retry_api) {
> > > +   if (!priv_params->api)
> > > +   return -ENOENT;
> > > +   api_max = priv_params->api - 1;
> > > +   } else
> > > +   api_max = req_params->api_max;
> > 
> > Braces.
> 
> Not sure what you mean here, the else is a 1 liner so I skip the brace.

It's really a nitpick, but the CodingStyle[1] says:

"Do not unnecessarily use braces where a single statement will do.
[...]
This does not apply if only one branch of a conditional statement is a
single statement; in the latter case use braces in both branches:

if (condition) {
do_this();
do_that();
} else {
otherwise();
}"

[1] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst#n175

--
Cheers,
Luca.

Re: [PATCH v6 2/5] firmware: add extensible driver data API

[Luca Coelho]

On Thu, 2017-04-27 at 05:16 +0200, Luis R. Rodriguez wrote:
> > > @@ -1460,6 +1471,128 @@ void release_firmware(const struct firmware *fw)
> > >   }
> > >   EXPORT_SYMBOL(release_firmware);
> > >   
> > > +static int _driver_data_request_api(struct driver_data_params *params,
> > > +   struct device *device,
> > > +   const char *name)
> > > +{
> > > +   struct driver_data_priv_params *priv_params = >priv_params;
> > > +   const struct driver_data_req_params *req_params = >req_params;
> > > +   int ret;
> > > +   char *try_name;
> > > +   u8 api_max;
> > > +
> > > +   if (priv_params->retry_api) {
> > > +   if (!priv_params->api)
> > > +   return -ENOENT;
> > > +   api_max = priv_params->api - 1;
> > > +   } else
> > > +   api_max = req_params->api_max;
> > 
> > Braces.
> 
> Not sure what you mean here, the else is a 1 liner so I skip the brace.

It's really a nitpick, but the CodingStyle[1] says:

"Do not unnecessarily use braces where a single statement will do.
[...]
This does not apply if only one branch of a conditional statement is a
single statement; in the latter case use braces in both branches:

if (condition) {
do_this();
do_that();
} else {
otherwise();
}"

[1] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst#n175

--
Cheers,
Luca.

Re: [PATCH v6 4/5] iwlwifi: convert to use driver data API

[Luca Coelho]

On Wed, 2017-03-29 at 20:25 -0700, Luis R. Rodriguez wrote:
> The driver data API provides support for looking for firmware
> from a specific set of API ranges, so just use that. Since we
> free the firmware on the callback immediately after consuming it,
> this also takes avantage of that feature.
> 
> Signed-off-by: Luis R. Rodriguez 
> ---

Looks find, with one nitpick.


>  drivers/net/wireless/intel/iwlwifi/iwl-drv.c | 67 
> ++--
>  1 file changed, 23 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/net/wireless/intel/iwlwifi/iwl-drv.c 
> b/drivers/net/wireless/intel/iwlwifi/iwl-drv.c
> index be466a074c1d..b6643aa5b344 100644
> --- a/drivers/net/wireless/intel/iwlwifi/iwl-drv.c
> +++ b/drivers/net/wireless/intel/iwlwifi/iwl-drv.c

[...]

> @@ -1541,11 +1522,9 @@ struct iwl_drv *iwl_drv_start(struct iwl_trans *trans)
>   }
>  #endif
>  
> - ret = iwl_request_firmware(drv, true);
> - if (ret) {
> - IWL_ERR(trans, "Couldn't request the fw\n");
> + ret = iwl_request_firmware(drv);
> + if (ret)
>   goto err_fw;
> - }

Why remove the error message here?

--
Cheers,
Luca.

Re: [PATCH v6 4/5] iwlwifi: convert to use driver data API

[Luca Coelho]

On Wed, 2017-03-29 at 20:25 -0700, Luis R. Rodriguez wrote:
> The driver data API provides support for looking for firmware
> from a specific set of API ranges, so just use that. Since we
> free the firmware on the callback immediately after consuming it,
> this also takes avantage of that feature.
> 
> Signed-off-by: Luis R. Rodriguez 
> ---

Looks find, with one nitpick.


>  drivers/net/wireless/intel/iwlwifi/iwl-drv.c | 67 
> ++--
>  1 file changed, 23 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/net/wireless/intel/iwlwifi/iwl-drv.c 
> b/drivers/net/wireless/intel/iwlwifi/iwl-drv.c
> index be466a074c1d..b6643aa5b344 100644
> --- a/drivers/net/wireless/intel/iwlwifi/iwl-drv.c
> +++ b/drivers/net/wireless/intel/iwlwifi/iwl-drv.c

[...]

> @@ -1541,11 +1522,9 @@ struct iwl_drv *iwl_drv_start(struct iwl_trans *trans)
>   }
>  #endif
>  
> - ret = iwl_request_firmware(drv, true);
> - if (ret) {
> - IWL_ERR(trans, "Couldn't request the fw\n");
> + ret = iwl_request_firmware(drv);
> + if (ret)
>   goto err_fw;
> - }

Why remove the error message here?

--
Cheers,
Luca.

Re: [PATCH v6 1/5] firmware: add extensible driver data params

[Luca Coelho]

On Wed, 2017-03-29 at 20:25 -0700, Luis R. Rodriguez wrote:
> As the firmware API evolves we keep extending functions with more arguments.
> Stop this nonsense by proving an extensible data structure which can be used
> to represent both user parameters and private internal parameters.
> 
> We introduce 3 data structures:
> 
>   o struct driver_data_req_params  - used for user specified parameters
>   o struct driver_data_priv_params - used for internal use
>   o struct driver_data_params - stiches both of the the above together,
>   also only for internal use
> 
> This starts off by just making the existing APIs use the new data
> structures, it will make subsequent changes easier to review which will
> be adding new flexible APIs.
> 
> This commit should introduces no functional changes (TM).
> 
> Signed-off-by: Luis R. Rodriguez 
> ---

Looks fine with a few nitpicks.


> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index 54fc4c42f126..f702566554e1 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c

[...]

> @@ -40,6 +41,77 @@ MODULE_AUTHOR("Manuel Estrada Sainz");
>  MODULE_DESCRIPTION("Multi purpose firmware loading support");
>  MODULE_LICENSE("GPL");
>  
> +struct driver_data_priv_params {
> + bool use_fallback;
> + bool use_fallback_uevent;
> + bool no_cache;
> + void *alloc_buf;
> + size_t alloc_buf_size;
> +};
> +
> +struct driver_data_params {
> + const struct firmware *driver_data;
> + const struct driver_data_req_params req_params;
> + struct driver_data_priv_params priv_params;
> +};
> +
> +/*
> + * These are kept to remain backward compatible with old behaviour. Do not
> + * modify them unless you know what you are doing. These are to be used only
> + * by the old API, so:
> + *
> + * Old sync APIs:
> + *   o request_firmware():   __DATA_REQ_FIRMWARE()
> + *   o request_firmware_direct():__DATA_REQ_FIRMWARE_DIRECT()
> + *   o request_firmware_into_buf():  __DATA_REQ_FIRMWARE_BUF()
> + *
> + * Old async API:
> + *   o request_firmware_nowait():__DATA_REQ_FIRMWARE_NOWAIT()
> + */
> +#define __DATA_REQ_FIRMWARE()
> \
> + .priv_params = {\
> + .use_fallback = !!FW_OPT_FALLBACK,  \

use_fallback is a boolean, so you don't need the double negation here.


[...]

> @@ -1332,12 +1433,15 @@ request_firmware_into_buf(const struct firmware 
> **firmware_p, const char *name,
> struct device *device, void *buf, size_t size)
>  {
>   int ret;
> + struct driver_data_params data_params = {
> + __DATA_REQ_FIRMWARE_BUF(buf, size),
> + };
>  
>   __module_get(THIS_MODULE);
> - ret = _request_firmware(firmware_p, name, device, buf, size,
> - FW_OPT_UEVENT | FW_OPT_FALLBACK |
> - FW_OPT_NOCACHE);
> + ret = _request_firmware(firmware_p, name, _params, device);
>   module_put(THIS_MODULE);
> +
> +

Double empty-lines here.


>   return ret;
>  }
>  EXPORT_SYMBOL(request_firmware_into_buf);
> 

[...]

> diff --git a/include/linux/driver_data.h b/include/linux/driver_data.h
> new file mode 100644
> index ..c3d3a4129838
> --- /dev/null
> +++ b/include/linux/driver_data.h
> @@ -0,0 +1,88 @@
> +#ifndef _LINUX_DRIVER_DATA_H
> +#define _LINUX_DRIVER_DATA_H
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +/*
> + * Driver Data internals
> + *
> + * Copyright (C) 2017 Luis R. Rodriguez 
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of copyleft-next (version 0.3.1 or later) as published
> + * at http://copyleft-next.org/.
> + */
> +
> +/**
> + * enum driver_data_mode - driver data mode of operation
> + *
> + * DRIVER_DATA_SYNC: your call to request driver data is synchronous. We will
> + *   look for the driver data file you have requested immediatley.
> + * DRIVER_DATA_ASYNC: your call to request driver data is asynchronous. We 
> will

It should be @DRIVER_DATA_SYNC and @DRIVER_DATA_ASYNC here.

--
Cheers,
Luca.

Re: [PATCH v6 1/5] firmware: add extensible driver data params

[Luca Coelho]

On Wed, 2017-03-29 at 20:25 -0700, Luis R. Rodriguez wrote:
> As the firmware API evolves we keep extending functions with more arguments.
> Stop this nonsense by proving an extensible data structure which can be used
> to represent both user parameters and private internal parameters.
> 
> We introduce 3 data structures:
> 
>   o struct driver_data_req_params  - used for user specified parameters
>   o struct driver_data_priv_params - used for internal use
>   o struct driver_data_params - stiches both of the the above together,
>   also only for internal use
> 
> This starts off by just making the existing APIs use the new data
> structures, it will make subsequent changes easier to review which will
> be adding new flexible APIs.
> 
> This commit should introduces no functional changes (TM).
> 
> Signed-off-by: Luis R. Rodriguez 
> ---

Looks fine with a few nitpicks.


> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index 54fc4c42f126..f702566554e1 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c

[...]

> @@ -40,6 +41,77 @@ MODULE_AUTHOR("Manuel Estrada Sainz");
>  MODULE_DESCRIPTION("Multi purpose firmware loading support");
>  MODULE_LICENSE("GPL");
>  
> +struct driver_data_priv_params {
> + bool use_fallback;
> + bool use_fallback_uevent;
> + bool no_cache;
> + void *alloc_buf;
> + size_t alloc_buf_size;
> +};
> +
> +struct driver_data_params {
> + const struct firmware *driver_data;
> + const struct driver_data_req_params req_params;
> + struct driver_data_priv_params priv_params;
> +};
> +
> +/*
> + * These are kept to remain backward compatible with old behaviour. Do not
> + * modify them unless you know what you are doing. These are to be used only
> + * by the old API, so:
> + *
> + * Old sync APIs:
> + *   o request_firmware():   __DATA_REQ_FIRMWARE()
> + *   o request_firmware_direct():__DATA_REQ_FIRMWARE_DIRECT()
> + *   o request_firmware_into_buf():  __DATA_REQ_FIRMWARE_BUF()
> + *
> + * Old async API:
> + *   o request_firmware_nowait():__DATA_REQ_FIRMWARE_NOWAIT()
> + */
> +#define __DATA_REQ_FIRMWARE()
> \
> + .priv_params = {\
> + .use_fallback = !!FW_OPT_FALLBACK,  \

use_fallback is a boolean, so you don't need the double negation here.


[...]

> @@ -1332,12 +1433,15 @@ request_firmware_into_buf(const struct firmware 
> **firmware_p, const char *name,
> struct device *device, void *buf, size_t size)
>  {
>   int ret;
> + struct driver_data_params data_params = {
> + __DATA_REQ_FIRMWARE_BUF(buf, size),
> + };
>  
>   __module_get(THIS_MODULE);
> - ret = _request_firmware(firmware_p, name, device, buf, size,
> - FW_OPT_UEVENT | FW_OPT_FALLBACK |
> - FW_OPT_NOCACHE);
> + ret = _request_firmware(firmware_p, name, _params, device);
>   module_put(THIS_MODULE);
> +
> +

Double empty-lines here.


>   return ret;
>  }
>  EXPORT_SYMBOL(request_firmware_into_buf);
> 

[...]

> diff --git a/include/linux/driver_data.h b/include/linux/driver_data.h
> new file mode 100644
> index ..c3d3a4129838
> --- /dev/null
> +++ b/include/linux/driver_data.h
> @@ -0,0 +1,88 @@
> +#ifndef _LINUX_DRIVER_DATA_H
> +#define _LINUX_DRIVER_DATA_H
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +/*
> + * Driver Data internals
> + *
> + * Copyright (C) 2017 Luis R. Rodriguez 
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of copyleft-next (version 0.3.1 or later) as published
> + * at http://copyleft-next.org/.
> + */
> +
> +/**
> + * enum driver_data_mode - driver data mode of operation
> + *
> + * DRIVER_DATA_SYNC: your call to request driver data is synchronous. We will
> + *   look for the driver data file you have requested immediatley.
> + * DRIVER_DATA_ASYNC: your call to request driver data is asynchronous. We 
> will

It should be @DRIVER_DATA_SYNC and @DRIVER_DATA_ASYNC here.

--
Cheers,
Luca.

Re: Issues with FW with Intel 720 wifi card

[Luca Coelho]

Hi Bharat,


On Fri, 2017-02-03 at 16:27 +, Bharat Kumar Gogada wrote:
> Hi,
> 
> Im using linux 4.6 kernel, I get following error when I do interface up.
> 
> Here is the boot log.
> [4.407681] iwlwifi :01:00.0: loaded firmware version 16.242414.0 
> op_mode iwlmvm
> [4.407742] iwlwifi :01:00.0: Detected Intel(R) Wireless N 7260, 
> REV=0x144
> [4.407831] iwlwifi :01:00.0: L1 Disabled - LTR Disabled
> [4.408116] iwlwifi :01:00.0: L1 Disabled - LTR Disabled
> 
> When I do interface up I get following :
> [9.41] iwlwifi :01:00.0: Failed to load firmware chunk!
> [9.425947] iwlwifi :01:00.0: Could not load the [0] uCode section
> [9.432472] iwlwifi :01:00.0: Failed to start INIT ucode: -110
> [9.438899] iwlwifi :01:00.0: Failed to run INIT ucode: -110
> [9.444858] iwlwifi :01:00.0: L1 Disabled - LTR Disabled

It's hard to say why this is happening.


> I downloaded firmware from here
> https://wireless.wiki.kernel.org/en/users/Drivers/iwlwifi

Why did you have to download the firmware? Doesn't your distro include
the binaries from linux-firmware.git?

The firmware you are using is version 16, but that kernel already
supports version -17.  I recommend you try to take the latest firmware
for the 7260 NIC from our copy of linux-firmware.git[2].

If this doesn't help, the best thing to do would be to report this as a
bug in bugzilla.kernel.org following the instructions in our wiki[1]. 
This way we can help you more easily.


[1] https://wireless.wiki.kernel.org/en/users/drivers/iwlwifi/debugging
[2] 
http://git.kernel.org/cgit/linux/kernel/git/iwlwifi/linux-firmware.git/plain/iwlwifi-7260-17.ucode

--
Cheers,
Luca.

Re: Issues with FW with Intel 720 wifi card

[Luca Coelho]

Hi Bharat,


On Fri, 2017-02-03 at 16:27 +, Bharat Kumar Gogada wrote:
> Hi,
> 
> Im using linux 4.6 kernel, I get following error when I do interface up.
> 
> Here is the boot log.
> [4.407681] iwlwifi :01:00.0: loaded firmware version 16.242414.0 
> op_mode iwlmvm
> [4.407742] iwlwifi :01:00.0: Detected Intel(R) Wireless N 7260, 
> REV=0x144
> [4.407831] iwlwifi :01:00.0: L1 Disabled - LTR Disabled
> [4.408116] iwlwifi :01:00.0: L1 Disabled - LTR Disabled
> 
> When I do interface up I get following :
> [9.41] iwlwifi :01:00.0: Failed to load firmware chunk!
> [9.425947] iwlwifi :01:00.0: Could not load the [0] uCode section
> [9.432472] iwlwifi :01:00.0: Failed to start INIT ucode: -110
> [9.438899] iwlwifi :01:00.0: Failed to run INIT ucode: -110
> [9.444858] iwlwifi :01:00.0: L1 Disabled - LTR Disabled

It's hard to say why this is happening.


> I downloaded firmware from here
> https://wireless.wiki.kernel.org/en/users/Drivers/iwlwifi

Why did you have to download the firmware? Doesn't your distro include
the binaries from linux-firmware.git?

The firmware you are using is version 16, but that kernel already
supports version -17.  I recommend you try to take the latest firmware
for the 7260 NIC from our copy of linux-firmware.git[2].

If this doesn't help, the best thing to do would be to report this as a
bug in bugzilla.kernel.org following the instructions in our wiki[1]. 
This way we can help you more easily.


[1] https://wireless.wiki.kernel.org/en/users/drivers/iwlwifi/debugging
[2] 
http://git.kernel.org/cgit/linux/kernel/git/iwlwifi/linux-firmware.git/plain/iwlwifi-7260-17.ucode

--
Cheers,
Luca.

Re: kernel 4.9 iwlwifi startup error

[Luca Coelho]

On Tue, 2017-01-10 at 09:21 +0100, Fabio Coatti wrote:
> In data martedì 10 gennaio 2017 00:21:51 CET, Luca Coelho ha scritto:
> > On Tue, 2017-01-03 at 13:42 +1100, Andrew Donnellan wrote:
> > > On 02/01/17 21:12, Fabio Coatti wrote:
> > > > Hi all,
> > > > I'm using kernel 4.9 and maybe half of the times I boot my laptop I get
> > > > the
> > > > error reported below, and the wifi does not work. I have to remove
> > > > iwlwifi (like modprobe -r iwldvm iwlwifi) and insert it again to get
> > > > things workig again. This seems a bit random, it does not happens all
> > > > the times so it could be a timing issue or even a flaky hardware
> > > > (unlikely, as I see only this issue with wifi, once it starts it works
> > > > just fine)
> > > > I'm pretty sure to have seen the same behaviour at some point in 4.8.X
> > > > release, but right now I lost the related notes.
> > > > Environment:
> > > > Distro: gentoo
> > > > gcc 5.4.0
> > > > HW: Hewlett-Packard HP EliteBook Folio 9470m/18DF, BIOS 68IBD Ver. F.63
> > > > 04/26/2016
> > > > Intel(R) Core(TM) i5-3427U CPU @ 1.80GHz
> > > > Network controller: Intel Corporation Centrino Advanced-N 6235 (rev 24)
> > > > 
> > > > Of course if more info is needed, just drop me a note.
> > > > 
> > > > I'm not subscribed to mailing lists, so please keep me in CC: for any
> > > > information request.
> > > > 
> > > > Many thanks.
> > > 
> > > I've so far seen this once on my laptop, a Samsung NP540U3C (don't have
> > > it with me right now, so I'm not sure what the wifi chipset is), running
> > > with a Debian 4.9 kernel.
> > 
> > This bug has already been reported in bugzilla:
> > 
> > https://bugzilla.kernel.org/show_bug.cgi?id=190281
> > 
> > Did you have any problems with older kernel versions? If so, would it be
> > possible for you to bisect?
> 
> 
> Well, it happens on a laptop that I use quite intensively, so it will take 
> some time to bisect. However, in the meantime I checked the old logs and the 
> first recorded occurrence happened with 4.8.10 release and firmware version 
> 18.168.6.1

Thanks! Let's continue tracking this in bugzilla.

--
Cheers,
Luca.

Re: kernel 4.9 iwlwifi startup error

[Luca Coelho]

On Tue, 2017-01-10 at 09:21 +0100, Fabio Coatti wrote:
> In data martedì 10 gennaio 2017 00:21:51 CET, Luca Coelho ha scritto:
> > On Tue, 2017-01-03 at 13:42 +1100, Andrew Donnellan wrote:
> > > On 02/01/17 21:12, Fabio Coatti wrote:
> > > > Hi all,
> > > > I'm using kernel 4.9 and maybe half of the times I boot my laptop I get
> > > > the
> > > > error reported below, and the wifi does not work. I have to remove
> > > > iwlwifi (like modprobe -r iwldvm iwlwifi) and insert it again to get
> > > > things workig again. This seems a bit random, it does not happens all
> > > > the times so it could be a timing issue or even a flaky hardware
> > > > (unlikely, as I see only this issue with wifi, once it starts it works
> > > > just fine)
> > > > I'm pretty sure to have seen the same behaviour at some point in 4.8.X
> > > > release, but right now I lost the related notes.
> > > > Environment:
> > > > Distro: gentoo
> > > > gcc 5.4.0
> > > > HW: Hewlett-Packard HP EliteBook Folio 9470m/18DF, BIOS 68IBD Ver. F.63
> > > > 04/26/2016
> > > > Intel(R) Core(TM) i5-3427U CPU @ 1.80GHz
> > > > Network controller: Intel Corporation Centrino Advanced-N 6235 (rev 24)
> > > > 
> > > > Of course if more info is needed, just drop me a note.
> > > > 
> > > > I'm not subscribed to mailing lists, so please keep me in CC: for any
> > > > information request.
> > > > 
> > > > Many thanks.
> > > 
> > > I've so far seen this once on my laptop, a Samsung NP540U3C (don't have
> > > it with me right now, so I'm not sure what the wifi chipset is), running
> > > with a Debian 4.9 kernel.
> > 
> > This bug has already been reported in bugzilla:
> > 
> > https://bugzilla.kernel.org/show_bug.cgi?id=190281
> > 
> > Did you have any problems with older kernel versions? If so, would it be
> > possible for you to bisect?
> 
> 
> Well, it happens on a laptop that I use quite intensively, so it will take 
> some time to bisect. However, in the meantime I checked the old logs and the 
> first recorded occurrence happened with 4.8.10 release and firmware version 
> 18.168.6.1

Thanks! Let's continue tracking this in bugzilla.

--
Cheers,
Luca.

Re: kernel 4.9 iwlwifi startup error

[Luca Coelho]

On Tue, 2017-01-03 at 07:41 -0800, Alexander Morozov wrote:
> I have a similar problem on Gentoo. But in my case, it just can't load
> firmware: "no suitable firmware found". I've tried to reinstall
> firmware with no luck. Everything is ok with 4.8.6.

This is a completely different issue.  This means that you don't have
the correct firmware installed for this kernel version.  We do support
several versions back from the latest, but we can't support older
firmwares forever, so if you're using a recent kernel you must upgrade
the firmwares too.

Let me know which device you have, so I can tell you which firmware you
need to use.  But as a general rule, if you clone the linux-firmware
repository[1] and install all the iwlwifi*.ucode files in /lib/firmware
(or wherever gentoo looks for the firmware in your filesystem), you will
surely get the latest version of the firmware that works with your
kernel.

[1] http://git.kernel.org/cgit/linux/kernel/git/firmware/linux-firmware.git

--
Cheers,
Luca.

Re: kernel 4.9 iwlwifi startup error

[Luca Coelho]

On Tue, 2017-01-03 at 07:41 -0800, Alexander Morozov wrote:
> I have a similar problem on Gentoo. But in my case, it just can't load
> firmware: "no suitable firmware found". I've tried to reinstall
> firmware with no luck. Everything is ok with 4.8.6.

This is a completely different issue.  This means that you don't have
the correct firmware installed for this kernel version.  We do support
several versions back from the latest, but we can't support older
firmwares forever, so if you're using a recent kernel you must upgrade
the firmwares too.

Let me know which device you have, so I can tell you which firmware you
need to use.  But as a general rule, if you clone the linux-firmware
repository[1] and install all the iwlwifi*.ucode files in /lib/firmware
(or wherever gentoo looks for the firmware in your filesystem), you will
surely get the latest version of the firmware that works with your
kernel.

[1] http://git.kernel.org/cgit/linux/kernel/git/firmware/linux-firmware.git

--
Cheers,
Luca.

Re: kernel 4.9 iwlwifi startup error

[Luca Coelho]

On Tue, 2017-01-03 at 13:42 +1100, Andrew Donnellan wrote:
> On 02/01/17 21:12, Fabio Coatti wrote:
> > Hi all,
> > I'm using kernel 4.9 and maybe half of the times I boot my laptop I get the
> > error reported below, and the wifi does not work. I have to remove iwlwifi 
> > (like
> > modprobe -r iwldvm iwlwifi) and insert it again to get things workig again.
> > This seems a bit random, it does not happens all the times so it could be a
> > timing issue or even a flaky hardware (unlikely, as I see only this issue 
> > with
> > wifi, once it starts it works just fine)
> > I'm pretty sure to have seen the same behaviour at some point in 4.8.X
> > release, but right now I lost the related notes.
> > Environment:
> > Distro: gentoo
> > gcc 5.4.0
> > HW: Hewlett-Packard HP EliteBook Folio 9470m/18DF, BIOS 68IBD Ver. F.63
> > 04/26/2016
> > Intel(R) Core(TM) i5-3427U CPU @ 1.80GHz
> > Network controller: Intel Corporation Centrino Advanced-N 6235 (rev 24)
> > 
> > Of course if more info is needed, just drop me a note.
> > 
> > I'm not subscribed to mailing lists, so please keep me in CC: for any
> > information request.
> > 
> > Many thanks.
> 
> I've so far seen this once on my laptop, a Samsung NP540U3C (don't have 
> it with me right now, so I'm not sure what the wifi chipset is), running 
> with a Debian 4.9 kernel.

This bug has already been reported in bugzilla:

https://bugzilla.kernel.org/show_bug.cgi?id=190281

Did you have any problems with older kernel versions? If so, would it be
possible for you to bisect?

--
Cheers,
Luca.

Re: kernel 4.9 iwlwifi startup error

[Luca Coelho]

On Tue, 2017-01-03 at 13:42 +1100, Andrew Donnellan wrote:
> On 02/01/17 21:12, Fabio Coatti wrote:
> > Hi all,
> > I'm using kernel 4.9 and maybe half of the times I boot my laptop I get the
> > error reported below, and the wifi does not work. I have to remove iwlwifi 
> > (like
> > modprobe -r iwldvm iwlwifi) and insert it again to get things workig again.
> > This seems a bit random, it does not happens all the times so it could be a
> > timing issue or even a flaky hardware (unlikely, as I see only this issue 
> > with
> > wifi, once it starts it works just fine)
> > I'm pretty sure to have seen the same behaviour at some point in 4.8.X
> > release, but right now I lost the related notes.
> > Environment:
> > Distro: gentoo
> > gcc 5.4.0
> > HW: Hewlett-Packard HP EliteBook Folio 9470m/18DF, BIOS 68IBD Ver. F.63
> > 04/26/2016
> > Intel(R) Core(TM) i5-3427U CPU @ 1.80GHz
> > Network controller: Intel Corporation Centrino Advanced-N 6235 (rev 24)
> > 
> > Of course if more info is needed, just drop me a note.
> > 
> > I'm not subscribed to mailing lists, so please keep me in CC: for any
> > information request.
> > 
> > Many thanks.
> 
> I've so far seen this once on my laptop, a Samsung NP540U3C (don't have 
> it with me right now, so I'm not sure what the wifi chipset is), running 
> with a Debian 4.9 kernel.

This bug has already been reported in bugzilla:

https://bugzilla.kernel.org/show_bug.cgi?id=190281

Did you have any problems with older kernel versions? If so, would it be
possible for you to bisect?

--
Cheers,
Luca.

Re: drivers/net/wireless/intel/iwlwifi/pcie/trans.c: 2 * suspicious code ?

[Luca Coelho]

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: drivers/net/wireless/intel/iwlwifi/pcie/trans.c: 2 * suspicious code ?

[Luca Coelho]

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: Intel Wireless 7260 failed to work

[Luca Coelho]

On Wed, 2016-12-28 at 10:17 +0200, Emmanuel Grumbach wrote:
> On Wed, Dec 28, 2016 at 10:10 AM, Peter Xu <pet...@redhat.com> wrote:
> > On Wed, Dec 28, 2016 at 09:27:15AM +0200, Luca Coelho wrote:
> > 
> > [...]
> > 
> > > > > > Is this a known issue? Please let me know if anyone wants more info 
> > > > > > or
> > > > > > logs, since this error triggers easily (everytime I boot).
> > > > > 
> > > > > The error message isn't really telling much to the user (hint hint) 
> > > > > but
> > > > > I suspect this is by design:
> > > > > 
> > > > > "iwlwifi: remove support for fw older than -17 and -22
> > > 
> > > Right, we only maintain support for a certain number of firmware
> > > versions.  The FW APIs change with time and we don't want to keep all
> > > legacy code supporting old firmwares in the driver forever.
> > > 
> > > I agree that "no suitable firmware found!" is a bit scarce.  I'll see
> > > if we can improve that with something: "no suitable firmware found! You
> > > need iwlwifi-7260-17.ucode ()".
> > 
> > That'll be great if we can have this info in the log. Maybe no need
> > for a full URL, the firmware name would suffice at least for me.
> 
> In this case, I think we should also print the range that is supported
> when we fail to find any suitable firmware.

Sure, I'll do that.  I just wanted to keep it simple in this thread. ;)

--
Luca.

Re: Intel Wireless 7260 failed to work

[Luca Coelho]

On Wed, 2016-12-28 at 10:17 +0200, Emmanuel Grumbach wrote:
> On Wed, Dec 28, 2016 at 10:10 AM, Peter Xu  wrote:
> > On Wed, Dec 28, 2016 at 09:27:15AM +0200, Luca Coelho wrote:
> > 
> > [...]
> > 
> > > > > > Is this a known issue? Please let me know if anyone wants more info 
> > > > > > or
> > > > > > logs, since this error triggers easily (everytime I boot).
> > > > > 
> > > > > The error message isn't really telling much to the user (hint hint) 
> > > > > but
> > > > > I suspect this is by design:
> > > > > 
> > > > > "iwlwifi: remove support for fw older than -17 and -22
> > > 
> > > Right, we only maintain support for a certain number of firmware
> > > versions.  The FW APIs change with time and we don't want to keep all
> > > legacy code supporting old firmwares in the driver forever.
> > > 
> > > I agree that "no suitable firmware found!" is a bit scarce.  I'll see
> > > if we can improve that with something: "no suitable firmware found! You
> > > need iwlwifi-7260-17.ucode ()".
> > 
> > That'll be great if we can have this info in the log. Maybe no need
> > for a full URL, the firmware name would suffice at least for me.
> 
> In this case, I think we should also print the range that is supported
> when we fail to find any suitable firmware.

Sure, I'll do that.  I just wanted to keep it simple in this thread. ;)

--
Luca.

Re: Intel Wireless 7260 failed to work

[Luca Coelho]

On Wed, 2016-12-28 at 11:59 +0800, Peter Xu wrote:
> On Tue, Dec 27, 2016 at 09:46:55PM +0200, Kalle Valo wrote:
> > Peter Xu  writes:
> > 
> > > Looks like latest Linux master (4.10-rc1, 7ce7d89f) cannot work well
> > > with my wireless card, which is:
> > > 
> > >   Intel Corporation Wireless 7260 (rev bb)
> > > 
> > > Boot message shows that no suitable firmware found:
> > > 
> > > # journalctl -kp3
> > > Dec 27 16:38:00 kernel: Error parsing PCC subspaces from PCCT
> > > Dec 27 16:38:00 kernel: mmc0: Unknown controller version (3). You may 
> > > experience problems.
> > > Dec 27 16:38:02 kernel: DMAR: DRHD: handling fault status reg 2
> > > Dec 27 16:38:02 kernel: DMAR: [DMA Write] Request device [00:02.0] 
> > > fault addr 72 [fault reason 05] PTE Write a
> > > Dec 27 16:38:03 kernel: tpm tpm0: A TPM error (6) occurred attempting 
> > > to read a pcr value
> > > Dec 27 16:38:04 kernel: iwlwifi :03:00.0: no suitable firmware 
> > > found!
> > > 
> > > Linux stable/master (Linux 4.8-rc8, 08895a8b6b) works for me. And no
> > > further tests have been done yet.
> > > 
> > > Is this a known issue? Please let me know if anyone wants more info or
> > > logs, since this error triggers easily (everytime I boot).
> > 
> > The error message isn't really telling much to the user (hint hint) but
> > I suspect this is by design:
> > 
> > "iwlwifi: remove support for fw older than -17 and -22

Right, we only maintain support for a certain number of firmware
versions.  The FW APIs change with time and we don't want to keep all
legacy code supporting old firmwares in the driver forever.

I agree that "no suitable firmware found!" is a bit scarce.  I'll see
if we can improve that with something: "no suitable firmware found! You
need iwlwifi-7260-17.ucode ()".


> > FW versions older than -17 for 3160 and 7260 and older than -22 for
> > newer NICs are not supported anymore.  Don't load these versions
> > and remove code that handles them."
> > 
> > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=4b87e5af638b6056bd6c20b0954d09a5a58633be
> > 
> > Adding luca.
> 
> Thanks for the triage.
> 
> Larry's link for -17 firmware solved my issue. Though I still don't
> know whether it is too aggresive if we just remove the support for -16
> (which is the one I was using with the old 4.6 kernel, btw I am using
> Fedora 24, which is relatively new as well).

I don't think we are very aggressive, we have been supporting -17 since
v4.3:

https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=5865f3658ba37c54e346b0fdee08a1c7a152681b

And we have published the firmware about half a year ago:

http://git.kernel.org/cgit/linux/kernel/git/firmware/linux-firmware.git/commit/iwlwifi-7260-17.ucode?id=f2cf4d67e8eced29c8a473d3a27057aa2df57c42

I understand your concern, but if you want to be on the bleeding-edge
kernel, you should be on the bleeding-edge linux-firmware as well. ;)

But as I said, I'll try to improve the error message, as that should
make it easier to figure out.

--
Cheers,
Luca.

Re: Intel Wireless 7260 failed to work

[Luca Coelho]

On Wed, 2016-12-28 at 11:59 +0800, Peter Xu wrote:
> On Tue, Dec 27, 2016 at 09:46:55PM +0200, Kalle Valo wrote:
> > Peter Xu  writes:
> > 
> > > Looks like latest Linux master (4.10-rc1, 7ce7d89f) cannot work well
> > > with my wireless card, which is:
> > > 
> > >   Intel Corporation Wireless 7260 (rev bb)
> > > 
> > > Boot message shows that no suitable firmware found:
> > > 
> > > # journalctl -kp3
> > > Dec 27 16:38:00 kernel: Error parsing PCC subspaces from PCCT
> > > Dec 27 16:38:00 kernel: mmc0: Unknown controller version (3). You may 
> > > experience problems.
> > > Dec 27 16:38:02 kernel: DMAR: DRHD: handling fault status reg 2
> > > Dec 27 16:38:02 kernel: DMAR: [DMA Write] Request device [00:02.0] 
> > > fault addr 72 [fault reason 05] PTE Write a
> > > Dec 27 16:38:03 kernel: tpm tpm0: A TPM error (6) occurred attempting 
> > > to read a pcr value
> > > Dec 27 16:38:04 kernel: iwlwifi :03:00.0: no suitable firmware 
> > > found!
> > > 
> > > Linux stable/master (Linux 4.8-rc8, 08895a8b6b) works for me. And no
> > > further tests have been done yet.
> > > 
> > > Is this a known issue? Please let me know if anyone wants more info or
> > > logs, since this error triggers easily (everytime I boot).
> > 
> > The error message isn't really telling much to the user (hint hint) but
> > I suspect this is by design:
> > 
> > "iwlwifi: remove support for fw older than -17 and -22

Right, we only maintain support for a certain number of firmware
versions.  The FW APIs change with time and we don't want to keep all
legacy code supporting old firmwares in the driver forever.

I agree that "no suitable firmware found!" is a bit scarce.  I'll see
if we can improve that with something: "no suitable firmware found! You
need iwlwifi-7260-17.ucode ()".


> > FW versions older than -17 for 3160 and 7260 and older than -22 for
> > newer NICs are not supported anymore.  Don't load these versions
> > and remove code that handles them."
> > 
> > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=4b87e5af638b6056bd6c20b0954d09a5a58633be
> > 
> > Adding luca.
> 
> Thanks for the triage.
> 
> Larry's link for -17 firmware solved my issue. Though I still don't
> know whether it is too aggresive if we just remove the support for -16
> (which is the one I was using with the old 4.6 kernel, btw I am using
> Fedora 24, which is relatively new as well).

I don't think we are very aggressive, we have been supporting -17 since
v4.3:

https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=5865f3658ba37c54e346b0fdee08a1c7a152681b

And we have published the firmware about half a year ago:

http://git.kernel.org/cgit/linux/kernel/git/firmware/linux-firmware.git/commit/iwlwifi-7260-17.ucode?id=f2cf4d67e8eced29c8a473d3a27057aa2df57c42

I understand your concern, but if you want to be on the bleeding-edge
kernel, you should be on the bleeding-edge linux-firmware as well. ;)

But as I said, I'll try to improve the error message, as that should
make it easier to figure out.

--
Cheers,
Luca.