Re: [PATCH v2 2/3] fpga: microchip-spi: rewrite status polling in a time measurable way

[Ivan Bornyakov]

On Mon, Dec 26, 2022 at 08:24:06PM +0200, Ilpo Järvinen wrote:
> On Mon, 26 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.
> > 
> > Fixes: 5f8d4a900830 ("fpga: microchip-spi: add Microchip MPF FPGA manager")
> > Signed-off-by: Ivan Bornyakov <i.bornyakov@xxxxxxxxxxx>
> > ---
> >  drivers/fpga/microchip-spi.c | 25 +++++++++++--------------
> >  1 file changed, 11 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/fpga/microchip-spi.c b/drivers/fpga/microchip-spi.c
> > index e72fedd93a27..f3ddfab87499 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,17 @@ 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 || (status & mask))),
> 
> I'm just noting that this code does not match function's comment 
> (neither pre nor post diff code). The comment claims "mask bits are set" 
> but the code tests "any mask bit set". 
> 
> However, currently it causes no issue because only MPF_STATUS_READY is 
> passed and it's BIT(1).
> 
> I think the condition matching to the comment would be this (IMHO, it is 
> also slightly simpler because !mask doesn't need to be special cased):
> 
> ((status & (MPF_STATUS_BUSY | mask)) == mask)
> 
> (But not necessarily to be changed in this patch.)
> 
> -- 
>  i.
> 

Agree. Thank you for pointing out!

> > +				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)
> > 
>