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,
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ<value>,
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ<value>
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ}
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ})
> > 
> > ...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)
>         {
>             0x80000000, 
>             0x80000000, 
>             0x80000000
>         }, 
>     
>         Package (0x03)
>         {
>            0x80000000, 
>            0x80000000, 
>            0x80000000
>         }, 
>     
>         Package (0x03)
>         {
>             0x80000000, 
>             0x80000000, 
>             0x80000000
>         }
>     })

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.