Re: [PATCH stable] Separate multicast configuration for mesh and wlan interfaces.
On Mon, 2008-05-19 at 07:01 -0400, Dan Williams wrote: Is the firmware multicast address limit the same for every firmware from 5.0.x up to 9? Is it something that people with the firmware dev kit can change with a recompile? Because if it changes between any of the firmware revisions already out there (including for 8385 CF, 8686 SDIO, 8388 USB, etc) then we'll probably need a lookup table. If we can avoid a lookup table, that would be nice. Perhaps we could list the MAC addresses after trying to set them, and see how many we get back? Unless there's a better way... I just hope the different firmwares does something sensible when they get more than they can handle? Like automatically going into ALLMULTI mode? That would be nice, but I find it unlikely. -- dwmw2 ___ Devel mailing list Devel@lists.laptop.org http://lists.laptop.org/listinfo/devel
Re: [PATCH stable] Separate multicast configuration for mesh and wlan interfaces.
On Sun, 2008-05-18 at 20:48 +0100, David Woodhouse wrote:
On Tue, 2008-05-13 at 15:38 +0100, David Woodhouse wrote:
On an SMP host, are you sure we can't end up setting the multicast list
simultaneously on the two logical devices?
(A: No.)
Try it like this... completely untested and hence probably broken in
some stupid and minor way, but testing is something for tomorrow, not
Sunday night when I'm supposed to be cooking dinner.
We now merge duplicates from the two address lists while we're building
the CMD_MAC_MULTICAST_ADR packet to send to the device, so we don't
artificially limit each device to MRVDRV_MAX_MULTICAST_LIST_SIZE/2
addresses. We'll go into allmulti mode only if the total number of
addresses, excluding duplicates, exceeds the limit. Although I'm not
wonderfully happy that we don't have any way of interrogating the
firmware for its limit; what happens when we send more addresses than
the firmware can cope with?
Is the firmware multicast address limit the same for every firmware from
5.0.x up to 9? Is it something that people with the firmware dev kit
can change with a recompile? Because if it changes between any of the
firmware revisions already out there (including for 8385 CF, 8686 SDIO,
8388 USB, etc) then we'll probably need a lookup table. I just hope the
different firmwares does something sensible when they get more than they
can handle?
Dan
We also deal with the locking issues -- that we could be in
lbs_set_multicast_list() for eth0 and msh0 simultaneously on two
different CPUs -- by punting the actual work to a workqueue. Which can
lock and use the multicast lists directly from each device, so we don't
need our own copy of each.
And it moves CMD_MAC_MULTICAST_ADR to a direct command, as we have been
doing for all commands.
Overall, it results in the net addition of five lines of code, instead
of the 64 lines added by the previous version :)
diff --git a/drivers/net/wireless/libertas/cmd.c
b/drivers/net/wireless/libertas/cmd.c
index 6328b95..2473ba8 100644
--- a/drivers/net/wireless/libertas/cmd.c
+++ b/drivers/net/wireless/libertas/cmd.c
@@ -778,28 +778,6 @@ out:
return ret;
}
-static int lbs_cmd_mac_multicast_adr(struct lbs_private *priv,
- struct cmd_ds_command *cmd,
- u16 cmd_action)
-{
- struct cmd_ds_mac_multicast_adr *pMCastAdr = cmd-params.madr;
-
- lbs_deb_enter(LBS_DEB_CMD);
- cmd-size = cpu_to_le16(sizeof(struct cmd_ds_mac_multicast_adr) +
- S_DS_GEN);
- cmd-command = cpu_to_le16(CMD_MAC_MULTICAST_ADR);
-
- lbs_deb_cmd(MULTICAST_ADR: setting %d addresses\n,
pMCastAdr-nr_of_adrs);
- pMCastAdr-action = cpu_to_le16(cmd_action);
- pMCastAdr-nr_of_adrs =
- cpu_to_le16((u16) priv-nr_of_multicastmacaddr);
- memcpy(pMCastAdr-maclist, priv-multicastlist,
-priv-nr_of_multicastmacaddr * ETH_ALEN);
-
- lbs_deb_leave(LBS_DEB_CMD);
- return 0;
-}
-
/**
* @brief Get the radio channel
*
@@ -1279,8 +1257,7 @@ void lbs_set_mac_control(struct lbs_private *priv)
cmd.action = cpu_to_le16(priv-mac_control);
cmd.reserved = 0;
- lbs_cmd_async(priv, CMD_MAC_CONTROL,
- cmd.hdr, sizeof(cmd));
+ lbs_cmd_async(priv, CMD_MAC_CONTROL, cmd.hdr, sizeof(cmd));
lbs_deb_leave(LBS_DEB_CMD);
}
@@ -1392,10 +1369,6 @@ int lbs_prepare_and_send_command(struct lbs_private
*priv,
cmdptr, cmd_action);
break;
- case CMD_MAC_MULTICAST_ADR:
- ret = lbs_cmd_mac_multicast_adr(priv, cmdptr, cmd_action);
- break;
-
case CMD_802_11_MONITOR_MODE:
ret = lbs_cmd_802_11_monitor_mode(cmdptr,
cmd_action, pdata_buf);
@@ -1484,7 +1457,7 @@ int lbs_prepare_and_send_command(struct lbs_private
*priv,
ret = lbs_cmd_bcn_ctrl(priv, cmdptr, cmd_action);
break;
default:
- lbs_deb_host(PREP_CMD: unknown command 0x%04x\n, cmd_no);
+ lbs_pr_err(PREP_CMD: unknown command 0x%04x\n, cmd_no);
ret = -1;
break;
}
diff --git a/drivers/net/wireless/libertas/cmdresp.c
b/drivers/net/wireless/libertas/cmdresp.c
index 5abecb7..4c3c5ec 100644
--- a/drivers/net/wireless/libertas/cmdresp.c
+++ b/drivers/net/wireless/libertas/cmdresp.c
@@ -316,7 +316,6 @@ static inline int handle_cmd_response(struct lbs_private
*priv,
break;
- case CMD_RET(CMD_MAC_MULTICAST_ADR):
case CMD_RET(CMD_802_11_RESET):
case CMD_RET(CMD_802_11_AUTHENTICATE):
case CMD_RET(CMD_802_11_BEACON_STOP):
@@ -376,8 +375,8 @@ static inline int handle_cmd_response(struct lbs_private
*priv,
break;
default:
- lbs_deb_host(CMD_RESP:
Re: [PATCH stable] Separate multicast configuration for mesh and wlan interfaces.
On Mon, 2008-05-19 at 12:05 +0100, David Woodhouse wrote: On Mon, 2008-05-19 at 07:01 -0400, Dan Williams wrote: Is the firmware multicast address limit the same for every firmware from 5.0.x up to 9? Is it something that people with the firmware dev kit can change with a recompile? Because if it changes between any of the firmware revisions already out there (including for 8385 CF, 8686 SDIO, 8388 USB, etc) then we'll probably need a lookup table. If we can avoid a lookup table, that would be nice. Perhaps we could list the MAC addresses after trying to set them, and see how many we get back? Unless there's a better way... That wouldn't help if some version of firmware couldn't handle the case when it's given more than it's max (see below). I just hope the different firmwares does something sensible when they get more than they can handle? Like automatically going into ALLMULTI mode? That would be nice, but I find it unlikely. No, like not bounds-checking the given list against an internal max value and overrunning a buffer, because it expects the driver to be bounds-checking the value and not giving it more than it can handle. That's probably not the case, but would be nice to have some confirmation that various firmware versions do handle this OK. If they do, then we can play some tricks like reading it back to see if they all get set correctly or whatnot. Dan ___ Devel mailing list Devel@lists.laptop.org http://lists.laptop.org/listinfo/devel
Re: [PATCH stable] Separate multicast configuration for mesh and wlan interfaces.
On Sun, 2008-05-18 at 20:48 +0100, David Woodhouse wrote:
Try it like this... completely untested and hence probably broken in
some stupid and minor way, but testing is something for tomorrow, not
Sunday night when I'm supposed to be cooking dinner.
This version seems to work, and as an added bonus even gets it right
when you bring devices up and down.
diff --git a/drivers/net/wireless/libertas/cmd.c
b/drivers/net/wireless/libertas/cmd.c
index 6328b95..2473ba8 100644
--- a/drivers/net/wireless/libertas/cmd.c
+++ b/drivers/net/wireless/libertas/cmd.c
@@ -778,28 +778,6 @@ out:
return ret;
}
-static int lbs_cmd_mac_multicast_adr(struct lbs_private *priv,
- struct cmd_ds_command *cmd,
- u16 cmd_action)
-{
- struct cmd_ds_mac_multicast_adr *pMCastAdr = cmd-params.madr;
-
- lbs_deb_enter(LBS_DEB_CMD);
- cmd-size = cpu_to_le16(sizeof(struct cmd_ds_mac_multicast_adr) +
-S_DS_GEN);
- cmd-command = cpu_to_le16(CMD_MAC_MULTICAST_ADR);
-
- lbs_deb_cmd(MULTICAST_ADR: setting %d addresses\n,
pMCastAdr-nr_of_adrs);
- pMCastAdr-action = cpu_to_le16(cmd_action);
- pMCastAdr-nr_of_adrs =
- cpu_to_le16((u16) priv-nr_of_multicastmacaddr);
- memcpy(pMCastAdr-maclist, priv-multicastlist,
- priv-nr_of_multicastmacaddr * ETH_ALEN);
-
- lbs_deb_leave(LBS_DEB_CMD);
- return 0;
-}
-
/**
* @brief Get the radio channel
*
@@ -1279,8 +1257,7 @@ void lbs_set_mac_control(struct lbs_private *priv)
cmd.action = cpu_to_le16(priv-mac_control);
cmd.reserved = 0;
- lbs_cmd_async(priv, CMD_MAC_CONTROL,
- cmd.hdr, sizeof(cmd));
+ lbs_cmd_async(priv, CMD_MAC_CONTROL, cmd.hdr, sizeof(cmd));
lbs_deb_leave(LBS_DEB_CMD);
}
@@ -1392,10 +1369,6 @@ int lbs_prepare_and_send_command(struct lbs_private
*priv,
cmdptr, cmd_action);
break;
- case CMD_MAC_MULTICAST_ADR:
- ret = lbs_cmd_mac_multicast_adr(priv, cmdptr, cmd_action);
- break;
-
case CMD_802_11_MONITOR_MODE:
ret = lbs_cmd_802_11_monitor_mode(cmdptr,
cmd_action, pdata_buf);
@@ -1484,7 +1457,7 @@ int lbs_prepare_and_send_command(struct lbs_private *priv,
ret = lbs_cmd_bcn_ctrl(priv, cmdptr, cmd_action);
break;
default:
- lbs_deb_host(PREP_CMD: unknown command 0x%04x\n, cmd_no);
+ lbs_pr_err(PREP_CMD: unknown command 0x%04x\n, cmd_no);
ret = -1;
break;
}
diff --git a/drivers/net/wireless/libertas/cmdresp.c
b/drivers/net/wireless/libertas/cmdresp.c
index 5abecb7..4c3c5ec 100644
--- a/drivers/net/wireless/libertas/cmdresp.c
+++ b/drivers/net/wireless/libertas/cmdresp.c
@@ -316,7 +316,6 @@ static inline int handle_cmd_response(struct lbs_private
*priv,
break;
- case CMD_RET(CMD_MAC_MULTICAST_ADR):
case CMD_RET(CMD_802_11_RESET):
case CMD_RET(CMD_802_11_AUTHENTICATE):
case CMD_RET(CMD_802_11_BEACON_STOP):
@@ -376,8 +375,8 @@ static inline int handle_cmd_response(struct lbs_private
*priv,
break;
default:
- lbs_deb_host(CMD_RESP: unknown cmd response 0x%04x\n,
-le16_to_cpu(resp-command));
+ lbs_pr_err(CMD_RESP: unknown cmd response 0x%04x\n,
+ le16_to_cpu(resp-command));
break;
}
lbs_deb_leave(LBS_DEB_HOST);
diff --git a/drivers/net/wireless/libertas/dev.h
b/drivers/net/wireless/libertas/dev.h
index 0d9edb9..e12ce65 100644
--- a/drivers/net/wireless/libertas/dev.h
+++ b/drivers/net/wireless/libertas/dev.h
@@ -140,6 +140,8 @@ struct lbs_private {
wait_queue_head_t waitq;
struct workqueue_struct *work_thread;
+ struct work_struct mcast_work;
+
/** Scanning */
struct delayed_work scan_work;
struct delayed_work assoc_work;
diff --git a/drivers/net/wireless/libertas/hostcmd.h
b/drivers/net/wireless/libertas/hostcmd.h
index f29bc5b..c36ab31 100644
--- a/drivers/net/wireless/libertas/hostcmd.h
+++ b/drivers/net/wireless/libertas/hostcmd.h
@@ -219,6 +219,7 @@ struct cmd_ds_mac_control {
};
struct cmd_ds_mac_multicast_adr {
+ struct cmd_header hdr;
__le16 action;
__le16 nr_of_adrs;
u8 maclist[ETH_ALEN * MRVDRV_MAX_MULTICAST_LIST_SIZE];
@@ -703,7 +704,6 @@ struct cmd_ds_command {
struct cmd_ds_802_11_rf_antenna rant;
struct cmd_ds_802_11_monitor_mode monitor;
struct cmd_ds_802_11_rate_adapt_rateset rateset;
- struct cmd_ds_mac_multicast_adr madr;
struct cmd_ds_802_11_ad_hoc_join adj;
struct cmd_ds_802_11_rssi rssi;
Re: [PATCH stable] Separate multicast configuration for mesh and wlan interfaces.
On Tue, 2008-05-13 at 15:38 +0100, David Woodhouse wrote:
On an SMP host, are you sure we can't end up setting the multicast list
simultaneously on the two logical devices?
(A: No.)
Try it like this... completely untested and hence probably broken in
some stupid and minor way, but testing is something for tomorrow, not
Sunday night when I'm supposed to be cooking dinner.
We now merge duplicates from the two address lists while we're building
the CMD_MAC_MULTICAST_ADR packet to send to the device, so we don't
artificially limit each device to MRVDRV_MAX_MULTICAST_LIST_SIZE/2
addresses. We'll go into allmulti mode only if the total number of
addresses, excluding duplicates, exceeds the limit. Although I'm not
wonderfully happy that we don't have any way of interrogating the
firmware for its limit; what happens when we send more addresses than
the firmware can cope with?
We also deal with the locking issues -- that we could be in
lbs_set_multicast_list() for eth0 and msh0 simultaneously on two
different CPUs -- by punting the actual work to a workqueue. Which can
lock and use the multicast lists directly from each device, so we don't
need our own copy of each.
And it moves CMD_MAC_MULTICAST_ADR to a direct command, as we have been
doing for all commands.
Overall, it results in the net addition of five lines of code, instead
of the 64 lines added by the previous version :)
diff --git a/drivers/net/wireless/libertas/cmd.c
b/drivers/net/wireless/libertas/cmd.c
index 6328b95..2473ba8 100644
--- a/drivers/net/wireless/libertas/cmd.c
+++ b/drivers/net/wireless/libertas/cmd.c
@@ -778,28 +778,6 @@ out:
return ret;
}
-static int lbs_cmd_mac_multicast_adr(struct lbs_private *priv,
- struct cmd_ds_command *cmd,
- u16 cmd_action)
-{
- struct cmd_ds_mac_multicast_adr *pMCastAdr = cmd-params.madr;
-
- lbs_deb_enter(LBS_DEB_CMD);
- cmd-size = cpu_to_le16(sizeof(struct cmd_ds_mac_multicast_adr) +
-S_DS_GEN);
- cmd-command = cpu_to_le16(CMD_MAC_MULTICAST_ADR);
-
- lbs_deb_cmd(MULTICAST_ADR: setting %d addresses\n,
pMCastAdr-nr_of_adrs);
- pMCastAdr-action = cpu_to_le16(cmd_action);
- pMCastAdr-nr_of_adrs =
- cpu_to_le16((u16) priv-nr_of_multicastmacaddr);
- memcpy(pMCastAdr-maclist, priv-multicastlist,
- priv-nr_of_multicastmacaddr * ETH_ALEN);
-
- lbs_deb_leave(LBS_DEB_CMD);
- return 0;
-}
-
/**
* @brief Get the radio channel
*
@@ -1279,8 +1257,7 @@ void lbs_set_mac_control(struct lbs_private *priv)
cmd.action = cpu_to_le16(priv-mac_control);
cmd.reserved = 0;
- lbs_cmd_async(priv, CMD_MAC_CONTROL,
- cmd.hdr, sizeof(cmd));
+ lbs_cmd_async(priv, CMD_MAC_CONTROL, cmd.hdr, sizeof(cmd));
lbs_deb_leave(LBS_DEB_CMD);
}
@@ -1392,10 +1369,6 @@ int lbs_prepare_and_send_command(struct lbs_private
*priv,
cmdptr, cmd_action);
break;
- case CMD_MAC_MULTICAST_ADR:
- ret = lbs_cmd_mac_multicast_adr(priv, cmdptr, cmd_action);
- break;
-
case CMD_802_11_MONITOR_MODE:
ret = lbs_cmd_802_11_monitor_mode(cmdptr,
cmd_action, pdata_buf);
@@ -1484,7 +1457,7 @@ int lbs_prepare_and_send_command(struct lbs_private *priv,
ret = lbs_cmd_bcn_ctrl(priv, cmdptr, cmd_action);
break;
default:
- lbs_deb_host(PREP_CMD: unknown command 0x%04x\n, cmd_no);
+ lbs_pr_err(PREP_CMD: unknown command 0x%04x\n, cmd_no);
ret = -1;
break;
}
diff --git a/drivers/net/wireless/libertas/cmdresp.c
b/drivers/net/wireless/libertas/cmdresp.c
index 5abecb7..4c3c5ec 100644
--- a/drivers/net/wireless/libertas/cmdresp.c
+++ b/drivers/net/wireless/libertas/cmdresp.c
@@ -316,7 +316,6 @@ static inline int handle_cmd_response(struct lbs_private
*priv,
break;
- case CMD_RET(CMD_MAC_MULTICAST_ADR):
case CMD_RET(CMD_802_11_RESET):
case CMD_RET(CMD_802_11_AUTHENTICATE):
case CMD_RET(CMD_802_11_BEACON_STOP):
@@ -376,8 +375,8 @@ static inline int handle_cmd_response(struct lbs_private
*priv,
break;
default:
- lbs_deb_host(CMD_RESP: unknown cmd response 0x%04x\n,
-le16_to_cpu(resp-command));
+ lbs_pr_err(CMD_RESP: unknown cmd response 0x%04x\n,
+ le16_to_cpu(resp-command));
break;
}
lbs_deb_leave(LBS_DEB_HOST);
diff --git a/drivers/net/wireless/libertas/dev.h
b/drivers/net/wireless/libertas/dev.h
index 0d9edb9..e12ce65 100644
--- a/drivers/net/wireless/libertas/dev.h
+++ b/drivers/net/wireless/libertas/dev.h
@@ -140,6 +140,8 @@ struct lbs_private {
Re: [PATCH stable] Separate multicast configuration for mesh and wlan interfaces.
On Tue, 2008-05-13 at 16:15 -0700, Andrew Morton wrote: On Tue, 13 May 2008 19:12:27 -0400 Andres Salomon [EMAIL PROTECTED] wrote: And FWIW, I like the 80 char limit _except_ when it comes to strings. I don't normally bother about the strings, unless it is obvious that the surrounding code has worked to prevent them from wrapping (and if I notice that). Or if they make code particularly hard to read and alter. I've seen some which wander out to column 130, which is getting daft. The code at http://userweb.kernel.org/~akpm/x.jpg has short strings, but it has gone and stuffed the _arguments_ onto the same line too, which is just obnoxious. I would probably accept patches to move the arguments onto the next line; I don't think that would make the overall code less readable. In fact, I would normally put the arguments on the next line these days anyway -- but that particular piece of debugging code dates from before all this pointless fuss about 80 columns got started, so it was never an issue. It could probably do with printk priorities too, while we're at it. -- dwmw2 ___ Devel mailing list Devel@lists.laptop.org http://lists.laptop.org/listinfo/devel
Re: [PATCH stable] Separate multicast configuration for mesh and wlan interfaces.
On Tue, 2008-05-13 at 15:06 -0700, Andrew Morton wrote: On Tue, 13 May 2008 22:59:26 +0100 David Woodhouse [EMAIL PROTECTED] wrote: On Tue, 2008-05-13 at 12:30 -0700, Andrew Morton wrote: On Tue, 13 May 2008 13:20:19 -0400 Andres Salomon [EMAIL PROTECTED] wrote: On Tue, 13 May 2008 15:45:39 +0100 David Woodhouse [EMAIL PROTECTED] wrote: On Tue, 2008-05-13 at 15:38 +0100, David Woodhouse wrote: And even without that, it doesn't seem to do the right thing. Set IFF_PROMISC mode on one interface, then on the other, then clear it on the first it should remain set in hardware. And AFAICT it doesn't. I'll see if I can make it work Hm, a single cup of tea mostly achieves that; sorry :) I was missing the fact that priv-packetfilter[] is now an array. It got a bit clearer after I reformatted it to stop trying to fit in 80 columns. Sometimes the code just doesn't fit; it's painful to try to make it. Gosh, I sure wish you, Andrew, checkpatch.pl, and Ingo[0] were all on the same page regarding that.. it would sure make my life easier. David is off in his own little world on this and can be safely ignored. Meanwhile the rest of us are forced to stare at crocks of shit like http://userweb.kernel.org/~akpm/x.jpg, wondering who hates us and why. I think the large amount of whitespace in the screenshot at http://david.woodhou.se/narrow.png shows quite effectively why I think you're talking nonsense on this particular topic. That's an 80-column display. Seems to be something like an 80-row display too. Even if you have one of those weird rotatable monitors and you've put it into portrait mode, it's not particularly realistic. Mine, on the other hand, is less contrived -- it's a web browser which is no wider than it _has_ to be these days, to view news.bbc.co.uk. And the code in question is not the example you chose, which I would accept patches for, but lbs_set_if_multicast_list() -- shown at http://david.woodhou.se/then-and-now.html in both the original 80-column and the more readable slightly wider versions. If that's the best your can do, you have nothing. In the past, after fixing the 80-column nonsense to make code more readable, I've immediately spotted bugs which weren't apparent before (commit f6f0f818, for example). I do try to keep code within 80 columns where I can; it's a reasonable guideline -- but I also accept that sometimes it just doesn't fit, and it would be foolish and counter-productive to try to force it. I'm sorry if that offends you, but making code more readable helps me find real bugs, and that is more important to me than the 80-column rule. -- dwmw2 ___ Devel mailing list Devel@lists.laptop.org http://lists.laptop.org/listinfo/devel
Re: [PATCH stable] Separate multicast configuration for mesh and wlan interfaces.
On Wed, 14 May 2008 09:44:12 +0100 David Woodhouse [EMAIL PROTECTED] wrote: I'm sorry if that offends you, but making code more readable helps me find real bugs, and that is more important to me than the 80-column rule. Code which wraps due to excess line sizes is less readable that code which avoids this. Your rhetorical trick of saying it is more readable is of course unsubstantiated and incorrect, which makes everything else you say baseless. ___ Devel mailing list Devel@lists.laptop.org http://lists.laptop.org/listinfo/devel
Re: [PATCH stable] Separate multicast configuration for mesh and wlan interfaces.
On Wed, 14 May 2008 10:39:19 +0100 David Woodhouse [EMAIL PROTECTED] wrote: On Wed, 2008-05-14 at 02:17 -0700, Andrew Morton wrote: On Wed, 14 May 2008 09:44:12 +0100 David Woodhouse [EMAIL PROTECTED] wrote: I'm sorry if that offends you, but making code more readable helps me find real bugs, and that is more important to me than the 80-column rule. Code which wraps due to excess line sizes is less readable that code which avoids this. Your rhetorical trick of saying it is more readable is of course unsubstantiated and incorrect, which makes everything else you say baseless. Andrew, I'm disappointed in you. You actually _removed_ a concrete example which substantiated my observation (an observation which Linus has also made), and then called it 'unsubstantiated and incorrect'. Some random individual anecdote is neither here nor there. I'm sure there are counter-anecdotes, but so what? Code which wraps in an 80-col display is hard to read in an 80-col display. Often very hard. ___ Devel mailing list Devel@lists.laptop.org http://lists.laptop.org/listinfo/devel
Re: [PATCH stable] Separate multicast configuration for mesh and wlan interfaces.
On Tue, 2008-05-13 at 19:12 -0400, Andres Salomon wrote: Can we come to a consensus for the sake of outside contributors? Rather than telling the cozybit folks one thing, and having checkpatch.pl and CodingStyle claim another (Dave, surely you wouldn't argue against using checkpatch?), can we get our stories straight? Please? Checkpatch is a useful tool but I use it with the line length check patched out, and I take the rest of its output with a pinch of salt. As for getting our stories straight... let's defer to Linus, who at various times has said the following: Quite frankly, I've several times been *this* close (holds up fingers so you can't even see between them) to just remove checkpatch entirely. I'm personally of the opinion that a lot of checkpatch fixes are anything but. That mainly concerns fixing overlong lines (where the fixed version is usually worse than the original), but it's been true for some other warnings too. -- http://lkml.org/lkml/2008/2/21/334 Quite frankly, I personally am considering removing checkpatch.pl. That thing is just a nazi dream. That hard-coded 80-character limit etc is just bad taste. Dammit, code cleanliness is not about automated and mindless slavish following of rules. A process that is too inflexible is a *bad* process. I'd much rather have a few 80+ character lines than stupid and unreadable line wrapping just because the line hit 87 characters in length. I don't have 25 lines on a screen either. -- http://lkml.org/lkml/2007/6/23/189 -- dwmw2 ___ Devel mailing list Devel@lists.laptop.org http://lists.laptop.org/listinfo/devel
Re: [PATCH stable] Separate multicast configuration for mesh and wlan interfaces.
On Wed, 2008-05-14 at 02:17 -0700, Andrew Morton wrote: On Wed, 14 May 2008 09:44:12 +0100 David Woodhouse [EMAIL PROTECTED] wrote: I'm sorry if that offends you, but making code more readable helps me find real bugs, and that is more important to me than the 80-column rule. Code which wraps due to excess line sizes is less readable that code which avoids this. Your rhetorical trick of saying it is more readable is of course unsubstantiated and incorrect, which makes everything else you say baseless. Andrew, I'm disappointed in you. You actually _removed_ a concrete example which substantiated my observation (an observation which Linus has also made), and then called it 'unsubstantiated and incorrect'. That's somewhat disingenuous of you. -- dwmw2 ___ Devel mailing list Devel@lists.laptop.org http://lists.laptop.org/listinfo/devel
Re: [PATCH stable] Separate multicast configuration for mesh and wlan interfaces.
-- http://lkml.org/lkml/2008/2/21/334 +1 -- http://lkml.org/lkml/2007/6/23/189 +1 I always mostly create patches which are checkpatch.pl clean, even following the 80 columns rule mostly, but sometimes I delibertaly ignored this rule. For me, it should be a 132 columns rule :-) ___ Devel mailing list Devel@lists.laptop.org http://lists.laptop.org/listinfo/devel
Re: [PATCH stable] Separate multicast configuration for mesh and wlan interfaces.
On Fri, 2008-05-09 at 21:00 -0700, [EMAIL PROTECTED] wrote: Each device maintains its own list of bound multicast addresses. Those lists are merged and purged from duplicate addresses before being sent to firmware. The maximum number of multicast addresses per virtual device has been cut in half to ensure that the merged list can be accommodated by the hardware. Also, configuration flags are ORed before being sent to firmware, which appears to be the least conflicting way of combining two virtual operating modes into one for a single physical device. Based on patches from Ashish Shukla and David Woodhouse. Signed-off-by: Javier Cardona [EMAIL PROTECTED] Tested by: Ricardo Carrano [EMAIL PROTECTED] Looks good, but please don't introduce any more of the 'u8' and 'u32' nonsense. If types are user-visible, we have to use the '__u32' form. If not, we might as well use the types that the C language provides. I've been slowly fixing that throughout the libertas driver as I've been rewriting it (which task is a bit on hold right now, I know...) On an SMP host, are you sure we can't end up setting the multicast list simultaneously on the two logical devices? -- dwmw2 ___ Devel mailing list Devel@lists.laptop.org http://lists.laptop.org/listinfo/devel
Re: [PATCH stable] Separate multicast configuration for mesh and wlan interfaces.
On Tue, 2008-05-13 at 13:47 +0100, David Woodhouse wrote: On Fri, 2008-05-09 at 21:00 -0700, [EMAIL PROTECTED] wrote: Each device maintains its own list of bound multicast addresses. Those lists are merged and purged from duplicate addresses before being sent to firmware. The maximum number of multicast addresses per virtual device has been cut in half to ensure that the merged list can be accommodated by the hardware. Also, configuration flags are ORed before being sent to firmware, which appears to be the least conflicting way of combining two virtual operating modes into one for a single physical device. Based on patches from Ashish Shukla and David Woodhouse. Signed-off-by: Javier Cardona [EMAIL PROTECTED] Tested by: Ricardo Carrano [EMAIL PROTECTED] Looks good, but please don't introduce any more of the 'u8' and 'u32' nonsense. If types are user-visible, we have to use the '__u32' form. If not, we might as well use the types that the C language provides. I've been slowly fixing that throughout the libertas driver as I've been rewriting it (which task is a bit on hold right now, I know...) On an SMP host, are you sure we can't end up setting the multicast list simultaneously on the two logical devices? (A: No.) And even without that, it doesn't seem to do the right thing. Set IFF_PROMISC mode on one interface, then on the other, then clear it on the first it should remain set in hardware. And AFAICT it doesn't. I'll see if I can make it work -- dwmw2 ___ Devel mailing list Devel@lists.laptop.org http://lists.laptop.org/listinfo/devel
Re: [PATCH stable] Separate multicast configuration for mesh and wlan interfaces.
On Tue, 2008-05-13 at 15:38 +0100, David Woodhouse wrote: And even without that, it doesn't seem to do the right thing. Set IFF_PROMISC mode on one interface, then on the other, then clear it on the first it should remain set in hardware. And AFAICT it doesn't. I'll see if I can make it work Hm, a single cup of tea mostly achieves that; sorry :) I was missing the fact that priv-packetfilter[] is now an array. It got a bit clearer after I reformatted it to stop trying to fit in 80 columns. Sometimes the code just doesn't fit; it's painful to try to make it. -- dwmw2 ___ Devel mailing list Devel@lists.laptop.org http://lists.laptop.org/listinfo/devel
Re: [PATCH stable] Separate multicast configuration for mesh and wlan interfaces.
The maximum number of multicast addresses per virtual device has been cut in half to ensure that the merged list can be accommodated by the hardware. If we allocated DRAM this way, no process could use more than 1/N of the memory, where N is the number of processes. Surely this is inappropriate. If eth0 is not in use, msh0 should be able to use all of the hardware's multicast addresses. And vice verse, and all points in between. Signal an error to the caller when both devices combined would exceed the hardware's capacity. John ___ Devel mailing list Devel@lists.laptop.org http://lists.laptop.org/listinfo/devel
Re: [PATCH stable] Separate multicast configuration for mesh and wlan interfaces.
On Tue, 13 May 2008 12:30:53 -0700
Andrew Morton [EMAIL PROTECTED] wrote:
On Tue, 13 May 2008 13:20:19 -0400
Andres Salomon [EMAIL PROTECTED] wrote:
On Tue, 13 May 2008 15:45:39 +0100
David Woodhouse [EMAIL PROTECTED] wrote:
On Tue, 2008-05-13 at 15:38 +0100, David Woodhouse wrote:
And even without that, it doesn't seem to do the right thing.
Set IFF_PROMISC mode on one interface, then on the other, then
clear it on the first it should remain set in hardware. And
AFAICT it doesn't.
I'll see if I can make it work
Hm, a single cup of tea mostly achieves that; sorry :)
I was missing the fact that priv-packetfilter[] is now an array.
It got a bit clearer after I reformatted it to stop trying to fit
in 80 columns. Sometimes the code just doesn't fit; it's painful
to try to make it.
Gosh, I sure wish you, Andrew, checkpatch.pl, and Ingo[0] were all
on the same page regarding that.. it would sure make my life easier.
David is off in his own little world on this and can be safely
ignored.
Meanwhile the rest of us are forced to stare at crocks of shit like
http://userweb.kernel.org/~akpm/x.jpg, wondering who hates us and why.
And Ingo's comments?
While I normally agree w/ breaking long lines up, when the entire line
is a string, I find it a large pain to break it up. I'm curious if
people would be against checkpatch.pl not complaining about lines 80
chars if the line contains a quoted string.
Actually, I wonder if the following patch would be acceptable (ignoring
the fact that it fails checkpatch.pl, of course :)
From: Andres Salomon [EMAIL PROTECTED]
Subject: [PATCH] checkpatch: make the 80-char-line check slightly more lax;
allow long strings
Currently, the 80-char-line check in checkpatch.pl doesn't allow the following:
+ printk(KERN_WARNING
+ one two three
fooo\n);
Instead, one must break the string up into multiple lines, like so:
+ printk(KERN_WARNING
+ one two three
+ fooo\n);
This is not really easier to read, and as Ingo has pointed out, it makes
life harder for people grepping for kernel messages. Of course, we don't
want to allow gratuitously long printk lines if they're unnecessary.
This patch allows a line to be 80 chars if it contains only a string and
some extra stuff at the end (',', ';', or ')'). Thus,
+ printk(KERN_WARNING
+ one two three
fooo\n);
and
+ printk(KERN_WARNING
+ one two three
fo\n,
+ xyz);
are allowed, but
+ printk(KERN_WARNING one two three
fooo\n);
and
+ printk(KERN_WARNING
+ one two three
foo\n, xyz);
are not.
Signed-off-by: Andres Salomon [EMAIL PROTECTED]
---
scripts/checkpatch.pl |2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index b6bbbcd..00f3d05 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1117,7 +1117,7 @@ sub process {
ERROR(trailing whitespace\n . $herevet);
}
#80 column limit
- if ($line =~ /^\+/ !($prevrawline=~/\/\*\*/) $length
80) {
+ if ($line =~ /^\+/ !($prevrawline=~/\/\*\*/) !($line =~
/^\+\s*/ $line =~ /[);,\s]*$/) $length 80) {
WARN(line over 80 characters\n . $herecurr);
}
--
1.5.5.1
___
Devel mailing list
Devel@lists.laptop.org
http://lists.laptop.org/listinfo/devel
Re: [PATCH stable] Separate multicast configuration for mesh and wlan interfaces.
On Tue, 13 May 2008 13:20:19 -0400 Andres Salomon [EMAIL PROTECTED] wrote: On Tue, 13 May 2008 15:45:39 +0100 David Woodhouse [EMAIL PROTECTED] wrote: On Tue, 2008-05-13 at 15:38 +0100, David Woodhouse wrote: And even without that, it doesn't seem to do the right thing. Set IFF_PROMISC mode on one interface, then on the other, then clear it on the first it should remain set in hardware. And AFAICT it doesn't. I'll see if I can make it work Hm, a single cup of tea mostly achieves that; sorry :) I was missing the fact that priv-packetfilter[] is now an array. It got a bit clearer after I reformatted it to stop trying to fit in 80 columns. Sometimes the code just doesn't fit; it's painful to try to make it. Gosh, I sure wish you, Andrew, checkpatch.pl, and Ingo[0] were all on the same page regarding that.. it would sure make my life easier. David is off in his own little world on this and can be safely ignored. Meanwhile the rest of us are forced to stare at crocks of shit like http://userweb.kernel.org/~akpm/x.jpg, wondering who hates us and why. ___ Devel mailing list Devel@lists.laptop.org http://lists.laptop.org/listinfo/devel
Re: [PATCH stable] Separate multicast configuration for mesh and wlan interfaces.
On Tue, 2008-05-13 at 12:30 -0700, Andrew Morton wrote: On Tue, 13 May 2008 13:20:19 -0400 Andres Salomon [EMAIL PROTECTED] wrote: On Tue, 13 May 2008 15:45:39 +0100 David Woodhouse [EMAIL PROTECTED] wrote: On Tue, 2008-05-13 at 15:38 +0100, David Woodhouse wrote: And even without that, it doesn't seem to do the right thing. Set IFF_PROMISC mode on one interface, then on the other, then clear it on the first it should remain set in hardware. And AFAICT it doesn't. I'll see if I can make it work Hm, a single cup of tea mostly achieves that; sorry :) I was missing the fact that priv-packetfilter[] is now an array. It got a bit clearer after I reformatted it to stop trying to fit in 80 columns. Sometimes the code just doesn't fit; it's painful to try to make it. Gosh, I sure wish you, Andrew, checkpatch.pl, and Ingo[0] were all on the same page regarding that.. it would sure make my life easier. David is off in his own little world on this and can be safely ignored. Meanwhile the rest of us are forced to stare at crocks of shit like http://userweb.kernel.org/~akpm/x.jpg, wondering who hates us and why. I think the large amount of whitespace in the screenshot at http://david.woodhou.se/narrow.png shows quite effectively why I think you're talking nonsense on this particular topic. -- dwmw2 ___ Devel mailing list Devel@lists.laptop.org http://lists.laptop.org/listinfo/devel
Re: [PATCH stable] Separate multicast configuration for mesh and wlan interfaces.
On Tue, 13 May 2008 22:59:26 +0100 David Woodhouse [EMAIL PROTECTED] wrote: On Tue, 2008-05-13 at 12:30 -0700, Andrew Morton wrote: On Tue, 13 May 2008 13:20:19 -0400 Andres Salomon [EMAIL PROTECTED] wrote: On Tue, 13 May 2008 15:45:39 +0100 David Woodhouse [EMAIL PROTECTED] wrote: On Tue, 2008-05-13 at 15:38 +0100, David Woodhouse wrote: And even without that, it doesn't seem to do the right thing. Set IFF_PROMISC mode on one interface, then on the other, then clear it on the first it should remain set in hardware. And AFAICT it doesn't. I'll see if I can make it work Hm, a single cup of tea mostly achieves that; sorry :) I was missing the fact that priv-packetfilter[] is now an array. It got a bit clearer after I reformatted it to stop trying to fit in 80 columns. Sometimes the code just doesn't fit; it's painful to try to make it. Gosh, I sure wish you, Andrew, checkpatch.pl, and Ingo[0] were all on the same page regarding that.. it would sure make my life easier. David is off in his own little world on this and can be safely ignored. Meanwhile the rest of us are forced to stare at crocks of shit like http://userweb.kernel.org/~akpm/x.jpg, wondering who hates us and why. I think the large amount of whitespace in the screenshot at http://david.woodhou.se/narrow.png shows quite effectively why I think you're talking nonsense on this particular topic. That's an 80-column display. If that's the best your can do, you have nothing. ___ Devel mailing list Devel@lists.laptop.org http://lists.laptop.org/listinfo/devel
Re: [PATCH stable] Separate multicast configuration for mesh and wlan interfaces.
On Tue, 13 May 2008 15:06:23 -0700 Andrew Morton [EMAIL PROTECTED] wrote: On Tue, 13 May 2008 22:59:26 +0100 David Woodhouse [EMAIL PROTECTED] wrote: On Tue, 2008-05-13 at 12:30 -0700, Andrew Morton wrote: On Tue, 13 May 2008 13:20:19 -0400 Andres Salomon [EMAIL PROTECTED] wrote: On Tue, 13 May 2008 15:45:39 +0100 David Woodhouse [EMAIL PROTECTED] wrote: On Tue, 2008-05-13 at 15:38 +0100, David Woodhouse wrote: And even without that, it doesn't seem to do the right thing. Set IFF_PROMISC mode on one interface, then on the other, then clear it on the first it should remain set in hardware. And AFAICT it doesn't. I'll see if I can make it work Hm, a single cup of tea mostly achieves that; sorry :) I was missing the fact that priv-packetfilter[] is now an array. It got a bit clearer after I reformatted it to stop trying to fit in 80 columns. Sometimes the code just doesn't fit; it's painful to try to make it. Gosh, I sure wish you, Andrew, checkpatch.pl, and Ingo[0] were all on the same page regarding that.. it would sure make my life easier. David is off in his own little world on this and can be safely ignored. Meanwhile the rest of us are forced to stare at crocks of shit like http://userweb.kernel.org/~akpm/x.jpg, wondering who hates us and why. I think the large amount of whitespace in the screenshot at http://david.woodhou.se/narrow.png shows quite effectively why I think you're talking nonsense on this particular topic. That's an 80-column display. If that's the best your can do, you have nothing. Can we come to a consensus for the sake of outside contributors? Rather than telling the cozybit folks one thing, and having checkpatch.pl and CodingStyle claim another (Dave, surely you wouldn't argue against using checkpatch?), can we get our stories straight? Please? And FWIW, I like the 80 char limit _except_ when it comes to strings. I've wasted too much time truncating strings, changing error messages so that the printk doesn't exceed 80 chars, etc. CodingStyle gives a nice example of how unreadable that sort of thing is. ___ Devel mailing list Devel@lists.laptop.org http://lists.laptop.org/listinfo/devel
Re: [PATCH stable] Separate multicast configuration for mesh and wlan interfaces.
On Tue, 13 May 2008 19:12:27 -0400 Andres Salomon [EMAIL PROTECTED] wrote: And FWIW, I like the 80 char limit _except_ when it comes to strings. I don't normally bother about the strings, unless it is obvious that the surrounding code has worked to prevent them from wrapping (and if I notice that). Or if they make code particularly hard to read and alter. I've seen some which wander out to column 130, which is getting daft. The code at http://userweb.kernel.org/~akpm/x.jpg has short strings, but it has gone and stuffed the _arguments_ onto the same line too, which is just obnoxious. ___ Devel mailing list Devel@lists.laptop.org http://lists.laptop.org/listinfo/devel
[PATCH stable] Separate multicast configuration for mesh and wlan interfaces.
Each device maintains its own list of bound multicast addresses. Those lists
are merged and purged from duplicate addresses before being sent to firmware.
The maximum number of multicast addresses per virtual device has been cut in
half to ensure that the merged list can be accommodated by the hardware.
Also, configuration flags are ORed before being sent to firmware, which
appears to be the least conflicting way of combining two virtual operating
modes into one for a single physical device.
Based on patches from Ashish Shukla and David Woodhouse.
Signed-off-by: Javier Cardona [EMAIL PROTECTED]
Tested by: Ricardo Carrano [EMAIL PROTECTED]
---
drivers/net/wireless/libertas/cmd.c | 58 ++--
drivers/net/wireless/libertas/defs.h |2 +
drivers/net/wireless/libertas/dev.h |9 +++-
drivers/net/wireless/libertas/main.c | 100 +++---
4 files changed, 117 insertions(+), 52 deletions(-)
diff --git a/drivers/net/wireless/libertas/cmd.c
b/drivers/net/wireless/libertas/cmd.c
index 0ae9851..a3cb4bd 100644
--- a/drivers/net/wireless/libertas/cmd.c
+++ b/drivers/net/wireless/libertas/cmd.c
@@ -814,21 +814,67 @@ static int lbs_cmd_mac_multicast_adr(struct lbs_private
*priv,
u16 cmd_action)
{
struct cmd_ds_mac_multicast_adr *pMCastAdr = cmd-params.madr;
+ u8 *mc_list_ptr = pMCastAdr-maclist;
+ u32 mc_count = 0, i, j, m, w;
+ DECLARE_MAC_BUF(mac);
+ int mesh_nr_mcastaddr, wlan_nr_mcastaddr;
lbs_deb_enter(LBS_DEB_CMD);
cmd-size = cpu_to_le16(sizeof(struct cmd_ds_mac_multicast_adr) +
S_DS_GEN);
cmd-command = cpu_to_le16(CMD_MAC_MULTICAST_ADR);
-
- lbs_deb_cmd(MULTICAST_ADR: setting %d addresses\n,
pMCastAdr-nr_of_adrs);
pMCastAdr-action = cpu_to_le16(cmd_action);
- pMCastAdr-nr_of_adrs =
- cpu_to_le16((u16) priv-nr_of_multicastmacaddr);
- memcpy(pMCastAdr-maclist, priv-multicastlist,
- priv-nr_of_multicastmacaddr * ETH_ALEN);
+ /* There are two multicast address lists in priv. Combine them into
+* one before sending to firmware.
+*/
+ m = MESH_MCAST_FILTER;
+ w = WLAN_MCAST_FILTER;
+
+ /* Only include addresses for interfaces that are up */
+ wlan_nr_mcastaddr = (priv-dev-flags IFF_UP) ?
+ priv-nr_of_multicastmacaddr[w] : 0;
+
+ mesh_nr_mcastaddr =
+ (priv-mesh_dev priv-mesh_dev-flags IFF_UP) ?
+ priv-nr_of_multicastmacaddr[m] : 0;
+
+ for (i = 0; i wlan_nr_mcastaddr; i++) {
+ /* Skip duplicate address */
+ int found = 0;
+ char *maddr = (char *)priv-multicastlist[w][i];
+
+ for (j = 0; j mesh_nr_mcastaddr; j++) {
+ if (!memcmp(maddr, priv-multicastlist[m][j],
+ ETH_ALEN)) {
+ found = 1;
+ break;
+ }
+ }
+ if (!found) {
+ memcpy(mc_list_ptr, maddr, ETH_ALEN);
+ lbs_deb_cmd(MULTICAST_ADR: %s added to mcast filter\n,
+ print_mac(mac, maddr));
+ mc_list_ptr += ETH_ALEN;
+ mc_count++;
+ }
+ }
+
+ /* Copy rest of the list */
+ for (j = 0; j mesh_nr_mcastaddr; j++) {
+ memcpy(mc_list_ptr, priv-multicastlist[m][j], ETH_ALEN);
+ lbs_deb_cmd(MULTICAST_ADR: %s added to mcast filter\n,
+ print_mac(mac, priv-multicastlist[m][j]));
+ mc_list_ptr += ETH_ALEN;
+ mc_count++;
+ }
+
+ pMCastAdr-nr_of_adrs = cpu_to_le16((u16)mc_count);
+ lbs_deb_cmd(MULTICAST_ADR: setting %d addresses\n,
+ pMCastAdr-nr_of_adrs);
lbs_deb_leave(LBS_DEB_CMD);
return 0;
+
}
/**
diff --git a/drivers/net/wireless/libertas/defs.h
b/drivers/net/wireless/libertas/defs.h
index 3053cc2..71ecb4c 100644
--- a/drivers/net/wireless/libertas/defs.h
+++ b/drivers/net/wireless/libertas/defs.h
@@ -126,6 +126,8 @@ static inline void lbs_deb_hex(unsigned int grp, const char
*prompt, u8 *buf, in
* station firmware to store Rx packet information.
*
* Current version of MAC has a 32x6 multicast address buffer.
+* The driver maintains separate multicast address lists for mesh and
+* wlan interfaces, each one of size MRVDRV_MAX_MULTICAST_LIST_SIZE/2.
*
* 802.11b can have up to 14 channels, the driver keeps the
* BSSID(MAC address) of each APs or Ad hoc stations it has sensed.
diff --git a/drivers/net/wireless/libertas/dev.h
b/drivers/net/wireless/libertas/dev.h
index 1c6ec4d..03e018e 100644
--- a/drivers/net/wireless/libertas/dev.h
+++ b/drivers/net/wireless/libertas/dev.h
@@ -230,8 +230,13
