On Mon, Mar 04, 2019 at 11:46:52AM +0100, Paolo Bonzini wrote:
> From: Paolo Bonzini <pbonzini@xxxxxxxxxx>
> Subject: [PATCH] wlcore: simplify/fix/optimize reg_ch_conf_pending operations
>
> Bitmaps are defined on unsigned longs, so the usage of u32[2] in the
> wlcore driver is incorrect. As noted by Peter Zijlstra, casting arrays
> to a bitmap is incorrect for big-endian architectures.
>
> When looking at it I observed that:
>
> - operations on reg_ch_conf_pending is always under the wl_lock mutex,
> so set_bit is overkill
>
> - the only case where reg_ch_conf_pending is accessed a u32 at a time is
> unnecessary too.
>
> This patch cleans up everything in this area, and changes tmp_ch_bitmap
> to have the proper alignment.
>
> Reported-by: Fenghua Yu <fenghua.yu@xxxxxxxxx>
> Signed-off-by: Paolo Bonzini <pbonzini@xxxxxxxxxx>
>
> diff --git a/drivers/net/wireless/ti/wlcore/cmd.c b/drivers/net/wireless/ti/wlcore/cmd.c
> index 903968735a74..3e093f3a7ec8 100644
> --- a/drivers/net/wireless/ti/wlcore/cmd.c
> +++ b/drivers/net/wireless/ti/wlcore/cmd.c
> @@ -1700,14 +1700,14 @@ void wlcore_set_pending_regdomain_ch(struct wl1271 *wl, u16 channel,
> ch_bit_idx = wlcore_get_reg_conf_ch_idx(band, channel);
>
> if (ch_bit_idx >= 0 && ch_bit_idx <= WL1271_MAX_CHANNELS)
> - set_bit(ch_bit_idx, (long *)wl->reg_ch_conf_pending);
> + __set_bit_le(ch_bit_idx, (long *)wl->reg_ch_conf_pending);
> }
>
> int wlcore_cmd_regdomain_config_locked(struct wl1271 *wl)
> {
> struct wl12xx_cmd_regdomain_dfs_config *cmd = NULL;
> int ret = 0, i, b, ch_bit_idx;
> - u32 tmp_ch_bitmap[2];
> + u32 tmp_ch_bitmap[2] __aligned(sizeof(unsigned long));
Also mark it as __le32 ?
> struct wiphy *wiphy = wl->hw->wiphy;
> struct ieee80211_supported_band *band;
> bool timeout = false;
> @@ -1717,7 +1717,7 @@ int wlcore_cmd_regdomain_config_locked(struct wl1271 *wl)
>
> wl1271_debug(DEBUG_CMD, "cmd reg domain config");
>
> - memset(tmp_ch_bitmap, 0, sizeof(tmp_ch_bitmap));
> + memcpy(tmp_ch_bitmap, wl->reg_ch_conf_pending, sizeof(tmp_ch_bitmap));
How about using:
bitmap_to_arr32(tmp_ch_bitmap, wl->reg_ch_conf_pending, sizeof(tmp_ch_bitmap));
for (i=0; i<2; i++)
tmp_ch_bitmap[i] = cpu_to_le32(tmp_ch_bitmap[i]);
(or add bitmap_to_arr32_le ?)
> for (b = NL80211_BAND_2GHZ; b <= NL80211_BAND_5GHZ; b++) {
> band = wiphy->bands[b];
> @@ -1738,13 +1738,10 @@ int wlcore_cmd_regdomain_config_locked(struct wl1271 *wl)
> if (ch_bit_idx < 0)
> continue;
>
> - set_bit(ch_bit_idx, (long *)tmp_ch_bitmap);
> + __set_bit_le(ch_bit_idx, (long *)tmp_ch_bitmap);
But you copied in reg_ch_conf_pending without doing an LE swizzle.
With the proposed change, we have two __le32 here and it works again.
> }
> }
>
> - tmp_ch_bitmap[0] |= wl->reg_ch_conf_pending[0];
> - tmp_ch_bitmap[1] |= wl->reg_ch_conf_pending[1];
> -
> if (!memcmp(tmp_ch_bitmap, wl->reg_ch_conf_last, sizeof(tmp_ch_bitmap)))
> goto out;
>
And then remove the cpu_to_le32() on assignment to ch_bit_map*.
> diff --git a/drivers/net/wireless/ti/wlcore/wlcore.h b/drivers/net/wireless/ti/wlcore/wlcore.h
> index dd14850b0603..870eea3e7a27 100644
> --- a/drivers/net/wireless/ti/wlcore/wlcore.h
> +++ b/drivers/net/wireless/ti/wlcore/wlcore.h
> @@ -320,9 +320,9 @@ struct wl1271 {
> bool watchdog_recovery;
>
> /* Reg domain last configuration */
> - u32 reg_ch_conf_last[2] __aligned(8);
> + DECLARE_BITMAP(reg_ch_conf_last, 64);
Is never actually used as a bitmap but used as opaque storage with
memcpy and memcmp against tmp_ch_bitmap.
> /* Reg domain pending configuration */
> - u32 reg_ch_conf_pending[2];
> + DECLARE_BITMAP(reg_ch_conf_pending, 64);
>
> /* Pointer that holds DMA-friendly block for the mailbox */
> void *mbox;
