Hi Geert, Am 22.05.2018 um 20:18 schrieb Geert Uytterhoeven: > Hi Michael, > > On Mon, May 14, 2018 at 1:10 PM, Michael Schmitz <[email protected]> wrote: > > On kernels with 020/030 support ... > >> get_io_area leaves an IO_SIZE gap between mappings which is added to >> the vm_struct representing the mapping. __ioremap() uses the actual >> requested size (after alignment), while __iounmap() is passed the >> size from the vm_struct. >> >> On 020/030, early termination descriptors are used to set up mappings of >> extent 'size', which are validated on unmapping. The unmapped gap of size >> IO_SIZE defeats the sanity check of the pmd tables, causing __iounmap to >> loop forever on 030. >> >> On 040/040, unmapping of page table entries does not check for a valid > > 040/060
My bad ... >> mapping, so the umapping loop always completes there. >> >> Adjust size to be unmapped by the gap that had been added in the vm_struct >> prior. >> >> This fixes the hang in atari_platform_init() reported a long time ago, and >> a similar one reported by Finn recently (addressed by removing ioremap() >> use from the SWIM driver. >> >> Tested on my Falcon in 030 mode - untested but should work the same on >> 040/060 (the extra page tables cleared there would never have been set up >> anyway). >> >> Comment on whether the gap size should be considered in looking for a >> suitable address to place the next mapping in get_io_area() would be welcome. > > At first sight (and looking in full-history-linux git history), I see no > reason for the gap. I'd assume having a block with address and size aligned > to 256 KiB (which the caller already takes care of: IO_SIZE is 256 KiB if > 020/030 support is enabled) should be sufficient to use early termination > tables. My guess is that someone wanted to catch out of bounds accesses by leaving the unmapped areas in between ioremapped 256 kB chunks. The unmapped gaps must 256 kB as well to avoid disturbing the alignment. The adjustment for gap size was dropped sometime between 2.4.30 and 2.6.18. At the same time, a comment in get_io_area was removed that stated 'leave a gap of IO_SIZE'. I haven't looked at the full history to find out who removed that comment (and the adjustment). What I hoped for comments on is this: when searching for an available unmapped chunk in get_io_area(), the minimum size requirement is 'size', not 'size+IO_SIZE'. If we find an area that just fits the minimum size, can we end up with no gap between the new mapping and the following one? As far as I now can see, what get_io_area() does is safe. > Obviously removing the gap would fix the issue as well, but that's more > risky... Yep, even though the code doesn't seem to get used a lot I'd rather not audit all callers. >> Signed-off-by: Michael Schmitz <[email protected]> >> --- >> arch/m68k/mm/kmap.c | 3 ++- >> 1 files changed, 2 insertions(+), 1 deletions(-) >> >> diff --git a/arch/m68k/mm/kmap.c b/arch/m68k/mm/kmap.c >> index c2a3832..3b420f6 100644 >> --- a/arch/m68k/mm/kmap.c >> +++ b/arch/m68k/mm/kmap.c >> @@ -89,7 +89,8 @@ static inline void free_io_area(void *addr) >> for (p = &iolist ; (tmp = *p) ; p = &tmp->next) { >> if (tmp->addr == addr) { >> *p = tmp->next; >> - __iounmap(tmp->addr, tmp->size); >> + /* remove gap added in get_io_area() */ >> + __iounmap(tmp->addr, tmp->size - IO_SIZE); >> kfree(tmp); >> return; >> } > > Looks good to me, so I will apply and queue for v4.18 with the patch > description slightly modified, but will wait one more day, in case someone > has a comment. Thanks, let's see whether this helps some 030 users ... Cheers, Michael > Thanks a lot! > > Gr{oetje,eeting}s, > > Geert > -- To unsubscribe from this list: send the line "unsubscribe linux-m68k" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
