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,
+ libertas_devinfo_fops);
And then here do a loop over that array.
Done.
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More
[PATCH] Marvell Libertas 8388 802.11b/g USB driver (v3)
Hi,
Here goes an updated patch of the Marvell Libertas 8388 802.11 USB
driver, addressing pretty much all comments from Arnd (the remaining
ones are listed in TODO entry below).
Diff can be found at
http://dev.laptop.org/~marcelo/libertas-8388-10022007.patch
Please review.
TODO:
- 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.
- Convert chan_freq_power/region_cfp_table to generic code, if possible.
Changes since v2:
- remove unused Command Processing States and Options defines
- remove unused HostCmd_DELAY_* defines
- remove unused DEFAULT_CHANNEL_* definitions
- remove changelog from host.h
- remove unused Marvell specific OIDs from host.h
- remove unused HostCmd_CMD_NONE from host.h
- remove changelog from all files
- remove wlan_ prefix from files
- remove usage of UTF8 character from LICENSE file
- remove unused HostCmd_DS_EEPROM_UPDATE structure
- remove unused code from 11d.h
- convert 11d.{c,h} from SillyCaps to kernel-style and remove
- convert {rx,tx}.c from SillyCaps to kernel-style
- use shared ieee80211_hdr_4addr instead of libertas_ieee80211_hdr
- ioctl.c needs to #include net/ieee80211.h
- use generic ARRAY_SIZE instead of self-defined macro
- remove unused defines from defs.h
- convert defs.h to kernel-style and remove unused code
- get rid of unnecessary ENTER/EXIT in cmd related fn's, random cleanups
- get rid of struct ethii_hdr, use generic struct ethhdr def.
- remove unused defines from host.h
- remove unused SDIO_HEADER_LEN define
- get rid of WLAN_STATUS_SUCCESS/FAILURE/NOT_ACCEPTED
- use BootCMDStr on if_usb_issue_boot_command stack
- increase nr_cmd_pending max value warning from 32 to 128
- remove reassociation code
- HostCmd_DS_802_11_ASSOCIATE_RSP struct requires no packed attr.
- reorder struct WLAN_802_11_KEY to avoid padding
- get rid of empty command response handlers in cmdresp.c
- replace (-ENOMEM) with -ENOMEM in if_usb_prob()
- convert HostCmd_DS_802_11_AFC to anonymous union/struct
- do not use extern declarations in .c files
- get rid of dynamic wlan_add_card/remove_card
- remove unnecessary/misleading commentary from if_usb.c
- usb_int_cause should be a per-device variable
- move bootcmdresp inside if_usb_receive_fwload stack
- reorganize if_usb.c
- remove unused declarations from if_usb.h
- cleanup sbi.h
- get rid of ASSERT, use WARN_ON
- convert printk statements to lbs_dev_xxx
- libertas_mpp_{get,set} should be static
- wake_pending_cmdnodes should be static
- mark WEXT local handlers as static
- do not set data.pointer in libertas_send_iwevcustom_event
- mark local functions as static in ethtool.c
- mark local functions from debugfs.c as static
- fix sparse warnings in cmdresp.c
- fix 11d.c and 11d.h signedness issues
- remove unused ioctl code
- ioctl.c using plain integer as NULL pointer
- remove unused WLAN_SETCONF_GETCONF ioctl
- remove useless ENTRY/LEAVE pairs from if_usb.c
- remove useless ENTRY/LEAVE pairs from 11d.c
- remove useless commentary from cmd.c
- readd version.h
- get rid of HandleDisconnectEvent()
- remove useless commentary from cmd.c
- cleanup fw.c commentary and ENTER/EXIT points
- remove useless commentary from ioctl.c
- remove useless commentary from join.c, assorted cleanups
- remove useless commentary from main.c
- remove useless commentary from {tx,rx}.c
- remove useless commentary from wext.c
- remove mention to reassoc code in README
- remove mention to getcis from README
- remove unused definitions from fw.h
- remove unused definitions from host.h
- no need for dynamic allocation of URB callback private data
- remove unused ASSOC_RSP_BUF_SIZE definition from join.h
- CmdCtrlNode - cmd_ctrl_node
- !DEBUG lbs_dbg_hex typo
- fix !DEBUG lbs_pr_debug
- hostcmd.h sillycaps conversion (1st pass)
- remove unused structure definitions from hostcmd.h
- hostcmd.h sillycaps conversion (2nd pass)
- fix OID_802 definitions
- convert host.h to normalcaps (1st pass)
- convert scan.c to normalcaps
- convert cmd.c to normalcaps
- convert cmdresp.c to normalcaps
- convert debugfs.c to normalcaps
- convert fw.c to normalcaps
- convert if_bootcmd.c to normalcaps
- convert if_usb.c, if_bootcmd and ioctl.c to normalcaps
- convert join.c to normalcaps
- convert wext.c to normalcaps
- convert main.c to normalcaps
- convert small headers to normalcaps
- convert types.h to normalcaps
- convert host.h to normalcaps (2nd pass)
- convert function names to normalcaps
- move processing of CMD_TYPE_{REQUEST,DATA} to inline functions
- don't do explicit casts of void* pointers
- convert remaining names to normalcaps
- wrap the subcmd data in a macro
- unify empty cases in libertas_process_rx_command
- move switch() from libertas_process_rx_command() to inline fn
- use arrays for debugfs file operations
- use get_zeroed_page/free_page on file methods (debugfs interface)
Changes since v1:
- reset usb device if boot2 init command fails
- resend initial boot command in case
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)
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_AUTHENTICATE0x0001 +#define HostCmd_ACT_DEAUTHENTICATE 0x0002 + +/* Define action or option for HostCmd_CMD_802_11_ASSOCIATE */ +#define HostCmd_ACT_ASSOCIATE
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 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 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)
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 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
- * @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)
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 netdev@vger.kernel.org, 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)
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 netdev@vger.kernel.org, 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)
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
[PATCH] Marvell Libertas 8388 802.11b/g USB driver (v2)
Announcing an updated patch of the Marvell Libertas 8388 802.11 USB driver. Diff can be found at http://dev.laptop.org/~marcelo/libertas-8388-16012007.patch _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 - 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
/* Channel flags. */
Did you send this part to netbsd also? We really don't want to fork
radiotap. ;) Also, this should be in a separate patch, but I'm guessing
it's
all rolled together for convenience.
No, especially since NetBSD is where I keep the authoritative definitions.
How have you defined RX_FLAGS and TX_FLAGS?
Oh yes, missed that part of the patch. Sorry.
BTW, such definitions are also used by Madwifi
(http://madwifi.org/changeset/1055) and other copies of the radiotap
header.
diff --git a/include/net/ieee80211_radiotap.h b/include/net/ieee80211_radiotap.h
index 429b738..c6e0d81 100644
--- a/include/net/ieee80211_radiotap.h
+++ b/include/net/ieee80211_radiotap.h
@@ -168,6 +168,23 @@ struct ieee80211_radiotap_header {
* Unitless indication of the Rx/Tx antenna for this packet.
* The first antenna is antenna 0.
*
+ * IEEE80211_RADIOTAP_RX_FLAGS u_int16_t bitmap
+ *
+ * Properties of received frames. See flags defined below.
+ *
+ * IEEE80211_RADIOTAP_TX_FLAGS u_int16_t bitmap
+ *
+ * Properties of transmitted frames. See flags defined below.
+ *
+ * IEEE80211_RADIOTAP_RTS_RETRIES u_int8_tdata
+ *
+ * Number of rts retries a transmitted frame used.
+ *
+ * IEEE80211_RADIOTAP_DATA_RETRIES u_int8_tdata
+ *
+ * Number of unicast retries a transmitted frame used.
+ *
+ *
* IEEE80211_RADIOTAP_FCS u32 data
*
* FCS from frame in network byte order.
@@ -187,7 +204,11 @@ enum ieee80211_radiotap_type {
IEEE80211_RADIOTAP_ANTENNA = 11,
IEEE80211_RADIOTAP_DB_ANTSIGNAL = 12,
IEEE80211_RADIOTAP_DB_ANTNOISE = 13,
- IEEE80211_RADIOTAP_EXT = 31,
+ IEEE80211_RADIOTAP_RX_FLAGS = 14,
+ IEEE80211_RADIOTAP_TX_FLAGS = 15,
+ IEEE80211_RADIOTAP_RTS_RETRIES = 16,
+ IEEE80211_RADIOTAP_DATA_RETRIES = 17,
+ IEEE80211_RADIOTAP_EXT = 31
};
/* Channel flags. */
@@ -219,6 +240,14 @@ #defineIEEE80211_RADIOTAP_F_DATAPAD0x2
* 802.11 header and payload
* (to 32-bit boundary)
*/
+/* For IEEE80211_RADIOTAP_RX_FLAGS */
+#define IEEE80211_RADIOTAP_F_RX_BADFCS 0x0001 /* frame failed crc check */
+
+/* For IEEE80211_RADIOTAP_TX_FLAGS */
+#define IEEE80211_RADIOTAP_F_TX_FAIL 0x0001 /* failed due to excessive
+* retries */
+#define IEEE80211_RADIOTAP_F_TX_CTS0x0002 /* used cts 'protection' */
+#define IEEE80211_RADIOTAP_F_TX_RTS0x0004 /* used rts/cts handshake */
/* Ugly macro to convert literal channel numbers into their mhz equivalents
* There are certianly some conditions that will break this (like feeding it
'30')
-
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
[PATCH] Marvell Libertas 8388 802.11b/g USB driver
Hi, Marvell released a GPL driver for their 8388 wireless chip for USB interface back in April. The 8388 is the device being used by the OLPC laptop. We have been working on cleaning it up for mainline inclusion. I think the moment for wider review has arrived. Since the diff is pretty large (700K) I've put it on the web at: http://dev.laptop.org/~marcelo/libertas-8388-15122006.patch And its GIT tree resides at: http://git.infradead.org/?p=libertas-2.6.git;a=log Comments are welcome - 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-2.4] forcedeth update to 0.50
On Wed, May 31, 2006 at 07:54:38AM +0200, Willy Tarreau wrote: On Wed, May 31, 2006 at 07:50:32AM +0200, Manfred Spraul wrote: Hi Willy, Willy Tarreau wrote: I started from the latest backport you sent in september (0.42) and incrementally applied 2.6 updates. I stopped at 0.50 which provides VLAN support, because after this one, there are some 2.4-incompatible changes (64bit consistent memory allocation for rings, and MSI/MSIX support). I agree, 2.4 needs a backport. Either a full backport as you did, or a minimal one-liner fix. Right now, the driver is not usable due to an incorrect initialization. Or to be more accurate: # modprobe # ifup works. But # modprobe # ifup # ifdown # ifup causes a misconfiguration, and the nic hangs hard after a few MB. And recent distros do the equivalent of ifup/ifdown/ifup somewhere in the initialization. That's what I read in one of the changelogs, but I'm not sure at all that it's what happened, because I had the problem after an ifup only. What I was doing with this box was pure performance tests which drew me to compare the broadcom and nforce performance. My tests measured 3 creteria : - number of HTTP/1.0 hits/s - maximum data rate - maximum packets/s on tg3, I got around 45 khits/s, 949 Mbps (TCP, =1.0 Gbps on wire) and 1.05 Mpps receive (I want to build a high speed load-balancer and a sniffer). This was stable. On the nforce, I tried with the hits/s first because it's a good indication of hardware-based and driver-based optimizations. It reached 18 khits/s with a lot of difficulty and the machine was stuck at 100% of one CPU. But it ran for a few minutes like this. Then I tried data rate (which is the same test with 1MB objects), and it failed after about 2 seconds and few megabytes (or hundreds of megabytes) transferred. I had to reboot to get it to work again. And I'm fairly sure that I did not do down/up this time as well, but the test came to the same end. That's why I'm not sure at all that the one-liner will be enough. Moreover, after the update, I reached the same performance as with the broadcom, with a slight improvement on packet reception (1.09 Mpps), and low CPU usage (15%). So basically, the upgrade rendered the driver from barely usable for SSH to very performant. Marcelo: Do you need a one-liner, or could you apply a large backport patch? I would really vote for the full backport, and I can break it into pieces if needed (I have them at hand, just have to re-inject the changelogs). However, I have separate changes from 0.42 to 0.50, because I started with your 0.30-0.42 backport patch. I have this machine till the end of the week, so I can perform other tests if you're interested in trying specific things. Since v2.4.33 should be out RSN, my opinion is that applying the one-liner to fix the bringup problem for now is more prudent.. Full patch could go into v2.4.34... - 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-2.4] forcedeth update to 0.50
On Wed, May 31, 2006 at 09:43:45PM +0200, Willy Tarreau wrote: On Wed, May 31, 2006 at 09:50:38PM +0200, Manfred Spraul wrote: Marcelo Tosatti wrote: Since v2.4.33 should be out RSN, my opinion is that applying the one-liner to fix the bringup problem for now is more prudent.. It's attached. Untested, but it should work. Just the relevant hunk from the 0.42 patch. I will test it tomorrow morning. John might be interested in merging it too, as I have checked today that RHEL3 was affected by the same problem (rmmod followed by modprobe). But I would disagree with waiting for 2.3.34 for a full backport: 0.30 basically doesn't work, thus the update to 0.50 would be a big step forward - it can't be worse that 0.30. Seconded ! Manfred, if you have some corner cases in mind, are aware of anything which might sometimes break, or have a few experimental patches to try, I'm OK for a few tests while I have the machine (it's SMP BTW). Alright then - will apply Willy's backport. - 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,2.4,SECURITY,NET] orinoco: CVE-2005-3180: Information leakage due to incorrect padding
On Wed, Feb 08, 2006 at 06:35:27PM +0900, Horms wrote: [PATCH] Better fixup for the orinoco driver The latest kernel added a pretty ugly fix for the orinoco etherleak bug which contains bogus skb-len checks already done by the caller and causes copies of all odd sized frames (which are quite common) While the skb-len check should be ripped out the other fix is harder to do properly so I'm proposing for this the -mm tree only until next 2.6.x so that it gets tested. Instead of copying buffers around blindly this code implements a padding aware version of the hermes buffer writing function which does padding as the buffer is loaded and thus more cleanly and without bogus 1.5K copies. Signed-off-by: Alan Cox [EMAIL PROTECTED] Signed-off-by: Andrew Morton [EMAIL PROTECTED] Signed-off-by: Jeff Garzik [EMAIL PROTECTED] The above is a patch included in 2.6.16 as a fix for CVE-2005-3180. It to be applicable to 2.4. I have made a backport below, with the only semi-significant change being including the ALIGN macro in orinoco.c, as it doesn't exist in 2.4. As yet untested Applied. In regard to testing: - similarity of 2.6 code guarantees certain testing coverage - v2.4.33 is in -pre stage, plenty of time for the fix to be on trial. Thanks Horms! - 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
