Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 28 Dec 2022 14:50:01 +0000
From:   Conor Dooley <conor@kernel.org>
To:     Ivan Bornyakov <i.bornyakov@metrotek.ru>
Cc:     Conor Dooley <conor.dooley@microchip.com>,
        Moritz Fischer <mdf@kernel.org>, Wu Hao <hao.wu@intel.com>,
        Xu Yilun <yilun.xu@intel.com>, Tom Rix <trix@redhat.com>,
        Ilpo =?iso-8859-1?Q?J=E4rvinen?= <ilpo.jarvinen@linux.intel.com>,
        linux-fpga@vger.kernel.org, linux-kernel@vger.kernel.org,
        system@metrotek.ru
Subject: Re: [PATCH v3 2/3] fpga: microchip-spi: rewrite status polling in a
 time measurable way
Message-ID: <Y6xXmcCa4L9iz5qm@spud>
References: <20221227100450.2257-1-i.bornyakov@metrotek.ru>
 <20221227100450.2257-3-i.bornyakov@metrotek.ru>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha256;
        protocol="application/pgp-signature"; boundary="+k8pTdWXhUpj/t3q"
Content-Disposition: inline
In-Reply-To: <20221227100450.2257-3-i.bornyakov@metrotek.ru>
Precedence: bulk


--+k8pTdWXhUpj/t3q
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

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.
>=20
> 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).
>=20
> 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(-)
>=20
> 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 @@
> =20
>  #define	MPF_BITS_PER_COMPONENT_SIZE	22
> =20
> -#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 =3D MPF_STATUS_POLL_RETRIES;
> +	int ret, status;
> =20
> -	while (retries--) {
> -		status =3D 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 =3D read_poll_timeout(mpf_read_status, status,
> +				(status < 0) ||
> +				((status & (MPF_STATUS_BUSY | mask)) =3D=3D mask),
> +	if (ret < 0)
> +		return ret;
> =20
> -	return -EBUSY;
> +	return status;
>  }
> =20
>  static int mpf_spi_write(struct mpf_priv *priv, const void *buf, size_t =
buf_size)
> --=20
> 2.38.2
>=20
>=20

--+k8pTdWXhUpj/t3q
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iHUEABYIAB0WIQRh246EGq/8RLhDjO14tDGHoIJi0gUCY6xXmQAKCRB4tDGHoIJi
0ojBAQDaodmMFhTX1JcUQ7P9gZqx73+VFueROx+yv3Msvec1ngD/Q6BW5eZ1I22Y
Icgcw/pWs3wHFbUeZ4cIeuuc+wQtMwk=
=Qpli
-----END PGP SIGNATURE-----

--+k8pTdWXhUpj/t3q--