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

[Marc Zyngier <marc.zyngier@arm.com>]

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@huawei.com>

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.

Thanks,

	M.

-- 
Jazz is not dead, it just smell funny.