* [PATCH] irqchip/gic-v3-its: handle queue wrapping in its_wait_for_range_completion() @ 2017-08-16 13:04 Yang Yingliang 2017-08-16 15:12 ` Marc Zyngier 0 siblings, 1 reply; 3+ messages in thread From: Yang Yingliang @ 2017-08-16 13:04 UTC (permalink / raw) To: linux-arm-kernel 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@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; -- 2.5.0 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* [PATCH] irqchip/gic-v3-its: handle queue wrapping in its_wait_for_range_completion() 2017-08-16 13:04 [PATCH] irqchip/gic-v3-its: handle queue wrapping in its_wait_for_range_completion() Yang Yingliang @ 2017-08-16 15:12 ` Marc Zyngier 2017-08-19 2:25 ` Yang Yingliang 0 siblings, 1 reply; 3+ messages in thread From: Marc Zyngier @ 2017-08-16 15:12 UTC (permalink / raw) To: linux-arm-kernel 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@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? 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? Thanks, M. -- Jazz is not dead. It just smells funny... ^ permalink raw reply related [flat|nested] 3+ messages in thread
* [PATCH] irqchip/gic-v3-its: handle queue wrapping in its_wait_for_range_completion() 2017-08-16 15:12 ` Marc Zyngier @ 2017-08-19 2:25 ` Yang Yingliang 0 siblings, 0 replies; 3+ messages in thread From: Yang Yingliang @ 2017-08-19 2:25 UTC (permalink / raw) To: linux-arm-kernel 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@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. > ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2017-08-19 2:25 UTC | newest] Thread overview: 3+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-08-16 13:04 [PATCH] irqchip/gic-v3-its: handle queue wrapping in its_wait_for_range_completion() Yang Yingliang 2017-08-16 15:12 ` Marc Zyngier 2017-08-19 2:25 ` Yang Yingliang
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox