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

2016-10-13 Thread Luca Coelho 
On Thu, 2016-10-13 at 14:55 +0200, Paul Bolle wrote:
> On Thu, 2016-10-13 at 15:44 +0300, Luca Coelho wrote:
> >  Even though there is apparently something wrong with this part of the
> > ACPI table on you laptop, since it doesn't match our specifications.
> >  In any case, it's mostly harmless.
> 
> 
> Would a correct implementation by Dell have any benefits for the users
> of these laptops? In other words: should I bother somehow contacting
> Dell and point them to this discussion in order to have them fix this?

This value provides a way for the OEM to fine tune the power budget of
the device.  This is (usually) used to prevent parts of the platform
from getting too hot.  We have a certain default that is good enough
for most cases.  If Dell didn't want to set proper limits for this
device, it probably means that it is not a concern.  Dell does set
these values correctly for other platforms.

So, I guess it's up to you if you want to clarify this with them.  This
could be some default "blank" values when they don't want to change
them.

--
Luca.

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

2016-10-13 Thread Luca Coelho 
On Thu, 2016-10-13 at 08:56 -0500, Chris Rorvick wrote:
> Hi Luca,
> 
> > On Thu, 2016-10-13 at 13:21 +0300, Luca Coelho wrote:
> > Could you please give this a spin? I have tested it with some handmade
> > ACPI tables in QEMU and it seems to work fine now.
> 
> 
> Tested-by: Chris Rorvick 
> 
> I think the debug output looks as expected, see below for the first 20
> lines or so.  And, more importantly, everything seems to be working!
> :-)

Yes, you got exactly what was expected.  Thanks for testing!

--
Luca.

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

2016-10-13 Thread 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 

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: fix SPLC structure parsing

2016-10-13 Thread Paul Bolle 
On Thu, 2016-10-13 at 15:44 +0300, Luca Coelho wrote:
>  Even though there is apparently something wrong with this part of the
> ACPI table on you laptop, since it doesn't match our specifications.
>  In any case, it's mostly harmless.

Would a correct implementation by Dell have any benefits for the users
of these laptops? In other words: should I bother somehow contacting
Dell and point them to this discussion in order to have them fix this?


Paul Bolle

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

2016-10-13 Thread Luca Coelho 
On Thu, 2016-10-13 at 14:36 +0200, Paul Bolle wrote:
> On Thu, 2016-10-13 at 14:30 +0300, Luca Coelho wrote:
> > I forgot to say... could you load the iwlwifi module with debug=0x01
> > (module parameter), so we can see the messages the driver is printing
> > when it doesn't find a proper structure?
> 
> 
> That makes a lot of noise! Here's the first 100 lines or so, that
> apparently are generated directly after modprobing iwlwifi.
> 
> After these 100 lines there's a ten second gap (I guess it took me ten
> seconds to actually use the wifi). I assume you don't care about that
> part of the debug messages.
> 
> Have fun!

Thanks, Paul!

Yeah, this is the "INFO" debugging level and it is a sort of fallback
bucket when there is nothing more specific.  Sorry for that.


> <7>[  767.691342] iwlwifi :3a:00.0: U iwl_pcie_prepare_card_hw 
> iwl_trans_prepare_card_hw enter
> <7>[  767.691362] iwlwifi :3a:00.0: U iwl_pcie_set_hw_ready hardware ready
> <7>[  767.692127] iwlwifi :3a:00.0: U iwl_request_firmware attempting to 
> load firmware 'iwlwifi-8000C-24.ucode'
> <7>[  767.692322] iwlwifi :3a:00.0: U splc_get_pwr_limit No element for 
> the WiFi domain returned by the SPLC method.

This is the line I was looking for. :) So everything looks fine now.
 Even though there is apparently something wrong with this part of the
ACPI table on you laptop, since it doesn't match our specifications.
 In any case, it's mostly harmless.

Thanks for the help!

--
Cheers,
Luca.

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

2016-10-13 Thread Paul Bolle 
On Thu, 2016-10-13 at 14:30 +0300, Luca Coelho wrote:
> I forgot to say... could you load the iwlwifi module with debug=0x01
> (module parameter), so we can see the messages the driver is printing
> when it doesn't find a proper structure?

That makes a lot of noise! Here's the first 100 lines or so, that
apparently are generated directly after modprobing iwlwifi.

After these 100 lines there's a ten second gap (I guess it took me ten
seconds to actually use the wifi). I assume you don't care about that
part of the debug messages.

Have fun!


Paul Bolle

<7>[  767.691342] iwlwifi :3a:00.0: U iwl_pcie_prepare_card_hw 
iwl_trans_prepare_card_hw enter
<7>[  767.691362] iwlwifi :3a:00.0: U iwl_pcie_set_hw_ready hardware ready
<7>[  767.692127] iwlwifi :3a:00.0: U iwl_request_firmware attempting to 
load firmware 'iwlwifi-8000C-24.ucode'
<7>[  767.692322] iwlwifi :3a:00.0: U splc_get_pwr_limit No element for the 
WiFi domain returned by the SPLC method.
<7>[  767.692324] iwlwifi :3a:00.0: U set_dflt_pwr_limit Default power 
limit set to 0
<4>[  767.692672] iwlwifi :3a:00.0: Direct firmware load for 
iwlwifi-8000C-24.ucode failed with error -2
<7>[  767.692674] iwlwifi :3a:00.0: U iwl_request_firmware attempting to 
load firmware 'iwlwifi-8000C-23.ucode'
<4>[  767.692683] iwlwifi :3a:00.0: Direct firmware load for 
iwlwifi-8000C-23.ucode failed with error -2
<7>[  767.692684] iwlwifi :3a:00.0: U iwl_request_firmware attempting to 
load firmware 'iwlwifi-8000C-22.ucode'
<7>[  767.693267] iwlwifi :3a:00.0: U iwl_req_fw_callback Loaded firmware 
file 'iwlwifi-8000C-22.ucode' (2120860 bytes).
<7>[  767.693271] iwlwifi :3a:00.0: U iwl_parse_tlv_firmware Found debug 
memory segment: 0
<7>[  767.693272] iwlwifi :3a:00.0: U iwl_parse_tlv_firmware Found debug 
memory segment: 1
<7>[  767.693274] iwlwifi :3a:00.0: U iwl_parse_tlv_firmware Found debug 
memory segment: 2
<7>[  767.693276] iwlwifi :3a:00.0: U iwl_parse_tlv_firmware unknown TLV: 48
<7>[  767.693278] iwlwifi :3a:00.0: U iwl_parse_tlv_firmware GSCAN is 
supported but capabilities TLV is unavailable
<6>[  767.693829] iwlwifi :3a:00.0: loaded firmware version 22.361476.0 
op_mode iwlmvm
<6>[  767.747133] iwlwifi :3a:00.0: Detected Intel(R) Dual Band Wireless AC 
8260, REV=0x208
<7>[  767.747141] iwlwifi :3a:00.0: U iwl_pcie_prepare_card_hw 
iwl_trans_prepare_card_hw enter
<7>[  767.747168] iwlwifi :3a:00.0: U iwl_pcie_set_hw_ready hardware ready
<7>[  767.749221] iwlwifi :3a:00.0: U iwl_pcie_apm_init Init card's basic 
functions
<6>[  767.749257] iwlwifi :3a:00.0: L1 Enabled - LTR Disabled
<7>[  767.749855] iwlwifi :3a:00.0: U iwl_pcie_prepare_card_hw 
iwl_trans_prepare_card_hw enter
<7>[  767.749870] iwlwifi :3a:00.0: U iwl_pcie_set_hw_ready hardware ready
<7>[  767.749878] iwlwifi :3a:00.0: U iwl_pcie_apm_init Init card's basic 
functions
<6>[  767.749896] iwlwifi :3a:00.0: L1 Enabled - LTR Disabled
<7>[  767.749906] iwlwifi :3a:00.0: U iwl_mvm_nic_config Radio 
type=0x0-0x2-0x1
<7>[  767.750608] iwlwifi :3a:00.0: U iwl_pcie_nic_init Enabling shadow 
registers in device
<7>[  767.750624] iwlwifi :3a:00.0: U iwl_pcie_rsa_race_bug_wa can't access 
the RSA semaphore it is write protected
<7>[  767.810446] iwlwifi :3a:00.0: I iwl_mvm_rx_mfuart_notif MFUART: 
installed ver: 0x12000415, external ver: 0x12000415, status: 0x00010080, 
duration: 0x0006
<7>[  767.810861] iwlwifi :3a:00.0: U iwl_pcie_send_hcmd_sync Attempting to 
send sync command BT_CONFIG
<7>[  767.810862] iwlwifi :3a:00.0: U iwl_pcie_send_hcmd_sync Setting 
HCMD_ACTIVE for command BT_CONFIG
<7>[  767.810984] iwlwifi :3a:00.0: I iwl_pcie_hcmd_complete Clearing 
HCMD_ACTIVE for command BT_CONFIG
<7>[  767.811023] iwlwifi :3a:00.0: U iwl_pcie_send_hcmd_sync Attempting to 
send sync command NVM_ACCESS_CMD
<7>[  767.811025] iwlwifi :3a:00.0: U iwl_pcie_send_hcmd_sync Setting 
HCMD_ACTIVE for command NVM_ACCESS_CMD
<7>[  767.811098] iwlwifi :3a:00.0: I iwl_pcie_hcmd_complete Clearing 
HCMD_ACTIVE for command NVM_ACCESS_CMD
<7>[  767.811102] iwlwifi :3a:00.0: U iwl_pcie_send_hcmd_sync Attempting to 
send sync command NVM_ACCESS_CMD
<7>[  767.811103] iwlwifi :3a:00.0: U iwl_pcie_send_hcmd_sync Setting 
HCMD_ACTIVE for command NVM_ACCESS_CMD
<7>[  767.811212] iwlwifi :3a:00.0: I iwl_pcie_hcmd_complete Clearing 
HCMD_ACTIVE for command NVM_ACCESS_CMD
<7>[  767.811222] iwlwifi :3a:00.0: U iwl_pcie_send_hcmd_sync Attempting to 
send sync command NVM_ACCESS_CMD
<7>[  767.811223] iwlwifi :3a:00.0: U iwl_pcie_send_hcmd_sync Setting 
HCMD_ACTIVE for command NVM_ACCESS_CMD
<7>[  767.811318] iwlwifi :3a:00.0: I iwl_pcie_hcmd_complete Clearing 
HCMD_ACTIVE for command NVM_ACCESS_CMD
<7>[  767.811326] iwlwifi :3a:00.0: U iwl_pcie_send_hcmd_sync Attempting to 
send sync command NVM_ACCESS_CMD
<7>[  767.811327] iwlwifi :3a:00.0: U

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

2016-10-13 Thread Luca Coelho 
On Thu, 2016-10-13 at 13:27 +0200, Paul Bolle wrote:
> Luca,
> 
> On Thu, 2016-10-13 at 13:21 +0300, Luca Coelho wrote:
> > Could you please give this a spin? I have tested it with some handmade
> > ACPI tables in QEMU and it seems to work fine now.
> 
> 
> Tested-by: Paul Bolle 
> 
> Not that this test was worth a lot: it builds cleanly (on top of
> 4.8.1), the error at boot is gone, obviously, and wifi still works (as
> you're reading a message that was sent out over wifi). And I haven't
> even tested this on another machine than my XPS 13 (9350).

Thanks for testing!

I forgot to say... could you load the iwlwifi module with debug=0x01
(module parameter), so we can see the messages the driver is printing
when it doesn't find a proper structure?

--
Luca.

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

2016-10-13 Thread Paul Bolle 
Luca,

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

Tested-by: Paul Bolle 

Not that this test was worth a lot: it builds cleanly (on top of
4.8.1), the error at boot is gone, obviously, and wifi still works (as
you're reading a message that was sent out over wifi). And I haven't
even tested this on another machine than my XPS 13 (9350).

Thanks!


Paul Bolle

[PATCH] iwlwifi: pcie: fix SPLC structure parsing

2016-10-13 Thread Luca Coelho 
From: Luca Coelho 

The SPLC data parsing is too restrictive and was not trying find the
correct element for WiFi.  This causes problems with some BIOSes where
the SPLC method exists, but doesn't have a WiFi entry on the first
element of the list.  The domain type values are also incorrect
according to the specification.

Fix this by complying with the actual specification.

Additionally, replace all occurrences of SPLX to SPLC, since SPLX is
only a structure internal to the ACPI tables, and may not even exist.

Fixes: bcb079a14d75 ("iwlwifi: pcie: retrieve and parse ACPI power limitations")
Reported-by: Chris Rorvick 
Signed-off-by: Luca Coelho 
---

Paul, Chris,

Could you please give this a spin? I have tested it with some handmade
ACPI tables in QEMU and it seems to work fine now.

Thanks!


drivers/net/wireless/intel/iwlwifi/pcie/drv.c | 79 ---
 1 file changed, 48 insertions(+), 31 deletions(-)

diff --git a/drivers/net/wireless/intel/iwlwifi/pcie/drv.c 
b/drivers/net/wireless/intel/iwlwifi/pcie/drv.c
index 001be40..da4810f 100644
--- a/drivers/net/wireless/intel/iwlwifi/pcie/drv.c
+++ b/drivers/net/wireless/intel/iwlwifi/pcie/drv.c
@@ -541,48 +541,64 @@ static const struct pci_device_id iwl_hw_card_ids[] = {
 MODULE_DEVICE_TABLE(pci, iwl_hw_card_ids);
 
 #ifdef CONFIG_ACPI
-#define SPL_METHOD "SPLC"
-#define SPL_DOMAINTYPE_MODULE  BIT(0)
-#define SPL_DOMAINTYPE_WIFIBIT(1)
-#define SPL_DOMAINTYPE_WIGIG   BIT(2)
-#define SPL_DOMAINTYPE_RFEMBIT(3)
+#define ACPI_SPLC_METHOD   "SPLC"
+#define ACPI_SPLC_DOMAIN_WIFI  (0x07)
 
-static u64 splx_get_pwr_limit(struct iwl_trans *trans, union acpi_object *splx)
+static u64 splc_get_pwr_limit(struct iwl_trans *trans, union acpi_object *splc)
 {
-   union acpi_object *limits, *domain_type, *power_limit;
-
-   if (splx->type != ACPI_TYPE_PACKAGE ||
-   splx->package.count != 2 ||
-   splx->package.elements[0].type != ACPI_TYPE_INTEGER ||
-   splx->package.elements[0].integer.value != 0) {
-   IWL_ERR(trans, "Unsupported splx structure\n");
+   union acpi_object *data_pkg, *dflt_pwr_limit;
+   int i;
+
+   /* We need at least two elemets, one for the revision and one
+* for the data itself.  Also check that the revision is
+* supported (currently only revision 0).
+   */
+   if (splc->type != ACPI_TYPE_PACKAGE ||
+   splc->package.count < 2 ||
+   splc->package.elements[0].type != ACPI_TYPE_INTEGER ||
+   splc->package.elements[0].integer.value != 0) {
+   IWL_DEBUG_INFO(trans,
+  "Unsupported structure returned by the SPLC 
method.  Ignoring.\n");
return 0;
}
 
-   limits = >package.elements[1];
-   if (limits->type != ACPI_TYPE_PACKAGE ||
-   limits->package.count < 2 ||
-   limits->package.elements[0].type != ACPI_TYPE_INTEGER ||
-   limits->package.elements[1].type != ACPI_TYPE_INTEGER) {
-   IWL_ERR(trans, "Invalid limits element\n");
-   return 0;
+   /* loop through all the packages to find the one for WiFi */
+   for (i = 1; i < splc->package.count; i++) {
+   union acpi_object *domain;
+
+   data_pkg = >package.elements[i];
+
+   /* Skip anything that is not a package with the right
+* amount of elements (i.e. at least 2 integers).
+*/
+   if (data_pkg->type != ACPI_TYPE_PACKAGE ||
+   data_pkg->package.count < 2 ||
+   data_pkg->package.elements[0].type != ACPI_TYPE_INTEGER ||
+   data_pkg->package.elements[1].type != ACPI_TYPE_INTEGER)
+   continue;
+
+   domain = _pkg->package.elements[0];
+   if (domain->integer.value == ACPI_SPLC_DOMAIN_WIFI)
+   break;
+
+   data_pkg = NULL;
}
 
-   domain_type = >package.elements[0];
-   power_limit = >package.elements[1];
-   if (!(domain_type->integer.value & SPL_DOMAINTYPE_WIFI)) {
-   IWL_DEBUG_INFO(trans, "WiFi power is not limited\n");
+   if (!data_pkg) {
+   IWL_DEBUG_INFO(trans,
+  "No element for the WiFi domain returned by the 
SPLC method.\n");
return 0;
}
 
-   return power_limit->integer.value;
+   dflt_pwr_limit = _pkg->package.elements[1];
+   return dflt_pwr_limit->integer.value;
 }
 
 static void set_dflt_pwr_limit(struct iwl_trans *trans, struct pci_dev *pdev)
 {
acpi_handle pxsx_handle;
acpi_handle handle;
-   struct acpi_buffer splx = {ACPI_ALLOCATE_BUFFER, NULL};
+   struct acpi_buffer splc = {ACPI_ALLOCATE_BUFFER, NULL};
acpi_status status;
 
pxsx_handle = ACPI_HANDLE(>dev);
@@ -593,23 +609,24 @@