linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] net: phy: mdio-gpio: fix access that may sleep
@ 2018-11-14  6:17 Martin Schiller
  2018-11-14  6:37 ` [PATCH v2] " Martin Schiller
                   ` (4 more replies)
  0 siblings, 5 replies; 19+ messages in thread
From: Martin Schiller @ 2018-11-14  6:17 UTC (permalink / raw)
  To: netdev, linux-kernel; +Cc: andrew, f.fainelli, davem, 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


^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH v2] net: phy: mdio-gpio: fix access that may sleep
  2018-11-14  6:17 [PATCH] net: phy: mdio-gpio: fix access that may sleep Martin Schiller
@ 2018-11-14  6:37 ` Martin Schiller
  2018-11-14  7:05   ` Andrew Lunn
                     ` (2 more replies)
  2018-11-14 10:12 ` [PATCH v3] net: phy: mdio-gpio: Fix working over slow can_sleep GPIOs Martin Schiller
                   ` (3 subsequent siblings)
  4 siblings, 3 replies; 19+ messages in thread
From: Martin Schiller @ 2018-11-14  6:37 UTC (permalink / raw)
  To: netdev, linux-kernel; +Cc: andrew, f.fainelli, davem, 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


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2] net: phy: mdio-gpio: fix access that may sleep
  2018-11-14  6:37 ` [PATCH v2] " Martin Schiller
@ 2018-11-14  7:05   ` Andrew Lunn
  2018-11-14  7:43     ` Martin Schiller
  2018-11-14  9:20   ` Sergei Shtylyov
  2018-11-17  3:52   ` David Miller
  2 siblings, 1 reply; 19+ messages in thread
From: Andrew Lunn @ 2018-11-14  7:05 UTC (permalink / raw)
  To: Martin Schiller; +Cc: netdev, linux-kernel, f.fainelli, davem

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2] net: phy: mdio-gpio: fix access that may sleep
  2018-11-14  7:05   ` Andrew Lunn
@ 2018-11-14  7:43     ` Martin Schiller
  2018-11-14 20:46       ` Andrew Lunn
  0 siblings, 1 reply; 19+ messages in thread
From: Martin Schiller @ 2018-11-14  7:43 UTC (permalink / raw)
  To: Andrew Lunn; +Cc: netdev, linux-kernel, f.fainelli, davem

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2] net: phy: mdio-gpio: fix access that may sleep
  2018-11-14  6:37 ` [PATCH v2] " Martin Schiller
  2018-11-14  7:05   ` Andrew Lunn
@ 2018-11-14  9:20   ` Sergei Shtylyov
  2018-11-17  3:52   ` David Miller
  2 siblings, 0 replies; 19+ messages in thread
From: Sergei Shtylyov @ 2018-11-14  9:20 UTC (permalink / raw)
  To: Martin Schiller, netdev, linux-kernel; +Cc: andrew, f.fainelli, davem

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH v3] net: phy: mdio-gpio: Fix working over slow can_sleep GPIOs
  2018-11-14  6:17 [PATCH] net: phy: mdio-gpio: fix access that may sleep Martin Schiller
  2018-11-14  6:37 ` [PATCH v2] " Martin Schiller
@ 2018-11-14 10:12 ` Martin Schiller
  2018-11-14 11:03   ` Sergei Shtylyov
  2018-11-14 11:54 ` [PATCH v4] " Martin Schiller
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 19+ messages in thread
From: Martin Schiller @ 2018-11-14 10:12 UTC (permalink / raw)
  To: andrew, sergei.shtylyov, f.fainelli
  Cc: davem, netdev, linux-kernel, 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


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v3] net: phy: mdio-gpio: Fix working over slow can_sleep GPIOs
  2018-11-14 10:12 ` [PATCH v3] net: phy: mdio-gpio: Fix working over slow can_sleep GPIOs Martin Schiller
@ 2018-11-14 11:03   ` Sergei Shtylyov
  0 siblings, 0 replies; 19+ messages in thread
From: Sergei Shtylyov @ 2018-11-14 11:03 UTC (permalink / raw)
  To: Martin Schiller, andrew, f.fainelli; +Cc: davem, netdev, linux-kernel

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH v4] net: phy: mdio-gpio: Fix working over slow can_sleep GPIOs
  2018-11-14  6:17 [PATCH] net: phy: mdio-gpio: fix access that may sleep Martin Schiller
  2018-11-14  6:37 ` [PATCH v2] " Martin Schiller
  2018-11-14 10:12 ` [PATCH v3] net: phy: mdio-gpio: Fix working over slow can_sleep GPIOs Martin Schiller
@ 2018-11-14 11:54 ` Martin Schiller
  2018-11-17  4:25   ` David Miller
  2018-11-15  5:24 ` [PATCH v5] " Martin Schiller
  2018-11-16  7:38 ` [PATCH v6] " Martin Schiller
  4 siblings, 1 reply; 19+ messages in thread
From: Martin Schiller @ 2018-11-14 11:54 UTC (permalink / raw)
  To: andrew, sergei.shtylyov, f.fainelli
  Cc: davem, netdev, linux-kernel, 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


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2] net: phy: mdio-gpio: fix access that may sleep
  2018-11-14  7:43     ` Martin Schiller
@ 2018-11-14 20:46       ` Andrew Lunn
  0 siblings, 0 replies; 19+ messages in thread
From: Andrew Lunn @ 2018-11-14 20:46 UTC (permalink / raw)
  To: Martin Schiller; +Cc: netdev, linux-kernel, f.fainelli, davem

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH v5] net: phy: mdio-gpio: Fix working over slow can_sleep GPIOs
  2018-11-14  6:17 [PATCH] net: phy: mdio-gpio: fix access that may sleep Martin Schiller
                   ` (2 preceding siblings ...)
  2018-11-14 11:54 ` [PATCH v4] " Martin Schiller
@ 2018-11-15  5:24 ` Martin Schiller
  2018-11-15 20:12   ` Andrew Lunn
  2018-11-17  7:04   ` David Miller
  2018-11-16  7:38 ` [PATCH v6] " Martin Schiller
  4 siblings, 2 replies; 19+ messages in thread
From: Martin Schiller @ 2018-11-15  5:24 UTC (permalink / raw)
  To: andrew, sergei.shtylyov, f.fainelli
  Cc: davem, netdev, linux-kernel, 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


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v5] net: phy: mdio-gpio: Fix working over slow can_sleep GPIOs
  2018-11-15  5:24 ` [PATCH v5] " Martin Schiller
@ 2018-11-15 20:12   ` Andrew Lunn
  2018-11-17  7:04   ` David Miller
  1 sibling, 0 replies; 19+ messages in thread
From: Andrew Lunn @ 2018-11-15 20:12 UTC (permalink / raw)
  To: Martin Schiller; +Cc: sergei.shtylyov, f.fainelli, davem, netdev, linux-kernel

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH v6] net: phy: mdio-gpio: Fix working over slow can_sleep GPIOs
  2018-11-14  6:17 [PATCH] net: phy: mdio-gpio: fix access that may sleep Martin Schiller
                   ` (3 preceding siblings ...)
  2018-11-15  5:24 ` [PATCH v5] " Martin Schiller
@ 2018-11-16  7:38 ` Martin Schiller
  2018-11-16  7:53   ` Andrew Lunn
                     ` (2 more replies)
  4 siblings, 3 replies; 19+ messages in thread
From: Martin Schiller @ 2018-11-16  7:38 UTC (permalink / raw)
  To: andrew, sergei.shtylyov, f.fainelli
  Cc: davem, netdev, linux-kernel, 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


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v6] net: phy: mdio-gpio: Fix working over slow can_sleep GPIOs
  2018-11-16  7:38 ` [PATCH v6] " Martin Schiller
@ 2018-11-16  7:53   ` Andrew Lunn
  2018-11-17 19:52   ` Florian Fainelli
  2018-11-18  5:12   ` David Miller
  2 siblings, 0 replies; 19+ messages in thread
From: Andrew Lunn @ 2018-11-16  7:53 UTC (permalink / raw)
  To: Martin Schiller; +Cc: sergei.shtylyov, f.fainelli, davem, netdev, linux-kernel

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2] net: phy: mdio-gpio: fix access that may sleep
  2018-11-14  6:37 ` [PATCH v2] " Martin Schiller
  2018-11-14  7:05   ` Andrew Lunn
  2018-11-14  9:20   ` Sergei Shtylyov
@ 2018-11-17  3:52   ` David Miller
  2 siblings, 0 replies; 19+ messages in thread
From: David Miller @ 2018-11-17  3:52 UTC (permalink / raw)
  To: ms; +Cc: netdev, linux-kernel, andrew, f.fainelli

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.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v4] net: phy: mdio-gpio: Fix working over slow can_sleep GPIOs
  2018-11-14 11:54 ` [PATCH v4] " Martin Schiller
@ 2018-11-17  4:25   ` David Miller
  2018-11-17  6:28     ` Andrew Lunn
  0 siblings, 1 reply; 19+ messages in thread
From: David Miller @ 2018-11-17  4:25 UTC (permalink / raw)
  To: ms; +Cc: andrew, sergei.shtylyov, f.fainelli, netdev, linux-kernel

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.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v4] net: phy: mdio-gpio: Fix working over slow can_sleep GPIOs
  2018-11-17  4:25   ` David Miller
@ 2018-11-17  6:28     ` Andrew Lunn
  0 siblings, 0 replies; 19+ messages in thread
From: Andrew Lunn @ 2018-11-17  6:28 UTC (permalink / raw)
  To: David Miller; +Cc: ms, sergei.shtylyov, f.fainelli, netdev, linux-kernel

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




^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v5] net: phy: mdio-gpio: Fix working over slow can_sleep GPIOs
  2018-11-15  5:24 ` [PATCH v5] " Martin Schiller
  2018-11-15 20:12   ` Andrew Lunn
@ 2018-11-17  7:04   ` David Miller
  1 sibling, 0 replies; 19+ messages in thread
From: David Miller @ 2018-11-17  7:04 UTC (permalink / raw)
  To: ms; +Cc: andrew, sergei.shtylyov, f.fainelli, netdev, linux-kernel

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.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v6] net: phy: mdio-gpio: Fix working over slow can_sleep GPIOs
  2018-11-16  7:38 ` [PATCH v6] " Martin Schiller
  2018-11-16  7:53   ` Andrew Lunn
@ 2018-11-17 19:52   ` Florian Fainelli
  2018-11-18  5:12   ` David Miller
  2 siblings, 0 replies; 19+ messages in thread
From: Florian Fainelli @ 2018-11-17 19:52 UTC (permalink / raw)
  To: Martin Schiller, andrew, sergei.shtylyov; +Cc: davem, netdev, linux-kernel



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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v6] net: phy: mdio-gpio: Fix working over slow can_sleep GPIOs
  2018-11-16  7:38 ` [PATCH v6] " Martin Schiller
  2018-11-16  7:53   ` Andrew Lunn
  2018-11-17 19:52   ` Florian Fainelli
@ 2018-11-18  5:12   ` David Miller
  2 siblings, 0 replies; 19+ messages in thread
From: David Miller @ 2018-11-18  5:12 UTC (permalink / raw)
  To: ms; +Cc: andrew, sergei.shtylyov, f.fainelli, netdev, linux-kernel

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.

^ permalink raw reply	[flat|nested] 19+ messages in thread

end of thread, other threads:[~2018-11-18  5:12 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-14  6:17 [PATCH] net: phy: mdio-gpio: fix access that may sleep Martin Schiller
2018-11-14  6:37 ` [PATCH v2] " Martin Schiller
2018-11-14  7:05   ` Andrew Lunn
2018-11-14  7:43     ` Martin Schiller
2018-11-14 20:46       ` Andrew Lunn
2018-11-14  9:20   ` Sergei Shtylyov
2018-11-17  3:52   ` David Miller
2018-11-14 10:12 ` [PATCH v3] net: phy: mdio-gpio: Fix working over slow can_sleep GPIOs Martin Schiller
2018-11-14 11:03   ` Sergei Shtylyov
2018-11-14 11:54 ` [PATCH v4] " Martin Schiller
2018-11-17  4:25   ` David Miller
2018-11-17  6:28     ` Andrew Lunn
2018-11-15  5:24 ` [PATCH v5] " Martin Schiller
2018-11-15 20:12   ` Andrew Lunn
2018-11-17  7:04   ` David Miller
2018-11-16  7:38 ` [PATCH v6] " Martin Schiller
2018-11-16  7:53   ` Andrew Lunn
2018-11-17 19:52   ` Florian Fainelli
2018-11-18  5:12   ` David Miller

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).