Re: [PATCH 13/17] staging: wfx: fix endianness of the field 'len'
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
> wrote:
> > From: Jérôme Pouiller
> >
> > 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).
> > + u16len;
>
> 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.
--
Jérôme Pouiller
Re: [PATCH 13/17] staging: wfx: fix endianness of the field 'len'
Hi Jerome,
On Mon, May 11, 2020 at 5:53 PM Jerome Pouiller
wrote:
> From: Jérôme Pouiller
>
> 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).
> + u16len;
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
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: [PATCH 13/17] staging: wfx: fix endianness of the field 'len'
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
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,
[PATCH 13/17] staging: wfx: fix endianness of the field 'len'
From: Jérôme Pouiller
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
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.
Signed-off-by: Jérôme Pouiller
---
drivers/staging/wfx/bh.c | 7 ---
drivers/staging/wfx/data_tx.c | 2 +-
drivers/staging/wfx/hif_api_general.h | 8 ++--
drivers/staging/wfx/hif_tx.c | 2 +-
4 files changed, 12 insertions(+), 7 deletions(-)
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 *)>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(>len);
+ le16_to_cpus((__le16 *)>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(>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,
