Re: Regression: bcm4312 lp-phy device not working in 4.17-rc5 kernel

[Michael Büsch]

On Mon, 14 May 2018 19:55:39 +0200
Michael Büsch <m...@bues.ch> wrote:

> On Mon, 14 May 2018 18:49:53 +0100
> Chris Vine <vine35792...@gmail.com> wrote:
> 
> > I have an old netbook with a Broadcom bcm4312 802.11b/g lp-phy
> > [14e4:4315] (rev 01) wifi device.  This works up to and including the
> > 4.16 kernel, but not with 4.17-rc5.
> > 
> > The b43, mac80211, cfg80211, ssb and mmc_core modules are loaded OK, as
> > is the firmware, and the wlan0 interface will come up, but any attempt
> > to use the interface fails and it cannot (for example) scan.
> > 
> > No useful error messages are given.  The interface (wlan0) is just
> > reported as not being ready.  
> 
> Hi,
> 
> thanks for your report.
> 
> Can you please provide all kernel log messages anyway?
> And what does "not ready" mean exactly?
> 
> It would be extremely helpful if you'd do a git bisect to find the
> commit that broke it.
> 

Ok, I just noticed that mainline still contains the ssb breakage.

So you are most likely hitting this:
https://patchwork.kernel.org/patch/10393729/


-- 
Michael


pgpFvz8XhdUlG.pgp
Description: OpenPGP digital signature

Re: Regression: bcm4312 lp-phy device not working in 4.17-rc5 kernel

[Michael Büsch]

On Mon, 14 May 2018 18:49:53 +0100
Chris Vine  wrote:

> I have an old netbook with a Broadcom bcm4312 802.11b/g lp-phy
> [14e4:4315] (rev 01) wifi device.  This works up to and including the
> 4.16 kernel, but not with 4.17-rc5.
> 
> The b43, mac80211, cfg80211, ssb and mmc_core modules are loaded OK, as
> is the firmware, and the wlan0 interface will come up, but any attempt
> to use the interface fails and it cannot (for example) scan.
> 
> No useful error messages are given.  The interface (wlan0) is just
> reported as not being ready.

Hi,

thanks for your report.

Can you please provide all kernel log messages anyway?
And what does "not ready" mean exactly?

It would be extremely helpful if you'd do a git bisect to find the
commit that broke it.

-- 
Michael


pgpSMDNxSWKvg.pgp
Description: OpenPGP digital signature

Re: [PATCH V2 1/2] Revert "ssb: Prevent build of PCI host features in module"

[Michael Büsch]

On Sat, 12 May 2018 12:00:07 +0200
Rafał Miłecki  wrote:

> > Yes, I'm OK with the patch, if we have a third patch that cleans up the
> > PCI_DRIVERS_LEGACY dependency by moving it to SSB_PCICORE_HOSTMODE
> > where it belongs. (This doesn't need to go into the stable tree.)
> > We currently implicitly get that via dependency chain, so this is OK
> > for now as-is.  
> 
> I'm planning to handle PCI_DRIVERS_LEGACY cleanup once my patches hit
> net-next.git and then wireless-drivers-next.git. It's to avoid
> conflicts.

Yes, thanks. Take your time. We're not in a hurry. :)
This change should not make a functional difference.

-- 
Michael


pgpglTvJQFeD4.pgp
Description: OpenPGP digital signature

Re: [PATCH V2 1/2] Revert "ssb: Prevent build of PCI host features in module"

[Michael Büsch]

On Sat, 12 May 2018 10:50:42 +0300
Kalle Valo  wrote:

> Larry Finger  writes:
> 
> > On 05/11/2018 05:13 AM, Kalle Valo wrote:  
> >> Rafał Miłecki  writes:
> >>  
> >>> On 11 May 2018 at 11:17, Rafał Miłecki  wrote:  
>  [...]  
> >>>
> >>> As these patches fix regression/build error, I believe both should get
> >>> into 4.17.  
> >>
> >> How much confidence do we have that we don't need to end up reverting
> >> patch 2 as well? I rather be pushing patch 2 to 4.18 so that there's
> >> more time for testing and waiting for feedback.  
> >
> > Although I do not have the hardware to test the builds, I worked
> > closely with the OP in the bug at b.r.c noted above. From that effort,
> > it became clear what configuration variables were missing to cause the
> > x86 failures. Patch 2 satisfies the requirement, and prevents the
> > build problems found by the MIPS users. Both patches are needed in
> > 4.17.  
> 
> And I assume Michael is ok with this approach as well as I haven't heard
> from him. I'll then push both of these to 4.17.
> 

Yes, I'm OK with the patch, if we have a third patch that cleans up the
PCI_DRIVERS_LEGACY dependency by moving it to SSB_PCICORE_HOSTMODE
where it belongs. (This doesn't need to go into the stable tree.)
We currently implicitly get that via dependency chain, so this is OK
for now as-is.

-- 
Michael


pgpP2GzSA5_HQ.pgp
Description: OpenPGP digital signature

Re: [PATCH 4.17 2/2] ssb: make SSB_PCICORE_HOSTMODE depend on SSB = y

[Michael Büsch]

On Thu, 10 May 2018 13:20:01 +0200
Rafał Miłecki <zaj...@gmail.com> wrote:

> On 10 May 2018 at 13:17, Michael Büsch <m...@bues.ch> wrote:
> > On Thu, 10 May 2018 13:14:01 +0200
> > Rafał Miłecki <zaj...@gmail.com> wrote:
> >  
> >> From: Rafał Miłecki <ra...@milecki.pl>
> >>
> >> SSB_PCICORE_HOSTMODE protects MIPS specific code that calls not exported
> >> symbols pcibios_enable_device and register_pci_controller. This code is
> >> supposed to be compiled only with ssb builtin.
> >>
> >> This fixes:
> >> ERROR: "pcibios_enable_device" [drivers/ssb/ssb.ko] undefined!
> >> ERROR: "register_pci_controller" [drivers/ssb/ssb.ko] undefined!
> >> make[1]: *** [scripts/Makefile.modpost:92: __modpost] Error 1
> >>
> >> Signed-off-by: Rafał Miłecki <ra...@milecki.pl>
> >> ---
> >>  drivers/ssb/Kconfig | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/ssb/Kconfig b/drivers/ssb/Kconfig
> >> index b3f5cae98ea6..c574dd210500 100644
> >> --- a/drivers/ssb/Kconfig
> >> +++ b/drivers/ssb/Kconfig
> >> @@ -131,7 +131,7 @@ config SSB_DRIVER_PCICORE
> >>
> >>  config SSB_PCICORE_HOSTMODE
> >>   bool "Hostmode support for SSB PCI core"
> >> - depends on SSB_DRIVER_PCICORE && SSB_DRIVER_MIPS
> >> + depends on SSB_DRIVER_PCICORE && SSB_DRIVER_MIPS && SSB = y
> >>   help
> >> PCIcore hostmode operation (external PCI bus).
> >>  
> >
> >
> > I think we also need to depend on PCI_DRIVERS_LEGACY.
> > See the other patch that floats around.  
> 
> I believe it's already handled by SSB_PCIHOST_POSSIBLE's dependency on
> PCI_DRIVERS_LEGACY.


That dependency seems to be wrong there.
Was it added among some other "let's just unbreak some random
build" change as well?

SSB_PCIHOST enables support for SSB on top of PCI. (Which is 99% of it
uses). I don't see how this uses the legacy API.

SSB_PCICORE_HOSTMODE enables PCI on top of SSB. Which is a MIPS corner
case. This uses the legacy MIPS API to register a PCI bus.

-- 
Michael


pgpuKFmEADZfs.pgp
Description: OpenPGP digital signature

Re: [PATCH 4.17 2/2] ssb: make SSB_PCICORE_HOSTMODE depend on SSB = y

[Michael Büsch]

On Thu, 10 May 2018 13:14:01 +0200
Rafał Miłecki  wrote:

> From: Rafał Miłecki 
> 
> SSB_PCICORE_HOSTMODE protects MIPS specific code that calls not exported
> symbols pcibios_enable_device and register_pci_controller. This code is
> supposed to be compiled only with ssb builtin.
> 
> This fixes:
> ERROR: "pcibios_enable_device" [drivers/ssb/ssb.ko] undefined!
> ERROR: "register_pci_controller" [drivers/ssb/ssb.ko] undefined!
> make[1]: *** [scripts/Makefile.modpost:92: __modpost] Error 1
> 
> Signed-off-by: Rafał Miłecki 
> ---
>  drivers/ssb/Kconfig | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/ssb/Kconfig b/drivers/ssb/Kconfig
> index b3f5cae98ea6..c574dd210500 100644
> --- a/drivers/ssb/Kconfig
> +++ b/drivers/ssb/Kconfig
> @@ -131,7 +131,7 @@ config SSB_DRIVER_PCICORE
>  
>  config SSB_PCICORE_HOSTMODE
>   bool "Hostmode support for SSB PCI core"
> - depends on SSB_DRIVER_PCICORE && SSB_DRIVER_MIPS
> + depends on SSB_DRIVER_PCICORE && SSB_DRIVER_MIPS && SSB = y
>   help
> PCIcore hostmode operation (external PCI bus).
>  


I think we also need to depend on PCI_DRIVERS_LEGACY.
See the other patch that floats around.


-- 
Michael


pgp6YTKn0ZUg2.pgp
Description: OpenPGP digital signature

Re: [PATCH v2] drivers/net/wireless/broadcom/b43: fix transmit failure when VT is switched

[Michael Büsch]

On Thu, 10 May 2018 19:30:45 +0900
Taketo Kabe <k...@sra-tohoku.co.jp> wrote:

> Signed-off-by: Taketo Kabe <k...@sra-tohoku.co.jp>
> ---
> diff -up ./drivers/net/wireless/broadcom/b43/dma.c.b43 
> ./drivers/net/wireless/broadcom/b43/dma.c
> --- ./drivers/net/wireless/broadcom/b43/dma.c.b43 2018-05-04 
> 15:18:12.0 +0900
> +++ ./drivers/net/wireless/broadcom/b43/dma.c 2018-05-10 18:46:36.0 
> +0900
> @@ -1484,7 +1484,7 @@ void b43_dma_handle_txstatus(struct b43_
>   int slot, firstused;
>   bool frame_succeed;
>   int skip;
> - static u8 err_out1, err_out2;
> + static u8 err_out1;
> 
>   ring = parse_cookie(dev, status->cookie, );
>   if (unlikely(!ring))
> @@ -1518,13 +1518,13 @@ void b43_dma_handle_txstatus(struct b43_
>   }
>   } else {
>   /* More than a single header/data pair were missed.
> -  * Report this error once.
> +  * Report this error, and reset the controller to
> +  * revive operation.
>*/
> - if (!err_out2)
> - b43dbg(dev->wl,
> -"Out of order TX status report on DMA 
> ring %d. Expected %d, but got %d\n",
> -ring->index, firstused, slot);
> - err_out2 = 1;
> + b43dbg(dev->wl,
> +"Out of order TX status report on DMA ring %d. 
> Expected %d, but got %d\n",
> +ring->index, firstused, slot);
> +     b43_controller_restart(dev, "Out of order TX");
>   return;
>   }
>   }


Reviewed-by: Michael Büsch <m...@bues.ch>

I think this is a good thing to have.
It improves robustness against firmware/DMA misbehavior.

-- 
Michael


pgpcNqd0vtBr1.pgp
Description: OpenPGP digital signature

Re: Regression caused by commit 882164a4a928

[Michael Büsch]

On Thu, 10 May 2018 11:24:12 +0100
Matt Redfearn  wrote:

> > Could you please try this?
> > 
> > config SSB_DRIVER_PCICORE_POSSIBLE
> > depends on SSB_PCIHOST
> > 
> > config SSB_PCICORE_HOSTMODE
> > depends on SSB_DRIVER_PCICORE && SSB_DRIVER_MIPS && (SSB = y) && 
> > PCI_DRIVERS_LEGACY
> > 
> > 
> > The affected API pcibios_enable_device() and register_pci_controller()
> > is only used in HOSTMODE. So I think it makes sense to make HOSTMODE
> > depend on SSB=y and PCI_DRIVERS_LEGACY.
> > 
> > PCICore itself does not use the API, if hostmode is disabled.
> >   
> 
> Sure - I've tested the patch:
> 
> --- a/drivers/ssb/Kconfig
> +++ b/drivers/ssb/Kconfig
> @@ -117,7 +117,7 @@ config SSB_SERIAL
> 
>   config SSB_DRIVER_PCICORE_POSSIBLE
>  bool
> -   depends on SSB_PCIHOST && SSB = y
> +   depends on SSB_PCIHOST
>  default y
> 
>   config SSB_DRIVER_PCICORE
> @@ -131,7 +131,7 @@ config SSB_DRIVER_PCICORE
> 
>   config SSB_PCICORE_HOSTMODE
>  bool "Hostmode support for SSB PCI core"
> -   depends on SSB_DRIVER_PCICORE && SSB_DRIVER_MIPS
> +   depends on SSB_DRIVER_PCICORE && SSB_DRIVER_MIPS && (SSB = y) && 
> PCI_DRIVERS_LEGACY
>  help
>PCIcore hostmode operation (external PCI bus).
> 
> 
> And this seems to work for MIPS, we don't get the build error from 
> building the SSB module under nec_markeins allmodconfig, and 
> SSB_PCICORE_HOSTMODE=y for bcm47xx allmodconfig, which selects SSB=y.
> 
> So this looks like a good fix for MIPS, at least.
> 
> Tested-by: Matt Redfearn 


Thanks.
Could you please submit it?
You can add my Acked-by.

-- 
Michael


pgpyBllLXxWNs.pgp
Description: OpenPGP digital signature

Re: Regression caused by commit 882164a4a928

[Michael Büsch]

On Wed, 9 May 2018 13:55:43 +0100
Matt Redfearn  wrote:

> Hi Larry
> 
> On 07/05/18 16:44, Larry Finger wrote:
> > Matt,
> > 
> > Although commit 882164a4a928 ("ssb: Prevent build of PCI host features 
> > in module") appeared to be harmless, it leads to complete failure of 
> > drivers b43. and b43legacy, and likely affects b44 as well. The problem 
> > is that CONFIG_SSB_PCIHOST is undefined, which prevents the compilation 
> > of the code that controls the PCI cores of the device. See 
> > https://bugzilla.redhat.com/show_bug.cgi?id=1572349 for details.  
> 
> Sorry for the breakage :-/
> 
> > 
> > As the underlying errors ("pcibios_enable_device" undefined, and 
> > "register_pci_controller" undefined) do not appear on the architectures 
> > that I have tested (x86_64, x86, and ppc), I suspect something in the 
> > arch-specific code for your setup (MIPS?). As I have no idea on how to 
> > fix that problem, would the following patch work for you?
> > 
> > diff --git a/drivers/ssb/Kconfig b/drivers/ssb/Kconfig
> > index 9371651d8017..3743533c8057 100644
> > --- a/drivers/ssb/Kconfig
> > +++ b/drivers/ssb/Kconfig
> > @@ -117,7 +117,7 @@ config SSB_SERIAL
> > 
> >   config SSB_DRIVER_PCICORE_POSSIBLE
> >      bool
> > -   depends on SSB_PCIHOST && SSB = y
> > +   depends on SSB_PCIHOST && (SSB = y || !MIPS)
> >      default y
> > 
> >   config SSB_DRIVER_PCICORE  
> 
> I believe that the problem stems from these drivers being used for some 
> wireless AP functionality built into some MIPS based SoCs. The Kconfig 
> rules sort out building this additional functionality when configured 
> for MIPS (in a round about sort of way), but it allowed it even when SSB 
> is a module, leading to build failures. My patch was intended to prevent 
> that.
> 
> There was a similar issue in the same Kconfig file, introduced by 
> c5611df96804 and fixed by a9e6d44ddecc. It was fixed the same way as you 
> suggest. I've tested the above patch and it does work for MIPS 
> (preventing the PCICORE being built into the module).
> 
> Tested-by: Matt Redfearn 


Could you please try this?

config SSB_DRIVER_PCICORE_POSSIBLE
depends on SSB_PCIHOST

config SSB_PCICORE_HOSTMODE
depends on SSB_DRIVER_PCICORE && SSB_DRIVER_MIPS && (SSB = y) && 
PCI_DRIVERS_LEGACY


The affected API pcibios_enable_device() and register_pci_controller()
is only used in HOSTMODE. So I think it makes sense to make HOSTMODE
depend on SSB=y and PCI_DRIVERS_LEGACY.

PCICore itself does not use the API, if hostmode is disabled.

-- 
Michael


pgpH8hefzhTTn.pgp
Description: OpenPGP digital signature

Re: Regression caused by commit 882164a4a928

[Michael Büsch]

On Mon, 07 May 2018 22:03:58 +0300
Kalle Valo <kv...@codeaurora.org> wrote:

> Michael Büsch <m...@bues.ch> writes:
> 
> > On Mon, 7 May 2018 10:44:34 -0500
> > Larry Finger <larry.fin...@lwfinger.net> wrote:
> >  
> >> Although commit 882164a4a928 ("ssb: Prevent build of PCI host features in 
> >> module") appeared to be harmless, it leads to complete failure of drivers 
> >> b43.   
> >  
> >>   config SSB_DRIVER_PCICORE_POSSIBLE
> >>  bool
> >> -   depends on SSB_PCIHOST && SSB = y
> >> +   depends on SSB_PCIHOST && (SSB = y || !MIPS)
> >>  default y
> >> 
> >>   config SSB_DRIVER_PCICORE  
> >
> >
> > https://patchwork.kernel.org/patch/10161131/
> >
> > Could we _please_ switch to not applying patches to ssb or b43, if
> > nobody acked (or better reviewed) a patch?
> >
> > We had multiple changes to ssb and b43 in the recent past that did not
> > have a review at all and broke something. I don't think such software
> > quality is acceptable at all.
> > So please revert 882164a4a928.  
> 
> Yes, someone please send a revert so that this can be fixed quickly for
> v4.17.

Uhm, can you just type git revert 882164a4a928? :)
Or do I have to send you a pull request?

> > I'm sorry that this patch slipped through the cracks of my inbox.
> > But the reaction to that shall not be to just apply the patch. It
> > shall be to resubmit it for review.  
> 
> The thing is that in general I do not have time to ping people for every
> patch, I get enough of emails as is. If there are no review comments I
> have to assume the patch is ok to apply.

Yes, I understand that pinging people can be annoying and time
consuming. But we have tools like patchwork. Why isn't pinging
(semi)automated? Patchwork should really track the review status of a
patch.
I think the concept of no-comments = everything-ok is
fundamentally broken. But it has always been that way for wireless and
lots of other subsystems.

> But as ssb has had two major regressions recently I'm going to
> significantly raise the bar for ssb patches, and will refuse to apply
> random patches if they have not been tested with b43/b44.

Thanks.

-- 
Michael


pgpx8JVQIFwMB.pgp
Description: OpenPGP digital signature

Re: Regression caused by commit 882164a4a928

[Michael Büsch]

On Mon, 7 May 2018 10:44:34 -0500
Larry Finger  wrote:

> Although commit 882164a4a928 ("ssb: Prevent build of PCI host features in 
> module") appeared to be harmless, it leads to complete failure of drivers 
> b43. 

>   config SSB_DRIVER_PCICORE_POSSIBLE
>  bool
> -   depends on SSB_PCIHOST && SSB = y
> +   depends on SSB_PCIHOST && (SSB = y || !MIPS)
>  default y
> 
>   config SSB_DRIVER_PCICORE


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

Could we _please_ switch to not applying patches to ssb or b43, if
nobody acked (or better reviewed) a patch?

We had multiple changes to ssb and b43 in the recent past that did not
have a review at all and broke something. I don't think such software
quality is acceptable at all.
So please revert 882164a4a928.

I'm sorry that this patch slipped through the cracks of my inbox.
But the reaction to that shall not be to just apply the patch. It
shall be to resubmit it for review.



But back to the technical topic.
I don't remember why SSB_DRIVER_PCICORE_POSSIBLE depends on SSB_PCIHOST.
But that looks and feels wrong.

I would say it should rather look like

config SSB_DRIVER_PCICORE_POSSIBLE
depends on SSB && (PCI = y || PCI = SSB)

completely untested, though.

-- 
Michael


pgpqzKxv24XQl.pgp
Description: OpenPGP digital signature

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


pgpHWZAi1Njhu.pgp
Description: OpenPGP digital signature

Re: [PATCH v2] ssb: use put_device() if device_register fail

[Michael Büsch]

On Thu,  8 Mar 2018 10:39:49 +0530
Arvind Yadav  wrote:

> Never directly free @dev after calling device_register(), even
> if it returned an error! Always use put_device() to give up the
> reference initialized.
> 
> Signed-off-by: Arvind Yadav 
> ---
> changes in v2:
>  Removed kfree() call for @dev.
> 
>  drivers/ssb/main.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/ssb/main.c b/drivers/ssb/main.c
> index 65420a9..a7a062b 100644
> --- a/drivers/ssb/main.c
> +++ b/drivers/ssb/main.c
> @@ -521,8 +521,8 @@ static int ssb_devices_register(struct ssb_bus *bus)
>   ssb_err("Could not register %s\n", dev_name(dev));
>   /* Set dev to NULL to not unregister
>* dev on error unwinding. */
> + put_device(dev);
>   sdev->dev = NULL;
> - kfree(devwrap);
>   goto error;
>   }
>   dev_idx++;


Would you please put the put_device where the kfree was?
So that the comment still matches.
Thanks.


-- 
Michael


pgpH2DNLDdqWB.pgp
Description: OpenPGP digital signature

Re: [PATCH] ssb:: use put_device() if device_register fail

[Michael Büsch]

On Wed, 7 Mar 2018 22:46:14 +0530
arvindY  wrote:
> >> diff --git a/drivers/ssb/main.c b/drivers/ssb/main.c
> >> index 65420a9..c4449e0 100644
> >> --- a/drivers/ssb/main.c
> >> +++ b/drivers/ssb/main.c
> >> @@ -521,6 +521,7 @@ static int ssb_devices_register(struct ssb_bus *bus)
> >>ssb_err("Could not register %s\n", dev_name(dev));
> >>/* Set dev to NULL to not unregister
> >> * dev on error unwinding. */
> >> +  put_device(dev);
> >>sdev->dev = NULL;
> >>kfree(devwrap);
> >>goto error;  
> >
> > I don't think this is correct.
> > The dev structure is allocated as part of devwrap, which is freed here.
> >
> > Why do you think we need put_device here?
> >  
> Yes this patch is not correct, We must not use kfree() after you called 
> device_register() (even
> if it was not successful!) -- see the comment for device_register().
> I will delete kfree() and send updated patch.


Is device_put() going to call ssb_release_dev() to free the structure?

Can you please elaborate on why device_put() must be used? The comment
is not really of any use here.

-- 
Michael


pgp9IEPbSlMjr.pgp
Description: OpenPGP digital signature

Re: [PATCH] ssb:: use put_device() if device_register fail

[Michael Büsch]

On Wed,  7 Mar 2018 15:31:30 +0530
Arvind Yadav  wrote:

> if device_register() returned an error! Always use put_device()
> to give up the reference initialized.
> 
> Signed-off-by: Arvind Yadav 
> ---
>  drivers/ssb/main.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/ssb/main.c b/drivers/ssb/main.c
> index 65420a9..c4449e0 100644
> --- a/drivers/ssb/main.c
> +++ b/drivers/ssb/main.c
> @@ -521,6 +521,7 @@ static int ssb_devices_register(struct ssb_bus *bus)
>   ssb_err("Could not register %s\n", dev_name(dev));
>   /* Set dev to NULL to not unregister
>* dev on error unwinding. */
> + put_device(dev);
>   sdev->dev = NULL;
>   kfree(devwrap);
>   goto error;


I don't think this is correct.
The dev structure is allocated as part of devwrap, which is freed here.

Why do you think we need put_device here?

-- 
Michael


pgp9lFUSVai7N.pgp
Description: OpenPGP digital signature

Re: B43 driver no longer works in Linux 4.15 (bisected)

[Michael Büsch]

On Mon, 5 Feb 2018 13:14:28 -0500
Adric Blake  wrote:

> In the time between Linux 4.15-rc8 and -rc9, my wireless driver, b43, would
> no longer load automatically. When I modprobe the b43 (and ssb) modules,
> the device still didn't appear in NetworkManager. Comparing the kernel logs
> between working (4.14.16-lts) and nonworking (4.15) kernels reveals that
> there is zero output from the ssb module, and no devices recognized by the
> b43 driver:


Thanks for your bug report.
A fix for this exists already:
https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/wireless-drivers.git/commit/?id=a9e6d44ddeccd3522670e641f1ed9b068e746ff7

-- 
Michael


pgppKiWy8UTXl.pgp
Description: OpenPGP digital signature

Re: [BUG] ssb: Possible sleep-in-atomic bugs in ssb_pcmcia_read8

[Michael Büsch]

On Mon, 9 Oct 2017 09:29:17 +0800
Jia-Ju Bai  wrote:

> According to pcmcia.c, the driver may sleep under a spinlock.
> The function call paths are:
> ssb_pcmcia_read8 (acquire the spinlock)
>select_core_and_segment
>  ssb_pcmcia_switch_segment
>ssb_pcmcia_cfg_write
>  pcmcia_write_config_byte
>pcmcia_access_config (drivers/pcmcia/pcmcia_resource.c)
>  mutex_lock --> may sleep
> 
> ssb_pcmcia_read8 (acquire the spinlock)
>select_core_and_segment
>  ssb_pcmcia_switch_segment
>sssb_pcmcia_cfg_read
>  pcmcia_read_config_byte
>pcmcia_access_config (drivers/pcmcia/pcmcia_resource.c)
>  mutex_lock --> may sleep
> 
> A possible fix is to use spinlock instead of mutex lock in 
> pcmcia_access_config in drivers/pcmcia/pcmcia_resource.c.
> 
> These bugs are found by my static analysis tool and my code review.



Thanks for scanning and your resulting bug notification.
I currently don't have the hardware at hand to develop and test a
proper fix for this.
That said, I'm not so sure anymore why bar_lock is a spinlock instead
of a mutex. It might be possible to convert this to mutex.

I will try to look into this.

-- 
Michael


pgpYQl0jBhPrU.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


pgpx0sLEHOMzB.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


pgpKiCc9zRaf4.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


pgpbrN75q7eC0.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


pgpwpTCkTnyIz.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


pgpLflSNCwtUb.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


pgp07Gnx5plxH.pgp
Description: OpenPGP digital signature

Re: [PATCH] ssb: Delete an error message for a failed memory allocation in ssb_devices_register()

[Michael Büsch]

On Wed, 17 May 2017 18:22:49 +0200
SF Markus Elfring <elfr...@users.sourceforge.net> wrote:

> From: Markus Elfring <elfr...@users.sourceforge.net>
> Date: Wed, 17 May 2017 18:12:16 +0200
> 
> Omit an extra message for a memory allocation failure in this function.
> 
> This issue was detected by using the Coccinelle software.
> 
> Link: 
> http://events.linuxfoundation.org/sites/events/files/slides/LCJ16-Refactor_Strings-WSang_0.pdf
> Signed-off-by: Markus Elfring <elfr...@users.sourceforge.net>
> ---
>  drivers/ssb/main.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/ssb/main.c b/drivers/ssb/main.c
> index d1a750760cf3..65420a9f0e82 100644
> --- a/drivers/ssb/main.c
> +++ b/drivers/ssb/main.c
> @@ -480,7 +480,6 @@ static int ssb_devices_register(struct ssb_bus *bus)
>  
>   devwrap = kzalloc(sizeof(*devwrap), GFP_KERNEL);
>   if (!devwrap) {
> - ssb_err("Could not allocate device\n");
>   err = -ENOMEM;
>   goto error;
>   }


This looks good.

Acked-by: Michael Büsch <m...@bues.ch>


-- 
Michael


pgpFfsUCp_TBq.pgp
Description: OpenPGP digital signature

Re: [PATCH] ssb: main.c: This patch removes unnecessary return statement using spatch tool

[Michael Büsch]

> > @@ -1272,9 +1272,7 @@ u32 ssb_admatch_size(u32 adm)
> > default:
> > SSB_WARN_ON(1);
> > }
> > -   size = (1 << (size + 1));
> > -
> > -   return size;
> > +   return (1 << (size + 1));
> >  }
> >  EXPORT_SYMBOL(ssb_admatch_size);  

I'm all for cleaning up code, but I don't really see how this
change improves the code.

-- 
Michael


pgpIx1qT9ED5h.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


pgpC6lIyUs1I_.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


pgpXSZ2AS2Xgt.pgp
Description: OpenPGP digital signature

Re: [PATCH] drivers: ssb: Change bare unsigned to unsigned int to suit coding style

[Michael Büsch]

On Sat,  4 Jun 2016 12:50:05 +0100
Hugh Sipière  wrote:

> These lines just have unsigned gpio rather than unsigned int gpio.
> I changed it to suit the coding style.
> 
> Signed-off-by: Hugh Sipière 

Acked-by: Michael Buesch 

Please send this to the MIPS tree, because this basically is MIPS-only
code:

MIPS
M:  Ralf Baechle 
L:  linux-m...@linux-mips.org


-- 
Michael


pgprMkyfNGSBd.pgp
Description: OpenPGP digital signature

Re: [PATCH] drivers: ssb: Fix comments to suit coding style

[Michael Büsch]

On Sat,  4 Jun 2016 12:32:13 +0100
Hugh Sipière  wrote:

> I changed it so that these two comments do not end on a line with text.
> 
> Signed-off-by: Hugh Sipière 
> ---
>  drivers/ssb/driver_gpio.c | 6 --
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/ssb/driver_gpio.c b/drivers/ssb/driver_gpio.c
> index 180e027..f2435f3 100644
> --- a/drivers/ssb/driver_gpio.c
> +++ b/drivers/ssb/driver_gpio.c
> @@ -231,7 +231,8 @@ static int ssb_gpio_chipco_init(struct ssb_bus *bus)
>   chip->ngpio = 16;
>   /* There is just one SoC in one device and its GPIO addresses should be
>* deterministic to address them more easily. The other buses could get
> -  * a random base number. */
> +  * a random base number.
> +  */
>   if (bus->bustype == SSB_BUSTYPE_SSB)
>   chip->base  = 0;
>   else
> @@ -424,7 +425,8 @@ static int ssb_gpio_extif_init(struct ssb_bus *bus)
>   chip->ngpio = 5;
>   /* There is just one SoC in one device and its GPIO addresses should be
>* deterministic to address them more easily. The other buses could get
> -  * a random base number. */
> +  * a random base number.
> +  */
>   if (bus->bustype == SSB_BUSTYPE_SSB)
>   chip->base  = 0;
>   else


What's the benefit from this change?
Will the next person submit a patch to remove wasted lines that just
contain a */?


-- 
Michael


pgpt0LkZKe3iZ.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


pgp1mgBgFViAO.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


pgpSoZWK1SGwH.pgp
Description: OpenPGP digital signature

Re: [PATCH] b43: only hardcode LED behavior if SPROM doesn't encode any

[Michael Büsch]

On Fri,  3 Jun 2016 23:04:03 +0200
Lucas Stach  wrote:

> Only hardcode the LED behavior if the SROM doesn't provide any for all
> LEDs of the card. This avoids instantiating LED triggers for unconnected
> LEDs, while (hopefully) keeping things working for old cards with a
> blank SROM.
> 
> Signed-off-by: Lucas Stach 

Acked-by: Michael Buesch 


> ---
>  drivers/net/wireless/broadcom/b43/leds.c | 8 ++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/wireless/broadcom/b43/leds.c 
> b/drivers/net/wireless/broadcom/b43/leds.c
> index d79ab2a..cb987c2 100644
> --- a/drivers/net/wireless/broadcom/b43/leds.c
> +++ b/drivers/net/wireless/broadcom/b43/leds.c
> @@ -222,7 +222,7 @@ static void b43_led_get_sprominfo(struct b43_wldev *dev,
>   sprom[2] = dev->dev->bus_sprom->gpio2;
>   sprom[3] = dev->dev->bus_sprom->gpio3;
>  
> - if (sprom[led_index] == 0xFF) {
> + if ((sprom[0] & sprom[1] & sprom[2] & sprom[3]) == 0xff) {
>   /* There is no LED information in the SPROM
>* for this LED. Hardcode it here. */
>   *activelow = false;
> @@ -250,7 +250,11 @@ static void b43_led_get_sprominfo(struct b43_wldev *dev,
>   return;
>   }
>   } else {
> - *behaviour = sprom[led_index] & B43_LED_BEHAVIOUR;
> + /* keep LED disabled if no mapping is defined */
> + if (sprom[led_index] == 0xff)
> + *behaviour = B43_LED_OFF;
> + else
> + *behaviour = sprom[led_index] & B43_LED_BEHAVIOUR;
>   *activelow = !!(sprom[led_index] & B43_LED_ACTIVELOW);
>   }
>  }




-- 
Michael


pgpFcNHStfkhI.pgp
Description: OpenPGP digital signature

Re: BCM4331 reset leads to wl lockup

[Michael Büsch]

On Thu, 26 May 2016 14:12:10 +0200
Lukas Wunner  wrote:

> + mmio = early_ioremap(addr, BCM4331_MMIO_SIZE);
> + if (!mmio) {
> + pr_err("Cannot iomap Apple AirPort card\n");
> + return;
> + }
> + pr_info("Resetting Apple AirPort card\n");
> + iowrite32(BCMA_RESET_CTL_RESET,
> +   mmio + (1 * BCMA_CORE_SIZE) + BCMA_RESET_CTL);
> + early_iounmap(mmio, BCM4331_MMIO_SIZE);

Just writing that bit is not the correct reset procedure.
So it might cause problems depending on how wl does the core reset
later.

Please try this:
- wait for BCMA_RESET_ST to be 0
- set reset bit
- flush
- wait 1us
- reset reset bit
- flush
- wait 10us

See bcma_core_disable()

-- 
Michael


pgpcEp3gXCD9F.pgp
Description: OpenPGP digital signature

Re: [b43] don't unconditionally fall back to CCK if the rate is 6MB OFDM.

[Michael Büsch]

On Sun, 15 May 2016 07:23:25 -0700
Adrian Chadd  wrote:

> @@ -438,7 +446,7 @@ int b43_generate_txhdr(struct b43_wldev *dev,
> 
> rts_rate = rts_cts_rate ? rts_cts_rate->hw_value :
> B43_CCK_RATE_1MB;

I just noticed that this is incorrect for 5GHz, too.
Maybe there are more such bugs.

-- 
Michael


pgpughOY4vs0r.pgp
Description: OpenPGP digital signature

Re: [b43] don't unconditionally fall back to CCK if the rate is 6MB OFDM.

[Michael Büsch]

On Sun, 15 May 2016 07:23:25 -0700
Adrian Chadd  wrote:

> Author: Adrian Chadd 
> Date:   Sun May 15 07:15:54 2016 -0700
> 
> [b43] don't unconditionally fall back to CCK if the rate is 6MB OFDM.
> 
> Check the current PHY operating mode (gmode) to see if we should
> fall back from 6MB OFDM to 11MB CCK.  For 5GHz operation this isn't
> allowed.
> 
> Note, the fallback lookup is only done for RTS rates; normal fallback
> rates are done via mac80211 and aren't affected by this change.
> 
> Signed-off-by: Adrian Chadd 


This makes sense. I guess you tested this on actual hardware?

In the final submission please send this to Kalle Valo and add [PATCH]
to the subject, so tools can pick this up.


> diff --git a/drivers/net/wireless/broadcom/b43/xmit.c
> b/drivers/net/wireless/broadcom/b43/xmit.c
> index f620126..fbf0e92 100644
> --- a/drivers/net/wireless/broadcom/b43/xmit.c
> +++ b/drivers/net/wireless/broadcom/b43/xmit.c
> @@ -205,7 +205,7 @@ static u16 b43_generate_tx_phy_ctl1(struct
> b43_wldev *dev, u8 bitrate)
> return control;
>  }
> 
> -static u8 b43_calc_fallback_rate(u8 bitrate)
> +static u8 b43_calc_fallback_rate(u8 bitrate, int gmode)
>  {
> switch (bitrate) {
> case B43_CCK_RATE_1MB:
> @@ -216,8 +216,16 @@ static u8 b43_calc_fallback_rate(u8 bitrate)
> return B43_CCK_RATE_2MB;
> case B43_CCK_RATE_11MB:
> return B43_CCK_RATE_5MB;
> +
> +   /*
> +* Don't just fallback to CCK; it may be in 5GHz operation
> +* and falling back to CCK won't work out very well.
> +*/
> case B43_OFDM_RATE_6MB:
> -   return B43_CCK_RATE_5MB;
> +   if (gmode)
> +   return B43_CCK_RATE_5MB;
> +   else
> +   return B43_OFDM_RATE_6MB;
> case B43_OFDM_RATE_9MB:
> return B43_OFDM_RATE_6MB;
> case B43_OFDM_RATE_12MB:
> @@ -438,7 +446,7 @@ int b43_generate_txhdr(struct b43_wldev *dev,
> 
> rts_rate = rts_cts_rate ? rts_cts_rate->hw_value :
> B43_CCK_RATE_1MB;
> rts_rate_ofdm = b43_is_ofdm_rate(rts_rate);
> -   rts_rate_fb = b43_calc_fallback_rate(rts_rate);
> +   rts_rate_fb = b43_calc_fallback_rate(rts_rate, phy->gmode);
> rts_rate_fb_ofdm = b43_is_ofdm_rate(rts_rate_fb);
> 
> if (rates[0].flags & IEEE80211_TX_RC_USE_CTS_PROTECT) {


pgpfTfExD_JxO.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


pgpUu_MVFo6Gt.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


pgpVoaONJ_ARw.pgp
Description: OpenPGP digital signature

Re: [PATCH] PCI: Add Broadcom 4331 reset quirk to prevent IRQ storm

[Michael Büsch]

On Wed, 6 Apr 2016 08:31:51 -0500
Bjorn Helgaas  wrote:

> Even for interrupts, it's only a 90% solution because we do still get
> a few interrupts before the quirk runs.  There may not be enough to
> trigger the "IRQ nobody cared" check,

If no driver requested the shared interrupt yet, it should be disabled
in the interrupt controller. So the interrupts do not reach the CPU.
The interrupt storm (on the CPU) starts as soon as some driver
that shares the interrupt with b43 requests and thus enables the
interrupt. And that always happens after the PCI fixup. Thus this is
safe.

-- 
Michael


pgp8pqESvQDtE.pgp
Description: OpenPGP digital signature

Re: [PATCH] PCI: Add Broadcom 4331 reset quirk to prevent IRQ storm

[Michael Büsch]

On Tue, 5 Apr 2016 14:40:15 -0500
Bjorn Helgaas  wrote:

> [+cc Matthew]
> 
> Hi Lukas,
> 
> On Tue, Mar 29, 2016 at 08:20:30PM +0200, Lukas Wunner wrote:
> > Broadcom 4331 wireless cards built into Apple Macs unleash an IRQ storm
> > on boot until they are reset, causing spurious interrupts if the IRQ is
> > shared. Apparently the EFI bootloader enables the device and does not
> > disable it before passing control to the OS. The bootloader contains a
> > driver for the wireless card which allows it to phone home to Cupertino.
> > This is used for Internet Recovery (download and install OS X images)
> > and probably also for Back to My Mac (remote access, RFC 6281) and to
> > discover stolen hardware.
> > 
> > The issue is most pronounced on 2011 and 2012 MacBook Pros where the IRQ
> > is shared with 3 other devices (Light Ridge Thunderbolt controller, SDXC
> > reader, HDA card on discrete GPU). As soon as an interrupt handler is
> > installed for one of these devices, the ensuing storm of spurious IRQs
> > causes the kernel to disable the IRQ and switch to polling. This lasts
> > until the b43 driver loads and resets the device.
> > 
> > Loading the b43 driver first is not always an option, in particular with
> > the Light Ridge Thunderbolt controller: The PCI hotplug IRQ handler gets
> > installed early on because it is built in, unlike b43 which is usually
> > a module.
> > 
> > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=79301
> > Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=895951
> > Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1009819
> > Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1149632
> > Tested-by: Lukas Wunner  [MacBookPro9,1]
> > Signed-off-by: Lukas Wunner 
> > ---
> >  drivers/bcma/bcma_private.h |  2 --
> >  drivers/pci/quirks.c| 27 +++
> >  include/linux/bcma/bcma.h   |  1 +
> >  3 files changed, 28 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/bcma/bcma_private.h b/drivers/bcma/bcma_private.h
> > index eda0909..f642c42 100644
> > --- a/drivers/bcma/bcma_private.h
> > +++ b/drivers/bcma/bcma_private.h
> > @@ -8,8 +8,6 @@
> >  #include 
> >  #include 
> >  
> > -#define BCMA_CORE_SIZE 0x1000
> > -
> >  #define bcma_err(bus, fmt, ...) \
> > pr_err("bus%d: " fmt, (bus)->num, ##__VA_ARGS__)
> >  #define bcma_warn(bus, fmt, ...) \
> > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> > index 8e67802..d4fb5ee 100644
> > --- a/drivers/pci/quirks.c
> > +++ b/drivers/pci/quirks.c
> > @@ -25,6 +25,8 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> > +#include 
> >  #include/* isa_dma_bridge_buggy */
> >  #include "pci.h"
> >  
> > @@ -3282,6 +3284,31 @@ DECLARE_PCI_FIXUP_RESUME_EARLY(PCI_VENDOR_ID_INTEL, 
> > 0x156d,
> >quirk_apple_wait_for_thunderbolt);
> >  #endif
> >  
> > +/*
> > + * Broadcom 4331 wireless cards built into Apple Macs unleash an IRQ storm
> > + * on boot until they are reset, causing spurious interrupts if the IRQ is
> > + * shared. Apparently the EFI bootloader enables the device to phone home
> > + * to Cupertino and does not disable it before passing control to the OS.
> > + */
> > +static void quirk_apple_b43_reset(struct pci_dev *dev)
> > +{
> > +   void __iomem *mmio;
> > +
> > +   if (!dmi_match(DMI_BOARD_VENDOR, "Apple Inc.") || !dev->bus->self ||
> > +   pci_pcie_type(dev->bus->self) != PCI_EXP_TYPE_ROOT_PORT)
> > +   return;
> > +
> > +   mmio = pci_iomap(dev, 0, 0);
> > +   if (!mmio) {
> > +   pr_err("b43 quirk: Cannot iomap device, IRQ storm ahead\n");
> > +   return;
> > +   }
> > +   iowrite32(BCMA_RESET_CTL_RESET,
> > + mmio + (1 * BCMA_CORE_SIZE) + BCMA_RESET_CTL);
> > +   pci_iounmap(dev, mmio);  
> 
> https://bugzilla.kernel.org/show_bug.cgi?id=111781 and
> https://mjg59.dreamwidth.org/11235.html describe a sort of similar
> issue, but with DMA.  An interrupt from the device is probably to
> signal a DMA completion, but these problem reports only mention the
> "IRQ nobody cared" issue; I don't see anything about memory
> corruption.
> 
> If this resets the device, I guess that should prevent future DMA as
> well as future interrupts.  Would pci_reset_function() do the same
> thing in a more generic way?
> 
> I'm a little bit hesitant to put a quirk like this in Linux because
> it's only a 90% solution.  If the only problem is the interrupts, it's
> probably OK since a few extra interrupts doesn't hurt anything.  But
> if there is also DMA that might corrupt something, a 90% solution just
> makes it harder to debug for the remaining 10%.

This was already discussed in this thread.

It is not a 90% solution. It fixes the IRQ issue. It is not supposed to
fix the DMA issue. And it can't. It does mitigate it a tiny bit though.

-- 
Michael


pgpTVbWuOsMJq.pgp
Description: OpenPGP digital signature

Re: [PATCH] PCI: Add Broadcom 4331 reset quirk to prevent IRQ storm

[Michael Büsch]

On Sat, 2 Apr 2016 00:46:46 +0200
Lukas Wunner  wrote:

> Hi Chris,
> 
> On Fri, Apr 01, 2016 at 12:13:46AM +0100, Chris Bainbridge wrote:
> > On Tue, Mar 29, 2016 at 07:41:30PM +0200, Lukas Wunner wrote:  
> > > Broadcom 4331 wireless cards built into Apple Macs unleash an IRQ storm
> > > on boot until they are reset, causing spurious interrupts if the IRQ is
> > > shared. Apparently the EFI bootloader enables the device and does not
> > > disable it before passing control to the OS. The bootloader contains a
> > > driver for the wireless card which allows it to phone home to Cupertino.
> > > This is used for Internet Recovery (download and install OS X images)
> > > and probably also for Back to My Mac (remote access, RFC 6281) and to
> > > discover stolen hardware.
> > > 
> > > The issue is most pronounced on 2011 and 2012 MacBook Pros where the IRQ
> > > is shared with 3 other devices (Light Ridge Thunderbolt controller, SDXC
> > > reader, HDA card on discrete GPU). As soon as an interrupt handler is
> > > installed for one of these devices, the ensuing storm of spurious IRQs
> > > causes the kernel to disable the IRQ and switch to polling. This lasts
> > > until the b43 driver loads and resets the device.
> > > 
> > > Loading the b43 driver first is not always an option, in particular with
> > > the Light Ridge Thunderbolt controller: The PCI hotplug IRQ handler gets
> > > installed early on because it is built in, unlike b43 which is usually
> > > a module.
> > > 
> > > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=79301
> > > Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=895951
> > > Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1009819
> > > Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1149632  
> > 
> > Should also fix https://bugzilla.kernel.org/show_bug.cgi?id=111781 ?
> > Given that this is a serious bug that can corrupt filesystems it would
> > be good to see the fix in stable too.  
> 
> I cannot reproduce this particular issue on my MBP9,1 even though it
> is architecturally very similar to your MBP10,2. I tested it with
> "iommu=force intel_iommu=on", blacklisted b43 and stressed the machine
> a bit with kernel compiles. No issues.

I think you will have to stress the wireless, not the kernel.
Enable iommu and let the wireless receive packets that go through the
hw filters. If you have a calm network and nobody sends data to you,
the card won't write anything to DMA. (This all depends on how the
firmware configured the filters).

-- 
Michael


pgpVgi2n7bP1d.pgp
Description: OpenPGP digital signature

Re: [PATCH] PCI: Add Broadcom 4331 reset quirk to prevent IRQ storm

[Michael Büsch]

On Fri, 1 Apr 2016 00:13:46 +0100
Chris Bainbridge  wrote:

> Should also fix https://bugzilla.kernel.org/show_bug.cgi?id=111781 ?
> Given that this is a serious bug that can corrupt filesystems it would
> be good to see the fix in stable too.

Wow, this is horrible.

This patch should help a bit. But it's probably hard to workaround that
DMA issue completely. There still is a chance of corruption, as soon as
the kernel allocates and uses some memory that the DMA controller still
uses from boot-up before the fixup is applied.

-- 
Michael


pgpCmaAMSroAK.pgp
Description: OpenPGP digital signature

Re: [PATCH] PCI: Add Broadcom 4331 reset quirk to prevent IRQ storm

[Michael Büsch]

On Tue, 29 Mar 2016 19:41:30 +0200
Lukas Wunner  wrote:

> +static void quirk_apple_b43_reset(struct pci_dev *dev)
> +{
> + void __iomem *mmio;
> +
> + if (!dmi_match(DMI_BOARD_VENDOR, "Apple Inc.") || !dev->bus->self ||
> + pci_pcie_type(dev->bus->self) != PCI_EXP_TYPE_ROOT_PORT)
> + return;
> +
> + mmio = pci_iomap(dev, 0, 0);
> + if (!mmio) {
> + pr_err("b43 quirk: Cannot iomap device, IRQ storm ahead\n");
> + return;
> + }
> + iowrite32(BCMA_RESET_CTL_RESET,
> +   mmio + (1 * BCMA_CORE_SIZE) + BCMA_RESET_CTL);
> + pci_iounmap(dev, mmio);
> +}
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_BROADCOM, 0x4331, 
> quirk_apple_b43_reset);

Looks good, as long as the wireless core is always guaranteed to be
the second core on that PCI device.

-- 
Michael


pgpycS4__v7D9.pgp
Description: OpenPGP digital signature

Re: [v2, resend] b43: Fix memory leaks in b43_bus_dev_ssb_init and b43_bus_dev_bcma_init

[Michael Büsch]

On Fri, 11 Mar 2016 21:52:30 +0530
Sudip Mukherjee  wrote:

> On Sat, Jan 16, 2016 at 09:08:10PM +0800, Jia-Ju Bai wrote:
> > The memory allocated by kzalloc in b43_bus_dev_ssb_init and
> > b43_bus_dev_bcma_init is not freed.
> > This patch fixes the bug by adding kfree in b43_ssb_remove,
> > b43_bcma_remove and error handling code of b43_bcma_probe.
> > 
> > Thanks Michael for his suggestion.
> > 
> > Signed-off-by: Jia-Ju Bai   
> 
> The patch did not apply cleanly. I had to edit the patch to point to
> drivers/net/wireless/broadcom/b43/main.c
> 
> For CONFIG_B43_SSB part-
> Tested-by: Sudip Mukherjee 


Ok thanks a lot.
We possibly don't get an bcma tester anytime soon. As this is quite an
important fix, we should probably apply it.

Sudip, can you please update the patch so it applies cleanly on the
current tree? You can add my acked-by if you want.

Kalle, can you please apply it afterwards?

-- 
Michael


pgpOWD77V0ixA.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


pgpJnxedgnW8s.pgp
Description: OpenPGP digital signature

Re: [PATCH 1/2] b43: Remove unnecessary synchronize_irq() before free_irq()

[Michael Büsch]

On Mon,  8 Feb 2016 21:41:12 +0100
Lars-Peter Clausen  wrote:

> Calling synchronize_irq() right before free_irq() is quite useless. On one
> hand the IRQ can easily fire again before free_irq() is entered,

Well, that depends on whether the interrupt is shared and whether we
disabled the interrupt mask inside of the device (which we did).

> on the
> other hand free_irq() itself calls synchronize_irq() internally (in a race
> condition free way), before any state associated with the IRQ is freed.

Ok, fair enough.

-- 
Michael


pgpK8kRWHD8VE.pgp
Description: OpenPGP digital signature

Re: (bug report) b43: impossible conditions in debugfs

[Michael Büsch]

On Thu, 26 Nov 2015 14:59:24 +0300
Dan Carpenter  wrote:

> Hello Michael Buesch,
> 
> The patch 6bbc321a96d4: "b43: Add debugfs files for random SHM
> access" from Jun 19, 2008, leads to the following static checker
> warning:
> 
>   drivers/net/wireless/broadcom/b43/debugfs.c:217 shm32write__write_file()
>   warn: impossible condition '(mask > 4294967295) => (0-u32max > u32max)'
> 
> drivers/net/wireless/broadcom/b43/debugfs.c
>198  static int shm32write__write_file(struct b43_wldev *dev,
>199const char *buf, size_t count)
>200  {
>201  unsigned int routing, addr, mask, set;
>202  u32 val;
>203  int res;
>204  
>205  res = sscanf(buf, "0x%X 0x%X 0x%X 0x%X",
>206   , , , );
>207  if (res != 4)
>208  return -EINVAL;
>209  if (routing > B43_MAX_SHM_ROUTING)
>210  return -EADDRNOTAVAIL;
>211  if (addr > B43_MAX_SHM_ADDR)
>212  return -EADDRNOTAVAIL;
>213  if (routing == B43_SHM_SHARED) {
>214  if ((addr % 2) != 0)
>215  return -EADDRNOTAVAIL;
>216  }
>217  if ((mask > 0x) || (set > 0x))
> 
> Both of these conditions are impossible.
> 
>218  return -E2BIG;
>219  
>220  if (mask == 0)
>221  val = 0;
>222  else
>223  val = b43_shm_read32(dev, routing, addr);
>224  val &= mask;
>225  val |= set;
>226  b43_shm_write32(dev, routing, addr, val);
>227  
>228  return 0;
>229  }
> 
> See also:
> drivers/net/wireless/broadcom/b43/debugfs.c:346 mmio32write__write_file() 
> warn: impossible condition '(mask > 4294967295) => (0-u32max > u32max)'
> drivers/net/wireless/broadcom/b43/debugfs.c:346 mmio32write__write_file() 
> warn: impossible condition '(set > 4294967295) => (0-u32max > u32max)'



Sure. These are intentional.
The compiler will optimize this out.

-- 
Michael


pgpVxbGfqgyhy.pgp
Description: OpenPGP digital signature

Re: (bug report) b43: precendence error

[Michael Büsch]

On Thu, 26 Nov 2015 14:58:43 +0300
Dan Carpenter  wrote:

> [ All the old wireless Smatch warnings are showing up as new ones
>   because of the path reshuffle in linux-next.  I'm going through and
>   reporting the extra suspicious ones.  -dan ]
> 
> Hello Rafał Miłecki,
> 
> The patch 6f98e62a9f1b: "b43: update cordic code to match current
> specs" from Jan 25, 2010, leads to the following static checker
> warning:
> 
>   drivers/net/wireless/broadcom/b43/phy_lp.c:1803 lpphy_start_tx_tone()
>   warn: mask and shift to zero
> 
> drivers/net/wireless/broadcom/b43/phy_lp.c
>   1800  for (i = 0; i < samples; i++) {
>   1801  sample = b43_cordic(angle);
>   1802  angle += rotation;
>   1803  buf[i] = CORDIC_CONVERT((sample.i * max) & 0xFF) << 8;
>   1804  buf[i] |= CORDIC_CONVERT((sample.q * max) & 0xFF);
> 
> Maybe the intention was:
> 
>   buf[i] = (CORDIC_CONVERT(sample.i * max) & 0xFF) << 8;
>   buf[i] |= CORDIC_CONVERT((sample.q * max) & 0xFF;
> 
>   1805  }
>   1806  


This looks like a bug indeed.
Rafał, do you have hw to check this?

buf[i] = (CORDIC_CONVERT(sample.i * max) & 0xFF) << 8;
buf[i] |= CORDIC_CONVERT(sample.q * max) & 0xFF;

-- 
Michael


pgpF77HLoans5.pgp
Description: OpenPGP digital signature

Re: (bug report) b43: impossible conditions in debugfs

[Michael Büsch]

On Thu, 26 Nov 2015 16:00:34 +0300
Dan Carpenter <dan.carpen...@oracle.com> wrote:

> On Thu, Nov 26, 2015 at 01:32:41PM +0100, Michael Büsch wrote:
> > > See also:
> > > drivers/net/wireless/broadcom/b43/debugfs.c:346 mmio32write__write_file() 
> > > warn: impossible condition '(mask > 4294967295) => (0-u32max > u32max)'
> > > drivers/net/wireless/broadcom/b43/debugfs.c:346 mmio32write__write_file() 
> > > warn: impossible condition '(set > 4294967295) => (0-u32max > u32max)'  
> > 
> > 
> > 
> > Sure. These are intentional.
> > The compiler will optimize this out.  
> 
> Hm...  We try to ignore when people do intentional comparisons with zero
> like this:
> 
>   if (unsigned_var < 0 || unsigned_var >= 10)
>   return -EINVAL;
> 
> Because they are obviously harmless and they don't hurt readability.
> Also Linus doesn't like removing these.


It just checks whether the value will fit into a 32 bit unsigned int
variable. It doesn't make assumptions on what sizeof(unsigned int) is,
although it would be safe to assume 4 here and omit the check.

-- 
Michael


pgpoqLXE7TGyO.pgp
Description: OpenPGP digital signature

Re: [PATCH 1/2] ssb: move functions specific to SoC hosted bus to separated file

[Michael Büsch]

On Sun, 25 Oct 2015 19:32:42 +0100
Rafał Miłecki  wrote:

> This cleans main.c a bit and will allow us to compile SoC related code
> conditionally in the future.

Looks good.

Acked-by: Michael Buesch 

> +#ifdef CONFIG_SSB_BLOCKIO

I wonder whether we should remove that option entirely. It does not save
that much space and most people will probably have it enabled anyway.
It's required for non-DMA PIO transfers.

So if you want, you could remove that in a separate patch.

-- 
Michael


pgpRzETD1dRmg.pgp
Description: OpenPGP digital signature

Re: [PATCH 2/2] ssb: add Kconfig entry for compiling SoC related code

[Michael Büsch]

On Sun, 25 Oct 2015 19:32:43 +0100
Rafał Miłecki  wrote:

> This allows saving a little of space when not using ssb on Broadcom SoC.

I'm not sure whether it's worth it.
But I don't mind having this anyway.

Acked-by: Michael Buesch 

-- 
Michael


pgpEvtCPoQGTU.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


pgpIFA3qjrco0.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


pgpqZl2TBbIhx.pgp
Description: OpenPGP digital signature

Re: [PATCH][RFC][RFT] ssb: pick PCMCIA host code support from b43 driver

[Michael Büsch]

On Wed, 14 Oct 2015 13:17:16 +0200
Rafał Miłecki  wrote:

> If all PCI-bus-host-specific code is always the same,

And that's the point. It's always the same _except_ for the device IDs.
Which obviously are PCI-device specific.

-- 
Michael


pgpOqekO9MF4R.pgp
Description: OpenPGP digital signature

Re: [PATCH][RFC][RFT] ssb: pick PCMCIA host code support from b43 driver

[Michael Büsch]

On Wed, 23 Sep 2015 12:02:48 +0200
Rafał Miłecki <zaj...@gmail.com> wrote:

> On 21 September 2015 at 18:20, Michael Büsch <m...@bues.ch> wrote:
> > On Mon, 21 Sep 2015 11:04:19 +0200
> > Rafał Miłecki <zaj...@gmail.com> wrote:
> >> @@ -1464,6 +1463,12 @@ static int __init ssb_modinit(void)
> >>   /* don't fail SSB init because of this */
> >>   err = 0;
> >>   }
> >> + err = ssb_host_pcmcia_init();
> >> + if (err) {
> >> + ssb_err("PCMCIA host initialization failed\n");
> >> + /* don't fail SSB init because of this */
> >
> > Why not? What's the point of not failing here?
> 
> I just copied the logic from few lines above where we handle PCI init.
> I guess the point was to support other host devices even is PCI host
> registration fails.


Ah I misread it. This is at modinit time. That might make sense then.


> >> +static const struct pcmcia_device_id ssb_host_pcmcia_tbl[] = {
> >> + PCMCIA_DEVICE_MANF_CARD(0x2D0, 0x448),
> >> + PCMCIA_DEVICE_MANF_CARD(0x2D0, 0x476),
> >> + PCMCIA_DEVICE_NULL,
> >> +};
> >
> > This doesn't belong into ssb'c pcmcia.c, IMO.
> > It should be in a new file called b43_pcmcia_bridge.c, just like we have
> > b43_pci_bridge.c.
> > The bridge code technically (also for pci) doesn't belong into ssb. But
> > it makes kconfig simpler.
> 
> This is something I don't understand. This PCI bridge was also always
> confusing me.
> Why do we want a separated file for that? What's wrong with having 1
> file for host (PCI/PCMCIA) driver (probe and remove functions) *and*
> ssb initialization?


Because that's not ssb code. These are device IDs for b43 devices.
We just keep it in ssb to make module handling easier.
Ssb also runs non-b43 devices.
Think of it like PCI IDs that belong into the driver and not the PCI
subsystem.

-- 
Michael


pgpPBi6G9abee.pgp
Description: OpenPGP digital signature

Re: [PATCH][RFC][RFT] ssb: pick PCMCIA host code support from b43 driver

[Michael Büsch]

On Mon, 21 Sep 2015 11:04:19 +0200
Rafał Miłecki  wrote:

> ssb bus can be found on various "host" devices like PCI/PCMCIA/SDIO.
> Every ssb bus contains cores AKA devices.
> The main idea is to have ssb driver scan/initialize bus and register
> ready-to-use cores. This way ssb drivers can operate on a single core
> mostly ignoring underlaying details.
> 
> For some reason PCMCIA support was split between ssb and b43. We got
> PCMCIA host device probing in b43, then bus scanning in ssb and then
> wireless core probing back in b43. The truth is it's very unlikely we
> will ever see PCMCIA ssb device with no 802.11 core but I still don't
> see any advantage of the current architecture.

The idea basically was that b43 is the only user of that code. So the
code was put there.

> With proposed change we get the same functionality with a simpler
> architecture, less Kconfig symbols, one killed EXPORT and hopefully
> cleaner b43. Since b43 supports both: ssb & bcma I prefer to keep ssb
> specific code in ssb driver.

I agree that this makes the architecture a bit cleaner. So this
basically looks good. I currently can't test it, because I don't have
that device here right now. In two weeks or so I'll probably be able to
test it, though.


> @@ -1464,6 +1463,12 @@ static int __init ssb_modinit(void)
>   /* don't fail SSB init because of this */
>   err = 0;
>   }
> + err = ssb_host_pcmcia_init();
> + if (err) {
> + ssb_err("PCMCIA host initialization failed\n");
> + /* don't fail SSB init because of this */

Why not? What's the point of not failing here?

> + err = 0;
> + }


> +static const struct pcmcia_device_id ssb_host_pcmcia_tbl[] = {
> + PCMCIA_DEVICE_MANF_CARD(0x2D0, 0x448),
> + PCMCIA_DEVICE_MANF_CARD(0x2D0, 0x476),
> + PCMCIA_DEVICE_NULL,
> +};

This doesn't belong into ssb'c pcmcia.c, IMO.
It should be in a new file called b43_pcmcia_bridge.c, just like we have
b43_pci_bridge.c.
The bridge code technically (also for pci) doesn't belong into ssb. But
it makes kconfig simpler.

-- 
Michael


pgpUissozXCJo.pgp
Description: OpenPGP digital signature

Re: [PATCH][RFC][RFT] ssb: pick PCMCIA host code support from b43 driver

[Michael Büsch]

On Mon, 21 Sep 2015 11:14:32 -0500
Larry Finger  wrote:

> This patch has been tested on PPC architecture with Linksys WPC54G PCMCIA 
> cards.

Are you sure that this really is a 16 bit PCMCIA card and not a PC-Card?
If it shows up in lspci, it's not a PCMCIA card.

> It probably does not matter here, but I prefer that hexadecimal constants in 
> device tables contain only the lower-case versions of a-f. That makes 
> searching 
> for such constants with grep a lot easier.

I prefer coffee over tea. That doesn't make coffee any better, though.

Is it really so that the rest of the kernel only uses lower case here?
Grep also supports case insensitive regexes, if done correctly. And if
not everybody uses lower case here, you'll have to do that anyway.

-- 
Michael


pgpW8NyqHbRv2.pgp
Description: OpenPGP digital signature

Re: [PATCH 1/3] ssb: unexport ssb_bus_pcibus_register

[Michael Büsch]

On Mon, 21 Sep 2015 09:47:18 +0200
Rafał Miłecki  wrote:

> It isn't used anywhere out of ssb code and we don't (plan to) build
> pcihost_wrapper.c as a separated module.
> 
> Signed-off-by: Rafał Miłecki 
> ---
>  drivers/ssb/main.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/ssb/main.c b/drivers/ssb/main.c
> index a48a743..8cf23a2 100644
> --- a/drivers/ssb/main.c
> +++ b/drivers/ssb/main.c
> @@ -876,7 +876,6 @@ int ssb_bus_pcibus_register(struct ssb_bus *bus, struct 
> pci_dev *host_pci)
>  
>   return err;
>  }
> -EXPORT_SYMBOL(ssb_bus_pcibus_register);
>  #endif /* CONFIG_SSB_PCIHOST */
>  
>  #ifdef CONFIG_SSB_PCMCIAHOST

Acked-by: Michael Buesch 


-- 
Michael


pgp0Wq7Tjy5pG.pgp
Description: OpenPGP digital signature

Re: [PATCH] ssb: make ssb_pcmcia_switch_core static

[Michael Büsch]

On Mon, 21 Sep 2015 09:55:26 +0200
Rafał Miłecki  wrote:

> Signed-off-by: Rafał Miłecki 
> ---
>  drivers/ssb/pcmcia.c  | 3 +--
>  drivers/ssb/ssb_private.h | 7 ---
>  2 files changed, 1 insertion(+), 9 deletions(-)
> 
> diff --git a/drivers/ssb/pcmcia.c b/drivers/ssb/pcmcia.c
> index b413e01..f03422b 100644
> --- a/drivers/ssb/pcmcia.c
> +++ b/drivers/ssb/pcmcia.c
> @@ -147,8 +147,7 @@ error:
>   return err;
>  }
>  
> -int ssb_pcmcia_switch_core(struct ssb_bus *bus,
> -struct ssb_device *dev)
> +static int ssb_pcmcia_switch_core(struct ssb_bus *bus, struct ssb_device 
> *dev)
>  {
>   int err;
>  
> diff --git a/drivers/ssb/ssb_private.h b/drivers/ssb/ssb_private.h
> index 426c805..8a2ebc8 100644
> --- a/drivers/ssb/ssb_private.h
> +++ b/drivers/ssb/ssb_private.h
> @@ -85,8 +85,6 @@ static inline int ssb_pci_init(struct ssb_bus *bus)
>  
>  /* pcmcia.c */
>  #ifdef CONFIG_SSB_PCMCIAHOST
> -extern int ssb_pcmcia_switch_core(struct ssb_bus *bus,
> -   struct ssb_device *dev);
>  extern int ssb_pcmcia_switch_coreidx(struct ssb_bus *bus,
>u8 coreidx);
>  extern int ssb_pcmcia_switch_segment(struct ssb_bus *bus,
> @@ -98,11 +96,6 @@ extern void ssb_pcmcia_exit(struct ssb_bus *bus);
>  extern int ssb_pcmcia_init(struct ssb_bus *bus);
>  extern const struct ssb_bus_ops ssb_pcmcia_ops;
>  #else /* CONFIG_SSB_PCMCIAHOST */
> -static inline int ssb_pcmcia_switch_core(struct ssb_bus *bus,
> -  struct ssb_device *dev)
> -{
> - return 0;
> -}
>  static inline int ssb_pcmcia_switch_coreidx(struct ssb_bus *bus,
>   u8 coreidx)
>  {


Acked-by: Michael Buesch 


-- 
Michael


pgpb67XYfAMOr.pgp
Description: OpenPGP digital signature

Re: [PATCH 2/3] ssb: make ssb_sdio_switch_core static

[Michael Büsch]

On Mon, 21 Sep 2015 09:47:19 +0200
Rafał Miłecki  wrote:

> It's used locally only.
> 
> Signed-off-by: Rafał Miłecki 
> ---
>  drivers/ssb/sdio.c| 2 +-
>  drivers/ssb/ssb_private.h | 6 --
>  2 files changed, 1 insertion(+), 7 deletions(-)
> 
> diff --git a/drivers/ssb/sdio.c b/drivers/ssb/sdio.c
> index b2d36f7..2278e43 100644
> --- a/drivers/ssb/sdio.c
> +++ b/drivers/ssb/sdio.c
> @@ -200,7 +200,7 @@ out:
>  }
>  
>  /* host must be already claimed */
> -int ssb_sdio_switch_core(struct ssb_bus *bus, struct ssb_device *dev)
> +static int ssb_sdio_switch_core(struct ssb_bus *bus, struct ssb_device *dev)
>  {
>   u8 coreidx = dev->core_index;
>   u32 sbaddr;
> diff --git a/drivers/ssb/ssb_private.h b/drivers/ssb/ssb_private.h
> index eb507a5..1d2256e 100644
> --- a/drivers/ssb/ssb_private.h
> +++ b/drivers/ssb/ssb_private.h
> @@ -132,7 +132,6 @@ extern int ssb_sdio_get_invariants(struct ssb_bus *bus,
>struct ssb_init_invariants *iv);
>  
>  extern u32 ssb_sdio_scan_read32(struct ssb_bus *bus, u16 offset);
> -extern int ssb_sdio_switch_core(struct ssb_bus *bus, struct ssb_device *dev);
>  extern int ssb_sdio_scan_switch_coreidx(struct ssb_bus *bus, u8 coreidx);
>  extern int ssb_sdio_hardware_setup(struct ssb_bus *bus);
>  extern void ssb_sdio_exit(struct ssb_bus *bus);
> @@ -144,11 +143,6 @@ static inline u32 ssb_sdio_scan_read32(struct ssb_bus 
> *bus, u16 offset)
>  {
>   return 0;
>  }
> -static inline int ssb_sdio_switch_core(struct ssb_bus *bus,
> -  struct ssb_device *dev)
> -{
> - return 0;
> -}
>  static inline int ssb_sdio_scan_switch_coreidx(struct ssb_bus *bus, u8 
> coreidx)
>  {
>   return 0;


Acked-by: Michael Buesch 


-- 
Michael


pgprwKYlFUVtn.pgp
Description: OpenPGP digital signature

Re: [PATCH 3/3] ssb: drop declaration of non existing ssb_sdio_hardware_setup

[Michael Büsch]

On Mon, 21 Sep 2015 09:47:20 +0200
Rafał Miłecki  wrote:

> Signed-off-by: Rafał Miłecki 
> ---
>  drivers/ssb/ssb_private.h | 5 -
>  1 file changed, 5 deletions(-)
> 
> diff --git a/drivers/ssb/ssb_private.h b/drivers/ssb/ssb_private.h
> index 1d2256e..426c805 100644
> --- a/drivers/ssb/ssb_private.h
> +++ b/drivers/ssb/ssb_private.h
> @@ -133,7 +133,6 @@ extern int ssb_sdio_get_invariants(struct ssb_bus *bus,
>  
>  extern u32 ssb_sdio_scan_read32(struct ssb_bus *bus, u16 offset);
>  extern int ssb_sdio_scan_switch_coreidx(struct ssb_bus *bus, u8 coreidx);
> -extern int ssb_sdio_hardware_setup(struct ssb_bus *bus);
>  extern void ssb_sdio_exit(struct ssb_bus *bus);
>  extern int ssb_sdio_init(struct ssb_bus *bus);
>  
> @@ -147,10 +146,6 @@ static inline int ssb_sdio_scan_switch_coreidx(struct 
> ssb_bus *bus, u8 coreidx)
>  {
>   return 0;
>  }
> -static inline int ssb_sdio_hardware_setup(struct ssb_bus *bus)
> -{
> - return 0;
> -}
>  static inline void ssb_sdio_exit(struct ssb_bus *bus)
>  {
>  }


Acked-by: Michael Buesch 


-- 
Michael


pgpDhULM0rHFq.pgp
Description: OpenPGP digital signature

Re: [PATCH] b43: Fix typo in function name

[Michael Büsch]

On Mon, 29 Jun 2015 20:29:24 -0500
Larry Finger larry.fin...@lwfinger.net wrote:

 On 06/29/2015 07:45 PM, Nik Nyby wrote:
  This fixes a typo in the b43_lo_g_maintenance_work function
  name.
 
  Signed-off-by: Nik Nyby niko...@gnu.org
  ---
drivers/net/wireless/b43/lo.c| 4 ++--
drivers/net/wireless/b43/lo.h| 2 +-
drivers/net/wireless/b43/phy_g.c | 2 +-
3 files changed, 4 insertions(+), 4 deletions(-)
 
 This patch is OK; however, can there really be a typo in a function name? How 
 do 
 you ever know that the author did not mean to use a name that looks like a 
 typo?
 
 Please be careful with these.
 
 Acked-by: Larry Finger larry.fin...@lwfinger.net


Nice finding.
Me as the original author of that mess gets a brown paper bag and acks
this.

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


-- 
Michael


pgpi64vcvAW3z.pgp
Description: OpenPGP digital signature

Re: [PATCH] rtlwifi: Fix EFUSE_ANA8M map value

[Michael Büsch]

On Sat, 23 May 2015 00:15:23 -0500
Larry Finger larry.fin...@lwfinger.net wrote:

 No, it is able to scan and connect with the vanilla kernel, but it is not 
 terribly reliable with about 25% ping loss. I just got a report of a 
 regression. 
 Kernel 4.0 fails whereas 3.19 works. I have been seeing a lot of patches from 
 various sources. I thought they were OK, but I will need to bisect.

I tried with 3.16 and 4.0 based Debian kernels. Neither worked.
I use the firmware provided by Debian's firmware-realtek package.
I have two of these devices, so I think the hardware should be ok, unless they 
are broken both.

When I first tried to use it, I forgot to install firmware. It tried to load 
it, then some 'alternative firmware' and then froze the system partially. I 
guess there's some unreleased lock or such in an error path somewhere. I didn't 
check this, because it didn't seem too terribly important for now.

I will also try your driver from your github repo soon and see if that makes it 
work.

 Some days I am sorry that I ever heard of Linux. :)

Retired, but still out of time. :)

-- 
Michael


pgpevipzoSj74.pgp
Description: OpenPGP digital signature

Re: [PATCH] rtlwifi: Fix EFUSE_ANA8M map value

[Michael Büsch]

On Fri, 22 May 2015 15:21:48 -0500
Larry Finger larry.fin...@lwfinger.net wrote:
 What does lsusb say about your device?
 7392:7811, which is an Edimax EW-7811Un

It is exactly this device.
Does yours also fail with vanilla kernel?

-- 
Michael


pgpOeLg89H8Ts.pgp
Description: OpenPGP digital signature

[PATCH] b43: Fix locking FIXME in beacon update top half

[Michael Büsch]

b43 has a FIXME about locking in the mac80211 set-beacon-int callback for a 
long time.
As it turns out there actually is a tiny race window that could result in
a use-after-free bug of the 'current_beacon' memory.
Nobody ever reported this, so it probably never happened.

Fix this by adding a spin lock that protects the current_beacon access.
We must not be in atomic context while accessing hardware (due to SDIO),
so the beacon update bottom half has to clone the skb and release the lock
before writing it to hardware.

Let's all hope that this stops the troll who is trying to submit incorrect
fixes for this issue repeatedly.
And let's hope that I'm not a troll, too, who just hides even more evil code
in an even more complex attempt to fix the issue.

Signed-off-by: Michael Buesch m...@bues.ch
Tested-by: Larry Finger larry.fin...@lwfinger.net

---

Index: linux/drivers/net/wireless/b43/b43.h
===
--- linux.orig/drivers/net/wireless/b43/b43.h
+++ linux/drivers/net/wireless/b43/b43.h
@@ -941,6 +941,7 @@ struct b43_wl {
bool beacon1_uploaded;
bool beacon_templates_virgin; /* Never wrote the templates? */
struct work_struct beacon_update_trigger;
+   spinlock_t beacon_lock;
 
/* The current QOS parameters for the 4 queues. */
struct b43_qos_params qos_params[B43_QOS_QUEUE_NUM];
Index: linux/drivers/net/wireless/b43/main.c
===
--- linux.orig/drivers/net/wireless/b43/main.c
+++ linux/drivers/net/wireless/b43/main.c
@@ -1601,12 +1601,26 @@ static void b43_write_beacon_template(st
unsigned int rate;
u16 ctl;
int antenna;
-   struct ieee80211_tx_info *info = 
IEEE80211_SKB_CB(dev-wl-current_beacon);
+   struct ieee80211_tx_info *info;
+   unsigned long flags;
+   struct sk_buff *beacon_skb;
 
-   bcn = (const struct ieee80211_mgmt *)(dev-wl-current_beacon-data);
-   len = min_t(size_t, dev-wl-current_beacon-len,
- 0x200 - sizeof(struct b43_plcp_hdr6));
+   spin_lock_irqsave(dev-wl-beacon_lock, flags);
+   info = IEEE80211_SKB_CB(dev-wl-current_beacon);
rate = ieee80211_get_tx_rate(dev-wl-hw, info)-hw_value;
+   /* Clone the beacon, so it cannot go away, while we write it to hw. */
+   beacon_skb = skb_clone(dev-wl-current_beacon, GFP_ATOMIC);
+   spin_unlock_irqrestore(dev-wl-beacon_lock, flags);
+
+   if (!beacon_skb) {
+   b43dbg(dev-wl, Could not upload beacon. 
+  Failed to clone beacon skb.);
+   return;
+   }
+
+   bcn = (const struct ieee80211_mgmt *)(beacon_skb-data);
+   len = min_t(size_t, beacon_skb-len,
+   0x200 - sizeof(struct b43_plcp_hdr6));
 
b43_write_template_common(dev, (const u8 *)bcn,
  len, ram_offset, shm_size_offset, rate);
@@ -1674,6 +1688,8 @@ static void b43_write_beacon_template(st
B43_SHM_SH_DTIMPER, 0);
}
b43dbg(dev-wl, Updated beacon template at 0x%x\n, ram_offset);
+
+   dev_kfree_skb_any(beacon_skb);
 }
 
 static void b43_upload_beacon0(struct b43_wldev *dev)
@@ -1790,13 +1806,13 @@ static void b43_beacon_update_trigger_wo
mutex_unlock(wl-mutex);
 }
 
-/* Asynchronously update the packet templates in template RAM.
- * Locking: Requires wl-mutex to be locked. */
+/* Asynchronously update the packet templates in template RAM. */
 static void b43_update_templates(struct b43_wl *wl)
 {
-   struct sk_buff *beacon;
+   struct sk_buff *beacon, *old_beacon;
+   unsigned long flags;
 
-   /* This is the top half of the ansynchronous beacon update.
+   /* This is the top half of the asynchronous beacon update.
 * The bottom half is the beacon IRQ.
 * Beacon update must be asynchronous to avoid sending an
 * invalid beacon. This can happen for example, if the firmware
@@ -1810,12 +1826,17 @@ static void b43_update_templates(struct
if (unlikely(!beacon))
return;
 
-   if (wl-current_beacon)
-   dev_kfree_skb_any(wl-current_beacon);
+   spin_lock_irqsave(wl-beacon_lock, flags);
+   old_beacon = wl-current_beacon;
wl-current_beacon = beacon;
wl-beacon0_uploaded = false;
wl-beacon1_uploaded = false;
+   spin_unlock_irqrestore(wl-beacon_lock, flags);
+
ieee80211_queue_work(wl-hw, wl-beacon_update_trigger);
+
+   if (old_beacon)
+   dev_kfree_skb_any(old_beacon);
 }
 
 static void b43_set_beacon_int(struct b43_wldev *dev, u16 beacon_int)
@@ -5094,7 +5115,6 @@ static int b43_op_beacon_set_tim(struct
 {
struct b43_wl *wl = hw_to_b43_wl(hw);
 
-   /* FIXME: add locking */
b43_update_templates(wl);
 
return 0;
@@ -5584,6 +5604,7 @@ static struct b43_wl *b43_wireless_init(
wl-hw = hw;

Re: [PATCH] drivers:net:wireless: Add mutex locking for function, b43_op_beacon_set_time in main.c

[Michael Büsch]

On Fri, 28 Nov 2014 22:32:30 -0500
nick xerofo...@gmail.com wrote:

 I don't have hardware for this driver on me, so I didn't test it. However 
 this seems to 
 be correct from my reading of the code around this function and other locking 
 related
 to this driver.

From the current docs:

  * @set_tim: Set TIM bit. mac80211 calls this function when a TIM bit
  *  must be set or cleared for a given STA. Must be atomic.

So it is not allowed to lock a mutex here.

-- 
Michael


pgpa3UGjB71JW.pgp
Description: OpenPGP digital signature

Re: [PATCH] drivers:net:wireless: Add mutex locking for function, b43_op_beacon_set_time in main.c

[Michael Büsch]

On Fri, 28 Nov 2014 23:40:46 +0100
Rafał Miłecki zaj...@gmail.com wrote:

  @@ -5094,8 +5094,9 @@ static int b43_op_beacon_set_tim(struct ieee80211_hw 
  *hw,
   {
  struct b43_wl *wl = hw_to_b43_wl(hw);
 
  -   /* FIXME: add locking */
  +   mutex_lock(wl-mutex);
  b43_update_templates(wl);
  +   mutex_unlock(wl-mutex);
 
  return 0;
   }
 
 Does anyone remember why this simple solution wasn't implemented
 earlier? Michael?

I think the callback used to be (is?) in atomic context.

 Nicholas: did you test it anyhow?

-- 
Michael


pgpwiY9PcR90H.pgp
Description: OpenPGP digital signature