[v2,net-next] drivers: net: davinci_mdio: prevent spurious timeout

Message ID 20180509110024.1772-1-nsekhar@ti.com
State New
Headers show
Series
  • [v2,net-next] drivers: net: davinci_mdio: prevent spurious timeout
Related show

Commit Message

Sekhar Nori May 9, 2018, 11 a.m.
A well timed kernel preemption in the time_after() loop
in wait_for_idle() can result in a spurious timeout
error to be returned.

Fix it by using readl_poll_timeout() which takes care of
this issue.

Signed-off-by: Sekhar Nori <nsekhar@ti.com>

---
v2: use readl_poll_timeout() per suggestion from Andrew.

The issue has not been personally observed by me, but has
been reported by users. Sending for next-next given the
non-critical nature. There is seems to be no easy way to
reproduce this.

 drivers/net/ethernet/ti/davinci_mdio.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

-- 
2.16.2

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Andrew Lunn May 9, 2018, 1:30 p.m. | #1
On Wed, May 09, 2018 at 04:30:24PM +0530, Sekhar Nori wrote:
> A well timed kernel preemption in the time_after() loop

> in wait_for_idle() can result in a spurious timeout

> error to be returned.

> 

> Fix it by using readl_poll_timeout() which takes care of

> this issue.

> 

> Signed-off-by: Sekhar Nori <nsekhar@ti.com>

> ---

> v2: use readl_poll_timeout() per suggestion from Andrew.

> 

> The issue has not been personally observed by me, but has

> been reported by users. Sending for next-next given the

> non-critical nature. There is seems to be no easy way to

> reproduce this.

> 

>  drivers/net/ethernet/ti/davinci_mdio.c | 15 +++++++++------

>  1 file changed, 9 insertions(+), 6 deletions(-)

> 

> diff --git a/drivers/net/ethernet/ti/davinci_mdio.c b/drivers/net/ethernet/ti/davinci_mdio.c

> index 3c33f4504d8e..d073432a5dbe 100644

> --- a/drivers/net/ethernet/ti/davinci_mdio.c

> +++ b/drivers/net/ethernet/ti/davinci_mdio.c

> @@ -34,6 +34,7 @@

>  #include <linux/clk.h>

>  #include <linux/err.h>

>  #include <linux/io.h>

> +#include <linux/iopoll.h>

>  #include <linux/pm_runtime.h>

>  #include <linux/davinci_emac.h>

>  #include <linux/of.h>

> @@ -227,14 +228,16 @@ static inline int wait_for_user_access(struct davinci_mdio_data *data)

>  static inline int wait_for_idle(struct davinci_mdio_data *data)

>  {

>  	struct davinci_mdio_regs __iomem *regs = data->regs;

> -	unsigned long timeout = jiffies + msecs_to_jiffies(MDIO_TIMEOUT);

> +	u32 val, ret;

>  

> -	while (time_after(timeout, jiffies)) {

> -		if (__raw_readl(&regs->control) & CONTROL_IDLE)

> -			return 0;

> +	ret = readl_poll_timeout(&regs->control, val, val & CONTROL_IDLE,

> +				 0, MDIO_TIMEOUT * 1000);

> +	if (ret) {

> +		dev_err(data->dev, "timed out waiting for idle\n");

> +		return ret;

>  	}

> -	dev_err(data->dev, "timed out waiting for idle\n");

> -	return -ETIMEDOUT;

> +

> +	return 0;

>  }


Hi Sekhar

You could simplify this to:

> +	if (ret)

> +		dev_err(data->dev, "timed out waiting for idle\n");

> +	return ret;


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


    Andrew
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sekhar Nori May 9, 2018, 3:46 p.m. | #2
On Wednesday 09 May 2018 07:00 PM, Andrew Lunn wrote:
> On Wed, May 09, 2018 at 04:30:24PM +0530, Sekhar Nori wrote:

>> A well timed kernel preemption in the time_after() loop

>> in wait_for_idle() can result in a spurious timeout

>> error to be returned.

>>

>> Fix it by using readl_poll_timeout() which takes care of

>> this issue.

>>

>> Signed-off-by: Sekhar Nori <nsekhar@ti.com>

>> ---

>> v2: use readl_poll_timeout() per suggestion from Andrew.

>>

>> The issue has not been personally observed by me, but has

>> been reported by users. Sending for next-next given the

>> non-critical nature. There is seems to be no easy way to

>> reproduce this.

>>

>>  drivers/net/ethernet/ti/davinci_mdio.c | 15 +++++++++------

>>  1 file changed, 9 insertions(+), 6 deletions(-)

>>

>> diff --git a/drivers/net/ethernet/ti/davinci_mdio.c b/drivers/net/ethernet/ti/davinci_mdio.c

>> index 3c33f4504d8e..d073432a5dbe 100644

>> --- a/drivers/net/ethernet/ti/davinci_mdio.c

>> +++ b/drivers/net/ethernet/ti/davinci_mdio.c

>> @@ -34,6 +34,7 @@

>>  #include <linux/clk.h>

>>  #include <linux/err.h>

>>  #include <linux/io.h>

>> +#include <linux/iopoll.h>

>>  #include <linux/pm_runtime.h>

>>  #include <linux/davinci_emac.h>

>>  #include <linux/of.h>

>> @@ -227,14 +228,16 @@ static inline int wait_for_user_access(struct davinci_mdio_data *data)

>>  static inline int wait_for_idle(struct davinci_mdio_data *data)

>>  {

>>  	struct davinci_mdio_regs __iomem *regs = data->regs;

>> -	unsigned long timeout = jiffies + msecs_to_jiffies(MDIO_TIMEOUT);

>> +	u32 val, ret;

>>  

>> -	while (time_after(timeout, jiffies)) {

>> -		if (__raw_readl(&regs->control) & CONTROL_IDLE)

>> -			return 0;

>> +	ret = readl_poll_timeout(&regs->control, val, val & CONTROL_IDLE,

>> +				 0, MDIO_TIMEOUT * 1000);

>> +	if (ret) {

>> +		dev_err(data->dev, "timed out waiting for idle\n");

>> +		return ret;

>>  	}

>> -	dev_err(data->dev, "timed out waiting for idle\n");

>> -	return -ETIMEDOUT;

>> +

>> +	return 0;

>>  }

> 

> Hi Sekhar

> 

> You could simplify this to:

> 

>> +	if (ret)

>> +		dev_err(data->dev, "timed out waiting for idle\n");

>> +	return ret;

> 

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


Indeed. v3 sent.

Thanks,
Sekhar
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/net/ethernet/ti/davinci_mdio.c b/drivers/net/ethernet/ti/davinci_mdio.c
index 3c33f4504d8e..d073432a5dbe 100644
--- a/drivers/net/ethernet/ti/davinci_mdio.c
+++ b/drivers/net/ethernet/ti/davinci_mdio.c
@@ -34,6 +34,7 @@ 
 #include <linux/clk.h>
 #include <linux/err.h>
 #include <linux/io.h>
+#include <linux/iopoll.h>
 #include <linux/pm_runtime.h>
 #include <linux/davinci_emac.h>
 #include <linux/of.h>
@@ -227,14 +228,16 @@  static inline int wait_for_user_access(struct davinci_mdio_data *data)
 static inline int wait_for_idle(struct davinci_mdio_data *data)
 {
 	struct davinci_mdio_regs __iomem *regs = data->regs;
-	unsigned long timeout = jiffies + msecs_to_jiffies(MDIO_TIMEOUT);
+	u32 val, ret;
 
-	while (time_after(timeout, jiffies)) {
-		if (__raw_readl(&regs->control) & CONTROL_IDLE)
-			return 0;
+	ret = readl_poll_timeout(&regs->control, val, val & CONTROL_IDLE,
+				 0, MDIO_TIMEOUT * 1000);
+	if (ret) {
+		dev_err(data->dev, "timed out waiting for idle\n");
+		return ret;
 	}
-	dev_err(data->dev, "timed out waiting for idle\n");
-	return -ETIMEDOUT;
+
+	return 0;
 }
 
 static int davinci_mdio_read(struct mii_bus *bus, int phy_id, int phy_reg)