Search Linux Wireless

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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Steve,

On Fri, Apr 27, 2018 at 5:47 PM, Steve deRosier <derosier@xxxxxxxxx> wrote:
> Hi Alfonso,
>
> On Thu, Apr 12, 2018 at 8:42 AM Alfonso Sánchez-Beato <
> alfonso.sanchez-beato@xxxxxxxxxxxxx> 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-beato@xxxxxxxxxxxxx>
>> ---
>>   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(&info->beacon, (u8 *) &rsn_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 *) &rsn_capab,
>> -                                           sizeof(rsn_capab));
>> +       if (!ath6kl_get_rsn_capab(&info->beacon, (u8 *)&rsn_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

>
>
>> @@ -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
> device with the flag, but I'm willing to bet you neither tested it nor that
> the assumption is true.  I'm guessing this change just broke the 4-way
> handshake for the majority of devices.
>
>>                  ath6kl_band_2ghz.ht_cap.cap = 0;
>>                  ath6kl_band_2ghz.ht_cap.ht_supported = false;
>>                  ath6kl_band_5ghz.ht_cap.cap = 0;
>> diff --git a/drivers/net/wireless/ath/ath6kl/main.c
> b/drivers/net/wireless/ath/ath6kl/main.c
>> index db95f85751e3..4e186f8b3950 100644
>> --- a/drivers/net/wireless/ath/ath6kl/main.c
>> +++ b/drivers/net/wireless/ath/ath6kl/main.c
>> @@ -579,13 +579,9 @@ static int ath6kl_commit_ch_switch(struct ath6kl_vif
> *vif, u16 channel)
>>                   * reconfigure any saved RSN IE capabilites in the beacon
> /
>>                   * probe response to stay in sync with the supplicant.
>>                   */
>> -               if (vif->rsn_capab &&
>> -                   test_bit(ATH6KL_FW_CAPABILITY_RSN_CAP_OVERRIDE,
>> -                            ar->fw_capabilities))
>> -                       ath6kl_wmi_set_ie_cmd(ar->wmi, vif->fw_vif_idx,
>> -                                             WLAN_EID_RSN,
> WMI_RSN_IE_CAPB,
>> -                                             (const u8 *)
> &vif->rsn_capab,
>> -                                             sizeof(vif->rsn_capab));
>> +               if (vif->rsn_capab)
>> +                       ath6kl_wmi_set_rsn_cap(ar->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,
>>                                                      &vif->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.

Agreed. I do not have access to much information for the ath6k, so
this was more like a shot in the dark.

>
>> +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?

This is hw 3.0, there is actually no ath6k/AR6004/hw3.0 folder in
linux-firmware. I would appreciate pointers for more modern fw for
this hardware version if you have them.

>
> Thanks,
> - Steve

Thanks,
Alfonso

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



[Index of Archives]     [Linux Host AP]     [ATH6KL]     [Linux Wireless Personal Area Network]     [Linux Bluetooth]     [Wireless Regulations]     [Linux Netdev]     [Kernel Newbies]     [Linux Kernel]     [IDE]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite Hiking]     [MIPS Linux]     [ARM Linux]     [Linux RAID]

  Powered by Linux