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

[Yang Yingliang]

Hi, Marc

On 2017/8/16 23:12, Marc Zyngier wrote:
> Hi Yang,
>
> On 16/08/17 14:04, Yang Yingliang wrote:
>> If the to_idx wrap to 0, (rd_idx >= to_idx) will be true,
>> but the command is not completed.
>>
>> When to_idx is smaller than from_idx, it means the queue
>> is wrapped. For handling this case, add ITS_CMD_QUEUE_SZ
>> to to_idx and rd_idx to.
>>
>> Signed-off-by: Yang Yingliang <yangyingliang at huawei.com>
>> ---
>>   drivers/irqchip/irq-gic-v3-its.c | 7 ++++++-
>>   1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
>> index 6893287..8e47515 100644
>> --- a/drivers/irqchip/irq-gic-v3-its.c
>> +++ b/drivers/irqchip/irq-gic-v3-its.c
>> @@ -446,13 +446,18 @@ static void its_wait_for_range_completion(struct its_node *its,
>>   					  struct its_cmd_block *to)
>>   {
>>   	u64 rd_idx, from_idx, to_idx;
>> +	u32 wrap_offset = 0;
>>   	u32 count = 1000000;	/* 1s! */
>>
>>   	from_idx = its_cmd_ptr_to_offset(its, from);
>>   	to_idx = its_cmd_ptr_to_offset(its, to);
>> +	if (to_idx < from_idx) {
>> +		wrap_offset = ITS_CMD_QUEUE_SZ;
>> +		to_idx += ITS_CMD_QUEUE_SZ;
>> +	}
>>
>>   	while (1) {
>> -		rd_idx = readl_relaxed(its->base + GITS_CREADR);
>> +		rd_idx = readl_relaxed(its->base + GITS_CREADR) + wrap_offset;
>>   		if (rd_idx >= to_idx || rd_idx < from_idx)
>>   			break;
>>
>>
>
> I think you've spotted a real issue, but the way you solve it hurts my
> brain. I'd rather have something that makes the cases explicit. Does the
> following work for you?
Yes, it works for me.

>
> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
> index 22e228500357..374977681384 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -453,7 +453,13 @@ static void its_wait_for_range_completion(struct its_node *its,
>
>   	while (1) {
>   		rd_idx = readl_relaxed(its->base + GITS_CREADR);
> -		if (rd_idx >= to_idx || rd_idx < from_idx)
> +
> +		/* Direct case */
> +		if (from_idx < to_idx && rd_idx >= to_idx)
> +			break;
> +
> +		/* Wrapped case */
> +		if (from_idx >= to_idx && rd_idx >= to_idx && rd_idx < from_idx)
>   			break;
>
>   		count--;
>
> Secondary question: have you seen this breaking in practice? The worse
> case seems that we exit early, and potentially fail to detect a hung
> ITS. But that should never happen, right?
Yes, you are right.
>
> Thanks,
>
> 	M.
>