| Message ID | 20200724071518.430515-3-joel@jms.id.au |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | fsi: SBE FIFO fixes | expand |
On 7/24/20 12:15 AM, Joel Stanley wrote: > From: Joachim Fenkes <FENKES@de.ibm.com> > > On BMCs with lower timer resolution than 1ms, msleep(1) will take > way longer than 1ms, so looping 10k times won't wait for 10s but > significantly longer. > > Fix this by using jiffies like the rest of the code. > > Fixes: 9f4a8a2d7f9d ("fsi/sbefifo: Add driver for the SBE FIFO") > Signed-off-by: Joachim Fenkes <fenkes@de.ibm.com> > Signed-off-by: Joel Stanley <joel@jms.id.au> > --- > drivers/fsi/fsi-sbefifo.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/fsi/fsi-sbefifo.c b/drivers/fsi/fsi-sbefifo.c > index 655b45c1f6ba..3ad9510ad4a4 100644 > --- a/drivers/fsi/fsi-sbefifo.c > +++ b/drivers/fsi/fsi-sbefifo.c > @@ -325,6 +325,7 @@ static int sbefifo_up_write(struct sbefifo *sbefifo, __be32 word) > static int sbefifo_request_reset(struct sbefifo *sbefifo) > { > struct device *dev = &sbefifo->fsi_dev->dev; > + unsigned long end_time; > u32 status, timeout; > int rc; > > @@ -341,7 +342,8 @@ static int sbefifo_request_reset(struct sbefifo *sbefifo) > } > > /* Wait for it to complete */ > - for (timeout = 0; timeout < SBEFIFO_RESET_TIMEOUT; timeout++) { > + end_time = jiffies + msecs_to_jiffies(SBEFIFO_RESET_TIMEOUT); > + while (!time_after(jiffies, end_time)) { > rc = sbefifo_regr(sbefifo, SBEFIFO_UP | SBEFIFO_STS, &status); > if (rc) { > dev_err(dev, "Failed to read UP fifo status during reset" > @@ -355,7 +357,7 @@ static int sbefifo_request_reset(struct sbefifo *sbefifo) > return 0; > } > > - msleep(1); > + cond_resched(); A hot loop ? Are you sure ? usleep_range() might make more sense here. Thanks, Guenter > } > dev_err(dev, "FIFO reset timed out\n"); > >
On Fri, 24 Jul 2020 at 14:34, Guenter Roeck <linux@roeck-us.net> wrote: > > On 7/24/20 12:15 AM, Joel Stanley wrote: > > From: Joachim Fenkes <FENKES@de.ibm.com> > > > > On BMCs with lower timer resolution than 1ms, msleep(1) will take > > way longer than 1ms, so looping 10k times won't wait for 10s but > > significantly longer. > > > > Fix this by using jiffies like the rest of the code. > > > > Fixes: 9f4a8a2d7f9d ("fsi/sbefifo: Add driver for the SBE FIFO") > > Signed-off-by: Joachim Fenkes <fenkes@de.ibm.com> > > Signed-off-by: Joel Stanley <joel@jms.id.au> > > --- > > drivers/fsi/fsi-sbefifo.c | 6 ++++-- > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/fsi/fsi-sbefifo.c b/drivers/fsi/fsi-sbefifo.c > > index 655b45c1f6ba..3ad9510ad4a4 100644 > > --- a/drivers/fsi/fsi-sbefifo.c > > +++ b/drivers/fsi/fsi-sbefifo.c > > @@ -325,6 +325,7 @@ static int sbefifo_up_write(struct sbefifo *sbefifo, __be32 word) > > static int sbefifo_request_reset(struct sbefifo *sbefifo) > > { > > struct device *dev = &sbefifo->fsi_dev->dev; > > + unsigned long end_time; > > u32 status, timeout; > > int rc; > > > > @@ -341,7 +342,8 @@ static int sbefifo_request_reset(struct sbefifo *sbefifo) > > } > > > > /* Wait for it to complete */ > > - for (timeout = 0; timeout < SBEFIFO_RESET_TIMEOUT; timeout++) { > > + end_time = jiffies + msecs_to_jiffies(SBEFIFO_RESET_TIMEOUT); > > + while (!time_after(jiffies, end_time)) { > > rc = sbefifo_regr(sbefifo, SBEFIFO_UP | SBEFIFO_STS, &status); > > if (rc) { > > dev_err(dev, "Failed to read UP fifo status during reset" > > @@ -355,7 +357,7 @@ static int sbefifo_request_reset(struct sbefifo *sbefifo) > > return 0; > > } > > > > - msleep(1); > > + cond_resched(); > > A hot loop ? Are you sure ? usleep_range() might make more sense here. Thanks for the review. Joachim, I can fix this up for you. Do you have a suggestion for how long to wait? What's the best case completion time for a sbe reset? We also have a hot loop in sbefifo_wait. I'm open to suggestions for how long that should delay for. Cheers, Joel
diff --git a/drivers/fsi/fsi-sbefifo.c b/drivers/fsi/fsi-sbefifo.c index 655b45c1f6ba..3ad9510ad4a4 100644 --- a/drivers/fsi/fsi-sbefifo.c +++ b/drivers/fsi/fsi-sbefifo.c @@ -325,6 +325,7 @@ static int sbefifo_up_write(struct sbefifo *sbefifo, __be32 word) static int sbefifo_request_reset(struct sbefifo *sbefifo) { struct device *dev = &sbefifo->fsi_dev->dev; + unsigned long end_time; u32 status, timeout; int rc; @@ -341,7 +342,8 @@ static int sbefifo_request_reset(struct sbefifo *sbefifo) } /* Wait for it to complete */ - for (timeout = 0; timeout < SBEFIFO_RESET_TIMEOUT; timeout++) { + end_time = jiffies + msecs_to_jiffies(SBEFIFO_RESET_TIMEOUT); + while (!time_after(jiffies, end_time)) { rc = sbefifo_regr(sbefifo, SBEFIFO_UP | SBEFIFO_STS, &status); if (rc) { dev_err(dev, "Failed to read UP fifo status during reset" @@ -355,7 +357,7 @@ static int sbefifo_request_reset(struct sbefifo *sbefifo) return 0; } - msleep(1); + cond_resched(); } dev_err(dev, "FIFO reset timed out\n");