Re: [PATCH 1/4] irqchip/gic-v3: Implement read polling with dedicated API

[Marc Zyngier]

On Thu, 18 Jan 2024 11:27:36 +0000,
Dawei Li <dawei.li@xxxxxxxxxxxx> wrote:
> 
> Kernel provide read*_poll_* API family to support looping based polling
> code pattern like below:
> while (...)
> {
> 	val = op(addr);
> 	condition = cond(val);
> 	if (condition)
> 		break;
> 
> 	/* Maybe some timeout handling stuff */
> 
> 	cpu_relax();
> 	udelay();
> }
> 
> As such, use readl_relaxed_poll_timeout_atomic() to implement atomic
> register polling logic in gic-v3.
> 
> Signed-off-by: Dawei Li <dawei.li@xxxxxxxxxxxx>
> ---
>  drivers/irqchip/irq-gic-v3.c | 27 ++++++++-------------------
>  1 file changed, 8 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> index 98b0329b7154..b9d9375a3434 100644
> --- a/drivers/irqchip/irq-gic-v3.c
> +++ b/drivers/irqchip/irq-gic-v3.c
> @@ -19,6 +19,7 @@
>  #include <linux/percpu.h>
>  #include <linux/refcount.h>
>  #include <linux/slab.h>
> +#include <linux/iopoll.h>
>  
>  #include <linux/irqchip.h>
>  #include <linux/irqchip/arm-gic-common.h>
> @@ -251,17 +252,11 @@ static inline void __iomem *gic_dist_base(struct irq_data *d)
>  
>  static void gic_do_wait_for_rwp(void __iomem *base, u32 bit)
>  {
> -	u32 count = 1000000;	/* 1s! */
> +	u32 val;
>  
> -	while (readl_relaxed(base + GICD_CTLR) & bit) {
> -		count--;
> -		if (!count) {
> -			pr_err_ratelimited("RWP timeout, gone fishing\n");
> -			return;
> -		}
> -		cpu_relax();
> -		udelay(1);
> -	}
> +	if (readl_relaxed_poll_timeout_atomic(base + GICD_CTLR,
> +		val, !(val & bit), 1, 1000000) == -ETIMEDOUT)

If you are doing this, please use a constant such as USEC_PER_SEC for
the timeout. And fix the alignment of the second line so that the
parameters are aligned vertically.

> +		pr_err_ratelimited("RWP timeout, gone fishing\n");
>  }
>  
>  /* Wait for completion of a distributor change */
> @@ -279,7 +274,6 @@ static void gic_redist_wait_for_rwp(void)
>  static void gic_enable_redist(bool enable)
>  {
>  	void __iomem *rbase;
> -	u32 count = 1000000;	/* 1s! */
>  	u32 val;
>  
>  	if (gic_data.flags & FLAGS_WORKAROUND_GICR_WAKER_MSM8996)
> @@ -301,14 +295,9 @@ static void gic_enable_redist(bool enable)
>  			return;	/* No PM support in this redistributor */
>  	}
>  
> -	while (--count) {
> -		val = readl_relaxed(rbase + GICR_WAKER);
> -		if (enable ^ (bool)(val & GICR_WAKER_ChildrenAsleep))
> -			break;
> -		cpu_relax();
> -		udelay(1);
> -	}
> -	if (!count)
> +	if (readl_relaxed_poll_timeout_atomic(rbase + GICR_WAKER,
> +		val, enable ^ (bool)(val & GICR_WAKER_ChildrenAsleep),
> +		1, 1000000) == -ETIMEDOUT)
>  		pr_err_ratelimited("redistributor failed to %s...\n",
>  				   enable ? "wakeup" : "sleep");
>  }

Same thing here.

	M.

-- 
Without deviation from the norm, progress is not possible.