| Message ID | 20221227100450.2257-3-i.bornyakov@metrotek.ru (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Reliability improvements for Microchip MPF FPGA manager | expand |
On Tue, 27 Dec 2022, Ivan Bornyakov wrote: > Original busy loop with retries count in mpf_poll_status() is not too > reliable, as it takes different times on different systems. Replace it > with read_poll_timeout() macro. > > While at it, fix polling stop condition to met function's original > intention declared in the comment. The issue with original polling stop > condition is that it stops if any of mask bits is set, while intention > was to stop if all mask bits is set. This was not noticible because only > MPF_STATUS_READY is passed as mask argument and it is BIT(1). > > Fixes: 5f8d4a900830 ("fpga: microchip-spi: add Microchip MPF FPGA manager") > Signed-off-by: Ivan Bornyakov <i.bornyakov@metrotek.ru> > --- > drivers/fpga/microchip-spi.c | 24 ++++++++++-------------- > 1 file changed, 10 insertions(+), 14 deletions(-) > > diff --git a/drivers/fpga/microchip-spi.c b/drivers/fpga/microchip-spi.c > index e72fedd93a27..8d1d9476d0cc 100644 > --- a/drivers/fpga/microchip-spi.c > +++ b/drivers/fpga/microchip-spi.c > @@ -6,6 +6,7 @@ > #include <asm/unaligned.h> > #include <linux/delay.h> > #include <linux/fpga/fpga-mgr.h> > +#include <linux/iopoll.h> > #include <linux/module.h> > #include <linux/of_device.h> > #include <linux/spi/spi.h> > @@ -33,7 +34,7 @@ > > #define MPF_BITS_PER_COMPONENT_SIZE 22 > > -#define MPF_STATUS_POLL_RETRIES 10000 > +#define MPF_STATUS_POLL_TIMEOUT (2 * USEC_PER_SEC) > #define MPF_STATUS_BUSY BIT(0) > #define MPF_STATUS_READY BIT(1) > #define MPF_STATUS_SPI_VIOLATION BIT(2) > @@ -197,21 +198,16 @@ static int mpf_ops_parse_header(struct fpga_manager *mgr, > /* Poll HW status until busy bit is cleared and mask bits are set. */ > static int mpf_poll_status(struct mpf_priv *priv, u8 mask) > { > - int status, retries = MPF_STATUS_POLL_RETRIES; > + int ret, status; > > - while (retries--) { > - status = mpf_read_status(priv); > - if (status < 0) > - return status; > - > - if (status & MPF_STATUS_BUSY) > - continue; > - > - if (!mask || (status & mask)) > - return status; > - } > + ret = read_poll_timeout(mpf_read_status, status, > + (status < 0) || > + ((status & (MPF_STATUS_BUSY | mask)) == mask), > + 0, MPF_STATUS_POLL_TIMEOUT, false, priv); > + if (ret < 0) > + return ret; > > - return -EBUSY; > + return status; > } > > static int mpf_spi_write(struct mpf_priv *priv, const void *buf, size_t buf_size) Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
On Tue, Dec 27, 2022 at 01:04:49PM +0300, Ivan Bornyakov wrote: > Original busy loop with retries count in mpf_poll_status() is not too > reliable, as it takes different times on different systems. Replace it > with read_poll_timeout() macro. > > While at it, fix polling stop condition to met function's original > intention declared in the comment. The issue with original polling stop > condition is that it stops if any of mask bits is set, while intention > was to stop if all mask bits is set. This was not noticible because only > MPF_STATUS_READY is passed as mask argument and it is BIT(1). > > Fixes: 5f8d4a900830 ("fpga: microchip-spi: add Microchip MPF FPGA manager") > Signed-off-by: Ivan Bornyakov <i.bornyakov@metrotek.ru> > --- > drivers/fpga/microchip-spi.c | 24 ++++++++++-------------- > 1 file changed, 10 insertions(+), 14 deletions(-) > > diff --git a/drivers/fpga/microchip-spi.c b/drivers/fpga/microchip-spi.c > index e72fedd93a27..8d1d9476d0cc 100644 > --- a/drivers/fpga/microchip-spi.c > +++ b/drivers/fpga/microchip-spi.c > @@ -6,6 +6,7 @@ > #include <asm/unaligned.h> > #include <linux/delay.h> > #include <linux/fpga/fpga-mgr.h> > +#include <linux/iopoll.h> > #include <linux/module.h> > #include <linux/of_device.h> > #include <linux/spi/spi.h> > @@ -33,7 +34,7 @@ > > #define MPF_BITS_PER_COMPONENT_SIZE 22 > > -#define MPF_STATUS_POLL_RETRIES 10000 > +#define MPF_STATUS_POLL_TIMEOUT (2 * USEC_PER_SEC) > #define MPF_STATUS_BUSY BIT(0) > #define MPF_STATUS_READY BIT(1) > #define MPF_STATUS_SPI_VIOLATION BIT(2) > @@ -197,21 +198,16 @@ static int mpf_ops_parse_header(struct fpga_manager *mgr, > /* Poll HW status until busy bit is cleared and mask bits are set. */ > static int mpf_poll_status(struct mpf_priv *priv, u8 mask) > { > - int status, retries = MPF_STATUS_POLL_RETRIES; > + int ret, status; > > - while (retries--) { > - status = mpf_read_status(priv); > - if (status < 0) > - return status; > - > - if (status & MPF_STATUS_BUSY) > - continue; > - > - if (!mask || (status & mask)) > - return status; > - } Would you mind moving the small comment before the function down to here and mentioning handling the failure case? The current implementation is a bit easier to understand than the new version that's wrapped in read_poll_timeout(). Otherwise, change seems like a good idea. Acked-by: Conor Dooley <conor.dooley@microchip.com> > + ret = read_poll_timeout(mpf_read_status, status, > + (status < 0) || > + ((status & (MPF_STATUS_BUSY | mask)) == mask), > + if (ret < 0) > + return ret; > > - return -EBUSY; > + return status; > } > > static int mpf_spi_write(struct mpf_priv *priv, const void *buf, size_t buf_size) > -- > 2.38.2 > >
diff --git a/drivers/fpga/microchip-spi.c b/drivers/fpga/microchip-spi.c index e72fedd93a27..8d1d9476d0cc 100644 --- a/drivers/fpga/microchip-spi.c +++ b/drivers/fpga/microchip-spi.c @@ -6,6 +6,7 @@ #include <asm/unaligned.h> #include <linux/delay.h> #include <linux/fpga/fpga-mgr.h> +#include <linux/iopoll.h> #include <linux/module.h> #include <linux/of_device.h> #include <linux/spi/spi.h> @@ -33,7 +34,7 @@ #define MPF_BITS_PER_COMPONENT_SIZE 22 -#define MPF_STATUS_POLL_RETRIES 10000 +#define MPF_STATUS_POLL_TIMEOUT (2 * USEC_PER_SEC) #define MPF_STATUS_BUSY BIT(0) #define MPF_STATUS_READY BIT(1) #define MPF_STATUS_SPI_VIOLATION BIT(2) @@ -197,21 +198,16 @@ static int mpf_ops_parse_header(struct fpga_manager *mgr, /* Poll HW status until busy bit is cleared and mask bits are set. */ static int mpf_poll_status(struct mpf_priv *priv, u8 mask) { - int status, retries = MPF_STATUS_POLL_RETRIES; + int ret, status; - while (retries--) { - status = mpf_read_status(priv); - if (status < 0) - return status; - - if (status & MPF_STATUS_BUSY) - continue; - - if (!mask || (status & mask)) - return status; - } + ret = read_poll_timeout(mpf_read_status, status, + (status < 0) || + ((status & (MPF_STATUS_BUSY | mask)) == mask), + 0, MPF_STATUS_POLL_TIMEOUT, false, priv); + if (ret < 0) + return ret; - return -EBUSY; + return status; } static int mpf_spi_write(struct mpf_priv *priv, const void *buf, size_t buf_size)
Original busy loop with retries count in mpf_poll_status() is not too reliable, as it takes different times on different systems. Replace it with read_poll_timeout() macro. While at it, fix polling stop condition to met function's original intention declared in the comment. The issue with original polling stop condition is that it stops if any of mask bits is set, while intention was to stop if all mask bits is set. This was not noticible because only MPF_STATUS_READY is passed as mask argument and it is BIT(1). Fixes: 5f8d4a900830 ("fpga: microchip-spi: add Microchip MPF FPGA manager") Signed-off-by: Ivan Bornyakov <i.bornyakov@metrotek.ru> --- drivers/fpga/microchip-spi.c | 24 ++++++++++-------------- 1 file changed, 10 insertions(+), 14 deletions(-)