Re: [PATCH] brcmfmac: avoid duplicated suspend/resume operation

[Arend van Spriel]

On 01/22/15 14:54, Sergei Shtylyov wrote:

Hello.

On 1/22/2015 4:49 PM, Kalle Valo wrote:


>From 04d3fa673897ca4ccbea6c76836d0092dba2484a Mon Sep 17 00:00:00 2001
From: Zhonghui Fu 
Date: Tue, 20 Jan 2015 11:14:13 +0800
Subject: [PATCH] brcmfmac: avoid duplicated suspend/resume operation



WiFi chip has 2 SDIO functions, and PM core will trigger
twice suspend/resume operations for one WiFi chip to do
the same things. This patch avoid this case.



Acked-by: Arend van Spriel 
Acked-by: Sergei Shtylyov 
Acked-by: Kalle Valo 
Signed-off-by: Zhonghui Fu 



I don't remember giving Acked-by to this (or for matter to anything for
a long time). What about Sergei or Arend?


I haven't ACK'ed this patch either.


I did ACK the initial patch and felt it still valid for this 'V2' patch.

Regards,
Arend
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v5 2/3] i2c: iproc: Add Broadcom iProc I2C Driver

[Arend van Spriel]

On 01/18/15 12:56, Uwe Kleine-König wrote:

Hello,

On Sun, Jan 18, 2015 at 12:46:51PM +0100, Arend van Spriel wrote:

On 01/18/15 12:17, Uwe Kleine-König wrote:

Hello Wolfram,

On Sun, Jan 18, 2015 at 12:06:58PM +0100, Wolfram Sang wrote:

On Sun, Jan 18, 2015 at 10:47:41AM +0100, Uwe Kleine-König wrote:

On Sun, Jan 18, 2015 at 10:14:04AM +0100, Arend van Spriel wrote:

On 01/17/15 00:42, Ray Jui wrote:

+   complete_all(&iproc_i2c->done);


Looking over this code it seems to me there is always a single
process waiting for iproc_i2c->done to complete. So using complete()
here would suffice.

Yeah, there is always only a single thread waiting. That means both
complete and complete_all are suitable. AFAIK there is no reason to pick
one over the other in this case.


Clarity?

And which do you consider more clear? complete_all might result in the
question: "Is there>1 waiter?" and complete might yield to "What about
the other waiters?". If you already know there is only one, both are on
par on clarity. Might only be me?! I don't care much.


Maybe it is me, but it is not about questions but it is about
implicit statements that the code makes (or reader derives from it).
When using complete_all you indicate to the reader "there can be
more than one waiter". When using complete it indicates "there is
only one waiter". If those statements are not true that is a code

No, complete works just fine in the presence of>1 waiter. It just wakes
a single waiter and all others continue to wait.


Yes. Agree.


That is, for single-waiter situations there is no semantic difference
between complete and complete_all. But there is a difference for
multi-waiter queues.


Indeed.


I think this is just a matter of your POV in the single-waiter
situation: complete might be intuitive because you just completed a
single task and complete_all might be intuitive because it signals
"I'm completely done, there is noone waiting for me any more.".


Ok. Let's leave it to the author's intuition or to say it differently 
"sorry for the noise" ;-)


Regards,
Arend


Best regards
Uwe



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v5 2/3] i2c: iproc: Add Broadcom iProc I2C Driver

[Arend van Spriel]

On 01/18/15 12:17, Uwe Kleine-König wrote:

Hello Wolfram,

On Sun, Jan 18, 2015 at 12:06:58PM +0100, Wolfram Sang wrote:

On Sun, Jan 18, 2015 at 10:47:41AM +0100, Uwe Kleine-König wrote:

On Sun, Jan 18, 2015 at 10:14:04AM +0100, Arend van Spriel wrote:

On 01/17/15 00:42, Ray Jui wrote:

+   complete_all(&iproc_i2c->done);


Looking over this code it seems to me there is always a single
process waiting for iproc_i2c->done to complete. So using complete()
here would suffice.

Yeah, there is always only a single thread waiting. That means both
complete and complete_all are suitable. AFAIK there is no reason to pick
one over the other in this case.


Clarity?

And which do you consider more clear? complete_all might result in the
question: "Is there>1 waiter?" and complete might yield to "What about
the other waiters?". If you already know there is only one, both are on
par on clarity. Might only be me?! I don't care much.


Maybe it is me, but it is not about questions but it is about implicit 
statements that the code makes (or reader derives from it). When using 
complete_all you indicate to the reader "there can be more than one 
waiter". When using complete it indicates "there is only one waiter". If 
those statements are not true that is a code issue/bug.


Regards,
Arend


Best regards
Uwe



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v5 2/3] i2c: iproc: Add Broadcom iProc I2C Driver

[Arend van Spriel]

On 01/17/15 00:42, Ray Jui wrote:

[...]


+/*
+ * Can be expanded in the future if more interrupt status bits are utilized
+ */
+#define ISR_MASK (1<<  IS_M_START_BUSY_SHIFT)
+
+static irqreturn_t bcm_iproc_i2c_isr(int irq, void *data)
+{
+   struct bcm_iproc_i2c_dev *iproc_i2c = data;
+   u32 status = readl(iproc_i2c->base + IS_OFFSET);
+
+   status&= ISR_MASK;
+
+   if (!status)
+   return IRQ_NONE;
+
+   writel(status, iproc_i2c->base + IS_OFFSET);
+   complete_all(&iproc_i2c->done);


Looking over this code it seems to me there is always a single process 
waiting for iproc_i2c->done to complete. So using complete() here would 
suffice.


Regards,
Arend


+
+   return IRQ_HANDLED;
+}


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] brcmfmac: avoid duplicated suspend/resume operation

[Arend van Spriel]

On 01/12/15 15:06, Sergei Shtylyov wrote:

Hello.

On 1/12/2015 9:41 AM, Fu, Zhonghui wrote:


From 8685c3c2746b4275fc808d9db23c364b2f54b52a Mon Sep 17 00:00:00 2001
From: Zhonghui Fu 
Date: Mon, 12 Jan 2015 14:25:46 +0800
Subject: [PATCH] brcmfmac: avoid duplicated suspend/resume operation


The lines above are not needed.


WiFi chip has 2 SDIO functions, and PM core will trigger
twice suspend/resume operations for one WiFi chip to do
the same things. This patch avoid this case.



Acked-by: Arend van Spriel
Acked-by: Sergei Shtylyov
Signed-off-by: Zhonghui Fu 
---
drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c | 21
+
1 files changed, 17 insertions(+), 4 deletions(-)



diff --git a/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c
b/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c
index 9880dae..8f71485 100644
--- a/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c
+++ b/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c
@@ -1070,7 +1070,7 @@ static int brcmf_ops_sdio_probe(struct sdio_func
*func,
*/
if ((sdio_get_host_pm_caps(sdiodev->func[1]) & MMC_PM_KEEP_POWER) &&
((sdio_get_host_pm_caps(sdiodev->func[1]) & MMC_PM_WAKE_SDIO_IRQ) ||
- (sdiodev->pdata && sdiodev->pdata->oob_irq_supported)))
+ (sdiodev->pdata->oob_irq_supported)))


Inner parens not needed on this line.


Well, actually this patch should not affect those line as it would 
reintroduce a recently fixed issue.


Regards,
Arend


[...]

WBR, Sergei



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [Patch v5 2/2] gpio: Document GPIO hogging mechanism

[Arend van Spriel]

On 01/12/15 11:20, Linus Walleij wrote:

On Fri, Dec 19, 2014 at 9:07 PM, Benoit Parrot  wrote:


Add GPIO hogging documentation to gpio.txt

Signed-off-by: Benoit Parrot
Reviewed-by: Alexandre Courbot


This is starting to look good ...


+   line_b {
+   gpio-hog;
+   gpios =<6 0>;
+   state = "output-low";


I don't like the state string.

Instead have boolean properties for all states.

line_b {
 gpio-hog;
 gpios =<6 0>;
 output-low;
 line-name = "foo-bar-gpio";
}

Then use of_property_read_bool() in the code to check which
state is to be selected intially. You can check that no mutually
exclusive state are selected, I don't like that an arbitrary string
select the state like that, if we do it that way an enumerator would
be better, I prefer bools.


To avoid the mutual exclusive state checking, would it not be more 
straightforward to use numeric enum values defined in boot/dts/include.


Regards,
Arend


Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] brcmfmac: avoid duplicated suspend/resume operation

[Arend van Spriel]

On 01/12/15 07:41, Fu, Zhonghui wrote:

 From 8685c3c2746b4275fc808d9db23c364b2f54b52a Mon Sep 17 00:00:00 2001
From: Zhonghui Fu
Date: Mon, 12 Jan 2015 14:25:46 +0800
Subject: [PATCH] brcmfmac: avoid duplicated suspend/resume operation

WiFi chip has 2 SDIO functions, and PM core will trigger
twice suspend/resume operations for one WiFi chip to do
the same things. This patch avoid this case.

Acked-by: Arend van Spriel
Acked-by: Sergei Shtylyov
Signed-off-by: Zhonghui Fu


This patch needs to be rebased.

Kalle,

Please drop this one.

Regards,
Arend


---
  drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c |   21 +
  1 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c 
b/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c
index 9880dae..8f71485 100644
--- a/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c
+++ b/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c
@@ -1070,7 +1070,7 @@ static int brcmf_ops_sdio_probe(struct sdio_func *func,
 */
if ((sdio_get_host_pm_caps(sdiodev->func[1])&  MMC_PM_KEEP_POWER)&&
((sdio_get_host_pm_caps(sdiodev->func[1])&  MMC_PM_WAKE_SDIO_IRQ) ||
-(sdiodev->pdata&&  sdiodev->pdata->oob_irq_supported)))
+(sdiodev->pdata->oob_irq_supported)))
bus_if->wowl_supported = true;
  #endif

@@ -1139,11 +1139,17 @@ void brcmf_sdio_wowl_config(struct device *dev, bool 
enabled)
  static int brcmf_ops_sdio_suspend(struct device *dev)
  {
struct brcmf_bus *bus_if = dev_get_drvdata(dev);
-   struct brcmf_sdio_dev *sdiodev = bus_if->bus_priv.sdio;
+   struct brcmf_sdio_dev *sdiodev;
mmc_pm_flag_t sdio_flags;
+   struct sdio_func *func = dev_to_sdio_func(dev);

brcmf_dbg(SDIO, "Enter\n");

+   if (func->num == 2)
+   return 0;
+
+   sdiodev = bus_if->bus_priv.sdio;
+
atomic_set(&sdiodev->suspend, true);

if (sdiodev->wowl_enabled) {
@@ -1164,10 +1170,17 @@ static int brcmf_ops_sdio_suspend(struct device *dev)
  static int brcmf_ops_sdio_resume(struct device *dev)
  {
struct brcmf_bus *bus_if = dev_get_drvdata(dev);
-   struct brcmf_sdio_dev *sdiodev = bus_if->bus_priv.sdio;
+   struct brcmf_sdio_dev *sdiodev;
+   struct sdio_func *func = dev_to_sdio_func(dev);

brcmf_dbg(SDIO, "Enter\n");
-   if (sdiodev->pdata&&  sdiodev->pdata->oob_irq_supported)
+
+   if (func->num == 2)
+   return 0;
+
+   sdiodev = bus_if->bus_priv.sdio;
+
+   if (sdiodev->pdata->oob_irq_supported)
disable_irq_wake(sdiodev->pdata->oob_irq_nr);
brcmf_sdio_wd_timer(sdiodev->bus, BRCMF_WD_POLL_MS);
atomic_set(&sdiodev->suspend, false);
-- 1.7.1



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/2] Fix copy-paste bug: assign from src struct not dest

[Arend van Spriel]

On 01/07/15 23:16, Johannes Berg wrote:

On Wed, 2015-01-07 at 20:18 +0100, Giel van Schijndel wrote:


IMO the aligned block of code has the significant advantage of taking
advantage of humans' ability to spot things that break a pattern. Which
in this case becomes *very* visible when properly aligned, because
without the alignment there is no (visual) pattern (or at least not one
very suitable for my "visual processing system", I know the same applies
to at least some others).


Yeah, well, but why even invoke that "visual processing system"?

If you look, for example, at the __skb_clone function it just uses a
macro:

#define C(x) n->x = skb->x


This requires fixed names so I generally prefer to add them:

#define C(d, s, f)  (d)->f = (s)->f


and then

C(len);
C(data_len);


C(acx, conf, window_size);
C(acx, conf, increase_time);

Regards,
Arend



etc.

johannes

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] brcm80211: brcmsmac: dma: Remove some unused functions

[Arend van Spriel]

On 01/07/15 07:29, Julia Lawall wrote:



On Wed, 7 Jan 2015, Rickard Strandqvist wrote:


2015-01-05 12:06 GMT+01:00 Arend van Spriel:

On 01/05/15 11:49, Kalle Valo wrote:


Rickard Strandqvist   writes:


As I hope you can see I have made some changes regarding the
subject-line. Thought it was an advantage to be able to see which file
I actually removed something from. There seems to be a big focus on
getting right on subject-line right in recent weeks.

I wonder why there is a script that takes a file name, and respond
with an appropriate subject line?



Is there a script for this? Anyway, I would say driver name is enough.
Enough about the subject line ;-) I would like to give some general remarks
as you seem to touch a lot of kernel code. First off, I think it is good to
remove unused stuff. However, I would like some more explanation on your
methodology apart from "partially found by using a static code analysis
program". So a cover-letter explaining that would have been nice (maybe
still is). Things like Kconfig option can affect whether function are used
or not so how did you cover that.

Regards,
Arend



I don't think you can really automate this as some drivers do this a bit
differently. You always need to manually check the commit log.


But ok, I change my script accordingly. Should I submit the patch again?



Yes, please resubmit.





Hi Arend

Yes, a script that had been excellent, I think!
I have one as part of my git send-email script, until a week ago, it
was enough that I removed the "drivers/" and changed all "/" to ": "
I have now been expanded my sed pipe a lot (tell me if anyone is interested)
But now I've seen everything from uppercase and [DIR], etc.
So I can not understand how anyone should be able to get the right
name without a good help.

Sure i like to share how I use cppcheck, but is very hesitant to write
this with each patch mails I send though!

I run:
cppcheck --force --quiet --enable=all .

Or a specific file instead of .

This will include, among other things get a lot of error message such,
+4000 for the kernel.
(style) The function 'xxx' is never used

For these I made a script that searched through all the files after
the function name (cppcheck missed a few). And save the rest so I go
through them and possibly send patches.


I think that the question was about what methodology is cppcheck using to
find the given issue.  But probably cppcheck is a black box that does
whatever it does, so the user doesn't know what the rationale is.


That would have been nice, but I also wanted to know what his subsequent 
steps were to validate the output from cppcheck. I went through some 
cppcheck web pages, but they only elaborate on what is can do and not 
the how. But hey, it is an open-source tool so there is always the code 
to check.



However, I think you mentioned that cppcheck found only some of the
issues.  You could thus describe what was the methodology for finding the
other ones.


Maybe upon removing an unused function it had a ripple effect on others 
becoming unused as well? Still this is speculating and with this kind of 
cleanup effort all over the place it is better to review the methodology.


Regards,
Arend


julia


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] brcm80211: brcmsmac: dma: Remove some unused functions

[Arend van Spriel]

On 01/07/15 00:33, Rickard Strandqvist wrote:

2015-01-05 12:06 GMT+01:00 Arend van Spriel:

On 01/05/15 11:49, Kalle Valo wrote:


Rickard Strandqvist   writes:


As I hope you can see I have made some changes regarding the
subject-line. Thought it was an advantage to be able to see which file
I actually removed something from. There seems to be a big focus on
getting right on subject-line right in recent weeks.

I wonder why there is a script that takes a file name, and respond
with an appropriate subject line?



Is there a script for this? Anyway, I would say driver name is enough.
Enough about the subject line ;-) I would like to give some general remarks
as you seem to touch a lot of kernel code. First off, I think it is good to
remove unused stuff. However, I would like some more explanation on your
methodology apart from "partially found by using a static code analysis
program". So a cover-letter explaining that would have been nice (maybe
still is). Things like Kconfig option can affect whether function are used
or not so how did you cover that.

Regards,
Arend



I don't think you can really automate this as some drivers do this a bit
differently. You always need to manually check the commit log.


But ok, I change my script accordingly. Should I submit the patch again?



Yes, please resubmit.





Hi Arend

Yes, a script that had been excellent, I think!
I have one as part of my git send-email script, until a week ago, it
was enough that I removed the "drivers/" and changed all "/" to ": "
I have now been expanded my sed pipe a lot (tell me if anyone is interested)
But now I've seen everything from uppercase and [DIR], etc.
So I can not understand how anyone should be able to get the right
name without a good help.

Sure i like to share how I use cppcheck, but is very hesitant to write
this with each patch mails I send though!

I run:
cppcheck --force --quiet --enable=all .


And . is the top-level directory in the kernel repo? I am not familiar 
with cppcheck, but does it invoke the kernel Makefile. From a quick 
glance on cppcheck webpage I guess you could enable only the unused 
function checker.



Or a specific file instead of .

This will include, among other things get a lot of error message such,
+4000 for the kernel.
(style) The function 'xxx' is never used

For these I made a script that searched through all the files after
the function name (cppcheck missed a few). And save the rest so I go
through them and possibly send patches.


All the file? Within the same driver or kernel-wide. So now "go through 
them" means compile testing with applicable Kconfig selections?


Gr. AvS
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] Revert "ipw2200: select CFG80211_WEXT"

[Arend van Spriel]

On 01/05/15 23:05, Paul Bolle wrote:

On Mon, 2015-01-05 at 19:57 +0100, Johannes Berg wrote:

Multiple other groups of ioctls could be converted in similar patches,
until at the end you can completely remove ipw_wx_handlers and rely
entirely on cfg80211's wext compatibility.

So far the theory - in practice nobody cared enough to start working on
any of these drivers, let alone actually has the hardware today.


So my suggestion to make ipw2200 no longer use cfg80211_wext_giwname()
would actually be backwards. What's actually needed, in theory, is to
use more of what's provided under CFG80211_WEXT (and, I guess, less of
what's provided under WIRELESS_EXT). Did I get that right?


Yes, but as Johannes indicated it needs consideration what to group in 
the patches.


Regards,
Arend


Thanks,


Paul Bolle



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] Revert "ipw2200: select CFG80211_WEXT"

[Arend van Spriel]

On 01/05/15 18:38, Paul Bolle wrote:

On Mon, 2015-01-05 at 11:14 +0100, Arend van Spriel wrote:

On 01/03/15 23:28, Paul Bolle wrote:

Side note: am I correct in thinking that there's some successor to
CFG80211_WEXT and that the ipw2200 driver could, at least in theory, be
ported to that successor? (ipw2200 hardware appears to be a bit old, so
probably no one would care enough to actually do that.)
net/wireless/kconfig doesn't mention anything like that, so probably I'm
just confused.


ipw2200 is a WEXT driver using some wext functionality (and struct
wiphy) provided by cfg80211 hence it needs CFG80211_WEXT. I guess that
is what makes it confusing.


It doesn't help that I hardly know anything about mac80211, cfg80211 and
nl80211 (and lib80211 for that matter). To me these are mostly just
names that end in 80211.


Grapjas ;-)

cfg80211 provides thin-layer API for fullmac drivers (running 802.11 
stack on the device) and mac80211-based drivers (running 802.11 stack in 
kernel).



Anyhow, concerning, CFG80211_WEXT: it seems the only functionality
provided by that symbol that ipw2200 uses directly is
cfg80211_wext_giwname(). Perhaps ipw2200 could have a private version of
that function, something like ipw2100's ipw2100_wx_get_name(). Should be
trivial to implement (ie, it could take _me_ a day or two).


Indeed or even an hour or two.


But perhaps ipw2200 uses CFG80211_WEXT _indirectly_ too. Ie, in
net/wireless/core.c I stumbled on
 #ifdef CONFIG_CFG80211_WEXT
 rdev->wiphy.wext =&cfg80211_wext_handler;
 #endif


This is the "wext compatibility" being enabled for any cfg80211 or 
mac80211 based driver.




But I net/wireless/wext-core.c I then found
 #ifdef CONFIG_CFG80211_WEXT
 if (dev->ieee80211_ptr&&  dev->ieee80211_ptr->wiphy)
 handlers = dev->ieee80211_ptr->wiphy->wext;
 #endif


wext-core is the WEXT framework and here it extracts WEXT handlers from 
a cfg80211/mac80211-based driver that are store in wiphy structure.



 #ifdef CONFIG_WIRELESS_EXT
 if (dev->wireless_handlers)
 handlers = dev->wireless_handlers;
 #endif


Here wext-core extracts WEXT handlers from a WEXT driver. struct 
net_device::wireless_handlers is only defined for CONFIG_WIRELESS_EXT.



(There's much more to discover about WEXT, of course.) Anyhow, IPW2200
uses both CFG80211_WEXT and WIRELESS_EXT and cfg80211_wext_handler and
ipw2200's wireless_handlers appear to cover the same set of IOCTLS (one
exception: SIOCSIWPMKSA). So by now I'm really puzzled how this all fits
together.


I think ipw2200 is a bit of both worlds indeed adopting the use of 
struct wiphy and wiphy_register() call. That seems to suggest it is a 
cfg80211 driver, but it does not register any cfg80211 driver callbacks 
(see libipw_config_ops in libipw_module.c). So it overrides the WEXT 
ioctls because it needs that to interact with the device.


Regards,
Arend


Thanks,


Paul Bolle



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] brcm80211: brcmsmac: dma: Remove some unused functions

[Arend van Spriel]

On 01/05/15 11:49, Kalle Valo wrote:

Rickard Strandqvist  writes:


As I hope you can see I have made some changes regarding the
subject-line. Thought it was an advantage to be able to see which file
I actually removed something from. There seems to be a big focus on
getting right on subject-line right in recent weeks.

I wonder why there is a script that takes a file name, and respond
with an appropriate subject line?


Is there a script for this? Anyway, I would say driver name is enough. 
Enough about the subject line ;-) I would like to give some general 
remarks as you seem to touch a lot of kernel code. First off, I think it 
is good to remove unused stuff. However, I would like some more 
explanation on your methodology apart from "partially found by using a 
static code analysis program". So a cover-letter explaining that would 
have been nice (maybe still is). Things like Kconfig option can affect 
whether function are used or not so how did you cover that.


Regards,
Arend


I don't think you can really automate this as some drivers do this a bit
differently. You always need to manually check the commit log.


But ok, I change my script accordingly. Should I submit the patch again?


Yes, please resubmit.



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] brcmfmac: avoid duplicated suspend/resume operation

[Arend van Spriel]

On 01/05/15 03:34, Fu, Zhonghui wrote:

Hi Arend,

Where to find your patch for this?


Well, we did not submit it. Hence my "Acked-by:" to your patch below.

Regards,
Arend



Thanks,
Zhonghui

On 2014/12/31 17:56, Arend van Spriel wrote:

On 12/31/14 09:20, Fu, Zhonghui wrote:

   From e34419970a07bfcd365f9c66bdfa552188a0cd26 Mon Sep 17 00:00:00 2001
From: Zhonghui Fu
Date: Mon, 29 Dec 2014 21:25:31 +0800
Subject: [PATCH] brcmfmac: avoid duplicated suspend/resume operation

WiFi chip has 2 SDIO functions, and PM core will trigger
twice suspend/resume operations for one WiFi chip to do
the same things. This patch avoid this case.


We have a patch queued up for this as well, but this one looks good enough 
although I personally prefer container_of() instead of dev_to_sdio_func().

Acked-by: Arend van Spriel

Signed-off-by: Zhonghui Fu
---
   drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c |   19 +--
   1 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c 
b/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c
index 3c06e93..eee7818 100644
--- a/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c
+++ b/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c
@@ -1139,11 +1139,18 @@ void brcmf_sdio_wowl_config(struct device *dev, bool 
enabled)
   static int brcmf_ops_sdio_suspend(struct device *dev)
   {
   struct brcmf_bus *bus_if = dev_get_drvdata(dev);
-struct brcmf_sdio_dev *sdiodev = bus_if->bus_priv.sdio;
+struct brcmf_sdio_dev *sdiodev;
   mmc_pm_flag_t sdio_flags;
+struct sdio_func *func = dev_to_sdio_func(dev);

   brcmf_dbg(SDIO, "Enter\n");

+if (func->num == 2) {
+return 0;
+}
+
+sdiodev = bus_if->bus_priv.sdio;
+
   atomic_set(&sdiodev->suspend, true);

   if (sdiodev->wowl_enabled) {
@@ -1164,9 +1171,17 @@ static int brcmf_ops_sdio_suspend(struct device *dev)
   static int brcmf_ops_sdio_resume(struct device *dev)
   {
   struct brcmf_bus *bus_if = dev_get_drvdata(dev);
-struct brcmf_sdio_dev *sdiodev = bus_if->bus_priv.sdio;
+struct brcmf_sdio_dev *sdiodev;
+struct sdio_func *func = dev_to_sdio_func(dev);

   brcmf_dbg(SDIO, "Enter\n");
+
+if (func->num == 2) {
+return 0;
+}
+
+sdiodev = bus_if->bus_priv.sdio;
+
   if (sdiodev->pdata->oob_irq_supported)
   disable_irq_wake(sdiodev->pdata->oob_irq_nr);
   brcmf_sdio_wd_timer(sdiodev->bus, BRCMF_WD_POLL_MS);
-- 1.7.1



--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html




--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] Revert "ipw2200: select CFG80211_WEXT"

[Arend van Spriel]

On 01/03/15 23:28, Paul Bolle wrote:

On Sat, 2015-01-03 at 10:07 -0800, Linus Torvalds wrote:

On Sat, Jan 3, 2015 at 10:02 AM, Marcel Holtmann  wrote:


why would you revert this? It is obviously the correct change to actually 
select CFG80211_WEXT.


I don't know about obvious, but yeah, I think the select in this case
is actually the better idea anyway.


Obviously it wasn't obvious to me!

My reasoning was that the "ipw2200: select CFG80211_WEXT" commit was
_solely_ a workaround for the breakage introduced by that other patch.
And since that one is now reverted the workaround wasn't needed anymore.

Besied, I thought we try to avoid select-ing symbols that can also be
set manually. As that makes it more likely to trigger circular
dependency problems in the kconfig tools, doesn't it?


We could make the CFG80211_WEXT help message be very negative so that
people aren't encouraged to select it even if they can, but then if
they need the ipw driver it gets selected because of that. Because the
ipw driver is probably the more important of the two if you just
happen to have old hardware but are upgrading yout software (and
anybody who recompiles their own kernel is obviously doing the
latter).


Side note: am I correct in thinking that there's some successor to
CFG80211_WEXT and that the ipw2200 driver could, at least in theory, be
ported to that successor? (ipw2200 hardware appears to be a bit old, so
probably no one would care enough to actually do that.)
net/wireless/kconfig doesn't mention anything like that, so probably I'm
just confused.


ipw2200 is a WEXT driver using some wext functionality (and struct 
wiphy) provided by cfg80211 hence it needs CFG80211_WEXT. I guess that 
is what makes it confusing.


Regards,
Arend



Paul Bolle

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] Revert "cfg80211: make WEXT compatibility unselectable"

[Arend van Spriel]

On 01/01/15 11:56, Andreas Hartmann wrote:

Arend van Spriel wrote:

On 12/31/14 16:14, Andreas Hartmann wrote:

[...]

All in all:
If you want to get rid of wext, you still have to go a *very* long way
to get the same *stable* and high throughput quality with *all* chips
depending on mac80211 and not just a few flagship drivers like Atheros.


Hi Andreas,

That's a nice list of unrelated stuff. This has all nothing to do with
WEXT. Actually, you can build rt5572sta with cfg80211 support
(RT_CFG80211_SUPPORT).


You seem to know sources I don't know off. Could you please tell me,
where to find them?

I have DPO_RT5572_LinuxSTA_2.6.0.1_20120629 which doesn't compile with
HAS_CFG80211_SUPPORT=y because -DCONFIG_AP_SUPPORT, on which
RT_CFG80211_SUPPORT relies, is broken.

DPO_RT5572_LinuxSTA_2.6.1.3_20121022 removed the necessary broken AP
code completely.


Nice.


This thread is about the configuration API and
not about driver performance.


I know.

I tried to show, why WEXT as a whole is still necessary even if there is
a mac80211 based driver, because of the weakness of rt2800usb:
Nip it in the bud.


Yes. WEXT needs to stay for a while. Not arguing that. Just saying this 
is really about cfg80211 providing "WEXT compatibility" so WEXT 
user-space apps can interact with cfg80211-based drivers and how to come 
up with a plan to phase out "WEXT compatibility", not WEXT.


Regards,
Arend


Kind regards,
Andreas


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] Revert "cfg80211: make WEXT compatibility unselectable"

[Arend van Spriel]

On 12/31/14 22:57, Linus Torvalds wrote:

On Wed, Dec 31, 2014 at 1:44 PM, Theodore Ts'o  wrote:


Yeah, the confusing part is that "ip" tends to use "verb object"
scheme, which is consistent with the Cisco IOS command set it was
trying to emulate.


Side note: does anybody think that was really a good idea to begin
with? I mean, Cisco iOS is just _s_ universally loved, right?


I think I sense a bit cynical (under)tone here ;-)


And yeah, I refuse to use "ip link" or other insane commands. Let's
face it, "ifconfig" and "route" are perfectly fine commands, and a
whole lot less confusing than "ip" with random crap after it.  I'm
really not seeing why that "ip" command was seen as an improvement.


So does "ip" provide the same functionality as "ifconfig" and "route" or 
does the package have more to offer.


The "iw" tool provides much more subcommands to perform different 
wireless tasks that are not provided by "iwconfig" and friends. So 
functionally "iw" provides a superset. Just have to get equivalent 
output format to mimic "iwconfig" as Ted suggested.


Well, that's something for next year as we are getting close to midnight 
over here.


Regards,
Arend


(Ok, "ip route" isn't any more complex than "route", but "ip link"
sure as hell isn't simpler than "ifconfig" for most things I can think
of)

  Linus


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] Revert "cfg80211: make WEXT compatibility unselectable"

[Arend van Spriel]

On 12/31/14 22:44, Theodore Ts'o wrote:

On Wed, Dec 31, 2014 at 09:32:13PM +0100, Arend van Spriel wrote:


Agree. I can't even recall using "ip" ever. iw help system does provide
command specific help. The phy keyword is both a command and a selector key,
which I realize is confusing to the user, eg. 'iw help info' does provide
help for the 'info' subcommand.


Yeah, the confusing part is that "ip" tends to use "verb object"
scheme, which is consistent with the Cisco IOS command set it was
trying to emulate.   So for ip, you do something like

ip link info eth0

Where as for "iw" it's almost exactly backwards, i.e.:

iw wlan0 info

It's actually rather unfortunate that there is no consistency between
many of these tools, for example:

ethtool --show-features eth0

If we were going to create a new interface, wouldn't be nice if we
could have some kind of consistency?  Sigh; oh well, water under the
bridge at this point.


And on that water there are different ships with different captains ;-)


Thanks. If there are still drivers, upstream or out-of-tree, providing only
WEXT API this will not work unless iwconfig/iwlist can distinguish those
from cfg80211-based drivers (which is possible) and fallback to WEXT ioctl
syscalls. Just not sure if it is worth the effort. As you stated below, it
does not seem "evil" to retain WEXT if that is providing users what they
need.


Is it really that much effort?  Unless there is some license
incompatibility nonsense (i.e., GPLv2 vs GPLv3), the code's already
there in the wireless-tools source.  It would just be a matter of
trying the new ioctls first, and then falling back to the WEXT ones if
needed, right?


I don't think it is much effort. I think the nl80211 netlink api is not 
an ioctl, but yeah it seems trivial. But if WEXT needs to stay for 
people using WEXT-only drivers, it may be fine to keep cfg80211 wext 
compatibility in place.


Regards,
Arend


Cheers,

- Ted


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] Revert "cfg80211: make WEXT compatibility unselectable"

[Arend van Spriel]

On 12/31/14 18:31, Theodore Ts'o wrote:

On Wed, Dec 31, 2014 at 04:02:24PM +0100, Arend van Spriel wrote:


It is unfortunately indeed. I think iwconfig and friends will never go away
although iw is a better alternative, simply because people don't like to
change their home-made scripts/tools. WIRELESS_EXT actually is largely, but
not entirely, gone in upstream drivers and what we are talking about here is
CFG80211_WEXT which allows WEXT userspace to interact with cfg80211-based
drivers through a compatibility layer.


Most poeple are still using "route" and "ifconfig" instead of "ip".
Deal with it.  Personally, I find it much easier to use the existing
commands instead of figuring all of the various subcommands, and the
options to the subcommands to commands like "ip" and "iw".  At least
"ip help route" will give me all of the options to "ip route", where
as "iw help phy" doesn't tell give me the options; instead I have to
paw through 300 lines of "iw help" in order to find the command I
need.  So having a better user interface / help system so people can
better understand how to use iw would be a great step forward.


Agree. I can't even recall using "ip" ever. iw help system does provide 
command specific help. The phy keyword is both a command and a selector 
key, which I realize is confusing to the user, eg. 'iw help info' does 
provide help for the 'info' subcommand.



Better yet, why not hack into the "iw" command backwards compatibility
so that if argv[0] is "iwlist" or "iwconfig", it provides the limited
subset compatibility to the legacy commands.  Then all you need to do
is to convince the distributions to set up the packaging rules so that
"iw" conflicts with wireless-tools, and you will be able to get
everyone switched over to iw after at least seven years.


Thanks. If there are still drivers, upstream or out-of-tree, providing 
only WEXT API this will not work unless iwconfig/iwlist can distinguish 
those from cfg80211-based drivers (which is possible) and fallback to 
WEXT ioctl syscalls. Just not sure if it is worth the effort. As you 
stated below, it does not seem "evil" to retain WEXT if that is 
providing users what they need.


Regards,
Arend


Note that I said *seven* years --- there are people who try to use an
enterprise kernel, or an older Debian Stable or Ubuntu LTS userspace,
with a newer kernel, and and if said users notice, and complain, Linus
*will* revert the commit.  (Note that I've worked at more than one
company where I was forced to use an older Ubuntu LTS or RHEL distro
if I wanted to connect to the intranet, and I was using bleeding edge
kernels --- and if anything like that had broken, I would have
complained directly to Linus, cc'ing the patch author and the wireless
maintainers with the revert.  And while I fortunately am not trying to
do upstream development with a stable distro, be sure there are other
such folks around who have to live with similar restrictions.)

   - Ted

P.S.  If you really think it's evil that users use the
simpler-to-understand iwconfig/iwlist interface over the iw command
line interface, if you provide full backwards compatibility for the
iwconfig/iwlist commands so you can "take over" from wireless-tools,
you could even have a mode which, in addition to doing what the user
wants, prints a "by the way, here's the equivalent if you want to use
the iw command instead".  I don't see the reason of allowing users to
continue to use iwconfig and iwlist, though --- face it, route and
ifconfig are going to be around for a long time; why not let users use
iwconfig and iwlist if it's sufficient for their needs?



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] Revert "cfg80211: make WEXT compatibility unselectable"

[Arend van Spriel]

On 12/31/14 16:14, Andreas Hartmann wrote:

Jiri Kosina wrote:

On Wed, 31 Dec 2014, Arend van Spriel wrote:


The thing with WEXT is that it will stay as is. So if tools like wicd
want to support new features like P2P it will need to make the switch. I
checked out wicd repo and found a number of iwconfig calls and they kick
off wpa_supplicant with wext driver.


Unfortunately this is by no means just about wicd. I have already received
a few off-list mails from people who were wondering why their home-made
scripts / tools, which are running 'iwconfig' directly suddenly stopped to
work, and that it was indeed fallout of WEXT going away. Given the very
short time this has been in mainline, you can probably imagine the
fireworks once this appears in major release.


It is not just the userspace tools (I prefer them, too), which need
wext, but a lot of drivers, too, such as Mediathek drivers e.g. which
perform *much* better compared to rt2x00, especially concerning USB
chips like the one used by Linksys AE3000 (3x3 Mimo)
(https://wikidevi.com/wiki/Linksys_AE3000), which achieves average
throughputs around 14 MB/s *average* with scp of big (>  10 GB) crypted
files even through reinforced-concrete floor(!) - rt2x00 is *far* away
of providing such a performance.

Next bad point of rt2x00 e.g. is the huge CPU overhead - compare
rt5572sta on Raspi with rt2x00 running netperf and you will see the huge
problem of rt2x00 (which is covered on x86 by mostly oversized multi
core CPUs).

Another big advantage of rt5572sta is: it is *stable* over a lot of
kernel versions (as long as the kernel didn't break interfaces - but
there are patches to catch them).

Even ath9k, which usually is a really fine driver, is broken on some
kernel versions (link and throughput is not stable - my use case depends
*heavily* on very high and longterm stable throughput). That's why I'm
using a VM for my ath9k-device to be independent of these quality
problems of mac80211 (or maybe ath9k - don't know) over different kernel
versions.


All in all:
If you want to get rid of wext, you still have to go a *very* long way
to get the same *stable* and high throughput quality with *all* chips
depending on mac80211 and not just a few flagship drivers like Atheros.


Hi Andreas,

That's a nice list of unrelated stuff. This has all nothing to do with 
WEXT. Actually, you can build rt5572sta with cfg80211 support 
(RT_CFG80211_SUPPORT). This thread is about the configuration API and 
not about driver performance.


Regards,
Arend


Kind regards,
Andreas Hartmann
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] Revert "cfg80211: make WEXT compatibility unselectable"

[Arend van Spriel]

On 12/31/14 15:07, Jiri Kosina wrote:

On Wed, 31 Dec 2014, Arend van Spriel wrote:


The thing with WEXT is that it will stay as is. So if tools like wicd
want to support new features like P2P it will need to make the switch. I
checked out wicd repo and found a number of iwconfig calls and they kick
off wpa_supplicant with wext driver.


Unfortunately this is by no means just about wicd. I have already received
a few off-list mails from people who were wondering why their home-made
scripts / tools, which are running 'iwconfig' directly suddenly stopped to
work, and that it was indeed fallout of WEXT going away. Given the very
short time this has been in mainline, you can probably imagine the
fireworks once this appears in major release.


It is unfortunately indeed. I think iwconfig and friends will never go 
away although iw is a better alternative, simply because people don't 
like to change their home-made scripts/tools. WIRELESS_EXT actually is 
largely, but not entirely, gone in upstream drivers and what we are 
talking about here is CFG80211_WEXT which allows WEXT userspace to 
interact with cfg80211-based drivers through a compatibility layer.


Regards,
Arend
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] Revert "cfg80211: make WEXT compatibility unselectable"

[Arend van Spriel]

On 12/31/14 12:10, Grumbach, Emmanuel wrote:

On 12/30/14 23:52, Jiri Kosina wrote:

This reverts commit 24a0aa212ee2dbe44360288684478d76a8e20a0a.

It's causing severe userspace breakage. Namely, all the utilities from
wireless-utils which are relying on CONFIG_WEXT (which means tools
like 'iwconfig', 'iwlist', etc) are not working anymore. There is a
'iw' utility in newer wireless-tools, which is supposed to be a
replacement for all the "deprecated" binaries, but it's far away from
being massively adopted.

Please see [1] for example of the userspace breakage this is causing.

In addition to that, Larry Finger reports [2] that this patch is also
causing ipw2200 driver being impossible to build.

To me this clearly shows that CONFIG_WEXT is far, far away from being
"deprecated enough" to be removed.


Hi Jiri,

You mentioned in the discussion and I quote: "*If* wireless maintainers think
otherwise, I'll send a revert request to Linus for consideration.". However,
you did not wait for any response from the wireless maintainers nor from the
author of the patch you are reverting.
Seems like an overreaction to me though personally I do not disgree with the
revert itself.


Not to mention the patch has already been applied on Linus's tree...


Water under the bridge. The thing with WEXT is that it will stay as is. 
So if tools like wicd want to support new features like P2P it will need 
to make the switch. I checked out wicd repo and found a number of 
iwconfig calls and they kick off wpa_supplicant with wext driver.


With libnl python support wicd could use nl80211 directly avoiding 
screen-scraping iw output, but it seems not included in distros.


Regards,
Arend



Regards,
Arend


[1] http://thread.gmane.org/gmane.linux.kernel/1857010
[2] http://thread.gmane.org/gmane.linux.network/343688

Signed-off-by: Jiri Kosina
---
   net/wireless/Kconfig | 2 +-
   1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/wireless/Kconfig b/net/wireless/Kconfig index
22ba971..29c8675 100644
--- a/net/wireless/Kconfig
+++ b/net/wireless/Kconfig
@@ -175,7 +175,7 @@ config CFG80211_INTERNAL_REGDB
  Most distributions have a CRDA package.  So if unsure, say N.

   config CFG80211_WEXT
-   bool
+   bool "cfg80211 wireless extensions compatibility"
depends on CFG80211
select WEXT_CORE
help




--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] Revert "cfg80211: make WEXT compatibility unselectable"

[Arend van Spriel]

On 12/30/14 23:52, Jiri Kosina wrote:

This reverts commit 24a0aa212ee2dbe44360288684478d76a8e20a0a.

It's causing severe userspace breakage. Namely, all the utilities
from wireless-utils which are relying on CONFIG_WEXT (which means
tools like 'iwconfig', 'iwlist', etc) are not working anymore. There
is a 'iw' utility in newer wireless-tools, which is supposed to be
a replacement for all the "deprecated" binaries, but it's far away
from being massively adopted.

Please see [1] for example of the userspace breakage this is causing.

In addition to that, Larry Finger reports [2] that this patch is also
causing ipw2200 driver being impossible to build.

To me this clearly shows that CONFIG_WEXT is far, far away from being
"deprecated enough" to be removed.


Hi Jiri,

You mentioned in the discussion and I quote: "*If* wireless maintainers 
think otherwise, I'll send a revert request to Linus for 
consideration.". However, you did not wait for any response from the 
wireless maintainers nor from the author of the patch you are reverting. 
Seems like an overreaction to me though personally I do not disgree with 
the revert itself.


Regards,
Arend


[1] http://thread.gmane.org/gmane.linux.kernel/1857010
[2] http://thread.gmane.org/gmane.linux.network/343688

Signed-off-by: Jiri Kosina
---
  net/wireless/Kconfig | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/wireless/Kconfig b/net/wireless/Kconfig
index 22ba971..29c8675 100644
--- a/net/wireless/Kconfig
+++ b/net/wireless/Kconfig
@@ -175,7 +175,7 @@ config CFG80211_INTERNAL_REGDB
  Most distributions have a CRDA package.  So if unsure, say N.

  config CFG80211_WEXT
-   bool
+   bool "cfg80211 wireless extensions compatibility"
depends on CFG80211
select WEXT_CORE
help


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] brcmfmac: avoid duplicated suspend/resume operation

[Arend van Spriel]

On 12/31/14 09:20, Fu, Zhonghui wrote:

 From e34419970a07bfcd365f9c66bdfa552188a0cd26 Mon Sep 17 00:00:00 2001
From: Zhonghui Fu
Date: Mon, 29 Dec 2014 21:25:31 +0800
Subject: [PATCH] brcmfmac: avoid duplicated suspend/resume operation

WiFi chip has 2 SDIO functions, and PM core will trigger
twice suspend/resume operations for one WiFi chip to do
the same things. This patch avoid this case.


We have a patch queued up for this as well, but this one looks good 
enough although I personally prefer container_of() instead of 
dev_to_sdio_func().


Acked-by: Arend van Spriel 

Signed-off-by: Zhonghui Fu
---
  drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c |   19 +--
  1 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c 
b/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c
index 3c06e93..eee7818 100644
--- a/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c
+++ b/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c
@@ -1139,11 +1139,18 @@ void brcmf_sdio_wowl_config(struct device *dev, bool 
enabled)
  static int brcmf_ops_sdio_suspend(struct device *dev)
  {
struct brcmf_bus *bus_if = dev_get_drvdata(dev);
-   struct brcmf_sdio_dev *sdiodev = bus_if->bus_priv.sdio;
+   struct brcmf_sdio_dev *sdiodev;
mmc_pm_flag_t sdio_flags;
+   struct sdio_func *func = dev_to_sdio_func(dev);

brcmf_dbg(SDIO, "Enter\n");

+   if (func->num == 2) {
+   return 0;
+   }
+
+   sdiodev = bus_if->bus_priv.sdio;
+
atomic_set(&sdiodev->suspend, true);

if (sdiodev->wowl_enabled) {
@@ -1164,9 +1171,17 @@ static int brcmf_ops_sdio_suspend(struct device *dev)
  static int brcmf_ops_sdio_resume(struct device *dev)
  {
struct brcmf_bus *bus_if = dev_get_drvdata(dev);
-   struct brcmf_sdio_dev *sdiodev = bus_if->bus_priv.sdio;
+   struct brcmf_sdio_dev *sdiodev;
+   struct sdio_func *func = dev_to_sdio_func(dev);

brcmf_dbg(SDIO, "Enter\n");
+
+   if (func->num == 2) {
+   return 0;
+   }
+
+   sdiodev = bus_if->bus_priv.sdio;
+
if (sdiodev->pdata->oob_irq_supported)
disable_irq_wake(sdiodev->pdata->oob_irq_nr);
brcmf_sdio_wd_timer(sdiodev->bus, BRCMF_WD_POLL_MS);
-- 1.7.1



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] bcma: fix three coding style issues, more than 80 characters per line

[Arend van Spriel]

On 12/28/14 15:02, Rafał Miłecki wrote:

On 28 December 2014 at 10:12, Sedat Dilek  wrote:

On Sun, Dec 28, 2014 at 9:53 AM, Rafał Miłecki  wrote:

On 28 December 2014 at 06:50, Sedat Dilek  wrote:

On Sun, Dec 28, 2014 at 12:44 AM, Rafał Miłecki  wrote:

On 27 December 2014 at 20:24, Oscar Forner Martinez
  wrote:

Three lines with more than 80 characters per line have been split in several 
lines.

Signed-off-by: Oscar Forner Martinez


Acked-by: Rafał Miłecki



As for the comment-line changes... 80+ chars are allowed for better readability.
So, please don't do that.
[ Checkpatch should not warn on this especially for comments. ]


We almost always split long comments into separated lines in the
kernel. What's different with this case?



1st it is not mandatory.

2nd it is more readable in one line.

-   /* 4706 CC and PMU watchdogs are clocked at
1/4 of ALP clock */
+   /* 4706 CC and PMU watchdogs are clocked at 1/4 of ALP
+* clock
+*/

I agree with you when the comment would be longer.


So I guess there is some rule like
"Don't use 2 lines if there is 80 chars + one word"
? Where can I find such rules?


Personally, I don't like taking this sliding slope. Just stick to the 80 
characters rule and add a few words in the comment, eg:


/* The BCM4706 ChipCommon and PMU watchdogs are
 * clocked at one quarter of the ALP clock.
 */

Regards,
Arend

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] NFC: port100: Introduce the use of function put_unaligned_le16

[Arend van Spriel]

On 12/24/14 03:35, Vaishali Thakkar wrote:

On Wed, Dec 24, 2014 at 12:54 AM, Arend van Spriel  wrote:

On 12/23/14 16:27, Vaishali Thakkar wrote:


This patch introduces the use of function put_unaligned_le16.

This is done using Coccinelle and semantic patch used is as follows:

@a@
typedef u16, __le16;
{u16,__le16} e16;
identifier tmp;
expression ptr;
expression y,e;
type T;
@@

- tmp = cpu_to_le16(y);

<+... when != tmp
(
- memcpy(ptr, (T)&tmp, \(2\|sizeof(u16)\|sizeof(__le16)\|sizeof(e16)\));
+ put_unaligned_le16(y,ptr);
|
- memcpy(ptr, (T)&tmp, ...);
+ put_unaligned_le16(y,ptr);
)
...+>
? tmp = e

@@ type T; identifier a.tmp; @@

- T tmp;
...when != tmp

Signed-off-by: Vaishali Thakkar
---
   drivers/nfc/port100.c | 6 ++
   1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/nfc/port100.c b/drivers/nfc/port100.c
index 4ac4d31..f7cbca8 100644
--- a/drivers/nfc/port100.c
+++ b/drivers/nfc/port100.c
@@ -18,6 +18,7 @@
   #include
   #include
   #include
+#include



It is incorrect to use this header file as was pointed out by Johannes to me
in a brcmfmac driver patch [1]. Unless this driver is architecture specific.

Regards,
Arend

[1]
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=a1d69c60c44


Ok. Thanks for pointing out. I didn't know this. So, I guess using
  could make sense. Should I include this??


That's the one.

Regards,
Arend


   #define VERSION "0.1"

@@ -1136,7 +1137,6 @@ static int port100_in_send_cmd(struct
nfc_digital_dev *ddev,
   {
 struct port100 *dev = nfc_digital_get_drvdata(ddev);
 struct port100_cb_arg *cb_arg;
-   __le16 timeout;

 cb_arg = kzalloc(sizeof(struct port100_cb_arg), GFP_KERNEL);
 if (!cb_arg)
@@ -1145,9 +1145,7 @@ static int port100_in_send_cmd(struct
nfc_digital_dev *ddev,
 cb_arg->complete_cb = cb;
 cb_arg->complete_arg = arg;

-   timeout = cpu_to_le16(_timeout * 10);
-
-   memcpy(skb_push(skb, sizeof(__le16)),&timeout, sizeof(__le16));
+   put_unaligned_le16(_timeout * 10, skb_push(skb, sizeof(__le16)));

 return port100_send_cmd_async(dev, PORT100_CMD_IN_COMM_RF, skb,
   port100_in_comm_rf_complete,
cb_arg);









--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] Drivers: bcma: Fix three coding style issues, more than 80 characters per line.

[Arend van Spriel]

On 12/24/14 08:20, Kalle Valo wrote:

Oscar Forner Martinez  writes:


Three lines with more than 80 characters per line have been split in several 
lines.

Signed-off-by: Oscar Forner Martinez
---
  drivers/bcma/driver_chipcommon.c | 10 +++---
  1 file changed, 7 insertions(+), 3 deletions(-)


Just to handle the bureaucracy before v2 is submitted:

To which tree should this go to? I see that earlier John has applied
patches to drivers/bcma/, but what about now? Should I take these? John,
any suggestions?


It is a bit of an odd ball, but there are couple of wireless driver 
relying on bcma and it is more than just a bus driver. It also does some 
steps of the device initialization (which is convenient, but not sure I 
like it). So I would hope it can stay with wireless subsystem to catch 
issue caused in that area early on.


Regards,
Arend


Oscar, the patchwork entry for this patch looked odd. I'm guessing it
was because your time (or timezone) is wrong:

https://patchwork.kernel.org/patch/5535751/



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] NFC: port100: Introduce the use of function put_unaligned_le16

[Arend van Spriel]

On 12/23/14 16:27, Vaishali Thakkar wrote:

This patch introduces the use of function put_unaligned_le16.

This is done using Coccinelle and semantic patch used is as follows:

@a@
typedef u16, __le16;
{u16,__le16} e16;
identifier tmp;
expression ptr;
expression y,e;
type T;
@@

- tmp = cpu_to_le16(y);

   <+... when != tmp
(
- memcpy(ptr, (T)&tmp, \(2\|sizeof(u16)\|sizeof(__le16)\|sizeof(e16)\));
+ put_unaligned_le16(y,ptr);
|
- memcpy(ptr, (T)&tmp, ...);
+ put_unaligned_le16(y,ptr);
)
   ...+>
? tmp = e

@@ type T; identifier a.tmp; @@

- T tmp;
...when != tmp

Signed-off-by: Vaishali Thakkar
---
  drivers/nfc/port100.c | 6 ++
  1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/nfc/port100.c b/drivers/nfc/port100.c
index 4ac4d31..f7cbca8 100644
--- a/drivers/nfc/port100.c
+++ b/drivers/nfc/port100.c
@@ -18,6 +18,7 @@
  #include
  #include
  #include
+#include


It is incorrect to use this header file as was pointed out by Johannes 
to me in a brcmfmac driver patch [1]. Unless this driver is architecture 
specific.


Regards,
Arend

[1] 
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=a1d69c60c44



  #define VERSION "0.1"

@@ -1136,7 +1137,6 @@ static int port100_in_send_cmd(struct nfc_digital_dev 
*ddev,
  {
struct port100 *dev = nfc_digital_get_drvdata(ddev);
struct port100_cb_arg *cb_arg;
-   __le16 timeout;

cb_arg = kzalloc(sizeof(struct port100_cb_arg), GFP_KERNEL);
if (!cb_arg)
@@ -1145,9 +1145,7 @@ static int port100_in_send_cmd(struct nfc_digital_dev 
*ddev,
cb_arg->complete_cb = cb;
cb_arg->complete_arg = arg;

-   timeout = cpu_to_le16(_timeout * 10);
-
-   memcpy(skb_push(skb, sizeof(__le16)),&timeout, sizeof(__le16));
+   put_unaligned_le16(_timeout * 10, skb_push(skb, sizeof(__le16)));

return port100_send_cmd_async(dev, PORT100_CMD_IN_COMM_RF, skb,
  port100_in_comm_rf_complete, cb_arg);


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] brcmfmac: Do not crash if platform data is not populated

[Arend van Spriel]

On 12/23/14 18:00, Kalle Valo wrote:

Arend van Spriel  writes:


On 12/23/14 16:47, Mika Westerberg wrote:

On Tue, Dec 23, 2014 at 04:37:21PM +0100, Arend van Spriel wrote:

On 12/23/14 15:48, Mika Westerberg wrote:

The driver looks for pdata->oob_irq_supported to find out if wowl can be
supported. However, not all platforms populate pdata in which case we crash
the kernel because of NULL pointer dereference.


Thanks, Mika

However, this was already reported by Dan Carpenter and I submitted a patch
for that a couple of days ago: "[PATCH 02/10] brcmfmac: Fix possible
dereference of NULL pointer." [1].

Regards,
Arend

[1]
http://mid.gmane.org/1419162233-19492-3-git-send-email-ar...@broadcom.com


Oh, good. I didn't notice that one.

Thanks for fixing it :)

BTW, that patch seems to miss brcmf_ops_sdio_resume(), perhaps it is
fixed in another patch?


Aargh, a nice "BTW" this is as there is no other patch for the resume
code. Maybe I should ask Kalle to apply your patch instead of ours. If
it did not already got applied.


I have been waiting for net-next to open so I haven't applied any -next
patches yet. So if you want I can take Mika's patch as well. Just send a
reply to your patch I should drop.

And should this patch go to 3.19?


Yeah, the issue was introduced in 3.19 so that makes sense.

Regards,
Arend


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] brcmfmac: Do not crash if platform data is not populated

[Arend van Spriel]

On 12/23/14 16:47, Mika Westerberg wrote:

On Tue, Dec 23, 2014 at 04:37:21PM +0100, Arend van Spriel wrote:

On 12/23/14 15:48, Mika Westerberg wrote:

The driver looks for pdata->oob_irq_supported to find out if wowl can be
supported. However, not all platforms populate pdata in which case we crash
the kernel because of NULL pointer dereference.


Thanks, Mika

However, this was already reported by Dan Carpenter and I submitted a patch
for that a couple of days ago: "[PATCH 02/10] brcmfmac: Fix possible
dereference of NULL pointer." [1].

Regards,
Arend

[1]
http://mid.gmane.org/1419162233-19492-3-git-send-email-ar...@broadcom.com


Oh, good. I didn't notice that one.

Thanks for fixing it :)

BTW, that patch seems to miss brcmf_ops_sdio_resume(), perhaps it is
fixed in another patch?


Aargh, a nice "BTW" this is as there is no other patch for the resume 
code. Maybe I should ask Kalle to apply your patch instead of ours. If 
it did not already got applied.


Regards,
Arend
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] brcmfmac: Do not crash if platform data is not populated

[Arend van Spriel]

On 12/23/14 15:48, Mika Westerberg wrote:

The driver looks for pdata->oob_irq_supported to find out if wowl can be
supported. However, not all platforms populate pdata in which case we crash
the kernel because of NULL pointer dereference.


Thanks, Mika

However, this was already reported by Dan Carpenter and I submitted a 
patch for that a couple of days ago: "[PATCH 02/10] brcmfmac: Fix 
possible dereference of NULL pointer." [1].


Regards,
Arend

[1] 
http://mid.gmane.org/1419162233-19492-3-git-send-email-ar...@broadcom.com



Fixes: 330b4e4be937 ("brcmfmac: Add wowl support for SDIO devices.")
Reported-by: Christophe Prigent
Signed-off-by: Mika Westerberg
---
  drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c 
b/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c
index 3c06e9365949..9880dae2a569 100644
--- a/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c
+++ b/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c
@@ -1070,7 +1070,7 @@ static int brcmf_ops_sdio_probe(struct sdio_func *func,
 */
if ((sdio_get_host_pm_caps(sdiodev->func[1])&  MMC_PM_KEEP_POWER)&&
((sdio_get_host_pm_caps(sdiodev->func[1])&  MMC_PM_WAKE_SDIO_IRQ) ||
-(sdiodev->pdata->oob_irq_supported)))
+(sdiodev->pdata&&  sdiodev->pdata->oob_irq_supported)))
bus_if->wowl_supported = true;
  #endif

@@ -1167,7 +1167,7 @@ static int brcmf_ops_sdio_resume(struct device *dev)
struct brcmf_sdio_dev *sdiodev = bus_if->bus_priv.sdio;

brcmf_dbg(SDIO, "Enter\n");
-   if (sdiodev->pdata->oob_irq_supported)
+   if (sdiodev->pdata&&  sdiodev->pdata->oob_irq_supported)
disable_irq_wake(sdiodev->pdata->oob_irq_nr);
brcmf_sdio_wd_timer(sdiodev->bus, BRCMF_WD_POLL_MS);
atomic_set(&sdiodev->suspend, false);


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 1/4] pci: iProc: define Broadcom iProc PCIe binding

[Arend van Spriel]

On 12/13/14 20:46, Arnd Bergmann wrote:

On Saturday 13 December 2014 11:05:52 Arend van Spriel wrote:


Makes sense. I think that is what Hauke meant by "adding
additional support for registering to bcma". So the discovery info is a
piece of read-only memory in the chip. Its address is stored in the
chipcommon core register space. BCMA parses that memory blob resulting
in a list of cores which register address info. We could add DT support
in BCMA matching the compatible string and register a core for it.


Ah, interesting idea. That would mirror what we do for drivers/amba,
I like the idea.


+ Rafal

Let's explore this. Although I don't have the iProc hardware to verify it.


However, apart from the discovery info a "discoverable ARM AXI" chip has
a register space per core that provides common procedures like
enable/disable, reset, core status, which are implemented in BCMA. I am
not seeing that register space in the DT examples so I guess this IP
block is not there for iProc chips.


I wouldn't draw conclusions from the absence of some node. Maybe these
registers are present but just not used by the original BSP.


I do not intend to. We have raised the question internally to iProc chip 
designers.


Regards,
Arend
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 1/4] pci: iProc: define Broadcom iProc PCIe binding

[Arend van Spriel]

On 12/12/14 18:14, Arnd Bergmann wrote:

On Friday 12 December 2014 08:53:44 Ray Jui wrote:


On 12/12/2014 4:14 AM, Arnd Bergmann wrote:

On Thursday 11 December 2014 18:36:54 Ray Jui wrote:

index 000..040bc0f
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
@@ -0,0 +1,74 @@
+* Broadcom iProc PCIe controller
+
+Required properties:
+- compatible: Must be "brcm,iproc-pcie"
+- reg: base address and length of the PCIe controller and the MDIO interface
+  that controls the PCIe PHY
+- #interrupt-cells: set to<1>
+- interrupts: interrupt IDs


How many, and what are they?


Different iProc SoCs might have different number of interrupts. I'll
elaborate more on the next patch.


Ok.


+- interrupt-map-mask and interrupt-map, standard PCI properties to define the
+  mapping of the PCIe interface to interrupt numbers
+- bus-range: PCI bus numbers covered
+- #address-cells: set to<3>
+- #size-cells: set to<2>
+- device_type: set to "pci"
+- ranges: ranges for the PCI memory and I/O regions
+- phy-addr: MDC/MDIO adddress of the PCIe PHY


It looks like the phy controller is separate from the PCI controller,
and you even list the same register range for both PHYs. Better make
that a separate driver and put the phy address into the "phys" reference.


Okay. In this case, I need to create a separate PHY driver under the
drivers/phy directory and have the PCIe host driver reference it through
the standard PHY API.


Yes, that is what I meant. In particular, that has the advantage of letting
you reuse the two drivers separately if some new SoC comes up that uses
one but not the other. A lot of PHY implementations can support multiple
protocols (e.g. pcie and usb3), but I don't know if yours does.


+- have-msi-inten-reg: Required for legacy iProc PCIe controllers that need the
+  MSI interrupt enable register to be set explicitly
+
+The Broadcom iProc PCie driver adapts the multi-domain structure, i.e., each
+interface has its own domain and therefore has its own device node
+Example:
+
+SoC specific DT Entry:
+
+   pcie0: pcie@18012000 {
+   compatible = "brcm,iproc-pcie";
+   reg =<0x18012000 0x1000>,
+<0x18002000 0x1000>;


I guess the addresses should be relative to the BCMA bus, and this node
get moved under that. Please see Hauke's patch series, we've discussed
this in great length already.



As Arend van Spriel pointed out in the previous discussion:

BCMA core is the bus driver for discoverable ARM AXI interconnect. Apart
from that it also provides drivers for some cores. For the chips to be
discoverable it needs additional IP logic.

Not all iProc family of SoCs have the additional IP logic and for those
which don't, they cannot use the BCMA bus.


Ok, but the one from your example almost certainly does because the
addresses are exactly the same ones as on bcm53xx.

The same problem likely occurs on other peripherals, not just PCI,
so we will have to come up with a way to have a common driver for
bcma_bus and platform_bus for USB, SPI, brcmsmac, and likely others
too.


Makes sense. I think that is what Hauke meant by "adding
additional support for registering to bcma". So the discovery info is a 
piece of read-only memory in the chip. Its address is stored in the 
chipcommon core register space. BCMA parses that memory blob resulting 
in a list of cores which register address info. We could add DT support 
in BCMA matching the compatible string and register a core for it.


However, apart from the discovery info a "discoverable ARM AXI" chip has 
a register space per core that provides common procedures like 
enable/disable, reset, core status, which are implemented in BCMA. I am 
not seeing that register space in the DT examples so I guess this IP 
block is not there for iProc chips.


Regards,
Arend


+   #interrupt-cells =<1>;
+   interrupts =,
+,
+,
+,
+,
+;





+   interrupt-map-mask =<0 0 0 0>;
+   interrupt-map =<0 0 0 0&gic GIC_SPI 100 IRQ_TYPE_NONE>;


This interrupt is also listed in the "interrupts" above, which is
probably a mistake, unless the IRQ line is shared between all PCI
devices and the PCI host itself.


interrupts are for MSI interrupt support and interrupt-map is for legacy
INTx support. To my best knowledge, MSI and INTx cannot be used at the
same time. "nvidia,tegra20-pcie.txt" and "rcar-pci.txt" have similar
configurations.


Linux drivers will absolutely use MSI and legacy interrupts together, because
some drivers don't support MSI and others enable it unconditionally.

In both your examples (tegra and rcar), the interrupts that share the same
number are auxiliary and are correctly used with IRQF_SHARED, so that works.
If a device MSI just maps to a host IRQ however, you wouldn't be able to
use I

Re: [PATCH 2/4] PCI: iproc: Add Broadcom iProc PCIe driver

[Arend van Spriel]

+ Rafal

On 12/10/14 19:46, Florian Fainelli wrote:

2014-12-10 8:46 GMT-08:00 Scott Branden:

On 14-12-10 03:31 AM, Arnd Bergmann wrote:


On Tuesday 09 December 2014 16:04:29 Ray Jui wrote:


Add initial version of the Broadcom iProc PCIe driver. This driver
has been tested on NSP and Cygnus and is expected to work on all iProc
family of SoCs that deploys the same PCIe host controller

The driver also supports MSI

Signed-off-by: Ray Jui
Reviewed-by: Scott Branden



The driver looks suspiciously like the one that Hauke already submitted a
while ago for bcm53xx. Please come up with a merged driver that works for
both.


Could you please be a little more specific.  What driver did "Hauke already
submitted"?  I do not see any driver in the kernel you are talking about.


https://www.marc.info/?l=linux-pci&m=141547043110684&w=2




Are you sure that iProc isn't based on the BCMA bus infrastructure after
all? Even the physical address of your PCI host falls into the address
range that is used for the internal BCMA bus on the other chips!


BCMA seems to be for MIPS architectures.  It seems to be quite specific to
those architectures using BCMA.  I see no use of it in bcm53xx code?


BCMA lives in its own directory in drivers/bcma/ and is not specific
to MIPS actually. Older BCM47xx/BCM53xx MIPS-based SoCs traditionally
started with a discoverable Silicon Sonics Backplane (drivers/ssb) and
progressively migrated to BCMA (drivers/bcma), both subsystems offer a
very similar bus/device/driver abstraction and discovery mechanism.


BCMA core is the bus driver for discoverable ARM AXI interconnect. Apart 
from that it also provides drivers for some cores. For the chips to be 
discoverable it needs additional IP logic. If that is not used in the 
iProc family devices, it can not use the BCMA-based PCIe controller 
driver that Hauke submitted unless BCMA would provide an API to provide 
the chips' core information statically either per core or a full list.


Regards,
Arend
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: using DMA-API on ARM

[Arend van Spriel]

On 12/09/14 11:29, Russell King - ARM Linux wrote:

On Tue, Dec 09, 2014 at 11:19:40AM +0100, Arend van Spriel wrote:

The issue did not trigger overnight so it seems setting bit 22  solves the issue over here. Now the question is
how to move forward with this. As I understood from Catalin this patch was
not included as it was not considered responsibility of the linux kernel.


It is preferable for firmware to configure the L2 cache appropriately,
which includes things like the prefetch offsets as well as feature bits
like bit 22.

I think what I'll do is queue up a patch which adds a warning if bit 22
is not set, suggesting that firmware is updated to set this bit.


I was thinking in the same direction. Thanks to you all for looking into 
this. It did not feel right to use the dma_sync_single_for_cpu() for 
memory allocated with dma_alloc_coherent() and I am glad this got 
cleared up.


Regards,
Arend

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: using DMA-API on ARM

[Arend van Spriel]

On 12/08/14 18:01, Arend van Spriel wrote:

On 12/08/14 17:03, Catalin Marinas wrote:

On Mon, Dec 08, 2014 at 03:01:32PM +, Arnd Bergmann wrote:

[ 0.00] PL310 OF: cache setting yield illegal associativity
[ 0.00] PL310 OF: -1069781724 calculated, only 8 and 16 legal
[ 0.00] L2C-310 enabling early BRESP for Cortex-A9
[ 0.00] L2C-310 full line of zeros enabled for Cortex-A9
[ 0.00] L2C-310 dynamic clock gating enabled, standby mode enabled
[ 0.00] L2C-310 cache controller enabled, 16 ways, 256 kB
[ 0.00] L2C-310: CACHE_ID 0x41c8, AUX_CTRL 0x4e130001


If the above value is correct, they should make sure bit 22 is set in
AUX_CTRL.


Hante applied the patch and it now says:

[ 0.00] PL310 OF: cache setting yield illegal associativity
[ 0.00] PL310 OF: -1069781724 calculated, only 8 and 16 legal
[ 0.00] L2C-310 enabling early BRESP for Cortex-A9
[ 0.00] L2C-310 full line of zeros enabled for Cortex-A9
[ 0.00] L2C-310 dynamic clock gating enabled, standby mode enabled
[ 0.00] L2C-310 cache controller enabled, 16 ways, 256 kB
[ 0.00] L2C-310: CACHE_ID 0x41c8, AUX_CTRL 0x4e530001

He started running a test overnight. So will see if it hits the failure
with this L2 cache configuration.


The issue did not trigger overnight so it seems setting bit 22 Attribute _Override_ Enable> solves the issue over here. Now the 
question is how to move forward with this. As I understood from Catalin 
this patch was not included as it was not considered responsibility of 
the linux kernel.


Regards,
Arend
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: using DMA-API on ARM

[Arend van Spriel]

On 12/08/14 17:03, Catalin Marinas wrote:

On Mon, Dec 08, 2014 at 03:01:32PM +, Arnd Bergmann wrote:

[0.00] PL310 OF: cache setting yield illegal associativity
[0.00] PL310 OF: -1069781724 calculated, only 8 and 16 legal
[0.00] L2C-310 enabling early BRESP for Cortex-A9
[0.00] L2C-310 full line of zeros enabled for Cortex-A9
[0.00] L2C-310 dynamic clock gating enabled, standby mode enabled
[0.00] L2C-310 cache controller enabled, 16 ways, 256 kB
[0.00] L2C-310: CACHE_ID 0x41c8, AUX_CTRL 0x4e130001


If the above value is correct, they should make sure bit 22 is set in
AUX_CTRL.


Hante applied the patch and it now says:

[0.00] PL310 OF: cache setting yield illegal associativity
[0.00] PL310 OF: -1069781724 calculated, only 8 and 16 legal
[0.00] L2C-310 enabling early BRESP for Cortex-A9
[0.00] L2C-310 full line of zeros enabled for Cortex-A9
[0.00] L2C-310 dynamic clock gating enabled, standby mode enabled
[0.00] L2C-310 cache controller enabled, 16 ways, 256 kB
[0.00] L2C-310: CACHE_ID 0x41c8, AUX_CTRL 0x4e530001

He started running a test overnight. So will see if it hits the failure 
with this L2 cache configuration.


Regards,
Arend
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: using DMA-API on ARM

[Arend van Spriel]

On 12/08/14 16:01, Arnd Bergmann wrote:

On Monday 08 December 2014 13:47:38 Hante Meuleman wrote:

Still using outlook, but will limit the line length, I hope that works for the
moment. Attached is a log with the requested information, it is a little
bit non-standard though. The dump code from the mm was copied in
the driver and called from there, mapping the prints back to our local
printf, but it should produce the same. I did this because I didn't realize
the table is static.

Some background on the test setup: I'm using a Broadcom reference
design AP platform with an BRCM 4708 host SOC.


I think you are using the wrong dtb file, the log says this is
a "Buffalo WZR-1750DHP", not the reference design.


That router is close enough to the reference design.


For the AP router
platform the opensource packet OpenWRT was used. Some small
modifications were made to get it to work on our HW. Only one core
is enabled for the moment (no time to figure out how to enable the
other one). Openwrt was configured to use kernel 3.18-rc2 and
the brcmfmac of the compat-wireless code was updated with our
latest code (minor patches, which have been submitted already).
The device used is 43602 pcie device. Some modifications to the build
system were made to enable PCIE. The test is to connect with a
client to the AP and run iperf (TCP). The test can run for many hours
without a problem, but sometimes fails very quickly.


The bcm4708 platform is maintained by Hauke Mehrtens, adding him to Cc.


Thanks. While going through the DTS files I intended to add him as well ;-)


In your log, I see this message:

[0.00] PL310 OF: cache setting yield illegal associativity
[0.00] PL310 OF: -1069781724 calculated, only 8 and 16 legal
[0.00] L2C-310 enabling early BRESP for Cortex-A9
[0.00] L2C-310 full line of zeros enabled for Cortex-A9
[0.00] L2C-310 dynamic clock gating enabled, standby mode enabled
[0.00] L2C-310 cache controller enabled, 16 ways, 256 kB
[0.00] L2C-310: CACHE_ID 0x41c8, AUX_CTRL 0x4e130001

Evidently the cache controller information in DT is incorrect and
the setup may be wrong as a consequence, which may explain cache
coherency problems.


While staring at the DTS files I suspect there are some parts still 
missing. I have attached them for reference. Catalin pointed us to a 
patch in the l2 cache [1]. We have not tried that yet.



Can you verify that the AUX_CTRL value is the same one you see
in a working kernel?


The log: first the ring allocation info is printed. Starting at
16.124847, ring 2, 3 and 4 are rings used for device to host. In this
log the failure is on a read of ring 3. Ring 3 is 1024 entries of each
16 bytes. The next thing printed is the kernel page tables. Then some
OpenWRT info and the logging of part of the connection setup. Then at
1780.130752 the logging of the failure starts. The sequence number is
modulo 253 with ring size of 1024 matches an "old" entry (read 40,
expected 52). Then the different pointers are printed followed by
the kernel page table. The code does then a cache invalidate on the
dma_handle and the next read the sequence number is correct.


How do you invalidate the cache? A dma_handle is of type dma_addr_t
and we don't define an operation for that, nor does it make sense
on an allocation from dma_alloc_coherent(). What happens if you
take out the invalidate?


dma_sync_single_for_cpu(, DMA_FROM_DEVICE) which ends up invalidating 
the cache (or that is our suspicion).



Can you post the patch that you use (both platform and driver) relative
to the snapshot of the the mainline kernel you are basing on?

Arnd



Regards,
Arend

[1] http://www.arm.linux.org.uk/developer/patches/viewpatch.php?id=6529/1


bcm-dt-files.tar.bz2
Description: BZip2 compressed data

Re: using DMA-API on ARM

[Arend van Spriel]

On 12/05/14 19:53, Catalin Marinas wrote:

On Fri, Dec 05, 2014 at 06:39:45PM +, Catalin Marinas wrote:

On Fri, Dec 05, 2014 at 09:22:22AM +, Arend van Spriel wrote:

For our brcm80211 development we are working on getting brcmfmac driver
up and running on a Broadcom ARM-based platform. The wireless device is
a PCIe device, which is hooked up to the system behind a PCIe host
bridge, and we transfer information between host and device using a
descriptor ring buffer allocated using dma_alloc_coherent(). We mostly
tested on x86 and seen no issue. However, on this ARM platform
(single-core A9) we detect occasionally that the descriptor content is
invalid. When this occurs we do a dma_sync_single_for_cpu() and this is
retried a number of times if the problem persists. Actually, found out
that someone made a mistake by using virt_to_dma(va) to get the
dma_handle parameter. So probably we only provided a delay in the retry
loop. After fixing that a single call to dma_sync_single_for_cpu() is
sufficient. The DMA-API-HOWTO clearly states that:


Does your system have an L2 cache? What's the SoC topology, can PCIe see
such L2 cache (or snoop the L1 caches)?


BTW, if you really have a PL310-like L2 cache, have a look at some
patches (I've seen similar symptoms) and make sure your configuration is
correct:

http://www.arm.linux.org.uk/developer/patches/viewpatch.php?id=6395/1

http://www.arm.linux.org.uk/developer/patches/viewpatch.php?id=6529/1

The first one is vexpress specific. The second one was eventually
discarded by Russell (I don't remember the reason, I guess it's because
SoC code is supposed to set the right bits in there anyway). In your
case, such bits may be set up by firmware, so Linux cannot fix anything
up.


I guess by firmware you mean to bootloader. This one boots with CFE 
bootloader which Broadcom maintains itself so could look into that.


Regards,
Arend

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: using DMA-API on ARM

[Arend van Spriel]

On 12/05/14 19:28, Catalin Marinas wrote:

On Fri, Dec 05, 2014 at 03:06:48PM +, Russell King - ARM Linux wrote:

I've been doing more digging into the current DMA code, and I'm dismayed
to see that there's new bugs in it...

commit 513510ddba9650fc7da456eefeb0ead7632324f6
Author: Laura Abbott
Date:   Thu Oct 9 15:26:40 2014 -0700

 common: dma-mapping: introduce common remapping functions

This uses map_vm_area() to achieve the remapping of pages allocated inside
dma_alloc_coherent().  dma_alloc_coherent() is documented in a rather
round-about way in Documentation/DMA-API.txt:

| Part Ia - Using large DMA-coherent buffers
| --
|
| void *
| dma_alloc_coherent(struct device *dev, size_t size,
|  dma_addr_t *dma_handle, gfp_t flag)
|
| void
| dma_free_coherent(struct device *dev, size_t size, void *cpu_addr,
|dma_addr_t dma_handle)
|
| Free a region of consistent memory you previously allocated.  dev,
| size and dma_handle must all be the same as those passed into
| dma_alloc_coherent().  cpu_addr must be the virtual address returned by
| the dma_alloc_coherent().
|
| Note that unlike their sibling allocation calls, these routines
| may only be called with IRQs enabled.

Note that very last paragraph.  What this says is that it is explicitly
permitted to call dma_alloc_coherent() with IRQs disabled.


This is solved by using a pre-allocated, pre-mapped atomic_pool which
avoids any further mapping. __dma_alloc() calls __alloc_from_pool() when
!__GFP_WAIT.


So we are actually calling dma_alloc_coherent() with GFP_KERNEL during 
device probe. That last paragraph Russell pointed out seems to suggest 
this is not allowed.



This code got pretty complex and we may find bugs. It can be simplified
by a pre-allocated non-cacheable region that is safe in atomic context
(how big you allocate this is hard to say).


If the problem which you (Broadcom) are suffering from is down to the
issue I suspect (that being having mappings with different cache
attributes) then I'm not sure that there's anything we can realistically
do about that.  There's a number of issues which make it hard to see a
way forward.


I'm still puzzled by this problem, so I don't have any suggestion yet. I
wouldn't blame the mismatched attributes yet as I haven't seen such
problem in practice (but you never know).

How does the DT describe this device? Could it have some dma-coherent
property in there that causes dma_alloc_coherent() to create a cacheable
memory?


Ok. Will add it to our todo list: check DTS files for dma-coherent property.

Thanks,
Arend


The reverse could also cause problems: the device is coherent but the
CPU creates a non-cacheable mapping.



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: using DMA-API on ARM

[Arend van Spriel]

On 12/05/14 15:20, Hante Meuleman wrote:

Ok, I'll add the necessary debug to get all the information out,
but it will take some time to get it done, so I won't have anything
before Monday.

-Original Message-
From: Russell King - ARM Linux [mailto:li...@arm.linux.org.uk]
Sent: vrijdag 5 december 2014 14:24
To: Hante Meuleman
Cc: Will Deacon; Arend Van Spriel; Marek Szyprowski; 
linux-arm-ker...@lists.infradead.org; David Miller; 
linux-kernel@vger.kernel.org; brcm80211-dev-list; linux-wireless
Subject: Re: using DMA-API on ARM

Please wrap your message - replying to a message which looks like this in
my editor is far from easy, and gives me much more work to /manually/
reformat it before I can reply to it:


That's what happens with corporate IT forcing to use Outlook. We can 
workaround that using Thunderbird on Citrix. I will enlighten Hante 
about that option :-)


Regards,
Arend


On Fri, Dec 05, 2014 at 12:56:45PM +, Hante Meuleman wrote:

The problem is with data coming from device, so DMA from device to host. The $

However: this indicates that dma_alloc_coherent on an ARM target may result i$

Regards,
Hante


Thanks.

On Fri, Dec 05, 2014 at 12:56:45PM +, Hante Meuleman wrote:

However: this indicates that dma_alloc_coherent on an ARM target may
result in a memory buffer which can be cached which conflicts with
the API of this function.


If the memory has an alias which is cacheable, it is possible for cache
lines to get allocated via that alias, even if the alias has no explicit
accesses to it.

This is something which I've been going on for quite literally /years/ -
mismatched cache attributes can cause unpredictable behaviour.  I've had
a lot of push back from people who are of the opinion that "if it works
for me, then there isn't a problem" and I eventually gave up fighting
the battle, especially as the ARM architecture people weakened my
reasoning behind it by publishing a relaxation of the "no differing
attributes" issue.  This was particularly true of those who wanted to
use ioremap() on system memory - and cases such as
dma_init_coherent_memory().

So, I never fixed this problem in the original DMA allocator code; I
basically gave up with it.  It's a latent bug which did need to be fixed,
and is still present today in the non-CMA case.

The symptoms which you are reporting sound very much like this kind of
problem - the virtual address for the memory returned by
dma_alloc_coherent() will not be cacheable memory - it will have been
remapped using map_vm_area().  However, there could very well be a fully
cacheable lowmem mapping of that memory, which if a read (speculative or
otherwise) will bring a cache line in, and because the caches are VIPT
or PIPT, that cache line can be hit via the non-cacheable mapping too.

What I /really/ need is more evidence of this to tell those disbelievers
where to stick their flawed arguments. :)



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: using DMA-API on ARM

[Arend van Spriel]

On 05-12-14 10:45, Russell King - ARM Linux wrote:

On Fri, Dec 05, 2014 at 10:22:22AM +0100, Arend van Spriel wrote:

For our brcm80211 development we are working on getting brcmfmac driver
up and running on a Broadcom ARM-based platform. The wireless device is
a PCIe device, which is hooked up to the system behind a PCIe host
bridge, and we transfer information between host and device using a
descriptor ring buffer allocated using dma_alloc_coherent(). We mostly
tested on x86 and seen no issue. However, on this ARM platform
(single-core A9) we detect occasionally that the descriptor content is
invalid. When this occurs we do a dma_sync_single_for_cpu() and this is
retried a number of times if the problem persists. Actually, found out
that someone made a mistake by using virt_to_dma(va) to get the
dma_handle parameter. So probably we only provided a delay in the retry
loop. After fixing that a single call to dma_sync_single_for_cpu() is
sufficient. The DMA-API-HOWTO clearly states that:

"""
the hardware should guarantee that the device and the CPU can access the
data in parallel and will see updates made by each other without any
explicit software flushing.
"""

So it seems incorrect that we would need to do a dma_sync for this
memory. That we do need it seems like this memory can end up in
cache(?), or whatever happens, in some rare condition. Is there anyway
to investigate this situation either through DMA-API or some low-level
ARM specific functions.


It's been a long while since I looked at the code, and the code for
dma_alloc_coherent() has completely changed since then with the
addition of CMA.  I'm afraid that anything I would say about it would
not be accurate without research into the possible paths through that
code - it's no longer just a simple allocator.


I know. On this particular platform we are not using CMA.


What you say is correct however: the memory should not have any cache
lines associated with it, if it does, there's a bug somewhere.

Also, the memory will be weakly ordered, which means that writes to such
memory can be reordered.  If ordering matters, barriers should be used.
rmb() and wmb() can be used for this.


Ok. You already had a peek in our code checking the memory barriers, 
which does not have the dma_sync_single_for_cpu() "workaround" yet. So 
here some more background. The problem is in DMA_FROM_DEVICE direction. 
Because of the possible reordering issue we first tried using rmb() in 
the retry loop but that did not solve it. Another experiment was to 
ignore the failed ring descriptor entry and proceed. So we get interrupt 
from device and access the ring descriptor entry. This should contain 
expected value X, however we get X-1 back. When proceeding everything 
works find until hitting the same ring descriptor entry again reading 
X-1 when X+1 would be valid. This lead us to the assumption that somehow 
this entry ended up in cache lines. The issue goes away using the 
dma_sync_single_for_cpu() with DMA_FROM_DEVICE in direction parameter. 
We are not longer using virt_to_dma() so that is no longer an issue. So 
is there any function interface to verify cache status.


Regards,
Arend


(Added Marek for comment on dma_alloc_coherent(), Will for comment on
barrier stuff.)



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

using DMA-API on ARM

[Arend van Spriel]

Hi Russell,

For our brcm80211 development we are working on getting brcmfmac driver
up and running on a Broadcom ARM-based platform. The wireless device is
a PCIe device, which is hooked up to the system behind a PCIe host
bridge, and we transfer information between host and device using a
descriptor ring buffer allocated using dma_alloc_coherent(). We mostly
tested on x86 and seen no issue. However, on this ARM platform
(single-core A9) we detect occasionally that the descriptor content is
invalid. When this occurs we do a dma_sync_single_for_cpu() and this is
retried a number of times if the problem persists. Actually, found out
that someone made a mistake by using virt_to_dma(va) to get the
dma_handle parameter. So probably we only provided a delay in the retry
loop. After fixing that a single call to dma_sync_single_for_cpu() is
sufficient. The DMA-API-HOWTO clearly states that:

"""
the hardware should guarantee that the device and the CPU can access the
data in parallel and will see updates made by each other without any
explicit software flushing.
"""

So it seems incorrect that we would need to do a dma_sync for this
memory. That we do need it seems like this memory can end up in
cache(?), or whatever happens, in some rare condition. Is there anyway
to investigate this situation either through DMA-API or some low-level
ARM specific functions.

Regards,
Arend
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: linux-next: manual merge of the driver-core tree with the net-next tree

[Arend van Spriel]

On 04-12-14 11:19, Jeremiah Mahler wrote:
> Arend,
> 
> I haven't heard if you have looked at this bug at all yet.  I was
> curious so I looked at it some more and I have some more information
> that might be helpful.
> 
> On Wed, Dec 03, 2014 at 01:41:28PM -0800, Jeremiah Mahler wrote:
>> On Wed, Dec 03, 2014 at 01:06:59PM -0800, Jeremiah Mahler wrote:
>>> Arend,
>>>
> [...]
>
> I took a look at the patch that is causing this problem (d32394fae95).
> My config negates everything in the patch except for a one line change
> to ath9k/pci.c.  If I remove this change (shown below) the problem goes
> away.

 Ok. But then it will likely crash when you cat one of the changed debugfs
 files. Guess this commit needs to be reverted entirely.

> [...]
> 
> Referring to the code snippet below, notice that both pci_set_drvdata()
> and dev_set_drvdata() are called.  If the call to dev_set_drvdata() is
> removed the bug goes away.
> 
>   drivers/net/wireless/ath/ath9k/pci.c
> 
>   861 SET_IEEE80211_DEV(hw, &pdev->dev);
>   862 pci_set_drvdata(pdev, hw);
>   863
>   864 sc = hw->priv;
>   865 sc->hw = hw;
>   866 sc->dev = &pdev->dev;
>   867 dev_set_drvdata(sc->dev, sc);
>   868 sc->mem = pcim_iomap_table(pdev)[0];
>   869 sc->driver_data = id->driver_data;
> 
> Translating the call to pci_set_drvdata() produces the following.
> 
>   pci_set_drvdata(pdev, hw);
> (translating from include/linux/pci.h)
> dev_set_drvdata(&pdev->dev, hw)
>   (translating from include/linux/device.h)
>   &pdev->dev->driver = hw;
> 
> And doing the same for dev_set_drvdata().
> 
>   dev_set_drvdata(sc->dev, sc)
> (sc->dev = &pdev->dev)
> dev_set_drvdata(&pdev->dev, sc)
>   (translating from include/linux/device.h)
>   &pdev->dev->driver = sc;
> 
> The same destination is being set with two different values.  Then calls
> to pci_get_drvdata(), which expect hw, are getting sc, and it breaks.

Yes, that was my suspicion as well. This is why I asked to revert the
patch entirely. I see that sc equals hw->priv so I know how to correct
it. However, with merge window around the corner it may be easier to do
a revert.

Regards,
Arend
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: linux-next: manual merge of the driver-core tree with the net-next tree

[Arend van Spriel]

On 12/03/14 17:21, Greg KH wrote:

On Wed, Dec 03, 2014 at 01:49:00PM +0100, Arend van Spriel wrote:

On 12/03/14 11:51, Jeremiah Mahler wrote:

On Wed, Dec 03, 2014 at 12:36:55AM -0800, Jeremiah Mahler wrote:

all,

On Mon, Dec 01, 2014 at 08:34:05AM +0100, Arend van Spriel wrote:

On 01-12-14 08:19, Stephen Rothwell wrote:

Hi Greg,

Today's linux-next merge of the driver-core tree got a conflict in
drivers/net/wireless/ath/ath9k/debug.c between commits 70e535ed0029
("ath9k: clean up debugfs print of reset causes"), 7b8aaead958e
("ath9k: restart hardware after noise floor calibration failure") and
325e18817668 ("ath9k: fix misc debugfs when not using chan context")

>from the net-next tree and commit 631bee257bd5 ("ath: use seq_file api

for ath9k debugfs files") from the driver-core tree.

I fixed it up (see below) and can carry the fix as necessary (no action
is required).

Greg, I am not sure why those 2 commits are even in your tree.  Do they
depend on something else in your tree?


They do. The three commits below are related:

d32394f ath: ath9k: use debugfs_create_devm_seqfile() helper for
seq_file entrie
631bee2 ath: use seq_file api for ath9k debugfs files
98210b7 debugfs: add helper function to create device related seq_file

The ath patches were made to provide example of using the new helper
function and get some idea about code savings. Greg and John discussed
who would take them. I noticed other ath changes in net-next so I kinda
expected this email ;-)

Regards,
Arend
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


I just ran in to a problem with one of these commits.

On an Acer C720 laptop if a suspend is performed the screen freezes,
the machine locks up, and according to the indicator lights it does
not enter suspend.  A hard reset is required to get it running again.

I have bisected the kernel and found that the following is the first bad
commit.

   commit d32394fae95741d733b174ec1446f27765f80233
   Author: Arend van Spriel
   Date:   Sun Nov 9 11:32:00 2014 +0100

   ath: ath9k: use debugfs_create_devm_seqfile() helper for seq_file
   entries

   Use the helper to get rid of the file operations per debugfs file.
   The
   struct ath9k_softc pointer is set as device driver data to be
   obtained
   in the seq_file read operation.

   Signed-off-by: Arend van Spriel
   Signed-off-by: Greg Kroah-Hartman

Let me know if I can do anything else to help.

--
- Jeremiah Mahler


I took a look at the patch that is causing this problem (d32394fae95).
My config negates everything in the patch except for a one line change
to ath9k/pci.c.  If I remove this change (shown below) the problem goes
away.


Ok. But then it will likely crash when you cat one of the changed debugfs
files. Guess this commit needs to be reverted entirely.


What commit, d32394fae95741d733b174ec1446f27765f80233?


Indeed.

Regards,
Arend

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [patch] CodingStyle: add some more error handling guidelines

[Arend van Spriel]

On 12/03/14 17:00, SF Markus Elfring wrote:

Which name pattern do you find more appropriate in such
an use case?


I think Dan wants the label to be descriptive about the tasks
needed in the exception handling itself.


I would usually prefer also such a target-oriented labelling
for the affected identifiers.
How are the chances to express an expectation in this direction
unambiguously for the proposed coding style update?



This makes sense as the exception handling steps may be reused
for different failures in the code.


I would stress a different reason from my point of view.


I meant as apposed to using a goto-/source-oriented labelling. Please 
provide your point of view. That way the explanations given in this 
email exchange might be incorporated in the next round of the proposed 
update or at least be used as input.


Regards,
Arend
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [patch] CodingStyle: add some more error handling guidelines

[Arend van Spriel]

On 12/03/14 14:24, SF Markus Elfring wrote:

Sorry.  I misread your email.  If the code looks like this:

foo = kmalloc();
if (!foo)
goto kmalloc_failed;

The "kmalloc_failed" doesn't add any information.


I find that this such a name approach would fit to your
expectation of a source-oriented labeling of these identifiers.



We can see that kmalloc failed from the context.


Which name pattern do you find more appropriate in such
an use case?


I think Dan wants the label to be descriptive about the tasks needed in 
the exception handling itself. This makes sense as the exception 
handling steps may be reused for different failures in the code.


void foo(void)
{
if (check_a())
goto do_bar;

sub_foo1();

if (checck_b())
goto do_bar;

sub_foo2();
return;

do_bar:
bar();
}

Regards,
Arend


Regards,
Markus
--
To unsubscribe from this list: send the line "unsubscribe backports" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: linux-next: manual merge of the driver-core tree with the net-next tree

[Arend van Spriel]

On 12/03/14 11:51, Jeremiah Mahler wrote:

On Wed, Dec 03, 2014 at 12:36:55AM -0800, Jeremiah Mahler wrote:

all,

On Mon, Dec 01, 2014 at 08:34:05AM +0100, Arend van Spriel wrote:

On 01-12-14 08:19, Stephen Rothwell wrote:

Hi Greg,

Today's linux-next merge of the driver-core tree got a conflict in
drivers/net/wireless/ath/ath9k/debug.c between commits 70e535ed0029
("ath9k: clean up debugfs print of reset causes"), 7b8aaead958e
("ath9k: restart hardware after noise floor calibration failure") and
325e18817668 ("ath9k: fix misc debugfs when not using chan context")
from the net-next tree and commit 631bee257bd5 ("ath: use seq_file api
for ath9k debugfs files") from the driver-core tree.

I fixed it up (see below) and can carry the fix as necessary (no action
is required).

Greg, I am not sure why those 2 commits are even in your tree.  Do they
depend on something else in your tree?


They do. The three commits below are related:

d32394f ath: ath9k: use debugfs_create_devm_seqfile() helper for
seq_file entrie
631bee2 ath: use seq_file api for ath9k debugfs files
98210b7 debugfs: add helper function to create device related seq_file

The ath patches were made to provide example of using the new helper
function and get some idea about code savings. Greg and John discussed
who would take them. I noticed other ath changes in net-next so I kinda
expected this email ;-)

Regards,
Arend
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


I just ran in to a problem with one of these commits.

On an Acer C720 laptop if a suspend is performed the screen freezes,
the machine locks up, and according to the indicator lights it does
not enter suspend.  A hard reset is required to get it running again.

I have bisected the kernel and found that the following is the first bad
commit.

   commit d32394fae95741d733b174ec1446f27765f80233
   Author: Arend van Spriel
   Date:   Sun Nov 9 11:32:00 2014 +0100

   ath: ath9k: use debugfs_create_devm_seqfile() helper for seq_file
   entries

   Use the helper to get rid of the file operations per debugfs file.
   The
   struct ath9k_softc pointer is set as device driver data to be
   obtained
   in the seq_file read operation.

   Signed-off-by: Arend van Spriel
   Signed-off-by: Greg Kroah-Hartman

Let me know if I can do anything else to help.

--
- Jeremiah Mahler


I took a look at the patch that is causing this problem (d32394fae95).
My config negates everything in the patch except for a one line change
to ath9k/pci.c.  If I remove this change (shown below) the problem goes
away.


Ok. But then it will likely crash when you cat one of the changed 
debugfs files. Guess this commit needs to be reverted entirely.


Regards,
Arend


diff --git a/drivers/net/wireless/ath/ath9k/pci.c
b/drivers/net/wireless/ath/ath9k/pci.c
index 90c9e3c..c018dea 100644
--- a/drivers/net/wireless/ath/ath9k/pci.c
+++ b/drivers/net/wireless/ath/ath9k/pci.c
@@ -856,7 +856,6 @@ static int ath_pci_probe(struct pci_dev *pdev, const
struct pci_device_id *id)
 sc = hw->priv;
 sc->hw = hw;
 sc->dev =&pdev->dev;
-   dev_set_drvdata(sc->dev, sc);
 sc->mem = pcim_iomap_table(pdev)[0];
 sc->driver_data = id->driver_data;



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [patch] CodingStyle: add some more error handling guidelines

[Arend van Spriel]

On 12/03/14 13:31, SF Markus Elfring wrote:

diff --git a/Documentation/CodingStyle b/Documentation/CodingStyle
index 9f28b14..9c8a234 100644
--- a/Documentation/CodingStyle
+++ b/Documentation/CodingStyle
@@ -392,7 +392,12 @@ The goto statement comes in handy when a function exits 
from multiple
  locations and some common work such as cleanup has to be done.  If there is no
  cleanup needed then just return directly.

-The rationale is:
+Choose label names which say what the goto does or why the goto exists.  An
+[...]  Avoid
+using GW-BASIC names like "err1:" and "err2:".  Also don't name them after the
+goto location like "err_kmalloc_failed:"


I find this documentation approach not safe and clear enough so far.

* How should the reference to an other programming language help in the 
understanding
   of the recommended naming convention for jump labels?

* To which source code place should the word "location" refer to?
   - jump source
   - jump target


I think you digested the paragraph in too small bits. The term "goto 
location" looks synonymous to "jump source" to me.


Regards,
Arend


Regards,
Markus
--
To unsubscribe from this list: send the line "unsubscribe backports" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: linux-next: build warnings after merge of the driver-core tree

[Arend van Spriel]

On 01-12-14 08:42, Stephen Rothwell wrote:
> Hi Greg,
> 
> After merging the driver-core tree, today's linux-next build (arm
> multi_v7_defconfig) produced this warning:
> 
> In file included from kernel/power/main.c:16:0:
> include/linux/debugfs.h:105:10: warning: 'struct device' declared inside 
> parameter list
>   void *data));
>   ^
> 
> And many more like this.
> 
> Introduced by commit 98210b7f73f1 ("debugfs: add helper function to
> create device related seq_file").  See Rule 1 from
> Documentation/SubmitChecklist - though in this case a simple "struct
> device;" would probably do.
> 

I submitted a patch for that yesterday [1] and got (automated) email
from Greg that it is going into his driver-core tree.

Regards,
Arend

[1] http://mid.gmane.org/1417361481-1136-1-git-send-email-ar...@broadcom.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: linux-next: manual merge of the driver-core tree with the net-next tree

[Arend van Spriel]

On 01-12-14 08:19, Stephen Rothwell wrote:
> Hi Greg,
> 
> Today's linux-next merge of the driver-core tree got a conflict in
> drivers/net/wireless/ath/ath9k/debug.c between commits 70e535ed0029
> ("ath9k: clean up debugfs print of reset causes"), 7b8aaead958e
> ("ath9k: restart hardware after noise floor calibration failure") and
> 325e18817668 ("ath9k: fix misc debugfs when not using chan context")
> from the net-next tree and commit 631bee257bd5 ("ath: use seq_file api
> for ath9k debugfs files") from the driver-core tree.
> 
> I fixed it up (see below) and can carry the fix as necessary (no action
> is required).
> 
> Greg, I am not sure why those 2 commits are even in your tree.  Do they
> depend on something else in your tree?

They do. The three commits below are related:

d32394f ath: ath9k: use debugfs_create_devm_seqfile() helper for
seq_file entrie
631bee2 ath: use seq_file api for ath9k debugfs files
98210b7 debugfs: add helper function to create device related seq_file

The ath patches were made to provide example of using the new helper
function and get some idea about code savings. Greg and John discussed
who would take them. I noticed other ath changes in net-next so I kinda
expected this email ;-)

Regards,
Arend
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] fs: debugfs: add forward declaration for struct device type

[Arend van Spriel]

The function debugfs_create_devm_seqfile() has a parameter of type
struct device pointer. This type needs to be forward declared to
avoid compilation issues on certain architectures and/or kernel
configurations. The function was introduced with:

  commit 98210b7f73f1db182bd9a558a031093cd166e907
  Author: Arend van Spriel 
  Date:   Sun Nov 9 11:31:58 2014 +0100

 debugfs: add helper function to create device related seq_file

The reported build failure for sparc64 architecture was:

  make.cross ARCH=sparc64

All warnings:

   In file included from fs/debugfs/file.c:21:0:
   include/linux/debugfs.h:105:10: warning: 'struct device' declared
   inside parameter list
 void *data));
 ^

Reported-by: Fengguang Wu 
Signed-off-by: Arend van Spriel 
---
 include/linux/debugfs.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/linux/debugfs.h b/include/linux/debugfs.h
index 4bbe2ac..e3e9f31 100644
--- a/include/linux/debugfs.h
+++ b/include/linux/debugfs.h
@@ -20,6 +20,7 @@
 
 #include 
 
+struct device;
 struct file_operations;
 
 struct debugfs_blob_wrapper {
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: bluetooth related firmware loader spew on resume.

[Arend van Spriel]

On 11/27/14 10:29, Arend van Spriel wrote:

On 11/27/14 10:17, Takashi Iwai wrote:

At Thu, 27 Nov 2014 09:59:12 +0100,
Arend van Spriel wrote:


On 11/26/14 19:13, Takashi Iwai wrote:

At Wed, 26 Nov 2014 18:42:46 +0100,
Arend van Spriel wrote:


On 11/26/14 16:27, Takashi Iwai wrote:

At Wed, 26 Nov 2014 17:26:15 +0200,
Mihai Donțu wrote:


On Wed, 26 Nov 2014 16:19:49 +0100
Takashi Iwai wrote:


At Wed, 26 Nov 2014 16:56:09 +0200,
Mihai Donțu wrote:


On Tue, 11 Nov 2014 13:12:28 -0500 Dave Jones wrote:

Since the addition of 10d4c6736ea "Bluetooth: btusb: Add
Broadcom patch
RAM support", I (and a number of other people[*]) have been
seeing
this trace on resume from suspend.

WARNING: CPU: 1 PID: 8565 at
drivers/base/firmware_class.c:1127
_request_firmware+0x4c1/0x7c0()
CPU: 1 PID: 8565 Comm: kworker/u17:0 Not tainted
3.17.2-200.fc20.x86_64 #1
Hardware name: LENOVO 2356JK8/2356JK8, BIOS G7ET94WW (2.54 )
04/30/2013
Workqueue: hci0 hci_power_on [bluetooth]
 f52a564b 8800a8c63be8
817271cc
 8800a8c63c20 81094ced
8800a8c63d10
8801365ddf00 8801387b4b00 8800a8c63d08
fff5
Call Trace:
[] dump_stack+0x45/0x56
[] warn_slowpath_common+0x7d/0xa0
[] warn_slowpath_null+0x1a/0x20
[] _request_firmware+0x4c1/0x7c0
[] ? snprintf+0x49/0x70
[] request_firmware+0x31/0x50
[] btusb_setup_bcm_patchram+0x83/0x550 [btusb]
[] ? rpm_idle+0xd6/0x2b0
[] hci_dev_do_open+0xe1/0xa60 [bluetooth]
ACPI: \_SB_.PCI0.LPC_.EC__.BAT1: docking
Restarting tasks ...
[] ? ttwu_do_activate.constprop.90+0x5d/0x70
[] hci_power_on+0x40/0x1e0 [bluetooth]
[] ? lock_timer_base.isra.34+0x2b/0x50
[] process_one_work+0x149/0x3d0
[] worker_thread+0x11b/0x490
[] ? rescuer_thread+0x2e0/0x2e0
[] kthread+0xd8/0xf0
[] ? kthread_create_on_node+0x190/0x190
[] ret_from_fork+0x7c/0xb0
[] ? kthread_create_on_node+0x190/0x190
---[ end trace 75a0e9c7f33ebb4c ]---
bluetooth hci0: firmware: brcm/BCM20702A0-0a5c-21e6.hcd will
not be loaded
Bluetooth: hci0: BCM: patch brcm/BCM20702A0-0a5c-21e6.hcd not
found


At first I thought it was just over-reaction to the file being
missing, but
looking at the WARN_ON, it appears that we're trying to invoke
the firmware
loader before userspace is back up ?

In this (and probably other related) kernel,
CONFIG_FW_LOADER_USER_HELPER is unset,
in case that matters at all.

Dave

[*] https://bugzilla.kernel.org/show_bug.cgi?id=81821
https://bugzilla.redhat.com/show_bug.cgi?id=1133378


I have the following during normal boot:

[ 5.620796] Bluetooth: hci0: read Intel version:
370710018002030d00
[ 5.620822] bluetooth hci0: Direct firmware load for
intel/ibt-hw-37.7.10-fw-1.80.2.3.d.bseq failed with error -2
[ 5.620827] Bluetooth: hci0 failed to open Intel firmware file:
intel/ibt-hw-37.7.10-fw-1.80.2.3.d.bseq(-2)
[ 5.620920] bluetooth hci0: Direct firmware load for
intel/ibt-hw-37.7.bseq failed with error -2
[ 5.620922] Bluetooth: hci0 failed to open default Intel fw
file: intel/ibt-hw-37.7.bseq
[ 5.629910] EXT4-fs (sda2): mounted filesystem with ordered
data mode. Opts: (null)
[ 5.629916] VFS: Mounted root (ext4 filesystem) readonly on
device 8:2.

The driver is trying to load the firmware before root is
mounted. Do I
really need an initramfs?


If btusb driver is loaded in initrd, you'd need the corresponding
firmware in initrd, too.


The driver is built into the kernel and I don't use an initrd. I
could
probably create one, but it's a bit tricky with UEFI and a tad
harder
to maintain.


Then you can build the firmware file into kernel, too.


huh? The whole idea of the firmware API was to keep (often
proprietary)
firmware out of the kernel. Has that strategy been abandoned recently?


See CONFIG_EXTRA_FIRMARE. It doesn't mean to include the binary blob
into the kernel source tree. It just allows to *build* into your
kernel.


So I looked at this Kconfig option and reading the help I came across 
this warning:


"""
WARNING: If you include additional firmware files into your binary 
kernel image that are not available under the terms of the GPL, 
then it may be a violation of the GPL to distribute the resulting 
image since it combines both GPL and non-GPL work. You should 
consult a lawyer of your own before distributing such an image.

"""

This is exactly what I meant, by "(often proprietary) firmware". So we 
should conclude that if a device needs proprietary firmware it can not 
be built-in the kernel if intended for distribution.


Regards,
Arend


I see. Thanks for the info. I still do not understand the resume
scenario. I though firmware api did some sort of caching of the firmware
images.


My theory is that this is triggered when the firmware file doesn't
exist. Then it's neither remembered nor cached, so it's retried in
the resume path. But the information is missing, so I cannot say
surely about it.


Agree. So the same warning should have o

Re: bluetooth related firmware loader spew on resume.

[Arend van Spriel]

On 11/27/14 10:17, Takashi Iwai wrote:

At Thu, 27 Nov 2014 09:59:12 +0100,
Arend van Spriel wrote:


On 11/26/14 19:13, Takashi Iwai wrote:

At Wed, 26 Nov 2014 18:42:46 +0100,
Arend van Spriel wrote:


On 11/26/14 16:27, Takashi Iwai wrote:

At Wed, 26 Nov 2014 17:26:15 +0200,
Mihai Donțu wrote:


On Wed, 26 Nov 2014 16:19:49 +0100
Takashi Iwaiwrote:


At Wed, 26 Nov 2014 16:56:09 +0200,
Mihai Donțu wrote:


On Tue, 11 Nov 2014 13:12:28 -0500 Dave Jones wrote:

Since the addition of 10d4c6736ea "Bluetooth: btusb: Add Broadcom patch
RAM support", I (and a number of other people[*]) have been seeing
this trace on resume from suspend.

WARNING: CPU: 1 PID: 8565 at drivers/base/firmware_class.c:1127 
_request_firmware+0x4c1/0x7c0()
CPU: 1 PID: 8565 Comm: kworker/u17:0 Not tainted 3.17.2-200.fc20.x86_64 #1
Hardware name: LENOVO 2356JK8/2356JK8, BIOS G7ET94WW (2.54 ) 04/30/2013
Workqueue: hci0 hci_power_on [bluetooth]
 f52a564b 8800a8c63be8 817271cc
 8800a8c63c20 81094ced 8800a8c63d10
8801365ddf00 8801387b4b00 8800a8c63d08 fff5
Call Trace:
[] dump_stack+0x45/0x56
[] warn_slowpath_common+0x7d/0xa0
[] warn_slowpath_null+0x1a/0x20
[] _request_firmware+0x4c1/0x7c0
[] ? snprintf+0x49/0x70
[] request_firmware+0x31/0x50
[] btusb_setup_bcm_patchram+0x83/0x550 [btusb]
[] ? rpm_idle+0xd6/0x2b0
[] hci_dev_do_open+0xe1/0xa60 [bluetooth]
ACPI: \_SB_.PCI0.LPC_.EC__.BAT1: docking
Restarting tasks ...
[] ? ttwu_do_activate.constprop.90+0x5d/0x70
[] hci_power_on+0x40/0x1e0 [bluetooth]
[] ? lock_timer_base.isra.34+0x2b/0x50
[] process_one_work+0x149/0x3d0
[] worker_thread+0x11b/0x490
[] ? rescuer_thread+0x2e0/0x2e0
[] kthread+0xd8/0xf0
[] ? kthread_create_on_node+0x190/0x190
[] ret_from_fork+0x7c/0xb0
[] ? kthread_create_on_node+0x190/0x190
---[ end trace 75a0e9c7f33ebb4c ]---
bluetooth hci0: firmware: brcm/BCM20702A0-0a5c-21e6.hcd will not be loaded
Bluetooth: hci0: BCM: patch brcm/BCM20702A0-0a5c-21e6.hcd not found


At first I thought it was just over-reaction to the file being missing, but
looking at the WARN_ON, it appears that we're trying to invoke the firmware
loader before userspace is back up ?

In this (and probably other related) kernel, CONFIG_FW_LOADER_USER_HELPER is 
unset,
in case that matters at all.

Dave

[*] https://bugzilla.kernel.org/show_bug.cgi?id=81821
   https://bugzilla.redhat.com/show_bug.cgi?id=1133378


I have the following during normal boot:

[5.620796] Bluetooth: hci0: read Intel version: 370710018002030d00
[5.620822] bluetooth hci0: Direct firmware load for 
intel/ibt-hw-37.7.10-fw-1.80.2.3.d.bseq failed with error -2
[5.620827] Bluetooth: hci0 failed to open Intel firmware file: 
intel/ibt-hw-37.7.10-fw-1.80.2.3.d.bseq(-2)
[5.620920] bluetooth hci0: Direct firmware load for intel/ibt-hw-37.7.bseq 
failed with error -2
[5.620922] Bluetooth: hci0 failed to open default Intel fw file: 
intel/ibt-hw-37.7.bseq
[5.629910] EXT4-fs (sda2): mounted filesystem with ordered data mode. Opts: 
(null)
[5.629916] VFS: Mounted root (ext4 filesystem) readonly on device 8:2.

The driver is trying to load the firmware before root is mounted. Do I
really need an initramfs?


If btusb driver is loaded in initrd, you'd need the corresponding
firmware in initrd, too.


The driver is built into the kernel and I don't use an initrd. I could
probably create one, but it's a bit tricky with UEFI and a tad harder
to maintain.


Then you can build the firmware file into kernel, too.


huh? The whole idea of the firmware API was to keep (often proprietary)
firmware out of the kernel. Has that strategy been abandoned recently?


See CONFIG_EXTRA_FIRMARE.  It doesn't mean to include the binary blob
into the kernel source tree.  It just allows to *build* into your
kernel.


I see. Thanks for the info. I still do not understand the resume
scenario. I though firmware api did some sort of caching of the firmware
images.


My theory is that this is triggered when the firmware file doesn't
exist.  Then it's neither remembered nor cached, so it's retried in
the resume path.  But the information is missing, so I cannot say
surely about it.


Agree. So the same warning should have occurred upon system boot. Maybe 
Mihai can confirm that.


Regards,
Arend



Takashi


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: bluetooth related firmware loader spew on resume.

[Arend van Spriel]

On 11/26/14 19:13, Takashi Iwai wrote:

At Wed, 26 Nov 2014 18:42:46 +0100,
Arend van Spriel wrote:


On 11/26/14 16:27, Takashi Iwai wrote:

At Wed, 26 Nov 2014 17:26:15 +0200,
Mihai Donțu wrote:


On Wed, 26 Nov 2014 16:19:49 +0100
Takashi Iwai   wrote:


At Wed, 26 Nov 2014 16:56:09 +0200,
Mihai Donțu wrote:


On Tue, 11 Nov 2014 13:12:28 -0500 Dave Jones wrote:

Since the addition of 10d4c6736ea "Bluetooth: btusb: Add Broadcom patch
RAM support", I (and a number of other people[*]) have been seeing
this trace on resume from suspend.

WARNING: CPU: 1 PID: 8565 at drivers/base/firmware_class.c:1127 
_request_firmware+0x4c1/0x7c0()
CPU: 1 PID: 8565 Comm: kworker/u17:0 Not tainted 3.17.2-200.fc20.x86_64 #1
Hardware name: LENOVO 2356JK8/2356JK8, BIOS G7ET94WW (2.54 ) 04/30/2013
Workqueue: hci0 hci_power_on [bluetooth]
 f52a564b 8800a8c63be8 817271cc
 8800a8c63c20 81094ced 8800a8c63d10
8801365ddf00 8801387b4b00 8800a8c63d08 fff5
Call Trace:
[] dump_stack+0x45/0x56
[] warn_slowpath_common+0x7d/0xa0
[] warn_slowpath_null+0x1a/0x20
[] _request_firmware+0x4c1/0x7c0
[] ? snprintf+0x49/0x70
[] request_firmware+0x31/0x50
[] btusb_setup_bcm_patchram+0x83/0x550 [btusb]
[] ? rpm_idle+0xd6/0x2b0
[] hci_dev_do_open+0xe1/0xa60 [bluetooth]
ACPI: \_SB_.PCI0.LPC_.EC__.BAT1: docking
Restarting tasks ...
[] ? ttwu_do_activate.constprop.90+0x5d/0x70
[] hci_power_on+0x40/0x1e0 [bluetooth]
[] ? lock_timer_base.isra.34+0x2b/0x50
[] process_one_work+0x149/0x3d0
[] worker_thread+0x11b/0x490
[] ? rescuer_thread+0x2e0/0x2e0
[] kthread+0xd8/0xf0
[] ? kthread_create_on_node+0x190/0x190
[] ret_from_fork+0x7c/0xb0
[] ? kthread_create_on_node+0x190/0x190
---[ end trace 75a0e9c7f33ebb4c ]---
bluetooth hci0: firmware: brcm/BCM20702A0-0a5c-21e6.hcd will not be loaded
Bluetooth: hci0: BCM: patch brcm/BCM20702A0-0a5c-21e6.hcd not found


At first I thought it was just over-reaction to the file being missing, but
looking at the WARN_ON, it appears that we're trying to invoke the firmware
loader before userspace is back up ?

In this (and probably other related) kernel, CONFIG_FW_LOADER_USER_HELPER is 
unset,
in case that matters at all.

Dave

[*] https://bugzilla.kernel.org/show_bug.cgi?id=81821
  https://bugzilla.redhat.com/show_bug.cgi?id=1133378


I have the following during normal boot:

[5.620796] Bluetooth: hci0: read Intel version: 370710018002030d00
[5.620822] bluetooth hci0: Direct firmware load for 
intel/ibt-hw-37.7.10-fw-1.80.2.3.d.bseq failed with error -2
[5.620827] Bluetooth: hci0 failed to open Intel firmware file: 
intel/ibt-hw-37.7.10-fw-1.80.2.3.d.bseq(-2)
[5.620920] bluetooth hci0: Direct firmware load for intel/ibt-hw-37.7.bseq 
failed with error -2
[5.620922] Bluetooth: hci0 failed to open default Intel fw file: 
intel/ibt-hw-37.7.bseq
[5.629910] EXT4-fs (sda2): mounted filesystem with ordered data mode. Opts: 
(null)
[5.629916] VFS: Mounted root (ext4 filesystem) readonly on device 8:2.

The driver is trying to load the firmware before root is mounted. Do I
really need an initramfs?


If btusb driver is loaded in initrd, you'd need the corresponding
firmware in initrd, too.


The driver is built into the kernel and I don't use an initrd. I could
probably create one, but it's a bit tricky with UEFI and a tad harder
to maintain.


Then you can build the firmware file into kernel, too.


huh? The whole idea of the firmware API was to keep (often proprietary)
firmware out of the kernel. Has that strategy been abandoned recently?


See CONFIG_EXTRA_FIRMARE.  It doesn't mean to include the binary blob
into the kernel source tree.  It just allows to *build* into your
kernel.


I see. Thanks for the info. I still do not understand the resume 
scenario. I though firmware api did some sort of caching of the firmware 
images.


Regards,
Arend
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: bluetooth related firmware loader spew on resume.

[Arend van Spriel]

On 11/26/14 16:27, Takashi Iwai wrote:

At Wed, 26 Nov 2014 17:26:15 +0200,
Mihai Donțu wrote:


On Wed, 26 Nov 2014 16:19:49 +0100
Takashi Iwai  wrote:


At Wed, 26 Nov 2014 16:56:09 +0200,
Mihai Donțu wrote:


On Tue, 11 Nov 2014 13:12:28 -0500 Dave Jones wrote:

Since the addition of 10d4c6736ea "Bluetooth: btusb: Add Broadcom patch
RAM support", I (and a number of other people[*]) have been seeing
this trace on resume from suspend.

WARNING: CPU: 1 PID: 8565 at drivers/base/firmware_class.c:1127 
_request_firmware+0x4c1/0x7c0()
CPU: 1 PID: 8565 Comm: kworker/u17:0 Not tainted 3.17.2-200.fc20.x86_64 #1
Hardware name: LENOVO 2356JK8/2356JK8, BIOS G7ET94WW (2.54 ) 04/30/2013
Workqueue: hci0 hci_power_on [bluetooth]
 f52a564b 8800a8c63be8 817271cc
 8800a8c63c20 81094ced 8800a8c63d10
8801365ddf00 8801387b4b00 8800a8c63d08 fff5
Call Trace:
[] dump_stack+0x45/0x56
[] warn_slowpath_common+0x7d/0xa0
[] warn_slowpath_null+0x1a/0x20
[] _request_firmware+0x4c1/0x7c0
[] ? snprintf+0x49/0x70
[] request_firmware+0x31/0x50
[] btusb_setup_bcm_patchram+0x83/0x550 [btusb]
[] ? rpm_idle+0xd6/0x2b0
[] hci_dev_do_open+0xe1/0xa60 [bluetooth]
ACPI: \_SB_.PCI0.LPC_.EC__.BAT1: docking
Restarting tasks ...
[] ? ttwu_do_activate.constprop.90+0x5d/0x70
[] hci_power_on+0x40/0x1e0 [bluetooth]
[] ? lock_timer_base.isra.34+0x2b/0x50
[] process_one_work+0x149/0x3d0
[] worker_thread+0x11b/0x490
[] ? rescuer_thread+0x2e0/0x2e0
[] kthread+0xd8/0xf0
[] ? kthread_create_on_node+0x190/0x190
[] ret_from_fork+0x7c/0xb0
[] ? kthread_create_on_node+0x190/0x190
---[ end trace 75a0e9c7f33ebb4c ]---
bluetooth hci0: firmware: brcm/BCM20702A0-0a5c-21e6.hcd will not be loaded
Bluetooth: hci0: BCM: patch brcm/BCM20702A0-0a5c-21e6.hcd not found


At first I thought it was just over-reaction to the file being missing, but
looking at the WARN_ON, it appears that we're trying to invoke the firmware
loader before userspace is back up ?

In this (and probably other related) kernel, CONFIG_FW_LOADER_USER_HELPER is 
unset,
in case that matters at all.

Dave

[*] https://bugzilla.kernel.org/show_bug.cgi?id=81821
 https://bugzilla.redhat.com/show_bug.cgi?id=1133378


I have the following during normal boot:

[5.620796] Bluetooth: hci0: read Intel version: 370710018002030d00
[5.620822] bluetooth hci0: Direct firmware load for 
intel/ibt-hw-37.7.10-fw-1.80.2.3.d.bseq failed with error -2
[5.620827] Bluetooth: hci0 failed to open Intel firmware file: 
intel/ibt-hw-37.7.10-fw-1.80.2.3.d.bseq(-2)
[5.620920] bluetooth hci0: Direct firmware load for intel/ibt-hw-37.7.bseq 
failed with error -2
[5.620922] Bluetooth: hci0 failed to open default Intel fw file: 
intel/ibt-hw-37.7.bseq
[5.629910] EXT4-fs (sda2): mounted filesystem with ordered data mode. Opts: 
(null)
[5.629916] VFS: Mounted root (ext4 filesystem) readonly on device 8:2.

The driver is trying to load the firmware before root is mounted. Do I
really need an initramfs?


If btusb driver is loaded in initrd, you'd need the corresponding
firmware in initrd, too.


The driver is built into the kernel and I don't use an initrd. I could
probably create one, but it's a bit tricky with UEFI and a tad harder
to maintain.


Then you can build the firmware file into kernel, too.


huh? The whole idea of the firmware API was to keep (often proprietary) 
firmware out of the kernel. Has that strategy been abandoned recently?


Regards,
Arend


Takashi
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/1] net: brcm80211: Deletion of unnecessary checks before two function calls

[Arend van Spriel]

On 11/20/14 16:50, SF Markus Elfring wrote:

From: Markus Elfring
Date: Thu, 20 Nov 2014 16:42:51 +0100

The functions brcmu_pkt_buf_free_skb() and release_firmware() test whether
their argument is NULL and then return immediately. Thus the test around
the call is not needed.

This issue was detected by using the Coccinelle software.


Goodo for coccinelle and you for running it.

Acked-by: Arend van Spriel 

Signed-off-by: Markus Elfring
---
  drivers/net/wireless/brcm80211/brcmfmac/dhd_sdio.c | 3 +--
  drivers/net/wireless/brcm80211/brcmfmac/firmware.c | 3 +--
  drivers/net/wireless/brcm80211/brcmfmac/msgbuf.c   | 3 +--
  drivers/net/wireless/brcm80211/brcmsmac/main.c | 3 +--
  4 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/net/wireless/brcm80211/brcmfmac/dhd_sdio.c 
b/drivers/net/wireless/brcm80211/brcmfmac/dhd_sdio.c
index f55f625..8ff7037 100644
--- a/drivers/net/wireless/brcm80211/brcmfmac/dhd_sdio.c
+++ b/drivers/net/wireless/brcm80211/brcmfmac/dhd_sdio.c
@@ -2539,8 +2539,7 @@ static void brcmf_sdio_bus_stop(struct device *dev)
brcmu_pktq_flush(&bus->txq, true, NULL, NULL);

/* Clear any held glomming stuff */
-   if (bus->glomd)
-   brcmu_pkt_buf_free_skb(bus->glomd);
+   brcmu_pkt_buf_free_skb(bus->glomd);
brcmf_sdio_free_glom(bus);

/* Clear rx control and wake any waiters */
diff --git a/drivers/net/wireless/brcm80211/brcmfmac/firmware.c 
b/drivers/net/wireless/brcm80211/brcmfmac/firmware.c
index 8ea9f28..3a2d014 100644
--- a/drivers/net/wireless/brcm80211/brcmfmac/firmware.c
+++ b/drivers/net/wireless/brcm80211/brcmfmac/firmware.c
@@ -262,8 +262,7 @@ static void brcmf_fw_request_nvram_done(const struct 
firmware *fw, void *ctx)

  fail:
brcmf_dbg(TRACE, "failed: dev=%s\n", dev_name(fwctx->dev));
-   if (fwctx->code)
-   release_firmware(fwctx->code);
+   release_firmware(fwctx->code);
device_release_driver(fwctx->dev);
kfree(fwctx);
  }
diff --git a/drivers/net/wireless/brcm80211/brcmfmac/msgbuf.c 
b/drivers/net/wireless/brcm80211/brcmfmac/msgbuf.c
index 8f8b937..0cb00dc 100644
--- a/drivers/net/wireless/brcm80211/brcmfmac/msgbuf.c
+++ b/drivers/net/wireless/brcm80211/brcmfmac/msgbuf.c
@@ -506,8 +506,7 @@ static int brcmf_msgbuf_query_dcmd(struct brcmf_pub *drvr, 
int ifidx,
memcpy(buf, skb->data, (len<  msgbuf->ioctl_resp_ret_len) ?
   len : msgbuf->ioctl_resp_ret_len);
}
-   if (skb)
-   brcmu_pkt_buf_free_skb(skb);
+   brcmu_pkt_buf_free_skb(skb);

return msgbuf->ioctl_resp_status;
  }
diff --git a/drivers/net/wireless/brcm80211/brcmsmac/main.c 
b/drivers/net/wireless/brcm80211/brcmsmac/main.c
index 1b47482..ce538a1 100644
--- a/drivers/net/wireless/brcm80211/brcmsmac/main.c
+++ b/drivers/net/wireless/brcm80211/brcmsmac/main.c
@@ -1009,8 +1009,7 @@ brcms_c_dotxstatus(struct brcms_c_info *wlc, struct 
tx_status *txs)
if (txh)
trace_brcms_txdesc(&wlc->hw->d11core->dev, txh,
   sizeof(*txh));
-   if (p)
-   brcmu_pkt_buf_free_skb(p);
+   brcmu_pkt_buf_free_skb(p);
}

if (dma&&  queue<  NFIFO) {


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] brcmfmac: fix error handling of irq_of_parse_and_map

[Arend van Spriel]

On 11/14/14 23:12, Dmitry Torokhov wrote:

Return value of irq_of_parse_and_map() is unsigned int, with 0
indicating failure, so testing for negative result never works.


Whoops, that is bad. Thanks for catching this. It probably needs to go 
to stable as well for 3.17 kernel.


+Cc: sta...@vger.kernel.org # v3.17
+Acked-by: Arend van Spriel 

Signed-off-by: Dmitry Torokhov
---

Not tested, found by casual code inspection.

  drivers/net/wireless/brcm80211/brcmfmac/of.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/brcm80211/brcmfmac/of.c 
b/drivers/net/wireless/brcm80211/brcmfmac/of.c
index eb3fce82..c824570 100644
--- a/drivers/net/wireless/brcm80211/brcmfmac/of.c
+++ b/drivers/net/wireless/brcm80211/brcmfmac/of.c
@@ -40,8 +40,8 @@ void brcmf_of_probe(struct brcmf_sdio_dev *sdiodev)
return;

irq = irq_of_parse_and_map(np, 0);
-   if (irq<  0) {
-   brcmf_err("interrupt could not be mapped: err=%d\n", irq);
+   if (!irq) {
+   brcmf_err("interrupt could not be mapped\n");
devm_kfree(dev, sdiodev->pdata);
return;
}


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] brcmfmac: kill URB when request timed out

[Arend van Spriel]

On 13-11-14 03:33, Mathy Vanhoef wrote:
> Kill the submitted URB in brcmf_usb_dl_cmd if the request timed out. This
> assures the URB is never submitted twice. It also prevents a possible
> use-after-free of the URB transfer buffer if a timeout occurs.
> 
Acked-by: Arend van Spriel 
> Signed-off-by: Mathy Vanhoef 
> ---
> For a discussion about this patch and the underlying problem, see the mails
> with as subject "[PATCH] brcmfmac: unlink URB when request timed out".
> 
>  drivers/net/wireless/brcm80211/brcmfmac/usb.c |6 --
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/wireless/brcm80211/brcmfmac/usb.c 
> b/drivers/net/wireless/brcm80211/brcmfmac/usb.c
> index 5265aa7..4572def 100644
> --- a/drivers/net/wireless/brcm80211/brcmfmac/usb.c
> +++ b/drivers/net/wireless/brcm80211/brcmfmac/usb.c
> @@ -738,10 +738,12 @@ static int brcmf_usb_dl_cmd(struct brcmf_usbdev_info 
> *devinfo, u8 cmd,
>   goto finalize;
>   }
>  
> - if (!brcmf_usb_ioctl_resp_wait(devinfo))
> + if (!brcmf_usb_ioctl_resp_wait(devinfo)) {
> + usb_kill_urb(devinfo->ctl_urb);
>   ret = -ETIMEDOUT;
> - else
> + } else {
>   memcpy(buffer, tmpbuf, buflen);
> + }
>  
>  finalize:
>   kfree(tmpbuf);
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/1] linux-wireless: Added psk in struct cfg80211_connect_params needed for offloading 4way handshake to driver

[Arend van Spriel]

On 11-11-14 11:38, Johannes Berg wrote:
> On Tue, 2014-11-11 at 11:35 +0100, Arend van Spriel wrote:
> 
>> What did pop up is the wiphy flags vs. nl80211 feature flags. When that
>> comes up it looks like 'potAtoes, potaetoes' to me.
>>
>> So is there are clear design rule for when to use which flag. For me the
>> wiphy object represents the device/firmware and 4-way handshake offload
>> support is determined by what the device/firmware supports.
> 
> There are three types of flags:
> 
>  * wiphy flag attributes - deprecated as far as I'm concerned

Ok. deprecated is clear enough ;-)

>  * wiphy nl80211 feature flags - much easier to use in kernel (and
> userspace)
>  * nl80211 protocol flags - only one exists
> (NL80211_PROTOCOL_FEATURE_SPLIT_WIPHY_DUMP)

Thanks,
Arend

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/1] linux-wireless: Added psk in struct cfg80211_connect_params needed for offloading 4way handshake to driver

[Arend van Spriel]

On 11-11-14 11:03, Johannes Berg wrote:
> On Tue, 2014-11-11 at 10:54 +0100, Arend van Spriel wrote:
> 
>>> Also, there's a competing approach from QCA that's far more suited.
>>
>> I probably was not paying attention to it, but would you have a
>> reference to this.
> 
> ... digs around in email ...
> 
> http://mid.gmane.org/1343907187-6686-1-git-send-email-qca_vkond...@qca.qualcomm.com
> 
> Anyway, looking back at that, it wasn't really all that different, just
> a bit more complete maybe.

Read through the whole thread. It seems some comments from you needed to
be addressed and Vladimir wanted to evaluate it. So that was the end of
the thread.

What did pop up is the wiphy flags vs. nl80211 feature flags. When that
comes up it looks like 'potAtoes, potaetoes' to me.

So is there are clear design rule for when to use which flag. For me the
wiphy object represents the device/firmware and 4-way handshake offload
support is determined by what the device/firmware supports.

Regards,
Arend

> johannes
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/1] linux-wireless: Added psk in struct cfg80211_connect_params needed for offloading 4way handshake to driver

[Arend van Spriel]

On 11-11-14 10:29, Johannes Berg wrote:
> On Tue, 2014-11-11 at 05:56 +, Gautam (Gautam Kumar) Shukla wrote:
>> For offloading 4 way handshake to driver, currently we don't have any
>> member  of struct cfg80211_connect_params to pass PSK from supplicant
>> to driver. I have added psk for the same and added rest of the code
>> needed in nl80211.h and nl80211.c to parse and make it available to
>> driver.
>> From supplicant, we already have psk member field in assoc_params to
>> use .
>>
>> Tested on x86 linux.
> 
> Your commit message needs serious work.
> 
> Also, there's a competing approach from QCA that's far more suited.

I probably was not paying attention to it, but would you have a
reference to this.

> In either case though, I'm going to ask which driver is going to use
> this and when it's going to land in mainline.

I had the same question ;-)

> johannes
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/2] linux-wireless:Added wiphy capability flag for offloading 4way handshake to driver

[Arend van Spriel]

On 11-11-14 07:27, Gautam (Gautam Kumar) Shukla wrote:
> 
> For offloading 4 way handshake to driver , currently we don't have WIPHY 
> capability flag to communicate same to supplicant.
> I have added the flag NL80211_ATTR_4WAY_KEY_HANDSHAKE  and rest of the code 
> for the same.
> 
> Tested on x86 linux machine
> 
> Signed-off-by: Gautam kumar shukla 
> ---
>  include/net/cfg80211.h   | 4 
>  include/uapi/linux/nl80211.h | 5 -
>  net/wireless/nl80211.c   | 4 
>  3 files changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
> index 6f744e0..b37de0a 100644
> --- a/include/net/cfg80211.h
> +++ b/include/net/cfg80211.h
> @@ -2629,7 +2629,10 @@ struct cfg80211_ops {
>   *   TSPEC sessions (TID aka TSID 0-7) with the NL80211_CMD_ADD_TX_TS
>   *   command. Standard IEEE 802.11 TSPEC setup is not yet supported, it
>   *   needs to be able to handle Block-Ack agreements and other things.
> + *   @WIPHY_FLAG_SUPPORTS_4WAY_HANDHSHAKE:the device supports 4way handshake

Indentation seems wrong here. Also add space after the colon sign.

> + *   in the driver/firmware.
>   */
> +
>  enum wiphy_flags {
>   WIPHY_FLAG_SUPPORTS_WMM_ADMISSION   = BIT(0),
>   /* use hole at 1 */
> @@ -2654,6 +2657,7 @@ enum wiphy_flags {
>   WIPHY_FLAG_HAS_REMAIN_ON_CHANNEL= BIT(21),
>   WIPHY_FLAG_SUPPORTS_5_10_MHZ= BIT(22),
>   WIPHY_FLAG_HAS_CHANNEL_SWITCH   = BIT(23),
> + WIPHY_FLAG_SUPPORTS_4WAY_HANDSHAKE  = BIT(24),
>  };
>  
>  /**
> diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
> index b01d5dd..2c474a3 100644
> --- a/include/uapi/linux/nl80211.h
> +++ b/include/uapi/linux/nl80211.h
> @@ -1640,9 +1640,11 @@ enum nl80211_commands {
>   *
>   *   @NL80211_ATTR_PSK: a PSK value (u8 attribute).This is 32-octet (256-bit)
>   *   PSK.
> + * @NL80211_ATTR_4WAY_KEY_HANDSHAKE: Indicates whether the driver is capable
> + * of 4way key handshake

convention is to have a tab in the continuing line.

>   *
>   *
> - * @NL80211_ATTR_MAX: highest attribute number currently defined
> + * * @NL80211_ATTR_MAX: highest attribute number currently defined

huh?

>   * @__NL80211_ATTR_AFTER_LAST: internal use
>   */
>  enum nl80211_attrs {
> @@ -1995,6 +1997,7 @@ enum nl80211_attrs {
>   NL80211_ATTR_SMPS_MODE,
>  
>   NL80211_ATTR_PSK,
> + NL80211_ATTR_4WAY_KEY_HANDSHAKE,
>  
>   /* add attributes here, update the policy in nl80211.c */
>  
> diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
> index 91c24b1..5f85c41 100644
> --- a/net/wireless/nl80211.c
> +++ b/net/wireless/nl80211.c
> @@ -396,6 +396,7 @@ static const struct nla_policy 
> nl80211_policy[NL80211_ATTR_MAX+1] = {
>   [NL80211_ATTR_ADMITTED_TIME] = { .type = NLA_U16 },
>   [NL80211_ATTR_SMPS_MODE] = { .type = NLA_U8 },
>   [NL80211_ATTR_PSK] = { .type = NLA_BINARY, .len = 32 },
> + [NL80211_ATTR_4WAY_KEY_HANDSHAKE] = { .type = NLA_FLAG },
>  };
>  
>  /* policy for the key attributes */
> @@ -1303,6 +1304,9 @@ static int nl80211_send_wiphy(struct 
> cfg80211_registered_device *rdev,
>   if ((rdev->wiphy.flags & WIPHY_FLAG_SUPPORTS_FW_ROAM) &&
>   nla_put_flag(msg, NL80211_ATTR_ROAM_SUPPORT))
>   goto nla_put_failure;
> + if ((rdev->wiphy.flags & WIPHY_FLAG_SUPPORTS_4WAY_HANDSHAKE) &&
> + nla_put_flag(msg,NL80211_ATTR_4WAY_KEY_HANDSHAKE))
> + goto nla_put_failure;
>   if ((rdev->wiphy.flags & WIPHY_FLAG_SUPPORTS_TDLS) &&
>   nla_put_flag(msg, NL80211_ATTR_TDLS_SUPPORT))
>   goto nla_put_failure;
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/1] linux-wireless: Added psk in struct cfg80211_connect_params needed for offloading 4way handshake to driver

[Arend van Spriel]

On 11-11-14 06:56, Gautam (Gautam Kumar) Shukla wrote:
>

Hi Gautam,

Good to see more upstream contributions, but it might be useful to have
a driver implementation as well in this series. Maybe we can take a shot
with brcmfmac for obvious reasons. Would you happen to have
wpa_supplicant changes as well?

I added some inline comments below.

Regards,
Arend

> For offloading 4 way handshake to driver, currently we don't have any member  
> of struct cfg80211_connect_params to pass PSK from supplicant to driver. I 
> have added psk for the same and added rest of the code needed in nl80211.h 
> and nl80211.c to parse and make it available to driver.
> From supplicant, we already have psk member field in assoc_params to use .

In the commit message you should not describe what you did, but what
problem you are trying to solve and/or what functional change the patch
provides.

> Tested on x86 linux.
> 
> Signed-off-by: Gautam kumar shukla 
> ---
> include/net/cfg80211.h   | 2 ++
>  include/uapi/linux/nl80211.h | 8 +++-
>  net/wireless/nl80211.c   | 4 
>  3 files changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h index 
> a2ddcf2..6f744e0 100644
> --- a/include/net/cfg80211.h
> +++ b/include/net/cfg80211.h
> @@ -1758,6 +1758,7 @@ struct cfg80211_ibss_params {
>   *allowed to ignore this @bssid_hint if it has knowledge of a better BSS
>   *to use.
>   * @ssid: SSID
> + * @psk:preshared key for WPA2-PSK connection or %NULL if not specified

add space after the colon sign.

>   * @ssid_len: Length of ssid in octets
>   * @auth_type: Authentication type (algorithm)
>   * @ie: IEs for association request
> @@ -1783,6 +1784,7 @@ struct cfg80211_connect_params {
>  const u8 *bssid;
>  const u8 *bssid_hint;
>  const u8 *ssid;
> +const u8 *psk;
>  size_t ssid_len;
>  enum nl80211_auth_type auth_type;
>  const u8 *ie;
> diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h 
> index 4b28dc0..b01d5dd 100644
> --- a/include/uapi/linux/nl80211.h
> +++ b/include/uapi/linux/nl80211.h
> @@ -421,7 +421,7 @@
>   *%NL80211_ATTR_MAC, %NL80211_ATTR_WIPHY_FREQ, 
> %NL80211_ATTR_CONTROL_PORT,
>   *%NL80211_ATTR_CONTROL_PORT_ETHERTYPE,
>   *%NL80211_ATTR_CONTROL_PORT_NO_ENCRYPT, %NL80211_ATTR_MAC_HINT, and
> - *%NL80211_ATTR_WIPHY_FREQ_HINT.
> + *%NL80211_ATTR_WIPHY_FREQ_HINT, and %NL80211_ATTR_PSK.
>   *If included, %NL80211_ATTR_MAC and %NL80211_ATTR_WIPHY_FREQ are
>   *restrictions on BSS selection, i.e., they effectively prevent roaming
>   *within the ESS. %NL80211_ATTR_MAC_HINT and 
> %NL80211_ATTR_WIPHY_FREQ_HINT @@ -1638,6 +1638,10 @@ enum nl80211_commands {
>   * @NL80211_ATTR_SMPS_MODE: SMPS mode to use (ap mode). see
>   *&enum nl80211_smps_mode.
>   *
> + *@NL80211_ATTR_PSK: a PSK value (u8 attribute).This is 32-octet 
> + (256-bit)
> + *PSK.
> + *

Some indentation gone haywire here. I would remove '(u8 attribute)'. The
mention of 32-octet seems sufficient to me.

>   * @NL80211_ATTR_MAX: highest attribute number currently defined
>   * @__NL80211_ATTR_AFTER_LAST: internal use
>   */
> @@ -1990,6 +1994,8 @@ enum nl80211_attrs {
>  
>  NL80211_ATTR_SMPS_MODE,
>  
> +NL80211_ATTR_PSK,
> +
>  /* add attributes here, update the policy in nl80211.c */
>  
>  __NL80211_ATTR_AFTER_LAST,
> diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c index 
> 5839c85..91c24b1 100644
> --- a/net/wireless/nl80211.c
> +++ b/net/wireless/nl80211.c
> @@ -395,6 +395,7 @@ static const struct nla_policy 
> nl80211_policy[NL80211_ATTR_MAX+1] = {
>  [NL80211_ATTR_USER_PRIO] = { .type = NLA_U8 },
>  [NL80211_ATTR_ADMITTED_TIME] = { .type = NLA_U16 },
>  [NL80211_ATTR_SMPS_MODE] = { .type = NLA_U8 },
> +[NL80211_ATTR_PSK] = { .type = NLA_BINARY, .len = 32 },
>  };
>  
>  /* policy for the key attributes */
> @@ -7310,6 +7311,9 @@ static int nl80211_connect(struct sk_buff *skb, struct 
> genl_info *info)
>  connect.flags |= ASSOC_REQ_USE_RRM;
>  }
>  
> +if (info->attrs[NL80211_ATTR_PSK])
> +connect.psk = nla_data(info->attrs[NL80211_ATTR_PSK]);
> +
>  wdev_lock(dev->ieee80211_ptr);
>  err = cfg80211_connect(rdev, dev, &connect, connkeys, NULL);
>  wdev_unlock(dev->ieee80211_ptr);
> --
> 1.9.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] staging: rtl8723au: change typecast to match type returned by htons()

[Arend van Spriel]

On 10-11-14 21:21, Jes Sorensen wrote:
> Chris Ruffin  writes:
>> Using a u16 pointer typecast for a result from htons() results in the 
>> following warning from sparse:
>>
>> drivers/staging/rtl8723au/core/rtw_xmit.c:1279:36: warning: incorrect type 
>> in assignment (different base types)
>> drivers/staging/rtl8723au/core/rtw_xmit.c:1279:36:expected unsigned 
>> short [unsigned] [short] [usertype] 
>> drivers/staging/rtl8723au/core/rtw_xmit.c:1279:36:got restricted __be16 
>> [usertype] 
>>
>> This patch fixes the issue by using an endian-specific typecast
>> that will always match the type returned by htons().
>>
>> Signed-off-by: Chris Ruffin 
>> ---
>>  drivers/staging/rtl8723au/core/rtw_xmit.c |2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> Looks fine to me
> 
> Signed-off-by: Jes Sorensen 
> 
>>
>> diff --git a/drivers/staging/rtl8723au/core/rtw_xmit.c 
>> b/drivers/staging/rtl8723au/core/rtw_xmit.c
>> index a0f7e27..44ef55c 100644
>> --- a/drivers/staging/rtl8723au/core/rtw_xmit.c
>> +++ b/drivers/staging/rtl8723au/core/rtw_xmit.c
>> @@ -1276,7 +1276,7 @@ s32 rtw_put_snap23a(u8 *data, u16 h_proto)
>>  snap->oui[0] = oui[0];
>>  snap->oui[1] = oui[1];
>>  snap->oui[2] = oui[2];
>> -*(u16 *)(data + SNAP_SIZE) = htons(h_proto);
>> +*(__be16 *)(data + SNAP_SIZE) = htons(h_proto);

Could (data + SNAP_SIZE) be on a unaligned address?

Regards,
Arend

>>  return SNAP_SIZE + sizeof(u16);
>>  }
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] debugfs: add helper function to create device related seq_file

[Arend van Spriel]

On 12-10-14 10:13, Arend van Spriel wrote:
> On 11-10-14 22:17, Greg Kroah-Hartman wrote:
>> On Sat, Oct 11, 2014 at 06:01:55PM +0200, Arend van Spriel wrote:
>>> This patch adds a helper function that simplifies adding a
>>> sequence file for device drivers. The calling device driver
>>> needs to provide a read function and a device pointer. The
>>> field struct seq_file::private will reference the device
>>> pointer upon call to the read function so the driver can
>>> obtain his data from it and do its seq_printf() calls.
>>>
>>> Signed-off-by: Arend van Spriel 
>>> ---
>>>  fs/debugfs/file.c   | 54 
>>> +
>>>  include/linux/debugfs.h | 16 ++-
>>>  2 files changed, 69 insertions(+), 1 deletion(-)
>>
>> Do you have some kernel code pending that needs this change?  Or can
>> existing users be moved to it, saving them code?
> 
> Yes and maybe. I made a similar change in our brcm80211 drivers, but
> figured it may be useful for other device drivers. So when this gets in
> the kernel I want to align the brcm80211 drivers. I will also have a
> quick look at the wireless drivers and get some metrics about code savings.

Hi, Greg

Killed a bit of time between lpc sessions in dusseldorf. I picked the
ath9k wireless driver as a test case. I made the changes in two steps.
First step was changing to use seq_file api and second step made use of
the helper function:

originalseq_filehelper
logstat -   173+/255-   32+/96-
ath9k.o 115968  113224  111024  (.text size)

I am considering to make an attempt at capturing the transition in a
SmPL script so it can be applied to other drivers as well. Have to do a
bit of self-study for that ;-)

Regards,
Arend

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] debugfs: add helper function to create device related seq_file

[Arend van Spriel]

On 11-10-14 22:17, Greg Kroah-Hartman wrote:
> On Sat, Oct 11, 2014 at 06:01:55PM +0200, Arend van Spriel wrote:
>> This patch adds a helper function that simplifies adding a
>> sequence file for device drivers. The calling device driver
>> needs to provide a read function and a device pointer. The
>> field struct seq_file::private will reference the device
>> pointer upon call to the read function so the driver can
>> obtain his data from it and do its seq_printf() calls.
>>
>> Signed-off-by: Arend van Spriel 
>> ---
>>  fs/debugfs/file.c   | 54 
>> +
>>  include/linux/debugfs.h | 16 ++-
>>  2 files changed, 69 insertions(+), 1 deletion(-)
> 
> Do you have some kernel code pending that needs this change?  Or can
> existing users be moved to it, saving them code?

Yes and maybe. I made a similar change in our brcm80211 drivers, but
figured it may be useful for other device drivers. So when this gets in
the kernel I want to align the brcm80211 drivers. I will also have a
quick look at the wireless drivers and get some metrics about code savings.

Regards,
Arend
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] net: wireless: brcm80211: brcmfmac: dhd_sdio.c: Cleaning up missing null-terminate in conjunction with strncpy

[Arend van Spriel]

On 12-10-14 01:52, Rickard Strandqvist wrote:
> Replacing strncpy with strlcpy to avoid strings that lacks null terminate.
> And changed from using strncpy to strlcpy to simplify code.

Looks good to me. Just two small process related remarks:

- It is sufficient to prefix the patch with brcmfmac (skip net:...).
- Send the patch to the wireless maintainer, ie. John Linville.

Acked-by: Arend van Spriel 
> Signed-off-by: Rickard Strandqvist 
> ---
>  drivers/net/wireless/brcm80211/brcmfmac/dhd_sdio.c |   25 
> ++--
>  1 file changed, 12 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/net/wireless/brcm80211/brcmfmac/dhd_sdio.c 
> b/drivers/net/wireless/brcm80211/brcmfmac/dhd_sdio.c
> index f55f625..d20d4e6 100644
> --- a/drivers/net/wireless/brcm80211/brcmfmac/dhd_sdio.c
> +++ b/drivers/net/wireless/brcm80211/brcmfmac/dhd_sdio.c
> @@ -670,7 +670,6 @@ static int brcmf_sdio_get_fwnames(struct brcmf_chip *ci,
> struct brcmf_sdio_dev *sdiodev)
>  {
>   int i;
> - uint fw_len, nv_len;
>   char end;
>  
>   for (i = 0; i < ARRAY_SIZE(brcmf_fwname_data); i++) {
> @@ -684,25 +683,25 @@ static int brcmf_sdio_get_fwnames(struct brcmf_chip *ci,
>   return -ENODEV;
>   }
>  
> - fw_len = sizeof(sdiodev->fw_name) - 1;
> - nv_len = sizeof(sdiodev->nvram_name) - 1;
>   /* check if firmware path is provided by module parameter */
>   if (brcmf_firmware_path[0] != '\0') {
> - strncpy(sdiodev->fw_name, brcmf_firmware_path, fw_len);
> - strncpy(sdiodev->nvram_name, brcmf_firmware_path, nv_len);
> - fw_len -= strlen(sdiodev->fw_name);
> - nv_len -= strlen(sdiodev->nvram_name);
> + strlcpy(sdiodev->fw_name, brcmf_firmware_path,
> + sizeof(sdiodev->fw_name));
> + strlcpy(sdiodev->nvram_name, brcmf_firmware_path,
> + sizeof(sdiodev->nvram_name));
>  
>   end = brcmf_firmware_path[strlen(brcmf_firmware_path) - 1];
>   if (end != '/') {
> - strncat(sdiodev->fw_name, "/", fw_len);
> - strncat(sdiodev->nvram_name, "/", nv_len);
> - fw_len--;
> - nv_len--;
> + strlcat(sdiodev->fw_name, "/",
> + sizeof(sdiodev->fw_name));
> + strlcat(sdiodev->nvram_name, "/",
> + sizeof(sdiodev->nvram_name));
>   }
>   }
> - strncat(sdiodev->fw_name, brcmf_fwname_data[i].bin, fw_len);
> - strncat(sdiodev->nvram_name, brcmf_fwname_data[i].nv, nv_len);
> + strlcat(sdiodev->fw_name, brcmf_fwname_data[i].bin,
> + sizeof(sdiodev->fw_name));
> + strlcat(sdiodev->nvram_name, brcmf_fwname_data[i].nv,
> + sizeof(sdiodev->nvram_name));
>  
>   return 0;
>  }
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] debugfs: add helper function to create device related seq_file

[Arend van Spriel]

On 10/11/14 18:01, Arend van Spriel wrote:

This patch adds a helper function that simplifies adding a
sequence file for device drivers. The calling device driver
needs to provide a read function and a device pointer. The
field struct seq_file::private will reference the device
pointer upon call to the read function so the driver can
obtain his data from it and do its seq_printf() calls.


Maybe should have made this RFC first. Anyways, it is intended for 3.19, 
although I don't mind if it would end up in 3.18 :-p


Regards,
Arend


Signed-off-by: Arend van Spriel
---
  fs/debugfs/file.c   | 54 +
  include/linux/debugfs.h | 16 ++-
  2 files changed, 69 insertions(+), 1 deletion(-)

diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
index 76c08c2..088b3fc 100644
--- a/fs/debugfs/file.c
+++ b/fs/debugfs/file.c
@@ -22,6 +22,7 @@
  #include
  #include
  #include
+#include

  static ssize_t default_read_file(struct file *file, char __user *buf,
 size_t count, loff_t *ppos)
@@ -761,3 +762,56 @@ struct dentry *debugfs_create_regset32(const char *name, 
umode_t mode,
  EXPORT_SYMBOL_GPL(debugfs_create_regset32);

  #endif /* CONFIG_HAS_IOMEM */
+
+struct debugfs_devm_entry {
+   int (*read)(struct seq_file *seq, void *data);
+   struct device *dev;
+};
+
+static int debugfs_devm_entry_open(struct inode *inode, struct file *f)
+{
+   struct debugfs_devm_entry *entry = inode->i_private;
+
+   return single_open(f, entry->read, entry->dev);
+}
+
+static const struct file_operations debugfs_devm_entry_ops = {
+   .owner = THIS_MODULE,
+   .open = debugfs_devm_entry_open,
+   .release = single_release,
+   .read = seq_read,
+   .llseek = seq_lseek
+};
+
+/**
+ * debugfs_create_devm_seqfile - create a debugfs file that is bound to device.
+ *
+ * @dev: device related to this debugfs file.
+ * @name: name of the debugfs file.
+ * @parent: a pointer to the parent dentry for this file.  This should be a
+ * directory dentry if set.  If this parameter is %NULL, then the
+ * file will be created in the root of the debugfs filesystem.
+ * @read_fn: function pointer called to print the seq_file content.
+ */
+struct dentry *debugfs_create_devm_seqfile(struct device *dev, const char 
*name,
+  struct dentry *parent,
+  int (*read_fn)(struct seq_file *s,
+ void *data))
+{
+   struct debugfs_devm_entry *entry;
+
+   if (IS_ERR(parent))
+   return ERR_PTR(-ENOENT);
+
+   entry = devm_kzalloc(dev, sizeof(*entry), GFP_KERNEL);
+   if (!entry)
+   return ERR_PTR(-ENOMEM);
+
+   entry->read = read_fn;
+   entry->dev = dev;
+
+   return debugfs_create_file(name, S_IRUGO, parent, entry,
+   &debugfs_devm_entry_ops);
+}
+EXPORT_SYMBOL_GPL(debugfs_create_devm_seqfile);
+
diff --git a/include/linux/debugfs.h b/include/linux/debugfs.h
index 4d0b4d1..f8c0db4 100644
--- a/include/linux/debugfs.h
+++ b/include/linux/debugfs.h
@@ -99,13 +99,18 @@ struct dentry *debugfs_create_u32_array(const char *name, 
umode_t mode,
struct dentry *parent,
u32 *array, u32 elements);

+struct dentry *debugfs_create_devm_seqfile(struct device *dev, const char 
*name,
+  struct dentry *parent,
+  int (*read_fn)(struct seq_file *s,
+ void *data));
+
  bool debugfs_initialized(void);

  #else

  #include

-/*
+/*
   * We do not return NULL from these functions if CONFIG_DEBUG_FS is not 
enabled
   * so users have a chance to detect if there was a real error or not.  We 
don't
   * want to duplicate the design decision mistakes of procfs and devfs again.
@@ -251,6 +256,15 @@ static inline struct dentry 
*debugfs_create_u32_array(const char *name, umode_t
return ERR_PTR(-ENODEV);
  }

+static inline struct dentry *debugfs_create_devm_seqfile(struct device *dev,
+const char *name,
+struct dentry *parent,
+  int (*read_fn)(struct seq_file *s,
+ void *data))
+{
+   return ERR_PTR(-ENODEV);
+}
+
  #endif

  #endif


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] debugfs: add helper function to create device related seq_file

[Arend van Spriel]

This patch adds a helper function that simplifies adding a
sequence file for device drivers. The calling device driver
needs to provide a read function and a device pointer. The
field struct seq_file::private will reference the device
pointer upon call to the read function so the driver can
obtain his data from it and do its seq_printf() calls.

Signed-off-by: Arend van Spriel 
---
 fs/debugfs/file.c   | 54 +
 include/linux/debugfs.h | 16 ++-
 2 files changed, 69 insertions(+), 1 deletion(-)

diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
index 76c08c2..088b3fc 100644
--- a/fs/debugfs/file.c
+++ b/fs/debugfs/file.c
@@ -22,6 +22,7 @@
 #include 
 #include 
 #include 
+#include 
 
 static ssize_t default_read_file(struct file *file, char __user *buf,
 size_t count, loff_t *ppos)
@@ -761,3 +762,56 @@ struct dentry *debugfs_create_regset32(const char *name, 
umode_t mode,
 EXPORT_SYMBOL_GPL(debugfs_create_regset32);
 
 #endif /* CONFIG_HAS_IOMEM */
+
+struct debugfs_devm_entry {
+   int (*read)(struct seq_file *seq, void *data);
+   struct device *dev;
+};
+
+static int debugfs_devm_entry_open(struct inode *inode, struct file *f)
+{
+   struct debugfs_devm_entry *entry = inode->i_private;
+
+   return single_open(f, entry->read, entry->dev);
+}
+
+static const struct file_operations debugfs_devm_entry_ops = {
+   .owner = THIS_MODULE,
+   .open = debugfs_devm_entry_open,
+   .release = single_release,
+   .read = seq_read,
+   .llseek = seq_lseek
+};
+
+/**
+ * debugfs_create_devm_seqfile - create a debugfs file that is bound to device.
+ *
+ * @dev: device related to this debugfs file.
+ * @name: name of the debugfs file.
+ * @parent: a pointer to the parent dentry for this file.  This should be a
+ * directory dentry if set.  If this parameter is %NULL, then the
+ * file will be created in the root of the debugfs filesystem.
+ * @read_fn: function pointer called to print the seq_file content.
+ */
+struct dentry *debugfs_create_devm_seqfile(struct device *dev, const char 
*name,
+  struct dentry *parent,
+  int (*read_fn)(struct seq_file *s,
+ void *data))
+{
+   struct debugfs_devm_entry *entry;
+
+   if (IS_ERR(parent))
+   return ERR_PTR(-ENOENT);
+
+   entry = devm_kzalloc(dev, sizeof(*entry), GFP_KERNEL);
+   if (!entry)
+   return ERR_PTR(-ENOMEM);
+
+   entry->read = read_fn;
+   entry->dev = dev;
+
+   return debugfs_create_file(name, S_IRUGO, parent, entry,
+  &debugfs_devm_entry_ops);
+}
+EXPORT_SYMBOL_GPL(debugfs_create_devm_seqfile);
+
diff --git a/include/linux/debugfs.h b/include/linux/debugfs.h
index 4d0b4d1..f8c0db4 100644
--- a/include/linux/debugfs.h
+++ b/include/linux/debugfs.h
@@ -99,13 +99,18 @@ struct dentry *debugfs_create_u32_array(const char *name, 
umode_t mode,
struct dentry *parent,
u32 *array, u32 elements);
 
+struct dentry *debugfs_create_devm_seqfile(struct device *dev, const char 
*name,
+  struct dentry *parent,
+  int (*read_fn)(struct seq_file *s,
+ void *data));
+
 bool debugfs_initialized(void);
 
 #else
 
 #include 
 
-/* 
+/*
  * We do not return NULL from these functions if CONFIG_DEBUG_FS is not enabled
  * so users have a chance to detect if there was a real error or not.  We don't
  * want to duplicate the design decision mistakes of procfs and devfs again.
@@ -251,6 +256,15 @@ static inline struct dentry 
*debugfs_create_u32_array(const char *name, umode_t
return ERR_PTR(-ENODEV);
 }
 
+static inline struct dentry *debugfs_create_devm_seqfile(struct device *dev,
+const char *name,
+struct dentry *parent,
+  int (*read_fn)(struct seq_file *s,
+ void *data))
+{
+   return ERR_PTR(-ENODEV);
+}
+
 #endif
 
 #endif
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] bcma: Add dependency on OF_ADDRESS

[Arend van Spriel]

On 10/09/14 23:48, Hauke Mehrtens wrote:

On 10/09/2014 09:25 PM, Guenter Roeck wrote:

On Thu, Oct 09, 2014 at 08:28:31PM +0200, Hauke Mehrtens wrote:

On 10/09/2014 07:29 PM, Guenter Roeck wrote:

On Thu, Oct 09, 2014 at 07:18:31PM +0200, Arend van Spriel wrote:

On 10/09/14 19:15, Arend van Spriel wrote:

On 10/09/14 18:54, Rafał Miłecki wrote:

On 9 October 2014 18:41, Guenter Roeck  wrote:

Commit 2101e533f41a ("bcma: register bcma as device tree driver")
introduces a hard dependency on OF_ADDRESS into the bcma driver.
OF_ADDRESS is specifically disabled for the sparc architecture.
This results in the following error when building sparc64:allmodconfig.


Does this mean on sparc (using allmodconfig) you will get CONFIG_OF and
!CONFIG_OF_ADDRESS? Does that makes sense?


Is CONFIG_OF is used on sparc to access OpenBoot information?


I have no idea. All I know is that the driver doesn't build anymore with OF
enabled and OF_ADDRESS disabled.


Device tree support in bcma is only needed on some SoC, when this is
used on a PCIe card it is not needed.

I would just deactivate the parts that are using device tree in bcma
when it is not available. I will send a patch after having something to eat.


Devicetree dependency is already covered with #ifdef CONFIG_OF. Problem is
that it really needs #ifdef CONFIG_OF_ADDRESS. Though even that might be
better than my patch, since it would at least build the driver on sparc
as it used to do.


Is there a better method which is compatible with SPARC than using
of_translate_address() to get the reg address and also take the ranges
attribute of the bus into account?


No idea, sorry. Can you by any chance use pcie device information
instead of depending on devicetree data ?


Device tree is not used for PCIe devices in bcma. We only use it when
bcma is used for the system bus on some Broadcom SoCs, currently there
is no plan to use device tree for PCIe devices in bcma. I think bcma is
only used on wifi cards connected via PCIe on Sparc systems.


When we mainlined I verified the brcmsmac was working on a sparc system. 
However, at that time we were not using BCMA. I do not recall whether or 
not we tested on sparc with BCMA support. I might give it a spin again.


Regards,
Arend
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] bcma: Add dependency on OF_ADDRESS

[Arend van Spriel]

On 10/09/14 19:15, Arend van Spriel wrote:

On 10/09/14 18:54, Rafał Miłecki wrote:

On 9 October 2014 18:41, Guenter Roeck wrote:

Commit 2101e533f41a ("bcma: register bcma as device tree driver")
introduces a hard dependency on OF_ADDRESS into the bcma driver.
OF_ADDRESS is specifically disabled for the sparc architecture.
This results in the following error when building sparc64:allmodconfig.


Does this mean on sparc (using allmodconfig) you will get CONFIG_OF and
!CONFIG_OF_ADDRESS? Does that makes sense?


Is CONFIG_OF is used on sparc to access OpenBoot information?

Regards,
Arend


btw. the OF_ADDRESS dependency was introduced by commit "bcma: get IRQ
numbers from dt".

Regards,
Arend


Won't this make bcma im-POSSIBLE on MIPS? And maybe x86(_64), or at
least add an unneeded dependency? I think we should somehow limit this
to BCM5301X arch.
--
To unsubscribe from this list: send the line "unsubscribe
linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] bcma: Add dependency on OF_ADDRESS

[Arend van Spriel]

On 10/09/14 18:54, Rafał Miłecki wrote:

On 9 October 2014 18:41, Guenter Roeck  wrote:

Commit 2101e533f41a ("bcma: register bcma as device tree driver")
introduces a hard dependency on OF_ADDRESS into the bcma driver.
OF_ADDRESS is specifically disabled for the sparc architecture.
This results in the following error when building sparc64:allmodconfig.


Does this mean on sparc (using allmodconfig) you will get CONFIG_OF and 
!CONFIG_OF_ADDRESS? Does that makes sense?


btw. the OF_ADDRESS dependency was introduced by commit "bcma: get IRQ 
numbers from dt".


Regards,
Arend


Won't this make bcma im-POSSIBLE on MIPS? And maybe x86(_64), or at
least add an unneeded dependency? I think we should somehow limit this
to BCM5301X arch.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] brcmfmac: Fix off by one bug in brcmf_count_20mhz_channels()

[Arend van Spriel]

On 09/23/14 01:08, Emil Goode wrote:

Hello Arend,

Sorry for the late reply. I have attached a kernel log with brcmfmac
debugging enabled (without my patch applied).

Let me know if I can provide any other useful information.


No problem, Emil

I was wondering what was returned on "chanspecs" query. So 17 channel 
configs which is expected.


Regards,
Arend


Best regards,

Emil

On Mon, Sep 22, 2014 at 11:56:43AM +0200, Arend van Spriel wrote:

On 09/21/14 00:58, Emil Goode wrote:

In the brcmf_count_20mhz_channels function we are looping through a list
of channels received from firmware. Since the index of the first channel
is 0 the condition leads to an off by one bug. This is causing us to hit
the WARN_ON_ONCE(1) calls in the brcmu_d11n_decchspec function, which is
how I discovered the bug.


The fix is fine. Would like to know what exactly is going wrong. Can you
provide a kernel log with brcmfmac debugging enabled, ie. insmod brcmfmac.ko
debug=0x1416

Regards,
Arend


Introduced by:
commit b48d891676f756d48b4d0ee131e4a7a5d43ca417
("brcmfmac: rework wiphy structure setup")

Signed-off-by: Emil Goode
---
  drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c 
b/drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c
index 02fe706..93b5dd9 100644
--- a/drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c
+++ b/drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c
@@ -4918,7 +4918,7 @@ static void brcmf_count_20mhz_channels(struct 
brcmf_cfg80211_info *cfg,
struct brcmu_chan ch;
int i;

-   for (i = 0; i<= total; i++) {
+   for (i = 0; i<   total; i++) {
ch.chspec = (u16)le32_to_cpu(chlist->element[i]);
cfg->d11inf.decchspec(&ch);





--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] brcmfmac: Fix off by one bug in brcmf_count_20mhz_channels()

[Arend van Spriel]

On 09/23/14 00:49, Emil Goode wrote:

In the brcmf_count_20mhz_channels function we are looping through a list
of channels received from firmware. Since the index of the first channel
is 0 the condition leads to an off by one bug. This is causing us to hit
the WARN_ON_ONCE(1) calls in the brcmu_d11n_decchspec function, which is
how I discovered the bug.

Introduced by:
commit b48d891676f756d48b4d0ee131e4a7a5d43ca417
("brcmfmac: rework wiphy structure setup")



Hi John,

This bug was introduced in 3.17 so can it still go in the wireless tree? 
I verified it applies to wireless/master branch.


Regards,
Arend


Acked-by: Arend van Spriel
Signed-off-by: Emil Goode
---
v2: Added Arends "Acked-by" tag.

  drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c 
b/drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c
index 12a60ca..0517687 100644
--- a/drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c
+++ b/drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c
@@ -4924,7 +4924,7 @@ static void brcmf_count_20mhz_channels(struct 
brcmf_cfg80211_info *cfg,
struct brcmu_chan ch;
int i;

-   for (i = 0; i<= total; i++) {
+   for (i = 0; i<  total; i++) {
ch.chspec = (u16)le32_to_cpu(chlist->element[i]);
cfg->d11inf.decchspec(&ch);



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] brcmfmac: Fix off by one bug in brcmf_count_20mhz_channels()

[Arend van Spriel]

On 09/21/14 00:58, Emil Goode wrote:

In the brcmf_count_20mhz_channels function we are looping through a list
of channels received from firmware. Since the index of the first channel
is 0 the condition leads to an off by one bug. This is causing us to hit
the WARN_ON_ONCE(1) calls in the brcmu_d11n_decchspec function, which is
how I discovered the bug.


The fix is fine. Would like to know what exactly is going wrong. Can you 
provide a kernel log with brcmfmac debugging enabled, ie. insmod 
brcmfmac.ko debug=0x1416


Regards,
Arend


Introduced by:
commit b48d891676f756d48b4d0ee131e4a7a5d43ca417
("brcmfmac: rework wiphy structure setup")

Signed-off-by: Emil Goode
---
  drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c 
b/drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c
index 02fe706..93b5dd9 100644
--- a/drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c
+++ b/drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c
@@ -4918,7 +4918,7 @@ static void brcmf_count_20mhz_channels(struct 
brcmf_cfg80211_info *cfg,
struct brcmu_chan ch;
int i;

-   for (i = 0; i<= total; i++) {
+   for (i = 0; i<  total; i++) {
ch.chspec = (u16)le32_to_cpu(chlist->element[i]);
cfg->d11inf.decchspec(&ch);



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] brcmfmac: Fix off by one bug in brcmf_count_20mhz_channels()

[Arend van Spriel]

On 09/21/14 00:58, Emil Goode wrote:

In the brcmf_count_20mhz_channels function we are looping through a list
of channels received from firmware. Since the index of the first channel
is 0 the condition leads to an off by one bug. This is causing us to hit
the WARN_ON_ONCE(1) calls in the brcmu_d11n_decchspec function, which is
how I discovered the bug.

Introduced by:
commit b48d891676f756d48b4d0ee131e4a7a5d43ca417
("brcmfmac: rework wiphy structure setup")


My bad :-(. You can add:

Acked-by: Arend van Spriel 

Signed-off-by: Emil Goode
---
  drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c 
b/drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c
index 02fe706..93b5dd9 100644
--- a/drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c
+++ b/drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c
@@ -4918,7 +4918,7 @@ static void brcmf_count_20mhz_channels(struct 
brcmf_cfg80211_info *cfg,
struct brcmu_chan ch;
int i;

-   for (i = 0; i<= total; i++) {
+   for (i = 0; i<  total; i++) {
ch.chspec = (u16)le32_to_cpu(chlist->element[i]);
cfg->d11inf.decchspec(&ch);



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 2/2] dmaengine: omap-dma: Restore the CLINK_CTRL in resume path

[Arend van Spriel]

On 09/16/14 15:56, Russell King - ARM Linux wrote:

On Tue, Sep 16, 2014 at 04:53:49PM +0300, Peter Ujfalusi wrote:

When the audio stream is paused or suspended we stop the sDMA and when it
is unpaused/resumed we start the channel without reconfiguring it.
The omap_dma_stop() clears the link configuration when we pause the dma, but
it is not setting it back on start. This will result only one audio buffer
to be played back and the DMA will stop, since the linking is disabled.
We need to restore the CLINK_CTRL register in case of resume.

Signed-off-by: Peter Ujfalusi


Both of these patches now look good, thanks.  For both:

Acked-by: Russell King


---
  drivers/dma/omap-dma.c | 3 +++
  1 file changed, 3 insertions(+)

diff --git a/drivers/dma/omap-dma.c b/drivers/dma/omap-dma.c
index c01ea505ee7c..e0990c505889 100644
--- a/drivers/dma/omap-dma.c
+++ b/drivers/dma/omap-dma.c
@@ -1019,6 +1019,9 @@ static int omap_dma_resume(struct omap_chan *c)
if (c->paused) {
mb();

+   /* Restore chanel link register */


Just too bad that the comment seems to have a typo.


+   omap_dma_chan_write(c, CLNK_CTRL, c->desc->clnk_ctrl);
+
omap_dma_start(c, c->desc);
c->paused = false;
}
--
2.1.0





--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 0/4 linux-next] drivers/net: use container_of where possible

[Arend van Spriel]

On 09/13/14 22:38, Fabian Frederick wrote:

Small patchset using container_of instead of casting on first structure member 
address.


Hi Fabian,

I think you need to split up this series. One for the ethernet drivers 
and one for the wireless driver. As there is not dependency between the 
patches they can each go through their own subsystem git tree. Generally 
I tend to submit patches to the subsystem maintainer and Cc the 
appropriate mailing lists.


Regarding the patches for brcmsmac, you can add

Acked-by: Arend van Spriel 

Regards,
Arend


Fabian Frederick (4):
   net: fec: use container_of to resolve bufdesc_ex from bufdesc
   bna: use container_of to resolve bufdesc_ex from bufdesc
   brcm80211: use container_of to resolve brcms_phy from brcms_phy_pub
   brcm80211: use container_of to resolve dma_info from dma_pub

  drivers/net/ethernet/brocade/bna/bna_enet.c|   9 +-
  drivers/net/ethernet/brocade/bna/bna_tx_rx.c   |   4 +-
  drivers/net/ethernet/freescale/fec_main.c  |   4 +-
  drivers/net/wireless/brcm80211/brcmsmac/dma.c  |  38 +++
  .../net/wireless/brcm80211/brcmsmac/phy/phy_cmn.c  | 122 ++---
  .../net/wireless/brcm80211/brcmsmac/phy/phy_lcn.c  |   6 +-
  .../net/wireless/brcm80211/brcmsmac/phy/phy_n.c|   8 +-
  7 files changed, 97 insertions(+), 94 deletions(-)



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC v2] device coredump: add new device coredump class

[Arend van Spriel]

On 09/05/14 10:50, Johannes Berg wrote:

From: Johannes Berg

Many devices run firmware and/or complex hardware, and most of that
can have bugs. When it misbehaves, however, it is often much harder
to debug than software running on the host.

Introduce a "device coredump" mechanism to allow dumping internal
device/firmware state through a generalized mechanism. As devices
are different and information needed can vary accordingly, this
doesn't prescribe a file format - it just provides mechanism to
get data to be able to capture it in a generalized way (e.g. in
distributions.)


Although it can be found in the patch it would not hurt to point out 
where the coredump can be found in sysfs in this patch description.


[...]


+static struct class devcd_class = {
+   .name   = "devcoredump",
+   .owner  = THIS_MODULE,
+   .dev_release= devcd_dev_release,
+   .dev_groups = devcd_dev_groups,
+};


[...]


+/**
+ * dev_coredumpm - create firmware coredump with read/free methods
+ * @dev: the struct device for the crashed device
+ * @data: data cookie for the @read/@free functions
+ * @datalen: length of the data
+ * @gfp: allocation flags
+ * @read: function to read from the given buffer
+ * @free: function to free the given buffer
+ */
+void dev_coredumpm(struct device *dev, struct module *owner,
+  const void *data, size_t datalen, gfp_t gfp,
+  ssize_t (*read)(char *buffer, loff_t offset, size_t count,
+  const void *data, size_t datalen),
+  void (*free)(const void *data))
+{
+   static atomic_t devcd_count = ATOMIC_INIT(0);
+   struct devcd_entry *devcd;
+   struct device *existing;
+
+   existing = class_find_device(&devcd_class, NULL, dev,
+devcd_match_failing);
+   if (existing) {
+   put_device(existing);
+   return;
+   }
+
+   if (!try_module_get(owner))
+   return;
+
+   devcd = kzalloc(sizeof(*devcd), gfp);
+   if (!devcd)
+   goto put_module;
+
+   devcd->owner = owner;
+   devcd->data = data;
+   devcd->datalen = datalen;
+   devcd->read = read;
+   devcd->free = free;
+   devcd->failing_dev = get_device(dev);
+
+   device_initialize(&devcd->devcd_dev);
+
+   dev_set_name(&devcd->devcd_dev, "devcd%d",
+atomic_inc_return(&devcd_count));
+   devcd->devcd_dev.class =&devcd_class;
+
+   if (device_add(&devcd->devcd_dev))
+   goto put_device;
+
+   if (sysfs_create_link(&devcd->devcd_dev.kobj,&dev->kobj,
+ "failing_device"))
+   /* nothing - symlink will be missing */;
+
+   if (sysfs_create_link(&dev->kobj,&devcd->devcd_dev.kobj,
+ "dev_coredump"))
+   /* nothing - symlink will be missing */;


The class is called "devcoredump" so you may want to be consistent here 
and get rid of the underscore.


Regards,
Arend
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] staging: xillybus: Move out of staging

[Arend van Spriel]

On 09/01/14 14:13, Dan Carpenter wrote:

Pretty nice.  This is very special purpose hardware and the UAPI for
this is fine.  The documentation seems good.  I had some minor style
comments but nothing major stands out.


Maybe it would be better to use the DMA-API instead of the PCI wrappers.

Regards,
Arend


These days I quite like the --strict option for checkpatch.pl.

for i in $(find drivers/staging/xillybus/ -name \*\.c)
do ./scripts/checkpatch.pl --strict -f $i
done


+irqreturn_t xillybus_isr(int irq, void *data)
+{
+   struct xilly_endpoint *ep = data;
+   u32 *buf;
+   unsigned int buf_size;
+   int i;
+   int opcode;
+   unsigned int msg_channel, msg_bufno, msg_data, msg_dir;
+   struct xilly_channel *channel;
+
+   buf = ep->msgbuf_addr;
+   buf_size = ep->msg_buf_size/sizeof(u32);
+
+   ep->ephw->hw_sync_sgl_for_cpu(ep,
+ ep->msgbuf_dma_addr,
+ ep->msg_buf_size,
+ DMA_FROM_DEVICE);
+
+   for (i = 0; i<  buf_size; i += 2)


Add an open curly brace here for the multi-line indent.


+   if (((buf[i+1]>>  28)&  0xf) != ep->msg_counter) {
+   malformed_message(ep,&buf[i]);
+   dev_warn(ep->dev,
+"Sending a NACK on counter %x (instead of %x) on 
entry %d\n",
+   ((buf[i+1]>>  28)&  0xf),
+   ep->msg_counter,
+   i/2);
+
+   if (++ep->failed_messages>  10) {
+   dev_err(ep->dev,
+   "Lost sync with interrupt messages. 
Stopping.\n");
+   } else {
+   ep->ephw->hw_sync_sgl_for_device(
+   ep,
+   ep->msgbuf_dma_addr,
+   ep->msg_buf_size,
+   DMA_FROM_DEVICE);
+
+   iowrite32(0x01,  /* Message NACK */
+ ep->registers + fpga_msg_ctrl_reg);
+   }
+   return IRQ_HANDLED;
+   } else if (buf[i]&  (1<<  22)) /* Last message */
+   break;
+
+   if (i>= buf_size) {
+   dev_err(ep->dev, "Bad interrupt message. Stopping.\n");
+   return IRQ_HANDLED;
+   }
+
+   buf_size = i;


The buf_size is actually "i + 2".  "i" is the second last index.


+
+   for (i = 0; i<= buf_size; i += 2) { /* Scan through messages */


Then this loop becomes a more normal "i<  buf_size".


+   opcode = (buf[i]>>  24)&  0xff;
+
+   msg_dir = buf[i]&  1;
+   msg_channel = (buf[i]>>  1)&  0x7ff;
+   msg_bufno = (buf[i]>>  12)&  0x3ff;
+   msg_data = buf[i+1]&  0xfff;
+
+   switch (opcode) {
+   case XILLYMSG_OPCODE_RELEASEBUF:
+
+   if ((msg_channel>  ep->num_channels) ||
+   (msg_channel == 0)) {
+   malformed_message(ep,&buf[i]);
+   break;
+   }
+
+   channel = ep->channels[msg_channel];
+
+   if (msg_dir) { /* Write channel */
+   if (msg_bufno>= channel->num_wr_buffers) {
+   malformed_message(ep,&buf[i]);
+   break;
+   }
+   spin_lock(&channel->wr_spinlock);
+   channel->wr_buffers[msg_bufno]->end_offset =
+   msg_data;
+   channel->wr_fpga_buf_idx = msg_bufno;
+   channel->wr_empty = 0;
+   channel->wr_sleepy = 0;
+   spin_unlock(&channel->wr_spinlock);
+
+   wake_up_interruptible(&channel->wr_wait);
+
+   } else {
+   /* Read channel */
+
+   if (msg_bufno>= channel->num_rd_buffers) {
+   malformed_message(ep,&buf[i]);
+   break;
+   }
+
+   spin_lock(&channel->rd_spinlock);
+   channel->rd_fpga_buf_idx = msg_bufno;
+   channel->rd_full = 0;
+   spin_unlock(&channel->rd_spinlock);
+
+   wake_up_interruptible(&channel->rd_wait);
+   if (!channel->rd_synchronous)
+   queue_delayed_work(
+

Re: [PATCH] brcmfmac: BRCMFMAC should depend on HAS_DMA

[Arend van Spriel]

On 09/01/14 10:44, Geert Uytterhoeven wrote:

Hi Arend,

On Mon, Sep 1, 2014 at 10:21 AM, Arend van Spriel  wrote:

   config BRCMFMAC
 tristate "Broadcom IEEE802.11n embedded FullMAC WLAN driver"
-   depends on CFG80211
+   depends on CFG80211A&&   HAS_DMA


Not sure what happened here, but CFG80211 kconfig option did was not renamed
as far as I know. Anyway, I would like to propose a different solution. Will


Oops, that was me typing the vim append-command twice :-(
Thanks for noticing!


get back to you when I have it ready.


OK. Thanks again!


Hi Geert,

I added Kconfig options for the proto layer which are selected by the 
host interface selection. So the HAS_DMA dependency is checked for 
BRCMFMAC_PCIE. I am wondering whether GENERIC_IO should also be checked.


Can you verify the patch below works with your configuration?

Regards,
Arend
---
diff --git a/drivers/net/wireless/brcm80211/Kconfig 
b/drivers/net/wireless/brcm80211/Kconfig

index b8e2561..fe3dc12 100644
--- a/drivers/net/wireless/brcm80211/Kconfig
+++ b/drivers/net/wireless/brcm80211/Kconfig
@@ -27,10 +27,17 @@ config BRCMFMAC
  one of the bus interface support. If you choose to build a module,
  it'll be called brcmfmac.ko.

+config BRCMFMAC_PROTO_BCDC
+   bool
+
+config BRCMFMAC_PROTO_MSGBUF
+   bool
+
 config BRCMFMAC_SDIO
bool "SDIO bus interface support for FullMAC driver"
depends on (MMC = y || MMC = BRCMFMAC)
depends on BRCMFMAC
+   select BRCMFMAC_PROTO_BCDC
select FW_LOADER
default y
---help---
@@ -42,6 +49,7 @@ config BRCMFMAC_USB
bool "USB bus interface support for FullMAC driver"
depends on (USB = y || USB = BRCMFMAC)
depends on BRCMFMAC
+   select BRCMFMAC_PROTO_BCDC
select FW_LOADER
---help---
  This option enables the USB bus interface support for Broadcom
@@ -52,6 +60,8 @@ config BRCMFMAC_PCIE
bool "PCIE bus interface support for FullMAC driver"
depends on BRCMFMAC
depends on PCI
+   depends on HAS_DMA
+   select BRCMFMAC_PROTO_MSGBUF
select FW_LOADER
---help---
  This option enables the PCIE bus interface support for Broadcom
diff --git a/drivers/net/wireless/brcm80211/brcmfmac/Makefile 
b/drivers/net/wireless/brcm80211/brcmfmac/Makefile

index c35adf4..90a977f 100644
--- a/drivers/net/wireless/brcm80211/brcmfmac/Makefile
+++ b/drivers/net/wireless/brcm80211/brcmfmac/Makefile
@@ -30,16 +30,18 @@ brcmfmac-objs += \
fwsignal.o \
p2p.o \
proto.o \
-   bcdc.o \
-   commonring.o \
-   flowring.o \
-   msgbuf.o \
dhd_common.o \
dhd_linux.o \
firmware.o \
feature.o \
btcoex.o \
vendor.o
+brcmfmac-$(CONFIG_BRCMFMAC_PROTO_BCDC) += \
+   bcdc.o
+brcmfmac-$(CONFIG_BRCMFMAC_PROTO_MSGBUF) += \
+   commonring.o \
+   flowring.o \
+   msgbuf.o
 brcmfmac-$(CONFIG_BRCMFMAC_SDIO) += \
dhd_sdio.o \
bcmsdh.o
diff --git a/drivers/net/wireless/brcm80211/brcmfmac/bcdc.h 
b/drivers/net/wireless/brcm80211/brcmfmac/bcdc.h

index 17e8c03..6003179 100644
--- a/drivers/net/wireless/brcm80211/brcmfmac/bcdc.h
+++ b/drivers/net/wireless/brcm80211/brcmfmac/bcdc.h
@@ -16,9 +16,12 @@
 #ifndef BRCMFMAC_BCDC_H
 #define BRCMFMAC_BCDC_H

-
+#ifdef CONFIG_BRCMFMAC_PROTO_BCDC
 int brcmf_proto_bcdc_attach(struct brcmf_pub *drvr);
 void brcmf_proto_bcdc_detach(struct brcmf_pub *drvr);
-
+#else
+static inline int brcmf_proto_bcdc_attach(struct brcmf_pub *drvr) { 
return 0; }

+static inline void brcmf_proto_bcdc_detach(struct brcmf_pub *drvr) {}
+#endif

 #endif /* BRCMFMAC_BCDC_H */
diff --git a/drivers/net/wireless/brcm80211/brcmfmac/msgbuf.h 
b/drivers/net/wireless/brcm80211/brcmfmac/msgbuf.h

index f901ae5..e6395ee 100644
--- a/drivers/net/wireless/brcm80211/brcmfmac/msgbuf.h
+++ b/drivers/net/wireless/brcm80211/brcmfmac/msgbuf.h
@@ -32,9 +32,16 @@


 int brcmf_proto_msgbuf_rx_trigger(struct device *dev);
+void brcmf_msgbuf_delete_flowring(struct brcmf_pub *drvr, u8 flowid);
+#ifdef CONFIG_BRCMFMAC_PROTO_MSGBUF
 int brcmf_proto_msgbuf_attach(struct brcmf_pub *drvr);
 void brcmf_proto_msgbuf_detach(struct brcmf_pub *drvr);
-void brcmf_msgbuf_delete_flowring(struct brcmf_pub *drvr, u8 flowid);
-
+#else
+static inline int brcmf_proto_msgbuf_attach(struct brcmf_pub *drvr)
+{
+   return 0;
+}
+static inline void brcmf_proto_msgbuf_detach(struct brcmf_pub *drvr) {}
+#endif

 #endif /* BRCMFMAC_MSGBUF_H */

Gr{oetje,eeting}s,

 Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talkin

Re: [PATCH] brcmfmac: BRCMFMAC should depend on HAS_DMA

[Arend van Spriel]

On 08/29/14 18:24, Geert Uytterhoeven wrote:

If NO_DMA=y:

drivers/built-in.o: In function `brcmf_msgbuf_release_array':
msgbuf.c:(.text+0x34dbbe): undefined reference to `dma_unmap_single'
drivers/built-in.o: In function `brcmf_proto_msgbuf_detach':
(.text+0x34dca4): undefined reference to `dma_free_coherent'
drivers/built-in.o: In function `brcmf_msgbuf_get_pktid':
msgbuf.c:(.text+0x34dd2a): undefined reference to `dma_unmap_single'
drivers/built-in.o: In function `brcmf_msgbuf_alloc_pktid':
msgbuf.c:(.text+0x34de12): undefined reference to `dma_map_single'
msgbuf.c:(.text+0x34de20): undefined reference to `dma_mapping_error'
drivers/built-in.o: In function `brcmf_msgbuf_remove_flowring':
msgbuf.c:(.text+0x34e3d6): undefined reference to `dma_free_coherent'
drivers/built-in.o: In function `brcmf_msgbuf_flowring_create':
msgbuf.c:(.text+0x34e4f8): undefined reference to `dma_alloc_coherent'
drivers/built-in.o: In function `brcmf_proto_msgbuf_attach':
(.text+0x34f5fe): undefined reference to `dma_alloc_coherent'
drivers/built-in.o: In function `brcmf_proto_msgbuf_attach':
(.text+0x34f798): undefined reference to `dma_free_coherent'

Signed-off-by: Geert Uytterhoeven
---
  drivers/net/wireless/brcm80211/Kconfig | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/brcm80211/Kconfig 
b/drivers/net/wireless/brcm80211/Kconfig
index b8e2561ea645..2e0da8925834 100644
--- a/drivers/net/wireless/brcm80211/Kconfig
+++ b/drivers/net/wireless/brcm80211/Kconfig
@@ -19,7 +19,7 @@ config BRCMSMAC

  config BRCMFMAC
tristate "Broadcom IEEE802.11n embedded FullMAC WLAN driver"
-   depends on CFG80211
+   depends on CFG80211A&&  HAS_DMA


Hi Geert,

Not sure what happened here, but CFG80211 kconfig option did was not 
renamed as far as I know. Anyway, I would like to propose a different 
solution. Will get back to you when I have it ready.


Regards,
Arend


select BRCMUTIL
---help---
  This module adds support for embedded wireless adapters based on


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] brcmfmac: prevent watchdog from interfering with scanning and connecting

[Arend van Spriel]

On 08/04/14 18:52, Russell King - ARM Linux wrote:

On Tue, Aug 05, 2014 at 12:36:49AM +0800, Fu, Zhonghui wrote:

Hi, Arend

I investigated this issue, and its root cause is still that sdio
controller can't receive interrupts from WiFi chip on sdio bus when
sdio controller is in runtime suspend status. I am running 3.16-rc5
linux kernel on ASUS T100TA tablet, and using sdhci-acpi driver.


That's the root cause.  I fixed this with the Freescale i.MX SD driver
which is now able to report pending SDIO interrupts while runtime PM
suspended.

Other host drivers probably need fixing too, or having runtime PM
disabled on them - if you can't receive SDIO interrupts while runtime
PM suspended, then entering runtime PM while you have a SDIO device
attached is a bug.

This is something for the MMC people to deal with rather than Arend.


Occasionally, the itch is there to fix mmc code, but this looks a bit 
tricky. The fun starts in sdio.c:mmc_attach_sdio():


/*
 * Enable runtime PM only if supported by host+card+board
 */
if (host->caps & MMC_CAP_POWER_OFF_CARD) {
/*
 * Let runtime PM core know our card is active
 */
err = pm_runtime_set_active(&card->dev);
if (err)
goto remove;

/*
 * Enable runtime PM for this card
 */
pm_runtime_enable(&card->dev);
}

The comment above the if statement seems to be stating the right idea, 
but the code only looks at the host controller capability flags.


Regards,
Arend

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] brcmfmac: prevent watchdog from interfering with scanning and connecting

[Arend van Spriel]

On 08/04/14 18:36, Fu, Zhonghui wrote:


On 2014/7/24 23:22, Fu, Zhonghui wrote:

On 2014/7/21 15:42, Fu, Zhonghui wrote:

On 2014/6/20 0:37, Arend van Spriel wrote:

On 19-06-14 18:28, Fu, Zhonghui wrote:

On 2014/6/16 16:15, Arend van Spriel wrote:

On 16-06-14 07:49, Fu, Zhonghui wrote:

From 14485894add32aedacb3e486ebb2cc2b73861abf Mon Sep 17 00:00:00 2001
From: Fu zhonghui
Date: Wed, 11 Jun 2014 11:06:55 +0800
Subject: [PATCH] brcmfmac: prevent watchdog from interfering with scanning and 
connecting

Watchdog in brcmfmac driver may make WiFi chip enter sleep mode
before completion of scanning or connecting.

This will lead to scanning or connecting failure.

Increasing temporarily idle-time threshold during scanning or
connecting can ensure scanning or connecting success without
watchdog interference.

Are you sure you are not having runtime PM enabled. That could be your problem 
as the host controller code disables sdio irqs. Please see this thread [1]. For 
our device runtime pm should not be enabled, but I am not sure if that is taken 
into account in mmc_attach_sdio() (see code below).

I have tried brcmfmac driver with "CONFIG_PM_RUNTIME is not set", and scanning 
and connecting  always succeeded. This means that CONFIG_PM_RUNTIME should not be set if 
we use SDIO WiFi device driven by brcmfmac driver, right?

Depends which kernel you are using. In 3.16-rc1 the sdio interrupt issue has 
been fixed so you should be fine to use CONFIG_PM_RUNTIME. Here is the commit


Hi, Arend

I investigated this issue, and its root cause is still that sdio controller 
can't receive interrupts from WiFi chip on sdio bus when sdio controller is in 
runtime suspend status. I am running 3.16-rc5 linux kernel on ASUS T100TA 
tablet, and using sdhci-acpi driver.


Thanks, Zhonghui

Given that the patches Russell made were covering sdhci (as far as I 
recall) your issue may be specific to the sdhci-acpi driver. In 
sdhci_acpi_probe() it is enabled by use_runtime_pm field in private data:


c->use_runtime_pm = sdhci_acpi_flag(c, SDHCI_ACPI_RUNTIME_PM);

Regards,
Arend


Thanks,
Zhonghui

Any clues or comments to the following issue?

I run 3.16-rc4 and 3.16-rc5 kernel with CONFIG_PM_RUNTIME set, the issue still 
exists as follows:

1 step: modprobe brcmfmac
2 step: iwlist wlan1 scan get scanning result successfully.
3 step: wait a while, no any operation
4 step: iwlist wlan1 scan scanning timeout, can't get scanning result.


Thanks,
Zhonghui


commit be138554a7923658ded799b0e8794d9c1d08a6e5
Author: Russell King
Date:   Fri Apr 25 12:55:56 2014 +0100

 mmc: sdhci: allow sdio interrupts while sdhci runtime suspended

Regards,
Arend


Thanks,
Zhonghui

Gr. AvS

[1] http://www.spinics.net/lists/linux-mmc/msg25978.html

--8<

  /*
   * Enable runtime PM only if supported by host+card+board
   */
  if (host->caps&  MMC_CAP_POWER_OFF_CARD) {
  /*
   * Let runtime PM core know our card is active
   */
  err = pm_runtime_set_active(&card->dev);
  if (err)
  goto remove;

  /*
   * Enable runtime PM for this card
   */
  pm_runtime_enable(&card->dev);
  }


Signed-off-by: Fu zhonghui
---
drivers/net/wireless/brcm80211/brcmfmac/dhd_sdio.c |   18 --
.../net/wireless/brcm80211/brcmfmac/wl_cfg80211.c  |3 ++-
2 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/brcm80211/brcmfmac/dhd_sdio.c 
b/drivers/net/wireless/brcm80211/brcmfmac/dhd_sdio.c
index 13c89a0..729deab 100644
--- a/drivers/net/wireless/brcm80211/brcmfmac/dhd_sdio.c
+++ b/drivers/net/wireless/brcm80211/brcmfmac/dhd_sdio.c
@@ -34,6 +34,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -43,6 +44,10 @@
#include "sdio_host.h"
#include "chip.h"
#include "nvram.h"
+#include "dhd.h"
+#include "fwil_types.h"
+#include "p2p.h"
+#include "wl_cfg80211.h"

#define DCMD_RESP_TIMEOUT  2000/* In milli second */

@@ -307,6 +312,7 @@ struct rte_console {
 * when idle
 */
#define BRCMF_IDLE_INTERVAL1
+#define BRCMF_IDLE_INTERVAL_SCANNING_CONNECTING100

#define KSO_WAIT_US 50
#define MAX_KSO_ATTEMPTS (PMU_MAX_TRANSITION_DLY/KSO_WAIT_US)
@@ -3613,9 +3619,9 @@ void brcmf_sdio_isr(struct brcmf_sdio *bus)

static bool brcmf_sdio_bus_watchdog(struct brcmf_sdio *bus)
{
-#ifdef DEBUG
struct brcmf_bus *bus_if = dev_get_drvdata(bus->sdiodev->dev);
-#endif/* DEBUG */
+struct brcmf_cfg80211_info *cfg = bus_if->drvr->config;
+struct brcmf_if *ifp = cfg->pub->iflist[0];

brcmf_dbg(TIMER, "Enter\n");

@@ -3678,6 +3684,14 @@ static bool brc

Re: [PATCH v7 29/33] net: brcmfmac - set name assign type

[Arend van Spriel]

On 10-07-14 22:24, Tom Gundersen wrote:
> On Thu, Jul 10, 2014 at 10:08 PM, Arend van Spriel  wrote:
>> On 10-07-14 10:17, Tom Gundersen wrote:
>>> The name is given by the firmware, so we assume it is predictable.
>>
>> How about the scenario where one would have multiple broadcom wifi
>> devices in the system. Both driver instances would alloc_netdev with
>> predictable but also the same ifname. Wondering whether we should ignore
>> the firmware ifname altogether.
> 
> Hm, that would just fail irrespective of this patch, right? Sounds
> like ignoring the firmware names is the right thing to do.

True. I realized that when starting to type my reply. I think at the
moment we never run into the scenario that trigger this code path. Will
make a separate patch for that. Probably on top of your changes.

Regards,
Arend

> Cheers,
> 
> Tom
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v7 29/33] net: brcmfmac - set name assign type

[Arend van Spriel]

On 10-07-14 10:17, Tom Gundersen wrote:
> The name is given by the firmware, so we assume it is predictable.

How about the scenario where one would have multiple broadcom wifi
devices in the system. Both driver instances would alloc_netdev with
predictable but also the same ifname. Wondering whether we should ignore
the firmware ifname altogether.

Regards,
Arend

> Signed-off-by: Tom Gundersen 
> Cc: Brett Rudley 
> Cc: Arend van Spriel 
> Cc: "Franky (Zhenhui) Lin" 
> Cc: Hante Meuleman 
> Cc: John Linville 
> Cc: linux-wirel...@vger.kernel.org
> Cc: brcm80211-dev-l...@broadcom.com
> ---
>  drivers/net/wireless/brcm80211/brcmfmac/fweh.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/wireless/brcm80211/brcmfmac/fweh.c 
> b/drivers/net/wireless/brcm80211/brcmfmac/fweh.c
> index f6990f2..8d32721 100644
> --- a/drivers/net/wireless/brcm80211/brcmfmac/fweh.c
> +++ b/drivers/net/wireless/brcm80211/brcmfmac/fweh.c
> @@ -201,7 +201,8 @@ static void brcmf_fweh_handle_if_event(struct brcmf_pub 
> *drvr,
>   brcmf_dbg(EVENT, "adding %s (%pM)\n", emsg->ifname,
> emsg->addr);
>   ifp = brcmf_add_if(drvr, ifevent->bssidx, ifevent->ifidx,
> -emsg->ifname, NET_NAME_UNKNOWN, emsg->addr);
> +emsg->ifname, NET_NAME_PREDICTABLE,
> +emsg->addr);
>   if (IS_ERR(ifp))
>   return;
>   brcmf_fws_add_interface(ifp);
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v7 01/33] net: add name_assign_type netdev attribute

[Arend van Spriel]

On 10-07-14 10:17, Tom Gundersen wrote:
> Based on a patch by David Herrmann.
> 
> The name_assign_type attribute gives hints where the interface name of a
> given net-device comes from. These values are currently defined:
>   NET_NAME_ENUM:
> The ifname is provided by the kernel with an enumerated
> suffix, typically based on order of discovery. Names may
> be reused and unpredictable.
>   NET_NAME_PREDICTABLE:
> The ifname has been assigned by the kernel in a predictable way
> that is guaranteed to avoid reuse and always be the same for a
> given device. Examples include statically created devices like
> the loopback device and names deduced from hardware properties
> (including being given explicitly by the firmware). Names
> depending on the order of discovery, or in any other way on the
> existence of other devices, must not be marked as PREDICTABLE.
>   NET_NAME_USER:
> The ifname was provided by user-space during net-device setup.
>   NET_NAME_RENAMED:
> The net-device has been renamed from userspace. Once this type is set,
> it cannot change again.
>   NET_NAME_UNKNOWN:
> This is an internal placeholder to indicate that we yet haven't yet
> categorized the name. It will not be exposed to userspace, rather
> -EINVAL is returned.
> 
> The aim of these patches is to improve user-space renaming of interfaces. As
> a general rule, userspace must rename interfaces to guarantee that names stay
> the same every time a given piece of hardware appears (at boot, or when
> attaching it). However, there are several situations where userspace should
> not perform the renaming, and that depends on both the policy of the local
> admin, but crucially also on the nature of the current interface name.
> 
> If an interface was created in repsonse to a userspace request, and userspace
> already provided a name, we most probably want to leave that name alone. The
> main instance of this is wifi-P2P devices created over nl80211, which 
> currently
> have a long-standing bug where they are getting renamed by udev. We label such
> names NET_NAME_USER.
> 
> If an interface, unbeknown to us, has already been renamed from userspace, we
> most probably want to leave also that alone. This will typically happen when
> third-party plugins (for instance to udev, but the interface is generic so 
> could
> be from anywhere) renames the interface without informing udev about it. A
> typical situation is when you switch root from an installer or an initrd to 
> the
> real system and the new instance of udev does not know what happened before
> the switch. These types of problems have caused repeated issues in the past. 
> To
> solve this, once an interface has been renamed, its name is labelled
> NET_NAME_RENAMED.
> 
> In many cases, the kernel is actually able to name interfaces in such a
> way that there is no need for userspace to rename them. This is the case when
> the enumeration order of devices, or in fact any other (non-parent) device on
> the system, can not influence the name of the interface. Examples include
> statically created devices, or any naming schemes based on hardware properties
> of the interface. In this case the admin may prefer to use the kernel-provided
> names, and to make that possible we label such names NET_NAME_PREDICTABLE.
> We want the kernel to have tho possibilty of performing predictable interface
> naming itself (and exposing to userspace that it has), as the information
> necessary for a proper naming scheme for a certain class of devices may not
> be exposed to userspace.
> 
> The case where renaming is almost certainly desired, is when the kernel has
> given the interface a name using global device enumeration based on order of
> discovery (ethX, wlanY, etc). These naming schemes are labelled NET_NAME_ENUM.
> 
> Lastly, a fallback is left as NET_NAME_UNKNOWN, to indicate that a driver has
> not yet been ported. This is mostly useful as a transitionary measure, 
> allowing
> us to label the various naming schemes bit by bit.
> 
> Signed-off-by: Tom Gundersen 
> Reviewed-by: David Herrmann 
> Reviewed-by: Kay Sievers 
> ---
>  Documentation/ABI/testing/sysfs-class-net | 11 +++
>  include/linux/netdevice.h |  2 ++
>  include/uapi/linux/netdevice.h|  6 ++
>  net/core/net-sysfs.c  | 20 
>  4 files changed, 39 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-class-net 
> b/Documentation/ABI/testing/sysfs-class-net
> index 416c5d5..d34280a 100644
> --- a/Documentation/ABI/testing/sysfs-class-net
> +++ b/Documentation/ABI/testing/sysfs-class-net
> @@ -1,3 +1,14 @@
> +What:/sys/class/net//name_assign_type
> +Date:July 2014
> +KernelVersion:   3.2

Is this a copy&paste error. I guess it will be 3.17+, right?

> +Contact: net...@vger.kernel.org
> +Description:
> + Indicates the name assignment type

Re: [RFC 1/2] pwrseq: Add subsystem to handle complex power sequences

[Arend van Spriel]

On 01-07-14 18:42, Ulf Hansson wrote:
> On 20 June 2014 10:14, Hans de Goede  wrote:
>> Hi,
>>
>> On 06/20/2014 10:02 AM, Olof Johansson wrote:
>>> On Fri, Jun 20, 2014 at 12:27 AM, Hans de Goede  wrote:
 Hi,

 On 06/19/2014 07:18 PM, Olof Johansson wrote:
> Hi,
>
>
>
> On Thu, Jun 19, 2014 at 6:04 AM, Ulf Hansson  
> wrote:
>> The pwrseq subsystem handles complex power sequences, typically useful
>> for subsystems that makes use of discoverable buses, like for example
>> MMC and I2C.
>>
>> The pwrseq subsystem is dependant on CONFIG_OF to be able to parse a DT
>> childnode to find out what power sequence method to bind for a device.
>>
>> From the DT childnode, the pwrseq DT parser tries to locate a
>> "power-method" property, which string is matched towards the list of
>> supported pwrseq methods.
>>
>> Each pwrseq method implements it's own power sequence and interfaces
>> the pwrseq core through a few callback functions.
>>
>> To instantiate a pwrseq method, clients shall use the devm_pwrseq_get()
>> API. If needed, clients can explicity drop the references to a pwrseq
>> method using devm_pwrseq_put() API.
>>
>> Besides instantiation, the pwrseq API provides clients opportunity to
>> select a certain power state. In this intial version, PWRSEQ_POWER_ON
>> and PWRSEQ_POWER_OFF are supported. Those are also mandatory for each
>> pwrseq method to support.
>>
>> Signed-off-by: Ulf Hansson 
>> ---
>>  .../devicetree/bindings/pwrseq/pwrseq.txt  |   48 ++
>>  drivers/Makefile   |2 +-
>>  drivers/pwrseq/Makefile|2 +
>>  drivers/pwrseq/core.c  |  175 
>> 
>>  drivers/pwrseq/core.h  |   37 +
>>  drivers/pwrseq/method.c|   38 +
>>  include/linux/pwrseq.h |   50 ++
>>  7 files changed, 351 insertions(+), 1 deletion(-)
>>  create mode 100644 Documentation/devicetree/bindings/pwrseq/pwrseq.txt
>>  create mode 100644 drivers/pwrseq/Makefile
>>  create mode 100644 drivers/pwrseq/core.c
>>  create mode 100644 drivers/pwrseq/core.h
>>  create mode 100644 drivers/pwrseq/method.c
>>  create mode 100644 include/linux/pwrseq.h
>>
>> diff --git a/Documentation/devicetree/bindings/pwrseq/pwrseq.txt 
>> b/Documentation/devicetree/bindings/pwrseq/pwrseq.txt
>> new file mode 100644
>> index 000..80848ae
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/pwrseq/pwrseq.txt
>> @@ -0,0 +1,48 @@
>> +Power sequence DT bindings
>> +
>> +Each power sequence method has a corresponding "power-method" property 
>> string.
>> +This property shall be set in a subnode for a device. That subnode 
>> should also
>> +describe resourses which are specific to that power method.
>> +
>> +Do note, power sequences as such isn't encoded through DT. Instead 
>> those are
>> +implemented by each power method.
>> +
>> +Required subnode properties:
>> +- power-method: should contain the string for the power method to bind.
>> +
>> +   Supported power methods: None.
>> +
>> +Example:
>> +
>> +Note, the "clock" power method in this example isn't actually 
>> supported, but
>> +used to visualize how a childnode could be described.
>> +
>> +// WLAN SDIO channel
>> +sdi1_per2@80118000 {
>> +   compatible = "arm,pl18x", "arm,primecell";
>> +   reg = <0x80118000 0x1000>;
>> +   interrupts = <0 50 IRQ_TYPE_LEVEL_HIGH>;
>> +
>> +   dmas = <&dma 32 0 0x2>, /* Logical - DevToMem */
>> +  <&dma 32 0 0x0>; /* Logical - MemToDev */
>> +   dma-names = "rx", "tx";
>> +
>> +   clocks = <&prcc_kclk 2 4>, <&prcc_pclk 2 6>;
>> +   clock-names = "sdi", "apb_pclk";
>> +
>> +   arm,primecell-periphid = <0x10480180>;
>> +   max-frequency = <1>;
>> +   bus-width = <4>;
>> +   non-removable;
>> +   pinctrl-names = "default", "sleep";
>> +   pinctrl-0 = <&sdi1_default_mode>;
>> +   pinctrl-1 = <&sdi1_sleep_mode>;
>> +
>> +   status = "okay";
>> +
>> +   pwrseq: pwrseq1 {
>> +   power-method = "clock";
>> +   clocks = <&someclk 1 2>, <&someclk 3 4>;
>> +   clock-names = "pwrseq1", "pwrseq2";
>> +   };
>
> I am strongly against the subnode approach as a general framework. We
> don't have a subnode for interrupts, nor for clocks or pinctrl. So why
> should we have it for the power sequencing?
>
> Sure, that fits the linux driver model better, but that's irrelevant
> w.r.t. d

Re: [PATCH v2 1/9] dma-buf: move to drivers/dma-buf

[Arend van Spriel]

On 01-07-14 12:57, Maarten Lankhorst wrote:
> Signed-off-by: Maarten Lankhorst 

It would help to use '-M' option with format-patch for this kind of rework.

Regards,
Arend

> ---
>  Documentation/DocBook/device-drivers.tmpl |3 
>  MAINTAINERS   |4 
>  drivers/Makefile  |1 
>  drivers/base/Makefile |1 
>  drivers/base/dma-buf.c|  743 
> -
>  drivers/base/reservation.c|   39 --
>  drivers/dma-buf/Makefile  |1 
>  drivers/dma-buf/dma-buf.c |  743 
> +
>  drivers/dma-buf/reservation.c |   39 ++
>  9 files changed, 787 insertions(+), 787 deletions(-)
>  delete mode 100644 drivers/base/dma-buf.c
>  delete mode 100644 drivers/base/reservation.c
>  create mode 100644 drivers/dma-buf/Makefile
>  create mode 100644 drivers/dma-buf/dma-buf.c
>  create mode 100644 drivers/dma-buf/reservation.c
> 
> diff --git a/Documentation/DocBook/device-drivers.tmpl 
> b/Documentation/DocBook/device-drivers.tmpl
> index cc63f30de166..ac61ebd92875 100644
> --- a/Documentation/DocBook/device-drivers.tmpl
> +++ b/Documentation/DocBook/device-drivers.tmpl
> @@ -128,8 +128,7 @@ X!Edrivers/base/interface.c
>  !Edrivers/base/bus.c
>   
>   Device Drivers DMA Management
> -!Edrivers/base/dma-buf.c
> -!Edrivers/base/reservation.c
> +!Edrivers/dma-buf/dma-buf.c
>  !Iinclude/linux/reservation.h
>  !Edrivers/base/dma-coherent.c
>  !Edrivers/base/dma-mapping.c
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 134483f206e4..c948e53a4ee6 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2882,8 +2882,8 @@ S:  Maintained
>  L:   linux-me...@vger.kernel.org
>  L:   dri-de...@lists.freedesktop.org
>  L:   linaro-mm-...@lists.linaro.org
> -F:   drivers/base/dma-buf*
> -F:   include/linux/dma-buf*
> +F:   drivers/dma-buf/
> +F:   include/linux/dma-buf* include/linux/reservation.h
>  F:   Documentation/dma-buf-sharing.txt
>  T:   git git://git.linaro.org/people/sumitsemwal/linux-dma-buf.git
>  
> diff --git a/drivers/Makefile b/drivers/Makefile
> index f98b50d8251d..c00337be5351 100644
> --- a/drivers/Makefile
> +++ b/drivers/Makefile
> @@ -61,6 +61,7 @@ obj-$(CONFIG_FB_INTEL)  += video/fbdev/intelfb/
>  
>  obj-$(CONFIG_PARPORT)+= parport/
>  obj-y+= base/ block/ misc/ mfd/ nfc/
> +obj-$(CONFIG_DMA_SHARED_BUFFER) += dma-buf/
>  obj-$(CONFIG_NUBUS)  += nubus/
>  obj-y+= macintosh/
>  obj-$(CONFIG_IDE)+= ide/
> diff --git a/drivers/base/Makefile b/drivers/base/Makefile
> index 04b314e0fa51..4aab26ec0292 100644
> --- a/drivers/base/Makefile
> +++ b/drivers/base/Makefile
> @@ -10,7 +10,6 @@ obj-$(CONFIG_DMA_CMA) += dma-contiguous.o
>  obj-y+= power/
>  obj-$(CONFIG_HAS_DMA)+= dma-mapping.o
>  obj-$(CONFIG_HAVE_GENERIC_DMA_COHERENT) += dma-coherent.o
> -obj-$(CONFIG_DMA_SHARED_BUFFER) += dma-buf.o reservation.o
>  obj-$(CONFIG_ISA)+= isa.o
>  obj-$(CONFIG_FW_LOADER)  += firmware_class.o
>  obj-$(CONFIG_NUMA)   += node.o
> diff --git a/drivers/base/dma-buf.c b/drivers/base/dma-buf.c
> deleted file mode 100644
> index 840c7fa80983..
> --- a/drivers/base/dma-buf.c
> +++ /dev/null
> @@ -1,743 +0,0 @@
> -/*
> - * Framework for buffer objects that can be shared across devices/subsystems.
> - *
> - * Copyright(C) 2011 Linaro Limited. All rights reserved.
> - * Author: Sumit Semwal 
> - *
> - * Many thanks to linaro-mm-sig list, and specially
> - * Arnd Bergmann , Rob Clark  and
> - * Daniel Vetter  for their support in creation and
> - * refining of this idea.
> - *
> - * This program is free software; you can redistribute it and/or modify it
> - * under the terms of the GNU General Public License version 2 as published 
> by
> - * the Free Software Foundation.
> - *
> - * This program is distributed in the hope that it will be useful, but 
> WITHOUT
> - * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> - * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> - * more details.
> - *
> - * You should have received a copy of the GNU General Public License along 
> with
> - * this program.  If not, see .
> - */
> -
> -#include 
> -#include 
> -#include 
> -#include 
> -#include 
> -#include 
> -#include 
> -
> -static inline int is_dma_buf_file(struct file *);
> -
> -struct dma_buf_list {
> - struct list_head head;
> - struct mutex lock;
> -};
> -
> -static struct dma_buf_list db_list;
> -
> -static int dma_buf_release(struct inode *inode, struct file *file)
> -{
> - struct dma_buf *dmabuf;
> -
> - if (!is_dma_buf_file(file))
> - return -EINVAL;
> -
> - dmabuf = file->private_data;
> -
> - BUG_ON(dmabuf->vmapping_counter);
> -
> - dmabuf->ops->release(dmabu

Re: [PATCH 3/3] p54: use request_firmware_direct() for optional EEPROM override

[Arend van Spriel]

On 25-06-14 00:39, Luis R. Rodriguez wrote:

From: "Luis R. Rodriguez" 

The p54 driver uses request_firmware() twice, once for actual
firmware and then another time for an optional user overide on
EEPROM, 3826.eeprom. The custom EEPROM  is optional but if not
present we'll introduce an extra lag of 60 seconds with udev
present. Annotate we don't want udev nonsense here to avoid
the lag in case its not present.


I guess the fact that EEPROM is optional does not matter much. If doing 
a second request you could always use request_firmware_direct(), right?


Regards,
Arend


This was found with the following SmPL patch.

@ firmware_not_critical @
expression cf;
expression config_file;
expression dev;
int ret;
identifier l;
statement S;
@@

-   ret = request_firmware(&cf, config_file, dev);
+   ret = request_firmware_direct(&cf, config_file, dev);
if (ret < 0) {
... when != goto l;
when != return ret;
when any
} else {
...
release_firmware(cf);
...
}

Cc: Takashi Iwai 
Cc: Christian Lamparter 
Cc: linux-wirel...@vger.kernel.org
Cc: co...@systeme.lip6.fr
Signed-off-by: Luis R. Rodriguez 
---
  drivers/net/wireless/p54/p54spi.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/p54/p54spi.c 
b/drivers/net/wireless/p54/p54spi.c
index de15171..63de5ee 100644
--- a/drivers/net/wireless/p54/p54spi.c
+++ b/drivers/net/wireless/p54/p54spi.c
@@ -193,7 +193,7 @@ static int p54spi_request_eeprom(struct ieee80211_hw *dev)
/* allow users to customize their eeprom.
 */

-   ret = request_firmware(&eeprom, "3826.eeprom", &priv->spi->dev);
+   ret = request_firmware_direct(&eeprom, "3826.eeprom", &priv->spi->dev);
if (ret < 0) {
  #ifdef CONFIG_P54_SPI_DEFAULT_EEPROM
dev_info(&priv->spi->dev, "loading default eeprom...\n");



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] net/wireless/brcm80211/brcmfmac: Make return type and name reflect actual semantics

[Arend van Spriel]

On 06/22/14 11:27, Arend van Spriel wrote:

On 06/22/14 00:55, Rasmus Villemoes wrote:

Applying ++ to a bool is equivalent to setting it true, regardless of
its initial value (bools are not uint1_t). Hence the function
wl_get_vif_state_all can only ever return true/false. The only in-tree
caller uses its return value as a boolean. So update its return type,
and since the list traversal and bit testing have no side effects,
just return true immediately. Its return value tells if any vif is up,



- Now I may be really nit-picking, but the return value if any vif is *in
- the specified state*.
+ Now I may be really nit-picking, but the return value tells if any
+ vif is *in the specified state*.


Regards,
Arend


so also rename it to brcmf_get_vif_state_any.

Reviewed-by: Arend van Spriel
Signed-off-by: Rasmus Villemoes
---

Notes:
v2: Rename wl_get_vif_state_all => brcmf_get_vif_state_any as
requested by Arend.

drivers/net/wireless/brcm80211/brcmfmac/p2p.c | 2 +-
drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c | 7 +++
drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.h | 2 +-
3 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/net/wireless/brcm80211/brcmfmac/p2p.c
b/drivers/net/wireless/brcm80211/brcmfmac/p2p.c
index f3445ac..588fdbd 100644
--- a/drivers/net/wireless/brcm80211/brcmfmac/p2p.c
+++ b/drivers/net/wireless/brcm80211/brcmfmac/p2p.c
@@ -708,7 +708,7 @@ static s32 brcmf_p2p_escan(struct brcmf_p2p_info
*p2p, u32 num_chans,
active = P2PAPI_SCAN_SOCIAL_DWELL_TIME_MS;
else if (num_chans == AF_PEER_SEARCH_CNT)
active = P2PAPI_SCAN_AF_SEARCH_DWELL_TIME_MS;
- else if (wl_get_vif_state_all(p2p->cfg, BRCMF_VIF_STATUS_CONNECTED))
+ else if (brcmf_get_vif_state_any(p2p->cfg, BRCMF_VIF_STATUS_CONNECTED))
active = -1;
else
active = P2PAPI_SCAN_DWELL_TIME_MS;
diff --git a/drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c
b/drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c
index d8fa276..93b1809 100644
--- a/drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c
+++ b/drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c
@@ -5625,16 +5625,15 @@ enum nl80211_iftype
brcmf_cfg80211_get_iftype(struct brcmf_if *ifp)
return wdev->iftype;
}

-u32 wl_get_vif_state_all(struct brcmf_cfg80211_info *cfg, unsigned
long state)
+bool brcmf_get_vif_state_any(struct brcmf_cfg80211_info *cfg,
unsigned long state)
{
struct brcmf_cfg80211_vif *vif;
- bool result = 0;

list_for_each_entry(vif,&cfg->vif_list, list) {
if (test_bit(state,&vif->sme_state))
- result++;
+ return true;
}
- return result;
+ return false;
}

static inline bool vif_event_equals(struct brcmf_cfg80211_vif_event
*event,
diff --git a/drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.h
b/drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.h
index 283c525..f9fb109 100644
--- a/drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.h
+++ b/drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.h
@@ -477,7 +477,7 @@ const struct brcmf_tlv *
brcmf_parse_tlvs(const void *buf, int buflen, uint key);
u16 channel_to_chanspec(struct brcmu_d11inf *d11inf,
struct ieee80211_channel *ch);
-u32 wl_get_vif_state_all(struct brcmf_cfg80211_info *cfg, unsigned
long state);
+bool brcmf_get_vif_state_any(struct brcmf_cfg80211_info *cfg,
unsigned long state);
void brcmf_cfg80211_arm_vif_event(struct brcmf_cfg80211_info *cfg,
struct brcmf_cfg80211_vif *vif);
bool brcmf_cfg80211_vif_event_armed(struct brcmf_cfg80211_info *cfg);




--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] net/wireless/brcm80211/brcmfmac: Make return type and name reflect actual semantics

[Arend van Spriel]

On 06/22/14 00:55, Rasmus Villemoes wrote:

Applying ++ to a bool is equivalent to setting it true, regardless of
its initial value (bools are not uint1_t). Hence the function
wl_get_vif_state_all can only ever return true/false. The only in-tree
caller uses its return value as a boolean. So update its return type,
and since the list traversal and bit testing have no side effects,
just return true immediately. Its return value tells if any vif is up,


Now I may be really nit-picking, but the return value if any vif is *in 
the specified state*.


Regards,
Arend


so also rename it to brcmf_get_vif_state_any.

Reviewed-by: Arend van Spriel
Signed-off-by: Rasmus Villemoes
---

Notes:
 v2: Rename wl_get_vif_state_all =>  brcmf_get_vif_state_any as
 requested by Arend.

  drivers/net/wireless/brcm80211/brcmfmac/p2p.c | 2 +-
  drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c | 7 +++
  drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.h | 2 +-
  3 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/net/wireless/brcm80211/brcmfmac/p2p.c 
b/drivers/net/wireless/brcm80211/brcmfmac/p2p.c
index f3445ac..588fdbd 100644
--- a/drivers/net/wireless/brcm80211/brcmfmac/p2p.c
+++ b/drivers/net/wireless/brcm80211/brcmfmac/p2p.c
@@ -708,7 +708,7 @@ static s32 brcmf_p2p_escan(struct brcmf_p2p_info *p2p, u32 
num_chans,
active = P2PAPI_SCAN_SOCIAL_DWELL_TIME_MS;
else if (num_chans == AF_PEER_SEARCH_CNT)
active = P2PAPI_SCAN_AF_SEARCH_DWELL_TIME_MS;
-   else if (wl_get_vif_state_all(p2p->cfg, BRCMF_VIF_STATUS_CONNECTED))
+   else if (brcmf_get_vif_state_any(p2p->cfg, BRCMF_VIF_STATUS_CONNECTED))
active = -1;
else
active = P2PAPI_SCAN_DWELL_TIME_MS;
diff --git a/drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c 
b/drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c
index d8fa276..93b1809 100644
--- a/drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c
+++ b/drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c
@@ -5625,16 +5625,15 @@ enum nl80211_iftype brcmf_cfg80211_get_iftype(struct 
brcmf_if *ifp)
return wdev->iftype;
  }

-u32 wl_get_vif_state_all(struct brcmf_cfg80211_info *cfg, unsigned long state)
+bool brcmf_get_vif_state_any(struct brcmf_cfg80211_info *cfg, unsigned long 
state)
  {
struct brcmf_cfg80211_vif *vif;
-   bool result = 0;

list_for_each_entry(vif,&cfg->vif_list, list) {
if (test_bit(state,&vif->sme_state))
-   result++;
+   return true;
}
-   return result;
+   return false;
  }

  static inline bool vif_event_equals(struct brcmf_cfg80211_vif_event *event,
diff --git a/drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.h 
b/drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.h
index 283c525..f9fb109 100644
--- a/drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.h
+++ b/drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.h
@@ -477,7 +477,7 @@ const struct brcmf_tlv *
  brcmf_parse_tlvs(const void *buf, int buflen, uint key);
  u16 channel_to_chanspec(struct brcmu_d11inf *d11inf,
struct ieee80211_channel *ch);
-u32 wl_get_vif_state_all(struct brcmf_cfg80211_info *cfg, unsigned long state);
+bool brcmf_get_vif_state_any(struct brcmf_cfg80211_info *cfg, unsigned long 
state);
  void brcmf_cfg80211_arm_vif_event(struct brcmf_cfg80211_info *cfg,
  struct brcmf_cfg80211_vif *vif);
  bool brcmf_cfg80211_vif_event_armed(struct brcmf_cfg80211_info *cfg);


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] net/wireless/brcm80211/brcmfmac: Make return type reflect actual semantics

[Arend van Spriel]

On 06/20/14 23:32, Rasmus Villemoes wrote:

Applying ++ to a bool is equivalent to setting it true, regardless of
its initial value (bools are not uint1_t). Hence the function
wl_get_vif_state_all can only ever return true/false. The only
in-tree caller uses its return value as a boolean. So update its
return type, and since the list traversal and bit testing have no side
effects, just return true immediately.


Hi Rasmus,

At the moment the function is indeed only used to determine whether any 
vif is in connected state. I am ok with your patch if you would also 
rename the function to brcmf_get_vif_state_any(). You may add 
'Reviewed-by: Arend van Spriel ' to the patch.


Regards,
Arend


Signed-off-by: Rasmus Villemoes
---

Notes:
 Alternatively, if the function is supposed to return a count, the
 one-line fix would be

 -  bool result = 0;
 +  u32 result = 0;

  drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c | 7 +++
  drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.h | 2 +-
  2 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c 
b/drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c
index d8fa276..ec5f8c5 100644
--- a/drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c
+++ b/drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c
@@ -5625,16 +5625,15 @@ enum nl80211_iftype brcmf_cfg80211_get_iftype(struct 
brcmf_if *ifp)
return wdev->iftype;
  }

-u32 wl_get_vif_state_all(struct brcmf_cfg80211_info *cfg, unsigned long state)
+bool wl_get_vif_state_all(struct brcmf_cfg80211_info *cfg, unsigned long state)
  {
struct brcmf_cfg80211_vif *vif;
-   bool result = 0;

list_for_each_entry(vif,&cfg->vif_list, list) {
if (test_bit(state,&vif->sme_state))
-   result++;
+   return true;
}
-   return result;
+   return false;
  }

  static inline bool vif_event_equals(struct brcmf_cfg80211_vif_event *event,
diff --git a/drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.h 
b/drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.h
index 283c525..c702e4e 100644
--- a/drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.h
+++ b/drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.h
@@ -477,7 +477,7 @@ const struct brcmf_tlv *
  brcmf_parse_tlvs(const void *buf, int buflen, uint key);
  u16 channel_to_chanspec(struct brcmu_d11inf *d11inf,
struct ieee80211_channel *ch);
-u32 wl_get_vif_state_all(struct brcmf_cfg80211_info *cfg, unsigned long state);
+bool wl_get_vif_state_all(struct brcmf_cfg80211_info *cfg, unsigned long 
state);
  void brcmf_cfg80211_arm_vif_event(struct brcmf_cfg80211_info *cfg,
  struct brcmf_cfg80211_vif *vif);
  bool brcmf_cfg80211_vif_event_armed(struct brcmf_cfg80211_info *cfg);


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] brcmfmac: prevent watchdog from interfering with scanning and connecting

[Arend van Spriel]

On 19-06-14 18:28, Fu, Zhonghui wrote:


On 2014/6/16 16:15, Arend van Spriel wrote:

On 16-06-14 07:49, Fu, Zhonghui wrote:

  From 14485894add32aedacb3e486ebb2cc2b73861abf Mon Sep 17 00:00:00 2001
From: Fu zhonghui 
Date: Wed, 11 Jun 2014 11:06:55 +0800
Subject: [PATCH] brcmfmac: prevent watchdog from interfering with scanning and 
connecting

Watchdog in brcmfmac driver may make WiFi chip enter sleep mode
before completion of scanning or connecting.

This will lead to scanning or connecting failure.

Increasing temporarily idle-time threshold during scanning or
connecting can ensure scanning or connecting success without
watchdog interference.


Are you sure you are not having runtime PM enabled. That could be your problem 
as the host controller code disables sdio irqs. Please see this thread [1]. For 
our device runtime pm should not be enabled, but I am not sure if that is taken 
into account in mmc_attach_sdio() (see code below).

I have tried brcmfmac driver with "CONFIG_PM_RUNTIME is not set", and scanning 
and connecting  always succeeded. This means that CONFIG_PM_RUNTIME should not be set if 
we use SDIO WiFi device driven by brcmfmac driver, right?


Depends which kernel you are using. In 3.16-rc1 the sdio interrupt issue 
has been fixed so you should be fine to use CONFIG_PM_RUNTIME. Here is 
the commit


commit be138554a7923658ded799b0e8794d9c1d08a6e5
Author: Russell King 
Date:   Fri Apr 25 12:55:56 2014 +0100

mmc: sdhci: allow sdio interrupts while sdhci runtime suspended

Regards,
Arend


Thanks,
Zhonghui


Gr. AvS

[1] http://www.spinics.net/lists/linux-mmc/msg25978.html

--8<

 /*
  * Enable runtime PM only if supported by host+card+board
  */
 if (host->caps & MMC_CAP_POWER_OFF_CARD) {
 /*
  * Let runtime PM core know our card is active
  */
 err = pm_runtime_set_active(&card->dev);
 if (err)
 goto remove;

 /*
  * Enable runtime PM for this card
  */
 pm_runtime_enable(&card->dev);
 }


Signed-off-by: Fu zhonghui 
---
   drivers/net/wireless/brcm80211/brcmfmac/dhd_sdio.c |   18 --
   .../net/wireless/brcm80211/brcmfmac/wl_cfg80211.c  |3 ++-
   2 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/brcm80211/brcmfmac/dhd_sdio.c 
b/drivers/net/wireless/brcm80211/brcmfmac/dhd_sdio.c
index 13c89a0..729deab 100644
--- a/drivers/net/wireless/brcm80211/brcmfmac/dhd_sdio.c
+++ b/drivers/net/wireless/brcm80211/brcmfmac/dhd_sdio.c
@@ -34,6 +34,7 @@
   #include 
   #include 
   #include 
+#include 
   #include 
   #include 
   #include 
@@ -43,6 +44,10 @@
   #include "sdio_host.h"
   #include "chip.h"
   #include "nvram.h"
+#include "dhd.h"
+#include "fwil_types.h"
+#include "p2p.h"
+#include "wl_cfg80211.h"

   #define DCMD_RESP_TIMEOUT  2000/* In milli second */

@@ -307,6 +312,7 @@ struct rte_console {
* when idle
*/
   #define BRCMF_IDLE_INTERVAL1
+#define BRCMF_IDLE_INTERVAL_SCANNING_CONNECTING100

   #define KSO_WAIT_US 50
   #define MAX_KSO_ATTEMPTS (PMU_MAX_TRANSITION_DLY/KSO_WAIT_US)
@@ -3613,9 +3619,9 @@ void brcmf_sdio_isr(struct brcmf_sdio *bus)

   static bool brcmf_sdio_bus_watchdog(struct brcmf_sdio *bus)
   {
-#ifdef DEBUG
   struct brcmf_bus *bus_if = dev_get_drvdata(bus->sdiodev->dev);
-#endif/* DEBUG */
+struct brcmf_cfg80211_info *cfg = bus_if->drvr->config;
+struct brcmf_if *ifp = cfg->pub->iflist[0];

   brcmf_dbg(TIMER, "Enter\n");

@@ -3678,6 +3684,14 @@ static bool brcmf_sdio_bus_watchdog(struct brcmf_sdio 
*bus)

   /* On idle timeout clear activity flag and/or turn off clock */
   if ((bus->idletime > 0) && (bus->clkstate == CLK_AVAIL)) {
+
+if (test_bit(BRCMF_SCAN_STATUS_BUSY, &cfg->scan_status) ||
+test_bit(BRCMF_VIF_STATUS_CONNECTING, &ifp->vif->sme_state)) {
+bus->idletime = BRCMF_IDLE_INTERVAL_SCANNING_CONNECTING;
+} else {
+bus->idletime = BRCMF_IDLE_INTERVAL;
+}
+
   if (++bus->idlecount >= bus->idletime) {
   bus->idlecount = 0;
   if (bus->activity) {
diff --git a/drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c 
b/drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c
index be19852..e76517e 100644
--- a/drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c
+++ b/drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c
@@ -913,6 +913,8 @@ brcmf_cfg80211_escan(struct wiphy *wiphy, struct 
brcmf_cfg80211_vif *vif,
   return -EAGAIN;
   }

+set_bit(BRCMF_SCAN_STATUS_BUSY, &cfg->scan_status);
+
   /* If scan req comes for p2p0, send it over primary I/F *

Re: [PATCH 2/2] mmc: rtsx: add support for async request

[Arend van Spriel]

On 16-06-14 10:42, Ulf Hansson wrote:

On 6 June 2014 09:05,   wrote:

From: Micky Ching 

Add support for non-blocking request, pre_req() runs dma_map_sg() and
post_req() runs dma_unmap_sg(). This patch can increase card read/write
speed, especially for high speed card and slow speed CPU.


Interesting statement about slow speed CPU, but why then test with CPU 
running at 2.3GHz. Did you also run at 800MHz?


Regards,
Arend


Test on intel i3(800MHz - 2.3GHz) performance mode(2.3GHz), SD card
clock 208MHz

run dd if=/dev/mmcblk0 of=/dev/null bs=64k count=1024
before:
67108864 bytes (67 MB) copied, 0.85427 s, 78.6 MB/s
after:
67108864 bytes (67 MB) copied, 0.74799 s, 89.7 MB/s

Signed-off-by: Micky Ching 
---
  drivers/mmc/host/rtsx_pci_sdmmc.c |  133 +++--
  1 file changed, 127 insertions(+), 6 deletions(-)

diff --git a/drivers/mmc/host/rtsx_pci_sdmmc.c 
b/drivers/mmc/host/rtsx_pci_sdmmc.c
index 1c68e0d..a2c0858 100644
--- a/drivers/mmc/host/rtsx_pci_sdmmc.c
+++ b/drivers/mmc/host/rtsx_pci_sdmmc.c
@@ -24,6 +24,7 @@
  #include 
  #include 
  #include 
+#include 
  #include 
  #include 
  #include 
@@ -36,7 +37,10 @@ struct realtek_pci_sdmmc {
 struct rtsx_pcr *pcr;
 struct mmc_host *mmc;
 struct mmc_request  *mrq;
+   struct workqueue_struct *workq;
+#define SDMMC_WORKQ_NAME   "rtsx_pci_sdmmc_workq"

+   struct work_struct  work;


I am trying to understand why you need a work/workqueue to implement
this feature. Is that really the case?

Could you elaborate on the reasons?

Kind regards
Uffe


 struct mutexhost_mutex;

 u8  ssc_depth;
@@ -48,6 +52,11 @@ struct realtek_pci_sdmmc {
 int power_state;
  #define SDMMC_POWER_ON 1
  #define SDMMC_POWER_OFF0
+
+   unsigned intsg_count;
+   s32 cookie;
+   unsigned intcookie_sg_count;
+   boolusing_cookie;
  };

  static inline struct device *sdmmc_dev(struct realtek_pci_sdmmc *host)
@@ -86,6 +95,77 @@ static void sd_print_debug_regs(struct realtek_pci_sdmmc 
*host)
  #define sd_print_debug_regs(host)
  #endif /* DEBUG */

+/*
+ * sd_pre_dma_transfer - do dma_map_sg() or using cookie
+ *
+ * @pre: if called in pre_req()
+ * return:
+ * 0 - do dma_map_sg()
+ * 1 - using cookie
+ */
+static int sd_pre_dma_transfer(struct realtek_pci_sdmmc *host,
+   struct mmc_data *data, bool pre)
+{
+   struct rtsx_pcr *pcr = host->pcr;
+   int read = data->flags & MMC_DATA_READ;
+   int count = 0;
+   int using_cookie = 0;
+
+   if (!pre && data->host_cookie && data->host_cookie != host->cookie) {
+   dev_err(sdmmc_dev(host),
+   "error: data->host_cookie = %d, host->cookie = %d\n",
+   data->host_cookie, host->cookie);
+   data->host_cookie = 0;
+   }
+
+   if (pre || data->host_cookie != host->cookie) {
+   count = rtsx_pci_dma_map_sg(pcr, data->sg, data->sg_len, read);
+   } else {
+   count = host->cookie_sg_count;
+   using_cookie = 1;
+   }
+
+   if (pre) {
+   host->cookie_sg_count = count;
+   if (++host->cookie < 0)
+   host->cookie = 1;
+   data->host_cookie = host->cookie;
+   } else {
+   host->sg_count = count;
+   }
+
+   return using_cookie;
+}
+
+static void sdmmc_pre_req(struct mmc_host *mmc, struct mmc_request *mrq,
+   bool is_first_req)
+{
+   struct realtek_pci_sdmmc *host = mmc_priv(mmc);
+   struct mmc_data *data = mrq->data;
+
+   if (data->host_cookie) {
+   dev_err(sdmmc_dev(host),
+   "error: reset data->host_cookie = %d\n",
+   data->host_cookie);
+   data->host_cookie = 0;
+   }
+
+   sd_pre_dma_transfer(host, data, true);
+   dev_dbg(sdmmc_dev(host), "pre dma sg: %d\n", host->cookie_sg_count);
+}
+
+static void sdmmc_post_req(struct mmc_host *mmc, struct mmc_request *mrq,
+   int err)
+{
+   struct realtek_pci_sdmmc *host = mmc_priv(mmc);
+   struct rtsx_pcr *pcr = host->pcr;
+   struct mmc_data *data = mrq->data;
+   int read = data->flags & MMC_DATA_READ;
+
+   rtsx_pci_dma_unmap_sg(pcr, data->sg, data->sg_len, read);
+   data->host_cookie = 0;
+}
+
  static int sd_read_data(struct realtek_pci_sdmmc *host, u8 *cmd, u16 byte_cnt,
 u8 *buf, int buf_len, int timeout)
  {
@@ -415,7 +495,7 @@ static int sd_rw_multi(struct realtek_pci_sdmmc *host, 
struct mmc_request *mrq)

 rtsx_pci_send_cmd_no_wait(pcr);

-   err = rtsx_pci_transfer_data(pcr, data->sg, data->sg_len, read, 1);
+   err = rtsx_pci_dma_transfer(pcr, data->sg, host->sg_count, read, 1);
 if (err < 0) {

Re: [PATCH] brcmfmac: prevent watchdog from interfering with scanning and connecting

[Arend van Spriel]

On 16-06-14 07:49, Fu, Zhonghui wrote:

 From 14485894add32aedacb3e486ebb2cc2b73861abf Mon Sep 17 00:00:00 2001
From: Fu zhonghui 
Date: Wed, 11 Jun 2014 11:06:55 +0800
Subject: [PATCH] brcmfmac: prevent watchdog from interfering with scanning and 
connecting

Watchdog in brcmfmac driver may make WiFi chip enter sleep mode
before completion of scanning or connecting.

This will lead to scanning or connecting failure.

Increasing temporarily idle-time threshold during scanning or
connecting can ensure scanning or connecting success without
watchdog interference.


Are you sure you are not having runtime PM enabled. That could be your 
problem as the host controller code disables sdio irqs. Please see this 
thread [1]. For our device runtime pm should not be enabled, but I am 
not sure if that is taken into account in mmc_attach_sdio() (see code 
below).


Gr. AvS

[1] http://www.spinics.net/lists/linux-mmc/msg25978.html

--8<

/*
 * Enable runtime PM only if supported by host+card+board
 */
if (host->caps & MMC_CAP_POWER_OFF_CARD) {
/*
 * Let runtime PM core know our card is active
 */
err = pm_runtime_set_active(&card->dev);
if (err)
goto remove;

/*
 * Enable runtime PM for this card
 */
pm_runtime_enable(&card->dev);
}


Signed-off-by: Fu zhonghui 
---
  drivers/net/wireless/brcm80211/brcmfmac/dhd_sdio.c |   18 --
  .../net/wireless/brcm80211/brcmfmac/wl_cfg80211.c  |3 ++-
  2 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/brcm80211/brcmfmac/dhd_sdio.c 
b/drivers/net/wireless/brcm80211/brcmfmac/dhd_sdio.c
index 13c89a0..729deab 100644
--- a/drivers/net/wireless/brcm80211/brcmfmac/dhd_sdio.c
+++ b/drivers/net/wireless/brcm80211/brcmfmac/dhd_sdio.c
@@ -34,6 +34,7 @@
  #include 
  #include 
  #include 
+#include 
  #include 
  #include 
  #include 
@@ -43,6 +44,10 @@
  #include "sdio_host.h"
  #include "chip.h"
  #include "nvram.h"
+#include "dhd.h"
+#include "fwil_types.h"
+#include "p2p.h"
+#include "wl_cfg80211.h"

  #define DCMD_RESP_TIMEOUT  2000   /* In milli second */

@@ -307,6 +312,7 @@ struct rte_console {
 * when idle
 */
  #define BRCMF_IDLE_INTERVAL   1
+#define BRCMF_IDLE_INTERVAL_SCANNING_CONNECTING100

  #define KSO_WAIT_US 50
  #define MAX_KSO_ATTEMPTS (PMU_MAX_TRANSITION_DLY/KSO_WAIT_US)
@@ -3613,9 +3619,9 @@ void brcmf_sdio_isr(struct brcmf_sdio *bus)

  static bool brcmf_sdio_bus_watchdog(struct brcmf_sdio *bus)
  {
-#ifdef DEBUG
struct brcmf_bus *bus_if = dev_get_drvdata(bus->sdiodev->dev);
-#endif /* DEBUG */
+   struct brcmf_cfg80211_info *cfg = bus_if->drvr->config;
+   struct brcmf_if *ifp = cfg->pub->iflist[0];

brcmf_dbg(TIMER, "Enter\n");

@@ -3678,6 +3684,14 @@ static bool brcmf_sdio_bus_watchdog(struct brcmf_sdio 
*bus)

/* On idle timeout clear activity flag and/or turn off clock */
if ((bus->idletime > 0) && (bus->clkstate == CLK_AVAIL)) {
+
+   if (test_bit(BRCMF_SCAN_STATUS_BUSY, &cfg->scan_status) ||
+   test_bit(BRCMF_VIF_STATUS_CONNECTING, 
&ifp->vif->sme_state)) {
+   bus->idletime = BRCMF_IDLE_INTERVAL_SCANNING_CONNECTING;
+   } else {
+   bus->idletime = BRCMF_IDLE_INTERVAL;
+   }
+
if (++bus->idlecount >= bus->idletime) {
bus->idlecount = 0;
if (bus->activity) {
diff --git a/drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c 
b/drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c
index be19852..e76517e 100644
--- a/drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c
+++ b/drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c
@@ -913,6 +913,8 @@ brcmf_cfg80211_escan(struct wiphy *wiphy, struct 
brcmf_cfg80211_vif *vif,
return -EAGAIN;
}

+   set_bit(BRCMF_SCAN_STATUS_BUSY, &cfg->scan_status);
+
/* If scan req comes for p2p0, send it over primary I/F */
if (vif == cfg->p2p.bss_idx[P2PAPI_BSSCFG_DEVICE].vif)
vif = cfg->p2p.bss_idx[P2PAPI_BSSCFG_PRIMARY].vif;
@@ -933,7 +935,6 @@ brcmf_cfg80211_escan(struct wiphy *wiphy, struct 
brcmf_cfg80211_vif *vif,
}

cfg->scan_request = request;
-   set_bit(BRCMF_SCAN_STATUS_BUSY, &cfg->scan_status);
if (escan_req) {
cfg->escan_info.run = brcmf_run_escan;
err = brcmf_p2p_scan_prep(wiphy, request, vif);
-- 1.7.1



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.ht

Re: [RFC] dt: bindings: add bindings for Broadcom bcm43xx sdio devices

[Arend van Spriel]

On 10/04/14 23:27, Jörg Krause wrote:

On 02/13/14 10:28, Chen-Yu Tsai wrote:

Hi,

On Thu, Feb 13, 2014 at 5:13 PM, Tomasz Figa<tomasz.figa@>  wrote:

Hi Arend,


On 10.02.2014 20:17, Arend van Spriel wrote:


[...]

Hi Chen-Yu,

picking up this thread.


AFAIK, the pinctrl in tied to the device node, and is selected when the
device
is registered. The MMC subsystem currently does not register child nodes,
so
this would be useless.


So if MMC does not register child nodes, brcmfmac will not be probed
with of_node set? Have there been patches submitted for this in mmc
subsystem recently.

[...]



Sascha Hauer submitted a patch a week ago. Link:
https://lkml.org/lkml/2014/4/3/522 <https://lkml.org/lkml/2014/4/3/522>


Thanks, Jörg

It is a partial solution, but it seems to conflict with my change. So 
thanks for the heads up. I am not convinced whether the GPIO and clock 
should be bound to the function. They seem more a property of the 
card/device inserted.


Regards,
Arend

--
View this message in context: 
http://linux-kernel.2935.n7.nabble.com/RFC-dt-bindings-add-bindings-for-Broadcom-bcm43xx-sdio-devices-tp801976p838046.html
Sent from the Linux Kernel mailing list archive at Nabble.com.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: Bumping required kernels to 3.0 for Linux backports ?

[Arend van Spriel]

On 10/04/14 20:56, Luis R. Rodriguez wrote:

On Thu, Apr 10, 2014 at 10:04 AM, Arend van Spriel  wrote:

Ok, I guess my voice was cracking when I mentioned 2.6.38 as being used over
here. I am probably alone in that desert.


I thought broadcom didn't use backports? If they do can you explain
how? Also what drivers do you need enabled for your use case ?


That was 2 years ago when you asked me ;-) Since then I have been using 
it to backport the brcm80211 mainline drivers to 1) Android kernel, ie. 
3.4 kernel, and 2) Fedora 19 which is actually fixed to 3.11 kernel.


So we use backports these days for enabling brcm80211 drivers on various 
test equipment that uses older kernels.


Regards,
Arend

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: Bumping required kernels to 3.0 for Linux backports ?

[Arend van Spriel]

On 04/10/14 18:59, Luis R. Rodriguez wrote:

On Thu, Apr 10, 2014 at 12:44 AM, Takashi Iwai  wrote:

At Wed, 9 Apr 2014 14:06:13 -0700,
Greg Kroah-Hartman wrote:


On Wed, Apr 09, 2014 at 01:52:29PM -0700, Luis R. Rodriguez wrote:

On Wed, Apr 9, 2014 at 1:22 PM, Greg Kroah-Hartman
  wrote:

On Wed, Apr 09, 2014 at 01:01:23PM -0700, Luis R. Rodriguez wrote:

On Wed, Apr 9, 2014 at 12:12 PM, Greg Kroah-Hartman
  wrote:

On Wed, Apr 09, 2014 at 11:28:55AM -0700, Luis R. Rodriguez wrote:

On Wed, Apr 9, 2014 at 2:18 AM, Felix Fietkau  wrote:

The oldest kernel in OpenWrt that we're still supporting with updates of
the backports tree is 3.3, so raising the minimum requirement to 3.0 is
completely fine with me.


OK note that 3.3 is not listed on kernel.org as supported. I'm fine in
carrying the stuff for those for now but ultimately it'd also be nice
if we didn't even have to test the kernels in between which are not
listed. This does however raise the question of how often a kernel in
between a list of supported kernels gets picked up to be supported
eventually. Greg, Jiri, do you happen to know what the likelyhood of
that can be?


I don't know of anything ever getting picked up after I have said it
would not be supported anymore.


Great! How soon after a release do you mention whether or not it will
be supported? Like say, 3.14, which was just released.


I only mention it around the time that it would normally go end-of-life.

For example, if 3.13 were to be a release that was going to be "long
term", I would only say something around the normal time I would be no
longer supporting it.  Like in 2-3 weeks from now.

So for 3.14, I'll not say anything about that until 3.16-rc1 is out,
give or take a week or two.


Also, as of late are you aware any distribution picking an unsupported
kernel for their next choice of kernel?


Sure, lots do, as they don't line up with my release cycles (I only pick
1 long term kernel to maintain each year).  Look at the Ubuntu releases
for examples of that.  Also openSUSE and Fedora (although Fedora does
rev their kernel pretty regularly) don't usually line up.  The
"enterprise" distros are different, but even then, they don't always
line up either (which is why Jiri is maintaining 3.12...)

Hope this helps,


It does! Unless I don't hear any complaints then given that some
distributions might choose a kernel in between and given also your
great documented story behind the gains on trying to steer folks
together on the 'ol 2.6.32 [0] and this now being faded, I'll be
bumping backports to only support>= 3.0 soon, but we'll include all
the series from 3.0 up to the latest. That should shrink compile /
test time / support time on backports to 1/2.


Why 3.0?  That's not supported by anyone anymore for "new hardware", I'd
move to 3.2 if you could, as that's the Debian stable release that will
be maintained for quite some time yet:
   https://www.kernel.org/category/releases.html


Well, the support for "new hardware" is what backports project itself
does, isn't it?

Besides, SLES11 is still supported, so yes, including 3.0.x would be
helpful.


That's two stakeholders for 3.0 -- but nothing is voiced for anything
older than that. Today I will rip the older kernels into oblivion.
Thanks for all the feedback!


Ok, I guess my voice was cracking when I mentioned 2.6.38 as being used 
over here. I am probably alone in that desert.


Regards,
Arend


  Luis
--
To unsubscribe from this list: send the line "unsubscribe backports" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/