Messages in this thread | | | Subject | Re: drm pull for v5.3-rc1 | From | Thomas Hellström (VMware) <> | Date | Mon, 15 Jul 2019 21:35:45 +0200 |
| |
Hi, All.
On 7/15/19 8:00 PM, Linus Torvalds wrote: > On Mon, Jul 15, 2019 at 10:37 AM Linus Torvalds > <torvalds@linux-foundation.org> wrote: >> I'm not pulling this. Why did you merge it into your tree, when >> apparently you were aware of how questionable it is judging by the drm >> pull request. > Looking at some of the fallout, I also see that you then added that > "adjust apply_to_pfn_range interface for dropped token" patch that > seems to be for easier merging of this all. > > But you remove the 'token' entirely in one place, and in another you > keep it and just say "whatever, it's unused, pass in NULL". WHAA? > > As part of looking at this all, I also note that some of this is also > very non-kernely. > > The whole thing with trying to implement a "closure" in C is simply > not how we do things in the kernel (although I've admittedly seen > signs of it in some drivers). > > If this should be done at all (and that's questionable), at least do > it in the canonical kernel way: pass in a separate "actor" function > pointer and an argument block, don't try to mix function pointers and > argument data and call it a "closure". > > We try to keep data and functions separate. It's not even for security > concerns (although those have caused some splits in the past - avoid > putting function pointers in structures that you then can't mark > read-only!), it's a more generic issue of just keeping arguments as > arguments - even if you then make a structure of them in order to not > make the calling convention very complicated. > > (Yes, we do have the pattern of sometimes mixing function pointers > with "describing data", ie the "struct file_operations" structure > isn't _just_ actual function pointers, it also contains the module > owner, for example. But those aren't about mixing function pointers > with their arguments, it's about basically "describing" an object > interface with more than just the operation pointers). > > So some of this code is stuff that I would have let go if it was in > some individual driver ("Closures? C doesn't have closures! But > whatever - that driver writer came from some place that taught lamda > calculus before techning C"). > > But in the core mm code, I want reviews. And I want the code to follow > normal kernel conventions.
Sorry for creating this mess, I guess I need to take another spin at this, but first I'd like to straighten out a few details:
- I've never had any kernel code more reviewed than this. It's been out on LKML and mm-list and maintainers I think 8 times including the RFC. The last time I was explicitly asking if anybody had any objections because I wanted to get it merged. It's not an internally-reviewed-only thing. There have been a number of people looking at the code and leaving comments and requesting fixes including Ralph Campbell, Jerome Glisse, Souptick Joarder, Nadav Amit and Christoph Hellwig. Perhaps I should have been more explicit in requesting R-Bs after fixing up all review comments, but I didn't. None of them had any issues similar to the ones you describe above.
- The combined callback / argument struct: It was strongly inspired by the struct mm_walk (mm.h), the page walk code being quite similar in functionality. "Closure" is perhaps a bad name. Originates in X server code.
Thanks, Thomas
> Linus > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
| |