Messages in this thread Patch in this message |  | | Date | Sun, 11 Feb 2018 18:45:04 +0000 | From | Marc Zyngier <> | Subject | Re: [PATCH] irqchip/gic-v3-its: handle rd_idx wrapping in its_wait_for_range_completion() |
| |
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.
|  |