BCM4331 reset leads to wl lockup

BCM4331 reset leads to wl lockup

[Lukas Wunner]

Dear Broadcom support,

on Macs equipped with a BCM4331, a reset of the wireless core is needed
early in the boot process to prevent spurious IRQs and memory corruption.
This is achieved by the below patch.

Unfortunately the patch seems to cause a lockup with wl depending on the
amount of traffic transmitted: A user has reported that when sending only
pings, everything works fine. However a larger amount of traffic such as
opening a website in a browser causes the system to lock up.

The issue only occurs with wl, not the open source b43 driver. All the
patch does is set the reset bit ((1 * BCMA_CORE_SIZE) + BCMA_RESET_CTL)
in the wireless core's mmio space.

Please advise how the patch should be amended to avoid the lockups.

Thanks,

Lukas

-- >8 --

BCM4331 reset leads to wl lockup

[Michael Büsch]

On Thu, 26 May 2016 14:12:10 +0200
Lukas Wunner <lukas@wunner.de> 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/b43-dev/attachments/20160526/cabe4088/attachment.sig>

BCM4331 reset leads to wl lockup

[Lukas Wunner]

On Thu, May 26, 2016 at 02:42:46PM +0200, Michael B?sch wrote:
> On Thu, 26 May 2016 14:12:10 +0200 Lukas Wunner <lukas@wunner.de> 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()

It turned out that the lockups are triggered by bec3cfdca36b
("net: skb_segment() provides list head and tail") in Linux 3.18
and that Eric Duzamet has kindly provided a fix for broadcom-sta:
https://bugs.gentoo.org/show_bug.cgi?id=523326#c24
https://523326.bugs.gentoo.org/attachment.cgi?id=393374

@Broadcom: Please consider releasing a new driver version which
incorporates that patch. The latest version 6.30.223.271 of your
driver is still missing it even though the issue has existed for
almost 18 months now.

Nevertheless I amended my patch to follow the reset procedure you
specified above, just to cover all bases. Thanks Michael.

Best regards,

Lukas

BCM4331 reset leads to wl lockup

[Arend van Spriel]

On 29-05-16 13:02, Lukas Wunner wrote:
> On Thu, May 26, 2016 at 02:42:46PM +0200, Michael B?sch wrote:
>> On Thu, 26 May 2016 14:12:10 +0200 Lukas Wunner <lukas@wunner.de> 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()
> 
> It turned out that the lockups are triggered by bec3cfdca36b
> ("net: skb_segment() provides list head and tail") in Linux 3.18
> and that Eric Duzamet has kindly provided a fix for broadcom-sta:
> https://bugs.gentoo.org/show_bug.cgi?id=523326#c24
> https://523326.bugs.gentoo.org/attachment.cgi?id=393374
> 
> @Broadcom: Please consider releasing a new driver version which
> incorporates that patch. The latest version 6.30.223.271 of your
> driver is still missing it even though the issue has existed for
> almost 18 months now.
> 
> Nevertheless I amended my patch to follow the reset procedure you
> specified above, just to cover all bases. Thanks Michael.

That reset procedure is a bit superseded. For brcmfmac we decided to
talk to some hardware engineers and looking at ai_core_disable (from
which bcma_core_disable() is (probably) derived) they shook their head
in horror. So better check brcmf_chip_ai_coredisable() in
.../brcm80211/brcmfmac/chip.c. The function needs a prereset and reset
parameter which are core specific. For the 802.11 core you need
preset=D11_BCMA_IOCTL_PHYRESET | D11_BCMA_IOCTL_PHYCLOCKEN and
reset=D11_BCMA_IOCTL_PHYCLOCKEN.

/* D11 core specific control flag bits */
#define D11_BCMA_IOCTL_PHYCLOCKEN	0x0004
#define D11_BCMA_IOCTL_PHYRESET		0x0008

I still have a patch for bcma lying around for this, but did not
complete the work for submission.

Regards,
Arend

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

BCM4331 reset leads to wl lockup

[Arend van Spriel]

On 29-05-16 13:02, Lukas Wunner wrote:
> On Thu, May 26, 2016 at 02:42:46PM +0200, Michael B?sch wrote:
>> On Thu, 26 May 2016 14:12:10 +0200 Lukas Wunner <lukas@wunner.de> 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()
> 
> It turned out that the lockups are triggered by bec3cfdca36b
> ("net: skb_segment() provides list head and tail") in Linux 3.18
> and that Eric Duzamet has kindly provided a fix for broadcom-sta:
> https://bugs.gentoo.org/show_bug.cgi?id=523326#c24
> https://523326.bugs.gentoo.org/attachment.cgi?id=393374

Looked at the patch and it provides little context. So before diving in
the code would you know if the patched broadcom-sta driver works for
kernels before 3.18?

Regards,
Arend

BCM4331 reset leads to wl lockup

[Lukas Wunner]

[cc += Eric Dumazet]

On Sun, May 29, 2016 at 08:55:01PM +0200, Arend van Spriel wrote:
> On 29-05-16 13:02, Lukas Wunner wrote:
> > On Thu, May 26, 2016 at 02:42:46PM +0200, Michael B?sch wrote:
> >> On Thu, 26 May 2016 14:12:10 +0200 Lukas Wunner <lukas@wunner.de> 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()
> > 
> > It turned out that the lockups are triggered by bec3cfdca36b
> > ("net: skb_segment() provides list head and tail") in Linux 3.18
> > and that Eric Dumazet has kindly provided a fix for broadcom-sta:
> > https://bugs.gentoo.org/show_bug.cgi?id=523326#c24
> > https://523326.bugs.gentoo.org/attachment.cgi?id=393374
> 
> Looked at the patch and it provides little context. So before diving in
> the code would you know if the patched broadcom-sta driver works for
> kernels before 3.18?

I'm not familiar with the broadcom-sta code but I'm inclined to say yes.

The function modified by the patch, wl_start(), contains an if/else
statement, the if-branch puts a packet to be transmitted on a work
queue and the else-branch transmits it straight away. Apparently
skb->prev isn't initialized to NULL for the else-branch which wasn't
an issue until bec3cfdca36b. That's my superficial understanding of
that code, I'm sure you have access to the full source and revision
history and can make more sense of it than I do.

Best regards,

Lukas