Re: [PATCH] iwlwifi: pcie: fix SPLC structure parsing

[Chris Rorvick]

Hi Luca,

> On Thu, 2016-10-13 at 13:21 +0300, Luca Coelho wrote:
> Could you please give this a spin? I have tested it with some handmade
> ACPI tables in QEMU and it seems to work fine now.

Tested-by: Chris Rorvick <ch...@rorvick.com>

I think the debug output looks as expected, see below for the first 20
lines or so.  And, more importantly, everything seems to be working!
:-)

Thanks!

Chris

==

iwlwifi :3a:00.0: L1 Enabled - LTR Disabled
iwlwifi: module verification failed: signature and/or required key
missing - tainting kernel
Intel(R) Wireless WiFi driver for Linux
Copyright(c) 2003- 2015 Intel Corporation
iwlwifi :3a:00.0: U iwl_pcie_prepare_card_hw iwl_trans_prepare_card_hw enter
iwlwifi :3a:00.0: U iwl_pcie_set_hw_ready hardware ready
iwlwifi :3a:00.0: U iwl_request_firmware attempting to load
firmware 'iwlwifi-8000C-24.ucode'
iwlwifi :3a:00.0: Direct firmware load for iwlwifi-8000C-24.ucode
failed with error -2
iwlwifi :3a:00.0: U iwl_request_firmware attempting to load
firmware 'iwlwifi-8000C-23.ucode'
iwlwifi :3a:00.0: Direct firmware load for iwlwifi-8000C-23.ucode
failed with error -2
iwlwifi :3a:00.0: U iwl_request_firmware attempting to load
firmware 'iwlwifi-8000C-22.ucode'
iwlwifi :3a:00.0: Direct firmware load for iwlwifi-8000C-22.ucode
failed with error -2
iwlwifi :3a:00.0: U iwl_request_firmware attempting to load
firmware 'iwlwifi-8000C-21.ucode'
iwlwifi :3a:00.0: U iwl_req_fw_callback Loaded firmware file
'iwlwifi-8000C-21.ucode' (2394060 bytes).
iwlwifi :3a:00.0: U iwl_parse_tlv_firmware unknown TLV: 48
iwlwifi :3a:00.0: U iwl_parse_tlv_firmware GSCAN is supported but
capabilities TLV is unavailable
iwlwifi :3a:00.0: U splc_get_pwr_limit No element for the WiFi
domain returned by the SPLC method.
iwlwifi :3a:00.0: U set_dflt_pwr_limit Default power limit set to 0
iwlwifi :3a:00.0: loaded firmware version 21.302800.0 op_mode iwlmvm
iwlwifi :3a:00.0: Detected Intel(R) Dual Band Wireless AC 8260, REV=0x208

Re: [PATCH] iwlwifi: pcie: reduce "unsupported splx" to a warning

[Chris Rorvick]

On Wed, Oct 12, 2016 at 1:05 PM, Paul Bolle <pebo...@tiscali.nl> wrote:
> On Wed, 2016-10-12 at 12:50 -0500, Chris Rorvick wrote:
>> This may already be apparent, but Dell sells two versions of the 9350:
>> one with the Broadcom adapter and one with the AC 8260.
>
> Off topic, for most readers: my version (with the AC 8260) came with
> Ubuntu preinstalled. Perhaps Chris' version came preinstalled with
> something else?

Exactly.  Mine came with Windows 10 and the Broadcom adapter, while
the "Developer Edition" comes preloaded with Ubuntu and has the AC
8260.  The Broadcom adapter has been supported since 4.4 but still
seems to have issues.  For example, I had it working at home but I was
not able to connect to a friend's AT U-Verse wireless gateway;
something was failing with -EBUSY.  I ordered the AC 8260 and it works
fine after the upgrade.

Chris

Re: [PATCH] iwlwifi: pcie: reduce "unsupported splx" to a warning

[Chris Rorvick]

Hi Luca,

FYI, It seems that Google does not like your email as I'm not
receiving any of your messages in gmail.  Some responses below:

On Wed, 2016-10-12 at 15:24 +0300, Luca Coelho wrote:
> Hi Chris,
> On Tue, 2016-10-11 at 09:09 -0500, Chris Rorvick wrote:
> > On Tue, Oct 11, 2016 at 5:11 AM, Paul Bolle  wrote:
> > > > This is not coming from the NIC itself, but from the platform's ACPI
> > > > tables. Can you tell us which platform you are using?
> >
> > Interesting. I'm running a Dell XPS 13 9350. I replaced the
> > factory-provided Broadcom card with an AC 8260. I can update the
> > commit log to reflect this.
>
> Okay, so this makes sense. Those entries are probably formatted for
> the Broadcom card, which the iwlwifi driver obviously doesn't
> understand. The best we can do, as I already said, is to ignore values
> we don't understand.

This may already be apparent, but Dell sells two versions of the 9350:
one with the Broadcom adapter and one with the AC 8260.  I just
happened to find the former version at a deep discount at Costco so
decided to chance it.  Turns out the Broadcom card is not so good even
with new kernels so I upgraded.  Anyway, since Paul is seeing the same
issue I don't think the values are intended to be Broadcom-specific.

On Wed, 2016-10-12 at 17:21 +0300, Luca Coelho wrote:
> And, the values in the SPLX structs are being changed here, to DOM1,
> LIM1, TIM1 etc., before being returned. Â This also matches your
> description that, at runtime, you got something different than the pure
> dump. Â If you follow these DOM*, LIM*, TIM* symbols, you'll probably
> end up getting the values you observed at runtime.

Probably not important, but it seems that there is some additional
indirection.  The only values I'm seeing associated with those symbols
are 8 and 16:

$ grep -e 'DOM[0-9]' -e 'LIM[0-9]' -e 'TIM[0-9]' dsdt.dsl | grep -v Store
DOM1,   8,
LIM1,   16,
TIM1,   16,
DOM2,   8,
LIM2,   16,
TIM2,   16,
DOM3,   8,
LIM3,   16,
TIM3,   16,

> I'll send you a patch for testing soon.

I will keep an eye on the list archive, thanks!

Chris

Re: [PATCH] iwlwifi: pcie: reduce "unsupported splx" to a warning

[Chris Rorvick]

On Tue, Oct 11, 2016 at 5:11 AM, Paul Bolle  wrote:
> For what it's worth, on my machine I have twenty (!) SPLX entries, all
> reading:
> Name (SPLX, Package (0x04)
> {
> Zero,
> Package (0x03)
> {
> 0x8000,
> 0x8000,
> 0x8000
> },
>
> Package (0x03)
> {
>0x8000,
>0x8000,
>0x8000
> },
>
> Package (0x03)
> {
> 0x8000,
> 0x8000,
> 0x8000
> }
> })

I actually see exactly the same on my Dell XPS 13 (9350) when I  use
acpidump, etc.  I typed the entry I included in the commit log by hand
based on what the driver gets back from the SPLC method (I added a
function to dump the returned object.)

Chris

Re: [PATCH] iwlwifi: pcie: reduce "unsupported splx" to a warning

[Chris Rorvick]

On Tue, Oct 11, 2016 at 9:09 AM, Chris Rorvick <ch...@rorvick.com> wrote:
> I didn't receive your email so I'll try to respond via Paul's.

>>> If this is really bothering you, I guess I could apply this patch for
>>> now.  But as I said, this is not solving the actual problem.
>>
>> Bikeshedding: I think IWL_INFO() is more appropriate, as info doesn't
>> imply one needs to act on this message, while warn does imply that
>> action is needed.
>
> Agreed.  I still think making this a warning is appropriate, but it
> seems pretty clear this is not an error.  This has nothing to do with
> how much it bothers me.  An error tells the user something needs to be
> fixed, but in this case the interface is working fine.  Making it a
> warning with an improved message will result in fewer people wasting
> their time.

I found your original email on lkml.org... should have looked there in
the first place!  Yes, if there is a fix for the underlying issue then
that is obviously preferred.  When I investigated this I saw several
reports spanning at least a few distros and kernel versions with at
least some concluding "this is normal".

Again, thanks!

Chris

Re: [PATCH] iwlwifi: pcie: reduce "unsupported splx" to a warning

[Chris Rorvick]

Hi Luca,

I didn't receive your email so I'll try to respond via Paul's.

On Tue, Oct 11, 2016 at 5:11 AM, Paul Bolle  wrote:
>> This is not coming from the NIC itself, but from the platform's ACPI
>> tables.  Can you tell us which platform you are using?

Interesting.  I'm running a Dell XPS 13 9350.  I replaced the
factory-provided Broadcom card with an AC 8260.  I can update the
commit log to reflect this.

>> There are other things that look a bit inconsistent in this code...
>> I'll try to find the official ACPI table definitions for this entries
>> to make sure it's correct.
>
> When I looked into this error, some time ago, I searched around a bit
> for documentation on this splx stuff. Sadly, commit bcb079a14d75
> ("iwlwifi: pcie: retrieve and parse ACPI power limitations") provides
> very few clues and my searches turned up nothing useful. So a pointer
> or two would be really appreciated.

Ditto.

>> If this is really bothering you, I guess I could apply this patch for
>> now.  But as I said, this is not solving the actual problem.
>
> Bikeshedding: I think IWL_INFO() is more appropriate, as info doesn't
> imply one needs to act on this message, while warn does imply that
> action is needed.

Agreed.  I still think making this a warning is appropriate, but it
seems pretty clear this is not an error.  This has nothing to do with
how much it bothers me.  An error tells the user something needs to be
fixed, but in this case the interface is working fine.  Making it a
warning with an improved message will result in fewer people wasting
their time.

Thanks!

Chris

[PATCH] iwlwifi: pcie: reduce "unsupported splx" to a warning

[Chris Rorvick]

Commit bcb079a14d75 ("iwlwifi: pcie: retrieve and parse ACPI power
limitations") looks for a specific structure in the ACPI tables for
setting the default power limit.  The data returned for at least some
dual band chipsets is not recognized, though.  For example, the AC 8260
reports the following:

Name (SPLX, Package (0x04)
{
Zero,
Package (0x03)
{
0,
1200,
1000
},
Package (0x03)
{
0,
1200,
1000
},
Package (0x03)
{
0,
1200,
1000
}
})

The current logic expects exactly two elements in the outer package,
causing the above to be ignored and the power limit unset.

Despite the interface being fully functional after initialization, the
above condition is reported as an error.  Knock the message down to a
warning and provide better context for understanding its consequence.

Signed-off-by: Chris Rorvick <ch...@rorvick.com>
---
 drivers/net/wireless/intel/iwlwifi/pcie/drv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/intel/iwlwifi/pcie/drv.c 
b/drivers/net/wireless/intel/iwlwifi/pcie/drv.c
index 78cf9a7..19b531f 100644
--- a/drivers/net/wireless/intel/iwlwifi/pcie/drv.c
+++ b/drivers/net/wireless/intel/iwlwifi/pcie/drv.c
@@ -540,7 +540,7 @@ static u64 splx_get_pwr_limit(struct iwl_trans *trans, 
union acpi_object *splx)
splx->package.count != 2 ||
splx->package.elements[0].type != ACPI_TYPE_INTEGER ||
splx->package.elements[0].integer.value != 0) {
-   IWL_ERR(trans, "Unsupported splx structure\n");
+   IWL_WARN(trans, "Unsupported splx structure, not limiting WiFi 
power\n");
return 0;
}
 
-- 
2.10.1