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
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 <[email protected]>
---
drivers/net/wireless/marvell/mwifiex/sdio.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c b/drivers/net/wireless/marvell/mwifiex/sdio.c
index 1ffbb972318f..1c17e624547a 100644
--- a/drivers/net/wireless/marvell/mwifiex/sdio.c
+++ b/drivers/net/wireless/marvell/mwifiex/sdio.c
@@ -187,8 +187,13 @@ mwifiex_sdio_probe(struct sdio_func *func, const struct sdio_device_id *id)
}
/* device tree node parsing and platform specific configuration*/
- if (func->dev.of_node)
- mwifiex_sdio_probe_of(&func->dev, card);
+ if (func->dev.of_node) {
+ ret = mwifiex_sdio_probe_of(&func->dev, card);
+ if (ret) {
+ dev_err(&func->dev, "SDIO dt node parse failed\n");
+ goto err_disable;
+ }
+ }
ret = mwifiex_add_card(card, &add_remove_card_sem, &sdio_ops,
MWIFIEX_SDIO);
--
2.5.5
Hi Javier,
2016-05-27 16:18 GMT+02:00 Javier Martinez Canillas <[email protected]>:
> 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 <[email protected]>
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
>
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 <[email protected]>
---
drivers/net/wireless/marvell/mwifiex/sdio.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c b/drivers/net/wireless/marvell/mwifiex/sdio.c
index bdc51ffd43ec..285b1b68f7e9 100644
--- a/drivers/net/wireless/marvell/mwifiex/sdio.c
+++ b/drivers/net/wireless/marvell/mwifiex/sdio.c
@@ -102,8 +102,7 @@ static int mwifiex_sdio_probe_of(struct device *dev, struct sdio_mmc_card *card)
struct mwifiex_plt_wake_cfg *cfg;
int ret;
- if (!dev->of_node ||
- !of_match_node(mwifiex_sdio_of_match_table, dev->of_node)) {
+ if (!of_match_node(mwifiex_sdio_of_match_table, dev->of_node)) {
dev_err(dev, "sdio platform data not available\n");
return -1;
}
@@ -189,7 +188,8 @@ mwifiex_sdio_probe(struct sdio_func *func, const struct sdio_device_id *id)
}
/* device tree node parsing and platform specific configuration*/
- mwifiex_sdio_probe_of(&func->dev, card);
+ if (func->dev.of_node)
+ mwifiex_sdio_probe_of(&func->dev, card);
if (mwifiex_add_card(card, &add_remove_card_sem, &sdio_ops,
MWIFIEX_SDIO)) {
--
2.5.5
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 <[email protected]>
---
drivers/net/wireless/marvell/mwifiex/sdio.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c b/drivers/net/wireless/marvell/mwifiex/sdio.c
index 285b1b68f7e9..ab64507c84e1 100644
--- a/drivers/net/wireless/marvell/mwifiex/sdio.c
+++ b/drivers/net/wireless/marvell/mwifiex/sdio.c
@@ -184,7 +184,7 @@ mwifiex_sdio_probe(struct sdio_func *func, const struct sdio_device_id *id)
if (ret) {
pr_err("%s: failed to enable function\n", __func__);
kfree(card);
- return -EIO;
+ return ret;
}
/* device tree node parsing and platform specific configuration*/
--
2.5.5
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 <[email protected]>
---
drivers/net/wireless/marvell/mwifiex/sdio.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c b/drivers/net/wireless/marvell/mwifiex/sdio.c
index 1c17e624547a..8b3292eaecb2 100644
--- a/drivers/net/wireless/marvell/mwifiex/sdio.c
+++ b/drivers/net/wireless/marvell/mwifiex/sdio.c
@@ -114,7 +114,7 @@ static int mwifiex_sdio_probe_of(struct device *dev, struct sdio_mmc_card *card)
if (cfg && card->plt_of_node) {
cfg->irq_wifi = irq_of_parse_and_map(card->plt_of_node, 0);
if (!cfg->irq_wifi) {
- dev_err(dev,
+ dev_dbg(dev,
"fail to parse irq_wifi from device tree\n");
} else {
ret = devm_request_irq(dev, cfg->irq_wifi,
--
2.5.5
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 <[email protected]>
---
drivers/net/wireless/marvell/mwifiex/sdio.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c b/drivers/net/wireless/marvell/mwifiex/sdio.c
index ab64507c84e1..81003fbe5025 100644
--- a/drivers/net/wireless/marvell/mwifiex/sdio.c
+++ b/drivers/net/wireless/marvell/mwifiex/sdio.c
@@ -191,14 +191,14 @@ mwifiex_sdio_probe(struct sdio_func *func, const struct sdio_device_id *id)
if (func->dev.of_node)
mwifiex_sdio_probe_of(&func->dev, card);
- if (mwifiex_add_card(card, &add_remove_card_sem, &sdio_ops,
- MWIFIEX_SDIO)) {
+ ret = mwifiex_add_card(card, &add_remove_card_sem, &sdio_ops,
+ MWIFIEX_SDIO);
+ if (ret) {
pr_err("%s: add card failed\n", __func__);
kfree(card);
sdio_claim_host(func);
- ret = sdio_disable_func(func);
+ sdio_disable_func(func);
sdio_release_host(func);
- ret = -1;
}
return ret;
--
2.5.5
It's better to have the device name prefixed in the error message.
Signed-off-by: Javier Martinez Canillas <[email protected]>
---
drivers/net/wireless/marvell/mwifiex/sdio.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c b/drivers/net/wireless/marvell/mwifiex/sdio.c
index 7aeee88b858f..1ffbb972318f 100644
--- a/drivers/net/wireless/marvell/mwifiex/sdio.c
+++ b/drivers/net/wireless/marvell/mwifiex/sdio.c
@@ -182,7 +182,7 @@ mwifiex_sdio_probe(struct sdio_func *func, const struct sdio_device_id *id)
sdio_release_host(func);
if (ret) {
- pr_err("%s: failed to enable function\n", __func__);
+ dev_err(&func->dev, "failed to enable function\n");
goto err_free;
}
@@ -193,7 +193,7 @@ mwifiex_sdio_probe(struct sdio_func *func, const struct sdio_device_id *id)
ret = mwifiex_add_card(card, &add_remove_card_sem, &sdio_ops,
MWIFIEX_SDIO);
if (ret) {
- pr_err("%s: add card failed\n", __func__);
+ dev_err(&func->dev, "add card failed\n");
goto err_disable;
}
--
2.5.5
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 <[email protected]>
---
drivers/net/wireless/marvell/mwifiex/sdio.c | 17 +++++++++++------
1 file changed, 11 insertions(+), 6 deletions(-)
diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c b/drivers/net/wireless/marvell/mwifiex/sdio.c
index 81003fbe5025..7aeee88b858f 100644
--- a/drivers/net/wireless/marvell/mwifiex/sdio.c
+++ b/drivers/net/wireless/marvell/mwifiex/sdio.c
@@ -183,8 +183,7 @@ mwifiex_sdio_probe(struct sdio_func *func, const struct sdio_device_id *id)
if (ret) {
pr_err("%s: failed to enable function\n", __func__);
- kfree(card);
- return ret;
+ goto err_free;
}
/* device tree node parsing and platform specific configuration*/
@@ -195,12 +194,18 @@ mwifiex_sdio_probe(struct sdio_func *func, const struct sdio_device_id *id)
MWIFIEX_SDIO);
if (ret) {
pr_err("%s: add card failed\n", __func__);
- kfree(card);
- sdio_claim_host(func);
- sdio_disable_func(func);
- sdio_release_host(func);
+ goto err_disable;
}
+ return 0;
+
+err_disable:
+ sdio_claim_host(func);
+ sdio_disable_func(func);
+ sdio_release_host(func);
+err_free:
+ kfree(card);
+
return ret;
}
--
2.5.5
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 <[email protected]>
---
drivers/net/wireless/marvell/mwifiex/sdio.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c b/drivers/net/wireless/marvell/mwifiex/sdio.c
index 8b3292eaecb2..e6d56be04e08 100644
--- a/drivers/net/wireless/marvell/mwifiex/sdio.c
+++ b/drivers/net/wireless/marvell/mwifiex/sdio.c
@@ -103,8 +103,8 @@ static int mwifiex_sdio_probe_of(struct device *dev, struct sdio_mmc_card *card)
int ret;
if (!of_match_node(mwifiex_sdio_of_match_table, dev->of_node)) {
- dev_err(dev, "sdio platform data not available\n");
- return -1;
+ dev_err(dev, "required compatible string missing\n");
+ return -EINVAL;
}
card->plt_of_node = dev->of_node;
--
2.5.5
Hi All,
On Sat, May 28, 2016 at 12:18 AM, Javier Martinez Canillas
<[email protected]> 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 <[email protected]>
This looks sensible to me.
Reviewed-by: Julian Calaby <[email protected]>
>
> ---
>
> drivers/net/wireless/marvell/mwifiex/sdio.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
Thanks,
--
Julian Calaby
Email: [email protected]
Profile: http://www.google.com/profiles/julian.calaby/
Hi All,
On Sat, May 28, 2016 at 12:18 AM, Javier Martinez Canillas
<[email protected]> 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 <[email protected]>
This looks sensible to me.
Reviewed-by: Julian Calaby <[email protected]>
> ---
>
> drivers/net/wireless/marvell/mwifiex/sdio.c | 17 +++++++++++------
> 1 file changed, 11 insertions(+), 6 deletions(-)
Thanks,
--
Julian Calaby
Email: [email protected]
Profile: http://www.google.com/profiles/julian.calaby/
PiBGcm9tOiBKdWxpYW4gQ2FsYWJ5IFttYWlsdG86anVsaWFuLmNhbGFieUBnbWFpbC5jb21dDQo+
IFNlbnQ6IFRodXJzZGF5LCBKdW5lIDAyLCAyMDE2IDQ6NDQgQU0NCj4gVG86IEphdmllciBNYXJ0
aW5leiBDYW5pbGxhczsgWGlubWluZyBIdQ0KPiBDYzogbGludXgta2VybmVsQHZnZXIua2VybmVs
Lm9yZzsgQW1pdGt1bWFyIEthcndhcjsgS2FsbGUgVmFsbzsgbmV0ZGV2Ow0KPiBsaW51eC13aXJl
bGVzczsgTmlzaGFudCBTYXJtdWthZGFtDQo+IFN1YmplY3Q6IFJlOiBbUEFUQ0ggNy84XSBtd2lm
aWV4OiBkb24ndCBwcmludCBhbiBlcnJvciBpZiBhbiBvcHRpb25hbCBEVA0KPiBwcm9wZXJ0eSBp
cyBtaXNzaW5nDQo+IA0KPiBIaSBKYXZpZXIsDQo+IA0KPiBPbiBXZWQsIEp1biAxLCAyMDE2IGF0
IDExOjUxIFBNLCBKYXZpZXIgTWFydGluZXogQ2FuaWxsYXMNCj4gPGphdmllckBvc2cuc2Ftc3Vu
Zy5jb20+IHdyb3RlOg0KPiA+IEhlbGxvIEp1bGlhbiwNCj4gPg0KPiA+IFRoYW5rcyBhIGxvdCBm
b3IgeW91ciBmZWVkYmFjayBhbmQgcmV2aWV3cy4NCj4gPg0KPiA+IE9uIDA2LzAxLzIwMTYgMTI6
MjAgQU0sIEp1bGlhbiBDYWxhYnkgd3JvdGU6DQo+ID4+IEhpIEFsbCwNCj4gPj4NCj4gPj4gT24g
U2F0LCBNYXkgMjgsIDIwMTYgYXQgMTI6MTggQU0sIEphdmllciBNYXJ0aW5leiBDYW5pbGxhcw0K
PiA+PiA8amF2aWVyQG9zZy5zYW1zdW5nLmNvbT4gd3JvdGU6DQo+ID4+PiBUaGUNCj4gPj4+IERv
Y3VtZW50YXRpb24vZGV2aWNldHJlZS9iaW5kaW5ncy9uZXQvd2lyZWxlc3MvbWFydmVsbC1zZDh4
eHgudHh0IERUDQo+ID4+PiBiaW5kaW5nIGRvY3VtZW50IHNheSB0aGF0IHRoZSAiaW50ZXJydXB0
cyIgcHJvcGVydHkgaW4gdGhlIGNoaWxkDQo+IG5vZGUgaXMgb3B0aW9uYWwuIFNvIHRoZSBwcm9w
ZXJ0eSBiZWluZyBtaXNzZWQgc2hvdWxkbid0IGJlIHRyZWF0ZWQgYXMNCj4gYW4gZXJyb3IuDQo+
ID4+DQo+ID4+IEhhdmUgeW91IGNoZWNrZWQgd2hldGhlciBpdCBpcyB0cnVseSBvcHRpb25hbD8g
SS5lLiBub3RoaW5nIGVsc2UNCj4gPj4gYnJlYWtzIGlmIHRoaXMgcHJvcGVydHkgaXNuJ3Qgc2V0
Pw0KPiA+Pg0KPiA+DQo+ID4gVGhhdCdzIHdoYXQgdGhlIERUIGJpbmRpbmcgc2F5cyBhbmQgdGhl
IElSUSBpcyBvbmx5IHVzZWQgYXMgYSB3YWtldXANCj4gPiBzb3VyY2UgZHVyaW5nIHN5c3RlbSBz
dXNwZW5kLCBpdCBpcyBub3QgdXNlZCBkdXJpbmcgcnVudGltZS4gQW5kIHRoYXQNCj4gPiBpcyB3
aHkgdGhlDQo+ID4gbXdpZmlleF9zZGlvX3Byb2JlX29mKCkgZnVuY3Rpb24gZG9lcyBub3QgZmFp
bCBpZiB0aGUgSVJRIGlzIG1pc3NpbmcuDQo+IA0KPiBBd2Vzb21lLCB0aGF0J3Mgd2hhdCBJIHdh
bnRlZCB0byBrbm93Lg0KPiANCj4gPiBOb3csIEkganVzdCBnb3QgdG8gdGhhdCBjb25jbHVzaW9u
IGJ5IHJlYWRpbmcgdGhlIGJpbmRpbmcgZG9jcywgdGhlDQo+ID4gbWVzc2FnZSBpbiB0aGUgY29t
bWl0cyB0aGF0IGludHJvZHVjZWQgdGhpcyBhbmQgdGhlIGRyaXZlciBjb2RlLg0KPiA+IFhpbm1p
bmcgSHUgc2hvdWxkIGNvbW1lbnQgb24gaG93IGNyaXRpY2FsIHRoaXMgZmVhdHVyZSBpcyBmb3Ig
c3lzdGVtcw0KPiB0aGF0IG5lZWRzIHRvIGJlIHdha2V1cC4NCj4gDQo+IFhpbm1pbmcsIGNvdWxk
IHlvdSByZXZpZXcgdGhpcyBhbHNvPw0KPiANCg0KWWVzLiBJUlEgaXMgdGhlIG9wdGlvbmFsIHBh
cmFtZXRlci4gU3lzdGVtIGhhcyBhIGZsZXhpYmlsaXR5IHRvIG5vdCB1c2UgaXQsIGJ1dCBpdCBz
dGlsbCBjYW4gY29uZmlndXJlIG90aGVyIGRldmljZSB0cmVlIHBhcmFtZXRlcnMuIFRoZSBwYXRj
aCBsb29rcyBnb29kLg0KDQpSZWdhcmRzLA0KQW1pdGt1bWFyDQo=
Hi All,
On Sat, May 28, 2016 at 12:18 AM, Javier Martinez Canillas
<[email protected]> 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 <[email protected]>
This looks sensible to me.
Reviewed-by: Julian Calaby <[email protected]>
> ---
>
> drivers/net/wireless/marvell/mwifiex/sdio.c | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
Thanks,
--
Julian Calaby
Email: [email protected]
Profile: http://www.google.com/profiles/julian.calaby/
Hi All,
On Sat, May 28, 2016 at 12:18 AM, Javier Martinez Canillas
<[email protected]> 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 <[email protected]>
This looks sensible to me.
Reviewed-by: Julian Calaby <[email protected]>
> drivers/net/wireless/marvell/mwifiex/sdio.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
Thanks,
--
Julian Calaby
Email: [email protected]
Profile: http://www.google.com/profiles/julian.calaby/
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
> <[email protected]> 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?
>
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).
>> Signed-off-by: Javier Martinez Canillas <[email protected]>
>
> Other than that, this looks sensible to me.
>
> Reviewed-by: Julian Calaby <[email protected]>
>
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
<[email protected]> wrote:
> 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
>> <[email protected]> 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?
>>
>
> 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).
>
>>> Signed-off-by: Javier Martinez Canillas <[email protected]>
>>
>> Other than that, this looks sensible to me.
>>
>> Reviewed-by: Julian Calaby <[email protected]>
>>
>
> Best regards,
Thanks,
--
Julian Calaby
Email: [email protected]
Profile: http://www.google.com/profiles/julian.calaby/
Hi All,
On Sat, May 28, 2016 at 12:18 AM, Javier Martinez Canillas
<[email protected]> 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 <[email protected]>
This looks sensible to me.
Reviewed-by: Julian Calaby <[email protected]>
> ---
>
> drivers/net/wireless/marvell/mwifiex/sdio.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Thanks,
--
Julian Calaby
Email: [email protected]
Profile: http://www.google.com/profiles/julian.calaby/
> From: Javier Martinez Canillas [mailto:[email protected]]
> Sent: Friday, May 27, 2016 7:48 PM
> To: [email protected]
> Cc: Xinming Hu; Javier Martinez Canillas; Amitkumar Karwar; Kalle Valo;
> [email protected]; [email protected]; 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 <[email protected]>
Regards,
Amitkumar
Hi All,
On Sat, May 28, 2016 at 12:18 AM, Javier Martinez Canillas
<[email protected]> 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 <[email protected]>
This looks sensible to me.
Reviewed-by: Julian Calaby <[email protected]>
> ---
>
> drivers/net/wireless/marvell/mwifiex/sdio.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
Thanks,
--
Julian Calaby
Email: [email protected]
Profile: http://www.google.com/profiles/julian.calaby/
Hi All,
On Sat, May 28, 2016 at 12:18 AM, Javier Martinez Canillas
<[email protected]> wrote:
> It's better to have the device name prefixed in the error message.
>
> Signed-off-by: Javier Martinez Canillas <[email protected]>
This looks right to me.
Reviewed-by: Julian Calaby <[email protected]>
> ---
>
> drivers/net/wireless/marvell/mwifiex/sdio.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
Thanks,
--
Julian Calaby
Email: [email protected]
Profile: http://www.google.com/profiles/julian.calaby/
Javier Martinez Canillas <[email protected]> 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 <[email protected]>
> Reviewed-by: Julian Calaby <[email protected]>
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/
Hi All,
On Sat, May 28, 2016 at 12:18 AM, Javier Martinez Canillas
<[email protected]> 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 <[email protected]>
Other than that, this looks sensible to me.
Reviewed-by: Julian Calaby <[email protected]>
> ---
>
> drivers/net/wireless/marvell/mwifiex/sdio.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Thanks,
--
Julian Calaby
Email: [email protected]
Profile: http://www.google.com/profiles/julian.calaby/