Re: [PATCH] mac80211: rewrite Kconfig text for mesh

[Steve deRosier]

On Tue, Dec 4, 2018 at 7:25 AM Bob Copeland  wrote:
>
> Lubomir Rintel recently pointed out a dead link for o11s.org, and
> repointed it to a still live, but also stale website.  As far as I
> know, no one is updating the content at open80211s.org.
>

Indeed.  The o11s effort more or less ended after cozybit finished
writing and upstreamed the mesh code. A little maintenance and changes
happened via o11s until the 802.11s stuff was ratified, in, stable,
etc...  including by me and others at cozybit. You are correct -
AFAIK, no one is updating that site, o11s.org is down and actually
cozybit as an entity is no more either. And all of us are long
elsewhere.

> Since this Kconfig text was originally written, though, the 802.11s
> mesh drafts were approved and ultimately rolled into 802.11 proper.
> Meanwhile, the implementation has converged on the final standard,
> so we can lose all of the text here and provide something that's a
> little more helpful and accurate.
>
> Signed-off-by: Bob Copeland 
> ---
>  net/mac80211/Kconfig | 11 +--
>  1 file changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/net/mac80211/Kconfig b/net/mac80211/Kconfig
> index f869e35d0974..6b09355dfc90 100644
> --- a/net/mac80211/Kconfig
> +++ b/net/mac80211/Kconfig
> @@ -57,14 +57,13 @@ comment "Some wireless drivers require a rate control 
> algorithm"
> depends on MAC80211 && MAC80211_HAS_RC=n
>
>  config MAC80211_MESH
> -   bool "Enable mac80211 mesh networking (pre-802.11s) support"
> +   bool "Enable mac80211 mesh networking support"
> depends on MAC80211
> ---help---
> -This options enables support of Draft 802.11s mesh networking.
> -The implementation is based on Draft 2.08 of the Mesh Networking
> -amendment.  However, no compliance with that draft is claimed or even
> -possible, as drafts leave a number of identifiers to be defined after
> -ratification.  For more information visit http://o11s.org/.
> + Select this option to enable 802.11 mesh operation in mac80211
> + drivers that support it.  802.11 mesh connects multiple stations
> + over (possibly multi-hop) wireless links to form a single logical
> + LAN.
>

Looks good to me.

Reviewed-by: Steve deRosier 

- Steve

Re: Where to report mpdus tx vs failed?

[Steve deRosier]

On Fri, Oct 19, 2018 at 10:34 AM Ben Greear  wrote:
>
> While debugging rate-ctrl in ath10k, I found the amount of mpdus transmitted 
> vs failed
> ratio useful.  Probably more useful than retries since retries could count an 
> attempt at
> 80Mhz followed by HW trying a 40Mhz rate (afaik).
>
> Is there a good way to report this up the stack in a useful manner?  I 
> currently only
> get this stat for the first frame in an transmitted ampdu.
>

debugfs? Is it useful for someone working on it, like a sysadmin or
kernel programmer?  Or is it useful for programs to know?  In the
first case I'd say debugfs, in the second case I'd suggest it goes in
some tx stats structure that is reported by netlink.

Also, is it something that is given by (or should be given by) all
drivers, or is it very driver-specific?

- Steve

Re: list archive

[Steve deRosier]

On Tue, Sep 25, 2018 at 12:22 AM Johannes Berg
 wrote:
>
> Hi all,
>
> Konstantin has graciously added this list to lore, so we now have a web
> archive here:
>
> https://lore.kernel.org/linux-wireless
>

That is fantastic! Thank you Konstantin, Johannes, and Jouni! While
the development on this list is great,
it's these sort of toolings that make it easier to work on linux-wireless.

- Steve

Re: Build iw for 32bit ARM

[Steve deRosier]

Hi Jakov,

On Fri, Aug 31, 2018 at 5:59 AM Jakov Simunic
 wrote:
>
> Hello,
> I am working on an embedded project which uses an armv7l architecture, which 
> is a 32bit platform, and I am trying to compile iw-4.9 for it, and I don't 
> know how to compile it for 32bit arches.

Unless you're building your Linux system 100% from scratch, you should
leverage the build system for your platform. It will automatically do
the right thing. You're likely using Buildroot, OpenWRT, OpenEmbedded,
Yocto or some other platform that has compilers, build systems,
packages and so on. First off, most likely iw is already included. And
even if you want to build from your own modified version, theres ways
to build using the systems above to just build it in-tree. And
finally, even if not, you should use the target libraries and the
cross-compilers that are used by your platform.

Step one, if you haven't already, is to get the build system for your
platform and configure and build it. Then try building your modified
version of iw if the stock one isn't helpful for you. I'd integrate it
into the platform build system, but you could build out-of-tree by
setting up the right flags and CROSS_COMPILER to point to the
toolchain used for your platform. If I were doing that, I'd build the
platform in verbose output mode and examine the compiler commandlines
for hints.

> Tried adding -m32 to the CFLAGS, everything passes the build except the iw 
> binary, which says:
>
> iw.o: could not read symbols: File in wrong format
> collect2: ld returned 1 exit status
> make: *** [iw] Error 1
>

Most likely your architecture flags to the linker don't match what you
gave in the compiler stage. You could always also confirm the format
of iw.o with the `file` command.

- Steve

Re: [PATCH v2] ath6kl: mark expected switch fall-throughs

[Steve deRosier]

On Fri, May 25, 2018 at 11:23 AM Gustavo A. R. Silva
<gust...@embeddedor.com>
wrote:

> In preparation to enabling -Wimplicit-fallthrough, mark switch cases
> where we are expecting to fall through.

> Signed-off-by: Gustavo A. R. Silva <gust...@embeddedor.com>
> ---
> Changes in v2:
>- Place code comments on a line of their own.

>drivers/net/wireless/ath/ath6kl/cfg80211.c | 3 +++
>1 file changed, 3 insertions(+)

> diff --git a/drivers/net/wireless/ath/ath6kl/cfg80211.c
b/drivers/net/wireless/ath/ath6kl/cfg80211.c
> index 2ba8cf3..a16ee5d 100644
> --- a/drivers/net/wireless/ath/ath6kl/cfg80211.c
> +++ b/drivers/net/wireless/ath/ath6kl/cfg80211.c
> @@ -3899,16 +3899,19 @@ int ath6kl_cfg80211_init(struct ath6kl *ar)
>   switch (ar->hw.cap) {
>   case WMI_11AN_CAP:
>   ht = true;
> +   /* fall through */
>   case WMI_11A_CAP:
>   band_5gig = true;
>   break;
>   case WMI_11GN_CAP:
>   ht = true;
> +   /* fall through */
>   case WMI_11G_CAP:
>   band_2gig = true;
>   break;
>   case WMI_11AGN_CAP:
>   ht = true;
> +   /* fall through */
>   case WMI_11AG_CAP:
>   band_2gig = true;
>   band_5gig = true;
> --
> 2.7.4


Gustavo,

Thanks for the adjustment.  It now looks good to me.

Reviewed-by: Steve deRosier <deros...@cal-sierra.com>

Re: [PATCH] Added Dell Wireless 1537 Ath6kl sdio card (0x19) to the list.

[Steve deRosier]

Hi Guy,

On Mon, May 21, 2018 at 9:28 AM Guy Chronister 
wrote:

> Signed-off-by: Guy Chronister 
> ---
>   drivers/net/wireless/ath/ath6kl/sdio.c | 1 +
>   1 file changed, 1 insertion(+)

> diff --git a/drivers/net/wireless/ath/ath6kl/sdio.c
b/drivers/net/wireless/ath/ath6kl/sdio.c
> index 2195b1b7a8a6..bb50680580f3 100644
> --- a/drivers/net/wireless/ath/ath6kl/sdio.c
> +++ b/drivers/net/wireless/ath/ath6kl/sdio.c
> @@ -1415,6 +1415,7 @@ static const struct sdio_device_id
ath6kl_sdio_devices[] = {
>  {SDIO_DEVICE(MANUFACTURER_CODE, (MANUFACTURER_ID_AR6004_BASE |
0x1))},
>  {SDIO_DEVICE(MANUFACTURER_CODE, (MANUFACTURER_ID_AR6004_BASE |
0x2))},
>  {SDIO_DEVICE(MANUFACTURER_CODE, (MANUFACTURER_ID_AR6004_BASE |
0x18))},
> +   {SDIO_DEVICE(MANUFACTURER_CODE, (MANUFACTURER_ID_AR6004_BASE |
0x19))},
>  {},
>   };


Thanks for this. I'd like to see more information in the git commit
message. While I get that this is some Dell card, it would be helpful to
know what is the actual chip, which firmware, sdio id, and what tests were
done to confirm this worked. Honestly I'm a bit surprised to see a new
ath6k chip ID.

Thanks,
- Steve

Re: [PATCH] ath6kl: use WMI to set RSN capabilities

[Steve deRosier]

On Fri, Apr 27, 2018 at 9:57 AM Alfonso Sanchez-Beato <
alfonso.sanchez-be...@canonical.com> wrote:

> Hi Steve,

> On Fri, Apr 27, 2018 at 5:47 PM, Steve deRosier <deros...@gmail.com>
wrote:
> > Hi Alfonso,
> >
> > On Thu, Apr 12, 2018 at 8:42 AM Alfonso Sánchez-Beato <
> > alfonso.sanchez-be...@canonical.com> wrote:
> >
> >> This fixes AP mode when the ATH6KL_FW_CAPABILITY_RSN_CAP_OVERRIDE flag
> >> is not present in the FW. The id of some WMI commands is also fixed
> >> (there was an error in the enum order), and a function to set RSN caps
> >> is added.
> >
> >> Signed-off-by: Alfonso Sánchez-Beato <
alfonso.sanchez-be...@canonical.com>
> >> ---
> >>   drivers/net/wireless/ath/ath6kl/cfg80211.c | 21 ++---
> >>   drivers/net/wireless/ath/ath6kl/main.c | 10 +++---
> >>   drivers/net/wireless/ath/ath6kl/wmi.c  | 23
+++
> >>   drivers/net/wireless/ath/ath6kl/wmi.h  | 15 +++
> >>   4 files changed, 43 insertions(+), 26 deletions(-)
> >
> >> diff --git a/drivers/net/wireless/ath/ath6kl/cfg80211.c
> > b/drivers/net/wireless/ath/ath6kl/cfg80211.c
> >> index 2ba8cf3f38af..1b89c53d47e7 100644
> >> --- a/drivers/net/wireless/ath/ath6kl/cfg80211.c
> >> +++ b/drivers/net/wireless/ath/ath6kl/cfg80211.c
> >> @@ -2933,13 +2933,11 @@ static int ath6kl_start_ap(struct wiphy *wiphy,
> > struct net_device *dev,
> >>   * advertise the same in beacon/probe response. Send
> >>   * the complete RSN IE capability field to firmware
> >>   */
> >> -   if (!ath6kl_get_rsn_capab(>beacon, (u8 *) _capab) &&
> >> -   test_bit(ATH6KL_FW_CAPABILITY_RSN_CAP_OVERRIDE,
> >> -ar->fw_capabilities)) {
> >> -   res = ath6kl_wmi_set_ie_cmd(ar->wmi, vif->fw_vif_idx,
> >> -   WLAN_EID_RSN,
WMI_RSN_IE_CAPB,
> >> -   (const u8 *) _capab,
> >> -   sizeof(rsn_capab));
> >> +   if (!ath6kl_get_rsn_capab(>beacon, (u8 *)_capab)) {
> >> +   ath6kl_dbg(ATH6KL_DBG_WLAN_CFG, "RSN caps %d\n",
> > rsn_capab);
> >> +
> >> +   res = ath6kl_wmi_set_rsn_cap(ar->wmi, vif->fw_vif_idx,
> >> +rsn_capab);
> >>  vif->rsn_capab = rsn_capab;
> >>  if (res < 0)
> >>  return res;
> >
> > So, let me see if I understand this correctly.  Original flow was:
> >
> > 1. get RSN capable from the beacon if one
> > 2. if the firmware is capable of the override, set the IE  else don't do
> > anything
> >
> > New flow:
> > 1. get RSN capable from the beacon if one
> > 2. unconditionally send the rsn_cap WMI down
> >
> > So, what happens on a firmware that _does_ have the rsn-cap-override
flag
> > set? I'm guessing nothing good.  Admittedly I haven't tried your patch
on
> > my platform.
> >
> > I think that simply ignoring the flag and using a WMI instead of setting
> > the IE on all devices AR6002..AR6004 is likely going to cause problems
for
> > everyone else.

> Admittedly, I have not a wide range of devices to test. This patch was
> mostly an RFC to see if the issue is only for a particular fw revision
> and to expose it for people that might find it useful. Note that I am
> not the first person to hit this, see for instance

> http://www.spinics.net/lists/linux-wireless/msg115085.html


Fair enough. However, breaking it for the rest of the devices out there
isn't good. I welcome the fixing of it, but it has to be done across the
spectrum of the supported chips.



> >
> >
> >> @@ -3918,14 +3916,7 @@ int ath6kl_cfg80211_init(struct ath6kl *ar)
> >>  return -EINVAL;
> >>  }
> >
> >> -   /*
> >> -* Even if the fw has HT support, advertise HT cap only when
> >> -* the firmware has support to override RSN capability,
otherwise
> >> -* 4-way handshake would fail.
> >> -*/
> >> -   if (!(ht &&
> >> - test_bit(ATH6KL_FW_CAPABILITY_RSN_CAP_OVERRIDE,
> >> -  ar->fw_capabilities))) {
> >> +   if (!ht) {
> >
> > Perhaps the comment isn't relevant if we're using the RSN WMI command
on a
> > devi

Re: [PATCH] ath6kl: use WMI to set RSN capabilities

[Steve deRosier]

gt;wmi, vif->fw_vif_idx,
> +  vif->rsn_capab);


So, basically same comment as the first one.

>  return ath6kl_wmi_ap_profile_commit(ar->wmi,
vif->fw_vif_idx,
>  >profile);
> diff --git a/drivers/net/wireless/ath/ath6kl/wmi.c
b/drivers/net/wireless/ath/ath6kl/wmi.c
> index 777acc564ac9..d7de9224d755 100644
> --- a/drivers/net/wireless/ath/ath6kl/wmi.c
> +++ b/drivers/net/wireless/ath/ath6kl/wmi.c
> @@ -4168,3 +4168,26 @@ void ath6kl_wmi_shutdown(struct wmi *wmi)
>  kfree(wmi->last_mgmt_tx_frame);
>  kfree(wmi);
>   }
> +
> +int ath6kl_wmi_set_rsn_cap(struct wmi *wmi, u8 if_idx, u16 rsn_cap)
> +{
> +   struct sk_buff *skb;
> +   struct wmi_rsn_cap_cmd *cmd;
> +
> +   skb = ath6kl_wmi_get_new_buf(sizeof(*cmd));
> +   if (!skb)
> +   return -ENOMEM;
> +
> +   ath6kl_dbg(ATH6KL_DBG_WMI, "set_rsn_cap: 0x%04x\n", rsn_cap);
> +
> +   cmd = (struct wmi_rsn_cap_cmd *)skb->data;
> +   cmd->rsn_cap = cpu_to_le16(rsn_cap);
> +
> +   return ath6kl_wmi_cmd_send(wmi, if_idx, skb,
WMI_SET_RSN_CAP_CMDID,
> +  NO_SYNC_WMIFLAG);
> +}
> +
> +int ath6kl_wmi_get_rsn_cap(struct wmi *wmi, u8 if_idx)
> +{
> +   return ath6kl_wmi_simple_cmd(wmi, if_idx, WMI_GET_RSN_CAP_CMDID);
> +}
> diff --git a/drivers/net/wireless/ath/ath6kl/wmi.h
b/drivers/net/wireless/ath/ath6kl/wmi.h
> index a60bb49fe920..011d4b6fb5ab 100644
> --- a/drivers/net/wireless/ath/ath6kl/wmi.h
> +++ b/drivers/net/wireless/ath/ath6kl/wmi.h
> @@ -597,10 +597,6 @@ enum wmi_cmd_id {

>  WMI_GREENTX_PARAMS_CMDID,

> -   WMI_RTT_MEASREQ_CMDID,
> -   WMI_RTT_CAPREQ_CMDID,
> -   WMI_RTT_STATUSREQ_CMDID,
> -
>  /* WPS Commands */
>  WMI_WPS_START_CMDID,
>  WMI_GET_WPS_STATUS_CMDID,
> @@ -621,6 +617,10 @@ enum wmi_cmd_id {
>  WMI_RX_FILTER_COALESCE_FILTER_OP_CMDID,
>  WMI_RX_FILTER_SET_FRAME_TEST_LIST_CMDID,

> +   WMI_RTT_MEASREQ_CMDID,
> +   WMI_RTT_CAPREQ_CMDID,
> +   WMI_RTT_STATUSREQ_CMDID,
> +
>  WMI_SEND_MGMT_CMDID,
>  WMI_BEGIN_SCAN_CMDID,

> @@ -2533,6 +2533,10 @@ enum wmi_sync_flag {
>  END_WMIFLAG
>   };


I can factually state that the above reordering of the commands is wrong
for a 6003. The order in the WMI file is consistent for a 6003. Your
reordering is consistent for a 6004. Now, the only commands affected by the
reordering aren't utilized by the driver, other than your added get/set
RSN_CAP_CMDIDs.  But on a 6003, your now used WMI_SET_RSN_CAP_CMDID will
trigger a WMI_GET_OPPPS_CMDID, which isn't what you want.

I've encountered this issue a before - wmi code mismatch between the
different firmwares and the driver. This is a limitation of the driver that
probably should be resolved in some meaningful way. To date, it's been
mitigated by the driver just not using the higher-numbered commands.  But
by "meaningful way" I don't include redefining command IDs in favor of one
particular person's firmware and causing problems on the other 99% of
devices out there.

> +struct wmi_rsn_cap_cmd {
> +   __le16 rsn_cap;
> +} __packed;
> +
>   enum htc_endpoint_id ath6kl_wmi_get_control_ep(struct wmi *wmi);
>   void ath6kl_wmi_set_control_ep(struct wmi *wmi, enum htc_endpoint_id
ep_id);
>   int ath6kl_wmi_dix_2_dot3(struct wmi *wmi, struct sk_buff *skb);
> @@ -2728,4 +2732,7 @@ void *ath6kl_wmi_init(struct ath6kl *devt);
>   void ath6kl_wmi_shutdown(struct wmi *wmi);
>   void ath6kl_wmi_reset(struct wmi *wmi);

> +int ath6kl_wmi_set_rsn_cap(struct wmi *wmi, u8 if_idx, u16 rsn_cap);
> +int ath6kl_wmi_get_rsn_cap(struct wmi *wmi, u8 if_idx);
> +
>   #endif /* WMI_H */
> --
> 2.14.1


 From your answer to Kalle re: what hardware

> I have tested this in an Atheros QCA6234. kmsg shows this about the fw:

> ath6kl: ar6004 hw 3.0 sdio fw 3.5.0.604 api 1
> ath6kl: firmware supports: 64bit-rates,ap-inactivity-mins

The firmware you're using is old. Mine for the QCA6234 is more advanced
than that and has the rsn-cap-override flag, but even the stock one in
linux-firmware is more up-to-date: 3.5.0.2356 api 3. I haven't run it
recently to see if it also has the rsn-cap-override flag, but it might.
Maybe you can try the current firmware to see if it solves your issue?

Thanks,
- Steve

--
Steve deRosier
Cal-Sierra Consulting
https://www.cal-sierra.com/

Re: ath confusing log message "country IE"

[Steve deRosier]

On Wed, Apr 18, 2018 at 7:55 AM, Toke Høiland-Jørgensen  wrote:
> Bernhard Gabler  writes:
>
>> Dear all,
>>
>> would it be possible to re-write the log message from
>> "regdomain ... dynamically updated by country IE"
>> in a less ambiguous way, e.g. to:
>> "regdomain ... dynamically updated by country-IE"
>> "regdomain ... dynamically updated by country information element"
>> , please?
>>
>> To the occasional reader, the following system log messages seem to
>> convey that
>>   -  "DE" (Germany) was intended to be used, but
>>   -  "IE" (Ireland) was configured as regdomain.
>>
>>  [ 2723.739071] ath: EEPROM regdomain: 0x8114
>>  [ 2723.739072] ath: EEPROM indicates we should expect a country code
>>  [ 2723.739073] ath: doing EEPROM country->regdmn map search
>>  [ 2723.739074] ath: country maps to regdmn code: 0x37
>>  [ 2723.739075] ath: Country alpha2 being used: DE
>>  [ 2723.739076] ath: Regpair used: 0x37
>>  [ 2723.739077] ath: regdomain 0x8114 dynamically updated by country IE
>>
>> Only after intense googling, I now know that IE stands for "information
>> element", not for Ireland. But this is not obvious at. To prevent others
>> from confusion and wasting time on a non-problem, could please someone
>> rewrite the log message?
>
> Heh, that is a good point, I think. The patch below should fix it; but I
> worry that the log lines become quite long, then? Maybe better to use
> "country-IE", but that could also be confused. "country inf.elem"
> perhaps?
>

Instead of just rewording the message, perhaps adjusting it to show
more information would make it more clear:

`regdomain 0x8114 dynamically updated by country I.E. to DE`

Assuming the message two lines higher is related, I know the
information is already there, but this way the line stands on its own,
is more information rich, and is also clear.

- Steve

Re: second wifi card enforce CN reg dom

[Steve deRosier]

On Thu, Apr 12, 2018 at 10:25 AM, solsTiCe d'Hiver
<solstice.dhi...@gmail.com> wrote:
> It's the second time that you (Ben and Steve) are implying that I
> might break the law.
>

No implication intended. All I said is regulatory operation is
constrained by laws in various jurisdictions. And how the unit behaves
is likely simply following the rules programmed into it for those
purposes. And, there's nothing we can do to change that.

> But why are you saying that ? I am not gonna repeat myself again.
>

If the radio only works as CN and won't let you change it, it's
probably a CN radio and is hard-coded to do that. And, the
intersection of your FR regulatory domain and the CN is what it is.
Have you plugged it in alone? And if so, can you get it on FR or does
it stay on CN?

- Steve

--
Steve deRosier
Cal-Sierra Consulting LLC
https://www.cal-sierra.com/

Re: second wifi card enforce CN reg dom

[Steve deRosier]

On Thu, Apr 12, 2018 at 3:51 AM, Arend van Spriel
<arend.vanspr...@broadcom.com> wrote:
> On 4/12/2018 10:42 AM, solsTiCe d'Hiver wrote:
>>
>> Hi.
>>
>> This is beyond my comprehension that you could assert this is a non issue.
>
>
> Well. I am just saying that it is by design. There is no way for the
> regulatory code to determine where you and your hardware actually reside so
> instead it takes a conservative approach.
>

To say it another way: mixing regulatory domains on your host system
should result in a _smaller_ set of channels - ie only those channels
at the intersection of the two.

And another wrinkle to consider - one of the 802.11 amendments (can't
remember which one) actually causes the radio to listen to the beacons
around it, determine what the local regulatory domain is based on the
beacons it hears, and then lock to that regulatory domain. It's
possible for that information to be propagated up to the card's host
and the regulatory domain then would affect both cards. That's how
it's supposed to work, though I don't factually know Linux does this
in all cases.  Could it be you're somewhere where CN is the local
regulatory domain and the TL-WN722N has this feature?

In any case, as Arend points out, despite the hand-wringing that
regulatory domains cause users trying to do something particular,
between certain rules and regulations and certain manufacturers bad
interpretations and implementations around it, there's little that can
be done about it. Fact is, your radio must comply to whatever
regulatory domain you are in, otherwise it's breaking the rules. And
people breaking the regulatory rules is part of what's gotten
governments to pass even worse (for us OSS guys) laws that tighten
those rules down further.

You asked who to contact. Its not the LKML - it's your relevant
government body. And certain manufacturers who improperly interpret
said rules because it's easier for them.

- Steve

--
Steve deRosier
Cal-Sierra Consulting LLC
https://www.cal-sierra.com/

Re: RTL8723BE performance regression

[Steve deRosier]

On Tue, Apr 3, 2018 at 6:51 PM, João Paulo Rechi Vita <jprv...@gmail.com> wrote:
>
> This are the results (testing with speedtest.net) I got at some key points:
>
> VersionCommitPingDownUp
>
> v4.11a351e9b1225.445.99
> v4.11a351e9b131  17.025.89
>
> v4.13569dbb8174  14.080.00
> v4.13569dbb8261  8.41  0.00
>
> v4.15+revert d8a5b801923.861.41
> v4.15+revert d8a5b80189  18.691.39
>

I recommend doing throughput testing in a closed system using iperf.
speedtest.net is potentially useful for testing your ISP's bandwidth
at some particular point in time, but little else as it exposes you to
too many variables. I wouldn't take those numbers to mean much and the
inconclusive results you're getting could be explained by external
network loading and having little to do with your bisect effort. I can
get that spread in numbers from speedtest.net without making any
changes other than the time of day I do the test.

Here's how to do it. Install iperf2 (you could use iperf3, personal
choice) on two machines, one being your device under test (DUT). Setup
a network configuration that looks similar to this:

server <==hardwire==> AP <--wireless link--> DUT

Be sure your hardwire is more bandwidth than your wireless link is
capable of, or set it up where the server is the AP. What you're
looking for here is environmental consistency, not maximum throughput
numbers.

On the computer hardwired to the network, start the server, we'll
assume it has an ip of 192.168.33.18:

iperf -s

On your DUT:

iperf -c 192.168.33.18

That's the most basic setup, check the man page for more options.

You will get best results if you can exclude other computers from your
test network and other wireless devices from your airspace.

- Steve

--
Steve deRosier
Cal-Sierra Consulting LLC
https://www.cal-sierra.com/

Re: Wi-Fi Disconnection on Suspend for no wowlan triggers

[Steve deRosier]

Hi Sunil,

On Tue, Apr 3, 2018 at 5:39 AM, Sunil Dutt Undekari
<usd...@qti.qualcomm.com> wrote:
> Hi All ,
>
> I would like to discuss on the commit 
> 8125696991194aacb1173b6e8196d19098b44e17 (cfg80211/mac80211: disconnect on 
> suspend) which triggers the STA disconnection on suspend if no wowlan 
> triggers are registered.

This commit went in over five years ago. Sometime around v3.9

> I guess the intention here is to disconnect from the AP if the device cannot 
> wake up on a packet / not configured with wowlan triggers .

It being a long time and not feeling like digging too deep into the
patch, what I can guess from a quick look is basically this:

* Before the patch, waking up from suspend would throw warnings into
the log. This patch fixes this.
* As you can't wake the processor, and you're a thin-mac chip who
depends on mac80211, you _can't_ do anything without wowlan.
* If your chip is not configured for wowlan, on a suspend, it might as
well go to sleep, as there's no sense in wasting power as you can't do
anything anyway.
* If you're going to sleep and can't respond to the AP, it's polite to
disconnect. And the AP is going to forcibly do so to you anyway after
some time of not hearing from you.

Hence the above in general concept seems perfectly correct and I don't
see any point in doing anything else with a mac80211 device.

> Can this behavior get enhanced to only do so on driver's preference -  do a 
> disconnect for only the host drivers that would want to do so (through an 
> additional feature capability) ?
>

Can? Perhaps. Send a patch. And maybe you could explain in detail what
the actual problem is you're trying to solve. Patches that are "maybe,
someday, someone might want to..." don't have a terribly high chance
of getting merged.  But solutions to real-life actual problems do. And
it's really difficult for us to speak on vague hypothetical musings.

- Steve

--
Steve deRosier
Cal-Sierra Consulting LLC
https://www.cal-sierra.com/

Re: [RFC 0/4] wireless: Per-sta NoAck and offload support

[Steve deRosier]

On Tue, Mar 27, 2018 at 11:09 PM,  <vthia...@codeaurora.org> wrote:
> On 2018-03-27 22:18, Steve deRosier wrote:
>>
>> Hi Vasanthakumar,
>>
>> On Tue, Mar 27, 2018 at 1:42 AM, Vasanthakumar Thiagarajan
>> <vthia...@codeaurora.org> wrote:
>>>
>>> Adds infrastructure for driver to offload NoAck functionality, driver
>>> like ath10k could make use of it. Also extends the current ndev wide
>>
>>
>> I'm not really much of a fan of adding a feature without some use of
>> the feature. Perhaps if drivers "like" ath10k could use it, maybe you
>> should add a patch(s) to the series where one of those drivers
>> actually uses the feature.  An API without an example of use is also
>> harder to evaluate effectively.
>
>
> I agree driver patch using the new NoAck infrastructure would help with
> understanding, ill post it once it is ready. But not sure the driver patch
> can be part of the same series.
>

It can. I think both Arend and Johannes already covered it.


>>
>> Additionally, if it's relevant, adding use of the feature to hwsim
>> would both serve the above comment as well as provide testing
>> capability.
>
>
> Does not seem like this offload feature is something applicable for hwsim
> especially mac80211 already offers the same functionality.
>

Well, if we desire hwsim to be able to test all the features of
mac80211 (which I don't know if that's true), then it would be
appropriate to place the functionality in hwsim as an optional
turn-on-able feature and have it utilize this API if it's turned on.
It actually would be nice to add it for that purpose. But, admittedly
it's a bit of work as you have to replicate the "hardware" offload
portion in hwsim which obviously you don't if you're working with
actual hardware that implements this feature.

I'd say it's a nice-to-have. If only to keep hwsim in-sync with
mac80211 features for testing. But, I admit I'm asking for work that
is perhaps out-of-scope.

All I really want is a driver that actually uses this as an example of use.

>
> The NoAck configuration is a bitmap of tid which is used to set NoAck in Qos
> control field of the data frame for that particular tid. Perhaps you could
> look at Ack policy subfield section in 802.11 spec.
>

Thank you. Because you gave it a name, I thought we were talking about
something more...involved. I'd appreciate if you pointed out that
context in the commit comment of the first patch in the series.
Something mentioning the Ack policy subfield specifically would give
context for those of us trying to tie it to specific 802.11
specifications.

Thanks,
- Steve

Re: [RFC 0/4] wireless: Per-sta NoAck and offload support

[Steve deRosier]

Hi Vasanthakumar,

On Tue, Mar 27, 2018 at 1:42 AM, Vasanthakumar Thiagarajan
 wrote:
> Adds infrastructure for driver to offload NoAck functionality, driver
> like ath10k could make use of it. Also extends the current ndev wide

I'm not really much of a fan of adding a feature without some use of
the feature. Perhaps if drivers "like" ath10k could use it, maybe you
should add a patch(s) to the series where one of those drivers
actually uses the feature.  An API without an example of use is also
harder to evaluate effectively.

Additionally, if it's relevant, adding use of the feature to hwsim
would both serve the above comment as well as provide testing
capability.


> NoAck policy to per-station, with sta level NoAck policy configuration
> userspace could selectively turn off/on Noack based on various connection
> parameters of the station.
>

This is my own ignorance, perhaps from missing recent netdev
conferences - can you send a link to some documentation of what NoAck
is? Certain things in 802.11 use ack transmissions and
interoperability would be compromised if we didn't conform to spec. I
don't imagine that's what's going on here but I'd like to understand
what the heck NoAck is and I failed to bring up anything useful when I
Googled it.

- Steve

Re: [RFC] ethtool: Support ETHTOOL_GSTATS2 command.

[Steve deRosier]

On Tue, Mar 20, 2018 at 8:39 AM, Ben Greear  wrote:
> On 03/20/2018 03:37 AM, Michal Kubecek wrote:
>>
>> On Wed, Mar 07, 2018 at 11:51:29AM -0800, gree...@candelatech.com wrote:
>>>
>>> From: Ben Greear 
>>>
>>> This is similar to ETHTOOL_GSTATS, but it allows you to specify
>>> a 'level'.  This level can be used by the driver to decrease the
>>> amount of stats refreshed.  In particular, this helps with ath10k
>>> since getting the firmware stats can be slow.
>>>
>>> Signed-off-by: Ben Greear 
>>> ---
>>>
>>> NOTE:  I know to make it upstream I would need to split the patch and
>>> remove the #define for 'backporting' that I added.  But, is the
>>> feature in general wanted?  If so, I'll do the patch split and
>>> other tweaks that might be suggested.
>>
>>
>
> Yes, but that would require changing all drivers at once, and would make
> backporting
> and out-of-tree drivers harder to manage.  I had low hopes that this feature
> would
> make it upstream, so I didn't want to propose any large changes up front.
>

Hi Ben,

I find the feature OK, but I'm not thrilled with the arbitrary scale
of "level". Maybe there could be some named values, either on a
spectrum as level already is, similar to the kernel log DEBUG, WARN,
INFO  type levels. Or named bit flags like the way the ath drivers
do their debug flags for granular results.  Thoughts?

- Steve

Re: brcmfmac signal/interference issues

[Steve deRosier]

On Thu, Mar 8, 2018 at 2:47 AM, Arend van Spriel
<arend.vanspr...@broadcom.com> wrote:
> On 2/23/2018 2:49 PM, Daniel Drake wrote:
>>
>> On Fri, Feb 23, 2018 at 12:54 PM, Arend van Spriel
>> <arend.vanspr...@broadcom.com> wrote:
>>>
>>> Yup. Windows firmware talks NDIS. If you run 'strings 4345r6rtecdc.bin |
>>> tail -1' you can see the firmware build target and it likely has 'ndis'
>>> in
>>> it.
>
>
> Hi Daniel,
>
> Bit late response. Sorry.
>
>>
>> 43455c0-roml/sdio-ag-ndis-vista-pktfilter-d0c-pno-aoe-p2p-dhdoid-ndoe-gtkoe-mfp-proptxstatus-dmatxrc-keepalive-ap-ampduretry-pclose-txbf
>>
>> Yes, ndis. So no easy way to run the same firmware on the 2 OSes.
>
>
> Indeed. I could try building nearly same firmware target. Can you provide
> the firmware version as well.
>
> Now reading over your orignal email again:
>
>> If I place both antenna terminals inside the Linux MiniPC case, the
>> Linux pings are bad but the Windows pings are fine.
>>
>> If I place both antenna terminals inside the Windows MiniPC case, it
>> is the same: Linux pings are bad, but the Windows pings are fine.
>>
>> And when the Linux antenna is placed outside of both cases, the Linux
>> pings are fine. I've repeated these tests a handful of times in quick
>> succession to make sure that I'm not going crazy and that this is not
>> a case of the problem intermittency causing misleading results. These
>> findings appear very solid.
>
> So it picks up something in the PC. Some sources of interference that I have
> seen before are USB3 and HDMI. Maybe try to shield those if present and see
> if that helps. The nvram contains sensitivity parameters, but as you stated
> you are using the same nvram for windows and linux for now we can rule it
> out for debugging the issue.
>

Hi Daniel,

I'll jump in here too...

Did you check the Bluetooth?  I don't know if this chip has it or if
it's an independent chip on this board, but if Linux is leaving it
powered up but not properly configured you could have issues. And in
some designs, the BT and WiFi will share a single antenna. Note that
I'm not saying you've configured BT to run, I'm actually suggesting
that the pin that enables it is on, but you might not be loading the
BT drivers and firmware and so the thing is just in a wonky
uninitialized state. Or you do have it enabled and should try turning
it off.  Either way.

And WiFi/BT coex has always been a bit of a problem (speaking
generally, I don't know the status with this particular chip) in
Linux. I see WiFi and BT interfering with each other frequently in my
testing setups with my dev boards. Often I can magically make problems
go away by simply pulling the enable line high (which is "off").

- Steve

--
Steve deRosier
Cal-Sierra Consulting LLC
https://www.cal-sierra.com/

Re: Max clients on wifi access point with 7265

[Steve deRosier]

On Tue, Feb 27, 2018 at 12:33 AM, Kalle Valo <kv...@codeaurora.org> wrote:
> Luca Coelho <l...@coelho.fi> writes:
>
>>> > We do have wiphy::max_ap_assoc_sta, but I see only ath10k, qtnfmac
>>> > and
>>> > rsi_91x setting it. I wish all drivers would use that.
>>> >
>>> >  * @max_ap_assoc_sta: maximum number of associated stations
>>> > supported in AP mode
>>> >  * (including P2P GO) or 0 to indicate no such limit is advertised.
>>> > The
>>> >  * driver is allowed to advertise a theoretical limit that it can
>>> > reach in
>>> >  * some cases, but may not always reach.
>>>
>>> Cool, I hadn't noticed this before.  I'll add a patch to iwlwifi to
>>> add it.
>>
>> Actually this is not so straightforward, because every time we add a
>> p2p vif, we lose one more station.  So the max_ap_assoc_sta value must
>> be dynamic (or we can state the theoretical lowest number to start
>> with, which would not be very nice).
>>
>> I don't think this feature is worth the trouble, so I'll skip it for
>> now.
>
> I think the documentation answers that part pretty well:
>
>   "The driver is allowed to advertise a theoretical limit that it can
>reach in some cases, but may not always reach."
>
> So I still thank having the drivers to advertise the theoretical maximum
> numbers of client is useful, even if it would not be always 100%
> correct. For example, an average user most likely will not have any clue
> if the limit is 10, 50 or 100 clients. And besides, very few people use
> P2P anyway ;)
>
> But of course this is just nice-to-have category cand we have far more
> important things to fix first.
>

>From a practical point-of-view, many chipsets don't advertise this
information. Luca was able to look the limit up or knew it for his
chip. For example, on the ath6kl chips I most commonly have worked
with, the number is not specified by QCA and was only determined by us
via experimentation. And even on the same chip, it'll change with
firmware version as the necessary resources get consumed by new
features or fixes. If the firmware can send that number up to the
driver (or the driver can reliably know it because it can't change),
then expose it, but otherwise I'd advise publishing a value of 0. I'd
rather see the unknown flag rather than relying on a number that may
or may not be accurate.

- Steve

--
Steve deRosier
Cal-Sierra Consulting LLC
https://www.cal-sierra.com/

Re: Support on vendor id and device id

[Steve deRosier]

Hi Harsha,

Part 2 - the darn cmd-enter shortcut in gmail got me again. Sorry for
the premature send.

>
> As both Larry and Arend mention, while it's possible to have multiple
> drivers with the same IDs, for obvious reasons it's not desirable.
>
> In theory the vendor IDs shouldn't be duplicated on fundamentally
> different devices, assuming that the manufacturers are doing things
> "right". The VID is paid for by buying it from the SD Association.
>
> My suspicion is that your device, is fundamentally a wilc1000 and that
> the existing wilc1000 driver will likely largely work for it and all
> you really need to do is modify the existing driver to handle the
> quirks of your particular implementation of the wilc1000 chip. And,
> often WiFi chips will let you change the VID/PID somewhere within
> whatever non-volatile storage it has (like where it stores the MAC
> address).

What I was going to mention is this is how it's worked for me many
times on several chips from QCA, Broadcom and Marvell included. The
use-case in this case would be building a custom WiFi module using a
vendor's chip. Very common - Silex, Laird and others do this all the
time: buy a chip from QCA (or Broadcom or Marvell or...), put it on a
board with lna, pa, passives, antennas, etc...  And you package it and
sell that often with value-add software.  Sometimes it's fine if it
represents like the original chip (for example on the ath6kl chips
Laird sells, it just uses the QCA IDs), but sometimes you want it to
represent as "my chip" (again, as Laird has to do on the chips they
sell to the Windows market to avoid the kind of driver conflicts
you're encountering). All of these chips usually have a way to store a
MAC address and at least all that I've worked on will also allow you
to change the SDIO VID in the same one-time-programable memory.
Obviously the method changes depending on the chip.

However, in nearly every case, the chip is still fundamentally
whatever it was and we still use the upstream driver, though often
with tweaks and customizations based on our implementation. In the
cases where we've added a custom VID, we've also had to add the new
VID to the original driver and then key our quirks off that.

I'm not saying this is definitely applicable in your case, but it is
common and maybe it will help.

Also thanks for bringing this issue up. Something I haven't thought
about in a long time but I'm adding it to my talk at ELC on WiFi
module interfacing. http://sched.co/DXn3

- Steve

--
Steve deRosier
Cal-Sierra Consulting LLC
https://www.cal-sierra.com/

Re: Support on vendor id and device id

[Steve deRosier]

Hi Harsha,

On Tue, Feb 27, 2018 at 7:29 AM, Harsha Rao  wrote:
> On Tue, Feb 27, 2018 at 8:44 PM, Larry Finger  
> wrote:
>> On 02/27/2018 07:30 AM, Harsha Rao wrote:
>>>
>>> On Tue, Feb 27, 2018 at 4:45 PM, Arend van Spriel
>>>  wrote:

 On 2/27/2018 11:16 AM, Harsha Rao wrote:
>
>
> Hi folks,
> I am developing a new wifi driver for our sdio based wifi device.
>
> I see that SDIO_VENDOR_ID and SDIO_DEVICE_ID for our device (We had
> bought the SDIO IP from 3rd party)  is already  been used by some
> other vendor and its already into staging .
>
> Please suggest me how can I move forward to submit the driver for
> staging.



 Can you be more specific about the conflicting vendor id and device id?
>>>
>>>
>>> Hi,
>>> My driver kicks in  with vendor id 0x296 and device id 0x5347.
>>> But when I grepped the the kernel source I could see an other driver
>>> wilc1000 using the same vendor ID and device ID
>>> ( We have a different hw than them !)
>>>
> Is there a way to reconfigure the values in the SDIO host for
> different value or does Linux allow two drivers with same values to
> survive mutual exclusively



 First come first serve. When a device is detected, the driver core looks
 for
 drivers supporting it based on device table and the first that
 successfully
 returns from the .probe() callback is bound to the device.

 Regards,
 Arend
>>>
>>>
>>> Does Linux allow two drivers with conflicting device and vendor IDs to
>>> stay on in kernel ?
>>
>>
>> Yes. We have a similar situation with the rtl8192e driver in staging and the
>> rtl8192se driver in the wireless tree, which share PCI ID 0bda:8192. Our
>> solution was to insert code in the probe routine that tests some value in
>> addition to the PCI ID. In our case, that was the PCI revision id. Line 2496
>> of file drivers/staging/rtl8192e/rtl8192e/rtl_core.c shows the test in the
>> staging driver.
>>
>> The downsides of this unfortunate duplication are that if the wrong driver
>> loads first, then it remains loaded, you will need to cooperate with the
>> maintainer of the other driver to insert the extra detection code, and you
>> may need to do quite a bit of processing to be able to determine whether you
>> have the correct device.
>>
>> Larry
>>

As both Larry and Arend mention, while it's possible to have multiple
drivers with the same IDs, for obvious reasons it's not desirable.

In theory the vendor IDs shouldn't be duplicated on fundamentally
different devices, assuming that the manufacturers are doing things
"right". The VID is paid for by buying it from the SD Association.

My suspicion is that your device, is fundamentally a wilc1000 and that
the existing wilc1000 driver will likely largely work for it and all
you really need to do is modify the existing driver to handle the
quirks of your particular implementation of the wilc1000 chip. And,
often WiFi chips will let you change the VID/PID somewhere within
whatever non-volatile storage it has (like where it stores the MAC
address).

Re: [PATCH v12] ath10k: add LED and GPIO controlling support for various chipsets

[Steve deRosier]

On Mon, Feb 26, 2018 at 12:44 AM, <s.gottsch...@dd-wrt.com> wrote:
>
> From: Sebastian Gottschall <s.gottsch...@newmedia-net.de>
>
> Adds LED and GPIO Control support for 988x, 9887, 9888, 99x0, 9984 based 
> chipsets with on chipset connected led's
> using WMI Firmware API.
> The LED device will get available named as "ath10k-phyX" at sysfs and can be 
> controlled with various triggers.
> adds also debugfs interface for gpio control.
>
> Signed-off-by: Sebastian Gottschall <s.gottsch...@dd-wrt.com>
>
> v2  add correct gpio count per chipset and remove IPQ4019 support since this 
> chipset does not make use of specific gpios)
> v5  fix compiling without LED_CLASS and GPIOLIB support, fix also error by 
> kbuild test robot which does not occur in standard builds. curious
> v6  correct return values and fix comment style
> v7  fix ath10k_unregister_led for compiling without LED_CLASS
> v8  fix various code design issues reported by reviewers
> v9  move led and led code to separate sourcefile (gpio.c)
> v10 compile fix if gpiolib isnt included
> v11 make register_gpio_chip static. advise by krobot
> v12 fix warning
> ---
>  drivers/net/wireless/ath/ath10k/Kconfig   |  10 ++
>  drivers/net/wireless/ath/ath10k/Makefile  |   1 +
>  drivers/net/wireless/ath/ath10k/core.c|  28 -
>  drivers/net/wireless/ath/ath10k/core.h|  62 +-
>  drivers/net/wireless/ath/ath10k/debug.c   | 146 ++
>  drivers/net/wireless/ath/ath10k/gpio.c| 196 
> ++
>  drivers/net/wireless/ath/ath10k/hw.h  |   2 +
>  drivers/net/wireless/ath/ath10k/mac.c |   5 +
>  drivers/net/wireless/ath/ath10k/wmi-ops.h |  36 +-
>  drivers/net/wireless/ath/ath10k/wmi-tlv.c |  65 ++
>  drivers/net/wireless/ath/ath10k/wmi.c |  46 +++
>  drivers/net/wireless/ath/ath10k/wmi.h |  36 ++
>  12 files changed, 630 insertions(+), 3 deletions(-)
>  create mode 100644 drivers/net/wireless/ath/ath10k/gpio.c


Assuming that kbuild robot doesn't kick back another build-time
warning, it looks OK to me.

Reviewed-by: Steve deRosier <deros...@cal-sierra.com>

--
Steve deRosier
Cal-Sierra Consulting LLC
https://www.cal-sierra.com/

Re: [PATCH v9] ath10k: add LED and GPIO controlling support for various chipsets

[Steve deRosier]

Hi Sebastian,

On Thu, Feb 22, 2018 at 6:09 PM, Sebastian Gottschall
<s.gottsch...@dd-wrt.com> wrote:
> Am 22.02.2018 um 23:17 schrieb Steve deRosier:
>>
>> On Thu, Feb 22, 2018 at 3:15 AM,  <s.gottsch...@dd-wrt.com> wrote:
>>>
>>> From: Sebastian Gottschall <s.gottsch...@newmedia-net.de>
>>>
>>> Adds LED and GPIO Control support for 988x, 9887, 9888, 99x0, 9984 based
>>> chipsets with on chipset connected led's
>>> using WMI Firmware API.
>>> The LED device will get available named as "ath10k-phyX" at sysfs and can
>>> be controlled with various triggers.
>>> adds also debugfs interface for gpio control.
>>>
>>> Signed-off-by: Sebastian Gottschall <s.gottsch...@dd-wrt.com>
>>>
>>> v2  add correct gpio count per chipset and remove IPQ4019 support since
>>> this chipset does not make use of specific gpios)
>>> v5  fix compiling without LED_CLASS and GPIOLIB support, fix also error
>>> by kbuild test robot which does not occur in standard builds. curious
>>> v6  correct return values and fix comment style
>>> v7  fix ath10k_unregister_led for compiling without LED_CLASS
>>> v8  fix various code design issues reported by reviewers
>>> v9  move led and led code to separate sourcefile (gpio.c)
>>> ---
>>>   drivers/net/wireless/ath/ath10k/Kconfig   |   3 +
>>>   drivers/net/wireless/ath/ath10k/Makefile  |   1 +
>>>   drivers/net/wireless/ath/ath10k/core.c|  28 -
>>>   drivers/net/wireless/ath/ath10k/core.h|  33 -
>>>   drivers/net/wireless/ath/ath10k/debug.c   | 142 ++
>>>   drivers/net/wireless/ath/ath10k/gpio.c| 196
>>> ++
>>>   drivers/net/wireless/ath/ath10k/hw.h  |   2 +
>>>   drivers/net/wireless/ath/ath10k/mac.c |   5 +
>>>   drivers/net/wireless/ath/ath10k/wmi-ops.h |  36 +-
>>>   drivers/net/wireless/ath/ath10k/wmi-tlv.c |  65 ++
>>>   drivers/net/wireless/ath/ath10k/wmi.c |  46 +++
>>>   drivers/net/wireless/ath/ath10k/wmi.h |  36 ++
>>>   12 files changed, 590 insertions(+), 3 deletions(-)
>>>   create mode 100644 drivers/net/wireless/ath/ath10k/gpio.c
>>>
>>> diff --git a/drivers/net/wireless/ath/ath10k/Kconfig
>>> b/drivers/net/wireless/ath/ath10k/Kconfig
>>> index deb5ae21a559..c46b7658a820 100644
>>> --- a/drivers/net/wireless/ath/ath10k/Kconfig
>>> +++ b/drivers/net/wireless/ath/ath10k/Kconfig
>>> @@ -2,6 +2,9 @@ config ATH10K
>>>   tristate "Atheros 802.11ac wireless cards support"
>>>   depends on MAC80211 && HAS_DMA
>>>  select ATH_COMMON
>>> +select MAC80211_LEDS
>>> +select LEDS_CLASS
>>> +select NEW_LEDS
>>>  select CRC32
>>>  select WANT_DEV_COREDUMP
>>>   ---help---
>>
>> Pure question: do we require LEDS, or is this an option? I assumed all
>> along it was an optional thing - ie if GPIOs and/or LEDs were
>> configured, we added this code. And if not optional, then what happens
>> if we are on a platform that can't support LEDS_CLASS or NEW_LEDS,
>> assuming there is such a thing?
>
> i took this behaviour from ath9k/ath5k, both driver auto select
> MAC80211_LEDS and LEDS_CLASS (by the way MAC80211_LEDS auto selects
> LEDS_CLASS too).  only chipsets which are known to have preconfigured leds
> are used
> and led will only turn on if you enable it in sysfs by assigning a trigger
> to the led.
> for sure this can be removed from the Kconfig. it will compile without it as
> well. so this is optional. i just added it since all other drivers behave in
> the same way.
> in addition. LEDS_CLASS  / NEW_LEDS is a kernel api, not a driver. so all
> platforms to support LEDS_CLASS. just enabling LEDS_CLASS will not cause any
> platform specific dependency.
>>
>>
>>
>>> diff --git a/drivers/net/wireless/ath/ath10k/Makefile
>>> b/drivers/net/wireless/ath/ath10k/Makefile
>>> index 6739ac26fd29..bcb234f0b940 100644
>>> --- a/drivers/net/wireless/ath/ath10k/Makefile
>>> +++ b/drivers/net/wireless/ath/ath10k/Makefile
>>> @@ -20,6 +20,7 @@ ath10k_core-$(CONFIG_NL80211_TESTMODE) += testmode.o
>>>   ath10k_core-$(CONFIG_ATH10K_TRACING) += trace.o
>>>   ath10k_core-$(CONFIG_THERMAL) += thermal.o
>>>   ath10k_core-$(CONFIG_MAC80211_DEBUGFS) += debugfs_sta.o
>>> +ath10k_core-y += gpio.o
>>>   ath10k_core-$(CONFIG_PM) += wo

Re: [PATCH v9] ath10k: add LED and GPIO controlling support for various chipsets

[Steve deRosier]

On Thu, Feb 22, 2018 at 3:15 AM,  <s.gottsch...@dd-wrt.com> wrote:
> From: Sebastian Gottschall <s.gottsch...@newmedia-net.de>
>
> Adds LED and GPIO Control support for 988x, 9887, 9888, 99x0, 9984 based 
> chipsets with on chipset connected led's
> using WMI Firmware API.
> The LED device will get available named as "ath10k-phyX" at sysfs and can be 
> controlled with various triggers.
> adds also debugfs interface for gpio control.
>
> Signed-off-by: Sebastian Gottschall <s.gottsch...@dd-wrt.com>
>
> v2  add correct gpio count per chipset and remove IPQ4019 support since this 
> chipset does not make use of specific gpios)
> v5  fix compiling without LED_CLASS and GPIOLIB support, fix also error by 
> kbuild test robot which does not occur in standard builds. curious
> v6  correct return values and fix comment style
> v7  fix ath10k_unregister_led for compiling without LED_CLASS
> v8  fix various code design issues reported by reviewers
> v9  move led and led code to separate sourcefile (gpio.c)
> ---
>  drivers/net/wireless/ath/ath10k/Kconfig   |   3 +
>  drivers/net/wireless/ath/ath10k/Makefile  |   1 +
>  drivers/net/wireless/ath/ath10k/core.c|  28 -
>  drivers/net/wireless/ath/ath10k/core.h|  33 -
>  drivers/net/wireless/ath/ath10k/debug.c   | 142 ++
>  drivers/net/wireless/ath/ath10k/gpio.c| 196 
> ++
>  drivers/net/wireless/ath/ath10k/hw.h  |   2 +
>  drivers/net/wireless/ath/ath10k/mac.c |   5 +
>  drivers/net/wireless/ath/ath10k/wmi-ops.h |  36 +-
>  drivers/net/wireless/ath/ath10k/wmi-tlv.c |  65 ++
>  drivers/net/wireless/ath/ath10k/wmi.c |  46 +++
>  drivers/net/wireless/ath/ath10k/wmi.h |  36 ++
>  12 files changed, 590 insertions(+), 3 deletions(-)
>  create mode 100644 drivers/net/wireless/ath/ath10k/gpio.c
>
> diff --git a/drivers/net/wireless/ath/ath10k/Kconfig 
> b/drivers/net/wireless/ath/ath10k/Kconfig
> index deb5ae21a559..c46b7658a820 100644
> --- a/drivers/net/wireless/ath/ath10k/Kconfig
> +++ b/drivers/net/wireless/ath/ath10k/Kconfig
> @@ -2,6 +2,9 @@ config ATH10K
>  tristate "Atheros 802.11ac wireless cards support"
>  depends on MAC80211 && HAS_DMA
> select ATH_COMMON
> +select MAC80211_LEDS
> +select LEDS_CLASS
> +select NEW_LEDS
> select CRC32
> select WANT_DEV_COREDUMP
>  ---help---

Pure question: do we require LEDS, or is this an option? I assumed all
along it was an optional thing - ie if GPIOs and/or LEDs were
configured, we added this code. And if not optional, then what happens
if we are on a platform that can't support LEDS_CLASS or NEW_LEDS,
assuming there is such a thing?


> diff --git a/drivers/net/wireless/ath/ath10k/Makefile 
> b/drivers/net/wireless/ath/ath10k/Makefile
> index 6739ac26fd29..bcb234f0b940 100644
> --- a/drivers/net/wireless/ath/ath10k/Makefile
> +++ b/drivers/net/wireless/ath/ath10k/Makefile
> @@ -20,6 +20,7 @@ ath10k_core-$(CONFIG_NL80211_TESTMODE) += testmode.o
>  ath10k_core-$(CONFIG_ATH10K_TRACING) += trace.o
>  ath10k_core-$(CONFIG_THERMAL) += thermal.o
>  ath10k_core-$(CONFIG_MAC80211_DEBUGFS) += debugfs_sta.o
> +ath10k_core-y += gpio.o
>  ath10k_core-$(CONFIG_PM) += wow.o
>  ath10k_core-$(CONFIG_DEV_COREDUMP) += coredump.o
>

I'd expect it would be something like `ath10k_core-$(CONFIG_GPIO) +=
gpoi.o`.  Maybe I'm mistaken.
(Note, I didn't look up the actual config option name, I'm guessing.
Assume I wrote something reasonable for my example.)

I only ask the above two questions, not because I think it's wrong,
but because I don't quite know the intention and it makes me wonder.

Other than those questions, which are optional as the code looks
correct to me, the rest of it looks fine to me.

Reviewed-by: Steve deRosier <deros...@cal-sierra.com>

- Steve

Re: [PATCH v7] ath10k: add LED and GPIO controlling support for various chipsets

[Steve deRosier]

On Tue, Feb 20, 2018 at 10:06 AM, Sebastian Gottschall
<s.gottsch...@dd-wrt.com> wrote:
> Am 20.02.2018 um 17:52 schrieb Steve deRosier:
>>
>>
>>> +static int ath10k_register_gpio_chip(struct ath10k *ar)
>>> +{
>>> +   struct ath10k_gpiocontrol *gpio;
>>> +   gpio = kzalloc(sizeof(struct ath10k_gpiocontrol), GFP_KERNEL);
>>> +   if (!gpio) {
>>> +   return -ENOMEM;
>>> +   }
>>> +
>>> +   snprintf(gpio->label, sizeof(gpio->label), "ath10k-%s",
>>> +wiphy_name(ar->hw->wiphy));
>>> +#if LINUX_VERSION_CODE >= KERNEL_VERSION(4,5,0)
>>> +   gpio->gchip.parent = ar->dev;
>>> +#else
>>> +   gpio->gchip.dev = ar->dev;
>>> +#endif
>>> +   gpio->gchip.base = -1;  /* determine base automatically */
>>
>> I may be wrong, but I think that version guards are unwelcome here.
>> The patch is supposed to go and apply specifically to the upstream
>> version, which is clearly >= KERNEL_VERSION(4,5,0). I think you can
>> drop this, only use the 'gpio->gchip.parent = ar->dev;' and call it
>> good. This fixup should be picked up by Backports.  I understand that
>> you might need to keep it for your internal tree, but I don't think
>> the maintainers want to see this.
>
> the original patch comes from my sources which are supposed to work with
> compat-wireless für multiple kernels. but for sure i can change that :-)
>

I understand. I've got some code that looks similar. However, I don't
think the practice is to allow it upstream, the practice now is to
rely on Backports to know about these tranformations. Someone here
will correct me if I'm wrong.

Just prune it for going upstream. :-)


>>
>>
>>> +/* remove GPIO chip */
>>> +static void ath10k_unregister_gpio_chip(struct ath10k *ar)
>>> +{
>>> +#ifdef CONFIG_GPIOLIB
>>> +   struct ath10k_gpiocontrol *gpio = ar->gpio;
>>> +   if (gpio) {
>>> +   gpiochip_remove(>gchip);
>>> +   kfree(gpio);
>>> +   }
>>> +#endif
>>> +}
>>
>> Instead of entering/exiting the guards inside every function, I think
>> the usually accepted way to handle this is to place a block in the
>> that looks basically like:
>>
>> #ifdef CONFIG_GPIOLIB
>> static void ath10k_unregister_gpio_chip(struct ath10k *ar) {
>> struct ath10k_gpiocontrol *gpio = ar->gpio;
>> if (gpio) {
>> gpiochip_remove(>gchip);
>> kfree(gpio);
>> }
>>   }
>>
>> next function
>> next function...
>>
>> #else
>> static void ath10k_unregister_gpio_chip(struct ath10k *ar) {
>>   }
>> next function
>> next function...
>> #endif
>>
>> If they weren't static, you'd do it in the .h file, with the
>> declaration of the full function there and the definition in the .c.
>>
>> I may be nit-picking and your style is perfectly acceptable to the
>> other maintainers.
>
> i'm not a fan of bloat code. and making funktions static which are just
> called one will allow the compiler to optimize the code better
> than making it non static. that makes no sense here for sure. its just a
> question how you handle it.

Agreed, I am also not a fan of bloat code. I wasn't suggesting you
move it to the .h, just saying that if it were not static, that's how
I'd suggest doing it.

In this case, keep it static, keep it in the .c.  And use the blocking
I suggested. You will get the optimizer friendly result you're looking
for, but make it more readable and more-inline with the dominant style
for these things.


>>
>>
>>
>>> +
>>> +static void ath10k_unregister_led(struct ath10k *ar)
>>> +{
>>> +#if defined (CONFIG_GPIOLIB) && defined(CONFIG_LEDS_CLASS)
>>> +   if (ar->gpio)
>>> +   led_classdev_unregister(>gpio->cdev);
>>> +#endif
>>> +}
>>> +
>>> +
>>>   int ath10k_core_start(struct ath10k *ar, enum ath10k_firmware_mode
>>> mode,
>>>const struct ath10k_fw_components *fw)
>>>   {
>>>  int status;
>>>  u32 val;
>>> -
>>>  lockdep_assert_held(>conf_mutex);
>>>
>> Whitespace change for no apparent reason.
>
> count it as typo
>
>>
>>> @@ -2372,8 +2524,32 @@ int ath10k_core_start(struct ath10k *ar, enum
>>> ath10k_firmware_mode mod

Re: [PATCH v7] ath10k: add LED and GPIO controlling support for various chipsets

[Steve deRosier]

e,
> .gen_echo = ath10k_wmi_op_gen_echo,
> +   .gen_gpio_config = ath10k_wmi_op_gen_gpio_config,
> +   .gen_gpio_output = ath10k_wmi_op_gen_gpio_output,
> /* .gen_bcn_tmpl not implemented */
> /* .gen_prb_tmpl not implemented */
> /* .gen_p2p_go_bcn_ie not implemented */
> @@ -8294,6 +8334,8 @@ static const struct wmi_ops wmi_10_2_ops = {
> .gen_delba_send = ath10k_wmi_op_gen_delba_send,
> .fw_stats_fill = ath10k_wmi_10x_op_fw_stats_fill,
> .get_vdev_subtype = ath10k_wmi_op_get_vdev_subtype,
> +   .gen_gpio_config = ath10k_wmi_op_gen_gpio_config,
> +   .gen_gpio_output = ath10k_wmi_op_gen_gpio_output,
> /* .gen_pdev_enable_adaptive_cca not implemented */
>  };
>
> @@ -8364,6 +8406,8 @@ static const struct wmi_ops wmi_10_2_4_ops = {
> .gen_pdev_enable_adaptive_cca =
> ath10k_wmi_op_gen_pdev_enable_adaptive_cca,
> .get_vdev_subtype = ath10k_wmi_10_2_4_op_get_vdev_subtype,
> +   .gen_gpio_config = ath10k_wmi_op_gen_gpio_config,
> +   .gen_gpio_output = ath10k_wmi_op_gen_gpio_output,
> /* .gen_bcn_tmpl not implemented */
> /* .gen_prb_tmpl not implemented */
> /* .gen_p2p_go_bcn_ie not implemented */
> @@ -8439,6 +8483,8 @@ static const struct wmi_ops wmi_10_4_ops = {
> .gen_pdev_bss_chan_info_req = 
> ath10k_wmi_10_2_op_gen_pdev_bss_chan_info,
> .gen_echo = ath10k_wmi_op_gen_echo,
> .gen_pdev_get_tpc_config = 
> ath10k_wmi_10_2_4_op_gen_pdev_get_tpc_config,
> +   .gen_gpio_config = ath10k_wmi_op_gen_gpio_config,
> +   .gen_gpio_output = ath10k_wmi_op_gen_gpio_output,
>  };
>
>  int ath10k_wmi_attach(struct ath10k *ar)
> diff --git a/drivers/net/wireless/ath/ath10k/wmi.h 
> b/drivers/net/wireless/ath/ath10k/wmi.h
> index c7b30ed9015d..dc180a86dc3b 100644
> --- a/drivers/net/wireless/ath/ath10k/wmi.h
> +++ b/drivers/net/wireless/ath/ath10k/wmi.h
> @@ -2906,6 +2906,42 @@ enum wmi_10_4_feature_mask {
>
>  };
>
> +/* WMI_GPIO_CONFIG_CMDID */
> +enum {
> +WMI_GPIO_PULL_NONE,
> +WMI_GPIO_PULL_UP,
> +WMI_GPIO_PULL_DOWN,
> +};
> +
> +enum {
> +WMI_GPIO_INTTYPE_DISABLE,
> +WMI_GPIO_INTTYPE_RISING_EDGE,
> +WMI_GPIO_INTTYPE_FALLING_EDGE,
> +WMI_GPIO_INTTYPE_BOTH_EDGE,
> +WMI_GPIO_INTTYPE_LEVEL_LOW,
> +WMI_GPIO_INTTYPE_LEVEL_HIGH
> +};
> +
> +/* WMI_GPIO_CONFIG_CMDID */
> +struct wmi_gpio_config_cmd {
> +__le32 gpio_num; /* GPIO number to be setup */
> +__le32 input;/* 0 - Output/ 1 - Input */
> +__le32 pull_type;/* Pull type defined above */
> +__le32 intr_mode;/* Interrupt mode defined above (Input) */
> +} __packed;
> +
> +/* WMI_GPIO_OUTPUT_CMDID */
> +struct wmi_gpio_output_cmd {
> +__le32 gpio_num;/* GPIO number to be setup */
> +__le32 set; /* Set the GPIO pin*/
> +} __packed;
> +
> +/* WMI_GPIO_INPUT_EVENTID */
> +struct wmi_gpio_input_event {
> +__le32 gpio_num;/* GPIO number which changed state */
> +} __packed;
> +
> +
>  struct wmi_ext_resource_config_10_4_cmd {
> /* contains enum wmi_host_platform_type */
> __le32 host_platform_config;
> --
> 2.14.1
>

The rest of this looks OK to me.

- Steve

--
Steve deRosier
Cal-Sierra Consulting LLC
https://www.cal-sierra.com/

Re: testing tool for packet delay

[Steve deRosier]

On Mon, Feb 19, 2018 at 9:19 AM, sdnlabs Janakaraj <wsupra...@gmail.com> wrote:
>
> Hello All,
>
> I am working on delay analysis of packets in wireless stacks. I am
> able to see lots of papers talk about therotical analysis. But I am
> more interested in practical evaluation.
>
> I am reaching out to developers community to learn how they actually
> evaluate the performance of the stack. In particular, I am looking for
> ideas to evaluate the delay experienced by packets within the
> mac802.11 stack.
>

Hi Prabhu,

Sounds like an interesting project. More often my instrumentation
focuses on aggregate performance and iperf throughput numbers suffices
for most of that. Occasionally I utilize internal performance measures
and/or data provided me by ftrace and related tools.

If you're unfamiliar with it, I suggest you look at the Bufferbloat
and Make-wifi-fast projects. I suspect that their data and techniques
and tools might be of interest to you:

https://www.bufferbloat.net/projects/make-wifi-fast/

Also (shameless plug), I'll be discussing WiFi integration debugging
techniques and tools at the ELC-NA conference in March. Some of it
might be relevant. If you're going, please stop by and say hi.

http://sched.co/DXn3

I hope that helps.

- Steve

--
Steve deRosier
Cal-Sierra Consulting LLC
https://www.cal-sierra.com/

Re: [PATCH] v2 ath10k: add LED and GPIO controlling support for various chipsets

[Steve deRosier]

On Fri, Feb 16, 2018 at 2:30 AM,   wrote:
> From: Sebastian Gottschall 
>
> Adds LED and GPIO Control support for 988x, 9887, 9888, 99x0, 9984 and 
> ipq4019 based chipsets with on chipset connected led's
> using WMI Firmware API.
> The LED device will get available named as "ath10k-phyX" at sysfs and can be 
> controlled with various triggers.
> adds also debugfs interface for gpio control.
>

Hi Sebastian,

First off, let me say I love this and the effort to make the gpios
hanging off the wifi chip as proper system gpios. Years ago I had to
do a hack to make some wifi-chip provided pins available to a customer
who needed more GPIOs on our SoM. It would have been nice if provided
hardware functionality had already been properly supported by the
driver.

In looking through, it looks like you're hooking into the standard
gpio-chip driver interface. Which is great. So, why are you providing
a separate out-of-band debugfs access interface for gpio control?
Seems to me to be a duplicate of the interface in sysfs that provides
gpio access (`/sys/class/gpio`). My preference is we don't duplicate
functionality and that we use the standard provided features. That way
we don't possibly confuse users, and we also reduce our maintenance
effort.

Additionally, what happens when the relevant GPIO configuration
options aren't enabled? Do we have a compilation issue? I'm more
familiar with being a _consumer_ of gpiolib in my drivers and I know
if CONFIG_GPIOLIB isn't checked I end up with problems. I'm unsure
what happens when you're presenting a gpio-chip interface without the
relevant CONFIG_ being enabled. Note that I didn't test this, I'm just
asking the question to be sure we don't forget anything.

- Steve

Re: Max clients on wifi access point with 7265

[Steve deRosier]

On Wed, Feb 14, 2018 at 2:04 PM, Samuel Sieb <sam...@sieb.net> wrote:
> On 02/14/2018 03:30 AM, Johannes Berg wrote:
>>
>> On Wed, 2018-02-14 at 10:55 +, Mickaël PANNEQUIN wrote:
>>
>>>
>>> Do you know the limit of the number of users connected at the same
>>> time on the wifi? Works fine with 14 connected devices but not more.
>>>
>>> How to increase it?
>>
>>
>> You can't.
>>
>>> This limit is hardware?
>>
>>
>> More or less, yes. The HW/FW can support 16 STAs, but needs two for
>> other bookkeeping.
>
>
> As a general question, is there a standard way to determine this limit for
> any particular hardware?


Yes, but there's no easy way. Put it in AP mode and connect clients to
it until it starts rejecting new clients or dropping the old ones.
Usually while watching it with a sniffer. That's how I've always had
to do it.

Different chips will have different limits, and there's no reliable
way to determine the limit across all chips other than trial and
error. It seems a common desire of people to try to use client chips
as poor-man APs. While it will work for very limited number of
clients, they're not intended as AP chips. If you want something to
work as an AP, I recommend you choose an AP chip.

- Steve

--
Steve deRosier
Cal-Sierra Consulting LLC
https://www.cal-sierra.com/

Re: [PATCH v2] ath9k: mark RSSI as invalid if frame received during channel setup

[Steve deRosier]

On Wed, Feb 14, 2018 at 8:26 AM, Jean Pierre TOSONI  wrote:
> ath9k returns a wrong RSSI value for frames received in a 30ms time
> window after a channel change. The correct value is typically 10dB
> below the returned value.
>
> This was found with a Atheros AR9300 Rev:3 chip (WLE350NX / JWX6083
> cards), during offchannel scans.
>

Is this true for ALL ath9k chips or only for the specific device? If
it's not generally true, we shouldn't be adding code that applies to
all. Perhaps a quirk or keyed to a specific chip id or DT entry or
other way to guard it?

- Steve

Presentation at ELC on debugging wifi modules

[Steve deRosier]

Hi Linux Wireless driver maintainers,

I'm giving a talk at ELC NA next month in Portland and I'd like a
little help from our other driver maintainers. The talk is titled:
"Reliable Linux Wireless - Techniques for Debugging Wireless Module
Integrations". While I'm familiar with a fairly wide variety of WiFi
chips and adapters and drivers, I don't know them all. I will speak
from my experience and I'm sure I won't have time to get into the
detailed ins-and-outs of every driver, but I'd love to include value
for everyone no matter which wifi chip vendor the audience member is
using.

So, if you have any specific tips or tricks you'd like to share with
me so I can include them in the materials for my talk I would
appreciate it.  I'm looking for those harder to find things: the
little known debugfs knobs, module parameters, device tree bindings,
and so on. Or, those things that are are common tripping up points.
Anything you find useful or think others might.

Also, part of my talk will be about how to reach out to those of us on
the list for help. We all know the calls for help come to us in
varying quality and I'd like to explain all the stuff to do first and
all the information to gather and send to us as they reach out. So
please send me any thoughts on what you'd like to see in the emails
when people ask for help.

You're welcome to do it on-list if you desire to elicit discussion,
but in order to keep the noise down feel free to reply to me directly
off-list.

For those that are not of the maintainer types that happen to be on
this list, if you're looking for help or tips and tricks, please
attend my talk at ELC or otherwise come and find me. All are welcome.

Thanks,
- Steve

Re: [RFC v4 00/18] ath10k high latency

[Steve deRosier]

Hi Erik,

On Sun, Dec 31, 2017 at 9:29 AM, Erik Stromdahl
 wrote:
> Uplink test results (WUSB6100M -> other computer):
>
> # iperf-client.sh 192.168.1.244
> /usr/bin/iperf
> 
> Client connecting to 192.168.1.244, TCP port 1234
> TCP window size: 85.0 KByte (default)
> 
> [  3] local 192.168.1.190 port 50524 connected with 192.168.1.244 port
> 1234
> [ ID] Interval   Transfer Bandwidth
> [  3]  0.0-10.1 sec  10.8 MBytes  8.95 Mbits/sec
>
> The result is not so good (I expect more from an 11ac device),
> but hopefully it is configuration related.
>

Indeed. As a point of reference, an AR6004 device I have on my desk
(which is a/b/g/n, no ac), built into a custom SBC, I can normally get
iperf runs of >35 Mbits/sec.  So, in short, that run is showing
there's an issue. I don't have your device so I can't really help out
much more than that, but in the next week or so I'll try to look at
the patches and see if something pops to my eye.

- Steve

Re: [PATCH] ath6kl: remove redundant variable ies_len

[Steve deRosier]

On Sat, Nov 25, 2017 at 9:38 PM, Kenneth Lu <kuohsian...@gmail.com> wrote:
> To get rid of W=1 warning: variable ‘ies_len’ set but not used.
> Variable ies_len is being assigned but never read.
>
> Signed-off-by: Kenneth Lu <kuohsian...@gmail.com>
> ---
>  drivers/net/wireless/ath/ath6kl/cfg80211.c | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath6kl/cfg80211.c 
> b/drivers/net/wireless/ath/ath6kl/cfg80211.c
> index b53eb2b..2ba8cf3 100644
> --- a/drivers/net/wireless/ath/ath6kl/cfg80211.c
> +++ b/drivers/net/wireless/ath/ath6kl/cfg80211.c
> @@ -2766,7 +2766,6 @@ static int ath6kl_start_ap(struct wiphy *wiphy, struct 
> net_device *dev,
> struct ieee80211_mgmt *mgmt;
> bool hidden = false;
> u8 *ies;
> -   int ies_len;
> struct wmi_connect_cmd p;
> int res;
> int i, ret;
> @@ -2804,7 +2803,6 @@ static int ath6kl_start_ap(struct wiphy *wiphy, struct 
> net_device *dev,
> ies = mgmt->u.beacon.variable;
> if (ies > info->beacon.head + info->beacon.head_len)
> return -EINVAL;
> -   ies_len = info->beacon.head + info->beacon.head_len - ies;
>
> if (info->ssid == NULL)
> return -EINVAL;

Oddly, ies_len was never even used in the original patch that added
it. Probably used in some debugging code that was stripped before
submitting. Seems safe to kill it.

Reviewed-by: Steve deRosier <deros...@gmail.com>

- Steve

Re: ath6kl : intermittent Wi-Fi on Fedora 27 (Linux 4.13.12-300)

[Steve deRosier]

Hi Ken,

On Fri, Nov 17, 2017 at 11:58 AM, Ken Harris  wrote:
> Yesterday, I installed Fedora 27 on a Dell Venue 8 Pro (5830).
>
> I installed the ath6kl firmware from https://github.com/qca/ath6kl-firmware/
>
> The Wi-Fi works for a while, but then stops.  It seems to start up
> again about 30 minutes later.
>
> Is this a known problem ? Is there any to fix it ?

Hard to know. You've not really given us enough information that would
be helpful to help you.  Kernel version? dmesg dump? Backports? Which
chip? SDIO or USB? Network manager or other?  All of this information
is necessary to help.

Usually your distro would come with the right firmware, and if it
doesn't, usually the proper place to get it from is
https://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-firmware.git/
 Though, if you're needing ar6004/hw3.0 it's missing for some reason
(@Kalle, why isn't that fw in linux-firmware?)

What I can tell you is I have ath6kl on many kernel versions on both
6003 and 6004 connected solid for months at a time with 0 noticeable
drops.

This might help:
https://wireless.wiki.kernel.org/en/users/documentation/reporting_bugs

>
> FYI, I added :
>
> echo options ath6kl_core debug_mask=0x140400 > /etc/modprobe.d/55-ath6k.conf
>
> ... so I'm getting more debug info.  Are the other debug bits I should try ?
>

Not bad, but I'd also suggest adding the recovery bit.  0x940400 would
be a good place to start.

Get us a kernel messages dump and maybe we can help more.

- Steve

Re: PS-Poll not behaving as expected

[Steve deRosier]

Hi Daniel,

On Mon, Nov 13, 2017 at 2:40 PM, Daniel Ghansah  wrote:
>Here is the problem we experience:
>
> When the station (wifi devices) sends a null QoS frame with power save
> telling our AP that it is going into power save mode. RIght now, our
> problem is that the AP will not stop sending data packets to the
> station. Correct behavior is that the AP will buffer data packets and
> stop sending any data or null packets (WiFi Alliance allows at most 2
> of these packets to be sent by the AP before station sends ps-poll,
> exclude retries) until a PS-Poll packet is sent by the station to
> collect data. The AP should also respond only 1 packet per PS-Poll
> (exclude any retries). Right now, we are also experiencing that the AP
> will, at times, send more than 1 data packet to the station per
> PS-Poll. The first and second problems may be linked together.
>

You've given no context with which we can help you. Things like the
Linux version, the hardware running on the AP and client, which it is
you're trying to report a bug on (the AP or Client), dmesg logs,
wireless traces, and so on would all be somewhere between mandatory to
helpful information. Trying to give us nothing but your interpretation
of what the 802.11 standard says (which most of us know very well)
isn't a useful bug report.

This might help:
https://wireless.wiki.kernel.org/en/users/documentation/reporting_bugs

Please try again and send us the information we need to be able to help.

Thanks,
- Steve

Re: [PATCH] iw: add command to register and capture mgmt frames

[Steve deRosier]

Hi Sergey,

On Sat, Oct 14, 2017 at 2:00 PM, Sergey Matyukevich
 wrote:
> Add new command to register for receiving multiple mgmt frames,
> capture and print them. Frames are selected by their type and
> pattern containing their the first several bytes of the frame
> that should match.
>

Forgive me for asking, what does this provide that isn't already
available in tshark? And since we already have user-space tools
tailored for capturing and filtering any frames, including management
frames, why do we need to add this to iw?

- Steve

Re: [PATCH 02/11] ath10k_sdio: wb396 reference card fix

[Steve deRosier]

Hi Alagu,


On Sat, Sep 30, 2017 at 10:37 AM,  wrote:
>
> From: Alagu Sankar 
>
> The QCA9377-3 WB396 sdio reference card does not get initialized
> due to the conflict in uart gpio pins. This fix is not required
> for other QCA9377 sdio cards.
>
> Signed-off-by: Alagu Sankar 
> ---
>  drivers/net/wireless/ath/ath10k/core.c | 9 -
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/ath/ath10k/core.c 
> b/drivers/net/wireless/ath/ath10k/core.c
> index b4f66cd..86247c8 100644
> --- a/drivers/net/wireless/ath/ath10k/core.c
> +++ b/drivers/net/wireless/ath/ath10k/core.c
> @@ -1708,8 +1708,15 @@ static int ath10k_init_uart(struct ath10k *ar)
> return ret;
> }
>
> -   if (!uart_print)
> +   if (!uart_print) {
> +   /* Hack: override dbg TX pin to avoid side effects of default
> +* GPIO_6 in QCA9377 WB396 reference card
> +*/
> +   if (ar->hif.bus == ATH10K_BUS_SDIO)
> +   ath10k_bmi_write32(ar, hi_dbg_uart_txpin,
> +  ar->hw_params.uart_pin);

If it is indeed a "hack", then I don't think the maintainer should
accept this upstream. If you want it upstream you need a clean enough
implementation that doesn't need to be labeled a "hack".

Your commit message states that this is only needed for a very
specific card and not for other QCA9377 sdio cards. Yet, you're doing
this for all ATH10K_BUS_SDIO devices. Not good. I think that it's a
quirk and it's limited to a particular implementation of the device.
My suggestion: if it can be automatically determined, then do so
explicitly. If not, then it needs to be a DT setting or a module
parameter or something like that so the platform maker can decide to
do it. Having it affect all users of a SDIO QCA9377 when it doesn't
apply doesn't seem like a good idea to me.


- Steve

carl9170 issue with sniffer mode and dropping probe responses

[Steve deRosier]

Hi,

A patch went in to the carl9170 driver about 5 years go that addressed
an issue with spurious ack noise from the hardware when it was put in
sniffer mode. It changed the driver to drop the sniffer mode and
instead keep it in STA mode with relaxed RX filtering. Patch [1]
Discussion [2]

All was thought to be well, but as it turns out it causes a specific
problem with injection as reported on the Kali Linux bug list [3].
Turns out it's not an issue with injection as the device transmits the
injected packets to the air just fine, but it's an issue with the
hardware filtering out the probe responses that are sent back to the
fake randomized MAC address that aireplay-ng is using as the sender on
the injected packets. aireplay-ng needs to get these probe response
packets back but they're getting filtered out by the hardware and
never even make it back up to the driver.

I've played with every RX filter flag and various attempts to
manipulate the MAC filters and I can't get anything to work. Only by
putting the AR9170_MAC_SNIFFER_ENABLE_PROMISC flag back in so we have
real monitor mode again, I was able to solve the issue. By putting the
flag back, the relevant probe responses are able to get back up to the
driver and aireplay-ng works again with carl9170.

I spent a long time avoiding adding sniffer mode back in due to the
explanation comments in the code. I played with the rx_fliter_cmd and
turned off the filters. I played with REG_MAC_ADDR, REG_BSSID and
REG_FRAMETYPE_FILTER. I attempted to manipulate the MAC filters by
playing with REG_GROUP_HASH_TBL.  Basically, I thoroughly went through
carl9170.h and played with every register  and command that looked
relevant.  If there's any other filter or MAC address filtering I
missed, please advise.

As mentioned, I put the sniffer back in play. But I chose to keep
AR9170_MAC_RX_CTRL_ACK_IN_SNIFFER off. I throughly tested the
degradation mentioned that resulted in the earlier patch. With the
sniffer flag ON but dropping RX_CTRL_ACK_IN_SNIFFER, I didn't see the
channel throughput hit. The second I toggled the RX_CTRL flag back on,
the throughput on the channel dropped and I started seeing the effects
the previous patch mentioned.

So, I think we can add AR9170_MAC_SNIFFER_ENABLE_PROMISC back in with
AR9170_MAC_RX_CTRL_ACK_IN_SNIFFER kept off and end up with carl9170
working in true sniffer mode but without getting the earlier problem.
Note that this is with the most current firmware, the 1.9.9, which is
newer than the original fix. Perhaps changes in firmware behavior are
resulting in the differences?

For specific point of discussion, I'll put my patch at the end of this
email.  Please advise to approach or anything that looks wrong and
I'll make adjustments and respin. Or if it looks OK, I'll submit the
patch.


Thanks,
- Steve

[1] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit?id=e0509d3bdd7365d06c9bf570bf9f118cae6cbd58
[2] https://marc.info/?l=linux-wireless=134517238506033
[3] https://bugs.kali.org/view.php?id=4220

>From c46a994dd78befbe94e66771db41c18351be2aae Mon Sep 17 00:00:00 2001
From: Steve deRosier <deros...@cal-sierra.com>
Date: Fri, 29 Sep 2017 10:48:19 -0700
Subject: [PATCH] wireless: carl9170: Enable sniffer mode promisc flag to fix
 injection

The removal of the AR9170_MAC_SNIFFER_ENABLE_PROMISC flag to fix an issue
many years ago caused the AR9170 to not be able to pass probe response
packets with different MAC addresses back up to the driver. In general
operation, this doesn't matter, but in the case of packet injection with
aireplay-ng it is important. aireplay-ng specifically injects packets with
spoofed MAC addresses on the probe requests and looks for probe responses
back to those addresses. No other combination of filter flags seem to fix
this issue and so AR9170_MAC_SNIFFER_ENABLE is required to get these packets.

This was originally caused by commit e0509d3bdd7365d06c9bf570bf9f11 which
removed this flag in order to avoid spurious ack noise from the hardware.
In testing for this issue, keeping this flag but not restoring the
AR9170_MAC_RX_CTRL_ACK_IN_SNIFFER flag on the rc_ctrl seems to solve this
issue, at least with the most current firmware v1.9.9.

Signed-off-by: Steve deRosier <deros...@cal-sierra.com>
---
 drivers/net/wireless/ath/carl9170/mac.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/wireless/ath/carl9170/mac.c
b/drivers/net/wireless/ath/carl9170/mac.c
index 7d4a72dc98db..c617e883f47a 100644
--- a/drivers/net/wireless/ath/carl9170/mac.c
+++ b/drivers/net/wireless/ath/carl9170/mac.c
@@ -309,6 +309,7 @@ int carl9170_set_operating_mode(struct ar9170 *ar)
  u32 rx_ctrl = AR9170_MAC_RX_CTRL_DEAGG |
   AR9170_MAC_RX_CTRL_SHORT_FILTER;
  u32 sniffer = AR9170_MAC_SNIFFER_DEFAULTS;
+ u32 mac_ftf = AR9170_MAC_FTF_DEFAULTS;
  int err = 0;

  rcu_read_lock();
@@ -373,6 +374,9 @@ int carl9170_set_operating_mode(struct ar9170 *ar)

  if (ar->sniffer_ena

Re: Kernel developer to work on wifi injection in mainline

[Steve deRosier]

Hi Raphael,

On Thu, Sep 14, 2017 at 7:31 AM, Raphael Hertzog  wrote:
> (Please keep-me in CC and use reply-to all)
>
...
>
> Thus we are looking for a kernel developer that we could work with on a
> contractor basis to:
> - make wifi injection work on the current Kali kernel on various drivers
>   that we want to support
> - upstream as much as possible of changes needed for wifi injection to
>   work
> - ensure continued support of the feature in new Linux releases
>

I do this sort of consulting work. While I kept this reply on the list
due to your instructions, I'll send you information directly from my
consulting address.

> I wonder whether there are non-technical hurdles that would prevent
> merging of wifi injection patches. Are there legal challenges to this for
> instance?
>

I don't think there would be a legal challenge provided the code is
yours to sign-off on and isn't patent encumbered.

Personally, I think the biggest issue is interest of the Linux
community as a whole. In other words, if you can convince maintainers
that the change is:
1. valuable to the Linux community members
2. not harmful to the community as a whole
3. worth maintaining (and in part will be responsible for maintaining)
4. not purely based on closed business reasons
then I think it'll be possible to get it upstream.

I'm not that familiar with the patch(es) in question but from my quick
look I don't see an obvious blocking issue with the concepts here.

- Steve

Re: [PATCH v2] brcmfmac: Correctly fail to suspend when SDIO does not support power on suspend

[Steve deRosier]

On Fri, Sep 1, 2017 at 1:02 PM, Arend van Spriel
<arend.vanspr...@broadcom.com> wrote:
> Also note that the wifi chip (the term "radio" does not quite cover it) has
> not really lost power. It is quite common that it is not powered through the
> SDIO bus. With the power-sequence support in the MMC stack these days the
> suspend may result in loss of power. Otherwise, it is just the bus that
> loses power (and clock) and the flags affect what tricks the MMC stack tries
> to pull to get the device accessible again upon resume.
>
Arend,

Indeed - I'm familiar with the platform that Eric is using and it is
exactly as you say: the chip is powered externally. There's some
platform issues here that need to be resolved in addition, but the
change to brcmfmac stands alone OK.

> Maybe you can incorporate some of this in the commit message, but you should
> at least add:
>
> Fixes: 330b4e4be937 ("brcmfmac: Add wowl support for SDIO devices.")
> Fixes: bdf1340cf20f ("brcmfmac: fix sdio suspend and resume")
> Reviewed-by: Arend van Spriel <arend.vanspr...@broadcom.com>
>
>> Signed-off-by: Eric Bentley eric.bent...@lairdtech.com

I've tested the patch on my platforms and works as expected. Eric,
please add my:

Tested-by: Steve deRosier <deros...@gmail.com>

Thanks,
- Steve

Re: [PATCH] Correctly fail to suspend when SDIO does not support power on suspend

[Steve deRosier]

On Thu, Aug 31, 2017 at 4:44 PM, Eric Bentley
 wrote:
> -   if (sdio_set_host_pm_flags(sdiodev->func[1], sdio_flags))
> +   if (sdio_set_host_pm_flags(sdiodev->func[1], sdio_flags)) (

Hi Eric,

Shouldn't that be a '{' instead of '('?  Maybe I'm missing some
context, but that doesn't look compilable.

:)

- Steve

Re: [PATCH v4] brcmfmac: add CLM download support

[Steve deRosier]

On Mon, Aug 28, 2017 at 2:25 AM, Wright Feng  wrote:
> From: Chung-Hsien Hsu 
>
> The firmware for brcmfmac devices includes information regarding
> regulatory constraints. For certain devices this information is kept
> separately in a binary form that needs to be downloaded to the device.
> This patch adds support to download this so-called CLM blob file. It
> uses the same naming scheme as the other firmware files with extension
> of .clm_blob.
>

Not to bikeshed this, but why not just ".clm"?  ".clm_blob" seems
unnecessary and could feel "unprofessional" to users.

To me simple seems better.

- Steve

Re: [PATCH v2 02/20] ath6kl: constify usb_device_id

[Steve deRosier]

On Wed, Aug 9, 2017 at 9:23 AM, Arvind Yadav <arvind.yadav...@gmail.com> wrote:
> usb_device_id are not supposed to change at runtime. All functions
> working with usb_device_id provided by  work with
> const usb_device_id. So mark the non-const structs as const.
>
> Signed-off-by: Arvind Yadav <arvind.yadav...@gmail.com>
> ---
> changes in v2:
>   Re-submitting wireless separately.
>
>  drivers/net/wireless/ath/ath6kl/usb.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/ath/ath6kl/usb.c 
> b/drivers/net/wireless/ath/ath6kl/usb.c
> index 9da3594..4defb7a 100644
> --- a/drivers/net/wireless/ath/ath6kl/usb.c
> +++ b/drivers/net/wireless/ath/ath6kl/usb.c
> @@ -1201,7 +1201,7 @@ static int ath6kl_usb_pm_resume(struct usb_interface 
> *interface)
>  #endif
>
>  /* table of devices that work with this driver */
> -static struct usb_device_id ath6kl_usb_ids[] = {
> +static const struct usb_device_id ath6kl_usb_ids[] = {
> {USB_DEVICE(0x0cf3, 0x9375)},
> {USB_DEVICE(0x0cf3, 0x9374)},
> { /* Terminating entry */ },
> --
> 2.7.4
>

Looks good. Also builds and works properly.

Reviewed-by: Steve deRosier <deros...@gmail.com>
Tested-by: Steve deRosier <deros...@gmail.com>

- Steve

Re: [PATCH] ath6kl: fix spelling mistake: "Indicat" -> "Indicate"

[Steve deRosier]

On Sun, Jun 4, 2017 at 9:36 AM, Colin King <colin.k...@canonical.com> wrote:
> From: Colin Ian King <colin.k...@canonical.com>
>
> Trivial fix to spelling mistake in ath6kl_dbg debug message
>
> Signed-off-by: Colin Ian King <colin.k...@canonical.com>
> ---
>  drivers/net/wireless/ath/ath6kl/htc_pipe.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/ath/ath6kl/htc_pipe.c 
> b/drivers/net/wireless/ath/ath6kl/htc_pipe.c
> index d127a08d60df..d4fd9e40fffb 100644
> --- a/drivers/net/wireless/ath/ath6kl/htc_pipe.c
> +++ b/drivers/net/wireless/ath/ath6kl/htc_pipe.c
> @@ -383,7 +383,7 @@ static enum htc_send_queue_result htc_try_send(struct 
> htc_target *target,
> list_for_each_entry_safe(packet, tmp_pkt,
>  txq, list) {
> ath6kl_dbg(ATH6KL_DBG_HTC,
> -  "%s: Indicat overflowed TX pkts: 
> %p\n",
> +  "%s: Indicate overflowed TX pkts: 
> %p\n",
>__func__, packet);
> action = ep->ep_cb.tx_full(ep->target, 
> packet);
>     if (action == HTC_SEND_FULL_DROP) {
> --
> 2.11.0
>

Looks good to me.

Reviewed-by: Steve deRosier <deros...@gmail.com>

- Steve

Re: [v2,1/3] ath9k: Support channels in licensed bands

[Steve deRosier]

Hi Ben,

On Fri, May 12, 2017 at 7:12 AM, Ben Greear  wrote:
>
>
> On 05/11/2017 04:38 AM, Kalle Valo wrote:
>>
>> Simon Wunderlich  writes:
>>
>>> it seems like there was some discussion here and I wouldn't expect too
>>> many
>>> more opinions ... do you think we can have a decision based on what has
>>> been
>>> discussed here?
>>
>>
>> Well, there was an excellent reply from Steve and quite a few "in my
>> opinion this is safe" type of answers. [1] But bluntly said it does not
>> really matter what we (the engineers) think. What really matters here
>> are regulatory authorities' opinions and rulings. In that respect here
>> are few items I want to point out:
>>
>> o I suspect that from someone's, who is not familiar with software
>>   engineering, point of view there is still quite a diffference from
>>   modifiying source code or just enabling few options from Kconfig with
>>   a custom regdb rule.
>
>
> If someone with real authority ever complains, then the code could be
> backed out again.  Your opinion seems not much more informed than the
> rest of us discussing this.
>

With all due respect, _I_ am quite well informed about this issue.
>From my conversations with him, I'd also judge Kalle to be quite
informed likewise.

"Someone with real authority" has already complained. That's the
entire point here. For the past two+ years it's been difficult to get
modular approvals through the FCC and their TCBs. The standard they
are applying is both pointless and technically misinformed, but
they've got the authority and they're using it. I can't speak for any
other manufacturers other than what I specifically have experience
with, but I expect the rest are having similar conversations with the
TCBs. From my work with them, I know that the chip manufacturers like
QCA, Marvell and Cypress/Broadcom are also having similar issues.
However, the chip manufacturers, and end-user equipment manufacturers
are less vulnerable here than the module manufacturers.


>>
>> o I don't know about other countries, but in Finland applying for any
>>   type of license (even if just to a license to drive a moped) needs
>>   both time and money. So there is significant effort anyway needed to
>>   legally use this band. And how many users there really are is unclear.
>
>
> This is almost certainly not going to be used by regular end-users.  It is
> going to be
> used by public safety organizations who are buying equipment from companies
> using open-wrt, LEDE, or similar, or possibly small teams at public safety
> organizations doing the work themselves.  Rather than having each of these
> vendors

I agree 100% that this won't be used by "regular end-users", if you
define those users as the 99% of the population who will go to Best
Buy and buy a router, plug it into their network and use it to get
their iOS updates and download movies from Netflix. These aren't the
users that the FCC is worried about.  They're concerned about the user
who buys an OpenWRT compatible router, installs OpenWRT on it, builds
a custom kernel. And that person happens to live in an area with
congested WiFi bands, the person notices an option to use these other
non-congested bands, decides that ignoring the warning would be
harmless and the FCC won't ever know anyway.

And then there's a fire or other event in the area, and the emergency
workers can't communicate. At best case, the FCC investigates and
slaps a $25,000 fine on the operator. At worst case, someone dies.

I don't feel it's responsible as an engineer to hide behind "well, we
warned them that they shouldn't do that" while ignoring my culpability
in the matter. Setting the stage for others to fail due to their
ignorance is not ethical.

Anyone building a custom kernel is able to enable a config option. You
can say that, "sure they can pull the patch from here and enable it
anyway" or "they can figure it out and do it themselves, it's easy",
but I do think there's a significant difference between being able to
build a custom kernel with a configuration enabled vs being able to
find and apply a random patch to a kernel and get it building and
working. That's a different level of expertise. And going through that
effort gives a person some time to reflect and think about if it
should be done in the first place.

Adding this code to mainline makes it a feature of Linux and this
driver. There's no way around that argument. Saying that "this is
almost certainly not going to be used by regular end-users" is both
fairly true as well as disingenuous. You're still making it an
accepted feature.


> hack their own crap into their kernels or forcing the the organizations to
> use Cisco gear,
> we could work on it together.
>

I absolutely concur with the idea of "we could work on it together."
Working together means coming up with a holistic plan that will work
and satisfy the regulatory agencies (and I'm not just talking FCC,
though they're right now 

Re: [PATCH] ath10k: add configurable debugging.

[Steve deRosier]

Hi Adrian,

On Wed, May 10, 2017 at 9:25 AM, Adrian Chadd  wrote:

> diff --git a/drivers/net/wireless/ath/ath10k/debug.h 
> b/drivers/net/wireless/ath/ath10k/debug.h
> index 257d10985c6e..7bd461927029 100644
> --- a/drivers/net/wireless/ath/ath10k/debug.h
> +++ b/drivers/net/wireless/ath/ath10k/debug.h
> @@ -200,27 +200,43 @@ void ath10k_sta_update_rx_duration(struct ath10k *ar,
>  #endif /* CONFIG_MAC80211_DEBUGFS */
>
>  #ifdef CONFIG_ATH10K_DEBUG
> -__printf(3, 4) void ath10k_dbg(struct ath10k *ar,
> +static inline int
> +_ath10k_do_dbg(struct ath10k *ar, enum ath10k_debug_mask mask)
> +{
> +   if (ar->trace_debug_mask & mask)
> +   return (1);
> +   if (ar->debug_mask & mask)
> +   return (1);
> +   return (0);
> +}
> +
> +void _ath10k_dbg(struct ath10k *ar,
>enum ath10k_debug_mask mask,
>const char *fmt, ...);
> -void ath10k_dbg_dump(struct ath10k *ar,
> +
> +void _ath10k_dbg_dump(struct ath10k *ar,
>  enum ath10k_debug_mask mask,
>  const char *msg, const char *prefix,
>  const void *buf, size_t len);
> +
> +#defineath10k_dbg(ar, mask, ...) 
>   \
> +   do {\
> +   if (_ath10k_do_dbg(ar, mask)) { \
> +   _ath10k_dbg((ar), (mask), __VA_ARGS__); \
> +   };  \
> +   } while (0)
> +

Looks to me you dropped the "__printf(3, 4)" safety check. Was that intentional?

Thanks,
- Steve

Re: ath6kl: assure headroom of skbuff is writable in .start_xmit()

[Steve deRosier]

On Wed, Apr 26, 2017 at 12:54 PM, James Hughes
<james.hug...@raspberrypi.org> wrote:
> On 26 April 2017 at 19:03, Arend Van Spriel
> <arend.vanspr...@broadcom.com> wrote:
>>
>>
>> On 26-4-2017 17:44, Steve deRosier wrote:
>>> On Wed, Apr 26, 2017 at 1:53 AM, Kalle Valo <kv...@codeaurora.org> wrote:
>>>> Arend Van Spriel <arend.vanspr...@broadcom.com> wrote:
>>>>> An issue was found brcmfmac driver in which a skbuff in .start_xmit()
>>>>> callback was actually cloned. So instead of checking for sufficient
>>>>> headroom it should also be writable. Hence use skb_cow_head() to
>>>>> check and expand the headroom appropriately.
>>>>>
>>>>> Signed-off-by: Arend van Spriel <arend.vanspr...@broadcom.com>
>>>>
>>>> Steve, would you have time to run a quick test with this?
>>>>
>>>> Patch set to Deferred.
>>>>
>>>
>>> Happy to give it a quick spin on both of my platforms.
>>>

@ Arend and James, thanks for the info. I understand it, but
unfortunately I can't seem to replicate the problems on my platforms
with the limited time I have available to test it. It also may have to
do with my platforms having special custom bridging related code, or
just me having setup too simple of a test.

That said...

@Kalle: I have tested on both my 6004 and 6003 platforms. I didn't
notice any incorrect behavior in my testing. But I don't have a test
setup that would have shown the original problem as reported on the
brcm driver so I can't say that the change actually _fixes_ anything.
Only that in my testing it doesn't seem to break anything.

Tested-by: Steve deRosier <deros...@gmail.com>

- Steve

Re: ath6kl: assure headroom of skbuff is writable in .start_xmit()

[Steve deRosier]

On Wed, Apr 26, 2017 at 1:53 AM, Kalle Valo  wrote:
> Arend Van Spriel  wrote:
>> An issue was found brcmfmac driver in which a skbuff in .start_xmit()
>> callback was actually cloned. So instead of checking for sufficient
>> headroom it should also be writable. Hence use skb_cow_head() to
>> check and expand the headroom appropriately.
>>
>> Signed-off-by: Arend van Spriel 
>
> Steve, would you have time to run a quick test with this?
>
> Patch set to Deferred.
>

Happy to give it a quick spin on both of my platforms.

@Arend: is there some demonstrable before/after that shows a problem I
can detect at runtime?  I understand your thought about putting a
skb_clone() in there, but what are the expectations?  And is any
problem evident without explicitly modding the code with the clone?

- Steve

Re: [v2,1/3] ath9k: Support channels in licensed bands

[Steve deRosier]

Hi,

(sorry, resending due to my not noticing that gmail had changed my
default compose mode to HTML. Why does it randomly do that
sometimes?!?!)

On Tue, Apr 18, 2017 at 7:50 AM, Simon Wunderlich  
wrote:
>
> Hi,
>
> On Tuesday, April 18, 2017 2:36:54 PM CEST Kalle Valo wrote:
> > Simon Wunderlich  wrote:
> > > From: Ben Greear 
> > >
> > > Many chips support channels in licensed bands. Add support for those,
> > > along with a corresponding kernel config option to disable them by

...

> > I am not sure that we should support unlicensed bands in Linux and hence
> > hesitant to apply these. My view is that due to regulatory restrictions we
> > should not make it too easy to use unlicensed bands. But I'm open for
> > discussion, this is a challenging area and my knowledge here is limited.
>
...
>
>
> In my personal view, we have quite a few obstacles which I consider "enough",
> but would be interesting to hear others opinions ...
>

I'll throw in my 2-cents. This patch is treading on very dangerous
ground. I can't speak to other regulatory environments, but at least
the FCC is cracking down on even the possibility that anyone can
operate a WiFi device outside the regulatory bounds.

I know this is going to be TLDR, but please bear with me.

The testing groups are (incorrectly in my opinion) interpreting the
current FCC guidelines to be that no one with the device could ever
get in and change it to operate outside the permitted frequencies and
powers. And I'm not even talking about having a command-line interface
and issuing a command as sudo. To the degree that if it's even
possible to recompile a driver or otherwise change the source code and
put that changed code on the device they're denying modular
certifications.

The end result is we have a lot of chip manufacturers' scrambling to
do things like require eeproms, signed board files, etc. Module
manufacturers (think of things like Laird's msd45nbt, msd50nbt, or the
Sterling LWB) are scrambling even harder trying to think of ways to
force chips to fail to function if they aren't using their regulatory
file or other strategies to manage to fulfill their customer's needs
while still getting it to pass through the regulatory agencies.

@Simon, with much respect: With the current regulatory environment,
those obstacles which you are considering "enough", the testing boards
aren't considering "enough".

I happen to agree with you that it's more than enough. And just
because it's possible for a customer of a module who is integrating a
device to operate it out of the regulatory bounds, doesn't mean it
shouldn't get the modular certification. In fact, depending on the
exact situations, it might be _necessary_ for them to make setting
adjustments for their products. And the reality is, anyone with a
soldering iron can make the thing operate outside the regulatory
domain anyway. The whole thing just makes it impossible for modular
operators and for the Linux community instead of solving any real
problems.

What's going on in the FCC regulatory environment has stark
implications that those of us in the Linux wireless community can't
afford to ignore anymore. This is a direct threat to mac80211, Open
Source and the ability to do anything sane with our devices. And even
if you're more practical (like me) about these things, think of it:
each manufacturer being forced to make it harder and harder for anyone
to change the code or work with their chips - you think it's hard to
work with a Marvell or Broadcom chip right now with minimal and
non-accessible documentation? Imagine how it's going to be when
everything is locked behind Secure Boot and signed proprietary drivers
where the hardware itself is locked so it can only work with the (bad
quality and buggy) closed-source driver.

I brought this up at Wireless Summit at the last LPC and basically got
a room of shrugs. Admittedly I wasn't terribly eloquent on the subject
so it's solely my fault I didn't impress. I know that most of us are
representing various companies (though not I anymore) and each has
their own proprietary way to deal with it and no one wants to rock the
boat*. And many of the people in the room are just implementing code
to make stuff work and don't actually know that much depth about the
regulatory environment in which we're working. But we all need to get
together and come up with a better solution.

What's going on right now doesn't serve our interests. I know it's an
agenda being pushed by someone and while I have a few suspects I
really don't know for sure. In any case, who it's being pushed by and
for doesn't matter too much - we as a community aren't pushing back as
a cohesive group; we aren't even talking about it! And so, we're going
to get the short end of the stick.

So, with re: to the patch, that's the environment it will live in.
Personally I don't really care one way or another specifically to what
Simon's 

Re: [PATCH] ath6kl: Add __printf verification to ath6kl_dbg

[Steve deRosier]

On Fri, Mar 31, 2017 at 10:45 AM, Joe Perches <j...@perches.com> wrote:
> On Fri, 2017-03-31 at 10:34 -0700, Steve deRosier wrote:
>> On Fri, Mar 31, 2017 at 10:23 AM, Joe Perches <j...@perches.com> wrote:
>> > On Fri, 2017-03-31 at 10:19 -0700, Steve deRosier wrote:
>> > > On Thu, Mar 30, 2017 at 3:57 PM, Joe Perches <j...@perches.com> wrote:
>> > > > Fix fallout too.
>> >
>> > []
>> > > My only question is why bother doing a format check on something
>> > > that's going to be compiled out anyway?
>> >
>> > To avoid introducing defects when writing new code
>> > and not using the debugging code path.
>> >
>>
>> Fair enough. And I totally agree with the defensive programming here
>> in that case and feel it's worth the tradeoff (if indeed there really
>> is any cost, I'm unsure what gcc actually does in this instance).
>>
>> For sake of discussion though - shouldn't anything not using the debug
>> code path in this case always be of the form that compiles out? ie
>> would be empty functions intended here just to make compilation work
>> and the code that depends on it simpler? Thus, there really should
>> never be a risk of introducing said defects. If any "real" code were
>> put in that else clause, that'd be a big red-flag in the review of
>> said hypothetical patch.
>
> Generically, all debugging forms should strive to avoid side-effects.
>

Yes. Of course. Lightbulb.

I wasn't even thinking of the fact someone could load the printf
arguments with code that might have side-effects instead of simple
variables to print. I never do it for obvious reasons, but I could see
it happening.

Thanks for spending the time going back and forth with me about it.

Thanks,
- Steve

Re: [PATCH] ath6kl: Add __printf verification to ath6kl_dbg

[Steve deRosier]

On Fri, Mar 31, 2017 at 10:23 AM, Joe Perches <j...@perches.com> wrote:
> On Fri, 2017-03-31 at 10:19 -0700, Steve deRosier wrote:
>> On Thu, Mar 30, 2017 at 3:57 PM, Joe Perches <j...@perches.com> wrote:
>> > Fix fallout too.
> []
>> My only question is why bother doing a format check on something
>> that's going to be compiled out anyway?
>
> To avoid introducing defects when writing new code
> and not using the debugging code path.
>

Fair enough. And I totally agree with the defensive programming here
in that case and feel it's worth the tradeoff (if indeed there really
is any cost, I'm unsure what gcc actually does in this instance).

For sake of discussion though - shouldn't anything not using the debug
code path in this case always be of the form that compiles out? ie
would be empty functions intended here just to make compilation work
and the code that depends on it simpler? Thus, there really should
never be a risk of introducing said defects. If any "real" code were
put in that else clause, that'd be a big red-flag in the review of
said hypothetical patch.

Thanks,
- Steve

Re: [PATCH] ath6kl: Add __printf verification to ath6kl_dbg

[Steve deRosier]

On Thu, Mar 30, 2017 at 3:57 PM, Joe Perches <j...@perches.com> wrote:
> Fix fallout too.
>
> Signed-off-by: Joe Perches <j...@perches.com>
> ---
>  drivers/net/wireless/ath/ath6kl/debug.h| 2 ++
>  drivers/net/wireless/ath/ath6kl/htc_pipe.c | 2 +-
>  drivers/net/wireless/ath/ath6kl/wmi.c  | 2 +-
>  3 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath6kl/debug.h 
> b/drivers/net/wireless/ath/ath6kl/debug.h
> index 0614393dd7ae..94297572914f 100644
> --- a/drivers/net/wireless/ath/ath6kl/debug.h
> +++ b/drivers/net/wireless/ath/ath6kl/debug.h
> @@ -63,6 +63,7 @@ int ath6kl_read_tgt_stats(struct ath6kl *ar, struct 
> ath6kl_vif *vif);
>
>  #ifdef CONFIG_ATH6KL_DEBUG
>
> +__printf(2, 3)
>  void ath6kl_dbg(enum ATH6K_DEBUG_MASK mask, const char *fmt, ...);
>  void ath6kl_dbg_dump(enum ATH6K_DEBUG_MASK mask,
>  const char *msg, const char *prefix,
> @@ -83,6 +84,7 @@ int ath6kl_debug_init_fs(struct ath6kl *ar);
>  void ath6kl_debug_cleanup(struct ath6kl *ar);
>
>  #else
> +__printf(2, 3)
>  static inline void ath6kl_dbg(enum ATH6K_DEBUG_MASK dbg_mask,
>   const char *fmt, ...)
>  {

My only question is why bother doing a format check on something
that's going to be compiled out anyway? I suppose the only harm is a
tiny extra bit of compile time due to the check and I'm sure that's
measured in micro-seconds on full development systems, but if we do it
everywhere those tiny bits of time would eventually add up.

Admittedly it's a comment that probably isn't worth redoing the commit
over.  I guess I'm bringing up the point more discuss the question:
"Should we add the printf format verification on the clauses that get
compiled out?"

So, it looks good to me as is, or if you feel like making the change
I'm suggesting, that's fine too.  And it builds and runs on my
platforms.

Reviewed-by: Steve deRosier <deros...@gmail.com>

- Steve

Re: [PATCH v9] Add new mac80211 driver mwlwifi.

[Steve deRosier]

On Tue, Feb 7, 2017 at 10:15 PM, David Lin <d...@marvell.com> wrote:
>> Rafał Miłecki [mailto:zaj...@gmail.com] wrote:
>> On 7 February 2017 at 20:12, Steve deRosier <deros...@gmail.com> wrote:
>> >> + /* look for all matching property names */
>> >> + for_each_property_of_node(priv->dt_node, prop) { if
>> >> + (strcmp(prop->name, "marvell,2ghz") == 0)
>> >> + priv->disable_2g = true;
>> >> + if (strcmp(prop->name, "marvell,5ghz") == 0)
>> >> + priv->disable_5g = true;
>> >> + if (strcmp(prop->name, "marvell,chainmask") == 0) { prop_value =
>> >> + be32_to_cpu(*((__be32 *)prop->value)); if (prop_value == 2)
>> >> + priv->antenna_tx = ANTENNA_TX_2;
>> >> +
>> >> + prop_value = be32_to_cpu(*((__be32 *) (prop->value + 4))); if
>> >> + (prop_value == 2)
>> >> + priv->antenna_rx = ANTENNA_RX_2;
>> >> + }
>> >> + }
>> >> +
>> >> + priv->pwr_node = of_find_node_by_name(priv->dt_node,
>> >> +  "marvell,powertable");
>> >> +#endif
>> >> +}
>> >
>> > AFAICT, there's no documentation for these DT bindings. The mwlwifi
>> > node and the marvell,powertable both need full documentation in
>> > Documentation/devicetree/bindings/... .
>> >
>> > Frankly I have a feeling I'm going to need these DT nodes and I'd like
>> > to not have to guess-and-check based on the code.
>>
>> Please use ieee80211-freq-limit:
>> https://git.kernel.org/cgit/linux/kernel/git/davem/net-next.git/commit/?id=b3
>> 30b25eaabda00d74e47566d9200907da381896
>>
>> Most likely with wiphy_read_of_freq_limits helper:
>> https://git.kernel.org/cgit/linux/kernel/git/davem/net-next.git/commit/?id=e6
>> 91ac2f75b69bee743f0370d79454ba4429b175
>
> I already replied meaning of these parameters:
> <marvell,2ghz> is used to disable 2g band.
> <marvell,5ghz> is used to disable 5g band.
> <marvell, chainmask> is used to specify antenna number (if default number is 
> suitable for you, there is no need to use this parameter).
> <marvell,powertable> should not be used for chip with device power table. In 
> fact, this parameter should not be used any more.
>

David, I think you're not understanding the comment, or at least
that's what it looks like to me. Yes, you did reply as to the meaning.
And, your reply, while informative, didn't tell us you understood and
were willing to fix the problem. I doubt you meant it this way, but it
feels defensive and like a brush-off.

First off, you will still have to document any DT bindings you're
adding. Just because you answer the question in the review doesn't
mean you're not responsible for doing so.

And second off, I think that Rafal (and sorry about my spelling, looks
like there's some sort of accent on the l that I don't know how to
make my keyboard do) is saying: there's already some generic bindings
that can be used to disable the 2g or 5g bands. Granted they're even
newer than your patch, but I do think if said bindings exist and are
appropriate, you should use them.

- Steve

Re: [PATCH v9] Add new mac80211 driver mwlwifi.

[Steve deRosier]

Hi David,

First off, I wanted to say thank-you for your work and effort in trying
to get mwlwifi upstream. My comments are in-line with my general notes
afterwards.

On Tue, Dec 20, 2016 at 8:11 PM, David Lin  wrote:

> diff --git a/drivers/net/wireless/marvell/mwlwifi/debugfs.h 
> b/drivers/net/wireless/marvell/mwlwifi/debugfs.h
> new file mode 100644
> index 000..b4c3eb3
> --- /dev/null
> +++ b/drivers/net/wireless/marvell/mwlwifi/debugfs.h
> @@ -0,0 +1,24 @@
> +/*
> + * Copyright (C) 2006-2016, Marvell International Ltd.
> + *
> + * This software file (the "File") is distributed by Marvell International
> + * Ltd. under the terms of the GNU General Public License Version 2, June 
> 1991
> + * (the "License").  You may use, redistribute and/or modify this File in
> + * accordance with the terms and conditions of the License, a copy of which
> + * is available by writing to the Free Software Foundation, Inc.
> + *
> + * THE FILE IS DISTRIBUTED AS-IS, WITHOUT WARRANTY OF ANY KIND, AND THE
> + * IMPLIED WARRANTIES OF MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE
> + * ARE EXPRESSLY DISCLAIMED.  The License provides additional details about
> + * this warranty disclaimer.
> + */
> +
> +/* Description:  This file defines debug fs related functions. */
> +
> +#ifndef _MWL_DEBUGFS_H_
> +#define _MWL_DEBUGFS_H_
> +
> +void mwl_debugfs_init(struct ieee80211_hw *hw);
> +void mwl_debugfs_remove(struct ieee80211_hw *hw);
> +

You should guard these so they define to empty functions if CONFIG_DEBUG_FS
isn't enabled so they can be used by a caller without explicit guards.

> diff --git a/drivers/net/wireless/marvell/mwlwifi/dev.h 
> b/drivers/net/wireless/marvell/mwlwifi/dev.h
> new file mode 100644
> index 000..c7b10ac
> --- /dev/null
> +++ b/drivers/net/wireless/marvell/mwlwifi/dev.h
...
> +struct mwl_ampdu_stream {
> + struct ieee80211_sta *sta;
> + u8 tid;
> + u8 state;
> + u8 idx;
> +};
> +
> +#ifdef CONFIG_DEBUG_FS
> +#define MAC_REG_ADDR_PCI(offset)  ((priv->iobase1 + 0xA000) + (offset))
> +
> +#define MWL_ACCESS_MAC1
> +#define MWL_ACCESS_RF 2
> +#define MWL_ACCESS_BBP3
> +#define MWL_ACCESS_CAU4
> +#define MWL_ACCESS_ADDR0  5
> +#define MWL_ACCESS_ADDR1  6
> +#define MWL_ACCESS_ADDR   7
> +#endif

OK, I get that you're only using the above in the debugfs functions, but
as for generic register access functions, these would be valid no matter if
CONFIG_DEBUG_FS is defined. Having the guard here just seems to complicate
things.


> +
> +struct mwl_priv {
> + struct ieee80211_hw *hw;
> + struct firmware *fw_ucode;
> + bool fw_device_pwrtbl;
> + bool forbidden_setting;
> + bool regulatory_set;
> + u32 fw_region_code;
> + char fw_alpha2[2];
> + u8 number_of_channels;
> + struct mwl_device_pwr_tbl device_pwr_tbl[SYSADPT_MAX_NUM_CHANNELS];
> + int chip_type;
> +
> + struct device_node *dt_node;
> + struct device_node *pwr_node;
> + bool disable_2g;
> + bool disable_5g;
> + int antenna_tx;
> + int antenna_rx;
> +
> + struct mwl_tx_pwr_tbl tx_pwr_tbl[SYSADPT_MAX_NUM_CHANNELS];
> + bool cdd;
> + u16 txantenna2;
> + u8 powinited;
> + u16 max_tx_pow[SYSADPT_TX_POWER_LEVEL_TOTAL]; /* max tx power (dBm) */
> + u16 target_powers[SYSADPT_TX_POWER_LEVEL_TOTAL]; /* target powers   */
> +
> + struct pci_dev *pdev;
> + struct device *dev;
> + void __iomem *iobase0; /* MEM Base Address Register 0  */
> + void __iomem *iobase1; /* MEM Base Address Register 1  */
> + u32 next_bar_num;
> +
> + struct mutex fwcmd_mutex;/* for firmware command */
> + unsigned short *pcmd_buf;/* pointer to CmdBuf (virtual)  */
> + dma_addr_t pphys_cmd_buf;/* pointer to CmdBuf (physical) */
> + bool in_send_cmd;
> +
> + int irq;
> + struct mwl_hw_data hw_data;  /* Adapter HW specific info */
> +
> + /* various descriptor data */
> + /* for tx descriptor data  */
> + spinlock_t tx_desc_lock cacheline_aligned_in_smp;
> + struct mwl_desc_data desc_data[SYSADPT_NUM_OF_DESC_DATA];
> + struct sk_buff_head txq[SYSADPT_NUM_OF_DESC_DATA];
> + struct sk_buff_head delay_q;
> + /* number of descriptors owned by fw at any one time */
> + int fw_desc_cnt[SYSADPT_NUM_OF_DESC_DATA];
> +
> + struct tasklet_struct tx_task;
> + struct tasklet_struct tx_done_task;
> + struct tasklet_struct rx_task;
> + struct tasklet_struct qe_task;
> + int txq_limit;
> + bool is_tx_done_schedule;
> + int recv_limit;
> + bool is_rx_schedule;
> + bool is_qe_schedule;
> + u32 qe_trigger_num;
> + unsigned long qe_trigger_time;
> +
> + struct timer_list period_timer;
> +
> + s8 noise;/* Most recently reported noise in dBm */
> + struct ieee80211_supported_band band_24;
> + struct ieee80211_channel channels_24[BAND_24_CHANNEL_NUM];
> + struct ieee80211_rate rates_24[BAND_24_RATE_NUM];
> + struct ieee80211_supported_band band_50;
> + struct ieee80211_channel channels_50[BAND_50_CHANNEL_NUM];
> + struct 

Re: [RFC 0/5] ath6kl: non WMI data service support

[Steve deRosier]

Hi Eric,

On Thu, Oct 13, 2016 at 9:39 AM, Erik Stromdahl
 wrote:
> This patch series is intended to prepare the ath6kl driver
> for newer chipsets that doesn't use the current WMI data
> endpoints for data traffic.
>
> The chipset I have been working with (and used for testing)
> is QCA6584. It is SDIO based (at least the variant I have
> been using) with 802.11p WAVE DSRC capabilities.
>
> This chipset is different from the AR600X family in that
> it does not use the WMI data services (service id's 0x101
> to 0x104 ) for data traffic.
> Instead it uses the HTT data service for data and wmi unified
> for control messages.
> It is also different when it comes to mailbox addresses
> and HTC header format as well, but these differences are not
> part of this patch series.
>

I've only taken a quick look and I'll make specific comments to
specific patches later, but I've got to ask the question, should these
changes go into ath6kl or be a new driver?

Just because the number starts with 6000 doesn't mean it's a ath6kl
chip. The 10k series chips all start with 9000 for example, but they
rate their own driver.

You state that all of the underpinnings of the communication with the
chip are totally different:
* Doesn't use WMI
* Different mailboxing
* Different HTC layer

If all of the commands and all of the communication layers to the chip
are totally different, then perhaps it isn't an ath6kl chip. So if
it's largely similar, then OK, but seems to me with the changes you're
saying above, it's mostly different.

I'm saying all that without any knowledge of this chip. My experience
is limited to various versions of the 6003 and 6004 chips.

Just trying to stimulate discussion here.

Thanks,
- Steve

Re: [PATCH] ath6k1: add Dell OEM SDIO I/O for the Venue 8 Pro

[Steve deRosier]

Hi Alan,

Sorry to quibble, but the subsystem label on the commit subject line
should be "ath6kl:"  it's a lower-case "L", not a one.


On Wed, Oct 12, 2016 at 11:08 AM, Alan <a...@linux.intel.com> wrote:
> From: Adam Williamson <ad...@happyassassin.net>
>
> SDIO ID 0271:0418
>
> Signed-off-by: Alan Cox <a...@linux.intel.com>
> Bugzilla-ID: https://bugzilla.kernel.org/show_bug.cgi?id=67921
>
> diff --git a/drivers/net/wireless/ath/ath6kl/sdio.c 
> b/drivers/net/wireless/ath/ath6kl/sdio.c
> index eab0ab9..76eb336 100644
> --- a/drivers/net/wireless/ath/ath6kl/sdio.c
> +++ b/drivers/net/wireless/ath/ath6kl/sdio.c
> @@ -1401,6 +1401,7 @@ static const struct sdio_device_id 
> ath6kl_sdio_devices[] = {
> {SDIO_DEVICE(MANUFACTURER_CODE, (MANUFACTURER_ID_AR6004_BASE | 0x0))},
> {SDIO_DEVICE(MANUFACTURER_CODE, (MANUFACTURER_ID_AR6004_BASE | 0x1))},
> {SDIO_DEVICE(MANUFACTURER_CODE, (MANUFACTURER_ID_AR6004_BASE | 0x2))},
> +   {SDIO_DEVICE(MANUFACTURER_CODE, (MANUFACTURER_ID_AR6004_BASE | 
> 0x18))},
> {},
>  };
>
>

I see nothing wrong with this if the chip does indeed identify itself
this way.  So please fix the subject and you can add:

Reviewed-by: Steve deRosier <steve.deros...@lairdtech.com>

- Steve

Re: [PATCH] ath6kl: enable firmware crash dumps on the AR6004

[Steve deRosier]

On Wed, Aug 3, 2016 at 1:43 PM,  <engineer...@keppy.com> wrote:
> From: Dan Kephart <dan.keph...@lairdtech.com>
>
> The firmware crash dumps on the 6004 are the same as the 6003. Remove the
> statement guarding it from dumping on the 6004.  Renamed the
> REG_DUMP_COUNT_AR6003 to reflect support on both chips.
>
> Signed-off-by: Dan Kephart <dan.keph...@lairdtech.com>

Since ath6kl only supports the 6003 and 6004, this looks fine to me
and works OK on my platforms.

Reviewed-by: Steve deRosier <steve.deros...@lairdtech.com>
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ath6kl: Add ability to set debug uart baud rate

[Steve deRosier]

Hi Julian,

First off, let me say I do appreciate your comments and I do
understand your perspective. I also generally prefer not to let users
shoot-themselves in the foot if it's avoidable.

In this case, however, I don't happen to agree with you. For one
specific reason: I don't want to say what baud rates are "safe"
because I think that's even more dangerous than not checking - because
we have no way of actually knowing what is or isn't for a particular
chip.

Oddly enough I thought this all out before I ever sent the patch up.

>
> I'm of the opinion that one should never underestimate the ability of
> people to attempt to shoot themselves in the foot. However this is
> only a debugging interface so you do make a good point.
>
> I guess I'm worried that some idiot is going to set it to 2 baud or 2
> billion baud for some dumb reason then come complaining to us when
> their wireless card crashes or locks up or something.
>
> Maybe we can just sweep this all under the carpet by putting all the
> debug uart stuff behind some nice #ifdef.

Well, first off the debugging stuff was never under some #ifdef. So,
we should make it even more complicated and add an #ifdef and yet
another kconfig option?

AFAIK, the firmware would be perfectly happy with 2 baud. I'm not in
my lab to try it right now, but it might well be (though your
throughput would be crap). Nor do I know the upper limit of the
register the firmware uses.

My firmware guy wanted 115200, and I could've hard-coded it to that
value, but I figured a bit of flexibility was warranted and would be
more upstreamable. I don't know every single valid or invalid value
for every ar6xxx chip. If we have it check for the value, then we have
some obligation to know the values we let in are valid for either all
or at least the chip the user is using. I don't know what was invalid
for many species of 6002. Or even all of the 6003 and 6004s and I've
been working with both the firmware and driver for these chips for 3
years now. What might be valid on the yet to be imagined 6009?

If we check, we are saying, "these are safe values and we want you to use them".

99.999% of users don't have access to this pin without a soldering
iron. I think someone who is going to tack a wire to their 6k chip is
entitled to set even stupid values if they think they have a reason.

Again, simply my perspective.

On a compromise: do you have a specific list of baud rates you'd like
to support and you know are valid across a wide swath of ath6kl chips?
Every rate I've tried, normal or weird, works fine. Granted, I haven't
tried anything slower than 9600, nor have I bothered checking the
clock error on the weird rates. If you have said list, I'd be happy to
code it up. But I think that specifically checking for rates is the
same as saying "this rate is supported" and I don't know that, so I
hope you do know what ones are valid.

Maybe you agree with my line of thinking (now that you know what it
is) or maybe not. That's OK. ;)

Thanks,
- Steve
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ath6kl: Add ability to set debug uart baud rate

[Steve deRosier]

Hi Julian,

Thanks for looking at the patch.

On Sun, Apr 17, 2016 at 5:07 PM, Julian Calaby  wrote:
> Hi Steve,
>
> This looks like it's using standard serial rates. Does it accept
> non-standard rates? If not, should this be checked before being passed
> to the hardware?
>

It should use standard serial rates, and obviously I defaulted it to a
sensible one. However, there's nothing that prevents you from using
odd and arbitrary rates in the firmware and it should just suck it in
and use it. In other words, the firmware will happily try to set 9507
baud if you tell it to.

This is specifically for debugging and working with the firmware and
IMHO, if you are using this feature and feel a need to set it to
something weird, I think the user should be allowed to. If you know
enough to be able to shoot yourself in the foot with this, then you
should know enough of what you should or shouldn't do and should be
allowed to. It's a debugging tool only and I don't see any reason to
waste lines in the driver to validate this input.

That's my perspective on the issue, but I recognize other opinions may
differ. Or you may know of a specific chip where it does matter (the
6003s and 6004s I'm using don't seem to care). If you or Kalle insist
on it, I'll be happy to add in some value checking for this parameter.
It just doesn't seem worth-while to me.

- Steve
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] ath6kl: Add ability to set debug uart baud rate

[Steve deRosier]

It's useful to permit the customization of the debug uart baud rate. Enable
this and send down the value to the chip if we're enabling debug.

Signed-off-by: Steve deRosier <steve.deros...@lairdtech.com>
---
 drivers/net/wireless/ath/ath6kl/core.c | 3 +++
 drivers/net/wireless/ath/ath6kl/core.h | 1 +
 drivers/net/wireless/ath/ath6kl/init.c | 8 
 3 files changed, 12 insertions(+)

diff --git a/drivers/net/wireless/ath/ath6kl/core.c 
b/drivers/net/wireless/ath/ath6kl/core.c
index 4ec02ce..ebb9f16 100644
--- a/drivers/net/wireless/ath/ath6kl/core.c
+++ b/drivers/net/wireless/ath/ath6kl/core.c
@@ -31,6 +31,7 @@ unsigned int debug_mask;
 static unsigned int suspend_mode;
 static unsigned int wow_mode;
 static unsigned int uart_debug;
+static unsigned int uart_rate = 115200;
 static unsigned int ath6kl_p2p;
 static unsigned int testmode;
 static unsigned int recovery_enable;
@@ -40,6 +41,7 @@ module_param(debug_mask, uint, 0644);
 module_param(suspend_mode, uint, 0644);
 module_param(wow_mode, uint, 0644);
 module_param(uart_debug, uint, 0644);
+module_param(uart_rate, uint, 0644);
 module_param(ath6kl_p2p, uint, 0644);
 module_param(testmode, uint, 0644);
 module_param(recovery_enable, uint, 0644);
@@ -180,6 +182,7 @@ int ath6kl_core_init(struct ath6kl *ar, enum 
ath6kl_htc_type htc_type)
 
if (uart_debug)
ar->conf_flags |= ATH6KL_CONF_UART_DEBUG;
+   ar->hw.uarttx_rate = uart_rate;
 
set_bit(FIRST_BOOT, >flag);
 
diff --git a/drivers/net/wireless/ath/ath6kl/core.h 
b/drivers/net/wireless/ath/ath6kl/core.h
index 5f3acfe..80ce54d 100644
--- a/drivers/net/wireless/ath/ath6kl/core.h
+++ b/drivers/net/wireless/ath/ath6kl/core.h
@@ -781,6 +781,7 @@ struct ath6kl {
u32 board_addr;
u32 refclk_hz;
u32 uarttx_pin;
+   u32 uarttx_rate;
u32 testscript_addr;
u8 tx_ant;
u8 rx_ant;
diff --git a/drivers/net/wireless/ath/ath6kl/init.c 
b/drivers/net/wireless/ath/ath6kl/init.c
index 3daeb27..58fb227 100644
--- a/drivers/net/wireless/ath/ath6kl/init.c
+++ b/drivers/net/wireless/ath/ath6kl/init.c
@@ -651,6 +651,14 @@ int ath6kl_configure_target(struct ath6kl *ar)
if (status)
return status;
 
+   /* Only set the baud rate if we're actually doing debug */
+   if (ar->conf_flags & ATH6KL_CONF_UART_DEBUG) {
+   status = ath6kl_bmi_write_hi32(ar, hi_desired_baud_rate,
+  ar->hw.uarttx_rate);
+   if (status)
+   return status;
+   }
+
/* Configure target refclk_hz */
if (ar->hw.refclk_hz != 0) {
status = ath6kl_bmi_write_hi32(ar, hi_refclk_hz,
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] ath6kl: fix missing uart debug pin for 6004 HW 3.0

[Steve deRosier]

For some reason, the 6004 HW 3.0 definition was missing the value for the
uarttx_pin (used for firmware debug). This corrects this situation.

Signed-off-by: Steve deRosier <steve.deros...@lairdtech.com>
---
 drivers/net/wireless/ath/ath6kl/init.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/wireless/ath/ath6kl/init.c 
b/drivers/net/wireless/ath/ath6kl/init.c
index da557dc..3daeb27 100644
--- a/drivers/net/wireless/ath/ath6kl/init.c
+++ b/drivers/net/wireless/ath/ath6kl/init.c
@@ -173,6 +173,7 @@ static const struct ath6kl_hw hw_list[] = {
.reserved_ram_size  = 7168,
.board_addr = 0x436400,
.testscript_addr= 0,
+   .uarttx_pin = 11,
.flags  = 0,
 
.fw = {
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] ath6kl: ignore WMI_TXE_NOTIFY_EVENTID based on fw capability flags

[Steve deRosier]

Certain 6004 firmware releases redefine the WMI_TXE_NOTIFY_EVENTID event
number and sends the new event frequently. However it doesn't have the
tx-err-notify feature and thus this firmware capability flag isn't set on
the firmware package. By guarding the processing of this event by the same
method we guard the sending of the WMI_SET_TXE_NOTIFY_CMDID command, we
can ignore the spurious event that we don't know how to process.

Without this change we call cfg80211_cqm_txe_notify() with possibly bad
data.

Signed-off-by: Steve deRosier <steve.deros...@lairdtech.com>
---
 drivers/net/wireless/ath/ath6kl/wmi.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/wireless/ath/ath6kl/wmi.c 
b/drivers/net/wireless/ath/ath6kl/wmi.c
index a5e1de7..0b3e9c0 100644
--- a/drivers/net/wireless/ath/ath6kl/wmi.c
+++ b/drivers/net/wireless/ath/ath6kl/wmi.c
@@ -1584,6 +1584,11 @@ static int ath6kl_wmi_txe_notify_event_rx(struct wmi 
*wmi, u8 *datap, int len,
if (len < sizeof(*ev))
return -EINVAL;
 
+   if (vif->nw_type != INFRA_NETWORK ||
+   !test_bit(ATH6KL_FW_CAPABILITY_TX_ERR_NOTIFY,
+ vif->ar->fw_capabilities))
+   return -EOPNOTSUPP;
+
if (vif->sme_state != SME_CONNECTED)
return -ENOTCONN;
 
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 1/2] ath6kl_sdio: Add reset gpio module parameter for CHIP_PWD_L pin

[Steve deRosier]

Many ath6k chips have a reset pin, usually labeled CHIP_PWD_L. This pin can
be pulled low by the host processor to hold the wifi chip in reset. By
holding the chip in reset, the lowest power consumption available can be
achieved.

This adds a module parameter so the ath6kl_sdio driver can control the
CHIP_PWD_L pin if the implementer so desires. This code is only available
if GPIOLIB is configured.

Signed-off-by: Steve deRosier <steve.deros...@lairdtech.com>
---
 drivers/net/wireless/ath/ath6kl/sdio.c | 70 +-
 1 file changed, 69 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/ath/ath6kl/sdio.c 
b/drivers/net/wireless/ath/ath6kl/sdio.c
index eab0ab9..d06aa41 100644
--- a/drivers/net/wireless/ath/ath6kl/sdio.c
+++ b/drivers/net/wireless/ath/ath6kl/sdio.c
@@ -23,6 +23,7 @@
 #include 
 #include 
 #include 
+#include 
 #include "hif.h"
 #include "hif-ops.h"
 #include "target.h"
@@ -69,6 +70,20 @@ struct ath6kl_sdio {
spinlock_t wr_async_lock;
 };
 
+/* Delay to avoid the mmc driver calling the probe on the prior notice of
+ * the chip, which we just killed. If this is missing, it results in a
+ * spurious warning:
+ * "ath6kl_sdio: probe of mmc0:0001:1 failed with error -110"
+ *
+ * Time chosen experimentally, with padding
+ */
+#define ATH6KL_MMC_PROBE_DELAY 150
+static unsigned int reset_pwd_gpio = ARCH_NR_GPIOS;
+#ifdef CONFIG_GPIOLIB
+module_param(reset_pwd_gpio, uint, 0644);
+MODULE_PARM_DESC(reset_pwd_gpio, "WIFI CHIP_PWD reset pin GPIO");
+#endif
+
 #define CMD53_ARG_READ  0
 #define CMD53_ARG_WRITE 1
 #define CMD53_ARG_BLOCK_BASIS   1
@@ -1414,20 +1429,73 @@ static struct sdio_driver ath6kl_sdio_driver = {
.drv.pm = ATH6KL_SDIO_PM_OPS,
 };
 
+static int ath6kl_sdio_init_gpio(void)
+{
+   int ret = 0;
+
+   if (gpio_is_valid(reset_pwd_gpio)) {
+   /* Request the reset GPIO, and assert it to make sure we get a
+* clean boot in-case we had a floating input or other issue.
+*/
+   ret = gpio_request_one(reset_pwd_gpio,
+  GPIOF_OUT_INIT_LOW |
+  GPIOF_EXPORT_DIR_FIXED,
+  "WIFI_RESET");
+   if (ret) {
+   ath6kl_err("Unable to get WIFI power gpio: %d\n", ret);
+   return ret;
+   }
+
+   ath6kl_dbg(ATH6KL_DBG_SUSPEND, "Setup wifi gpio #%d\n",
+  reset_pwd_gpio);
+   usleep_range(20, 50); /* Pin must be asserted at least 5 usec */
+   gpio_set_value(reset_pwd_gpio, 1); /* De-assert the pin */
+
+   msleep(ATH6KL_MMC_PROBE_DELAY);
+   }
+
+   return 0;
+}
+
+static void ath6kl_sdio_release_gpio(void)
+{
+   if (gpio_is_valid(reset_pwd_gpio)) {
+   /* Be sure we leave the chip in reset when we unload and also
+* release the GPIO
+*/
+   gpio_set_value(reset_pwd_gpio, 0);
+   gpio_free(reset_pwd_gpio);
+   }
+}
+
 static int __init ath6kl_sdio_init(void)
 {
int ret;
 
-   ret = sdio_register_driver(_sdio_driver);
+   ret = ath6kl_sdio_init_gpio();
if (ret)
+   goto err_gpio;
+
+   ret = sdio_register_driver(_sdio_driver);
+   if (ret) {
ath6kl_err("sdio driver registration failed: %d\n", ret);
+   goto err_register;
+   }
 
return ret;
+
+err_register:
+   ath6kl_sdio_release_gpio();
+
+err_gpio:
+   return ret;
 }
 
 static void __exit ath6kl_sdio_exit(void)
 {
sdio_unregister_driver(_sdio_driver);
+
+   ath6kl_sdio_release_gpio();
 }
 
 module_init(ath6kl_sdio_init);
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 0/2] ath6kl_sdio: add control of CHIP_PWD_L via GPIO

[Steve deRosier]

This set of two patches adds the ablity for ath6kl_sdio to control the
CHIP_PWD_L pin on startup and for suspend/wakeup. This is importaint because
on some platforms, this is the only way to achieve minimum power consumption.
The CHIP_PWD_L pin is used to hold the ath chip in reset and this is the
proper way to achieve its lowest power state (per QCA's datasheets).

This GPIO is controled by the kernel standard GPIOLIB and as such depends on
CONFIG_GPIOLIB. If this isn't enabled, then the various usages will generally
compile out. Even if enabled, by default the GPIO is set to an invalid number,
and each use will check: if not a valid GPIO, then behavior will be unchanged.

This adds a new module parameter to allow the user to set the GPIO to use.

To utilize:
modprobe ath6kl_sdio reset_pwd_gpio=28
Where "28" should be replaced by the GPIO id of the pin connected to the
CHIP_PWD_L pin on the wifi chip.

v3: Minor changes due to review comments
* Move constant to top of file
* Remove unnecessary gpio valid check

v2: Changes due to review comments
* Fix __init and __exit on gpio init/cleanup functions
* Remove unnecessary #ifdef defines on GPIO as gpio.h already takes care of it
* Utilize already available ARCH_NR_GPIOS
* Remove msleep on exit as the problem it resolves is taken care of a different
  patch.

This applies against kvalo/ath.git master branch

Steve deRosier (2):
  ath6kl_sdio: Add reset gpio module parameter for CHIP_PWD_L pin
  ath6kl_sdio: Add power gpio reset feature into sdio driver for suspend

 drivers/net/wireless/ath/ath6kl/sdio.c | 113 -
 1 file changed, 110 insertions(+), 3 deletions(-)

-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 1/2] ath6kl_sdio: Add reset gpio module parameter for CHIP_PWD_L pin

[Steve deRosier]

Many ath6k chips have a reset pin, usually labeled CHIP_PWD_L. This pin can
be pulled low by the host processor to hold the wifi chip in reset. By
holding the chip in reset, the lowest power consumption available can be
achieved.

This adds a module parameter so the ath6kl_sdio driver can control the
CHIP_PWD_L pin if the implementer so desires. This code is only available
if GPIOLIB is configured.

Signed-off-by: Steve deRosier <steve.deros...@lairdtech.com>
---
 drivers/net/wireless/ath/ath6kl/sdio.c | 73 +-
 1 file changed, 72 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/ath/ath6kl/sdio.c 
b/drivers/net/wireless/ath/ath6kl/sdio.c
index eab0ab9..c0755eb 100644
--- a/drivers/net/wireless/ath/ath6kl/sdio.c
+++ b/drivers/net/wireless/ath/ath6kl/sdio.c
@@ -23,6 +23,7 @@
 #include 
 #include 
 #include 
+#include 
 #include "hif.h"
 #include "hif-ops.h"
 #include "target.h"
@@ -67,8 +68,15 @@ struct ath6kl_sdio {
 
/* protects access to wr_asyncq */
spinlock_t wr_async_lock;
+
 };
 
+static unsigned int reset_pwd_gpio = ARCH_NR_GPIOS;
+#ifdef CONFIG_GPIOLIB
+module_param(reset_pwd_gpio, uint, 0644);
+MODULE_PARM_DESC(reset_pwd_gpio, "WIFI CHIP_PWD reset pin GPIO");
+#endif
+
 #define CMD53_ARG_READ  0
 #define CMD53_ARG_WRITE 1
 #define CMD53_ARG_BLOCK_BASIS   1
@@ -1414,20 +1422,83 @@ static struct sdio_driver ath6kl_sdio_driver = {
.drv.pm = ATH6KL_SDIO_PM_OPS,
 };
 
+/* Delay to avoid the mmc driver calling the probe on the prior notice of
+ * the chip, which we just killed. If this is missing, it results in a
+ * spurious warning:
+ * "ath6kl_sdio: probe of mmc0:0001:1 failed with error -110"
+ *
+ * Time chosen experimentally, with padding
+ */
+#define ATH6KL_MMC_PROBE_DELAY 150
+
+static int ath6kl_sdio_init_gpio(void)
+{
+   int ret = 0;
+
+   if (gpio_is_valid(reset_pwd_gpio)) {
+   /* Request the reset GPIO, and assert it to make sure we get a
+* clean boot in-case we had a floating input or other issue.
+*/
+   ret = gpio_request_one(reset_pwd_gpio,
+  GPIOF_OUT_INIT_LOW |
+  GPIOF_EXPORT_DIR_FIXED,
+  "WIFI_RESET");
+   if (ret) {
+   ath6kl_err("Unable to get WIFI power gpio: %d\n", ret);
+   return ret;
+   }
+
+   ath6kl_dbg(ATH6KL_DBG_SUSPEND, "Setup wifi gpio #%d\n",
+  reset_pwd_gpio);
+   usleep_range(20, 50); /* Pin must be asserted at least 5 usec */
+   gpio_set_value(reset_pwd_gpio, 1); /* De-assert the pin */
+
+   msleep(ATH6KL_MMC_PROBE_DELAY);
+   }
+
+   return 0;
+}
+
+static void ath6kl_sdio_release_gpio(void)
+{
+   if (gpio_is_valid(reset_pwd_gpio)) {
+   /* Be sure we leave the chip in reset when we unload and also
+* release the GPIO
+*/
+   gpio_set_value(reset_pwd_gpio, 0);
+   gpio_free(reset_pwd_gpio);
+   }
+}
+
 static int __init ath6kl_sdio_init(void)
 {
int ret;
 
-   ret = sdio_register_driver(_sdio_driver);
+   ret = ath6kl_sdio_init_gpio();
if (ret)
+   goto err_gpio;
+
+   ret = sdio_register_driver(_sdio_driver);
+   if (ret) {
ath6kl_err("sdio driver registration failed: %d\n", ret);
+   goto err_register;
+   }
 
return ret;
+
+err_register:
+   ath6kl_sdio_release_gpio();
+
+err_gpio:
+   return ret;
 }
 
 static void __exit ath6kl_sdio_exit(void)
 {
sdio_unregister_driver(_sdio_driver);
+
+   if (gpio_is_valid(reset_pwd_gpio))
+   ath6kl_sdio_release_gpio();
 }
 
 module_init(ath6kl_sdio_init);
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 0/2] ath6kl_sdio: add control of CHIP_PWD_L via GPIO

[Steve deRosier]

This set of two patches adds the ablity for ath6kl_sdio to control the
CHIP_PWD_L pin on startup and for suspend/wakeup. This is importaint because
on some platforms, this is the only way to achieve minimum power consumption.
The CHIP_PWD_L pin is used to hold the ath chip in reset and this is the
proper way to achieve its lowest power state (per QCA's datasheets).

This GPIO is controled by the kernel standard GPIOLIB and as such depends on
CONFIG_GPIOLIB. If this isn't enabled, then the various usages will generally
compile out. Even if enabled, by default the GPIO is set to an invalid number,
and each use will check: if not a valid GPIO, then behavior will be unchanged.

This adds a new module parameter to allow the user to set the GPIO to use.

To utilize:
modprobe ath6kl_sdio reset_pwd_gpio=28
Where "28" should be replaced by the GPIO id of the pin connected to the
CHIP_PWD_L pin on the wifi chip.

v2:

Changes due to review comments
* Fix __init and __exit on gpio init/cleanup functions
* Remove unnecessary #ifdef defines on GPIO as gpio.h already takes care of it
* Utilize already available ARCH_NR_GPIOS
* Remove msleep on exit as the problem it resolves is taken care of a different
  patch.

This applies against kvalo/ath.git master branch

Steve deRosier (2):
  ath6kl_sdio: Add reset gpio module parameter for CHIP_PWD_L pin
  ath6kl_sdio: Add power gpio reset feature into sdio driver for suspend

 drivers/net/wireless/ath/ath6kl/sdio.c | 113 -
 1 file changed, 110 insertions(+), 3 deletions(-)

-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 2/2] ath6kl_sdio: Add power gpio reset feature into sdio driver for suspend

[Steve deRosier]

This change adds pm suspend callbacks in order to trigger a gpio line
to push the CHIP_PWD_L reset/power line low on suspend. This puts the chip
into the lowest power state on suspend. On resume, it releases the line,
allowing the chip to boot. Slower, but provides a clean reset of the
chip and recovery from standby.

Signed-off-by: Steve deRosier <steve.deros...@lairdtech.com>
---
 drivers/net/wireless/ath/ath6kl/sdio.c | 40 --
 1 file changed, 38 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/ath/ath6kl/sdio.c 
b/drivers/net/wireless/ath/ath6kl/sdio.c
index c0755eb..c411033 100644
--- a/drivers/net/wireless/ath/ath6kl/sdio.c
+++ b/drivers/net/wireless/ath/ath6kl/sdio.c
@@ -1288,8 +1288,44 @@ static int ath6kl_sdio_pm_resume(struct device *device)
return 0;
 }
 
-static SIMPLE_DEV_PM_OPS(ath6kl_sdio_pm_ops, ath6kl_sdio_pm_suspend,
-ath6kl_sdio_pm_resume);
+/* Below handlers leverage the PM system to make sure we turn on and off
+ * the power gpio at the right time. If we do it in the earlier power on
+ * and off handlers for the sdio, we get errors from the mmc subsystem.
+ */
+static int ath6kl_sdio_pm_suspend_late(struct device *device)
+{
+   ath6kl_dbg(ATH6KL_DBG_SUSPEND, "sdio pm ath6kl_sdio_pm_suspend_late\n");
+
+   if (gpio_is_valid(reset_pwd_gpio))
+   gpio_set_value(reset_pwd_gpio, 0);
+
+   return 0;
+}
+
+static int ath6kl_sdio_pm_resume_early(struct device *device)
+{
+   ath6kl_dbg(ATH6KL_DBG_SUSPEND, "sdio pm ath6kl_sdio_pm_resume_early\n");
+
+   if (gpio_is_valid(reset_pwd_gpio)) {
+   gpio_set_value(reset_pwd_gpio, 1);
+   usleep_range(1000, 5000); /* wait for power up */
+   }
+   return 0;
+}
+
+/* The GPIO version requires the more complex dev_pm_ops setup */
+const struct dev_pm_ops ath6kl_sdio_pm_ops = {
+   .suspend = ath6kl_sdio_pm_suspend,
+   .suspend_late = ath6kl_sdio_pm_suspend_late,
+   .resume_early = ath6kl_sdio_pm_resume_early,
+   .resume = ath6kl_sdio_pm_resume,
+   .freeze = ath6kl_sdio_pm_suspend,
+   .thaw = ath6kl_sdio_pm_resume,
+   .poweroff = ath6kl_sdio_pm_suspend,
+   .poweroff_late = ath6kl_sdio_pm_suspend_late,
+   .restore_early = ath6kl_sdio_pm_resume_early,
+   .restore = ath6kl_sdio_pm_resume,
+};
 
 #define ATH6KL_SDIO_PM_OPS (_sdio_pm_ops)
 
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] ath6kl: Don't print error message when recv is canceled

[Steve deRosier]

An error message ath6kl_htc_rxmsg_pending_handler isn't appropate for when
the error is ECANCELED. This could be the result of a perfectly appropriate
RX cancel due to shutdown or suspend. This allows the right cleanup to
continue, but without an alarming error message in this particular case.

Signed-off-by: Steve deRosier <steve.deros...@lairdtech.com>
---
 drivers/net/wireless/ath/ath6kl/htc_mbox.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/ath/ath6kl/htc_mbox.c 
b/drivers/net/wireless/ath/ath6kl/htc_mbox.c
index fffb65b..65c31da 100644
--- a/drivers/net/wireless/ath/ath6kl/htc_mbox.c
+++ b/drivers/net/wireless/ath/ath6kl/htc_mbox.c
@@ -,8 +,9 @@ int ath6kl_htc_rxmsg_pending_handler(struct htc_target 
*target,
}
 
if (status) {
-   ath6kl_err("failed to get pending recv messages: %d\n",
-  status);
+   if (status != -ECANCELED)
+   ath6kl_err("failed to get pending recv messages: %d\n",
+  status);
 
/* cleanup any packets in sync completion queue */
list_for_each_entry_safe(packets, tmp_pkt, _pktq, list) {
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] ath6kl_sdio: Add reset gpio module parameter for CHIP_PWD_L pin

[Steve deRosier]

Hi Julian,

Thanks for looking at this.

In short - I agree with your review and will do most of them. As a
well as a few minor changes suggested by the kbuild test robot. Expect
a new version shortly.

On Mon, Nov 16, 2015 at 10:25 PM, Julian Calaby  wrote:
>>
>>  static void __exit ath6kl_sdio_exit(void)
>>  {
>> sdio_unregister_driver(_sdio_driver);
>> +
>> +#ifdef CONFIG_GPIOLIB
>> +   /* Delay to avoid pulling the plug on the chip when an irq is pending
>> +* and then getting a spurious message:
>> +* "ath6kl: failed to get pending recv messages: -125"
>> +*/
>> +   msleep(300);
>
> Is there some actual synchronisation you can do here against the IRQ?
> 300msec isn't a long time to wait, but it seems wrong here.
>

Considering this is only called on exit, I wasn't too worried about
this, but then again, on reboot as well as a few types of
reconfiguring the interface speed is an important consideration for
our system.

I'm open to suggestions.  I had looked at this and didn't see an
obvious way to try to sync with the IRQ as it goes through several
subsystems.

But looking at it again in a different light...

125 is ECANCELED.  Which is exactly right and appropriate, and
ath6kl_htc_rxmsg_pending_handler() where the message comes from does a
proper cleanup, or so it looks like. I wonder if maybe we can consider
ECANCELED not an error that needs an error message as it might be a
reasonable case?

So instead of synchronization, just consider that particular status an
expected and properly handled exception condition and not print an
error?

Thoughts?

- Steve
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 0/2] ath6kl_sdio: add control of CHIP_PWD_L via GPIO

[Steve deRosier]

This set of two patches adds the ablity for ath6kl_sdio to control the
CHIP_PWD_L pin on startup and for suspend/wakeup. This is importaint because
on some platforms, this is the only way to achieve minimum power consumption.
The CHIP_PWD_L pin is used to hold the ath chip in reset and this is the
proper way to achieve its lowest power state (per QCA's datasheets).

This GPIO is controled by the kernel standard GPIOLIB and as such depends on
CONFIG_GPIOLIB. If this isn't enabled, then the various usages will generally
compile out. Even if enabled, by default the GPIO is set to an invalid number,
and each use will check: if not a valid GPIO, then behavior will be unchanged.

This adds a new module parameter to allow the user to set the GPIO to use.

To utilize:
modprobe ath6kl_sdio reset_pwd_gpio=28
Where "28" should be replaced by the GPIO id of the pin connected to the
CHIP_PWD_L pin on the wifi chip.


Steve deRosier (2):
  ath6kl_sdio: Add reset gpio module parameter for CHIP_PWD_L pin
  ath6kl_sdio: Add power gpio reset feature into sdio driver for suspend

 drivers/net/wireless/ath/ath6kl/sdio.c | 125 -
 1 file changed, 124 insertions(+), 1 deletion(-)

-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/2] ath6kl_sdio: Add reset gpio module parameter for CHIP_PWD_L pin

[Steve deRosier]

Many ath6k chips have a reset pin, usually labeled CHIP_PWD_L. This pin can
be pulled low by the host processor to hold the wifi chip in reset. By
holding the chip in reset, the lowest power consumption available can be
achieved.

This adds a module parameter so the ath6kl_sdio driver can control the
CHIP_PWD_L pin if the implementer so desires. This code is only available
if GPIOLIB is configured.

Signed-off-by: Steve deRosier <steve.deros...@lairdtech.com>
---
 drivers/net/wireless/ath/ath6kl/sdio.c | 80 +-
 1 file changed, 79 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/ath/ath6kl/sdio.c 
b/drivers/net/wireless/ath/ath6kl/sdio.c
index eab0ab9..7a732f3 100644
--- a/drivers/net/wireless/ath/ath6kl/sdio.c
+++ b/drivers/net/wireless/ath/ath6kl/sdio.c
@@ -67,8 +67,18 @@ struct ath6kl_sdio {
 
/* protects access to wr_asyncq */
spinlock_t wr_async_lock;
+
 };
 
+#ifdef CONFIG_GPIOLIB
+#include 
+#define NO_GPIO0x
+
+static unsigned int reset_pwd_gpio = NO_GPIO;
+module_param(reset_pwd_gpio, uint, 0644);
+MODULE_PARM_DESC(reset_pwd_gpio, "WIFI CHIP_PWD reset pin GPIO");
+#endif
+
 #define CMD53_ARG_READ  0
 #define CMD53_ARG_WRITE 1
 #define CMD53_ARG_BLOCK_BASIS   1
@@ -1414,20 +1424,88 @@ static struct sdio_driver ath6kl_sdio_driver = {
.drv.pm = ATH6KL_SDIO_PM_OPS,
 };
 
+static int __init ath6kl_sdio_init_gpio(void)
+{
+   int ret = 0;
+
+#ifdef CONFIG_GPIOLIB
+   if (!gpio_is_valid(reset_pwd_gpio))
+   return 0;
+
+   /* Request the reset GPIO, and assert it to make sure we get a 100%
+* clean boot in-case we had a floating input or other issue.
+*/
+   ret = gpio_request_one(reset_pwd_gpio,
+  GPIOF_OUT_INIT_LOW | GPIOF_EXPORT_DIR_FIXED,
+  "WIFI_RESET");
+   if (ret) {
+   ath6kl_err("Unable to get WIFI power gpio: %d\n", ret);
+   return ret;
+   }
+
+   ath6kl_dbg(ATH6KL_DBG_SUSPEND, "Setup wifi gpio #%d\n", reset_pwd_gpio);
+   usleep_range(20, 50); /* Pin must be asserted at least 5 usec */
+   gpio_set_value(reset_pwd_gpio, 1); /* De-assert the pin for operation */
+
+   /* Delay to avoid the mmc driver calling the probe on the prior notice
+* of the chip, which we just killed. If this is missing, it results in
+* a spurious warning:
+* "ath6kl_sdio: probe of mmc0:0001:1 failed with error -110"
+*/
+   msleep(150); /* Time chosen experimentally, with padding */
+#endif
+
+   return ret;
+}
+
+static void __exit ath6kl_sdio_release_gpio(void)
+{
+#ifdef CONFIG_GPIOLIB
+   if (gpio_is_valid(reset_pwd_gpio)) {
+   /* Be sure we leave the chip in reset when we unload and also
+* release the GPIO
+*/
+   gpio_set_value(reset_pwd_gpio, 0);
+   gpio_free(reset_pwd_gpio);
+   }
+#endif
+}
+
 static int __init ath6kl_sdio_init(void)
 {
int ret;
 
-   ret = sdio_register_driver(_sdio_driver);
+   ret = ath6kl_sdio_init_gpio();
if (ret)
+   goto err_gpio;
+
+   ret = sdio_register_driver(_sdio_driver);
+   if (ret) {
ath6kl_err("sdio driver registration failed: %d\n", ret);
+   goto err_register;
+   }
+
+   return ret;
+
+err_register:
+   ath6kl_sdio_release_gpio();
 
+err_gpio:
return ret;
 }
 
 static void __exit ath6kl_sdio_exit(void)
 {
sdio_unregister_driver(_sdio_driver);
+
+#ifdef CONFIG_GPIOLIB
+   /* Delay to avoid pulling the plug on the chip when an irq is pending
+* and then getting a spurious message:
+* "ath6kl: failed to get pending recv messages: -125"
+*/
+   msleep(300);
+   ath6kl_sdio_release_gpio();
+#endif
 }
 
 module_init(ath6kl_sdio_init);
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html