| Message ID | 20220519204017.15586-1-paskripkin@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: ocelot: fix wront time_after usage | expand |
On Thu, May 19, 2022 at 11:40:17PM +0300, Pavel Skripkin wrote: > Accidentally noticed, that this driver is the only user of > while (timer_after(jiffies...)). > > It looks like typo, because likely this while loop will finish after 1st > iteration, because time_after() returns true when 1st argument _is after_ > 2nd one. > > Fix it by negating time_after return value inside while loops statement > > Fixes: 753a026cfec1 ("net: ocelot: add FDMA support") > Signed-off-by: Pavel Skripkin <paskripkin@gmail.com> > --- > drivers/net/ethernet/mscc/ocelot_fdma.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/mscc/ocelot_fdma.c b/drivers/net/ethernet/mscc/ocelot_fdma.c > index dffa597bffe6..4500fed3ce5c 100644 > --- a/drivers/net/ethernet/mscc/ocelot_fdma.c > +++ b/drivers/net/ethernet/mscc/ocelot_fdma.c > @@ -104,7 +104,7 @@ static int ocelot_fdma_wait_chan_safe(struct ocelot *ocelot, int chan) > safe = ocelot_fdma_readl(ocelot, MSCC_FDMA_CH_SAFE); > if (safe & BIT(chan)) > return 0; > - } while (time_after(jiffies, timeout)); > + } while (!time_after(jiffies, timeout)); > > return -ETIMEDOUT; > } > -- > 2.36.1 > +Clement. Also, there seems to be a typo in the commit message (wront -> wrong), but maybe this isn't so important.
Le Thu, 19 May 2022 23:13:01 +0000, Vladimir Oltean <vladimir.oltean@nxp.com> a écrit : > On Thu, May 19, 2022 at 11:40:17PM +0300, Pavel Skripkin wrote: > > Accidentally noticed, that this driver is the only user of > > while (timer_after(jiffies...)). > > > > It looks like typo, because likely this while loop will finish after 1st > > iteration, because time_after() returns true when 1st argument _is after_ > > 2nd one. > > > > Fix it by negating time_after return value inside while loops statement > > > > Fixes: 753a026cfec1 ("net: ocelot: add FDMA support") > > Signed-off-by: Pavel Skripkin <paskripkin@gmail.com> > > --- > > drivers/net/ethernet/mscc/ocelot_fdma.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/net/ethernet/mscc/ocelot_fdma.c b/drivers/net/ethernet/mscc/ocelot_fdma.c > > index dffa597bffe6..4500fed3ce5c 100644 > > --- a/drivers/net/ethernet/mscc/ocelot_fdma.c > > +++ b/drivers/net/ethernet/mscc/ocelot_fdma.c > > @@ -104,7 +104,7 @@ static int ocelot_fdma_wait_chan_safe(struct ocelot *ocelot, int chan) > > safe = ocelot_fdma_readl(ocelot, MSCC_FDMA_CH_SAFE); > > if (safe & BIT(chan)) > > return 0; > > - } while (time_after(jiffies, timeout)); > > + } while (!time_after(jiffies, timeout)); > > > > return -ETIMEDOUT; > > } > > -- > > 2.36.1 > > > > +Clement. Also, there seems to be a typo in the commit message (wront -> wrong), > but maybe this isn't so important. Hi Pavel, Thanks for this fix which is indeed necessary. Acked-by: Clément Léger <clement.leger@bootlin.com>
On Thu, May 19, 2022 at 11:40:17PM +0300, Pavel Skripkin wrote: > Accidentally noticed, that this driver is the only user of > while (timer_after(jiffies...)). > > It looks like typo, because likely this while loop will finish after 1st > iteration, because time_after() returns true when 1st argument _is after_ > 2nd one. > > Fix it by negating time_after return value inside while loops statement A better fix would be to use one of the helpers in linux/iopoll.h. There is a second bug in the current code: static int ocelot_fdma_wait_chan_safe(struct ocelot *ocelot, int chan) { unsigned long timeout; u32 safe; timeout = jiffies + usecs_to_jiffies(OCELOT_FDMA_CH_SAFE_TIMEOUT_US); do { safe = ocelot_fdma_readl(ocelot, MSCC_FDMA_CH_SAFE); if (safe & BIT(chan)) return 0; } while (time_after(jiffies, timeout)); return -ETIMEDOUT; } The scheduler could put the thread to sleep, and it does not get woken up for OCELOT_FDMA_CH_SAFE_TIMEOUT_US. During that time, the hardware has done its thing, but you exit the while loop and return -ETIMEDOUT. linux/iopoll.h handles this correctly by testing the state one more time after the timeout has expired. Andrew
Hi Andrew, On 5/20/22 15:40, Andrew Lunn wrote: > On Thu, May 19, 2022 at 11:40:17PM +0300, Pavel Skripkin wrote: >> Accidentally noticed, that this driver is the only user of >> while (timer_after(jiffies...)). >> >> It looks like typo, because likely this while loop will finish after 1st >> iteration, because time_after() returns true when 1st argument _is after_ >> 2nd one. >> >> Fix it by negating time_after return value inside while loops statement > > A better fix would be to use one of the helpers in linux/iopoll.h. > > There is a second bug in the current code: > > static int ocelot_fdma_wait_chan_safe(struct ocelot *ocelot, int chan) > { > unsigned long timeout; > u32 safe; > > timeout = jiffies + usecs_to_jiffies(OCELOT_FDMA_CH_SAFE_TIMEOUT_US); > do { > safe = ocelot_fdma_readl(ocelot, MSCC_FDMA_CH_SAFE); > if (safe & BIT(chan)) > return 0; > } while (time_after(jiffies, timeout)); > > return -ETIMEDOUT; > } > > The scheduler could put the thread to sleep, and it does not get woken > up for OCELOT_FDMA_CH_SAFE_TIMEOUT_US. During that time, the hardware > has done its thing, but you exit the while loop and return -ETIMEDOUT. > > linux/iopoll.h handles this correctly by testing the state one more > time after the timeout has expired. > I wasn't aware about these macros. Thanks for pointing out to that header! Will send v2 soon, With regards, Pavel Skripkin
diff --git a/drivers/net/ethernet/mscc/ocelot_fdma.c b/drivers/net/ethernet/mscc/ocelot_fdma.c index dffa597bffe6..4500fed3ce5c 100644 --- a/drivers/net/ethernet/mscc/ocelot_fdma.c +++ b/drivers/net/ethernet/mscc/ocelot_fdma.c @@ -104,7 +104,7 @@ static int ocelot_fdma_wait_chan_safe(struct ocelot *ocelot, int chan) safe = ocelot_fdma_readl(ocelot, MSCC_FDMA_CH_SAFE); if (safe & BIT(chan)) return 0; - } while (time_after(jiffies, timeout)); + } while (!time_after(jiffies, timeout)); return -ETIMEDOUT; }
Accidentally noticed, that this driver is the only user of while (timer_after(jiffies...)). It looks like typo, because likely this while loop will finish after 1st iteration, because time_after() returns true when 1st argument _is after_ 2nd one. Fix it by negating time_after return value inside while loops statement Fixes: 753a026cfec1 ("net: ocelot: add FDMA support") Signed-off-by: Pavel Skripkin <paskripkin@gmail.com> --- drivers/net/ethernet/mscc/ocelot_fdma.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)