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

[Luca Coelho]

On Wed, 2016-10-12 at 12:50 -0500, Chris Rorvick wrote:
> Hi Luca,
> 
> FYI, It seems that Google does not like your email as I'm not
> receiving any of your messages in gmail.  Some responses below:

That's odd.  It works for me.  Replied privately to try to sort this
out.


> On Wed, 2016-10-12 at 15:24 +0300, Luca Coelho wrote:
> > Hi Chris,
> > On Tue, 2016-10-11 at 09:09 -0500, Chris Rorvick wrote:
> > > On Tue, Oct 11, 2016 at 5:11 AM, Paul Bolle  wrote:
> > > > > This is not coming from the NIC itself, but from the platform's ACPI
> > > > > tables. Can you tell us which platform you are using?
> > > 
> > > 
> > > Interesting. I'm running a Dell XPS 13 9350. I replaced the
> > > factory-provided Broadcom card with an AC 8260. I can update the
> > > commit log to reflect this.
> > 
> > 
> > Okay, so this makes sense. Those entries are probably formatted for
> > the Broadcom card, which the iwlwifi driver obviously doesn't
> > understand. The best we can do, as I already said, is to ignore values
> > we don't understand.
> 
> 
> This may already be apparent, but Dell sells two versions of the 9350:
> one with the Broadcom adapter and one with the AC 8260.  I just
> happened to find the former version at a deep discount at Costco so
> decided to chance it.  Turns out the Broadcom card is not so good even
> with new kernels so I upgraded.  Anyway, since Paul is seeing the same
> issue I don't think the values are intended to be Broadcom-specific.

Right, this is not for Broadcom.  I found out that this is something
Intel specifies as part of the entire platform's ACPI recommendations.


> On Wed, 2016-10-12 at 17:21 +0300, Luca Coelho wrote:
> > And, the values in the SPLX structs are being changed here, to DOM1,
> > LIM1, TIM1 etc., before being returned. Â This also matches your
> > description that, at runtime, you got something different than the pure
> > dump. Â If you follow these DOM*, LIM*, TIM* symbols, you'll probably
> > end up getting the values you observed at runtime.
> 
> 
> Probably not important, but it seems that there is some additional
> indirection.  The only values I'm seeing associated with those symbols
> are 8 and 16:
> 
> $ grep -e 'DOM[0-9]' -e 'LIM[0-9]' -e 'TIM[0-9]' dsdt.dsl | grep -v Store
> DOM1,   8,
> LIM1,   16,
> TIM1,   16,
> DOM2,   8,
> LIM2,   16,
> TIM2,   16,
> DOM3,   8,
> LIM3,   16,
> TIM3,   16,

Yeah, there are often many levels of indirection.  These settings can
be also tied to the configuration that is reachable by the user in the
BIOS setup, so you never know.

The easiest way is probably to run the ASL with acpiexec and execute
the method...

> > I'll send you a patch for testing soon.
> 
> 
> I will keep an eye on the list archive, thanks!

I'll ping you from my Intel address and provide you with a link to the
patch, to make it easier for you. ;)

--
Cheers,
Luca.

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

[Chris Rorvick]

On Wed, Oct 12, 2016 at 1:05 PM, Paul Bolle  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

[Paul Bolle]

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?

Thanks,


Paul Bolle

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

[Paul Bolle]

On Wed, 2016-10-12 at 15:24 +0300, Luca Coelho wrote:
> Okay... Actually this is a structure in the BIOS and the actual method
> we call is SPLC.  The SPLC method may return one item from this table,
> or something entirely different, possible one of the three values
> depending on a configuration option or so.
> 
> Can you to find and send me the actual SPLC method that we call, from
> your BIOS?

It seems Chris and I basically have identical setups, so I'll answer.

There are 20 SPLC methods in the BIOS. The first reads
Method (SPLC, 0, Serialized)
{
DerefOf (SPLX [One]) [Zero] = DOM1 /* \DOM1 */
DerefOf (SPLX [One]) [One] = LIM1 /* \LIM1 */
DerefOf (SPLX [One]) [0x02] = TIM1 /* \TIM1 */
DerefOf (SPLX [0x02]) [Zero] = DOM2 /* \DOM2 */
DerefOf (SPLX [0x02]) [One] = LIM2 /* \LIM2 */
DerefOf (SPLX [0x02]) [0x02] = TIM2 /* \TIM2 */
DerefOf (SPLX [0x03]) [Zero] = DOM3 /* \DOM3 */
DerefOf (SPLX [0x03]) [One] = LIM3 /* \LIM3 */
DerefOf (SPLX [0x03]) [0x02] = TIM3 /* \TIM3 */
Return (SPLX) /* \_SB_.PCI0.RP01.PXSX.SPLX */
}

The only difference is in the last comment. Ie, RP01 is increased until
it reaches RP20. (The machine has 20 PCI devices according to lspci. I
have no clue how to match that RPxx number to the 20 devices showing up
in lspci, sorry.)


Paul Bolle

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

[Luca Coelho]

On Tue, 2016-10-11 at 23:32 -0500, Chris Rorvick wrote:
> On Tue, Oct 11, 2016 at 5:11 AM, Paul Bolle  wrote:
> > For what it's worth, on my machine I have twenty (!) SPLX entries, all
> > reading:
> > Name (SPLX, Package (0x04)
> > {
> > Zero,
> > Package (0x03)
> > {
> > 0x8000,
> > 0x8000,
> > 0x8000
> > },
> > 
> > Package (0x03)
> > {
> >0x8000,
> >0x8000,
> >0x8000
> > },
> > 
> > Package (0x03)
> > {
> > 0x8000,
> > 0x8000,
> > 0x8000
> > }
> > })
> 
> 
> I actually see exactly the same on my Dell XPS 13 (9350) when I  use
> acpidump, etc.  I typed the entry I included in the commit log by hand
> based on what the driver gets back from the SPLC method (I added a
> function to dump the returned object.)

Okay... Actually this is a structure in the BIOS and the actual method
we call is SPLC.  The SPLC method may return one item from this table,
or something entirely different, possible one of the three values
depending on a configuration option or so.

Can you to find and send me the actual SPLC method that we call, from
your BIOS?

--
Cheers,
Luca.

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

[Paul Bolle]

Luca,

On Wed, 2016-10-12 at 09:11 +0300, Luca Coelho wrote:
> By "platform" I meant the PC you are using.  The ACPI table is created
> by the OEM, so different PCs have different tables.

Like Chris I use a Dell XPS 13 (9350), but mine came with an AC 8260
out of it's assembly plant:
    Detected Intel(R) Dual Band Wireless AC 8260, REV=0x208

> In any case, I will rework this code, so I'd prefer if we skip this
> patch entirely.

Feel free to prod me for testing whatever you come up with.


Paul Bolle

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

[Luca Coelho]

Hi Paul,
On Tue, 2016-10-11 at 12:11 +0200, Paul Bolle wrote:
> On Mon, 2016-10-10 at 17:02 +0300, Luca Coelho wrote:
> > On Mon, 2016-10-10 at 02:19 -0500, Chris Rorvick wrote:
> > This is not coming from the NIC itself, but from the platform's ACPI
> > tables.  Can you tell us which platform you are using?
> 
> 
> On my machine I'm seeing the same error as Chris. So what exactly do
> you mean with "platform" here?

By "platform" I meant the PC you are using.  The ACPI table is created
by the OEM, so different PCs have different tables.


> 
> > > Name (SPLX, Package (0x04)
> > > {
> > > Zero,
> > > Package (0x03)
> > > {
> > > 0,
> > > 1200,
> > > 1000
> > > },
> > > Package (0x03)
> > > {
> > > 0,
> > > 1200,
> > > 1000
> > > },
> > > Package (0x03)
> > > {
> > > 0,
> > > 1200,
> > > 1000
> > > }
> > > })
> > 
> > 
> > This is not the structure that we are expecting.  We expect this:
> > 
> >    Name (SPLX, Package (0x02)
> >    {
> >    Zero,
> >    Package (0x03)
> >    {
> >    0x07,
> >    ,
> >    
> >    }
> >    })
> > 
> > ...as you correctly pointed out.  The data in the structure you have is
> > not for WiFi (actually I don't think 0 is a valid value, but I'll
> > double-check).
> 
> 
> For what it's worth, on my machine I have twenty (!) SPLX entries, all
> reading:
> Name (SPLX, Package (0x04)
> {
> Zero, 
> Package (0x03)
> {
> 0x8000, 
> 0x8000, 
> 0x8000
> }, 
> 
> Package (0x03)
> {
>0x8000, 
>0x8000, 
>0x8000
> }, 
> 
> Package (0x03)
> {
> 0x8000, 
> 0x8000, 
> 0x8000
> }
> })

Thanks.  So this is another case where the first value doesn't match
what we are expecting and we should just ignore that.


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

Yeah, I looked into that commit too and there's not much there.  I'll
try to find the documentation and, if I can, I'll share it with you.


> > > --- a/drivers/net/wireless/intel/iwlwifi/pcie/drv.c
> > > +++ b/drivers/net/wireless/intel/iwlwifi/pcie/drv.c
> > > @@ -540,7 +540,7 @@ static u64 splx_get_pwr_limit(struct iwl_trans 
> > > *trans, union acpi_object *splx)
> > >   splx->package.count != 2 ||
> > >   splx->package.elements[0].type != ACPI_TYPE_INTEGER ||
> > >   splx->package.elements[0].integer.value != 0) {
> > > - IWL_ERR(trans, "Unsupported splx structure\n");
> > > + IWL_WARN(trans, "Unsupported splx structure, not limiting WiFi 
> > > power\n");
> > >   return 0;
> > >   }
> > 
> > 
> > If this is really bothering you, I guess I could apply this patch for
> > now.  But as I said, this is not solving the actual problem.
> 
> 
> Bikeshedding: I think IWL_INFO() is more appropriate, as info doesn't
> imply one needs to act on this message, while warn does imply that
> action is needed.

Right, but in fact, the code considers that if the SPLX method exists,
it must return a value iwlwifi can understand, thus the error.  That
assumption is wrong, so we should just ignore entries that don't match
and continue without printing anything out (as would happen if the splx
method were not even there).

In any case, I will rework this code, so I'd prefer if we skip this
patch entirely.

--
Cheers,
Luca.

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

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

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

[Paul Bolle]

Hi Luca,

On Mon, 2016-10-10 at 17:02 +0300, Luca Coelho wrote:
> On Mon, 2016-10-10 at 02:19 -0500, Chris Rorvick wrote:
> This is not coming from the NIC itself, but from the platform's ACPI
> tables.  Can you tell us which platform you are using?

On my machine I'm seeing the same error as Chris. So what exactly do
you mean with "platform" here?

> > Name (SPLX, Package (0x04)
> > {
> > Zero,
> > Package (0x03)
> > {
> > 0,
> > 1200,
> > 1000
> > },
> > Package (0x03)
> > {
> > 0,
> > 1200,
> > 1000
> > },
> > Package (0x03)
> > {
> > 0,
> > 1200,
> > 1000
> > }
> > })
> 
> This is not the structure that we are expecting.  We expect this:
> 
>    Name (SPLX, Package (0x02)
>    {
>    Zero,
>    Package (0x03)
>    {
>    0x07,
>    ,
>    
>    }
>    })
> 
> ...as you correctly pointed out.  The data in the structure you have is
> not for WiFi (actually I don't think 0 is a valid value, but I'll
> double-check).

For what it's worth, on my machine I have twenty (!) SPLX entries, all
reading:
Name (SPLX, Package (0x04)
{
Zero, 
Package (0x03)
{
0x8000, 
0x8000, 
0x8000
}, 

Package (0x03)
{
   0x8000, 
   0x8000, 
   0x8000
}, 

Package (0x03)
{
0x8000, 
0x8000, 
0x8000
}
})

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

> > --- a/drivers/net/wireless/intel/iwlwifi/pcie/drv.c
> > +++ b/drivers/net/wireless/intel/iwlwifi/pcie/drv.c
> > @@ -540,7 +540,7 @@ static u64 splx_get_pwr_limit(struct iwl_trans *trans, 
> > union acpi_object *splx)
> >     splx->package.count != 2 ||
> >     splx->package.elements[0].type != ACPI_TYPE_INTEGER ||
> >     splx->package.elements[0].integer.value != 0) {
> > -   IWL_ERR(trans, "Unsupported splx structure\n");
> > +   IWL_WARN(trans, "Unsupported splx structure, not limiting WiFi 
> > power\n");
> >     return 0;
> >     }
> 
> If this is really bothering you, I guess I could apply this patch for
> now.  But as I said, this is not solving the actual problem.

Bikeshedding: I think IWL_INFO() is more appropriate, as info doesn't
imply one needs to act on this message, while warn does imply that
action is needed.

Thanks,


Paul Bolle

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

[Luca Coelho]

Hi,
On Mon, 2016-10-10 at 02:19 -0500, Chris Rorvick wrote:
> Commit bcb079a14d75 ("iwlwifi: pcie: retrieve and parse ACPI power
> limitations") looks for a specific structure in the ACPI tables for
> setting the default power limit.  The data returned for at least some
> dual band chipsets is not recognized, though.  For example, the AC 8260
> reports the following:

This is not coming from the NIC itself, but from the platform's ACPI
tables.  Can you tell us which platform you are using?


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

This is not the structure that we are expecting.  We expect this:

   Name (SPLX, Package (0x02)
   {
   Zero,
   Package (0x03)
   {
   0x07,
   ,
   
   }
   })

...as you correctly pointed out.  The data in the structure you have is
not for WiFi (actually I don't think 0 is a valid value, but I'll
double-check).


> The current logic expects exactly two elements in the outer package,
> causing the above to be ignored and the power limit unset.
> 
> Despite the interface being fully functional after initialization, the
> above condition is reported as an error.  Knock the message down to a
> warning and provide better context for understanding its consequence.

Reducing this to a warning is an easy way to reduce the verbosity of
the problem, but I think the correct thing to do would be to accept
multiple entries and ignore the ones that don't have the WIFI marker.
 And only type-check the WIFI ones.

There are other things that look a bit inconsistent in this code...
I'll try to find the official ACPI table definitions for this entries
to make sure it's correct.


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

If this is really bothering you, I guess I could apply this patch for
now.  But as I said, this is not solving the actual problem.

--
Cheers,
Luca.