Re: [PATCH] Marvell Libertas 8388 802.11b/g USB driver (v2)

Re: [PATCH] Marvell Libertas 8388 802.11b/g USB driver (v2)

[Marcelo Tosatti]

Got dropped somehow...

----- Forwarded message from Marcelo Tosatti <marcelo@kvack.org> -----

Date: Thu, 18 Jan 2007 13:43:51 -0200
To: Jiri Benc <jbenc@suse.cz>
Cc: Dan Williams <dcbw@redhat.com>,
	Johannes Berg <johannes@sipsolutions.net>,
	Marcelo Tosatti <marcelo@kvack.org>,
	netdev <netdev@vger.kernel.org>, Jeff Garzik <jgarzik@pobox.com>,
	"John W. Linville" <linville@redhat.com>,
	"Luis R. Rodriguez" <mcgrof@gmail.com>,
	Arnd Bergmann <arnd@arndb.de>,
	Arnaldo Carvalho de Melo <acme@ghostprotocols.net>
In-Reply-To: <20070117190726.13aaf245@griffin.suse.cz>
Subject: Re: [PATCH] Marvell Libertas 8388 802.11b/g USB driver (v2)

Hi Jiri,

On Wed, Jan 17, 2007 at 07:07:26PM +0100, Jiri Benc wrote:
> On Wed, 17 Jan 2007 12:43:08 -0500, Dan Williams wrote:
> > 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).
> 
> > I thought these 2 things were essentially _the_ definition of fullmac,
> > please correct me if I'm wrong.
> 
> I think you're right.
> 
> > 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.
> 
> > [...]
> > 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.

Most notably because the firmware provides a vast number of
configuration commands, and all of them are supported by the driver.

For instance, it supports sleep mode, which makes normal operation way
more complex than if it didnt (the driver queue's packets internally to
hand them out once the awake window has been opened).

----- End forwarded message -----

Re: [PATCH] Marvell Libertas 8388 802.11b/g USB driver (v2)

[Marcelo Tosatti]

Resending, apparently the first message got dropped somehow...

From: Marcelo Tosatti <marcelo@kvack.org>
Date: Thu, 18 Jan 2007 13:02:13 -0200
To: Jiri Benc <jbenc@suse.cz>
Cc: Dan Williams <dcbw@redhat.com>, Marcelo Tosatti <marcelo@kvack.org>,
	netdev <netdev@vger.kernel.org>, Jeff Garzik <jgarzik@pobox.com>,
	"John W. Linville" <linville@redhat.com>,
	"Luis R. Rodriguez" <mcgrof@gmail.com>,
	Arnd Bergmann <arnd@arndb.de>,
	Arnaldo Carvalho de Melo <acme@ghostprotocols.net>
In-Reply-To: <20070117164328.47ab60da@griffin.suse.cz>
Subject: Re: [PATCH] Marvell Libertas 8388 802.11b/g USB driver (v2)

Hi Jiri,

On Wed, Jan 17, 2007 at 04:43:28PM +0100, Jiri Benc wrote:
> On Wed, 17 Jan 2007 10:01:12 -0500, Dan Williams wrote:
> > 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)

Such as? The bit definitions (IEEE80211_RADIOTAP_F_{TX,RX}...) have been
moved to include/net/ieee80211_radiotap.h in the second version of the
patch. Is that what you were referring to?

As for kernel coding style in that file, can you specify what looks
wrong? It appears alright to me.

> - 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
> 
> > +/** 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
> > +#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

Removed.

> or those ENTER and LEAVE statements in each other function,

What is the problem with it? The entry/exit debug points have been
pretty useful for debugging. A bunch of in-tree drivers contain similar
code.

You might argue that ENTER/LEAVE should be in lower caps?

> non-conforming coding style, 

Send patches or point them out? :) 

I've ran scripts/Lindent over it, but there might still be some sections
in need of love.

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

Removed them all, moved the Marvell copyright + GNU header to LICENSE
file.

> a lot of duplicated code, etc. 

Again, please point it out.

> I stopped looking t it in the half, asorry.

Thanks for your comments so far!

----- End forwarded message -----

Re: [PATCH] Marvell Libertas 8388 802.11b/g USB driver (v2)

[Marcelo Tosatti]

Hi Jiri,

On Sat, Jan 20, 2007 at 06:15:54PM -0200, Marcelo Tosatti wrote:
> On Wed, Jan 17, 2007 at 04:43:28PM +0100, Jiri Benc wrote:
> > On Wed, 17 Jan 2007 10:01:12 -0500, Dan Williams wrote:
> > > 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)
> 
> Such as? The bit definitions (IEEE80211_RADIOTAP_F_{TX,RX}...) have been
> moved to include/net/ieee80211_radiotap.h in the second version of the
> patch. Is that what you were referring to?

I also converted the last piece of code which could be shared from
generic Linux code in radiotap.h, which is "struct ieee80211_hdr_4addr".

Can you go over it again and point any problems you might see? 

http://git.infradead.org/?p=libertas-2.6.git;a=blob;h=5d118f40cfbc43206369ec77082e220822318f11;hb=6a2277e41e4fd69e4f33cc7abc574aca8bca8abe;f=drivers/net/wireless/libertas/radiotap.h

> As for kernel coding style in that file, can you specify what looks
> wrong? It appears alright to me.

Again, coding style looks fine to me.

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

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

[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.

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

Changes since v1:

- reset usb device if boot2 init command fails
- resend initial boot command in case of non-response
- inform which command is being resent due to timeout
- do not evaluate boot command response if boot2 < v3106
- allocate bulk_out_buffer with GFP_KERNEL instead of GFP_DMA
- add event capability reporting
- remove "@file" and "@brief" headers from beginning of files
- remove now obsolete comments about driver building from README
- remove unused Makefile.old
- remove -DUPDATE_BOOT2_BY_MFG from Makefile
- fix typo and add "8388" to Kconfig entry
- remove unecessary commentary from if_bootcmd.c
- remove unecessary static attr of "struct HostCmd_DS_MESH_ACCESS" instances
- remove SLEEP_PERIOD command support code (not implemented in fw)
- hexdump should use KERN_DEBUG
- disable debugging output by default
- add might_sleep() to wait command response path
- Don't sleep inside get_wireless_stats
- version bump correction (320p0 instead of 321p0)
- destroy association worker in wlan_add_card EH path
- proper pending command accounting
- move radiotap definitions to include/net/ieee80211_radiotap.h
- added support for MPP activation/deactivation through sysfs
- remove pointer to dev structure on libertas_devs in card removal

Re: [PATCH] Marvell Libertas 8388 802.11b/g USB driver (v2)

[Arkadiusz Miskiewicz]

On Tuesday 16 January 2007 19:55, you wrote:
> 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)

?

-- 
Arkadiusz Miśkiewicz        PLD/Linux Team
arekm / maven.pl            http://ftp.pld-linux.org/

Re: [PATCH] Marvell Libertas 8388 802.11b/g USB driver (v2)

[Dan Williams]

On Tue, 2007-01-16 at 20:32 +0100, Arkadiusz Miskiewicz wrote:
> On Tuesday 16 January 2007 19:55, you wrote:
> > 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)

Not that close as far as we know; the 8388 and the 8338 are different
product groups within Marvell and different chips.  But we can probably
find out.  They seem to like naming different hardware with similar
product numbers.

dan

Re: [PATCH] Marvell Libertas 8388 802.11b/g USB driver (v2)

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 580 bytes --]

On Tue, 2007-01-16 at 16:55 -0200, Marcelo Tosatti wrote:

> _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.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 190 bytes --]

Re: [PATCH] Marvell Libertas 8388 802.11b/g USB driver (v2)

[Marcelo Tosatti]

On Wed, Jan 17, 2007 at 07:52:13AM +0000, Johannes Berg wrote:
> On Tue, 2007-01-16 at 16:55 -0200, Marcelo Tosatti wrote:
> 
> > _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.

How come is it a generic wireless stack? As Dan mentioned, the driver is
large, but the wireless stack is _inside_ the firmware.

It does not implement a wireless stack. Its simply an interface to the
firmware.

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).

Quoting Dan Williams, who quoted the device documentation:

"This basic architecture is typical of a thick firmware architecture,
where the WLAN firmware handles all 802.11 MAC Management tasks."

"The host driver downloads standard 802.3 frames to the firmware to
transmit over the wireless link as 802.11 frames."

So it really looks like a fullmac chip, although it might require higher
interaction from the driver part, in comparison with IPW.

Its also important to note, regarding the size in LOC, that the driver
implements most of the firmware/chip configuration knobs, and there are
a _lot_ of them, unlike smaller in-tree drivers. (ie. the driver is
highly configurable). See the README file for a list.

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

Please go ahead and comment on all minor issues you see so we can fix
them.

Re: [PATCH] Marvell Libertas 8388 802.11b/g USB driver (v2)

[Christoph Hellwig]

On Wed, Jan 17, 2007 at 04:42:04PM -0200, Marcelo Tosatti wrote:
> 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.

Re: [PATCH] Marvell Libertas 8388 802.11b/g USB driver (v2)

[Dan Williams]

On Wed, 2007-01-17 at 23:06 +0000, Christoph Hellwig wrote:
> On Wed, Jan 17, 2007 at 04:42:04PM -0200, Marcelo Tosatti wrote:
> > 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.

Dan

Re: [PATCH] Marvell Libertas 8388 802.11b/g USB driver (v2)

[Christoph Hellwig]

On Thu, Jan 18, 2007 at 10:41:45AM -0500, Dan Williams wrote:
> 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.

Re: [PATCH] Marvell Libertas 8388 802.11b/g USB driver (v2)

[Jon Smirl]

On 1/18/07, Christoph Hellwig <hch@infradead.org> wrote:
> On Thu, Jan 18, 2007 at 10:41:45AM -0500, Dan Williams wrote:
> > 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?

-- 
Jon Smirl
jonsmirl@gmail.com

Re: [PATCH] Marvell Libertas 8388 802.11b/g USB driver (v2)

[Dan Williams]

On Thu, 2007-01-18 at 17:54 -0500, Jon Smirl wrote:
> On 1/18/07, Christoph Hellwig <hch@infradead.org> wrote:
> > On Thu, Jan 18, 2007 at 10:41:45AM -0500, Dan Williams wrote:
> > > 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?

I believe Javier Cordona (who will also be at the Linux Wireless Summit
this weekend) is going to do a d80211-based implementation alongside the
Libertas 8388 firmware and driver bits too.  802.11s networking in Linux
is still quite immature, and we need to get people interested in a
standard stack talking to each other.

Dan

Re: [PATCH] Marvell Libertas 8388 802.11b/g USB driver (v2)

[Dan Williams]

On Thu, 2007-01-18 at 22:40 +0000, Christoph Hellwig wrote:
> On Thu, Jan 18, 2007 at 10:41:45AM -0500, Dan Williams wrote:
> > 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.

Of course; the we-need-it-now-so-you-take-it-now attitude never gets
vendors or anyone else anywhere, so there's no reason to expect it would
work for OLPC either.  OLPC does not need, nor does any of us who work
on it ask for, special treatment.  Stuff will go through the correct
channels and will follow the normal kernel process.  That's all we can
expect, and that's all we ask.

Cheers,
Dan

Re: [PATCH] Marvell Libertas 8388 802.11b/g USB driver (v2)

[Jiri Benc]

On Tue, 16 Jan 2007 16:55:24 -0200, Marcelo Tosatti wrote:
> 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.

 Jiri

-- 
Jiri Benc
SUSE Labs

Re: [PATCH] Marvell Libertas 8388 802.11b/g USB driver (v2)

[Dan Williams]

On Wed, 2007-01-17 at 14:11 +0100, Jiri Benc wrote:
> On Tue, 16 Jan 2007 16:55:24 -0200, Marcelo Tosatti wrote:
> > 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.

There certainly isn't; with all the whining I've been doing over the
past two years about unified interfaces, we're not going to let a driver
through with Yet Another 802.11 Stack.  Sheesh, at least look at the
thing first.

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?

Cheers,
Dan

[1] I've done a fair amount of work on this driver along with Marcelo

Re: [PATCH] Marvell Libertas 8388 802.11b/g USB driver (v2)

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 1522 bytes --]

On Wed, 2007-01-17 at 10:01 -0500, Dan Williams wrote:

> 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.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 190 bytes --]

Re: [PATCH] Marvell Libertas 8388 802.11b/g USB driver (v2)

[Dan Williams]

On Wed, 2007-01-17 at 15:18 +0000, Johannes Berg wrote:
> On Wed, 2007-01-17 at 10:01 -0500, Dan Williams wrote:
> 
> > 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.

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.

> 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.

> 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.

> 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

Re: [PATCH] Marvell Libertas 8388 802.11b/g USB driver (v2)

[Dan Williams]

On Wed, 2007-01-17 at 12:43 -0500, Dan Williams wrote:
> On Wed, 2007-01-17 at 15:18 +0000, Johannes Berg wrote:
> > On Wed, 2007-01-17 at 10:01 -0500, Dan Williams wrote:
> > 
> > > 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.

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*.

You certainly cannot do that with even a somewhat softmac part that has
the processing and management functions running on the host CPU.  Which
is one big reason Atheros-based parts were out of the question, not to
mention the binary HAL junk.

Dan


> 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.
> 
> > 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.
> 
> > 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.
> 
> > 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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Marvell Libertas 8388 802.11b/g USB driver (v2)

[Christoph Hellwig]

On Wed, Jan 17, 2007 at 01:00:47PM -0500, Dan Williams wrote:
> 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.

Re: [PATCH] Marvell Libertas 8388 802.11b/g USB driver (v2)

[Jeff Garzik]

Christoph Hellwig wrote:
> On Wed, Jan 17, 2007 at 01:00:47PM -0500, Dan Williams wrote:
>> 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.

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.

	Jeff

Re: [PATCH] Marvell Libertas 8388 802.11b/g USB driver (v2)

[John W. Linville]

On Wed, Jan 17, 2007 at 06:19:07PM -0500, Jeff Garzik wrote:
> Christoph Hellwig wrote:
> >On Wed, Jan 17, 2007 at 01:00:47PM -0500, Dan Williams wrote:

> >>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.
> 
> 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?

John
-- 
John W. Linville
linville@tuxdriver.com

Re: [PATCH] Marvell Libertas 8388 802.11b/g USB driver (v2)

[Marcelo Tosatti]

On Thu, Jan 18, 2007 at 03:22:50AM -0500, John W. Linville wrote:
> On Wed, Jan 17, 2007 at 06:19:07PM -0500, Jeff Garzik wrote:
> > Christoph Hellwig wrote:
> > >On Wed, Jan 17, 2007 at 01:00:47PM -0500, Dan Williams wrote:
> 
> > >>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.
> > 
> > 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?

Re: [PATCH] Marvell Libertas 8388 802.11b/g USB driver (v2)

[Dan Williams]

On Wed, 2007-01-24 at 13:26 -0200, Marcelo Tosatti wrote:
> On Thu, Jan 18, 2007 at 03:22:50AM -0500, John W. Linville wrote:
> > On Wed, Jan 17, 2007 at 06:19:07PM -0500, Jeff Garzik wrote:
> > > Christoph Hellwig wrote:
> > > >On Wed, Jan 17, 2007 at 01:00:47PM -0500, Dan Williams wrote:
> > 
> > > >>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.
> > > 
> > > 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.

Dan

Re: [PATCH] Marvell Libertas 8388 802.11b/g USB driver (v2)

[John W. Linville]

On Wed, Jan 24, 2007 at 01:52:35PM -0500, Dan Williams wrote:

> 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.

ACK

-- 
John W. Linville
linville@tuxdriver.com

Re: [PATCH] Marvell Libertas 8388 802.11b/g USB driver (v2)

[Dan Williams]

On Wed, 2007-01-17 at 23:09 +0000, Christoph Hellwig wrote:
> On Wed, Jan 17, 2007 at 01:00:47PM -0500, Dan Williams wrote:
> > 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.

Nor are we going to try to push highly OLPC-specific changes into this
driver in a non-modular manner.  This chip is also used in, for example,
the X-Box 360 wireless dongle, and other non-OLPC people are working on
adding support for the 8385 SDIO/CF variant.

It would be arrogant of us to think that the driver would _just_ be
useful for OLPC, and that's why nobody ever said that it was going to be
an OLPC specific driver, not I, nor Marcelo.  Give us some credit here
before you start jumping all over something neither of us have said or
implied.

If we do make any OLPC specific changes (there are currently none [1]),
they will be properly abstracted, conditionalized, and/or generalized.
We will not be throwing random crap into this driver.

Cheers,
Dan

[1] The code for the 802.11s mesh interface is in the driver, but that
will/should automatically turn itself off if the firmware doesn't
support that functionality.  Furthermore, none of the mesh bits are
OLPC-specific and may be used with any platform on which the driver
runs.

Re: [PATCH] Marvell Libertas 8388 802.11b/g USB driver (v2)

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 2714 bytes --]

On Wed, 2007-01-17 at 12:43 -0500, Dan Williams wrote:

> 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.

> 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 .

> 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.

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

Can we rephrase this in terms of the MLME interface etc that the 802.11
standard speaks about? That'd be easier I think.

> 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.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 190 bytes --]

Re: [PATCH] Marvell Libertas 8388 802.11b/g USB driver (v2)

[Marcelo Tosatti]

On Wed, Jan 17, 2007 at 06:01:24PM +0000, Johannes Berg wrote:
> On Wed, 2007-01-17 at 12:43 -0500, Dan Williams wrote:
> 
> > 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.
> 
> > 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 .
> 
> > 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.

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?

Re: [PATCH] Marvell Libertas 8388 802.11b/g USB driver (v2)

[Jon Smirl]

On 1/22/07, Marcelo Tosatti <marcelo@kvack.org> wrote:
> On Wed, Jan 17, 2007 at 06:01:24PM +0000, Johannes Berg wrote:
> > On Wed, 2007-01-17 at 12:43 -0500, Dan Williams wrote:
> >
> > > 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.
> >
> > > 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 .
> >
> > > 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.
>
> So using the d80211 stack is completly out of question?

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.

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

-- 
Jon Smirl
jonsmirl@gmail.com

Re: [PATCH] Marvell Libertas 8388 802.11b/g USB driver (v2)

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 864 bytes --]

On Mon, 2007-01-22 at 10:20 -0500, Jon Smirl wrote:

> What is the general solution for 802.11s?

None yet.

> 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.

> 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.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 190 bytes --]

Re: [PATCH] Marvell Libertas 8388 802.11b/g USB driver (v2)

[Jon Smirl]

On 1/23/07, Johannes Berg <johannes@sipsolutions.net> wrote:
> On Mon, 2007-01-22 at 10:20 -0500, Jon Smirl wrote:
>
> > What is the general solution for 802.11s?
>
> None yet.
>
> > 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.

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.

> > 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.

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.

-- 
Jon Smirl
jonsmirl@gmail.com

Re: [PATCH] Marvell Libertas 8388 802.11b/g USB driver (v2)

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 752 bytes --]

On Tue, 2007-01-23 at 11:18 -0500, Jon Smirl wrote:

> 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.

> 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.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 190 bytes --]

Re: [PATCH] Marvell Libertas 8388 802.11b/g USB driver (v2)

[Jon Smirl]

On 1/23/07, Johannes Berg <johannes@sipsolutions.net> wrote:
> On Tue, 2007-01-23 at 11:18 -0500, Jon Smirl wrote:
>
> > 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.
>
> > 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?

-- 
Jon Smirl
jonsmirl@gmail.com

Re: [PATCH] Marvell Libertas 8388 802.11b/g USB driver (v2)

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 269 bytes --]

On Tue, 2007-01-23 at 12:14 -0500, Jon Smirl wrote:

> 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'm pretty sure bcm43xx would need firmware changes.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 190 bytes --]

Re: [PATCH] Marvell Libertas 8388 802.11b/g USB driver (v2)

[Dan Williams]

On Tue, 2007-01-23 at 12:14 -0500, Jon Smirl wrote:
> On 1/23/07, Johannes Berg <johannes@sipsolutions.net> wrote:
> > On Tue, 2007-01-23 at 11:18 -0500, Jon Smirl wrote:
> >
> > > 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.
> >
> > > 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?

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.

Dan

Re: [PATCH] Marvell Libertas 8388 802.11b/g USB driver (v2)

[Jon Smirl]

On 1/23/07, Dan Williams <dcbw@redhat.com> wrote:
> On Tue, 2007-01-23 at 12:14 -0500, Jon Smirl wrote:
> > On 1/23/07, Johannes Berg <johannes@sipsolutions.net> wrote:
> > > On Tue, 2007-01-23 at 11:18 -0500, Jon Smirl wrote:
> > >
> > > > 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.
> > >
> > > > 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?
>
> 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 am working with the zd1211b which is also softmac. I'm willing to do
some work on 11s support for dscape but I need a spec. I'll probably
need some help too since I haven't worked on the wireless code before.

-- 
Jon Smirl
jonsmirl@gmail.com

Re: [PATCH] Marvell Libertas 8388 802.11b/g USB driver (v2)

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 436 bytes --]

On Tue, 2007-01-23 at 12:59 -0500, Dan Williams wrote:

> 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.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 190 bytes --]

Re: [PATCH] Marvell Libertas 8388 802.11b/g USB driver (v2)

[Jon Smirl]

On 1/23/07, Johannes Berg <johannes@sipsolutions.net> wrote:
> On Tue, 2007-01-23 at 12:59 -0500, Dan Williams wrote:
>
> > 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.

Daniel,  would the Zydas employees be interested in supplying us with
a MAC that would support the development of a software based 802.11s
stack?

-- 
Jon Smirl
jonsmirl@gmail.com

Re: [PATCH] Marvell Libertas 8388 802.11b/g USB driver (v2)

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 424 bytes --]

On Tue, 2007-01-23 at 19:30 +0100, Johannes Berg wrote:

> .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.

This assertion might have been premature, it's entirely possible that
the firmware acks those new unicast frames anyway. Best to just inject
them and try, I suppose.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 190 bytes --]

Re: [PATCH] Marvell Libertas 8388 802.11b/g USB driver (v2)

[Marcelo Tosatti]

On Wed, Jan 17, 2007 at 06:01:24PM +0000, Johannes Berg wrote:

> > 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.

Three issues related to command dispatching as is performed in the
current code:

1) First problem is that there are certain commands which are sent      
in interrupt context, or in contexes which can't sleep (such as         
get_wireless_stats).                                                    

We need a mutex to synchronize command download to firmware, and certain
contexes, as mentioned above, can't sleep.

Thus the need for a thread (and the associated switch(cmd_num) and all
of its "unnecessary complexity") to handle command dispatching.

2) Another issue is that during the sleep period certain commands can't
be sent down to firmware and must remain in the command queue until the
awake window has been opened.

3) Having commands go through a unified queue allows the main thread to
know whether it can tell the HW to go back to sleep mode (in case there
are no commands pending), which is not possible (or suboptimal) if each
command is unaware of other commands being sent.

That said I'm not entirely sure that we want commands to be sent
directly to firmware, or an equivalent of that (have an "awake window
opened" waitqueue in which workqueue's handlers on behalf of commands
are triggered).

That sounds even more complex and suboptimal with reference to "ready to
go back to sleep state".

So IMHO the current way command dispatching works is optimal given the
driver support for sending commands from non-sleepable contexes and for
sending commands while the HW is in sleep mode.

Comments?

Follows a previous attempt at modifying command dispatching:

diff --git a/drivers/net/wireless/libertas/hostcmd.h b/drivers/net/wireless/libertas/hostcmd.h
index de01d1b..2e21b10 100644
--- a/drivers/net/wireless/libertas/hostcmd.h
+++ b/drivers/net/wireless/libertas/hostcmd.h
@@ -94,9 +94,6 @@ #if defined(__KERNEL__)
 
 /* CmdCtrlNode */
 struct CmdCtrlNode {
-	/* CMD link list */
-	struct list_head list;
-
 	u32 Status;
 
 	/* CMD ID */
@@ -116,7 +113,9 @@ struct CmdCtrlNode {
 	/* wait queue */
 	u16 CmdWaitQWoken;
 	wait_queue_head_t cmdwait_q;
-} __attribute__ ((packed));
+
+	struct kref kref;
+};
 
 #endif
 
diff --git a/drivers/net/wireless/libertas/if_usb.c b/drivers/net/wireless/libertas/if_usb.c
index 20b70f6..1c47239 100644
--- a/drivers/net/wireless/libertas/if_usb.c
+++ b/drivers/net/wireless/libertas/if_usb.c
@@ -144,6 +144,9 @@ static void if_usb_write_bulk_callback(s
 			netif_wake_queue(dev);
 	}
 
+	if (priv->adapter->PSState != PS_STATE_SLEEP)
+		if_usb_interrupt(priv, 0, TX_SUCCESS);
+
 	LEAVE();
 	return;
 }
diff --git a/drivers/net/wireless/libertas/if_usb.h b/drivers/net/wireless/libertas/if_usb.h
diff --git a/drivers/net/wireless/libertas/sbi.h b/drivers/net/wireless/libertas/sbi.h
diff --git a/drivers/net/wireless/libertas/wlan_cmd.c b/drivers/net/wireless/libertas/wlan_cmd.c
index dd4d0b2..d41e0f6 100644
--- a/drivers/net/wireless/libertas/wlan_cmd.c
+++ b/drivers/net/wireless/libertas/wlan_cmd.c
@@ -35,7 +35,8 @@ Change log:
 	      implementation through generic hostcmd API
 ********************************************************/
 
-#include	"include.h"
+#include "include.h"
+#include <linux/workqueue.h>
 
 /********************************************************
 		Local Variables
@@ -1184,60 +1185,6 @@ static int wlan_cmd_dft_access(wlan_priv
 	return WLAN_STATUS_SUCCESS;
 }
 
-/** 
- *  @brief This function queues the command to cmd list.
- *  
- *  @param Adapter	A pointer to wlan_adapter structure
- *  @param CmdNode   	A pointer to CmdCtrlNode structure
- *  @param addtail	specify if the cmd needs to be queued in the header or tail
- *  @return 	   	n/a
- */
-void QueueCmd(wlan_adapter * Adapter, struct CmdCtrlNode *CmdNode, u8 addtail)
-{
-	ulong flags;
-	struct HostCmd_DS_COMMAND *CmdPtr;
-
-	ENTER();
-
-	if (!CmdNode) {
-		PRINTM(INFO, "QUEUE_CMD: CmdNode is NULL\n");
-		goto done;
-	}
-
-	CmdPtr = (struct HostCmd_DS_COMMAND *)CmdNode->BufVirtualAddr;
-	if (!CmdPtr) {
-		PRINTM(INFO, "QUEUE_CMD: CmdPtr is NULL\n");
-		goto done;
-	}
-
-	/* Exit_PS command needs to be queued in the header always. */
-	if (CmdPtr->Command == HostCmd_CMD_802_11_PS_MODE) {
-		struct HostCmd_DS_802_11_PS_MODE *psm = &CmdPtr->params.psmode;
-		if (psm->Action == HostCmd_SubCmd_Exit_PS) {
-			if (Adapter->PSState != PS_STATE_FULL_POWER)
-				addtail = FALSE;
-		}
-	}
-
-	spin_lock_irqsave(&Adapter->QueueSpinLock, flags);
-
-	if (addtail)
-		list_add_tail((struct list_head *)CmdNode,
-			      &Adapter->CmdPendingQ);
-	else
-		list_add((struct list_head *)CmdNode, &Adapter->CmdPendingQ);
-
-	spin_unlock_irqrestore(&Adapter->QueueSpinLock, flags);
-
-	PRINTM(INFO, "QUEUE_CMD: Inserted node=0x%x, cmd=0x%x in CmdPendingQ\n",
-	       (u32) CmdNode,
-	       ((struct HostCmd_DS_GEN *)CmdNode->BufVirtualAddr)->Command);
-
-      done:
-	LEAVE();
-	return;
-}
-
 /*
  * TODO: Fix the issue when DownloadCommandToStation is being called the
  * second time when the command timesout. All the CmdPtr->xxx are in little
@@ -1256,7 +1203,6 @@ void QueueCmd(wlan_adapter * Adapter, st
 static int DownloadCommandToStation(wlan_private * priv,
 				    struct CmdCtrlNode *CmdNode)
 {
-	ulong flags;
 	struct HostCmd_DS_COMMAND *CmdPtr;
 	wlan_adapter *Adapter = priv->adapter;
 	int ret = WLAN_STATUS_SUCCESS;
@@ -1269,25 +1215,24 @@ static int DownloadCommandToStation(wlan
 		PRINTM(INFO, "DNLD_CMD: Adapter = %#x, CmdNode = %#x\n",
 		       (int)Adapter, (int)CmdNode);
 		if (CmdNode)
-			CleanupAndInsertCmd(priv, CmdNode);
+			free_command(CmdNode);
 		ret = WLAN_STATUS_FAILURE;
 		goto done;
 	}
 
 	CmdPtr = (struct HostCmd_DS_COMMAND *)CmdNode->BufVirtualAddr;
 
+	Adapter->CurCmd = CmdNode;
+	Adapter->CurCmdRetCode = 0;
+
 	if (!CmdPtr || !CmdPtr->Size) {
 		PRINTM(INFO, "DNLD_CMD: CmdPtr is Null or Cmd Size is Zero, "
 		       "Not sending\n");
-		CleanupAndInsertCmd(priv, CmdNode);
+		free_command(CmdNode);
 		ret = WLAN_STATUS_FAILURE;
 		goto done;
 	}
 
-	spin_lock_irqsave(&Adapter->QueueSpinLock, flags);
-	Adapter->CurCmd = CmdNode;
-	spin_unlock_irqrestore(&Adapter->QueueSpinLock, flags);
-	Adapter->CurCmdRetCode = 0;
 	PRINTM(INFO, "DNLD_CMD:: Before download, Size of Cmd = %d\n",
 	       CmdPtr->Size);
 
@@ -1302,10 +1247,7 @@ static int DownloadCommandToStation(wlan
 
 	if (ret != 0) {
 		PRINTM(INFO, "DNLD_CMD: Host to Card Failed\n");
-		CleanupAndInsertCmd(priv, Adapter->CurCmd);
-		spin_lock_irqsave(&Adapter->QueueSpinLock, flags);
-		Adapter->CurCmd = NULL;
-		spin_unlock_irqrestore(&Adapter->QueueSpinLock, flags);
+		free_command(Adapter->CurCmd);
 		ret = WLAN_STATUS_FAILURE;
 		goto done;
 	}
@@ -1323,7 +1265,7 @@ static int DownloadCommandToStation(wlan
 
 	ret = WLAN_STATUS_SUCCESS;
 
-      done:
+done:
 	LEAVE();
 	return ret;
 }
@@ -1358,32 +1300,6 @@ static int wlan_cmd_mac_control(wlan_pri
 		Global Functions
 ********************************************************/
 
-/** 
- *  @brief This function inserts command node to CmdFreeQ
- *  after cleans it.
- *  
- *  @param priv		A pointer to wlan_private structure
- *  @param pTempCmd	A pointer to CmdCtrlNode structure
- *  @return 		n/a
- */
-void CleanupAndInsertCmd(wlan_private * priv, struct CmdCtrlNode *pTempCmd)
-{
-	ulong flags;
-	wlan_adapter *Adapter = priv->adapter;
-
-	ENTER();
-
-	if (!pTempCmd)
-		goto done;
-
-	spin_lock_irqsave(&Adapter->QueueSpinLock, flags);
-	CleanUpCmdCtrlNode(pTempCmd);
-	list_add_tail((struct list_head *)pTempCmd, &Adapter->CmdFreeQ);
-	spin_unlock_irqrestore(&Adapter->QueueSpinLock, flags);
-
-      done:
-	LEAVE();
-}
 
 /** 
  *  @brief This function sets radio control.
@@ -1432,6 +1348,9 @@ int SetMacPacketFilter(wlan_private * pr
 	return ret;
 }
 
+
+void destroy_command(struct kref *kref);
+
 /** 
  *  @brief This function prepare the command before send to firmware.
  *  
@@ -1467,7 +1386,7 @@ int PrepareAndSendCommand(wlan_private *
 		goto done;
 	}
 
-	CmdNode = GetFreeCmdCtrlNode(priv);
+	CmdNode = allocate_command(priv);
 
 	if (CmdNode == NULL) {
 		PRINTM(MSG, "PREP_CMD: No free CmdNode\n");
@@ -1485,13 +1404,6 @@ int PrepareAndSendCommand(wlan_private *
 	PRINTM(INFO, "PREP_CMD: Val of Cmd ptr =0x%x, command=0x%X\n",
 	       (u32) CmdPtr, cmd_no);
 
-	if (!CmdPtr) {
-		PRINTM(MSG, "PREP_CMD: BufVirtualAddr of CmdNode is NULL\n");
-		CleanupAndInsertCmd(priv, CmdNode);
-		ret = WLAN_STATUS_FAILURE;
-		goto done;
-	}
-
 	/* Set sequence number, command and INT option */
 	Adapter->SeqNum++;
 	CmdPtr->SeqNum = wlan_cpu_to_le16(Adapter->SeqNum);
@@ -1733,15 +1645,16 @@ #define ACTION_NUMLED_TLVTYPE_LEN_FIELDS
 	/* return error, since the command preparation failed */
 	if (ret != WLAN_STATUS_SUCCESS) {
 		PRINTM(INFO, "PREP_CMD: Command preparation failed\n");
-		CleanupAndInsertCmd(priv, CmdNode);
+		free_command(CmdNode);
 		ret = WLAN_STATUS_FAILURE;
 		goto done;
 	}
 
 	CmdNode->CmdWaitQWoken = FALSE;
 
-	QueueCmd(Adapter, CmdNode, TRUE);
-	wake_up_interruptible(&priv->MainThread.waitQ);
+	kref_get(&CmdNode->kref);
+
+	execute_command(priv, CmdNode, wait_option);
 
 	sbi_reenable_host_interrupt(priv, 0x00);
 
@@ -1751,6 +1664,8 @@ #define ACTION_NUMLED_TLVTYPE_LEN_FIELDS
 					 CmdNode->CmdWaitQWoken);
 	}
 
+	kref_put(&CmdNode->kref, &destroy_command);
+
 	if (Adapter->CurCmdRetCode) {
 		PRINTM(INFO, "PREP_CMD: Command failed with return code=%d\n",
 		       Adapter->CurCmdRetCode);
@@ -1764,175 +1679,67 @@ #define ACTION_NUMLED_TLVTYPE_LEN_FIELDS
 }
 
 /** 
- *  @brief This function allocates the command buffer and link
- *  it to command free queue.
- *  
- *  @param priv		A pointer to wlan_private structure
- *  @return 		WLAN_STATUS_SUCCESS or WLAN_STATUS_FAILURE
- */
-int AllocateCmdBuffer(wlan_private * priv)
-{
-	int ret = WLAN_STATUS_SUCCESS;
-	u32 ulBufSize;
-	u32 i;
-	struct CmdCtrlNode *TempCmdArray;
-	u8 *pTempVirtualAddr;
-	wlan_adapter *Adapter = priv->adapter;
-
-	ENTER();
-
-	/* Allocate and initialize CmdCtrlNode */
-	ulBufSize = sizeof(struct CmdCtrlNode) * MRVDRV_NUM_OF_CMD_BUFFER;
-
-	if (!(TempCmdArray = kmalloc(ulBufSize, GFP_KERNEL))) {
-		PRINTM(INFO,
-		       "ALLOC_CMD_BUF: Failed to allocate TempCmdArray\n");
-		ret = WLAN_STATUS_FAILURE;
-		goto done;
-	}
-
-	Adapter->CmdArray = TempCmdArray;
-	memset(Adapter->CmdArray, 0, ulBufSize);
-
-	/* Allocate and initialize command buffers */
-	ulBufSize = MRVDRV_SIZE_OF_CMD_BUFFER;
-	for (i = 0; i < MRVDRV_NUM_OF_CMD_BUFFER; i++) {
-		if (!(pTempVirtualAddr = kmalloc(ulBufSize, GFP_KERNEL))) {
-			PRINTM(INFO,
-			       "ALLOC_CMD_BUF: pTempVirtualAddr: out of memory\n");
-			ret = WLAN_STATUS_FAILURE;
-			goto done;
-		}
-
-		memset(pTempVirtualAddr, 0, ulBufSize);
-
-		/* Update command buffer virtual */
-		TempCmdArray[i].BufVirtualAddr = pTempVirtualAddr;
-	}
-
-	for (i = 0; i < MRVDRV_NUM_OF_CMD_BUFFER; i++) {
-		init_waitqueue_head(&TempCmdArray[i].cmdwait_q);
-		CleanupAndInsertCmd(priv, &TempCmdArray[i]);
-	}
-
-	ret = WLAN_STATUS_SUCCESS;
-      done:
-	LEAVE();
-	return ret;
-}
-
-/** 
- *  @brief This function frees the command buffer.
- *  
- *  @param priv		A pointer to wlan_private structure
- *  @return 		WLAN_STATUS_SUCCESS or WLAN_STATUS_FAILURE
- */
-int FreeCmdBuffer(wlan_private * priv)
-{
-	u32 ulBufSize;
-	unsigned int i;
-	struct CmdCtrlNode *TempCmdArray;
-	wlan_adapter *Adapter = priv->adapter;
-
-	ENTER();
-
-	/* need to check if cmd array is allocated or not */
-	if (Adapter->CmdArray == NULL) {
-		PRINTM(INFO, "FREE_CMD_BUF: CmdArray is Null\n");
-		goto done;
-	}
-
-	TempCmdArray = Adapter->CmdArray;
-
-	/* Release shared memory buffers */
-	ulBufSize = MRVDRV_SIZE_OF_CMD_BUFFER;
-	for (i = 0; i < MRVDRV_NUM_OF_CMD_BUFFER; i++) {
-		if (TempCmdArray[i].BufVirtualAddr) {
-			PRINTM(INFO, "Free all the array\n");
-			kfree(TempCmdArray[i].BufVirtualAddr);
-			TempCmdArray[i].BufVirtualAddr = NULL;
-		}
-	}
-
-	/* Release CmdCtrlNode */
-	if (Adapter->CmdArray) {
-		PRINTM(INFO, "Free CmdArray\n");
-		kfree(Adapter->CmdArray);
-		Adapter->CmdArray = NULL;
-	}
-
-      done:
-	LEAVE();
-	return WLAN_STATUS_SUCCESS;
-}
-
-/** 
- *  @brief This function gets a free command node if available in
- *  command free queue.
+ *  @brief This function allocates a command.
  *  
  *  @param priv		A pointer to wlan_private structure
  *  @return CmdCtrlNode A pointer to CmdCtrlNode structure or NULL
  */
-struct CmdCtrlNode *GetFreeCmdCtrlNode(wlan_private * priv)
+struct CmdCtrlNode *allocate_command(wlan_private * priv)
 {
 	struct CmdCtrlNode *TempNode;
-	wlan_adapter *Adapter = priv->adapter;
-	ulong flags;
 
 	ENTER();
 
-	if (!Adapter)
+	TempNode = kmalloc(sizeof(struct CmdCtrlNode), GFP_ATOMIC);
+
+	if (!TempNode)
 		return NULL;
 
-	spin_lock_irqsave(&Adapter->QueueSpinLock, flags);
+	TempNode->BufVirtualAddr = kmalloc(MRVDRV_SIZE_OF_CMD_BUFFER,
+					   GFP_ATOMIC);
 
-	if (!list_empty(&Adapter->CmdFreeQ)) {
-		TempNode = (struct CmdCtrlNode *)Adapter->CmdFreeQ.next;
-		list_del((struct list_head *)TempNode);
-	} else {
-		PRINTM(INFO, "GET_CMD_NODE: CmdCtrlNode is not available\n");
-		TempNode = NULL;
+	if (!TempNode->BufVirtualAddr) {
+		kfree(TempNode);
+		return NULL;
 	}
 
-	spin_unlock_irqrestore(&Adapter->QueueSpinLock, flags);
-
-	if (TempNode) {
-		PRINTM(INFO, "GET_CMD_NODE: CmdCtrlNode available\n");
-		PRINTM(INFO, "GET_CMD_NODE: CmdCtrlNode Address = %p\n",
-		       TempNode);
-		CleanUpCmdCtrlNode(TempNode);
-	}
+	kref_init(&TempNode->kref);
+	init_waitqueue_head(&TempNode->cmdwait_q);
 
 	LEAVE();
 	return TempNode;
 }
 
+void destroy_command(struct kref *kref)
+{
+	struct CmdCtrlNode *cmd = container_of(kref, struct CmdCtrlNode, kref);
+	kfree(cmd->BufVirtualAddr);
+	kfree(cmd);
+}
+
 /** 
- *  @brief This function cleans command node.
+ *  @brief This function frees command node.
  *  
  *  @param pTempNode	A pointer to CmdCtrlNode structure
  *  @return 		n/a
  */
-void CleanUpCmdCtrlNode(struct CmdCtrlNode *pTempNode)
+void free_command(struct CmdCtrlNode *cmd)
 {
 	ENTER();
 
-	if (!pTempNode)
+	if (!cmd)
 		return;
-	pTempNode->CmdWaitQWoken = TRUE;
-	wake_up_interruptible(&pTempNode->cmdwait_q);
-	pTempNode->Status = 0;
-	pTempNode->cmd_oid = (u32) 0;
-	pTempNode->wait_option = 0;
-	pTempNode->pdata_buf = NULL;
 
-	if (pTempNode->BufVirtualAddr != NULL)
-		memset(pTempNode->BufVirtualAddr, 0, MRVDRV_SIZE_OF_CMD_BUFFER);
+	cmd->CmdWaitQWoken = TRUE;
+	wake_up_interruptible(&cmd->cmdwait_q);
+
+	kref_put(&cmd->kref, &destroy_command);
 
 	LEAVE();
 	return;
 }
 
+
 /** 
  *  @brief This function initializes the command node.
  *  
@@ -1959,6 +1766,99 @@ void SetCmdCtrlNode(wlan_private * priv,
 	LEAVE();
 }
 
+void wait_on_full_power(wlan_adapter *adapter)
+{
+	wait_queue_t wait;
+	init_waitqueue_entry(&wait, current);
+
+	do {
+		adapter->NeedToWakeup = TRUE;
+		add_wait_queue(&adapter->full_power_waitq, &wait);
+		spin_unlock(&adapter->psstate_lock);
+		set_current_state(TASK_INTERRUPTIBLE);
+		schedule_timeout(HZ);
+		spin_lock(&adapter->psstate_lock);
+	} while (adapter->PSState != PS_STATE_FULL_POWER);
+
+	remove_wait_queue(&adapter->full_power_waitq, &wait);
+	set_current_state(TASK_RUNNING);
+}
+
+void wait_on_awake(wlan_adapter *adapter)
+{
+	wait_queue_t wait;
+	init_waitqueue_entry(&wait, current);
+
+	do {
+		adapter->NeedToWakeup = TRUE;
+		add_wait_queue(&adapter->ds_awake_q, &wait);
+		spin_unlock(&adapter->psstate_lock);
+		set_current_state(TASK_INTERRUPTIBLE);
+		schedule_timeout(HZ);
+		spin_lock(&adapter->psstate_lock);
+	} while ((adapter->PSState == PS_STATE_SLEEP) ||
+		 (adapter->PSState == PS_STATE_PRE_SLEEP));
+
+	remove_wait_queue(&adapter->ds_awake_q, &wait);
+	set_current_state(TASK_RUNNING);
+}
+
+int scheduled_cmds = 0;
+
+struct priv_and_cmd {
+	struct work_struct *w;
+	struct CmdCtrlNode *cmd;
+	wlan_private *priv;
+};
+
+static void run_scheduled_command (void *data)
+{
+	struct priv_and_cmd *cbdata = (struct priv_and_cmd *)data;
+	struct HostCmd_DS_COMMAND *CmdPtr = cbdata->cmd->BufVirtualAddr;
+
+	printk(KERN_ERR "run scheduled command (0x%x) nr:%d\n", CmdPtr->Command,
+			scheduled_cmds);
+
+	scheduled_cmds--;
+
+	execute_command(cbdata->priv, cbdata->cmd, 1);	
+
+	kfree (cbdata->w);
+	kfree (cbdata);
+}
+
+static int schedule_command(wlan_private *priv, struct CmdCtrlNode *cmdnode)
+{
+	struct priv_and_cmd *cbdata;
+	struct work_struct *work;
+	struct HostCmd_DS_COMMAND *CmdPtr = cmdnode->BufVirtualAddr;
+
+	printk(KERN_ERR "scheduled command (0x%x) nr:%d\n", CmdPtr->Command,
+			scheduled_cmds);
+
+	cbdata = kmalloc(sizeof(struct priv_and_cmd), GFP_ATOMIC);
+	if (!cbdata)
+		return -1;
+
+	work = kmalloc(sizeof(struct work_struct), GFP_ATOMIC);
+	if (!work) { 
+		kfree(cbdata);
+		return -1;
+	}
+
+	cbdata->cmd = cmdnode;
+	cbdata->priv = priv;
+	cbdata->w = work;
+
+	INIT_WORK(work, run_scheduled_command, cbdata);
+
+	schedule_work(work);
+
+	scheduled_cmds++;
+
+	return 0;
+}
+
 /** 
  *  @brief This function executes next command in command
  *  pending queue. It will put fimware back to PS mode
@@ -1967,12 +1867,10 @@ void SetCmdCtrlNode(wlan_private * priv,
  *  @param priv     A pointer to wlan_private structure
  *  @return 	   WLAN_STATUS_SUCCESS or WLAN_STATUS_FAILURE
  */
-int ExecuteNextCommand(wlan_private * priv)
+int execute_command(wlan_private * priv, struct CmdCtrlNode *cmdnode, int can_sleep)
 {
 	wlan_adapter *Adapter = priv->adapter;
-	struct CmdCtrlNode *CmdNode = NULL;
 	struct HostCmd_DS_COMMAND *CmdPtr;
-	ulong flags;
 	int ret = WLAN_STATUS_SUCCESS;
 
 	ENTER();
@@ -1983,141 +1881,116 @@ int ExecuteNextCommand(wlan_private * pr
 		goto done;
 	}
 
-	spin_lock_irqsave(&Adapter->QueueSpinLock, flags);
-
-	if (Adapter->CurCmd) {
-		PRINTM(MSG, "EXEC_NEXT_CMD: there is command in processing!\n");
-		spin_unlock_irqrestore(&Adapter->QueueSpinLock, flags);
-		ret = WLAN_STATUS_FAILURE;
+	CmdPtr = (struct HostCmd_DS_COMMAND *)cmdnode->BufVirtualAddr;
+	if (!can_sleep && CmdPtr->Command != wlan_cpu_to_le16(HostCmd_CMD_802_11_PS_MODE)) {
+		schedule_command(priv, cmdnode);
+		ret = WLAN_STATUS_SUCCESS;
 		goto done;
 	}
 
-	if (!list_empty(&Adapter->CmdPendingQ)) {
-		CmdNode = (struct CmdCtrlNode *)
-		    Adapter->CmdPendingQ.next;
-	}
-
-	spin_unlock_irqrestore(&Adapter->QueueSpinLock, flags);
+	spin_lock(&Adapter->psstate_lock);
 
-	if (CmdNode) {
-		PRINTM(INFO,
-		       "EXEC_NEXT_CMD: Got next command from CmdPendingQ\n");
-		CmdPtr = (struct HostCmd_DS_COMMAND *)CmdNode->BufVirtualAddr;
+	if ((Adapter->PSState == PS_STATE_SLEEP) ||
+	    (Adapter->PSState == PS_STATE_PRE_SLEEP)) {
+		if (CmdPtr->Command == wlan_cpu_to_le16(HostCmd_CMD_802_11_PS_MODE))
+			wait_on_awake(Adapter);
+		else
+			wait_on_full_power(Adapter);
+	}
 
+	if (Adapter->PSState == PS_STATE_AWAKE) {
+		struct HostCmd_DS_802_11_PS_MODE *psm;
+		/*
+		 * 1. Non-PS command: 
+		 * Queue it. set NeedToWakeup to TRUE if current state 
+		 * is SLEEP, otherwise call PSWakeup to send Exit_PS.
+		 * 2. PS command but not Exit_PS: 
+		 * Ignore it.
+		 * 3. PS command Exit_PS:
+		 * Set NeedToWakeup to TRUE if current state is SLEEP, 
+		 * otherwise send this command down to firmware
+		 * immediately.
+		 */
 		if (Is_Command_Allowed_In_PS(CmdPtr->Command)) {
-			if ((Adapter->PSState == PS_STATE_SLEEP)
-			    || (Adapter->PSState == PS_STATE_PRE_SLEEP)
-			    ) {
-				PRINTM(INFO,
-				       "EXEC_NEXT_CMD: Cannot send cmd 0x%x in PSState %d\n",
-				       CmdPtr->Command, Adapter->PSState);
-				ret = WLAN_STATUS_FAILURE;
-				goto done;
-			}
 			PRINTM(INFO, "EXEC_NEXT_CMD: OK to send command "
-			       "0x%x in PSState %d\n",
-			       CmdPtr->Command, Adapter->PSState);
-		} else if (Adapter->PSState != PS_STATE_FULL_POWER) {
+				      "0x%x in PSState %d\n",
+				      CmdPtr->Command, Adapter->PSState);
+			goto execute_cmd;
+		}	
+
+		if (CmdPtr->Command ==
+		    wlan_cpu_to_le16(HostCmd_CMD_802_11_PS_MODE)) {
 			/*
-			 * 1. Non-PS command: 
-			 * Queue it. set NeedToWakeup to TRUE if current state 
-			 * is SLEEP, otherwise call PSWakeup to send Exit_PS.
-			 * 2. PS command but not Exit_PS: 
-			 * Ignore it.
-			 * 3. PS command Exit_PS:
-			 * Set NeedToWakeup to TRUE if current state is SLEEP, 
-			 * otherwise send this command down to firmware
-			 * immediately.
+			 * PS command. Ignore it if it is not Exit_PS.
+			 * otherwise send it down immediately.
 			 */
-			if (CmdPtr->Command !=
-			    wlan_cpu_to_le16(HostCmd_CMD_802_11_PS_MODE)) {
-				/*  Prepare to send Exit PS,
-				 *  this non PS command will be sent later */
-				if ((Adapter->PSState == PS_STATE_SLEEP)
-				    || (Adapter->PSState == PS_STATE_PRE_SLEEP)
-				    ) {
-					/* w/ new scheme, it will not reach here.
-					   since it is blocked in main_thread. */
-					Adapter->NeedToWakeup = TRUE;
-				} else
-					PSWakeup(priv, 0);
+			psm = &CmdPtr->params.psmode;
 
+			if (psm->Action !=
+			    wlan_cpu_to_le16(HostCmd_SubCmd_Exit_PS)) {
+				PRINTM(INFO,
+				       "EXEC_NEXT_CMD: Ignore ExitPS cmd"
+					"in sleep\n");
 				ret = WLAN_STATUS_SUCCESS;
 				goto done;
-			} else {
-				/*
-				 * PS command. Ignore it if it is not Exit_PS. 
-				 * otherwise send it down immediately.
-				 */
-				struct HostCmd_DS_802_11_PS_MODE *psm =
-				    &CmdPtr->params.psmode;
-
-				PRINTM(INFO,
-				       "EXEC_NEXT_CMD: PS cmd- Action=0x%x\n",
-				       psm->Action);
-				if (psm->Action !=
-				    wlan_cpu_to_le16(HostCmd_SubCmd_Exit_PS)) {
-					PRINTM(INFO,
-					       "EXEC_NEXT_CMD: Ignore Enter PS cmd\n");
-					list_del((struct list_head *)CmdNode);
-					CleanupAndInsertCmd(priv, CmdNode);
-
-					ret = WLAN_STATUS_SUCCESS;
-					goto done;
-				}
-
-				if ((Adapter->PSState == PS_STATE_SLEEP)
-				    || (Adapter->PSState == PS_STATE_PRE_SLEEP)
-				    ) {
-					PRINTM(INFO,
-					       "EXEC_NEXT_CMD: Ignore ExitPS cmd in sleep\n");
-					list_del((struct list_head *)CmdNode);
-					CleanupAndInsertCmd(priv, CmdNode);
-					Adapter->NeedToWakeup = TRUE;
-
-					ret = WLAN_STATUS_SUCCESS;
-					goto done;
-				}
-
-				PRINTM(INFO,
-				       "EXEC_NEXT_CMD: Sending Exit_PS down...\n");
-			}
-		}
-		list_del((struct list_head *)CmdNode);
-		PRINTM(INFO, "EXEC_NEXT_CMD: Sending 0x%04X Command\n",
-		       CmdPtr->Command);
-		DownloadCommandToStation(priv, CmdNode);
-	} else {
-		/*
-		 * check if in power save mode, if yes, put the device back
-		 * to PS mode
-		 */
-		if ((Adapter->PSMode != Wlan802_11PowerModeCAM) &&
-		    (Adapter->PSState == PS_STATE_FULL_POWER) &&
-		    (Adapter->MediaConnectStatus == WlanMediaStateConnected)) {
-			if (Adapter->SecInfo.WPAEnabled
-			    || Adapter->SecInfo.WPA2Enabled) {
-				if (Adapter->IsGTK_SET) {
-					PRINTM(INFO,
-					       "EXEC_NEXT_CMD: WPA enabled and GTK_SET"
-					       " go back to PS_SLEEP");
-					PSSleep(priv, 0);
-				}
-			} else {
-				PRINTM(INFO,
-				       "EXEC_NEXT_CMD: Command PendQ is empty,"
-				       " go back to PS_SLEEP");
-				PSSleep(priv, 0);
 			}
+	//		PRINTM(INFO,
+	//		       "EXEC_NEXT_CMD: Sending Exit_PS down...\n");
+			printk(KERN_ERR "Sending Exit_PS down\n");
+			Adapter->NeedToWakeup = TRUE;
+			goto execute_cmd;
 		}
+
+		spin_unlock(&Adapter->psstate_lock);
+		PSWakeup(priv, 1);
+		spin_lock(&Adapter->psstate_lock);
+		wait_on_full_power(Adapter);
 	}
 
+execute_cmd:
+	spin_unlock(&Adapter->psstate_lock);
+//	down(&Adapter->command_sem);
+	DownloadCommandToStation(priv, cmdnode);
+
 	ret = WLAN_STATUS_SUCCESS;
-      done:
+done:
 	LEAVE();
 	return ret;
 }
 
+/* XXX: execute at timer */
+void put_back_to_ps(unsigned long data)
+{
+#if 0
+	wlan_private *priv = (wlan_private *)data;
+	wlan_adapter *Adapter = priv->adapter;
+	printk(KERN_ERR "putting back to ps mode!\n");
+	/*
+	 * check if in power save mode, if yes, put the device back
+	 * to PS mode
+	 */
+	if ((Adapter->PSMode != Wlan802_11PowerModeCAM) &&
+	    (Adapter->PSState == PS_STATE_FULL_POWER) &&
+	    (Adapter->MediaConnectStatus == WlanMediaStateConnected)) {
+		if (Adapter->SecInfo.WPAEnabled
+		    || Adapter->SecInfo.WPA2Enabled) {
+			if (Adapter->IsGTK_SET) {
+				PRINTM(INFO,
+				       "EXEC_NEXT_CMD: WPA enabled and GTK_SET"
+				       " go back to PS_SLEEP");
+				PSSleep(priv, 0);
+			}
+		} else {
+			PRINTM(INFO,
+			       "EXEC_NEXT_CMD: Command PendQ is empty,"
+			       " go back to PS_SLEEP");
+			PSSleep(priv, 0);
+			}
+		}
+	mod_timer(&Adapter->reenter_ps_timer, jiffies + (2*HZ));
+#endif
+}
+
 #if WIRELESS_EXT > 14
 /** 
  *  @brief This function sends customized event to application.
diff --git a/drivers/net/wireless/libertas/wlan_cmdresp.c b/drivers/net/wireless/libertas/wlan_cmdresp.c
index be08627..d65083b 100644
--- a/drivers/net/wireless/libertas/wlan_cmdresp.c
+++ b/drivers/net/wireless/libertas/wlan_cmdresp.c
@@ -779,9 +779,50 @@ static int wlan_ret_get_log(wlan_private
 	return WLAN_STATUS_SUCCESS;
 }
 
-/********************************************************
-		Global Functions
-********************************************************/
+#if 0
+struct cmdresp_wq {
+	struct work_struct *work;
+	struct sk_buff *skb;
+	unsigned int len;
+	wlan_private *priv;
+};
+
+static void do_process_rx_command (void *data)
+{
+	struct cmdresp_wq *cmdresp_wq = (struct cmdresp_wq*)data;
+
+	wlan_process_rx_command(cmdresp_wq->priv, cmdresp_wq->skb, cmdresp_wq->len);
+
+	kfree_skb(cmdresp_wq->skb);
+	kfree(cmdresp_wq->work);
+	kfree(cmdresp_wq);
+}
+
+void process_cmd_response(wlan_private *priv, struct sk_buff *skb, unsigned int len)
+{
+	struct cmdresp_wq *cmdresp_wq;
+	struct work_struct *work;
+
+	work = kzalloc(sizeof(struct work_struct), GFP_ATOMIC);
+	if (!work) 
+		return;
+
+	cmdresp_wq = kzalloc(sizeof(struct cmdresp_wq), GFP_ATOMIC);
+	if (!cmdresp_wq) {
+		kfree(work);
+		return;
+	}
+
+	cmdresp_wq->skb = skb;
+	cmdresp_wq->len = len;
+	cmdresp_wq->work = work; 
+	cmdresp_wq->priv = priv;
+
+	INIT_WORK(work, do_process_rx_command, cmdresp_wq);
+
+	schedule_work(work);
+}
+#endif
 
 /** 
  *  @brief This function handles the command response
@@ -806,26 +847,26 @@ int wlan_process_rx_command(wlan_private
 	del_timer(&Adapter->command_timer);
 
 	if (!Adapter->CurCmd) {
-		PRINTM(INFO, "CMD_RESP: NULL CurCmd=%p\n", Adapter->CurCmd);
-		ret = WLAN_STATUS_FAILURE;
-		goto done;
+		PRINTM(INFO, "CMD_RESP: NULL CurCmd\n");
+
+		return WLAN_STATUS_FAILURE;
 	}
+		
 	resp = (struct HostCmd_DS_COMMAND *)(Adapter->CurCmd->BufVirtualAddr);
 
-	HEXDUMP("CMD_RESP:", Adapter->CurCmd->BufVirtualAddr,
-		priv->wlan_dev.upld_len);
-
+       HEXDUMP("CMD_RESP:", Adapter->CurCmd->BufVirtualAddr,
+               priv->wlan_dev.upld_len);
 	RespCmd = wlan_le16_to_cpu(resp->Command);
 
 	Result = wlan_le16_to_cpu(resp->Result);
 
+
 	PRINTM(INFO, "CMD_RESP: %x Result: %d Length: %d\n", RespCmd,
 	       Result, priv->wlan_dev.upld_len);
 
 	if (!(RespCmd & 0x8000)) {
 		PRINTM(INFO, "Invalid response to command!");
-		Adapter->CurCmdRetCode = WLAN_STATUS_FAILURE;
-		CleanupAndInsertCmd(priv, Adapter->CurCmd);
+		free_command(Adapter->CurCmd);
 		spin_lock_irqsave(&Adapter->QueueSpinLock, flags);
 		Adapter->CurCmd = NULL;
 		spin_unlock_irqrestore(&Adapter->QueueSpinLock, flags);
@@ -837,9 +878,15 @@ int wlan_process_rx_command(wlan_private
 	/* Store the response code to CurCmdRetCode. */
 	Adapter->CurCmdRetCode = wlan_le16_to_cpu(resp->Result);
 
+
+	printk(KERN_ERR "wlan_process_rx_command0\n");
+
+
 	if (RespCmd == HostCmd_RET_802_11_PS_MODE) {
 		struct HostCmd_DS_802_11_PS_MODE *psmode;
 
+		printk(KERN_ERR "HostCmd_RET_802_11_PS_MODE!\n");
+
 		psmode = &resp->params.psmode;
 		PRINTM(INFO,
 		       "CMD_RESP: PS_MODE cmd reply result=%#x action=0x%X\n",
@@ -878,15 +925,19 @@ int wlan_process_rx_command(wlan_private
 				PSWakeup(priv, 0);
 			}
 		} else if (psmode->Action == HostCmd_SubCmd_Exit_PS) {
+			spin_lock(&Adapter->psstate_lock);
 			Adapter->NeedToWakeup = FALSE;
 			Adapter->PSState = PS_STATE_FULL_POWER;
+			printk(KERN_ERR "FULL POWER!!\n");
+			wake_up_interruptible_all(&Adapter->full_power_waitq);
+			spin_unlock(&Adapter->psstate_lock);
 			PRINTM(INFO, "CMD_RESP: Exit_PS command response\n");
 		} else {
 			PRINTM(INFO, "CMD_RESP: PS- Action=0x%X\n",
 			       psmode->Action);
 		}
 
-		CleanupAndInsertCmd(priv, Adapter->CurCmd);
+		free_command(Adapter->CurCmd);
 		spin_lock_irqsave(&Adapter->QueueSpinLock, flags);
 		Adapter->CurCmd = NULL;
 		spin_unlock_irqrestore(&Adapter->QueueSpinLock, flags);
@@ -895,13 +946,19 @@ int wlan_process_rx_command(wlan_private
 		goto done;
 	}
 
-	if (Adapter->CurCmd->CmdFlags & CMD_F_HOSTCMD) {
-		/* Copy the response back to response buffer */
-		memcpy(Adapter->CurCmd->pdata_buf, resp, resp->Size);
+	printk(KERN_ERR "wlan_process_rx_command1\n");
 
+	if (Adapter->CurCmd && (Adapter->CurCmd->CmdFlags & CMD_F_HOSTCMD)) {
+		/* Copy the response back to response buffer */
+		printk(KERN_ERR "copying to pdata_buf=%x\n", Adapter->CurCmd->pdata_buf);
+		if (Adapter->CurCmd->pdata_buf)
+			memcpy(Adapter->CurCmd->pdata_buf, resp, resp->Size);
 		Adapter->CurCmd->CmdFlags &= ~CMD_F_HOSTCMD;
 	}
 
+	printk(KERN_ERR "wlan_process_rx_command2\n");
+
+
 	/* If the command is not successful, cleanup and return failure */
 	if ((Result != HostCmd_RESULT_OK || !(RespCmd & 0x8000))) {
 		PRINTM(INFO, "CMD_RESP: command reply %#x result=%#x\n",
@@ -918,14 +975,19 @@ int wlan_process_rx_command(wlan_private
 
 		}
 
-		CleanupAndInsertCmd(priv, Adapter->CurCmd);
+		free_command(Adapter->CurCmd);
 		spin_lock_irqsave(&Adapter->QueueSpinLock, flags);
 		Adapter->CurCmd = NULL;
 		spin_unlock_irqrestore(&Adapter->QueueSpinLock, flags);
 
+		up(&Adapter->command_sem);
 		return WLAN_STATUS_FAILURE;
 	}
 
+
+	printk(KERN_ERR "wlan_process_rx_command3\n");
+
+
 	switch (RespCmd) {
 	case HostCmd_RET_MAC_REG_ACCESS:
 	case HostCmd_RET_BBP_REG_ACCESS:
@@ -1095,13 +1157,14 @@ int wlan_process_rx_command(wlan_private
 
 	if (Adapter->CurCmd) {
 		/* Clean up and Put current command back to CmdFreeQ */
-		CleanupAndInsertCmd(priv, Adapter->CurCmd);
+		free_command(Adapter->CurCmd);
 		spin_lock_irqsave(&Adapter->QueueSpinLock, flags);
 		Adapter->CurCmd = NULL;
 		spin_unlock_irqrestore(&Adapter->QueueSpinLock, flags);
 	}
 
-      done:
+done:
+	up(&Adapter->command_sem);
 	LEAVE();
 	return ret;
 }
@@ -1180,6 +1243,7 @@ int wlan_process_event(wlan_private * pr
 		}
 
 		Adapter->PSState = PS_STATE_AWAKE;
+		wake_up_interruptible_all(&priv->adapter->ds_awake_q);
 
 		if (Adapter->NeedToWakeup == TRUE) {
 			/*
@@ -1189,7 +1253,7 @@ int wlan_process_event(wlan_private * pr
 			 * in PSWakeup()
 			 */
 			PRINTM(INFO, "Waking up...\n");
-			PSWakeup(priv, 0);
+			PSWakeup(priv, 1);
 		}
 		break;
 
diff --git a/drivers/net/wireless/libertas/wlan_decl.h b/drivers/net/wireless/libertas/wlan_decl.h
index 534f44f..fb150e6 100644
--- a/drivers/net/wireless/libertas/wlan_decl.h
+++ b/drivers/net/wireless/libertas/wlan_decl.h
@@ -42,9 +42,11 @@ void SendTxFeedback(wlan_private * priv)
 u8 CheckLastPacketIndication(wlan_private * priv);
 
 void Wep_encrypt(wlan_private * priv, u8 * Buf, u32 Len);
-int FreeCmdBuffer(wlan_private * priv);
-void CleanUpCmdCtrlNode(struct CmdCtrlNode *pTempNode);
-struct CmdCtrlNode *GetFreeCmdCtrlNode(wlan_private * priv);
+struct CmdCtrlNode *allocate_command(wlan_private * priv);
+void free_command(struct CmdCtrlNode *);
+void destroy_command(struct kref *kref);
+int execute_command(wlan_private *, struct CmdCtrlNode *, int);
+
 
 void SetCmdCtrlNode(wlan_private * priv,
 		    struct CmdCtrlNode *pTempNode,
@@ -55,11 +57,7 @@ int PrepareAndSendCommand(wlan_private *
 			  u16 cmd_action,
 			  u16 wait_option, u32 cmd_oid, void *pdata_buf);
 
-void QueueCmd(wlan_adapter * Adapter, struct CmdCtrlNode *CmdNode, u8 addtail);
-
 int SetDeepSleep(wlan_private * priv, u8 bDeepSleep);
-int AllocateCmdBuffer(wlan_private * priv);
-int ExecuteNextCommand(wlan_private * priv);
 int wlan_process_event(wlan_private * priv);
 void wlan_interrupt(struct net_device *);
 int SetRadioControl(wlan_private * priv);
@@ -68,6 +66,7 @@ u8 data_rate_to_index(u32 rate);
 void HexDump(char *prompt, u8 * data, int len);
 void get_version(wlan_adapter * adapter, char *version, int maxlen);
 void wlan_read_write_rfreg(wlan_private * priv);
+void put_back_to_ps(unsigned long data);
 
 /** The proc fs interface */
 void wlan_proc_entry(wlan_private * priv, struct net_device *dev);
@@ -75,6 +74,7 @@ void wlan_proc_remove(wlan_private * pri
 void wlan_debug_entry(wlan_private * priv, struct net_device *dev);
 void wlan_debug_remove(wlan_private * priv);
 int wlan_process_rx_command(wlan_private * priv);
+void process_cmd_response(wlan_private *priv, struct sk_buff *skb, unsigned int len);
 int wlan_process_tx(wlan_private * priv, struct sk_buff *skb);
 void CleanupAndInsertCmd(wlan_private * priv, struct CmdCtrlNode *pTempCmd);
 void command_timer_fn(unsigned long data);
@@ -97,8 +97,6 @@ void PSWakeup(wlan_private * priv, int w
 
 #define SDIO_HEADER_LEN		4
 
-void wlan_send_rxskbQ(wlan_private * priv);
-
 extern CHANNEL_FREQ_POWER *find_cfp_by_band_and_channel(wlan_adapter * adapter,
 							u8 band, u16 channel);
 
diff --git a/drivers/net/wireless/libertas/wlan_dev.h b/drivers/net/wireless/libertas/wlan_dev.h
index fbc19cd..fd8b38d 100644
--- a/drivers/net/wireless/libertas/wlan_dev.h
+++ b/drivers/net/wireless/libertas/wlan_dev.h
@@ -133,16 +133,10 @@ typedef struct _wlan_dev {
 	void *card;
 	/** IO port */
 	u32 ioport;
-	/** Upload received */
-	u32 upld_rcv;
-	/** Upload type */
-	u32 upld_typ;
-	/** Upload length */
 	u32 upld_len;
+	u8 upld_buf[WLAN_UPLD_SIZE];
 	/** netdev pointer */
 	struct net_device *netdev;
-	/* Upload buffer */
-	u8 upld_buf[WLAN_UPLD_SIZE];
 	/* Download sent: 
 	   bit0 1/0=data_sent/data_tx_done, 
 	   bit1 1/0=cmd_sent/cmd_tx_done, 
@@ -220,6 +214,7 @@ #endif				/* REASSOCIATION */
 	/** Event Queues */
 	wait_queue_head_t ds_awake_q;
 
+
 	u8 HisRegCpy;
 
 	/** current ssid/bssid related parameters*/
@@ -251,6 +246,8 @@ #ifdef REASSOCIATION
 	struct semaphore ReassocSem;
 #endif				/* REASSOCIATION */
 
+	struct semaphore command_sem;
+
 	u8 ATIMEnabled;
 
 	/** MAC address information */
@@ -306,8 +303,14 @@ #endif				/* REASSOCIATION */
 	u16 PSMode;		/* Wlan802_11PowerModeCAM=disable
 				   Wlan802_11PowerModeMAX_PSP=enable */
 	u16 MultipleDtim;
+	spinlock_t psstate_lock;
 	u32 PSState;
 	u8 NeedToWakeup;
+	/* Exit power save mode (transition to full power) event */
+	wait_queue_head_t full_power_waitq;
+	struct timer_list reenter_ps_timer;
+
+
 
 	struct PS_CMD_ConfirmSleep PSConfirmSleep;
 	u16 LocalListenInterval;
diff --git a/drivers/net/wireless/libertas/wlan_fw.c b/drivers/net/wireless/libertas/wlan_fw.c
index df90bbc..866730b 100644
--- a/drivers/net/wireless/libertas/wlan_fw.c
+++ b/drivers/net/wireless/libertas/wlan_fw.c
@@ -161,8 +161,7 @@ static int wlan_allocate_adapter(wlan_pr
 
 	spin_lock_init(&Adapter->QueueSpinLock);
 
-	/* Allocate the command buffers */
-	AllocateCmdBuffer(priv);
+	spin_lock_init(&Adapter->psstate_lock);
 
 	memset(&Adapter->PSConfirmSleep, 0, sizeof(struct PS_CMD_ConfirmSleep));
 	Adapter->PSConfirmSleep.SeqNum = wlan_cpu_to_le16(++Adapter->SeqNum);
@@ -227,6 +226,7 @@ static void wlan_init_adapter(wlan_priva
 #ifdef REASSOCIATION
 	init_MUTEX(&Adapter->ReassocSem);
 #endif
+	init_MUTEX(&Adapter->command_sem);
 
 	Adapter->Prescan = CMD_ENABLED;
 
@@ -307,6 +307,9 @@ int wlan_init_fw(wlan_private * priv)
 	setup_timer(&Adapter->command_timer, command_timer_fn,
 			(unsigned long)priv);
 
+	setup_timer(&Adapter->reenter_ps_timer, put_back_to_ps,
+			(unsigned long)priv);
+
 #ifdef REASSOCIATION
         /* Initialize the timer for the reassociation */
 	setup_timer(&Adapter->reassoc_timer, reassoc_timer_fn,
@@ -345,9 +348,6 @@ void wlan_free_adapter(wlan_private * pr
 		return;
 	}
 
-	PRINTM(INFO, "Free Command buffer\n");
-	FreeCmdBuffer(priv);
-
 	PRINTM(INFO, "Free CommandTimer\n");
 	del_timer(&Adapter->command_timer);
 #ifdef REASSOCIATION
@@ -399,7 +399,7 @@ void command_timer_fn(unsigned long data
 	spin_unlock_irqrestore(&Adapter->QueueSpinLock, flags);
 
 	PRINTM(INFO, "Re-sending same command as it timeout...!\n");
-	QueueCmd(Adapter, pTempNode, FALSE);
+	execute_command(priv, pTempNode, 0);
 
 	wake_up_interruptible(&priv->MainThread.waitQ);
 
diff --git a/drivers/net/wireless/libertas/wlan_fw.h b/drivers/net/wireless/libertas/wlan_fw.h
index fe11c7e..bd6a3d8 100644
--- a/drivers/net/wireless/libertas/wlan_fw.h
+++ b/drivers/net/wireless/libertas/wlan_fw.h
@@ -43,6 +43,5 @@ #define FIRMWARE_TRANSFER_BLOCK_SIZE    
 int wlan_init_fw(wlan_private * priv);
 int wlan_disable_host_int(wlan_private * priv, u8 reg);
 int wlan_enable_host_int(wlan_private * priv, u8 mask);
-int wlan_free_cmd_buffers(wlan_private * priv);
 
 #endif				/* _WLAN_FW_H_ */
diff --git a/drivers/net/wireless/libertas/wlan_main.c b/drivers/net/wireless/libertas/wlan_main.c
index 1b1e167..a67cf67 100644
--- a/drivers/net/wireless/libertas/wlan_main.c
+++ b/drivers/net/wireless/libertas/wlan_main.c
@@ -744,11 +744,6 @@ static int wlan_service_main_thread(void
 			continue;
 		}
 
-		/* Execute the next command */
-		if (!priv->wlan_dev.dnld_sent && !Adapter->CurCmd) {
-			ExecuteNextCommand(priv);
-		}
-
 	}
 
 	wlan_deactivate_thread(thread);
@@ -823,6 +818,8 @@ #define NETIF_F_DYNALLOC 16
 
 	init_waitqueue_head(&priv->adapter->ds_awake_q);
 
+	init_waitqueue_head(&priv->adapter->full_power_waitq);
+
 #ifdef ENABLE_PM
 	if (!(wlan_pm_dev = pm_register(PM_UNKNOWN_DEV, 0, wlan_pm_callback)))
 		PRINTM(MSG, "Failed to register PM callback\n");
diff --git a/drivers/net/wireless/libertas/wlan_wext.c b/drivers/net/wireless/libertas/wlan_wext.c
index 63dda4b..fc058b2 100644
--- a/drivers/net/wireless/libertas/wlan_wext.c
+++ b/drivers/net/wireless/libertas/wlan_wext.c
@@ -1849,6 +1849,8 @@ static int wlan_set_power(struct net_dev
 
 	if (vwrq->disabled) {
 		Adapter->PSMode = Wlan802_11PowerModeCAM;
+
+		del_timer(&Adapter->reenter_ps_timer);
 		if (Adapter->PSState != PS_STATE_FULL_POWER) {
 			PSWakeup(priv, HostCmd_OPTION_WAITFORRSP);
 		}
@@ -1873,6 +1875,7 @@ static int wlan_set_power(struct net_dev
 
 	if (Adapter->MediaConnectStatus == WlanMediaStateConnected) {
 		PSSleep(priv, HostCmd_OPTION_WAITFORRSP);
+		mod_timer(&Adapter->reenter_ps_timer, jiffies + (2*HZ));
 	}
 
 	LEAVE();
@@ -2508,7 +2511,6 @@ static int wlan_hostcmd_ioctl(struct net
 	struct CmdCtrlNode *pCmdNode;
 	struct HostCmd_DS_GEN *gencmd, *pCmdPtr;
 	wlan_private *priv = dev->priv;
-	wlan_adapter *Adapter = priv->adapter;
 	u16 wait_option = 0;
 
 	ENTER();
@@ -2516,8 +2518,8 @@ static int wlan_hostcmd_ioctl(struct net
 	/*
 	 * Get a free command control node 
 	 */
-	if (!(pCmdNode = GetFreeCmdCtrlNode(priv))) {
-		PRINTM(INFO, "Failed GetFreeCmdCtrlNode\n");
+	if (!(pCmdNode = allocate_command(priv))) {
+		PRINTM(INFO, "Failed allocate_command\n");
 		return -ENOMEM;
 	}
 
@@ -2556,8 +2558,7 @@ static int wlan_hostcmd_ioctl(struct net
 	       req->ifr_data, pCmdPtr);
 
 	pCmdNode->CmdWaitQWoken = FALSE;
-	QueueCmd(Adapter, pCmdNode, TRUE);
-	wake_up_interruptible(&priv->MainThread.waitQ);
+	execute_command(priv, pCmdNode, 1);
 
 	if (wait_option & HostCmd_OPTION_WAITFORRSP) {
 		/* Sleep until response is generated by FW */

Re: [PATCH] Marvell Libertas 8388 802.11b/g USB driver (v2)

[Jiri Benc]

On Wed, 17 Jan 2007 12:43:08 -0500, Dan Williams wrote:
> 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).

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

I think you're right.

> 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.

> [...]
> 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.

> [...]
> 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.

It probably couldn't. But you could use cfg80211, at least.

> 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.

Yes. That's the reason why not inventing another solution is probably a
good idea.

Thanks,

 Jiri

-- 
Jiri Benc
SUSE Labs

Re: [PATCH] Marvell Libertas 8388 802.11b/g USB driver (v2)

[Marcelo Tosatti]

Hi Jiri,

On Wed, Jan 17, 2007 at 07:07:26PM +0100, Jiri Benc wrote:
> On Wed, 17 Jan 2007 12:43:08 -0500, Dan Williams wrote:
> > 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).
> 
> > I thought these 2 things were essentially _the_ definition of fullmac,
> > please correct me if I'm wrong.
> 
> I think you're right.
> 
> > 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.
> 
> > [...]
> > 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.

Most notably because the firmware provides a vast number of
configuration commands, and all of them are supported by the driver.

For instance, it supports sleep mode, which makes normal operation way
more complex than if it didnt (the driver queue's packets internally to
hand them out once the awake window has been opened).

Re: [PATCH] Marvell Libertas 8388 802.11b/g USB driver (v2)

[Jiri Benc]

On Wed, 17 Jan 2007 10:01:12 -0500, Dan Williams wrote:
> 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

> +/** 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
> +#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
at it in the half, sorry.

 Jiri

-- 
Jiri Benc
SUSE Labs

Re: [PATCH] Marvell Libertas 8388 802.11b/g USB driver (v2)

[Marcelo Tosatti]

Hi Jiri,

On Wed, Jan 17, 2007 at 04:43:28PM +0100, Jiri Benc wrote:
> On Wed, 17 Jan 2007 10:01:12 -0500, Dan Williams wrote:
> > 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)

Such as? The bit definitions (IEEE80211_RADIOTAP_F_{TX,RX}...) have been
moved to include/net/ieee80211_radiotap.h in the second version of the
patch. Is that what you were referring to?

As for kernel coding style in that file, can you specify what looks
wrong? It appears alright to me.

> - 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
> 
> > +/** 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
> > +#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

Removed.

> or those ENTER and LEAVE statements in each other function,

What is the problem with it? The entry/exit debug points have been
pretty useful for debugging. A bunch of in-tree drivers contain similar
code.

You might argue that ENTER/LEAVE should be in lower caps?

> non-conforming coding style, 

Send patches or point them out? :) 

I've ran scripts/Lindent over it, but there might still be some sections
in need of love.

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

Removed them all, moved the Marvell copyright + GNU header to LICENSE
file.

> a lot of duplicated code, etc. 

Again, please point it out.

> I stopped looking t it in the half, asorry.

Thanks for your comments so far!

Re: [PATCH] Marvell Libertas 8388 802.11b/g USB driver (v2)

[Arnd Bergmann]

On Tuesday 16 January 2007 19:55, Marcelo Tosatti wrote:
> 
> 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.

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.

> --- /dev/null
> +++ b/drivers/net/wireless/libertas/Makefile
> @@ -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.

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

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

> +  * 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.

> +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

> +/** 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

> +
> +/** 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 */
> +#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.

> +/* 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
> +
> +#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.

> +/* 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 */
> +	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...

> +struct WLAN_802_11_KEY {
> +	u16 type; /* KEY_TYPE_* from wlan_defs.h */
> +	u32 len;
> +	u8 key[MRVL_MAX_KEY_WPA_KEY_LENGTH];
> +	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.

> +/* 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 */
> +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.

> +/* 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?

> +/* 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));
> +
> +#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.

> +		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.

> +
> +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?

> +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;
> +	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?

> +
> +	/* 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.

> +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?

> +static u8 usb_int_cause = 0;

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

> +struct BootCMDStr       sBootCMD;

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

> +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);
> +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).

> +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;
> +
> +	/* 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.

> +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...

> +	return (-ENOMEM);

	return -ENOMEM;

> +	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.

> +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).

> +/**  
> + *  @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.

> +/** 
> + *  @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.

> +	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.

> +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.

> +/** 
> + *  @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.

> +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.

> +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,
> +	.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), },
	...
};

> +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.

> +/** 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
> +#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.

> +
> +/** 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
> +#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.

> +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.

> +#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

> +#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

> +
> +#if defined(DEBUG)
> +static inline void HEXDUMP(char *prompt, u8 * buf, int len)
> +{
> +	int i = 0;
> +
> +	if (!libertas_debug)
> +		return;
> +
> +	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.

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

array_size()

> +static int wlan_setup_station_hw(wlan_private * priv)
> +{
> +	int ret = WLAN_STATUS_FAILURE;
> +	wlan_adapter *adapter = priv->adapter;
> +
> +	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.

> diff --git a/drivers/net/wireless/libertas/wlan_ioctl.c b/drivers/net/wireless/libertas/wlan_ioctl.c
> new file mode 100644
> index 0000000..0cc852b
> --- /dev/null
> +++ b/drivers/net/wireless/libertas/wlan_ioctl.c
> @@ -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.

> +#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},
> +	{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?

	Arnd <><

Re: [PATCH] Marvell Libertas 8388 802.11b/g USB driver (v2)

[Marcelo Tosatti]

Hi Arnd,

On Sat, Jan 27, 2007 at 02:53:07AM +0100, Arnd Bergmann wrote:
> On Tuesday 16 January 2007 19:55, Marcelo Tosatti wrote:
> > 
> > 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.
> 
> 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.

Thanks so much! Really useful. Thats the sort of commentary I was
looking for.

> 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.

Well, I think the driver is pretty complete (support for the large
number of parameters the firmware exposes is implemented), and even
though it contains a lot of bloat it is worthwhile to continue the
cleanup process, IMHO. 

Also, the driver has been pretty well tested already.

> 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.
> 
> > --- /dev/null
> > +++ b/drivers/net/wireless/libertas/Makefile
> > @@ -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.

Done.

> > +
> > + (c) Copyright  2003-2006, Marvell International Ltd. 
> > + All Rights Reserved
> > +
> 
> 'All Rights Reserved' sounds wrong, considering it's actually GPL.

A bunch of other drivers also have "All Rights Reserved" for the company
or individual who wrote the driver. I don't see it as a problem,
considering its GPL anyway.

> > +  * 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.

Done.

> > +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

Done.

> > +/** 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
> 
> > +
> > +/** 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 */
> > +#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.

Done.

> > +/* 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
> > +
> > +#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.

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

> > +/* 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 */
> > +	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...

I've started conversion to normalcaps, have to finish it...

> > +struct WLAN_802_11_KEY {
> > +	u16 type; /* KEY_TYPE_* from wlan_defs.h */
> > +	u32 len;
> > +	u8 key[MRVL_MAX_KEY_WPA_KEY_LENGTH];
> > +	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.

The former, reordered.

> > +/* 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 */
> > +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.

The command structures are defined and interpreted by the firmware (ie
used by hardware), and as such should be packed (to avoid padding) and
can't be reordered.

> > +/* 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?

This one is indeed not necessary, since "struct IEEEtypes_AssocRsp" is
attributed as packed.

> > +/* 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));
> > +
> > +#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.

Done. Also anonymize "manual_mode"/"auto_mode" structures.

> > +		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.

Can't do: 

int libertas_ret_80211_associate(wlan_private * priv,
                  struct HostCmd_DS_COMMAND *resp)
{
    wlan_adapter *Adapter = priv->adapter;
    int ret = 0;
    union iwreq_data wrqu;
    struct IEEEtypes_AssocRsp *pAssocRsp;
    struct bss_descriptor *pBSSDesc;

    ENTER();

    pAssocRsp = (struct IEEEtypes_AssocRsp *) & resp->params;

> > +
> > +extern struct BootCMDStr	sBootCMD;
> > +
> 
> 'extern' declarations should _all_ go into header files. Normally,
> sparse should catch this. 

Done.

> Did you run the file through a recent sparse successfully?

Spits a bunch of warnings, looking at them.

> > +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;
> > +	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?

No point, moved to if_usb_issue_boot_command's stack.

> > +
> > +	/* 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.

Done.

> > +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?

Done.

> > +static u8 usb_int_cause = 0;
> 
> This is a global variable, but you seem to use it per device. Is that
> intended?

No, its a bug. Fixed.

> > +struct BootCMDStr       sBootCMD;
> 
> This variable is used only in one file, but defined in another...

Fixed.

> > +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);
> > +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).

Done.

> > +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;
> > +
> > +	/* 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.
> 
> > +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.

> > +	return (-ENOMEM);
> 
> 	return -ENOMEM;

Done.

> 
> > +	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.

This one should be removed anyway, but yes, I get the point.

> > +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).

Yeah, done.

> > +/**  
> > + *  @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.
> 
> > +/** 
> > + *  @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.
> 
> > +	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. 

This is part of WEXT API, I'm sorry. :) 

Will turn it into a helper function.

> > +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.
> 
> > +/** 
> > + *  @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.
> 
> > +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.
> 
> > +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,
> > +	.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), },
> 	...
> };
> 
> > +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.
> 
> > +/** 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
> > +#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.
>
> > +
> > +/** 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
> > +#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.

Done.

> > +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.
> 
> > +#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

Done.

> > +#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.

> > +
> > +#if defined(DEBUG)
> > +static inline void HEXDUMP(char *prompt, u8 * buf, int len)
> > +{
> > +	int i = 0;
> > +
> > +	if (!libertas_debug)
> > +		return;
> > +
> > +	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.
> 
> > +#ifndef NELEMENTS
> > +#define NELEMENTS(x) (sizeof(x)/sizeof(x[0]))
> > +#endif
> 
> array_size()

Done.

> > +static int wlan_setup_station_hw(wlan_private * priv)
> > +{
> > +	int ret = WLAN_STATUS_FAILURE;
> > +	wlan_adapter *adapter = priv->adapter;
> > +
> > +	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.
> 
> > diff --git a/drivers/net/wireless/libertas/wlan_ioctl.c b/drivers/net/wireless/libertas/wlan_ioctl.c
> > new file mode 100644
> > index 0000000..0cc852b
> > --- /dev/null
> > +++ b/drivers/net/wireless/libertas/wlan_ioctl.c
> > @@ -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. 

But most of these ioctl's are driver-specific anyway (and I personally
consider it one of the good points of the driver, from a completness
POV).

> > +#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},
> > +	{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?

Yeah, it is. I'll look into using generic code.

Again, thanks!

Re: [PATCH] Marvell Libertas 8388 802.11b/g USB driver (v2)

[John W. Linville]

On Sat, Feb 03, 2007 at 08:43:49PM -0200, Marcelo Tosatti wrote:
> On Sat, Jan 27, 2007 at 02:53:07AM +0100, Arnd Bergmann wrote:
> > On Tuesday 16 January 2007 19:55, Marcelo Tosatti wrote:

> > > +#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.

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.

John
-- 
John W. Linville
linville@tuxdriver.com

Re: [PATCH] Marvell Libertas 8388 802.11b/g USB driver (v2)

[Arnd Bergmann]

On Monday 05 February 2007 15:01, John W. Linville wrote:
> 
> > 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.

I don't think there is a formal rule. My personal opinion is that
you should trace events that come from the hardware of from the user,
if you trace at all, but never trace function call sequences that
can be simply identified by knowing the source code.

	Arnd <><

Re: [PATCH] Marvell Libertas 8388 802.11b/g USB driver (v2)

[Christoph Hellwig]

On Mon, Feb 05, 2007 at 09:01:47AM -0500, John W. Linville wrote:
> > 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.

We don't want this generally.  It's trivial to imlement this kind of
thing using gcc mcount instrumentation, but no one managed to submit
a generic implementation of this yet.  acme is hacking on some cool
tools to make similar things possible.  acme, any chance you might
have a cool idea about something based on ostra that we could merge
to allow people to do this without messing up the source code [1].

[1] yes, the l33t crowd would call this aspect oriented programming,
	I'd cool this cool and useful tool :)

Re: [PATCH] Marvell Libertas 8388 802.11b/g USB driver (v2)

[Marcelo Tosatti]

On Sat, Feb 03, 2007 at 08:43:49PM -0200, Marcelo Tosatti wrote:
> Hi Arnd,

> > 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.

Done.

> > Did you run the file through a recent sparse successfully?
> 
> Spits a bunch of warnings, looking at them.

Fixed sparse warnings.

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

Done.

> > 
> > > +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.

Removed a bunch of ENTER/LEAVE macro calls.

Re: [PATCH] Marvell Libertas 8388 802.11b/g USB driver (v2)

[Marcelo Tosatti]

On Sat, Jan 27, 2007 at 02:53:07AM +0100, Arnd Bergmann wrote:
> > +/** 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

Done.

> 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.

> > +/**  
> > + *  @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.

Moved processing of CMD_TYPE_REQUEST and CMD_TYPE_DATA to inline 
functions.

> > +/** 
> > + *  @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.

Fixed.

> > +	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.

Converted to a macro.

> > +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.
> 
> > +/** 
> > + *  @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.

Moved the switch() to an inline function, that should make it much more
readable.

> > +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.

Borrowed the idea from bcm43xx, but yeah, I agree that using
get_zeroed_page/free_page is cleaner and safer.

Done.

> > +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,
> > +	.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), },
> 	...
> };
> 
> > +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.

Done.