Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1408101 > unrolled thread

[PATCH 0/8] mwifiex: Fix some error handling issues in mwifiex_sdio_probe() function

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/8] mwifiex: Fix some error handling issues in mwifiex_sdio_probe() function Javier Martinez Canillas <javier@osg.samsung.com> - 2016-05-27 16:20 +0200
    [PATCH 6/8] mwifiex: check if mwifiex_sdio_probe_of() fails and return error Javier Martinez Canillas <javier@osg.samsung.com> - 2016-05-27 16:20 +0200
    [PATCH 4/8] mwifiex: consolidate mwifiex_sdio_probe() error paths Javier Martinez Canillas <javier@osg.samsung.com> - 2016-05-27 16:20 +0200
    [PATCH 1/8] mwifiex: only call mwifiex_sdio_probe_of() if dev has an OF node Javier Martinez Canillas <javier@osg.samsung.com> - 2016-05-27 16:20 +0200
    [PATCH 8/8] mwifiex: use better message and error code when OF node doesn't match Javier Martinez Canillas <javier@osg.samsung.com> - 2016-05-27 16:20 +0200
    [PATCH 5/8] mwifiex: use dev_err() instead of pr_err() in mwifiex_sdio_probe() Javier Martinez Canillas <javier@osg.samsung.com> - 2016-05-27 16:20 +0200
    [PATCH 7/8] mwifiex: don't print an error if an optional DT property is missing Javier Martinez Canillas <javier@osg.samsung.com> - 2016-05-27 16:30 +0200
    Re: [PATCH 0/8] mwifiex: Fix some error handling issues in  mwifiex_sdio_probe() function Enric Balletbo Serra <eballetbo@gmail.com> - 2016-05-30 12:00 +0200

#1408101 — [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(-)

-- 
2.5.5

[toc] | [next] | [standalone]

#1408102 — [PATCH 6/8] mwifiex: check if mwifiex_sdio_probe_of() fails and return error

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 <javier@osg.samsung.com>
---

 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

[toc] | [prev] | [next] | [standalone]

#1408105 — [PATCH 4/8] mwifiex: consolidate mwifiex_sdio_probe() error paths

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 <javier@osg.samsung.com>
---

 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

[toc] | [prev] | [next] | [standalone]

#1408106 — [PATCH 1/8] mwifiex: only call mwifiex_sdio_probe_of() if dev has an OF node

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 <javier@osg.samsung.com>
---

 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

[toc] | [prev] | [next] | [standalone]

#1408108 — [PATCH 8/8] mwifiex: use better message and error code when OF node doesn't match

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 <javier@osg.samsung.com>

---

 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

[toc] | [prev] | [next] | [standalone]

#1408109 — [PATCH 5/8] mwifiex: use dev_err() instead of pr_err() in mwifiex_sdio_probe()

It's better to have the device name prefixed in the error message.

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

 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

[toc] | [prev] | [next] | [standalone]

#1408113 — [PATCH 7/8] mwifiex: don't print an error if an optional DT property is missing

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 <javier@osg.samsung.com>
---

 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

[toc] | [prev] | [next] | [standalone]

#1408962 — Re: [PATCH 0/8] mwifiex: Fix some error handling issues in mwifiex_sdio_probe() function

Hi Javier,

2016-05-27 16:18 GMT+02:00 Javier Martinez Canillas <javier@osg.samsung.com>:
> 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 <enric.balletbo@collabora.com>

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
>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web