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

[Dawei Li]

Hi Marc,

Thanks for the quick review.

On Thu, Jan 18, 2024 at 02:00:15PM +0000, Marc Zyngier wrote:
> 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.

Agreed, well defined constant is always preferable than magic number;

And yes, alignment is for better readability.

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

Ack.

I will address two issues above and send respin of V2.

Thanks,
	Dawei

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