p54: Fix for big-endian architecture

RFC/RFT New Larry Finger 4 participants 20 messages Oct 9, 2008 – Oct 14, 2008 Networking drivers (wireless)

Share & tools

Larry Finger

author

2008-10-09 15:19 UTC #0 lore raw

This patch is meant for testing on big-endian hardware. Every indication is that the data in the bootrec structure is little endian. This patch fixes a usage of the u32 data array as a string of u8's.

Signed-off-by Larry Finger <[email protected]>

---

drivers/net/wireless/p54/p54common.h +4 -1

Index: wireless-testing/drivers/net/wireless/p54/p54common.h
===================================================================
@@ -18,7 +18,10 @@
 struct bootrec {
 	__le32 code;
 	__le32 len;
-	u32 data[10];
+	union {
+		u32 data[10];
+		u8 data_char[40];
+	} __attribute__((packed));
 	__le16 rx_mtu;
 } __attribute__((packed));

drivers/net/wireless/p54/p54common.c +2 -2

Index: wireless-testing/drivers/net/wireless/p54/p54common.c
===================================================================
@@ -171,8 +171,8 @@ int p54_parse_firmware(struct ieee80211_
 			break;
 		case BR_CODE_COMPONENT_VERSION:
 			/* 24 bytes should be enough for all firmwares */
-			if (strnlen((unsigned char*)bootrec->data, 24) < 24)
-				fw_version = (unsigned char*)bootrec->data;
+			if (strnlen(bootrec->data_char, 24) < 24)
+				fw_version = bootrec->data_char;
 			break;
 		case BR_CODE_DESCR: {
 			struct bootrec_desc *desc =

Christian Lamparter

2008-10-09 16:54 UTC #1 lore raw

Quoted 13 lines · On Thursday 09 October 2008 17:19:23 Larry Finger wrote:

This patch is meant for testing on big-endian hardware. Every indication is that the data in the bootrec structure is little endian. This patch fixes a usage of the u32 data array as a string of u8's.
Signed-off-by Larry Finger <[email protected]>
---
Index: wireless-testing/drivers/net/wireless/p54/p54common.h
===================================================================
drivers/net/wireless/p54/p54common.h +0 -0
@@ -18,7 +18,10 @@
 struct bootrec {

drivers/net/wireless/p54/p54common.h line 27

Quoted 10 lines

drivers/net/wireless/p54/p54common.h +4 -1

 	__le32 code;
 	__le32 len;
-	u32 data[10];
+	union {
+		u32 data[10];
+		u8 data_char[40];
+	} __attribute__((packed));
 	__le16 rx_mtu;
 } __attribute__((packed));

? how did rx_mtu end up there? it should be a part of bootrec_desc (which is a few lines down in p54common.h)

struct bootrec_desc {
        __le16 modes;
        __le16 flags;
        __le32 rx_start;
        __le32 rx_end;
        u8 headroom;
        u8 tailroom;
        u8 unimportant[6];
        u8 rates[16];

+ __le16 rx_mtu;
} __attribute__((packed));

This structure is equivalent to struct s_lm_descr from "lmac_longbow.h", which can be found on wireless.kernel.org's resource page.

Regards,
Chr

Larry Finger

author

2008-10-10 00:39 UTC #2 lore raw

Quoted 31 lines · Christian Lamparter

Quoted 6 lines · On Thursday 09 October 2008 17:19:23 Larry Finger wrote:
Index: wireless-testing/drivers/net/wireless/p54/p54common.h
===================================================================
drivers/net/wireless/p54/p54common.h +0 -0
@@ -18,7 +18,10 @@
 struct bootrec {
drivers/net/wireless/p54/p54common.h line 27
Quoted 10 lines
drivers/net/wireless/p54/p54common.h +4 -1
 	__le32 code;
 	__le32 len;
-	u32 data[10];
+	union {
+		u32 data[10];
+		u8 data_char[40];
+	} __attribute__((packed));
 	__le16 rx_mtu;
 } __attribute__((packed));
 
? how did rx_mtu end up there? it should be a part of bootrec_desc (which is a few lines down in p54common.h)
struct bootrec_desc {
        __le16 modes;
        __le16 flags;
        __le32 rx_start;
        __le32 rx_end;
        u8 headroom;
        u8 tailroom;
        u8 unimportant[6];
        u8 rates[16];
(quoted diff) +1 -0
+	__le16 rx_mtu;
} __attribute__((packed));

Quoted 2 lines

This structure is equivalent to struct s_lm_descr from "lmac_longbow.h", which can be found on wireless.kernel.org's resource page.

As the original code got the data from bootrec, I kept it the same. On reflection, the statement above could be

priv->rx_mtu = le16_to_cpu(desc->rx_mtu);

I think which structure it is in is purely a matter of taste, but I will make that change, test it, and submit a new patch. The big-endian fix above will also need to be changed. I considered converting to a single struct using unions, but that would get pretty messy.

Larry

Christian Lamparter

2008-10-10 01:27 UTC #3 lore raw

Quoted 5 lines · On Friday 10 October 2008 02:39:14 Larry Finger wrote:

Christian Lamparter wrote:
As the original code got the data from bootrec, I kept it the same. On reflection, the statement above could be

Quoted 8 lines

priv->rx_mtu = le16_to_cpu(desc->rx_mtu);
I think which structure it is in is purely a matter of taste, but I will make that change, test it, and submit a new patch. The big-endian fix above will also need to be changed. I considered converting to a single struct using unions, but that would get pretty messy.
Larry

I finally found the definitions by Intersil (again):

Quoted 2 lines

http://gxaafoot.homelinux.org/cgi-bin/archzoom.cgi/[email protected]/prism54-usb--devo--0.0--patch-182/islsm_bra.h?diff=2
tinyurl link: http://tinyurl.com/3wo45y

Unfortunately, back in 2002 noone though about big/little endian and 64bit :-D

/*
* Each boot record is identified by a unique code, followed
* by the length (in 32 bit words) of the data in the record. */

struct s_bootrec_hdr {
	unsigned long code;
	unsigned long length;	
};

/* Generic bootrecord structure that can be mapped to each bootrecord */

struct s_bootrecord {
	struct s_bootrec_hdr hdr;
	char start_of_data;
};

[...]

so, what about changing bootrec to:

struct bootrec {
        __le32 code;
        __le32 len;
	char data[0];
} __attribute__((packed));

And put this endless discussion to rest

Regards,
Chr

Pavel Roskin

2008-10-13 21:44 UTC #4 lore raw

In reply to Larry Finger

Quoted 3 lines · Larry Finger

This patch is meant for testing on big-endian hardware. Every indication is that the data in the bootrec structure is little endian. This patch fixes a usage of the u32 data array as a string of u8's.

I'm sorry, I didn't test your patch yet, but I could capture the output on the serial console. It doesn't appear to be related to p54 specific data structures. The driver is initialized correctly. I can bring the interface up. But once I try scanning, I get a sequence of the following messages in the kernel log:

Badness at /home/proski/src/linux-2.6/net/mac80211/rx.c:2200
NIP: c02bc850 LR: c02ab268 CTR: 00000000
REGS: ef01fcc0 TRAP: 0700 Tainted: G W (2.6.27-wl)
MSR: 00029032 <EE,ME,IR,DR> CR: 22004084 XER: 20000000
TASK = c1a58800[1778] 'p54pci' THREAD: ef01e000
GPR00: 00000008 ef01fd70 c1a58800 c1a51180 ef082cc0 ef01fdb8 ef01fddc 0000000c
GPR08: 00000080 c1a51044 f103a6d0 0000000c 42004082 00000000 01729138 000000db
GPR16: 0172920c 41400000 0173dba4 00241678 c03b237c ef84dfb0 ef84dfac 00000000
GPR24: c041dde0 c0420a8c c1a51254 00000000 c1a51180 ef01fdb8 c1a51180 ef082cc0 NIP [c02bc850] __ieee80211_rx+0x17c/0x638
LR [c02ab268] ieee80211_tasklet_handler+0x104/0x120

Call Trace:

[ef01fd70] [c1a0c020] 0xc1a0c020 (unreliable) [ef01fdb0] [c02ab268] ieee80211_tasklet_handler+0x104/0x120 [ef01fe00] [c002e318] tasklet_action+0x80/0xfc [ef01fe20] [c002ebf8] __do_softirq+0x8c/0xfc [ef01fe50] [c00062ac] do_softirq+0x58/0x5c [ef01fe60] [c002eaf8] irq_exit+0x84/0x88 [ef01fe70] [c0006320] do_IRQ+0x70/0xc8
[ef01fe80] [c00106fc] ret_from_except+0x0/0x14
--- Exception: 501 at ieee80211_hw_config+0xac/0xbc

    LR = ieee80211_hw_config+0xa0/0xbc

[ef01ff50] [c02b04f0] ieee80211_scan_work+0x144/0x1b8 [ef01ff60] [c003b26c] run_workqueue+0xc4/0x160
[ef01ff90] [c003b850] worker_thread+0x54/0xb8
[ef01ffd0] [c003f788] kthread+0x50/0x88
[ef01fff0] [c000fe88] kernel_thread+0x44/0x60
Instruction dump:
6c09ffff 2f898800 419e0244 80010044 7f83e378 7f04c378 7fa5eb78 7ea6ab78 baa10014 7c0803a6 38210040 4bfff4d4 <0fe00000> 80010044 baa10014 38210040

That's the current wireless-testing.

If the patch makes any difference, I'll report it separately.

Signature · 2 lines

Regards,
Pavel Roskin

Johannes Berg

maintainer

2008-10-13 22:25 UTC #5 lore raw

Quoted 5 lines · Pavel Roskin

Quoted 3 lines · Larry Finger
This patch is meant for testing on big-endian hardware. Every indication is that the data in the bootrec structure is little endian. This patch fixes a usage of the u32 data array as a string of u8's.

Quoted 7 lines

I'm sorry, I didn't test your patch yet, but I could capture the output on the serial console. It doesn't appear to be related to p54 specific data structures. The driver is initialized correctly. I can bring the interface up. But once I try scanning, I get a sequence of the following messages in the kernel log:
Badness at /home/proski/src/linux-2.6/net/mac80211/rx.c:2200

That _is_ the driver's fault, it's not setting the rate index correctly.

johannes

Pavel Roskin

2008-10-13 22:36 UTC #6 lore raw

Quoted 12 lines · Johannes Berg

Quoted 11 lines · Pavel Roskin
Quoted 3 lines · Larry Finger
This patch is meant for testing on big-endian hardware. Every indication is that the data in the bootrec structure is little endian. This patch fixes a usage of the u32 data array as a string of u8's.
I'm sorry, I didn't test your patch yet, but I could capture the output on the serial console. It doesn't appear to be related to p54 specific data structures. The driver is initialized correctly. I can bring the interface up. But once I try scanning, I get a sequence of the following messages in the kernel log:

Quoted 3 lines

Quoted 1 line
Badness at /home/proski/src/linux-2.6/net/mac80211/rx.c:2200
That _is_ the driver's fault, it's not setting the rate index correctly.

I put a printk() there, and it shows:
status->rate_idx = 12, sband->n_bitrates = 8

status->rate_idx should be less or equal sband->n_bitrates.

I don't get that on i386. But the values don't seem to be corrupted by byte-swapping.

Signature · 2 lines

Regards,
Pavel Roskin

Christian Lamparter

2008-10-13 22:51 UTC #7 lore raw

Quoted 17 lines · On Tuesday 14 October 2008 00:36:32 Pavel Roskin wrote:

Quoted 12 lines · Johannes Berg
Quoted 11 lines · Pavel Roskin
Quoted 3 lines · Larry Finger
This patch is meant for testing on big-endian hardware. Every indication is that the data in the bootrec structure is little endian. This patch fixes a usage of the u32 data array as a string of u8's.
I'm sorry, I didn't test your patch yet, but I could capture the output on the serial console. It doesn't appear to be related to p54 specific data structures. The driver is initialized correctly. I can bring the interface up. But once I try scanning, I get a sequence of the following messages in the kernel log:
Quoted 3 lines
Quoted 1 line
Badness at /home/proski/src/linux-2.6/net/mac80211/rx.c:2200
That _is_ the driver's fault, it's not setting the rate index correctly.

Quoted 8 lines

I put a printk() there, and it shows:
status->rate_idx = 12, sband->n_bitrates = 8
status->rate_idx should be less or equal sband->n_bitrates.
I don't get that on i386. But the values don't seem to be corrupted by byte-swapping.

Hmm, this should be impossible since:
"p54: report appropriate rate and band values for 802.11a" 5f840304b5f7dff0028407fa9b284aecb85a94aa

regards,
Chr

Pavel Roskin

2008-10-13 22:55 UTC #8 lore raw

Quoted 7 lines · Christian Lamparter

Quoted 1 line
I don't get that on i386. But the values don't seem to be corrupted
by
Quoted 2 lines
byte-swapping.
Hmm, this should be impossible since:
"p54: report appropriate rate and band values for 802.11a" 5f840304b5f7dff0028407fa9b284aecb85a94aa

I see. It's recent code, so maybe it wasn't broken yet when I was testing it on i386.

This code in drivers/net/wireless/p54/p54common.c sets rate_idx:

rx_status.rate_idx = (dev->conf.channel->band == IEEE80211_BAND_2GHZ ? hdr->rate : (hdr->rate - 4)) & 0xf;

printk() shows:

dev->conf.channel->band = 1, hdr->rate = 0, rx_status.rate_idx = 12

Signature · 2 lines

Regards,
Pavel Roskin

Pavel Roskin

2008-10-13 23:05 UTC #9 lore raw

Quoted 12 lines · Pavel Roskin

Quoted 7 lines · Christian Lamparter
Quoted 1 line
I don't get that on i386. But the values don't seem to be corrupted
by
Quoted 2 lines
byte-swapping.
Hmm, this should be impossible since:
"p54: report appropriate rate and band values for 802.11a" 5f840304b5f7dff0028407fa9b284aecb85a94aa
I see. It's recent code, so maybe it wasn't broken yet when I was testing it on i386.

Quoted 8 lines

This code in drivers/net/wireless/p54/p54common.c sets rate_idx:
rx_status.rate_idx = (dev->conf.channel->band == IEEE80211_BAND_2GHZ ? hdr->rate : (hdr->rate - 4)) & 0xf;
printk() shows:
dev->conf.channel->band = 1, hdr->rate = 0, rx_status.rate_idx = 12

And if I set rx_status.rate_idx to 0, I still get that badness for the same reason (status->rate_idx = 12, sband->n_bitrates = 8). Also, there is another badness reported sometimes:

Badness at /home/proski/src/linux-2.6/net/mac80211/main.c:232
NIP: c02aa6a8 LR: c02aa6a0 CTR: c02cb578
REGS: eeacfe90 TRAP: 0700 Tainted: G W (2.6.27-wl)
MSR: 00029032 <EE,ME,IR,DR> CR: 24004024 XER: 20000000
TASK = ef081800[6660] 'p54pci' THREAD: eeace000
GPR00: 00000001 eeacff40 ef081800 ffffffea c1bd1a20 00000056 00000031 ef10a000
GPR08: 00000031 ffffffe9 00000056 c02cb578 00000011 00000000 01729138 000000db
GPR16: 0172920c 41400000 0173dba4 00241678 c03b237c ef84dfb0 ef84dfac 00000001
GPR24: c032cfa0 c0359d90 c032cfd8 00000001 00000000 eeace000 f103a6a8 c1a19180 NIP [c02aa6a8] ieee80211_hw_config+0xa8/0xbc
LR [c02aa6a0] ieee80211_hw_config+0xa0/0xbc

Call Trace:
[eeacff40] [c02aa6a0] ieee80211_hw_config+0xa0/0xbc (unreliable)
[eeacff50] [c02b04f0] ieee80211_scan_work+0x144/0x1b8
[eeacff60] [c003b26c] run_workqueue+0xc4/0x160
[eeacff90] [c003b850] worker_thread+0x54/0xb8
[eeacffd0] [c003f788] kthread+0x50/0x88
[eeacfff0] [c000fe88] kernel_thread+0x44/0x60

Instruction dump:
80090014 7f805800 41bdffd0 4bffffc0 812306e8 4bffff90 81230060 81290014 7d2903a6 4e800421 3123ffff 7c091910 <0f000000> 80010014 38210010 7c0803a6

The code is:

        if (changed && local->open_count) {
                ret = local->ops->config(local_to_hw(local), changed);
                /*
                 * HW reconfiguration should never fail, the driver has told
                 * us what it can support so it should live up to that promise.
                 */
                WARN_ON(ret);
        }

Signature · 2 lines

Regards,
Pavel Roskin

Johannes Berg

maintainer

2008-10-13 23:08 UTC #10 lore raw

Quoted 2 lines · Pavel Roskin

And if I set rx_status.rate_idx to 0, I still get that badness for the same reason (status->rate_idx = 12, sband->n_bitrates = 8).

??
That's not right, if you set rx_status.rate_idx to 0 status->rate_idx should be 0 :)

Quoted 2 lines

Also, there
is another badness reported sometimes:

That's new. On a whim I decided that I wanted to know why config() had a return value ;) Will see why p54 makes it fail, maybe there's a mac80211 bug.

johannes

Pavel Roskin

2008-10-13 23:36 UTC #11 lore raw

Quoted 8 lines · Johannes Berg

Quoted 2 lines · Pavel Roskin
And if I set rx_status.rate_idx to 0, I still get that badness for the same reason (status->rate_idx = 12, sband->n_bitrates = 8).
??
That's not right, if you set rx_status.rate_idx to 0 status->rate_idx should be 0 :)

Sorry, I didn't code it right. You are correct.

Quoted 6 lines

Quoted 2 lines
Also, there
is another badness reported sometimes:
That's new. On a whim I decided that I wanted to know why config() had a return value ;) Will see why p54 makes it fail, maybe there's a mac80211 bug.

p54_config() calls p54_set_freq(), which can fail with -EINVAL. I'm looking why it can happen.

Signature · 2 lines

Regards,
Pavel Roskin

Pavel Roskin

2008-10-14 00:00 UTC #12 lore raw

Quoted 2 lines · Pavel Roskin

p54_config() calls p54_set_freq(), which can fail with -EINVAL. I'm looking why it can happen.

The calibration table only has frequencies in the 2.4GHz band, but the scan goes through the 5GHz band as well. That's how it fails:

freq = 5825
i = 0, priv->iq_autocal[i].freq = 2412 i = 1, priv->iq_autocal[i].freq = 2417 i = 2, priv->iq_autocal[i].freq = 2422 i = 3, priv->iq_autocal[i].freq = 2427 i = 4, priv->iq_autocal[i].freq = 2432 i = 5, priv->iq_autocal[i].freq = 2437 i = 6, priv->iq_autocal[i].freq = 2442 i = 7, priv->iq_autocal[i].freq = 2447 i = 8, priv->iq_autocal[i].freq = 2452 i = 9, priv->iq_autocal[i].freq = 2457 i = 10, priv->iq_autocal[i].freq = 2462 i = 11, priv->iq_autocal[i].freq = 2467 i = 12, priv->iq_autocal[i].freq = 2472 i = 13, priv->iq_autocal[i].freq = 2484
phy9: frequency change failed
ret = -22

Either we should be using a different table for the 5GHz band, or the driver should not announce 5GHz channels if the calibration table lacks them.

Signature · 2 lines

Regards,
Pavel Roskin

Christian Lamparter

2008-10-14 00:11 UTC #13 lore raw

Quoted 26 lines · On Tuesday 14 October 2008 02:00:39 Pavel Roskin wrote:

Quoted 2 lines · Pavel Roskin
p54_config() calls p54_set_freq(), which can fail with -EINVAL. I'm looking why it can happen.
The calibration table only has frequencies in the 2.4GHz band, but the scan goes through the 5GHz band as well. That's how it fails:
freq = 5825
i = 0, priv->iq_autocal[i].freq = 2412 i = 1, priv->iq_autocal[i].freq = 2417 i = 2, priv->iq_autocal[i].freq = 2422 i = 3, priv->iq_autocal[i].freq = 2427 i = 4, priv->iq_autocal[i].freq = 2432 i = 5, priv->iq_autocal[i].freq = 2437 i = 6, priv->iq_autocal[i].freq = 2442 i = 7, priv->iq_autocal[i].freq = 2447 i = 8, priv->iq_autocal[i].freq = 2452 i = 9, priv->iq_autocal[i].freq = 2457 i = 10, priv->iq_autocal[i].freq = 2462 i = 11, priv->iq_autocal[i].freq = 2467 i = 12, priv->iq_autocal[i].freq = 2472 i = 13, priv->iq_autocal[i].freq = 2484
phy9: frequency change failed
ret = -22

Quoted 3 lines

Either we should be using a different table for the 5GHz band, or the driver should not announce 5GHz channels if the calibration table lacks them.

I ask again:
What Card/MAC & RF-Chip do you have? Indigo/Duette/Firsbee or XBow?

see switch instruction in p54common.c line 430 in p54_parse_eeprom

Regards,
Chr

Pavel Roskin

2008-10-14 01:20 UTC #14 lore raw

Quoting Christian Lamparter <[email protected]>:

Quoted 2 lines

I ask again:
What Card/MAC & RF-Chip do you have? Indigo/Duette/Firsbee or XBow?

Sorry if I missed your question.

This is a MiniPCI card that has a label with "XG-600" printed on it. The driver shows following in the kernel log:

firmware: requesting isl3886
p54: LM86 firmware
p54: FW rev 2.7.0.0 - Softmac protocol 4.1
p54: unknown eeprom code : 0x1
p54: unknown eeprom code : 0x3
p54: unknown eeprom code : 0x1007
p54: unknown eeprom code : 0x1008
p54: unknown eeprom code : 0x1100
p54: unknown eeprom code : 0x1905
phy10: hwaddr 00:60:b3:c9:04:f2, MAC:isl3890 RF:Indigo?
phy10: Selected rate control algorithm 'pid'
udev: renamed network interface wlan1 to wlan2
firmware: requesting isl3886

I understand the driver detects it as Indigo.

# lspci -v -s 01:04.0
01:04.0 Network controller: Intersil Corporation ISL3890 [Prism GT/Prism Duette]/ISL3886 [Prism Javelin/Prism Xbow] (rev 01)
        Subsystem: Z-Com, Inc. XG-600 and clones Wireless Adapter
        Flags: bus master, medium devsel, latency 16, IRQ 25
        Memory at 80882000 (32-bit, non-prefetchable) [size=8K]
        Capabilities: [dc] Power Management version 1
        Kernel driver in use: p54pci
        Kernel modules: prism54, p54pci

Signature · 2 lines

Regards,
Pavel Roskin

Christian Lamparter

2008-10-14 02:06 UTC #15 lore raw

Quoted 6 lines · On Tuesday 14 October 2008 03:20:39 Pavel Roskin wrote:

Quoting Christian Lamparter <[email protected]>:
Quoted 2 lines
I ask again:
What Card/MAC & RF-Chip do you have? Indigo/Duette/Firsbee or XBow?
Sorry if I missed your question.

Quoted 20 lines

This is a MiniPCI card that has a label with "XG-600" printed on it. The driver shows following in the kernel log:
firmware: requesting isl3886
p54: LM86 firmware
p54: FW rev 2.7.0.0 - Softmac protocol 4.1
p54: unknown eeprom code : 0x1
p54: unknown eeprom code : 0x3
p54: unknown eeprom code : 0x1007
p54: unknown eeprom code : 0x1008
p54: unknown eeprom code : 0x1100
p54: unknown eeprom code : 0x1905
phy10: hwaddr 00:60:b3:c9:04:f2, MAC:isl3890 RF:Indigo?
phy10: Selected rate control algorithm 'pid'
udev: renamed network interface wlan1 to wlan2
firmware: requesting isl3886
I understand the driver detects it as Indigo.

Quoted 10 lines

# lspci -v -s 01:04.0
01:04.0 Network controller: Intersil Corporation ISL3890 [Prism  
GT/Prism Duette]/ISL3886 [Prism Javelin/Prism Xbow] (rev 01)
         Subsystem: Z-Com, Inc. XG-600 and clones Wireless Adapter
         Flags: bus master, medium devsel, latency 16, IRQ 25
         Memory at 80882000 (32-bit, non-prefetchable) [size=8K]
         Capabilities: [dc] Power Management version 1
         Kernel driver in use: p54pci
         Kernel modules: prism54, p54pci

Alright, your eeprom says that your card is technically 802.11a capable. It has the right MAC (ISL3890 Duette) and a 5GHz Phy/Synth/RF (whatever it's called).

but unfortunatly, it doesn't provide any calibration data...
And that's the problem here. Since without it, p54_set_freq can't put the card into 802.11a and the card stays at the last selected channel (somewhere in the 2.4GHz spectrum!). However after p54_config returns with -EINVAL, mac80211 doesn't revert dev->conf.channel back to the original 2.4GHz setting...

with your card still listening on 802.11bg channels, it could capture a beacon, or something else that is transmitted with 1MBit (hdr->rate = 0) and crash.
note: hdr->rate is a u8. so: 0 - 4 = 0xfc and (0xfc & 0xf) = 0xc => 12

so, I guess we have to do two things...
1. check if calibration data includes 5GHz channels AND if it has the Phy/Synth/RF.
2. cache dev->conf.channel locally in the driver.

Regards,
Chr

Christian Lamparter

2008-10-14 02:37 UTC #16 lore raw

PATCH 1/1 p54: enable 2.4/5GHz spectrum by eeprom bits.

Rather than setting the available bands by looking only at the phy-chip, we now use bits in the eeprom, which indicates the real capabilities.

Reported-by Pavel Roskin <[email protected]>

Signed-off-by Christian Lamparter

---

drivers/net/wireless/p54/p54common.c +10 ~~-18~~

index 0180466..48f91f3 100644
@@ -306,8 +306,8 @@ static int p54_convert_rev1(struct ieee80211_hw *dev,
 	return 0;
 }
 
-static const char *p54_rf_chips[] = { "NULL", "Indigo?", "Duette",
-                              "Frisbee", "Xbow", "Longbow" };
+static const char *p54_rf_chips[] = { "NULL", "Duette3", "Duette2",
+                              "Frisbee", "Xbow", "Longbow", "NULL", "NULL" };
 static int p54_init_xbow_synth(struct ieee80211_hw *dev);
 
 static int p54_parse_eeprom(struct ieee80211_hw *dev, void *eeprom, int len)

@@ -319,6 +319,7 @@ static int p54_parse_eeprom(struct ieee80211_hw *dev, void *eeprom, int len)
 	void *tmp;
 	int err;
 	u8 *end = (u8 *)eeprom + len;
+	u16 synth;
 	DECLARE_MAC_BUF(mac);
 
 	wrap = (struct eeprom_pda_wrap *) eeprom;

@@ -400,8 +401,8 @@ static int p54_parse_eeprom(struct ieee80211_hw *dev, void *eeprom, int len)
 			tmp = entry->data;
 			while ((u8 *)tmp < entry->data + data_len) {
 				struct bootrec_exp_if *exp_if = tmp;
-				if (le16_to_cpu(exp_if->if_id) == 0xF)
-					priv->rxhw = le16_to_cpu(exp_if->variant) & 0x07;
+				if (le16_to_cpu(exp_if->if_id) == 0xf)
+					synth = le16_to_cpu(exp_if->variant);
 				tmp += sizeof(struct bootrec_exp_if);
 			}
 			break;

@@ -427,22 +428,13 @@ static int p54_parse_eeprom(struct ieee80211_hw *dev, void *eeprom, int len)
 		goto err;
 	}
 
-	switch (priv->rxhw) {
-	case 4: /* XBow */
+	priv->rxhw = synth & 0x07;
+	if (priv->rxhw == 4)
 		p54_init_xbow_synth(dev);
-	case 1: /* Indigo? */
-	case 2: /* Duette */
-		dev->wiphy->bands[IEEE80211_BAND_5GHZ] = &band_5GHz;
-	case 3: /* Frisbee */
-	case 5: /* Longbow */
+	if (!(synth & 0x40))
 		dev->wiphy->bands[IEEE80211_BAND_2GHZ] = &band_2GHz;
-		break;
-	default:
-		printk(KERN_ERR "%s: unsupported RF-Chip\n",
-			wiphy_name(dev->wiphy));
-		err = -EINVAL;
-		goto err;
-	}
+	if (!(synth & 0x80))
+		dev->wiphy->bands[IEEE80211_BAND_5GHZ] = &band_5GHz;
 
 	if (!is_valid_ether_addr(dev->wiphy->perm_addr)) {
 		u8 perm_addr[ETH_ALEN];

Johannes Berg

maintainer

2008-10-14 08:16 UTC #17 lore raw

In reply to Christian Lamparter

Quoted 8 lines · Christian Lamparter

Alright, your eeprom says that your card is technically 802.11a capable. It has the right MAC (ISL3890 Duette) and a 5GHz Phy/Synth/RF (whatever it's called).
but unfortunatly, it doesn't provide any calibration data...
And that's the problem here. Since without it, p54_set_freq can't put the card into 802.11a and the card stays at the last selected channel (somewhere in the 2.4GHz spectrum!). However after p54_config returns with -EINVAL, mac80211 doesn't revert dev->conf.channel back to the original 2.4GHz setting...

Heh, so this actually helped catch a problem, nice.

Quoted 7 lines

with your card still listening on 802.11bg channels, it could capture a beacon, or something else that is transmitted with 1MBit (hdr->rate = 0) and crash.
note: hdr->rate is a u8. so: 0 - 4 = 0xfc and (0xfc & 0xf) = 0xc => 12
so, I guess we have to do two things...
1. check if calibration data includes 5GHz channels AND if it has the Phy/Synth/RF.
2. cache dev->conf.channel locally in the driver.

Just (1) as your patch did seems fine to me, then mac80211 will not attempt to go to the other channels.

johannes

Christian Lamparter

2008-10-13 23:27 UTC #18 lore raw

In reply to Pavel Roskin

Quoted 18 lines · On Tuesday 14 October 2008 01:05:51 Pavel Roskin wrote:

Quoted 17 lines · Pavel Roskin
Quoted 7 lines · Christian Lamparter
Quoted 1 line
I don't get that on i386. But the values don't seem to be corrupted
by
Quoted 2 lines
byte-swapping.
Hmm, this should be impossible since:
"p54: report appropriate rate and band values for 802.11a" 5f840304b5f7dff0028407fa9b284aecb85a94aa
I see. It's recent code, so maybe it wasn't broken yet when I was testing it on i386.
This code in drivers/net/wireless/p54/p54common.c sets rate_idx:
rx_status.rate_idx = (dev->conf.channel->band == IEEE80211_BAND_2GHZ ? hdr->rate : (hdr->rate - 4)) & 0xf;

Quoted 8 lines

Quoted 3 lines
printk() shows:
dev->conf.channel->band = 1, hdr->rate = 0, rx_status.rate_idx = 12
And if I set rx_status.rate_idx to 0, I still get that badness for the same reason (status->rate_idx = 12, sband->n_bitrates = 8). Also, there is another badness reported sometimes:

hmm, strange...
so dev->conf.channel->band = 1 (=> IEEE80211_BAND_2GHZ)
Ok, but why are there only 8 rates? It should be twelve for 2.4GHz Band! (What Card/MAC & RF-Chip have you got? Indigo/Duette/Firsbee or XBow?)

Regards,
Chr

Johannes Berg

maintainer

2008-10-13 23:37 UTC #19 lore raw

Quoted 2 lines · Christian Lamparter

hmm, strange...
so dev->conf.channel->band = 1 (=> IEEE80211_BAND_2GHZ)

Ummm, no, 0 is 2 GHz, 1 is 5 GHz.

johannes