From: Ajay Singh <[email protected]>
Sparse reported below warning
>> drivers/net/wireless/microchip/wilc1000/cfg80211.c:361:42: sparse: sparse: incorrect type in assignment (different base types) @@ expected unsigned int key_mgmt_suite @@ got restricted __be32 [usertype] @@
'key_mgmt_suite' value is expected in big-endian format. After receiving
mgmt_suite value from wpa_s convert to cpu endianness because the same
value is return back using cfg80211_external_auth_request(). Use
be32_to_cpu() conversion API to avoid the sparse warning.
Reported-by: kernel test robot <[email protected]>
Fixes: c5b331d4f550fb78 ("wifi: wilc1000: add WPA3 SAE support")
Signed-off-by: Ajay Singh <[email protected]>
---
drivers/net/wireless/microchip/wilc1000/cfg80211.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wireless/microchip/wilc1000/cfg80211.c b/drivers/net/wireless/microchip/wilc1000/cfg80211.c
index 5c2c7f1dbffd..19862d932f1f 100644
--- a/drivers/net/wireless/microchip/wilc1000/cfg80211.c
+++ b/drivers/net/wireless/microchip/wilc1000/cfg80211.c
@@ -359,7 +359,7 @@ static int connect(struct wiphy *wiphy, struct net_device *dev,
memcpy(vif->auth.ssid.ssid, sme->ssid, sme->ssid_len);
vif->auth.ssid.ssid_len = sme->ssid_len;
}
- vif->auth.key_mgmt_suite = cpu_to_be32(sme->crypto.akm_suites[0]);
+ vif->auth.key_mgmt_suite = be32_to_cpu((__force __be32)sme->crypto.akm_suites[0]);
ether_addr_copy(vif->auth.bssid, sme->bssid);
break;
--
2.34.1
From: Ajay Singh <[email protected]>
WID_TX_POWER WID value is fetched from the firmware so it should be
added in'g_cfg_byte' array to store the data which is received from
firmware.
Signed-off-by: Ajay Singh <[email protected]>
---
drivers/net/wireless/microchip/wilc1000/wlan_cfg.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/net/wireless/microchip/wilc1000/wlan_cfg.c b/drivers/net/wireless/microchip/wilc1000/wlan_cfg.c
index dba301378b7f..60eaf62fd164 100644
--- a/drivers/net/wireless/microchip/wilc1000/wlan_cfg.c
+++ b/drivers/net/wireless/microchip/wilc1000/wlan_cfg.c
@@ -22,6 +22,7 @@ static const struct wilc_cfg_byte g_cfg_byte[] = {
{WID_STATUS, 0},
{WID_RSSI, 0},
{WID_LINKSPEED, 0},
+ {WID_TX_POWER, 0},
{WID_WOWLAN_TRIGGER, 0},
{WID_NIL, 0}
};
--
2.34.1
From: Ajay Singh <[email protected]>
For string type WID packet, the data length is received as 16-bit value
so use 'get_unaligned_le16' conversion API to extract the correct length.
Signed-off-by: Ajay Singh <[email protected]>
---
drivers/net/wireless/microchip/wilc1000/wlan_cfg.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/net/wireless/microchip/wilc1000/wlan_cfg.c b/drivers/net/wireless/microchip/wilc1000/wlan_cfg.c
index 60eaf62fd164..131388886acb 100644
--- a/drivers/net/wireless/microchip/wilc1000/wlan_cfg.c
+++ b/drivers/net/wireless/microchip/wilc1000/wlan_cfg.c
@@ -181,9 +181,10 @@ static void wilc_wlan_parse_response_frame(struct wilc *wl, u8 *info, int size)
i++;
if (cfg->s[i].id == wid)
- memcpy(cfg->s[i].str, &info[2], info[2] + 2);
+ memcpy(cfg->s[i].str, &info[2],
+ get_unaligned_le16(&info[2]) + 2);
- len = 2 + info[2];
+ len = 2 + get_unaligned_le16(&info[2]);
break;
default:
--
2.34.1
From: Ajay Singh <[email protected]>
Set 'close' variable to '1' to indicate closing operation when
initialisation fails during wlan_initialize_threads() call.
Signed-off-by: Ajay Singh <[email protected]>
---
drivers/net/wireless/microchip/wilc1000/netdev.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wireless/microchip/wilc1000/netdev.c b/drivers/net/wireless/microchip/wilc1000/netdev.c
index fcc4e61592ee..7879446f282f 100644
--- a/drivers/net/wireless/microchip/wilc1000/netdev.c
+++ b/drivers/net/wireless/microchip/wilc1000/netdev.c
@@ -472,7 +472,7 @@ static int wlan_initialize_threads(struct net_device *dev)
"%s-tx", dev->name);
if (IS_ERR(wilc->txq_thread)) {
netdev_err(dev, "couldn't create TXQ thread\n");
- wilc->close = 0;
+ wilc->close = 1;
return PTR_ERR(wilc->txq_thread);
}
wait_for_completion(&wilc->txq_thread_started);
--
2.34.1
From: Ajay Singh <[email protected]>
Similar to SPI priv data, add 'isinit' variable in SDIO priv. Make use
of the state to invoke hif_init() once, and acquire the lock before
accessing hif function.
Signed-off-by: Ajay Singh <[email protected]>
---
drivers/net/wireless/microchip/wilc1000/sdio.c | 13 +++++++++++++
drivers/net/wireless/microchip/wilc1000/spi.c | 8 ++++++++
drivers/net/wireless/microchip/wilc1000/wlan.c | 9 ++++++---
drivers/net/wireless/microchip/wilc1000/wlan.h | 1 +
4 files changed, 28 insertions(+), 3 deletions(-)
diff --git a/drivers/net/wireless/microchip/wilc1000/sdio.c b/drivers/net/wireless/microchip/wilc1000/sdio.c
index 7962c11cfe84..600cc57e9da2 100644
--- a/drivers/net/wireless/microchip/wilc1000/sdio.c
+++ b/drivers/net/wireless/microchip/wilc1000/sdio.c
@@ -26,6 +26,7 @@ static const struct sdio_device_id wilc_sdio_ids[] = {
struct wilc_sdio {
bool irq_gpio;
u32 block_size;
+ bool isinit;
int has_thrpt_enh3;
};
@@ -193,6 +194,13 @@ static int wilc_sdio_reset(struct wilc *wilc)
return 0;
}
+static bool wilc_sdio_is_init(struct wilc *wilc)
+{
+ struct wilc_sdio *sdio_priv = wilc->bus_data;
+
+ return sdio_priv->isinit;
+}
+
static int wilc_sdio_suspend(struct device *dev)
{
struct sdio_func *func = dev_to_sdio_func(dev);
@@ -581,6 +589,9 @@ static int wilc_sdio_read(struct wilc *wilc, u32 addr, u8 *buf, u32 size)
static int wilc_sdio_deinit(struct wilc *wilc)
{
+ struct wilc_sdio *sdio_priv = wilc->bus_data;
+
+ sdio_priv->isinit = false;
return 0;
}
@@ -700,6 +711,7 @@ static int wilc_sdio_init(struct wilc *wilc, bool resume)
sdio_priv->has_thrpt_enh3);
}
+ sdio_priv->isinit = true;
return 0;
}
@@ -981,6 +993,7 @@ static const struct wilc_hif_func wilc_hif_sdio = {
.enable_interrupt = wilc_sdio_enable_interrupt,
.disable_interrupt = wilc_sdio_disable_interrupt,
.hif_reset = wilc_sdio_reset,
+ .hif_is_init = wilc_sdio_is_init,
};
static int wilc_sdio_resume(struct device *dev)
diff --git a/drivers/net/wireless/microchip/wilc1000/spi.c b/drivers/net/wireless/microchip/wilc1000/spi.c
index 2ae8dd3411ac..b0fc5e68feec 100644
--- a/drivers/net/wireless/microchip/wilc1000/spi.c
+++ b/drivers/net/wireless/microchip/wilc1000/spi.c
@@ -1029,6 +1029,13 @@ static int wilc_spi_reset(struct wilc *wilc)
return result;
}
+static bool wilc_spi_is_init(struct wilc *wilc)
+{
+ struct wilc_spi *spi_priv = wilc->bus_data;
+
+ return spi_priv->isinit;
+}
+
static int wilc_spi_deinit(struct wilc *wilc)
{
struct wilc_spi *spi_priv = wilc->bus_data;
@@ -1250,4 +1257,5 @@ static const struct wilc_hif_func wilc_hif_spi = {
.hif_block_rx_ext = wilc_spi_read,
.hif_sync_ext = wilc_spi_sync_ext,
.hif_reset = wilc_spi_reset,
+ .hif_is_init = wilc_spi_is_init,
};
diff --git a/drivers/net/wireless/microchip/wilc1000/wlan.c b/drivers/net/wireless/microchip/wilc1000/wlan.c
index f3f504d12873..947d9a0a494e 100644
--- a/drivers/net/wireless/microchip/wilc1000/wlan.c
+++ b/drivers/net/wireless/microchip/wilc1000/wlan.c
@@ -1481,9 +1481,12 @@ int wilc_wlan_init(struct net_device *dev)
wilc->quit = 0;
- if (wilc->hif_func->hif_init(wilc, false)) {
- ret = -EIO;
- goto fail;
+ if (!wilc->hif_func->hif_is_init(wilc)) {
+ acquire_bus(wilc, WILC_BUS_ACQUIRE_ONLY);
+ ret = wilc->hif_func->hif_init(wilc, false);
+ release_bus(wilc, WILC_BUS_RELEASE_ONLY);
+ if (ret)
+ goto fail;
}
if (!wilc->tx_buffer)
diff --git a/drivers/net/wireless/microchip/wilc1000/wlan.h b/drivers/net/wireless/microchip/wilc1000/wlan.h
index b45e72789a0e..a72cd5cac81d 100644
--- a/drivers/net/wireless/microchip/wilc1000/wlan.h
+++ b/drivers/net/wireless/microchip/wilc1000/wlan.h
@@ -373,6 +373,7 @@ struct wilc_hif_func {
int (*enable_interrupt)(struct wilc *nic);
void (*disable_interrupt)(struct wilc *nic);
int (*hif_reset)(struct wilc *wilc);
+ bool (*hif_is_init)(struct wilc *wilc);
};
#define WILC_MAX_CFG_FRAME_SIZE 1468
--
2.34.1
From: Ajay Singh <[email protected]>
Cancel the ongoing connection request to avoid any issue if the
interface is set down before the connection request is completed.
host_int_handle_disconnect was already available, so renamed it and used
the same API for 'ndio_close' cb.
Signed-off-by: Ajay Singh <[email protected]>
---
drivers/net/wireless/microchip/wilc1000/hif.c | 6 ++----
drivers/net/wireless/microchip/wilc1000/hif.h | 1 +
drivers/net/wireless/microchip/wilc1000/netdev.c | 1 +
3 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/net/wireless/microchip/wilc1000/hif.c b/drivers/net/wireless/microchip/wilc1000/hif.c
index 021e0db80bd2..b89519ab9205 100644
--- a/drivers/net/wireless/microchip/wilc1000/hif.c
+++ b/drivers/net/wireless/microchip/wilc1000/hif.c
@@ -635,7 +635,7 @@ static inline void host_int_parse_assoc_resp_info(struct wilc_vif *vif,
conn_info->req_ies_len = 0;
}
-static inline void host_int_handle_disconnect(struct wilc_vif *vif)
+inline void wilc_handle_disconnect(struct wilc_vif *vif)
{
struct host_if_drv *hif_drv = vif->hif_drv;
@@ -647,8 +647,6 @@ static inline void host_int_handle_disconnect(struct wilc_vif *vif)
if (hif_drv->conn_info.conn_result)
hif_drv->conn_info.conn_result(CONN_DISCONN_EVENT_DISCONN_NOTIF,
0, hif_drv->conn_info.arg);
- else
- netdev_err(vif->ndev, "%s: conn_result is NULL\n", __func__);
eth_zero_addr(hif_drv->assoc_bssid);
@@ -684,7 +682,7 @@ static void handle_rcvd_gnrl_async_info(struct work_struct *work)
host_int_parse_assoc_resp_info(vif, mac_info->status);
} else if (mac_info->status == WILC_MAC_STATUS_DISCONNECTED) {
if (hif_drv->hif_state == HOST_IF_CONNECTED) {
- host_int_handle_disconnect(vif);
+ wilc_handle_disconnect(vif);
} else if (hif_drv->usr_scan_req.scan_result) {
del_timer(&hif_drv->scan_timer);
handle_scan_done(vif, SCAN_EVENT_ABORTED);
diff --git a/drivers/net/wireless/microchip/wilc1000/hif.h b/drivers/net/wireless/microchip/wilc1000/hif.h
index d8dd94dcfe14..69ba1d469e9f 100644
--- a/drivers/net/wireless/microchip/wilc1000/hif.h
+++ b/drivers/net/wireless/microchip/wilc1000/hif.h
@@ -215,4 +215,5 @@ void wilc_gnrl_async_info_received(struct wilc *wilc, u8 *buffer, u32 length);
void *wilc_parse_join_bss_param(struct cfg80211_bss *bss,
struct cfg80211_crypto_settings *crypto);
int wilc_set_default_mgmt_key_index(struct wilc_vif *vif, u8 index);
+inline void wilc_handle_disconnect(struct wilc_vif *vif);
#endif
diff --git a/drivers/net/wireless/microchip/wilc1000/netdev.c b/drivers/net/wireless/microchip/wilc1000/netdev.c
index 7879446f282f..2de5838a4426 100644
--- a/drivers/net/wireless/microchip/wilc1000/netdev.c
+++ b/drivers/net/wireless/microchip/wilc1000/netdev.c
@@ -780,6 +780,7 @@ static int wilc_mac_close(struct net_device *ndev)
if (vif->ndev) {
netif_stop_queue(vif->ndev);
+ wilc_handle_disconnect(vif);
wilc_deinit_host_int(vif->ndev);
}
--
2.34.1
From: Ajay Singh <[email protected]>
Set station_info->filled to indicate signal level only when its value is
received successfully using wilc_get_rssi().
Signed-off-by: Ajay Singh <[email protected]>
---
drivers/net/wireless/microchip/wilc1000/cfg80211.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/net/wireless/microchip/wilc1000/cfg80211.c b/drivers/net/wireless/microchip/wilc1000/cfg80211.c
index 19862d932f1f..1e184ac93afa 100644
--- a/drivers/net/wireless/microchip/wilc1000/cfg80211.c
+++ b/drivers/net/wireless/microchip/wilc1000/cfg80211.c
@@ -1312,12 +1312,11 @@ static int dump_station(struct wiphy *wiphy, struct net_device *dev,
if (idx != 0)
return -ENOENT;
- sinfo->filled |= BIT_ULL(NL80211_STA_INFO_SIGNAL);
-
ret = wilc_get_rssi(vif, &sinfo->signal);
if (ret)
return ret;
+ sinfo->filled |= BIT_ULL(NL80211_STA_INFO_SIGNAL);
memcpy(mac, vif->priv.associated_bss, ETH_ALEN);
return 0;
}
--
2.34.1
From: Ajay Singh <[email protected]>
For consistency, use an existing 'iftype' element which was already
having the interface type. Replace 'mode' with 'iftype' as it was used
for the same purpose.
Signed-off-by: Ajay Singh <[email protected]>
---
drivers/net/wireless/microchip/wilc1000/netdev.c | 6 +++---
drivers/net/wireless/microchip/wilc1000/netdev.h | 1 -
2 files changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/net/wireless/microchip/wilc1000/netdev.c b/drivers/net/wireless/microchip/wilc1000/netdev.c
index 2de5838a4426..9b319a455b96 100644
--- a/drivers/net/wireless/microchip/wilc1000/netdev.c
+++ b/drivers/net/wireless/microchip/wilc1000/netdev.c
@@ -97,12 +97,12 @@ static struct net_device *get_if_handler(struct wilc *wilc, u8 *mac_header)
struct ieee80211_hdr *h = (struct ieee80211_hdr *)mac_header;
list_for_each_entry_rcu(vif, &wilc->vif_list, list) {
- if (vif->mode == WILC_STATION_MODE)
+ if (vif->iftype == WILC_STATION_MODE)
if (ether_addr_equal_unaligned(h->addr2, vif->bssid)) {
ndev = vif->ndev;
goto out;
}
- if (vif->mode == WILC_AP_MODE)
+ if (vif->iftype == WILC_AP_MODE)
if (ether_addr_equal_unaligned(h->addr1, vif->bssid)) {
ndev = vif->ndev;
goto out;
@@ -122,7 +122,7 @@ void wilc_wlan_set_bssid(struct net_device *wilc_netdev, const u8 *bssid,
else
eth_zero_addr(vif->bssid);
- vif->mode = mode;
+ vif->iftype = mode;
}
int wilc_wlan_get_num_conn_ifcs(struct wilc *wilc)
diff --git a/drivers/net/wireless/microchip/wilc1000/netdev.h b/drivers/net/wireless/microchip/wilc1000/netdev.h
index 822e65d00f53..43c085c74b7a 100644
--- a/drivers/net/wireless/microchip/wilc1000/netdev.h
+++ b/drivers/net/wireless/microchip/wilc1000/netdev.h
@@ -177,7 +177,6 @@ struct wilc_vif {
u8 bssid[ETH_ALEN];
struct host_if_drv *hif_drv;
struct net_device *ndev;
- u8 mode;
struct timer_list during_ip_timer;
struct timer_list periodic_rssi;
struct rf_info periodic_stat;
--
2.34.1
<[email protected]> wrote:
> From: Ajay Singh <[email protected]>
>
> WID_TX_POWER WID value is fetched from the firmware so it should be
> added in'g_cfg_byte' array to store the data which is received from
> firmware.
>
> Signed-off-by: Ajay Singh <[email protected]>
7 patches applied to wireless-next.git, thanks.
2f6e44ee6e96 wifi: wilc1000: add WID_TX_POWER WID in g_cfg_byte array
f589b5d941c7 wifi: wilc1000: set correct value of 'close' variable in failure case
33d4a577c7b1 wifi: wilc1000: set station_info flag only when signal value is valid
12fb1ae537a4 wifi: wilc1000: get correct length of string WID from received config packet
ad3e683ae4dc wifi: wilc1000: cancel the connect operation during interface down
39d0f1b0bf91 wifi: wilc1000: add 'isinit' flag for SDIO bus similar to SPI
4c2742146de0 wifi: wilc1000: use existing iftype variable to store the interface type
--
https://patchwork.kernel.org/project/linux-wireless/patch/[email protected]/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
<[email protected]> writes:
> From: Ajay Singh <[email protected]>
>
> Sparse reported below warning
>>> drivers/net/wireless/microchip/wilc1000/cfg80211.c:361:42: sparse: sparse: incorrect type in assignment (different base types) @@ expected unsigned int key_mgmt_suite @@ got restricted __be32 [usertype] @@
>
> 'key_mgmt_suite' value is expected in big-endian format. After receiving
> mgmt_suite value from wpa_s convert to cpu endianness because the same
> value is return back using cfg80211_external_auth_request(). Use
> be32_to_cpu() conversion API to avoid the sparse warning.
>
> Reported-by: kernel test robot <[email protected]>
> Fixes: c5b331d4f550fb78 ("wifi: wilc1000: add WPA3 SAE support")
> Signed-off-by: Ajay Singh <[email protected]>
> ---
> drivers/net/wireless/microchip/wilc1000/cfg80211.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/microchip/wilc1000/cfg80211.c b/drivers/net/wireless/microchip/wilc1000/cfg80211.c
> index 5c2c7f1dbffd..19862d932f1f 100644
> --- a/drivers/net/wireless/microchip/wilc1000/cfg80211.c
> +++ b/drivers/net/wireless/microchip/wilc1000/cfg80211.c
> @@ -359,7 +359,7 @@ static int connect(struct wiphy *wiphy, struct net_device *dev,
> memcpy(vif->auth.ssid.ssid, sme->ssid, sme->ssid_len);
> vif->auth.ssid.ssid_len = sme->ssid_len;
> }
> - vif->auth.key_mgmt_suite = cpu_to_be32(sme->crypto.akm_suites[0]);
> + vif->auth.key_mgmt_suite = be32_to_cpu((__force __be32)sme->crypto.akm_suites[0]);
No time to investigate in detail but the cast is just ugly. Isn't there
a better way to fix this?
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Hi Kalle,
On 27/07/22 18:30, Kalle Valo wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> <[email protected]> writes:
>
>> From: Ajay Singh <[email protected]>
>>
>> Sparse reported below warning
>>>> drivers/net/wireless/microchip/wilc1000/cfg80211.c:361:42: sparse: sparse: incorrect type in assignment (different base types) @@ expected unsigned int key_mgmt_suite @@ got restricted __be32 [usertype] @@
>> 'key_mgmt_suite' value is expected in big-endian format. After receiving
>> mgmt_suite value from wpa_s convert to cpu endianness because the same
>> value is return back using cfg80211_external_auth_request(). Use
>> be32_to_cpu() conversion API to avoid the sparse warning.
>>
>> Reported-by: kernel test robot <[email protected]>
>> Fixes: c5b331d4f550fb78 ("wifi: wilc1000: add WPA3 SAE support")
>> Signed-off-by: Ajay Singh <[email protected]>
>> ---
>> drivers/net/wireless/microchip/wilc1000/cfg80211.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/wireless/microchip/wilc1000/cfg80211.c b/drivers/net/wireless/microchip/wilc1000/cfg80211.c
>> index 5c2c7f1dbffd..19862d932f1f 100644
>> --- a/drivers/net/wireless/microchip/wilc1000/cfg80211.c
>> +++ b/drivers/net/wireless/microchip/wilc1000/cfg80211.c
>> @@ -359,7 +359,7 @@ static int connect(struct wiphy *wiphy, struct net_device *dev,
>> memcpy(vif->auth.ssid.ssid, sme->ssid, sme->ssid_len);
>> vif->auth.ssid.ssid_len = sme->ssid_len;
>> }
>> - vif->auth.key_mgmt_suite = cpu_to_be32(sme->crypto.akm_suites[0]);
>> + vif->auth.key_mgmt_suite = be32_to_cpu((__force __be32)sme->crypto.akm_suites[0]);
> No time to investigate in detail but the cast is just ugly. Isn't there
> a better way to fix this?
I think, there is an another way to handle this issue. 'key_mgmt_suite'
element in 'cfg80211_external_auth_params' struct should be converted to
'__be32' type(like below code snippet) because wpa_s expects the value
in big-endian format . After this change, the type case can be avoided.
Though I am not sure if these changes can have impact on other driver.
diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index cc8a9880b9d6..92df70afe445 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -3509,7 +3509,7 @@ struct cfg80211_external_auth_params {
enum nl80211_external_auth_action action;
u8 bssid[ETH_ALEN] __aligned(2);
struct cfg80211_ssid ssid;
- unsigned int key_mgmt_suite;
+ __be32 key_mgmt_suite;
u16 status;
const u8 *pmkid;
};
Regards,
Ajay
<[email protected]> writes:
> Hi Kalle,
>
> On 27/07/22 18:30, Kalle Valo wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you
>> know the content is safe
>>
>> <[email protected]> writes:
>>
>>> From: Ajay Singh <[email protected]>
>>>
>>> Sparse reported below warning
>>>>> drivers/net/wireless/microchip/wilc1000/cfg80211.c:361:42: sparse: sparse: incorrect type in assignment (different base types) @@ expected unsigned int key_mgmt_suite @@ got restricted __be32 [usertype] @@
>>> 'key_mgmt_suite' value is expected in big-endian format. After receiving
>>> mgmt_suite value from wpa_s convert to cpu endianness because the same
>>> value is return back using cfg80211_external_auth_request(). Use
>>> be32_to_cpu() conversion API to avoid the sparse warning.
>>>
>>> Reported-by: kernel test robot <[email protected]>
>>> Fixes: c5b331d4f550fb78 ("wifi: wilc1000: add WPA3 SAE support")
>>> Signed-off-by: Ajay Singh <[email protected]>
>>> ---
>>> drivers/net/wireless/microchip/wilc1000/cfg80211.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/wireless/microchip/wilc1000/cfg80211.c b/drivers/net/wireless/microchip/wilc1000/cfg80211.c
>>> index 5c2c7f1dbffd..19862d932f1f 100644
>>> --- a/drivers/net/wireless/microchip/wilc1000/cfg80211.c
>>> +++ b/drivers/net/wireless/microchip/wilc1000/cfg80211.c
>>> @@ -359,7 +359,7 @@ static int connect(struct wiphy *wiphy, struct net_device *dev,
>>> memcpy(vif->auth.ssid.ssid, sme->ssid, sme->ssid_len);
>>> vif->auth.ssid.ssid_len = sme->ssid_len;
>>> }
>>> - vif->auth.key_mgmt_suite = cpu_to_be32(sme->crypto.akm_suites[0]);
>>> + vif->auth.key_mgmt_suite = be32_to_cpu((__force __be32)sme->crypto.akm_suites[0]);
>> No time to investigate in detail but the cast is just ugly. Isn't there
>> a better way to fix this?
>
>
> I think, there is an another way to handle this issue. 'key_mgmt_suite'
> element in 'cfg80211_external_auth_params' struct should be converted to
> '__be32' type(like below code snippet) because wpa_s expects the value
> in big-endian format . After this change, the type case can be avoided.
> Though I am not sure if these changes can have impact on other driver.
>
>
> diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
> index cc8a9880b9d6..92df70afe445 100644
> --- a/include/net/cfg80211.h
> +++ b/include/net/cfg80211.h
> @@ -3509,7 +3509,7 @@ struct cfg80211_external_auth_params {
> enum nl80211_external_auth_action action;
> u8 bssid[ETH_ALEN] __aligned(2);
> struct cfg80211_ssid ssid;
> - unsigned int key_mgmt_suite;
> + __be32 key_mgmt_suite;
> u16 status;
> const u8 *pmkid;
> };
So just that I understand correctly: struct
cfg80211_crypto_settings::akm_suites is in cpu endian but struct struct
cfg80211_external_auth_params::key_mgmt_suite is in big endian? But your
original patch uses be32_to_cpu() so I must be confused.
It would be good to document this in cfg80211.h, the documentation there
doesn't mention anything about endian.
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
On Wed, 2022-07-27 at 17:32 +0000, [email protected] wrote:
>
> I think, there is an another way to handle this issue. 'key_mgmt_suite'
> element in 'cfg80211_external_auth_params' struct should be converted to
> '__be32' type(like below code snippet) because wpa_s expects the value
> in big-endian format . After this change, the type case can be avoided.
> Though I am not sure if these changes can have impact on other driver.
>
Ugh. I think maybe it would be better to fix wpa_supplicant?
Thing is, we use the NL80211_ATTR_AKM_SUITES attribute here for it, and
even wpa_supplicant mostly uses that in host endian:
num_suites = wpa_key_mgmt_to_suites(params->key_mgmt_suites,
suites, ARRAY_SIZE(suites));
...
nla_put(msg, NL80211_ATTR_AKM_SUITES, num_suites * sizeof(u32),
suites))
with
static int wpa_key_mgmt_to_suites(unsigned int key_mgmt_suites, u32 suites[],
int max_suites)
{
int num_suites = 0;
#define __AKM(a, b) \
if (num_suites < max_suites && \
(key_mgmt_suites & (WPA_KEY_MGMT_ ## a))) \
suites[num_suites++] = (RSN_AUTH_KEY_MGMT_ ## b)
__AKM(IEEE8021X, UNSPEC_802_1X);
and also
case WPA_KEY_MGMT_FT_FILS_SHA384:
mgmt = RSN_AUTH_KEY_MGMT_FT_FILS_SHA384;
break;
case WPA_KEY_MGMT_PSK:
default:
mgmt = RSN_AUTH_KEY_MGMT_PSK_OVER_802_1X;
break;
}
wpa_printf(MSG_DEBUG, " * akm=0x%x", mgmt);
if (nla_put_u32(msg, NL80211_ATTR_AKM_SUITES, mgmt))
return -1;
Now those are all userspace->kernel direction, but also:
wiphy_info_akm_suites(info, tb[NL80211_ATTR_AKM_SUITES]);
which eventually uses
static unsigned int get_akm_suites_info(struct nlattr *tb)
{
int i, num;
unsigned int key_mgmt = 0;
u32 *akms;
if (!tb)
return 0;
num = nla_len(tb) / sizeof(u32);
akms = nla_data(tb);
for (i = 0; i < num; i++) {
switch (akms[i]) {
case RSN_AUTH_KEY_MGMT_UNSPEC_802_1X:
so again it's in native endianness.
So IMHO
commit 5ff39c1380d9dea794c5102c0b6d11d1b1e23ad0
Author: Sunil Dutt <[email protected]>
Date: Thu Feb 1 17:01:28 2018 +0530
SAE: Support external authentication offload for driver-SME cases
is the problem there in that it assumed big endian for a value that's
clearly not meant to be big endian. And what garbage out-of-tree drivers
do we don't know ...
Even in the kernel, we have
static int
qtnf_event_handle_external_auth(struct qtnf_vif *vif,
const struct qlink_event_external_auth *ev,
u16 len)
{
struct cfg80211_external_auth_params auth = {0};
[...]
auth.key_mgmt_suite = le32_to_cpu(ev->akm_suite);
[...]
ret = cfg80211_external_auth_request(vif->netdev, &auth, GFP_KERNEL);
but maybe that was never tested?
johannes
On Thu, Oct 13, 2022 at 09:39:56AM +0200, Johannes Berg wrote:
> On Wed, 2022-07-27 at 17:32 +0000, [email protected] wrote:
> > I think, there is an another way to handle this issue. 'key_mgmt_suite'
> > element in 'cfg80211_external_auth_params' struct should be converted to
> > '__be32' type(like below code snippet) because wpa_s expects the value
> > in big-endian format . After this change, the type case can be avoided.
> > Though I am not sure if these changes can have impact on other driver.
> >
>
> Ugh. I think maybe it would be better to fix wpa_supplicant?
Assuming you are referring to what sme_external_auth_trigger() does,
yes, the use of RSN_SELECTOR_GET() there on an unsigned int variable is
clearly wrong. As a workaround, it could be modified to accept both the
big endian and host byte order since the risk of conflicting with a
vendor specific AKM suite here would be very minimal.. Furthermore, it
looks like this has missed the new RSN_AUTH_KEY_MGMT_SAE_EXT_KEY
selector update as well.
> Thing is, we use the NL80211_ATTR_AKM_SUITES attribute here for it, and
> even wpa_supplicant mostly uses that in host endian:
By the way, that use of a u32 attribute (or an array of such) in an
interface is quite confusing for suite selectors (both AKM and cipher)
since a suite selector is really a four octet binary string that starts
with three octet OUI that is followed with a single octet integer
value. nl80211.h does not seem to provide any documentation on what the
exact value is supposed to be.
I can understand use of u32 and native byte order as an implementation
internal thing, but it should not really be used in an interface since
it is just waiting for this type of issues to show up. It's always
confusing to review the driver_nl80211* changes for this area since I
have to convince myself each time that this really is a native byte
order u32 value.. That sme_external_auth_trigger() case is outside
driver_nl80211* so it did not get the same detail of review
unfortunately.
--
Jouni Malinen PGP id EFC895FA
On Thu, 2022-10-13 at 12:40 +0300, Jouni Malinen wrote:
> On Thu, Oct 13, 2022 at 09:39:56AM +0200, Johannes Berg wrote:
> > On Wed, 2022-07-27 at 17:32 +0000, [email protected] wrote:
> > > I think, there is an another way to handle this issue. 'key_mgmt_suite'
> > > element in 'cfg80211_external_auth_params' struct should be converted to
> > > '__be32' type(like below code snippet) because wpa_s expects the value
> > > in big-endian format . After this change, the type case can be avoided.
> > > Though I am not sure if these changes can have impact on other driver.
> > >
> >
> > Ugh. I think maybe it would be better to fix wpa_supplicant?
>
> Assuming you are referring to what sme_external_auth_trigger() does,
> yes, the use of RSN_SELECTOR_GET() there on an unsigned int variable is
> clearly wrong.
Right, that's what I meant.
> As a workaround, it could be modified to accept both the
> big endian and host byte order since the risk of conflicting with a
> vendor specific AKM suite here would be very minimal..
True.
> > Thing is, we use the NL80211_ATTR_AKM_SUITES attribute here for it, and
> > even wpa_supplicant mostly uses that in host endian:
>
> By the way, that use of a u32 attribute (or an array of such) in an
> interface is quite confusing for suite selectors (both AKM and cipher)
> since a suite selector is really a four octet binary string that starts
> with three octet OUI that is followed with a single octet integer
> value. nl80211.h does not seem to provide any documentation on what the
> exact value is supposed to be.
I guess we should fix the documentation then, but basically, for a
selector of
O-U-I:D
we use
(O << 24) | (U << 16) | (I << 8) | D
for the ID. If we had consistently used be32 then that'd actually at
least match the four octet binary string and it'd be irrelevant, but ...
we clearly didn't.
> I can understand use of u32 and native byte order as an implementation
> internal thing, but it should not really be used in an interface since
> it is just waiting for this type of issues to show up.
Yeah, can't really disagree with that, though I think it's a bit late
now, unfortunately.
johannes