Re: [GIT PULL] Driver core fixes for 5.7-rc7 - take 2

[Sasha Levin]

On Sun, May 24, 2020 at 05:00:18PM +0200, Greg KH wrote:
> On Sat, May 23, 2020 at 11:14:28AM -0700, Linus Torvalds wrote:
> > On Sat, May 23, 2020 at 8:29 AM Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
> > >
> > > The kobject patch that was originally in here has now been reverted, as
> > > Guenter reported boot problems with it on some of his systems.
> >
> > Hmm. That original patch looks obviously buggy: in kobject_cleanup()
> > it would end up doing "kobject_put(parent)" regardless of whether it
> > had actually done __kobject_del() or not.
> >
> > That _could_ have been intentional, but considering the commit
> > message, it clearly wasn't in this case.  It might be worth re-trying
> > to the commit, just with that fixed.
>
> Turns out that wasn't the real problem here, the culprit is the
> lib/test_printf.c code trying to tear down a kobject tree from the
> parent down to the children (i.e. in the backwards order).
>
> > Btw, when you end up reverting a patch that was already the top patch,
> > you might as well just remove it entirely from that tree instead (ie
> > "git reset --hard HEAD^" instead of "git revert HEAD").
> >
> > Unless somebody else uses your branches and you are afraid that the
> > non-reverted commit escaped out in the wild that way?
>
> I don't like rebasing or changing the HEAD like that on a public branch.
> As proof, syzbot started sending me a bunch of "this is the failed
> commit" messages right after your email, based on it's testing of the
> tree in linux-next.

OTOH, leaving commits like this may result in confusion later on because
of confusion around the "correct" patch.

Consider this:

1. Someone writes a patch named "close memory leak when freeing XYZ"
2. We revert it a day later with 'Revert "close memory leak when
freeing XYZ"'
3. Now, what would the author of the original patch do? That's right -
re-submit a patch with an identical subject line and patch description,
but with a subtle change in the code to fix the bug the original patch
was reverted for.

So now we end up with two "close memory leak when freeing XYZ" commits
in our git history that are nearly identical. Recipe for a disaster :)

> What is really odd now, is that 'git log lib/kobject.c' does not show
> the change/revert at all.  Is that because there was a revert?  Or is it
> a git config option/default somewhere that prevents that from showing
> up?
>
> Odd, 'git blame lib/kobject.c' doesn't show it either.  Yet e6764aa0e553
> ("Revert "kobject: Make sure the parent does not get released before its
> children"") is in your tree.  What am I missing here?

You need to use the '--follow' flag here:

$ git log -n3 --oneline --follow lib/kobject.c
e6764aa0e553 Revert "kobject: Make sure the parent does not get released before its children"
4ef12f719802 kobject: Make sure the parent does not get released before its children
122f8ec7b78e lib : kobject: fix refcount imblance on kobject_rename

$ git log -n3 --oneline lib/kobject.c
122f8ec7b78e lib : kobject: fix refcount imblance on kobject_rename
70e16a620e07 kobject: clean up the kobject add documentation a bit more
ed856349dc08 kobject: Fix kernel-doc comment first line

--
Thanks,
Sasha