CHECK drivers/net/wireless/wl12xx/wl1251_tx.c
drivers/net/wireless/wl12xx/wl1251_tx.c:118:32: warning: incorrect type in assignment (different base types)
drivers/net/wireless/wl12xx/wl1251_tx.c:118:32: expected unsigned short [unsigned] [usertype] frag_threshold
drivers/net/wireless/wl12xx/wl1251_tx.c:118:32: got restricted __le16 [usertype] <noident>
drivers/net/wireless/wl12xx/wl1251_tx.c:164:24: warning: incorrect type in assignment (different base types)
drivers/net/wireless/wl12xx/wl1251_tx.c:164:24: expected unsigned short [unsigned] [usertype] length
drivers/net/wireless/wl12xx/wl1251_tx.c:164:24: got restricted __le16 [usertype] <noident>
drivers/net/wireless/wl12xx/wl1251_tx.c:166:22: warning: incorrect type in assignment (different base types)
drivers/net/wireless/wl12xx/wl1251_tx.c:166:22: expected unsigned short [unsigned] [usertype] rate
drivers/net/wireless/wl12xx/wl1251_tx.c:166:22: got restricted __le16 [usertype] <noident>
drivers/net/wireless/wl12xx/wl1251_tx.c:167:29: warning: incorrect type in assignment (different base types)
drivers/net/wireless/wl12xx/wl1251_tx.c:167:29: expected unsigned int [unsigned] [usertype] expiry_time
drivers/net/wireless/wl12xx/wl1251_tx.c:167:29: got restricted __le32 [usertype] <noident>
drivers/net/wireless/wl12xx/wl1251_tx.c:200:43: warning: incorrect type in argument 1 (different base types)
drivers/net/wireless/wl12xx/wl1251_tx.c:200:43: expected restricted __le16 [usertype] fc
drivers/net/wireless/wl12xx/wl1251_tx.c:200:43: got unsigned short [unsigned] [assigned] [usertype] fc
CHECK drivers/net/wireless/wl12xx/wl1251_cmd.c
drivers/net/wireless/wl12xx/wl1251_cmd.c:428:39: warning: incorrect type in assignment (different base types)
drivers/net/wireless/wl12xx/wl1251_cmd.c:428:39: expected unsigned int [unsigned] [usertype] rx_config_options
drivers/net/wireless/wl12xx/wl1251_cmd.c:428:39: got restricted __le32 [usertype] <noident>
drivers/net/wireless/wl12xx/wl1251_cmd.c:429:39: warning: incorrect type in assignment (different base types)
drivers/net/wireless/wl12xx/wl1251_cmd.c:429:39: expected unsigned int [unsigned] [usertype] rx_filter_options
drivers/net/wireless/wl12xx/wl1251_cmd.c:429:39: got restricted __le32 [usertype] <noident>
drivers/net/wireless/wl12xx/wl1251_cmd.c:435:29: warning: incorrect type in assignment (different base types)
drivers/net/wireless/wl12xx/wl1251_cmd.c:435:29: expected unsigned short [unsigned] [usertype] tx_rate
drivers/net/wireless/wl12xx/wl1251_cmd.c:435:29: got restricted __le16 [usertype] <noident>
drivers/net/wireless/wl12xx/wl1251_cmd.c:439:47: warning: incorrect type in assignment (different base types)
drivers/net/wireless/wl12xx/wl1251_cmd.c:439:47: expected unsigned int [unsigned] [usertype] min_duration
drivers/net/wireless/wl12xx/wl1251_cmd.c:439:47: got restricted __le32 [usertype] <noident>
drivers/net/wireless/wl12xx/wl1251_cmd.c:441:47: warning: incorrect type in assignment (different base types)
drivers/net/wireless/wl12xx/wl1251_cmd.c:441:47: expected unsigned int [unsigned] [usertype] max_duration
drivers/net/wireless/wl12xx/wl1251_cmd.c:441:47: got restricted __le32 [usertype] <noident>
CHECK drivers/net/wireless/wl12xx/wl1251_boot.c
drivers/net/wireless/wl12xx/wl1251_boot.c:228:22: warning: symbol 'interrupt' shadows an earlier one
/home/linville/git/wireless-next-2.6/arch/x86/include/asm/hw_irq.h:132:13: originally declared here
drivers/net/wireless/wl12xx/wl1251_boot.c:470:21: warning: incorrect type in assignment (different base types)
drivers/net/wireless/wl12xx/wl1251_boot.c:470:21: expected unsigned int [unsigned] [assigned] [usertype] val
drivers/net/wireless/wl12xx/wl1251_boot.c:470:21: got restricted __le32 [usertype] <noident>
Signed-off-by: John W. Linville <[email protected]>
---
I made some assumptions about the endian-ness of some structure members
-- please verify! Also, I'm reasonably certain there are more endian
issues lurking...
drivers/net/wireless/wl12xx/wl1251_boot.c | 10 +++++-----
drivers/net/wireless/wl12xx/wl1251_cmd.h | 12 ++++++------
drivers/net/wireless/wl12xx/wl1251_tx.c | 10 ++++++----
drivers/net/wireless/wl12xx/wl1251_tx.h | 8 ++++----
4 files changed, 21 insertions(+), 19 deletions(-)
diff --git a/drivers/net/wireless/wl12xx/wl1251_boot.c b/drivers/net/wireless/wl12xx/wl1251_boot.c
index 2545123..c688895 100644
--- a/drivers/net/wireless/wl12xx/wl1251_boot.c
+++ b/drivers/net/wireless/wl12xx/wl1251_boot.c
@@ -225,7 +225,7 @@ static void wl1251_boot_set_ecpu_ctrl(struct wl1251 *wl, u32 flag)
int wl1251_boot_run_firmware(struct wl1251 *wl)
{
int loop, ret;
- u32 chip_id, interrupt;
+ u32 chip_id, acx_intr;
wl1251_boot_set_ecpu_ctrl(wl, ECPU_CONTROL_HALT);
@@ -242,15 +242,15 @@ int wl1251_boot_run_firmware(struct wl1251 *wl)
loop = 0;
while (loop++ < INIT_LOOP) {
udelay(INIT_LOOP_DELAY);
- interrupt = wl1251_reg_read32(wl, ACX_REG_INTERRUPT_NO_CLEAR);
+ acx_intr = wl1251_reg_read32(wl, ACX_REG_INTERRUPT_NO_CLEAR);
- if (interrupt == 0xffffffff) {
+ if (acx_intr == 0xffffffff) {
wl1251_error("error reading hardware complete "
"init indication");
return -EIO;
}
/* check that ACX_INTR_INIT_COMPLETE is enabled */
- else if (interrupt & WL1251_ACX_INTR_INIT_COMPLETE) {
+ else if (acx_intr & WL1251_ACX_INTR_INIT_COMPLETE) {
wl1251_reg_write32(wl, ACX_REG_INTERRUPT_ACK,
WL1251_ACX_INTR_INIT_COMPLETE);
break;
@@ -467,7 +467,7 @@ static int wl1251_boot_upload_nvs(struct wl1251 *wl)
val = (nvs_ptr[0] | (nvs_ptr[1] << 8)
| (nvs_ptr[2] << 16) | (nvs_ptr[3] << 24));
- val = cpu_to_le32(val);
+ val = (u32 __force) cpu_to_le32(val);
wl1251_debug(DEBUG_BOOT,
"nvs write table 0x%x: 0x%x",
diff --git a/drivers/net/wireless/wl12xx/wl1251_cmd.h b/drivers/net/wireless/wl12xx/wl1251_cmd.h
index 4ad67ca..ca1cb24 100644
--- a/drivers/net/wireless/wl12xx/wl1251_cmd.h
+++ b/drivers/net/wireless/wl12xx/wl1251_cmd.h
@@ -175,8 +175,8 @@ struct cmd_read_write_memory {
#define WL1251_SCAN_NUM_PROBES 3
struct wl1251_scan_parameters {
- u32 rx_config_options;
- u32 rx_filter_options;
+ __le32 rx_config_options;
+ __le32 rx_filter_options;
/*
* Scan options:
@@ -186,7 +186,7 @@ struct wl1251_scan_parameters {
* bit 2: voice mode, 0 for normal scan.
* bit 3: scan priority, 1 for high priority.
*/
- u16 scan_options;
+ __le16 scan_options;
/* Number of channels to scan */
u8 num_channels;
@@ -195,7 +195,7 @@ struct wl1251_scan_parameters {
u8 num_probe_requests;
/* Rate and modulation for probe requests */
- u16 tx_rate;
+ __le16 tx_rate;
u8 tid_trigger;
u8 ssid_len;
@@ -204,8 +204,8 @@ struct wl1251_scan_parameters {
} __attribute__ ((packed));
struct wl1251_scan_ch_parameters {
- u32 min_duration; /* in TU */
- u32 max_duration; /* in TU */
+ __le32 min_duration; /* in TU */
+ __le32 max_duration; /* in TU */
u32 bssid_lsb;
u16 bssid_msb;
diff --git a/drivers/net/wireless/wl12xx/wl1251_tx.c b/drivers/net/wireless/wl12xx/wl1251_tx.c
index c822318..a38ec19 100644
--- a/drivers/net/wireless/wl12xx/wl1251_tx.c
+++ b/drivers/net/wireless/wl12xx/wl1251_tx.c
@@ -117,7 +117,7 @@ static void wl1251_tx_frag_block_num(struct tx_double_buffer_desc *tx_hdr)
frag_threshold = IEEE80211_MAX_FRAG_THRESHOLD;
tx_hdr->frag_threshold = cpu_to_le16(frag_threshold);
- payload_len = tx_hdr->length + MAX_MSDU_SECURITY_LENGTH;
+ payload_len = le16_to_cpu(tx_hdr->length) + MAX_MSDU_SECURITY_LENGTH;
if (payload_len > frag_threshold) {
mem_blocks_per_frag =
@@ -191,11 +191,13 @@ static int wl1251_tx_send_packet(struct wl1251 *wl, struct sk_buff *skb,
if (control->control.hw_key &&
control->control.hw_key->alg == ALG_TKIP) {
int hdrlen;
- u16 fc;
+ __le16 fc;
+ u16 length;
u8 *pos;
- fc = *(u16 *)(skb->data + sizeof(*tx_hdr));
- tx_hdr->length += WL1251_TKIP_IV_SPACE;
+ fc = *(__le16 *)(skb->data + sizeof(*tx_hdr));
+ length = le16_to_cpu(tx_hdr->length) + WL1251_TKIP_IV_SPACE;
+ tx_hdr->length = cpu_to_le16(length);
hdrlen = ieee80211_hdrlen(fc);
diff --git a/drivers/net/wireless/wl12xx/wl1251_tx.h b/drivers/net/wireless/wl12xx/wl1251_tx.h
index 55856c6..dff127f 100644
--- a/drivers/net/wireless/wl12xx/wl1251_tx.h
+++ b/drivers/net/wireless/wl12xx/wl1251_tx.h
@@ -114,7 +114,7 @@ struct tx_control {
struct tx_double_buffer_desc {
/* Length of payload, including headers. */
- u16 length;
+ __le16 length;
/*
* A bit mask that specifies the initial rate to be used
@@ -133,10 +133,10 @@ struct tx_double_buffer_desc {
* 0x0800 - 48Mbits
* 0x1000 - 54Mbits
*/
- u16 rate;
+ __le16 rate;
/* Time in us that a packet can spend in the target */
- u32 expiry_time;
+ __le32 expiry_time;
/* index of the TX queue used for this packet */
u8 xmit_queue;
@@ -150,7 +150,7 @@ struct tx_double_buffer_desc {
* The FW should cut the packet into fragments
* of this size.
*/
- u16 frag_threshold;
+ __le16 frag_threshold;
/* Numbers of HW queue blocks to be allocated */
u8 num_mem_blocks;
--
1.7.1.1
On Thu, Jul 22, 2010 at 3:45 AM, Kalle Valo <[email protected]> wrote:
>>> @@ -191,11 +191,13 @@ static int wl1251_tx_send_packet(struct wl1251 *wl, struct sk_buff *skb,
>>> ? ? ?if (control->control.hw_key &&
>>> ? ? ? ? ?control->control.hw_key->alg == ALG_TKIP) {
>>> ? ? ? ? ? ? ?int hdrlen;
>>> - ? ? ? ? ? ?u16 fc;
>>> + ? ? ? ? ? ?__le16 fc;
>>> + ? ? ? ? ? ?u16 length;
>>> ? ? ? ? ? ? ?u8 *pos;
[...]
>>> + ? ? ? ? ? ?length = le16_to_cpu(tx_hdr->length) + WL1251_TKIP_IV_SPACE;
>>> + ? ? ? ? ? ?tx_hdr->length = cpu_to_le16(length);
>>
>> ...which is treated correctly here.
>
> This is different. Here we are adding something to a __le16 value, not
> calculating with pointers.
Just throwing in my 2 cents, unless there's some other reason to add the
length temporary, here you can just do:
le16_add_cpu(&tx_hdr->length, WL1251_TKIP_IV_SPACE);
--
Bob Copeland %% http://www.bobcopeland.com
On Thu, Jul 22, 2010 at 09:45:10AM +0200, Kalle Valo wrote:
> On 07/22/2010 08:34 AM, Luciano Coelho wrote:
> >> @@ -467,7 +467,7 @@ static int wl1251_boot_upload_nvs(struct wl1251 *wl)
> >> val = (nvs_ptr[0] | (nvs_ptr[1] << 8)
> >> | (nvs_ptr[2] << 16) | (nvs_ptr[3] << 24));
> >>
> >> - val = cpu_to_le32(val);
> >> + val = (u32 __force) cpu_to_le32(val);
> >
> > This will work, but such casts always make me a bit suspicious. I think
> > this is fine for now
>
> This line was very suspicious already from beginning, I can't remember
> why it was added and I don't see why it's needed here.
It certainly is a bit strange, and rather ugly as well. I agree that
the write should probably just take the le32 instead, but I was more
interested in silencing sparse than in rewriting a driver for which
I have not hardware. :-)
I could drop that hunk for the time being?
John
--
John W. Linville Someday the world will need a hero, and you
[email protected] might be all we have. Be ready.
Hi John,
Great that someone finally made the endianess changes that we never had
the time to do for wl1251. Thanks for that! :)
I took a look at your patch and I have a few minor comments below, but
the best person to comment would certainly be Kalle, not me ;)
On Wed, 2010-07-21 at 18:31 +0200, ext John W. Linville wrote:
> diff --git a/drivers/net/wireless/wl12xx/wl1251_boot.c b/drivers/net/wireless/wl12xx/wl1251_boot.c
> index 2545123..c688895 100644
> --- a/drivers/net/wireless/wl12xx/wl1251_boot.c
> +++ b/drivers/net/wireless/wl12xx/wl1251_boot.c
[...]
> @@ -467,7 +467,7 @@ static int wl1251_boot_upload_nvs(struct wl1251 *wl)
> val = (nvs_ptr[0] | (nvs_ptr[1] << 8)
> | (nvs_ptr[2] << 16) | (nvs_ptr[3] << 24));
>
> - val = cpu_to_le32(val);
> + val = (u32 __force) cpu_to_le32(val);
This will work, but such casts always make me a bit suspicious. I think
this is fine for now, but later I think we should make sure that all the
_write() functions explicitly receive __le32 as val, or receives the
cpu's u32 and converts it before actually writing the value, for clarity
reasons. Kalle, what do you think?
> diff --git a/drivers/net/wireless/wl12xx/wl1251_tx.c b/drivers/net/wireless/wl12xx/wl1251_tx.c
> index c822318..a38ec19 100644
> --- a/drivers/net/wireless/wl12xx/wl1251_tx.c
> +++ b/drivers/net/wireless/wl12xx/wl1251_tx.c
[...]
> @@ -191,11 +191,13 @@ static int wl1251_tx_send_packet(struct wl1251 *wl, struct sk_buff *skb,
> if (control->control.hw_key &&
> control->control.hw_key->alg == ALG_TKIP) {
> int hdrlen;
> - u16 fc;
> + __le16 fc;
> + u16 length;
> u8 *pos;
>
> - fc = *(u16 *)(skb->data + sizeof(*tx_hdr));
> - tx_hdr->length += WL1251_TKIP_IV_SPACE;
> + fc = *(__le16 *)(skb->data + sizeof(*tx_hdr));
Is this going to work? sizeof(*tx_hdr), and the operation, will be in
the cpu's endianess, right? Wouldn't the following be the right thing to
do then?
fc = cpu_to_le16(le16_to_cpu(skb->data) + sizeof(*tx_hdr));
Maybe some casts are needed too, I didn't check that, but regarding the
endianess, I think this is how it should go. It's the same thing as the
length parameter:
> + length = le16_to_cpu(tx_hdr->length) + WL1251_TKIP_IV_SPACE;
> + tx_hdr->length = cpu_to_le16(length);
...which is treated correctly here.
--
Cheers,
Luca.
On Thu, 2010-07-22 at 09:45 +0200, ext Kalle Valo wrote:
> On 07/22/2010 08:34 AM, Luciano Coelho wrote:
> >> @@ -191,11 +191,13 @@ static int wl1251_tx_send_packet(struct wl1251 *wl, struct sk_buff *skb,
> >> if (control->control.hw_key &&
> >> control->control.hw_key->alg == ALG_TKIP) {
> >> int hdrlen;
> >> - u16 fc;
> >> + __le16 fc;
> >> + u16 length;
> >> u8 *pos;
> >>
> >> - fc = *(u16 *)(skb->data + sizeof(*tx_hdr));
> >> - tx_hdr->length += WL1251_TKIP_IV_SPACE;
> >> + fc = *(__le16 *)(skb->data + sizeof(*tx_hdr));
> >
> > Is this going to work? sizeof(*tx_hdr), and the operation, will be in
> > the cpu's endianess, right?
>
> I think this is correct. We first calculate a pointer to a __le16 value
> and then dereference that value to a __le16 variable.
Yes, you're right, I got totally confused for some reason, as I already
pointed out on my other email. I completely missed the fact that we
were dealing with pointers here. It seems like my brain works as that
of a Java programmer before my morning dose of caffeine :P
--
Cheers,
Luca.
On 07/22/2010 03:21 PM, John W. Linville wrote:
> On Thu, Jul 22, 2010 at 09:45:10AM +0200, Kalle Valo wrote:
>> On 07/22/2010 08:34 AM, Luciano Coelho wrote:
>
>>>> @@ -467,7 +467,7 @@ static int wl1251_boot_upload_nvs(struct wl1251 *wl)
>>>> val = (nvs_ptr[0] | (nvs_ptr[1] << 8)
>>>> | (nvs_ptr[2] << 16) | (nvs_ptr[3] << 24));
>>>>
>>>> - val = cpu_to_le32(val);
>>>> + val = (u32 __force) cpu_to_le32(val);
>>>
>>> This will work, but such casts always make me a bit suspicious. I think
>>> this is fine for now
>>
>> This line was very suspicious already from beginning, I can't remember
>> why it was added and I don't see why it's needed here.
>
> It certainly is a bit strange, and rather ugly as well. I agree that
> the write should probably just take the le32 instead, but I was more
> interested in silencing sparse than in rewriting a driver for which
> I have not hardware. :-)
>
> I could drop that hunk for the time being?
Yeah, drop that hunk for now. Better to create a separate patch which
removes that val = cpu_to_le32(val) line altogether.
Kalle
On Thu, Jul 22, 2010 at 08:04:02AM -0400, Bob Copeland wrote:
> On Thu, Jul 22, 2010 at 3:45 AM, Kalle Valo <[email protected]> wrote:
> >>> @@ -191,11 +191,13 @@ static int wl1251_tx_send_packet(struct wl1251 *wl, struct sk_buff *skb,
> >>> ? ? ?if (control->control.hw_key &&
> >>> ? ? ? ? ?control->control.hw_key->alg == ALG_TKIP) {
> >>> ? ? ? ? ? ? ?int hdrlen;
> >>> - ? ? ? ? ? ?u16 fc;
> >>> + ? ? ? ? ? ?__le16 fc;
> >>> + ? ? ? ? ? ?u16 length;
> >>> ? ? ? ? ? ? ?u8 *pos;
> [...]
> >>> + ? ? ? ? ? ?length = le16_to_cpu(tx_hdr->length) + WL1251_TKIP_IV_SPACE;
> >>> + ? ? ? ? ? ?tx_hdr->length = cpu_to_le16(length);
> >>
> >> ...which is treated correctly here.
> >
> > This is different. Here we are adding something to a __le16 value, not
> > calculating with pointers.
>
> Just throwing in my 2 cents, unless there's some other reason to add the
> length temporary, here you can just do:
>
> le16_add_cpu(&tx_hdr->length, WL1251_TKIP_IV_SPACE);
Ah, thanks! I thought there was something like that, but I couldn't remember. :-)
John
--
John W. Linville Someday the world will need a hero, and you
[email protected] might be all we have. Be ready.
On Thu, 2010-07-22 at 08:34 +0200, Coelho Luciano (Nokia-MS/Helsinki)
wrote:
> > @@ -191,11 +191,13 @@ static int wl1251_tx_send_packet(struct wl1251 *wl, struct sk_buff *skb,
> > if (control->control.hw_key &&
> > control->control.hw_key->alg == ALG_TKIP) {
> > int hdrlen;
> > - u16 fc;
> > + __le16 fc;
> > + u16 length;
> > u8 *pos;
> >
> > - fc = *(u16 *)(skb->data + sizeof(*tx_hdr));
> > - tx_hdr->length += WL1251_TKIP_IV_SPACE;
> > + fc = *(__le16 *)(skb->data + sizeof(*tx_hdr));
>
> Is this going to work? sizeof(*tx_hdr), and the operation, will be in
> the cpu's endianess, right? Wouldn't the following be the right thing to
> do then?
>
> fc = cpu_to_le16(le16_to_cpu(skb->data) + sizeof(*tx_hdr));
Ugh, as Johannes pointed out, what I said here is completely non-sense.
Please ignore this before-my-morning-coffee lapse. ;)
--
Cheers,
Luca.
Hi John,
On 07/21/2010 06:31 PM, John W. Linville wrote:
> @@ -225,7 +225,7 @@ static void wl1251_boot_set_ecpu_ctrl(struct wl1251 *wl, u32 flag)
> int wl1251_boot_run_firmware(struct wl1251 *wl)
> {
> int loop, ret;
> - u32 chip_id, interrupt;
> + u32 chip_id, acx_intr;
I don't get why you did this change. I don't object renaming the
variable, but I can't figure out the reasoning behind it.
Kalle
On Thu, 2010-07-22 at 10:45 +0200, ext Kalle Valo wrote:
> Hi John,
>
> On 07/21/2010 06:31 PM, John W. Linville wrote:
> > @@ -225,7 +225,7 @@ static void wl1251_boot_set_ecpu_ctrl(struct wl1251 *wl, u32 flag)
> > int wl1251_boot_run_firmware(struct wl1251 *wl)
> > {
> > int loop, ret;
> > - u32 chip_id, interrupt;
> > + u32 chip_id, acx_intr;
>
> I don't get why you did this change. I don't object renaming the
> variable, but I can't figure out the reasoning behind it.
Sparse complains that interrupt is shadowing the definition in hw_irq.h:
> drivers/net/wireless/wl12xx/wl1251_boot.c:228:22: warning: symbol
> 'interrupt' shadows an earlier one
> /home/linville/git/wireless-next-2.6/arch/x86/include/asm/hw_irq.h:132:13: originally declared here
So, it's just a cosmetic change, I guess.
--
Cheers,
Luca.
On 07/22/2010 10:52 AM, Luciano Coelho wrote:
> On Thu, 2010-07-22 at 10:45 +0200, ext Kalle Valo wrote:
>> Hi John,
>>
>> On 07/21/2010 06:31 PM, John W. Linville wrote:
>>> @@ -225,7 +225,7 @@ static void wl1251_boot_set_ecpu_ctrl(struct wl1251 *wl, u32 flag)
>>> int wl1251_boot_run_firmware(struct wl1251 *wl)
>>> {
>>> int loop, ret;
>>> - u32 chip_id, interrupt;
>>> + u32 chip_id, acx_intr;
>>
>> I don't get why you did this change. I don't object renaming the
>> variable, but I can't figure out the reasoning behind it.
>
> Sparse complains that interrupt is shadowing the definition in hw_irq.h:
>
>> drivers/net/wireless/wl12xx/wl1251_boot.c:228:22: warning: symbol
>> 'interrupt' shadows an earlier one
>> /home/linville/git/wireless-next-2.6/arch/x86/include/asm/hw_irq.h:132:13: originally declared here
>
> So, it's just a cosmetic change, I guess.
Ah, I was blind as usual. It makes all sense now, thanks.
Kalle
On 07/22/2010 08:34 AM, Luciano Coelho wrote:
> Hi John,
Hi Luca and John,
> Great that someone finally made the endianess changes that we never had
> the time to do for wl1251. Thanks for that! :)
Yeah, it really is. wl1251 endian support has been broken almost from
day one. Unfortunately I don't any hardware to test wl1251 right now, so
we have to rely on careful review :/
>> @@ -467,7 +467,7 @@ static int wl1251_boot_upload_nvs(struct wl1251 *wl)
>> val = (nvs_ptr[0] | (nvs_ptr[1] << 8)
>> | (nvs_ptr[2] << 16) | (nvs_ptr[3] << 24));
>>
>> - val = cpu_to_le32(val);
>> + val = (u32 __force) cpu_to_le32(val);
>
> This will work, but such casts always make me a bit suspicious. I think
> this is fine for now
This line was very suspicious already from beginning, I can't remember
why it was added and I don't see why it's needed here.
> but later I think we should make sure that all the
> _write() functions explicitly receive __le32 as val, or receives the
> cpu's u32 and converts it before actually writing the value, for
> clarity reasons. Kalle, what do you think?
I agree that we should change write() to handle endianess properly. I'm
in favor of having the conversion in write(), but I didn't think this
that much.
>> @@ -191,11 +191,13 @@ static int wl1251_tx_send_packet(struct wl1251 *wl, struct sk_buff *skb,
>> if (control->control.hw_key &&
>> control->control.hw_key->alg == ALG_TKIP) {
>> int hdrlen;
>> - u16 fc;
>> + __le16 fc;
>> + u16 length;
>> u8 *pos;
>>
>> - fc = *(u16 *)(skb->data + sizeof(*tx_hdr));
>> - tx_hdr->length += WL1251_TKIP_IV_SPACE;
>> + fc = *(__le16 *)(skb->data + sizeof(*tx_hdr));
>
> Is this going to work? sizeof(*tx_hdr), and the operation, will be in
> the cpu's endianess, right?
I think this is correct. We first calculate a pointer to a __le16 value
and then dereference that value to a __le16 variable.
> Wouldn't the following be the right thing to
> do then?
>
> fc = cpu_to_le16(le16_to_cpu(skb->data) + sizeof(*tx_hdr));
>
> Maybe some casts are needed too, I didn't check that, but regarding the
> endianess, I think this is how it should go. It's the same thing as the
> length parameter:
>
>> + length = le16_to_cpu(tx_hdr->length) + WL1251_TKIP_IV_SPACE;
>> + tx_hdr->length = cpu_to_le16(length);
>
> ...which is treated correctly here.
This is different. Here we are adding something to a __le16 value, not
calculating with pointers.
Kalle