net: phy: mdio-gpio: fix access that may sleep

PATCH Superseded Martin Schiller 5 participants 19 messages Nov 14, 2018 – Nov 18, 2018

Subsystems (3)

Networking Network drivers Ethernet PHY

lore

Share & tools

lore

Martin Schiller

author

2018-11-14 06:17 UTC #0 lore raw

PATCH 1/1 net: phy: mdio-gpio: fix access that may sleep

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 <[email protected]>

---
 drivers/net/phy/mdio-gpio.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

drivers/net/phy/mdio-gpio.c +10 -5

index 33265747bf39..d4430631ca6a 100644
@@ -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) {

Signature · 1 line

2.11.0

Martin Schiller

author

2018-11-14 06:37 UTC #1 lore raw

PATCH v2 1/1 net: phy: mdio-gpio: fix access that may sleep

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 <[email protected]>

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

drivers/net/phy/mdio-gpio.c +10 -5

index 33265747bf39..6c1cca14689b 100644
@@ -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) {

Signature · 1 line

2.11.0

Andrew Lunn

maintainer

2018-11-14 07:05 UTC #2 lore raw

Quoted 5 lines · Martin Schiller

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

Martin Schiller

author

2018-11-14 07:43 UTC #3 lore raw

Quoted 7 lines · Andrew Lunn

Quoted 5 lines · Martin Schiller
This commit re-enables support for slow GPIO pins. It was initially introduced by commit
	2d6c9091ab7630dfcf34417c6683ce4764d7d40a
and got lost by commit
	7e5fbd1e0700f1bdb94508f84ec2aeb01eed7b12

Quoted 5 lines

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

Quoted 5 lines

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

Sergei Shtylyov

2018-11-14 09:20 UTC #5 lore raw

In reply to Martin Schiller

Hello!

Quoted 5 lines · Martin Schiller

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 ("").

Quoted 15 lines

Also add a warning about slow GPIO pins like it is done in i2c-gpio.
Signed-off-by Martin Schiller <[email protected]>
---
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(-)
drivers/net/phy/mdio-gpio.c +0 -0
index 33265747bf39..6c1cca14689b 100644

[...]

drivers/net/phy/mdio-gpio.c static int mdio_gpio_probe(struct platform_device *pdev) · line 166

Quoted 6 lines

drivers/net/phy/mdio-gpio.c +2 -0

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

drivers/net/phy/mdio-gpio.c static int mdio_gpio_probe(struct platform_device *pdev) · line 172

Quoted 6 lines

drivers/net/phy/mdio-gpio.c +3 -0

+		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

Martin Schiller

author

2018-11-14 10:12 UTC #7 lore raw

PATCH v3 1/1 net: phy: mdio-gpio: Fix working over slow can_sleep GPIOs

In reply to 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 <[email protected]>

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

drivers/net/phy/mdio-gpio.c +10 -5

index 33265747bf39..e1c305089172 100644
@@ -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) {

Signature · 1 line

2.11.0

Sergei Shtylyov

2018-11-14 11:03 UTC #8 lore raw

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

Quoted 24 lines · 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 <[email protected]>
---
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(-)
drivers/net/phy/mdio-gpio.c +0 -0
index 33265747bf39..e1c305089172 100644

[...]

drivers/net/phy/mdio-gpio.c static int mdio_gpio_probe(struct platform_device *pdev) · line 168

Quoted 8 lines

drivers/net/phy/mdio-gpio.c +4 -0

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

Martin Schiller

author

2018-11-14 11:54 UTC #9 lore raw

PATCH v4 1/1 net: phy: mdio-gpio: Fix working over slow can_sleep GPIOs

In reply to Martin Schiller

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

Signed-off-by Martin Schiller <[email protected]>

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

drivers/net/phy/mdio-gpio.c +9 -5

index 33265747bf39..3a5a24daf384 100644
@@ -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) {

Signature · 1 line

2.11.0

Andrew Lunn

maintainer

2018-11-14 20:46 UTC #4 lore raw

In reply to Martin Schiller

Quoted 19 lines · Martin Schiller

Quoted 7 lines · Andrew Lunn
Quoted 5 lines · Martin Schiller
This commit re-enables support for slow GPIO pins. It was initially introduced by commit
	2d6c9091ab7630dfcf34417c6683ce4764d7d40a
and got lost by commit
	7e5fbd1e0700f1bdb94508f84ec2aeb01eed7b12
Quoted 5 lines
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.

Quoted 7 lines

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.

Quoted 7 lines

Quoted 5 lines
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

Martin Schiller

author

2018-11-15 05:24 UTC #12 lore raw

PATCH v5 1/1 net: phy: mdio-gpio: Fix working over slow can_sleep GPIOs

In reply to 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 <[email protected]>

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

drivers/net/phy/mdio-gpio.c +9 -5

index 33265747bf39..3a5a24daf384 100644
@@ -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) {

Signature · 1 line

2.11.0

Andrew Lunn

maintainer

2018-11-15 20:12 UTC #13 lore raw

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

Quoted 6 lines · 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.

Quoted 7 lines

Fixes 7e5fbd1e0700 ("net: mdio-gpio: Convert to use gpiod functions where possible")
Signed-off-by Martin Schiller <[email protected]>
---
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.

drivers/net/phy/mdio-gpio.c static int mdio_gpio_probe(struct platform_device *pdev) · line 168

Quoted 8 lines

drivers/net/phy/mdio-gpio.c +4 -0

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

Martin Schiller

author

2018-11-16 07:38 UTC #15 lore raw

PATCH v6 1/1 net: phy: mdio-gpio: Fix working over slow can_sleep GPIOs

In reply to Martin Schiller

Fixes 7e5fbd1e0700 ("net: mdio-gpio: Convert to use gpiod functions where possible")

Signed-off-by Martin Schiller <[email protected]>

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

drivers/net/phy/mdio-gpio.c +5 -5

index 33265747bf39..0fbcedcdf6e2 100644
@@ -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 = {

Signature · 1 line

2.11.0

Andrew Lunn

maintainer Reviewed-by on v6 1/1

2018-11-16 07:53 UTC #16 lore raw

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

Quoted 8 lines · 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 <[email protected]>

Reviewed-by Andrew Lunn <[email protected]>

Andrew

David Miller

maintainer

2018-11-17 03:52 UTC #6 lore raw

In reply to Martin Schiller

From: Martin Schiller <[email protected]>
Date: Wed, 14 Nov 2018 07:37:03 +0100

drivers/net/phy/mdio-gpio.c static int mdio_gpio_probe(struct platform_device *pdev) · line 166

Quoted 6 lines

drivers/net/phy/mdio-gpio.c +2 -0

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

David Miller

maintainer Applied

2018-11-17 04:25 UTC #10 lore raw

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

In reply to Martin Schiller

From: Martin Schiller <[email protected]>
Date: Wed, 14 Nov 2018 12:54:49 +0100

Quoted 8 lines

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 <[email protected]>

Applied, thanks.

Andrew Lunn

maintainer

2018-11-17 06:29 UTC #11 lore raw

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

Quoted 8 lines · David Miller

From: Martin Schiller <[email protected]>
Date: Wed, 14 Nov 2018 12:54:49 +0100
Quoted 5 lines
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").

Quoted 5 lines

Quoted 3 lines
Also add a warning about slow GPIO pins like it is done in i2c-gpio.
Signed-off-by Martin Schiller <[email protected]>
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

David Miller

maintainer

2018-11-17 07:04 UTC #14 lore raw

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

In reply to Martin Schiller

From: Martin Schiller <[email protected]>
Date: Thu, 15 Nov 2018 06:24:28 +0100

Quoted 8 lines

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 <[email protected]>

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.

Florian Fainelli

Reviewed-by on v6 1/1

2018-11-17 19:52 UTC #17 lore raw

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

In reply to Martin Schiller

Quoted 8 lines · 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 <[email protected]>

Reviewed-by Florian Fainelli <[email protected]>

Thanks Martin!

Signature · 1 line

Florian

David Miller

maintainer

2018-11-18 05:12 UTC #18 lore raw

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

In reply to Martin Schiller

From: Martin Schiller <[email protected]>
Date: Fri, 16 Nov 2018 08:38:36 +0100

Quoted 8 lines

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 <[email protected]>

Applied and queued up for -stable.