* [PATCH v3 3/3] mwifiex: check hw_status in suspend and resume handlers
From: Amitkumar Karwar @ 2016-10-06 17:30 UTC (permalink / raw)
To: linux-wireless
Cc: Cathy Luo, Nishant Sarmukadam, rajatja, briannorris,
Amitkumar Karwar
In-Reply-To: <1475775028-20306-1-git-send-email-akarwar@marvell.com>
We have observed a kernel crash when system immediately suspends
after booting. There is a race between suspend and driver initialization
paths. This patch adds hw_status checks in suspend/resume to fix this issue
and other corner cases.
Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
---
v2: Return failure in suspend/resume handler in this scenario.
v3: Handle "hw_status" not READY cases carefully. Return 0 if
init or shutdown is in progress. Return -EBUSY if firmware download
failed or command timeout ocurred(non-recoverable). (Brian Norris)
---
drivers/net/wireless/marvell/mwifiex/pcie.c | 27 +++++++++++++++++++++++----
1 file changed, 23 insertions(+), 4 deletions(-)
diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c b/drivers/net/wireless/marvell/mwifiex/pcie.c
index 1e27dbf..cc18e4d 100644
--- a/drivers/net/wireless/marvell/mwifiex/pcie.c
+++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
@@ -121,13 +121,22 @@ static int mwifiex_pcie_suspend(struct device *dev)
struct pci_dev *pdev = to_pci_dev(dev);
card = pci_get_drvdata(pdev);
- if (!card || !card->adapter) {
- pr_err("Card or adapter structure is not valid\n");
+ if (!card || !card->adapter ||
+ card->adapter->hw_status == MWIFIEX_HW_STATUS_RESET ||
+ card->adapter->hw_status == MWIFIEX_HW_STATUS_NOT_READY) {
+ pr_err("Card or adapter structure is not valid or hw_status not ready\n");
return 0;
}
adapter = card->adapter;
+ if (adapter->hw_status == MWIFIEX_HW_STATUS_INITIALIZING ||
+ adapter->hw_status == MWIFIEX_HW_STATUS_INIT_DONE ||
+ adapter->hw_status == MWIFIEX_HW_STATUS_CLOSING) {
+ pr_err("We are in the middle of initialzaion or closing\n");
+ return -EBUSY;
+ }
+
/* Enable the Host Sleep */
if (!mwifiex_enable_hs(adapter)) {
mwifiex_dbg(adapter, ERROR,
@@ -160,13 +169,23 @@ static int mwifiex_pcie_resume(struct device *dev)
struct pci_dev *pdev = to_pci_dev(dev);
card = pci_get_drvdata(pdev);
- if (!card || !card->adapter) {
- pr_err("Card or adapter structure is not valid\n");
+
+ if (!card || !card->adapter ||
+ card->adapter->hw_status == MWIFIEX_HW_STATUS_RESET ||
+ card->adapter->hw_status == MWIFIEX_HW_STATUS_NOT_READY) {
+ pr_err("Card or adapter structure is not valid or hw_status not ready\n");
return 0;
}
adapter = card->adapter;
+ if (adapter->hw_status == MWIFIEX_HW_STATUS_INITIALIZING ||
+ adapter->hw_status == MWIFIEX_HW_STATUS_INIT_DONE ||
+ adapter->hw_status == MWIFIEX_HW_STATUS_CLOSING) {
+ pr_err("We are in the middle of initialzaion or closing\n");
+ return -EBUSY;
+ }
+
if (!adapter->is_suspended) {
mwifiex_dbg(adapter, WARN,
"Device already resumed\n");
--
1.9.1
^ permalink raw reply related
* [PATCH v3 2/3] mwifiex: remove redundant pdev check in suspend/resume handlers
From: Amitkumar Karwar @ 2016-10-06 17:30 UTC (permalink / raw)
To: linux-wireless
Cc: Cathy Luo, Nishant Sarmukadam, rajatja, briannorris,
Amitkumar Karwar
In-Reply-To: <1475775028-20306-1-git-send-email-akarwar@marvell.com>
to_pci_dev() would just do struct offset arithmetic on struct
device to get 'pdev' pointer. We never get NULL pdev pointer
Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
---
New patch prepared as per inputs from Brian Norris.
It wasn't part of v1 and v2 series
---
drivers/net/wireless/marvell/mwifiex/pcie.c | 22 ++++++----------------
1 file changed, 6 insertions(+), 16 deletions(-)
diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c b/drivers/net/wireless/marvell/mwifiex/pcie.c
index ba9e068..1e27dbf 100644
--- a/drivers/net/wireless/marvell/mwifiex/pcie.c
+++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
@@ -120,14 +120,9 @@ static int mwifiex_pcie_suspend(struct device *dev)
struct pcie_service_card *card;
struct pci_dev *pdev = to_pci_dev(dev);
- if (pdev) {
- card = pci_get_drvdata(pdev);
- if (!card || !card->adapter) {
- pr_err("Card or adapter structure is not valid\n");
- return 0;
- }
- } else {
- pr_err("PCIE device is not specified\n");
+ card = pci_get_drvdata(pdev);
+ if (!card || !card->adapter) {
+ pr_err("Card or adapter structure is not valid\n");
return 0;
}
@@ -164,14 +159,9 @@ static int mwifiex_pcie_resume(struct device *dev)
struct pcie_service_card *card;
struct pci_dev *pdev = to_pci_dev(dev);
- if (pdev) {
- card = pci_get_drvdata(pdev);
- if (!card || !card->adapter) {
- pr_err("Card or adapter structure is not valid\n");
- return 0;
- }
- } else {
- pr_err("PCIE device is not specified\n");
+ card = pci_get_drvdata(pdev);
+ if (!card || !card->adapter) {
+ pr_err("Card or adapter structure is not valid\n");
return 0;
}
--
1.9.1
^ permalink raw reply related
* [PATCH v3 1/3] mwifiex: reset card->adapter during device unregister
From: Amitkumar Karwar @ 2016-10-06 17:30 UTC (permalink / raw)
To: linux-wireless
Cc: Cathy Luo, Nishant Sarmukadam, rajatja, briannorris, Xinming Hu,
Amitkumar Karwar
From: Xinming Hu <huxm@marvell.com>
card->adapter gets initialized during device registration.
As it's not cleared, we may end up accessing invalid memory
in some corner cases. This patch fixes the problem.
Signed-off-by: Xinming Hu <huxm@marvell.com>
Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
---
v3: Same as v1 and v2
---
drivers/net/wireless/marvell/mwifiex/pcie.c | 1 +
drivers/net/wireless/marvell/mwifiex/sdio.c | 1 +
2 files changed, 2 insertions(+)
diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c b/drivers/net/wireless/marvell/mwifiex/pcie.c
index f1eeb73..ba9e068 100644
--- a/drivers/net/wireless/marvell/mwifiex/pcie.c
+++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
@@ -3042,6 +3042,7 @@ static void mwifiex_unregister_dev(struct mwifiex_adapter *adapter)
pci_disable_msi(pdev);
}
}
+ card->adapter = NULL;
}
/* This function initializes the PCI-E host memory space, WCB rings, etc.
diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c b/drivers/net/wireless/marvell/mwifiex/sdio.c
index 8718950..4cad1c2 100644
--- a/drivers/net/wireless/marvell/mwifiex/sdio.c
+++ b/drivers/net/wireless/marvell/mwifiex/sdio.c
@@ -2066,6 +2066,7 @@ mwifiex_unregister_dev(struct mwifiex_adapter *adapter)
struct sdio_mmc_card *card = adapter->card;
if (adapter->card) {
+ card->adapter = NULL;
sdio_claim_host(card->func);
sdio_disable_func(card->func);
sdio_release_host(card->func);
--
1.9.1
^ permalink raw reply related
* RE: [PATCH v2 2/2] mwifiex: check hw_status in suspend and resume handlers
From: Amitkumar Karwar @ 2016-10-06 17:28 UTC (permalink / raw)
To: Brian Norris
Cc: linux-wireless@vger.kernel.org, Cathy Luo, Nishant Sarmukadam,
rajatja@google.com, Xinming Hu
In-Reply-To: <20161005164138.GB54237@google.com>
Hi Brain,
> From: Brian Norris [mailto:briannorris@chromium.org]
> Sent: Wednesday, October 05, 2016 10:12 PM
> To: Amitkumar Karwar
> Cc: linux-wireless@vger.kernel.org; Cathy Luo; Nishant Sarmukadam;
> rajatja@google.com; Xinming Hu
> Subject: Re: [PATCH v2 2/2] mwifiex: check hw_status in suspend and
> resume handlers
>
> Hi Amit,
>
> On Wed, Oct 05, 2016 at 12:26:36PM +0000, Amitkumar Karwar wrote:
> > > From: Brian Norris [mailto:briannorris@chromium.org]
> > > Sent: Wednesday, October 05, 2016 2:35 AM
> > > To: Amitkumar Karwar
> > > Cc: linux-wireless@vger.kernel.org; Cathy Luo; Nishant Sarmukadam;
> > > rajatja@google.com; briannorris@google.com; Xinming Hu
> > > Subject: Re: [PATCH v2 2/2] mwifiex: check hw_status in suspend and
> > > resume handlers
> > >
> > > On Tue, Oct 04, 2016 at 10:38:25PM +0530, Amitkumar Karwar wrote:
> > > > From: Xinming Hu <huxm@marvell.com>
> > > >
> > > > We have observed a kernel crash when system immediately suspends
> > > > after booting. There is a race between suspend and driver
> > > > initialization paths.
> > > > This patch adds hw_status checks to fix the problem
> > > >
> > > > Signed-off-by: Xinming Hu <huxm@marvell.com>
> > > > Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
> > > > ---
> > > > v2: Return failure in suspend/resume handler in this scenario.
> > > > ---
> > > > drivers/net/wireless/marvell/mwifiex/pcie.c | 10 ++++++----
> > > > 1 file changed, 6 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c
> > > > b/drivers/net/wireless/marvell/mwifiex/pcie.c
> > > > index ba9e068..fa6bf85 100644
> > > > --- a/drivers/net/wireless/marvell/mwifiex/pcie.c
> > > > +++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
> > > > @@ -122,9 +122,10 @@ static int mwifiex_pcie_suspend(struct device
> > > > *dev)
> > > >
> > > > if (pdev) {
> > > > card = pci_get_drvdata(pdev);
> > > > - if (!card || !card->adapter) {
> > > > + if (!card || !card->adapter ||
> > > > + card->adapter->hw_status !=
> MWIFIEX_HW_STATUS_READY) {
> > >
> > > Wait, is there no locking on the 'hw_status' field? That is
> > > inherently an unsafe race all on its own; you're not guaranteed that
> > > this will be read/written atomically. And you also aren't guaranteed
> > > that writes to this happen in the order they appear in the code --
> > > in other words, reading this flag doesn't necessarily guarantee that
> > > initialization is actually complete (even if that's very likely to
> > > be true, given that it's probably just a single-instruction
> > > word-access, and any prior HW polling or interrupts likely have done
> > > some synchronization and can't be reordered).
> > > actually complete
> >
> > Here is the brief info on how "hw_status" flag is updated.
> > 1) It gets changed incrementally during initialization.
> > MWIFIEX_HW_STATUS_INITIALIZING -> MWIFIEX_HW_STATUS_INIT_DONE ->
> > MWIFIEX_HW_STATUS_READY
> >
> > 2) Status will remain READY once driver+firmware is up and running.
> >
> > 3) Below is status during teardown
> > MWIFIEX_HW_STATUS_READY -> MWIFIEX_HW_STATUS_RESET ->
> > MWIFIEX_HW_STATUS_CLOSING -> MWIFIEX_HW_STATUS_NOT_READY
> >
> > As the events occur one after another, we don't expect a race and
> > don't need locking here for the flag. Flag status
> > MWIFIEX_HW_STATUS_READY guarantees that initialization is completed.
>
> It seems like, as with patch 1, you're mostly arguing about the writes
> to this variable. But writes race with reads as well; how do you
> guarantee that you're not seeing incorrect values of 'hw_status' here in
> suspend() -- e.g., either old or new values of it, or even partially-
> written values, if for some reason the compiler decides it can't
> read/write this all in one go?
Got it. I think, we need to define "hw_status" as atomic variable to resolve the issue. I will create separate patch for this.
>
> > In worst case scenario, only first system suspend attempt issued
> > immediately after system boot will be aborted with BUSY error. I
> > think, that should be fine.
>
> (For the record, my concern about -EBUSY is separate from my concern
> about the potential race condition.)
>
> > Let me know if you have any concerns.
>
> Sorry, I probably didn't completely flesh out my thought here.
>
> I think I was concerned about a failed initialization causing the system
> to never enter suspend again. So specifically: what happens if (e.g.)
> the firmware fails to load? AFAICT, the device doesn't actually unbind
> itself from the driver, so instead, you have a device in limbo that will
> always return -EBUSY in suspend(), and your system can never again enter
> suspend. Am I correct? If so, that doesn't sound great.
You are right. I will add an extra check for this so that both the cases would be handled
Case 1:
Firmware has gone bad or has failed to initialize. We will return zero here, so that it won't block subsequent suspend attempts.
Case 2:
Init or teardown is in process. We will return -EBUSY here. Next suspend attempt would be successful.
I will submit v3 with these changes shortly.
Regards,
Amitkumar
^ permalink raw reply
* RE: [PATCH v2 1/2] mwifiex: reset card->adapter during device unregister
From: Amitkumar Karwar @ 2016-10-06 13:03 UTC (permalink / raw)
To: Brian Norris
Cc: linux-wireless@vger.kernel.org, Cathy Luo, Nishant Sarmukadam,
rajatja@google.com, Xinming Hu
In-Reply-To: <20161005163024.GA54237@google.com>
Hi Brian,
> From: linux-wireless-owner@vger.kernel.org [mailto:linux-wireless-
> owner@vger.kernel.org] On Behalf Of Brian Norris
> Sent: Wednesday, October 05, 2016 10:00 PM
> To: Amitkumar Karwar
> Cc: linux-wireless@vger.kernel.org; Cathy Luo; Nishant Sarmukadam;
> rajatja@google.com; Xinming Hu
> Subject: Re: [PATCH v2 1/2] mwifiex: reset card->adapter during device
> unregister
>
> Hi,
>
> On Wed, Oct 05, 2016 at 02:04:53PM +0000, Amitkumar Karwar wrote:
> > > From: Brian Norris [mailto:briannorris@chromium.org]
> > > Sent: Wednesday, October 05, 2016 3:28 AM
> > > To: Amitkumar Karwar
> > > Cc: linux-wireless@vger.kernel.org; Cathy Luo; Nishant Sarmukadam;
> > > rajatja@google.com; briannorris@google.com; Xinming Hu
> > > Subject: Re: [PATCH v2 1/2] mwifiex: reset card->adapter during
> > > device unregister
> > >
> > > Hi,
> > >
> > > On Tue, Oct 04, 2016 at 10:38:24PM +0530, Amitkumar Karwar wrote:
>
> > > > --- a/drivers/net/wireless/marvell/mwifiex/pcie.c
> > > > +++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
> > > > @@ -3042,6 +3042,7 @@ static void mwifiex_unregister_dev(struct
> > > mwifiex_adapter *adapter)
> > > > pci_disable_msi(pdev);
> > > > }
> > > > }
> > > > + card->adapter = NULL;
> > >
> > > I think you have a similar problem here as in patch 2; there is no
> > > locking to protect fields in struct pcie_service_card or struct
> > > sdio_mmc_card below. That problem kind of already exists, except
> > > that you only write the value of card->adapter once at registration
> > > time, so it's not actually unsafe. But now that you're introducing a
> > > second write, you have a problem.
> > >
> > > Brian
> > >
> >
> > We have a global "add_remove_card_sem" semaphore in our code for
> > synchronizing initialization and teardown threads. Ideally "init +
> > teardown/reboot" should not have a race issue with this logic
> >
> > Later there was a loophole introduced in this after using async
> > firmware download API. During initialization, firmware downloads
> > asynchronously in a separate thread where might have released the
> > semaphore. I am working on a patch to fix this.
> >
> > So "card->adapter" doesn't need to have locking. Even if we have two
> > write operations, those two threads can't run simultaneously due to
> > above mentioned logic.
>
> What about writes racing with reads? You have lots of unsynchronized
> cases that read this, although most of them should be halted by now
> (e.g., cmd processing). I was looking at suspend() in particular, which
> I thought you were looking at in this patch series.
Please note that "card->adapter" is used only in pcie.c/sdio.c/usb.c files
Writes won't have race with reads.
1) write 1 --- "card->adapter = adapter;" in mwifiex_register_dev()
This place is at the beginning of initialization.
mwifiex_pcie_probe() -> mwifiex_add_card() -> adapter->if_ops.register_dev()
There is no chance that "card->adapter" is read anywhere at this point. FW is not yet downloaded
2) write 2 ---- "card->adapter = NULL;" in mwifiex_unregister_dev()
This place the end of teardown phase.
Interrupts are disabled and all cleanup is done. We have "card->adapter" NULL checks at entry point of suspend/remove/resume, if they get called after this.
Regards,
Amitkumar
^ permalink raw reply
* Re: [PATCH 3/3] mac80211: multicast to unicast conversion
From: Johannes Berg @ 2016-10-06 11:55 UTC (permalink / raw)
To: michael-dev; +Cc: linux-wireless, projekt-wlan, netdev
In-Reply-To: <199ae52e4e3000456d8d65b81500d874@fami-braun.de>
On Thu, 2016-10-06 at 13:53 +0200, michael-dev wrote:
> Am 05.10.2016 13:58, schrieb Johannes Berg:
> >
> >
> > Anyway, perhaps this needs to change to take DMS/per-station into
> > account?
> >
> > Then again, this kind of setting - global multicast-to-unicast -
> > fundamentally *cannot* be done on a per-station basis, since if you
> > enable it for one station and not for another, the first station
> > that has it enabled would get the packets twice...
>
> as I see it, that is exactly how DMS is standarized.
>
> IEEE 802.11-2012 section 10.23.15 DMS procedures:
>
> "If the requested DMS is accepted by the AP, the AP shall send
> subsequent group addressed MSDUs that
> match the frame classifier specified in the DMS Descriptors to the
> requesting STA as A-MSDU subframes
> within an individually addressed A-MSDU frame (see 8.3.2.2 and
> 9.11)."
>
> -> so the multicast packets shall go out as unicast A-MSDU frames
> to stations that requested this
Correct.
> "The AP shall continue to transmit the matching frames as group
> addressed frames (see 9.3.6, and 10.2.1.16) if at least one
> associated
> STA has not requested DMS for these frames."
>
> -> so it will continue to send it as multicast frames as well.
>
> As with DMS the station requested DMS for a specific multicast
> address, it could then drop multicast frames addressed to the
> multicast address it registered for DMS.
Yes, the DMS spec tells it to do this. However, we can't implement non-
DMS similarly, because then the station won't request it and won't drop
the duplicates.
So for this non-standard multicast-to-unicast, it's all or nothing, it
can't be done for some stations only.
johannes
^ permalink raw reply
* Re: [PATCH 3/3] mac80211: multicast to unicast conversion
From: michael-dev @ 2016-10-06 11:53 UTC (permalink / raw)
To: Johannes Berg; +Cc: linux-wireless, projekt-wlan, netdev
In-Reply-To: <1475668688.4994.46.camel@sipsolutions.net>
Am 05.10.2016 13:58, schrieb Johannes Berg:
>
> Anyway, perhaps this needs to change to take DMS/per-station into
> account?
>
> Then again, this kind of setting - global multicast-to-unicast -
> fundamentally *cannot* be done on a per-station basis, since if you
> enable it for one station and not for another, the first station that
> has it enabled would get the packets twice...
as I see it, that is exactly how DMS is standarized.
IEEE 802.11-2012 section 10.23.15 DMS procedures:
"If the requested DMS is accepted by the AP, the AP shall send
subsequent group addressed MSDUs that
match the frame classifier specified in the DMS Descriptors to the
requesting STA as A-MSDU subframes
within an individually addressed A-MSDU frame (see 8.3.2.2 and 9.11)."
-> so the multicast packets shall go out as unicast A-MSDU frames to
stations that requested this
"The AP shall continue to transmit the matching frames as group
addressed frames (see 9.3.6, and 10.2.1.16) if at least one associated
STA has not requested DMS for these frames."
-> so it will continue to send it as multicast frames as well.
As with DMS the station requested DMS for a specific multicast address,
it could then drop multicast frames addressed to the multicast address
it registered for DMS.
Regards,
M. Braun
^ permalink raw reply
* Re: [PATCHv4] mac80211: check A-MSDU inner frame source address on AP interfaces
From: Johannes Berg @ 2016-10-06 11:36 UTC (permalink / raw)
To: michael-dev
Cc: linux-wireless, projekt-wlan, kvalo, akarwar, nishants,
Larry.Finger, Jes.Sorensen
In-Reply-To: <bb2ea10f30d2ae8a3cc048974ed148c8@fami-braun.de>
> The rules when to check sa/da should be independent of the driver
> and thus would likely be duplicated by each caller. This is why I
> had it in ieee80211_amsdu_to_8023s.
That does make sense, I guess. But I feel that it's overly complicated,
and most drivers don't actually support all those features, in
particular, 4-addr station/AP is currently exclusive to mac80211.
> My patch defaulted into not checking sa/da for interface type not
> explicitly given, whereas your code defaults to checking both for
> those.
> This makes a difference for IFTYPE_MONITOR, IFTYPE_P2P_*,
Monitor will never get there, and P2P_CLIENT == STATION, P2P_GO == AP
as far as mac80211's vif.type is concerned, so those are handled.
> IFTYPE_OCB,
That can't do A-MSDU I believe.
> IFTYPE_UNSPECIFIED,
That is invalid and can never happen here.
> IFTYPE_WDS.
This is ... it can't even negotiate HT, so I doubt A-MSDU could be used
in any way. This is old crap, and I'd actually rather remove it, the 4-
addr station/AP superseded it.
> I don't know if filtering is appropiate
> for those or if they can actually occur there.
I think it's all covered then.
Could you test my patches in your scenario to see they do what we want?
I'll resend as [PATCH] then, and think about applying them and perhaps
backporting also.
johannes
^ permalink raw reply
* Re: [PATCHv4] mac80211: check A-MSDU inner frame source address on AP interfaces
From: michael-dev @ 2016-10-06 11:30 UTC (permalink / raw)
To: Johannes Berg
Cc: linux-wireless, projekt-wlan, kvalo, akarwar, nishants,
Larry.Finger, Jes.Sorensen
In-Reply-To: <1475679558.5257.7.camel@sipsolutions.net>
Am 05.10.2016 16:59, schrieb Johannes Berg:
> So as you can see by my own version of this patch, I'm not super happy
> with the way you did things here :)
I'm happy with your version, so lets just drop mine.
> Instead of adding 4 new arguments, (ta, ra, is_4addr, is_tdls_data), I
> opted to just add two (check_da and check_sa) and make those NULL when
> no checks are desired.
>
> I *think* that works equivalently, but it'd be great if you could take
> a look.
The rules when to check sa/da should be independent of the driver and
thus would likely be duplicated by each caller.
This is why I had it in ieee80211_amsdu_to_8023s.
My patch defaulted into not checking sa/da for interface type not
explicitly given, whereas your code defaults to checking both for those.
This makes a difference for IFTYPE_MONITOR, IFTYPE_P2P_*, IFTYPE_OCB,
IFTYPE_UNSPECIFIED, IFTYPE_WDS. I don't know if filtering is appropiate
for those or if they can actually occur there.
> This also conflicts with the earlier patch I sent to just always drop
> when it's multicast.
Dropping multicast A-MSDU frames is fine for me.
michael
^ permalink raw reply
* Re: [PATCH 2/2] Revert "staging: wilc1000: Replace kthread with workqueue for host interface"
From: Greg KH @ 2016-10-06 10:17 UTC (permalink / raw)
To: Aditya Shankar
Cc: ganesh.krishna, linux-wireless, devel, linux-kernel,
Nicolas.Ferre
In-Reply-To: <20161006152659.15a4c38b1dc8d4f781c20e0a@microchip.com>
On Thu, Oct 06, 2016 at 03:26:59PM +0530, Aditya Shankar wrote:
> On Fri, 30 Sep 2016 15:22:15 +0200
> Greg KH <gregkh@linuxfoundation.org> wrote:
>
> > On Fri, Sep 30, 2016 at 03:43:18PM +0530, Aditya Shankar wrote:
> > > This reverts commit 2518ac59eb27 ("staging: wilc1000: Replace kthread
> > > with workqueue for host interface")
> > >
> > > This commit breaks wilc1000 driver init. A crash was seen
> > > everytime the wlan interface was brought up and wilc device
> > > open was attempted. This change is being reverted until we
> > > figure out the problem in this change. The driver is
> > > usable now with this change reverted.
> > >
> > > Signed-off-by: Aditya Shankar <Aditya.Shankar@microchip.com>
> > >
> > > Conflicts:
> > > drivers/staging/wilc1000/host_interface.c
> >
> > What is this line doing here?
> >
> > And shouldn't we add a cc: stable tag as well? Or at the least, put a
> > "fixes:" tag to let people know exactly what commit it is fixing (the
> > id that it is reverting.)
> >
> > thanks,
> >
> > greg k-h
>
> Apologies for this bad commit message.
>
> I have an update on this wilc1000 crash issue. I've figured out
> the cause for the crash and fixed it. Therefore,
> I request you to ignore the patch I sent out to
> revert the change causing the regression. The cause was a misplaced
> call to destroy workqueue soon after creating it.
> With this removed, the issue is not seen.
>
> I will send out a separate patch to fix the issue.
Wonderful, thanks for doing that, I'll drop these and use your fix when
you send it.
greg k-h
^ permalink raw reply
* Re: [PATCH 2/2] Revert "staging: wilc1000: Replace kthread with workqueue for host interface"
From: Aditya Shankar @ 2016-10-06 9:56 UTC (permalink / raw)
To: Greg KH
Cc: Aditya Shankar, ganesh.krishna, linux-wireless, devel,
linux-kernel, Nicolas.Ferre
In-Reply-To: <20160930132215.GB3016@kroah.com>
On Fri, 30 Sep 2016 15:22:15 +0200
Greg KH <gregkh@linuxfoundation.org> wrote:
> On Fri, Sep 30, 2016 at 03:43:18PM +0530, Aditya Shankar wrote:
> > This reverts commit 2518ac59eb27 ("staging: wilc1000: Replace kthread
> > with workqueue for host interface")
> >
> > This commit breaks wilc1000 driver init. A crash was seen
> > everytime the wlan interface was brought up and wilc device
> > open was attempted. This change is being reverted until we
> > figure out the problem in this change. The driver is
> > usable now with this change reverted.
> >
> > Signed-off-by: Aditya Shankar <Aditya.Shankar@microchip.com>
> >
> > Conflicts:
> > drivers/staging/wilc1000/host_interface.c
>
> What is this line doing here?
>
> And shouldn't we add a cc: stable tag as well? Or at the least, put a
> "fixes:" tag to let people know exactly what commit it is fixing (the
> id that it is reverting.)
>
> thanks,
>
> greg k-h
Apologies for this bad commit message.
I have an update on this wilc1000 crash issue. I've figured out
the cause for the crash and fixed it. Therefore,
I request you to ignore the patch I sent out to
revert the change causing the regression. The cause was a misplaced
call to destroy workqueue soon after creating it.
With this removed, the issue is not seen.
I will send out a separate patch to fix the issue.
Thanks,
Aditya
^ permalink raw reply
* Re: [PATCH] [wl18xx] Fix memory leakage if kzalloc fails
From: Julian Calaby @ 2016-10-06 9:35 UTC (permalink / raw)
To: Souptick Joarder; +Cc: linux-wireless, sahu.rameshwar73
In-Reply-To: <20161005115013.GA22099@symbol-HP-ZBook-15>
Hi,
On Wed, Oct 5, 2016 at 10:50 PM, Souptick Joarder <jrdr.linux@gmail.com> wrote:
> This patch is added to properly handle memory leak if kzalloc fails
> in wl18xx_scan_send() and wl18xx_scan_sched_scan_config()
What memory leak?
> Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>
> Signed-off-by: Rameshwar Sahu <sahu.rameshwar73@gmail.com>
Why two signed-off-bys?
> ---
> drivers/net/wireless/ti/wl18xx/scan.c | 20 ++++++++++----------
> 1 file changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/net/wireless/ti/wl18xx/scan.c b/drivers/net/wireless/ti/wl18xx/scan.c
> index 4e522154..aed22e1 100644
> --- a/drivers/net/wireless/ti/wl18xx/scan.c
> +++ b/drivers/net/wireless/ti/wl18xx/scan.c
> @@ -41,14 +41,13 @@ static void wl18xx_adjust_channels(struct wl18xx_cmd_scan_params *cmd,
> static int wl18xx_scan_send(struct wl1271 *wl, struct wl12xx_vif *wlvif,
> struct cfg80211_scan_request *req)
> {
> - struct wl18xx_cmd_scan_params *cmd;
> + struct wl18xx_cmd_scan_params *cmd = NULL;
> struct wlcore_scan_channels *cmd_channels = NULL;
> int ret;
>
> cmd = kzalloc(sizeof(*cmd), GFP_KERNEL);
> if (!cmd) {
> - ret = -ENOMEM;
> - goto out;
> + return -ENOMEM;
> }
>
> /* scan on the dev role if the regular one is not started */
> @@ -59,7 +58,7 @@ static int wl18xx_scan_send(struct wl1271 *wl, struct wl12xx_vif *wlvif,
>
> if (WARN_ON(cmd->role_id == WL12XX_INVALID_ROLE_ID)) {
> ret = -EINVAL;
> - goto out;
> + goto err_cmd_free;
> }
>
> cmd->scan_type = SCAN_TYPE_SEARCH;
> @@ -84,7 +83,7 @@ static int wl18xx_scan_send(struct wl1271 *wl, struct wl12xx_vif *wlvif,
> cmd_channels = kzalloc(sizeof(*cmd_channels), GFP_KERNEL);
> if (!cmd_channels) {
> ret = -ENOMEM;
> - goto out;
> + goto err_cmd_free;
> }
>
> wlcore_set_scan_chan_params(wl, cmd_channels, req->channels,
> @@ -153,6 +152,7 @@ static int wl18xx_scan_send(struct wl1271 *wl, struct wl12xx_vif *wlvif,
>
> out:
> kfree(cmd_channels);
> +err_cmd_free:
kfree(NULL) is valid, so therefore the out: and err_cmd_free: labels
are equivalent from a memory freeing perspective, so where exactly are
we leaking memory in this function?
> kfree(cmd);
> return ret;
> }
> @@ -171,7 +171,7 @@ int wl18xx_scan_sched_scan_config(struct wl1271 *wl,
> struct cfg80211_sched_scan_request *req,
> struct ieee80211_scan_ies *ies)
> {
> - struct wl18xx_cmd_scan_params *cmd;
> + struct wl18xx_cmd_scan_params *cmd = NULL;
> struct wlcore_scan_channels *cmd_channels = NULL;
> struct conf_sched_scan_settings *c = &wl->conf.sched_scan;
> int ret;
> @@ -185,15 +185,14 @@ int wl18xx_scan_sched_scan_config(struct wl1271 *wl,
>
> cmd = kzalloc(sizeof(*cmd), GFP_KERNEL);
> if (!cmd) {
> - ret = -ENOMEM;
> - goto out;
> + return -ENOMEM;
> }
>
> cmd->role_id = wlvif->role_id;
>
> if (WARN_ON(cmd->role_id == WL12XX_INVALID_ROLE_ID)) {
> ret = -EINVAL;
> - goto out;
> + goto err_cmd_free;
> }
>
> cmd->scan_type = SCAN_TYPE_PERIODIC;
> @@ -218,7 +217,7 @@ int wl18xx_scan_sched_scan_config(struct wl1271 *wl,
> cmd_channels = kzalloc(sizeof(*cmd_channels), GFP_KERNEL);
> if (!cmd_channels) {
> ret = -ENOMEM;
> - goto out;
> + goto err_cmd_free;
> }
>
> /* configure channels */
> @@ -296,6 +295,7 @@ int wl18xx_scan_sched_scan_config(struct wl1271 *wl,
>
> out:
> kfree(cmd_channels);
> +err_cmd_free:
Same question here.
> kfree(cmd);
> return ret;
> }
> --
> 1.9.1
>
--
Julian Calaby
Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/
^ permalink raw reply
* Re: nl80211 fine timing measurement support
From: Lior David @ 2016-10-06 8:41 UTC (permalink / raw)
To: Johannes Berg, Arend van Spriel, Luca Coelho
Cc: Maya Erez, Jouni Malinen, linux-wireless
In-Reply-To: <1475573142.5324.41.camel@sipsolutions.net>
On 10/4/2016 12:25 PM, Johannes Berg wrote:
>
>> If raw results are mainly used for analysis algorithms how about
>> providing raw measurement data through debugfs. May even consider
>> adding rtt calculation in cfg80211/mac80211 for drivers that choose
>> to provide raw measurement data and still only report final RTT in
>> nl80211 api.
>>
>
> I think the "analysis algorithms" in this case are what actually gives
> you the distance value.
>
> However, I don't think we should accept that everybody wants to run
> their proprietary algorithms on top and expose only the values needed
> for those. That makes the drivers only usable with additional
> proprietary software, which may even be incompatible with the GPL.
>
> If the algorithms are in the device, then we can expose the final
> results, and that's most useful for applications in the API.
>
> If they're not, then we need to expose something that can be used
> without additional proprietary and device-specific algorithms.
>
> If those algorithms cannot be run in the device, then we should put
> them into the driver instead.
>
After further internal discussion, we will be ok with reporting only the final
RTT. In our current internal implementation, the location framework in user
space gets the raw results (t1,t2,t3,t4) and does an algorithm which includes
drift compensation in order to derive the final RTT. We will need to move this
down to the driver, not trivial but can be done.
However I think there might be platforms where you might need a more complicated
algorithm which will need to run in user space so for the long term we may want
to consider an option to report the raw results. The raw results are definitely
important for debugging but Using debugfs is problematic because it is difficult
to synchronize with the measurement session, especially if you have multiple
bursts. It is probably best to report is as part of the session, together/in
place of the RTT for each burst.
Thanks,
Lior
> johannes
>
^ permalink raw reply
* [PATCH v2 2/2] ath10k: Remove extraneous error message in tx alloc
From: Mohammed Shafi Shajakhan @ 2016-10-06 8:39 UTC (permalink / raw)
To: ath10k; +Cc: mohammed, linux-wireless, Mohammed Shafi Shajakhan
In-Reply-To: <1475743188-12007-1-git-send-email-mohammed@qca.qualcomm.com>
From: Mohammed Shafi Shajakhan <mohammed@qti.qualcomm.com>
Remove extraneous error message in 'ath10k_htt_tx_alloc_cont_frag_desc'
as the caller 'ath10k_htt_tx_alloc' already dumps a proper error
message
Signed-off-by: Mohammed Shafi Shajakhan <mohammed@qti.qualcomm.com>
---
[v2 rebased over top of tree]
drivers/net/wireless/ath/ath10k/htt_tx.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/net/wireless/ath/ath10k/htt_tx.c b/drivers/net/wireless/ath/ath10k/htt_tx.c
index 786fbd7..4255c1a 100644
--- a/drivers/net/wireless/ath/ath10k/htt_tx.c
+++ b/drivers/net/wireless/ath/ath10k/htt_tx.c
@@ -283,10 +283,8 @@ static int ath10k_htt_tx_alloc_cont_frag_desc(struct ath10k_htt *htt)
htt->frag_desc.vaddr = dma_alloc_coherent(ar->dev, size,
&htt->frag_desc.paddr,
GFP_KERNEL);
- if (!htt->frag_desc.vaddr) {
- ath10k_err(ar, "failed to alloc fragment desc memory\n");
+ if (!htt->frag_desc.vaddr)
return -ENOMEM;
- }
return 0;
}
--
1.9.1
^ permalink raw reply related
* [PATCH v2 1/2] ath10k: clean up HTT tx buffer allocation and free
From: Mohammed Shafi Shajakhan @ 2016-10-06 8:39 UTC (permalink / raw)
To: ath10k; +Cc: mohammed, linux-wireless, Mohammed Shafi Shajakhan
From: Mohammed Shafi Shajakhan <mohammed@qti.qualcomm.com>
cleanup 'ath10k_htt_tx_alloc' by introducing the API's
'ath10k_htt_tx_alloc/free_{cont_txbuf, txdone_fifo} and
re-use them whereever needed
Signed-off-by: Mohammed Shafi Shajakhan <mohammed@qti.qualcomm.com>
---
[v2 rebased over top of tree]
drivers/net/wireless/ath/ath10k/htt_tx.c | 76 ++++++++++++++++++++++----------
1 file changed, 52 insertions(+), 24 deletions(-)
diff --git a/drivers/net/wireless/ath/ath10k/htt_tx.c b/drivers/net/wireless/ath/ath10k/htt_tx.c
index ae5b33f..786fbd7 100644
--- a/drivers/net/wireless/ath/ath10k/htt_tx.c
+++ b/drivers/net/wireless/ath/ath10k/htt_tx.c
@@ -229,6 +229,33 @@ void ath10k_htt_tx_free_msdu_id(struct ath10k_htt *htt, u16 msdu_id)
idr_remove(&htt->pending_tx, msdu_id);
}
+static void ath10k_htt_tx_free_cont_txbuf(struct ath10k_htt *htt)
+{
+ size_t size;
+
+ if (!htt->txbuf.vaddr)
+ return;
+
+ size = htt->max_num_pending_tx * sizeof(struct ath10k_htt_txbuf);
+ dma_free_coherent(htt->ar->dev, size, htt->txbuf.vaddr,
+ htt->txbuf.paddr);
+}
+
+static int ath10k_htt_tx_alloc_cont_txbuf(struct ath10k_htt *htt)
+{
+ struct ath10k *ar = htt->ar;
+ size_t size;
+
+ size = htt->max_num_pending_tx * sizeof(struct ath10k_htt_txbuf);
+ htt->txbuf.vaddr = dma_alloc_coherent(ar->dev, size,
+ &htt->txbuf.paddr,
+ GFP_KERNEL);
+ if (!htt->txbuf.vaddr)
+ return -ENOMEM;
+
+ return 0;
+}
+
static void ath10k_htt_tx_free_cont_frag_desc(struct ath10k_htt *htt)
{
size_t size;
@@ -310,10 +337,26 @@ static int ath10k_htt_tx_alloc_txq(struct ath10k_htt *htt)
return 0;
}
+static void ath10k_htt_tx_free_txdone_fifo(struct ath10k_htt *htt)
+{
+ WARN_ON(!kfifo_is_empty(&htt->txdone_fifo));
+ kfifo_free(&htt->txdone_fifo);
+}
+
+static int ath10k_htt_tx_alloc_txdone_fifo(struct ath10k_htt *htt)
+{
+ int ret;
+ size_t size;
+
+ size = roundup_pow_of_two(htt->max_num_pending_tx);
+ ret = kfifo_alloc(&htt->txdone_fifo, size, GFP_KERNEL);
+ return ret;
+}
+
int ath10k_htt_tx_alloc(struct ath10k_htt *htt)
{
struct ath10k *ar = htt->ar;
- int ret, size;
+ int ret;
ath10k_dbg(ar, ATH10K_DBG_BOOT, "htt tx max num pending tx %d\n",
htt->max_num_pending_tx);
@@ -321,13 +364,9 @@ int ath10k_htt_tx_alloc(struct ath10k_htt *htt)
spin_lock_init(&htt->tx_lock);
idr_init(&htt->pending_tx);
- size = htt->max_num_pending_tx * sizeof(struct ath10k_htt_txbuf);
- htt->txbuf.vaddr = dma_alloc_coherent(ar->dev, size,
- &htt->txbuf.paddr,
- GFP_KERNEL);
- if (!htt->txbuf.vaddr) {
- ath10k_err(ar, "failed to alloc tx buffer\n");
- ret = -ENOMEM;
+ ret = ath10k_htt_tx_alloc_cont_txbuf(htt);
+ if (ret) {
+ ath10k_err(ar, "failed to alloc cont tx buffer: %d\n", ret);
goto free_idr_pending_tx;
}
@@ -343,8 +382,7 @@ int ath10k_htt_tx_alloc(struct ath10k_htt *htt)
goto free_frag_desc;
}
- size = roundup_pow_of_two(htt->max_num_pending_tx);
- ret = kfifo_alloc(&htt->txdone_fifo, size, GFP_KERNEL);
+ ret = ath10k_htt_tx_alloc_txdone_fifo(htt);
if (ret) {
ath10k_err(ar, "failed to alloc txdone fifo: %d\n", ret);
goto free_txq;
@@ -359,10 +397,7 @@ free_frag_desc:
ath10k_htt_tx_free_cont_frag_desc(htt);
free_txbuf:
- size = htt->max_num_pending_tx *
- sizeof(struct ath10k_htt_txbuf);
- dma_free_coherent(htt->ar->dev, size, htt->txbuf.vaddr,
- htt->txbuf.paddr);
+ ath10k_htt_tx_free_cont_txbuf(htt);
free_idr_pending_tx:
idr_destroy(&htt->pending_tx);
@@ -388,22 +423,15 @@ static int ath10k_htt_tx_clean_up_pending(int msdu_id, void *skb, void *ctx)
void ath10k_htt_tx_free(struct ath10k_htt *htt)
{
- int size;
+ tasklet_kill(&htt->txrx_compl_task);
idr_for_each(&htt->pending_tx, ath10k_htt_tx_clean_up_pending, htt->ar);
idr_destroy(&htt->pending_tx);
- if (htt->txbuf.vaddr) {
- size = htt->max_num_pending_tx *
- sizeof(struct ath10k_htt_txbuf);
- dma_free_coherent(htt->ar->dev, size, htt->txbuf.vaddr,
- htt->txbuf.paddr);
- }
-
+ ath10k_htt_tx_free_cont_txbuf(htt);
ath10k_htt_tx_free_txq(htt);
ath10k_htt_tx_free_cont_frag_desc(htt);
- WARN_ON(!kfifo_is_empty(&htt->txdone_fifo));
- kfifo_free(&htt->txdone_fifo);
+ ath10k_htt_tx_free_txdone_fifo(htt);
}
void ath10k_htt_htc_tx_complete(struct ath10k *ar, struct sk_buff *skb)
--
1.9.1
^ permalink raw reply related
* Re: bcmdhd: Strange Power Save messages
From: Arend Van Spriel @ 2016-10-06 8:25 UTC (permalink / raw)
To: Gucea Doru
Cc: Krishna Chaitanya, Arend van Spriel, Andra Paraschiv,
linux-wireless
In-Reply-To: <CANfLQrY1NahfQF0xheocVFdJHgxhSE5YbFUwcm3t6e0Fur-YGg@mail.gmail.com>
On 6-10-2016 10:07, Gucea Doru wrote:
> On Wed, Oct 5, 2016 at 11:12 AM, Arend Van Spriel
> <arend.vanspriel@broadcom.com> wrote:
>> On 4-10-2016 13:39, Gucea Doru wrote:
>>> On Sat, Oct 1, 2016 at 2:52 PM, Arend van Spriel
>>>> <arend.vanspriel@broadcom.com> wrote:
>>>>>
>>>>>
>>>>> On 29-09-16 13:32, Gucea Doru wrote:
>>>>>> On Tue, Sep 27, 2016 at 12:03 PM, Gucea Doru <gucea.doru@gmail.com> wrote:
>>>>>>> What is the decision triggering the exit from the PS mode immediately
>>>>>>> after the ping request? I am asking this because 802.11 PS legacy
>>>>>>> specifies that the client should wait for a beacon with TIM set in
>>>>>>> order to wake up: in my case, there is no beacon between the ping
>>>>>>> request message and the Null frame that announces the exit from the PS
>>>>>>> mode.
>>>>>>
>>>>>>
>>>>>> Any help would be highly appreciated :)
>>>>>
>>>>> Actually though I already sent you are reply, but alas here it is.
>>>>>
>>>>> bcmdhd is our aosp driver. I am maintaining the upstream brcm80211
>>>>> drivers. Regardless your question is more for firmware running on the
>>>>> device. So like the same behavior would be observed when using brcmfmac
>>>>> with same firmware.
>>>>>
>>>>>> IEEE Std 802.11-2012, section 10.2.1.8 specifies that "when the STA
>>>>>> detects that the bit corresponding to its AID is 1 i the TIM, the STA
>>>>>> shall issue a PS Poll". In my capture there are cases when the STA
>>>>>> exits the PS mode without waiting for a beacon.
>>>>>
>>>>> It is a bit tricky, but the standard does not explicitly say the STA
>>>>> should be in power-save at any other time. So it is difficult to say
>>>>> what event occurred on the STA side to exit PS mode. Also STA means
>>>>> P2P-Client as you say. That means that you have multiple interfaces:
>>>>> regular STA and P2P-Client. So is the STA connected to some other AP or
>>>>> just not connected. wpa_supplicant will do intermittent scan or initiate
>>>>> scheduled scan by which firmware will scan at a certain interval. That
>>>>> is just some things I can come up with and I am sure there are more.
>>>
>>> I agree that there may be some events belonging to the regular STA
>>> interface that could trigger the Null Frame (which includes the exit
>>> from PS Mode). However, I would expect to see some management frames
>>> in the air before/after the Null Packet (e.g.: a Probe request in case
>>> of a scheduled scan). But in my case the trigger for the Null frame
>>> seems to be the ping request packet, the scenario is the same every
>>> time: ping request -> Block ACK -> Null Frame (Wireshark trace
>>> confirms this behavior).
>>>
>>> I thought that you had a power save optimization algorithm that keeps
>>> the card on a few milliseconds just to see if we can have a fast reply
>>> from the peer. Does this ring a bell? :)
>>
>> It does not. That would be implemented in firmware. As said I am working
>> on brcmfmac/brcmsmac. So bcmdhd and firmware are not my expertise.
>>
>
> Arend, could you please redirect me to a Broadcom firmware maintainer?
Can you please elaborate on your platform, broadcom chipset, and what
version of bcmdhd you are using.
Regards,
Arend
^ permalink raw reply
* Re: bcmdhd: Strange Power Save messages
From: Gucea Doru @ 2016-10-06 8:07 UTC (permalink / raw)
To: Arend Van Spriel
Cc: Krishna Chaitanya, Arend van Spriel, Andra Paraschiv,
linux-wireless
In-Reply-To: <db6d5803-3fec-64d9-5290-32216d1bd031@broadcom.com>
On Wed, Oct 5, 2016 at 11:12 AM, Arend Van Spriel
<arend.vanspriel@broadcom.com> wrote:
> On 4-10-2016 13:39, Gucea Doru wrote:
>> On Sat, Oct 1, 2016 at 2:52 PM, Arend van Spriel
>>> <arend.vanspriel@broadcom.com> wrote:
>>>>
>>>>
>>>> On 29-09-16 13:32, Gucea Doru wrote:
>>>>> On Tue, Sep 27, 2016 at 12:03 PM, Gucea Doru <gucea.doru@gmail.com> wrote:
>>>>>> What is the decision triggering the exit from the PS mode immediately
>>>>>> after the ping request? I am asking this because 802.11 PS legacy
>>>>>> specifies that the client should wait for a beacon with TIM set in
>>>>>> order to wake up: in my case, there is no beacon between the ping
>>>>>> request message and the Null frame that announces the exit from the PS
>>>>>> mode.
>>>>>
>>>>>
>>>>> Any help would be highly appreciated :)
>>>>
>>>> Actually though I already sent you are reply, but alas here it is.
>>>>
>>>> bcmdhd is our aosp driver. I am maintaining the upstream brcm80211
>>>> drivers. Regardless your question is more for firmware running on the
>>>> device. So like the same behavior would be observed when using brcmfmac
>>>> with same firmware.
>>>>
>>>>> IEEE Std 802.11-2012, section 10.2.1.8 specifies that "when the STA
>>>>> detects that the bit corresponding to its AID is 1 i the TIM, the STA
>>>>> shall issue a PS Poll". In my capture there are cases when the STA
>>>>> exits the PS mode without waiting for a beacon.
>>>>
>>>> It is a bit tricky, but the standard does not explicitly say the STA
>>>> should be in power-save at any other time. So it is difficult to say
>>>> what event occurred on the STA side to exit PS mode. Also STA means
>>>> P2P-Client as you say. That means that you have multiple interfaces:
>>>> regular STA and P2P-Client. So is the STA connected to some other AP or
>>>> just not connected. wpa_supplicant will do intermittent scan or initiate
>>>> scheduled scan by which firmware will scan at a certain interval. That
>>>> is just some things I can come up with and I am sure there are more.
>>
>> I agree that there may be some events belonging to the regular STA
>> interface that could trigger the Null Frame (which includes the exit
>> from PS Mode). However, I would expect to see some management frames
>> in the air before/after the Null Packet (e.g.: a Probe request in case
>> of a scheduled scan). But in my case the trigger for the Null frame
>> seems to be the ping request packet, the scenario is the same every
>> time: ping request -> Block ACK -> Null Frame (Wireshark trace
>> confirms this behavior).
>>
>> I thought that you had a power save optimization algorithm that keeps
>> the card on a few milliseconds just to see if we can have a fast reply
>> from the peer. Does this ring a bell? :)
>
> It does not. That would be implemented in firmware. As said I am working
> on brcmfmac/brcmsmac. So bcmdhd and firmware are not my expertise.
>
Arend, could you please redirect me to a Broadcom firmware maintainer?
Thank you,
Doru
^ permalink raw reply
* Re: [PATCH] ath10k: cache calibration data when the core is stopped.
From: Valo, Kalle @ 2016-10-06 7:40 UTC (permalink / raw)
To: Marty Faltesek; +Cc: ath10k@lists.infradead.org, linux-wireless@vger.kernel.org
In-Reply-To: <CAOiWkA_fuhzPeF7qPP0hLN3KTZUOi6_te5Zc3+vxKQ9GRj0VBw@mail.gmail.com>
Marty Faltesek <mfaltesek@google.com> writes:
> On Mon, Oct 3, 2016 at 3:46 AM, Valo, Kalle <kvalo@qca.qualcomm.com> wrot=
e:
>> Marty Faltesek <mfaltesek@google.com> writes:
>>
>>> Caching calibration data allows it to be accessed when the
>>> device is not active.
>>>
>>> Signed-off-by: Marty Faltesek <mfaltesek@google.com>
>>
>> No comma in the title, please.
>>
>> What tree did you use as the baseline? This doesn't seem to apply to
>> ath.git:
>
> We use backports 20160122 which has not been updated since earlier this y=
ear.
> I can forward port it to your tree, and make sure
> it builds but won't be able to test it. Will that be OK?
Sure, I can test it.
>> Also please note that this patch (which I'm queuing to 4.9) touches the
>> same area:
>>
>> ath10k: fix debug cal data file
>>
>> https://patchwork.kernel.org/patch/9340953/
>
> I've modified this too, and this won't be necessary, so can you drop
> it? If not, let me know and I'll pull it in and make sure I'm based
> off it too.
Actually I was first planning to push 9340953 to 4.9 and take your patch
to 4.10. But your patch is a cleaner approach to this and maybe I should
push that to 4.9 instead? Need to think a bit more.
--=20
Kalle Valo=
^ permalink raw reply
* Re: [1/2] ath10k: clean up HTT tx buffer allocation and free
From: Mohammed Shafi Shajakhan @ 2016-10-06 7:37 UTC (permalink / raw)
To: Kalle Valo; +Cc: Mohammed Shafi Shajakhan, ath10k, linux-wireless
In-Reply-To: <dc6b3afe60574d7bb60e1913da3f1ad4@euamsexm01a.eu.qualcomm.com>
On Thu, Oct 06, 2016 at 09:31:41AM +0200, Kalle Valo wrote:
> Mohammed Shafi Shajakhan <mohammed@qti.qualcomm.com> wrote:
> > From: Mohammed Shafi Shajakhan <mohammed@qti.qualcomm.com>
> >
> > cleanup 'ath10k_htt_tx_alloc' by introducing the API's
> > 'ath10k_htt_tx_alloc/free_{cont_txbuf, txdone_fifo} and
> > re-use them whereever needed
> >
> > Signed-off-by: Mohammed Shafi Shajakhan <mohammed@qti.qualcomm.com>
>
> Patch 1 doesn't apply and the conflict was not trivial
[shafi] oops will rebase it, not sure how i missed it :-(
>
> error: patch failed: drivers/net/wireless/ath/ath10k/htt_tx.c:388
> error: drivers/net/wireless/ath/ath10k/htt_tx.c: patch does not apply
> stg import: Diff does not apply cleanly
> error: patch failed: drivers/net/wireless/ath/ath10k/htt_tx.c:283
> error: drivers/net/wireless/ath/ath10k/htt_tx.c: patch does not apply
> stg import: Diff does not apply cleanly
>
> 2 patches set to Changes Requested.
>
> 9361741 [1/2] ath10k: clean up HTT tx buffer allocation and free
> 9361743 [2/2] ath10k: Remove extraneous error message in tx alloc
>
> --
> https://patchwork.kernel.org/patch/9361741/
>
> Documentation about submitting wireless patches and checking status
> from patchwork:
>
> https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
>
^ permalink raw reply
* Re: [1/2] ath10k: clean up HTT tx buffer allocation and free
From: Kalle Valo @ 2016-10-06 7:31 UTC (permalink / raw)
To: Mohammed Shafi Shajakhan
Cc: ath10k, mohammed, linux-wireless, Mohammed Shafi Shajakhan
In-Reply-To: <1475584772-4091-2-git-send-email-mohammed@qca.qualcomm.com>
Mohammed Shafi Shajakhan <mohammed@qti.qualcomm.com> wrote:
> From: Mohammed Shafi Shajakhan <mohammed@qti.qualcomm.com>
>
> cleanup 'ath10k_htt_tx_alloc' by introducing the API's
> 'ath10k_htt_tx_alloc/free_{cont_txbuf, txdone_fifo} and
> re-use them whereever needed
>
> Signed-off-by: Mohammed Shafi Shajakhan <mohammed@qti.qualcomm.com>
Patch 1 doesn't apply and the conflict was not trivial
error: patch failed: drivers/net/wireless/ath/ath10k/htt_tx.c:388
error: drivers/net/wireless/ath/ath10k/htt_tx.c: patch does not apply
stg import: Diff does not apply cleanly
error: patch failed: drivers/net/wireless/ath/ath10k/htt_tx.c:283
error: drivers/net/wireless/ath/ath10k/htt_tx.c: patch does not apply
stg import: Diff does not apply cleanly
2 patches set to Changes Requested.
9361741 [1/2] ath10k: clean up HTT tx buffer allocation and free
9361743 [2/2] ath10k: Remove extraneous error message in tx alloc
--
https://patchwork.kernel.org/patch/9361741/
Documentation about submitting wireless patches and checking status
from patchwork:
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
^ permalink raw reply
* Re: [1/3] ath6kl: fix busreqs so they can be reused when sg is cleaned up
From: Kalle Valo @ 2016-10-06 7:15 UTC (permalink / raw)
To: James Minor
Cc: linux-wireless, ath6kl, julia.cartwright, steve.derosier,
James Minor
In-Reply-To: <1475517604-17710-2-git-send-email-james.minor@ni.com>
James Minor <james.minor@ni.com> wrote:
> To reuse the busreqs in case of hardware restart, they must be
> properly reinitialized. If the scat_req pointer isn't reset to
> 0, __ath6kl_sdio_write_async() will assume there is sg work to be
> done (causing a kernel OOPS).
>
> Signed-off-by: James Minor <james.minor@ni.com>
> Reviewed-by: Steve deRosier <steve.derosier@lairdtech.com>
3 patches applied to ath-next branch of ath.git, thanks.
3605d751d5dd ath6kl: fix busreqs so they can be reused when sg is cleaned up
db14b18a73a1 ath6kl: after cleanup properly reflect that sg is disabled
fdb6e4839e3a ath6kl: configure SDIO when power is reapplied
--
https://patchwork.kernel.org/patch/9360777/
Documentation about submitting wireless patches and checking status
from patchwork:
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
^ permalink raw reply
* Re: [RFC 0/3] of: add common bindings to (de)activate IEEE 802.11 bands
From: Felix Fietkau @ 2016-10-05 20:34 UTC (permalink / raw)
To: Rob Herring
Cc: Martin Blumenstingl, Mark Rutland, Frank Rowand,
devicetree@vger.kernel.org, linux-wireless, ath9k-devel,
ath9k-devel, Kalle Valo
In-Reply-To: <CAL_JsqJfxEZCtdNi-wRgrtdmkPsGw0-ZMGhdbcoQ=seyij5LiQ@mail.gmail.com>
On 2016-10-05 22:31, Rob Herring wrote:
> On Wed, Oct 5, 2016 at 1:36 PM, Felix Fietkau <nbd@nbd.name> wrote:
>> On 2016-10-05 20:25, Martin Blumenstingl wrote:
>>> On Mon, Oct 3, 2016 at 5:22 PM, Rob Herring <robh+dt@kernel.org> wrote:
>>>> On Sun, Oct 2, 2016 at 5:50 PM, Martin Blumenstingl
>>>> <martin.blumenstingl@googlemail.com> wrote:
>>>>> There are at least two drivers (ath9k and mt76) out there, where
>>>>> devicetree authors need to override the enabled bands.
>>>>>
>>>>> For ath9k there is only one use-case: disabling a band which has been
>>>>> incorrectly enabled by the vendor in the EEPROM (enabling a band is not
>>>>> possible because the calibration data would be missing in the EEPROM).
>>>>> The mt76 driver (currently pending for review) however allows enabling
>>>>> and disabling the 2.4GHz and 5GHz band, see [0].
>>>>>
>>>>> Based on the discussion of (earlier versions of) my ath9k devicetree
>>>>> patch it was suggested [1] that we use enable- and disable- properties.
>>>>> The current patch implements:
>>>>> - bands can be enabled or disabled with the corresponding property
>>>>> - if both (disable and enable) properties are given and a driver asks
>>>>> whether the band is enabled then the logic will return false (= not
>>>>> enabled, preferring the disabled flag)
>>>>> - if both (disable and enable) properties are given and a driver asks
>>>>> whether the band is disabled then the logic will return true (again,
>>>>> preferring the disabled flag over the enabled flag)
>>>>>
>>>>> We can still change the logic to do what the mt76 driver does (I am
>>>>> fine with both solutions):
>>>>> - property not available: driver decides which bands to enable
>>>>> - property set to 0: disable the band
>>>>> - property set to 1: enable the band
>>>>
>>>> I prefer this way. Really, I'd prefer just a boolean disable property.
>>>> I'm not sure why you need the enable. We usually have these tri-state
>>>> properties when you want not present to mean use the bootloader or
>>>> default setting. Try to make not present the most common case.
>>> Felix: could you please give a few details why mt76 can not only
>>> disable but also enable a specific band?
>>> There is no specific comment in the sources [0] why this is needed.
>>> All other drivers that I am aware of (ath9k, rt2x00) only allow
>>> disabling a specific band, they never enable it (this has to be done
>>> explicitly by the EEPROM).
>> None of the devices use it at the moment, I probably added it when I
>> played with a device that had broken EEPROM data. I guess I decided to
>> do it this way because on many devices the band capability field simply
>> cannot be trusted.
>> I guess I would be okay with just having a disable property and adding
>> an enable property later only if we end up actually needing it.
>
> If EEPROM is commonly wrong or missing, then seems like you want to
> plan ahead and support both enable and disable.
>
> The other approach I've mentioned before is just put raw EEPROM data
> into DT to override the EEPROM. This would be better if there's a
> large number of settings you need.
So far, other EEPROM settings didn't need to be changed.
- Felix
^ permalink raw reply
* Re: [RFC 0/3] of: add common bindings to (de)activate IEEE 802.11 bands
From: Rob Herring @ 2016-10-05 20:31 UTC (permalink / raw)
To: Felix Fietkau
Cc: Martin Blumenstingl, Mark Rutland, Frank Rowand,
devicetree@vger.kernel.org, linux-wireless, ath9k-devel,
ath9k-devel, Kalle Valo
In-Reply-To: <23f49efe-ddef-ea16-00de-7477e4872d82@nbd.name>
On Wed, Oct 5, 2016 at 1:36 PM, Felix Fietkau <nbd@nbd.name> wrote:
> On 2016-10-05 20:25, Martin Blumenstingl wrote:
>> On Mon, Oct 3, 2016 at 5:22 PM, Rob Herring <robh+dt@kernel.org> wrote:
>>> On Sun, Oct 2, 2016 at 5:50 PM, Martin Blumenstingl
>>> <martin.blumenstingl@googlemail.com> wrote:
>>>> There are at least two drivers (ath9k and mt76) out there, where
>>>> devicetree authors need to override the enabled bands.
>>>>
>>>> For ath9k there is only one use-case: disabling a band which has been
>>>> incorrectly enabled by the vendor in the EEPROM (enabling a band is not
>>>> possible because the calibration data would be missing in the EEPROM).
>>>> The mt76 driver (currently pending for review) however allows enabling
>>>> and disabling the 2.4GHz and 5GHz band, see [0].
>>>>
>>>> Based on the discussion of (earlier versions of) my ath9k devicetree
>>>> patch it was suggested [1] that we use enable- and disable- properties.
>>>> The current patch implements:
>>>> - bands can be enabled or disabled with the corresponding property
>>>> - if both (disable and enable) properties are given and a driver asks
>>>> whether the band is enabled then the logic will return false (= not
>>>> enabled, preferring the disabled flag)
>>>> - if both (disable and enable) properties are given and a driver asks
>>>> whether the band is disabled then the logic will return true (again,
>>>> preferring the disabled flag over the enabled flag)
>>>>
>>>> We can still change the logic to do what the mt76 driver does (I am
>>>> fine with both solutions):
>>>> - property not available: driver decides which bands to enable
>>>> - property set to 0: disable the band
>>>> - property set to 1: enable the band
>>>
>>> I prefer this way. Really, I'd prefer just a boolean disable property.
>>> I'm not sure why you need the enable. We usually have these tri-state
>>> properties when you want not present to mean use the bootloader or
>>> default setting. Try to make not present the most common case.
>> Felix: could you please give a few details why mt76 can not only
>> disable but also enable a specific band?
>> There is no specific comment in the sources [0] why this is needed.
>> All other drivers that I am aware of (ath9k, rt2x00) only allow
>> disabling a specific band, they never enable it (this has to be done
>> explicitly by the EEPROM).
> None of the devices use it at the moment, I probably added it when I
> played with a device that had broken EEPROM data. I guess I decided to
> do it this way because on many devices the band capability field simply
> cannot be trusted.
> I guess I would be okay with just having a disable property and adding
> an enable property later only if we end up actually needing it.
If EEPROM is commonly wrong or missing, then seems like you want to
plan ahead and support both enable and disable.
The other approach I've mentioned before is just put raw EEPROM data
into DT to override the EEPROM. This would be better if there's a
large number of settings you need.
Rob
^ permalink raw reply
* [PATCH v2] ath10k: cache calibration data when the core is stopped
From: Marty Faltesek @ 2016-10-05 20:23 UTC (permalink / raw)
To: ath10k; +Cc: linux-wireless, Marty Faltesek
Caching calibration data allows it to be accessed when the
device is not active.
---
drivers/net/wireless/ath/ath10k/core.h | 1 +
drivers/net/wireless/ath/ath10k/debug.c | 72 ++++++++++++++-------------------
2 files changed, 31 insertions(+), 42 deletions(-)
diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireless/ath/ath10k/core.h
index 6e5aa2d..7274ebe 100644
--- a/drivers/net/wireless/ath/ath10k/core.h
+++ b/drivers/net/wireless/ath/ath10k/core.h
@@ -452,6 +452,7 @@ struct ath10k_debug {
u32 nf_cal_period;
struct ath10k_fw_crash_data *fw_crash_data;
+ void *cal_data;
};
enum ath10k_state {
diff --git a/drivers/net/wireless/ath/ath10k/debug.c b/drivers/net/wireless/ath/ath10k/debug.c
index 832da6e..668074c 100644
--- a/drivers/net/wireless/ath/ath10k/debug.c
+++ b/drivers/net/wireless/ath/ath10k/debug.c
@@ -1451,75 +1451,58 @@ static const struct file_operations fops_fw_dbglog = {
.llseek = default_llseek,
};
-static int ath10k_debug_cal_data_open(struct inode *inode, struct file *file)
+static int ath10k_debug_cal_data_fetch(struct ath10k *ar)
{
- struct ath10k *ar = inode->i_private;
- void *buf;
u32 hi_addr;
__le32 addr;
int ret;
- mutex_lock(&ar->conf_mutex);
-
- if (ar->state != ATH10K_STATE_ON &&
- ar->state != ATH10K_STATE_UTF) {
- ret = -ENETDOWN;
- goto err;
- }
-
- buf = vmalloc(ar->hw_params.cal_data_len);
- if (!buf) {
- ret = -ENOMEM;
- goto err;
- }
-
hi_addr = host_interest_item_address(HI_ITEM(hi_board_data));
ret = ath10k_hif_diag_read(ar, hi_addr, &addr, sizeof(addr));
+
if (ret) {
- ath10k_warn(ar, "failed to read hi_board_data address: %d\n", ret);
- goto err_vfree;
+ ath10k_warn(ar, "failed to read hi_board_data address: %d\n",
+ ret);
+ return ret;
}
- ret = ath10k_hif_diag_read(ar, le32_to_cpu(addr), buf,
+ ret = ath10k_hif_diag_read(ar, le32_to_cpu(addr), ar->debug.cal_data,
ar->hw_params.cal_data_len);
if (ret) {
ath10k_warn(ar, "failed to read calibration data: %d\n", ret);
- goto err_vfree;
+ return ret;
}
- file->private_data = buf;
+ return 0;
+}
- mutex_unlock(&ar->conf_mutex);
+static int ath10k_debug_cal_data_open(struct inode *inode, struct file *file)
+{
+ struct ath10k *ar = inode->i_private;
- return 0;
+ mutex_lock(&ar->conf_mutex);
-err_vfree:
- vfree(buf);
+ if (ar->state == ATH10K_STATE_ON ||
+ ar->state == ATH10K_STATE_UTF) {
+ ath10k_debug_cal_data_fetch(ar);
+ }
-err:
+ file->private_data = ar;
mutex_unlock(&ar->conf_mutex);
- return ret;
+ return 0;
}
static ssize_t ath10k_debug_cal_data_read(struct file *file,
- char __user *user_buf,
- size_t count, loff_t *ppos)
+ char __user *user_buf,
+ size_t count, loff_t *ppos)
{
struct ath10k *ar = file->private_data;
- void *buf = file->private_data;
return simple_read_from_buffer(user_buf, count, ppos,
- buf, ar->hw_params.cal_data_len);
-}
-
-static int ath10k_debug_cal_data_release(struct inode *inode,
- struct file *file)
-{
- vfree(file->private_data);
-
- return 0;
+ ar->debug.cal_data,
+ ar->hw_params.cal_data_len);
}
static ssize_t ath10k_write_ani_enable(struct file *file,
@@ -1580,7 +1563,6 @@ static const struct file_operations fops_ani_enable = {
static const struct file_operations fops_cal_data = {
.open = ath10k_debug_cal_data_open,
.read = ath10k_debug_cal_data_read,
- .release = ath10k_debug_cal_data_release,
.owner = THIS_MODULE,
.llseek = default_llseek,
};
@@ -1932,6 +1914,8 @@ void ath10k_debug_stop(struct ath10k *ar)
{
lockdep_assert_held(&ar->conf_mutex);
+ ath10k_debug_cal_data_fetch(ar);
+
/* Must not use _sync to avoid deadlock, we do that in
* ath10k_debug_destroy(). The check for htt_stats_mask is to avoid
* warning from del_timer(). */
@@ -2343,7 +2327,9 @@ int ath10k_debug_create(struct ath10k *ar)
ar->debug.fw_crash_data = vzalloc(sizeof(*ar->debug.fw_crash_data));
if (!ar->debug.fw_crash_data)
return -ENOMEM;
-
+ ar->debug.cal_data = vzalloc(ar->hw_params.cal_data_len);
+ if (!ar->debug.cal_data)
+ return -ENOMEM;
INIT_LIST_HEAD(&ar->debug.fw_stats.pdevs);
INIT_LIST_HEAD(&ar->debug.fw_stats.vdevs);
INIT_LIST_HEAD(&ar->debug.fw_stats.peers);
@@ -2355,7 +2341,9 @@ int ath10k_debug_create(struct ath10k *ar)
void ath10k_debug_destroy(struct ath10k *ar)
{
vfree(ar->debug.fw_crash_data);
+ vfree(ar->debug.cal_data);
ar->debug.fw_crash_data = NULL;
+ ar->debug.cal_data = NULL;
ath10k_debug_fw_stats_reset(ar);
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* Re: [PATCH v4] ath9k: Switch to using mac80211 intermediate software queues.
From: Toke Høiland-Jørgensen @ 2016-10-05 19:56 UTC (permalink / raw)
To: Kalle Valo
Cc: linux-wireless, make-wifi-fast, ath9k-devel, Tim Shepard,
Felix Fietkau
In-Reply-To: <874m4qof58.fsf@kamboji.qca.qualcomm.com>
Kalle Valo <kvalo@codeaurora.org> writes:
> Toke H=C3=B8iland-J=C3=B8rgensen <toke@toke.dk> writes:
>
>> Kalle Valo <kvalo@codeaurora.org> writes:
>>
>>> Toke H=C3=B8iland-J=C3=B8rgensen <toke@toke.dk> writes:
>>>
>>>> Kalle Valo <kvalo@codeaurora.org> writes:
>>>>
>>>>> Toke H=C3=B8iland-J=C3=B8rgensen <toke@toke.dk> writes:
>>>>>
>>>>> I understand your point, but I don't want to rush this to 4.9 and then
>>>>> start getting lots of bug reports and eventually forced to revert it.=
If
>>>>> we just found a new serious regression the chances are that there are
>>>>> more lurking somewhere and this patch is just not ready yet.
>>>>
>>>> So, the changes to mac80211 that fixes the known regressions of this
>>>> patch have gone in.
>>>
>>> I guess you mean this commit:
>>>
>>> bb42f2d13ffc mac80211: Move reorder-sensitive TX handlers to after TXQ =
dequeue
>>>
>>> (Just making sure that I have the same commit in my tree when I apply
>>> this)
>>
>> Yup, that's the one :)
>>
>>>> Any chance of seeing this merged during the current merge window? :)
>>>
>>> I sent last new feature ("-next") patches for 4.9 last week, sorry. So
>>> this has to wait for 4.10.
>>
>> Ah, right, I think I got my merge windows confused. You already said you
>> wouldn't take it for 4.9. So I guess what I'm asking is for you to put
>> it into the appropriate -next tree so it can get some wider exposure
>> ahead of the *next* merge window...
>
> Yeah, we have plenty of time for 4.10 :) So my plan is to apply this
> after I open wireless-drivers-next in 2-3 weeks or so. That would mean
> that the patch would hit Linus' tree when 4.10-rc1 is released
> (estimated to happen on 2017-01-01). The timing is actually perfect as
> now we get maximal testing time on -next.
So the -next trees are those that are open outside the merge window.
Right, got it; thanks :)
>>> And I assume I need to take v5:
>>>
>>> https://patchwork.kernel.org/patch/9311037/
>>
>> Yes. Haven't noticed anything that changed since that might conflict
>> with it, but let me know if I missed something and you want a refreshed
>> version.
>
> Thanks, I'll let you know if there are any problems.
Cool.
-Toke
^ 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