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

[Greg KH]

On Sun, May 24, 2020 at 10:05:28AM -0700, Linus Torvalds wrote:
> On Sun, May 24, 2020 at 8:00 AM Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
> >
> > On Sat, May 23, 2020 at 11:14:28AM -0700, Linus Torvalds wrote:
> > >
> > > 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).
> 
> Note that the "obviously buggy or at least not documented" behavior of
> that commit 4ef12f719802 ("kobject: Make sure the parent does not get
> released before its children") that got reverted is true regardless.
> 
> Should the parent be released unconditionally (like that commit does),
> or should it be released only when kobject_del() was called when it
> had "state_in_sysfs" set?
> 
> Even if the problem Guenter reported was due to something else, that
> other change is a rather fundamental change and should at least be
> mentioned by the commit log.
> 
> It's entirely possible that the parent dropping should always be done,
> but the way it was done in that reverted commit it looked kind of
> accidental.

I'll revisit this and try to figure it out, but I think what we have
today is still correct.  The only "problem" that people were having with
the original code is the kobject_uevent() path walk when a parent was
gone before the child.  I've sent a patch to solve that problem, so we
"should" be ok.

Unfortunatly, it turns out that the owner of the kobject in question was
assuming that it could always reach the parent when things were being
cleaned up, but it was tearing things down in the backwards order.  So
even if I did move the logic around "correctly" in this patch, it still
died a horrible death (and there was other under-run errors reported as
well by other subsystems.)

So again, I think what we have today is ok.  But I'll beat on it for a
while this week to ensure that.  Time to start using the kunit test
framework for kobjects it seems :)

> > 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?
> 
> No, it's fundamentally how git works.
> 
> Remember: git does _not_ track "changes".
> 
> Any SCM that tracks changes to a file is fundamentally broken, for
> fundamental reasons. It mostly boils down to "what happens when the
> source of the change the same file in two branches is different".
> Think "rename to X" and "create X", and remember all the problems SVN
> has when that happens.
> 
> So no, git never _ever_ tracks "what changed". Instead, git
> fundamentally tracks "what is the state". The "change" is not
> fundamental, it's something that gets computed afterwards when you
> have a "before and after" state.

Doh, ok, that makes more sense.  It's just that a apply/revert sequence
does not happen a lot that I happen to notice this when digging through
the logs for fixes.

> If you want it all, use "git log --full-history", but then you will
> _really_ get the full history and a lot of pointless noise. And even
> then, things like "blame" won't waste time on following merges that
> made no difference in the end.

I'll use --full-history for now on when trying to dig up stable changes,
as that should help.  But ugh, you are right, there is a lot more noise
in there, loads of merge commits that shouldn't matter.  Will add
--no-merges to the line as well, and that helps out.

thanks,

greg k-h