Re: [GIT PULL] Please pull powerpc/linux.git powerpc-5.7-1 tag

[Michael Ellerman]

Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> writes:
> On Sun, Apr 5, 2020 at 5:53 AM Michael Ellerman <mpe@xxxxxxxxxxxxxx> wrote:
>>
>> There is one conflict in fs/sysfs/group.c, between our:
>>
>>   9255782f7061 ("sysfs: Wrap __compat_only_sysfs_link_entry_to_kobj function to change the symlink name")
> [...]
>
> The conflict was trivial.
>
> But I want to kvetch a bit about that commit. It's doing some odd stuff.
>
> In particular, it's wrapping things "the wrong way". Our naming rules
> are that the double underscore versions are the internal helper
> functions that you generally shouldn't use unless you have some extra
> reason for it, and then the non-underscore versions are the preferred
> and simpler user interface to those internal implementations.
>
> IOW, the _wrapper_ doesn't have double underscores, it's the _wrappee_
> that has the underscores.
>
> That commit does the exact reverse of that usual pattern, which is
> very confusing.
>
> Now, I see _why_ you do that - normally the non-underscore version is
> the "real" interface and the one we've always exported, and then the
> double underscore is the special internal thing that maybe exposes
> some internal detail (or maybe only does one special case of it and
> leaves out locking or whatever).
>
> In this case, for hysterical raisins, we only _had_ that
> double-underscore version, and you basically added the new case and
> did it without the underscores.
>
> So I see why it happened the way it did, but I do think the end result
> makes no sense and is odd and surprising.

Yeah, that's fair.

I was a bit unsure about taking a fs/sysfs patch to begin with, so I
thought leaving the existing function unchanged was the least risky in
terms of causing any other breakage.

But in hindsight that was the wrong choice, the end result is not
actually what we want.

So we should have done the right patch and then either asked Greg to
take it or put it in a topic branch of my own.

> The thing is, we have exactly *one* user of that double-underscore
> version: tpm-chip.c (ok, there are two calls in that file, but it's a
> single user).
>
> So I think it should just have removed the __ version entirely. Make
> tpm-chip just use the new semantics, and pass in the extra NULL
> argument.
>
> I guess I'll just do that as a cleanup patch on top, but it feels a
> bit odd to have to do that cleanup when the original patch could have
> just done the obvious thing.

Thanks.

cheers