[PATCH] net: phy: mdio-gpio: fix access that may sleep

[PATCH] net: phy: mdio-gpio: fix access that may sleep

[Martin Schiller]

This commit re-enables support for slow GPIO pins. It was initially
introduced by commit
	2d6c9091ab7630dfcf34417c6683ce4764d7d40a
and got lost by commit
	7e5fbd1e0700f1bdb94508f84ec2aeb01eed7b12

Also add a warning about slow GPIO pins like it is done in i2c-gpio.

Signed-off-by: Martin Schiller <ms@dev.tdt.de>
---
 drivers/net/phy/mdio-gpio.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/net/phy/mdio-gpio.c b/drivers/net/phy/mdio-gpio.c
index 33265747bf39..d4430631ca6a 100644
--- a/drivers/net/phy/mdio-gpio.c
+++ b/drivers/net/phy/mdio-gpio.c
@@ -63,7 +63,7 @@ static void mdio_dir(struct mdiobb_ctrl *ctrl, int dir)
 		 * assume the pin serves as pull-up. If direction is
 		 * output, the default value is high.
 		 */
-		gpiod_set_value(bitbang->mdo, 1);
+		gpiod_set_value_cansleep(bitbang->mdo, 1);
 		return;
 	}
 
@@ -78,7 +78,7 @@ static int mdio_get(struct mdiobb_ctrl *ctrl)
 	struct mdio_gpio_info *bitbang =
 		container_of(ctrl, struct mdio_gpio_info, ctrl);
 
-	return gpiod_get_value(bitbang->mdio);
+	return gpiod_get_value_cansleep(bitbang->mdio);
 }
 
 static void mdio_set(struct mdiobb_ctrl *ctrl, int what)
@@ -87,9 +87,9 @@ static void mdio_set(struct mdiobb_ctrl *ctrl, int what)
 		container_of(ctrl, struct mdio_gpio_info, ctrl);
 
 	if (bitbang->mdo)
-		gpiod_set_value(bitbang->mdo, what);
+		gpiod_set_value_cansleep(bitbang->mdo, what);
 	else
-		gpiod_set_value(bitbang->mdio, what);
+		gpiod_set_value_cansleep(bitbang->mdio, what);
 }
 
 static void mdc_set(struct mdiobb_ctrl *ctrl, int what)
@@ -97,7 +97,7 @@ static void mdc_set(struct mdiobb_ctrl *ctrl, int what)
 	struct mdio_gpio_info *bitbang =
 		container_of(ctrl, struct mdio_gpio_info, ctrl);
 
-	gpiod_set_value(bitbang->mdc, what);
+	gpiod_set_value_cansleep(bitbang->mdc, what);
 }
 
 static const struct mdiobb_ops mdio_gpio_ops = {
@@ -162,6 +162,11 @@ static int mdio_gpio_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	if (gpiod_cansleep(bitbang->mdc) || gpiod_cansleep(bitbang->mdio)
+		|| gpiod_cansleep(bitbang->mdo))
+		dev_warn(&pdev->dev, "Slow GPIO pins might wreak havoc into"
+				     "I2C/SMBus bus timing");
+
 	if (pdev->dev.of_node) {
 		bus_id = of_alias_get_id(pdev->dev.of_node, "mdio-gpio");
 		if (bus_id < 0) {
-- 
2.11.0

[PATCH v2] net: phy: mdio-gpio: fix access that may sleep

[Martin Schiller]

This commit re-enables support for slow GPIO pins. It was initially
introduced by commit
	2d6c9091ab7630dfcf34417c6683ce4764d7d40a
and got lost by commit
	7e5fbd1e0700f1bdb94508f84ec2aeb01eed7b12

Also add a warning about slow GPIO pins like it is done in i2c-gpio.

Signed-off-by: Martin Schiller <ms@dev.tdt.de>
---
v2:
 - fixed copy/paste bug in warning about slow GPIO pins
---
 drivers/net/phy/mdio-gpio.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/net/phy/mdio-gpio.c b/drivers/net/phy/mdio-gpio.c
index 33265747bf39..6c1cca14689b 100644
--- a/drivers/net/phy/mdio-gpio.c
+++ b/drivers/net/phy/mdio-gpio.c
@@ -63,7 +63,7 @@ static void mdio_dir(struct mdiobb_ctrl *ctrl, int dir)
 		 * assume the pin serves as pull-up. If direction is
 		 * output, the default value is high.
 		 */
-		gpiod_set_value(bitbang->mdo, 1);
+		gpiod_set_value_cansleep(bitbang->mdo, 1);
 		return;
 	}
 
@@ -78,7 +78,7 @@ static int mdio_get(struct mdiobb_ctrl *ctrl)
 	struct mdio_gpio_info *bitbang =
 		container_of(ctrl, struct mdio_gpio_info, ctrl);
 
-	return gpiod_get_value(bitbang->mdio);
+	return gpiod_get_value_cansleep(bitbang->mdio);
 }
 
 static void mdio_set(struct mdiobb_ctrl *ctrl, int what)
@@ -87,9 +87,9 @@ static void mdio_set(struct mdiobb_ctrl *ctrl, int what)
 		container_of(ctrl, struct mdio_gpio_info, ctrl);
 
 	if (bitbang->mdo)
-		gpiod_set_value(bitbang->mdo, what);
+		gpiod_set_value_cansleep(bitbang->mdo, what);
 	else
-		gpiod_set_value(bitbang->mdio, what);
+		gpiod_set_value_cansleep(bitbang->mdio, what);
 }
 
 static void mdc_set(struct mdiobb_ctrl *ctrl, int what)
@@ -97,7 +97,7 @@ static void mdc_set(struct mdiobb_ctrl *ctrl, int what)
 	struct mdio_gpio_info *bitbang =
 		container_of(ctrl, struct mdio_gpio_info, ctrl);
 
-	gpiod_set_value(bitbang->mdc, what);
+	gpiod_set_value_cansleep(bitbang->mdc, what);
 }
 
 static const struct mdiobb_ops mdio_gpio_ops = {
@@ -162,6 +162,11 @@ static int mdio_gpio_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	if (gpiod_cansleep(bitbang->mdc) || gpiod_cansleep(bitbang->mdio)
+		|| gpiod_cansleep(bitbang->mdo))
+		dev_warn(&pdev->dev, "Slow GPIO pins might wreak havoc into"
+				     "MDIO bus timing");
+
 	if (pdev->dev.of_node) {
 		bus_id = of_alias_get_id(pdev->dev.of_node, "mdio-gpio");
 		if (bus_id < 0) {
-- 
2.11.0

Re: [PATCH v2] net: phy: mdio-gpio: fix access that may sleep

[Andrew Lunn]

On Wed, Nov 14, 2018 at 07:37:03AM +0100, Martin Schiller wrote:
> This commit re-enables support for slow GPIO pins. It was initially
> introduced by commit
> 	2d6c9091ab7630dfcf34417c6683ce4764d7d40a
> and got lost by commit
> 	7e5fbd1e0700f1bdb94508f84ec2aeb01eed7b12

Hi Martin

Was it really lost? It looks like _cansleep() just adds an extra check
might_sleep_if(extra_checks), but it does not change any
functionality.

So the change itself is O.K, i'm just not too sure about the commit
message.

	Andrew

Re: [PATCH v2] net: phy: mdio-gpio: fix access that may sleep

[Martin Schiller]

On 2018-11-14 08:05, Andrew Lunn wrote:
> On Wed, Nov 14, 2018 at 07:37:03AM +0100, Martin Schiller wrote:
>> This commit re-enables support for slow GPIO pins. It was initially
>> introduced by commit
>> 	2d6c9091ab7630dfcf34417c6683ce4764d7d40a
>> and got lost by commit
>> 	7e5fbd1e0700f1bdb94508f84ec2aeb01eed7b12
> 
> Hi Martin
> 
> Was it really lost? It looks like _cansleep() just adds an extra check
> might_sleep_if(extra_checks), but it does not change any
> functionality.

Well, you are right, the functionality itself is not broken, but using
the NON _cansleep() functions on GPIOs that have the cansleep flag set,
this leads to a lot of kernel warnings/backtraces which makes the system
in fact useless.

Thats the WARN_ON() here:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpio/gpiolib.c?h=v4.20-rc2#n2992

and here:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpio/gpiolib.c?h=v4.20-rc2#n3304

> 
> So the change itself is O.K, i'm just not too sure about the commit
> message.
> 
> 	Andrew

Hmm, ok. What would you suggest for a better commit message?

I thought it would be helpful to know that this was already in there
and got (inadvertently?) removed by another commit.

Martin

Re: [PATCH v2] net: phy: mdio-gpio: fix access that may sleep

[Andrew Lunn]

On Wed, Nov 14, 2018 at 08:43:49AM +0100, Martin Schiller wrote:
> On 2018-11-14 08:05, Andrew Lunn wrote:
> >On Wed, Nov 14, 2018 at 07:37:03AM +0100, Martin Schiller wrote:
> >>This commit re-enables support for slow GPIO pins. It was initially
> >>introduced by commit
> >>	2d6c9091ab7630dfcf34417c6683ce4764d7d40a
> >>and got lost by commit
> >>	7e5fbd1e0700f1bdb94508f84ec2aeb01eed7b12
> >
> >Hi Martin
> >
> >Was it really lost? It looks like _cansleep() just adds an extra check
> >might_sleep_if(extra_checks), but it does not change any
> >functionality.
> 
> Well, you are right, the functionality itself is not broken, but using
> the NON _cansleep() functions on GPIOs that have the cansleep flag set,
> this leads to a lot of kernel warnings/backtraces which makes the system
> in fact useless.
> 
> Thats the WARN_ON() here:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpio/gpiolib.c?h=v4.20-rc2#n2992
> 
> and here:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpio/gpiolib.c?h=v4.20-rc2#n3304

Hi Martin

Right, this is really what you are fixing. So the commit message
should talk about these WARN_ON being hit.

> >
> >So the change itself is O.K, i'm just not too sure about the commit
> >message.
> >
> >	Andrew
> 
> Hmm, ok. What would you suggest for a better commit message?

Up until commit XXX, the _cansleep variants of the gpio_ API was
used. After that commit and the change to gpiod_ API, the _cansleep()
was dropped. This then results in WARN_ON() when used with GPIO
devices which do sleep. Add back the _cansleep() to avoid this.

This is something which is useful in stable. So please base this fix
on DaveM net tree, not net-next, and include a Fixes: tag for the
patch which broke it.

      Thanks
	Andrew

Re: [PATCH v2] net: phy: mdio-gpio: fix access that may sleep

[Sergei Shtylyov]

Hello!

On 14.11.2018 9:37, Martin Schiller wrote:

> This commit re-enables support for slow GPIO pins. It was initially
> introduced by commit
> 	2d6c9091ab7630dfcf34417c6683ce4764d7d40a
> and got lost by commit
> 	7e5fbd1e0700f1bdb94508f84ec2aeb01eed7b12

    This is not how you cite a commit: 12 digits is enough and they should
be followed by the commit summary enclosed in ("").

> 
> Also add a warning about slow GPIO pins like it is done in i2c-gpio.
> 
> Signed-off-by: Martin Schiller <ms@dev.tdt.de>
> ---
> v2:
>   - fixed copy/paste bug in warning about slow GPIO pins
> ---
>   drivers/net/phy/mdio-gpio.c | 15 ++++++++++-----
>   1 file changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/phy/mdio-gpio.c b/drivers/net/phy/mdio-gpio.c
> index 33265747bf39..6c1cca14689b 100644
> --- a/drivers/net/phy/mdio-gpio.c
> +++ b/drivers/net/phy/mdio-gpio.c
[...]
> @@ -162,6 +162,11 @@ static int mdio_gpio_probe(struct platform_device *pdev)
>   	if (ret)
>   		return ret;
>   
> +	if (gpiod_cansleep(bitbang->mdc) || gpiod_cansleep(bitbang->mdio)
> +		|| gpiod_cansleep(bitbang->mdo))

    The line continued that way blends with the branch below, the networking
code uses a different style of line continuation where the continuation line
starts immediately below the 1st gpiod_cansleep() call. Also, || should be
left on the 1st line...

> +		dev_warn(&pdev->dev, "Slow GPIO pins might wreak havoc into"
> +				     "MDIO bus timing");
> +
>   	if (pdev->dev.of_node) {
>   		bus_id = of_alias_get_id(pdev->dev.of_node, "mdio-gpio");
>   		if (bus_id < 0) {

MBR, Sergei

Re: [PATCH v2] net: phy: mdio-gpio: fix access that may sleep

[David Miller]

From: Martin Schiller <ms@dev.tdt.de>
Date: Wed, 14 Nov 2018 07:37:03 +0100

> @@ -162,6 +162,11 @@ static int mdio_gpio_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> +	if (gpiod_cansleep(bitbang->mdc) || gpiod_cansleep(bitbang->mdio)
> +		|| gpiod_cansleep(bitbang->mdo))

Please fix the formatting here, operators like "||" and "&&" never begin
a line, they must always finish a line.

[PATCH v3] net: phy: mdio-gpio: Fix working over slow can_sleep GPIOs

[Martin Schiller]

This commit re-enables support for slow GPIO pins. It was initially
introduced by commit 2d6c9091ab76 ("net: mdio-gpio: support access that
may sleep") and got lost by commit 7e5fbd1e0700 ("net: mdio-gpio:
Convert to use gpiod functions where possible").

Also add a warning about slow GPIO pins like it is done in i2c-gpio.

Signed-off-by: Martin Schiller <ms@dev.tdt.de>
---
v3:
 - modified commit summary
 - fixed commit cites in commit message
 - fixed line continuation
 
v2:
 - fixed copy/paste bug in warning about slow GPIO pins
---
 drivers/net/phy/mdio-gpio.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/net/phy/mdio-gpio.c b/drivers/net/phy/mdio-gpio.c
index 33265747bf39..e1c305089172 100644
--- a/drivers/net/phy/mdio-gpio.c
+++ b/drivers/net/phy/mdio-gpio.c
@@ -63,7 +63,7 @@ static void mdio_dir(struct mdiobb_ctrl *ctrl, int dir)
 		 * assume the pin serves as pull-up. If direction is
 		 * output, the default value is high.
 		 */
-		gpiod_set_value(bitbang->mdo, 1);
+		gpiod_set_value_cansleep(bitbang->mdo, 1);
 		return;
 	}
 
@@ -78,7 +78,7 @@ static int mdio_get(struct mdiobb_ctrl *ctrl)
 	struct mdio_gpio_info *bitbang =
 		container_of(ctrl, struct mdio_gpio_info, ctrl);
 
-	return gpiod_get_value(bitbang->mdio);
+	return gpiod_get_value_cansleep(bitbang->mdio);
 }
 
 static void mdio_set(struct mdiobb_ctrl *ctrl, int what)
@@ -87,9 +87,9 @@ static void mdio_set(struct mdiobb_ctrl *ctrl, int what)
 		container_of(ctrl, struct mdio_gpio_info, ctrl);
 
 	if (bitbang->mdo)
-		gpiod_set_value(bitbang->mdo, what);
+		gpiod_set_value_cansleep(bitbang->mdo, what);
 	else
-		gpiod_set_value(bitbang->mdio, what);
+		gpiod_set_value_cansleep(bitbang->mdio, what);
 }
 
 static void mdc_set(struct mdiobb_ctrl *ctrl, int what)
@@ -97,7 +97,7 @@ static void mdc_set(struct mdiobb_ctrl *ctrl, int what)
 	struct mdio_gpio_info *bitbang =
 		container_of(ctrl, struct mdio_gpio_info, ctrl);
 
-	gpiod_set_value(bitbang->mdc, what);
+	gpiod_set_value_cansleep(bitbang->mdc, what);
 }
 
 static const struct mdiobb_ops mdio_gpio_ops = {
@@ -162,6 +162,11 @@ static int mdio_gpio_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	if (gpiod_cansleep(bitbang->mdc) || gpiod_cansleep(bitbang->mdio) ||
+	    gpiod_cansleep(bitbang->mdo))
+		dev_warn(&pdev->dev, "Slow GPIO pins might wreak havoc into"
+				     "MDIO bus timing");
+
 	if (pdev->dev.of_node) {
 		bus_id = of_alias_get_id(pdev->dev.of_node, "mdio-gpio");
 		if (bus_id < 0) {
-- 
2.11.0

Re: [PATCH v3] net: phy: mdio-gpio: Fix working over slow can_sleep GPIOs

[Sergei Shtylyov]

On 11/14/2018 01:12 PM, Martin Schiller wrote:

> This commit re-enables support for slow GPIO pins. It was initially
> introduced by commit 2d6c9091ab76 ("net: mdio-gpio: support access that
> may sleep") and got lost by commit 7e5fbd1e0700 ("net: mdio-gpio:
> Convert to use gpiod functions where possible").
> 
> Also add a warning about slow GPIO pins like it is done in i2c-gpio.
> 
> Signed-off-by: Martin Schiller <ms@dev.tdt.de>
> ---
> v3:
>  - modified commit summary
>  - fixed commit cites in commit message
>  - fixed line continuation
>  
> v2:
>  - fixed copy/paste bug in warning about slow GPIO pins
> ---
>  drivers/net/phy/mdio-gpio.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/phy/mdio-gpio.c b/drivers/net/phy/mdio-gpio.c
> index 33265747bf39..e1c305089172 100644
> --- a/drivers/net/phy/mdio-gpio.c
> +++ b/drivers/net/phy/mdio-gpio.c
[...]
> @@ -162,6 +162,11 @@ static int mdio_gpio_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> +	if (gpiod_cansleep(bitbang->mdc) || gpiod_cansleep(bitbang->mdio) ||
> +	    gpiod_cansleep(bitbang->mdo))
> +		dev_warn(&pdev->dev, "Slow GPIO pins might wreak havoc into"
> +				     "MDIO bus timing");

   Oops, missed this in the 1st review: you don't need to break the kernel messages
like that, they may violate the 80-column limit (to facilitate searching)...

[...]

MBR, Sergei

[PATCH v4] net: phy: mdio-gpio: Fix working over slow can_sleep GPIOs

[Martin Schiller]

This commit re-enables support for slow GPIO pins. It was initially
introduced by commit 2d6c9091ab76 ("net: mdio-gpio: support access that
may sleep") and got lost by commit 7e5fbd1e0700 ("net: mdio-gpio:
Convert to use gpiod functions where possible").

Also add a warning about slow GPIO pins like it is done in i2c-gpio.

Signed-off-by: Martin Schiller <ms@dev.tdt.de>
---
v4:
 - remove linewrap of kernel message
 
v3:
 - modified commit summary
 - fixed commit cites in commit message
 - fixed line continuation
 
v2:
 - fixed copy/paste bug in warning about slow GPIO pins
---
 drivers/net/phy/mdio-gpio.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/net/phy/mdio-gpio.c b/drivers/net/phy/mdio-gpio.c
index 33265747bf39..3a5a24daf384 100644
--- a/drivers/net/phy/mdio-gpio.c
+++ b/drivers/net/phy/mdio-gpio.c
@@ -63,7 +63,7 @@ static void mdio_dir(struct mdiobb_ctrl *ctrl, int dir)
 		 * assume the pin serves as pull-up. If direction is
 		 * output, the default value is high.
 		 */
-		gpiod_set_value(bitbang->mdo, 1);
+		gpiod_set_value_cansleep(bitbang->mdo, 1);
 		return;
 	}
 
@@ -78,7 +78,7 @@ static int mdio_get(struct mdiobb_ctrl *ctrl)
 	struct mdio_gpio_info *bitbang =
 		container_of(ctrl, struct mdio_gpio_info, ctrl);
 
-	return gpiod_get_value(bitbang->mdio);
+	return gpiod_get_value_cansleep(bitbang->mdio);
 }
 
 static void mdio_set(struct mdiobb_ctrl *ctrl, int what)
@@ -87,9 +87,9 @@ static void mdio_set(struct mdiobb_ctrl *ctrl, int what)
 		container_of(ctrl, struct mdio_gpio_info, ctrl);
 
 	if (bitbang->mdo)
-		gpiod_set_value(bitbang->mdo, what);
+		gpiod_set_value_cansleep(bitbang->mdo, what);
 	else
-		gpiod_set_value(bitbang->mdio, what);
+		gpiod_set_value_cansleep(bitbang->mdio, what);
 }
 
 static void mdc_set(struct mdiobb_ctrl *ctrl, int what)
@@ -97,7 +97,7 @@ static void mdc_set(struct mdiobb_ctrl *ctrl, int what)
 	struct mdio_gpio_info *bitbang =
 		container_of(ctrl, struct mdio_gpio_info, ctrl);
 
-	gpiod_set_value(bitbang->mdc, what);
+	gpiod_set_value_cansleep(bitbang->mdc, what);
 }
 
 static const struct mdiobb_ops mdio_gpio_ops = {
@@ -162,6 +162,10 @@ static int mdio_gpio_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	if (gpiod_cansleep(bitbang->mdc) || gpiod_cansleep(bitbang->mdio) ||
+	    gpiod_cansleep(bitbang->mdo))
+		dev_warn(&pdev->dev, "Slow GPIO pins might wreak havoc into MDIO bus timing");
+
 	if (pdev->dev.of_node) {
 		bus_id = of_alias_get_id(pdev->dev.of_node, "mdio-gpio");
 		if (bus_id < 0) {
-- 
2.11.0

Re: [PATCH v4] net: phy: mdio-gpio: Fix working over slow can_sleep GPIOs

[David Miller]

From: Martin Schiller <ms@dev.tdt.de>
Date: Wed, 14 Nov 2018 12:54:49 +0100

> This commit re-enables support for slow GPIO pins. It was initially
> introduced by commit 2d6c9091ab76 ("net: mdio-gpio: support access that
> may sleep") and got lost by commit 7e5fbd1e0700 ("net: mdio-gpio:
> Convert to use gpiod functions where possible").
> 
> Also add a warning about slow GPIO pins like it is done in i2c-gpio.
> 
> Signed-off-by: Martin Schiller <ms@dev.tdt.de>

Applied, thanks.

Re: [PATCH v4] net: phy: mdio-gpio: Fix working over slow can_sleep GPIOs

[Andrew Lunn]

On Fri, Nov 16, 2018 at 08:25:47PM -0800, David Miller wrote:
> From: Martin Schiller <ms@dev.tdt.de>
> Date: Wed, 14 Nov 2018 12:54:49 +0100
> 
> > This commit re-enables support for slow GPIO pins. It was initially
> > introduced by commit 2d6c9091ab76 ("net: mdio-gpio: support access that
> > may sleep") and got lost by commit 7e5fbd1e0700 ("net: mdio-gpio:
> > Convert to use gpiod functions where possible").
> > 
> > Also add a warning about slow GPIO pins like it is done in i2c-gpio.
> > 
> > Signed-off-by: Martin Schiller <ms@dev.tdt.de>
> 
> Applied, thanks.

Hi David

We were intending that v6 would get merged, and into net. I guess it
is too late now, but could you also add the v4 you merged into
net. You can find the fixes: tag in v6.

   Thanks
	Andrew

[PATCH v5] net: phy: mdio-gpio: Fix working over slow can_sleep GPIOs

[Martin Schiller]

Up until commit 7e5fbd1e0700 ("net: mdio-gpio: Convert to use gpiod
functions where possible"), the _cansleep variants of the gpio_ API was
used. After that commit and the change to gpiod_ API, the _cansleep()
was dropped. This then results in WARN_ON() when used with GPIO
devices which do sleep. Add back the _cansleep() to avoid this.

Fixes: 7e5fbd1e0700 ("net: mdio-gpio: Convert to use gpiod functions where possible")
Signed-off-by: Martin Schiller <ms@dev.tdt.de>
---
v5:
 - reworked commit message
 - added "Fixes:" tag
 - based on DaveM net tree instead of mainline
 
v4:
 - remove linewrap of kernel message
 
v3:
 - modified commit summary
 - fixed commit cites in commit message
 - fixed line continuation
 
v2:
 - fixed copy/paste bug in warning about slow GPIO pins
---
 drivers/net/phy/mdio-gpio.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/net/phy/mdio-gpio.c b/drivers/net/phy/mdio-gpio.c
index 33265747bf39..3a5a24daf384 100644
--- a/drivers/net/phy/mdio-gpio.c
+++ b/drivers/net/phy/mdio-gpio.c
@@ -63,7 +63,7 @@ static void mdio_dir(struct mdiobb_ctrl *ctrl, int dir)
 		 * assume the pin serves as pull-up. If direction is
 		 * output, the default value is high.
 		 */
-		gpiod_set_value(bitbang->mdo, 1);
+		gpiod_set_value_cansleep(bitbang->mdo, 1);
 		return;
 	}
 
@@ -78,7 +78,7 @@ static int mdio_get(struct mdiobb_ctrl *ctrl)
 	struct mdio_gpio_info *bitbang =
 		container_of(ctrl, struct mdio_gpio_info, ctrl);
 
-	return gpiod_get_value(bitbang->mdio);
+	return gpiod_get_value_cansleep(bitbang->mdio);
 }
 
 static void mdio_set(struct mdiobb_ctrl *ctrl, int what)
@@ -87,9 +87,9 @@ static void mdio_set(struct mdiobb_ctrl *ctrl, int what)
 		container_of(ctrl, struct mdio_gpio_info, ctrl);
 
 	if (bitbang->mdo)
-		gpiod_set_value(bitbang->mdo, what);
+		gpiod_set_value_cansleep(bitbang->mdo, what);
 	else
-		gpiod_set_value(bitbang->mdio, what);
+		gpiod_set_value_cansleep(bitbang->mdio, what);
 }
 
 static void mdc_set(struct mdiobb_ctrl *ctrl, int what)
@@ -97,7 +97,7 @@ static void mdc_set(struct mdiobb_ctrl *ctrl, int what)
 	struct mdio_gpio_info *bitbang =
 		container_of(ctrl, struct mdio_gpio_info, ctrl);
 
-	gpiod_set_value(bitbang->mdc, what);
+	gpiod_set_value_cansleep(bitbang->mdc, what);
 }
 
 static const struct mdiobb_ops mdio_gpio_ops = {
@@ -162,6 +162,10 @@ static int mdio_gpio_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	if (gpiod_cansleep(bitbang->mdc) || gpiod_cansleep(bitbang->mdio) ||
+	    gpiod_cansleep(bitbang->mdo))
+		dev_warn(&pdev->dev, "Slow GPIO pins might wreak havoc into MDIO bus timing");
+
 	if (pdev->dev.of_node) {
 		bus_id = of_alias_get_id(pdev->dev.of_node, "mdio-gpio");
 		if (bus_id < 0) {
-- 
2.11.0

Re: [PATCH v5] net: phy: mdio-gpio: Fix working over slow can_sleep GPIOs

[Andrew Lunn]

On Thu, Nov 15, 2018 at 06:24:28AM +0100, Martin Schiller wrote:
> Up until commit 7e5fbd1e0700 ("net: mdio-gpio: Convert to use gpiod
> functions where possible"), the _cansleep variants of the gpio_ API was
> used. After that commit and the change to gpiod_ API, the _cansleep()
> was dropped. This then results in WARN_ON() when used with GPIO
> devices which do sleep. Add back the _cansleep() to avoid this.
> 
> Fixes: 7e5fbd1e0700 ("net: mdio-gpio: Convert to use gpiod functions where possible")
> Signed-off-by: Martin Schiller <ms@dev.tdt.de>
> ---
> v5:
>  - reworked commit message
>  - added "Fixes:" tag
>  - based on DaveM net tree instead of mainline

Hi Martin

Thanks for these changes. We are much closer now.

> @@ -162,6 +162,10 @@ static int mdio_gpio_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> +	if (gpiod_cansleep(bitbang->mdc) || gpiod_cansleep(bitbang->mdio) ||
> +	    gpiod_cansleep(bitbang->mdo))
> +		dev_warn(&pdev->dev, "Slow GPIO pins might wreak havoc into MDIO bus timing");
> +

I talked with Florian about this. We would like this hunk of the patch
dropped

1) For a patch which is going to stable, it does not fit. It does not
actually fix anything.

2) I'm not sure it has any value. The hardware has been designed like
that. There is nothing which can be done about it. Printing a message
is not going to help users.

   Andrew

Re: [PATCH v5] net: phy: mdio-gpio: Fix working over slow can_sleep GPIOs

[David Miller]

From: Martin Schiller <ms@dev.tdt.de>
Date: Thu, 15 Nov 2018 06:24:28 +0100

> Up until commit 7e5fbd1e0700 ("net: mdio-gpio: Convert to use gpiod
> functions where possible"), the _cansleep variants of the gpio_ API was
> used. After that commit and the change to gpiod_ API, the _cansleep()
> was dropped. This then results in WARN_ON() when used with GPIO
> devices which do sleep. Add back the _cansleep() to avoid this.
> 
> Fixes: 7e5fbd1e0700 ("net: mdio-gpio: Convert to use gpiod functions where possible")
> Signed-off-by: Martin Schiller <ms@dev.tdt.de>

Ok it seems there was still discussion going on here and I accidently applied
v4 of this patch.

I'll revert it and wait for the review to conclude.

[PATCH v6] net: phy: mdio-gpio: Fix working over slow can_sleep GPIOs

[Martin Schiller]

Up until commit 7e5fbd1e0700 ("net: mdio-gpio: Convert to use gpiod
functions where possible"), the _cansleep variants of the gpio_ API was
used. After that commit and the change to gpiod_ API, the _cansleep()
was dropped. This then results in WARN_ON() when used with GPIO
devices which do sleep. Add back the _cansleep() to avoid this.

Fixes: 7e5fbd1e0700 ("net: mdio-gpio: Convert to use gpiod functions where possible")
Signed-off-by: Martin Schiller <ms@dev.tdt.de>
---
v6:
 - dropped warning about slow GPIO pins
 
v5:
 - reworked commit message
 - added "Fixes:" tag
 - based on DaveM net tree instead of mainline
 
v4:
 - remove linewrap of kernel message
 
v3:
 - modified commit summary
 - fixed commit cites in commit message
 - fixed line continuation
 
v2:
 - fixed copy/paste bug in warning about slow GPIO pins
---
 drivers/net/phy/mdio-gpio.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/net/phy/mdio-gpio.c b/drivers/net/phy/mdio-gpio.c
index 33265747bf39..0fbcedcdf6e2 100644
--- a/drivers/net/phy/mdio-gpio.c
+++ b/drivers/net/phy/mdio-gpio.c
@@ -63,7 +63,7 @@ static void mdio_dir(struct mdiobb_ctrl *ctrl, int dir)
 		 * assume the pin serves as pull-up. If direction is
 		 * output, the default value is high.
 		 */
-		gpiod_set_value(bitbang->mdo, 1);
+		gpiod_set_value_cansleep(bitbang->mdo, 1);
 		return;
 	}
 
@@ -78,7 +78,7 @@ static int mdio_get(struct mdiobb_ctrl *ctrl)
 	struct mdio_gpio_info *bitbang =
 		container_of(ctrl, struct mdio_gpio_info, ctrl);
 
-	return gpiod_get_value(bitbang->mdio);
+	return gpiod_get_value_cansleep(bitbang->mdio);
 }
 
 static void mdio_set(struct mdiobb_ctrl *ctrl, int what)
@@ -87,9 +87,9 @@ static void mdio_set(struct mdiobb_ctrl *ctrl, int what)
 		container_of(ctrl, struct mdio_gpio_info, ctrl);
 
 	if (bitbang->mdo)
-		gpiod_set_value(bitbang->mdo, what);
+		gpiod_set_value_cansleep(bitbang->mdo, what);
 	else
-		gpiod_set_value(bitbang->mdio, what);
+		gpiod_set_value_cansleep(bitbang->mdio, what);
 }
 
 static void mdc_set(struct mdiobb_ctrl *ctrl, int what)
@@ -97,7 +97,7 @@ static void mdc_set(struct mdiobb_ctrl *ctrl, int what)
 	struct mdio_gpio_info *bitbang =
 		container_of(ctrl, struct mdio_gpio_info, ctrl);
 
-	gpiod_set_value(bitbang->mdc, what);
+	gpiod_set_value_cansleep(bitbang->mdc, what);
 }
 
 static const struct mdiobb_ops mdio_gpio_ops = {
-- 
2.11.0

Re: [PATCH v6] net: phy: mdio-gpio: Fix working over slow can_sleep GPIOs

[Andrew Lunn]

On Fri, Nov 16, 2018 at 08:38:36AM +0100, Martin Schiller wrote:
> Up until commit 7e5fbd1e0700 ("net: mdio-gpio: Convert to use gpiod
> functions where possible"), the _cansleep variants of the gpio_ API was
> used. After that commit and the change to gpiod_ API, the _cansleep()
> was dropped. This then results in WARN_ON() when used with GPIO
> devices which do sleep. Add back the _cansleep() to avoid this.
> 
> Fixes: 7e5fbd1e0700 ("net: mdio-gpio: Convert to use gpiod functions where possible")
> Signed-off-by: Martin Schiller <ms@dev.tdt.de>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH v6] net: phy: mdio-gpio: Fix working over slow can_sleep GPIOs

[Florian Fainelli]

On 15/11/2018 23:38, Martin Schiller wrote:
> Up until commit 7e5fbd1e0700 ("net: mdio-gpio: Convert to use gpiod
> functions where possible"), the _cansleep variants of the gpio_ API was
> used. After that commit and the change to gpiod_ API, the _cansleep()
> was dropped. This then results in WARN_ON() when used with GPIO
> devices which do sleep. Add back the _cansleep() to avoid this.
> 
> Fixes: 7e5fbd1e0700 ("net: mdio-gpio: Convert to use gpiod functions where possible")
> Signed-off-by: Martin Schiller <ms@dev.tdt.de>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>

Thanks Martin!
-- 
Florian

Re: [PATCH v6] net: phy: mdio-gpio: Fix working over slow can_sleep GPIOs

[David Miller]

From: Martin Schiller <ms@dev.tdt.de>
Date: Fri, 16 Nov 2018 08:38:36 +0100

> Up until commit 7e5fbd1e0700 ("net: mdio-gpio: Convert to use gpiod
> functions where possible"), the _cansleep variants of the gpio_ API was
> used. After that commit and the change to gpiod_ API, the _cansleep()
> was dropped. This then results in WARN_ON() when used with GPIO
> devices which do sleep. Add back the _cansleep() to avoid this.
> 
> Fixes: 7e5fbd1e0700 ("net: mdio-gpio: Convert to use gpiod functions where possible")
> Signed-off-by: Martin Schiller <ms@dev.tdt.de>

Applied and queued up for -stable.