| Message ID | 20200511154930.190212-14-Jerome.Pouiller@silabs.com |
|---|---|
| State | Not Applicable |
| Delegated to: | David Miller |
| Headers | show |
| Series | staging: wfx: fix support for big-endian hosts | expand |
Hi Jerome,
I love your patch! Perhaps something to improve:
[auto build test WARNING on staging/staging-testing]
[also build test WARNING on next-20200511]
[cannot apply to v5.7-rc5]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Jerome-Pouiller/staging-wfx-fix-support-for-big-endian-hosts/20200512-031750
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git ae73e7784871ebe2c43da619b4a1e2c9ff81508d
config: m68k-allmodconfig (attached as .config)
compiler: m68k-linux-gcc (GCC) 9.3.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day GCC_VERSION=9.3.0 make.cross ARCH=m68k
If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
In file included from include/linux/byteorder/big_endian.h:5,
from arch/m68k/include/uapi/asm/byteorder.h:5,
from include/asm-generic/bitops/le.h:6,
from arch/m68k/include/asm/bitops.h:528,
from include/linux/bitops.h:29,
from include/linux/kernel.h:12,
from include/asm-generic/bug.h:19,
from arch/m68k/include/asm/bug.h:32,
from include/linux/bug.h:5,
from include/linux/gpio/consumer.h:6,
from drivers/staging/wfx/bh.c:8:
drivers/staging/wfx/bh.c: In function 'tx_helper':
>> drivers/staging/wfx/bh.c:202:39: warning: passing argument 1 of '__swab16s' makes pointer from integer without a cast [-Wint-conversion]
202 | cpu_to_le16s(((struct hif_msg *)data)->len);
include/uapi/linux/byteorder/big_endian.h:96:38: note: in definition of macro '__cpu_to_le16s'
96 | #define __cpu_to_le16s(x) __swab16s((x))
| ^
>> drivers/staging/wfx/bh.c:202:2: note: in expansion of macro 'cpu_to_le16s'
202 | cpu_to_le16s(((struct hif_msg *)data)->len);
| ^~~~~~~~~~~~
In file included from include/linux/swab.h:5,
from include/uapi/linux/byteorder/big_endian.h:13,
from include/linux/byteorder/big_endian.h:5,
from arch/m68k/include/uapi/asm/byteorder.h:5,
from include/asm-generic/bitops/le.h:6,
from arch/m68k/include/asm/bitops.h:528,
from include/linux/bitops.h:29,
from include/linux/kernel.h:12,
from include/asm-generic/bug.h:19,
from arch/m68k/include/asm/bug.h:32,
from include/linux/bug.h:5,
from include/linux/gpio/consumer.h:6,
from drivers/staging/wfx/bh.c:8:
include/uapi/linux/swab.h:240:37: note: expected '__u16 *' {aka 'short unsigned int *'} but argument is of type 'u16' {aka 'short unsigned int'}
240 | static inline void __swab16s(__u16 *p)
| ~~~~~~~^
vim +/__swab16s +202 drivers/staging/wfx/bh.c
169
170 static void tx_helper(struct wfx_dev *wdev, struct hif_msg *hif)
171 {
172 int ret;
173 void *data;
174 bool is_encrypted = false;
175 size_t len = hif->len;
176
177 WARN(len < sizeof(*hif), "try to send corrupted data");
178
179 hif->seqnum = wdev->hif.tx_seqnum;
180 wdev->hif.tx_seqnum = (wdev->hif.tx_seqnum + 1) % (HIF_COUNTER_MAX + 1);
181
182 if (wfx_is_secure_command(wdev, hif->id)) {
183 len = round_up(len - sizeof(hif->len), 16) + sizeof(hif->len) +
184 sizeof(struct hif_sl_msg_hdr) +
185 sizeof(struct hif_sl_tag);
186 // AES support encryption in-place. However, mac80211 access to
187 // 802.11 header after frame was sent (to get MAC addresses).
188 // So, keep origin buffer clear.
189 data = kmalloc(len, GFP_KERNEL);
190 if (!data)
191 goto end;
192 is_encrypted = true;
193 ret = wfx_sl_encode(wdev, hif, data);
194 if (ret)
195 goto end;
196 } else {
197 data = hif;
198 }
199 WARN(len > wdev->hw_caps.size_inp_ch_buf,
200 "%s: request exceed WFx capability: %zu > %d\n", __func__,
201 len, wdev->hw_caps.size_inp_ch_buf);
> 202 cpu_to_le16s(((struct hif_msg *)data)->len);
203 len = wdev->hwbus_ops->align_size(wdev->hwbus_priv, len);
204 ret = wfx_data_write(wdev, data, len);
205 if (ret)
206 goto end;
207
208 wdev->hif.tx_buffers_used++;
209 _trace_hif_send(hif, wdev->hif.tx_buffers_used);
210 end:
211 if (is_encrypted)
212 kfree(data);
213 }
214
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Hi Jerome, On Mon, May 11, 2020 at 5:53 PM Jerome Pouiller <Jerome.Pouiller@silabs.com> wrote: > From: Jérôme Pouiller <jerome.pouiller@silabs.com> > > The struct hif_msg is received from the hardware. So, it declared as > little endian. However, it is also accessed from many places in the > driver. Sparse complains about that: > > drivers/staging/wfx/bh.c:88:32: warning: restricted __le16 degrades to integer > drivers/staging/wfx/bh.c:88:32: warning: restricted __le16 degrades to integer > drivers/staging/wfx/bh.c:93:32: warning: restricted __le16 degrades to integer > drivers/staging/wfx/bh.c:93:32: warning: cast to restricted __le16 > drivers/staging/wfx/bh.c:93:32: warning: restricted __le16 degrades to integer > drivers/staging/wfx/bh.c:121:25: warning: incorrect type in argument 2 (different base types) > drivers/staging/wfx/bh.c:121:25: expected unsigned int len > drivers/staging/wfx/bh.c:121:25: got restricted __le16 [usertype] len > drivers/staging/wfx/hif_rx.c:27:22: warning: restricted __le16 degrades to integer > drivers/staging/wfx/hif_rx.c:347:39: warning: incorrect type in argument 7 (different base types) > drivers/staging/wfx/hif_rx.c:347:39: expected unsigned int [usertype] len > drivers/staging/wfx/hif_rx.c:347:39: got restricted __le16 const [usertype] len > drivers/staging/wfx/hif_rx.c:365:39: warning: incorrect type in argument 7 (different base types) > drivers/staging/wfx/hif_rx.c:365:39: expected unsigned int [usertype] len > drivers/staging/wfx/hif_rx.c:365:39: got restricted __le16 const [usertype] len > drivers/staging/wfx/./traces.h:195:1: warning: incorrect type in assignment (different base types) > drivers/staging/wfx/./traces.h:195:1: expected int msg_len > drivers/staging/wfx/./traces.h:195:1: got restricted __le16 const [usertype] len > drivers/staging/wfx/./traces.h:195:1: warning: incorrect type in assignment (different base types) > drivers/staging/wfx/./traces.h:195:1: expected int msg_len > drivers/staging/wfx/./traces.h:195:1: got restricted __le16 const [usertype] len > drivers/staging/wfx/debug.c:319:20: warning: restricted __le16 degrades to integer > drivers/staging/wfx/secure_link.c:85:27: warning: restricted __le16 degrades to integer > drivers/staging/wfx/secure_link.c:85:27: warning: restricted __le16 degrades to integer Thanks for your patch! > In order to make Sparse happy and to keep access from the driver easy, > this patch declare 'len' with native endianness. > > On reception of hardware data, this patch takes care to do byte-swap and > keep Sparse happy. Which means sparse can no longer do any checking on the field, and new bugs may/will creep in in the future, unnoticed. > --- a/drivers/staging/wfx/hif_api_general.h > +++ b/drivers/staging/wfx/hif_api_general.h > @@ -23,7 +23,10 @@ > #define HIF_COUNTER_MAX 7 > > struct hif_msg { > - __le16 len; > + // len is in fact little endian. However, it is widely used in the > + // driver, so we declare it in native byte order and we reorder just > + // before/after send/receive it (see bh.c). > + u16 len; While there's a small penalty associated with always doing the conversion on big-endian platforms, it will probably be lost in the noise anyway. Gr{oetje,eeting}s, Geert
On Tuesday 12 May 2020 09:43:34 CEST Geert Uytterhoeven wrote: > Hi Jerome, > > On Mon, May 11, 2020 at 5:53 PM Jerome Pouiller > <Jerome.Pouiller@silabs.com> wrote: > > From: Jérôme Pouiller <jerome.pouiller@silabs.com> > > > > The struct hif_msg is received from the hardware. So, it declared as > > little endian. However, it is also accessed from many places in the > > driver. Sparse complains about that: > > > > drivers/staging/wfx/bh.c:88:32: warning: restricted __le16 degrades to integer > > drivers/staging/wfx/bh.c:88:32: warning: restricted __le16 degrades to integer > > drivers/staging/wfx/bh.c:93:32: warning: restricted __le16 degrades to integer > > drivers/staging/wfx/bh.c:93:32: warning: cast to restricted __le16 > > drivers/staging/wfx/bh.c:93:32: warning: restricted __le16 degrades to integer > > drivers/staging/wfx/bh.c:121:25: warning: incorrect type in argument 2 (different base types) > > drivers/staging/wfx/bh.c:121:25: expected unsigned int len > > drivers/staging/wfx/bh.c:121:25: got restricted __le16 [usertype] len > > drivers/staging/wfx/hif_rx.c:27:22: warning: restricted __le16 degrades to integer > > drivers/staging/wfx/hif_rx.c:347:39: warning: incorrect type in argument 7 (different base types) > > drivers/staging/wfx/hif_rx.c:347:39: expected unsigned int [usertype] len > > drivers/staging/wfx/hif_rx.c:347:39: got restricted __le16 const [usertype] len > > drivers/staging/wfx/hif_rx.c:365:39: warning: incorrect type in argument 7 (different base types) > > drivers/staging/wfx/hif_rx.c:365:39: expected unsigned int [usertype] len > > drivers/staging/wfx/hif_rx.c:365:39: got restricted __le16 const [usertype] len > > drivers/staging/wfx/./traces.h:195:1: warning: incorrect type in assignment (different base types) > > drivers/staging/wfx/./traces.h:195:1: expected int msg_len > > drivers/staging/wfx/./traces.h:195:1: got restricted __le16 const [usertype] len > > drivers/staging/wfx/./traces.h:195:1: warning: incorrect type in assignment (different base types) > > drivers/staging/wfx/./traces.h:195:1: expected int msg_len > > drivers/staging/wfx/./traces.h:195:1: got restricted __le16 const [usertype] len > > drivers/staging/wfx/debug.c:319:20: warning: restricted __le16 degrades to integer > > drivers/staging/wfx/secure_link.c:85:27: warning: restricted __le16 degrades to integer > > drivers/staging/wfx/secure_link.c:85:27: warning: restricted __le16 degrades to integer > > Thanks for your patch! > > > In order to make Sparse happy and to keep access from the driver easy, > > this patch declare 'len' with native endianness. > > > > On reception of hardware data, this patch takes care to do byte-swap and > > keep Sparse happy. > > Which means sparse can no longer do any checking on the field, > and new bugs may/will creep in in the future, unnoticed. > > > --- a/drivers/staging/wfx/hif_api_general.h > > +++ b/drivers/staging/wfx/hif_api_general.h > > @@ -23,7 +23,10 @@ > > #define HIF_COUNTER_MAX 7 > > > > struct hif_msg { > > - __le16 len; > > + // len is in fact little endian. However, it is widely used in the > > + // driver, so we declare it in native byte order and we reorder just > > + // before/after send/receive it (see bh.c). > > + u16 len; > > While there's a small penalty associated with always doing the conversion > on big-endian platforms, it will probably be lost in the noise anyway. I have made the changes to show you that the code is far more complicated with a le16... and the result was not as complicated as I expected... I am going to post a v2.
diff --git a/drivers/staging/wfx/bh.c b/drivers/staging/wfx/bh.c index 55724e4295c4..0355b1a1c4bb 100644 --- a/drivers/staging/wfx/bh.c +++ b/drivers/staging/wfx/bh.c @@ -74,6 +74,7 @@ static int rx_helper(struct wfx_dev *wdev, size_t read_len, int *is_cnf) _trace_piggyback(piggyback, false); hif = (struct hif_msg *)skb->data; + le16_to_cpus((__le16 *)&hif->len); WARN(hif->encrypted & 0x1, "unsupported encryption type"); if (hif->encrypted == 0x2) { if (wfx_sl_decode(wdev, (void *)hif)) { @@ -84,12 +85,11 @@ static int rx_helper(struct wfx_dev *wdev, size_t read_len, int *is_cnf) // piggyback is probably correct. return piggyback; } - le16_to_cpus(&hif->len); + le16_to_cpus((__le16 *)&hif->len); computed_len = round_up(hif->len - sizeof(hif->len), 16) + sizeof(struct hif_sl_msg) + sizeof(struct hif_sl_tag); } else { - le16_to_cpus(&hif->len); computed_len = round_up(hif->len, 2); } if (computed_len != read_len) { @@ -172,7 +172,7 @@ static void tx_helper(struct wfx_dev *wdev, struct hif_msg *hif) int ret; void *data; bool is_encrypted = false; - size_t len = le16_to_cpu(hif->len); + size_t len = hif->len; WARN(len < sizeof(*hif), "try to send corrupted data"); @@ -199,6 +199,7 @@ static void tx_helper(struct wfx_dev *wdev, struct hif_msg *hif) WARN(len > wdev->hw_caps.size_inp_ch_buf, "%s: request exceed WFx capability: %zu > %d\n", __func__, len, wdev->hw_caps.size_inp_ch_buf); + cpu_to_le16s(((struct hif_msg *)data)->len); len = wdev->hwbus_ops->align_size(wdev->hwbus_priv, len); ret = wfx_data_write(wdev, data, len); if (ret) diff --git a/drivers/staging/wfx/data_tx.c b/drivers/staging/wfx/data_tx.c index 014fa36c8f78..84656d1a6278 100644 --- a/drivers/staging/wfx/data_tx.c +++ b/drivers/staging/wfx/data_tx.c @@ -384,7 +384,7 @@ static int wfx_tx_inner(struct wfx_vif *wvif, struct ieee80211_sta *sta, skb_push(skb, wmsg_len); memset(skb->data, 0, wmsg_len); hif_msg = (struct hif_msg *)skb->data; - hif_msg->len = cpu_to_le16(skb->len); + hif_msg->len = skb->len; hif_msg->id = HIF_REQ_ID_TX; hif_msg->interface = wvif->id; if (skb->len > wvif->wdev->hw_caps.size_inp_ch_buf) { diff --git a/drivers/staging/wfx/hif_api_general.h b/drivers/staging/wfx/hif_api_general.h index 995752b9f168..a359ae76511a 100644 --- a/drivers/staging/wfx/hif_api_general.h +++ b/drivers/staging/wfx/hif_api_general.h @@ -23,7 +23,10 @@ #define HIF_COUNTER_MAX 7 struct hif_msg { - __le16 len; + // len is in fact little endian. However, it is widely used in the + // driver, so we declare it in native byte order and we reorder just + // before/after send/receive it (see bh.c). + u16 len; u8 id; u8 reserved:1; u8 interface:2; @@ -277,7 +280,8 @@ struct hif_sl_msg_hdr { struct hif_sl_msg { struct hif_sl_msg_hdr hdr; - __le16 len; + // Same note than struct hif_msg + u16 len; u8 payload[]; } __packed; diff --git a/drivers/staging/wfx/hif_tx.c b/drivers/staging/wfx/hif_tx.c index 490a9de54faf..6c6618197b91 100644 --- a/drivers/staging/wfx/hif_tx.c +++ b/drivers/staging/wfx/hif_tx.c @@ -33,7 +33,7 @@ static void wfx_fill_header(struct hif_msg *hif, int if_id, WARN(size > 0xFFF, "requested buffer is too large: %zu bytes", size); WARN(if_id > 0x3, "invalid interface ID %d", if_id); - hif->len = cpu_to_le16(size + 4); + hif->len = size + 4; hif->id = cmd; hif->interface = if_id; }