Re: drm pull for v5.3-rc1

[Thomas HellstrÃm (VMware)]

Hi, All.

On 7/15/19 8:00 PM, Linus Torvalds wrote:
> On Mon, Jul 15, 2019 at 10:37 AM Linus Torvalds
> <torvalds@xxxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/dri-devel