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

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

[Javier Martinez Canillas]

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

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

[Javier Martinez Canillas]

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

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

[Julian Calaby]

Hi All,

On Sat, May 28, 2016 at 12:18 AM, Javier Martinez Canillas
<javier@osg.samsung.com> 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 <javier@osg.samsung.com>

This looks sensible to me.

Reviewed-by: Julian Calaby <julian.calaby@gmail.com>

>  drivers/net/wireless/marvell/mwifiex/sdio.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)

Thanks,

-- 
Julian Calaby

Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/

Re: [1/8] mwifiex: only call mwifiex_sdio_probe_of() if dev has an OF node

[Kalle Valo]

Javier Martinez Canillas <javier@osg.samsung.com> 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 <javier@osg.samsung.com>
> Reviewed-by: Julian Calaby <julian.calaby@gmail.com>

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/

[PATCH 2/8] mwifiex: propagate sdio_enable_func() errno code in mwifiex_sdio_probe()

[Javier Martinez Canillas]

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 <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 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

Re: [PATCH 2/8] mwifiex: propagate sdio_enable_func() errno code in mwifiex_sdio_probe()

[Julian Calaby]

Hi All,

On Sat, May 28, 2016 at 12:18 AM, Javier Martinez Canillas
<javier@osg.samsung.com> 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 <javier@osg.samsung.com>

This looks sensible to me.

Reviewed-by: Julian Calaby <julian.calaby@gmail.com>

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

Thanks,

-- 
Julian Calaby

Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/

[PATCH 3/8] mwifiex: propagate mwifiex_add_card() errno code in mwifiex_sdio_probe()

[Javier Martinez Canillas]

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

 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

Re: [PATCH 3/8] mwifiex: propagate mwifiex_add_card() errno code in mwifiex_sdio_probe()

[Julian Calaby]

Hi All,

On Sat, May 28, 2016 at 12:18 AM, Javier Martinez Canillas
<javier@osg.samsung.com> 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 <javier@osg.samsung.com>

This looks sensible to me.

Reviewed-by: Julian Calaby <julian.calaby@gmail.com>

> ---
>
>  drivers/net/wireless/marvell/mwifiex/sdio.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)

Thanks,

-- 
Julian Calaby

Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/

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

[Javier Martinez Canillas]

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

Re: [PATCH 4/8] mwifiex: consolidate mwifiex_sdio_probe() error paths

[Julian Calaby]

Hi All,

On Sat, May 28, 2016 at 12:18 AM, Javier Martinez Canillas
<javier@osg.samsung.com> 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 <javier@osg.samsung.com>

This looks sensible to me.

Reviewed-by: Julian Calaby <julian.calaby@gmail.com>

> ---
>
>  drivers/net/wireless/marvell/mwifiex/sdio.c | 17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)

Thanks,

-- 
Julian Calaby

Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/

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

[Javier Martinez Canillas]

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

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

[Julian Calaby]

Hi All,

On Sat, May 28, 2016 at 12:18 AM, Javier Martinez Canillas
<javier@osg.samsung.com> wrote:
> It's better to have the device name prefixed in the error message.
>
> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>

This looks right to me.

Reviewed-by: Julian Calaby <julian.calaby@gmail.com>

> ---
>
>  drivers/net/wireless/marvell/mwifiex/sdio.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Thanks,

-- 
Julian Calaby

Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/

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

[Javier Martinez Canillas]

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

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

[Julian Calaby]

Hi All,

On Sat, May 28, 2016 at 12:18 AM, Javier Martinez Canillas
<javier@osg.samsung.com> 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 <javier@osg.samsung.com>

This looks sensible to me.

Reviewed-by: Julian Calaby <julian.calaby@gmail.com>

> ---
>
>  drivers/net/wireless/marvell/mwifiex/sdio.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)

Thanks,

-- 
Julian Calaby

Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/

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

[Javier Martinez Canillas]

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

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

[Julian Calaby]

Hi All,

On Sat, May 28, 2016 at 12:18 AM, Javier Martinez Canillas
<javier@osg.samsung.com> 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 <javier@osg.samsung.com>

Other than that, this looks sensible to me.

Reviewed-by: Julian Calaby <julian.calaby@gmail.com>

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

Thanks,

-- 
Julian Calaby

Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/

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

[Javier Martinez Canillas]

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
> <javier@osg.samsung.com> 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 <javier@osg.samsung.com>
> 
> Other than that, this looks sensible to me.
> 
> Reviewed-by: Julian Calaby <julian.calaby@gmail.com>
> 

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

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

[Julian Calaby]

Hi Javier,

On Wed, Jun 1, 2016 at 11:51 PM, Javier Martinez Canillas
<javier@osg.samsung.com> 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
>> <javier@osg.samsung.com> 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 <javier@osg.samsung.com>
>>
>> Other than that, this looks sensible to me.
>>
>> Reviewed-by: Julian Calaby <julian.calaby@gmail.com>
>>
>
> Best regards,

Thanks,

-- 
Julian Calaby

Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/

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

[Amitkumar Karwar]

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

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

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

[Javier Martinez Canillas]

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

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

[Julian Calaby]

Hi All,

On Sat, May 28, 2016 at 12:18 AM, Javier Martinez Canillas
<javier@osg.samsung.com> 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 <javier@osg.samsung.com>

This looks sensible to me.

Reviewed-by: Julian Calaby <julian.calaby@gmail.com>

>
> ---
>
>  drivers/net/wireless/marvell/mwifiex/sdio.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Thanks,

-- 
Julian Calaby

Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/

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

[Enric Balletbo Serra]

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
>

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

[Amitkumar Karwar]

> 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 <akarwar@marvell.com>

Regards,
Amitkumar