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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Y66Wmxh0vWBMNMBU@yilunxu-OptiPlex-7050>
Date:   Fri, 30 Dec 2022 15:43:23 +0800
From:   Xu Yilun <yilun.xu@...el.com>
To:     Ivan Bornyakov <i.bornyakov@...rotek.ru>
Cc:     linux-fpga@...r.kernel.org,
        Conor Dooley <conor.dooley@...rochip.com>,
        Moritz Fischer <mdf@...nel.org>, Wu Hao <hao.wu@...el.com>,
        Tom Rix <trix@...hat.com>,
        Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
        linux-kernel@...r.kernel.org, system@...rotek.ru
Subject: Re: [PATCH v4 2/3] fpga: microchip-spi: rewrite status polling in a
 time measurable way

On 2022-12-29 at 13:46:03 +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@...rotek.ru>
> Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
> Acked-by: Conor Dooley <conor.dooley@...rochip.com>

Acked-by: Xu Yilun <yilun.xu@...el.com>

> ---
>  drivers/fpga/microchip-spi.c | 32 +++++++++++++++++---------------
>  1 file changed, 17 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/fpga/microchip-spi.c b/drivers/fpga/microchip-spi.c
> index e72fedd93a27..995b1964e0fe 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)
> @@ -194,24 +195,25 @@ static int mpf_ops_parse_header(struct fpga_manager *mgr,
>  	return 0;
>  }
>  
> -/* 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;
> -	}
> +	/*
> +	 * Busy poll HW status. Polling stops if any of the following
> +	 * conditions are met:
> +	 *  - timeout is reached
> +	 *  - mpf_read_status() returns an error
> +	 *  - busy bit is cleared AND mask bits are set
> +	 */
> +	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)
> -- 
> 2.39.0
> 
>