Re: drm pull for v5.3-rc1

[Dave Airlie]

On Tue, 16 Jul 2019 at 04:00, Linus Torvalds
<torvalds@xxxxxxxxxxxxxxxxxxxx> 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?

Not that I want to defend that code, but the mm patch that conflicts
already shows that removing the token is fine as nobody needs or
requires it. So the fixup patch in my tree was just a bridge to that patch,
which reduces conflicts. Rip the token out of the new API, pass it as NULL
to the old API until the mm patch is merged against it which drops the
token from the old API.

Dave.

>
> 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.
>
>                    Linus