Marvell Libertas 8388 802.11b/g USB driver (v2)

Announcing an updated patch of the Marvell Libertas 8388 802.11 USB driver.

Diff can be found at
http://dev.laptop.org/~marcelo/libertas-8388-16012007.patch

Quoted 5 lines · you

Announcing an updated patch of the Marvell Libertas 8388 802.11 USB driver.

Diff can be found at
http://dev.laptop.org/~marcelo/libertas-8388-16012007.patch

Is core chip somehow close to pci/pci-e:

00:07.0 Ethernet controller: Marvell Technology Group Ltd. 88W8310 and 88W8000G [Libertas] 802.11g client chipset (rev 07)

_Please_ review, this driver is targeted for mainline inclusion.

There have been almost no comments resulting from the first submission.

Diff can be found at
http://dev.laptop.org/~marcelo/libertas-8388-16012007.patch

_Please_ review, this driver is targeted for mainline inclusion.

Quoted 4 lines · Marcelo Tosatti

Diff can be found at
http://dev.laptop.org/~marcelo/libertas-8388-16012007.patch

_Please_ review, this driver is targeted for mainline inclusion.

As long as there is yet another 802.11 stack inside, your chances to get this included are small.

The part is a mostly fullmac part that until now has been targetted at the embedded market (PSP, N80 phone, etc). That means that the driver shouldn't be too large (airo is about 8000 kLOC without airo_cs) but for some reason it still is; we're working on that.

But could we please be constructive here, and actually take a look at the driver and point out problems?

(quoted diff) +23 -0

+/** Double-Word(32Bit) Bit definition */
+#define DW_BIT_0	0x00000001
+#define DW_BIT_1	0x00000002
+#define DW_BIT_2	0x00000004
+#define DW_BIT_3	0x00000008
+#define DW_BIT_4	0x00000010
+#define DW_BIT_5	0x00000020
+#define DW_BIT_6	0x00000040
+#define DW_BIT_7	0x00000080
+#define DW_BIT_8	0x00000100
+#define DW_BIT_9	0x00000200
+#define DW_BIT_10	0x00000400
+#define DW_BIT_11       0x00000800
+#define DW_BIT_12       0x00001000
+#define DW_BIT_13       0x00002000
+#define DW_BIT_14       0x00004000
+#define DW_BIT_15       0x00008000
+#define DW_BIT_16       0x00010000
+#define DW_BIT_17       0x00020000
+#define DW_BIT_18       0x00040000
+#define DW_BIT_19       0x00080000
+#define DW_BIT_20       0x00100000
+#define DW_BIT_21       0x00200000

Quoted 4 lines · Dan Williams

The part is a mostly fullmac part that until now has been targetted at the embedded market (PSP, N80 phone, etc). That means that the driver shouldn't be too large (airo is about 8000 kLOC without airo_cs) but for some reason it still is; we're working on that.

I disagree that this is a fullmac part. You need to tell it to authenticate and then you need to tell it to associate. IPW is a fullmac, you tell it "use SSID xxx". Not this, you tell it "auth to BSSID aaa" and then "associate to BSSID aaa" etc.

Besides the fact that I think this is a very dumb thing to do in new hardware due to IEEE 802.11w (protected management frames) being well underway which you can't handle at all that way, I also don't see how you can claim that this is fullmac when you need to tell it every step of the way.

The way I see it, you're telling the firmware to send an auth frame and then you wait for the firmware to come back and tell you "yes I have received a response" instead of just sending the frame yourself and then waiting for the response frame. So the difference is that you need to do a bit less frame parsing in the driver-stack.

I could drown you in technical comments on everything starting from the way commands are sent, via the fact that there are large chunks of dead code (#ifdef REASSOCIATION), down to the formatting of comments and other trivialities, but I'd rather address the larger issues first.

I said "mostly" fullmac. Sort of like ipw2100 (IIRC) is softmac but it does all the association stuff in firmware. It doesn't fit fullmac, but it's a lot more fullmac than atheros. There isn't any management frame processing, it doesn't do any data frame processing.

The 8388 architecture is typical of a thick firmware architecture, where the firmware handles all 802.11 MAC management tasks. The host driver downloads standard 802.3 frames to the firmware to be transmitted over the link as 802.11 frames.

I thought these 2 things were essentially _the_ definition of fullmac, please correct me if I'm wrong.

Perhaps I just don't understand how flexible d80211 has become; when we last were talking about it 8 months ago, it appears that it could not handle parts that did significant pieces of work in firmware, like the 8388 does. Do we have the functionality in d80211 yet to handle pieces that are a full mix of hybrid full/soft mac?

1) What, if any of this, could actually fit into the d80211 stack, given the mostly fullmac operations of this part?

3) Could you enumerate your issues with the command dispatching? Right now, many of the commands are blocking, which is suboptimal. This is an artifact of the embedded history of the part. We need to eventually fix that and make the upper layers not expect to block stuff like auth/assoc and scanning. What other stuff are you thinking of?

Quoted 7 lines · Johannes Berg

Quoted 4 lines · Dan Williams

The part is a mostly fullmac part that until now has been targetted at the embedded market (PSP, N80 phone, etc). That means that the driver shouldn't be too large (airo is about 8000 kLOC without airo_cs) but for some reason it still is; we're working on that.

Quoted 4 lines

I disagree that this is a fullmac part. You need to tell it to authenticate and then you need to tell it to associate. IPW is a fullmac, you tell it "use SSID xxx". Not this, you tell it "auth to BSSID aaa" and then "associate to BSSID aaa" etc.

I said "mostly" fullmac. Sort of like ipw2100 (IIRC) is softmac but it does all the association stuff in firmware. It doesn't fit fullmac, but it's a lot more fullmac than atheros. There isn't any management frame processing, it doesn't do any data frame processing.

The 8388 architecture is typical of a thick firmware architecture, where the firmware handles all 802.11 MAC management tasks. The host driver downloads standard 802.3 frames to the firmware to be transmitted over the link as 802.11 frames.

I thought these 2 things were essentially _the_ definition of fullmac, please correct me if I'm wrong.

Perhaps I just don't understand how flexible d80211 has become; when we last were talking about it 8 months ago, it appears that it could not handle parts that did significant pieces of work in firmware, like the 8388 does. Do we have the functionality in d80211 yet to handle pieces that are a full mix of hybrid full/soft mac?

Essentially, the auth/assoc path is this (D=driver, F=firmware):

D: AUTHENTICATE (BSSID)
F: look up in scan table
F: handle all Open System or Shared Key auth frames and housekeeping
F: send success to driver
D: ASSOCIATE (BSSID)
F: handle association request and response frames and housekeeping

I fail to see how this is handholding.

If you want handholding, look at atmel. That's a sort-of "fullmac" part in which you have to do a lot of handholding in the driver, including handle the authentication state machine and tell the firmware what step to go through. For example, from atmel.c:

static void send_authentication_request(struct atmel_private *priv, u16 system,
u8 *challenge, int challenge_len)
{
...

	if (priv->wep_is_on && priv->CurrentAuthentTransactionSeqNum != 1)
		/* no WEP for authentication frames with TrSeqNo 1 */
                header.frame_ctl |=  cpu_to_le16(IEEE80211_FCTL_PROTECTED);

	auth.alg = cpu_to_le16(system);

	auth.status = 0;
	auth.trans_seq = cpu_to_le16(priv->CurrentAuthentTransactionSeqNum);
	priv->ExpectedAuthentTransactionSeqNum = priv->CurrentAuthentTransactionSeqNum+1;
	priv->CurrentAuthentTransactionSeqNum += 2;

	if (challenge_len != 0)	{
		auth.el_id = 16; /* challenge_text */
		auth.chall_text_len = challenge_len;
		memcpy(auth.chall_text, challenge, challenge_len);
		atmel_transmit_management_frame(priv, &header, (u8 *)&auth, 8 + challenge_len);
	} else {
		atmel_transmit_management_frame(priv, &header, (u8 *)&auth, 6);
	}

...
}

Now _THATS_ what I call handholding. We don't have to do any of that here.

Quoted 5 lines

Besides the fact that I think this is a very dumb thing to do in new hardware due to IEEE 802.11w (protected management frames) being well underway which you can't handle at all that way, I also don't see how you can claim that this is fullmac when you need to tell it every step of the way.

The driver certainly doesn't need to tell it "every step of the way". The driver sends two commands, there are two steps. It does _NOT_ deal with management frames, challenge/response of Shared Key (like atmel), or even the details of open system.

After that, it just shoves 802.3 framed packets into the device from wlan_tx.c and gets 802.3 framed packets out from wlan_rx.c.

Quoted 5 lines

The way I see it, you're telling the firmware to send an auth frame and then you wait for the firmware to come back and tell you "yes I have received a response" instead of just sending the frame yourself and then waiting for the response frame. So the difference is that you need to do a bit less frame parsing in the driver-stack.

Not quite; the driver doesn't have to deal with the Open System handshake or the Shared Key handshake. Instead of Airo's 1-step process, this is a simple 2-step process. Look at wlan_join.c, wlan_associate(). That's what happens; nothing more.

Quoted 4 lines

I could drown you in technical comments on everything starting from the way commands are sent, via the fact that there are large chunks of dead code (#ifdef REASSOCIATION), down to the formatting of comments and other trivialities, but I'd rather address the larger issues first.

And here, you'd be completely correct. I could go into a long list of things that need to be cleaned up. Those of us working on the driver certainly have a lot of work to do, no questions asked. But...

I think you may have gotten the wrong impression about the part due to the cruft in the driver. We'll clean that stuff out, like the ENTER/LEAVE and a host of other junk. But what we'd like is comments on how to clean it up and what other people think should get re-arranged, and maybe how it could be integrated with d80211 more.

For example:

1) What, if any of this, could actually fit into the d80211 stack, given the mostly fullmac operations of this part? Other considerations include:
- Device requires firmware support for > 1 interface (rt2x00 also doesn't support more than one non-monitor mode interface, so this isn't that much of a problem)
- Scanning must be done in 3 channel groups so you don't overflow the firmware results buffer and truncate BSS data
- All packets are 802.3 framed (firmware does all 802.11 framing)
- Firmware handles all 802.11 management frame tasks
- There are separate firmware commands to join or start an adhoc network

2) How could we take advantage the region stuff and 802.11d code in d80211? I was also under the impression that the region code and regulatory domain stuff was nowhere near decided on. I believe there are full sessions on this at the LWS II this weekend.

3) Could you enumerate your issues with the command dispatching? Right now, many of the commands are blocking, which is suboptimal. This is an artifact of the embedded history of the part. We need to eventually fix that and make the upper layers not expect to block stuff like auth/assoc and scanning. What other stuff are you thinking of?

Thanks!
Dan

-
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

The 8388 architecture is typical of a thick firmware architecture, where the firmware handles all 802.11 MAC management tasks. The host driver downloads standard 802.3 frames to the firmware to be transmitted over the link as 802.11 frames.

I thought these 2 things were essentially _the_ definition of fullmac, please correct me if I'm wrong.

Perhaps I just don't understand how flexible d80211 has become; when we last were talking about it 8 months ago, it appears that it could not handle parts that did significant pieces of work in firmware, like the 8388 does. Do we have the functionality in d80211 yet to handle pieces that are a full mix of hybrid full/soft mac?

[...]
Not quite; the driver doesn't have to deal with the Open System handshake or the Shared Key handshake. Instead of Airo's 1-step process, this is a simple 2-step process. Look at wlan_join.c, wlan_associate(). That's what happens; nothing more.

[...]
I think you may have gotten the wrong impression about the part due to the cruft in the driver. We'll clean that stuff out, like the ENTER/LEAVE and a host of other junk. But what we'd like is comments on how to clean it up and what other people think should get re-arranged, and maybe how it could be integrated with d80211 more.

2) How could we take advantage the region stuff and 802.11d code in d80211? I was also under the impression that the region code and regulatory domain stuff was nowhere near decided on. I believe there are full sessions on this at the LWS II this weekend.

Quoted 3 lines · Marcelo Tosatti

_Please_ review, this driver is targeted for mainline inclusion.

There have been almost no comments resulting from the first submission.

We had looked over the previous version a few days ago and noticed that the biggest part of the code is a mostly generic wireless stack that you're including along with the driver, essentially all the wlan_* files. I don't think that's going to fly.

Also a bunch of minor things but I'd rather look over this version again before commenting on that.

And using the Marvell provided firmware is a requirement for OLPC machines, where the CPU will be shut down but the chip+firmware will continue to forward packets in the mesh network (there are extreme power saving constraints on these machines).

Furthermore, I might add that the entire reason this part was chosen for OLPC was because it _is_ fullmac.

To drive down power consumption, the OLPC puts the host CPU to sleep but keeps the 8388 powered on. That consumes around 400mW max. But it allows the 8388 to continue routing other laptops' packets over the mesh *while the host CPU is asleep*.

Quoted 7 lines · Dan Williams

Furthermore, I might add that the entire reason this part was chosen for OLPC was because it _is_ fullmac.

To drive down power consumption, the OLPC puts the host CPU to sleep but keeps the 8388 powered on. That consumes around 400mW max. But it allows the 8388 to continue routing other laptops' packets over the mesh *while the host CPU is asleep*.

We're not going to put a lot of junk into the kernel just because the OLPC folks decide to do odd powermanagment schemes.

Quoted 5 lines · Christoph Hellwig

Quoted 2 lines · Dan Williams

allows the 8388 to continue routing other laptops' packets over the mesh *while the host CPU is asleep*.

Quoted 2 lines

We're not going to put a lot of junk into the kernel just because the OLPC folks decide to do odd powermanagment schemes.

We're not going to ignore useful power management schemes just because they don't fit neatly into a pre-existing category.

I think the request to determine how all this maps into MLME is fair, though.

Quoted 7 lines · Dan Williams

Furthermore, I might add that the entire reason this part was chosen for OLPC was because it _is_ fullmac.

To drive down power consumption, the OLPC puts the host CPU to sleep but keeps the 8388 powered on. That consumes around 400mW max. But it allows the 8388 to continue routing other laptops' packets over the mesh *while the host CPU is asleep*.

We're not going to put a lot of junk into the kernel just because the OLPC folks decide to do odd powermanagment schemes.

Quoted 4 lines · Marcelo Tosatti

And using the Marvell provided firmware is a requirement for OLPC machines, where the CPU will be shut down but the chip+firmware will continue to forward packets in the mesh network (there are extreme power saving constraints on these machines).

Well, than it probably doesn't go into mainline if you want to continue doing this stupid layering violation.

In the future we'll likely need some layering to support the 8385 SDIO/CF variant, but most likely not in way the USB support is currently excessively layered and abstracted.

Quoted 3 lines · Dan Williams

In the future we'll likely need some layering to support the 8385 SDIO/CF variant, but most likely not in way the USB support is currently excessively layered and abstracted.

Yeah. Let's summarize my unfortunately a bit too nasty comments and your more helpfull replies :-)

This driver still needs a lot more work, both to cleanup a lot of crap and integrate it better with the wireless stack. And OLPC needs this is not going to be an excuse of it's own.

Quoted 3 lines · Dan Williams

In the future we'll likely need some layering to support the 8385 SDIO/CF variant, but most likely not in way the USB support is currently excessively layered and abstracted.

Yeah. Let's summarize my unfortunately a bit too nasty comments and your more helpfull replies :-)

This driver still needs a lot more work, both to cleanup a lot of crap and integreate it better with the wireless stack. And OLPC needs this is not going to be an excuse of it's own.

Quoted 11 lines · Christoph Hellwig

Quoted 3 lines · Dan Williams

In the future we'll likely need some layering to support the 8385 SDIO/CF variant, but most likely not in way the USB support is currently excessively layered and abstracted.

Yeah. Let's summarize my unfortunately a bit too nasty comments and your more helpfull replies :-)

This driver still needs a lot more work, both to cleanup a lot of crap and integrate it better with the wireless stack. And OLPC needs this is not going to be an excuse of it's own.

The main feature of this chip is the on-chip support for 802.11s in firmware. What is the plan for integrating 802.11s into the existing wireless stacks? Does it make sense to do a softmac type 802.11s implementation first to figure out the right places to put the hooks for the 8388 hardware implementation?

Quoted 4 lines · Dan Williams

I said "mostly" fullmac. Sort of like ipw2100 (IIRC) is softmac but it does all the association stuff in firmware. It doesn't fit fullmac, but it's a lot more fullmac than atheros. There isn't any management frame processing, it doesn't do any data frame processing.

Right. I hadn't looked at data frames but suspected this is the case.

Quoted 7 lines

The 8388 architecture is typical of a thick firmware architecture, where the firmware handles all 802.11 MAC management tasks. The host driver downloads standard 802.3 frames to the firmware to be transmitted over the link as 802.11 frames.

I thought these 2 things were essentially _the_ definition of fullmac, please correct me if I'm wrong.

Well we'll need more terms here. I had a short chat with Jouni and we agree that what the Marvell card is doing is exposing the MLME interface, and what d80211 is doing is implementing most of the MLME.

I guess the thing I'm saying is that exposing the MLME interface which changes fairly frequently is a bad thing and I'd love to have firmware that exposes lower level stuff like .

Quoted 5 lines

Perhaps I just don't understand how flexible d80211 has become; when we last were talking about it 8 months ago, it appears that it could not handle parts that did significant pieces of work in firmware, like the 8388 does. Do we have the functionality in d80211 yet to handle pieces that are a full mix of hybrid full/soft mac?

No, we don't have that yet, and we might never have. But I think I'm starting to understand the issues at hand a bit better and it seems that we'll need cfg80211 to be redefined.

Quoted 5 lines

3) Could you enumerate your issues with the command dispatching? Right now, many of the commands are blocking, which is suboptimal. This is an artifact of the embedded history of the part. We need to eventually fix that and make the upper layers not expect to block stuff like auth/assoc and scanning. What other stuff are you thinking of?

Well, one thing is that the whole command processing goes through a single function for no real benefit. You're basically saying command(...,CMD_NUM,...) and in command() you do switch(cmd_num) and multiplex it to a lot of functions again. I suppose I'd rather it called the functions directly and those in turn called the little common code there is in the dispatch routine.

Quoted 31 lines · Johannes Berg

Quoted 4 lines · Dan Williams

I said "mostly" fullmac. Sort of like ipw2100 (IIRC) is softmac but it does all the association stuff in firmware. It doesn't fit fullmac, but it's a lot more fullmac than atheros. There isn't any management frame processing, it doesn't do any data frame processing.

Right. I hadn't looked at data frames but suspected this is the case.

Quoted 7 lines

The 8388 architecture is typical of a thick firmware architecture, where the firmware handles all 802.11 MAC management tasks. The host driver downloads standard 802.3 frames to the firmware to be transmitted over the link as 802.11 frames.

I thought these 2 things were essentially _the_ definition of fullmac, please correct me if I'm wrong.

Well we'll need more terms here. I had a short chat with Jouni and we agree that what the Marvell card is doing is exposing the MLME interface, and what d80211 is doing is implementing most of the MLME.

I guess the thing I'm saying is that exposing the MLME interface which changes fairly frequently is a bad thing and I'd love to have firmware that exposes lower level stuff like .

Quoted 5 lines

Perhaps I just don't understand how flexible d80211 has become; when we last were talking about it 8 months ago, it appears that it could not handle parts that did significant pieces of work in firmware, like the 8388 does. Do we have the functionality in d80211 yet to handle pieces that are a full mix of hybrid full/soft mac?

Quoted 3 lines

No, we don't have that yet, and we might never have. But I think I'm starting to understand the issues at hand a bit better and it seems that we'll need cfg80211 to be redefined.

So using the d80211 stack is completly out of question?

Another detail is the way we deal with mesh networking: a separate device "mshX" is created, and that certainly does not fit d80211?

What is the general solution for 802.11s?

I'm working on an embedded
design that would benefit from 11s support and I'd rather not have to roll my own mesh implementation in user space.

I'd also like to get an
idea of what kind of hardware I need to design onto my board given that the 8388 is not available general consumption. My system has plenty of power and CPU available so a softmac type 11s solution is the most cost effective. It seems to me that the design of a software based 11s stack should be coordinated with the 8388 firmware one.

Quoted 1 line · Jon Smirl

What is the general solution for 802.11s?

None yet.

Quoted 3 lines

I'm working on an embedded
design that would benefit from 11s support and I'd rather not have to roll my own mesh implementation in user space.

Well, there is no mesh implementation that I'm aware of, you'll want to stick it along with the userspace MLME into wpa_supplicant.

Quoted 6 lines

I'd also like to get an
idea of what kind of hardware I need to design onto my board given that the 8388 is not available general consumption. My system has plenty of power and CPU available so a softmac type 11s solution is the most cost effective. It seems to me that the design of a software based 11s stack should be coordinated with the 8388 firmware one.

It should, but afaik no one is really working on it either way.

Is the 802.11s Draft 1.0 spec publicly available yet? It is supposed to be making changes at the very lowest MAC layers. It will be hard to be compatible with that from user space.

The software doesn't have to be written but there should be at least some design work done on how 11s should fit into the existing world for both a firmware and software implementation. I'd hate to see a different implementation for each vendor and another one for the software stack.

Quoted 3 lines · Jon Smirl

Is the 802.11s Draft 1.0 spec publicly available yet? It is supposed to be making changes at the very lowest MAC layers. It will be hard to be compatible with that from user space.

Good point, it's not possible to implement this without changes to the MAC.

Quoted 5 lines

The software doesn't have to be written but there should be at least some design work done on how 11s should fit into the existing world for both a firmware and software implementation. I'd hate to see a different implementation for each vendor and another one for the software stack.

Then we'll first have to find those vendors interested in actually changing their MAC to allow .11s I guess.

I haven't seen the 11s spec, are devices with softmac implementations flexible enough to implement it or do they need firmware changes too?

Quoted 15 lines · Johannes Berg

Quoted 3 lines · Jon Smirl

Is the 802.11s Draft 1.0 spec publicly available yet? It is supposed to be making changes at the very lowest MAC layers. It will be hard to be compatible with that from user space.

Good point, it's not possible to implement this without changes to the MAC.

Quoted 5 lines

The software doesn't have to be written but there should be at least some design work done on how 11s should fit into the existing world for both a firmware and software implementation. I'd hate to see a different implementation for each vendor and another one for the software stack.

Quoted 2 lines

Then we'll first have to find those vendors interested in actually changing their MAC to allow .11s I guess.

I haven't seen the 11s spec, are devices with softmac implementations flexible enough to implement it or do they need firmware changes too?

Quoted 19 lines · Jon Smirl

Quoted 17 lines · Johannes Berg

Quoted 3 lines · Jon Smirl

Is the 802.11s Draft 1.0 spec publicly available yet? It is supposed to be making changes at the very lowest MAC layers. It will be hard to be compatible with that from user space.

Good point, it's not possible to implement this without changes to the MAC.

Quoted 5 lines

The software doesn't have to be written but there should be at least some design work done on how 11s should fit into the existing world for both a firmware and software implementation. I'd hate to see a different implementation for each vendor and another one for the software stack.

Then we'll first have to find those vendors interested in actually changing their MAC to allow .11s I guess.

Quoted 2 lines

I haven't seen the 11s spec, are devices with softmac implementations flexible enough to implement it or do they need firmware changes too?

I believe it needs to at least support 4 address frames, which usually requires firmware changes on fullmac cards, but fully softmac cards (atheros, broadcom) probably would not.

I believe it needs to at least support 4 address frames, which usually requires firmware changes on fullmac cards, but fully softmac cards (atheros, broadcom) probably would not.

Quoted 3 lines · Dan Williams

I believe it needs to at least support 4 address frames, which usually requires firmware changes on fullmac cards, but fully softmac cards (atheros, broadcom) probably would not.

.11s also introduces new frame types/subtypes that probably need to be ACKed which usually the firmware won't do since it doesn't understand those frame types/subtypes yet.

.11s also introduces new frame types/subtypes that probably need to be ACKed which usually the firmware won't do since it doesn't understand those frame types/subtypes yet.

Quoted 12 lines · Jeff Garzik

Quoted 7 lines · Christoph Hellwig

Quoted 2 lines · Dan Williams

allows the 8388 to continue routing other laptops' packets over the mesh *while the host CPU is asleep*.

We're not going to put a lot of junk into the kernel just because the OLPC folks decide to do odd powermanagment schemes.

We're not going to ignore useful power management schemes just because they don't fit neatly into a pre-existing category.

Quoted 2 lines

I think the request to determine how all this maps into MLME is fair, though.

Definitely. Also, I wonder if there was any attempt to evaluate how the ieee80211 (or d80211) code might be extended in order to elimnate the need for some of the libertas wlan_* files?

Quoted 13 lines · John W. Linville

Quoted 12 lines · Jeff Garzik

Quoted 7 lines · Christoph Hellwig

Quoted 2 lines · Dan Williams

allows the 8388 to continue routing other laptops' packets over the mesh *while the host CPU is asleep*.

We're not going to put a lot of junk into the kernel just because the OLPC folks decide to do odd powermanagment schemes.

We're not going to ignore useful power management schemes just because they don't fit neatly into a pre-existing category.

Quoted 6 lines

Quoted 2 lines

I think the request to determine how all this maps into MLME is fair, though.

Definitely. Also, I wonder if there was any attempt to evaluate how the ieee80211 (or d80211) code might be extended in order to elimnate the need for some of the libertas wlan_* files?

The regulatory domain structures, channel information (struct ieee80211_channel), HW mode (struct ieee80211_hw_mode) compromised of supported channels and rates, and probably a few others in the same category. I can't see the possibility of using d80211 as it stands (designed for softmac cards dealing with 802.11 packets to/from the OS).

However, it does not make any sense to use the structures defined by d80211 if not effectively using it (we send/receive 802.3 frames to the firmware, after all), IMO.

As discussed on this thread, there is a lot of code to be cleanup up, but no structural changes AFAICT. Is there a general agreement on that, now?

I pushed for a general "lib80211" sort of thing at the Summit, which I think was agreed in principle with others like Intel. We need something to hold the bits that are common to d80211 and non-softmac drivers. These include scan result handling, some bits of the regulatory stuff (at least structures and definitions for allowed channels and txpower in each domain), and 802.3 <-> 802.11 framing conversion code. We should be able to fold more stuff in there as we go along. But there certainly will be code that both softmac/d80211 and non-softmac drivers (airo, ipw2x00, libertas, etc) can share.

Announcing an updated patch of the Marvell Libertas 8388 802.11 USB driver.

Diff can be found at
http://dev.laptop.org/~marcelo/libertas-8388-16012007.patch

_Please_ review, this driver is targeted for mainline inclusion.

There have been almost no comments resulting from the first submission.

drivers/net/wireless/libertas/Makefile new file +4 -0

+
+/** General Result Code*/
+/* OK */
+#define HostCmd_RESULT_OK                    0x0000

drivers/net/wireless/libertas/Makefile new file +15 -0

+/* Definition of action or option for each command */
+
+/* Define general purpose action */
+#define HostCmd_ACT_GEN_READ                    0x0000
+#define HostCmd_ACT_GEN_WRITE                   0x0001
+#define HostCmd_ACT_GEN_GET                     0x0000
+#define HostCmd_ACT_GEN_SET                     0x0001
+#define HostCmd_ACT_GEN_REMOVE			0x0002
+#define HostCmd_ACT_GEN_OFF                     0x0000
+#define HostCmd_ACT_GEN_ON                      0x0001
+
+/* Define action or option for HostCmd_CMD_802_11_AUTHENTICATE */
+#define HostCmd_ACT_AUTHENTICATE                0x0001
+#define HostCmd_ACT_DEAUTHENTICATE              0x0002
+

drivers/net/wireless/libertas/Makefile new file +12 -0

+/* TxPD descriptor */
+struct TxPD {
+	/* Current Tx packet status */
+	u32 TxStatus;
+	/* Tx Control */
+	u32 TxControl;
+	u32 TxPacketLocation;
+	/* Tx packet length */
+	u16 TxPacketLength;
+	/* First 2 byte of destination MAC address */
+	u8 TxDestAddrHigh[2];
+	/* Last 4 byte of destination MAC address */

drivers/net/wireless/libertas/Makefile new file +34 -0

+/* HostCmd_DS_GET_HW_SPEC */
+struct HostCmd_DS_GET_HW_SPEC {
+	/* HW Interface version number */
+	u16 HWIfVersion;
+
+	/* HW version number */
+	u16 Version;
+
+	/* Max number of TxPD FW can handle */
+	u16 NumOfTxPD;
+
+	/* Max no of Multicast address */
+	u16 NumOfMCastAdr;
+
+	/* MAC address */
+	u8 PermanentAddr[6];
+
+	/* Region Code */
+	u16 RegionCode;
+
+	/* Number of antenna used */
+	u16 NumberOfAntenna;
+
+	/* FW release number, example 0x1234=1.2.3.4 */
+	u32 FWReleaseNumber;
+
+	/* Base Address of TxPD queue */
+	u32 WcbBase;
+	/* Read Pointer of RxPd queue */
+	u32 RxPdRdPtr;
+
+	/* Write Pointer of RxPd queue */
+	u32 RxPdWrPtr;
+

drivers/net/wireless/libertas/Makefile new file +10 -0

+/* HostCmd_DS_802_11_AFC */
+struct HostCmd_DS_802_11_AFC {
+	u16 afc_auto;
+	union {
+		struct {
+			u16 threshold;
+			u16 period;
+		} auto_mode;
+		struct {
+			s16 timing_offset;

+static u8 usb_int_cause = 0;

+struct BootCMDStr sBootCMD;

drivers/net/wireless/libertas/Makefile new file +5 -0

+int if_usb_issue_boot_command(wlan_private *priv, int iValue);
+
+static int if_prog_firmware(wlan_private * priv);
+static void if_usb_receive(struct urb *urb);
+static void if_usb_receive_fwload(struct urb *urb);

drivers/net/wireless/libertas/Makefile new file +5 -0

+static void if_usb_write_bulk_callback(struct urb *urb)
+{
+	wlan_private *priv = (wlan_private *) (urb->context);
+	wlan_adapter *adapter = priv->adapter;
+	struct net_device *dev = priv->wlan_dev.netdev;

drivers/net/wireless/libertas/Makefile new file +10 -0

+static struct file_operations libertas_devinfo_fops = { 
+	.owner = THIS_MODULE,
+	.open = open_file_generic,
+	.write = write_file_dummy,
+	.read = libertas_dev_info,
+};
+
+static struct file_operations libertas_getscantable_fops = { 
+	.owner = THIS_MODULE,
+	.open = open_file_generic,

drivers/net/wireless/libertas/Makefile new file +23 -0

+/** Double-Word(32Bit) Bit definition */
+#define DW_BIT_0	0x00000001
+#define DW_BIT_1	0x00000002
+#define DW_BIT_2	0x00000004
+#define DW_BIT_3	0x00000008
+#define DW_BIT_4	0x00000010
+#define DW_BIT_5	0x00000020
+#define DW_BIT_6	0x00000040
+#define DW_BIT_7	0x00000080
+#define DW_BIT_8	0x00000100
+#define DW_BIT_9	0x00000200
+#define DW_BIT_10	0x00000400
+#define DW_BIT_11       0x00000800
+#define DW_BIT_12       0x00001000
+#define DW_BIT_13       0x00002000
+#define DW_BIT_14       0x00004000
+#define DW_BIT_15       0x00008000
+#define DW_BIT_16       0x00010000
+#define DW_BIT_17       0x00020000
+#define DW_BIT_18       0x00040000
+#define DW_BIT_19       0x00080000
+#define DW_BIT_20       0x00100000
+#define DW_BIT_21       0x00200000

drivers/net/wireless/libertas/Makefile new file +18 -0

+
+/** Word (16bit) Bit Definition*/
+#define W_BIT_0		0x0001
+#define W_BIT_1		0x0002
+#define W_BIT_2		0x0004
+#define W_BIT_3		0x0008
+#define W_BIT_4		0x0010
+#define W_BIT_5		0x0020
+#define W_BIT_6		0x0040
+#define W_BIT_7		0x0080
+#define W_BIT_8		0x0100
+#define W_BIT_9		0x0200
+#define W_BIT_10	0x0400
+#define W_BIT_11	0x0800
+#define W_BIT_12	0x1000
+#define W_BIT_13	0x2000
+#define W_BIT_14	0x4000
+#define W_BIT_15	0x8000

drivers/net/wireless/libertas/Makefile new file +9 -0

+
+#if defined(DEBUG)
+static inline void HEXDUMP(char *prompt, u8 * buf, int len)
+{
+	int i = 0;
+
+	if (!libertas_debug)
+		return;
+

drivers/net/wireless/libertas/Makefile new file +4 -0

+static int wlan_setup_station_hw(wlan_private * priv)
+{
+	int ret = WLAN_STATUS_FAILURE;
+	wlan_adapter *adapter = priv->adapter;

drivers/net/wireless/libertas/wlan_ioctl.c new file +60 -0

+#define WLAN_TX_PWR_DEFAULT		20	/*100mW */
+#define WLAN_TX_PWR_US_DEFAULT		20	/*100mW */
+#define WLAN_TX_PWR_JP_DEFAULT		16	/*50mW */
+#define WLAN_TX_PWR_FR_DEFAULT		20	/*100mW */
+#define WLAN_TX_PWR_EMEA_DEFAULT	20	/*100mW */
+
+/* Format { Channel, Frequency (MHz), MaxTxPower } */
+/* Band: 'B/G', Region: USA FCC/Canada IC */
+static struct chan_freq_power channel_freq_power_US_BG[] = {
+	{1, 2412, WLAN_TX_PWR_US_DEFAULT},
+	{2, 2417, WLAN_TX_PWR_US_DEFAULT},
+	{3, 2422, WLAN_TX_PWR_US_DEFAULT},
+	{4, 2427, WLAN_TX_PWR_US_DEFAULT},
+	{5, 2432, WLAN_TX_PWR_US_DEFAULT},
+	{6, 2437, WLAN_TX_PWR_US_DEFAULT},
+	{7, 2442, WLAN_TX_PWR_US_DEFAULT},
+	{8, 2447, WLAN_TX_PWR_US_DEFAULT},
+	{9, 2452, WLAN_TX_PWR_US_DEFAULT},
+	{10, 2457, WLAN_TX_PWR_US_DEFAULT},
+	{11, 2462, WLAN_TX_PWR_US_DEFAULT}
+};
+
+/* Band: 'B/G', Region: Europe ETSI */
+static struct chan_freq_power channel_freq_power_EU_BG[] = {
+	{1, 2412, WLAN_TX_PWR_EMEA_DEFAULT},
+	{2, 2417, WLAN_TX_PWR_EMEA_DEFAULT},
+	{3, 2422, WLAN_TX_PWR_EMEA_DEFAULT},
+	{4, 2427, WLAN_TX_PWR_EMEA_DEFAULT},
+	{5, 2432, WLAN_TX_PWR_EMEA_DEFAULT},
+	{6, 2437, WLAN_TX_PWR_EMEA_DEFAULT},
+	{7, 2442, WLAN_TX_PWR_EMEA_DEFAULT},
+	{8, 2447, WLAN_TX_PWR_EMEA_DEFAULT},
+	{9, 2452, WLAN_TX_PWR_EMEA_DEFAULT},
+	{10, 2457, WLAN_TX_PWR_EMEA_DEFAULT},
+	{11, 2462, WLAN_TX_PWR_EMEA_DEFAULT},
+	{12, 2467, WLAN_TX_PWR_EMEA_DEFAULT},
+	{13, 2472, WLAN_TX_PWR_EMEA_DEFAULT}
+};
+
+/* Band: 'B/G', Region: Spain */
+static struct chan_freq_power channel_freq_power_SPN_BG[] = {
+	{10, 2457, WLAN_TX_PWR_DEFAULT},
+	{11, 2462, WLAN_TX_PWR_DEFAULT}
+};
+
+/* Band: 'B/G', Region: France */
+static struct chan_freq_power channel_freq_power_FR_BG[] = {
+	{10, 2457, WLAN_TX_PWR_FR_DEFAULT},
+	{11, 2462, WLAN_TX_PWR_FR_DEFAULT},
+	{12, 2467, WLAN_TX_PWR_FR_DEFAULT},
+	{13, 2472, WLAN_TX_PWR_FR_DEFAULT}
+};
+
+/* Band: 'B/G', Region: Japan */
+static struct chan_freq_power channel_freq_power_JPN_BG[] = {
+	{1, 2412, WLAN_TX_PWR_JP_DEFAULT},
+	{2, 2417, WLAN_TX_PWR_JP_DEFAULT},
+	{3, 2422, WLAN_TX_PWR_JP_DEFAULT},
+	{4, 2427, WLAN_TX_PWR_JP_DEFAULT},
+	{5, 2432, WLAN_TX_PWR_JP_DEFAULT},

Quoted 9 lines · Marcelo Tosatti

Announcing an updated patch of the Marvell Libertas 8388 802.11 USB driver.

Diff can be found at
http://dev.laptop.org/~marcelo/libertas-8388-16012007.patch

_Please_ review, this driver is targeted for mainline inclusion.

Quoted 1 line

There have been almost no comments resulting from the first submission.

I think the main problem is the size of the driver. I have spend several hours on reviewing it, but this has barely scratched the surface of the problems that are still in it. Most of the important problems have already been discussed widely, but I'll still post what I noticed about the driver.

My general feeling is that even at this point it would be better to write a new driver for it from scratch than trying to remove all the bloat from the existing one. Of course, that requires deep knowledge of how the hardware works, so I don't consider the time you spent on hacking it lost.

I also think that your work done so far is an enormous improvement over where it started, you are definitely on the right track.

The level of detail in my review drops towards the end, mostly because some issues found in the first files are continuing to throughout the driver.

drivers/net/wireless/libertas/Makefile line 9

Quoted 12 lines

drivers/net/wireless/libertas/Makefile new file +9 -0

@@ -0,0 +1,21 @@
+# EXTRA_CFLAGS += -Wpacked
+
+usb8xxx-objs := wlan_main.o wlan_fw.o wlan_wext.o \
+		wlan_rx.o wlan_tx.o wlan_cmd.o 	  \
+		wlan_cmdresp.o wlan_scan.o	  \
+		wlan_join.o wlan_11d.o 		  \
+		wlan_ioctl.o wlan_debugfs.o	  \
+		wlan_ethtool.o wlan_assoc.o
+

I guess you can get rid of the wlan_ file name prefix, since all files are already in the same directory to give them a name space.

drivers/net/wireless/libertas/Makefile line 13

Quoted 4 lines

drivers/net/wireless/libertas/Makefile new file +4 -0

+
+ (c) Copyright  2003-2006, Marvell International Ltd. 
+ All Rights Reserved
+

'All Rights Reserved' sounds wrong, considering it's actually GPL.

drivers/net/wireless/libertas/Makefile line 16

Quoted 3 lines

drivers/net/wireless/libertas/Makefile new file +3 -0

+  * This file contains definitions of WLAN commands.
+  *  
+  * (c) Copyright © 2003-2006, Marvell International Ltd. 

Something went wrong with the (c) character here, better not use UTF8 characters in source.

drivers/net/wireless/libertas/Makefile

Quoted 7 lines

drivers/net/wireless/libertas/Makefile new file +7 -0

+Change log:
+    10/11/05: Add Doxygen format comments
+    01/11/06: Remove assoc response codes; full IEEE assoc resp now returned
+    04/06/06: Add TSPEC, queue metrics, and MSDU expiry support
+    04/10/06: Add power_adapt_cfg_ext command
+    04/18/06: Remove old Subscrive Event and add new Subscribe Event
+	      implementation through generic hostcmd API

changelog should go away

drivers/net/wireless/libertas/Makefile

Quoted 5 lines

drivers/net/wireless/libertas/Makefile new file +5 -0

+/** Command Processing States and Options */
+#define HostCmd_STATE_IDLE                    0x0000
+#define HostCmd_STATE_IN_USE_BY_HOST          0x0001
+#define HostCmd_STATE_IN_USE_BY_MINIPORT      0x0002
+#define HostCmd_STATE_FINISHED                0x000f

No SilLYcAps please

Quoted 9 lines

drivers/net/wireless/libertas/Makefile new file +9 -0

+
+/** General Result Code*/
+/* OK */
+#define HostCmd_RESULT_OK                    0x0000
+/* Genenral error */
+#define HostCmd_RESULT_ERROR                 0x0001
+/* Command is not valid */
+#define HostCmd_RESULT_NOT_SUPPORT           0x0002
+/* Command is pending */

drivers/net/wireless/libertas/Makefile

Quoted 5 lines

drivers/net/wireless/libertas/Makefile new file +5 -0

+#define HostCmd_RESULT_PENDING               0x0003
+/* System is busy (command ignored) */
+#define HostCmd_RESULT_BUSY                  0x0004
+/* Data buffer is not big enough */
+#define HostCmd_RESULT_PARTIAL_DATA          0x0005

These seem to be completely unused, except HostCmd_RESULT_OK, which you can then remove as well.

Quoted 20 lines

drivers/net/wireless/libertas/Makefile new file +20 -0

+/* Definition of action or option for each command */
+
+/* Define general purpose action */
+#define HostCmd_ACT_GEN_READ                    0x0000
+#define HostCmd_ACT_GEN_WRITE                   0x0001
+#define HostCmd_ACT_GEN_GET                     0x0000
+#define HostCmd_ACT_GEN_SET                     0x0001
+#define HostCmd_ACT_GEN_REMOVE			0x0002
+#define HostCmd_ACT_GEN_OFF                     0x0000
+#define HostCmd_ACT_GEN_ON                      0x0001
+
+/* Define action or option for HostCmd_CMD_802_11_AUTHENTICATE */
+#define HostCmd_ACT_AUTHENTICATE                0x0001
+#define HostCmd_ACT_DEAUTHENTICATE              0x0002
+
+/* Define action or option for HostCmd_CMD_802_11_ASSOCIATE */
+#define HostCmd_ACT_ASSOCIATE                   0x0001
+#define HostCmd_ACT_DISASSOCIATE                0x0002
+#define HostCmd_ACT_REASSOCIATE                 0x0003
+

drivers/net/wireless/libertas/Makefile

Quoted 5 lines

drivers/net/wireless/libertas/Makefile new file +5 -0

+#define HostCmd_CAPINFO_ESS                     0x0001
+#define HostCmd_CAPINFO_IBSS                    0x0002
+#define HostCmd_CAPINFO_CF_POLLABLE             0x0004
+#define HostCmd_CAPINFO_CF_REQUEST              0x0008
+#define HostCmd_CAPINFO_PRIVACY                 0x0010

Most of these are unused as well, I guess it would be good to go through the whole list and see what can be removed.

Quoted 16 lines

drivers/net/wireless/libertas/Makefile new file +16 -0

+/* TxPD descriptor */
+struct TxPD {
+	/* Current Tx packet status */
+	u32 TxStatus;
+	/* Tx Control */
+	u32 TxControl;
+	u32 TxPacketLocation;
+	/* Tx packet length */
+	u16 TxPacketLength;
+	/* First 2 byte of destination MAC address */
+	u8 TxDestAddrHigh[2];
+	/* Last 4 byte of destination MAC address */
+	u8 TxDestAddrLow[4];
+	/* Pkt Priority */
+	u8 Priority;
+	/* Pkt Trasnit Power control */

drivers/net/wireless/libertas/Makefile

Quoted 6 lines

drivers/net/wireless/libertas/Makefile new file +6 -0

+	u8 PowerMgmt;
+	/* Amount of time the packet has been queued in the driver (units = 2ms) */
+	u8 PktDelay_2ms;
+	/* Reserved */
+	u8 Reserved1;
+};

More SilLYcAps to convert...

Quoted 4 lines

drivers/net/wireless/libertas/Makefile new file +4 -0

+struct WLAN_802_11_KEY {
+	u16 type; /* KEY_TYPE_* from wlan_defs.h */
+	u32 len;
+	u8 key[MRVL_MAX_KEY_WPA_KEY_LENGTH];

drivers/net/wireless/libertas/Makefile

Quoted 3 lines

drivers/net/wireless/libertas/Makefile new file +3 -0

+	u32 flags;  /* KEY_INFO_* from wlan_defs.h */
+};
+

This structure has member misalignment which requires padding. If you use them only in the kernel, please reorder the members to avoid this, otherwise it's probably better to make the padding explicit.

Quoted 39 lines

drivers/net/wireless/libertas/Makefile new file +39 -0

+/* HostCmd_DS_GET_HW_SPEC */
+struct HostCmd_DS_GET_HW_SPEC {
+	/* HW Interface version number */
+	u16 HWIfVersion;
+
+	/* HW version number */
+	u16 Version;
+
+	/* Max number of TxPD FW can handle */
+	u16 NumOfTxPD;
+
+	/* Max no of Multicast address */
+	u16 NumOfMCastAdr;
+
+	/* MAC address */
+	u8 PermanentAddr[6];
+
+	/* Region Code */
+	u16 RegionCode;
+
+	/* Number of antenna used */
+	u16 NumberOfAntenna;
+
+	/* FW release number, example 0x1234=1.2.3.4 */
+	u32 FWReleaseNumber;
+
+	/* Base Address of TxPD queue */
+	u32 WcbBase;
+	/* Read Pointer of RxPd queue */
+	u32 RxPdRdPtr;
+
+	/* Write Pointer of RxPd queue */
+	u32 RxPdWrPtr;
+
+	/*FW/HW Capability */
+	u32 fwCapInfo;
+} __attribute__ ((packed));
+
+/* HostCmd_CMD_EEPROM_UPDATE */

drivers/net/wireless/libertas/Makefile

Quoted 5 lines

drivers/net/wireless/libertas/Makefile new file +5 -0

+struct HostCmd_DS_EEPROM_UPDATE {
+	u16 Action;
+	u32 Value;
+} __attribute__ ((packed));
+

Why do you want to have these structures packed? This often causes much worse code than reordering the members for proper alignment.

drivers/net/wireless/libertas/Makefile

Quoted 4 lines

drivers/net/wireless/libertas/Makefile new file +4 -0

+/* HostCmd_RET_802_11_ASSOCIATE */
+struct HostCmd_DS_802_11_ASSOCIATE_RSP {
+	struct IEEEtypes_AssocRsp assocRsp;
+} __attribute__ ((packed));

this ((packed)) looks really weird, are you sure it does what you want it to?

Quoted 15 lines

drivers/net/wireless/libertas/Makefile new file +15 -0

+/* HostCmd_DS_802_11_AFC */
+struct HostCmd_DS_802_11_AFC {
+	u16 afc_auto;
+	union {
+		struct {
+			u16 threshold;
+			u16 period;
+		} auto_mode;
+		struct {
+			s16 timing_offset;
+			s16 carrier_offset;
+		} manual_mode;
+	} b;
+} __attribute__ ((packed));
+

drivers/net/wireless/libertas/Makefile

Quoted 5 lines

drivers/net/wireless/libertas/Makefile new file +5 -0

+#define afc_data b.data
+#define afc_thre b.auto_mode.threshold
+#define afc_period b.auto_mode.period
+#define afc_toff b.manual_mode.timing_offset
+#define afc_foff b.manual_mode.carrier_offset

should we use anonymous unions here? all recent gcc versions allow it.

drivers/net/wireless/libertas/Makefile

Quoted 8 lines

drivers/net/wireless/libertas/Makefile new file +8 -0

+		struct HostCmd_TX_RATE_QUERY txrate;
+		struct HostCmd_DS_BT_ACCESS bt;
+		struct HostCmd_DS_FWT_ACCESS fwt;
+		struct HostCmd_DS_MESH_ACCESS mesh;
+		struct HostCmd_DS_GET_TSF gettsf;
+		struct HostCmd_DS_802_11_SUBSCRIBE_EVENT subscribe_event;
+	} params;
+} __attribute__ ((packed));

same here.

drivers/net/wireless/libertas/Makefile

Quoted 3 lines

drivers/net/wireless/libertas/Makefile new file +3 -0

+
+extern struct BootCMDStr	sBootCMD;
+

'extern' declarations should _all_ go into header files. Normally, sparse should catch this.

Did you run the file through a recent sparse successfully?

Quoted 7 lines

drivers/net/wireless/libertas/Makefile new file +7 -0

+int if_usb_issue_boot_command(wlan_private *priv, int iValue)
+{
+	struct usb_card_rec	*cardp = priv->wlan_dev.card;
+	int i;
+
+	/* Prepare Command */
+	sBootCMD.u32MagicNumber = BOOT_CMD_MAGIC_NUMBER;

drivers/net/wireless/libertas/Makefile

Quoted 4 lines

drivers/net/wireless/libertas/Makefile new file +4 -0

+	sBootCMD.u8CMD_Tag = iValue;
+	for (i=0; i<11; i++)
+		sBootCMD.au8Dumy[i]=0x00;
+	memcpy(cardp->bulk_out_buffer, &sBootCMD, sizeof(struct BootCMDStr));

It looks like a bad idea to use the global sBootCMD variable here. I can't see any locking to protect it, and it's small enough to fit on the stack. What's the point?

drivers/net/wireless/libertas/Makefile

Quoted 6 lines

drivers/net/wireless/libertas/Makefile new file +6 -0

+
+	/* Issue Command */
+	usb_tx_block(priv, cardp->bulk_out_buffer, sizeof(struct BootCMDStr));
+
+	return WLAN_STATUS_SUCCESS;
+}

WLAN_STATUS_SUCCESS is not a standard return code. It would be better to use the standard calling conventions returning '0' or '-ESOMETHING' for failure instead of defining your own.

drivers/net/wireless/libertas/Makefile

Quoted 2 lines

drivers/net/wireless/libertas/Makefile new file +2 -0

+static usb_notifier_fn_add wlan_card_add_cb = NULL;
+static usb_notifier_fn_remove wlan_card_remove_cb = NULL;

You currently link the usb parts and the wlan parts of the driver into a single module, which makes the dynamic registration of these callbacks rather pointless. Can't you just get rid of that entirely?

Quoted 1 line

+static u8 usb_int_cause = 0;

This is a global variable, but you seem to use it per device. Is that intended?

Quoted 1 line

+struct BootCMDStr sBootCMD;

This variable is used only in one file, but defined in another...

Quoted 10 lines

drivers/net/wireless/libertas/Makefile new file +10 -0

+int if_usb_issue_boot_command(wlan_private *priv, int iValue);
+
+static int if_prog_firmware(wlan_private * priv);
+static void if_usb_receive(struct urb *urb);
+static void if_usb_receive_fwload(struct urb *urb);
+static int if_usb_probe(struct usb_interface *intf,
+			const struct usb_device_id *id);
+static void if_usb_disconnect(struct usb_interface *intf);
+#ifdef CONFIG_PM
+static int if_usb_suspend(struct usb_interface *intf, pm_message_t message);

drivers/net/wireless/libertas/Makefile

Quoted 5 lines

drivers/net/wireless/libertas/Makefile new file +5 -0

+static int if_usb_resume(struct usb_interface *intf);
+#else
+#define if_usb_suspend NULL
+#define if_usb_resume NULL
+#endif

It would be good to avoid forward declarations by reordering the functions into their call order (bottom up).

Quoted 5 lines

drivers/net/wireless/libertas/Makefile new file +5 -0

+static void if_usb_write_bulk_callback(struct urb *urb)
+{
+	wlan_private *priv = (wlan_private *) (urb->context);
+	wlan_adapter *adapter = priv->adapter;
+	struct net_device *dev = priv->wlan_dev.netdev;

drivers/net/wireless/libertas/Makefile

Quoted 10 lines

drivers/net/wireless/libertas/Makefile new file +10 -0

+
+	/* handle the transmission complete validations */
+
+	if (urb->status != 0) {
+		/* print the failure status number for debug */
+		printk(KERN_INFO "URB in failure status\n");
+	} else {
+		dprintk(2, "URB status is successfull\n");
+		dprintk(2, "Actual length transmitted %d\n",
+		       urb->actual_length);

The printk and dprintk statements should probably be converted to dev_info/dev_dbg/... standard macros, or something derived from that.

drivers/net/wireless/libertas/Makefile

Quoted 3 lines

drivers/net/wireless/libertas/Makefile new file +3 -0

+void if_usb_free(struct usb_card_rec *cardp)
+{
+	ENTER();

Do you find the ENTER() and LEAVE() macros really useful? usually, you're better off taking them out...

drivers/net/wireless/libertas/Makefile

Quoted 1 line

drivers/net/wireless/libertas/Makefile new file +1 -0

+	return (-ENOMEM);

	return -ENOMEM;

drivers/net/wireless/libertas/Makefile

Quoted 5 lines

drivers/net/wireless/libertas/Makefile new file +4 -0

+	if (!(rinfo = kmalloc(sizeof(struct read_cb_info), GFP_ATOMIC))) {
+		printk(KERN_ERR "No free read_callback_info\n");
+		goto rx_ret;
+	}

I usually find it more readable to do the assignment first, and have the error check in a separate line.

drivers/net/wireless/libertas/Makefile

Quoted 2 lines

drivers/net/wireless/libertas/Makefile new file +2 -0

+struct BootCMDRespStr bootcmdresp;
+

This variable is used only inside of one function, so you should be able to just put it on the stack (it's small).

drivers/net/wireless/libertas/Makefile

Quoted 9 lines

drivers/net/wireless/libertas/Makefile new file +9 -0

+/**  
+ *  @brief This function reads of the packet into the upload buff, 
+ *  wake up the main thread and initialise the Rx callack.
+ *  
+ *  @param urb		pointer to struct urb
+ *  @return 	   	N/A
+ */
+static void if_usb_receive(struct urb *urb)
+{

This function is a little long, there is a switch() statement in it that can probably be converted into one function per case.

drivers/net/wireless/libertas/Makefile

Quoted 10 lines

drivers/net/wireless/libertas/Makefile new file +10 -0

+/** 
+ *  @brief Given a usb_card_rec return its wlan_private
+ *  @param card		pointer to a usb_card_rec
+ *  @return 	   	pointer to wlan_private
+ */
+wlan_private *libertas_sbi_get_priv(void *card)
+{
+	struct usb_card_rec *cardp = (struct usb_card_rec *)card;
+	return (wlan_private *)cardp->priv;
+}

Don't do explicit casts of void* pointers.

drivers/net/wireless/libertas/Makefile

Quoted 1 line

drivers/net/wireless/libertas/Makefile new file +1 -0

+	data = *((int *)(wrq->u.name + SUBCMD_OFFSET));

You use this weird line in many places, it would be good to make it a helper function.

drivers/net/wireless/libertas/Makefile

Quoted 13 lines

drivers/net/wireless/libertas/Makefile new file +13 -0

+static u8 Is_Command_Allowed_In_PS(u16 Command)
+{
+	int count = sizeof(Commands_Allowed_In_PS)
+	    / sizeof(Commands_Allowed_In_PS[0]);
+	int i;
+
+	for (i = 0; i < count; i++) {
+		if (Command == cpu_to_le16(Commands_Allowed_In_PS[i]))
+			return 1;
+	}
+
+	return 0;
+}

In places where you use variables that are strictly little-endian, it would be nice to use the __le32/__le16 types instead of u32/u16.

drivers/net/wireless/libertas/Makefile

Quoted 8 lines

drivers/net/wireless/libertas/Makefile new file +8 -0

+/** 
+ *  @brief This function handles the command response
+ *  
+ *  @param priv    A pointer to wlan_private structure
+ *  @return 	   WLAN_STATUS_SUCCESS or WLAN_STATUS_FAILURE
+ */
+int libertas_process_rx_command(wlan_private * priv)
+{

This function is incredibly long, to the point where it gets unreadable.

drivers/net/wireless/libertas/Makefile

Quoted 4 lines

drivers/net/wireless/libertas/Makefile new file +4 -0

+static struct dentry *libertas_dir = NULL;
+static const int big_buffer_len = 4096;
+static char big_buffer[4096];
+static DECLARE_MUTEX(big_buffer_sem);

This seems to provide a generalized interface for buffering debugfs files. Have you considered using the existing simple_attribute and seq_file interfaces instead?

Even if they don't work for this, I would consider it cleaner to use get_zeroed_page/free_page instead of the global buffer here.

Quoted 10 lines

drivers/net/wireless/libertas/Makefile new file +10 -0

+static struct file_operations libertas_devinfo_fops = { 
+	.owner = THIS_MODULE,
+	.open = open_file_generic,
+	.write = write_file_dummy,
+	.read = libertas_dev_info,
+};
+
+static struct file_operations libertas_getscantable_fops = { 
+	.owner = THIS_MODULE,
+	.open = open_file_generic,

drivers/net/wireless/libertas/Makefile

Quoted 10 lines

drivers/net/wireless/libertas/Makefile new file +10 -0

+	.write = write_file_dummy,
+	.read = libertas_getscantable,
+};
+
+static struct file_operations libertas_sleepparams_fops = { 
+	.owner = THIS_MODULE,
+	.open = open_file_generic,
+	.write = libertas_sleepparams_write,
+	.read = libertas_sleepparams_read,
+};

I would guess you can better express this with some array, like

#define FOPS(read, write) { \
	.owner = THIS_MODULE, \
	.open = open_file_generic, \
	.read = (read), \
	.write = (write), \
}

struct libertas_debugfs_files {
	char *name;
	int perm;
	struct file_operations fops;
} debugfs_files = {
	{ "devinfo", 0444, FOPS(libertas_dev_info, NULL), },
	{ "getscantable", 0444, FOPS(libertas_getscantable, NULL), },
	{ "sleepparams", 0644, FOPS(libertas_sleepparams_read,
					libertas_sleepparams_write), },
	...
};

drivers/net/wireless/libertas/Makefile

Quoted 13 lines

drivers/net/wireless/libertas/Makefile new file +13 -0

+void libertas_debugfs_init_one(wlan_private *priv, struct net_device *dev)
+{
+	if (!libertas_dir)
+		goto exit;
+
+	priv->debugfs_dir = debugfs_create_dir(dev->name, libertas_dir);
+	if (!priv->debugfs_dir)
+		goto exit;
+
+	priv->debugfs_devinfo = debugfs_create_file("info", 0444,
+						    priv->debugfs_dir,
+						    priv,
+						    &libertas_devinfo_fops);

And then here do a loop over that array.

Quoted 23 lines

drivers/net/wireless/libertas/Makefile new file +23 -0

+/** Double-Word(32Bit) Bit definition */
+#define DW_BIT_0	0x00000001
+#define DW_BIT_1	0x00000002
+#define DW_BIT_2	0x00000004
+#define DW_BIT_3	0x00000008
+#define DW_BIT_4	0x00000010
+#define DW_BIT_5	0x00000020
+#define DW_BIT_6	0x00000040
+#define DW_BIT_7	0x00000080
+#define DW_BIT_8	0x00000100
+#define DW_BIT_9	0x00000200
+#define DW_BIT_10	0x00000400
+#define DW_BIT_11       0x00000800
+#define DW_BIT_12       0x00001000
+#define DW_BIT_13       0x00002000
+#define DW_BIT_14       0x00004000
+#define DW_BIT_15       0x00008000
+#define DW_BIT_16       0x00010000
+#define DW_BIT_17       0x00020000
+#define DW_BIT_18       0x00040000
+#define DW_BIT_19       0x00080000
+#define DW_BIT_20       0x00100000
+#define DW_BIT_21       0x00200000

drivers/net/wireless/libertas/Makefile

Quoted 10 lines

drivers/net/wireless/libertas/Makefile new file +10 -0

+#define DW_BIT_22       0x00400000
+#define DW_BIT_23       0x00800000
+#define DW_BIT_24       0x01000000
+#define DW_BIT_25       0x02000000
+#define DW_BIT_26       0x04000000
+#define DW_BIT_27       0x08000000
+#define DW_BIT_28       0x10000000
+#define DW_BIT_29       0x20000000
+#define DW_BIT_30	0x40000000
+#define DW_BIT_31	0x80000000

These should go away.

Quoted 22 lines

drivers/net/wireless/libertas/Makefile new file +22 -0

+
+/** Word (16bit) Bit Definition*/
+#define W_BIT_0		0x0001
+#define W_BIT_1		0x0002
+#define W_BIT_2		0x0004
+#define W_BIT_3		0x0008
+#define W_BIT_4		0x0010
+#define W_BIT_5		0x0020
+#define W_BIT_6		0x0040
+#define W_BIT_7		0x0080
+#define W_BIT_8		0x0100
+#define W_BIT_9		0x0200
+#define W_BIT_10	0x0400
+#define W_BIT_11	0x0800
+#define W_BIT_12	0x1000
+#define W_BIT_13	0x2000
+#define W_BIT_14	0x4000
+#define W_BIT_15	0x8000
+
+/** Byte (8Bit) Bit definition*/
+#define B_BIT_0		0x01
+#define B_BIT_1		0x02

drivers/net/wireless/libertas/Makefile

Quoted 6 lines

drivers/net/wireless/libertas/Makefile new file +6 -0

+#define B_BIT_2		0x04
+#define B_BIT_3		0x08
+#define B_BIT_4		0x10
+#define B_BIT_5		0x20
+#define B_BIT_6		0x40
+#define B_BIT_7		0x80

These as well.

drivers/net/wireless/libertas/Makefile

Quoted 8 lines

drivers/net/wireless/libertas/Makefile new file +8 -0

+extern unsigned int libertas_debug;
+
+/** Debug Macro definition*/
+#ifdef	DEBUG
+#define dprintk(level,format, args...)	if (libertas_debug >= level) printk(KERN_INFO format, ##args)
+#else
+#define dprintk(format, args...)	do {} while (0)
+#endif

dev_dbg should be enough, if you want it dynamic, you can use netif_msg_* together with dev_info.

drivers/net/wireless/libertas/Makefile

Quoted 6 lines

drivers/net/wireless/libertas/Makefile new file +6 -0

+#define ASSERT(cond)						\
+do {								\
+	if (!(cond))						\
+		dprintk(1, "ASSERT: %s, %s:%i\n",		\
+		       __FUNCTION__, __FILE__, __LINE__);	\
+} while(0)

just use WARN_ON instead of defining your own ASSERT

drivers/net/wireless/libertas/Makefile

Quoted 4 lines

drivers/net/wireless/libertas/Makefile new file +4 -0

+#define	ENTER()			dprintk(1, "Enter: %s, %s:%i\n", __FUNCTION__, \
+							__FILE__, __LINE__)
+#define	LEAVE()			dprintk(1, "Leave: %s, %s:%i\n", __FUNCTION__, \
+							__FILE__, __LINE__)

As mentioned, these should probably just be removed

Quoted 9 lines

drivers/net/wireless/libertas/Makefile new file +9 -0

+
+#if defined(DEBUG)
+static inline void HEXDUMP(char *prompt, u8 * buf, int len)
+{
+	int i = 0;
+
+	if (!libertas_debug)
+		return;
+

drivers/net/wireless/libertas/Makefile

Quoted 10 lines

drivers/net/wireless/libertas/Makefile new file +10 -0

+	printk(KERN_DEBUG "%s: ", prompt);
+	for (i = 1; i <= len; i++) {
+		printk(KERN_DEBUG "%02x ", (u8) * buf);
+		buf++;
+	}
+	printk("\n");
+}
+#else
+#define HEXDUMP(x,y,z)
+#endif

The second definition should be an empty inline function so you don't break for code like

if (condition)

	HEXDUMP(x,y,z);
else
	something_else();

maybe rename to dev_dbg_hex() and move to the same place as the generic dev_dbg function.

drivers/net/wireless/libertas/Makefile

Quoted 3 lines

drivers/net/wireless/libertas/Makefile new file +3 -0

+#ifndef NELEMENTS
+#define NELEMENTS(x) (sizeof(x)/sizeof(x[0]))
+#endif

array_size()

Quoted 4 lines

drivers/net/wireless/libertas/Makefile new file +4 -0

+static int wlan_setup_station_hw(wlan_private * priv)
+{
+	int ret = WLAN_STATUS_FAILURE;
+	wlan_adapter *adapter = priv->adapter;

drivers/net/wireless/libertas/Makefile

Quoted 10 lines

drivers/net/wireless/libertas/Makefile new file +10 -0

+
+	ENTER();
+
+	if ((ret = request_firmware(&priv->firmware, libertas_fw_name,
+				    priv->hotplug_device)) < 0) {
+		printk(KERN_ERR "request_firmware() failed, error code = %#x\n",
+		       ret);
+		printk(KERN_ERR "%s not found in /lib/firmware\n", libertas_fw_name);
+		goto done;
+	}

Since we now have an Open Firmware based system, the libertas firmware could be stored inside of the firmware device tree. The most significant advantage of that is that you can boot with the libertas driver enabled before mounting the file system.

Another advantage is that if we get a forth implementation of the driver that can do PXE boot, the open firmware can use the same copy of the libertas firmware blob as Linux.

drivers/net/wireless/libertas/wlan_ioctl.c line 2

Quoted 8 lines

drivers/net/wireless/libertas/wlan_ioctl.c new file +2 -0

new file mode 100644
index 0000000..0cc852b
@@ -0,0 +1,2814 @@
+/** 
+  * This file contains ioctl functions

The number of private ioctl functions defined in this file (and some other places is a hint that there is still something very wrong with this driver. The best solution I can think of is to throw them all away and add new interface for those that are really needed, but do it in a generic way, in the common wireless extensions for Linux.

Quoted 65 lines

drivers/net/wireless/libertas/wlan_ioctl.c new file +65 -0

+#define WLAN_TX_PWR_DEFAULT		20	/*100mW */
+#define WLAN_TX_PWR_US_DEFAULT		20	/*100mW */
+#define WLAN_TX_PWR_JP_DEFAULT		16	/*50mW */
+#define WLAN_TX_PWR_FR_DEFAULT		20	/*100mW */
+#define WLAN_TX_PWR_EMEA_DEFAULT	20	/*100mW */
+
+/* Format { Channel, Frequency (MHz), MaxTxPower } */
+/* Band: 'B/G', Region: USA FCC/Canada IC */
+static struct chan_freq_power channel_freq_power_US_BG[] = {
+	{1, 2412, WLAN_TX_PWR_US_DEFAULT},
+	{2, 2417, WLAN_TX_PWR_US_DEFAULT},
+	{3, 2422, WLAN_TX_PWR_US_DEFAULT},
+	{4, 2427, WLAN_TX_PWR_US_DEFAULT},
+	{5, 2432, WLAN_TX_PWR_US_DEFAULT},
+	{6, 2437, WLAN_TX_PWR_US_DEFAULT},
+	{7, 2442, WLAN_TX_PWR_US_DEFAULT},
+	{8, 2447, WLAN_TX_PWR_US_DEFAULT},
+	{9, 2452, WLAN_TX_PWR_US_DEFAULT},
+	{10, 2457, WLAN_TX_PWR_US_DEFAULT},
+	{11, 2462, WLAN_TX_PWR_US_DEFAULT}
+};
+
+/* Band: 'B/G', Region: Europe ETSI */
+static struct chan_freq_power channel_freq_power_EU_BG[] = {
+	{1, 2412, WLAN_TX_PWR_EMEA_DEFAULT},
+	{2, 2417, WLAN_TX_PWR_EMEA_DEFAULT},
+	{3, 2422, WLAN_TX_PWR_EMEA_DEFAULT},
+	{4, 2427, WLAN_TX_PWR_EMEA_DEFAULT},
+	{5, 2432, WLAN_TX_PWR_EMEA_DEFAULT},
+	{6, 2437, WLAN_TX_PWR_EMEA_DEFAULT},
+	{7, 2442, WLAN_TX_PWR_EMEA_DEFAULT},
+	{8, 2447, WLAN_TX_PWR_EMEA_DEFAULT},
+	{9, 2452, WLAN_TX_PWR_EMEA_DEFAULT},
+	{10, 2457, WLAN_TX_PWR_EMEA_DEFAULT},
+	{11, 2462, WLAN_TX_PWR_EMEA_DEFAULT},
+	{12, 2467, WLAN_TX_PWR_EMEA_DEFAULT},
+	{13, 2472, WLAN_TX_PWR_EMEA_DEFAULT}
+};
+
+/* Band: 'B/G', Region: Spain */
+static struct chan_freq_power channel_freq_power_SPN_BG[] = {
+	{10, 2457, WLAN_TX_PWR_DEFAULT},
+	{11, 2462, WLAN_TX_PWR_DEFAULT}
+};
+
+/* Band: 'B/G', Region: France */
+static struct chan_freq_power channel_freq_power_FR_BG[] = {
+	{10, 2457, WLAN_TX_PWR_FR_DEFAULT},
+	{11, 2462, WLAN_TX_PWR_FR_DEFAULT},
+	{12, 2467, WLAN_TX_PWR_FR_DEFAULT},
+	{13, 2472, WLAN_TX_PWR_FR_DEFAULT}
+};
+
+/* Band: 'B/G', Region: Japan */
+static struct chan_freq_power channel_freq_power_JPN_BG[] = {
+	{1, 2412, WLAN_TX_PWR_JP_DEFAULT},
+	{2, 2417, WLAN_TX_PWR_JP_DEFAULT},
+	{3, 2422, WLAN_TX_PWR_JP_DEFAULT},
+	{4, 2427, WLAN_TX_PWR_JP_DEFAULT},
+	{5, 2432, WLAN_TX_PWR_JP_DEFAULT},
+	{6, 2437, WLAN_TX_PWR_JP_DEFAULT},
+	{7, 2442, WLAN_TX_PWR_JP_DEFAULT},
+	{8, 2447, WLAN_TX_PWR_JP_DEFAULT},
+	{9, 2452, WLAN_TX_PWR_JP_DEFAULT},
+	{10, 2457, WLAN_TX_PWR_JP_DEFAULT},

drivers/net/wireless/libertas/wlan_ioctl.c line 72

Quoted 5 lines

drivers/net/wireless/libertas/wlan_ioctl.c new file +5 -0

+	{11, 2462, WLAN_TX_PWR_JP_DEFAULT},
+	{12, 2467, WLAN_TX_PWR_JP_DEFAULT},
+	{13, 2472, WLAN_TX_PWR_JP_DEFAULT},
+	{14, 2484, WLAN_TX_PWR_JP_DEFAULT}
+};

None of this looks libertas specific to me. Is it perhaps already provided by the common code?

Quoted 8 lines · Arnd Bergmann

Review

Quoted 4 lines · Marcelo Tosatti

(quoted diff) +4 -0

+#define	ENTER()			dprintk(1, "Enter: %s, %s:%i\n", __FUNCTION__, \
+							__FILE__, __LINE__)
+#define	LEAVE()			dprintk(1, "Leave: %s, %s:%i\n", __FUNCTION__, \
+							__FILE__, __LINE__)

As mentioned, these should probably just be removed

I disagree, entry/exit points have been shown to be useful in practice to identify firmware problems on field.

Quoted 3 lines

I disagree, entry/exit points have been shown to be useful in practice to identify firmware problems on field.

I'm not too fond of the ENTER/LEAVE stuff either.  But, I do sympathize that they _can_ be useful in certain circumstances/workflows/whatever.

Is there an official "party line" on this documented somewhere (i.e. CodingStyle or elsewhere)?  A quick search doesn't reveal one to me.

Quoted 1 line

to identify firmware problems on field.

I'm not too fond of the ENTER/LEAVE stuff either. But, I do sympathize that they _can_ be useful in certain circumstances/workflows/whatever.

Is there an official "party line" on this documented somewhere (i.e. CodingStyle or elsewhere)? A quick search doesn't reveal one to me.

Hi Arnd,

Quoted 2 lines

Most of these are unused as well, I guess it would be good to go through the whole list and see what can be removed.

Done. Will go through the entire file list before posting the next patch.

Quoted 1 line

Did you run the file through a recent sparse successfully?

Spits a bunch of warnings, looking at them.

Quoted 3 lines

The printk and dprintk statements should probably be converted to dev_info/dev_dbg/... standard macros, or something derived from that.

Quoted 7 lines

Review

Quoted 4 lines

(quoted diff) +3 -0

+void if_usb_free(struct usb_card_rec *cardp)
+{
+	ENTER();

Do you find the ENTER() and LEAVE() macros really useful? usually, you're better off taking them out...

Yes, for instance the bug reported at http://dev.laptop.org/ticket/841
could be verified by checking the log file (by confirming that certain states were true/false).

However, only a few such macros, on key functions, are required. I'll go over and remove the unneeded ones.

Review

Quoted 5 lines

(quoted diff) +5 -0

+/** Command Processing States and Options */
+#define HostCmd_STATE_IDLE                    0x0000
+#define HostCmd_STATE_IN_USE_BY_HOST          0x0001
+#define HostCmd_STATE_IN_USE_BY_MINIPORT      0x0002
+#define HostCmd_STATE_FINISHED                0x000f

No SilLYcAps please

Most of these are unused as well, I guess it would be good to go through the whole list and see what can be removed.

Quoted 4 lines

+/**
+ * @brief This function reads of the packet into the upload buff,
+ * wake up the main thread and initialise the Rx callack.
+ *

Review

Quoted 5 lines

+ * @param urb pointer to struct urb
+ * @return N/A
+ */

(quoted diff) +2 -0

+static void if_usb_receive(struct urb *urb)
+{

This function is a little long, there is a switch() statement in it that can probably be converted into one function per case.

Quoted 4 lines

+/**

+ *  @brief Given a usb_card_rec return its wlan_private
+ *  @param card		pointer to a usb_card_rec
+ *  @return 	   	pointer to wlan_private

Review

Quoted 6 lines

+ */

(quoted diff) +5 -0

+wlan_private *libertas_sbi_get_priv(void *card)
+{
+	struct usb_card_rec *cardp = (struct usb_card_rec *)card;
+	return (wlan_private *)cardp->priv;
+}

Don't do explicit casts of void* pointers.

Review

Quoted 1 line

(quoted diff) +1 -0

+	data = *((int *)(wrq->u.name + SUBCMD_OFFSET));

You use this weird line in many places, it would be good to make it a helper function.

Review

Quoted 13 lines

(quoted diff) +13 -0

+static u8 Is_Command_Allowed_In_PS(u16 Command)
+{
+	int count = sizeof(Commands_Allowed_In_PS)
+	    / sizeof(Commands_Allowed_In_PS[0]);
+	int i;
+
+	for (i = 0; i < count; i++) {
+		if (Command == cpu_to_le16(Commands_Allowed_In_PS[i]))
+			return 1;
+	}
+
+	return 0;
+}

In places where you use variables that are strictly little-endian, it would be nice to use the __le32/__le16 types instead of u32/u16.

Review

Quoted 8 lines

(quoted diff) +8 -0

+/** 
+ *  @brief This function handles the command response
+ *  
+ *  @param priv    A pointer to wlan_private structure
+ *  @return 	   WLAN_STATUS_SUCCESS or WLAN_STATUS_FAILURE
+ */
+int libertas_process_rx_command(wlan_private * priv)
+{

This function is incredibly long, to the point where it gets unreadable.

Review

Quoted 4 lines

(quoted diff) +4 -0

+static struct dentry *libertas_dir = NULL;
+static const int big_buffer_len = 4096;
+static char big_buffer[4096];
+static DECLARE_MUTEX(big_buffer_sem);

This seems to provide a generalized interface for buffering debugfs files. Have you considered using the existing simple_attribute and seq_file interfaces instead?

Even if they don't work for this, I would consider it cleaner to use get_zeroed_page/free_page instead of the global buffer here.

Quoted 10 lines

(quoted diff) +10 -0

+static struct file_operations libertas_devinfo_fops = { 
+	.owner = THIS_MODULE,
+	.open = open_file_generic,
+	.write = write_file_dummy,
+	.read = libertas_dev_info,
+};
+
+static struct file_operations libertas_getscantable_fops = { 
+	.owner = THIS_MODULE,
+	.open = open_file_generic,

Review

Quoted 10 lines

(quoted diff) +10 -0

+	.write = write_file_dummy,
+	.read = libertas_getscantable,
+};
+
+static struct file_operations libertas_sleepparams_fops = { 
+	.owner = THIS_MODULE,
+	.open = open_file_generic,
+	.write = libertas_sleepparams_write,
+	.read = libertas_sleepparams_read,
+};

I would guess you can better express this with some array, like

#define FOPS(read, write) { \
	.owner = THIS_MODULE, \
	.open = open_file_generic, \
	.read = (read), \
	.write = (write), \
}

struct libertas_debugfs_files {
	char *name;
	int perm;
	struct file_operations fops;
} debugfs_files = {
	{ "devinfo", 0444, FOPS(libertas_dev_info, NULL), },
	{ "getscantable", 0444, FOPS(libertas_getscantable, NULL), },
	{ "sleepparams", 0644, FOPS(libertas_sleepparams_read,
					libertas_sleepparams_write), },
	...
};

Review

Quoted 13 lines

(quoted diff) +13 -0

+void libertas_debugfs_init_one(wlan_private *priv, struct net_device *dev)
+{
+	if (!libertas_dir)
+		goto exit;
+
+	priv->debugfs_dir = debugfs_create_dir(dev->name, libertas_dir);
+	if (!priv->debugfs_dir)
+		goto exit;
+
+	priv->debugfs_devinfo = debugfs_create_file("info", 0444,
+						    priv->debugfs_dir,
+						    priv,
+						    &libertas_devinfo_fops);

And then here do a loop over that array.

Quoted 4 lines · Dan Williams

The 8388 architecture is typical of a thick firmware architecture, where the firmware handles all 802.11 MAC management tasks. The host driver downloads standard 802.3 frames to the firmware to be transmitted over the link as 802.11 frames.

Ah. This isn't clear from the driver as it's too big to go through in detail. In particular, handing 802.3 frames to the firmware means you cannot use d80211 (nor ieee80211).

Quoted 2 lines

I thought these 2 things were essentially _the_ definition of fullmac, please correct me if I'm wrong.

I think you're right.

Quoted 5 lines

Perhaps I just don't understand how flexible d80211 has become; when we last were talking about it 8 months ago, it appears that it could not handle parts that did significant pieces of work in firmware, like the 8388 does. Do we have the functionality in d80211 yet to handle pieces that are a full mix of hybrid full/soft mac?

No.

Quoted 5 lines

[...]
Not quite; the driver doesn't have to deal with the Open System handshake or the Shared Key handshake. Instead of Airo's 1-step process, this is a simple 2-step process. Look at wlan_join.c, wlan_associate(). That's what happens; nothing more.

If everything is as simple as you wrote, why is the driver so incredibly big? It's almost impossible to review it in a reasonable time.

Quoted 2 lines · Dan Williams

But could we please be constructive here, and actually take a look at the driver and point out problems?

Just from a quick glance:

- drivers/net/wireless/libertas/radiotap.h file with a lot of constants already defined elsewhere (and not respecting kernel coding style, either)

- regulatory domain management (already in ieee80211, in progress for d80211)
- the full authentication and association state machine (or am I understanding it wrong?)

And I'm omitting things like

Quoted 25 lines

(quoted diff) +25 -0

+/** Double-Word(32Bit) Bit definition */
+#define DW_BIT_0	0x00000001
+#define DW_BIT_1	0x00000002
+#define DW_BIT_2	0x00000004
+#define DW_BIT_3	0x00000008
+#define DW_BIT_4	0x00000010
+#define DW_BIT_5	0x00000020
+#define DW_BIT_6	0x00000040
+#define DW_BIT_7	0x00000080
+#define DW_BIT_8	0x00000100
+#define DW_BIT_9	0x00000200
+#define DW_BIT_10	0x00000400
+#define DW_BIT_11       0x00000800
+#define DW_BIT_12       0x00001000
+#define DW_BIT_13       0x00002000
+#define DW_BIT_14       0x00004000
+#define DW_BIT_15       0x00008000
+#define DW_BIT_16       0x00010000
+#define DW_BIT_17       0x00020000
+#define DW_BIT_18       0x00040000
+#define DW_BIT_19       0x00080000
+#define DW_BIT_20       0x00100000
+#define DW_BIT_21       0x00200000
+#define DW_BIT_22       0x00400000
+#define DW_BIT_23       0x00800000

Review

Quoted 8 lines

(quoted diff) +8 -0

+#define DW_BIT_24       0x01000000
+#define DW_BIT_25       0x02000000
+#define DW_BIT_26       0x04000000
+#define DW_BIT_27       0x08000000
+#define DW_BIT_28       0x10000000
+#define DW_BIT_29       0x20000000
+#define DW_BIT_30	0x40000000
+#define DW_BIT_31	0x80000000

or those ENTER and LEAVE statements in each other function,

non-conforming coding style,

comments like "All rights reserved" or
"This software file (the "File") is distributed by Marvell International Ltd.",

a lot of duplicated code, etc.

I stopped looking t it in the half, asorry.