Re: [PATCH] net: wireless: b43legacy: Replace GFP_ATOMIC with GFP_KERNEL in dma_tx_fragment

[Michael Büsch]

On Tue, 10 Apr 2018 21:54:19 +0800
Jia-Ju Bai  wrote:

> dma_tx_fragment() is never called in atomic context.
> 
> dma_tx_fragment() is only called by b43legacy_dma_tx(), which is 
> only called by b43legacy_tx_work().
> b43legacy_tx_work() is only set a parameter of INIT_WORK() in 
> b43legacy_wireless_init().
> 
> Despite never getting called from atomic context,
> dma_tx_fragment() calls alloc_skb() with GFP_ATOMIC,
> which does not sleep for allocation.
> GFP_ATOMIC is not necessary and can be replaced with GFP_KERNEL,
> which can sleep and improve the possibility of sucessful allocation.
> 
> This is found by a static analysis tool named DCNS written by myself.
> And I also manually check it.
> 
> Signed-off-by: Jia-Ju Bai 
> ---
>  drivers/net/wireless/broadcom/b43legacy/dma.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/wireless/broadcom/b43legacy/dma.c 
> b/drivers/net/wireless/broadcom/b43legacy/dma.c
> index cfa617d..2f0c64c 100644
> --- a/drivers/net/wireless/broadcom/b43legacy/dma.c
> +++ b/drivers/net/wireless/broadcom/b43legacy/dma.c
> @@ -1064,7 +1064,7 @@ static int dma_tx_fragment(struct b43legacy_dmaring 
> *ring,
>   meta->dmaaddr = map_descbuffer(ring, skb->data, skb->len, 1);
>   /* create a bounce buffer in zone_dma on mapping failure. */
>   if (b43legacy_dma_mapping_error(ring, meta->dmaaddr, skb->len, 1)) {
> - bounce_skb = alloc_skb(skb->len, GFP_ATOMIC | GFP_DMA);
> + bounce_skb = alloc_skb(skb->len, GFP_KERNEL | GFP_DMA);
>   if (!bounce_skb) {
>   ring->current_slot = old_top_slot;
>   ring->used_slots = old_used_slots;


Ack.
I think the GFP_ATOMIC came from the days where we did DMA operations
under spinlock instead of mutex.

The same thing can be done in b43.

Also 
setup_rx_descbuffer(ring, desc, meta, GFP_ATOMIC)
could be GFP_KERNEL in dma_rx().
This function is called from IRQ thread context.

-- 
Michael


pgprN4aETqkQy.pgp
Description: OpenPGP digital signature

Re: [PATCH 2/2] b43legacy: fix unitialized reads of ret by initializing the array to zero

[Michael Büsch]

On Tue,  5 Sep 2017 19:16:58 +0100
Colin King  wrote:

> From: Colin Ian King 
> 
> The u8 char array ret is not being initialized and elements outside
> the range start to end contain just garbage values from the stack.
> This results in a later scan of the array to read potentially
> uninitialized values.  Fix this by initializing the array to zero.
> This seems to have been an issue since the very first commit.
> 
> Detected by CoverityScan CID#139653 ("Uninitialized scalar variable")
> 
> Signed-off-by: Colin Ian King 
> ---
>  drivers/net/wireless/broadcom/b43legacy/radio.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/wireless/broadcom/b43legacy/radio.c 
> b/drivers/net/wireless/broadcom/b43legacy/radio.c
> index 9501420340a9..eab1c9387846 100644
> --- a/drivers/net/wireless/broadcom/b43legacy/radio.c
> +++ b/drivers/net/wireless/broadcom/b43legacy/radio.c
> @@ -280,7 +280,7 @@ u8 b43legacy_radio_aci_detect(struct b43legacy_wldev 
> *dev, u8 channel)
>  u8 b43legacy_radio_aci_scan(struct b43legacy_wldev *dev)
>  {
>   struct b43legacy_phy *phy = >phy;
> - u8 ret[13];
> + u8 ret[13] = { 0 };
>   unsigned int channel = phy->channel;
>   unsigned int i;
>   unsigned int j;


This fix seems to be correct.
Thanks for finding and fixing the issue.

Reviewed-by: Michael Buesch 



-- 
Michael


pgp4fgRHHbeGW.pgp
Description: OpenPGP digital signature

Re: [PATCH 1/2] b43: fix unitialized reads of ret by initializing the array to zero

[Michael Büsch]

On Tue,  5 Sep 2017 19:15:50 +0100
Colin King  wrote:

> From: Colin Ian King 
> 
> The u8 char array ret is not being initialized and elements outside
> the range start to end contain just garbage values from the stack.
> This results in a later scan of the array to read potentially
> uninitialized values.  Fix this by initializing the array to zero.
> This seems to have been an issue since the very first commit.
> 
> Detected by CoverityScan CID#139652 ("Uninitialized scalar variable")
> 
> Signed-off-by: Colin Ian King 
> ---
>  drivers/net/wireless/broadcom/b43/phy_g.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/wireless/broadcom/b43/phy_g.c 
> b/drivers/net/wireless/broadcom/b43/phy_g.c
> index 822dcaa8ace6..f59c02166462 100644
> --- a/drivers/net/wireless/broadcom/b43/phy_g.c
> +++ b/drivers/net/wireless/broadcom/b43/phy_g.c
> @@ -2297,7 +2297,7 @@ static u8 b43_gphy_aci_detect(struct b43_wldev *dev, u8 
> channel)
>  static u8 b43_gphy_aci_scan(struct b43_wldev *dev)
>  {
>   struct b43_phy *phy = >phy;
> - u8 ret[13];
> + u8 ret[13] = { 0 };
>   unsigned int channel = phy->channel;
>   unsigned int i, j, start, end;
>  


This fix seems to be correct.
Thanks for finding and fixing the issue.

Reviewed-by: Michael Buesch 


-- 
Michael


pgp6xvXbtxiIn.pgp
Description: OpenPGP digital signature

Re: [PATCH] b43legacy: Fix a sleep-in-atomic bug in b43legacy_attr_interfmode_store

[Michael Büsch]

On Fri, 02 Jun 2017 09:18:14 +0800
Jia-Ju Bai  wrote:

> On 06/02/2017 12:11 AM, Jonathan Corbet wrote:
> > On Thu, 01 Jun 2017 09:05:07 +0800
> > Jia-Ju Bai  wrote:
> >  
> >> I admit my patches are not well tested, and they may not well fix the bugs.
> >> I am looking forward to opinions and suggestions :)  
> > May I politely suggest that sending out untested locking changes is a
> > dangerous thing to do?  You really should not be changing the locking in a
> > piece of kernel code without understanding very well what the lock is
> > protecting and being able to say why your changes are safe.  Without that,
> > the risk of introducing subtle bugs is very high.
> >
> > It looks like you have written a useful tool that could help us to make
> > the kernel more robust.  If you are interested in my suggestion, I would
> > recommend that you post the sleep-in-atomic scenarios that you are
> > finding, but refrain from "fixing" them in any case where you cannot offer
> > a strong explanation of why your fix is correct.
> >
> > Thanks for working to find bugs in the kernel!
> >
> > jon  
> Hi,
> 
> Thanks for your good and helpful advice. I am sorry for my improper patches.
> I will only report bugs instead of sending improper patches when I have 
> no good solution of fixing the bugs.


Is somebody still working on these fixes?
I think I found my old b43-legacy based 4306, so that I will
be able to get these patches into properly tested shape.

-- 
Michael


pgpO_IQ9cG7LX.pgp
Description: OpenPGP digital signature

Re: [PATCH] b43legacy: Fix a sleep-in-atomic bug in b43legacy_op_bss_info_changed

[Michael Büsch]

On Wed, 31 May 2017 19:07:15 -0500
Larry Finger <larry.fin...@lwfinger.net> wrote:

> On 05/31/2017 10:32 AM, Michael Büsch wrote:
> > On Wed, 31 May 2017 13:26:43 +0300
> > Kalle Valo <kv...@codeaurora.org> wrote:
> >   
> >> Jia-Ju Bai <baijiaju1...@163.com> writes:
> >>  
> >>> The driver may sleep under a spin lock, and the function call path is:
> >>> b43legacy_op_bss_info_changed (acquire the lock by spin_lock_irqsave)
> >>>b43legacy_synchronize_irq
> >>>  synchronize_irq --> may sleep
> >>>
> >>> To fix it, the lock is released before b43legacy_synchronize_irq, and the
> >>> lock is acquired again after this function.
> >>>
> >>> Signed-off-by: Jia-Ju Bai <baijiaju1...@163.com>
> >>> ---
> >>>   drivers/net/wireless/broadcom/b43legacy/main.c |2 ++
> >>>   1 file changed, 2 insertions(+)
> >>>
> >>> diff --git a/drivers/net/wireless/broadcom/b43legacy/main.c 
> >>> b/drivers/net/wireless/broadcom/b43legacy/main.c
> >>> index f1e3dad..31ead21 100644
> >>> --- a/drivers/net/wireless/broadcom/b43legacy/main.c
> >>> +++ b/drivers/net/wireless/broadcom/b43legacy/main.c
> >>> @@ -2859,7 +2859,9 @@ static void b43legacy_op_bss_info_changed(struct 
> >>> ieee80211_hw *hw,
> >>>   b43legacy_write32(dev, B43legacy_MMIO_GEN_IRQ_MASK, 0);
> >>>   
> >>>   if (changed & BSS_CHANGED_BSSID) {
> >>> + spin_unlock_irqrestore(>irq_lock, flags);
> >>>   b43legacy_synchronize_irq(dev);
> >>> + spin_lock_irqsave(>irq_lock, flags);  
> >>
> >> To me this looks like a fragile workaround and not a real fix. You can
> >> easily add new race conditions with releasing the lock like this.
> >>  
> > 
> > 
> > I think releasing the lock possibly is fine. It certainly is better than
> > sleeping with a lock held.
> > We disabled the device interrupts just before this line.
> > 
> > However I think the synchronize_irq should be outside of the
> > conditional right after the write to B43legacy_MMIO_GEN_IRQ_MASK. (So
> > two lines above)
> > I don't think it makes sense to only synchronize if BSS_CHANGED_BSSID
> > is set.
> > 
> > 
> > On the other hand b43 does not have this irq-disabling foobar anymore.
> > So somebody must have removed it. Maybe you can find the commit that
> > removed this stuff from b43 and port it to b43legacy?
> > 
> > 
> > So I would vote for moving the synchronize_irq up outside of the
> > conditional and put the unlock/lock sequence around it.
> > And as a second patch on top of that try to remove this stuff
> > altogether like b43 did.  
> 
> The patch that removed it in b43 is
> 
> commit 36dbd9548e92268127b0c31b0e121e63e9207108
> Author: Michael Buesch <m...@bu3sch.de>
> Date:   Fri Sep 4 22:51:29 2009 +0200

Damn it :D

>  b43: Use a threaded IRQ handler
> 
>  Use a threaded IRQ handler to allow locking the mutex and
>  sleeping while executing an interrupt.
>  This removes usage of the irq_lock spinlock, but introduces
>  a new hardirq_lock, which is _only_ used for the PCI/SSB lowlevel
>  hard-irq handler. Sleeping busses (SDIO) will use mutex instead.
> 
>  Signed-off-by: Michael Buesch <m...@bu3sch.de>
>  Tested-by: Larry Finger <larry.fin...@lwfinger.net>
>  Signed-off-by: John W. Linville <linvi...@tuxdriver.com>
> 
> I vaguely remember this patch. Although it is roughly a 1000-line fix, I will 
> try to port it to b43legacy. I still have an old BCM4306 PCMCIA card that I 
> can 
> test in a PowerBook G4.
> 
> I agree with Michael that this is the way to go. Both of Jia-Ju's patches 
> should 
> be rejected.


I'm not sure if it's worth it. There is a risk that this would
introduce new bugs.
But sure, please feel free to try it. This way we can find out how big
this change becomes.

-- 
Michael


pgpB8k7rQgxAO.pgp
Description: OpenPGP digital signature

Re: [PATCH] b43legacy: Fix a sleep-in-atomic bug in b43legacy_op_bss_info_changed

[Michael Büsch]

On Thu, 01 Jun 2017 07:27:20 +0300
Kalle Valo <kv...@codeaurora.org> wrote:

> Michael Büsch <m...@bues.ch> writes:
> 
> >> > --- a/drivers/net/wireless/broadcom/b43legacy/main.c
> >> > +++ b/drivers/net/wireless/broadcom/b43legacy/main.c
> >> > @@ -2859,7 +2859,9 @@ static void b43legacy_op_bss_info_changed(struct 
> >> > ieee80211_hw *hw,
> >> >  b43legacy_write32(dev, B43legacy_MMIO_GEN_IRQ_MASK, 0);
> >> >  
> >> >  if (changed & BSS_CHANGED_BSSID) {
> >> > +spin_unlock_irqrestore(>irq_lock, flags);
> >> >  b43legacy_synchronize_irq(dev);
> >> > +spin_lock_irqsave(>irq_lock, flags);
> >> 
> >> To me this looks like a fragile workaround and not a real fix. You can
> >> easily add new race conditions with releasing the lock like this.
> >>   
> >
> >
> > I think releasing the lock possibly is fine. It certainly is better than
> > sleeping with a lock held.  
> 
> Sure, but IMHO in general I think the practise of releasing the lock
> like this in a middle of function is dangerous as one can easily miss
> that upper and lower halves of the function are not actually atomic
> anymore. And in this case that it's under a conditional makes it even
> worse.
> 


Yes in general I agree. Releasing and re-acquiring a lock is dangerous.
But I think in this special case here it might be harmless.
The irq_lock is used mostly (if not exclusively; I don't fully
remember) to protect against the IRQ top and bottom half.
But we disabled the device IRQs a line above and the purpose of this
synchronize is to make sure the handler will finish and thus make
dropping the lock save.
Of course it does not make sense to do this with the lock held :)

-- 
Michael


pgpCKuuPTGIgU.pgp
Description: OpenPGP digital signature

Re: [PATCH] b43legacy: Fix a sleep-in-atomic bug in b43legacy_attr_interfmode_store

[Michael Büsch]

On Wed, 31 May 2017 18:29:07 +0800
Jia-Ju Bai  wrote:

> The driver may sleep under a spin lock, and the function call path is:
> b43legacy_attr_interfmode_store (acquire the lock by spin_lock_irqsave)
>   b43legacy_radio_set_interference_mitigation
> b43legacy_radio_interference_mitigation_disable
>   b43legacy_calc_nrssi_slope
> b43legacy_synth_pu_workaround
>   might_sleep and msleep --> may sleep
> 
> Fixing it may be complex, and a possible way is to remove 
> spin_lock_irqsave and spin_lock_irqrestore in 
> b43legacy_attr_interfmode_store, and the code has been protected by
> mutex_lock and mutex_unlock.
> 
> Signed-off-by: Jia-Ju Bai 
> ---
>  drivers/net/wireless/broadcom/b43legacy/sysfs.c |2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/net/wireless/broadcom/b43legacy/sysfs.c 
> b/drivers/net/wireless/broadcom/b43legacy/sysfs.c
> index 2a1da15..9ede143 100644
> --- a/drivers/net/wireless/broadcom/b43legacy/sysfs.c
> +++ b/drivers/net/wireless/broadcom/b43legacy/sysfs.c
> @@ -137,14 +137,12 @@ static ssize_t b43legacy_attr_interfmode_store(struct 
> device *dev,
>   }
>  
>   mutex_lock(>wl->mutex);
> - spin_lock_irqsave(>wl->irq_lock, flags);
>  
>   err = b43legacy_radio_set_interference_mitigation(wldev, mode);
>   if (err)
>   b43legacyerr(wldev->wl, "Interference Mitigation not "
>  "supported by device\n");
>   mmiowb();
> - spin_unlock_irqrestore(>wl->irq_lock, flags);
>   mutex_unlock(>wl->mutex);
>  
>   return err ? err : count;


Interference mitigation has never been properly implemented and tested.
As such nobody should use it and I would be surprised if anybody uses
this attribute.
So I would suggest to remove this sysfs attribute entirely instead of
having this incorrect fix.

-- 
Michael


pgpSmZJwH04EP.pgp
Description: OpenPGP digital signature

Re: [PATCH] b43legacy: Fix a sleep-in-atomic bug in b43legacy_op_bss_info_changed

[Michael Büsch]

On Wed, 31 May 2017 13:26:43 +0300
Kalle Valo  wrote:

> Jia-Ju Bai  writes:
> 
> > The driver may sleep under a spin lock, and the function call path is:
> > b43legacy_op_bss_info_changed (acquire the lock by spin_lock_irqsave)
> >   b43legacy_synchronize_irq
> > synchronize_irq --> may sleep
> >
> > To fix it, the lock is released before b43legacy_synchronize_irq, and the 
> > lock is acquired again after this function.
> >
> > Signed-off-by: Jia-Ju Bai 
> > ---
> >  drivers/net/wireless/broadcom/b43legacy/main.c |2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/net/wireless/broadcom/b43legacy/main.c 
> > b/drivers/net/wireless/broadcom/b43legacy/main.c
> > index f1e3dad..31ead21 100644
> > --- a/drivers/net/wireless/broadcom/b43legacy/main.c
> > +++ b/drivers/net/wireless/broadcom/b43legacy/main.c
> > @@ -2859,7 +2859,9 @@ static void b43legacy_op_bss_info_changed(struct 
> > ieee80211_hw *hw,
> > b43legacy_write32(dev, B43legacy_MMIO_GEN_IRQ_MASK, 0);
> >  
> > if (changed & BSS_CHANGED_BSSID) {
> > +   spin_unlock_irqrestore(>irq_lock, flags);
> > b43legacy_synchronize_irq(dev);
> > +   spin_lock_irqsave(>irq_lock, flags);  
> 
> To me this looks like a fragile workaround and not a real fix. You can
> easily add new race conditions with releasing the lock like this.
> 


I think releasing the lock possibly is fine. It certainly is better than
sleeping with a lock held.
We disabled the device interrupts just before this line.

However I think the synchronize_irq should be outside of the
conditional right after the write to B43legacy_MMIO_GEN_IRQ_MASK. (So
two lines above)
I don't think it makes sense to only synchronize if BSS_CHANGED_BSSID
is set.


On the other hand b43 does not have this irq-disabling foobar anymore.
So somebody must have removed it. Maybe you can find the commit that
removed this stuff from b43 and port it to b43legacy?


So I would vote for moving the synchronize_irq up outside of the
conditional and put the unlock/lock sequence around it.
And as a second patch on top of that try to remove this stuff
altogether like b43 did.

-- 
Michael


pgpzN6gXvx13N.pgp
Description: OpenPGP digital signature

Re: [v3,1/2] b43: Remove unused phy_a code

[Michael Büsch]

On Thu, 16 Jun 2016 18:56:14 +0300
Kalle Valo <kv...@codeaurora.org> wrote:

> Michael Büsch <m...@bues.ch> writes:
> 
> > On Thu, 16 Jun 2016 15:23:37 + (UTC)
> > Kalle Valo <kv...@codeaurora.org> wrote:
> >  
> >> Guenter Roeck <li...@roeck-us.net> wrote:  
> >> > gcc-6 reports the following error with -Werror=unused-const-variable.
> >> > 
> >> > drivers/net/wireless/broadcom/b43/phy_a.c:576:40: error:
> >> >  'b43_phyops_a' defined but not used
> >> > 
> >> > Per Michael Büsch: "All a-phy code is usused", so remove it all,
> >> > and move the remaining Type-G initialization code into phy_g.c.
> >> > 
> >> > Reported-by: Fengguang Wu <fengguang...@intel.com> [0-day test robot]
> >> > Cc: Michael Büsch <m...@bues.ch>
> >> > Signed-off-by: Guenter Roeck <li...@roeck-us.net>
> >> 
> >> Thanks, 2 patches applied to wireless-drivers-next.git:
> >> 
> >> 9791333a840f b43: Remove unused phy_a code
> >> afdfdc481ea9 b43: Completely remove support for phy_a  
> >
> > Did anybody test this on any hardware? I think this should be tested
> > on some G-PHY hardware, before it goes to Linus.  
> 
> Larry tested these:
> 
> "These two patches have been tested on a BCM4318."
> 
> https://patchwork.kernel.org/patch/9154719/


Ah OK. I forgot.
Everything is fine then.

-- 
Michael


pgpikMGCxoPXI.pgp
Description: OpenPGP digital signature

Re: [v3,1/2] b43: Remove unused phy_a code

[Michael Büsch]

On Thu, 16 Jun 2016 15:23:37 + (UTC)
Kalle Valo <kv...@codeaurora.org> wrote:

> Guenter Roeck <li...@roeck-us.net> wrote:
> > gcc-6 reports the following error with -Werror=unused-const-variable.
> > 
> > drivers/net/wireless/broadcom/b43/phy_a.c:576:40: error:
> > 'b43_phyops_a' defined but not used
> > 
> > Per Michael Büsch: "All a-phy code is usused", so remove it all,
> > and move the remaining Type-G initialization code into phy_g.c.
> > 
> > Reported-by: Fengguang Wu <fengguang...@intel.com> [0-day test robot]
> > Cc: Michael Büsch <m...@bues.ch>
> > Signed-off-by: Guenter Roeck <li...@roeck-us.net>  
> 
> Thanks, 2 patches applied to wireless-drivers-next.git:
> 
> 9791333a840f b43: Remove unused phy_a code
> afdfdc481ea9 b43: Completely remove support for phy_a


Did anybody test this on any hardware?
I think this should be tested on some G-PHY hardware, before it goes to
Linus.


-- 
Michael


pgp_uf3lwPJrf.pgp
Description: OpenPGP digital signature

Re: [PATCH v2 1/2] b43: Remove unused phy_a code

[Michael Büsch]

On Fri,  3 Jun 2016 21:11:51 -0700
Guenter Roeck  wrote:

> +static void __b43_phy_initg(struct b43_wldev *dev)
> +{
> + struct b43_phy *phy = >phy;
> +
> + might_sleep();
> +
> + if (phy->rev >= 6) {
> + if (b43_phy_read(dev, B43_PHY_ENCORE) & B43_PHY_ENCORE_EN)
> + b43_phy_set(dev, B43_PHY_ENCORE, 0x0010);
> + else
> + b43_phy_mask(dev, B43_PHY_ENCORE, ~0x1010);
> + }
> +
> + b43_wa_all(dev);
> +
> + if (dev->dev->bus_sprom->boardflags_lo & B43_BFL_PACTRL)
> + b43_phy_maskset(dev, B43_PHY_OFDM(0x6E), 0xE000, 0x3CF);
> +}
> +
>  static void b43_phy_initg(struct b43_wldev *dev)
>  {
>   struct b43_phy *phy = >phy;
> @@ -1999,7 +2019,7 @@ static void b43_phy_initg(struct b43_wldev *dev)
>   b43_phy_initb6(dev);
>  
>   if (phy->rev >= 2 || phy->gmode)
> - b43_phy_inita(dev);
> + __b43_phy_initg(dev);

This actually is correctly called inita(), because there are A-phy parts
in the G-phy.
So I wasn't 100% correct saying that _all_ a-phy code is unused.
I'm Ok with moving that into the g-phy file though. But don't rename it.


-- 
Michael


pgpzk5G3hRor2.pgp
Description: OpenPGP digital signature

Re: [PATCH] b43: Remove unused phy_a code

[Michael Büsch]

On Fri,  3 Jun 2016 14:32:46 -0700
Guenter Roeck  wrote:

> gcc-6 reports the following error with -Werror=unused-const-variable.
> 
> drivers/net/wireless/broadcom/b43/phy_a.c:576:40: error:
>   'b43_phyops_a' defined but not used
> 
> Turns out a lot of code in this file is unused, so let's remove it.


All a-phy code is usused.
So you can basically remove the whole file and any other A-PHY code.
... or just leave it as-is.


-- 
Michael


pgp1cjpfcyKcc.pgp
Description: OpenPGP digital signature

Re: [PATCH RFC] b43: stop hardcoding LED behavior

[Michael Büsch]

On Mon, 25 Apr 2016 20:21:36 +0200
Lucas Stach  wrote:

> > Numbers please. Did you measure that is actually causes more
> > _wakeups_?
> > How many?
> > The led work is placed in the mac80211 workqueue and LED updates only
> > happen on behalf of mac80211 activities (by default). It only causes
> > additional wakeups, if there's nothing else scheduled on the
> > workqueue
> > anyways (which might well be the case. So we need numbers. :)
> >   
> The blinking LEDs use a timer to enforce a constant blink rate at a
> 50ms on/off interval. While they are only triggered if there is some
> RX/TX activity in the system, they cause up to 20 wakeups/second/led.
> As the timers used for LED activity aren't deferrable, this hardcode is
> causing 40 unnecessary CPU wakeups/s in my system.


Ok this is 40 to 40k for the interrupt requests?
We need real measured numbers and a percentage on how much the b43 LEDs
increase the system wakeups in relation to all other wakeups.


> There are some people that find those kinds of blinking LEDs
> distracting,


Those can already disable them via the standard LED framework.


> so a module parameter to disable them altogether might be
> a good thing to have.


No. We have a standard API for this.


> Causing CPU wakeups in a system where those LEDs
> aren't even physically populated is clearly undesired behavior.


Yes, but this is not going to be fixed via regressions.


> If checking that the SPROM doesn't define any LED behavior is enough to
> not regress your use case, I would be glad to rework the patch
> accordingly.


As it turns out I don't have that card here and I don't have a dump of
its SPROM as I expected. So I cannot really verify this. But I'm pretty
sure that this card did not define any LEDs in its SPROM at all.
I'm not aware of any card that only partially defines LEDs in the
SPROM. So that fix would be OK.

-- 
Michael


pgpvQmxgfji05.pgp
Description: OpenPGP digital signature

Re: [PATCH RFC] b43: stop hardcoding LED behavior

[Michael Büsch]

On Mon, 25 Apr 2016 09:40:51 +0200
Lucas Stach  wrote:

> On my system the SPROM correctly defines the only wired LED (radio) but
> skips all others, leading to the hardcode to register LEDs with RX and TX
> triggers.

Hm ok. It probably is a good idea to change the condition from

if (sprom[led_index] == 0xFF)

to

if ((sprom[0] & sprom[1] & sprom[2] & sprom[3]) == 0xFF)

So the hardcoding only happens if there is no LED configured in the
SPROM. (I think my card does this (see below), but I can check that
later)


> These triggers cause many uneccesary CPU wakeups to drive LEDs
> that aren't even present in the system, reducing battery runtime.


Numbers please. Did you measure that is actually causes more _wakeups_?
How many?
The led work is placed in the mac80211 workqueue and LED updates only
happen on behalf of mac80211 activities (by default). It only causes
additional wakeups, if there's nothing else scheduled on the workqueue
anyways (which might well be the case. So we need numbers. :)


> Remove the hardcode to stop it from doing any harm. If this code is useful
> for others it should probably be reworked as a quirk table triggering only
> for individual systems that need it.


There are cards that need it. I don't know how many that are, but I own
an older 4306 PC-Card card that needs this.

So this effectively is a regression for this card.

So I don't think this is acceptable.
You should at least make this configurable via module parameter or such.
Or maybe the change from above already is enough. It should work for
your case.


> Signed-off-by: Lucas Stach 
> ---
>  drivers/net/wireless/broadcom/b43/leds.c | 26 ++
>  1 file changed, 2 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/net/wireless/broadcom/b43/leds.c 
> b/drivers/net/wireless/broadcom/b43/leds.c
> index d79ab2a..77d2dad 100644
> --- a/drivers/net/wireless/broadcom/b43/leds.c
> +++ b/drivers/net/wireless/broadcom/b43/leds.c
> @@ -224,31 +224,9 @@ static void b43_led_get_sprominfo(struct b43_wldev *dev,
>  
>   if (sprom[led_index] == 0xFF) {
>   /* There is no LED information in the SPROM
> -  * for this LED. Hardcode it here. */
> +  * for this LED. Keep it disabled. */
>   *activelow = false;
> - switch (led_index) {
> - case 0:
> - *behaviour = B43_LED_ACTIVITY;
> - *activelow = true;
> - if (dev->dev->board_vendor == PCI_VENDOR_ID_COMPAQ)
> - *behaviour = B43_LED_RADIO_ALL;
> - break;
> - case 1:
> - *behaviour = B43_LED_RADIO_B;
> - if (dev->dev->board_vendor == PCI_VENDOR_ID_ASUSTEK)
> - *behaviour = B43_LED_ASSOC;
> - break;
> - case 2:
> - *behaviour = B43_LED_RADIO_A;
> - break;
> - case 3:
> - *behaviour = B43_LED_OFF;
> - break;
> - default:
> - *behaviour = B43_LED_OFF;
> - B43_WARN_ON(1);
> - return;
> - }
> + *behaviour = B43_LED_OFF;
>   } else {
>   *behaviour = sprom[led_index] & B43_LED_BEHAVIOUR;
>   *activelow = !!(sprom[led_index] & B43_LED_ACTIVELOW);




-- 
Michael


pgpdRznyT6GTf.pgp
Description: OpenPGP digital signature

Re: [PATCH] b43: fix memory leak

[Michael Büsch]

On Fri, 19 Feb 2016 20:37:18 +0530
Sudip Mukherjee  wrote:

> > https://patchwork.kernel.org/patch/8049041/  
> 
> I have an old laptop running on 800Mhz CPU. It has "Broadcom BCM4311 
> [14e4:4311] (rev 01)".
> I will try to test it on this weekend.

Any news on this one?


-- 
Michael


pgpLl72Z376ek.pgp
Description: OpenPGP digital signature

Re: [PATCH] b43: fix memory leak

[Michael Büsch]

On Thu, 18 Feb 2016 18:04:36 +0530
Sudip Mukherjee  wrote:

> From: Sudip Mukherjee 
> 
> On error we jumped to the label bcma_out and returned the error code but
> we missed freeing dev.
> 
> Signed-off-by: Sudip Mukherjee 
> ---
>  drivers/net/wireless/broadcom/b43/main.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/net/wireless/broadcom/b43/main.c 
> b/drivers/net/wireless/broadcom/b43/main.c
> index c279211..78f670a 100644
> --- a/drivers/net/wireless/broadcom/b43/main.c
> +++ b/drivers/net/wireless/broadcom/b43/main.c
> @@ -5671,6 +5671,7 @@ static int b43_bcma_probe(struct bcma_device *core)
>   wl = b43_wireless_init(dev);
>   if (IS_ERR(wl)) {
>   err = PTR_ERR(wl);
> + kfree(dev);
>   goto bcma_out;
>   }
>  

We recently had a patch that fixes this, among more leaks. Subject:
[PATCH v2 resend] b43: Fix memory leaks in b43_bus_dev_ssb_init and
b43_bus_dev_bcma_init

Please test that patch instead, so we can finally apply it.

It needs to be tested on both ssb and bcma. Come on. This isn't too
hard. :) Please somebody with any hardware test it. (I currently don't
have any b43 hardware)

-- 
Michael


pgpkwnATLMZjW.pgp
Description: OpenPGP digital signature

Re: [PATCH] ssb: mark ssb_bus_register as __maybe_unused

[Michael Büsch]

On Tue, 03 Nov 2015 16:05:51 +0100
Arnd Bergmann  wrote:

> The SoC variant of the ssb code is now optional like the other
> ones, which means we can build the framwork without any
> front-end, but that results in a warning:
> 
> drivers/ssb/main.c:616:12: warning: 'ssb_bus_register' defined but not used 
> [-Wunused-function]
> 
> This annotates the ssb_bus_register function as __maybe_unused to
> shut up the warning. A configuration like this will not work on
> any hardware of course, but we still want this to silently build
> without warnings if the configuration is allowed in the first
> place.


Is there a simple way to disallow this configuration?

-- 
Michael


pgpwqvJa74ya6.pgp
Description: OpenPGP digital signature

Re: [PATCH] ssb: mark ssb_bus_register as __maybe_unused

[Michael Büsch]

On Tue, 03 Nov 2015 17:42:26 +0100
Arnd Bergmann <a...@arndb.de> wrote:

> On Tuesday 03 November 2015 17:27:21 Michael Büsch wrote:
> > On Tue, 03 Nov 2015 16:05:51 +0100
> > Arnd Bergmann <a...@arndb.de> wrote:
> >   
> > > The SoC variant of the ssb code is now optional like the other
> > > ones, which means we can build the framwork without any
> > > front-end, but that results in a warning:
> > > 
> > > drivers/ssb/main.c:616:12: warning: 'ssb_bus_register' defined but not 
> > > used [-Wunused-function]
> > > 
> > > This annotates the ssb_bus_register function as __maybe_unused to
> > > shut up the warning. A configuration like this will not work on
> > > any hardware of course, but we still want this to silently build
> > > without warnings if the configuration is allowed in the first
> > > place.  
> > 
> > 
> > Is there a simple way to disallow this configuration?  
> 
> I could not come up with a simple one. We could turn 'CONFIG_SSB' into
> a silent option and have it selected by each bus specific driver,
> but then we also have to change all the device drivers (usb and
> wireless I guess) to use 'depends on' rather than 'select'.

The other way around?
The drivers already select SSB. However I think we should have SSB
selectable by user for the embedded case.

But well, I can live with this patch then. Kalle, you might apply it
with

Acked-by: Michael Buesch <m...@bues.ch>

-- 
Michael


pgpmuggch9Wzm.pgp
Description: OpenPGP digital signature

Re: [PATCH v2 2/3] net: wireless: b43: Coding Style

[Michael Büsch]

On Mon, 19 Oct 2015 17:02:22 +0100
Paul McQuade  wrote:

> Fixed Pointer Coding Style
> 
> Signed-off-by: Paul McQuade 
> ---
>  drivers/net/wireless/b43/main.c | 6 +++---
>  drivers/net/wireless/b43/main.h | 2 +-
>  drivers/net/wireless/b43/xmit.h | 2 +-
>  3 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/wireless/b43/main.c b/drivers/net/wireless/b43/main.c
> index 2849070..040caa4 100644
> --- a/drivers/net/wireless/b43/main.c
> +++ b/drivers/net/wireless/b43/main.c
> @@ -364,7 +364,7 @@ static struct ieee80211_supported_band 
> b43_band_2ghz_limited = {
>  
>  static void b43_wireless_core_exit(struct b43_wldev *dev);
>  static int b43_wireless_core_init(struct b43_wldev *dev);
> -static struct b43_wldev * b43_wireless_core_stop(struct b43_wldev *dev);
> +static struct b43_wldev *b43_wireless_core_stop(struct b43_wldev *dev);
>  static int b43_wireless_core_start(struct b43_wldev *dev);
>  static void b43_op_bss_info_changed(struct ieee80211_hw *hw,
>   struct ieee80211_vif *vif,
> @@ -989,7 +989,7 @@ static void do_key_write(struct b43_wldev *dev,
>* 0x and let's b43_op_update_tkip_key provide a
>* correct pair.
>*/
> - rx_tkip_phase1_write(dev, index, 0x, (u16*)buf);
> + rx_tkip_phase1_write(dev, index, 0x, (u16 *)buf);
>   } else if (index >= pairwise_keys_start) /* clear it */
>   rx_tkip_phase1_write(dev, index, 0, NULL);
>   if (key)
> @@ -4334,7 +4334,7 @@ out_unlock:
>  /* Locking: wl->mutex
>   * Returns the current dev. This might be different from the passed in dev,
>   * because the core might be gone away while we unlocked the mutex. */
> -static struct b43_wldev * b43_wireless_core_stop(struct b43_wldev *dev)
> +static struct b43_wldev *b43_wireless_core_stop(struct b43_wldev *dev)
>  {
>   struct b43_wl *wl;
>   struct b43_wldev *orig_dev;
> diff --git a/drivers/net/wireless/b43/main.h b/drivers/net/wireless/b43/main.h
> index c46430c..fa56b22 100644
> --- a/drivers/net/wireless/b43/main.h
> +++ b/drivers/net/wireless/b43/main.h
> @@ -73,7 +73,7 @@ static inline int b43_is_ofdm_rate(int rate)
>  u8 b43_ieee80211_antenna_sanitize(struct b43_wldev *dev,
> u8 antenna_nr);
>  
> -void b43_tsf_read(struct b43_wldev *dev, u64 * tsf);
> +void b43_tsf_read(struct b43_wldev *dev, u64 *tsf);
>  void b43_tsf_write(struct b43_wldev *dev, u64 tsf);
>  
>  u32 b43_shm_read32(struct b43_wldev *dev, u16 routing, u16 offset);
> diff --git a/drivers/net/wireless/b43/xmit.h b/drivers/net/wireless/b43/xmit.h
> index ba61153..fed8657 100644
> --- a/drivers/net/wireless/b43/xmit.h
> +++ b/drivers/net/wireless/b43/xmit.h
> @@ -203,7 +203,7 @@ size_t b43_txhdr_size(struct b43_wldev *dev)
>  
>  
>  int b43_generate_txhdr(struct b43_wldev *dev,
> -u8 * txhdr,
> +u8 *txhdr,
>  struct sk_buff *skb_frag,
>  struct ieee80211_tx_info *txctl, u16 cookie);
>  


Acked-by: Michael Buesch 


-- 
Michael


pgpqDkIEI55nx.pgp
Description: OpenPGP digital signature

Re: [PATCH v2 3/3] net: wireless: b43: Statics are init to 0

[Michael Büsch]

On Mon, 19 Oct 2015 17:02:23 +0100
Paul McQuade  wrote:

> diff --git a/drivers/net/wireless/b43/phy_lp.c 
> b/drivers/net/wireless/b43/phy_lp.c
> index 058a9f2..086f0ba 100644
> --- a/drivers/net/wireless/b43/phy_lp.c
> +++ b/drivers/net/wireless/b43/phy_lp.c
> @@ -2502,7 +2502,7 @@ static int lpphy_b2063_tune(struct b43_wldev *dev,
>  {
>   struct ssb_bus *bus = dev->dev->sdev->bus;
>  
> - static const struct b206x_channel *chandata = NULL;
> + static const struct b206x_channel *chandata;
>   u32 crystal_freq = bus->chipco.pmu.crystalfreq * 1000;
>   u32 freqref, vco_freq, val1, val2, val3, timeout, timeoutref, count;
>   u16 old_comm15, scale;


Why on earth is this static anyway? That seems really wrong here.
The static should be removed (and the =NULL init be left in place.)


-- 
Michael


pgpYPirVq4UYc.pgp
Description: OpenPGP digital signature

Re: [PATCH 2/3] net: wireless: b43: Fixed Pointer issue

[Michael Büsch]

On Sun, 18 Oct 2015 00:01:37 +0100
Paul McQuade  wrote:

> Moved around pointer to avoid ERROR
> 
> Signed-off-by: Paul McQuade 
> ---
>  drivers/net/wireless/b43/dma.h  | 14 +++---
>  drivers/net/wireless/b43/main.c |  6 +++---
>  drivers/net/wireless/b43/main.h |  2 +-
>  drivers/net/wireless/b43/xmit.h |  2 +-
>  4 files changed, 12 insertions(+), 12 deletions(-)
>  
>  enum b43_dmatype {
> diff --git a/drivers/net/wireless/b43/main.c b/drivers/net/wireless/b43/main.c
> index 2849070..040caa4 100644
> --- a/drivers/net/wireless/b43/main.c
> +++ b/drivers/net/wireless/b43/main.c
> @@ -364,7 +364,7 @@ static struct ieee80211_supported_band 
> b43_band_2ghz_limited = {
>  
>  static void b43_wireless_core_exit(struct b43_wldev *dev);
>  static int b43_wireless_core_init(struct b43_wldev *dev);
> -static struct b43_wldev * b43_wireless_core_stop(struct b43_wldev *dev);
> +static struct b43_wldev *b43_wireless_core_stop(struct b43_wldev *dev);
>  static int b43_wireless_core_start(struct b43_wldev *dev);
>  static void b43_op_bss_info_changed(struct ieee80211_hw *hw,
>   struct ieee80211_vif *vif,


Can we have a sane commit message, please?
This is not an 'error' or an 'issue'. It's purely cosmetical and coding
style related.

-- 
Michael


pgpDahJqbAp9x.pgp
Description: OpenPGP digital signature

Re: [PATCH 1/1] ssb: remove unncessary out label

[Michael Büsch]

On Fri, 19 Jun 2015 14:23:45 +0530
Maninder Singh maninder...@samsung.com wrote:

 This patch removes unnecessary label out and
 some restructring for using device_create_file directly.
 
 Signed-off-by: Maninder Singh maninder...@samsung.com
 Reviewed-by: Rohit Thapliyal r.thapli...@samsung.com
 ---
  drivers/ssb/pci.c |8 +---
  1 files changed, 1 insertions(+), 7 deletions(-)
 
 diff --git a/drivers/ssb/pci.c b/drivers/ssb/pci.c
 index 0f28c08..d6ca4d3 100644
 --- a/drivers/ssb/pci.c
 +++ b/drivers/ssb/pci.c
 @@ -1173,17 +1173,11 @@ void ssb_pci_exit(struct ssb_bus *bus)
  int ssb_pci_init(struct ssb_bus *bus)
  {
   struct pci_dev *pdev;
 - int err;
  
   if (bus-bustype != SSB_BUSTYPE_PCI)
   return 0;
  
   pdev = bus-host_pci;
   mutex_init(bus-sprom_mutex);
 - err = device_create_file(pdev-dev, dev_attr_ssb_sprom);
 - if (err)
 - goto out;
 -
 -out:
 - return err;
 + return device_create_file(pdev-dev, dev_attr_ssb_sprom);
  }


I don't really think this change adds any value, but if you insist on
it you get my acked-by.


-- 
Michael


pgp8ErvjUlmpV.pgp
Description: OpenPGP digital signature

Re: [PATCH] ssb: fix handling of ssb_pmu_get_alp_clock()

[Michael Büsch]

On Sun,  7 Jun 2015 01:52:51 +0200
Hauke Mehrtens ha...@hauke-m.de wrote:

 Dan Carpenter reported missing brackets which resulted in reading a
 wrong crystalfreq value. I also noticed that the result of this
 function is ignored.
 
 Reported-By: Dan Carpenter dan.carpen...@oracle.com
 Signed-off-by: Hauke Mehrtens ha...@hauke-m.de
 ---
  drivers/ssb/driver_chipcommon_pmu.c | 6 +++---
  1 file changed, 3 insertions(+), 3 deletions(-)
 
 diff --git a/drivers/ssb/driver_chipcommon_pmu.c 
 b/drivers/ssb/driver_chipcommon_pmu.c
 index 0942841..c5352ea 100644
 --- a/drivers/ssb/driver_chipcommon_pmu.c
 +++ b/drivers/ssb/driver_chipcommon_pmu.c
 @@ -621,8 +621,8 @@ static u32 ssb_pmu_get_alp_clock_clk0(struct 
 ssb_chipcommon *cc)
   u32 crystalfreq;
   const struct pmu0_plltab_entry *e = NULL;
  
 - crystalfreq = chipco_read32(cc, SSB_CHIPCO_PMU_CTL) 
 -   SSB_CHIPCO_PMU_CTL_XTALFREQ  
 SSB_CHIPCO_PMU_CTL_XTALFREQ_SHIFT;
 + crystalfreq = (chipco_read32(cc, SSB_CHIPCO_PMU_CTL) 
 +SSB_CHIPCO_PMU_CTL_XTALFREQ)   
 SSB_CHIPCO_PMU_CTL_XTALFREQ_SHIFT;
   e = pmu0_plltab_find_entry(crystalfreq);
   BUG_ON(!e);
   return e-freq * 1000;
 @@ -634,7 +634,7 @@ u32 ssb_pmu_get_alp_clock(struct ssb_chipcommon *cc)
  
   switch (bus-chip_id) {
   case 0x5354:
 - ssb_pmu_get_alp_clock_clk0(cc);
 + return ssb_pmu_get_alp_clock_clk0(cc);
   default:
   ssb_err(ERROR: PMU alp clock unknown for device %04X\n,
   bus-chip_id);

Looks good.
Signed-off-by: Michael Buesch m...@bues.ch

Can some MIPS people take this, please?

-- 
Michael


pgpVFgYd75oLR.pgp
Description: OpenPGP digital signature