[PATCH v3] net: fec: Do a sanity check on the gpio number

[PATCH v3] net: fec: Do a sanity check on the gpio number

[Fabio Estevam]

From: Fabio Estevam <fabio.estevam@freescale.com>

Since commit 372e722ea4d (gpiolib: use descriptors internally) the following 
warning is seen on a mx28evk board:

[    5.116291] ------------[ cut here ]------------
[    5.121306] WARNING: at drivers/gpio/gpiolib.c:125 gpio_to_desc+0x30/0x44()
[    5.128491] invalid GPIO -2
[    5.131563] Modules linked in:
[    5.134846] [<c0014e20>] (unwind_backtrace+0x0/0xf4) from [<c001d428>] (warn_slowpath_common+0x4c/0x68)
[    5.144682] [<c001d428>] (warn_slowpath_common+0x4c/0x68) from [<c001d4d8>] (warn_slowpath_fmt+0x30/0x40)
[    5.154693] [<c001d4d8>] (warn_slowpath_fmt+0x30/0x40) from [<c0283434>] (gpio_to_desc+0x30/0x44)
[    5.164002] [<c0283434>] (gpio_to_desc+0x30/0x44) from [<c0285470>] (gpio_request_one+0x10/0xe8)
[    5.173294] [<c0285470>] (gpio_request_one+0x10/0xe8) from [<c0282f50>] (devm_gpio_request_one+0x40/0x74)
[    5.183332] [<c0282f50>] (devm_gpio_request_one+0x40/0x74) from [<c0319be0>] (fec_probe+0x2d0/0x99c)
[    5.192923] [<c0319be0>] (fec_probe+0x2d0/0x99c) from [<c02c8114>] (platform_drv_probe+0x14/0x18)
[    5.202228] [<c02c8114>] (platform_drv_probe+0x14/0x18) from [<c02c6e00>] (driver_probe_device+0x90/0x224)
[    5.212332] [<c02c6e00>] (driver_probe_device+0x90/0x224) from [<c02c7028>] (__driver_attach+0x94/0x98)
[    5.222162] [<c02c7028>] (__driver_attach+0x94/0x98) from [<c02c5750>] (bus_for_each_dev+0x78/0x98)
[    5.231642] [<c02c5750>] (bus_for_each_dev+0x78/0x98) from [<c02c5fd0>] (bus_add_driver+0x1a4/0x240)
[    5.241207] [<c02c5fd0>] (bus_add_driver+0x1a4/0x240) from [<c02c7608>] (driver_register+0x78/0x140)
[    5.250768] [<c02c7608>] (driver_register+0x78/0x140) from [<c00087a4>] (do_one_initcall+0x30/0x17c)
[    5.260347] [<c00087a4>] (do_one_initcall+0x30/0x17c) from [<c05fa29c>] (kernel_init_freeable+0xe8/0x1b0)
[    5.270381] [<c05fa29c>] (kernel_init_freeable+0xe8/0x1b0) from [<c044dfd4>] (kernel_init+0x8/0xe4)
[    5.279886] [<c044dfd4>] (kernel_init+0x8/0xe4) from [<c000f248>] (ret_from_fork+0x14/0x2c)
[    5.288740] ---[ end trace c15c72a22979d58d ]--- 

mx28evk has two ethernet controllers. The GPIO that performs the 
ethernet reset on both ports is the same GPIO, so on the board dts file, only in
one ethernet instance is passed the GPIO reset property.

Validate the gpio number prior to requesting it in order to avoid such warning.

With such gpio validation in place, it is also safe to change from pr_debug to 
dev_err in the case the gpio request fails.

Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
---
Changes since v2:
- Merge the two patches into a single one, and use dev_err
Changes since v1:
- Added Shawn's ack
 drivers/net/ethernet/freescale/fec.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/freescale/fec.c b/drivers/net/ethernet/freescale/fec.c
index 0fe68c4..1a2071b 100644
--- a/drivers/net/ethernet/freescale/fec.c
+++ b/drivers/net/ethernet/freescale/fec.c
@@ -1689,10 +1689,13 @@ static void fec_reset_phy(struct platform_device *pdev)
 		msec = 1;
 
 	phy_reset = of_get_named_gpio(np, "phy-reset-gpios", 0);
+	if (!gpio_is_valid(phy_reset))
+		return;
+
 	err = devm_gpio_request_one(&pdev->dev, phy_reset,
 				    GPIOF_OUT_INIT_LOW, "phy-reset");
 	if (err) {
-		pr_debug("FEC: failed to get gpio phy-reset: %d\n", err);
+		dev_err(&pdev->dev, "failed to get phy-reset-gpios: %d\n", err);
 		return;
 	}
 	msleep(msec);
-- 
1.7.9.5

Re: [PATCH v3] net: fec: Do a sanity check on the gpio number

[Shawn Guo]

On Sun, Feb 17, 2013 at 12:29:24PM -0300, Fabio Estevam wrote:
> From: Fabio Estevam <fabio.estevam@freescale.com>
> 
> Since commit 372e722ea4d (gpiolib: use descriptors internally) the following 
> warning is seen on a mx28evk board:
> 
> [    5.116291] ------------[ cut here ]------------
> [    5.121306] WARNING: at drivers/gpio/gpiolib.c:125 gpio_to_desc+0x30/0x44()
> [    5.128491] invalid GPIO -2
> [    5.131563] Modules linked in:
> [    5.134846] [<c0014e20>] (unwind_backtrace+0x0/0xf4) from [<c001d428>] (warn_slowpath_common+0x4c/0x68)
> [    5.144682] [<c001d428>] (warn_slowpath_common+0x4c/0x68) from [<c001d4d8>] (warn_slowpath_fmt+0x30/0x40)
> [    5.154693] [<c001d4d8>] (warn_slowpath_fmt+0x30/0x40) from [<c0283434>] (gpio_to_desc+0x30/0x44)
> [    5.164002] [<c0283434>] (gpio_to_desc+0x30/0x44) from [<c0285470>] (gpio_request_one+0x10/0xe8)
> [    5.173294] [<c0285470>] (gpio_request_one+0x10/0xe8) from [<c0282f50>] (devm_gpio_request_one+0x40/0x74)
> [    5.183332] [<c0282f50>] (devm_gpio_request_one+0x40/0x74) from [<c0319be0>] (fec_probe+0x2d0/0x99c)
> [    5.192923] [<c0319be0>] (fec_probe+0x2d0/0x99c) from [<c02c8114>] (platform_drv_probe+0x14/0x18)
> [    5.202228] [<c02c8114>] (platform_drv_probe+0x14/0x18) from [<c02c6e00>] (driver_probe_device+0x90/0x224)
> [    5.212332] [<c02c6e00>] (driver_probe_device+0x90/0x224) from [<c02c7028>] (__driver_attach+0x94/0x98)
> [    5.222162] [<c02c7028>] (__driver_attach+0x94/0x98) from [<c02c5750>] (bus_for_each_dev+0x78/0x98)
> [    5.231642] [<c02c5750>] (bus_for_each_dev+0x78/0x98) from [<c02c5fd0>] (bus_add_driver+0x1a4/0x240)
> [    5.241207] [<c02c5fd0>] (bus_add_driver+0x1a4/0x240) from [<c02c7608>] (driver_register+0x78/0x140)
> [    5.250768] [<c02c7608>] (driver_register+0x78/0x140) from [<c00087a4>] (do_one_initcall+0x30/0x17c)
> [    5.260347] [<c00087a4>] (do_one_initcall+0x30/0x17c) from [<c05fa29c>] (kernel_init_freeable+0xe8/0x1b0)
> [    5.270381] [<c05fa29c>] (kernel_init_freeable+0xe8/0x1b0) from [<c044dfd4>] (kernel_init+0x8/0xe4)
> [    5.279886] [<c044dfd4>] (kernel_init+0x8/0xe4) from [<c000f248>] (ret_from_fork+0x14/0x2c)
> [    5.288740] ---[ end trace c15c72a22979d58d ]--- 
> 
> mx28evk has two ethernet controllers. The GPIO that performs the 
> ethernet reset on both ports is the same GPIO, so on the board dts file, only in
> one ethernet instance is passed the GPIO reset property.
> 
> Validate the gpio number prior to requesting it in order to avoid such warning.
> 
> With such gpio validation in place, it is also safe to change from pr_debug to 
> dev_err in the case the gpio request fails.
> 
> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>

Reviewed-by: Shawn Guo <shawn.guo@linaro.org>

Re: [PATCH v3] net: fec: Do a sanity check on the gpio number

[David Miller]

From: Fabio Estevam <festevam@gmail.com>
Date: Sun, 17 Feb 2013 12:29:24 -0300

> Since commit 372e722ea4d (gpiolib: use descriptors internally) the following 
> warning is seen on a mx28evk board:

[davem@drr linux]$ git describe 372e722ea4d
fatal: Not a valid object name 372e722ea4d
[davem@drr linux]$ 

That commit ID doesn't exist in any tree I have access to.

I'm not applying this.

Re: [PATCH v3] net: fec: Do a sanity check on the gpio number

[Fabio Estevam]

On Mon, Feb 18, 2013 at 4:47 PM, David Miller <davem@davemloft.net> wrote:
> From: Fabio Estevam <festevam@gmail.com>
> Date: Sun, 17 Feb 2013 12:29:24 -0300
>
>> Since commit 372e722ea4d (gpiolib: use descriptors internally) the following
>> warning is seen on a mx28evk board:
>
> [davem@drr linux]$ git describe 372e722ea4d
> fatal: Not a valid object name 372e722ea4d
> [davem@drr linux]$
>
> That commit ID doesn't exist in any tree I have access to.

It exists in linux-next.

Anyway, even without this commit the gpio validation should be done.

> I'm not applying this.

Ok, would you apply it if I remove the mention to 372e722ea4d from the
commit log?

Re: [PATCH v3] net: fec: Do a sanity check on the gpio number

[David Miller]

From: Fabio Estevam <festevam@gmail.com>
Date: Mon, 18 Feb 2013 16:51:49 -0300

> On Mon, Feb 18, 2013 at 4:47 PM, David Miller <davem@davemloft.net> wrote:
>> From: Fabio Estevam <festevam@gmail.com>
>> Date: Sun, 17 Feb 2013 12:29:24 -0300
>>
>>> Since commit 372e722ea4d (gpiolib: use descriptors internally) the following
>>> warning is seen on a mx28evk board:
>>
>> [davem@drr linux]$ git describe 372e722ea4d
>> fatal: Not a valid object name 372e722ea4d
>> [davem@drr linux]$
>>
>> That commit ID doesn't exist in any tree I have access to.
> 
> It exists in linux-next.

linux-next commits are not valid to reference if they are not
either in the target tree where your patch belongs or Linus's
tree.

And if the change that causes the change to be needed doesn't even
exist in my tree or Linus's upstream, your "fix" has no business in my
tree either.