From: Javier Martinez Canillas <hidden> Date: 2016-05-27 14:18:40
Hello,
While booting a system with a mwifiex WiFi card, I noticed the following
missleading error message:
[ 12.480042] mwifiex_sdio mmc2:0001:1: sdio platform data not available
This error only applies to platforms that define a child node for the SDIO
device, but it's currently shown even in platforms that don't have a child
node defined.
So this series fixes this issue and others I found in the .probe function
(mostly related to error handling and the error path) while looking at it.
Best regards,
Javier
Javier Martinez Canillas (8):
mwifiex: only call mwifiex_sdio_probe_of() if dev has an OF node
mwifiex: propagate sdio_enable_func() errno code in
mwifiex_sdio_probe()
mwifiex: propagate mwifiex_add_card() errno code in
mwifiex_sdio_probe()
mwifiex: consolidate mwifiex_sdio_probe() error paths
mwifiex: use dev_err() instead of pr_err() in mwifiex_sdio_probe()
mwifiex: check if mwifiex_sdio_probe_of() fails and return error
mwifiex: don't print an error if an optional DT property is missing
mwifiex: use better message and error code when OF node doesn't match
drivers/net/wireless/marvell/mwifiex/sdio.c | 46 ++++++++++++++++++-----------
1 file changed, 28 insertions(+), 18 deletions(-)
--
2.5.5
From: Javier Martinez Canillas <hidden> Date: 2016-05-27 14:18:46
SDIO is an auto enumerable bus so the SDIO devices are matched using the
sdio_device_id table and not using compatible strings from a OF id table.
However, commit ce4f6f0c353b ("mwifiex: add platform specific wakeup
interrupt support") allowed to match nodes defined as child of the SDIO
host controller in the probe function using a compatible string to setup
platform specific parameters in the DT.
The problem is that the OF parse function is always called regardless if
the SDIO dev has an OF node associated or not, and prints an error if it
is not found. So, on a platform that doesn't have a node for a SDIO dev,
the following misleading error message will be printed:
[ 12.480042] mwifiex_sdio mmc2:0001:1: sdio platform data not available
Signed-off-by: Javier Martinez Canillas <redacted>
---
drivers/net/wireless/marvell/mwifiex/sdio.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
From: Javier Martinez Canillas <hidden> Date: 2016-05-27 14:18:52
If the sdio_enable_func() function fails on .probe, the -EIO errno code
is always returned but that could make more difficult to debug and find
the cause of why the function actually failed.
Since the driver/device core prints the value returned by .probe in its
error message propagate what was returned by sdio_enable_func() at fail.
Signed-off-by: Javier Martinez Canillas <redacted>
---
drivers/net/wireless/marvell/mwifiex/sdio.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: Javier Martinez Canillas <hidden> Date: 2016-05-27 14:18:58
There's only a check if mwifiex_add_card() returned a nonzero value, but
the actual error code is neither stored nor propagated to the caller. So
instead of always returning -1 (which is -EPERM and not a suitable errno
code in this case), propagate the value returned by mwifiex_add_card().
Patch also removes the assignment of sdio_disable_func() returned value
since it was overwritten anyways and what matters is to know the error
value returned by the first function that failed.
Signed-off-by: Javier Martinez Canillas <redacted>
---
drivers/net/wireless/marvell/mwifiex/sdio.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
From: Javier Martinez Canillas <hidden> Date: 2016-05-27 14:19:07
Instead of duplicating part of the cleanups needed in case of an error
in .probe callback, have a single error path and use goto labels as is
common practice in the kernel.
This also has the nice side effect that the cleanup operations are made
in the inverse order of their counterparts, which was not the case for
the mwifiex_add_card() error path.
Signed-off-by: Javier Martinez Canillas <redacted>
---
drivers/net/wireless/marvell/mwifiex/sdio.c | 17 +++++++++++------
1 file changed, 11 insertions(+), 6 deletions(-)
From: Javier Martinez Canillas <hidden> Date: 2016-05-27 14:19:11
It's better to have the device name prefixed in the error message.
Signed-off-by: Javier Martinez Canillas <redacted>
---
drivers/net/wireless/marvell/mwifiex/sdio.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
From: Javier Martinez Canillas <hidden> Date: 2016-05-27 14:19:23
The function can fail so the returned value should be checked
and the error propagated to the caller in case of a failure.
Signed-off-by: Javier Martinez Canillas <redacted>
---
drivers/net/wireless/marvell/mwifiex/sdio.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
From: Javier Martinez Canillas <hidden> Date: 2016-05-27 14:19:30
The Documentation/devicetree/bindings/net/wireless/marvell-sd8xxx.txt DT
binding document lists the possible compatible strings that a SDIO child
node can have, so the driver checks if the defined in the node matches.
But the error message when that's not the case is misleading, so change
for one that makes clear what the error really is. Also, returning a -1
as errno code is not correct since that's -EPERM. A -EINVAL seems to be
a more appropriate one.
Signed-off-by: Javier Martinez Canillas <redacted>
---
drivers/net/wireless/marvell/mwifiex/sdio.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
From: Javier Martinez Canillas <hidden> Date: 2016-05-27 14:19:54
The Documentation/devicetree/bindings/net/wireless/marvell-sd8xxx.txt DT
binding document say that the "interrupts" property in the child node is
optional. So the property being missed shouldn't be treated as an error.
Signed-off-by: Javier Martinez Canillas <redacted>
---
drivers/net/wireless/marvell/mwifiex/sdio.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
Hi Javier,
2016-05-27 16:18 GMT+02:00 Javier Martinez Canillas [off-list ref]:
Hello,
While booting a system with a mwifiex WiFi card, I noticed the following
missleading error message:
[ 12.480042] mwifiex_sdio mmc2:0001:1: sdio platform data not available
This error only applies to platforms that define a child node for the SDIO
device, but it's currently shown even in platforms that don't have a child
node defined.
So this series fixes this issue and others I found in the .probe function
(mostly related to error handling and the error path) while looking at it.
The patches looks good to me and tested on my Veyron Chromebook, so
for all this series:
Tested-by: Enric Balletbo i Serra <redacted>
Thanks,
Enric
Best regards,
Javier
Javier Martinez Canillas (8):
mwifiex: only call mwifiex_sdio_probe_of() if dev has an OF node
mwifiex: propagate sdio_enable_func() errno code in
mwifiex_sdio_probe()
mwifiex: propagate mwifiex_add_card() errno code in
mwifiex_sdio_probe()
mwifiex: consolidate mwifiex_sdio_probe() error paths
mwifiex: use dev_err() instead of pr_err() in mwifiex_sdio_probe()
mwifiex: check if mwifiex_sdio_probe_of() fails and return error
mwifiex: don't print an error if an optional DT property is missing
mwifiex: use better message and error code when OF node doesn't match
drivers/net/wireless/marvell/mwifiex/sdio.c | 46 ++++++++++++++++++-----------
1 file changed, 28 insertions(+), 18 deletions(-)
--
2.5.5
Hi All,
On Sat, May 28, 2016 at 12:18 AM, Javier Martinez Canillas
[off-list ref] wrote:
SDIO is an auto enumerable bus so the SDIO devices are matched using the
sdio_device_id table and not using compatible strings from a OF id table.
However, commit ce4f6f0c353b ("mwifiex: add platform specific wakeup
interrupt support") allowed to match nodes defined as child of the SDIO
host controller in the probe function using a compatible string to setup
platform specific parameters in the DT.
The problem is that the OF parse function is always called regardless if
the SDIO dev has an OF node associated or not, and prints an error if it
is not found. So, on a platform that doesn't have a node for a SDIO dev,
the following misleading error message will be printed:
[ 12.480042] mwifiex_sdio mmc2:0001:1: sdio platform data not available
Signed-off-by: Javier Martinez Canillas <redacted>
This looks sensible to me.
Reviewed-by: Julian Calaby <redacted>
Thanks,
--
Julian Calaby
Email: julian.calaby-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org
Profile: http://www.google.com/profiles/julian.calaby/
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi All,
On Sat, May 28, 2016 at 12:18 AM, Javier Martinez Canillas
[off-list ref] wrote:
If the sdio_enable_func() function fails on .probe, the -EIO errno code
is always returned but that could make more difficult to debug and find
the cause of why the function actually failed.
Since the driver/device core prints the value returned by .probe in its
error message propagate what was returned by sdio_enable_func() at fail.
Signed-off-by: Javier Martinez Canillas <redacted>
This looks sensible to me.
Reviewed-by: Julian Calaby <redacted>
Hi All,
On Sat, May 28, 2016 at 12:18 AM, Javier Martinez Canillas
[off-list ref] wrote:
There's only a check if mwifiex_add_card() returned a nonzero value, but
the actual error code is neither stored nor propagated to the caller. So
instead of always returning -1 (which is -EPERM and not a suitable errno
code in this case), propagate the value returned by mwifiex_add_card().
Patch also removes the assignment of sdio_disable_func() returned value
since it was overwritten anyways and what matters is to know the error
value returned by the first function that failed.
Signed-off-by: Javier Martinez Canillas <redacted>
This looks sensible to me.
Reviewed-by: Julian Calaby <redacted>
Hi All,
On Sat, May 28, 2016 at 12:18 AM, Javier Martinez Canillas
[off-list ref] wrote:
Instead of duplicating part of the cleanups needed in case of an error
in .probe callback, have a single error path and use goto labels as is
common practice in the kernel.
This also has the nice side effect that the cleanup operations are made
in the inverse order of their counterparts, which was not the case for
the mwifiex_add_card() error path.
Signed-off-by: Javier Martinez Canillas <redacted>
This looks sensible to me.
Reviewed-by: Julian Calaby <redacted>
Hi All,
On Sat, May 28, 2016 at 12:18 AM, Javier Martinez Canillas
[off-list ref] wrote:
The function can fail so the returned value should be checked
and the error propagated to the caller in case of a failure.
Signed-off-by: Javier Martinez Canillas <redacted>
This looks sensible to me.
Reviewed-by: Julian Calaby <redacted>
Hi All,
On Sat, May 28, 2016 at 12:18 AM, Javier Martinez Canillas
[off-list ref] wrote:
The Documentation/devicetree/bindings/net/wireless/marvell-sd8xxx.txt DT
binding document say that the "interrupts" property in the child node is
optional. So the property being missed shouldn't be treated as an error.
Have you checked whether it is truly optional? I.e. nothing else
breaks if this property isn't set?
Signed-off-by: Javier Martinez Canillas <redacted>
Other than that, this looks sensible to me.
Reviewed-by: Julian Calaby <redacted>
Hi All,
On Sat, May 28, 2016 at 12:18 AM, Javier Martinez Canillas
[off-list ref] wrote:
The Documentation/devicetree/bindings/net/wireless/marvell-sd8xxx.txt DT
binding document lists the possible compatible strings that a SDIO child
node can have, so the driver checks if the defined in the node matches.
But the error message when that's not the case is misleading, so change
for one that makes clear what the error really is. Also, returning a -1
as errno code is not correct since that's -EPERM. A -EINVAL seems to be
a more appropriate one.
Signed-off-by: Javier Martinez Canillas <redacted>
This looks sensible to me.
Reviewed-by: Julian Calaby <redacted>
From: Javier Martinez Canillas <hidden> Date: 2016-06-01 13:51:43
Hello Julian,
Thanks a lot for your feedback and reviews.
On 06/01/2016 12:20 AM, Julian Calaby wrote:
Hi All,
On Sat, May 28, 2016 at 12:18 AM, Javier Martinez Canillas
[off-list ref] wrote:
quoted
The Documentation/devicetree/bindings/net/wireless/marvell-sd8xxx.txt DT
binding document say that the "interrupts" property in the child node is
optional. So the property being missed shouldn't be treated as an error.
Have you checked whether it is truly optional? I.e. nothing else
breaks if this property isn't set?
That's what the DT binding says and the IRQ is only used as a wakeup source
during system suspend, it is not used during runtime. And that is why the
mwifiex_sdio_probe_of() function does not fail if the IRQ is missing.
Now, I just got to that conclusion by reading the binding docs, the message
in the commits that introduced this and the driver code. Xinming Hu should
comment on how critical this feature is for systems that needs to be wakeup.
In any case I think that the code should be consistent with what the binding
doc says and also the function does (i.e: dev_err only if returns an error).
quoted
Signed-off-by: Javier Martinez Canillas <redacted>
Other than that, this looks sensible to me.
Reviewed-by: Julian Calaby <redacted>
Best regards,
--
Javier Martinez Canillas
Open Source Group
Samsung Research America
Hi Javier,
On Wed, Jun 1, 2016 at 11:51 PM, Javier Martinez Canillas
[off-list ref] wrote:
Hello Julian,
Thanks a lot for your feedback and reviews.
On 06/01/2016 12:20 AM, Julian Calaby wrote:
quoted
Hi All,
On Sat, May 28, 2016 at 12:18 AM, Javier Martinez Canillas
[off-list ref] wrote:
quoted
The Documentation/devicetree/bindings/net/wireless/marvell-sd8xxx.txt DT
binding document say that the "interrupts" property in the child node is
optional. So the property being missed shouldn't be treated as an error.
Have you checked whether it is truly optional? I.e. nothing else
breaks if this property isn't set?
That's what the DT binding says and the IRQ is only used as a wakeup source
during system suspend, it is not used during runtime. And that is why the
mwifiex_sdio_probe_of() function does not fail if the IRQ is missing.
Awesome, that's what I wanted to know.
Now, I just got to that conclusion by reading the binding docs, the message
in the commits that introduced this and the driver code. Xinming Hu should
comment on how critical this feature is for systems that needs to be wakeup.
Xinming, could you review this also?
In any case I think that the code should be consistent with what the binding
doc says and also the function does (i.e: dev_err only if returns an error).
quoted
quoted
Signed-off-by: Javier Martinez Canillas <redacted>
Other than that, this looks sensible to me.
Reviewed-by: Julian Calaby <redacted>
From: Javier Martinez Canillas [mailto:javier@osg.samsung.com]
Sent: Friday, May 27, 2016 7:48 PM
To: linux-kernel@vger.kernel.org
Cc: Xinming Hu; Javier Martinez Canillas; Amitkumar Karwar; Kalle Valo;
netdev@vger.kernel.org; linux-wireless@vger.kernel.org; Nishant
Sarmukadam
Subject: [PATCH 0/8] mwifiex: Fix some error handling issues in
mwifiex_sdio_probe() function
Hello,
While booting a system with a mwifiex WiFi card, I noticed the following
missleading error message:
[ 12.480042] mwifiex_sdio mmc2:0001:1: sdio platform data not available
This error only applies to platforms that define a child node for the
SDIO device, but it's currently shown even in platforms that don't have
a child node defined.
So this series fixes this issue and others I found in the .probe
function (mostly related to error handling and the error path) while
looking at it.
Best regards,
Javier
Javier Martinez Canillas (8):
mwifiex: only call mwifiex_sdio_probe_of() if dev has an OF node
mwifiex: propagate sdio_enable_func() errno code in
mwifiex_sdio_probe()
mwifiex: propagate mwifiex_add_card() errno code in
mwifiex_sdio_probe()
mwifiex: consolidate mwifiex_sdio_probe() error paths
mwifiex: use dev_err() instead of pr_err() in mwifiex_sdio_probe()
mwifiex: check if mwifiex_sdio_probe_of() fails and return error
mwifiex: don't print an error if an optional DT property is missing
mwifiex: use better message and error code when OF node doesn't match
drivers/net/wireless/marvell/mwifiex/sdio.c | 46 ++++++++++++++++++----
-------
1 file changed, 28 insertions(+), 18 deletions(-)
Thanks for fixing the error handling code. These patches look fine to me.
Acked-by: Amitkumar Karwar <redacted>
Regards,
Amitkumar
From: Julian Calaby [mailto:julian.calaby@gmail.com]
Sent: Thursday, June 02, 2016 4:44 AM
To: Javier Martinez Canillas; Xinming Hu
Cc: linux-kernel@vger.kernel.org; Amitkumar Karwar; Kalle Valo; netdev;
linux-wireless; Nishant Sarmukadam
Subject: Re: [PATCH 7/8] mwifiex: don't print an error if an optional DT
property is missing
Hi Javier,
On Wed, Jun 1, 2016 at 11:51 PM, Javier Martinez Canillas
[off-list ref] wrote:
quoted
Hello Julian,
Thanks a lot for your feedback and reviews.
On 06/01/2016 12:20 AM, Julian Calaby wrote:
quoted
Hi All,
On Sat, May 28, 2016 at 12:18 AM, Javier Martinez Canillas
[off-list ref] wrote:
quoted
The
Documentation/devicetree/bindings/net/wireless/marvell-sd8xxx.txt DT
binding document say that the "interrupts" property in the child
node is optional. So the property being missed shouldn't be treated as
an error.
quoted
quoted
Have you checked whether it is truly optional? I.e. nothing else
breaks if this property isn't set?
That's what the DT binding says and the IRQ is only used as a wakeup
source during system suspend, it is not used during runtime. And that
is why the
mwifiex_sdio_probe_of() function does not fail if the IRQ is missing.
Awesome, that's what I wanted to know.
quoted
Now, I just got to that conclusion by reading the binding docs, the
message in the commits that introduced this and the driver code.
Xinming Hu should comment on how critical this feature is for systems
that needs to be wakeup.
Xinming, could you review this also?
Yes. IRQ is the optional parameter. System has a flexibility to not use it, but it still can configure other device tree parameters. The patch looks good.
Regards,
Amitkumar
From: Kalle Valo <hidden> Date: 2016-06-16 15:05:38
Javier Martinez Canillas [off-list ref] wrote:
SDIO is an auto enumerable bus so the SDIO devices are matched using the
sdio_device_id table and not using compatible strings from a OF id table.
However, commit ce4f6f0c353b ("mwifiex: add platform specific wakeup
interrupt support") allowed to match nodes defined as child of the SDIO
host controller in the probe function using a compatible string to setup
platform specific parameters in the DT.
The problem is that the OF parse function is always called regardless if
the SDIO dev has an OF node associated or not, and prints an error if it
is not found. So, on a platform that doesn't have a node for a SDIO dev,
the following misleading error message will be printed:
[ 12.480042] mwifiex_sdio mmc2:0001:1: sdio platform data not available
Signed-off-by: Javier Martinez Canillas <redacted>
Reviewed-by: Julian Calaby <redacted>
Thanks, 8 patches applied to wireless-drivers-next.git:
6f49208fec85 mwifiex: only call mwifiex_sdio_probe_of() if dev has an OF node
cc524d1706b7 mwifiex: propagate sdio_enable_func() errno code in mwifiex_sdio_probe()
032e0f546c7e mwifiex: propagate mwifiex_add_card() errno code in mwifiex_sdio_probe()
a82f65aae143 mwifiex: consolidate mwifiex_sdio_probe() error paths
d3f04ece53a4 mwifiex: use dev_err() instead of pr_err() in mwifiex_sdio_probe()
213d9421c165 mwifiex: check if mwifiex_sdio_probe_of() fails and return error
806dd220340d mwifiex: don't print an error if an optional DT property is missing
5e94913f676a mwifiex: use better message and error code when OF node doesn't match
--
Sent by pwcli
https://patchwork.kernel.org/patch/9138513/