Re: [PATCH] Marvell Libertas 8388 802.11b/g USB driver (v2)
On Sat, Jan 27, 2007 at 02:53:07AM +0100, Arnd Bergmann wrote:
> > +/** Command Processing States and Options */
> > +#define HostCmd_STATE_IDLE0x
> > +#define HostCmd_STATE_IN_USE_BY_HOST 0x0001
> > +#define HostCmd_STATE_IN_USE_BY_MINIPORT 0x0002
> > +#define HostCmd_STATE_FINISHED0x000f
>
> 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
> > + * @returnN/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 cardpointer to a usb_card_rec
> > + * @returnpointer 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 privA 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,
> > +
Re: [PATCH] Marvell Libertas 8388 802.11b/g USB driver (v2)
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.
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Marvell Libertas 8388 802.11b/g USB driver (v2)
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 :) - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Marvell Libertas 8388 802.11b/g USB driver (v2)
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 <>< - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Marvell Libertas 8388 802.11b/g USB driver (v2)
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 [EMAIL PROTECTED] - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Marvell Libertas 8388 802.11b/g USB driver (v2)
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_IDLE0x > > +#define HostCmd_STATE_IN_USE_BY_HOST 0x0001 > > +#define HostCmd_STATE_IN_USE_BY_MINIPORT 0x0002 > > +#define HostCmd_STATE_FINISHED0x000f > > No SilLYcAps please > > > + > > +/** General Result Code*/ > > +/* OK */ > > +#define HostCmd_RESULT_OK0x > > +/* 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_READ0x > > +#define HostCmd_ACT_GEN_WRITE 0x0001 > > +#define HostCmd_ACT_GEN_GET 0x > > +#define HostCmd_ACT_GEN_SET 0x0001 > > +#define HostCmd_ACT_GEN_REMOVE 0x0002 > > +#define HostCmd_ACT_GEN_OFF 0x > > +#define HostCmd_ACT_GEN_ON 0x0001 > > + > > +/* Define action or option for HostCmd_CMD_802_11_AUTHENTICATE */ > > +#define HostCmd_ACT_AUTHENTICATE0x00
Re: [PATCH] Marvell Libertas 8388 802.11b/g USB driver (v2)
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?)
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Marvell Libertas 8388 802.11b/g USB driver (v2)
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_IDLE0x
> +#define HostCmd_STATE_IN_USE_BY_HOST 0x0001
> +#define HostCmd_STATE_IN_USE_BY_MINIPORT 0x0002
> +#define HostCmd_STATE_FINISHED0x000f
No SilLYcAps please
> +
> +/** General Result Code*/
> +/* OK */
> +#define HostCmd_RESULT_OK0x
> +/* 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_READ0x
> +#define HostCmd_ACT_GEN_WRITE 0x0001
> +#define HostCmd_ACT_GEN_GET 0x
> +#define HostCmd_ACT_GEN_SET 0x0001
> +#define HostCmd_ACT_GEN_REMOVE 0x0002
> +#define HostCmd_ACT_GEN_OFF 0x
> +#define HostCmd_ACT_GEN_ON 0x0001
> +
> +/* Define action or option for HostCmd_CMD_802_11_AUTHENTICATE */
> +#define HostCmd_ACT_AUTHENTICATE0x0001
> +#define HostCmd_ACT_DEAUTHENTICATE 0x0002
> +
> +/* Define action or option for HostCmd_CMD_802_11_ASSOCIATE */
> +#define HostCmd_ACT_ASSOCIATE 0x0001
> +#define HostCmd_ACT_DISASSOCIATE0x0002
> +#define HostCmd_ACT_REASSOCIATE 0x0003
> +
> +#define HostCmd_CAPINFO_ESS 0x0001
> +#define HostCmd_CAPINFO_IBSS0x0002
> +#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;
> +
Re: [PATCH] Marvell Libertas 8388 802.11b/g USB driver (v2)
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 [EMAIL PROTECTED] - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Marvell Libertas 8388 802.11b/g USB driver (v2)
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 - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Marvell Libertas 8388 802.11b/g USB driver (v2)
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? - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Marvell Libertas 8388 802.11b/g USB driver (v2)
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 signature.asc Description: This is a digitally signed message part
Re: [PATCH] Marvell Libertas 8388 802.11b/g USB driver (v2)
On 1/23/07, Johannes Berg <[EMAIL PROTECTED]> 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 [EMAIL PROTECTED] - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Marvell Libertas 8388 802.11b/g USB driver (v2)
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 signature.asc Description: This is a digitally signed message part
Re: [PATCH] Marvell Libertas 8388 802.11b/g USB driver (v2)
On 1/23/07, Dan Williams <[EMAIL PROTECTED]> wrote: On Tue, 2007-01-23 at 12:14 -0500, Jon Smirl wrote: > On 1/23/07, Johannes Berg <[EMAIL PROTECTED]> 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 [EMAIL PROTECTED] - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Marvell Libertas 8388 802.11b/g USB driver (v2)
On Tue, 2007-01-23 at 12:14 -0500, Jon Smirl wrote: > On 1/23/07, Johannes Berg <[EMAIL PROTECTED]> 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 - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Marvell Libertas 8388 802.11b/g USB driver (v2)
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 signature.asc Description: This is a digitally signed message part
Re: [PATCH] Marvell Libertas 8388 802.11b/g USB driver (v2)
On 1/23/07, Johannes Berg <[EMAIL PROTECTED]> 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 [EMAIL PROTECTED] - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Marvell Libertas 8388 802.11b/g USB driver (v2)
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 signature.asc Description: This is a digitally signed message part
Re: [PATCH] Marvell Libertas 8388 802.11b/g USB driver (v2)
On 1/23/07, Johannes Berg <[EMAIL PROTECTED]> 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 [EMAIL PROTECTED] - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Marvell Libertas 8388 802.11b/g USB driver (v2)
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 signature.asc Description: This is a digitally signed message part
Re: [PATCH] Marvell Libertas 8388 802.11b/g USB driver (v2)
On 1/22/07, Marcelo Tosatti <[EMAIL PROTECTED]> wrote: On Wed, Jan 17, 2007 at 06:01:24PM +, 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 [EMAIL PROTECTED] - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Marvell Libertas 8388 802.11b/g USB driver (v2)
On Wed, Jan 17, 2007 at 06:01:24PM +, 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
/
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
- * @returnn/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) {
-
Re: [PATCH] Marvell Libertas 8388 802.11b/g USB driver (v2)
On Wed, Jan 17, 2007 at 06:01:24PM +, 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? - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Marvell Libertas 8388 802.11b/g USB driver (v2)
Resending, apparently the first message got dropped somehow...
From: Marcelo Tosatti <[EMAIL PROTECTED]>
Date: Thu, 18 Jan 2007 13:02:13 -0200
To: Jiri Benc <[EMAIL PROTECTED]>
Cc: Dan Williams <[EMAIL PROTECTED]>, Marcelo Tosatti <[EMAIL PROTECTED]>,
netdev , Jeff Garzik <[EMAIL PROTECTED]>,
"John W. Linville" <[EMAIL PROTECTED]>,
"Luis R. Rodriguez" <[EMAIL PROTECTED]>,
Arnd Bergmann <[EMAIL PROTECTED]>,
Arnaldo Carvalho de Melo <[EMAIL PROTECTED]>
In-Reply-To: <[EMAIL PROTECTED]>
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 0x0001
> > +#define DW_BIT_1 0x0002
> > +#define DW_BIT_2 0x0004
> > +#define DW_BIT_3 0x0008
> > +#define DW_BIT_4 0x0010
> > +#define DW_BIT_5 0x0020
> > +#define DW_BIT_6 0x0040
> > +#define DW_BIT_7 0x0080
> > +#define DW_BIT_8 0x0100
> > +#define DW_BIT_9 0x0200
> > +#define DW_BIT_10 0x0400
> > +#define DW_BIT_11 0x0800
> > +#define DW_BIT_12 0x1000
> > +#define DW_BIT_13 0x2000
> > +#define DW_BIT_14 0x4000
> > +#define DW_BIT_15 0x8000
> > +#define DW_BIT_16 0x0001
> > +#define DW_BIT_17 0x0002
> > +#define DW_BIT_18 0x0004
> > +#define DW_BIT_19 0x0008
> > +#define DW_BIT_20 0x0010
> > +#define DW_BIT_21 0x0020
> > +#define DW_BIT_22 0x0040
> > +#define DW_BIT_23 0x0080
> > +#define DW_BIT_24 0x0100
> > +#define DW_BIT_25 0x0200
> > +#define DW_BIT_26 0x0400
> > +#define DW_BIT_27 0x0800
> > +#define DW_BIT_28 0x1000
> > +#define DW_BIT_29 0x2000
> > +#define DW_BIT_30 0x4000
> > +#define DW_BIT_31 0x8000
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 -
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Marvell Libertas 8388 802.11b/g USB driver (v2)
Got dropped somehow... - Forwarded message from Marcelo Tosatti <[EMAIL PROTECTED]> - Date: Thu, 18 Jan 2007 13:43:51 -0200 To: Jiri Benc <[EMAIL PROTECTED]> Cc: Dan Williams <[EMAIL PROTECTED]>, Johannes Berg <[EMAIL PROTECTED]>, Marcelo Tosatti <[EMAIL PROTECTED]>, netdev , Jeff Garzik <[EMAIL PROTECTED]>, "John W. Linville" <[EMAIL PROTECTED]>, "Luis R. Rodriguez" <[EMAIL PROTECTED]>, Arnd Bergmann <[EMAIL PROTECTED]>, Arnaldo Carvalho de Melo <[EMAIL PROTECTED]> In-Reply-To: <[EMAIL PROTECTED]> 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 - - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Marvell Libertas 8388 802.11b/g USB driver (v2)
On Thu, 2007-01-18 at 17:54 -0500, Jon Smirl wrote: > On 1/18/07, Christoph Hellwig <[EMAIL PROTECTED]> 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 - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Marvell Libertas 8388 802.11b/g USB driver (v2)
On Thu, 2007-01-18 at 22:40 +, 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 - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Marvell Libertas 8388 802.11b/g USB driver (v2)
On 1/18/07, Christoph Hellwig <[EMAIL PROTECTED]> 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 [EMAIL PROTECTED] - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Marvell Libertas 8388 802.11b/g USB driver (v2)
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. - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Marvell Libertas 8388 802.11b/g USB driver (v2)
On Wed, 2007-01-17 at 23:09 +, 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. - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Marvell Libertas 8388 802.11b/g USB driver (v2)
On Wed, 2007-01-17 at 23:06 +, 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 - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Marvell Libertas 8388 802.11b/g USB driver (v2)
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 [EMAIL PROTECTED] - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Marvell Libertas 8388 802.11b/g USB driver (v2)
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 - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Marvell Libertas 8388 802.11b/g USB driver (v2)
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. - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Marvell Libertas 8388 802.11b/g USB driver (v2)
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. - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Marvell Libertas 8388 802.11b/g USB driver (v2)
On Wed, Jan 17, 2007 at 07:52:13AM +, 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. - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Marvell Libertas 8388 802.11b/g USB driver (v2)
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 - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Marvell Libertas 8388 802.11b/g USB driver (v2)
On Wed, 2007-01-17 at 12:43 -0500, Dan Williams wrote:
> On Wed, 2007-01-17 at 15:18 +, 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 yo
Re: [PATCH] Marvell Libertas 8388 802.11b/g USB driver (v2)
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 signature.asc Description: This is a digitally signed message part
Re: [PATCH] Marvell Libertas 8388 802.11b/g USB driver (v2)
On Wed, 2007-01-17 at 15:18 +, 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
Re: [PATCH] Marvell Libertas 8388 802.11b/g USB driver (v2)
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 0x0001 > +#define DW_BIT_1 0x0002 > +#define DW_BIT_2 0x0004 > +#define DW_BIT_3 0x0008 > +#define DW_BIT_4 0x0010 > +#define DW_BIT_5 0x0020 > +#define DW_BIT_6 0x0040 > +#define DW_BIT_7 0x0080 > +#define DW_BIT_8 0x0100 > +#define DW_BIT_9 0x0200 > +#define DW_BIT_100x0400 > +#define DW_BIT_11 0x0800 > +#define DW_BIT_12 0x1000 > +#define DW_BIT_13 0x2000 > +#define DW_BIT_14 0x4000 > +#define DW_BIT_15 0x8000 > +#define DW_BIT_16 0x0001 > +#define DW_BIT_17 0x0002 > +#define DW_BIT_18 0x0004 > +#define DW_BIT_19 0x0008 > +#define DW_BIT_20 0x0010 > +#define DW_BIT_21 0x0020 > +#define DW_BIT_22 0x0040 > +#define DW_BIT_23 0x0080 > +#define DW_BIT_24 0x0100 > +#define DW_BIT_25 0x0200 > +#define DW_BIT_26 0x0400 > +#define DW_BIT_27 0x0800 > +#define DW_BIT_28 0x1000 > +#define DW_BIT_29 0x2000 > +#define DW_BIT_300x4000 > +#define DW_BIT_310x8000 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 - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Marvell Libertas 8388 802.11b/g USB driver (v2)
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 signature.asc Description: This is a digitally signed message part
Re: [PATCH] Marvell Libertas 8388 802.11b/g USB driver (v2)
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 - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Marvell Libertas 8388 802.11b/g USB driver (v2)
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 - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Marvell Libertas 8388 802.11b/g USB driver (v2)
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 signature.asc Description: This is a digitally signed message part
Re: [PATCH] Marvell Libertas 8388 802.11b/g USB driver (v2)
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 - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Marvell Libertas 8388 802.11b/g USB driver (v2)
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śkiewiczPLD/Linux Team arekm / maven.plhttp://ftp.pld-linux.org/ - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
