Re: [PATCH] irqchip/gic-v3-its: handle rd_idx wrapping in its_wait_for_range_completion()

[Yang Yingliang]

On 2018/2/12 2:45, Marc Zyngier wrote:
Hi, Marc

Sorry for replying so late.
> On Sun, 11 Feb 2018 03:42:01 +0000,
> Yang Yingliang wrote:
>
> Hi Yang,
>
> > In direct case, rd_idx will wrap if other cpus post commands
> > that make rd_idx increase. When rd_idx wrapped, the driver prints
> > timeout messages but in fact the command is finished. To handle
> > this case by adding last_rd_id to check rd_idx whether wrapped.
> >
> > Signed-off-by: Yang Yingliang <yangyingliang@xxxxxxxxxx>
> Please always Cc LKML on irqchip related patches.
>
> > ---
> >   drivers/irqchip/irq-gic-v3-its.c | 84 ++++++++++++++++++++++------------------
> >   1 file changed, 46 insertions(+), 38 deletions(-)
> >
> > diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
> > index 06f025f..d7176d1 100644
> > --- a/drivers/irqchip/irq-gic-v3-its.c
> > +++ b/drivers/irqchip/irq-gic-v3-its.c
> > @@ -713,7 +713,8 @@ static void its_flush_cmd(struct its_node *its, struct its_cmd_block *cmd)
> >   
> >   static int its_wait_for_range_completion(struct its_node *its,
> >   					 struct its_cmd_block *from,
> > -					 struct its_cmd_block *to)
> > +					 struct its_cmd_block *to,
> > +					 u64 last_rd_idx)
> >   {
> >   	u64 rd_idx, from_idx, to_idx;
> >   	u32 count = 1000000;	/* 1s! */
> > @@ -724,9 +725,14 @@ static int its_wait_for_range_completion(struct its_node *its,
> >   	while (1) {
> >   		rd_idx = readl_relaxed(its->base + GITS_CREADR);
> >   
> > -		/* Direct case */
> > -		if (from_idx < to_idx && rd_idx >= to_idx)
> > -			break;
> > +
> > +		/*
> > +		 * Direct case. In this case, rd_idx may wrapped,
> > +		 * because other cpus may post commands that make
> > +		 * rd_idx increase.
> > +		 */
> > +		if (from_idx < to_idx && (rd_idx >= to_idx || rd_idx < last_rd_idx))
> > +				break;
> >   
> >   		/* Wrapped case */
> >   		if (from_idx >= to_idx && rd_idx >= to_idx && rd_idx < from_idx)
> > @@ -746,40 +752,42 @@ static int its_wait_for_range_completion(struct its_node *its,
> [...]
>
> > +	last_rd_idx = readl_relaxed(its->base + GITS_CREADR);			\
> What is this last_rd_idx exactly? It is just some random sampling of
> the read pointer after we've posted our commands. It can still be in
> any position. And if the reader as wrapped because other CPUs are
> feeding more commands to the queue, it could just as well overtake
> last_rd_idx, making your new condition just as false. Yes, this is
> unlikely, but still.
>
> If you're going to harden the command queue wrapping, I'd suggest you
> implement a generation mechanism so that we can easily work out if
> we've seen the queue wrapping or not. THis would lift any kind of
> ambiguity once and for all.
I get a lot of timeout messages, when I am doing set affinity stress 
testing which
will post many its commands. I will try another way to handle the 
wrapped case.

Regards,
Yang
> Thanks,
>
> 	M.