[PATCH] wireless: brcmfmac: initialize oob irq data before request_irq()

[PATCH] wireless: brcmfmac: initialize oob irq data before request_irq()

[Michał Mirosław]

This fixes spin-forever in irq handler when IRQ is already asserted
at request_irq() time.

Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
index 9b970dc2b922a..c653a72e3dead 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
@@ -108,12 +108,14 @@ int brcmf_sdiod_intr_register(struct brcmf_sdio_dev *sdiodev)
 	int ret = 0;
 	u8 data;
 	u32 addr, gpiocontrol;
-	unsigned long flags;
 
 	pdata = &sdiodev->settings->bus.sdio;
 	if (pdata->oob_irq_supported) {
 		brcmf_dbg(SDIO, "Enter, register OOB IRQ %d\n",
 			  pdata->oob_irq_nr);
+		spin_lock_init(&sdiodev->irq_en_lock);
+		sdiodev->irq_en = true;
+
 		ret = request_irq(pdata->oob_irq_nr, brcmf_sdiod_oob_irqhandler,
 				  pdata->oob_irq_flags, "brcmf_oob_intr",
 				  &sdiodev->func[1]->dev);
@@ -122,10 +124,6 @@ int brcmf_sdiod_intr_register(struct brcmf_sdio_dev *sdiodev)
 			return ret;
 		}
 		sdiodev->oob_irq_requested = true;
-		spin_lock_init(&sdiodev->irq_en_lock);
-		spin_lock_irqsave(&sdiodev->irq_en_lock, flags);
-		sdiodev->irq_en = true;
-		spin_unlock_irqrestore(&sdiodev->irq_en_lock, flags);
 
 		ret = enable_irq_wake(pdata->oob_irq_nr);
 		if (ret != 0) {
-- 
2.11.0

Re: [PATCH] wireless: brcmfmac: initialize oob irq data before request_irq()

[Kalle Valo]

Micha=C5=82 Miros=C5=82aw <mirq-linux@rere.qmqm.pl> writes:

> This fixes spin-forever in irq handler when IRQ is already asserted
> at request_irq() time.
>
> Signed-off-by: Micha=C5=82 Miros=C5=82aw <mirq-linux@rere.qmqm.pl>

Your name in patchwork is broken:

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

You could try to fix it by registering as then you have (onetime) option
to provide your full name.

--=20
Kalle Valo

Re: [PATCH] wireless: brcmfmac: initialize oob irq data before request_irq()

[Michał Mirosław]

On Wed, Jun 14, 2017 at 07:58:35AM +0300, Kalle Valo wrote:
> Michał Mirosław <mirq-linux@rere.qmqm.pl> writes:
> > This fixes spin-forever in irq handler when IRQ is already asserted
> > at request_irq() time.
> >
> > Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> 
> Your name in patchwork is broken:
> 
> https://patchwork.kernel.org/patch/9784251/
> 
> You could try to fix it by registering as then you have (onetime) option
> to provide your full name.

Doesn't like UTF-8? Registration didn't help though.

Best Regards,
Michał Mirosław

Re: [PATCH] wireless: brcmfmac: initialize oob irq data before request_irq()

[Kalle Valo]

Micha=C5=82 Miros=C5=82aw <mirq-linux@rere.qmqm.pl> writes:

> On Wed, Jun 14, 2017 at 07:58:35AM +0300, Kalle Valo wrote:
>> Micha=C5=82 Miros=C5=82aw <mirq-linux@rere.qmqm.pl> writes:
>> > This fixes spin-forever in irq handler when IRQ is already asserted
>> > at request_irq() time.
>> >
>> > Signed-off-by: Micha=C5=82 Miros=C5=82aw <mirq-linux@rere.qmqm.pl>
>>=20
>> Your name in patchwork is broken:
>>=20
>> https://patchwork.kernel.org/patch/9784251/
>>=20
>> You could try to fix it by registering as then you have (onetime) option
>> to provide your full name.
>
> Doesn't like UTF-8?

patchwork.kernel.org was updated something like a year ago to get proper
UTF-8 support and I have been succesfully applied patches from Rafal who
also uses UTF-8 with his name:

git log --author rafal@milecki.pl drivers/net/wireless/

You could always compare his posts with yours and see what you do
differently. Here's one example:

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

> Registration didn't help though.

Darn. This time I can manually apply the patch but for the future we
should try to solve this.

--=20
Kalle Valo

Re: [PATCH] wireless: brcmfmac: initialize oob irq data before request_irq()

[Michał Mirosław]

[resending reply to linux-wireless]

On Thu, Jun 15, 2017 at 05:40:01PM +0300, Kalle Valo wrote:
> Michał Mirosław <mirq-linux@rere.qmqm.pl> writes:
> 
> > On Wed, Jun 14, 2017 at 07:58:35AM +0300, Kalle Valo wrote:
> >> Michał Mirosław <mirq-linux@rere.qmqm.pl> writes:
> >> > This fixes spin-forever in irq handler when IRQ is already asserted
> >> > at request_irq() time.
> >> >
> >> > Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> >> 
> >> Your name in patchwork is broken:
> >> 
> >> https://patchwork.kernel.org/patch/9784251/
> >> 
> >> You could try to fix it by registering as then you have (onetime) option
> >> to provide your full name.
> >
> > Doesn't like UTF-8?
> 
> patchwork.kernel.org was updated something like a year ago to get proper
> UTF-8 support and I have been succesfully applied patches from Rafal who
> also uses UTF-8 with his name:
> 
> git log --author rafal@milecki.pl drivers/net/wireless/
> 
> You could always compare his posts with yours and see what you do
> differently. Here's one example:
> 
> https://patchwork.kernel.org/patch/9640743/
> 
> > Registration didn't help though.
> 
> Darn. This time I can manually apply the patch but for the future we
> should try to solve this.

Looks like my name, as known by Patchwork, got doubly UTF-8-encoded:

# this is how it is in From header in Patchwork:
$ echo TWljaGHDheKAmiBNaXJvc8OF4oCaYXc=|base64 -d|hexdump -C
00000000  4d 69 63 68 61 c3 85 e2  80 9a 20 4d 69 72 6f 73  |Micha.....
Miros|
00000010  c3 85 e2 80 9a 61 77                              |.....aw|
00000017

# this is how it should look like:
$ echo -n 'Michał Mirosław'|hexdump -C
00000000  4d 69 63 68 61 c5 82 20  4d 69 72 6f 73 c5 82 61  |Micha..
Miros..a|
00000010  77                                                |w|

Best Regards,
Michał Mirosław

Re: [PATCH] wireless: brcmfmac: initialize oob irq data before request_irq()

[Kalle Valo]

Micha=C5=82 Miros=C5=82aw <mirq-linux@rere.qmqm.pl> writes:

> This fixes spin-forever in irq handler when IRQ is already asserted
> at request_irq() time.
>
> Signed-off-by: Micha=C5=82 Miros=C5=82aw <mirq-linux@rere.qmqm.pl>

To avoid the UTF-8 problem I skipped patchwork and applied this manually
to wireless-drivers-next:

3f426c968955 brcmfmac: initialize oob irq data before request_irq()

But hopefully you can get your name in patchwork fixed so that I don't
need to manually apply your patches.

--=20
Kalle Valo

Re: [PATCH] wireless: brcmfmac: initialize oob irq data before request_irq()

[Arend van Spriel]

On 13-06-17 18:02, Michał Mirosław wrote:
> This fixes spin-forever in irq handler when IRQ is already asserted
> at request_irq() time.

With all the patchwork misery flying by I almost forgot to respond to
this. We suspect you are covering up a hardware issue here. At the time
of the request_irq() the device configuration for OOB is not yet done
and as such it is an input which makes the OOB line floating.

Can you tell me what platform you are seeing the spin-forever issue.
Could it be that is does not have a proper pull-up/down on the OOB irq line.

Could you try whether moving the request_irq() and enable_irq_wake()
calls further down after device configuration (see below) solve your
issue. Still it would be better to fix the hardware.

Regards,
Arend

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
b/drivers
index 9b970dc..e57a211 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
@@ -114,26 +114,11 @@ int brcmf_sdiod_intr_register(struct
brcmf_sdio_dev *sdiod
        if (pdata->oob_irq_supported) {
                brcmf_dbg(SDIO, "Enter, register OOB IRQ %d\n",
                          pdata->oob_irq_nr);
-               ret = request_irq(pdata->oob_irq_nr,
brcmf_sdiod_oob_irqhandler,
-                                 pdata->oob_irq_flags, "brcmf_oob_intr",
-                                 &sdiodev->func[1]->dev);
-               if (ret != 0) {
-                       brcmf_err("request_irq failed %d\n", ret);
-                       return ret;
-               }
-               sdiodev->oob_irq_requested = true;
                spin_lock_init(&sdiodev->irq_en_lock);
                spin_lock_irqsave(&sdiodev->irq_en_lock, flags);
                sdiodev->irq_en = true;
                spin_unlock_irqrestore(&sdiodev->irq_en_lock, flags);

-               ret = enable_irq_wake(pdata->oob_irq_nr);
-               if (ret != 0) {
-                       brcmf_err("enable_irq_wake failed %d\n", ret);
-                       return ret;
-               }
-               sdiodev->irq_wake = true;
-
                sdio_claim_host(sdiodev->func[1]);

                if (sdiodev->bus_if->chip == BRCM_CC_43362_CHIP_ID) {
@@ -161,6 +146,22 @@ int brcmf_sdiod_intr_register(struct brcmf_sdio_dev
*sdiode
                brcmf_sdiod_regwb(sdiodev, SDIO_CCCR_BRCM_SEPINT, data,
&ret);

                sdio_release_host(sdiodev->func[1]);
+
+               ret = request_irq(pdata->oob_irq_nr,
brcmf_sdiod_oob_irqhandler,
+                                 pdata->oob_irq_flags, "brcmf_oob_intr",
+                                 &sdiodev->func[1]->dev);
+               if (ret != 0) {
+                       brcmf_err("request_irq failed %d\n", ret);
+                       return ret;
+               }
+               sdiodev->oob_irq_requested = true;
+
+               ret = enable_irq_wake(pdata->oob_irq_nr);
+               if (ret != 0) {
+                       brcmf_err("enable_irq_wake failed %d\n", ret);
+                       return ret;
+               }
+               sdiodev->irq_wake = true;
        } else {
                brcmf_dbg(SDIO, "Entering\n");
                sdio_claim_host(sdiodev->func[1]);