* Re: Two rtlwifi drivers?
From: Greg Kroah-Hartman @ 2017-10-11 13:13 UTC (permalink / raw)
To: Kalle Valo
Cc: Larry Finger, Dan Carpenter, Ping-Ke Shih, Yan-Hsuan Chuang,
Johannes Berg, Souptick Joarder, devel, linux-wireless,
kernel-janitors
In-Reply-To: <87k202qcjr.fsf@kamboji.qca.qualcomm.com>
On Wed, Oct 11, 2017 at 12:06:00PM +0300, Kalle Valo wrote:
> (Sorry for taking so long with the reply, I wanted first to check what
> the rtlwifi in staging contains.)
>
> Larry Finger <Larry.Finger@lwfinger.net> writes:
>
> > On 08/24/2017 07:14 AM, Kalle Valo wrote:
> >> Dan Carpenter <dan.carpenter@oracle.com> writes:
> >>
> >>> Smatch is distrustful of the "capab" value and marks it as user
> >>> controlled. I think it actually comes from the firmware? Anyway, I
> >>> looked at other drivers and they added a bounds check and it seems like
> >>> a harmless thing to have so I have added it here as well.
> >>>
> >>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> >>>
> >>> diff --git a/drivers/staging/rtlwifi/base.c b/drivers/staging/rtlwifi/base.c
> >>> index f7f207cbaee3..a30b928d5ee1 100644
> >>> --- a/drivers/staging/rtlwifi/base.c
> >>> +++ b/drivers/staging/rtlwifi/base.c
> >>
> >> I'm getting slightly annoyed that we now apparently have two duplicate
> >> rtlwifi drivers (with the same name!) and I'm being spammed by staging
> >> patches. Was this really a smart thing to do? And what will be the
> >> future of these two drivers?
> >>
> >> (Of course this is not directed to Dan, he is just fixing bugs found by
> >> smatch, but more like a general question.)
> >
> > That was the decision that you and Greg made. The version in
> > wireless-drivers needs many patches to handle the new device. The
> > progress in applying these to wireless-drivers was very slow for many
> > reasons. Keeping a single version of that code would have required
> > coordination between you and Greg, which was discouraged.
>
> I don't recall deciding anything, all I did was asking for more info
> about the new code. I was against the idea since I first saw your mail
> but I tried to be diplomatic and not shot it down immeadiately. Shows
> that diplomacy is not really my thing...
>
> We always take extra measures to avoid forking code, why is it now all
> of sudden ok? Also this gives the wrong message to Realtek, and other
> vendors, that they can just fork the driver and push all sort of crap to
> staging.
>
> So just to make clear to everyone: I think forking drivers like this is
> a HORRIBLE idea and I do not want to have anything to do with that. If
> schedule goes over quality then a much better approach is to move to the
> whole driver to staging than to have duplicated drivers like we have
> now.
I think it's horrid too. But, if no one is able to do the real work
here, we hurt users who just need to use their hardware to get things
done.
And it seems like the company isn't willing to do the real work, so
dumping this in staging is the best we can do at the moment.
However, if this causes you problems, that's not good at all either.
Getting "real" support for this hardware is key, and hopefully can
happen somehow. But fixing up the staging driver is almost never the
way to do it, as you know :)
So what to do? Any ideas? What makes your life easier? You can just
ignore the staging tree, as it should not affect your portion of the
kernel at all, right?
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH] rtl8xxxu: mark expected switch fall-throughs
From: Jes Sorensen @ 2017-10-11 13:34 UTC (permalink / raw)
To: Kalle Valo; +Cc: Gustavo A. R. Silva, linux-wireless, netdev, linux-kernel
In-Reply-To: <87o9peqdo2.fsf@kamboji.qca.qualcomm.com>
On 10/11/2017 04:41 AM, Kalle Valo wrote:
> Jes Sorensen <jes.sorensen@gmail.com> writes:
>
>> On 10/10/2017 03:30 PM, Gustavo A. R. Silva wrote:
>>> In preparation to enabling -Wimplicit-fallthrough, mark switch cases
>>> where we are expecting to fall through.
>>
>> While this isn't harmful, to me this looks like pointless patch churn
>> for zero gain and it's just ugly.
>
> In general I find it useful to mark fall through cases. And it's just a
> comment with two words, so they cannot hurt your eyes that much.
I don't see them being harmful in the code, but I don't see them of much
use either. If it happened as part of natural code development, fine. My
objection is to people running around doing this systematically causing
patch churn for little to zero gain.
Jes
^ permalink raw reply
* [PATCH] mac80211: use crypto_aead_authsize()
From: Johannes Berg @ 2017-10-11 13:47 UTC (permalink / raw)
To: linux-wireless; +Cc: Johannes Berg
From: Johannes Berg <johannes.berg@intel.com>
Evidently this API is intended to be used to isolate against
API changes, so use it instead of accessing ->authsize.
Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
net/mac80211/aead_api.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/mac80211/aead_api.c b/net/mac80211/aead_api.c
index 347f13953b2c..160f9df30402 100644
--- a/net/mac80211/aead_api.c
+++ b/net/mac80211/aead_api.c
@@ -21,7 +21,7 @@
int aead_encrypt(struct crypto_aead *tfm, u8 *b_0, u8 *aad, size_t aad_len,
u8 *data, size_t data_len, u8 *mic)
{
- size_t mic_len = tfm->authsize;
+ size_t mic_len = crypto_aead_authsize(tfm);
struct scatterlist sg[3];
struct aead_request *aead_req;
int reqsize = sizeof(*aead_req) + crypto_aead_reqsize(tfm);
@@ -52,7 +52,7 @@ int aead_encrypt(struct crypto_aead *tfm, u8 *b_0, u8 *aad, size_t aad_len,
int aead_decrypt(struct crypto_aead *tfm, u8 *b_0, u8 *aad, size_t aad_len,
u8 *data, size_t data_len, u8 *mic)
{
- size_t mic_len = tfm->authsize;
+ size_t mic_len = crypto_aead_authsize(tfm);
struct scatterlist sg[3];
struct aead_request *aead_req;
int reqsize = sizeof(*aead_req) + crypto_aead_reqsize(tfm);
--
2.14.2
^ permalink raw reply related
* Re: [RFC] mac80211: Add airtime fairness accounting
From: Toke Høiland-Jørgensen @ 2017-10-11 13:50 UTC (permalink / raw)
To: Johannes Berg, make-wifi-fast, linux-wireless
In-Reply-To: <1507712135.1998.27.camel@sipsolutions.net>
Johannes Berg <johannes@sipsolutions.net> writes:
>> Yeah, ath9k certainly gets that as part of the RX information from
>> the chip.
>
> Yeah, though there are more complex cases, e.g. with hardware A-MSDU
> deaggregation like in ath10k.
Here be dragons; noted :)
>> Well, some of the tests I did while porting ath9k to the iTXQs
>> indicated that for voice-like traffic we can get very close to the
>> same actual end-to-end latency for BE-marked traffic that we do with
>> VO-marked traffic. This is in part because the FQ sparse flow
>> prioritisation makes sure that such flows get queueing priority.
>
> That really depends on medium saturation - if that's low, then yeah,
> the difference is in the EDCA parameters and the minimum backoff,
> which isn't a huge difference if you measure end-to-end.
>
> Given saturation/congestion though, and I think we have to take this
> into account since not everyone will be using this code, there are
> measurable advantages to using VO - like in the test I mentioned where
> you can block out BE "forever".
Yup, when the medium is saturated the tradeoff might be different. My
hope is that it is possible to dynamically detect when it might be
beneficial and change behaviour based on this. I know this is maybe a
bit hand-wavy for now, but otherwise it wouldn't be research... ;)
>
>> Now obviously, there are going to be tradeoffs, and scenarios where
>> latency will suffer. But I would at least like to be able to explore
>> this, and I think with the API we are currently discussing that will
>> be possible.
>
> I'm not sure how you envision this working with the API, since you
> still have to pull from multiple TXQs, but I have no objection to the
> API as is - I'm just not convinced that actually demoting traffic is a
> good idea :)
Sure, I realise I have some convincing to do on this point...
>> Another thing I want to explore is doing soft admission
>> control; i.e., if someone sends bulk traffic marked as VO, it will be
>> automatically demoted to BE traffic rather than locking everyone else
>> out. We've tested that with some success in the Cake scheduler, and
>> it may be applicable to WiFi as well.
>
> It seems pretty strange to do that at this level - we control what TID
> (and AC) is selected for the traffic? So why not just change at that
> level for this type of thing?
>
> You can probably even already do that with TC by re-marking the traffic
> depending on type or throughput or whatever else TC can classify on.
Mostly because I am expecting that the current queue state is an
important input variable. It's quite straight forward to (e.g.) remark
VO packets if they exceed a fixed rate. But what I want to do is demote
packets if they exceed what the network can currently handle. It may be
that visibility into the queueing is enough, though. I have yet to
actually experiment with this...
-Toke
^ permalink raw reply
* Re: [RFC 1/3] mac80211: Add TXQ scheduling API
From: Toke Høiland-Jørgensen @ 2017-10-11 13:54 UTC (permalink / raw)
To: Johannes Berg, make-wifi-fast, linux-wireless
In-Reply-To: <1507711593.1998.20.camel@sipsolutions.net>
Johannes Berg <johannes@sipsolutions.net> writes:
> On Tue, 2017-10-10 at 19:51 +0200, Toke H=C3=B8iland-J=C3=B8rgensen wrote:
>
>> > > + struct list_head active_txqs;
>> > > + spinlock_t active_txq_lock;
>> >=20
>> > Is there much point in having a separate lock? We probably need the
>> > fq lock in most places related to this anyway?
>>=20
>> Well, once the scheduler gets a bit smarter it may be necessary to
>> much with the order of TXQs on there without touching any of the
>> queues (e.g., when calculating airtime usage on TX and RX
>> completion). Not sure if that is enough to warrant a separate lock,
>> though; I hadn't thought about just grabbing fq->lock...
>
> Ok, dunno. It seemed sort of related but perhaps it's not really.
Yeah. I'll keep this in mind when I implement the airtime fairness
accounting.
>> > Also maybe you should only do that if the TXQ isn't *empty*, so the
>> > driver could call this unconditionally?
>>=20
>> There can be cases where the driver wants the queue to be scheduled
>> even though it looks empty from mac80211's point of view. For ath9k,
>> the driver keeps its retry queue in the drv_priv part of the txq
>> structure, so it will check if that is empty before deciding to call
>> the schedule function.
>>=20
>> This is also related to the PS behaviour, so guess this could be
>> changed once that is all TXQ-based...
>
> Interesting. I guess scheduling an empty queue doesn't really matter
> for mac80211 anyway though - just some extra work if we try to get
> frames from it.
Yes, and I figure giving the driver that option might be a good way to
have some flexibility.
-Toke
^ permalink raw reply
* Re: Two rtlwifi drivers?
From: Dan Carpenter @ 2017-10-11 13:54 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Kalle Valo, Larry Finger, Ping-Ke Shih, Yan-Hsuan Chuang,
Johannes Berg, Souptick Joarder, devel, linux-wireless,
kernel-janitors
In-Reply-To: <20171011131310.GF32250@kroah.com>
On Wed, Oct 11, 2017 at 03:13:10PM +0200, Greg Kroah-Hartman wrote:
> And it seems like the company isn't willing to do the real work, so
> dumping this in staging is the best we can do at the moment.
I'm more optimistic. There are a lot of @realtek.com addresses in the
CC list and that's a new thing.
regards,
dan carpenter
^ permalink raw reply
* Re: [RFC 1/3] mac80211: Add TXQ scheduling API
From: Toke Høiland-Jørgensen @ 2017-10-11 14:06 UTC (permalink / raw)
To: Johannes Berg, make-wifi-fast, linux-wireless
In-Reply-To: <1507711498.1998.18.camel@sipsolutions.net>
Johannes Berg <johannes@sipsolutions.net> writes:
>> Well I was trying to do The Right Thing(tm), obviously ;)
>
> :-)
>
>> The driver_buffered field comes from the previous behaviour of ath9k:
>> It would basically set the station_buffered flag if there was
>> something in the retry queue at the time it goes to sleep. And on
>> wakeup, it will reschedule the txq if it has anything in the retry
>> queue. And, well, it just seemed odd that there was this duplicated
>> logic in the driver and mac80211, if the driver could just signal to
>> mac80211 to reschedule for it...
>
> Ok. I'm not sure what ath9k does, but mac80211 makes a distinction
> between "do I have something buffered" and "does the driver have
> something buffered".
>
> When the station wakes up it's pretty easy, we tell the driver and it
> just has to send its own frames first.
>
> But when we get a PS-Poll/U-APSD it's more complicated, and we have to
> tell the driver "please release N frames you have buffered" or we may
> have to tell it "please allow me to send N frames that I have buffered"
> which is why we need the distinction of whether or not the driver has
> something buffered.
>
> Ultimately, with TXQs, I hope this distinction can go away because the
> driver wouldn't buffer all by itself.
Yes, that would be good. Okay, I'll keep PS buffering the way it works
now.
>> But I guess I was really just getting ahead of myself there; so the
>> right thing to do for now is to keep the old behaviour, and then fix
>> it properly afterwards?
>
> I guess that'd be easier.
>
>> Yes, this makes sense. Each sleep period is pretty short, right?
>
> Not necessarily.
>
>> I.e., we don't need to deal with interactions between CoDel and the
>> queue being stopped for a long period of time?
>
> I don't know enough about the possible interactions to answer that.
> Sleep periods may be long, though typically if there's traffic for
> stations then they want to retrieve that and will wake up relatively
> soon, but "relatively soon" may still be on the order of hundreds of
> milliseconds (or even seconds) because sometimes clients will only
> listen to DTIM beacons (DTIM period * beacon interval), and e.g. with
> WNM sleep mode it becomes even longer.
Ah. Right. Well, guess we'll have to cross that bridge once we get
there. Not sure what the right thing to do from the point of view of an
AQM in this instance. But at least making sure it doesn't drop the
packet at the head of every queue at wakeup would probably be good.
>> I was envisioning that next_txq() could also make those kinds of
>> decisions (i.e., preempt the normal scheduling algorithm when a
>> "special" TXQ needs to be scheduled immediately). But not sure if
>> that is enough for this case?
>
> Hmm, not sure. We really want this to be scheduled pretty much
> immediately because the other side will be waiting for the frames, and
> if we don't get an answer out quickly it'll have to assume we're
> broken. I don't know what the limit here is for our firmware, but we
> should really get this out as soon as possible in this case.
OK. But presumably it can't preempt packets already pushed to the
hardware, right? So telling the driver to immediately schedule a packet,
and making sure that the txq it gets from next_txq() is the right one
should do the trick? But I guess it's a bit of a roundabout way, which
may not be worth it to avoid an extra callback...
-Toke
^ permalink raw reply
* Re: [PATCH v3] mac80211: aead api to reduce redundancy
From: Xiang Gao @ 2017-10-11 14:12 UTC (permalink / raw)
To: Johannes Berg; +Cc: David S. Miller, linux-kernel, linux-wireless, netdev
In-Reply-To: <1507708082.1998.9.camel@sipsolutions.net>
Great ! Thanks !
Xiang Gao
2017-10-11 3:48 GMT-04:00 Johannes Berg <johannes@sipsolutions.net>:
> On Tue, 2017-10-10 at 22:31 -0400, Xiang Gao wrote:
>> Currently, the aes_ccm.c and aes_gcm.c are almost line by line copy
>> of
>> each other. This patch reduce code redundancy by moving the code in
>> these
>> two files to crypto/aead_api.c to make it a higher level aead api.
>> The
>> file aes_ccm.c and aes_gcm.c are removed and all the functions there
>> are
>> now implemented in their headers using the newly added aead api.
>
> Applied. FWIW, the sta_dynamic_down_up test that the robot complained
> about, and the PSK tests that heavily use CCMP all pass.
>
> johannes
^ permalink raw reply
* Re: [RFC 1/3] mac80211: Add TXQ scheduling API
From: Johannes Berg @ 2017-10-11 14:15 UTC (permalink / raw)
To: Toke Høiland-Jørgensen, make-wifi-fast, linux-wireless
In-Reply-To: <87tvz5pymm.fsf@toke.dk>
On Wed, 2017-10-11 at 16:06 +0200, Toke Høiland-Jørgensen wrote:
> > Hmm, not sure. We really want this to be scheduled pretty much
> > immediately because the other side will be waiting for the frames,
> > and
> > if we don't get an answer out quickly it'll have to assume we're
> > broken. I don't know what the limit here is for our firmware, but
> > we
> > should really get this out as soon as possible in this case.
>
> OK. But presumably it can't preempt packets already pushed to the
> hardware, right?
True. If there are still packets scheduled then it needs even more
driver tricks to drop those back to tx_pending first ...
> So telling the driver to immediately schedule a packet,
> and making sure that the txq it gets from next_txq() is the right one
> should do the trick? But I guess it's a bit of a roundabout way,
> which may not be worth it to avoid an extra callback...
Yeah, might work, but remember that we need to mangle the packet, and
for that we need to know how many packets will go out. E.g. with U-
APSD, if we tell the driver 8 packets are OK, and it only wants 6, then
that's acceptable, but we have to tag the last of those with EOSP ...
So one way or another I think we need a separate callback here, and
perhaps just have the driver do the EOSP tagging, or have a separate
function to pull the frames so mac80211 can do the tagging, dunno.
Note that this is only for _some_ drivers, others will implement much
of this in firmware.
johannes
^ permalink raw reply
* Re: Two rtlwifi drivers?
From: Larry Finger @ 2017-10-11 14:19 UTC (permalink / raw)
To: Greg Kroah-Hartman, Kalle Valo
Cc: Dan Carpenter, Ping-Ke Shih, Yan-Hsuan Chuang, Johannes Berg,
Souptick Joarder, devel, linux-wireless, kernel-janitors
In-Reply-To: <20171011131310.GF32250@kroah.com>
On 10/11/2017 08:13 AM, Greg Kroah-Hartman wrote:
> On Wed, Oct 11, 2017 at 12:06:00PM +0300, Kalle Valo wrote:
>> (Sorry for taking so long with the reply, I wanted first to check what
>> the rtlwifi in staging contains.)
>>
>> Larry Finger <Larry.Finger@lwfinger.net> writes:
>>
>>> On 08/24/2017 07:14 AM, Kalle Valo wrote:
>>>> Dan Carpenter <dan.carpenter@oracle.com> writes:
>>>>
>>>>> Smatch is distrustful of the "capab" value and marks it as user
>>>>> controlled. I think it actually comes from the firmware? Anyway, I
>>>>> looked at other drivers and they added a bounds check and it seems like
>>>>> a harmless thing to have so I have added it here as well.
>>>>>
>>>>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>>>>>
>>>>> diff --git a/drivers/staging/rtlwifi/base.c b/drivers/staging/rtlwifi/base.c
>>>>> index f7f207cbaee3..a30b928d5ee1 100644
>>>>> --- a/drivers/staging/rtlwifi/base.c
>>>>> +++ b/drivers/staging/rtlwifi/base.c
>>>>
>>>> I'm getting slightly annoyed that we now apparently have two duplicate
>>>> rtlwifi drivers (with the same name!) and I'm being spammed by staging
>>>> patches. Was this really a smart thing to do? And what will be the
>>>> future of these two drivers?
>>>>
>>>> (Of course this is not directed to Dan, he is just fixing bugs found by
>>>> smatch, but more like a general question.)
>>>
>>> That was the decision that you and Greg made. The version in
>>> wireless-drivers needs many patches to handle the new device. The
>>> progress in applying these to wireless-drivers was very slow for many
>>> reasons. Keeping a single version of that code would have required
>>> coordination between you and Greg, which was discouraged.
>>
>> I don't recall deciding anything, all I did was asking for more info
>> about the new code. I was against the idea since I first saw your mail
>> but I tried to be diplomatic and not shot it down immeadiately. Shows
>> that diplomacy is not really my thing...
>>
>> We always take extra measures to avoid forking code, why is it now all
>> of sudden ok? Also this gives the wrong message to Realtek, and other
>> vendors, that they can just fork the driver and push all sort of crap to
>> staging.
>>
>> So just to make clear to everyone: I think forking drivers like this is
>> a HORRIBLE idea and I do not want to have anything to do with that. If
>> schedule goes over quality then a much better approach is to move to the
>> whole driver to staging than to have duplicated drivers like we have
>> now.
>
> I think it's horrid too. But, if no one is able to do the real work
> here, we hurt users who just need to use their hardware to get things
> done.
>
> And it seems like the company isn't willing to do the real work, so
> dumping this in staging is the best we can do at the moment.
>
> However, if this causes you problems, that's not good at all either.
> Getting "real" support for this hardware is key, and hopefully can
> happen somehow. But fixing up the staging driver is almost never the
> way to do it, as you know :)
>
> So what to do? Any ideas? What makes your life easier? You can just
> ignore the staging tree, as it should not affect your portion of the
> kernel at all, right?
Greg and Kalle,
We can all agree that this situation is bad; however, several of the points made
in your E-mails need to be addressed.
First of all, I am not an employee of Realtek, and I have no knowledge of the
internals of any of their chips. I have never signed a non-disclosure agreement,
and the only thing that I have received from them are sample chips for testing.
My main interest is in helping the user experience. As there are a number of
users with the new RTL8822BE device, that meant getting an in-kernel driver to
them as soon as possible. As stated earlier, adding this particular device to
the rtlwifi family involved roughly 120K lines of new code. Given our recent
experience in getting code accepted into the wireless tree meant that this
additional code would not have been accepted for many months. For that reason,
we chose the staging route. It is important that we get this right as Realtek
tells me that there will be two additional new drivers in the coming 6 months.
As to the convergence of the rtlwifi code between staging and wireless, I
applied the 40 or 50 patches in my queue to the wireless version to create the
version that went into staging. If any of the current patches to wireless do not
seem to be in the staging version, sometimes temporary changes are necessary so
that the intermediate drivers will build and work. Yes, it is code churn, but
necessary to keep patches small. In keeping with Kalle's requests, only a
fraction of those patches have been submitted to him.
My intent is to have the RTL8822BE driver moved from staging to mainline no
later than 4.17. The affected drivers rtl_pci, rtlwifi and becoex will be moved
in that order. Of course, the required changes will need to be in wireless
before staging is changed, which will slow the process. As I see it, the switch
can only occur with a new version.
My opinion is that the company has gone a long ways toward meeting the
requirements of the Linux kernel. A lot of their code is written for Windows and
Linux, with emphasis on Windows, which complicates matters as not all of the
changes for Linux can be backported. Prior to the introduction of these drivers
in 2.6.38, drivers were released periodically as tar files with no information
regarding intermediate steps were recorded other than the SVN modification
number. At least now, we get relatively small changes.
I have been pleased and surprised at the stability and performance of the driver
in staging. Other than possible changes required by reviewers when it is moved,
it is ready for the wireless tree.
Finally, I have notified my Realtek contact that I do not plan to continue as
the maintainer of these drivers very much longer due to my age. I still have the
interest, but not always the energy. The people at Realtek have proposed a plan
that seems workable. Of course, there will be hiccups, but the current process
does not seem to be that smooth.
Larry
^ permalink raw reply
* Re: [RFC 1/3] mac80211: Add TXQ scheduling API
From: Toke Høiland-Jørgensen @ 2017-10-11 14:29 UTC (permalink / raw)
To: Johannes Berg, make-wifi-fast, linux-wireless
In-Reply-To: <1507731344.1998.36.camel@sipsolutions.net>
Johannes Berg <johannes@sipsolutions.net> writes:
> On Wed, 2017-10-11 at 16:06 +0200, Toke H=C3=B8iland-J=C3=B8rgensen wrote:
>
>> > Hmm, not sure. We really want this to be scheduled pretty much
>> > immediately because the other side will be waiting for the frames,
>> > and
>> > if we don't get an answer out quickly it'll have to assume we're
>> > broken. I don't know what the limit here is for our firmware, but
>> > we
>> > should really get this out as soon as possible in this case.
>>=20
>> OK. But presumably it can't preempt packets already pushed to the
>> hardware, right?=20
>
> True. If there are still packets scheduled then it needs even more
> driver tricks to drop those back to tx_pending first ...
Only for packets to the same station, right?
>> So telling the driver to immediately schedule a packet,
>> and making sure that the txq it gets from next_txq() is the right one
>> should do the trick? But I guess it's a bit of a roundabout way,
>> which may not be worth it to avoid an extra callback...
>
> Yeah, might work, but remember that we need to mangle the packet, and
> for that we need to know how many packets will go out. E.g. with U-
> APSD, if we tell the driver 8 packets are OK, and it only wants 6, then
> that's acceptable, but we have to tag the last of those with EOSP ...
>
> So one way or another I think we need a separate callback here, and
> perhaps just have the driver do the EOSP tagging, or have a separate
> function to pull the frames so mac80211 can do the tagging, dunno.
Yeah, sounds like it'll need a separate callback, or at least a flag.
What part of the standard do I have to read to learn how this is
supposed to work, BTW? Or even better, is there a resource that
describes how PS works that is more accessible than the standard itself?
> Note that this is only for _some_ drivers, others will implement much
> of this in firmware.
Right, of course. Fun times! :)
-Toke
^ permalink raw reply
* Re: [RFC 1/3] mac80211: Add TXQ scheduling API
From: Johannes Berg @ 2017-10-11 14:34 UTC (permalink / raw)
To: Toke Høiland-Jørgensen, make-wifi-fast, linux-wireless
In-Reply-To: <87fuapbvws.fsf@toke.dk>
On Wed, 2017-10-11 at 16:29 +0200, Toke Høiland-Jørgensen wrote:
> Johannes Berg <johannes@sipsolutions.net> writes:
>
> > On Wed, 2017-10-11 at 16:06 +0200, Toke Høiland-Jørgensen wrote:
> >
> > > > Hmm, not sure. We really want this to be scheduled pretty much
> > > > immediately because the other side will be waiting for the
> > > > frames,
> > > > and
> > > > if we don't get an answer out quickly it'll have to assume
> > > > we're
> > > > broken. I don't know what the limit here is for our firmware,
> > > > but
> > > > we
> > > > should really get this out as soon as possible in this case.
> > >
> > > OK. But presumably it can't preempt packets already pushed to the
> > > hardware, right?
> >
> > True. If there are still packets scheduled then it needs even more
> > driver tricks to drop those back to tx_pending first ...
>
> Only for packets to the same station, right?
Yes, but they need to be somehow taken off the queue to avoid
reordering. Usually the firmware will filter them, but synchronisation
is difficult, since what might happen is that some frames are already
filtered, others aren't, and then the later ones actually need to go
out while the earlier ones don't ... if that's not done, you get
reordering and the state machines get out of sync.
> What part of the standard do I have to read to learn how this is
> supposed to work, BTW? Or even better, is there a resource that
> describes how PS works that is more accessible than the standard
> itself?
Hmm. I don't think the standard will actually help you much here, it
just describes the over-the-air behaviour.
Most of the complexity here comes from having to deal with driver
buffering, mac80211 buffering, addressing the reordering problem
(described above) from queuing frames for multiple stations on the same
hw queue.
johannes
^ permalink raw reply
* Re: Setting single rate in ath10k broken by "reject/clear user rate mask if not usable"
From: Ben Greear @ 2017-10-11 14:51 UTC (permalink / raw)
To: Johannes Berg, linux-wireless@vger.kernel.org, ath10k, kirtika,
Johannes Berg
In-Reply-To: <1507708948.1998.15.camel@sipsolutions.net>
On 10/11/2017 01:02 AM, Johannes Berg wrote:
> Hi,
>
>> #iw dev vap206 set bitrates legacy-5 ht-mcs-5 0 vht-mcs-5
>> command failed: Invalid argument (-22)
>>
>> But, it actually *does* successfully set the rate in the driver
>> first, which is confusing at best.
>
> Huh?
The call to set the rate in the driver comes before the error
check.
if (ieee80211_hw_check(&local->hw, HAS_RATE_CONTROL)) {
ret = drv_set_bitrate_mask(local, sdata, mask);
if (ret) {
pr_err("%s: drv-set-bitrate-mask had error return: %d\n",
sdata->dev->name, ret);
return ret;
}
}
/*
* If active validate the setting and reject it if it doesn't leave
* at least one basic rate usable, since we really have to be able
* to send something, and if we're an AP we have to be able to do
* so at a basic rate so that all clients can receive it.
*/
if (rcu_access_pointer(sdata->vif.chanctx_conf) &&
sdata->vif.bss_conf.chandef.chan) {
u32 basic_rates = sdata->vif.bss_conf.basic_rates;
enum nl80211_band band = sdata->vif.bss_conf.chandef.chan->band;
if (!(mask->control[band].legacy & basic_rates)) {
#### I changed this code so I could set a single rate... --Ben
pr_err("%s: WARNING: no legacy rates for band[%d] in set-bitrate-mask.\n",
sdata->dev->name, band);
}
}
>
>> So, I think we should relax this check, at least for ath10k.
>
> Well, yes and no. I don't think we should make ath10k special here, and
> this fixes a real problem - namely that you can set up the system so
> that you have no usable rates at all, and then you just get a WARN_ON
> and start using the lowest possible rate...
Well, there are a million ways to set up a broken system, and setting a single
rate has a useful purpose, especially with ath10k since it has such limited
rate-setting capabilities.
There is basically never going to be a case where setting a single tx-rate on an
AP is a good idea in a general production environment, so maybe a possible WARN-ON is fine?
If you *do* set up an AP with a limited rateset, then it should simply fail to
allow a station to connect if it does not have any matching rates. This might go
back to a previous idea I had (and patches I posted and carry in my tree) to allow setting a different
advertise rateset vs usable tx-rateset.
For existing stations that might not match the new fixed rate, we could purposefully kick
them off I guess, but seems like a lot of work for a case that seems pretty irrelevant.
>
>> commit e8e4f5280ddd0a7b43a795f90a0758e3c99df6a6
>> Author: Johannes Berg <johannes.berg@intel.com>
>> Date: Wed Mar 8 11:12:10 2017 +0100
>>
>> mac80211: reject/clear user rate mask if not usable
>>
>> If the user rate mask results in no (basic) rates being usable,
>> clear it. Also, if we're already operating when it's set, reject
>> it instead.
>>
>> Technically, selecting basic rates as the criterion is a bit too
>> restrictive, but calculating the usable rates over all stations
>> (e.g. in AP mode) is harder, and all stations must support the
>> basic rates. Similarly, in client mode, the basic rates will be
>> used anyway for control frames.
>
> I guess you could implement this part? I.e. iterating the clients and
> checking that they all support the rate that is set. However, then you
> also need to implement that this gets reset when a new client that
> doesn't support this rate connects.
>
> Overall, this isn't very well defined for AP mode...
>
> Perhaps it'd be better - as you pointed out in the other thread - to
> have API to force a rate per station? We already have that for iwlwifi
> in debugfs, so perhaps that'd be something to consider for this too,
> I'm not sure there would be a real need to have it in nl80211?
I looked closely at the ath10k firmware yesterday, and it has no way to set a specific
single rate per peer. Sure, I could hack something into my CT firmware, but that
still leaves all the stock driver/firmware people out of luck, and my patches
won't make it upstream in the main kernel...
Thanks,
Ben
--
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc http://www.candelatech.com
^ permalink raw reply
* Re: [PATCH] rtl8xxxu: mark expected switch fall-throughs
From: Gustavo A. R. Silva @ 2017-10-11 14:32 UTC (permalink / raw)
To: Jes Sorensen; +Cc: Kalle Valo, linux-wireless, netdev, linux-kernel, Kees Cook
In-Reply-To: <eb89f2ad-f2d5-b97f-c224-daf3953d912c@gmail.com>
Hi Jes,
Quoting Jes Sorensen <jes.sorensen@gmail.com>:
> On 10/11/2017 04:41 AM, Kalle Valo wrote:
>> Jes Sorensen <jes.sorensen@gmail.com> writes:
>>
>>> On 10/10/2017 03:30 PM, Gustavo A. R. Silva wrote:
>>>> In preparation to enabling -Wimplicit-fallthrough, mark switch cases
>>>> where we are expecting to fall through.
>>>
>>> While this isn't harmful, to me this looks like pointless patch churn
>>> for zero gain and it's just ugly.
>>
>> In general I find it useful to mark fall through cases. And it's just a
>> comment with two words, so they cannot hurt your eyes that much.
>
> I don't see them being harmful in the code, but I don't see them of
> much use either. If it happened as part of natural code development,
> fine. My objection is to people running around doing this
> systematically causing patch churn for little to zero gain.
>
> Jes
I understand that you think this is of zero gain for you, but as
Florian Fainelli pointed out:
"That is the canonical way to tell static analyzers and compilers that
fall throughs are wanted and not accidental mistakes in the code. For
people that deal with these kinds of errors, it's quite helpful, unless
you suggest disabling that particular GCC warning specific for that
file/directory?"
this is very helpful for people working on fixing issues reported by
static analyzers. It saves a huge amount of time when dealing with
False Positives. Also, there are cases when an apparently intentional
fall-through turns out to be an actual missing break or continue.
So there is an ongoing effort to detect such cases and avoid them to
show up in the future by at least warning people about a potential
issue in their code. And this is helpful for everybody.
Thanks
--
Gustavo A. R. Silva
^ permalink raw reply
* Re: [PATCH] rtl8xxxu: mark expected switch fall-throughs
From: Kees Cook @ 2017-10-11 17:00 UTC (permalink / raw)
To: Gustavo A. R. Silva
Cc: Jes Sorensen, Kalle Valo, linux-wireless, Network Development,
LKML
In-Reply-To: <20171011093248.Horde.Jwmh0VKhmCeOxBgQzLLpYeZ@gator4166.hostgator.com>
On Wed, Oct 11, 2017 at 7:32 AM, Gustavo A. R. Silva
<garsilva@embeddedor.com> wrote:
> Quoting Jes Sorensen <jes.sorensen@gmail.com>:
>> On 10/11/2017 04:41 AM, Kalle Valo wrote:
>>> Jes Sorensen <jes.sorensen@gmail.com> writes:
>>>> On 10/10/2017 03:30 PM, Gustavo A. R. Silva wrote:
>>>>>
>>>>> In preparation to enabling -Wimplicit-fallthrough, mark switch cases
>>>>> where we are expecting to fall through.
>>>>
>>>> While this isn't harmful, to me this looks like pointless patch churn
>>>> for zero gain and it's just ugly.
>>>
>>> In general I find it useful to mark fall through cases. And it's just a
>>> comment with two words, so they cannot hurt your eyes that much.
>>
>> I don't see them being harmful in the code, but I don't see them of much
>> use either. If it happened as part of natural code development, fine. My
>> objection is to people running around doing this systematically causing
>> patch churn for little to zero gain.
>
> I understand that you think this is of zero gain for you, but as Florian
> Fainelli pointed out:
>
> "That is the canonical way to tell static analyzers and compilers that
> fall throughs are wanted and not accidental mistakes in the code. For
> people that deal with these kinds of errors, it's quite helpful, unless
> you suggest disabling that particular GCC warning specific for that
> file/directory?"
>
> this is very helpful for people working on fixing issues reported by static
> analyzers. It saves a huge amount of time when dealing with False Positives.
> Also, there are cases when an apparently intentional fall-through turns out
> to be an actual missing break or continue.
>
> So there is an ongoing effort to detect such cases and avoid them to show up
> in the future by at least warning people about a potential issue in their
> code. And this is helpful for everybody.
This is an unfortunate omission in the C language, and thankfully both
gcc and clang have stepped up to solve this the same way static
analyzers have solved it. It's not exactly pretty, but it does both
document the intention for humans and provide a way for analyzers to
report issues. Having the compiler help us not make mistakes is quite
handy, and with Gustavo grinding through all the Coverity warnings,
he's found actual bugs with missing "break"s, so I think this has a
demonstrable benefit to the code-base as a whole. It makes things
unambiguous to someone else reviewing the code.
-Kees
--
Kees Cook
Pixel Security
^ permalink raw reply
* Re: pull-request: mac80211-next 2017-10-11
From: David Miller @ 2017-10-11 17:15 UTC (permalink / raw)
To: johannes; +Cc: netdev, linux-wireless
In-Reply-To: <20171011123613.28890-1-johannes@sipsolutions.net>
From: Johannes Berg <johannes@sipsolutions.net>
Date: Wed, 11 Oct 2017 14:36:12 +0200
> Here's a -next pull request. The only bigger thing here is the
> addition of the regulatory database as firmware, which will
> allow us to - over time - get rid of CRDA, as well as having
> the option of adding more fields to the database where needed,
> this would've been extremely complex with CRDA because it had
> not been built with extensibility in mind.
>
> Please pull and let me know if there's any problem.
Pulled, thanks!
^ permalink raw reply
* Re: [PATCH] rtl8xxxu: mark expected switch fall-throughs
From: Joe Perches @ 2017-10-11 17:47 UTC (permalink / raw)
To: David Laight, Gustavo A. R. Silva, Jes Sorensen, Kalle Valo
Cc: linux-wireless@vger.kernel.org, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org, Arnaldo Carvalho de Melo
In-Reply-To: <063D6719AE5E284EB5DD2968C1650D6DD009123C@AcuExch.aculab.com>
On Wed, 2017-10-11 at 12:54 +0000, David Laight wrote:
> From: Joe Perches
> > Sent: 11 October 2017 11:21
> > On Tue, 2017-10-10 at 14:30 -0500, Gustavo A. R. Silva wrote:
> > > In preparation to enabling -Wimplicit-fallthrough, mark switch cases
> > > where we are expecting to fall through.
> >
> > perhaps use Arnaldo's idea:
> >
> > https://lkml.org/lkml/2017/2/9/845
> > https://lkml.org/lkml/2017/2/10/485
>
> gah, that is even uglier and requires a chase through
> headers to find out what it means.
Sure, if you think __fallthrough; isn't self-documenting.
case foo;
bar;
__fallthrough;
case baz;
etc...
^ permalink raw reply
* Atheros TL-721N adapter refuses to enumerate on the USB Bus (ath9k_htc)
From: M D @ 2017-10-11 18:17 UTC (permalink / raw)
To: linux-wireless
Hi,
My system details are as follows
Hardware:
CPU : ARM based platform, imx21
USB : USB Host Controller 1.1 Full Speed Mode
Software:
Firmware : /lib/firmware/htc_9271.fw
(version 1.3 / version 1.4)
Device Driver : Backports (v3.12.8-1)
OS : Linux Kernel 3.0.101, ARM
Freescale IMX21ADS
Problem Statement:
- The WiFi adapter is connected directly to the USB Port and is
configured in Full Speed mode
- After it is plugged in, it is enumerated on the Bus (that is,
executing the instruction lsusb shows the vendor and product
information)
- After repeated reboots, after one random reboot the issue
occurs. (usually 1 out of 5 times) the wlan0 node does not come up
- When the issue occurs the following details below describe
the issue at best
1) The adapter is visible on the USB bus (adapter gets enumerated)
2) The required modules are loaded
3) However the wireless networking node, wlan0 does not come up
4) One of the threads created during the init goes into a D state
root 1024 0.0 0.0 0 0 ? D 11:06 0:00
[firmware/htc_92]
In the file drivers/net/wireless/ath/ath9k/hif_usb.c
[
hif_usb.c
...
ret = request_firmware_nowait(THIS_MODULE, true, hif_dev->fw_name,
&hif_dev->udev->dev, GFP_KERNEL,
hif_dev, ath9k_hif_usb_firmware_cb);
compat_firmware_class.c
...
task = kthread_run(request_firmware_work_func, fw_work,
"firmware/%s", name);
]
5) This is recoverable only after manually unplugging out the Adapter.
ROOT CAUSE OF THE ISSUE:
6) The underlying cause is due to the wifi adapter being configured in
FULL SPEED mode. Due to this the endpoints 3 and 4 get configured as
BULK and not INT.
After some investigation, I made the following changes which helps to
recover the adapter when the issue occurs,
--- ../original-backports/backports-3.12.8-1/drivers/net/wireless/ath/ath9k/hif_usb.c
2017-10-11 10:50:05.043241000 -0700
+++ drivers/net/wireless/ath/ath9k/hif_usb.c 2017-10-11
10:50:03.154255000 -0700
@@ -24,6 +24,8 @@
MODULE_FIRMWARE(FIRMWARE_AR7010_1_1);
MODULE_FIRMWARE(FIRMWARE_AR9271);
+static void ath9k_hif_usb_reboot(struct usb_device *udev);
+
@@ -1189,7 +1193,12 @@
{
struct usb_device *udev = interface_to_usbdev(interface);
struct hif_device_usb *hif_dev;
- int ret = 0;
+
+ struct usb_host_interface *alt ;
+ struct usb_endpoint_descriptor *endp;
+
+ int ret;
+ int idx, EP = 0;
if (id->driver_info == STORAGE_DEVICE)
return send_eject_command(interface);
@@ -1212,6 +1221,44 @@
init_completion(&hif_dev->fw_done);
+ alt = &hif_dev->interface->altsetting[0];
+
+
+ for (idx = 0; idx < alt->desc.bNumEndpoints; idx++) {
+ endp = &alt->endpoint[idx].desc;
+ EP = idx + 1;
+ printk (KERN_ALERT "EndPoint %d is conf as %d\n", EP,
(endp->bmAttributes & USB_ENDPOINT_XFERTYPE_MASK));
+ /* First FIX EP3. EP3 should never show up as BULK.
+ If it does we have a serious problem.
+ */
+
+ if (endp->bEndpointAddress==0x83){
+ if ((endp->bmAttributes &
USB_ENDPOINT_XFERTYPE_MASK) == USB_ENDPOINT_XFER_BULK) {
+ endp->bmAttributes |= USB_ENDPOINT_XFER_INT;
+ endp->bInterval = 1;
+ ath9k_hif_usb_reboot(hif_dev->udev);
+ mdelay(1000);
+ ret = usb_reset_device(hif_dev->udev);
+ mdelay(1000);
+ if (ret)
+ return ret;
+ }
+ }
+
+ if (endp->bEndpointAddress==0x04){
+ if ((endp->bmAttributes &
USB_ENDPOINT_XFERTYPE_MASK) == USB_ENDPOINT_XFER_BULK) {
+ endp->bmAttributes |= USB_ENDPOINT_XFER_INT;
+ endp->bInterval = 1;
+ ath9k_hif_usb_reboot(hif_dev->udev);
+ mdelay(1000);
+ ret = usb_reset_device(hif_dev->udev);
+ mdelay(1000);
+ if (ret)
+ return ret;
+ }
+ }
+ }
+
/* Find out which firmware to load */
if (IS_AR7010_DEVICE(id->driver_info))
I see the following output in dmesg
EndPoint 1 is conf as 2
EndPoint 2 is conf as 2
EndPoint 3 is conf as 2
usb 1-2: reset full speed USB device number 2 using imx21-hcd
usb 1-2: device firmware changed
usb 1-2: USB disconnect, device number 2
usbcore: registered new interface driver ath9k_htc
usb 1-2: new full speed USB device number 3 using imx21-hcd
EndPoint 1 is conf as 2
EndPoint 2 is conf as 2
EndPoint 3 is conf as 3
EndPoint 4 is conf as 2
usb 1-2: reset full speed USB device number 3 using imx21-hcd
EndPoint 5 is conf as 2
EndPoint 6 is conf as 2
usb 1-2: ath9k_htc: Firmware htc_9271.fw requested
usb 1-2: ath9k_htc: Transferred FW: htc_9271.fw, size: 51272
ath9k_htc 1-2:1.0: ath9k_htc: HTC initialized with 33 credits
ath9k_htc 1-2:1.0: ath9k_htc: FW Version: 1.3
Completed ath9k_init_firmware_version
ath: EEPROM regdomain: 0x809c
ath: EEPROM indicates we should expect a country code
ath: doing EEPROM country->regdmn map search
ath: country maps to regdmn code: 0x52
ath: Country alpha2 being used: CN
ath: Regpair used: 0x52
ath9k_hw_set_reset_reg: 1
ieee80211 phy0: Atheros AR9271 Rev:1
Registered led device: ath9k_htc-phy0
These code changes help to reconfigure the WiFi adapter many times.
However at some point of reboot (say 30th reboot), I see
usb 1-2: device descriptor read/64, error -110
hub 1-0:1.0: unable to enumerate USB device on port 2
When this happens, the only way to recover is to manually unplug the
adapter from the USB port.
Could anyone offer any suggestions as to how to overcome this problem?
The issue occurs with both v1.3 and v1.4 htc_9271.fw.
Regards,
Max
^ permalink raw reply
* Re: [PATCH] mwifiex: Random MAC address during scanning
From: Brian Norris @ 2017-10-11 19:41 UTC (permalink / raw)
To: Ganapathi Bhat
Cc: linux-wireless, Cathy Luo, Xinming Hu, Zhiyuan Yang, James Cao,
Mangesh Malusare, Karthik Ananthapadmanabha
In-Reply-To: <1506682390-27716-1-git-send-email-gbhat@marvell.com>
On Fri, Sep 29, 2017 at 04:23:10PM +0530, Ganapathi Bhat wrote:
> From: Karthik Ananthapadmanabha <karthida@marvell.com>
>
> Driver will advertise RANDOM_MAC support only if the device
> supports this feature.
>
> Signed-off-by: Karthik Ananthapadmanabha <karthida@marvell.com>
> Signed-off-by: Ganapathi Bhat <gbhat@marvell.com>
I'd just like to point out that this is a very bad commit subject:
"[PATCH] mwifiex: Random MAC address during scanning"
It's borderline wrong, really. "Random MAC address during scanning" is
already supported. This patch is just adding a feature-flag check for
it, since some firmwares in the wild don't support it. A more accurate
description would be something like:
"[PATCH] mwifiex: Add feature flag support for MAC randomization"
The patch is already applied, so I'd only worry about it for future
submissions (no need to resend).
Brian
^ permalink raw reply
* Re: [PATCH] [net-next]NFC: Convert timers to use timer_setup()
From: Allen @ 2017-10-11 19:52 UTC (permalink / raw)
To: davem; +Cc: linux-wireless, netdev, keescook
In-Reply-To: <1507718024-2134-1-git-send-email-allen.pais@oracle.com>
> Switch to using the new timer_setup() and from_timer()
> for net/nfc/*
>
> Signed-off-by: Allen Pais <allen.pais@oracle.com>
> ---
> ---
> net/nfc/core.c | 8 +++-----
> net/nfc/hci/core.c | 7 +++----
> net/nfc/hci/llc_shdlc.c | 23 +++++++++--------------
> net/nfc/llcp_core.c | 14 ++++++--------
> 4 files changed, 21 insertions(+), 31 deletions(-)
>
My bad, I missed the Reviewed-by.
Reviewed-by: Kees Cook <keescook@chromium.org>
- Allen
^ permalink raw reply
* [PATCH] rtlwifi: Remove unused cur_rfstate variables
From: Christos Gkekas @ 2017-10-11 21:15 UTC (permalink / raw)
To: Larry Finger, Chaoming Li, Kalle Valo, linux-wireless, netdev,
linux-kernel
Cc: Christos Gkekas
Clean up unused cur_rfstate variables in rtl8188ee, rtl8723ae, rtl8723be
and rtl8821ae.
Signed-off-by: Christos Gkekas <chris.gekas@gmail.com>
---
drivers/net/wireless/realtek/rtlwifi/rtl8188ee/hw.c | 4 +---
drivers/net/wireless/realtek/rtlwifi/rtl8723ae/hw.c | 4 +---
drivers/net/wireless/realtek/rtlwifi/rtl8723be/hw.c | 4 +---
drivers/net/wireless/realtek/rtlwifi/rtl8821ae/hw.c | 4 +---
4 files changed, 4 insertions(+), 12 deletions(-)
diff --git a/drivers/net/wireless/realtek/rtlwifi/rtl8188ee/hw.c b/drivers/net/wireless/realtek/rtlwifi/rtl8188ee/hw.c
index 0ba26d2..4d843e6 100644
--- a/drivers/net/wireless/realtek/rtlwifi/rtl8188ee/hw.c
+++ b/drivers/net/wireless/realtek/rtlwifi/rtl8188ee/hw.c
@@ -2235,7 +2235,7 @@ bool rtl88ee_gpio_radio_on_off_checking(struct ieee80211_hw *hw, u8 *valid)
{
struct rtl_priv *rtlpriv = rtl_priv(hw);
struct rtl_ps_ctl *ppsc = rtl_psc(rtl_priv(hw));
- enum rf_pwrstate e_rfpowerstate_toset, cur_rfstate;
+ enum rf_pwrstate e_rfpowerstate_toset;
u32 u4tmp;
bool b_actuallyset = false;
@@ -2254,8 +2254,6 @@ bool rtl88ee_gpio_radio_on_off_checking(struct ieee80211_hw *hw, u8 *valid)
spin_unlock(&rtlpriv->locks.rf_ps_lock);
}
- cur_rfstate = ppsc->rfpwr_state;
-
u4tmp = rtl_read_dword(rtlpriv, REG_GPIO_OUTPUT);
e_rfpowerstate_toset = (u4tmp & BIT(31)) ? ERFON : ERFOFF;
diff --git a/drivers/net/wireless/realtek/rtlwifi/rtl8723ae/hw.c b/drivers/net/wireless/realtek/rtlwifi/rtl8723ae/hw.c
index 5ac7b81..4e1e1f8 100644
--- a/drivers/net/wireless/realtek/rtlwifi/rtl8723ae/hw.c
+++ b/drivers/net/wireless/realtek/rtlwifi/rtl8723ae/hw.c
@@ -2103,7 +2103,7 @@ bool rtl8723e_gpio_radio_on_off_checking(struct ieee80211_hw *hw, u8 *valid)
struct rtl_priv *rtlpriv = rtl_priv(hw);
struct rtl_ps_ctl *ppsc = rtl_psc(rtl_priv(hw));
struct rtl_phy *rtlphy = &(rtlpriv->phy);
- enum rf_pwrstate e_rfpowerstate_toset, cur_rfstate;
+ enum rf_pwrstate e_rfpowerstate_toset;
u8 u1tmp;
bool b_actuallyset = false;
@@ -2122,8 +2122,6 @@ bool rtl8723e_gpio_radio_on_off_checking(struct ieee80211_hw *hw, u8 *valid)
spin_unlock(&rtlpriv->locks.rf_ps_lock);
}
- cur_rfstate = ppsc->rfpwr_state;
-
rtl_write_byte(rtlpriv, REG_GPIO_IO_SEL_2,
rtl_read_byte(rtlpriv, REG_GPIO_IO_SEL_2)&~(BIT(1)));
diff --git a/drivers/net/wireless/realtek/rtlwifi/rtl8723be/hw.c b/drivers/net/wireless/realtek/rtlwifi/rtl8723be/hw.c
index 4d47b97..2ad1013 100644
--- a/drivers/net/wireless/realtek/rtlwifi/rtl8723be/hw.c
+++ b/drivers/net/wireless/realtek/rtlwifi/rtl8723be/hw.c
@@ -2486,7 +2486,7 @@ bool rtl8723be_gpio_radio_on_off_checking(struct ieee80211_hw *hw, u8 *valid)
struct rtl_priv *rtlpriv = rtl_priv(hw);
struct rtl_ps_ctl *ppsc = rtl_psc(rtl_priv(hw));
struct rtl_phy *rtlphy = &(rtlpriv->phy);
- enum rf_pwrstate e_rfpowerstate_toset, cur_rfstate;
+ enum rf_pwrstate e_rfpowerstate_toset;
u8 u1tmp;
bool b_actuallyset = false;
@@ -2505,8 +2505,6 @@ bool rtl8723be_gpio_radio_on_off_checking(struct ieee80211_hw *hw, u8 *valid)
spin_unlock(&rtlpriv->locks.rf_ps_lock);
}
- cur_rfstate = ppsc->rfpwr_state;
-
rtl_write_byte(rtlpriv, REG_GPIO_IO_SEL_2,
rtl_read_byte(rtlpriv, REG_GPIO_IO_SEL_2) & ~(BIT(1)));
diff --git a/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/hw.c b/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/hw.c
index 1d431d4..e756f94 100644
--- a/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/hw.c
+++ b/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/hw.c
@@ -3845,7 +3845,7 @@ bool rtl8821ae_gpio_radio_on_off_checking(struct ieee80211_hw *hw, u8 *valid)
struct rtl_priv *rtlpriv = rtl_priv(hw);
struct rtl_ps_ctl *ppsc = rtl_psc(rtl_priv(hw));
struct rtl_phy *rtlphy = &rtlpriv->phy;
- enum rf_pwrstate e_rfpowerstate_toset, cur_rfstate;
+ enum rf_pwrstate e_rfpowerstate_toset;
u8 u1tmp = 0;
bool b_actuallyset = false;
@@ -3864,8 +3864,6 @@ bool rtl8821ae_gpio_radio_on_off_checking(struct ieee80211_hw *hw, u8 *valid)
spin_unlock(&rtlpriv->locks.rf_ps_lock);
}
- cur_rfstate = ppsc->rfpwr_state;
-
rtl_write_byte(rtlpriv, REG_GPIO_IO_SEL_2,
rtl_read_byte(rtlpriv,
REG_GPIO_IO_SEL_2) & ~(BIT(1)));
--
2.7.4
^ permalink raw reply related
* Re: [PATCH 2/3] rsi: sdio: Add WOWLAN support for S4 hibernate state
From: Brian Norris @ 2017-10-12 0:33 UTC (permalink / raw)
To: Kalle Valo, Amitkumar Karwar
Cc: Amitkumar Karwar, linux-wireless, Amitkumar Karwar,
Prameela Rani Garnepudi, Karun Eagalapati
In-Reply-To: <87fuaqqbpa.fsf@kamboji.qca.qualcomm.com>
Hi Amitkumar,
On Wed, Oct 11, 2017 at 12:24:17PM +0300, Kalle Valo wrote:
> Amitkumar Karwar <amitkarwar@gmail.com> writes:
> > On Tue, Sep 26, 2017 at 3:27 PM, Kalle Valo <kvalo@codeaurora.org> wrote:
> >> And even if that would be the right approach it needs to be properly
> >> described in the commit log, a vague sentence in the end of a commit log
> >> is not enough.
> >
> > Understood. I will add detailed description and send updated version
> > if the patch is fine.
>
> Not sure if this is fine or not. I think what you do here is ugly but I
> guess it's better than nothing?
I don't see why you can't try to reuse the existing mac80211 reset;
seems like you'd need to factor out some pieces of rsi_reset_card() and
call that from the ieee80211_ops::start() method. Then, you just call
ieee80211_restart_hw() from the appropriate place, instead of
implementing your own tear-down/reset.
Brian
^ permalink raw reply
* Re: [PATCH] ath10k: fix core PCI suspend when WoWLAN is supported but disabled
From: Brian Norris @ 2017-10-12 0:38 UTC (permalink / raw)
To: Kalle Valo, Kalle Valo
Cc: ath10k, linux-wireless, Grant Grundler, linux-kernel, Ryan Hsu,
Michal Kazior
In-Reply-To: <20170919232416.108247-1-briannorris@chromium.org>
Ping? Any comments? I know there's more than one way to slice this
problem, but it's most definitely a problem...
Brian
On Tue, Sep 19, 2017 at 04:24:16PM -0700, Brian Norris wrote:
> For devices where the FW supports WoWLAN but user-space has not
> configured it, we don't do any PCI-specific suspend/resume operations,
> because mac80211 doesn't call drv_suspend() when !wowlan. This has
> particularly bad effects for some platforms, because we don't stop the
> power-save timer, and if this timer goes off after the PCI controller
> has suspended the link, Bad Things will happen.
>
> Commit 32faa3f0ee50 ("ath10k: add the PCI PM core suspend/resume ops")
> got some of this right, in that it understood there was a problem on
> non-WoWLAN firmware. But it forgot the $subject case.
>
> Fix this by moving all the PCI driver suspend/resume logic exclusively
> into the driver PM hooks. This shouldn't affect WoWLAN support much
> (this just gets executed later on).
>
> I would just as well kill the entirety of ath10k_hif_suspend(), as it's
> not even implemented on the USB or SDIO drivers. I expect that we don't
> need the callback, except to return "supported" (i.e., 0) or "not
> supported" (i.e., -EOPNOTSUPP).
>
> Fixes: 32faa3f0ee50 ("ath10k: add the PCI PM core suspend/resume ops")
> Fixes: 77258d409ce4 ("ath10k: enable pci soc powersaving")
> Signed-off-by: Brian Norris <briannorris@chromium.org>
> Cc: Ryan Hsu <ryanhsu@qti.qualcomm.com>
> Cc: Kalle Valo <kvalo@qca.qualcomm.com>
> Cc: Michal Kazior <michal.kazior@tieto.com>
> ---
> drivers/net/wireless/ath/ath10k/pci.c | 24 ++++++++++++++----------
> 1 file changed, 14 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
> index bc1633945a56..4655c944e3fd 100644
> --- a/drivers/net/wireless/ath/ath10k/pci.c
> +++ b/drivers/net/wireless/ath/ath10k/pci.c
> @@ -2580,6 +2580,12 @@ void ath10k_pci_hif_power_down(struct ath10k *ar)
> #ifdef CONFIG_PM
>
> static int ath10k_pci_hif_suspend(struct ath10k *ar)
> +{
> + /* Nothing to do; the important stuff is in the driver suspend. */
> + return 0;
> +}
> +
> +static int ath10k_pci_suspend(struct ath10k *ar)
> {
> /* The grace timer can still be counting down and ar->ps_awake be true.
> * It is known that the device may be asleep after resuming regardless
> @@ -2592,6 +2598,12 @@ static int ath10k_pci_hif_suspend(struct ath10k *ar)
> }
>
> static int ath10k_pci_hif_resume(struct ath10k *ar)
> +{
> + /* Nothing to do; the important stuff is in the driver resume. */
> + return 0;
> +}
> +
> +static int ath10k_pci_resume(struct ath10k *ar)
> {
> struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
> struct pci_dev *pdev = ar_pci->pdev;
> @@ -3403,11 +3415,7 @@ static int ath10k_pci_pm_suspend(struct device *dev)
> struct ath10k *ar = dev_get_drvdata(dev);
> int ret;
>
> - if (test_bit(ATH10K_FW_FEATURE_WOWLAN_SUPPORT,
> - ar->running_fw->fw_file.fw_features))
> - return 0;
> -
> - ret = ath10k_hif_suspend(ar);
> + ret = ath10k_pci_suspend(ar);
> if (ret)
> ath10k_warn(ar, "failed to suspend hif: %d\n", ret);
>
> @@ -3419,11 +3427,7 @@ static int ath10k_pci_pm_resume(struct device *dev)
> struct ath10k *ar = dev_get_drvdata(dev);
> int ret;
>
> - if (test_bit(ATH10K_FW_FEATURE_WOWLAN_SUPPORT,
> - ar->running_fw->fw_file.fw_features))
> - return 0;
> -
> - ret = ath10k_hif_resume(ar);
> + ret = ath10k_pci_resume(ar);
> if (ret)
> ath10k_warn(ar, "failed to resume hif: %d\n", ret);
>
> --
> 2.14.1.690.gbb1197296e-goog
>
^ permalink raw reply
* Re: [PATCH] ath10k: fix core PCI suspend when WoWLAN is supported but disabled
From: Adrian Chadd @ 2017-10-12 0:43 UTC (permalink / raw)
To: Brian Norris
Cc: Kalle Valo, Kalle Valo, ath10k@lists.infradead.org,
linux-wireless@vger.kernel.org, Grant Grundler,
Linux Kernel Mailing List, Ryan Hsu, Michal Kazior
In-Reply-To: <20171012003805.GA89253@google.com>
fwiw - I did this on my in progress ath10k port to freebsd, and I've
tested it on Rome and Peregrine. It seems to be the right thing to do
during suspend to at least cleanly shut things down.
-adrian
On 11 October 2017 at 17:38, Brian Norris <briannorris@chromium.org> wrote:
> Ping? Any comments? I know there's more than one way to slice this
> problem, but it's most definitely a problem...
>
> Brian
>
> On Tue, Sep 19, 2017 at 04:24:16PM -0700, Brian Norris wrote:
>> For devices where the FW supports WoWLAN but user-space has not
>> configured it, we don't do any PCI-specific suspend/resume operations,
>> because mac80211 doesn't call drv_suspend() when !wowlan. This has
>> particularly bad effects for some platforms, because we don't stop the
>> power-save timer, and if this timer goes off after the PCI controller
>> has suspended the link, Bad Things will happen.
>>
>> Commit 32faa3f0ee50 ("ath10k: add the PCI PM core suspend/resume ops")
>> got some of this right, in that it understood there was a problem on
>> non-WoWLAN firmware. But it forgot the $subject case.
>>
>> Fix this by moving all the PCI driver suspend/resume logic exclusively
>> into the driver PM hooks. This shouldn't affect WoWLAN support much
>> (this just gets executed later on).
>>
>> I would just as well kill the entirety of ath10k_hif_suspend(), as it's
>> not even implemented on the USB or SDIO drivers. I expect that we don't
>> need the callback, except to return "supported" (i.e., 0) or "not
>> supported" (i.e., -EOPNOTSUPP).
>>
>> Fixes: 32faa3f0ee50 ("ath10k: add the PCI PM core suspend/resume ops")
>> Fixes: 77258d409ce4 ("ath10k: enable pci soc powersaving")
>> Signed-off-by: Brian Norris <briannorris@chromium.org>
>> Cc: Ryan Hsu <ryanhsu@qti.qualcomm.com>
>> Cc: Kalle Valo <kvalo@qca.qualcomm.com>
>> Cc: Michal Kazior <michal.kazior@tieto.com>
>> ---
>> drivers/net/wireless/ath/ath10k/pci.c | 24 ++++++++++++++----------
>> 1 file changed, 14 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
>> index bc1633945a56..4655c944e3fd 100644
>> --- a/drivers/net/wireless/ath/ath10k/pci.c
>> +++ b/drivers/net/wireless/ath/ath10k/pci.c
>> @@ -2580,6 +2580,12 @@ void ath10k_pci_hif_power_down(struct ath10k *ar)
>> #ifdef CONFIG_PM
>>
>> static int ath10k_pci_hif_suspend(struct ath10k *ar)
>> +{
>> + /* Nothing to do; the important stuff is in the driver suspend. */
>> + return 0;
>> +}
>> +
>> +static int ath10k_pci_suspend(struct ath10k *ar)
>> {
>> /* The grace timer can still be counting down and ar->ps_awake be true.
>> * It is known that the device may be asleep after resuming regardless
>> @@ -2592,6 +2598,12 @@ static int ath10k_pci_hif_suspend(struct ath10k *ar)
>> }
>>
>> static int ath10k_pci_hif_resume(struct ath10k *ar)
>> +{
>> + /* Nothing to do; the important stuff is in the driver resume. */
>> + return 0;
>> +}
>> +
>> +static int ath10k_pci_resume(struct ath10k *ar)
>> {
>> struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
>> struct pci_dev *pdev = ar_pci->pdev;
>> @@ -3403,11 +3415,7 @@ static int ath10k_pci_pm_suspend(struct device *dev)
>> struct ath10k *ar = dev_get_drvdata(dev);
>> int ret;
>>
>> - if (test_bit(ATH10K_FW_FEATURE_WOWLAN_SUPPORT,
>> - ar->running_fw->fw_file.fw_features))
>> - return 0;
>> -
>> - ret = ath10k_hif_suspend(ar);
>> + ret = ath10k_pci_suspend(ar);
>> if (ret)
>> ath10k_warn(ar, "failed to suspend hif: %d\n", ret);
>>
>> @@ -3419,11 +3427,7 @@ static int ath10k_pci_pm_resume(struct device *dev)
>> struct ath10k *ar = dev_get_drvdata(dev);
>> int ret;
>>
>> - if (test_bit(ATH10K_FW_FEATURE_WOWLAN_SUPPORT,
>> - ar->running_fw->fw_file.fw_features))
>> - return 0;
>> -
>> - ret = ath10k_hif_resume(ar);
>> + ret = ath10k_pci_resume(ar);
>> if (ret)
>> ath10k_warn(ar, "failed to resume hif: %d\n", ret);
>>
>> --
>> 2.14.1.690.gbb1197296e-goog
>>
^ permalink raw reply
* Re: [PATCH] ath10k: fix core PCI suspend when WoWLAN is supported but disabled
From: Kalle Valo @ 2017-10-12 3:58 UTC (permalink / raw)
To: Brian Norris
Cc: ath10k@lists.infradead.org, linux-wireless@vger.kernel.org,
Grant Grundler, linux-kernel@vger.kernel.org, Ryan Hsu,
michal.kazior@tieto.com
In-Reply-To: <20171012003805.GA89253@google.com>
Brian Norris <briannorris@chromium.org> writes:
> Ping? Any comments? I know there's more than one way to slice this
> problem, but it's most definitely a problem...
I'm lagging behind with patches but I'll try to catch up this week.
Sorry.
--=20
Kalle Valo=
^ 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