| Message ID | 1453860843-5835-1-git-send-email-marex@denx.de |
|---|---|
| State | Accepted |
| Commit | fbb0de088b86a0f87e876531b5ae6077cc0ab14c |
| Delegated to: | Daniel Schwierzeck |
| Headers | show |
Am 27.01.2016 um 03:13 schrieb Marek Vasut: > This patch makes sure that the flush/invalidate_dcache_range() functions > can handle corner-case calls like this -- invalidate_dcache_range(0, 0, 0); > This call is valid and is happily produced by USB EHCI code for example. > The expected behavior of the cache function(s) in this case is that they > will do no operation, since the size is zero. > > The current implementation though has a problem where such invocation will > result in a hard CPU hang. This is because under such conditions, where the > start_addr = 0 and stop = 0, the addr = 0 and aend = 0xffffffe0 . The loop > will then try to iterate over the entire address space, which in itself is > wrong. But iterating over the entire address space might also hit some odd > address which will cause bus hang. The later happens on the Atheros MIPS. > > Signed-off-by: Marek Vasut <marex@denx.de> > Cc: Daniel Schwierzeck <daniel.schwierzeck@gmail.com> > Cc: Hans de Goede <hdegoede@redhat.com> > --- > arch/mips/lib/cache.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/arch/mips/lib/cache.c b/arch/mips/lib/cache.c > index bf8ff59..7482005 100644 > --- a/arch/mips/lib/cache.c > +++ b/arch/mips/lib/cache.c > @@ -95,6 +95,10 @@ void flush_dcache_range(ulong start_addr, ulong stop) > const void *addr = (const void *)(start_addr & ~(lsize - 1)); > const void *aend = (const void *)((stop - 1) & ~(lsize - 1)); > > + /* aend will be miscalculated when size is zero, so we return here */ > + if (start_addr == stop) > + return; > + you could additionally move the initialization of addr and aend behind this check like it's done in flush_cache()? That would save some CPU cycles. > while (1) { > mips_cache(HIT_WRITEBACK_INV_D, addr); > if (addr == aend) > @@ -109,6 +113,10 @@ void invalidate_dcache_range(ulong start_addr, ulong stop) > const void *addr = (const void *)(start_addr & ~(lsize - 1)); > const void *aend = (const void *)((stop - 1) & ~(lsize - 1)); > > + /* aend will be miscalculated when size is zero, so we return here */ > + if (start_addr == stop) > + return; > + > while (1) { > mips_cache(HIT_INVALIDATE_D, addr); > if (addr == aend) >
On Wednesday, January 27, 2016 at 03:56:36 PM, Daniel Schwierzeck wrote: > Am 27.01.2016 um 03:13 schrieb Marek Vasut: > > This patch makes sure that the flush/invalidate_dcache_range() functions > > can handle corner-case calls like this -- invalidate_dcache_range(0, 0, > > 0); This call is valid and is happily produced by USB EHCI code for > > example. The expected behavior of the cache function(s) in this case is > > that they will do no operation, since the size is zero. > > > > The current implementation though has a problem where such invocation > > will result in a hard CPU hang. This is because under such conditions, > > where the start_addr = 0 and stop = 0, the addr = 0 and aend = > > 0xffffffe0 . The loop will then try to iterate over the entire address > > space, which in itself is wrong. But iterating over the entire address > > space might also hit some odd address which will cause bus hang. The > > later happens on the Atheros MIPS. > > > > Signed-off-by: Marek Vasut <marex@denx.de> > > Cc: Daniel Schwierzeck <daniel.schwierzeck@gmail.com> > > Cc: Hans de Goede <hdegoede@redhat.com> > > --- > > > > arch/mips/lib/cache.c | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/arch/mips/lib/cache.c b/arch/mips/lib/cache.c > > index bf8ff59..7482005 100644 > > --- a/arch/mips/lib/cache.c > > +++ b/arch/mips/lib/cache.c > > @@ -95,6 +95,10 @@ void flush_dcache_range(ulong start_addr, ulong stop) > > > > const void *addr = (const void *)(start_addr & ~(lsize - 1)); > > const void *aend = (const void *)((stop - 1) & ~(lsize - 1)); > > > > + /* aend will be miscalculated when size is zero, so we return here */ > > + if (start_addr == stop) > > + return; > > + > > you could additionally move the initialization of addr and aend behind > this check like it's done in flush_cache()? That would save some CPU > cycles. The compiler would do the reordering anyway, it's not volatile so it's placement would not save any cycles. > > while (1) { > > > > mips_cache(HIT_WRITEBACK_INV_D, addr); > > if (addr == aend) > > > > @@ -109,6 +113,10 @@ void invalidate_dcache_range(ulong start_addr, ulong > > stop) > > > > const void *addr = (const void *)(start_addr & ~(lsize - 1)); > > const void *aend = (const void *)((stop - 1) & ~(lsize - 1)); > > > > + /* aend will be miscalculated when size is zero, so we return here */ > > + if (start_addr == stop) > > + return; > > + > > > > while (1) { > > > > mips_cache(HIT_INVALIDATE_D, addr); > > if (addr == aend) Best regards, Marek Vasut
2016-01-27 3:13 GMT+01:00 Marek Vasut <marex@denx.de>: > This patch makes sure that the flush/invalidate_dcache_range() functions > can handle corner-case calls like this -- invalidate_dcache_range(0, 0, 0); > This call is valid and is happily produced by USB EHCI code for example. > The expected behavior of the cache function(s) in this case is that they > will do no operation, since the size is zero. > > The current implementation though has a problem where such invocation will > result in a hard CPU hang. This is because under such conditions, where the > start_addr = 0 and stop = 0, the addr = 0 and aend = 0xffffffe0 . The loop > will then try to iterate over the entire address space, which in itself is > wrong. But iterating over the entire address space might also hit some odd > address which will cause bus hang. The later happens on the Atheros MIPS. > > Signed-off-by: Marek Vasut <marex@denx.de> > Cc: Daniel Schwierzeck <daniel.schwierzeck@gmail.com> > Cc: Hans de Goede <hdegoede@redhat.com> > --- > arch/mips/lib/cache.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > applied to u-boot-mips, thanks
On Monday, February 01, 2016 at 10:29:51 PM, Daniel Schwierzeck wrote: > 2016-01-27 3:13 GMT+01:00 Marek Vasut <marex@denx.de>: > > This patch makes sure that the flush/invalidate_dcache_range() functions > > can handle corner-case calls like this -- invalidate_dcache_range(0, 0, > > 0); This call is valid and is happily produced by USB EHCI code for > > example. The expected behavior of the cache function(s) in this case is > > that they will do no operation, since the size is zero. > > > > The current implementation though has a problem where such invocation > > will result in a hard CPU hang. This is because under such conditions, > > where the start_addr = 0 and stop = 0, the addr = 0 and aend = > > 0xffffffe0 . The loop will then try to iterate over the entire address > > space, which in itself is wrong. But iterating over the entire address > > space might also hit some odd address which will cause bus hang. The > > later happens on the Atheros MIPS. > > > > Signed-off-by: Marek Vasut <marex@denx.de> > > Cc: Daniel Schwierzeck <daniel.schwierzeck@gmail.com> > > Cc: Hans de Goede <hdegoede@redhat.com> > > --- > > > > arch/mips/lib/cache.c | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > applied to u-boot-mips, thanks Thanks! I'll pick the remaining four, ok ? Best regards, Marek Vasut
2016-02-01 22:31 GMT+01:00 Marek Vasut <marex@denx.de>: > On Monday, February 01, 2016 at 10:29:51 PM, Daniel Schwierzeck wrote: >> 2016-01-27 3:13 GMT+01:00 Marek Vasut <marex@denx.de>: >> > This patch makes sure that the flush/invalidate_dcache_range() functions >> > can handle corner-case calls like this -- invalidate_dcache_range(0, 0, >> > 0); This call is valid and is happily produced by USB EHCI code for >> > example. The expected behavior of the cache function(s) in this case is >> > that they will do no operation, since the size is zero. >> > >> > The current implementation though has a problem where such invocation >> > will result in a hard CPU hang. This is because under such conditions, >> > where the start_addr = 0 and stop = 0, the addr = 0 and aend = >> > 0xffffffe0 . The loop will then try to iterate over the entire address >> > space, which in itself is wrong. But iterating over the entire address >> > space might also hit some odd address which will cause bus hang. The >> > later happens on the Atheros MIPS. >> > >> > Signed-off-by: Marek Vasut <marex@denx.de> >> > Cc: Daniel Schwierzeck <daniel.schwierzeck@gmail.com> >> > Cc: Hans de Goede <hdegoede@redhat.com> >> > --- >> > >> > arch/mips/lib/cache.c | 8 ++++++++ >> > 1 file changed, 8 insertions(+) >> >> applied to u-boot-mips, thanks > > Thanks! I'll pick the remaining four, ok ? > fine with me. I don't want to break USB ;)
On Monday, February 01, 2016 at 10:40:26 PM, Daniel Schwierzeck wrote: > 2016-02-01 22:31 GMT+01:00 Marek Vasut <marex@denx.de>: > > On Monday, February 01, 2016 at 10:29:51 PM, Daniel Schwierzeck wrote: > >> 2016-01-27 3:13 GMT+01:00 Marek Vasut <marex@denx.de>: > >> > This patch makes sure that the flush/invalidate_dcache_range() > >> > functions can handle corner-case calls like this -- > >> > invalidate_dcache_range(0, 0, 0); This call is valid and is happily > >> > produced by USB EHCI code for example. The expected behavior of the > >> > cache function(s) in this case is that they will do no operation, > >> > since the size is zero. > >> > > >> > The current implementation though has a problem where such invocation > >> > will result in a hard CPU hang. This is because under such conditions, > >> > where the start_addr = 0 and stop = 0, the addr = 0 and aend = > >> > 0xffffffe0 . The loop will then try to iterate over the entire address > >> > space, which in itself is wrong. But iterating over the entire address > >> > space might also hit some odd address which will cause bus hang. The > >> > later happens on the Atheros MIPS. > >> > > >> > Signed-off-by: Marek Vasut <marex@denx.de> > >> > Cc: Daniel Schwierzeck <daniel.schwierzeck@gmail.com> > >> > Cc: Hans de Goede <hdegoede@redhat.com> > >> > --- > >> > > >> > arch/mips/lib/cache.c | 8 ++++++++ > >> > 1 file changed, 8 insertions(+) > >> > >> applied to u-boot-mips, thanks > > > > Thanks! I'll pick the remaining four, ok ? > > fine with me. I don't want to break USB ;) ;-) Best regards, Marek Vasut
diff --git a/arch/mips/lib/cache.c b/arch/mips/lib/cache.c index bf8ff59..7482005 100644 --- a/arch/mips/lib/cache.c +++ b/arch/mips/lib/cache.c @@ -95,6 +95,10 @@ void flush_dcache_range(ulong start_addr, ulong stop) const void *addr = (const void *)(start_addr & ~(lsize - 1)); const void *aend = (const void *)((stop - 1) & ~(lsize - 1)); + /* aend will be miscalculated when size is zero, so we return here */ + if (start_addr == stop) + return; + while (1) { mips_cache(HIT_WRITEBACK_INV_D, addr); if (addr == aend) @@ -109,6 +113,10 @@ void invalidate_dcache_range(ulong start_addr, ulong stop) const void *addr = (const void *)(start_addr & ~(lsize - 1)); const void *aend = (const void *)((stop - 1) & ~(lsize - 1)); + /* aend will be miscalculated when size is zero, so we return here */ + if (start_addr == stop) + return; + while (1) { mips_cache(HIT_INVALIDATE_D, addr); if (addr == aend)
This patch makes sure that the flush/invalidate_dcache_range() functions can handle corner-case calls like this -- invalidate_dcache_range(0, 0, 0); This call is valid and is happily produced by USB EHCI code for example. The expected behavior of the cache function(s) in this case is that they will do no operation, since the size is zero. The current implementation though has a problem where such invocation will result in a hard CPU hang. This is because under such conditions, where the start_addr = 0 and stop = 0, the addr = 0 and aend = 0xffffffe0 . The loop will then try to iterate over the entire address space, which in itself is wrong. But iterating over the entire address space might also hit some odd address which will cause bus hang. The later happens on the Atheros MIPS. Signed-off-by: Marek Vasut <marex@denx.de> Cc: Daniel Schwierzeck <daniel.schwierzeck@gmail.com> Cc: Hans de Goede <hdegoede@redhat.com> --- arch/mips/lib/cache.c | 8 ++++++++ 1 file changed, 8 insertions(+)