[PATCH] mwifiex: propagate error if IRQ request fails in mwifiex_sdio_of()

[PATCH] mwifiex: propagate error if IRQ request fails in mwifiex_sdio_of()

[Javier Martinez Canillas]

If request_irq() fails in mwifiex_sdio_probe_of(), only an error message
is printed but the actual error is not propagated to the caller function.

Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
---

 drivers/net/wireless/marvell/mwifiex/sdio.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c b/drivers/net/wireless/marvell/mwifiex/sdio.c
index d3e1561ca075..00727936ad6e 100644
--- a/drivers/net/wireless/marvell/mwifiex/sdio.c
+++ b/drivers/net/wireless/marvell/mwifiex/sdio.c
@@ -125,6 +125,7 @@ static int mwifiex_sdio_probe_of(struct device *dev, struct sdio_mmc_card *card)
 				dev_err(dev,
 					"Failed to request irq_wifi %d (%d)\n",
 					cfg->irq_wifi, ret);
+				return ret;
 			}
 			disable_irq(cfg->irq_wifi);
 		}
-- 
2.5.5

Re: [PATCH] mwifiex: propagate error if IRQ request fails in mwifiex_sdio_of()

[Arend van Spriel]

On 18-08-16 16:17, Javier Martinez Canillas wrote:
> If request_irq() fails in mwifiex_sdio_probe_of(), only an error message
> is printed but the actual error is not propagated to the caller function.

Hmm. The caller function, ie. mwifiex_sdio_probe(), does not seem to care.

The device may still function without this wake interrupt.

Regards,
Arend

> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
> ---
> 
>  drivers/net/wireless/marvell/mwifiex/sdio.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c b/drivers/net/wireless/marvell/mwifiex/sdio.c
> index d3e1561ca075..00727936ad6e 100644
> --- a/drivers/net/wireless/marvell/mwifiex/sdio.c
> +++ b/drivers/net/wireless/marvell/mwifiex/sdio.c
> @@ -125,6 +125,7 @@ static int mwifiex_sdio_probe_of(struct device *dev, struct sdio_mmc_card *card)
>  				dev_err(dev,
>  					"Failed to request irq_wifi %d (%d)\n",
>  					cfg->irq_wifi, ret);
> +				return ret;
>  			}
>  			disable_irq(cfg->irq_wifi);
>  		}
> 

Re: [PATCH] mwifiex: propagate error if IRQ request fails in mwifiex_sdio_of()

[Javier Martinez Canillas]

Hello Arend,

Thanks a lot for your feedback.

On 08/18/2016 03:14 PM, Arend van Spriel wrote:
> On 18-08-16 16:17, Javier Martinez Canillas wrote:
>> If request_irq() fails in mwifiex_sdio_probe_of(), only an error message
>> is printed but the actual error is not propagated to the caller function.
> 
> Hmm. The caller function, ie. mwifiex_sdio_probe(), does not seem to care.
>

Hmm, I'm not so sure about that. It's checking the wifiex_sdio_probe_of()
return value.

If the IRQ request failing is not an error, then at the very least the call
to disable_irq() should be avoided if request_irq() fails, and the message
should be changed from dev_err() to dev_dgb() or dev_info().
 
> The device may still function without this wake interrupt.
>

That's correct, the binding says that the "interrupts" property in the child
node is optional since is just a wakeup IRQ. Now the question is if should
be an error if the IRQ is defined but fails to be requested.

> Regards,
> Arend
> 

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: [PATCH] mwifiex: propagate error if IRQ request fails in mwifiex_sdio_of()

[Arend van Spriel]

On 18-08-16 21:29, Javier Martinez Canillas wrote:
> Hello Arend,
> 
> Thanks a lot for your feedback.
> 
> On 08/18/2016 03:14 PM, Arend van Spriel wrote:
>> On 18-08-16 16:17, Javier Martinez Canillas wrote:
>>> If request_irq() fails in mwifiex_sdio_probe_of(), only an error message
>>> is printed but the actual error is not propagated to the caller function.
>>
>> Hmm. The caller function, ie. mwifiex_sdio_probe(), does not seem to care.
>>
> 
> Hmm, I'm not so sure about that. It's checking the wifiex_sdio_probe_of()
> return value.

Ok. I looked at 4.7 sources on lxr [1].

> If the IRQ request failing is not an error, then at the very least the call
> to disable_irq() should be avoided if request_irq() fails, and the message
> should be changed from dev_err() to dev_dgb() or dev_info().

agree.

>> The device may still function without this wake interrupt.
>>
> 
> That's correct, the binding says that the "interrupts" property in the child
> node is optional since is just a wakeup IRQ. Now the question is if should
> be an error if the IRQ is defined but fails to be requested.

Clearly it indicates an error in the DT specification so behavior is not
as expected. Personally I would indeed consider it an error, but I was
just indicating that it might have done like this intentionally.

Regards,
Arend

[1]
http://lxr.free-electrons.com/source/drivers/net/wireless/marvell/mwifiex/sdio.c#L192

>> Regards,
>> Arend
>>
> 
> Best regards,
> 

Re: [PATCH] mwifiex: propagate error if IRQ request fails in mwifiex_sdio_of()

[Javier Martinez Canillas]

Hello Arend,

On 08/18/2016 03:49 PM, Arend van Spriel wrote:
> 
> 
> On 18-08-16 21:29, Javier Martinez Canillas wrote:
>> Hello Arend,
>>
>> Thanks a lot for your feedback.
>>
>> On 08/18/2016 03:14 PM, Arend van Spriel wrote:
>>> On 18-08-16 16:17, Javier Martinez Canillas wrote:
>>>> If request_irq() fails in mwifiex_sdio_probe_of(), only an error message
>>>> is printed but the actual error is not propagated to the caller function.
>>>
>>> Hmm. The caller function, ie. mwifiex_sdio_probe(), does not seem to care.
>>>
>>
>> Hmm, I'm not so sure about that. It's checking the wifiex_sdio_probe_of()
>> return value.
> 
> Ok. I looked at 4.7 sources on lxr [1].
>

Oh, right. That was fixed quite recently indeed.
 
>> If the IRQ request failing is not an error, then at the very least the call
>> to disable_irq() should be avoided if request_irq() fails, and the message
>> should be changed from dev_err() to dev_dgb() or dev_info().
> 
> agree.
> 
>>> The device may still function without this wake interrupt.
>>>
>>
>> That's correct, the binding says that the "interrupts" property in the child
>> node is optional since is just a wakeup IRQ. Now the question is if should
>> be an error if the IRQ is defined but fails to be requested.
> 
> Clearly it indicates an error in the DT specification so behavior is not
> as expected. Personally I would indeed consider it an error, but I was
> just indicating that it might have done like this intentionally.
>

Yes, might had been done intentionally indeed but I don't think that is
the case since the driver lacked error checking and propagation in many
places. But if someone thinks that's better to not honor the DT and at
least have the driver working without the wake up capability, then I'm
happy to respin the patch and change the print log level to info/debug.
 
> Regards,
> Arend
> 

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: mwifiex: propagate error if IRQ request fails in mwifiex_sdio_of()

[Kalle Valo]

Javier Martinez Canillas <javier@osg.samsung.com> wrote:
> If request_irq() fails in mwifiex_sdio_probe_of(), only an error message
> is printed but the actual error is not propagated to the caller function.
> 
> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>

What's the conclusion with this patch? Should I drop it or take it?

(The discussion is available from the patchwork link in the signature.)

-- 
Sent by pwcli
https://patchwork.kernel.org/patch/9288169/

Re: mwifiex: propagate error if IRQ request fails in mwifiex_sdio_of()

[Javier Martinez Canillas]

Hello Kalle,

On 09/03/2016 12:35 PM, Kalle Valo wrote:
> Javier Martinez Canillas <javier@osg.samsung.com> wrote:
>> If request_irq() fails in mwifiex_sdio_probe_of(), only an error message
>> is printed but the actual error is not propagated to the caller function.
>>
>> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
> 
> What's the conclusion with this patch? Should I drop it or take it?
> 
> (The discussion is available from the patchwork link in the signature.)
> 

My understanding is that Arend agrees with the patch and that the question
raised was caused by looking at an older kernel version. IOW, the patch is
OK and should be picked.

I'm adding Arend to cc, so can comment in case I misunderstood him though.

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

RE: mwifiex: propagate error if IRQ request fails in mwifiex_sdio_of()

[Amitkumar Karwar]

Hi Javier,

> From: Javier Martinez Canillas [mailto:javier@osg.samsung.com]
> Sent: Tuesday, September 06, 2016 5:43 PM
> To: Kalle Valo
> Cc: linux-kernel@vger.kernel.org; Amitkumar Karwar;
> netdev@vger.kernel.org; linux-wireless@vger.kernel.org; Nishant
> Sarmukadam; Arend van Spriel
> Subject: Re: mwifiex: propagate error if IRQ request fails in
> mwifiex_sdio_of()
> 
> Hello Kalle,
> 
> On 09/03/2016 12:35 PM, Kalle Valo wrote:
> > Javier Martinez Canillas <javier@osg.samsung.com> wrote:
> >> If request_irq() fails in mwifiex_sdio_probe_of(), only an error
> >> message is printed but the actual error is not propagated to the
> caller function.
> >>
> >> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
> >
> > What's the conclusion with this patch? Should I drop it or take it?
> >
> > (The discussion is available from the patchwork link in the
> > signature.)
> >
> 
> My understanding is that Arend agrees with the patch and that the
> question raised was caused by looking at an older kernel version. IOW,
> the patch is OK and should be picked.
> 
> I'm adding Arend to cc, so can comment in case I misunderstood him
> though.
> 

This error doesn't affect actual wifi functionality. Only thing is wakeup on interrupt when system is in suspended state won't work.
I think, we can make below change. 

------------------
@@ -122,9 +122,11 @@ static int mwifiex_sdio_probe_of(struct device *dev, struct sdio_mmc_card *card)
                                               IRQF_TRIGGER_LOW,
                                               "wifi_wake", cfg);
           if (ret) {
-                    dev_err(dev,
+                    dev_dbg(dev,
                             "Failed to request irq_wifi %d (%d)\n",
                             cfg->irq_wifi, ret);
+                    card->plt_wake_cfg = NULL;
+                    return 0;
             }
                disable_irq(cfg->irq_wifi);
            }
-----------------

Regards,
Amitkumar

Re: mwifiex: propagate error if IRQ request fails in mwifiex_sdio_of()

[Javier Martinez Canillas]

Hello Amitkumar,

On 09/08/2016 11:55 AM, Amitkumar Karwar wrote:
> Hi Javier,
> 
>> From: Javier Martinez Canillas [mailto:javier@osg.samsung.com]
>> Sent: Tuesday, September 06, 2016 5:43 PM
>> To: Kalle Valo
>> Cc: linux-kernel@vger.kernel.org; Amitkumar Karwar;
>> netdev@vger.kernel.org; linux-wireless@vger.kernel.org; Nishant
>> Sarmukadam; Arend van Spriel
>> Subject: Re: mwifiex: propagate error if IRQ request fails in
>> mwifiex_sdio_of()
>>
>> Hello Kalle,
>>
>> On 09/03/2016 12:35 PM, Kalle Valo wrote:
>>> Javier Martinez Canillas <javier@osg.samsung.com> wrote:
>>>> If request_irq() fails in mwifiex_sdio_probe_of(), only an error
>>>> message is printed but the actual error is not propagated to the
>> caller function.
>>>>
>>>> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
>>>
>>> What's the conclusion with this patch? Should I drop it or take it?
>>>
>>> (The discussion is available from the patchwork link in the
>>> signature.)
>>>
>>
>> My understanding is that Arend agrees with the patch and that the
>> question raised was caused by looking at an older kernel version. IOW,
>> the patch is OK and should be picked.
>>
>> I'm adding Arend to cc, so can comment in case I misunderstood him
>> though.
>>
> 
> This error doesn't affect actual wifi functionality. Only thing is wakeup on interrupt when system is in suspended state won't work.
> I think, we can make below change. 
> 
> ------------------
> @@ -122,9 +122,11 @@ static int mwifiex_sdio_probe_of(struct device *dev, struct sdio_mmc_card *card)
>                                                IRQF_TRIGGER_LOW,
>                                                "wifi_wake", cfg);
>            if (ret) {
> -                    dev_err(dev,
> +                    dev_dbg(dev,
>                              "Failed to request irq_wifi %d (%d)\n",
>                              cfg->irq_wifi, ret);
> +                    card->plt_wake_cfg = NULL;
> +                    return 0;
>              }

I'm OK with that change. Feel free too add my Reviewed-by if you post it.

>                 disable_irq(cfg->irq_wifi);
>             }
> -----------------
> 
> Regards,
> Amitkumar
> 

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America