* Re: [PATCH] sdio: pass unknown cis tuples to sdio drivers
From: Albert Herranz @ 2009-09-11 7:52 UTC (permalink / raw)
To: Andrew Morton; +Cc: linux-mmc, bcm43xx-dev, linux-wireless
In-Reply-To: <20090910203951.041d5c17.akpm@linux-foundation.org>
--- El vie, 11/9/09, Andrew Morton <akpm@linux-foundation.org> escribió:
> > Some manufacturers provide vendor information in
> non-vendor specific CIS
> > tuples. For example, Broadcom uses an Extended
> Function tuple to provide
> > the MAC address on some of their network cards, as in
> the case of the
> > Nintendo Wii WLAN daughter card.
> >
> > This patch allows passing correct tuples unknown to
> the SDIO core to
> > a matching SDIO driver instead of rejecting them and
> failing.
> >
>
> This looks leaky to me.
>
Hi Andrew, thanks for the review.
I hope I can clarify a bit what the patch does in the next comments.
>
> : if (i < ARRAY_SIZE(cis_tpl_list)) {
> : const struct cis_tpl *tpl = cis_tpl_list + i;
> : if (tpl_link < tpl->min_size) {
> : printk(KERN_ERR
> : "%s: bad CIS tuple 0x%02x"
> : " (length = %u, expected >= %u)\n",
> : mmc_hostname(card->host),
> : tpl_code, tpl_link, tpl->min_size);
> : ret = -EINVAL;
>
> ret == -EINVAL
>
At this point ret is not -EINVAL. If it was -EINVAL the code would have had exit at this snipped before:
if (ret) {
kfree(this);
break;
}
> : } else if (tpl->parse) {
> : ret = tpl->parse(card, func,
> : this->data, tpl_link);
> : }
> : /* already successfully parsed, not needed anymore */
> : if (!ret)
> : kfree(this);
>
> `this' doesn't get freed
>
Yes, that's the whole point of the patch. It must be freed _only_ if the SDIO core parsed it. If the SDIO core cannot parse it then it gets passed to the SDIO driver via the SDIO func struct tuple list (later).
> : } else {
> : /* unknown tuple */
> : ret = -EILSEQ;
> : }
> :
> : if (ret == -EILSEQ) {
>
> `this' doesn't get remembered.
>
When ret is -EILSEQ `this' is linked to the SDIO func tuple list (later).
> : /* this tuple is unknown to the core */
> : this->next = NULL;
> : this->code = tpl_code;
> : this->size = tpl_link;
> : *prev = this;
> : prev = &this->next;
> : pr_debug("%s: queuing CIS tuple 0x%02x length %u\n",
> : mmc_hostname(card->host), tpl_code, tpl_link);
> : /* keep on analyzing tuples */
> : ret = 0;
> : }
> :
> : ptr += tpl_link;
>
> `this' leaks.
>
`this' doesn't leak. `this' has been linked to the SDIO func tuple list in:
*prev = this;
> : } while (!ret);
>
> Please check?
>
Thanks a lot for you comments,
Albert
^ permalink raw reply
* Re: [PATCH] cfg80211: allow scanning on specified frequencies when using wext-compatibility
From: Holger Schurig @ 2009-09-11 7:52 UTC (permalink / raw)
To: Johannes Berg; +Cc: John W Linville, linux-wireless
In-Reply-To: <1252629664.23427.4.camel@johannes.local>
> This is a bit weird -- this way you don't report errors if the
> user specified frequencies that don't exist.
The old code did this:
Loop over all bands
Loop over all channels
Stick channel to scan request
I simply added this:
Loop over all bands
Loop over all channels
If scan-request hasn't this channel freq: continue
Stick channel to scan request
Now, if I want to report an -EINVAL for every possibly invalid
scan-request channel, I'd have to do this:
If scan-request has freqs:
Loop over all scan-request freqs
Loop over all bands
Loop over all channels
search for freq
if found:
Stick channel to scan request
else:
err = -EINVAL
else:
Loop over all bands
Loop over all channels
Stick channel to scan request
This is considerable code-bloat for such a seldom-used function.
I'd rather do it like this:
Loop over all bands
Loop over all channels
If scan-request hasn't this channel freq: continue
Stick channel to scan request
if no channels:
err = -EINVAL
That's a compromise :-)
--
http://www.holgerschurig.de
^ permalink raw reply
* Re: [PATCH] sdio: pass unknown cis tuples to sdio drivers
From: Andrew Morton @ 2009-09-11 8:01 UTC (permalink / raw)
To: Albert Herranz; +Cc: linux-mmc, bcm43xx-dev, linux-wireless
In-Reply-To: <354996.83066.qm@web28302.mail.ukl.yahoo.com>
On Fri, 11 Sep 2009 07:52:11 +0000 (GMT) Albert Herranz <albert_herranz@yahoo.es> wrote:
> --- El vie, 11/9/09, Andrew Morton <akpm@linux-foundation.org> escribi__:
> > > Some manufacturers provide vendor information in
> > non-vendor specific CIS
> > > tuples. For example, Broadcom uses an Extended
> > Function tuple to provide
> > > the MAC address on some of their network cards, as in
> > the case of the
> > > Nintendo Wii WLAN daughter card.
> > >
> > > This patch allows passing correct tuples unknown to
> > the SDIO core to
> > > a matching SDIO driver instead of rejecting them and
> > failing.
> > >
> >
> > This looks leaky to me.
> >
>
> Hi Andrew, thanks for the review.
> I hope I can clarify a bit what the patch does in the next comments.
>
> >
> > : ______ ______ if (i < ARRAY_SIZE(cis_tpl_list)) {
argh, now yahoo mail is converting tabs to 0xa0's as well. Despair.
> > : ______ ______ ______ const struct cis_tpl *tpl = cis_tpl_list + i;
> > : ______ ______ ______ if (tpl_link < tpl->min_size) {
> > : ______ ______ ______ ______ printk(KERN_ERR
> > : ______ ______ ______ ______ __ __ ______"%s: bad CIS tuple 0x%02x"
> > : ______ ______ ______ ______ __ __ ______" (length = %u, expected >= %u)\n",
> > : ______ ______ ______ ______mmc_hostname(card->host),
> > : ______ ______ ______ ______ __ __ ______tpl_code, tpl_link, tpl->min_size);
> > : ______ ______ ______ ______ ret = -EINVAL;
> >
> > ret == -EINVAL
> >
>
> At this point ret is not -EINVAL.
Yes it is. We just did
ret = -EINVAL;
If that assignment happens, we leak `this'.
^ permalink raw reply
* [PATCH] cfg80211: minimal error handling for wext-compat freq scanning
From: Holger Schurig @ 2009-09-11 8:13 UTC (permalink / raw)
To: John W Linville, linux-wireless; +Cc: Johannes Berg
Signed-off-by: Holger Schurig <hs4233@mail.mn-solutions.de>
Index: linux-wl/net/wireless/scan.c
===================================================================
--- linux-wl.orig/net/wireless/scan.c 2009-09-11 09:05:39.000000000 +0200
+++ linux-wl/net/wireless/scan.c 2009-09-11 09:05:42.000000000 +0200
@@ -680,6 +680,11 @@ int cfg80211_wext_siwscan(struct net_dev
err = -EINVAL;
goto out;
}
+ /* No channels found? */
+ if (!i) {
+ err = -EINVAL;
+ goto out;
+ }
/* Set real number of channels specified in creq->channels[] */
creq->n_channels = i;
--
http://www.holgerschurig.de
^ permalink raw reply
* Re: [PATCH] sdio: pass unknown cis tuples to sdio drivers
From: Albert Herranz @ 2009-09-11 8:21 UTC (permalink / raw)
To: Pierre Ossman; +Cc: akpm, linux-mmc, bcm43xx-dev, linux-wireless
In-Reply-To: <20090911080659.2ac822fc@mjolnir.ossman.eu>
--- El vie, 11/9/09, Pierre Ossman <pierre@ossman.eu> escribió:
> The description for this patch should be made clearer. The title
> suggests it adds functionality that's already in place. It should be
> something along the lines of "Also pass malformed tuples to
> card drivers".
Hi Pierre,
Thanks for your patch review.
I didn't want to use "malformed" in the first place. I used "unknown" as "unknown to the SDIO core". The SDIO core in Linux only knows about FUNCE tuples of type 1 (with a sane length) as described in the SDIO Simplified Spec V2.00.
I think we just have a language issue here, but if you prefer the "malformed" wording I'm ok with that.
> In the sake of sanity, you might want to add this behaviour to all
> parsers, not just the FUNCE one.
I didn't find an application for the other parsers yet, so I tried to stick to the strictly necessary and just did the FUNCE one which has a direct application for Broadcom cards.
>
> I'm also unclear on how this is supposed to work. What does the
> broadcom tuple look like? This patch looks like it will silence a lot
> of legitimate warnings, and possibly pollute the card structures with
> bogus data.
The contents of the Broadcom FUNCE (type 0x22) tuple that contains the MAC address of a card looks like the following (tuple size 8):
04 06 00 1d bc 62 79 fd
^^ ^^ ^^^^^^^^^^^^^^^^^
| | |
| | +--- MAC address
| +--------------------- length (6 bytes for a ethernet MAC address)
+------------------------ type 4 (CISTPL_FUNCE_LAN_NODE_ID)
If you prefer it, instead of passing al "unknown" (or "malformed") FUNCE tuples to SDIO drivers, I can let through only a subset of whitelisted FUNCE types (starting with type 4).
>
> > diff --git a/drivers/mmc/core/sdio_cis.c b/drivers/mmc/core/sdio_cis.c
> > index 963f293..87934ac 100644
> > --- a/drivers/mmc/core/sdio_cis.c
> > +++ b/drivers/mmc/core/sdio_cis.c
> > @@ -123,8 +123,9 @@ static int cistpl_funce_func(struct sdio_func *func,
> > vsn = func->card->cccr.sdio_vsn;
> > min_size = (vsn == SDIO_SDIO_REV_1_00) ? 28 : 42;
> >
> > + /* let the SDIO driver take care of unknown tuples */
> > if (size < min_size || buf[0] != 1)
>
> Misleading comment, the tuple is not unknown.
>
Same language issue described before.
> > - return -EINVAL;
> > + return -EILSEQ;
> >
>
> What does this change improve?
-EILSEQ is used to indicate that the tuple was not parsed by the SDIO core and should be passed to the SDIO driver via the SDIO func tuple list.
>
> > /* TPLFE_MAX_BLK_SIZE */
> > func->max_blksize =
> buf[12] | (buf[13] << 8);
> > @@ -154,13 +155,7 @@ static int cistpl_funce(struct mmc_card *card, struct sdio_func *func,
> > else
> > ret = cistpl_funce_common(card, buf, size);
> >
> > - if (ret) {
> > - printk(KERN_ERR "%s: bad CISTPL_FUNCE size %u "
> > - "type %u\n", mmc_hostname(card->host), size, buf[0]);
> > - return ret;
> > - }
> > -
> > - return 0;
> > + return ret;
> > }
> >
> > typedef int (tpl_parse_t)(struct mmc_card *, struct sdio_func *,
>
> Silencing a legitimate error.
>
Yes, I see your point.
I think we can keep this code but prevent displaying the error if ret == -EILSEQ (i.e. the tuple is "unknown"/"malformed" BUT should be passed to the SDIO driver for parsing).
> > + if (ret == -EILSEQ) {
> > +
> /* this tuple is unknown to the core */
>
> Misleading comment, the tuple might be known but malformed.
Same languange issue again.
>
> Rgds
> --
> -- Pierre Ossman
Thanks a lot for your comments,
Albert
^ permalink raw reply
* [PATCH] cfg80211: use cfg80211_wext_freq() for freq conversion
From: Holger Schurig @ 2009-09-11 8:13 UTC (permalink / raw)
To: John W Linville, linux-wireless, Johannes Berg
WEXT's "struct iw_freq" can also be used to handle a channel. This patch now
uses cfg80211_wext_freq() instead of hand-converting the frequency. That
allows user-space to specify channels as well, like with SIOCSIWFREQ.
Signed-off-by: Holger Schurig <hs4233@mail.mn-solutions.de>
Index: linux-wl/net/wireless/scan.c
===================================================================
--- linux-wl.orig/net/wireless/scan.c 2009-09-11 09:05:27.000000000 +0200
+++ linux-wl/net/wireless/scan.c 2009-09-11 09:05:39.000000000 +0200
@@ -662,7 +662,7 @@ int cfg80211_wext_siwscan(struct net_dev
int k;
int wiphy_freq = wiphy->bands[band]->channels[j].center_freq;
for (k = 0; k < wreq->num_channels; k++) {
- int wext_freq = wreq->channel_list[k].m / 100000;
+ int wext_freq = cfg80211_wext_freq(wiphy, &wreq->channel_list[k]);
if (wext_freq == wiphy_freq)
goto wext_freq_found;
}
--
M&N Solutions GmbH Ein Unternehmen der Datagroup AG
Holger Schurig
Raiffeisenstr. 10
61191 Rosbach
Tel: 06003/9141-15 Fax 06003/9141-49
http://www.mn-solutions.de/
Handelsregister Friedberg, HRB 5903
Geschäftsführer: P.Schrittenlocher
^ permalink raw reply
* Re: [PATCH] sdio: pass unknown cis tuples to sdio drivers
From: Albert Herranz @ 2009-09-11 8:28 UTC (permalink / raw)
To: Andrew Morton; +Cc: linux-mmc, bcm43xx-dev, linux-wireless
In-Reply-To: <20090911010102.4c7840b0.akpm@linux-foundation.org>
--- El vie, 11/9/09, Andrew Morton <akpm@linux-foundation.org> escribió:
> > >
> > > ret == -EINVAL
> > >
> >
> > At this point ret is not -EINVAL.
>
> Yes it is. We just did
>
> ret = -EINVAL;
>
>
> If that assignment happens, we leak `this'.
>
Hi Andrew,
I misunderstood you. I thought that you were trying to imply on your original comment that retval was _already_ -EINVAL at that point.
Now I see the issue. `this' should be freed if successfully parsed (!ret) or if invalid and not going to be passed to a SDIO driver (ret == -EINVAL).
Thanks for catching that. I'll send an updated patch.
Cheers,
Albert
^ permalink raw reply
* [PATCH] wireless: default CONFIG_WLAN to y
From: Luis R. Rodriguez @ 2009-09-11 8:43 UTC (permalink / raw)
To: linville; +Cc: linux-wireless, Luis R. Rodriguez
When this was added no defaults were set and it seems
this implies n. Default this to y.
Reported-by: Jouni Malinen <jouni.malinen@atheros.com>
Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>
---
drivers/net/wireless/Kconfig | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/drivers/net/wireless/Kconfig b/drivers/net/wireless/Kconfig
index ad89d23..49ea9c9 100644
--- a/drivers/net/wireless/Kconfig
+++ b/drivers/net/wireless/Kconfig
@@ -5,6 +5,7 @@
menuconfig WLAN
bool "Wireless LAN"
depends on !S390
+ default y
---help---
This section contains all the pre 802.11 and 802.11 wireless
device drivers. For a complete list of drivers and documentation
--
1.6.3.3
^ permalink raw reply related
* Re: iwlagn: order 2 page allocation failures
From: Mel Gorman @ 2009-09-11 8:45 UTC (permalink / raw)
To: reinette chatre
Cc: Frans Pop, Larry Finger, John W. Linville, Pekka Enberg,
linux-kernel@vger.kernel.org, linux-wireless@vger.kernel.org,
ipw3945-devel@lists.sourceforge.net, Andrew Morton,
cl@linux-foundation.org, Krauss, Assaf, Johannes Berg,
Abbas, Mohamed
In-Reply-To: <1252606547.30150.304.camel@rc-desk>
On Thu, Sep 10, 2009 at 11:15:47AM -0700, reinette chatre wrote:
> > > We can thus use ___GFP_NOWARN for the allocations in
> > > iwl_rx_allocate and leave it to the restocking to find the needed memory
> > > when it tried its allocations using GFP_KERNEL.
> > >
> >
> > Would it be possible to use __GFP_NOWARN *unless* this allocation is
> > necessary to receive the packet?
>
> I think so.
>
> > > I do think it is useful to let user know about these allocation
> > > failures, even if it does not result in packet loss. Wrapping it in
> > > net_ratelimit() will help though.
> > >
> >
> > If it does not distinguish between failures causing packet loss and just a
> > temporary issue, I'd be worried the messages would generate bug reports and
> > we genuinely won't know if it's a real problem or not.
>
> Good point.
>
> >
> > As a total aside, there is still the problem that the driver is depending on
> > order-2 allocations. On systems without swap, the allocation problem could be
> > more severe as there are fewer pages the system can use to regain contiguity.
>
> It seems that somebody did think about this in the initialization of
> max_pkt_size (which is priv->hw_params.rx_buf_size - 256). If we use
> max_pkt_size in the code that allocates the skb then the 256 added for
> alignment will not cause it to go to an order-2 allocation. I do not
> know why max_pkt_size is not used at the moment and will have to do some
> digging to find out.
>
Thanks
> > > How about the patch below?
> > >
> > > diff --git a/drivers/net/wireless/iwlwifi/iwl-rx.c b/drivers/net/wireless/iwlwifi/iwl-rx.c
> > > index b90adcb..f0ee72e 100644
> > > --- a/drivers/net/wireless/iwlwifi/iwl-rx.c
> > > +++ b/drivers/net/wireless/iwlwifi/iwl-rx.c
> > > @@ -252,10 +252,11 @@ void iwl_rx_allocate(struct iwl_priv *priv, gfp_t priority)
> > >
> > > /* Alloc a new receive buffer */
> > > skb = alloc_skb(priv->hw_params.rx_buf_size + 256,
> > > - priority);
> > > + priority | __GFP_NOWARN);
> > >
> >
> > So, would it be possible here to only remove __GFP_NOWARN if this is GFP_ATOMIC
> > (implying we have to refill now) and the rxq->free_count is really small
> > e.g. <= 2?
>
> I like your suggestion. Considering the issue I would like to remove
> __GFP_NOWARN even if it is GFP_KERNEL ... I think it is actually even
> more of a problem if we are in GFP_KERNEL and not able to allocate
> memory when running low on buffers. Also, with the queue size of 256 I
> think we can use RX_LOW_WATERMARK here, which is 8.
>
RX_LOW_WATERMARK sounds reasonable as if that watermark is reached, the
buffer count is pretty low. With order-2 allocations, I bet the system is
beginning to grind a bit to find contiguous pages at that point as well.
I agree that it's a greater problem if the system is unable to allocate
the pages as GFP_KERNEL - prehaps to the extent where it's worth
distinguishing between GFP_KERNEL and GFP_ATOMIC failures. If GFP_KERNEL
allocations are failure, packet loss is likely and the system may not
recover, particularly if there is no swap configured.
> >
>
> >
> > > if (!skb) {
> > > - IWL_CRIT(priv, "Can not allocate SKB buffers\n");
> > > + if (net_ratelimit())
> > > + IWL_CRIT(priv, "Can not allocate SKB buffer.\n");
> >
> > Similarly, could the message either be supressed when there is enough
> > buffers in the RX queue or differenciate between enough buffers and
> > things getting tight possibly causing packet loss?
>
> Frans also had comments in this regard. Will do.
>
> >
> > The IWL_CRIT() part even is a hint - there is no guarantee that the allocation
> > failure is really a critical problem.
>
> Right.
>
> How about this:
>
> >From bd2153dd9e4a0ad588adec38c580d67023d5587e Mon Sep 17 00:00:00 2001
> From: Reinette Chatre <reinette.chatre@intel.com>
> Date: Wed, 9 Sep 2009 15:41:00 -0700
> Subject: [PATCH] iwlwifi: reduce noise when skb allocation fails
>
> Replenishment of receive buffers is done in the tasklet handling
> received frames as well as in a workqueue. When we are in the tasklet
> we cannot sleep and thus attempt atomic skb allocations. It is generally
> not a big problem if this fails since iwl_rx_allocate is always followed
> by a call to iwl_rx_queue_restock which will queue the work to replenish
> the buffers at a time when sleeping is allowed.
>
> We thus add the __GFP_NOWARN to the skb allocation in iwl_rx_allocate to
> reduce the noise if such an allocation fails while we still have enough
> buffers. We do maintain the warning and the error message when we are low
> on buffers to communicate to the user that there is a potential problem with
> memory availability on system
>
> This addresses issue reported upstream in thread "iwlagn: order 2 page
> allocation failures" in
> http://thread.gmane.org/gmane.linux.kernel.wireless.general/39187
>
> Signed-off-by: Reinette Chatre <reinette.chatre@intel.com>
> ---
> drivers/net/wireless/iwlwifi/iwl-rx.c | 12 +++++++++---
> drivers/net/wireless/iwlwifi/iwl3945-base.c | 8 +++++++-
> 2 files changed, 16 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/wireless/iwlwifi/iwl-rx.c b/drivers/net/wireless/iwlwifi/iwl-rx.c
> index b90adcb..cb50cc7 100644
> --- a/drivers/net/wireless/iwlwifi/iwl-rx.c
> +++ b/drivers/net/wireless/iwlwifi/iwl-rx.c
> @@ -250,12 +250,18 @@ void iwl_rx_allocate(struct iwl_priv *priv, gfp_t priority)
> }
> spin_unlock_irqrestore(&rxq->lock, flags);
>
> + if (rxq->free_count > RX_LOW_WATERMARK)
> + priority |= __GFP_NOWARN;
Seems very reasonable.
> /* Alloc a new receive buffer */
> - skb = alloc_skb(priv->hw_params.rx_buf_size + 256,
> - priority);
> + skb = alloc_skb(priv->hw_params.rx_buf_size + 256, priority);
>
This change appears superflous. It don't change any functionality. Looks
like the style is just being made consistent with a similar code block
elsewhere.
> if (!skb) {
> - IWL_CRIT(priv, "Can not allocate SKB buffers\n");
> + if (net_ratelimit())
> + IWL_DEBUG_INFO("Failed to allocate SKB buffer.\n");
> + if ((rxq->free_count <= RX_LOW_WATERMARK) &&
> + net_ratelimit())
> + IWL_CRIT(priv, "Failed to allocate SKB buffer. Only %u free buffers remaining\n",
> + rxq->free_count);
To get a good idea of how screwed we really are, how about?
IWL_CRIT(priv, "Failed to allocate SKB buffer with %s. Only %u free buffers remaining\n",
priority == GFP_ATOMIC ? "GFP_ATOMIC" : "GFP_KERNEL",
rxq->free_count);
> /* We don't reschedule replenish work here -- we will
> * call the restock method and if it still needs
> * more buffers it will schedule replenish */
> diff --git a/drivers/net/wireless/iwlwifi/iwl3945-base.c b/drivers/net/wireless/iwlwifi/iwl3945-base.c
> index 0909668..0d96b17 100644
> --- a/drivers/net/wireless/iwlwifi/iwl3945-base.c
> +++ b/drivers/net/wireless/iwlwifi/iwl3945-base.c
> @@ -1146,11 +1146,17 @@ static void iwl3945_rx_allocate(struct iwl_priv *priv, gfp_t priority)
> }
> spin_unlock_irqrestore(&rxq->lock, flags);
>
> + if (rxq->free_count > RX_LOW_WATERMARK)
> + priority |= __GFP_NOWARN;
> /* Alloc a new receive buffer */
> skb = alloc_skb(priv->hw_params.rx_buf_size, priority);
> if (!skb) {
> if (net_ratelimit())
> - IWL_CRIT(priv, ": Can not allocate SKB buffers\n");
> + IWL_DEBUG_INFO("Failed to allocate SKB buffer.\n");
> + if ((rxq->free_count <= RX_LOW_WATERMARK) &&
> + net_ratelimit())
> + IWL_CRIT(priv, "Failed to allocate SKB buffer. Only %u free buffers remaining\n",
> + rxq->free_count);
> /* We don't reschedule replenish work here -- we will
> * call the restock method and if it still needs
> * more buffers it will schedule replenish */
Otherwise, it looks just the finest and I think it will address the
problem to some extent - in that it won't print alarming messages when
they are not needed.
The additional changes with respect to GFP_ATOMIC are optional. Whether
you do it or not.
Acked-by: Mel Gorman <mel@csn.ul.ie>
Thanks very much.
--
Mel Gorman
Part-time Phd Student Linux Technology Center
University of Limerick IBM Dublin Software Lab
^ permalink raw reply
* Re: iwlagn: order 2 page allocation failures
From: Mel Gorman @ 2009-09-11 8:47 UTC (permalink / raw)
To: reinette chatre
Cc: Frans Pop, Larry Finger, John W. Linville, Pekka Enberg,
linux-kernel@vger.kernel.org, linux-wireless@vger.kernel.org,
ipw3945-devel@lists.sourceforge.net, Andrew Morton,
cl@linux-foundation.org, Krauss, Assaf, Johannes Berg,
Abbas, Mohamed
In-Reply-To: <1252617290.30150.321.camel@rc-desk>
On Thu, Sep 10, 2009 at 02:14:50PM -0700, reinette chatre wrote:
> On Thu, 2009-09-10 at 02:02 -0700, Mel Gorman wrote:
>
> >
> > As a total aside, there is still the problem that the driver is depending on
> > order-2 allocations. On systems without swap, the allocation problem could be
> > more severe as there are fewer pages the system can use to regain contiguity.
>
> I looked more at the implementation and hardware interface but I do not
> see a way around this. We have to provide 8k buffer to device, and we
> have to make sure it is aligned.
>
That would imply an order-1 allocation instead of an order-2 though so
it would appear than we are being worse than we have to. It would appear
to be because of this +256 bytes that goes onto every buffer.
> Do you have any suggestions?
>
Nothing concrete. Finding an alternative to having the socket buffer
8192+256 to make it an order-1 allocation would be an improvement but I
don't know how that should be tackled. Lacking the hardware, I can't
experiment myself :(
--
Mel Gorman
Part-time Phd Student Linux Technology Center
University of Limerick IBM Dublin Software Lab
^ permalink raw reply
* [PATCH v2] sdio: recognize io card without powercycle
From: Albert Herranz @ 2009-09-11 9:54 UTC (permalink / raw)
To: akpm, pierre, linux-mmc; +Cc: bcm43xx-dev, linux-wireless, Albert Herranz
SDIO Simplified Specification V2.00 states that it is strongly recommended
that the host executes either a power reset or issues a CMD52 (I/O Reset) to
re-initialize an I/O only card or the I/O portion of a combo card.
Additionally, the CMD52 must be issued first because it cannot be issued
after a CMD0.
With this patch the Nintendo Wii SDIO-based WLAN card is detected after a
system reset, without requiring a complete system powercycle.
Signed-off-by: Albert Herranz <albert_herranz@yahoo.es>
---
v1
- reworded commit message
- CC'd akpm as suggested by mb
v2
- renamed sdio_go_idle to sdio_reset as suggested by Pierre Ossman
drivers/mmc/core/core.c | 1 +
drivers/mmc/core/sdio_ops.c | 36 ++++++++++++++++++++++++++++++------
drivers/mmc/core/sdio_ops.h | 1 +
3 files changed, 32 insertions(+), 6 deletions(-)
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 2649117..dd746d9 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -868,6 +868,7 @@ void mmc_rescan(struct work_struct *work)
mmc_claim_host(host);
mmc_power_up(host);
+ sdio_reset(host);
mmc_go_idle(host);
mmc_send_if_cond(host, host->ocr_avail);
diff --git a/drivers/mmc/core/sdio_ops.c b/drivers/mmc/core/sdio_ops.c
index 4eb7825..dea36d9 100644
--- a/drivers/mmc/core/sdio_ops.c
+++ b/drivers/mmc/core/sdio_ops.c
@@ -67,13 +67,13 @@ int mmc_send_io_op_cond(struct mmc_host *host, u32 ocr, u32 *rocr)
return err;
}
-int mmc_io_rw_direct(struct mmc_card *card, int write, unsigned fn,
- unsigned addr, u8 in, u8* out)
+static int mmc_io_rw_direct_host(struct mmc_host *host, int write, unsigned fn,
+ unsigned addr, u8 in, u8 *out)
{
struct mmc_command cmd;
int err;
- BUG_ON(!card);
+ BUG_ON(!host);
BUG_ON(fn > 7);
/* sanity check */
@@ -90,11 +90,11 @@ int mmc_io_rw_direct(struct mmc_card *card, int write, unsigned fn,
cmd.arg |= in;
cmd.flags = MMC_RSP_SPI_R5 | MMC_RSP_R5 | MMC_CMD_AC;
- err = mmc_wait_for_cmd(card->host, &cmd, 0);
+ err = mmc_wait_for_cmd(host, &cmd, 0);
if (err)
return err;
- if (mmc_host_is_spi(card->host)) {
+ if (mmc_host_is_spi(host)) {
/* host driver already reported errors */
} else {
if (cmd.resp[0] & R5_ERROR)
@@ -106,7 +106,7 @@ int mmc_io_rw_direct(struct mmc_card *card, int write, unsigned fn,
}
if (out) {
- if (mmc_host_is_spi(card->host))
+ if (mmc_host_is_spi(host))
*out = (cmd.resp[0] >> 8) & 0xFF;
else
*out = cmd.resp[0] & 0xFF;
@@ -115,6 +115,13 @@ int mmc_io_rw_direct(struct mmc_card *card, int write, unsigned fn,
return 0;
}
+int mmc_io_rw_direct(struct mmc_card *card, int write, unsigned fn,
+ unsigned addr, u8 in, u8 *out)
+{
+ BUG_ON(!card);
+ return mmc_io_rw_direct_host(card->host, write, fn, addr, in, out);
+}
+
int mmc_io_rw_extended(struct mmc_card *card, int write, unsigned fn,
unsigned addr, int incr_addr, u8 *buf, unsigned blocks, unsigned blksz)
{
@@ -182,3 +189,20 @@ int mmc_io_rw_extended(struct mmc_card *card, int write, unsigned fn,
return 0;
}
+int sdio_reset(struct mmc_host *host)
+{
+ int ret;
+ u8 abort;
+
+ /* SDIO Simplified Specification V2.0, 4.4 Reset for SDIO */
+
+ ret = mmc_io_rw_direct_host(host, 0, 0, SDIO_CCCR_ABORT, 0, &abort);
+ if (ret)
+ abort = 0x08;
+ else
+ abort |= 0x08;
+
+ ret = mmc_io_rw_direct_host(host, 1, 0, SDIO_CCCR_ABORT, abort, NULL);
+ return ret;
+}
+
diff --git a/drivers/mmc/core/sdio_ops.h b/drivers/mmc/core/sdio_ops.h
index e2e74b0..12a4d3a 100644
--- a/drivers/mmc/core/sdio_ops.h
+++ b/drivers/mmc/core/sdio_ops.h
@@ -17,6 +17,7 @@ int mmc_io_rw_direct(struct mmc_card *card, int write, unsigned fn,
unsigned addr, u8 in, u8* out);
int mmc_io_rw_extended(struct mmc_card *card, int write, unsigned fn,
unsigned addr, int incr_addr, u8 *buf, unsigned blocks, unsigned blksz);
+int sdio_reset(struct mmc_host *host);
#endif
--
1.6.0.4
^ permalink raw reply related
* [PATCH v2] sdio: pass whitelisted cis funce tuples to sdio drivers
From: Albert Herranz @ 2009-09-11 9:54 UTC (permalink / raw)
To: akpm, pierre, linux-mmc; +Cc: bcm43xx-dev, linux-wireless, Albert Herranz
In-Reply-To: <1252662852-20548-1-git-send-email-albert_herranz@yahoo.es>
Some manufacturers provide vendor information in non-vendor specific CIS
tuples. For example, Broadcom uses an Extended Function tuple to provide
the MAC address on some of their network cards, as in the case of the
Nintendo Wii WLAN daughter card.
This patch allows passing whitelisted FUNCE tuples unknown to the SDIO core to
a matching SDIO driver instead of rejecting them and failing.
Signed-off-by: Albert Herranz <albert_herranz@yahoo.es>
---
v1
- fixed typo in commit message
- CC'd akpm as suggested by mb
- required by commit 4ea602e183ca20a7577ebe253323d0e5d0f9847 in net-next-2.6
v2
- renamed from "sdio: pass unknown cis tuples to sdio drivers"
- added funce tuple whitelist concept
- fixed leak reported by akpm
drivers/mmc/core/sdio_cis.c | 65 ++++++++++++++++++++++++++++++++----------
1 files changed, 49 insertions(+), 16 deletions(-)
diff --git a/drivers/mmc/core/sdio_cis.c b/drivers/mmc/core/sdio_cis.c
index 963f293..4e4fac1 100644
--- a/drivers/mmc/core/sdio_cis.c
+++ b/drivers/mmc/core/sdio_cis.c
@@ -98,6 +98,22 @@ static const unsigned char speed_val[16] =
static const unsigned int speed_unit[8] =
{ 10000, 100000, 1000000, 10000000, 0, 0, 0, 0 };
+/* FUNCE tuples with these types get passed to SDIO drivers */
+static const unsigned char funce_type_whitelist[] = {
+ 4 /* CISTPL_FUNCE_LAN_NODE_ID used in Broadcom cards */
+};
+
+static int cistpl_funce_whitelisted(unsigned char type)
+{
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(funce_type_whitelist); i++) {
+ if (funce_type_whitelist[i] == type)
+ return 1;
+ }
+ return 0;
+}
+
static int cistpl_funce_common(struct mmc_card *card,
const unsigned char *buf, unsigned size)
{
@@ -120,6 +136,10 @@ static int cistpl_funce_func(struct sdio_func *func,
unsigned vsn;
unsigned min_size;
+ /* let SDIO drivers take care of whitelisted FUNCE tuples */
+ if (cistpl_funce_whitelisted(buf[0]))
+ return -EILSEQ;
+
vsn = func->card->cccr.sdio_vsn;
min_size = (vsn == SDIO_SDIO_REV_1_00) ? 28 : 42;
@@ -154,13 +174,12 @@ static int cistpl_funce(struct mmc_card *card, struct sdio_func *func,
else
ret = cistpl_funce_common(card, buf, size);
- if (ret) {
+ if (ret && ret != -EILSEQ) {
printk(KERN_ERR "%s: bad CISTPL_FUNCE size %u "
"type %u\n", mmc_hostname(card->host), size, buf[0]);
- return ret;
}
- return 0;
+ return ret;
}
typedef int (tpl_parse_t)(struct mmc_card *, struct sdio_func *,
@@ -253,21 +272,12 @@ static int sdio_read_cis(struct mmc_card *card, struct sdio_func *func)
for (i = 0; i < ARRAY_SIZE(cis_tpl_list); i++)
if (cis_tpl_list[i].code == tpl_code)
break;
- if (i >= ARRAY_SIZE(cis_tpl_list)) {
- /* this tuple is unknown to the core */
- this->next = NULL;
- this->code = tpl_code;
- this->size = tpl_link;
- *prev = this;
- prev = &this->next;
- printk(KERN_DEBUG
- "%s: queuing CIS tuple 0x%02x length %u\n",
- mmc_hostname(card->host), tpl_code, tpl_link);
- } else {
+ if (i < ARRAY_SIZE(cis_tpl_list)) {
const struct cis_tpl *tpl = cis_tpl_list + i;
if (tpl_link < tpl->min_size) {
printk(KERN_ERR
- "%s: bad CIS tuple 0x%02x (length = %u, expected >= %u)\n",
+ "%s: bad CIS tuple 0x%02x"
+ " (length = %u, expected >= %u)\n",
mmc_hostname(card->host),
tpl_code, tpl_link, tpl->min_size);
ret = -EINVAL;
@@ -275,7 +285,30 @@ static int sdio_read_cis(struct mmc_card *card, struct sdio_func *func)
ret = tpl->parse(card, func,
this->data, tpl_link);
}
- kfree(this);
+ /*
+ * We don't need the tuple anymore if it was
+ * successfully parsed by the SDIO core or if it is
+ * not going to be parsed by SDIO drivers.
+ */
+ if (!ret || ret != -EILSEQ)
+ kfree(this);
+ } else {
+ /* unknown tuple */
+ ret = -EILSEQ;
+ }
+
+ if (ret == -EILSEQ) {
+ /* this tuple is unknown to the core or whitelisted */
+ this->next = NULL;
+ this->code = tpl_code;
+ this->size = tpl_link;
+ *prev = this;
+ prev = &this->next;
+ printk(KERN_DEBUG
+ "%s: queuing CIS tuple 0x%02x length %u\n",
+ mmc_hostname(card->host), tpl_code, tpl_link);
+ /* keep on analyzing tuples */
+ ret = 0;
}
ptr += tpl_link;
--
1.6.0.4
^ permalink raw reply related
* Re: [PATCH 3/4] ath5k: define ath_common ops
From: Bob Copeland @ 2009-09-11 11:35 UTC (permalink / raw)
To: Luis R. Rodriguez
Cc: Jiri Slaby, Nick Kossifidis, devel, ath9k-devel, linux-wireless,
Alan Cox, Linus Torvalds, Jeff Garzik
In-Reply-To: <43e72e890909110023k62a512bejd712a3449cc8328d@mail.gmail.com>
On Fri, Sep 11, 2009 at 3:23 AM, Luis R. Rodriguez
<lrodriguez@atheros.com> wrote:
> ath9k/ath9k/ath9k_htc. But -- is there really is a measurable cost
> penalty?
>
> This is why I asked if someone can test and give measurable
> differences over this. If there really isn't then that's not strong
> point against it.
Honestly, it probably won't matter in the grand scheme of things, but I
think if you are proposing a patch that touches every hotpath in two
drivers, then you need to do the work to say "by the way, this has benefit
X which outweighs the very small (or absent) performance regression Y,
and here are the numbers.
--
Bob Copeland %% www.bobcopeland.com
^ permalink raw reply
* Questions about regulatory domain & passive scanning
From: Holger Schurig @ 2009-09-11 12:27 UTC (permalink / raw)
To: linux-wireless, Luis R. Rodriguez
Hi !
I'm playing with regulatory domain using wireless-testing, iw,
crda and ath5 on Debian. Here are some observations:
1) Debian sucks here (while it's otherwise great!). No
wireless-regdb, wireless-crda packages, not even in
experimental. Maybe I need to add some Ubuntu repository
as "deb-src" to my apt sources.list, apt-get source that
stuff and recompile it manually.
2) I remove CONFIG_WIRELESS_OLD_REGULATORY. Then, after inserting
the card, I see in "iw list":
* 2412 MHz [1] (20.0 dBm)
* 2417 MHz [2] (20.0 dBm)
* 2422 MHz [3] (20.0 dBm)
* 2427 MHz [4] (20.0 dBm)
* 2432 MHz [5] (20.0 dBm)
* 2437 MHz [6] (20.0 dBm)
* 2442 MHz [7] (20.0 dBm)
* 2447 MHz [8] (20.0 dBm)
* 2452 MHz [9] (20.0 dBm)
* 2457 MHz [10] (20.0 dBm)
* 2462 MHz [11] (20.0 dBm)
* 2467 MHz [12] (20.0 dBm) (passive scanning)
* 2472 MHz [13] (20.0 dBm) (passive scanning)
* 2484 MHz [14] (20.0 dBm) (passive scanning)
I'd like to highlight the "passive scanning". Is this info
from the card EEPROM?
3) After "iw reg set DE" (for germany), it's now
2412 MHz [1] (20.0 dBm)
2417 MHz [2] (20.0 dBm)
2422 MHz [3] (20.0 dBm)
2427 MHz [4] (20.0 dBm)
2432 MHz [5] (20.0 dBm)
2437 MHz [6] (20.0 dBm)
2442 MHz [7] (20.0 dBm)
2447 MHz [8] (20.0 dBm)
2452 MHz [9] (20.0 dBm)
2457 MHz [10] (20.0 dBm)
2462 MHz [11] (20.0 dBm)
2467 MHz [12] (20.0 dBm) (passive scanning)
2472 MHz [13] (20.0 dBm) (passive scanning)
2484 MHz [14] (disabled)
Great, CRDA worked obviously: channel 14 has been disabled.
And, as I understand it, CRDA can just limit settings
further, not widening it. Therefore channels 12 and 13
are still marked as "passive scanning".
If I want to get them to "active scanning", would I need to
modify the EEPROM of the card? ath_info says "Reg. Domain:
0x60".
--
http://www.holgerschurig.de
^ permalink raw reply
* Re: [PATCH] b43: Add Soft-MAC SDIO device support
From: Michael Buesch @ 2009-09-11 13:44 UTC (permalink / raw)
To: Gábor Stefanik
Cc: John W. Linville, Albert Herranz, linux-wireless,
Broadcom Wireless
In-Reply-To: <69e28c910909101423j535c9cffq5b1be1cb00877645@mail.gmail.com>
On Thursday 10 September 2009 23:23:19 Gábor Stefanik wrote:
> On Thu, Sep 10, 2009 at 7:34 PM, Michael Buesch <mb@bu3sch.de> wrote:
> > From: Albert Herranz <albert_herranz@yahoo.es>
> >
> > This adds support for Soft-MAC SDIO devices to b43.
> > The driver still lacks some fixes for SDIO devices, so it's currently
> > marked as BROKEN.
>
> Is it actually completely broken; or already testable, just incomplete?
incomplete
--
Greetings, Michael.
^ permalink raw reply
* [PATCH] b43: Fix SDIO interrupt handler deadlock
From: Michael Buesch @ 2009-09-11 14:00 UTC (permalink / raw)
To: John W. Linville; +Cc: Broadcom Wireless, linux-wireless, Albert Herranz
We need to release the SDIO host before locking the driver mutex.
Signed-off-by: Michael Buesch <mb@bu3sch.de>
---
Index: wireless-testing/drivers/net/wireless/b43/main.c
===================================================================
--- wireless-testing.orig/drivers/net/wireless/b43/main.c 2009-09-10 20:26:43.000000000 +0200
+++ wireless-testing/drivers/net/wireless/b43/main.c 2009-09-11 15:55:19.000000000 +0200
@@ -1914,20 +1914,14 @@ static irqreturn_t b43_interrupt_handler
static void b43_sdio_interrupt_handler(struct b43_wldev *dev)
{
struct b43_wl *wl = dev->wl;
- struct sdio_func *func = dev->dev->bus->host_sdio;
irqreturn_t ret;
- if (unlikely(b43_status(dev) < B43_STAT_STARTED))
- return;
-
mutex_lock(&wl->mutex);
- sdio_release_host(func);
ret = b43_do_interrupt(dev);
if (ret == IRQ_WAKE_THREAD)
b43_do_interrupt_thread(dev);
- sdio_claim_host(func);
mutex_unlock(&wl->mutex);
}
Index: wireless-testing/drivers/net/wireless/b43/sdio.c
===================================================================
--- wireless-testing.orig/drivers/net/wireless/b43/sdio.c 2009-09-10 19:23:20.000000000 +0200
+++ wireless-testing/drivers/net/wireless/b43/sdio.c 2009-09-11 15:54:05.000000000 +0200
@@ -54,7 +54,12 @@ static void b43_sdio_interrupt_dispatche
struct b43_sdio *sdio = sdio_get_drvdata(func);
struct b43_wldev *dev = sdio->irq_handler_opaque;
+ if (unlikely(b43_status(dev) < B43_STAT_STARTED))
+ return;
+
+ sdio_release_host(func);
sdio->irq_handler(dev);
+ sdio_claim_host(func);
}
int b43_sdio_request_irq(struct b43_wldev *dev,
--
Greetings, Michael.
^ permalink raw reply
* [PATCH] ssb: Disable verbose SDIO coreswitch
From: Michael Buesch @ 2009-09-11 14:08 UTC (permalink / raw)
To: John W. Linville; +Cc: Broadcom Wireless, linux-wireless, Albert Herranz
Disable SDIO coreswitch debugging.
Signed-off-by: Michael Buesch <mb@bu3sch.de>
---
Index: wireless-testing/drivers/ssb/sdio.c
===================================================================
--- wireless-testing.orig/drivers/ssb/sdio.c 2009-09-08 19:29:16.000000000 +0200
+++ wireless-testing/drivers/ssb/sdio.c 2009-09-11 16:05:25.000000000 +0200
@@ -21,7 +21,7 @@
#include "ssb_private.h"
/* Define the following to 1 to enable a printk on each coreswitch. */
-#define SSB_VERBOSE_SDIOCORESWITCH_DEBUG 1
+#define SSB_VERBOSE_SDIOCORESWITCH_DEBUG 0
/* Hardware invariants CIS tuples */
--
Greetings, Michael.
^ permalink raw reply
* Re: [PATCH 3/4] ath5k: define ath_common ops
From: Linus Torvalds @ 2009-09-11 14:24 UTC (permalink / raw)
To: Luis R. Rodriguez
Cc: Jiri Slaby, Nick Kossifidis, devel, ath9k-devel, linux-wireless,
Alan Cox, Jeff Garzik
In-Reply-To: <43e72e890909110023k62a512bejd712a3449cc8328d@mail.gmail.com>
On Fri, 11 Sep 2009, Luis R. Rodriguez wrote:
>
> That is the way I had it originally before submission, and I
> completely agree its reasonable to not incur additional cost at the
> expense of having two separate read/write paths, and perhaps we should
> only incur the extra cost on routines shared between
> ath9k/ath9k/ath9k_htc. But -- is there really is a measurable cost
> penalty?
There's a measurable size penalty, at least.
In fact, if you know what kind of IO op it is (ie "it's always MMIO"),
you'd be even better using "writel()" directly, in which case it turns
into just a single store on most architectures, and doesn't cause all the
register save/restore of a function call.
Linus
^ permalink raw reply
* mac80211: NOHZ: local_softirq_pending 08
From: Michael Buesch @ 2009-09-11 14:48 UTC (permalink / raw)
To: linux-wireless; +Cc: netdev, Kalle.Valo, Johannes Berg
Hi,
mac80211 (or some other part of the networking stack) triggers this warning
in the NOHZ code:
NOHZ: local_softirq_pending 08
08 seems to be NET_RX_SOFTIRQ.
It happens, because my test driver b43 handles all RX and TX-status callbacks in
process context. I guess some part of the networking stack expects RX to be in
tasklet and/or softirq context.
We also have a report of this warning in wl1251, so it's probably not a b43 problem.
--
Greetings, Michael.
^ permalink raw reply
* Re: mac80211: NOHZ: local_softirq_pending 08
From: Kalle Valo @ 2009-09-11 14:57 UTC (permalink / raw)
To: Michael Buesch; +Cc: linux-wireless, netdev, Johannes Berg
In-Reply-To: <200909111648.50902.mb@bu3sch.de>
Michael Buesch <mb@bu3sch.de> writes:
> Hi,
Hallo,
> mac80211 (or some other part of the networking stack) triggers this
> warning in the NOHZ code: NOHZ: local_softirq_pending 08
>
> 08 seems to be NET_RX_SOFTIRQ.
>
> It happens, because my test driver b43 handles all RX and TX-status
> callbacks in process context. I guess some part of the networking
> stack expects RX to be in tasklet and/or softirq context.
>
> We also have a report of this warning in wl1251, so it's probably not
> a b43 problem.
Yes, I see this with wl1251. It uses workqueues everywhere.
--
Kalle Valo
^ permalink raw reply
* Re: mac80211: NOHZ: local_softirq_pending 08
From: Michael Buesch @ 2009-09-11 15:07 UTC (permalink / raw)
To: Kalle Valo; +Cc: linux-wireless, netdev, Johannes Berg, John W. Linville
In-Reply-To: <877hw534wd.fsf@litku.valot.fi>
On Friday 11 September 2009 16:57:54 Kalle Valo wrote:
> Michael Buesch <mb@bu3sch.de> writes:
>
> > Hi,
>
> Hallo,
>
> > mac80211 (or some other part of the networking stack) triggers this
> > warning in the NOHZ code: NOHZ: local_softirq_pending 08
> >
> > 08 seems to be NET_RX_SOFTIRQ.
> >
> > It happens, because my test driver b43 handles all RX and TX-status
> > callbacks in process context. I guess some part of the networking
> > stack expects RX to be in tasklet and/or softirq context.
> >
> > We also have a report of this warning in wl1251, so it's probably not
> > a b43 problem.
>
> Yes, I see this with wl1251. It uses workqueues everywhere.
>
This patch seems to fix it.
Signed-off-by: Michael Buesch <mb@bu3sch.de>
Index: wireless-testing/net/mac80211/cfg.c
===================================================================
--- wireless-testing.orig/net/mac80211/cfg.c 2009-08-09 18:47:11.000000000 +0200
+++ wireless-testing/net/mac80211/cfg.c 2009-09-11 16:59:12.000000000 +0200
@@ -606,7 +606,7 @@ static void ieee80211_send_layer2_update
skb->dev = sta->sdata->dev;
skb->protocol = eth_type_trans(skb, sta->sdata->dev);
memset(skb->cb, 0, sizeof(skb->cb));
- netif_rx(skb);
+ ieee80211_netif_rx(skb);
}
static void sta_apply_parameters(struct ieee80211_local *local,
Index: wireless-testing/net/mac80211/ieee80211_i.h
===================================================================
--- wireless-testing.orig/net/mac80211/ieee80211_i.h 2009-08-23 00:06:41.000000000 +0200
+++ wireless-testing/net/mac80211/ieee80211_i.h 2009-09-11 17:02:05.000000000 +0200
@@ -1053,6 +1053,14 @@ void ieee80211_tx_pending(unsigned long
int ieee80211_monitor_start_xmit(struct sk_buff *skb, struct net_device *dev);
int ieee80211_subif_start_xmit(struct sk_buff *skb, struct net_device *dev);
+/* rx handling */
+static inline int ieee80211_netif_rx(struct sk_buff *skb)
+{
+ if (in_interrupt())
+ return netif_rx(skb);
+ return netif_rx_ni(skb);
+}
+
/* HT */
void ieee80211_ht_cap_ie_to_sta_ht_cap(struct ieee80211_supported_band *sband,
struct ieee80211_ht_cap *ht_cap_ie,
Index: wireless-testing/net/mac80211/main.c
===================================================================
--- wireless-testing.orig/net/mac80211/main.c 2009-08-23 00:06:41.000000000 +0200
+++ wireless-testing/net/mac80211/main.c 2009-09-11 16:59:35.000000000 +0200
@@ -591,7 +591,7 @@ void ieee80211_tx_status(struct ieee8021
skb2 = skb_clone(skb, GFP_ATOMIC);
if (skb2) {
skb2->dev = prev_dev;
- netif_rx(skb2);
+ ieee80211_netif_rx(skb2);
}
}
@@ -600,7 +600,7 @@ void ieee80211_tx_status(struct ieee8021
}
if (prev_dev) {
skb->dev = prev_dev;
- netif_rx(skb);
+ ieee80211_netif_rx(skb);
skb = NULL;
}
rcu_read_unlock();
Index: wireless-testing/net/mac80211/rx.c
===================================================================
--- wireless-testing.orig/net/mac80211/rx.c 2009-09-04 19:08:05.000000000 +0200
+++ wireless-testing/net/mac80211/rx.c 2009-09-11 17:00:08.000000000 +0200
@@ -309,7 +309,7 @@ ieee80211_rx_monitor(struct ieee80211_lo
skb2 = skb_clone(skb, GFP_ATOMIC);
if (skb2) {
skb2->dev = prev_dev;
- netif_rx(skb2);
+ ieee80211_netif_rx(skb2);
}
}
@@ -320,7 +320,7 @@ ieee80211_rx_monitor(struct ieee80211_lo
if (prev_dev) {
skb->dev = prev_dev;
- netif_rx(skb);
+ ieee80211_netif_rx(skb);
} else
dev_kfree_skb(skb);
@@ -1349,7 +1349,7 @@ ieee80211_deliver_skb(struct ieee80211_r
/* deliver to local stack */
skb->protocol = eth_type_trans(skb, dev);
memset(skb->cb, 0, sizeof(skb->cb));
- netif_rx(skb);
+ ieee80211_netif_rx(skb);
}
}
@@ -1943,7 +1943,7 @@ static void ieee80211_rx_cooked_monitor(
skb2 = skb_clone(skb, GFP_ATOMIC);
if (skb2) {
skb2->dev = prev_dev;
- netif_rx(skb2);
+ ieee80211_netif_rx(skb2);
}
}
@@ -1954,7 +1954,7 @@ static void ieee80211_rx_cooked_monitor(
if (prev_dev) {
skb->dev = prev_dev;
- netif_rx(skb);
+ ieee80211_netif_rx(skb);
skb = NULL;
} else
goto out_free_skb;
--
Greetings, Michael.
^ permalink raw reply
* Re: mac80211: NOHZ: local_softirq_pending 08
From: Kalle Valo @ 2009-09-11 16:07 UTC (permalink / raw)
To: Michael Buesch; +Cc: linux-wireless, netdev, Johannes Berg, John W. Linville
In-Reply-To: <200909111707.23971.mb@bu3sch.de>
Michael Buesch <mb@bu3sch.de> writes:
>> > mac80211 (or some other part of the networking stack) triggers this
>> > warning in the NOHZ code: NOHZ: local_softirq_pending 08
>> >
>> > 08 seems to be NET_RX_SOFTIRQ.
>> >
>> > It happens, because my test driver b43 handles all RX and TX-status
>> > callbacks in process context. I guess some part of the networking
>> > stack expects RX to be in tasklet and/or softirq context.
>> >
>> > We also have a report of this warning in wl1251, so it's probably not
>> > a b43 problem.
>>
>> Yes, I see this with wl1251. It uses workqueues everywhere.
>>
>
> This patch seems to fix it.
Yes, it does. Thanks.
> Signed-off-by: Michael Buesch <mb@bu3sch.de>
Tested-by: Kalle Valo <kalle.valo@nokia.com>
--
Kalle Valo
^ permalink raw reply
* Re: mac80211: NOHZ: local_softirq_pending 08
From: Oliver Hartkopp @ 2009-09-11 16:07 UTC (permalink / raw)
To: Michael Buesch
Cc: Kalle Valo, linux-wireless, netdev, Johannes Berg,
John W. Linville
In-Reply-To: <200909111707.23971.mb@bu3sch.de>
Michael Buesch wrote:
> On Friday 11 September 2009 16:57:54 Kalle Valo wrote:
>> Michael Buesch <mb@bu3sch.de> writes:
>>
>>> Hi,
>> Hallo,
>>
>>> mac80211 (or some other part of the networking stack) triggers this
>>> warning in the NOHZ code: NOHZ: local_softirq_pending 08
>>>
>>> 08 seems to be NET_RX_SOFTIRQ.
>>>
>>> It happens, because my test driver b43 handles all RX and TX-status
>>> callbacks in process context. I guess some part of the networking
>>> stack expects RX to be in tasklet and/or softirq context.
>>>
>>> We also have a report of this warning in wl1251, so it's probably not
>>> a b43 problem.
>> Yes, I see this with wl1251. It uses workqueues everywhere.
>>
>
> This patch seems to fix it.
>
> Signed-off-by: Michael Buesch <mb@bu3sch.de>
>
> Index: wireless-testing/net/mac80211/cfg.c
> ===================================================================
> --- wireless-testing.orig/net/mac80211/cfg.c 2009-08-09 18:47:11.000000000 +0200
> +++ wireless-testing/net/mac80211/cfg.c 2009-09-11 16:59:12.000000000 +0200
> @@ -606,7 +606,7 @@ static void ieee80211_send_layer2_update
> skb->dev = sta->sdata->dev;
> skb->protocol = eth_type_trans(skb, sta->sdata->dev);
> memset(skb->cb, 0, sizeof(skb->cb));
> - netif_rx(skb);
> + ieee80211_netif_rx(skb);
> }
>
> static void sta_apply_parameters(struct ieee80211_local *local,
> Index: wireless-testing/net/mac80211/ieee80211_i.h
> ===================================================================
> --- wireless-testing.orig/net/mac80211/ieee80211_i.h 2009-08-23 00:06:41.000000000 +0200
> +++ wireless-testing/net/mac80211/ieee80211_i.h 2009-09-11 17:02:05.000000000 +0200
> @@ -1053,6 +1053,14 @@ void ieee80211_tx_pending(unsigned long
> int ieee80211_monitor_start_xmit(struct sk_buff *skb, struct net_device *dev);
> int ieee80211_subif_start_xmit(struct sk_buff *skb, struct net_device *dev);
>
> +/* rx handling */
> +static inline int ieee80211_netif_rx(struct sk_buff *skb)
> +{
> + if (in_interrupt())
> + return netif_rx(skb);
> + return netif_rx_ni(skb);
> +}
> +
Hello Michael,
i know this NOHZ warning from the CAN stack also - but now, i know what caused
this warning. I fixed it in my local tree and it works. Thanks!
As there are several users in the kernel do exact this test and call the
appropriate netif_rx() function, i would suggest to create a static inline
function:
static inline int netif_rx_ti(struct sk_buff *skb)
{
if (in_interrupt())
return netif_rx(skb);
return netif_rx_ni(skb);
}
('ti' for test in_interrupt())
in include/linux/netdevice.h
What do you think about that?
Regards,
Oliver
^ permalink raw reply
* Re: mac80211: NOHZ: local_softirq_pending 08
From: Michael Buesch @ 2009-09-11 16:13 UTC (permalink / raw)
To: Oliver Hartkopp
Cc: Kalle Valo, linux-wireless, netdev, Johannes Berg,
John W. Linville
In-Reply-To: <4AAA75CB.6040803@hartkopp.net>
On Friday 11 September 2009 18:07:39 Oliver Hartkopp wrote:
> Michael Buesch wrote:
> > On Friday 11 September 2009 16:57:54 Kalle Valo wrote:
> >> Michael Buesch <mb@bu3sch.de> writes:
> >>
> >>> Hi,
> >> Hallo,
> >>
> >>> mac80211 (or some other part of the networking stack) triggers this
> >>> warning in the NOHZ code: NOHZ: local_softirq_pending 08
> >>>
> >>> 08 seems to be NET_RX_SOFTIRQ.
> >>>
> >>> It happens, because my test driver b43 handles all RX and TX-status
> >>> callbacks in process context. I guess some part of the networking
> >>> stack expects RX to be in tasklet and/or softirq context.
> >>>
> >>> We also have a report of this warning in wl1251, so it's probably not
> >>> a b43 problem.
> >> Yes, I see this with wl1251. It uses workqueues everywhere.
> >>
> >
> > This patch seems to fix it.
> >
> > Signed-off-by: Michael Buesch <mb@bu3sch.de>
> >
> > Index: wireless-testing/net/mac80211/cfg.c
> > ===================================================================
> > --- wireless-testing.orig/net/mac80211/cfg.c 2009-08-09 18:47:11.000000000 +0200
> > +++ wireless-testing/net/mac80211/cfg.c 2009-09-11 16:59:12.000000000 +0200
> > @@ -606,7 +606,7 @@ static void ieee80211_send_layer2_update
> > skb->dev = sta->sdata->dev;
> > skb->protocol = eth_type_trans(skb, sta->sdata->dev);
> > memset(skb->cb, 0, sizeof(skb->cb));
> > - netif_rx(skb);
> > + ieee80211_netif_rx(skb);
> > }
> >
> > static void sta_apply_parameters(struct ieee80211_local *local,
> > Index: wireless-testing/net/mac80211/ieee80211_i.h
> > ===================================================================
> > --- wireless-testing.orig/net/mac80211/ieee80211_i.h 2009-08-23 00:06:41.000000000 +0200
> > +++ wireless-testing/net/mac80211/ieee80211_i.h 2009-09-11 17:02:05.000000000 +0200
> > @@ -1053,6 +1053,14 @@ void ieee80211_tx_pending(unsigned long
> > int ieee80211_monitor_start_xmit(struct sk_buff *skb, struct net_device *dev);
> > int ieee80211_subif_start_xmit(struct sk_buff *skb, struct net_device *dev);
> >
> > +/* rx handling */
> > +static inline int ieee80211_netif_rx(struct sk_buff *skb)
> > +{
> > + if (in_interrupt())
> > + return netif_rx(skb);
> > + return netif_rx_ni(skb);
> > +}
> > +
>
> Hello Michael,
>
> i know this NOHZ warning from the CAN stack also - but now, i know what caused
> this warning. I fixed it in my local tree and it works. Thanks!
>
> As there are several users in the kernel do exact this test and call the
> appropriate netif_rx() function, i would suggest to create a static inline
> function:
>
> static inline int netif_rx_ti(struct sk_buff *skb)
> {
> if (in_interrupt())
> return netif_rx(skb);
> return netif_rx_ni(skb);
> }
>
> ('ti' for test in_interrupt())
>
> in include/linux/netdevice.h
>
> What do you think about that?
Yeah, I'm fine with that.
--
Greetings, Michael.
^ permalink raw reply
* Re: iwlagn: order 2 page allocation failures
From: reinette chatre @ 2009-09-11 16:14 UTC (permalink / raw)
To: Mel Gorman
Cc: Frans Pop, Larry Finger, John W. Linville, Pekka Enberg,
linux-kernel@vger.kernel.org, linux-wireless@vger.kernel.org,
ipw3945-devel@lists.sourceforge.net, Andrew Morton,
cl@linux-foundation.org, Krauss, Assaf, Johannes Berg,
Abbas, Mohamed
In-Reply-To: <20090911084542.GA32497@csn.ul.ie>
On Fri, 2009-09-11 at 01:45 -0700, Mel Gorman wrote:
> Otherwise, it looks just the finest and I think it will address the
> problem to some extent - in that it won't print alarming messages when
> they are not needed.
>
> The additional changes with respect to GFP_ATOMIC are optional. Whether
> you do it or not.
>
> Acked-by: Mel Gorman <mel@csn.ul.ie>
Thank you very much. I'll make the changes you suggested.
Reinette
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox