| Message ID | 20180529191143.13081-1-thibaut.robert@gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Kalle Valo |
| Headers | show |
On Tue, May 29, 2018 at 09:11:43PM +0200, Thibaut Robert wrote: > diff --git a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c > index e248702ee519..745bf5ca2622 100644 > --- a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c > +++ b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c > @@ -1431,7 +1431,7 @@ void wilc_wfi_p2p_rx(struct net_device *dev, u8 *buff, u32 size) > > freq = ieee80211_channel_to_frequency(curr_channel, NL80211_BAND_2GHZ); > > - if (!ieee80211_is_action(buff[FRAME_TYPE_ID])) { > + if (!ieee80211_is_action(cpu_to_le16(buff[FRAME_TYPE_ID]))) { "buff" comes from the network, it's going to be little endian, not cpu endian. The rest of the function treats it as CPU endian but I'm pretty sure it's wrong... > cfg80211_rx_mgmt(priv->wdev, freq, 0, buff, size, 0); > return; > } > diff --git a/drivers/staging/wilc1000/wilc_wlan.c b/drivers/staging/wilc1000/wilc_wlan.c > index 28c93f3f846e..a5ac1d26590b 100644 > --- a/drivers/staging/wilc1000/wilc_wlan.c > +++ b/drivers/staging/wilc1000/wilc_wlan.c > @@ -560,7 +560,8 @@ int wilc_wlan_handle_txq(struct net_device *dev, u32 *txq_count) > int ret = 0; > int counter; > int timeout; > - u32 vmm_table[WILC_VMM_TBL_SIZE]; > + __le32 vmm_table[WILC_VMM_TBL_SIZE]; > + u32 table_entry; > struct wilc_vif *vif; > struct wilc *wilc; > const struct wilc_hif_func *func; > @@ -598,10 +599,10 @@ int wilc_wlan_handle_txq(struct net_device *dev, u32 *txq_count) > if ((sum + vmm_sz) > LINUX_TX_SIZE) > break; > > - vmm_table[i] = vmm_sz / 4; > + table_entry = vmm_sz / 4; > if (tqe->type == WILC_CFG_PKT) > - vmm_table[i] |= BIT(10); > - vmm_table[i] = cpu_to_le32(vmm_table[i]); > + table_entry |= BIT(10); > + vmm_table[i] = cpu_to_le32(table_entry); > > i++; > sum += vmm_sz; > @@ -704,8 +705,7 @@ int wilc_wlan_handle_txq(struct net_device *dev, u32 *txq_count) > if (vmm_table[i] == 0) > break; > > - vmm_table[i] = cpu_to_le32(vmm_table[i]); > - vmm_sz = (vmm_table[i] & 0x3ff); > + vmm_sz = (le32_to_cpu(vmm_table[i]) & 0x3ff); > vmm_sz *= 4; > header = (tqe->type << 31) | > (tqe->buffer_size << 15) | > @@ -715,8 +715,7 @@ int wilc_wlan_handle_txq(struct net_device *dev, u32 *txq_count) > else > header &= ~BIT(30); > > - header = cpu_to_le32(header); > - memcpy(&txb[offset], &header, 4); > + *((__le32 *)&txb[offset]) = cpu_to_le32(header); I worry about alignment issues here. That might be the reason for the memcpy(). (I'm reading as fast as I can and don't the code so I may be wrong). > if (tqe->type == WILC_CFG_PKT) { > buffer_offset = ETH_CONFIG_PKT_HDR_OFFSET; > } else if (tqe->type == WILC_NET_PKT) { > @@ -770,8 +769,7 @@ static void wilc_wlan_handle_rx_buff(struct wilc *wilc, u8 *buffer, int size) > > do { > buff_ptr = buffer + offset; > - memcpy(&header, buff_ptr, 4); > - header = cpu_to_le32(header); > + header = le32_to_cpup((__le32 *)buff_ptr); Maybe the same, whenever you see a memcpy(). > > is_cfg_packet = (header >> 31) & 0x1; > pkt_offset = (header >> 22) & 0x1ff; > @@ -942,6 +940,7 @@ int wilc_wlan_firmware_download(struct wilc *wilc, const u8 *buffer, > u32 offset; > u32 addr, size, size2, blksz; > u8 *dma_buffer; > + const __le32 *header; > int ret = 0; > > blksz = BIT(12); > @@ -952,10 +951,9 @@ int wilc_wlan_firmware_download(struct wilc *wilc, const u8 *buffer, > > offset = 0; > do { > - memcpy(&addr, &buffer[offset], 4); > - memcpy(&size, &buffer[offset + 4], 4); > - addr = cpu_to_le32(addr); > - size = cpu_to_le32(size); > + header = (__le32 *)buffer + offset; > + addr = le32_to_cpu(header[0]); > + size = le32_to_cpu(header[1]); > acquire_bus(wilc, ACQUIRE_ONLY); > offset += 8; > while (((int)size) && (offset < buffer_size)) { > diff --git a/drivers/staging/wilc1000/wilc_wlan_cfg.c b/drivers/staging/wilc1000/wilc_wlan_cfg.c > index c0b9b700f4d7..4a914d8572aa 100644 > --- a/drivers/staging/wilc1000/wilc_wlan_cfg.c > +++ b/drivers/staging/wilc1000/wilc_wlan_cfg.c > @@ -275,14 +275,14 @@ static int wilc_wlan_cfg_set_bin(u8 *frame, u32 offset, u16 id, u8 *b, u32 size) > > static void wilc_wlan_parse_response_frame(u8 *info, int size) > { > - u32 wid, len = 0, i = 0; > + u32 wid; > + int len = 0, i = 0; Why did we make these int now? > > while (size > 0) { > i = 0; > - wid = info[0] | (info[1] << 8); > - wid = cpu_to_le32(wid); > + wid = le16_to_cpup((__le16 *)info); > > - switch ((wid >> 12) & 0x7) { > + switch (info[1] >> 4) { Why do we not need to mask by 0x7? Anyway, I feel like this isn't beautiful. We should be using a macro and "wid" instead of magically poking into info[1]. switch(SOME_MACRO(wid)) { regards, dan carpenter
On Tue, 29 May 2018 21:11:43 +0200 Thibaut Robert <thibaut.robert@gmail.com> wrote: > This commit fix a few sparse warnings. It mostly consists of fixing > the type declarations and avoiding the use of variables with mixed > endianness values. > > Signed-off-by: Thibaut Robert <thibaut.robert@gmail.com> > --- > drivers/staging/wilc1000/wilc_spi.c | 15 ++++++----- > .../staging/wilc1000/wilc_wfi_cfgoperations.c | 2 +- > drivers/staging/wilc1000/wilc_wlan.c | 26 > +++++++++---------- drivers/staging/wilc1000/wilc_wlan_cfg.c | > 8 +++--- drivers/staging/wilc1000/wilc_wlan_cfg.h | 4 +-- > 5 files changed, 27 insertions(+), 28 deletions(-) > > diff --git a/drivers/staging/wilc1000/wilc_spi.c > b/drivers/staging/wilc1000/wilc_spi.c index > 647526387784..e51f0d91a376 100644 --- > a/drivers/staging/wilc1000/wilc_spi.c +++ > b/drivers/staging/wilc1000/wilc_spi.c @@ -666,11 +666,11 @@ static > int spi_data_write(struct wilc *wilc, u8 *b, u32 sz) static int > spi_internal_write(struct wilc *wilc, u32 adr, u32 dat) { > struct spi_device *spi = to_spi_device(wilc->dev); > + __le32 le32dat = cpu_to_le32(dat); > int result; > > - dat = cpu_to_le32(dat); > - result = spi_cmd_complete(wilc, CMD_INTERNAL_WRITE, adr, (u8 > *)&dat, 4, > - 0); > + result = spi_cmd_complete(wilc, CMD_INTERNAL_WRITE, adr, (u8 > *)&le32dat, > + 4, 0); > if (result != N_OK) > dev_err(&spi->dev, "Failed internal write cmd...\n"); > > @@ -689,7 +689,7 @@ static int spi_internal_read(struct wilc *wilc, > u32 adr, u32 *data) return 0; > } > > - *data = cpu_to_le32(*data); > + le32_to_cpus(data); > > return 1; > } > @@ -706,15 +706,16 @@ static int wilc_spi_write_reg(struct wilc > *wilc, u32 addr, u32 data) int result = N_OK; > u8 cmd = CMD_SINGLE_WRITE; > u8 clockless = 0; > + __le32 le32data = cpu_to_le32(data); > > - data = cpu_to_le32(data); > if (addr < 0x30) { > /* Clockless register */ > cmd = CMD_INTERNAL_WRITE; > clockless = 1; > } > > - result = spi_cmd_complete(wilc, cmd, addr, (u8 *)&data, 4, > clockless); > + result = spi_cmd_complete(wilc, cmd, addr, (u8 *)&le32data, > 4, > + clockless); > if (result != N_OK) > dev_err(&spi->dev, "Failed cmd, write reg > (%08x)...\n", addr); > @@ -769,7 +770,7 @@ static int wilc_spi_read_reg(struct wilc *wilc, > u32 addr, u32 *data) return 0; > } > > - *data = cpu_to_le32(*data); > + le32_to_cpus(data); > > return 1; > } > diff --git a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c > b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c index > e248702ee519..745bf5ca2622 100644 --- > a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c +++ > b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c @@ -1431,7 > +1431,7 @@ void wilc_wfi_p2p_rx(struct net_device *dev, u8 *buff, u32 > size) freq = ieee80211_channel_to_frequency(curr_channel, > NL80211_BAND_2GHZ); > - if (!ieee80211_is_action(buff[FRAME_TYPE_ID])) { > + if (!ieee80211_is_action(cpu_to_le16(buff[FRAME_TYPE_ID]))) { > cfg80211_rx_mgmt(priv->wdev, freq, 0, buff, size, 0); > return; > } > diff --git a/drivers/staging/wilc1000/wilc_wlan.c > b/drivers/staging/wilc1000/wilc_wlan.c index > 28c93f3f846e..a5ac1d26590b 100644 --- > a/drivers/staging/wilc1000/wilc_wlan.c +++ > b/drivers/staging/wilc1000/wilc_wlan.c @@ -560,7 +560,8 @@ int > wilc_wlan_handle_txq(struct net_device *dev, u32 *txq_count) int ret > = 0; int counter; > int timeout; > - u32 vmm_table[WILC_VMM_TBL_SIZE]; > + __le32 vmm_table[WILC_VMM_TBL_SIZE]; > + u32 table_entry; > struct wilc_vif *vif; > struct wilc *wilc; > const struct wilc_hif_func *func; > @@ -598,10 +599,10 @@ int wilc_wlan_handle_txq(struct net_device > *dev, u32 *txq_count) if ((sum + vmm_sz) > LINUX_TX_SIZE) > break; > > - vmm_table[i] = vmm_sz / 4; > + table_entry = vmm_sz / 4; > if (tqe->type == WILC_CFG_PKT) > - vmm_table[i] |= BIT(10); > - vmm_table[i] = cpu_to_le32(vmm_table[i]); > + table_entry |= BIT(10); > + vmm_table[i] = cpu_to_le32(table_entry); > > i++; > sum += vmm_sz; > @@ -704,8 +705,7 @@ int wilc_wlan_handle_txq(struct net_device *dev, > u32 *txq_count) if (vmm_table[i] == 0) > break; > > - vmm_table[i] = cpu_to_le32(vmm_table[i]); > - vmm_sz = (vmm_table[i] & 0x3ff); > + vmm_sz = (le32_to_cpu(vmm_table[i]) & 0x3ff); > vmm_sz *= 4; > header = (tqe->type << 31) | > (tqe->buffer_size << 15) | > @@ -715,8 +715,7 @@ int wilc_wlan_handle_txq(struct net_device *dev, > u32 *txq_count) else > header &= ~BIT(30); > > - header = cpu_to_le32(header); > - memcpy(&txb[offset], &header, 4); > + *((__le32 *)&txb[offset]) = cpu_to_le32(header); > if (tqe->type == WILC_CFG_PKT) { > buffer_offset = ETH_CONFIG_PKT_HDR_OFFSET; > } else if (tqe->type == WILC_NET_PKT) { > @@ -770,8 +769,7 @@ static void wilc_wlan_handle_rx_buff(struct wilc > *wilc, u8 *buffer, int size) > do { > buff_ptr = buffer + offset; > - memcpy(&header, buff_ptr, 4); > - header = cpu_to_le32(header); > + header = le32_to_cpup((__le32 *)buff_ptr); > > is_cfg_packet = (header >> 31) & 0x1; > pkt_offset = (header >> 22) & 0x1ff; > @@ -942,6 +940,7 @@ int wilc_wlan_firmware_download(struct wilc > *wilc, const u8 *buffer, u32 offset; > u32 addr, size, size2, blksz; > u8 *dma_buffer; > + const __le32 *header; > int ret = 0; > > blksz = BIT(12); > @@ -952,10 +951,9 @@ int wilc_wlan_firmware_download(struct wilc > *wilc, const u8 *buffer, > offset = 0; > do { > - memcpy(&addr, &buffer[offset], 4); > - memcpy(&size, &buffer[offset + 4], 4); > - addr = cpu_to_le32(addr); > - size = cpu_to_le32(size); > + header = (__le32 *)buffer + offset; In addition to Dan's comments, I have one more observation. The above line has a crash(Oops), while downloading the firmware to WILC1000 module i.e "header = (__le32 *)buffer + offset;". The crash can be fixes by putting extra bracket like header = (__le32 *)(buffer + offset); But I suggest to avoid use of 'header' variable and only change the below 2 lines. addr = cpu_to_le32(addr); size = cpu_to_le32(size); to le32_to_cpus(addr); le32_to_cpus(size); > + addr = le32_to_cpu(header[0]); > + size = le32_to_cpu(header[1]); > acquire_bus(wilc, ACQUIRE_ONLY); > offset += 8; > while (((int)size) && (offset < buffer_size)) { > diff --git a/drivers/staging/wilc1000/wilc_wlan_cfg.c > b/drivers/staging/wilc1000/wilc_wlan_cfg.c index > c0b9b700f4d7..4a914d8572aa 100644 --- > a/drivers/staging/wilc1000/wilc_wlan_cfg.c +++ > b/drivers/staging/wilc1000/wilc_wlan_cfg.c @@ -275,14 +275,14 @@ > static int wilc_wlan_cfg_set_bin(u8 *frame, u32 offset, u16 id, u8 > *b, u32 size) static void wilc_wlan_parse_response_frame(u8 *info, > int size) { > - u32 wid, len = 0, i = 0; > + u32 wid; > + int len = 0, i = 0; > > while (size > 0) { > i = 0; > - wid = info[0] | (info[1] << 8); > - wid = cpu_to_le32(wid); > + wid = le16_to_cpup((__le16 *)info); > > - switch ((wid >> 12) & 0x7) { > + switch (info[1] >> 4) { > case WID_CHAR: > do { > if (g_cfg_byte[i].id == WID_NIL) > diff --git a/drivers/staging/wilc1000/wilc_wlan_cfg.h > b/drivers/staging/wilc1000/wilc_wlan_cfg.h index > 08092a551840..9e029338bcab 100644 --- > a/drivers/staging/wilc1000/wilc_wlan_cfg.h +++ > b/drivers/staging/wilc1000/wilc_wlan_cfg.h @@ -18,12 +18,12 @@ struct > wilc_cfg_byte { > struct wilc_cfg_hword { > u16 id; > - u16 val; > + __le16 val; > }; > > struct wilc_cfg_word { > u32 id; > - u32 val; > + __le32 val; > }; > > struct wilc_cfg_str {
Le mercredi 30 mai 2018 à 14:17:25 (+0300), Dan Carpenter a écrit : > On Tue, May 29, 2018 at 09:11:43PM +0200, Thibaut Robert wrote: > > diff --git a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c > > index e248702ee519..745bf5ca2622 100644 > > --- a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c > > +++ b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c > > @@ -1431,7 +1431,7 @@ void wilc_wfi_p2p_rx(struct net_device *dev, u8 *buff, u32 size) > > > > freq = ieee80211_channel_to_frequency(curr_channel, NL80211_BAND_2GHZ); > > > > - if (!ieee80211_is_action(buff[FRAME_TYPE_ID])) { > > + if (!ieee80211_is_action(cpu_to_le16(buff[FRAME_TYPE_ID]))) { > > "buff" comes from the network, it's going to be little endian, not cpu > endian. The rest of the function treats it as CPU endian but I'm pretty > sure it's wrong... buff comes from the network but we are looking at single byte here. ieee80211_is_action expects an le16, so we I added this to extend an u8 to an le16. Is this incorrect ? Or maybe we the buff has the second byte ? but that I can't tell. > > > cfg80211_rx_mgmt(priv->wdev, freq, 0, buff, size, 0); > > return; > > } > > > > diff --git a/drivers/staging/wilc1000/wilc_wlan.c b/drivers/staging/wilc1000/wilc_wlan.c > > index 28c93f3f846e..a5ac1d26590b 100644 > > --- a/drivers/staging/wilc1000/wilc_wlan.c > > +++ b/drivers/staging/wilc1000/wilc_wlan.c > > @@ -560,7 +560,8 @@ int wilc_wlan_handle_txq(struct net_device *dev, u32 *txq_count) > > int ret = 0; > > int counter; > > int timeout; > > - u32 vmm_table[WILC_VMM_TBL_SIZE]; > > + __le32 vmm_table[WILC_VMM_TBL_SIZE]; > > + u32 table_entry; > > struct wilc_vif *vif; > > struct wilc *wilc; > > const struct wilc_hif_func *func; > > @@ -598,10 +599,10 @@ int wilc_wlan_handle_txq(struct net_device *dev, u32 *txq_count) > > if ((sum + vmm_sz) > LINUX_TX_SIZE) > > break; > > > > - vmm_table[i] = vmm_sz / 4; > > + table_entry = vmm_sz / 4; > > if (tqe->type == WILC_CFG_PKT) > > - vmm_table[i] |= BIT(10); > > - vmm_table[i] = cpu_to_le32(vmm_table[i]); > > + table_entry |= BIT(10); > > + vmm_table[i] = cpu_to_le32(table_entry); > > > > i++; > > sum += vmm_sz; > > @@ -704,8 +705,7 @@ int wilc_wlan_handle_txq(struct net_device *dev, u32 *txq_count) > > if (vmm_table[i] == 0) > > break; > > > > - vmm_table[i] = cpu_to_le32(vmm_table[i]); > > - vmm_sz = (vmm_table[i] & 0x3ff); > > + vmm_sz = (le32_to_cpu(vmm_table[i]) & 0x3ff); > > vmm_sz *= 4; > > header = (tqe->type << 31) | > > (tqe->buffer_size << 15) | > > @@ -715,8 +715,7 @@ int wilc_wlan_handle_txq(struct net_device *dev, u32 *txq_count) > > else > > header &= ~BIT(30); > > > > - header = cpu_to_le32(header); > > - memcpy(&txb[offset], &header, 4); > > + *((__le32 *)&txb[offset]) = cpu_to_le32(header); > > I worry about alignment issues here. That might be the reason for the > memcpy(). (I'm reading as fast as I can and don't the code so I may > be wrong). > > > if (tqe->type == WILC_CFG_PKT) { > > buffer_offset = ETH_CONFIG_PKT_HDR_OFFSET; > > } else if (tqe->type == WILC_NET_PKT) { > > @@ -770,8 +769,7 @@ static void wilc_wlan_handle_rx_buff(struct wilc *wilc, u8 *buffer, int size) > > > > do { > > buff_ptr = buffer + offset; > > - memcpy(&header, buff_ptr, 4); > > - header = cpu_to_le32(header); > > + header = le32_to_cpup((__le32 *)buff_ptr); > > Maybe the same, whenever you see a memcpy(). > > > > > is_cfg_packet = (header >> 31) & 0x1; > > pkt_offset = (header >> 22) & 0x1ff; > > @@ -942,6 +940,7 @@ int wilc_wlan_firmware_download(struct wilc *wilc, const u8 *buffer, > > u32 offset; > > u32 addr, size, size2, blksz; > > u8 *dma_buffer; > > + const __le32 *header; > > int ret = 0; > > > > blksz = BIT(12); > > @@ -952,10 +951,9 @@ int wilc_wlan_firmware_download(struct wilc *wilc, const u8 *buffer, > > > > offset = 0; > > do { > > - memcpy(&addr, &buffer[offset], 4); > > - memcpy(&size, &buffer[offset + 4], 4); > > - addr = cpu_to_le32(addr); > > - size = cpu_to_le32(size); > > + header = (__le32 *)buffer + offset; > > + addr = le32_to_cpu(header[0]); > > + size = le32_to_cpu(header[1]); > > acquire_bus(wilc, ACQUIRE_ONLY); > > offset += 8; > > while (((int)size) && (offset < buffer_size)) { > > diff --git a/drivers/staging/wilc1000/wilc_wlan_cfg.c b/drivers/staging/wilc1000/wilc_wlan_cfg.c > > index c0b9b700f4d7..4a914d8572aa 100644 > > --- a/drivers/staging/wilc1000/wilc_wlan_cfg.c > > +++ b/drivers/staging/wilc1000/wilc_wlan_cfg.c > > @@ -275,14 +275,14 @@ static int wilc_wlan_cfg_set_bin(u8 *frame, u32 offset, u16 id, u8 *b, u32 size) > > > > static void wilc_wlan_parse_response_frame(u8 *info, int size) > > { > > - u32 wid, len = 0, i = 0; > > + u32 wid; > > + int len = 0, i = 0; > > Why did we make these int now? > > > > > while (size > 0) { > > i = 0; > > - wid = info[0] | (info[1] << 8); > > - wid = cpu_to_le32(wid); > > + wid = le16_to_cpup((__le16 *)info); > > > > - switch ((wid >> 12) & 0x7) { > > + switch (info[1] >> 4) { > > Why do we not need to mask by 0x7? Anyway, I feel like this isn't > beautiful. We should be using a macro and "wid" instead of magically > poking into info[1]. > > switch(SOME_MACRO(wid)) { > > > regards, > dan carpenter
On Tue, 5 Jun 2018 10:33:25 +0200 Thibaut Robert <thibaut.robert@gmail.com> wrote: > Le mardi 05 juin 2018 à 10:36:31 (+0300), Dan Carpenter a écrit : > > On Mon, Jun 04, 2018 at 09:32:50PM +0200, Thibaut Robert wrote: > > > Le mercredi 30 mai 2018 à 14:17:25 (+0300), Dan Carpenter a > > > écrit : > > > > On Tue, May 29, 2018 at 09:11:43PM +0200, Thibaut Robert > > > > wrote: > > > > > diff --git > > > > > a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c > > > > > b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c index > > > > > e248702ee519..745bf5ca2622 100644 --- > > > > > a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c +++ > > > > > b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c @@ > > > > > -1431,7 +1431,7 @@ void wilc_wfi_p2p_rx(struct net_device > > > > > *dev, u8 *buff, u32 size) freq = > > > > > ieee80211_channel_to_frequency(curr_channel, > > > > > NL80211_BAND_2GHZ); > > > > > - if (!ieee80211_is_action(buff[FRAME_TYPE_ID])) { > > > > > + if > > > > > (!ieee80211_is_action(cpu_to_le16(buff[FRAME_TYPE_ID]))) { > > > > > > > > "buff" comes from the network, it's going to be little endian, > > > > not cpu endian. The rest of the function treats it as CPU > > > > endian but I'm pretty sure it's wrong... > > > buff comes from the network but we are looking at single byte > > > here. ieee80211_is_action expects an le16, so we I added this to > > > extend an u8 to an le16. Is this incorrect ? > > > > > > Or maybe we the buff has the second byte ? but that I can't > > > tell. > > > > You raise a good point that I hadn't seen. The original code is > > clearly buggy. But your fix isn't correct either... The other > > thing to consider is that cpu_to_le16() is basically a cast to u16 > > on x86 so it's a no-op here. > The sparse warning is clearly spotting a real issue. I tried to at > least have big endian handle correctly the 0-255 case. I am willing > to drop the change (since I agree it's not very satisfying and will > mask an issue), but may I ask you to explain how it is wrong ? How > would you correctly expand an u8 to __le16 ? I think in big endian we > need to swap the bytes. > Yes, when sending the data to ieee80211_is_action() it should pass 16bit value instead of 8bit value. It should be corrected. But the buff received from the wifi module is already in little endian format. As the data is already in LE endian format so in my opinion the conversion is not required while calling ieee80211_is_action(). > > > > Really the right thing is to not treat buff as an array of u8 but a > > struct. The code assumes that frame_type is 0-255 but probably it's > > supposed to go up to U16_MAX. > > > > struct whatever { > > __le16 frame_type; > > ... > > > > There probably is already a struct like that, but I don't know what > > it is. I don't know this code at all, I'm just guessing. > > > I was thinking the same. I don't know whether this buf contains a > standard struct or something hw-specific. I'll try to dig further and > submit a separate patch if I can. Maybe Aditya can give more > information ? > The buffer structure contains both some custom information along with standard info. As the frame_control is the first 2 byte in the in buff so casting it can also help to extract frame_control. Please refer the code in mgmt_tx() which use like below const struct ieee80211_mgmt *mgmt; .... mgmt = (const struct ieee80211_mgmt *)buf; ieee80211_is_action(mgmt->frame_control) I think another approach can be by using proper type cast with first 2 bytes of buffer(to extract 16bit value) and pass it to ieee80211_is_action(). Regards, Ajay
On Mon, Jun 04, 2018 at 09:32:50PM +0200, Thibaut Robert wrote: > Le mercredi 30 mai 2018 à 14:17:25 (+0300), Dan Carpenter a écrit : > > On Tue, May 29, 2018 at 09:11:43PM +0200, Thibaut Robert wrote: > > > diff --git a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c > > > index e248702ee519..745bf5ca2622 100644 > > > --- a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c > > > +++ b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c > > > @@ -1431,7 +1431,7 @@ void wilc_wfi_p2p_rx(struct net_device *dev, u8 *buff, u32 size) > > > > > > freq = ieee80211_channel_to_frequency(curr_channel, NL80211_BAND_2GHZ); > > > > > > - if (!ieee80211_is_action(buff[FRAME_TYPE_ID])) { > > > + if (!ieee80211_is_action(cpu_to_le16(buff[FRAME_TYPE_ID]))) { > > > > "buff" comes from the network, it's going to be little endian, not cpu > > endian. The rest of the function treats it as CPU endian but I'm pretty > > sure it's wrong... > buff comes from the network but we are looking at single byte here. > ieee80211_is_action expects an le16, so we I added this to extend an u8 > to an le16. Is this incorrect ? > > Or maybe we the buff has the second byte ? but that I can't tell. You raise a good point that I hadn't seen. The original code is clearly buggy. But your fix isn't correct either... The other thing to consider is that cpu_to_le16() is basically a cast to u16 on x86 so it's a no-op here. Really the right thing is to not treat buff as an array of u8 but a struct. The code assumes that frame_type is 0-255 but probably it's supposed to go up to U16_MAX. struct whatever { __le16 frame_type; ... There probably is already a struct like that, but I don't know what it is. I don't know this code at all, I'm just guessing. regards, dan carpenter
Le mardi 05 juin 2018 à 10:36:31 (+0300), Dan Carpenter a écrit : > On Mon, Jun 04, 2018 at 09:32:50PM +0200, Thibaut Robert wrote: > > Le mercredi 30 mai 2018 à 14:17:25 (+0300), Dan Carpenter a écrit : > > > On Tue, May 29, 2018 at 09:11:43PM +0200, Thibaut Robert wrote: > > > > diff --git a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c > > > > index e248702ee519..745bf5ca2622 100644 > > > > --- a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c > > > > +++ b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c > > > > @@ -1431,7 +1431,7 @@ void wilc_wfi_p2p_rx(struct net_device *dev, u8 *buff, u32 size) > > > > > > > > freq = ieee80211_channel_to_frequency(curr_channel, NL80211_BAND_2GHZ); > > > > > > > > - if (!ieee80211_is_action(buff[FRAME_TYPE_ID])) { > > > > + if (!ieee80211_is_action(cpu_to_le16(buff[FRAME_TYPE_ID]))) { > > > > > > "buff" comes from the network, it's going to be little endian, not cpu > > > endian. The rest of the function treats it as CPU endian but I'm pretty > > > sure it's wrong... > > buff comes from the network but we are looking at single byte here. > > ieee80211_is_action expects an le16, so we I added this to extend an u8 > > to an le16. Is this incorrect ? > > > > Or maybe we the buff has the second byte ? but that I can't tell. > > You raise a good point that I hadn't seen. The original code is clearly > buggy. But your fix isn't correct either... The other thing to > consider is that cpu_to_le16() is basically a cast to u16 on x86 so it's > a no-op here. The sparse warning is clearly spotting a real issue. I tried to at least have big endian handle correctly the 0-255 case. I am willing to drop the change (since I agree it's not very satisfying and will mask an issue), but may I ask you to explain how it is wrong ? How would you correctly expand an u8 to __le16 ? I think in big endian we need to swap the bytes. > > Really the right thing is to not treat buff as an array of u8 but a > struct. The code assumes that frame_type is 0-255 but probably it's > supposed to go up to U16_MAX. > > struct whatever { > __le16 frame_type; > ... > > There probably is already a struct like that, but I don't know what it > is. I don't know this code at all, I'm just guessing. > I was thinking the same. I don't know whether this buf contains a standard struct or something hw-specific. I'll try to dig further and submit a separate patch if I can. Maybe Aditya can give more information ? > regards, > dan carpenter > Regards, Thibaut
On Tue, Jun 05, 2018 at 10:33:25AM +0200, Thibaut Robert wrote: > Le mardi 05 juin 2018 à 10:36:31 (+0300), Dan Carpenter a écrit : > > On Mon, Jun 04, 2018 at 09:32:50PM +0200, Thibaut Robert wrote: > > > Le mercredi 30 mai 2018 à 14:17:25 (+0300), Dan Carpenter a écrit : > > > > On Tue, May 29, 2018 at 09:11:43PM +0200, Thibaut Robert wrote: > > > > > diff --git a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c > > > > > index e248702ee519..745bf5ca2622 100644 > > > > > --- a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c > > > > > +++ b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c > > > > > @@ -1431,7 +1431,7 @@ void wilc_wfi_p2p_rx(struct net_device *dev, u8 *buff, u32 size) > > > > > > > > > > freq = ieee80211_channel_to_frequency(curr_channel, NL80211_BAND_2GHZ); > > > > > > > > > > - if (!ieee80211_is_action(buff[FRAME_TYPE_ID])) { > > > > > + if (!ieee80211_is_action(cpu_to_le16(buff[FRAME_TYPE_ID]))) { > > > > > > > > "buff" comes from the network, it's going to be little endian, not cpu > > > > endian. The rest of the function treats it as CPU endian but I'm pretty > > > > sure it's wrong... > > > buff comes from the network but we are looking at single byte here. > > > ieee80211_is_action expects an le16, so we I added this to extend an u8 > > > to an le16. Is this incorrect ? > > > > > > Or maybe we the buff has the second byte ? but that I can't tell. > > > > You raise a good point that I hadn't seen. The original code is clearly > > buggy. But your fix isn't correct either... The other thing to > > consider is that cpu_to_le16() is basically a cast to u16 on x86 so it's > > a no-op here. > The sparse warning is clearly spotting a real issue. I tried to at least > have big endian handle correctly the 0-255 case. I am willing to drop > the change (since I agree it's not very satisfying and will mask an issue), > but may I ask you to explain how it is wrong ? How would you correctly expand > an u8 to __le16 ? I think in big endian we need to swap the bytes. You're right that on big endian we would need to swap the bytes and cpu_to_le16() does that. regards, dan carpenter
diff --git a/drivers/staging/wilc1000/wilc_spi.c b/drivers/staging/wilc1000/wilc_spi.c index 647526387784..e51f0d91a376 100644 --- a/drivers/staging/wilc1000/wilc_spi.c +++ b/drivers/staging/wilc1000/wilc_spi.c @@ -666,11 +666,11 @@ static int spi_data_write(struct wilc *wilc, u8 *b, u32 sz) static int spi_internal_write(struct wilc *wilc, u32 adr, u32 dat) { struct spi_device *spi = to_spi_device(wilc->dev); + __le32 le32dat = cpu_to_le32(dat); int result; - dat = cpu_to_le32(dat); - result = spi_cmd_complete(wilc, CMD_INTERNAL_WRITE, adr, (u8 *)&dat, 4, - 0); + result = spi_cmd_complete(wilc, CMD_INTERNAL_WRITE, adr, (u8 *)&le32dat, + 4, 0); if (result != N_OK) dev_err(&spi->dev, "Failed internal write cmd...\n"); @@ -689,7 +689,7 @@ static int spi_internal_read(struct wilc *wilc, u32 adr, u32 *data) return 0; } - *data = cpu_to_le32(*data); + le32_to_cpus(data); return 1; } @@ -706,15 +706,16 @@ static int wilc_spi_write_reg(struct wilc *wilc, u32 addr, u32 data) int result = N_OK; u8 cmd = CMD_SINGLE_WRITE; u8 clockless = 0; + __le32 le32data = cpu_to_le32(data); - data = cpu_to_le32(data); if (addr < 0x30) { /* Clockless register */ cmd = CMD_INTERNAL_WRITE; clockless = 1; } - result = spi_cmd_complete(wilc, cmd, addr, (u8 *)&data, 4, clockless); + result = spi_cmd_complete(wilc, cmd, addr, (u8 *)&le32data, 4, + clockless); if (result != N_OK) dev_err(&spi->dev, "Failed cmd, write reg (%08x)...\n", addr); @@ -769,7 +770,7 @@ static int wilc_spi_read_reg(struct wilc *wilc, u32 addr, u32 *data) return 0; } - *data = cpu_to_le32(*data); + le32_to_cpus(data); return 1; } diff --git a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c index e248702ee519..745bf5ca2622 100644 --- a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c +++ b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c @@ -1431,7 +1431,7 @@ void wilc_wfi_p2p_rx(struct net_device *dev, u8 *buff, u32 size) freq = ieee80211_channel_to_frequency(curr_channel, NL80211_BAND_2GHZ); - if (!ieee80211_is_action(buff[FRAME_TYPE_ID])) { + if (!ieee80211_is_action(cpu_to_le16(buff[FRAME_TYPE_ID]))) { cfg80211_rx_mgmt(priv->wdev, freq, 0, buff, size, 0); return; } diff --git a/drivers/staging/wilc1000/wilc_wlan.c b/drivers/staging/wilc1000/wilc_wlan.c index 28c93f3f846e..a5ac1d26590b 100644 --- a/drivers/staging/wilc1000/wilc_wlan.c +++ b/drivers/staging/wilc1000/wilc_wlan.c @@ -560,7 +560,8 @@ int wilc_wlan_handle_txq(struct net_device *dev, u32 *txq_count) int ret = 0; int counter; int timeout; - u32 vmm_table[WILC_VMM_TBL_SIZE]; + __le32 vmm_table[WILC_VMM_TBL_SIZE]; + u32 table_entry; struct wilc_vif *vif; struct wilc *wilc; const struct wilc_hif_func *func; @@ -598,10 +599,10 @@ int wilc_wlan_handle_txq(struct net_device *dev, u32 *txq_count) if ((sum + vmm_sz) > LINUX_TX_SIZE) break; - vmm_table[i] = vmm_sz / 4; + table_entry = vmm_sz / 4; if (tqe->type == WILC_CFG_PKT) - vmm_table[i] |= BIT(10); - vmm_table[i] = cpu_to_le32(vmm_table[i]); + table_entry |= BIT(10); + vmm_table[i] = cpu_to_le32(table_entry); i++; sum += vmm_sz; @@ -704,8 +705,7 @@ int wilc_wlan_handle_txq(struct net_device *dev, u32 *txq_count) if (vmm_table[i] == 0) break; - vmm_table[i] = cpu_to_le32(vmm_table[i]); - vmm_sz = (vmm_table[i] & 0x3ff); + vmm_sz = (le32_to_cpu(vmm_table[i]) & 0x3ff); vmm_sz *= 4; header = (tqe->type << 31) | (tqe->buffer_size << 15) | @@ -715,8 +715,7 @@ int wilc_wlan_handle_txq(struct net_device *dev, u32 *txq_count) else header &= ~BIT(30); - header = cpu_to_le32(header); - memcpy(&txb[offset], &header, 4); + *((__le32 *)&txb[offset]) = cpu_to_le32(header); if (tqe->type == WILC_CFG_PKT) { buffer_offset = ETH_CONFIG_PKT_HDR_OFFSET; } else if (tqe->type == WILC_NET_PKT) { @@ -770,8 +769,7 @@ static void wilc_wlan_handle_rx_buff(struct wilc *wilc, u8 *buffer, int size) do { buff_ptr = buffer + offset; - memcpy(&header, buff_ptr, 4); - header = cpu_to_le32(header); + header = le32_to_cpup((__le32 *)buff_ptr); is_cfg_packet = (header >> 31) & 0x1; pkt_offset = (header >> 22) & 0x1ff; @@ -942,6 +940,7 @@ int wilc_wlan_firmware_download(struct wilc *wilc, const u8 *buffer, u32 offset; u32 addr, size, size2, blksz; u8 *dma_buffer; + const __le32 *header; int ret = 0; blksz = BIT(12); @@ -952,10 +951,9 @@ int wilc_wlan_firmware_download(struct wilc *wilc, const u8 *buffer, offset = 0; do { - memcpy(&addr, &buffer[offset], 4); - memcpy(&size, &buffer[offset + 4], 4); - addr = cpu_to_le32(addr); - size = cpu_to_le32(size); + header = (__le32 *)buffer + offset; + addr = le32_to_cpu(header[0]); + size = le32_to_cpu(header[1]); acquire_bus(wilc, ACQUIRE_ONLY); offset += 8; while (((int)size) && (offset < buffer_size)) { diff --git a/drivers/staging/wilc1000/wilc_wlan_cfg.c b/drivers/staging/wilc1000/wilc_wlan_cfg.c index c0b9b700f4d7..4a914d8572aa 100644 --- a/drivers/staging/wilc1000/wilc_wlan_cfg.c +++ b/drivers/staging/wilc1000/wilc_wlan_cfg.c @@ -275,14 +275,14 @@ static int wilc_wlan_cfg_set_bin(u8 *frame, u32 offset, u16 id, u8 *b, u32 size) static void wilc_wlan_parse_response_frame(u8 *info, int size) { - u32 wid, len = 0, i = 0; + u32 wid; + int len = 0, i = 0; while (size > 0) { i = 0; - wid = info[0] | (info[1] << 8); - wid = cpu_to_le32(wid); + wid = le16_to_cpup((__le16 *)info); - switch ((wid >> 12) & 0x7) { + switch (info[1] >> 4) { case WID_CHAR: do { if (g_cfg_byte[i].id == WID_NIL) diff --git a/drivers/staging/wilc1000/wilc_wlan_cfg.h b/drivers/staging/wilc1000/wilc_wlan_cfg.h index 08092a551840..9e029338bcab 100644 --- a/drivers/staging/wilc1000/wilc_wlan_cfg.h +++ b/drivers/staging/wilc1000/wilc_wlan_cfg.h @@ -18,12 +18,12 @@ struct wilc_cfg_byte { struct wilc_cfg_hword { u16 id; - u16 val; + __le16 val; }; struct wilc_cfg_word { u32 id; - u32 val; + __le32 val; }; struct wilc_cfg_str {
This commit fix a few sparse warnings. It mostly consists of fixing the type declarations and avoiding the use of variables with mixed endianness values. Signed-off-by: Thibaut Robert <thibaut.robert@gmail.com> --- drivers/staging/wilc1000/wilc_spi.c | 15 ++++++----- .../staging/wilc1000/wilc_wfi_cfgoperations.c | 2 +- drivers/staging/wilc1000/wilc_wlan.c | 26 +++++++++---------- drivers/staging/wilc1000/wilc_wlan_cfg.c | 8 +++--- drivers/staging/wilc1000/wilc_wlan_cfg.h | 4 +-- 5 files changed, 27 insertions(+), 28 deletions(-)