Re: [PATCH v5 03/18] wlcore: simplify/fix/optimize reg_ch_conf_pending operations

Paolo Bonzini [off-list ref] writes:

On 15/03/19 00:16, Fenghua Yu wrote:

quoted

Hi, Valo,

On Thu, Mar 14, 2019 at 03:16:33PM +0200, Kalle Valo wrote:

quoted

Fenghua Yu [off-list ref] writes:

quoted

From: Paolo Bonzini <pbonzini@redhat.com>

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.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Fenghua Yu <redacted>

[...]

quoted

 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));
 	struct wiphy *wiphy = wl->hw->wiphy;
 	struct ieee80211_supported_band *band;
 	bool timeout = false;

[...]

quoted

@@ -1754,8 +1751,8 @@ int wlcore_cmd_regdomain_config_locked(struct wl1271 *wl)
 		goto out;
 	}
 
-	cmd->ch_bit_map1 = cpu_to_le32(tmp_ch_bitmap[0]);
-	cmd->ch_bit_map2 = cpu_to_le32(tmp_ch_bitmap[1]);
+	cmd->ch_bit_map1 = tmp_ch_bitmap[0];
+	cmd->ch_bit_map2 = tmp_ch_bitmap[1];

Will sparse still be happy? AFAICS you are now assigning u32 to __le32:

struct wl12xx_cmd_regdomain_dfs_config {
       struct wl1271_cmd_header header;

       __le32 ch_bit_map1;
       __le32 ch_bit_map2;

Discussion between Peter and Paolo (https://lkml.org/lkml/2019/3/4/521)
may answer your question.

No, Kalle is right.  You do need to change

-	u32 tmp_ch_bitmap[2];
+	u32 tmp_ch_bitmap[2] __aligned(sizeof(unsigned long));

into

-	u32 tmp_ch_bitmap[2];
+	__le32 tmp_ch_bitmap[2] __aligned(sizeof(unsigned long));

The assignment from wl->reg_ch_conf_pending to tmp_ch_bitmap is fine
because it goes through memcpy.

Thanks for confirming, I'll then drop patch 3 and wait for a new
version. IMHO it would be easier to submit this patch separately to
linux-wireless, no need to have within this bigger patchset.

-- 
Kalle Valo