| Message ID | 1431644545-31904-1-git-send-email-stuart.yoder@freescale.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Stuart, On 15/05/15 00:02, Stuart Yoder wrote: > its_alloc_tables() needs to account for page sizes other than > 64KB. Without this change, when PAGE_SIZE=4KB its_alloc_tables() > gets stuck in an infinite loop. > > Signed-off-by: Stuart Yoder <stuart.yoder@freescale.com> > --- > > think this should go into 4.1 if at all possible...without it I am > unable to boot a 4.1 kernel on the LS2085 SoC What you are suggesting here is a effectively a revert of commit 790b57a, which would break other implementations. Can you please explain the actual issue? I'm failing to see how you end up in an infinite loop here (the system page size and the ITS base granule should be completely unrelated...). Or has it anything to do with Minghuan Lian's patch (https://lkml.org/lkml/2015/4/16/36), which looks more correct (if still massively under-documented)? Thanks, M. > > drivers/irqchip/irq-gic-v3-its.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c > index 9687f8a..58a6612 100644 > --- a/drivers/irqchip/irq-gic-v3-its.c > +++ b/drivers/irqchip/irq-gic-v3-its.c > @@ -800,7 +800,7 @@ static int its_alloc_tables(struct its_node *its) > { > int err; > int i; > - int psz = SZ_64K; > + int psz = PAGE_SIZE; > u64 shr = GITS_BASER_InnerShareable; > u64 cache = GITS_BASER_WaWb; > >
> -----Original Message----- > From: Marc Zyngier [mailto:marc.zyngier@arm.com] > Sent: Monday, May 18, 2015 8:23 AM > To: Yoder Stuart-B08248; tglx@linutronix.de > Cc: linux-arm-kernel@lists.infradead.org > Subject: Re: [PATCH] irqchip: GICv3: ITS: don't assume 64K page size in its_alloc_tables > > Hi Stuart, > > On 15/05/15 00:02, Stuart Yoder wrote: > > its_alloc_tables() needs to account for page sizes other than > > 64KB. Without this change, when PAGE_SIZE=4KB its_alloc_tables() > > gets stuck in an infinite loop. > > > > Signed-off-by: Stuart Yoder <stuart.yoder@freescale.com> > > --- > > > > think this should go into 4.1 if at all possible...without it I am > > unable to boot a 4.1 kernel on the LS2085 SoC > > What you are suggesting here is a effectively a revert of commit > 790b57a, which would break other implementations. > > Can you please explain the actual issue? I'm failing to see how you end > up in an infinite loop here (the system page size and the ITS base > granule should be completely unrelated...). Here is the problem line: val |= (alloc_size / psz) - 1; In our case: alloc_size=16K psz=64K ...so (alloc_size / psz) = 0, and thus val becomes -1, and everything is screwed up. We get stuck in a loop to retry_baser: Thanks, Stuart
On 18/05/15 14:38, Stuart Yoder wrote: > > >> -----Original Message----- >> From: Marc Zyngier [mailto:marc.zyngier@arm.com] >> Sent: Monday, May 18, 2015 8:23 AM >> To: Yoder Stuart-B08248; tglx@linutronix.de >> Cc: linux-arm-kernel@lists.infradead.org >> Subject: Re: [PATCH] irqchip: GICv3: ITS: don't assume 64K page size in its_alloc_tables >> >> Hi Stuart, >> >> On 15/05/15 00:02, Stuart Yoder wrote: >>> its_alloc_tables() needs to account for page sizes other than >>> 64KB. Without this change, when PAGE_SIZE=4KB its_alloc_tables() >>> gets stuck in an infinite loop. >>> >>> Signed-off-by: Stuart Yoder <stuart.yoder@freescale.com> >>> --- >>> >>> think this should go into 4.1 if at all possible...without it I am >>> unable to boot a 4.1 kernel on the LS2085 SoC >> >> What you are suggesting here is a effectively a revert of commit >> 790b57a, which would break other implementations. >> >> Can you please explain the actual issue? I'm failing to see how you end >> up in an infinite loop here (the system page size and the ITS base >> granule should be completely unrelated...). > > Here is the problem line: > > val |= (alloc_size / psz) - 1; > > In our case: > alloc_size=16K > psz=64K > > ...so (alloc_size / psz) = 0, and thus val becomes -1, and everything > is screwed up. We get stuck in a loop to retry_baser: If alloc_size is 16k, you have an order of 2, and I have to assume this is an allocation for a device table (otherwise order would be 4). So things fail because we've computed an alloc_size smaller than what we want to allocate as a minimum. Isn't that exactly what Minghuan's patch fixes? Thanks, M.
> -----Original Message----- > From: Marc Zyngier [mailto:marc.zyngier@arm.com] > Sent: Monday, May 18, 2015 9:09 AM > To: Yoder Stuart-B08248; tglx@linutronix.de > Cc: linux-arm-kernel@lists.infradead.org > Subject: Re: [PATCH] irqchip: GICv3: ITS: don't assume 64K page size in its_alloc_tables > > On 18/05/15 14:38, Stuart Yoder wrote: > > > > > >> -----Original Message----- > >> From: Marc Zyngier [mailto:marc.zyngier@arm.com] > >> Sent: Monday, May 18, 2015 8:23 AM > >> To: Yoder Stuart-B08248; tglx@linutronix.de > >> Cc: linux-arm-kernel@lists.infradead.org > >> Subject: Re: [PATCH] irqchip: GICv3: ITS: don't assume 64K page size in its_alloc_tables > >> > >> Hi Stuart, > >> > >> On 15/05/15 00:02, Stuart Yoder wrote: > >>> its_alloc_tables() needs to account for page sizes other than > >>> 64KB. Without this change, when PAGE_SIZE=4KB its_alloc_tables() > >>> gets stuck in an infinite loop. > >>> > >>> Signed-off-by: Stuart Yoder <stuart.yoder@freescale.com> > >>> --- > >>> > >>> think this should go into 4.1 if at all possible...without it I am > >>> unable to boot a 4.1 kernel on the LS2085 SoC > >> > >> What you are suggesting here is a effectively a revert of commit > >> 790b57a, which would break other implementations. > >> > >> Can you please explain the actual issue? I'm failing to see how you end > >> up in an infinite loop here (the system page size and the ITS base > >> granule should be completely unrelated...). > > > > Here is the problem line: > > > > val |= (alloc_size / psz) - 1; > > > > In our case: > > alloc_size=16K > > psz=64K > > > > ...so (alloc_size / psz) = 0, and thus val becomes -1, and everything > > is screwed up. We get stuck in a loop to retry_baser: > > If alloc_size is 16k, you have an order of 2, and I have to assume this > is an allocation for a device table (otherwise order would be 4). So > things fail because we've computed an alloc_size smaller than what we > want to allocate as a minimum. > > Isn't that exactly what Minghuan's patch fixes? Yes. I'll get with Minghuan and between the 2 of us we'll get a properly commented version of his patch sent out. Thanks, Stuart
On 18/05/15 16:33, Stuart Yoder wrote: > > >> -----Original Message----- >> From: Marc Zyngier [mailto:marc.zyngier@arm.com] >> Sent: Monday, May 18, 2015 9:09 AM >> To: Yoder Stuart-B08248; tglx@linutronix.de >> Cc: linux-arm-kernel@lists.infradead.org >> Subject: Re: [PATCH] irqchip: GICv3: ITS: don't assume 64K page size in its_alloc_tables >> >> On 18/05/15 14:38, Stuart Yoder wrote: >>> >>> >>>> -----Original Message----- >>>> From: Marc Zyngier [mailto:marc.zyngier@arm.com] >>>> Sent: Monday, May 18, 2015 8:23 AM >>>> To: Yoder Stuart-B08248; tglx@linutronix.de >>>> Cc: linux-arm-kernel@lists.infradead.org >>>> Subject: Re: [PATCH] irqchip: GICv3: ITS: don't assume 64K page size in its_alloc_tables >>>> >>>> Hi Stuart, >>>> >>>> On 15/05/15 00:02, Stuart Yoder wrote: >>>>> its_alloc_tables() needs to account for page sizes other than >>>>> 64KB. Without this change, when PAGE_SIZE=4KB its_alloc_tables() >>>>> gets stuck in an infinite loop. >>>>> >>>>> Signed-off-by: Stuart Yoder <stuart.yoder@freescale.com> >>>>> --- >>>>> >>>>> think this should go into 4.1 if at all possible...without it I am >>>>> unable to boot a 4.1 kernel on the LS2085 SoC >>>> >>>> What you are suggesting here is a effectively a revert of commit >>>> 790b57a, which would break other implementations. >>>> >>>> Can you please explain the actual issue? I'm failing to see how you end >>>> up in an infinite loop here (the system page size and the ITS base >>>> granule should be completely unrelated...). >>> >>> Here is the problem line: >>> >>> val |= (alloc_size / psz) - 1; >>> >>> In our case: >>> alloc_size=16K >>> psz=64K >>> >>> ...so (alloc_size / psz) = 0, and thus val becomes -1, and everything >>> is screwed up. We get stuck in a loop to retry_baser: >> >> If alloc_size is 16k, you have an order of 2, and I have to assume this >> is an allocation for a device table (otherwise order would be 4). So >> things fail because we've computed an alloc_size smaller than what we >> want to allocate as a minimum. >> >> Isn't that exactly what Minghuan's patch fixes? > > Yes. I'll get with Minghuan and between the 2 of us we'll get a properly > commented version of his patch sent out. Thanks Stuart. M.
diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c index 9687f8a..58a6612 100644 --- a/drivers/irqchip/irq-gic-v3-its.c +++ b/drivers/irqchip/irq-gic-v3-its.c @@ -800,7 +800,7 @@ static int its_alloc_tables(struct its_node *its) { int err; int i; - int psz = SZ_64K; + int psz = PAGE_SIZE; u64 shr = GITS_BASER_InnerShareable; u64 cache = GITS_BASER_WaWb;
its_alloc_tables() needs to account for page sizes other than 64KB. Without this change, when PAGE_SIZE=4KB its_alloc_tables() gets stuck in an infinite loop. Signed-off-by: Stuart Yoder <stuart.yoder@freescale.com> --- think this should go into 4.1 if at all possible...without it I am unable to boot a 4.1 kernel on the LS2085 SoC drivers/irqchip/irq-gic-v3-its.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)