From: Johannes Berg <[email protected]>
Just remove the extra asterisk to make it not be
kernel-doc formatted.
Signed-off-by: Johannes Berg <[email protected]>
---
drivers/net/wireless/intel/ipw2x00/ipw2100.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/wireless/intel/ipw2x00/ipw2100.c b/drivers/net/wireless/intel/ipw2x00/ipw2100.c
index ac36c899134e..b0f23cf1a621 100644
--- a/drivers/net/wireless/intel/ipw2x00/ipw2100.c
+++ b/drivers/net/wireless/intel/ipw2x00/ipw2100.c
@@ -6529,7 +6529,7 @@ static struct pci_driver ipw2100_pci_driver = {
.shutdown = ipw2100_shutdown,
};
-/**
+/*
* Initialize the ipw2100 driver/module
*
* @returns 0 if ok, < 0 errno node con error.
@@ -6561,7 +6561,7 @@ static int __init ipw2100_init(void)
return ret;
}
-/**
+/*
* Cleanup ipw2100 driver registration
*/
static void __exit ipw2100_exit(void)
--
2.37.2
From: Johannes Berg <[email protected]>
- endian swapping is required in one place, use the
already swapped 'bsssize' local
- lbs_disablemesh need not be exported and can be static
Signed-off-by: Johannes Berg <[email protected]>
---
drivers/net/wireless/marvell/libertas/cfg.c | 2 +-
drivers/net/wireless/marvell/libertas/main.c | 3 +--
2 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/net/wireless/marvell/libertas/cfg.c b/drivers/net/wireless/marvell/libertas/cfg.c
index 5e3ae00153b8..3e065cbb0af9 100644
--- a/drivers/net/wireless/marvell/libertas/cfg.c
+++ b/drivers/net/wireless/marvell/libertas/cfg.c
@@ -546,7 +546,7 @@ static int lbs_ret_scan(struct lbs_private *priv, unsigned long dummy,
pos = scanresp->bssdesc_and_tlvbuffer;
lbs_deb_hex(LBS_DEB_SCAN, "SCAN_RSP", scanresp->bssdesc_and_tlvbuffer,
- scanresp->bssdescriptsize);
+ bsssize);
tsfdesc = pos + bsssize;
tsfsize = 4 + 8 * scanresp->nr_sets;
diff --git a/drivers/net/wireless/marvell/libertas/main.c b/drivers/net/wireless/marvell/libertas/main.c
index 5c9f295536ea..8f5220cee112 100644
--- a/drivers/net/wireless/marvell/libertas/main.c
+++ b/drivers/net/wireless/marvell/libertas/main.c
@@ -39,8 +39,7 @@ unsigned int lbs_debug;
EXPORT_SYMBOL_GPL(lbs_debug);
module_param_named(libertas_debug, lbs_debug, int, 0644);
-unsigned int lbs_disablemesh;
-EXPORT_SYMBOL_GPL(lbs_disablemesh);
+static unsigned int lbs_disablemesh;
module_param_named(libertas_disablemesh, lbs_disablemesh, int, 0644);
--
2.37.2
From: Johannes Berg <[email protected]>
Clearly the value should be converted and then compared,
not the result of the comparison be converted. No binary
changes on x86.
Signed-off-by: Johannes Berg <[email protected]>
---
drivers/net/wireless/marvell/mwifiex/sta_event.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wireless/marvell/mwifiex/sta_event.c b/drivers/net/wireless/marvell/mwifiex/sta_event.c
index b95e90a7d124..b6315fccd1bb 100644
--- a/drivers/net/wireless/marvell/mwifiex/sta_event.c
+++ b/drivers/net/wireless/marvell/mwifiex/sta_event.c
@@ -623,7 +623,7 @@ mwifiex_fw_dump_info_event(struct mwifiex_private *priv,
adapter->event_skb->data, event_skb->len);
adapter->devdump_len += event_skb->len;
- if (le16_to_cpu(fw_dump_hdr->type == FW_DUMP_INFO_ENDED)) {
+ if (le16_to_cpu(fw_dump_hdr->type) == FW_DUMP_INFO_ENDED) {
mwifiex_dbg(adapter, MSG,
"receive end of transmission flag event!\n");
goto upload_dump;
--
2.37.2
From: Johannes Berg <[email protected]>
We can call this in one of two ways: through mac80211, where
we're already in an RCU read-side critical section, or from
some other code in the driver where this pointer can only be
NULL. In any case, we get a 'free' already protected pointer
to the sta through info->control.sta, so we can use it on
the stack without any further protection.
Remove the rcu_dereference() and critical section.
Signed-off-by: Johannes Berg <[email protected]>
---
Cc: Solomon Peachy <[email protected]>
---
drivers/net/wireless/st/cw1200/txrx.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/drivers/net/wireless/st/cw1200/txrx.c b/drivers/net/wireless/st/cw1200/txrx.c
index fde21fca6c5e..ab19e0403dc2 100644
--- a/drivers/net/wireless/st/cw1200/txrx.c
+++ b/drivers/net/wireless/st/cw1200/txrx.c
@@ -762,8 +762,7 @@ void cw1200_tx(struct ieee80211_hw *dev,
if (ret)
goto drop;
- rcu_read_lock();
- sta = rcu_dereference(t.sta);
+ sta = t.sta;
spin_lock_bh(&priv->ps_state_lock);
{
@@ -776,8 +775,6 @@ void cw1200_tx(struct ieee80211_hw *dev,
if (tid_update && sta)
ieee80211_sta_set_buffered(sta, t.txpriv.tid, true);
- rcu_read_unlock();
-
cw1200_bh_wakeup(priv);
return;
--
2.37.2
From: Johannes Berg <[email protected]>
This caused sparse warnings, and clearly is needed per
how other firmware interfaces behave.
Signed-off-by: Johannes Berg <[email protected]>
---
drivers/net/wireless/ti/wl18xx/event.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/net/wireless/ti/wl18xx/event.c b/drivers/net/wireless/ti/wl18xx/event.c
index 13d78ada4bb6..34d95f458e1a 100644
--- a/drivers/net/wireless/ti/wl18xx/event.c
+++ b/drivers/net/wireless/ti/wl18xx/event.c
@@ -131,10 +131,10 @@ int wl18xx_process_mailbox_events(struct wl1271 *wl)
if (vector & TIME_SYNC_EVENT_ID)
wlcore_event_time_sync(wl,
- mbox->time_sync_tsf_high_msb,
- mbox->time_sync_tsf_high_lsb,
- mbox->time_sync_tsf_low_msb,
- mbox->time_sync_tsf_low_lsb);
+ le16_to_cpu(mbox->time_sync_tsf_high_msb),
+ le16_to_cpu(mbox->time_sync_tsf_high_lsb),
+ le16_to_cpu(mbox->time_sync_tsf_low_msb),
+ le16_to_cpu(mbox->time_sync_tsf_low_lsb));
if (vector & RADAR_DETECTED_EVENT_ID) {
wl1271_info("radar event: channel %d type %s",
--
2.37.2
From: Johannes Berg <[email protected]>
There are two, just change them to have a "u8 data[]" type
member, and add casts where needed. No binary changes.
Signed-off-by: Johannes Berg <[email protected]>
---
drivers/net/wireless/marvell/mwifiex/fw.h | 4 ++--
drivers/net/wireless/marvell/mwifiex/sta_cmd.c | 4 ++--
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/net/wireless/marvell/mwifiex/fw.h b/drivers/net/wireless/marvell/mwifiex/fw.h
index 26a48d8f49be..b4f945a549f7 100644
--- a/drivers/net/wireless/marvell/mwifiex/fw.h
+++ b/drivers/net/wireless/marvell/mwifiex/fw.h
@@ -2104,7 +2104,7 @@ struct mwifiex_fw_mef_entry {
struct host_cmd_ds_mef_cfg {
__le32 criteria;
__le16 num_entries;
- struct mwifiex_fw_mef_entry mef_entry[];
+ u8 mef_entry_data[];
} __packed;
#define CONNECTION_TYPE_INFRA 0
@@ -2254,7 +2254,7 @@ struct coalesce_receive_filt_rule {
struct host_cmd_ds_coalesce_cfg {
__le16 action;
__le16 num_of_rules;
- struct coalesce_receive_filt_rule rule[];
+ u8 rule_data[];
} __packed;
struct host_cmd_ds_multi_chan_policy {
diff --git a/drivers/net/wireless/marvell/mwifiex/sta_cmd.c b/drivers/net/wireless/marvell/mwifiex/sta_cmd.c
index 512b5bb9cf6f..e2800a831c8e 100644
--- a/drivers/net/wireless/marvell/mwifiex/sta_cmd.c
+++ b/drivers/net/wireless/marvell/mwifiex/sta_cmd.c
@@ -1435,7 +1435,7 @@ mwifiex_cmd_mef_cfg(struct mwifiex_private *priv,
mef_entry = (struct mwifiex_fw_mef_entry *)pos;
mef_entry->mode = mef->mef_entry[i].mode;
mef_entry->action = mef->mef_entry[i].action;
- pos += sizeof(*mef_cfg->mef_entry);
+ pos += sizeof(*mef_entry);
if (mwifiex_cmd_append_rpn_expression(priv,
&mef->mef_entry[i], &pos))
@@ -1631,7 +1631,7 @@ mwifiex_cmd_coalesce_cfg(struct mwifiex_private *priv,
coalesce_cfg->action = cpu_to_le16(cmd_action);
coalesce_cfg->num_of_rules = cpu_to_le16(cfg->num_of_rules);
- rule = coalesce_cfg->rule;
+ rule = (void *)coalesce_cfg->rule_data;
for (cnt = 0; cnt < cfg->num_of_rules; cnt++) {
rule->header.type = cpu_to_le16(TLV_TYPE_COALESCE_RULE);
--
2.37.2
From: Johannes Berg <[email protected]>
These cause sparse warnings, and since the device generally
works in little endian we can assume the code is correct, so
just fix the casts accordingly. No binary changes on x86.
Signed-off-by: Johannes Berg <[email protected]>
---
drivers/net/wireless/marvell/mwifiex/usb.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/net/wireless/marvell/mwifiex/usb.c b/drivers/net/wireless/marvell/mwifiex/usb.c
index c2f2ce2a3f95..d3ab9572e711 100644
--- a/drivers/net/wireless/marvell/mwifiex/usb.c
+++ b/drivers/net/wireless/marvell/mwifiex/usb.c
@@ -911,14 +911,14 @@ static int mwifiex_usb_prepare_tx_aggr_skb(struct mwifiex_adapter *adapter,
memcpy(payload, skb_tmp->data, skb_tmp->len);
if (skb_queue_empty(&port->tx_aggr.aggr_list)) {
/* do not padding for last packet*/
- *(u16 *)payload = cpu_to_le16(skb_tmp->len);
- *(u16 *)&payload[2] =
+ *(__le16 *)payload = cpu_to_le16(skb_tmp->len);
+ *(__le16 *)&payload[2] =
cpu_to_le16(MWIFIEX_TYPE_AGGR_DATA_V2 | 0x80);
skb_trim(skb_aggr, skb_aggr->len - pad);
} else {
/* add aggregation interface header */
- *(u16 *)payload = cpu_to_le16(skb_tmp->len + pad);
- *(u16 *)&payload[2] =
+ *(__le16 *)payload = cpu_to_le16(skb_tmp->len + pad);
+ *(__le16 *)&payload[2] =
cpu_to_le16(MWIFIEX_TYPE_AGGR_DATA_V2);
}
@@ -1097,9 +1097,9 @@ static int mwifiex_usb_aggr_tx_data(struct mwifiex_adapter *adapter, u8 ep,
}
payload = skb->data;
- *(u16 *)&payload[2] =
+ *(__le16 *)&payload[2] =
cpu_to_le16(MWIFIEX_TYPE_AGGR_DATA_V2 | 0x80);
- *(u16 *)payload = cpu_to_le16(skb->len);
+ *(__le16 *)payload = cpu_to_le16(skb->len);
skb_send = skb;
context = &port->tx_data_list[port->tx_data_ix++];
return mwifiex_usb_construct_send_urb(adapter, port, ep,
--
2.37.2
From: Johannes Berg <[email protected]>
We need to read a value from the device to wake it, but if it
succeeds we don't really care about it. Mark the variable to
avoid a compiler warning.
Signed-off-by: Johannes Berg <[email protected]>
---
drivers/net/wireless/marvell/mwifiex/pcie.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c b/drivers/net/wireless/marvell/mwifiex/pcie.c
index f7f9277602a5..5dcf61761a16 100644
--- a/drivers/net/wireless/marvell/mwifiex/pcie.c
+++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
@@ -644,7 +644,7 @@ static int mwifiex_pm_wakeup_card(struct mwifiex_adapter *adapter)
{
struct pcie_service_card *card = adapter->card;
const struct mwifiex_pcie_card_reg *reg = card->pcie.reg;
- int retval;
+ int retval __maybe_unused;
mwifiex_dbg(adapter, EVENT,
"event: Wakeup device...\n");
--
2.37.2
From: Johannes Berg <[email protected]>
Instead of the code here that copies into a variable
first and then flips endianness, which confuses sparse,
just directly use get_unaligned_le64().
Signed-off-by: Johannes Berg <[email protected]>
---
Cc: Solomon Peachy <[email protected]>
---
drivers/net/wireless/st/cw1200/txrx.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/net/wireless/st/cw1200/txrx.c b/drivers/net/wireless/st/cw1200/txrx.c
index ab19e0403dc2..6894b919ff94 100644
--- a/drivers/net/wireless/st/cw1200/txrx.c
+++ b/drivers/net/wireless/st/cw1200/txrx.c
@@ -1142,8 +1142,7 @@ void cw1200_rx_cb(struct cw1200_common *priv,
/* Remove TSF from the end of frame */
if (arg->flags & WSM_RX_STATUS_TSF_INCLUDED) {
- memcpy(&hdr->mactime, skb->data + skb->len - 8, 8);
- hdr->mactime = le64_to_cpu(hdr->mactime);
+ hdr->mactime = get_unaligned_le64(skb->data + skb->len - 8);
if (skb->len >= 8)
skb_trim(skb, skb->len - 8);
} else {
--
2.37.2
From: Johannes Berg <[email protected]>
There's a TODO here, just move the dependency on phy->rev
into the comment. Not that this driver is likely to get
any updates.
Signed-off-by: Johannes Berg <[email protected]>
---
drivers/net/wireless/broadcom/b43/phy_n.c | 6 +-----
1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/drivers/net/wireless/broadcom/b43/phy_n.c b/drivers/net/wireless/broadcom/b43/phy_n.c
index aa5c99465674..2c0c019a815d 100644
--- a/drivers/net/wireless/broadcom/b43/phy_n.c
+++ b/drivers/net/wireless/broadcom/b43/phy_n.c
@@ -2479,11 +2479,7 @@ static void b43_nphy_gain_ctl_workarounds_rev19(struct b43_wldev *dev)
static void b43_nphy_gain_ctl_workarounds_rev7(struct b43_wldev *dev)
{
- struct b43_phy *phy = &dev->phy;
-
- switch (phy->rev) {
- /* TODO */
- }
+ /* TODO - should depend on phy->rev */
}
static void b43_nphy_gain_ctl_workarounds_rev3(struct b43_wldev *dev)
--
2.37.2
From: Johannes Berg <[email protected]>
Use "u8 bssid_data[]" with an appropriate cast. No binary
changes.
Signed-off-by: Johannes Berg <[email protected]>
---
drivers/net/wireless/rndis_wlan.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/net/wireless/rndis_wlan.c b/drivers/net/wireless/rndis_wlan.c
index 325933217b41..82a7458e01ae 100644
--- a/drivers/net/wireless/rndis_wlan.c
+++ b/drivers/net/wireless/rndis_wlan.c
@@ -251,7 +251,7 @@ struct ndis_80211_bssid_ex {
struct ndis_80211_bssid_list_ex {
__le32 num_items;
- struct ndis_80211_bssid_ex bssid[];
+ u8 bssid_data[];
} __packed;
struct ndis_80211_fixed_ies {
@@ -2084,7 +2084,8 @@ static int rndis_check_bssid_list(struct usbnet *usbdev, u8 *match_bssid,
netdev_dbg(usbdev->net, "%s(): buflen: %d\n", __func__, len);
bssid_len = 0;
- bssid = next_bssid_list_item(bssid_list->bssid, &bssid_len, buf, len);
+ bssid = next_bssid_list_item((void *)bssid_list->bssid_data,
+ &bssid_len, buf, len);
/* Device returns incorrect 'num_items'. Workaround by ignoring the
* received 'num_items' and walking through full bssid buffer instead.
--
2.37.2
From: Johannes Berg <[email protected]>
There are a number of these here, fix them by using
appropriate casts. No binary changes.
Signed-off-by: Johannes Berg <[email protected]>
---
drivers/net/wireless/intel/ipw2x00/libipw.h | 13 ++++++-------
drivers/net/wireless/intel/ipw2x00/libipw_rx.c | 10 +++++-----
2 files changed, 11 insertions(+), 12 deletions(-)
diff --git a/drivers/net/wireless/intel/ipw2x00/libipw.h b/drivers/net/wireless/intel/ipw2x00/libipw.h
index 7964ef7d15f0..bec7bc273748 100644
--- a/drivers/net/wireless/intel/ipw2x00/libipw.h
+++ b/drivers/net/wireless/intel/ipw2x00/libipw.h
@@ -405,7 +405,7 @@ struct libipw_auth {
__le16 transaction;
__le16 status;
/* challenge */
- struct libipw_info_element info_element[];
+ u8 variable[];
} __packed;
struct libipw_channel_switch {
@@ -423,7 +423,6 @@ struct libipw_action {
union {
struct libipw_action_exchange {
u8 token;
- struct libipw_info_element info_element[0];
} exchange;
struct libipw_channel_switch channel_switch;
@@ -441,7 +440,7 @@ struct libipw_disassoc {
struct libipw_probe_request {
struct libipw_hdr_3addr header;
/* SSID, supported rates */
- struct libipw_info_element info_element[];
+ u8 variable[];
} __packed;
struct libipw_probe_response {
@@ -451,7 +450,7 @@ struct libipw_probe_response {
__le16 capability;
/* SSID, supported rates, FH params, DS params,
* CF params, IBSS params, TIM (if beacon), RSN */
- struct libipw_info_element info_element[];
+ u8 variable[];
} __packed;
/* Alias beacon for probe_response */
@@ -462,7 +461,7 @@ struct libipw_assoc_request {
__le16 capability;
__le16 listen_interval;
/* SSID, supported rates, RSN */
- struct libipw_info_element info_element[];
+ u8 variable[];
} __packed;
struct libipw_reassoc_request {
@@ -470,7 +469,7 @@ struct libipw_reassoc_request {
__le16 capability;
__le16 listen_interval;
u8 current_ap[ETH_ALEN];
- struct libipw_info_element info_element[];
+ u8 variable[];
} __packed;
struct libipw_assoc_response {
@@ -479,7 +478,7 @@ struct libipw_assoc_response {
__le16 status;
__le16 aid;
/* supported rates */
- struct libipw_info_element info_element[];
+ u8 variable[];
} __packed;
struct libipw_txb {
diff --git a/drivers/net/wireless/intel/ipw2x00/libipw_rx.c b/drivers/net/wireless/intel/ipw2x00/libipw_rx.c
index 7a684b76f39b..48d6870bbf4e 100644
--- a/drivers/net/wireless/intel/ipw2x00/libipw_rx.c
+++ b/drivers/net/wireless/intel/ipw2x00/libipw_rx.c
@@ -1329,8 +1329,8 @@ static int libipw_handle_assoc_resp(struct libipw_device *ieee, struct libipw_as
network->wpa_ie_len = 0;
network->rsn_ie_len = 0;
- if (libipw_parse_info_param
- (frame->info_element, stats->len - sizeof(*frame), network))
+ if (libipw_parse_info_param((void *)frame->variable,
+ stats->len - sizeof(*frame), network))
return 1;
network->mode = 0;
@@ -1389,8 +1389,8 @@ static int libipw_network_init(struct libipw_device *ieee, struct libipw_probe_r
network->wpa_ie_len = 0;
network->rsn_ie_len = 0;
- if (libipw_parse_info_param
- (beacon->info_element, stats->len - sizeof(*beacon), network))
+ if (libipw_parse_info_param((void *)beacon->variable,
+ stats->len - sizeof(*beacon), network))
return 1;
network->mode = 0;
@@ -1510,7 +1510,7 @@ static void libipw_process_probe_response(struct libipw_device
struct libipw_network *target;
struct libipw_network *oldest = NULL;
#ifdef CONFIG_LIBIPW_DEBUG
- struct libipw_info_element *info_element = beacon->info_element;
+ struct libipw_info_element *info_element = (void *)beacon->variable;
#endif
unsigned long flags;
--
2.37.2
Johannes Berg <[email protected]> writes:
> From: Johannes Berg <[email protected]>
>
> There are a number of these here, fix them by using
> appropriate casts. No binary changes.
>
> Signed-off-by: Johannes Berg <[email protected]>
> ---
> drivers/net/wireless/intel/ipw2x00/libipw.h | 13 ++++++-------
> drivers/net/wireless/intel/ipw2x00/libipw_rx.c | 10 +++++-----
> 2 files changed, 11 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/net/wireless/intel/ipw2x00/libipw.h b/drivers/net/wireless/intel/ipw2x00/libipw.h
> index 7964ef7d15f0..bec7bc273748 100644
> --- a/drivers/net/wireless/intel/ipw2x00/libipw.h
> +++ b/drivers/net/wireless/intel/ipw2x00/libipw.h
> @@ -405,7 +405,7 @@ struct libipw_auth {
> __le16 transaction;
> __le16 status;
> /* challenge */
> - struct libipw_info_element info_element[];
> + u8 variable[];
Why u8 is better?
> --- a/drivers/net/wireless/intel/ipw2x00/libipw_rx.c
> +++ b/drivers/net/wireless/intel/ipw2x00/libipw_rx.c
> @@ -1329,8 +1329,8 @@ static int libipw_handle_assoc_resp(struct libipw_device *ieee, struct libipw_as
> network->wpa_ie_len = 0;
> network->rsn_ie_len = 0;
>
> - if (libipw_parse_info_param
> - (frame->info_element, stats->len - sizeof(*frame), network))
> + if (libipw_parse_info_param((void *)frame->variable,
> + stats->len - sizeof(*frame), network))
To me this look worse as we need to add an extra cast, and casts are
always problematic.
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Johannes Berg <[email protected]> writes:
> From: Johannes Berg <[email protected]>
>
> Use "u8 bssid_data[]" with an appropriate cast. No binary
> changes.
>
> Signed-off-by: Johannes Berg <[email protected]>
> ---
> drivers/net/wireless/rndis_wlan.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/wireless/rndis_wlan.c b/drivers/net/wireless/rndis_wlan.c
> index 325933217b41..82a7458e01ae 100644
> --- a/drivers/net/wireless/rndis_wlan.c
> +++ b/drivers/net/wireless/rndis_wlan.c
> @@ -251,7 +251,7 @@ struct ndis_80211_bssid_ex {
>
> struct ndis_80211_bssid_list_ex {
> __le32 num_items;
> - struct ndis_80211_bssid_ex bssid[];
> + u8 bssid_data[];
> } __packed;
>
> struct ndis_80211_fixed_ies {
> @@ -2084,7 +2084,8 @@ static int rndis_check_bssid_list(struct usbnet *usbdev, u8 *match_bssid,
> netdev_dbg(usbdev->net, "%s(): buflen: %d\n", __func__, len);
>
> bssid_len = 0;
> - bssid = next_bssid_list_item(bssid_list->bssid, &bssid_len, buf, len);
> + bssid = next_bssid_list_item((void *)bssid_list->bssid_data,
> + &bssid_len, buf, len);
Same comment as in patch 2, I just feel the code gets worse because of a
compiler warning.
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
On Sun, Sep 04, 2022 at 09:29:09PM +0200, Johannes Berg wrote:
> From: Johannes Berg <[email protected]>
>
> These cause sparse warnings, and since the device generally
> works in little endian we can assume the code is correct, so
> just fix the casts accordingly. No binary changes on x86.
>
> Signed-off-by: Johannes Berg <[email protected]>
Reviewed-by: Brian Norris <[email protected]>
Hi,
On Sun, Sep 04, 2022 at 09:29:07PM +0200, Johannes Berg wrote:
> From: Johannes Berg <[email protected]>
>
> There are two, just change them to have a "u8 data[]" type
> member, and add casts where needed. No binary changes.
Hmm, what exact warning are you looking at? This one?
https://clang.llvm.org/docs/DiagnosticsReference.html#wflexible-array-extensions
It's a little hard to suggest alternatives (or understand why this is
the only/best way) if I don't know what the alleged bug/warning is.
> Signed-off-by: Johannes Berg <[email protected]>
> ---
> drivers/net/wireless/marvell/mwifiex/fw.h | 4 ++--
> drivers/net/wireless/marvell/mwifiex/sta_cmd.c | 4 ++--
> 2 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/wireless/marvell/mwifiex/fw.h b/drivers/net/wireless/marvell/mwifiex/fw.h
> index 26a48d8f49be..b4f945a549f7 100644
> --- a/drivers/net/wireless/marvell/mwifiex/fw.h
> +++ b/drivers/net/wireless/marvell/mwifiex/fw.h
> @@ -2104,7 +2104,7 @@ struct mwifiex_fw_mef_entry {
> struct host_cmd_ds_mef_cfg {
> __le32 criteria;
> __le16 num_entries;
> - struct mwifiex_fw_mef_entry mef_entry[];
> + u8 mef_entry_data[];
Do you actually need this part of the change? The 'mef_entry' (or
'mef_entry_data') field is not actually used anywhere now, but I can't
tell what kind of warning is involved.
But also see the next comment:
> } __packed;
>
> #define CONNECTION_TYPE_INFRA 0
> @@ -2254,7 +2254,7 @@ struct coalesce_receive_filt_rule {
> struct host_cmd_ds_coalesce_cfg {
> __le16 action;
> __le16 num_of_rules;
> - struct coalesce_receive_filt_rule rule[];
> + u8 rule_data[];
These kinds of changes seem to be losing some valuable information. At a
minimum, it would be nice to leave a comment that points at the intended
struct; but ideally, we'd be able to still get the type safety from
actually using the struct, instead of relying on casts and/or u8/void.
But I don't know if that's possible, as I'm not familiar with the
compiler warning involved.
Brian
On Sun, Sep 04, 2022 at 09:29:08PM +0200, Johannes Berg wrote:
> From: Johannes Berg <[email protected]>
>
> Clearly the value should be converted and then compared,
> not the result of the comparison be converted. No binary
> changes on x86.
>
> Signed-off-by: Johannes Berg <[email protected]>
Reviewed-by: Brian Norris <[email protected]>
On Tue, 2022-09-06 at 15:20 -0700, Brian Norris wrote:
> Hi,
>
> On Sun, Sep 04, 2022 at 09:29:07PM +0200, Johannes Berg wrote:
> > From: Johannes Berg <[email protected]>
> >
> > There are two, just change them to have a "u8 data[]" type
> > member, and add casts where needed. No binary changes.
>
> Hmm, what exact warning are you looking at? This one?
> https://clang.llvm.org/docs/DiagnosticsReference.html#wflexible-array-extensions
>
I think gcc also has one with W=1 now?
But yes, it's similar to that one. I was looking at Jakub's netdev test
builds here:
https://patchwork.hopto.org/static/nipa/673828/12964988/build_32bit/stderr
../drivers/net/wireless/marvell/mwifiex/fw.h:2107:46: warning: array of flexible structures
../drivers/net/wireless/marvell/mwifiex/fw.h:2257:47: warning: array of flexible structures
> > @@ -2104,7 +2104,7 @@ struct mwifiex_fw_mef_entry {
> > struct host_cmd_ds_mef_cfg {
> > __le32 criteria;
> > __le16 num_entries;
> > - struct mwifiex_fw_mef_entry mef_entry[];
> > + u8 mef_entry_data[];
>
> Do you actually need this part of the change? The 'mef_entry' (or
> 'mef_entry_data') field is not actually used anywhere now, but I can't
> tell what kind of warning is involved.
But it itself is variable, so the compiler is (rightfully, IMHO) saying
that you can't have an array of something that has a variable size, even
if it's a variable array.
> > struct host_cmd_ds_coalesce_cfg {
> > __le16 action;
> > __le16 num_of_rules;
> > - struct coalesce_receive_filt_rule rule[];
> > + u8 rule_data[];
>
> These kinds of changes seem to be losing some valuable information. At a
> minimum, it would be nice to leave a comment that points at the intended
> struct; but ideally, we'd be able to still get the type safety from
> actually using the struct, instead of relying on casts and/or u8/void.
All fair points. This was the way we typically do this in e.g.
ieee80211.h ("u8 variable[]", not "struct element elems[]"), but YMMV.
I thought later after Kalle asked about making it a single entry such as
struct coalesce_receive_filt_rule first_rule;
but then the sizeof() is messed up and then the change has to be much
more careful.
Anyway, I was mostly trying to remove some warnings in drivers that
don't really have a very active maintainer anymore, since we have so
many in wireless it sometimes makes it hard to see which ones are new.
No particular feelings about this patch :-)
johannes
Johannes Berg <[email protected]> wrote:
> From: Johannes Berg <[email protected]>
>
> Just remove the extra asterisk to make it not be
> kernel-doc formatted.
>
> Signed-off-by: Johannes Berg <[email protected]>
9 patches applied to wireless-next.git, thanks.
76a8c54c53d8 wifi: ipw2100: fix warnings about non-kernel-doc
a08e3518bf45 wifi: libertas: fix a couple of sparse warnings
9d5b665775d6 wifi: wl18xx: add some missing endian conversions
3208ae450248 wifi: mwifiex: mark a variable unused
e1ff3b48996a wifi: mwifiex: fix endian conversion
fbe7e18581ef wifi: mwifiex: fix endian annotations in casts
df8e1af22cee wifi: cw1200: remove RCU STA pointer handling in TX
53b17c121f29 wifi: cw1200: use get_unaligned_le64()
8f15a8d6786c wifi: b43: remove empty switch statement
--
https://patchwork.kernel.org/project/linux-wireless/patch/20220904212910.8169e8c9090c.I0357e80cc86be2d4ac6205d1f53568444dcf7c9b@changeid/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
On Wed, Sep 07, 2022 at 08:57:28AM +0200, Johannes Berg wrote:
> On Tue, 2022-09-06 at 15:20 -0700, Brian Norris wrote:
> > On Sun, Sep 04, 2022 at 09:29:07PM +0200, Johannes Berg wrote:
> > > From: Johannes Berg <[email protected]>
> > >
> > > There are two, just change them to have a "u8 data[]" type
> > > member, and add casts where needed. No binary changes.
> >
> > Hmm, what exact warning are you looking at? This one?
> > https://clang.llvm.org/docs/DiagnosticsReference.html#wflexible-array-extensions
> >
>
> I think gcc also has one with W=1 now?
>
> But yes, it's similar to that one. I was looking at Jakub's netdev test
> builds here:
>
> https://patchwork.hopto.org/static/nipa/673828/12964988/build_32bit/stderr
>
> ../drivers/net/wireless/marvell/mwifiex/fw.h:2107:46: warning: array of flexible structures
> ../drivers/net/wireless/marvell/mwifiex/fw.h:2257:47: warning: array of flexible structures
I think you're actually looking at a sparse warning:
https://lore.kernel.org/linux-sparse/[email protected]/
I can convince clang to enable the warning I mentioned, but it's not on
by default, even with W=1. When enabled, it complains similarly, as well
as about a ton of other code too.
> > > @@ -2104,7 +2104,7 @@ struct mwifiex_fw_mef_entry {
> > > struct host_cmd_ds_mef_cfg {
> > > __le32 criteria;
> > > __le16 num_entries;
> > > - struct mwifiex_fw_mef_entry mef_entry[];
> > > + u8 mef_entry_data[];
> >
> > Do you actually need this part of the change? The 'mef_entry' (or
> > 'mef_entry_data') field is not actually used anywhere now, but I can't
> > tell what kind of warning is involved.
>
> But it itself is variable, so the compiler is (rightfully, IMHO) saying
> that you can't have an array of something that has a variable size, even
> if it's a variable array.
OK.
I suppose this warning makes sense when you're truly treating these as
arrays (of size more than 1). And in this case (mwifiex_cmd_mef_cfg()
and mwifiex_cmd_append_rpn_expression()), the code to (mostly?) properly
handle the flexible array is pretty ugly anyway, and doesn't really use
the type safety much (lots of casting through u8* and similar). So this
isn't really the best example to try to retain.
(mwifiex_cmd_coalesce_cfg() has some similarly ugly casting and math.)
But if the "array" is just an optional extension to a struct, and we
expect at most a single element, then I don't think the construct is
fundamentally wrong. It might still be hard to get correct in all cases
(e.g., ensuring the right buffer size), but it still feels like a decent
way to describe things.
> > > struct host_cmd_ds_coalesce_cfg {
> > > __le16 action;
> > > __le16 num_of_rules;
> > > - struct coalesce_receive_filt_rule rule[];
> > > + u8 rule_data[];
> >
> > These kinds of changes seem to be losing some valuable information. At a
> > minimum, it would be nice to leave a comment that points at the intended
> > struct; but ideally, we'd be able to still get the type safety from
> > actually using the struct, instead of relying on casts and/or u8/void.
>
> All fair points. This was the way we typically do this in e.g.
> ieee80211.h ("u8 variable[]", not "struct element elems[]"), but YMMV.
>
> I thought later after Kalle asked about making it a single entry such as
>
> struct coalesce_receive_filt_rule first_rule;
>
> but then the sizeof() is messed up and then the change has to be much
> more careful.
>
> Anyway, I was mostly trying to remove some warnings in drivers that
> don't really have a very active maintainer anymore, since we have so
> many in wireless it sometimes makes it hard to see which ones are new.
Sure, I guess it's good to clean some of these up.
Looking around, I don't see a great alternative, and per some of my
above notes, I don't think these are beautiful as-is.
> No particular feelings about this patch :-)
After a little more look, I'm fine with this patch. I'd probably
appreciate it better if there was a comment of some kind in the struct
definition, and perhaps mention sparse's -Wflexible-array-array. But
either way:
Reviewed-by: Brian Norris <[email protected]>
Thanks.
On Fri, 2022-09-09 at 13:45 -0700, Brian Norris wrote:
> > I think gcc also has one with W=1 now?
> >
> > But yes, it's similar to that one. I was looking at Jakub's netdev test
> > builds here:
> >
> > https://patchwork.hopto.org/static/nipa/673828/12964988/build_32bit/stderr
> >
> > ../drivers/net/wireless/marvell/mwifiex/fw.h:2107:46: warning: array of flexible structures
> > ../drivers/net/wireless/marvell/mwifiex/fw.h:2257:47: warning: array of flexible structures
>
> I think you're actually looking at a sparse warning:
>
> https://lore.kernel.org/linux-sparse/[email protected]/
>
> I can convince clang to enable the warning I mentioned, but it's not on
> by default, even with W=1. When enabled, it complains similarly, as well
> as about a ton of other code too.
Hm. I _think_ I saw it locally with just W=1, not C=1, but then that
would've been with gcc, probably 12.2 from Fedora. But I didn't try
again now, don't have much time.
> I suppose this warning makes sense when you're truly treating these as
> arrays (of size more than 1). And in this case (mwifiex_cmd_mef_cfg()
> and mwifiex_cmd_append_rpn_expression()), the code to (mostly?) properly
> handle the flexible array is pretty ugly anyway, and doesn't really use
> the type safety much (lots of casting through u8* and similar). So this
> isn't really the best example to try to retain.
Heh, fair point.
> (mwifiex_cmd_coalesce_cfg() has some similarly ugly casting and math.)
Yeah it kind of has to though, given that it's a variable-sized buffer
with variable-sized entries...
> But if the "array" is just an optional extension to a struct, and we
> expect at most a single element, then I don't think the construct is
> fundamentally wrong. It might still be hard to get correct in all cases
> (e.g., ensuring the right buffer size), but it still feels like a decent
> way to describe things.
Fair enough. It's like 0 or 1, maybe, but of course there's no way to
describe that to the compiler and say "I promise to never access [1] (or
higher)" :)
I guess the only real alternative would be to leave it as a _single_
entry, and then fix up all the sizeof() of the containing struct, if
any, to be offsetof(..., first_entry).
But that sort of describes a single entry should be present, which it
might not be.
> After a little more look, I'm fine with this patch. I'd probably
> appreciate it better if there was a comment of some kind in the struct
> definition, and perhaps mention sparse's -Wflexible-array-array.
>
Sure, I guess it makes sense to also figure out more precisely where the
warning appeared.
I'm running off for a trip for two weeks now-ish, so not going to send
it in the short term. If you wanted to do it instead, no objections, or
if Kalle wants to just add a comment while applying, or whatever.
In any case there are so many warnings in the drivers that still ought
to _have_ a maintainer (and I'm squinting at you for ath too, Kalle)
that it's pretty much irrelevant to fix this one quickly ;-)
johannes
Johannes Berg <[email protected]> wrote:
> From: Johannes Berg <[email protected]>
>
> There are a number of these here, fix them by using
> appropriate casts. No binary changes.
>
> Signed-off-by: Johannes Berg <[email protected]>
3 patches applied to wireless-next.git, thanks.
28255dd9a8de wifi: ipw2x00: fix array of flexible structures warnings
c70a9d6783cf wifi: rndis_wlan: fix array of flexible structures warning
4cf4cf6eb0bf wifi: mwifiex: fix array of flexible structures warnings
--
https://patchwork.kernel.org/project/linux-wireless/patch/20220904212910.645346411660.I471e8fadce54ea262920828f25b8e84545bcd07e@changeid/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches