* Re: [PATCH] wl1251: fix sparse-generated warnings
From: Luciano Coelho @ 2010-07-22 6:34 UTC (permalink / raw)
To: ext John W. Linville; +Cc: linux-wireless@vger.kernel.org, Kalle Valo
In-Reply-To: <1279729917-4451-1-git-send-email-linville@tuxdriver.com>
Hi John,
Great that someone finally made the endianess changes that we never had
the time to do for wl1251. Thanks for that! :)
I took a look at your patch and I have a few minor comments below, but
the best person to comment would certainly be Kalle, not me ;)
On Wed, 2010-07-21 at 18:31 +0200, ext John W. Linville wrote:
> diff --git a/drivers/net/wireless/wl12xx/wl1251_boot.c b/drivers/net/wireless/wl12xx/wl1251_boot.c
> index 2545123..c688895 100644
> --- a/drivers/net/wireless/wl12xx/wl1251_boot.c
> +++ b/drivers/net/wireless/wl12xx/wl1251_boot.c
[...]
> @@ -467,7 +467,7 @@ static int wl1251_boot_upload_nvs(struct wl1251 *wl)
> val = (nvs_ptr[0] | (nvs_ptr[1] << 8)
> | (nvs_ptr[2] << 16) | (nvs_ptr[3] << 24));
>
> - val = cpu_to_le32(val);
> + val = (u32 __force) cpu_to_le32(val);
This will work, but such casts always make me a bit suspicious. I think
this is fine for now, but later I think we should make sure that all the
_write() functions explicitly receive __le32 as val, or receives the
cpu's u32 and converts it before actually writing the value, for clarity
reasons. Kalle, what do you think?
> diff --git a/drivers/net/wireless/wl12xx/wl1251_tx.c b/drivers/net/wireless/wl12xx/wl1251_tx.c
> index c822318..a38ec19 100644
> --- a/drivers/net/wireless/wl12xx/wl1251_tx.c
> +++ b/drivers/net/wireless/wl12xx/wl1251_tx.c
[...]
> @@ -191,11 +191,13 @@ static int wl1251_tx_send_packet(struct wl1251 *wl, struct sk_buff *skb,
> if (control->control.hw_key &&
> control->control.hw_key->alg == ALG_TKIP) {
> int hdrlen;
> - u16 fc;
> + __le16 fc;
> + u16 length;
> u8 *pos;
>
> - fc = *(u16 *)(skb->data + sizeof(*tx_hdr));
> - tx_hdr->length += WL1251_TKIP_IV_SPACE;
> + fc = *(__le16 *)(skb->data + sizeof(*tx_hdr));
Is this going to work? sizeof(*tx_hdr), and the operation, will be in
the cpu's endianess, right? Wouldn't the following be the right thing to
do then?
fc = cpu_to_le16(le16_to_cpu(skb->data) + sizeof(*tx_hdr));
Maybe some casts are needed too, I didn't check that, but regarding the
endianess, I think this is how it should go. It's the same thing as the
length parameter:
> + length = le16_to_cpu(tx_hdr->length) + WL1251_TKIP_IV_SPACE;
> + tx_hdr->length = cpu_to_le16(length);
...which is treated correctly here.
--
Cheers,
Luca.
^ permalink raw reply
* Re: wl1271 firmware
From: Luciano Coelho @ 2010-07-22 6:40 UTC (permalink / raw)
To: ext Pazzo Da Legare
Cc: linux-wireless@vger.kernel.org, Levi, Shahar, Logan Gunthorpe
In-Reply-To: <AANLkTinnYeJPV-xxg7DdPXnWW_kD7X1TVcd9HrhupJWS@mail.gmail.com>
Hi Pazzo,
On Wed, 2010-07-21 at 19:27 +0200, ext Pazzo Da Legare wrote:
> Dear All again,
>
> I've just test with compact-wireless-2010-07-20 but I've the same problem:
>
> root@hawkboard:~# ifconfig wlan0 192.168.0.1 up
> [ 68.920000] wl1271_sdio mmc0:0001:2: firmware: requesting wl1271-fw.bin
> [ 69.100000] wl1271_sdio mmc0:0001:2: firmware: requesting wl1271-nvs.bin
> [ 69.510000] ADDRCONF(NETDEV_UP): wlan0: link is not ready
> root@hawkboard:~# [ 70.020000] wl1271: ERROR ELP wakeup timeout!
> [ 70.520000] wl1271: ERROR ELP wakeup timeout!
>
>
> looking at dmesg:
>
> [ 68.920000] wl1271_sdio mmc0:0001:2: firmware: requesting wl1271-fw.bin
> [ 69.100000] wl1271_sdio mmc0:0001:2: firmware: requesting wl1271-nvs.bin
> [ 69.500000] wl1271: firmware booted (Rev 6.1.0.0.310)
> [ 69.510000] ADDRCONF(NETDEV_UP): wlan0: link is not ready
> [ 70.020000] wl1271: ERROR ELP wakeup timeout!
> [ 70.520000] wl1271: ERROR ELP wakeup timeout!
>
> [ 68.920000] wl1271_sdio mmc0:0001:2: firmware: requesting wl1271-fw.bin
> [ 69.100000] wl1271_sdio mmc0:0001:2: firmware: requesting wl1271-nvs.bin
> [ 69.500000] wl1271: firmware booted (Rev 6.1.0.0.310)
> [ 70.020000] wl1271: ERROR ELP wakeup timeout!
> [ 70.520000] wl1271: ERROR ELP wakeup timeout!
>
> Do you have any clue?
I'm not sure what this is, but usually an ERROR ELP wakeup timeout means
that the firmware has crashed.
Could you enable the following DEBUG flags in wl1271.h and send the logs
again? Maybe they provide more clues...
#define DEBUG_LEVEL (DEBUG_BOOT | DEBUG_ACX | DEBUG_CMD | DEBUG_PSM | \
DEBUG_IRQ | DEBUG_EVENT)
--
Cheers,
Luca.
^ permalink raw reply
* Re: [PATCH] wl1251: fix sparse-generated warnings
From: Luciano Coelho @ 2010-07-22 7:38 UTC (permalink / raw)
To: ext John W. Linville; +Cc: linux-wireless@vger.kernel.org, Kalle Valo
In-Reply-To: <1279780458.2322.25.camel@powerslave>
On Thu, 2010-07-22 at 08:34 +0200, Coelho Luciano (Nokia-MS/Helsinki)
wrote:
> > @@ -191,11 +191,13 @@ static int wl1251_tx_send_packet(struct wl1251 *wl, struct sk_buff *skb,
> > if (control->control.hw_key &&
> > control->control.hw_key->alg == ALG_TKIP) {
> > int hdrlen;
> > - u16 fc;
> > + __le16 fc;
> > + u16 length;
> > u8 *pos;
> >
> > - fc = *(u16 *)(skb->data + sizeof(*tx_hdr));
> > - tx_hdr->length += WL1251_TKIP_IV_SPACE;
> > + fc = *(__le16 *)(skb->data + sizeof(*tx_hdr));
>
> Is this going to work? sizeof(*tx_hdr), and the operation, will be in
> the cpu's endianess, right? Wouldn't the following be the right thing to
> do then?
>
> fc = cpu_to_le16(le16_to_cpu(skb->data) + sizeof(*tx_hdr));
Ugh, as Johannes pointed out, what I said here is completely non-sense.
Please ignore this before-my-morning-coffee lapse. ;)
--
Cheers,
Luca.
^ permalink raw reply
* Re: [PATCH] wl1251: fix sparse-generated warnings
From: Kalle Valo @ 2010-07-22 7:45 UTC (permalink / raw)
To: Luciano Coelho; +Cc: ext John W. Linville, linux-wireless@vger.kernel.org
In-Reply-To: <1279780458.2322.25.camel@powerslave>
On 07/22/2010 08:34 AM, Luciano Coelho wrote:
> Hi John,
Hi Luca and John,
> Great that someone finally made the endianess changes that we never had
> the time to do for wl1251. Thanks for that! :)
Yeah, it really is. wl1251 endian support has been broken almost from
day one. Unfortunately I don't any hardware to test wl1251 right now, so
we have to rely on careful review :/
>> @@ -467,7 +467,7 @@ static int wl1251_boot_upload_nvs(struct wl1251 *wl)
>> val = (nvs_ptr[0] | (nvs_ptr[1] << 8)
>> | (nvs_ptr[2] << 16) | (nvs_ptr[3] << 24));
>>
>> - val = cpu_to_le32(val);
>> + val = (u32 __force) cpu_to_le32(val);
>
> This will work, but such casts always make me a bit suspicious. I think
> this is fine for now
This line was very suspicious already from beginning, I can't remember
why it was added and I don't see why it's needed here.
> but later I think we should make sure that all the
> _write() functions explicitly receive __le32 as val, or receives the
> cpu's u32 and converts it before actually writing the value, for
> clarity reasons. Kalle, what do you think?
I agree that we should change write() to handle endianess properly. I'm
in favor of having the conversion in write(), but I didn't think this
that much.
>> @@ -191,11 +191,13 @@ static int wl1251_tx_send_packet(struct wl1251 *wl, struct sk_buff *skb,
>> if (control->control.hw_key &&
>> control->control.hw_key->alg == ALG_TKIP) {
>> int hdrlen;
>> - u16 fc;
>> + __le16 fc;
>> + u16 length;
>> u8 *pos;
>>
>> - fc = *(u16 *)(skb->data + sizeof(*tx_hdr));
>> - tx_hdr->length += WL1251_TKIP_IV_SPACE;
>> + fc = *(__le16 *)(skb->data + sizeof(*tx_hdr));
>
> Is this going to work? sizeof(*tx_hdr), and the operation, will be in
> the cpu's endianess, right?
I think this is correct. We first calculate a pointer to a __le16 value
and then dereference that value to a __le16 variable.
> Wouldn't the following be the right thing to
> do then?
>
> fc = cpu_to_le16(le16_to_cpu(skb->data) + sizeof(*tx_hdr));
>
> Maybe some casts are needed too, I didn't check that, but regarding the
> endianess, I think this is how it should go. It's the same thing as the
> length parameter:
>
>> + length = le16_to_cpu(tx_hdr->length) + WL1251_TKIP_IV_SPACE;
>> + tx_hdr->length = cpu_to_le16(length);
>
> ...which is treated correctly here.
This is different. Here we are adding something to a __le16 value, not
calculating with pointers.
Kalle
^ permalink raw reply
* Re: [PATCH] wl1251: fix sparse-generated warnings
From: Luciano Coelho @ 2010-07-22 7:57 UTC (permalink / raw)
To: ext Kalle Valo; +Cc: ext John W. Linville, linux-wireless@vger.kernel.org
In-Reply-To: <4C47F706.4060102@iki.fi>
On Thu, 2010-07-22 at 09:45 +0200, ext Kalle Valo wrote:
> On 07/22/2010 08:34 AM, Luciano Coelho wrote:
> >> @@ -191,11 +191,13 @@ static int wl1251_tx_send_packet(struct wl1251 *wl, struct sk_buff *skb,
> >> if (control->control.hw_key &&
> >> control->control.hw_key->alg == ALG_TKIP) {
> >> int hdrlen;
> >> - u16 fc;
> >> + __le16 fc;
> >> + u16 length;
> >> u8 *pos;
> >>
> >> - fc = *(u16 *)(skb->data + sizeof(*tx_hdr));
> >> - tx_hdr->length += WL1251_TKIP_IV_SPACE;
> >> + fc = *(__le16 *)(skb->data + sizeof(*tx_hdr));
> >
> > Is this going to work? sizeof(*tx_hdr), and the operation, will be in
> > the cpu's endianess, right?
>
> I think this is correct. We first calculate a pointer to a __le16 value
> and then dereference that value to a __le16 variable.
Yes, you're right, I got totally confused for some reason, as I already
pointed out on my other email. I completely missed the fact that we
were dealing with pointers here. It seems like my brain works as that
of a Java programmer before my morning dose of caffeine :P
--
Cheers,
Luca.
^ permalink raw reply
* Re: [PATCH] wl1251: fix sparse-generated warnings
From: Kalle Valo @ 2010-07-22 8:45 UTC (permalink / raw)
To: John W. Linville; +Cc: linux-wireless, Luciano Coelho
In-Reply-To: <1279729917-4451-1-git-send-email-linville@tuxdriver.com>
Hi John,
On 07/21/2010 06:31 PM, John W. Linville wrote:
> @@ -225,7 +225,7 @@ static void wl1251_boot_set_ecpu_ctrl(struct wl1251 *wl, u32 flag)
> int wl1251_boot_run_firmware(struct wl1251 *wl)
> {
> int loop, ret;
> - u32 chip_id, interrupt;
> + u32 chip_id, acx_intr;
I don't get why you did this change. I don't object renaming the
variable, but I can't figure out the reasoning behind it.
Kalle
^ permalink raw reply
* [patch -next] ath9k: snprintf() returns largish values
From: Dan Carpenter @ 2010-07-22 8:50 UTC (permalink / raw)
To: Luis R. Rodriguez
Cc: Jouni Malinen, Sujith Manoharan, Vasanthakumar Thiagarajan,
Senthil Balasubramanian, John W. Linville, Vivek Natarajan,
Felix Fietkau, linux-wireless, ath9k-devel, kernel-janitors
The snprintf() function returns the number of characters that would have
been written (not counting the NUL character on the end). It could
potentially be larger than the size of the buffer.
Signed-off-by: Dan Carpenter <error27@gmail.com>
diff --git a/drivers/net/wireless/ath/ath9k/htc_drv_main.c b/drivers/net/wireless/ath/ath9k/htc_drv_main.c
index 3243877..cf9bcc6 100644
--- a/drivers/net/wireless/ath/ath9k/htc_drv_main.c
+++ b/drivers/net/wireless/ath/ath9k/htc_drv_main.c
@@ -524,6 +524,9 @@ static ssize_t read_file_tgt_stats(struct file *file, char __user *user_buf,
len += snprintf(buf + len, sizeof(buf) - len,
"%19s : %10u\n", "TX Rate", priv->debug.txrate);
+ if (len > sizeof(buf))
+ len = sizeof(buf);
+
return simple_read_from_buffer(user_buf, count, ppos, buf, len);
}
@@ -569,6 +572,9 @@ static ssize_t read_file_xmit(struct file *file, char __user *user_buf,
"%20s : %10u\n", "VO queued",
priv->debug.tx_stats.queue_stats[WME_AC_VO]);
+ if (len > sizeof(buf))
+ len = sizeof(buf);
+
return simple_read_from_buffer(user_buf, count, ppos, buf, len);
}
@@ -595,6 +601,9 @@ static ssize_t read_file_recv(struct file *file, char __user *user_buf,
"%20s : %10u\n", "SKBs Dropped",
priv->debug.rx_stats.skb_dropped);
+ if (len > sizeof(buf))
+ len = sizeof(buf);
+
return simple_read_from_buffer(user_buf, count, ppos, buf, len);
}
^ permalink raw reply related
* [patch -next] ath5k: snprintf() returns largish values
From: Dan Carpenter @ 2010-07-22 8:52 UTC (permalink / raw)
To: Luis R. Rodriguez
Cc: Nick Kossifidis, Jiri Slaby, Bob Copeland, John W. Linville,
Bruno Randolf, linux-wireless, ath5k-devel, kernel-janitors
snprintf() returns the number of characters that would have been written
(not counting the NUL character). So we can't use it as the limiter to
simple_read_from_buffer() without capping it first at sizeof(buf).
Signed-off-by: Dan Carpenter <error27@gmail.com>
diff --git a/drivers/net/wireless/ath/ath5k/debug.c b/drivers/net/wireless/ath/ath5k/debug.c
index ebb9c23..4cccc29 100644
--- a/drivers/net/wireless/ath/ath5k/debug.c
+++ b/drivers/net/wireless/ath/ath5k/debug.c
@@ -239,6 +239,9 @@ static ssize_t read_file_beacon(struct file *file, char __user *user_buf,
"TSF\t\t0x%016llx\tTU: %08x\n",
(unsigned long long)tsf, TSF_TO_TU(tsf));
+ if (len > sizeof(buf))
+ len = sizeof(buf);
+
return simple_read_from_buffer(user_buf, count, ppos, buf, len);
}
@@ -334,6 +337,9 @@ static ssize_t read_file_debug(struct file *file, char __user *user_buf,
sc->debug.level == dbg_info[i].level ? '+' : ' ',
dbg_info[i].level, dbg_info[i].desc);
+ if (len > sizeof(buf))
+ len = sizeof(buf);
+
return simple_read_from_buffer(user_buf, count, ppos, buf, len);
}
@@ -433,6 +439,9 @@ static ssize_t read_file_antenna(struct file *file, char __user *user_buf,
len += snprintf(buf+len, sizeof(buf)-len,
"AR5K_PHY_ANT_SWITCH_TABLE_1\t0x%08x\n", v);
+ if (len > sizeof(buf))
+ len = sizeof(buf);
+
return simple_read_from_buffer(user_buf, count, ppos, buf, len);
}
@@ -542,6 +551,9 @@ static ssize_t read_file_frameerrors(struct file *file, char __user *user_buf,
len += snprintf(buf+len, sizeof(buf)-len, "[TX all\t%d]\n",
st->tx_all_count);
+ if (len > sizeof(buf))
+ len = sizeof(buf);
+
return simple_read_from_buffer(user_buf, count, ppos, buf, len);
}
@@ -681,6 +693,9 @@ static ssize_t read_file_ani(struct file *file, char __user *user_buf,
ATH5K_ANI_CCK_TRIG_HIGH - (ATH5K_PHYERR_CNT_MAX -
ath5k_hw_reg_read(sc->ah, AR5K_PHYERR_CNT2)));
+ if (len > sizeof(buf))
+ len = sizeof(buf);
+
return simple_read_from_buffer(user_buf, count, ppos, buf, len);
}
@@ -766,6 +781,9 @@ static ssize_t read_file_queue(struct file *file, char __user *user_buf,
len += snprintf(buf+len, sizeof(buf)-len, " len: %d\n", n);
}
+ if (len > sizeof(buf))
+ len = sizeof(buf);
+
return simple_read_from_buffer(user_buf, count, ppos, buf, len);
}
^ permalink raw reply related
* Re: [PATCH] wl1251: fix sparse-generated warnings
From: Luciano Coelho @ 2010-07-22 8:52 UTC (permalink / raw)
To: ext Kalle Valo; +Cc: John W. Linville, linux-wireless@vger.kernel.org
In-Reply-To: <4C48053A.6080504@adurom.com>
On Thu, 2010-07-22 at 10:45 +0200, ext Kalle Valo wrote:
> Hi John,
>
> On 07/21/2010 06:31 PM, John W. Linville wrote:
> > @@ -225,7 +225,7 @@ static void wl1251_boot_set_ecpu_ctrl(struct wl1251 *wl, u32 flag)
> > int wl1251_boot_run_firmware(struct wl1251 *wl)
> > {
> > int loop, ret;
> > - u32 chip_id, interrupt;
> > + u32 chip_id, acx_intr;
>
> I don't get why you did this change. I don't object renaming the
> variable, but I can't figure out the reasoning behind it.
Sparse complains that interrupt is shadowing the definition in hw_irq.h:
> drivers/net/wireless/wl12xx/wl1251_boot.c:228:22: warning: symbol
> 'interrupt' shadows an earlier one
> /home/linville/git/wireless-next-2.6/arch/x86/include/asm/hw_irq.h:132:13: originally declared here
So, it's just a cosmetic change, I guess.
--
Cheers,
Luca.
^ permalink raw reply
* Re: [patch -next] ath5k: snprintf() returns largish values
From: Jiri Slaby @ 2010-07-22 8:56 UTC (permalink / raw)
To: Dan Carpenter
Cc: Luis R. Rodriguez, Nick Kossifidis, Bob Copeland,
John W. Linville, Bruno Randolf, linux-wireless, ath5k-devel,
kernel-janitors
In-Reply-To: <20100722085202.GV17585@bicker>
On 07/22/2010 10:52 AM, Dan Carpenter wrote:
> snprintf() returns the number of characters that would have been written
> (not counting the NUL character). So we can't use it as the limiter to
> simple_read_from_buffer() without capping it first at sizeof(buf).
Doesn't scnprintf make more sense here?
thanks,
--
js
^ permalink raw reply
* Re: [PATCH/RFC 3/3] ath5k: trace resets
From: Bruno Randolf @ 2010-07-22 9:21 UTC (permalink / raw)
To: Johannes Berg; +Cc: Bob Copeland, linux-wireless, ath5k-devel, Ben Gamari
In-Reply-To: <1279698801.3707.1.camel@jlt3.sipsolutions.net>
On Wed July 21 2010 16:53:21 Johannes Berg wrote:
> On Wed, 2010-07-21 at 14:17 +0900, Bruno Randolf wrote:
> > but that's for all tracepoints all over the kernel...
>
> Well you definitely don't want to enable like function graph tracing,
> that's expected to be more expensive unless you also have the callsite
> patching version of it. But _just_ enabling tracing + the ath5k tracer
> definitely will not have this effect.
true. i just checked it again and enabling tracing does not affect the
performance as i have wrongly stated before. i must have had some other
debugging stuff enabled as well. sorry...
bruno
^ permalink raw reply
* Re: [PATCH] wl1251: fix sparse-generated warnings
From: Kalle Valo @ 2010-07-22 9:21 UTC (permalink / raw)
To: Luciano Coelho; +Cc: John W. Linville, linux-wireless@vger.kernel.org
In-Reply-To: <1279788739.2322.39.camel@powerslave>
On 07/22/2010 10:52 AM, Luciano Coelho wrote:
> On Thu, 2010-07-22 at 10:45 +0200, ext Kalle Valo wrote:
>> Hi John,
>>
>> On 07/21/2010 06:31 PM, John W. Linville wrote:
>>> @@ -225,7 +225,7 @@ static void wl1251_boot_set_ecpu_ctrl(struct wl1251 *wl, u32 flag)
>>> int wl1251_boot_run_firmware(struct wl1251 *wl)
>>> {
>>> int loop, ret;
>>> - u32 chip_id, interrupt;
>>> + u32 chip_id, acx_intr;
>>
>> I don't get why you did this change. I don't object renaming the
>> variable, but I can't figure out the reasoning behind it.
>
> Sparse complains that interrupt is shadowing the definition in hw_irq.h:
>
>> drivers/net/wireless/wl12xx/wl1251_boot.c:228:22: warning: symbol
>> 'interrupt' shadows an earlier one
>> /home/linville/git/wireless-next-2.6/arch/x86/include/asm/hw_irq.h:132:13: originally declared here
>
> So, it's just a cosmetic change, I guess.
Ah, I was blind as usual. It makes all sense now, thanks.
Kalle
^ permalink raw reply
* [PATCH 1/2] ath9k: Fix inconsistency between txq->stopped and the actual queue state
From: Vasanthakumar Thiagarajan @ 2010-07-22 9:24 UTC (permalink / raw)
To: linville; +Cc: linux-wireless
Sometimes txq state(txq->stopped) can be marked as started but the actual
queue may not be started (in ATH_WIPHY_SCAN state, for example). Fix this.
Signed-off-by: Vasanthakumar Thiagarajan <vasanth@atheros.com>
---
drivers/net/wireless/ath/ath9k/ath9k.h | 2 +-
drivers/net/wireless/ath/ath9k/virtual.c | 6 +++++-
drivers/net/wireless/ath/ath9k/xmit.c | 4 ++--
3 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/drivers/net/wireless/ath/ath9k/ath9k.h b/drivers/net/wireless/ath/ath9k/ath9k.h
index 6e486a5..998ae2c 100644
--- a/drivers/net/wireless/ath/ath9k/ath9k.h
+++ b/drivers/net/wireless/ath/ath9k/ath9k.h
@@ -687,7 +687,7 @@ bool ath9k_all_wiphys_idle(struct ath_softc *sc);
void ath9k_set_wiphy_idle(struct ath_wiphy *aphy, bool idle);
void ath_mac80211_stop_queue(struct ath_softc *sc, u16 skb_queue);
-void ath_mac80211_start_queue(struct ath_softc *sc, u16 skb_queue);
+bool ath_mac80211_start_queue(struct ath_softc *sc, u16 skb_queue);
void ath_start_rfkill_poll(struct ath_softc *sc);
extern void ath9k_rfkill_poll_state(struct ieee80211_hw *hw);
diff --git a/drivers/net/wireless/ath/ath9k/virtual.c b/drivers/net/wireless/ath/ath9k/virtual.c
index 89423ca..fd20241 100644
--- a/drivers/net/wireless/ath/ath9k/virtual.c
+++ b/drivers/net/wireless/ath/ath9k/virtual.c
@@ -695,16 +695,18 @@ void ath9k_set_wiphy_idle(struct ath_wiphy *aphy, bool idle)
idle ? "idle" : "not-idle");
}
/* Only bother starting a queue on an active virtual wiphy */
-void ath_mac80211_start_queue(struct ath_softc *sc, u16 skb_queue)
+bool ath_mac80211_start_queue(struct ath_softc *sc, u16 skb_queue)
{
struct ieee80211_hw *hw = sc->pri_wiphy->hw;
unsigned int i;
+ bool txq_started = false;
spin_lock_bh(&sc->wiphy_lock);
/* Start the primary wiphy */
if (sc->pri_wiphy->state == ATH_WIPHY_ACTIVE) {
ieee80211_wake_queue(hw, skb_queue);
+ txq_started = true;
goto unlock;
}
@@ -718,11 +720,13 @@ void ath_mac80211_start_queue(struct ath_softc *sc, u16 skb_queue)
hw = aphy->hw;
ieee80211_wake_queue(hw, skb_queue);
+ txq_started = true;
break;
}
unlock:
spin_unlock_bh(&sc->wiphy_lock);
+ return txq_started;
}
/* Go ahead and propagate information to all virtual wiphys, it won't hurt */
diff --git a/drivers/net/wireless/ath/ath9k/xmit.c b/drivers/net/wireless/ath/ath9k/xmit.c
index 0644f1e..21aa5bd 100644
--- a/drivers/net/wireless/ath/ath9k/xmit.c
+++ b/drivers/net/wireless/ath/ath9k/xmit.c
@@ -2077,8 +2077,8 @@ static void ath_wake_mac80211_queue(struct ath_softc *sc, struct ath_txq *txq)
spin_lock_bh(&txq->axq_lock);
if (txq->stopped && sc->tx.pending_frames[qnum] < ATH_MAX_QDEPTH) {
- ath_mac80211_start_queue(sc, qnum);
- txq->stopped = 0;
+ if (ath_mac80211_start_queue(sc, qnum))
+ txq->stopped = 0;
}
spin_unlock_bh(&txq->axq_lock);
}
--
1.7.0.4
^ permalink raw reply related
* [PATCH 2/2] ath9k: Fix stop in tx date traffic after scan
From: Vasanthakumar Thiagarajan @ 2010-07-22 9:25 UTC (permalink / raw)
To: linville; +Cc: linux-wireless, johannes
When moving off channel during background scanning, driver
can not clear IEEE80211_QUEUE_STOP_REASON_DRIVER which was
set earliar due to a shortage in tx buffer, this will lead
to a situation where no data frames will be passed down to
driver when the hw is put back into operating channel. This
patch make sure no txq is stopped after moving to operating
channel after scan. This issue is seen only when NM is used
and exposed by the following commit
Author: Felix Fietkau <nbd@openwrt.org>
Date: Tue Jun 1 21:33:13 2010 +0200
ath9k: fix queue stop/start based on the number of pending frames
Signed-off-by: Vasanthakumar Thiagarajan <vasanth@atheros.com>
---
This is not a clean fix for this issue, but best available
from driver. For a decent fix from driver, the following
changes in mac80211 may be required
1. Make ieee80211_wake_queue() callable when moving to off channel
also just to clear IEEE80211_QUEUE_STOP_REASON_DRIVER or
2. For drivers which does not support hw scan, add a way to
tell them if the hw is being configured into operating channel
(driver can detect this, but this would be a hack in driver)
so that the driver can wake up the stopped queues after the
hw is configured back to operating channel.
Johannes, if the above mac80211 changes make any sense, I'll send
a new set of patches to fix this problem.
---
drivers/net/wireless/ath/ath9k/ath9k.h | 1 +
drivers/net/wireless/ath/ath9k/main.c | 6 ++++++
drivers/net/wireless/ath/ath9k/xmit.c | 2 +-
3 files changed, 8 insertions(+), 1 deletions(-)
diff --git a/drivers/net/wireless/ath/ath9k/ath9k.h b/drivers/net/wireless/ath/ath9k/ath9k.h
index 998ae2c..d8d8804 100644
--- a/drivers/net/wireless/ath/ath9k/ath9k.h
+++ b/drivers/net/wireless/ath/ath9k/ath9k.h
@@ -692,4 +692,5 @@ bool ath_mac80211_start_queue(struct ath_softc *sc, u16 skb_queue);
void ath_start_rfkill_poll(struct ath_softc *sc);
extern void ath9k_rfkill_poll_state(struct ieee80211_hw *hw);
+void ath_wake_mac80211_queue(struct ath_softc *sc, struct ath_txq *txq);
#endif /* ATH9K_H */
diff --git a/drivers/net/wireless/ath/ath9k/main.c b/drivers/net/wireless/ath/ath9k/main.c
index 6cf0410..7a0d633 100644
--- a/drivers/net/wireless/ath/ath9k/main.c
+++ b/drivers/net/wireless/ath/ath9k/main.c
@@ -1491,6 +1491,7 @@ static int ath9k_config(struct ieee80211_hw *hw, u32 changed)
struct ieee80211_conf *conf = &hw->conf;
struct ath_hw *ah = sc->sc_ah;
bool disable_radio;
+ int i;
mutex_lock(&sc->mutex);
@@ -1605,6 +1606,11 @@ static int ath9k_config(struct ieee80211_hw *hw, u32 changed)
mutex_unlock(&sc->mutex);
return -EINVAL;
}
+
+ for (i = 0; i < ATH9K_NUM_TX_QUEUES; i++) {
+ if (ATH_TXQ_SETUP(sc, i))
+ ath_wake_mac80211_queue(sc, &sc->tx.txq[i]);
+ }
}
skip_chan_change:
diff --git a/drivers/net/wireless/ath/ath9k/xmit.c b/drivers/net/wireless/ath/ath9k/xmit.c
index 21aa5bd..5bfec95 100644
--- a/drivers/net/wireless/ath/ath9k/xmit.c
+++ b/drivers/net/wireless/ath/ath9k/xmit.c
@@ -2067,7 +2067,7 @@ static void ath_tx_rc_status(struct ath_buf *bf, struct ath_tx_status *ts,
tx_info->status.rates[tx_rateindex].count = ts->ts_longretry + 1;
}
-static void ath_wake_mac80211_queue(struct ath_softc *sc, struct ath_txq *txq)
+void ath_wake_mac80211_queue(struct ath_softc *sc, struct ath_txq *txq)
{
int qnum;
--
1.7.0.4
^ permalink raw reply related
* Re: [PATCH 2/2] ath9k: Fix stop in tx date traffic after scan
From: Johannes Berg @ 2010-07-22 9:56 UTC (permalink / raw)
To: Vasanthakumar Thiagarajan; +Cc: linville, linux-wireless
In-Reply-To: <1279790703-11521-1-git-send-email-vasanth@atheros.com>
On Thu, 2010-07-22 at 02:25 -0700, Vasanthakumar Thiagarajan wrote:
> When moving off channel during background scanning, driver
> can not clear IEEE80211_QUEUE_STOP_REASON_DRIVER which was
> set earliar due to a shortage in tx buffer, this will lead
> to a situation where no data frames will be passed down to
> driver when the hw is put back into operating channel. This
> patch make sure no txq is stopped after moving to operating
> channel after scan. This issue is seen only when NM is used
> and exposed by the following commit
>
> Author: Felix Fietkau <nbd@openwrt.org>
> Date: Tue Jun 1 21:33:13 2010 +0200
> ath9k: fix queue stop/start based on the number of pending frames
>
> Signed-off-by: Vasanthakumar Thiagarajan <vasanth@atheros.com>
> ---
>
> This is not a clean fix for this issue, but best available
> from driver. For a decent fix from driver, the following
> changes in mac80211 may be required
>
> 1. Make ieee80211_wake_queue() callable when moving to off channel
> also just to clear IEEE80211_QUEUE_STOP_REASON_DRIVER or
>
> 2. For drivers which does not support hw scan, add a way to
> tell them if the hw is being configured into operating channel
> (driver can detect this, but this would be a hack in driver)
> so that the driver can wake up the stopped queues after the
> hw is configured back to operating channel.
>
> Johannes, if the above mac80211 changes make any sense, I'll send
> a new set of patches to fix this problem.
Really, that all doesn't make a whole lot of sense to me.
Just go and implement flush() and all these issues will go away and you
will stop thinking that you need to touch queues from channel switching.
They have nothing to do with each other.
johannes
^ permalink raw reply
* Re: [PATCH 2/2] ath9k: Fix stop in tx date traffic after scan
From: Vasanthakumar Thiagarajan @ 2010-07-22 10:11 UTC (permalink / raw)
To: Johannes Berg
Cc: Vasanth Thiagarajan, linville@tuxdriver.com,
linux-wireless@vger.kernel.org
In-Reply-To: <1279792601.12439.1.camel@jlt3.sipsolutions.net>
> Just go and implement flush() and all these issues will go away and you
> will stop thinking that you need to touch queues from channel switching.
> They have nothing to do with each other.
I thought about it also, but i'll hit the same issue
when ieee80211_scan_state_leave_oper_channel() flushes
the hw tx queues where driver is not supposed to wake
up the queues as drv_flush() is called only after stopping
all queues.
Vasanth
^ permalink raw reply
* Re: [PATCH 2/2] ath9k: Fix stop in tx date traffic after scan
From: Johannes Berg @ 2010-07-22 10:14 UTC (permalink / raw)
To: Vasanthakumar Thiagarajan
Cc: Vasanth Thiagarajan, linville@tuxdriver.com,
linux-wireless@vger.kernel.org
In-Reply-To: <20100722101142.GA4355@vasanth-laptop>
On Thu, 2010-07-22 at 15:41 +0530, Vasanthakumar Thiagarajan wrote:
> > Just go and implement flush() and all these issues will go away and you
> > will stop thinking that you need to touch queues from channel switching.
> > They have nothing to do with each other.
>
>
> I thought about it also, but i'll hit the same issue
> when ieee80211_scan_state_leave_oper_channel() flushes
> the hw tx queues where driver is not supposed to wake
> up the queues as drv_flush() is called only after stopping
> all queues.
I don't get it. The driver can start/stop queues at _any_ time it wants
to. Regardless of what mac80211 is doing, all that goes via
IEEE80211_QUEUE_STOP_REASON_DRIVER which is never touched by mac80211
itself.
johannes
^ permalink raw reply
* Re: [PATCH 2/2] ath9k: Fix stop in tx date traffic after scan
From: Vasanthakumar Thiagarajan @ 2010-07-22 10:20 UTC (permalink / raw)
To: Johannes Berg
Cc: Vasanth Thiagarajan, linville@tuxdriver.com,
linux-wireless@vger.kernel.org
In-Reply-To: <1279793694.12439.4.camel@jlt3.sipsolutions.net>
On Thu, Jul 22, 2010 at 03:44:54PM +0530, Johannes Berg wrote:
> On Thu, 2010-07-22 at 15:41 +0530, Vasanthakumar Thiagarajan wrote:
> > > Just go and implement flush() and all these issues will go away and you
> > > will stop thinking that you need to touch queues from channel switching.
> > > They have nothing to do with each other.
> >
> >
> > I thought about it also, but i'll hit the same issue
> > when ieee80211_scan_state_leave_oper_channel() flushes
> > the hw tx queues where driver is not supposed to wake
> > up the queues as drv_flush() is called only after stopping
> > all queues.
>
> I don't get it. The driver can start/stop queues at _any_ time it wants
> to. Regardless of what mac80211 is doing, all that goes via
> IEEE80211_QUEUE_STOP_REASON_DRIVER which is never touched by mac80211
> itself.
My understanding is, if driver wakes up the queues when operating on
a off-channel, it would get data frames from upper layer for
transmission but it should not send out these frames as the hw is on
non-operating channel.
Vasanth
^ permalink raw reply
* Re: [PATCH 2/2] ath9k: Fix stop in tx date traffic after scan
From: Johannes Berg @ 2010-07-22 10:32 UTC (permalink / raw)
To: Vasanthakumar Thiagarajan
Cc: Vasanth Thiagarajan, linville@tuxdriver.com,
linux-wireless@vger.kernel.org
In-Reply-To: <20100722102049.GB4355@vasanth-laptop>
On Thu, 2010-07-22 at 15:50 +0530, Vasanthakumar Thiagarajan wrote:
> On Thu, Jul 22, 2010 at 03:44:54PM +0530, Johannes Berg wrote:
> > On Thu, 2010-07-22 at 15:41 +0530, Vasanthakumar Thiagarajan wrote:
> > > > Just go and implement flush() and all these issues will go away and you
> > > > will stop thinking that you need to touch queues from channel switching.
> > > > They have nothing to do with each other.
> > >
> > >
> > > I thought about it also, but i'll hit the same issue
> > > when ieee80211_scan_state_leave_oper_channel() flushes
> > > the hw tx queues where driver is not supposed to wake
> > > up the queues as drv_flush() is called only after stopping
> > > all queues.
> >
> > I don't get it. The driver can start/stop queues at _any_ time it wants
> > to. Regardless of what mac80211 is doing, all that goes via
> > IEEE80211_QUEUE_STOP_REASON_DRIVER which is never touched by mac80211
> > itself.
>
> My understanding is, if driver wakes up the queues when operating on
> a off-channel, it would get data frames from upper layer for
> transmission but it should not send out these frames as the hw is on
> non-operating channel.
Ok, that seems to be true, but only because we don't properly manage the
interface queue stop in mac80211. Should be an easy fix there.
johannes
^ permalink raw reply
* Re: [patch -next] ath5k: snprintf() returns largish values
From: Dan Carpenter @ 2010-07-22 10:44 UTC (permalink / raw)
To: Jiri Slaby
Cc: Luis R. Rodriguez, Nick Kossifidis, Bob Copeland,
John W. Linville, Bruno Randolf, linux-wireless, ath5k-devel,
kernel-janitors
In-Reply-To: <4C4807AD.5090302@gmail.com>
On Thu, Jul 22, 2010 at 10:56:13AM +0200, Jiri Slaby wrote:
> On 07/22/2010 10:52 AM, Dan Carpenter wrote:
> > snprintf() returns the number of characters that would have been written
> > (not counting the NUL character). So we can't use it as the limiter to
> > simple_read_from_buffer() without capping it first at sizeof(buf).
>
> Doesn't scnprintf make more sense here?
>
Not really... It's nice to pass a negative number as the buffer size to
snprintf() instead of having to make that a special case.
regards,
dan carpenter
> thanks,
> --
> js
^ permalink raw reply
* potential null deref in minstrel_ht_update_caps()?
From: Dan Carpenter @ 2010-07-22 11:09 UTC (permalink / raw)
To: nbd; +Cc: linux-wireless
This is a smatch thing.
net/mac80211/rc80211_minstrel_ht.c +639 minstrel_ht_update_caps(15)
warn: variable dereferenced before check 'sta'
631 struct ieee80211_mcs_info *mcs = &sta->ht_cap.mcs;
632 struct ieee80211_local *local = hw_to_local(mp->hw);
633 u16 sta_cap = sta->ht_cap.cap;
^^^^^^^^^^^^^^^
Dereferenced here.
634 int ack_dur;
635 int stbc;
636 int i;
637
638 /* fall back to the old minstrel for legacy stations */
639 if (sta && !sta->ht_cap.ht_supported) {
^^^
Checked here.
640 msp->is_ht = false;
641 memset(&msp->legacy, 0, sizeof(msp->legacy));
It seems like a bug, but I'm not sure how to deal with it.
regards,
dan carpenter
^ permalink raw reply
* [patch net-next] mac80211: freeing the wrong variable
From: Dan Carpenter @ 2010-07-22 11:14 UTC (permalink / raw)
To: Felix Fietkau
Cc: Johannes Berg, David S. Miller, John W. Linville, Ming Lei,
linux-wireless, kernel-janitors
The intent was to free "msp->ratelist" here. "msp->sample_table" is
always NULL at this point.
Signed-off-by: Dan Carpenter <error27@gmail.com>
diff --git a/net/mac80211/rc80211_minstrel_ht.c b/net/mac80211/rc80211_minstrel_ht.c
index b5ace24..0ec0584 100644
--- a/net/mac80211/rc80211_minstrel_ht.c
+++ b/net/mac80211/rc80211_minstrel_ht.c
@@ -748,7 +748,7 @@ minstrel_ht_alloc_sta(void *priv, struct ieee80211_sta *sta, gfp_t gfp)
return msp;
error1:
- kfree(msp->sample_table);
+ kfree(msp->ratelist);
error:
kfree(msp);
return NULL;
^ permalink raw reply related
* Re: [PATCH v2 10/20] omap: zoom: add fixed regulator device for wlan
From: Roger Quadros @ 2010-07-22 11:16 UTC (permalink / raw)
To: ext Mark Brown
Cc: Ohad Ben-Cohen, linux-wireless@vger.kernel.org,
linux-mmc@vger.kernel.org, linux-omap@vger.kernel.org, Kalle Valo,
Pandita Vikram, linux@arm.linux.org.uk, Nicolas Pitre,
Tony Lindgren, San Mehat, Chikkature Rajashekar Madhusudhan,
Coelho Luciano (Nokia-MS/Helsinki), akpm@linux-foundation.org,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <20100721175930.GD10930@sirena.org.uk>
On 07/21/2010 08:59 PM, ext Mark Brown wrote:
> On Wed, Jul 21, 2010 at 08:33:44PM +0300, Ohad Ben-Cohen wrote:
>
>> +static struct regulator_consumer_supply zoom_vmmc3_supply = {
>> + .supply = "vmmc",
>> +};
>
> This looks like a misconfiguration on the consumer - I'd strongly expect
> the consumer to have a dev_name specified also with the name being in
> terms of the consumer device.
you need to add something like
.dev_name = "mmci-omap-hs.2"
regards,
-roger
^ permalink raw reply
* [patch -next] wireless: remove unneeded variable from regulatory_hint_11d()
From: Dan Carpenter @ 2010-07-22 11:26 UTC (permalink / raw)
To: Johannes Berg
Cc: John W. Linville, David S. Miller, Luis R. Rodriguez,
Benoit PAPILLAULT, linux-wireless, kernel-janitors
The "rd" variable isn't needed any more since 4f366c5dabcb
"wireless: only use alpha2 regulatory information from country IE"
Signed-off-by: Dan Carpenter <error27@gmail.com>
diff --git a/net/wireless/reg.c b/net/wireless/reg.c
index 48baf28..ec4e76f 100644
--- a/net/wireless/reg.c
+++ b/net/wireless/reg.c
@@ -1492,7 +1492,6 @@ void regulatory_hint_11d(struct wiphy *wiphy,
u8 *country_ie,
u8 country_ie_len)
{
- struct ieee80211_regdomain *rd = NULL;
char alpha2[2];
enum environment_cap env = ENVIRON_ANY;
struct regulatory_request *request;
@@ -1529,7 +1528,7 @@ void regulatory_hint_11d(struct wiphy *wiphy,
request = kzalloc(sizeof(struct regulatory_request), GFP_KERNEL);
if (!request)
- goto free_rd_out;
+ goto out;
request->wiphy_idx = get_wiphy_idx(wiphy);
request->alpha2[0] = alpha2[0];
@@ -1543,8 +1542,6 @@ void regulatory_hint_11d(struct wiphy *wiphy,
return;
-free_rd_out:
- kfree(rd);
out:
mutex_unlock(®_mutex);
}
^ permalink raw reply related
* Re: [PATCH v2 18/20] mmc: sdio: enable a default power off mode of the card
From: Roger Quadros @ 2010-07-22 11:35 UTC (permalink / raw)
To: ext Ohad Ben-Cohen
Cc: linux-wireless@vger.kernel.org, linux-mmc@vger.kernel.org,
linux-omap@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
linux@arm.linux.org.uk, Chikkature Rajashekar Madhusudhan,
Coelho Luciano (Nokia-MS/Helsinki), akpm@linux-foundation.org,
San Mehat, Tony Lindgren, Nicolas Pitre, Pandita Vikram,
Kalle Valo
In-Reply-To: <1279733634-21974-19-git-send-email-ohad@wizery.com>
On 07/21/2010 08:33 PM, ext Ohad Ben-Cohen wrote:
> Add support for an SDIO device to stay powered off even without
> the presence of an SDIO function driver. A host should explicitly
> ask for it by means of MMC_CAP_DONT_POWER_CARD, and the SDIO
> function driver should know it needs to call sdio_claim_power
> before accessing the device.
>
> Signed-off-by: Ohad Ben-Cohen<ohad@wizery.com>
Shouldn't this be the default behaviour? If there is no function driver for any
of the functions of the card, then sdio core shold power off the card.
I don't see a need for a special capability flag for this.
in fact MMC_CAP_DONT_POWER_CARD does not seem like an mmc host's capability
flag, it seems more like a request from the board to keep the card powered off.
regards,
-roger
^ 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