Linux wireless drivers development
 help / color / mirror / Atom feed
* Re: [PATCH] orinoco: Use shash instead of ahash for MIC calculations
From: Andy Lutomirski @ 2016-12-13 16:41 UTC (permalink / raw)
  To: Kalle Valo
  Cc: Andy Lutomirski, linux-kernel@vger.kernel.org, USB list,
	Linux Wireless List, Eric Biggers, linux-crypto, Herbert Xu,
	Stephan Mueller
In-Reply-To: <87mvg0kqno.fsf@purkki.adurom.net>

On Tue, Dec 13, 2016 at 3:35 AM, Kalle Valo <kvalo@codeaurora.org> wrote:
> Andy Lutomirski <luto@kernel.org> writes:
>
>> Eric Biggers pointed out that the orinoco driver pointed scatterlists
>> at the stack.
>>
>> Fix it by switching from ahash to shash.  The result should be
>> simpler, faster, and more correct.
>>
>> Cc: stable@vger.kernel.org # 4.9 only
>> Reported-by: Eric Biggers <ebiggers3@gmail.com>
>> Signed-off-by: Andy Lutomirski <luto@kernel.org>
>
> "more correct"? Does this fix a real user visible bug or what? And why
> just stable 4.9, does this maybe have something to do with
> CONFIG_VMAP_STACK?

Whoops, I had that text in some other patches but forgot to put it in
this one.  It'll blow up with CONFIG_VMAP_STACK=y if a debug option
like CONFIG_DEBUG_VIRTUAL=y is set.  It may work by accident if
debugging is off.

--Andy

^ permalink raw reply

* Re: [RFC V3 01/11] nl80211: add reporting of gscan capabilities
From: Johannes Berg @ 2016-12-13 16:15 UTC (permalink / raw)
  To: Arend van Spriel; +Cc: linux-wireless, Arend van Spriel
In-Reply-To: <1481543997-24624-2-git-send-email-arend.vanspriel@broadcom.com>


> +	case 14:
> +		if (!rdev->wiphy.gscan) {
> +			/* done */
> +			state->split_start = 0;
> +			break;
> +		}
> 

Nit, but I'm not really happy with this - this assumes that case 14 is
the last case, if anyone ever adds one we break this code but it would
still work if the device has gscan. Move the gscan stuff into a new
function and make that return immediately if gscan is NULL or so?

johannes

^ permalink raw reply

* Re: [RFC V3 04/11] nl80211: add driver api for gscan notifications
From: Johannes Berg @ 2016-12-13 16:20 UTC (permalink / raw)
  To: Arend van Spriel; +Cc: linux-wireless
In-Reply-To: <1481543997-24624-5-git-send-email-arend.vanspriel@broadcom.com>

On Mon, 2016-12-12 at 11:59 +0000, Arend van Spriel wrote:
> The driver can indicate gscan results are available or gscan
> operation has stopped.

This patch is renumbering the previous patches' nl80211 API, which is
best avoided, even if I do realize it doesn't matter now. :)

Even here it's not clear how things are reported though. Somehow I
thought that gscan was reporting only partial information through the
buckets, or is that not true?

johannes

^ permalink raw reply

* Re: [RFC V3 03/11] nl80211: add support for gscan
From: Johannes Berg @ 2016-12-13 16:19 UTC (permalink / raw)
  To: Arend van Spriel; +Cc: linux-wireless
In-Reply-To: <1481543997-24624-4-git-send-email-arend.vanspriel@broadcom.com>

On Mon, 2016-12-12 at 11:59 +0000, Arend van Spriel wrote:
> This patch adds support for GScan which is a scan offload feature
> used in Android.

Found a few places with spaces instead of tabs as indentation, and
spurious braces around single-statement things, but other than that it
looks fine from a patch/nl80211 POV.

Haven't really looked into the details of gscan itself now though,
sorry.

There's a bit of a weird hard-coded restriction to 16 channels too,
that's due to the bucket map?

johannes

^ permalink raw reply

* Re: [PATCH 3/3][RFC] nl80211/mac80211: Accept multiple RSSI thresholds for CQM
From: Johannes Berg @ 2016-12-13 16:11 UTC (permalink / raw)
  To: Andrew Zaborowski; +Cc: linux-wireless
In-Reply-To: <CAOq732K88CygXBmDfGmU21AcQHurcWx081Q3SM-b0ecm+848gA@mail.gmail.com>


> I wasn't clear: nl80211 sets the thresholds so that "high" is higher
> than last known value and "low" is lower than last known value, also
> the distance is at least 2 x hysteresis.  There's no purpose for
> reporting "middle" rssi events because we have to set a new range as
> soon as we receive a high or a low event.  I realize I need to
> document better.

But there can be a delay between reporting and reprogramming, and if
during that time a new event could be reported? I guess it doesn't
matter much if we assume that upon reprogramming the driver will always
report a new event if the current value falls outside the new range
(either high or low)... it just seemed a little bit more consistent to
unconditionally report a new event at the beginning, even if that new
event is "yup - falling into the middle of your range now".

johannes

^ permalink raw reply

* Re: [PATCH 2/4] cfg80211: Add new NL80211_CMD_SET_BTCOEX_PRIORITY to support BTCOEX
From: Johannes Berg @ 2016-12-13 16:09 UTC (permalink / raw)
  To: Tamizh chelvam; +Cc: c_traja, linux-wireless, ath10k
In-Reply-To: <5e5e8971c96293a81e7cb37bcdfbd593@codeaurora.org>


> > >  /**
> > > + * wiphy_btcoex_support_flags
> > > + *	This enum has the driver supported frame types for
> > > BTCOEX.
> > > + * @WIPHY_WLAN_BE_PREFERRED - Supports Best Effort frame for
> > > BTCOEX
> > > + * @WIPHY_WLAN_BK_PREFERRED - supports Background frame for
> > > BTCOEX
> > > + * @WIPHY_WLAN_VI_PREFERRED - supports Video frame for BTCOEX
> > > + * @WIPHY_WLAN_VO_PREFERRED - supports Voice frame for BTCOEX
> > > + * @WIPHY_WLAN_BEACON_PREFERRED - supports Beacon frame for
> > > BTCOEX
> > > + * @WIPHY_WLAN_MGMT_PREFERRED - supports Management frames for
> > > BTCOEX.
> > > + */
> > 
> > That's not making much sense to me?
> > 
> 
> is it fine to have as WIPHY_BTCOEX_BE_PREFERRED ?

It's not really clear to me what you intend to do this - if it's really
support flags then you really should name those better.

> > > +/**
> > > + * enum wiphy_btcoex_priority - BTCOEX priority level
> > > + *	This enum defines priority level for BTCOEX
> > > + * WIPHY_WLAN_PREFERRED_LOW - low priority frames over BT
> > > traffic
> > > + * WIPHY_WLAN_PREFERRED_HIGH - high priority frames over BT
> > > traffic
> > > + */
> > > +
> > > +enum wiphy_btcoex_priority {
> > > +	WIPHY_WLAN_PREFERRED_LOW = false,
> > > +	WIPHY_WLAN_PREFERRED_HIGH = true,
> > > +};
> > 
> > That false/true seems just strange.
> > 
> 
> I will just use as a enum without assigning false/true.

What do you even need this enum for though?

> > > +enum nl80211_btcoex_priority {
> > > +	__NL80211_WLAN_PREFERRED_INVALID,
> > > +	NL80211_WLAN_BE_PREFERRED,
> > > +	NL80211_WLAN_BK_PREFERRED,
> > > +	NL80211_WLAN_VI_PREFERRED,
> > > +	NL80211_WLAN_VO_PREFERRED,
> > > +	NL80211_WLAN_BEACON_PREFERRED,
> > > +	NL80211_WLAN_MGMT_PREFERRED,
> > > +	__NL80211_WLAN_PREFERRED_LAST,
> > > +	NL80211_WLAN_PREFERRED_MAX =
> > > +			__NL80211_WLAN_PREFERRED_LAST - 1,
> > > +};
> > 
> > Wouldn't a bitmap be easier?
> > 
> since this is to distinguish between different btcoex priorities and
> we 
> are not going to do any manipulations on these parameters.
> It is just used as flag attribute.

But why the (parsing) complexity, when a single bitmap would do?

johannes

^ permalink raw reply

* Re: [PATCH] RFC: Universal scan proposal
From: Johannes Berg @ 2016-12-13 16:06 UTC (permalink / raw)
  To: Dmitry Shmidt, Arend Van Spriel; +Cc: linux-wireless
In-Reply-To: <CAH7ZN-wGseBVzV3Vuq+6=kgaSL7e0UnndGXPdu4PQKZw8H47YQ@mail.gmail.com>


> Supporting requests (or more precisely requests and results)
> differentiated by user-space entity can be tricky. Right now we are
> not checking current caller pid, right? Maybe it is also good idea -
> or maybe we can just make result filtering per user-space caller?

Could be done.

You seem to be very worried about the partial results - I'm not too
worried about that I guess, the connection manager itself will always
be able to wait for the full scan to finish before making a decision,
but it may not even want to (see the separate discussion on per-channel 
"done" notifications etc.)

I'm much more worried about the "bucket reporting" since that doesn't
fit into the current full BSS reporting model at all. What's your
suggestion for this?

johannes

^ permalink raw reply

* Re: [PATCH] RFC: Universal scan proposal
From: Johannes Berg @ 2016-12-13 16:04 UTC (permalink / raw)
  To: Dmitry Shmidt; +Cc: linux-wireless
In-Reply-To: <CAH7ZN-wP+9AGrXFUS4RY65-RyfP-J46svBvLdytP2c=QPtiaug@mail.gmail.com>

> > Well eventually we also have to clear for location if we run out of
> > memory, that usually means dumping them out to the host, no?
> 
> Being out of memory and consuming more memory are different
> things, but I agree - maybe we don't need to worry about it.

Well, reaching the limit of what we're willing to spend on it is
equivalent I guess :)

> > I'm not entirely sure about this case - surely noticing "we can do
> > better now" is still better than waiting for being able to make the
> > perfect decision?
> 
> Maybe we can just keep flag saying that currently available results
> were not received by usual full scan.

Elsewhere we were planning per-channel results, and a cookie to filter
them - perhaps we could have a similar thing where you may even have to
request these scan results specifically with a certain cookie you got
from the scanning, or so. Or indicate the cookie there so you can tie
it back to the scan request somehow?

> So, let's summarize:
> Instead of creating new type of generic scan with special types,
> we want to go with additional expansion of scheduled scan options and
> parameters (in order not to "multiply entities"), including ability
> to send new scheduled scan request without stopping previous one.
> 
> Is it Ok?

Sounds fine to me.

johannes

^ permalink raw reply

* Re: [PATCH v2 2/2] cfg80211: Add support to sched scan to report better BSSs
From: Johannes Berg @ 2016-12-13 15:56 UTC (permalink / raw)
  To: Arend Van Spriel, Malinen, Jouni
  Cc: Vamsi, Krishna, linux-wireless@vger.kernel.org
In-Reply-To: <63343007-2245-1861-94fd-bdda0de2f7dc@broadcom.com>

Ok... this is getting complicated :)

Regarding reusing attributes, we have (for the BSS selection thing) the
attribute NL80211_BSS_SELECT_ATTR_RSSI_ADJUST, which is really quite
similar to your new NL80211_ATTR_SCHED_SCAN_RELATIVE_RSSI_5G_PREF since
while connected (which BSS_SELECT_ATTR_* assumes) the current BSS is
always part of the considered BSSes, I'd think.

However, I tend to think now that reusing the attribute is perhaps not
the right thing to do - but defining them with the same semantics would
still make sense.

Assuming that the value defined in NL80211_BSS_SELECT_ATTR_RSSI_ADJUST
applies also to the *current* BSS, it's actually quite pointless to
define there the band to adjust - if you want to adjust 2.4 GHz
positively you might as well adjust 5 GHz negatively, and vice versa,
and both ways are supported.

OTOH, the new NL80211_ATTR_SCHED_SCAN_RELATIVE_RSSI_5G_PREF doesn't
make this quite clear - is the current BSS to be adjusted before
comparing, if it's 5 GHz? If so, the semantics are equivalent. If not,
it doesn't actually make much sense ;-)
So assuming that it is in fact taken into account after the same
adjustment, the two attributes are equivalent, and then perhaps it
would make sense to use struct nl80211_bss_select_rssi_adjust for the
new attribute. If a driver doesn't support arbitrary bands, but just 5
GHz as in your example, it can just flip it around to 2.4 GHz by
switching the sign.

Perhaps we should even consider doing that in cfg80211 and adjusting
the internal API for both that way?

> I am not saying it should be avoided. Just looking at it conceptually
> the scheduled scan request holds so-called matchsets that specify the
> constraints to determine whether a BSS was found that is worth
> notifying the host/user-space about. As such I would expect the
> relative RSSI attribute(s) to be part of the matchset. That way you
> can specify it together with the currently connected SSID in a single
> matchset.

I think this makes a lot of sense.

We already have NL80211_SCHED_SCAN_MATCH_ATTR_RSSI, which asks to be
reporting only networks that have an *absolute* RSSI value above the
value of the attribute - a new attribute to make it relative to the
current network instead would make sense.

That would indeed be equivalent to NL80211_BSS_SELECT_ATTR_RSSI then.

Now, if we consider this, NL80211_ATTR_SCHED_SCAN_RELATIVE_RSSI
actually is equivalent to NL80211_BSS_SELECT_ATTR_RSSI (a flag
attribute indicating whether or not RSSI-based selection/matching is
done) and NL80211_ATTR_SCHED_SCAN_RELATIVE_RSSI_5G_PREF is equivalent
to NL80211_BSS_SELECT_ATTR_RSSI_ADJUST, both need to be given with the
flag and affect operation.

However, NL80211_BSS_SELECT_ATTR_BAND_PREF doesn't exist, and reusing
the BSS_SELECT namespace also doesn't make sense.


So, how about we move these into NL80211_SCHED_SCAN_MATCH_ATTR_* as
suggested by Arend, and define them with the same content as  the
corresponding NL80211_BSS_SELECT_ATTR_*?

If they're part of match attributes, we might even remove the feature
flag entirely - those were always defined to be optional, but it very
well be worthwhile for userspace to know if they're supported if it
wants to behave differently depending on whether they're supported or
not, I'll leave that up to you since presumably you know the userspace
implementation that you're planning to create.

johannes

^ permalink raw reply

* Re: [PATCH v3] cfg80211: NL80211_ATTR_SOCKET_OWNER support for CMD_CONNECT
From: Johannes Berg @ 2016-12-13 15:36 UTC (permalink / raw)
  To: Andrew Zaborowski, linux-wireless
In-Reply-To: <1481643200.20412.9.camel@sipsolutions.net>


> All the code you added to nl80211.c is racy.

Maybe it's not? But that'd be hard to reason about, having to look into
af_netlink.c and all, so it's easier to just make it not appear racy
here.

johannes

^ permalink raw reply

* Re: [PATCH v3] cfg80211: NL80211_ATTR_SOCKET_OWNER support for CMD_CONNECT
From: Johannes Berg @ 2016-12-13 15:33 UTC (permalink / raw)
  To: Andrew Zaborowski, linux-wireless
In-Reply-To: <20161212164500.691-1-andrew.zaborowski@intel.com>

[snip]

Please fix coding style, particularly indentation.

> +static void cfg80211_disconnect_wk(struct work_struct *work)
> +{
> +       struct cfg80211_registered_device *rdev;
> +       struct wireless_dev *wdev;
> +
> +       wdev = container_of(work, struct wireless_dev, disconnect_wk);
> +       rdev = wiphy_to_rdev(wdev->wiphy);


Those should also be possible as initializers on the same line, I
guess?

It might also be worthwhile moving this function into a better file,
even if then it needs a prototype in core.h (it can't be inlined anyway
since it's called through a function pointer in the work struct)

> +       if (!wdev->netdev)
> +               return;

This obviously cannot happen.

All the code you added to nl80211.c is racy.

johannes

^ permalink raw reply

* Re: [PATCH v2 1/2] mac80211: Remove invalid flag operations in mesh TSF synchronization
From: Johannes Berg @ 2016-12-13 15:23 UTC (permalink / raw)
  To: Masashi Honma, me; +Cc: linux-wireless
In-Reply-To: <1481159751-4097-1-git-send-email-masashi.honma@gmail.com>

On Thu, 2016-12-08 at 10:15 +0900, Masashi Honma wrote:
> mesh_sync_offset_adjust_tbtt() implements Extensible synchronization
> framework ([1] 13.13.2 Extensible synchronization framework). It
> shall
> not operate the flag "TBTT Adjusting subfield" ([1] 8.4.2.100.8 Mesh
> Capability), since it is used only for MBCA ([1] 13.13.4 Mesh beacon
> collision avoidance, see 13.13.4.4.3 TBTT scanning and adjustment
> procedures for detail). So this patch remove the flag operations.
> 

Both applied; I changed this patch to remove ifmsh->adjusting_tbtt
completely since it was now unused.

johannes

^ permalink raw reply

* Re: [PATCH v2 2/2] net: rfkill: Add rfkill-any LED trigger
From: Johannes Berg @ 2016-12-13 15:18 UTC (permalink / raw)
  To: Michał Kępień, David S . Miller
  Cc: linux-wireless, netdev, linux-kernel
In-Reply-To: <20161208073052.12988-2-kernel@kempniu.pl>

On Thu, 2016-12-08 at 08:30 +0100, Michał Kępień wrote:
> Add a new "global" (i.e. not per-rfkill device) LED trigger, rfkill-
> any,
> which may be useful on laptops with a single "radio LED" and multiple
> radio transmitters.  The trigger is meant to turn a LED on whenever
> there is at least one radio transmitter active and turn it off
> otherwise.
> 

Also applied, but I moved the discussion of the mutex into the recorded
commit log.

johannes

^ permalink raw reply

* Re: [PATCH v2 1/2] net: rfkill: Cleanup error handling in rfkill_init()
From: Johannes Berg @ 2016-12-13 15:16 UTC (permalink / raw)
  To: Michał Kępień, David S . Miller
  Cc: linux-wireless, netdev, linux-kernel
In-Reply-To: <20161208073052.12988-1-kernel@kempniu.pl>

On Thu, 2016-12-08 at 08:30 +0100, Michał Kępień wrote:
> Use a separate label per error condition in rfkill_init() to make it
> a bit cleaner and easier to extend.

applied.

johannes

^ permalink raw reply

* Re: [PATCH v2] mac80211: Ensure enough headroom when forwarding mesh pkt
From: Johannes Berg @ 2016-12-13 15:09 UTC (permalink / raw)
  To: Cedric Izoard, linux-wireless@vger.kernel.org
In-Reply-To: <1ffe01100a724290ab910d68980604ba@ceva-dsp.com>

On Wed, 2016-12-07 at 09:59 +0000, Cedric Izoard wrote:
> When a buffer is duplicated during MESH packet forwarding,
> this patch ensures that the new buffer has enough headroom.

Applied.

johannes

^ permalink raw reply

* Re: [PATCH] ath9k: unlock rcu read when returning early
From: Felix Fietkau @ 2016-12-13 13:52 UTC (permalink / raw)
  To: Tobias Klausmann, kvalo, helgaas, linux-kernel, linux-pci,
	marc.zyngier, Janusz.Dziedzic, rmanohar, ath9k-devel,
	linux-wireless, rmanohar, bharat.kumar.gogada
In-Reply-To: <7b4b7748-06d6-92d4-228c-e7ebf00f8699@mni.thm.de>

On 2016-12-13 14:41, Tobias Klausmann wrote:
> On 13.12.2016 11:41, Felix Fietkau wrote:
>> On 2016-12-12 19:50, Tobias Klausmann wrote:
>>> diff --git a/drivers/net/wireless/ath/ath9k/xmit.c b/drivers/net/wireless/ath/ath9k/xmit.c
>>> index 52bfbb988611..857d5ae09a1d 100644
>>> --- a/drivers/net/wireless/ath/ath9k/xmit.c
>>> +++ b/drivers/net/wireless/ath/ath9k/xmit.c
>>> @@ -2787,6 +2787,7 @@ void ath_tx_edma_tasklet(struct ath_softc *sc)
>>>   		fifo_list = &txq->txq_fifo[txq->txq_tailidx];
>>>   		if (list_empty(fifo_list)) {
>>>   			ath_txq_unlock(sc, txq);
>>> +			rcu_read_unlock();
>> Technically this is fine as well, but I'd prefer a fix where you replace
>> the 'return' with 'break', thus avoiding the duplication of
>> rcu_read_unlock()
> 
> Actually if you want to avoid it, maybe skipping over the rest is better 
> (as originally intended):
> 
> ...
> 
> ath_txq_unlock(sc, txq);
> 
> 
> goto unlock;
> }
> ...
> 
> unlock:
> rcu_read_unlock();
There are already other places that skip to the rcu_read_unlock() part
by using 'break'. I don't see how adding an unnecessary goto makes
things any better.

- Felix

^ permalink raw reply

* Re: [RFC v2 05/11] ath10k: htc: refactorization
From: Michal Kazior @ 2016-12-13 13:52 UTC (permalink / raw)
  To: Valo, Kalle
  Cc: Erik Stromdahl, linux-wireless@vger.kernel.org,
	ath10k@lists.infradead.org
In-Reply-To: <87inqoymd0.fsf@kamboji.qca.qualcomm.com>

On 13 December 2016 at 14:44, Valo, Kalle <kvalo@qca.qualcomm.com> wrote:
> Erik Stromdahl <erik.stromdahl@gmail.com> writes:
>
>> Code refactorization:
>>
>> Moved the code for ep 0 in ath10k_htc_rx_completion_handler
>> to ath10k_htc_control_rx_complete.
>>
>> This eases the implementation of SDIO/mbox significantly since
>> the ep_rx_complete cb is invoked directly from the SDIO/mbox
>> hif layer.
>>
>> Since the ath10k_htc_control_rx_complete already is present
>> (only containing a warning message) there is no reason for not
>> using it (instead of having a special case for ep 0 in
>> ath10k_htc_rx_completion_handler).
>>
>> Signed-off-by: Erik Stromdahl <erik.stromdahl@gmail.com>
>
> I tested this on QCA988X PCI board just to see if there are any
> regressions. It crashes immediately during module load, every time, and
> bisected that the crashing starts on this patch:
>
> [ 1239.715325] ath10k_pci 0000:02:00.0: pci irq msi oper_irq_mode 2 irq_m=
ode 0 reset_mode 0
> [ 1239.885125] ath10k_pci 0000:02:00.0: Direct firmware load for ath10k/p=
re-cal-pci-0000:02:00.0.bin failed with error -2
> [ 1239.885260] ath10k_pci 0000:02:00.0: Direct firmware load for ath10k/c=
al-pci-0000:02:00.0.bin failed with error -2
> [ 1239.885687] ath10k_pci 0000:02:00.0: qca988x hw2.0 target 0x4100016c c=
hip_id 0x043202ff sub 0000:0000
> [ 1239.885699] ath10k_pci 0000:02:00.0: kconfig debug 1 debugfs 1 tracing=
 1 dfs 1 testmode 1
> [ 1239.885899] ath10k_pci 0000:02:00.0: firmware ver 10.2.4.70.59-2 api 5=
 features no-p2p,raw-mode,mfp,allows-mesh-bcast crc32 4159f498
> [ 1239.941836] ath10k_pci 0000:02:00.0: Direct firmware load for ath10k/Q=
CA988X/hw2.0/board-2.bin failed with error -2
> [ 1239.941993] ath10k_pci 0000:02:00.0: board_file api 1 bmi_id N/A crc32=
 bebc7c08
> [ 1241.136693] BUG: unable to handle kernel NULL pointer dereference at  =
 (null)
> [ 1241.136738] IP: [<  (null)>]   (null)
> [ 1241.136759] *pdpt =3D 0000000000000000 *pde =3D f0002a55f0002a55 [ 124=
1.136781]
> [ 1241.136793] Oops: 0010 [#1] SMP
>
> What's odd is that when I added some printks on my own and enabled both
> boot and htc debug levels it doesn't crash anymore. After everything
> works normally after that, I can start AP mode and connect to it. Is it
> a race somewhere?

Yes. htc_wait_target() is called after hif_start(). The ep_rx_complete
is set in htc_wait_target() [changed patch 4, but still too late].

ep_rx_complete must be set prior to calling hif_start(). You probably
crash on end of ath10k_htc_rx_completion_handler() when trying to call
ep->ep_ops.ep_rx_complete(ar, skb).


Micha=C5=82

^ permalink raw reply

* Re: [RFC v2 05/11] ath10k: htc: refactorization
From: Valo, Kalle @ 2016-12-13 13:44 UTC (permalink / raw)
  To: Erik Stromdahl; +Cc: linux-wireless@vger.kernel.org, ath10k@lists.infradead.org
In-Reply-To: <1479496971-19174-6-git-send-email-erik.stromdahl@gmail.com>

Erik Stromdahl <erik.stromdahl@gmail.com> writes:

> Code refactorization:
>
> Moved the code for ep 0 in ath10k_htc_rx_completion_handler
> to ath10k_htc_control_rx_complete.
>
> This eases the implementation of SDIO/mbox significantly since
> the ep_rx_complete cb is invoked directly from the SDIO/mbox
> hif layer.
>
> Since the ath10k_htc_control_rx_complete already is present
> (only containing a warning message) there is no reason for not
> using it (instead of having a special case for ep 0 in
> ath10k_htc_rx_completion_handler).
>
> Signed-off-by: Erik Stromdahl <erik.stromdahl@gmail.com>

I tested this on QCA988X PCI board just to see if there are any
regressions. It crashes immediately during module load, every time, and
bisected that the crashing starts on this patch:

[ 1239.715325] ath10k_pci 0000:02:00.0: pci irq msi oper_irq_mode 2 irq_mod=
e 0 reset_mode 0
[ 1239.885125] ath10k_pci 0000:02:00.0: Direct firmware load for ath10k/pre=
-cal-pci-0000:02:00.0.bin failed with error -2
[ 1239.885260] ath10k_pci 0000:02:00.0: Direct firmware load for ath10k/cal=
-pci-0000:02:00.0.bin failed with error -2
[ 1239.885687] ath10k_pci 0000:02:00.0: qca988x hw2.0 target 0x4100016c chi=
p_id 0x043202ff sub 0000:0000
[ 1239.885699] ath10k_pci 0000:02:00.0: kconfig debug 1 debugfs 1 tracing 1=
 dfs 1 testmode 1
[ 1239.885899] ath10k_pci 0000:02:00.0: firmware ver 10.2.4.70.59-2 api 5 f=
eatures no-p2p,raw-mode,mfp,allows-mesh-bcast crc32 4159f498
[ 1239.941836] ath10k_pci 0000:02:00.0: Direct firmware load for ath10k/QCA=
988X/hw2.0/board-2.bin failed with error -2
[ 1239.941993] ath10k_pci 0000:02:00.0: board_file api 1 bmi_id N/A crc32 b=
ebc7c08
[ 1241.136693] BUG: unable to handle kernel NULL pointer dereference at   (=
null)
[ 1241.136738] IP: [<  (null)>]   (null)
[ 1241.136759] *pdpt =3D 0000000000000000 *pde =3D f0002a55f0002a55 [ 1241.=
136781]=20
[ 1241.136793] Oops: 0010 [#1] SMP

What's odd is that when I added some printks on my own and enabled both
boot and htc debug levels it doesn't crash anymore. After everything
works normally after that, I can start AP mode and connect to it. Is it
a race somewhere?

--=20
Kalle Valo=

^ permalink raw reply

* Re: [PATCH] ath9k: unlock rcu read when returning early
From: Tobias Klausmann @ 2016-12-13 13:41 UTC (permalink / raw)
  To: Felix Fietkau, kvalo, helgaas, linux-kernel, linux-pci,
	marc.zyngier, Janusz.Dziedzic, rmanohar, ath9k-devel,
	linux-wireless, rmanohar, bharat.kumar.gogada
In-Reply-To: <e15c5efc-d548-8b32-ca86-5ec26506ff7c@nbd.name>



On 13.12.2016 11:41, Felix Fietkau wrote:
> On 2016-12-12 19:50, Tobias Klausmann wrote:
>> Starting with ath9k: use ieee80211_tx_status_noskb where possible
>> [d94a461d7a7df68991fb9663531173f60ef89c68] the driver uses rcu_read_lock() &&
>> rcu_read_unlock() yet on returning early in ath_tx_edma_tasklet() the unlock is
>> missing leading to stalls and suspicious RCU usage:
>>
>>   ===============================
>>   [ INFO: suspicious RCU usage. ]
>>   4.9.0-rc8 #11 Not tainted
>>   -------------------------------
>>   kernel/rcu/tree.c:705 Illegal idle entry in RCU read-side critical section.!
>>
>>   other info that might help us debug this:
>>
>>   RCU used illegally from idle CPU!
>>   rcu_scheduler_active = 1, debug_locks = 0
>>   RCU used illegally from extended quiescent state!
>>   1 lock held by swapper/7/0:
>>   #0:
>>    (
>>   rcu_read_lock
>>   ){......}
>>   , at:
>>   [<ffffffffa06ed110>] ath_tx_edma_tasklet+0x0/0x450 [ath9k]
>>
>>   stack backtrace:
>>   CPU: 7 PID: 0 Comm: swapper/7 Not tainted 4.9.0-rc8 #11
>>   Hardware name: Acer Aspire V3-571G/VA50_HC_CR, BIOS V2.21 12/16/2013
>>    ffff88025efc3f38 ffffffff8132b1e5 ffff88017ede4540 0000000000000001
>>    ffff88025efc3f68 ffffffff810a25f7 ffff88025efcee60 ffff88017edebdd8
>>    ffff88025eeb5400 0000000000000091 ffff88025efc3f88 ffffffff810c3cd4
>>   Call Trace:
>>    <IRQ>
>>    [<ffffffff8132b1e5>] dump_stack+0x68/0x93
>>    [<ffffffff810a25f7>] lockdep_rcu_suspicious+0xd7/0x110
>>    [<ffffffff810c3cd4>] rcu_eqs_enter_common.constprop.85+0x154/0x200
>>    [<ffffffff810c5a54>] rcu_irq_exit+0x44/0xa0
>>    [<ffffffff81058631>] irq_exit+0x61/0xd0
>>    [<ffffffff81018d25>] do_IRQ+0x65/0x110
>>    [<ffffffff81672189>] common_interrupt+0x89/0x89
>>    <EOI>
>>    [<ffffffff814ffe11>] ? cpuidle_enter_state+0x151/0x200
>>    [<ffffffff814ffee2>] cpuidle_enter+0x12/0x20
>>    [<ffffffff8109a6ae>] call_cpuidle+0x1e/0x40
>>    [<ffffffff8109a8f6>] cpu_startup_entry+0x146/0x220
>>    [<ffffffff810336f8>] start_secondary+0x148/0x170
>>
>> Signed-off-by: Tobias Klausmann <tobias.johannes.klausmann@mni.thm.de>
>> ---
>>   drivers/net/wireless/ath/ath9k/xmit.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/net/wireless/ath/ath9k/xmit.c b/drivers/net/wireless/ath/ath9k/xmit.c
>> index 52bfbb988611..857d5ae09a1d 100644
>> --- a/drivers/net/wireless/ath/ath9k/xmit.c
>> +++ b/drivers/net/wireless/ath/ath9k/xmit.c
>> @@ -2787,6 +2787,7 @@ void ath_tx_edma_tasklet(struct ath_softc *sc)
>>   		fifo_list = &txq->txq_fifo[txq->txq_tailidx];
>>   		if (list_empty(fifo_list)) {
>>   			ath_txq_unlock(sc, txq);
>> +			rcu_read_unlock();
> Technically this is fine as well, but I'd prefer a fix where you replace
> the 'return' with 'break', thus avoiding the duplication of
> rcu_read_unlock()

Actually if you want to avoid it, maybe skipping over the rest is better 
(as originally intended):

...

ath_txq_unlock(sc, txq);


goto unlock;
}
...

unlock:
rcu_read_unlock();

Thanks,
Tobias
>
> Thanks,
>
> - Felix
>

^ permalink raw reply

* Re: [RFC v2 11/11] ath10k: Added sdio support
From: Valo, Kalle @ 2016-12-13 13:10 UTC (permalink / raw)
  To: Erik Stromdahl; +Cc: linux-wireless@vger.kernel.org, ath10k@lists.infradead.org
In-Reply-To: <1479496971-19174-12-git-send-email-erik.stromdahl@gmail.com>

Erik Stromdahl <erik.stromdahl@gmail.com> writes:

> Initial HIF sdio/mailbox implementation.
>
> Signed-off-by: Erik Stromdahl <erik.stromdahl@gmail.com>

While testing this I noticed few new warnings:

drivers/net/wireless/ath/ath10k/sdio.c: In function ath10k_sdio_probe:
drivers/net/wireless/ath/ath10k/sdio.c:1723:6: warning: 'ret' may be used u=
ninitialized in this function [-Wuninitialized]
drivers/net/wireless/ath/ath10k/sdio.c:375:5: warning: symbol 'ath10k_sdio_=
mbox_rxmsg_pending_handler' was not declared. Should it be static?
drivers/net/wireless/ath/ath10k/sdio.c:1018:5: warning: symbol 'ath10k_sdio=
_hif_tx_sg' was not declared. Should it be static?
drivers/net/wireless/ath/ath10k/sdio.c:1415:5: warning: symbol 'ath10k_sdio=
_hif_exchange_bmi_msg' was not declared. Should it be static?
drivers/net/wireless/ath/ath10k/sdio.c:1555:5: warning: symbol 'ath10k_sdio=
_hif_map_service_to_pipe' was not declared. Should it be static?
drivers/net/wireless/ath/ath10k/sdio.c:1635:6: warning: symbol 'ath10k_sdio=
_hif_get_default_pipe' was not declared. Should it be static?
drivers/net/wireless/ath/ath10k/htc.c:265: line over 90 characters
drivers/net/wireless/ath/ath10k/htc.c:355: line over 90 characters

--=20
Kalle Valo=

^ permalink raw reply

* Re: [PATCH 1/2] ath10k: add accounting for the extended peer statistics
From: Christian Lamparter @ 2016-12-13 12:41 UTC (permalink / raw)
  To: Mohammed Shafi Shajakhan; +Cc: linux-wireless, ath10k, Kalle Valo
In-Reply-To: <20161207062824.GA3404@atheros-ThinkPad-T61>

Hello,

It looks like google put your mail into the spam-can. 
I'm sorry for not answering sooner.

On Wednesday, December 7, 2016 11:58:24 AM CET Mohammed Shafi Shajakhan wrote:
> On Mon, Dec 05, 2016 at 10:52:45PM +0100, Christian Lamparter wrote:
> > The 10.4 firmware adds extended peer information to the
> > firmware's statistics payload. This additional info is
> > stored as a separate data field and the elements are
> > stored in their own "peers_extd" list.
> > 
> > These elements can pile up in the same way as the peer
> > information elements. This is because the
> > ath10k_wmi_10_4_op_pull_fw_stats() function tries to
> > pull the same amount (num_peer_stats) for every statistic
> > data unit.
> > 
> > Fixes: 4a49ae94a448faa ("ath10k: fix 10.4 extended peer stats update")
> > Signed-off-by: Christian Lamparter <chunkeey@googlemail.com>
> > ---
> >  drivers/net/wireless/ath/ath10k/debug.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/net/wireless/ath/ath10k/debug.c b/drivers/net/wireless/ath/ath10k/debug.c
> > index 82a4c67f3672..4acd9eb65910 100644
> > --- a/drivers/net/wireless/ath/ath10k/debug.c
> > +++ b/drivers/net/wireless/ath/ath10k/debug.c
> > @@ -399,6 +399,7 @@ void ath10k_debug_fw_stats_process(struct ath10k *ar, struct sk_buff *skb)
> >  			 * prevent firmware from DoS-ing the host.
> >  			 */
> >  			ath10k_fw_stats_peers_free(&ar->debug.fw_stats.peers);
> > +			ath10k_fw_extd_stats_peers_free(&ar->debug.fw_stats.peers_extd);
> 
> [shafi] thanks for fixing this !
> 
> >  			ath10k_warn(ar, "dropping fw peer stats\n");
> >  			goto free;
> >  		}
> > @@ -409,10 +410,12 @@ void ath10k_debug_fw_stats_process(struct ath10k *ar, struct sk_buff *skb)
> >  			goto free;
> >  		}
> >  
> > +		if (!list_empty(&stats.peers))
> 
> [shafi] sorry please correct me if i am wrong, for 'extended peer stats' we are checking
> for normal 'peer stats' ? Is this the fix intended, i had started a build to
> check your change and we will keep you posted, does this fix displaying
> 'rx_duration' in ath10k fw_stats.
The idea is not to queue any "extended peer stats" when there where no "peer stats" to
begin with. Because otherwise, the function is still vulnerable to OOM since the 
extended peers stats will be queued unchecked (not that this is currently a problem).
 
> > +			list_splice_tail_init(&stats.peers_extd,
> > +					      &ar->debug.fw_stats.peers_extd);
> > +
> >  		list_splice_tail_init(&stats.peers, &ar->debug.fw_stats.peers);
> >  		list_splice_tail_init(&stats.vdevs, &ar->debug.fw_stats.vdevs);
> > -		list_splice_tail_init(&stats.peers_extd,
> > -				      &ar->debug.fw_stats.peers_extd);
> >  	}
> >  
> >  	complete(&ar->debug.fw_stats_complete);

Regards,
Christian

^ permalink raw reply

* Re: [PATCH v2 0/7] ath9k: EEPROM swapping improvements
From: Valo, Kalle @ 2016-12-13 12:03 UTC (permalink / raw)
  To: Martin Blumenstingl
  Cc: ath9k-devel, linux-wireless@vger.kernel.org,
	ath9k-devel@lists.ath9k.org, devicetree@vger.kernel.org,
	arnd@arndb.de, chunkeey@googlemail.com, nbd@nbd.name
In-Reply-To: <CAFBinCC6JWBhZwma=66fBi3_to2SaHOMNDQS23jHNhcc+RUcYQ@mail.gmail.com>

Martin Blumenstingl <martin.blumenstingl@googlemail.com> writes:

> Hello Kalle,
>
> On Fri, Nov 25, 2016 at 4:06 PM, Valo, Kalle <kvalo@qca.qualcomm.com> wro=
te:
>> Kalle Valo <kvalo@codeaurora.org> writes:
>>
>>> Martin Blumenstingl <martin.blumenstingl@googlemail.com> writes:
>>>
>>>> There are two types of swapping the EEPROM data in the ath9k driver.
>>>> Before this series one type of swapping could not be used without the
>>>> other.
>>>>
>>>> The first type of swapping looks at the "magic bytes" at the start of
>>>> the EEPROM data and performs swab16 on the EEPROM contents if needed.
>>>> The second type of swapping is EEPROM format specific and swaps
>>>> specific fields within the EEPROM itself (swab16, swab32 - depends on
>>>> the EEPROM format).
>>>>
>>>> With this series the second part now looks at the EEPMISC register
>>>> inside the EEPROM, which uses a bit to indicate if the EEPROM data
>>>> is Big Endian (this is also done by the FreeBSD kernel).
>>>> This has a nice advantage: currently there are some out-of-tree hacks
>>>> (in OpenWrt and LEDE) where the EEPROM has a Big Endian header on a
>>>> Big Endian system (=3D no swab16 is performed) but the EEPROM itself
>>>> indicates that it's data is Little Endian. Until now the out-of-tree
>>>> code simply did a swab16 before passing the data to ath9k, so ath9k
>>>> first did the swab16 - this also enabled the format specific swapping.
>>>> These out-of-tree hacks are still working with the new logic, but it
>>>> is recommended to remove them. This implementation is based on a
>>>> discussion with Arnd Bergmann who raised concerns about the
>>>> robustness and portability of the swapping logic in the original OF
>>>> support patch review, see [0].
>>>>
>>>> After a second round of patches (=3D v1 of this series) neither Arnd
>>>> Bergmann nor I were really happy with the complexity of the EEPROM
>>>> swapping logic. Based on a discussion (see [1] and [2]) we decided
>>>> that ath9k should use a defined format (specifying the endianness
>>>> of the data - I went with __le16 and __le32) when accessing the
>>>> EEPROM fields. A benefit of this is that we enable the EEPMISC based
>>>> swapping logic by default, just like the FreeBSD driver, see [3]. On
>>>> the devices which I have tested (see below) ath9k now works without
>>>> having to specify the "endian_check" field in ath9k_platform_data (or
>>>> a similar logic which could provide this via devicetree) as ath9k now
>>>> detects the endianness automatically. Only EEPROMs which are mangled
>>>> by some out-of-tree code still need the endian_check flag (or one can
>>>> simply remove that mangling from the out-of-tree code).
>>>>
>>>> Testing:
>>>> - tested by myself on AR9287 with Big Endian EEPROM
>>>> - tested by myself on AR9227 with Little Endian EEPROM
>>>> - tested by myself on AR9381 (using the ar9003_eeprom implementation,
>>>>   which did not suffer from this whole problem)
>>>> - how do we proceed with testing? maybe we could keep this in a
>>>>   feature-branch and add these patches to LEDE once we have an ACK to
>>>>   get more people to test this
>>>>
>>>> This series depends on my other series (v7):
>>>> "add devicetree support to ath9k" - see [4]
>>>
>>> I think this looks pretty good. If there's a bug somewhere it should be
>>> quite easy to fix so I'm not that worried and would be willing to take
>>> these as soon as I have applied the dependency series. IIRC your
>>> devicetree patches will have at least one more review round so that wil=
l
>>> take some time still. In the meantime it would be great if LEDE folks
>>> could take a look at these and comment (or test).
>>
>> So are everyone happy with this? I haven't seen any comments. If I don't
>> here anything I'm planning to take these, most likely for 4.11.
>
> the patches have been in LEDE for almost two weeks now and I did not
> see any reports of ath9k breakage (footnote below).
>
> It seems that there are a few devices out there where the whole EEPROM
> is swab16'ed which switches the position of the 1-byte fields
> opCapFlags and eepMisc.
> those still work fine with the new code, however I had a second patch
> in LEDE [0] which results in ath9k_platform_data.endian_check NOT
> being set anymore.
> that endian_check flag was used before to swab16 the whole EEPROM, to
> correct the position of the 1-byte fields again.
> Currently we are fixing this in the firmware hotplug script: [1]
> This is definitely not a blocker for this series though (if we want to
> have a devicetree replacement for "ath9k_platform_data.endian_check"
> then I'd work on that within a separate series, but I somewhat
> consider these EEPROMs as "broken" so fixing them in
> userspace/firmware hotplug script is fine for me)

Sounds good to me, thanks for the thorough followup. I'm planning to
apply these any day.

--=20
Kalle Valo=

^ permalink raw reply

* Re: [PATCH] orinoco: Use shash instead of ahash for MIC calculations
From: Kalle Valo @ 2016-12-13 11:35 UTC (permalink / raw)
  To: Andy Lutomirski
  Cc: linux-kernel, linux-usb, linux-wireless, Eric Biggers,
	linux-crypto, Herbert Xu, Stephan Mueller
In-Reply-To: <8818c45b9ec6a04d85fabf9bb437cf119fd23659.1481575835.git.luto@kernel.org>

Andy Lutomirski <luto@kernel.org> writes:

> Eric Biggers pointed out that the orinoco driver pointed scatterlists
> at the stack.
>
> Fix it by switching from ahash to shash.  The result should be
> simpler, faster, and more correct.
>
> Cc: stable@vger.kernel.org # 4.9 only
> Reported-by: Eric Biggers <ebiggers3@gmail.com>
> Signed-off-by: Andy Lutomirski <luto@kernel.org>

"more correct"? Does this fix a real user visible bug or what? And why
just stable 4.9, does this maybe have something to do with
CONFIG_VMAP_STACK?

I'm just wondering should I push this to 4.10 or -next. This is a driver
for ancient hardware so I'm starting to lean for -next.

-- 
Kalle Valo

^ permalink raw reply

* Re: [PATCH] ath9k: unlock rcu read when returning early
From: Felix Fietkau @ 2016-12-13 10:41 UTC (permalink / raw)
  To: Tobias Klausmann, kvalo, helgaas, linux-kernel, linux-pci,
	marc.zyngier, Janusz.Dziedzic, rmanohar, ath9k-devel,
	linux-wireless, rmanohar, bharat.kumar.gogada
In-Reply-To: <20161212185001.3857-1-tobias.johannes.klausmann@mni.thm.de>

On 2016-12-12 19:50, Tobias Klausmann wrote:
> Starting with ath9k: use ieee80211_tx_status_noskb where possible
> [d94a461d7a7df68991fb9663531173f60ef89c68] the driver uses rcu_read_lock() &&
> rcu_read_unlock() yet on returning early in ath_tx_edma_tasklet() the unlock is
> missing leading to stalls and suspicious RCU usage:
> 
>  ===============================
>  [ INFO: suspicious RCU usage. ]
>  4.9.0-rc8 #11 Not tainted
>  -------------------------------
>  kernel/rcu/tree.c:705 Illegal idle entry in RCU read-side critical section.!
> 
>  other info that might help us debug this:
> 
>  RCU used illegally from idle CPU!
>  rcu_scheduler_active = 1, debug_locks = 0
>  RCU used illegally from extended quiescent state!
>  1 lock held by swapper/7/0:
>  #0:
>   (
>  rcu_read_lock
>  ){......}
>  , at:
>  [<ffffffffa06ed110>] ath_tx_edma_tasklet+0x0/0x450 [ath9k]
> 
>  stack backtrace:
>  CPU: 7 PID: 0 Comm: swapper/7 Not tainted 4.9.0-rc8 #11
>  Hardware name: Acer Aspire V3-571G/VA50_HC_CR, BIOS V2.21 12/16/2013
>   ffff88025efc3f38 ffffffff8132b1e5 ffff88017ede4540 0000000000000001
>   ffff88025efc3f68 ffffffff810a25f7 ffff88025efcee60 ffff88017edebdd8
>   ffff88025eeb5400 0000000000000091 ffff88025efc3f88 ffffffff810c3cd4
>  Call Trace:
>   <IRQ>
>   [<ffffffff8132b1e5>] dump_stack+0x68/0x93
>   [<ffffffff810a25f7>] lockdep_rcu_suspicious+0xd7/0x110
>   [<ffffffff810c3cd4>] rcu_eqs_enter_common.constprop.85+0x154/0x200
>   [<ffffffff810c5a54>] rcu_irq_exit+0x44/0xa0
>   [<ffffffff81058631>] irq_exit+0x61/0xd0
>   [<ffffffff81018d25>] do_IRQ+0x65/0x110
>   [<ffffffff81672189>] common_interrupt+0x89/0x89
>   <EOI>
>   [<ffffffff814ffe11>] ? cpuidle_enter_state+0x151/0x200
>   [<ffffffff814ffee2>] cpuidle_enter+0x12/0x20
>   [<ffffffff8109a6ae>] call_cpuidle+0x1e/0x40
>   [<ffffffff8109a8f6>] cpu_startup_entry+0x146/0x220
>   [<ffffffff810336f8>] start_secondary+0x148/0x170
> 
> Signed-off-by: Tobias Klausmann <tobias.johannes.klausmann@mni.thm.de>
> ---
>  drivers/net/wireless/ath/ath9k/xmit.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/net/wireless/ath/ath9k/xmit.c b/drivers/net/wireless/ath/ath9k/xmit.c
> index 52bfbb988611..857d5ae09a1d 100644
> --- a/drivers/net/wireless/ath/ath9k/xmit.c
> +++ b/drivers/net/wireless/ath/ath9k/xmit.c
> @@ -2787,6 +2787,7 @@ void ath_tx_edma_tasklet(struct ath_softc *sc)
>  		fifo_list = &txq->txq_fifo[txq->txq_tailidx];
>  		if (list_empty(fifo_list)) {
>  			ath_txq_unlock(sc, txq);
> +			rcu_read_unlock();
Technically this is fine as well, but I'd prefer a fix where you replace
the 'return' with 'break', thus avoiding the duplication of
rcu_read_unlock()

Thanks,

- Felix

^ permalink raw reply

* Re: [PATCH] ath9k: unlock rcu read when returning early
From: Kalle Valo @ 2016-12-13  9:59 UTC (permalink / raw)
  To: Tobias Klausmann
  Cc: helgaas, linux-kernel, linux-pci, marc.zyngier, Janusz.Dziedzic,
	rmanohar, ath9k-devel, linux-wireless, rmanohar,
	bharat.kumar.gogada, Felix Fietkau
In-Reply-To: <20161212185001.3857-1-tobias.johannes.klausmann@mni.thm.de>

Tobias Klausmann <tobias.johannes.klausmann@mni.thm.de> writes:

> Starting with ath9k: use ieee80211_tx_status_noskb where possible
> [d94a461d7a7df68991fb9663531173f60ef89c68]

The correct format to reference a commit in the commit log is:

Starting with commit d94a461d7a7d ("ath9k: use ieee80211_tx_status_noskb
where possible") the...

> the driver uses rcu_read_lock() && rcu_read_unlock() yet on returning
> early in ath_tx_edma_tasklet() the unlock is missing leading to stalls
> and suspicious RCU usage:
>
>  ===============================
>  [ INFO: suspicious RCU usage. ]
>  4.9.0-rc8 #11 Not tainted
>  -------------------------------
>  kernel/rcu/tree.c:705 Illegal idle entry in RCU read-side critical section.!
>
>  other info that might help us debug this:
>
>  RCU used illegally from idle CPU!
>  rcu_scheduler_active = 1, debug_locks = 0
>  RCU used illegally from extended quiescent state!
>  1 lock held by swapper/7/0:
>  #0:
>   (
>  rcu_read_lock
>  ){......}
>  , at:
>  [<ffffffffa06ed110>] ath_tx_edma_tasklet+0x0/0x450 [ath9k]
>
>  stack backtrace:
>  CPU: 7 PID: 0 Comm: swapper/7 Not tainted 4.9.0-rc8 #11
>  Hardware name: Acer Aspire V3-571G/VA50_HC_CR, BIOS V2.21 12/16/2013
>   ffff88025efc3f38 ffffffff8132b1e5 ffff88017ede4540 0000000000000001
>   ffff88025efc3f68 ffffffff810a25f7 ffff88025efcee60 ffff88017edebdd8
>   ffff88025eeb5400 0000000000000091 ffff88025efc3f88 ffffffff810c3cd4
>  Call Trace:
>   <IRQ>
>   [<ffffffff8132b1e5>] dump_stack+0x68/0x93
>   [<ffffffff810a25f7>] lockdep_rcu_suspicious+0xd7/0x110
>   [<ffffffff810c3cd4>] rcu_eqs_enter_common.constprop.85+0x154/0x200
>   [<ffffffff810c5a54>] rcu_irq_exit+0x44/0xa0
>   [<ffffffff81058631>] irq_exit+0x61/0xd0
>   [<ffffffff81018d25>] do_IRQ+0x65/0x110
>   [<ffffffff81672189>] common_interrupt+0x89/0x89
>   <EOI>
>   [<ffffffff814ffe11>] ? cpuidle_enter_state+0x151/0x200
>   [<ffffffff814ffee2>] cpuidle_enter+0x12/0x20
>   [<ffffffff8109a6ae>] call_cpuidle+0x1e/0x40
>   [<ffffffff8109a8f6>] cpu_startup_entry+0x146/0x220
>   [<ffffffff810336f8>] start_secondary+0x148/0x170
>
> Signed-off-by: Tobias Klausmann <tobias.johannes.klausmann@mni.thm.de>

A fixes line and cc stable would be good to have:

Fixes: d94a461d7a7d ("ath9k: use ieee80211_tx_status_noskb where possible")
Cc: <stable@vger.kernel.org> # v4.9

I can add those.

I'm planning to push this to 4.10 but would prefer to see an ack from
Felix (the author of d94a461d7a7d) first. I added him to Cc.

-- 
Kalle Valo

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox