Re: [PATCH 07/33] iwlwifi: mvm: use match_string() helper

2018-05-21 Thread 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: netdev@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)

2018-04-17 Thread 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 13/17] iwlwifi: mvm: Convert timers to use timer_setup()

2017-11-06 Thread Luca Coelho 
On Mon, 2017-11-06 at 11:45 -0800, Kees Cook wrote:
> On Sun, Oct 29, 2017 at 5:28 AM, Luca Coelho <l...@coelho.fi> wrote:
> > From: Kees Cook <keesc...@chromium.org>
> > 
> > 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: netdev@vger.kernel.org
> > Signed-off-by: Kees Cook <keesc...@chromium.org>
> > Signed-off-by: Luca Coelho <luciano.coe...@intel.com>
> > ---
> >  drivers/net/wireless/intel/iwlwifi/mvm/mvm.h  |  3 ++-
> >  drivers/net/wireless/intel/iwlwifi/mvm/rxmq.c |  4 ++--
> >  drivers/net/wireless/intel/iwlwifi/mvm/sta.c  | 18 +
> > -
> >  3 files changed, 13 insertions(+), 12 deletions(-)
> 
> Hi,
> 
> Thanks for taking this! I had a question on timing: is this expected
> to land for 4.15? If not, I would like to take this via the timers
> tree, since it is one of the few remaining conversions.

Hi Kees,

Yes, this should land for 4.15.  Kalle just pulled my pull-request
(which includes this) to wireless-drivers-next.  He told me he'll send
a pull-request for 4.15 during this week and hopefully Dave will pull
from him too.

I'll let you know if something doesn't go as planned.

--
Cheers,
Luca.

[PATCH 13/17] iwlwifi: mvm: Convert timers to use timer_setup()

2017-10-29 Thread Luca Coelho 
From: Kees Cook <keesc...@chromium.org>

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: netdev@vger.kernel.org
Signed-off-by: Kees Cook <keesc...@chromium.org>
Signed-off-by: Luca Coelho <luciano.coe...@intel.com>
---
 drivers/net/wireless/intel/iwlwifi/mvm/mvm.h  |  3 ++-
 drivers/net/wireless/intel/iwlwifi/mvm/rxmq.c |  4 ++--
 drivers/net/wireless/intel/iwlwifi/mvm/sta.c  | 18 +-
 3 files changed, 13 insertions(+), 12 deletions(-)

diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/mvm.h 
b/drivers/net/wireless/intel/iwlwifi/mvm/mvm.h
index 2f3d5bef4b9e..0e18c5066f04 100644
--- a/drivers/net/wireless/intel/iwlwifi/mvm/mvm.h
+++ b/drivers/net/wireless/intel/iwlwifi/mvm/mvm.h
@@ -652,6 +652,7 @@ struct iwl_mvm_baid_data {
u16 entries_per_queue;
unsigned long last_rx;
struct timer_list session_timer;
+   struct iwl_mvm_baid_data __rcu **rcu_ptr;
struct iwl_mvm *mvm;
struct iwl_mvm_reorder_buffer reorder_buf[IWL_MAX_RX_HW_QUEUES];
struct iwl_mvm_reorder_buf_entry entries[];
@@ -1853,7 +1854,7 @@ void iwl_mvm_tdls_ch_switch_work(struct work_struct 
*work);
 void iwl_mvm_sync_rx_queues_internal(struct iwl_mvm *mvm,
 struct iwl_mvm_internal_rxq_notif *notif,
 u32 size);
-void iwl_mvm_reorder_timer_expired(unsigned long data);
+void iwl_mvm_reorder_timer_expired(struct timer_list *t);
 struct ieee80211_vif *iwl_mvm_get_bss_vif(struct iwl_mvm *mvm);
 bool iwl_mvm_is_vif_assoc(struct iwl_mvm *mvm);
 
diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/rxmq.c 
b/drivers/net/wireless/intel/iwlwifi/mvm/rxmq.c
index 9852a4d62337..343bdc4266cd 100644
--- a/drivers/net/wireless/intel/iwlwifi/mvm/rxmq.c
+++ b/drivers/net/wireless/intel/iwlwifi/mvm/rxmq.c
@@ -460,9 +460,9 @@ static void iwl_mvm_release_frames(struct iwl_mvm *mvm,
}
 }
 
-void iwl_mvm_reorder_timer_expired(unsigned long data)
+void iwl_mvm_reorder_timer_expired(struct timer_list *t)
 {
-   struct iwl_mvm_reorder_buffer *buf = (void *)data;
+   struct iwl_mvm_reorder_buffer *buf = from_timer(buf, t, reorder_timer);
struct iwl_mvm_baid_data *baid_data =
iwl_mvm_baid_data_from_reorder_buf(buf);
struct iwl_mvm_reorder_buf_entry *entries =
diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/sta.c 
b/drivers/net/wireless/intel/iwlwifi/mvm/sta.c
index 039efcd2735d..c19f98489d4e 100644
--- a/drivers/net/wireless/intel/iwlwifi/mvm/sta.c
+++ b/drivers/net/wireless/intel/iwlwifi/mvm/sta.c
@@ -252,9 +252,11 @@ int iwl_mvm_sta_send_to_fw(struct iwl_mvm *mvm, struct 
ieee80211_sta *sta,
return ret;
 }
 
-static void iwl_mvm_rx_agg_session_expired(unsigned long data)
+static void iwl_mvm_rx_agg_session_expired(struct timer_list *t)
 {
-   struct iwl_mvm_baid_data __rcu **rcu_ptr = (void *)data;
+   struct iwl_mvm_baid_data *data =
+   from_timer(data, t, session_timer);
+   struct iwl_mvm_baid_data __rcu **rcu_ptr = data->rcu_ptr;
struct iwl_mvm_baid_data *ba_data;
struct ieee80211_sta *sta;
struct iwl_mvm_sta *mvm_sta;
@@ -2160,10 +2162,8 @@ static void iwl_mvm_init_reorder_buffer(struct iwl_mvm 
*mvm,
reorder_buf->head_sn = ssn;
reorder_buf->buf_size = buf_size;
/* rx reorder timer */
-   reorder_buf->reorder_timer.function =
-   iwl_mvm_reorder_timer_expired;
-   reorder_buf->reorder_timer.data = (unsigned long)reorder_buf;
-   init_timer(_buf->reorder_timer);
+   timer_setup(_buf->reorder_timer,
+   iwl_mvm_reorder_timer_expired, 0);
spin_lock_init(_buf->lock);
reorder_buf->mvm = mvm;
reorder_buf->queue = i;
@@ -2286,9 +2286,9 @@ int iwl_mvm_sta_rx_agg(struct iwl_mvm *mvm, struct 
ieee80211_sta *sta,
baid_data->baid = baid;
baid_data->timeout = timeout;
baid_data->last_rx = jiffies;
-   setup_timer(_data->session_timer,
-   iwl_mvm_rx_agg_session_expired,
-   (unsigned long)

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

2017-10-24 Thread 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: netdev@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.

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

2017-09-07 Thread 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

Re: [GIT] Networking

2017-09-06 Thread 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: [PATCH] iwlwifi: Demote messages about fw flags size to info

2017-08-01 Thread 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()'

2017-07-19 Thread 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

2017-06-29 Thread 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: fix iwl_mvm_sar_find_wifi_pkg corner case

2017-06-28 Thread 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

2017-06-28 Thread 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

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

2017-01-09 Thread 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: [PATCH 5/8] linux: drop __bitwise__ everywhere

2016-12-19 Thread Luca Coelho 
On Thu, 2016-12-15 at 07:15 +0200, Michael S. Tsirkin wrote:
> __bitwise__ used to mean "yes, please enable sparse checks
> unconditionally", but now that we dropped __CHECK_ENDIAN__
> __bitwise is exactly the same.
> There aren't many users, replace it by __bitwise everywhere.
> 
> Signed-off-by: Michael S. Tsirkin <m...@redhat.com>
> ---
>  arch/arm/plat-samsung/include/plat/gpio-cfg.h| 2 +-
>  drivers/md/dm-cache-block-types.h| 6 +++---
>  drivers/net/ethernet/sun/sunhme.h| 2 +-
>  drivers/net/wireless/intel/iwlwifi/iwl-fw-file.h | 4 ++--

For drivers/net/wireless/intel/iwlwifi/iwl-fw-file.h:

Acked-by: Luca Coelho <luciano.coe...@intel.com>

--
Luca.

Re: [PATCH 8/8] Makefile: drop -D__CHECK_ENDIAN__ from cflags

2016-12-19 Thread Luca Coelho 
On Thu, 2016-12-15 at 07:15 +0200, Michael S. Tsirkin wrote:
> That's the default now, no need for makefiles to set it.
> 
> Signed-off-by: Michael S. Tsirkin <m...@redhat.com>
> ---
>  drivers/bluetooth/Makefile| 2 --
>  drivers/net/can/Makefile  | 1 -
>  drivers/net/ethernet/altera/Makefile  | 1 -
>  drivers/net/ethernet/atheros/alx/Makefile | 1 -
>  drivers/net/ethernet/freescale/Makefile   | 2 --
>  drivers/net/wireless/ath/Makefile | 2 --
>  drivers/net/wireless/ath/wil6210/Makefile | 2 --
>  drivers/net/wireless/broadcom/brcm80211/brcmfmac/Makefile | 2 --
>  drivers/net/wireless/broadcom/brcm80211/brcmsmac/Makefile | 1 -
>  drivers/net/wireless/intel/iwlegacy/Makefile  | 2 --
>  drivers/net/wireless/intel/iwlwifi/Makefile   | 2 +-
>  drivers/net/wireless/intel/iwlwifi/dvm/Makefile   | 2 +-
>  drivers/net/wireless/intel/iwlwifi/mvm/Makefile   | 2 +-

For the drivers/net/wireless/intel/iwlwifi/ part:

Acked-by: Luca Coelho <luciano.coe...@intel.com>

--
Luca.

Re: Intel Wireless 7260 Microcode SW error detected

2016-10-19 Thread Luca Coelho 
Hi Tibor,

On Sun, 2016-10-16 at 20:08 +0200, Billes Tibor wrote:
> I have Lenovo laptop with an Intel Wireless 7260 wifi module and the 
> latest stable kernel 4.8.1. I was playing a movie from an sshfs mounted 
> file system, when the movie just froze and my network stopped working. 
> Dmesg said 'Microcode SW error detected'. Below is my full `dmesg -r` 
> output. I probably will not be able to reproduce this behaviour, because 
> it did not happen in the past, and it seems to work fine after reboot.

Can you file a bug in bugzilla so we can track this? It seems to be a
firmware problem that we'll have to investigate.  We have instructions
on how to report here:

https://wireless.wiki.kernel.org/en/users/drivers/iwlwifi/debugging


> Is there anything more that I can do to help you debug this?

It would be nice if you could enable firmware debugging so that, if
this happens again, we'll have the appropriate FW logs to investigate
it further.  There are instructions on how to enable it in the
"Firmware Debugging" section of that wiki page.

Please make sure you read and understand the privacy aspects of
submitting firmware debugging data, as explained here:

https://wireless.wiki.kernel.org/en/users/drivers/iwlwifi/debugging#privacy_aspects


> <3>[37750.190201] iwlwifi :08:00.0: Microcode SW error detected.  
> Restarting 0x200.
> <3>[37750.190206] iwlwifi :08:00.0: CSR values:
> <3>[37750.190207] iwlwifi :08:00.0: (2nd byte of CSR_INT_COALESCING 
> is CSR_INT_PERIODIC_REG)
> <3>[37750.190212] iwlwifi :08:00.0: CSR_HW_IF_CONFIG_REG: 0X40489204
> <3>[37750.190226] iwlwifi :08:00.0: CSR_INT_COALESCING: 0X8040
> <3>[37750.190239] iwlwifi :08:00.0: CSR_INT: 0X
> <3>[37750.190253] iwlwifi :08:00.0: CSR_INT_MASK: 0X
> <3>[37750.190267] iwlwifi :08:00.0: CSR_FH_INT_STATUS: 0X
> <3>[37750.190281] iwlwifi :08:00.0: CSR_GPIO_IN: 0X
> <3>[37750.190294] iwlwifi :08:00.0: CSR_RESET: 0X
> <3>[37750.190308] iwlwifi :08:00.0: CSR_GP_CNTRL: 0X080403c5
> <3>[37750.190322] iwlwifi :08:00.0: CSR_HW_REV: 0X0144
> <3>[37750.190336] iwlwifi :08:00.0: CSR_EEPROM_REG: 0X
> <3>[37750.190350] iwlwifi :08:00.0: CSR_EEPROM_GP: 0X8000
> <3>[37750.190363] iwlwifi :08:00.0: CSR_OTP_GP_REG: 0X803a
> <3>[37750.190377] iwlwifi :08:00.0: CSR_GIO_REG: 0X001f0042
> <3>[37750.190391] iwlwifi :08:00.0: CSR_GP_UCODE_REG: 0X
> <3>[37750.190405] iwlwifi :08:00.0: CSR_GP_DRIVER_REG: 0X
> <3>[37750.190419] iwlwifi :08:00.0: CSR_UCODE_DRV_GP1: 0X
> <3>[37750.190432] iwlwifi :08:00.0: CSR_UCODE_DRV_GP2: 0X
> <3>[37750.190446] iwlwifi :08:00.0: CSR_LED_REG: 0X0060
> <3>[37750.190460] iwlwifi :08:00.0: CSR_DRAM_INT_TBL_REG: 0X881530a3
> <3>[37750.190474] iwlwifi :08:00.0: CSR_GIO_CHICKEN_BITS: 0X27800200
> <3>[37750.190488] iwlwifi :08:00.0: CSR_ANA_PLL_CFG: 0Xd5d5
> <3>[37750.190501] iwlwifi :08:00.0: CSR_MONITOR_STATUS_REG: 0X3d0801bd
> <3>[37750.190515] iwlwifi :08:00.0: CSR_HW_REV_WA_REG: 0X0001001a
> <3>[37750.190529] iwlwifi :08:00.0: CSR_DBG_HPET_MEM_REG: 0X0010
> <3>[37750.190530] iwlwifi :08:00.0: FH register values:
> <3>[37750.190552] iwlwifi :08:00.0: FH_RSCSR_CHNL0_STTS_WPTR_REG: 
> 0X15005f00
> <3>[37750.190566] iwlwifi :08:00.0: FH_RSCSR_CHNL0_RBDCB_BASE_REG: 
> 0X015005e0
> <3>[37750.190580] iwlwifi :08:00.0: FH_RSCSR_CHNL0_WPTR: 0X00e8
> <3>[37750.190593] iwlwifi :08:00.0: FH_MEM_RCSR_CHNL0_CONFIG_REG: 
> 0X80801114
> <3>[37750.190607] iwlwifi :08:00.0: FH_MEM_RSSR_SHARED_CTRL_REG: 
> 0X00fc
> <3>[37750.190621] iwlwifi :08:00.0: FH_MEM_RSSR_RX_STATUS_REG: 
> 0X0703
> <3>[37750.190635] iwlwifi :08:00.0: 
> FH_MEM_RSSR_RX_ENABLE_ERR_IRQ2DRV: 0X
> <3>[37750.190648] iwlwifi :08:00.0: FH_TSSR_TX_STATUS_REG: 0X07ff0001
> <3>[37750.190662] iwlwifi :08:00.0: FH_TSSR_TX_ERROR_REG: 0X
> <3>[37750.190790] iwlwifi :08:00.0: Start IWL Error Log Dump:
> <3>[37750.190791] iwlwifi :08:00.0: Status: 0x, count: 6
> <3>[37750.190793] iwlwifi :08:00.0: Loaded firmware version: 16.242414.0

I recommend that you update your firmware to the latest version
(iwlwifi-7260-17.ucode) published in linux-firmware.git:

http://git.kernel.org/cgit/linux/kernel/git/firmware/linux-firmware.git

While I'm not sure the issue you saw is solved by that, we do have lots
of other fixes in it that are worth the effort.


<3>[37750.190794] iwlwifi :08:00.0: 0x307C | ADVANCED_SYSASSERT

I hadn't seen this for a while.  This is something related to the TX
queues and could have many different causes...

Thanks for reporting!

--
Cheers,
Luca.

Re: linux-4.9-rc1/drivers/net/wireless/intel/iwlwifi/mvm/debugfs.c:1561: poor error checking ?

2016-10-17 Thread Luca Coelho 
Hi David,
On Mon, 2016-10-17 at 07:40 +, David Binderman wrote:
> Hello there,
> 
> linux-4.9-rc1/drivers/net/wireless/intel/iwlwifi/mvm/debugfs.c:1561]: (style) 
> Checking if unsigned variable 'len' is less than zero.
> 
> Source code is
> 
> len = min((size_t)le32_to_cpu(rsp->len) << 2,
>   iwl_rx_packet_payload_len(hcmd.resp_pkt) - sizeof(*rsp));
> len = min(len - delta, count);
> if (len < 0) {
> ret = -EFAULT;
> goto out;
> }
> 
> Suggest improve error checking.

Thanks for reporting! A fix for this is already queued in our internal
tree and will be sent upstream soon.

--
Cheers,
Luca.

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 <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!
> :-)

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

--
Luca.

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 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 <pebo...@tiscali.nl>
> 
> 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.

[PATCH] iwlwifi: pcie: fix SPLC structure parsing

2016-10-13 Thread Luca Coelho 
From: Luca Coelho <luciano.coe...@intel.com>

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 <ch...@rorvick.com>
Signed-off-by: Luca Coelho <luciano.coe...@intel.com>
---

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_bu

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

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

2016-10-12 Thread Luca Coelho 
On Wed, 2016-10-12 at 14:36 +0200, Paul Bolle wrote:
> 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.

Thanks! Yeah, I implied any of you two. ;)


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

No problem, these BIOSes are usually quite cryptic. :) But what you're
saying makes sense.  They have added the SPLC method to all PCI root-
ports (which is what RP stands for here).

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.

Basically this tells me that indeed 3 "structs" are being returned (as
your dumps already showed).  And, according to the specs that I found
(which unfortunately are confidential, so I can't share) this is
correct and the driver code is broken.

I'll send you a patch for testing soon.

Thanks for all the help!

--
Cheers,
Luca.

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

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

2016-10-12 Thread Luca Coelho 
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.

I will also check what is the correct procedure in such cases, because
it is possible, in theory, that the format *matches* but applies only
to another device.


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

Yes, so I'll try to stop wasting people's timing by trying to do the
correct thing without bothering the user at all. :)

Thanks for pointing this all out!

--
Cheers,
Luca.

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

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

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

Re: [PATCH RESEND] iwlwifi, Do not implement thermal zone unless ucode is loaded

2016-07-13 Thread Luca Coelho 
On Wed, 2016-07-13 at 09:50 +0300, Kalle Valo wrote:
> Prarit Bhargava  writes:
> 
> > > We implement thermal zone because we do support it, but the
> > > problem is
> > > that we need the firmware to be loaded for that. So you can argue
> > > that
> > > we should register *later* when the firmware is loaded. But this
> > > is
> > > really not helping all that much because the firmware can also be
> > > stopped at any time. So you'd want us to register / unregister
> > > the
> > > thermal zone anytime the firmware is loaded / unloaded?
> > 
> > You might have to do that.  I think that if the firmware enables a
> > feature then
> > the act of loading the firmware should run the code that enables
> > the feature.
> > IMO of course.
> 
> But I suspect that the iwlwifi firmware is loaded during interface up
> (and unloaded during interface down) and in that case
> register/unregister would be happening all the time. That doesn't
> sound
> like a good idea. I would rather try to fix the thermal interface to
> handle the cases when the measurement is not available.

I totally agree with Emmanuel and Kalle.  We should not change this.
 It is a design decision to return an error when the interface is down,
this is very common with other subsystems as well.  The userspace
should be able to handle errors and report something like "unavailable"
when this kind of error is returned.

I'm not sure EIO is the best we can have, but for me that's exactly
what it is.  The thermal zone *is* there, but cannot be accessed
because the firmware is not available.  I'm okay to change it to EBUSY,
if that would help userspace, but I think that's a bit misleading.  The
device is not busy, on the contrary, it's not even running at all.

Furthermore, I don't think this is "breaking userspace" in the sense of
being a regression.  The userspace API has always been implemented with
the possibility of returning errors.  It's not a good design if a
single device returning an error causes all the other devices to also
fail.

--
Cheers,
Luca.

Re: [PATCH] iwlwifi: add missing type declaration

2016-07-12 Thread Luca Coelho 
On Mon, 2016-07-11 at 22:49 +0200, Arnd Bergmann wrote:
> The iwl-debug.h header relies in implicit inclusion of linux/device.h
> and
> we get a lot of warnings without that:
> 
> drivers/net/wireless/intel/iwlwifi/iwl-debug.h:44:23: error: 'struct
> device' declared inside parameter list will not be visible outside of
> this definition or declaration [-Werror]
>  void __iwl_err(struct device *dev, bool rfkill_prefix, bool
> only_trace,
>    ^~
> In file included from drivers/net/wireless/intel/iwlwifi/iwl-eeprom-
> read.h:66:0,
>  from drivers/net/wireless/intel/iwlwifi/iwl-eeprom-
> read.c:68:
> drivers/net/wireless/intel/iwlwifi/iwl-trans.h: In function
> 'iwl_trans_tx':
> drivers/net/wireless/intel/iwlwifi/iwl-trans.h:1030:348: error:
> passing argument 1 of '__iwl_err' from incompatible pointer type [-
> Werror=incompatible-pointer-types]
>    IWL_ERR(trans, "%s bad state = %d\n", __func__, trans->state);
>  
>  
>  
>  
>  
>    ^
> In file included from drivers/net/wireless/intel/iwlwifi/iwl-eeprom-
> read.c:67:0:
> drivers/net/wireless/intel/iwlwifi/iwl-debug.h:44:6: note: expected
> 'struct device *' but argument is of type 'struct device *'
>  void __iwl_err(struct device *dev, bool rfkill_prefix, bool
> only_trace,
>   ^
> 
> The easiest workaround is to just declare 'struct device' before its
> first use,
> rather than including the entire header file.
> 
> Signed-off-by: Arnd Bergmann <a...@arndb.de>
> Fixes: 21cb3222fe56 ("iwlwifi: decouple PCIe transport from
> mac80211")
> ---

Acked-by: Luca Coelho <luciano.coe...@intel.com>

Agree with Kalle that he will take this directly to wireless-drivers-
next.

--
Cheers,
Luca.

Re: [PATCH] iwlwifi: Remove unused array 'iwlagn_loose_lookup'

2016-06-06 Thread Luca Coelho 
On Fri, 2016-06-03 at 14:39 -0700, Guenter Roeck wrote:
> gcc-6 reports the following error if -Werror=unused-const-variable
> is enabled.
> 
> drivers/net/wireless/intel/iwlwifi/dvm/lib.c:210:21: error:
>   'iwlagn_loose_lookup' defined but not used
> 
> Signed-off-by: Guenter Roeck 
> ---

Thanks! I'm queuing this via our internal trees (adding "dvm: " to the
subject, to indicate this is related to the dvm module).

--
Cheers,
Luca.

[PATCH] iwlwifi: mvm: fix merge damage in tx.c

2016-05-18 Thread Luca Coelho 
From: Luca Coelho <luciano.coe...@intel.com>

During the merge in commit 909b27f70643 ("Merge
git://git.kernel.org/pub/scm/linux/kernel/git/davem/net"), there was a
small merge damage where one instance of info was not converted into
skb_info.  Fix this.

Signed-off-by: Luca Coelho <luciano.coe...@intel.com>
---
 drivers/net/wireless/intel/iwlwifi/mvm/tx.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/tx.c 
b/drivers/net/wireless/intel/iwlwifi/mvm/tx.c
index 8802109..c53aa0f 100644
--- a/drivers/net/wireless/intel/iwlwifi/mvm/tx.c
+++ b/drivers/net/wireless/intel/iwlwifi/mvm/tx.c
@@ -211,6 +211,7 @@ void iwl_mvm_set_tx_cmd(struct iwl_mvm *mvm, struct sk_buff 
*skb,
struct iwl_tx_cmd *tx_cmd,
struct ieee80211_tx_info *info, u8 sta_id)
 {
+   struct ieee80211_tx_info *skb_info = IEEE80211_SKB_CB(skb);
struct ieee80211_hdr *hdr = (void *)skb->data;
__le16 fc = hdr->frame_control;
u32 tx_flags = le32_to_cpu(tx_cmd->tx_flags);
@@ -294,7 +295,7 @@ void iwl_mvm_set_tx_cmd(struct iwl_mvm *mvm, struct sk_buff 
*skb,
tx_cmd->tx_flags = cpu_to_le32(tx_flags);
/* Total # bytes to be transmitted */
tx_cmd->len = cpu_to_le16((u16)skb->len +
-   (uintptr_t)info->driver_data[0]);
+   (uintptr_t)skb_info->driver_data[0]);
tx_cmd->life_time = cpu_to_le32(TX_CMD_LIFE_TIME_INFINITE);
tx_cmd->sta_id = sta_id;
 
-- 
2.8.1

Re: iwlwifi: mvm: add reorder buffer per queue

2016-05-16 Thread Luca Coelho 
On Fri, 2016-05-13 at 11:54 +0300, Dan Carpenter wrote:
> Hello Sara Sharon,
> 
> The patch b915c10174fb: "iwlwifi: mvm: add reorder buffer per queue"
> from Mar 23, 2016, leads to the following static checker warnings:
> 
>   drivers/net/wireless/intel/iwlwifi/mvm/rxmq.c:912
> iwl_mvm_rx_mpdu_mq()
>   error: potential NULL dereference 'sta'.
> 
>   drivers/net/wireless/intel/iwlwifi/mvm/rxmq.c:912
> iwl_mvm_rx_mpdu_mq()
>   error: we previously assumed 'sta' could be null (see line 796)

Thanks for the analysis and report, Dan!

I have queued a fix for this through our internal tree.

--
Cheers,
Luca.