irqchip/gic-v3-its: handle queue wrapping in its_wait_for_range_completion()
Subsystems (2)
PATCH 1/1 irqchip/gic-v3-its: handle queue wrapping in its_wait_for_range_completion()
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.
--- drivers/irqchip/irq-gic-v3-its.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
drivers/irqchip/irq-gic-v3-its.c +6 -1
@@ -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 ;
Signature · 1 line
2.5.0
PATCH 1/1 irqchip/gic-v3-its: handle queue wrapping in its_wait_for_range_completion()
Hi Yang,
Quoted 27 lines · Yang Yingliang
If the
to_idxwrap to 0, (rd_idx>=to_idx) will be true, but the command is not completed.When
to_idxis smaller thanfrom_idx, it means the queue is wrapped. For handling this case, addITS_CMD_QUEUE_SZtoto_idxandrd_idxto.Signed-off-by Yang Yingliang <[email protected]>--- drivers/irqchip/irq-gic-v3-its.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)drivers/irqchip/irq-gic-v3-its.c +2
-0@@ -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) {
drivers/irqchip/irq-gic-v3-its.c static void its_wait_for_range_completion(struct its_node *its, · line 463
Quoted 10 lines
drivers/irqchip/irq-gic-v3-its.c +4
-1+ 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?
drivers/irqchip/irq-gic-v3-its.c +7 -1
@@ -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?
Thanks,
M.
Signature · 1 line
Jazz is not dead. It just smells funny...
PATCH 1/1 irqchip/gic-v3-its: handle queue wrapping in its_wait_for_range_completion()
Hi, Marc
Quoted 33 lines · Marc Zyngier
Hi Yang,
Quoted 30 lines · Yang Yingliang
If the
to_idxwrap to 0, (rd_idx>=to_idx) will be true, but the command is not completed.When
to_idxis smaller thanfrom_idx, it means the queue is wrapped. For handling this case, addITS_CMD_QUEUE_SZtoto_idxandrd_idxto.Signed-off-by Yang Yingliang <[email protected]>--- drivers/irqchip/irq-gic-v3-its.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)drivers/irqchip/irq-gic-v3-its.c +5
-0@@ -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 ; + }
Quoted 11 lines
drivers/irqchip/irq-gic-v3-its.c static void its_wait_for_range_completion(struct its_node *its, · line 464
Quoted 7 lines
drivers/irqchip/irq-gic-v3-its.c +1
-1while (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.
Quoted 21 lines
drivers/irqchip/irq-gic-v3-its.c +7
-1@@ -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--;
Quoted 3 lines
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.
Quoted 5 lines
Thanks,
M.