[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 at 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.
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.
>