[GIT PULL] Driver core fixes for 5.7-rc7

[GIT PULL] Driver core fixes for 5.7-rc7

[Greg KH]

The following changes since commit 2ef96a5bb12be62ef75b5828c0aab838ebb29cb8:

  Linux 5.7-rc5 (2020-05-10 15:16:58 -0700)

are available in the Git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core.git tags/driver-core-5.7-rc7

for you to fetch changes up to 4ef12f7198023c09ad6d25b652bd8748c965c7fa:

  kobject: Make sure the parent does not get released before its children (2020-05-21 11:01:27 +0200)

----------------------------------------------------------------
Driver core / kobject fixes for 5.7-rc7

Here are 3 small driver core and kobject fixes for 5.7-rc7

The kobject fix resolves a bug that the should not normally ever be hit,
but the kunit tests were hitting pretty regularly.  It's been reviewed
and tested by a bunch of people and stared at by me for a long time, so
it should be good.

The driver core fixes are small ones for reported problems with the
device link code that came in 5.7-rc1.

All of these have been in linux-next with no reported issues.

Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

----------------------------------------------------------------
Heikki Krogerus (1):
      kobject: Make sure the parent does not get released before its children

Saravana Kannan (2):
      driver core: Fix SYNC_STATE_ONLY device link implementation
      driver core: Fix handling of SYNC_STATE_ONLY + STATELESS device links

 drivers/base/core.c | 55 +++++++++++++++++++++++++++++++++++------------------
 lib/kobject.c       | 30 +++++++++++++++++++----------
 2 files changed, 57 insertions(+), 28 deletions(-)

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

[Greg KH]

On Sat, May 23, 2020 at 03:17:59PM +0200, Greg KH wrote:
> The following changes since commit 2ef96a5bb12be62ef75b5828c0aab838ebb29cb8:
> 
>   Linux 5.7-rc5 (2020-05-10 15:16:58 -0700)
> 
> are available in the Git repository at:
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core.git tags/driver-core-5.7-rc7
> 
> for you to fetch changes up to 4ef12f7198023c09ad6d25b652bd8748c965c7fa:
> 
>   kobject: Make sure the parent does not get released before its children (2020-05-21 11:01:27 +0200)
> 
> ----------------------------------------------------------------
> Driver core / kobject fixes for 5.7-rc7
> 
> Here are 3 small driver core and kobject fixes for 5.7-rc7
> 
> The kobject fix resolves a bug that the should not normally ever be hit,
> but the kunit tests were hitting pretty regularly.  It's been reviewed
> and tested by a bunch of people and stared at by me for a long time, so
> it should be good.

Nope, it isn't, Guenter reports runtime failures with this patch
applied.

So please don't pull this just yet...

thanks,

greg k-h

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

[Greg KH]

The following changes since commit 2ef96a5bb12be62ef75b5828c0aab838ebb29cb8:

  Linux 5.7-rc5 (2020-05-10 15:16:58 -0700)

are available in the Git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core.git tags/driver-core-5.7-rc7

for you to fetch changes up to e6764aa0e5530066dd969eccea2a1a7d177859a8:

  Revert "kobject: Make sure the parent does not get released before its children" (2020-05-23 17:11:11 +0200)

----------------------------------------------------------------
Driver core fixes for 5.7-rc7 - take 2

So, turns out the kobject fix didn't quite work, so here are 4 patches
that in the end, result in just 2 driver core fixes for reported issues
that no one has had problems with.

The kobject patch that was originally in here has now been reverted, as
Guenter reported boot problems with it on some of his systems.

Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

----------------------------------------------------------------
Greg Kroah-Hartman (1):
      Revert "kobject: Make sure the parent does not get released before its children"

Heikki Krogerus (1):
      kobject: Make sure the parent does not get released before its children

Saravana Kannan (2):
      driver core: Fix SYNC_STATE_ONLY device link implementation
      driver core: Fix handling of SYNC_STATE_ONLY + STATELESS device links

 drivers/base/core.c | 55 +++++++++++++++++++++++++++++++++++------------------
 1 file changed, 37 insertions(+), 18 deletions(-)

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

[Linus Torvalds]

On Sat, May 23, 2020 at 8:29 AM Greg KH <gregkh@linuxfoundation.org> 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.

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?

            Linus

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

[pr-tracker-bot]

The pull request you sent on Sat, 23 May 2020 15:17:59 +0200:

> git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core.git tags/driver-core-5.7-rc7

has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/e6764aa0e5530066dd969eccea2a1a7d177859a8

Thank you!

-- 
Deet-doot-dot, I am a bot.
https://korg.wiki.kernel.org/userdoc/prtracker

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

[pr-tracker-bot]

The pull request you sent on Sat, 23 May 2020 17:29:22 +0200:

> git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core.git tags/driver-core-5.7-rc7

has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/23f0dac848412dafd197566b62d831d5a68b5b6b

Thank you!

-- 
Deet-doot-dot, I am a bot.
https://korg.wiki.kernel.org/userdoc/prtracker

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

[Greg KH]

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@linuxfoundation.org> 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.

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?

thanks,

greg k-h

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

[Greg KH]

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@linuxfoundation.org> 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).

The fix for this is now posted here:
	https://lore.kernel.org/lkml/20200524153041.2361-1-gregkh@linuxfoundation.org/

along with a kobject change to emit the remove uevent when the object is
removed from sysfs (and still has a valid parent pointer), and not some
unspecified time in the future.

Let's see if people find this a better solution, and if so, I'll send it
to you later in the week.

thanks,

greg k-h

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@linuxfoundation.org> 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

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

[Linus Torvalds]

On Sun, May 24, 2020 at 8:00 AM Greg KH <gregkh@linuxfoundation.org> 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.

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

Why does that matter?

In the current git tree, when you start looking at the history of
lib/kobject.c, it looks at my merge of your tree, and goes "the
contents of that file were the same before and after the merge, so the
side history from you is clearly irrelevant".

And git is clearly right: your branch made changes to the file, but
then reverted them all, so clearly that branch doesn't matter. Git
will by default only show the simplified history - the part that
matters.

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.

(This, btw, is also true if your branch _did_ make real changes, but
the merge itself ended up throwing them away - either because somebody
undid them in the merge, or because the main development line had
those same changes already, so that the branch that got merged didn't
actually matter. Again, this comes from the fact that git tracks the
history of the full _state_ of the tree, not "these are the changes
done here").

Sasha mentioned "--follow", which also happens to show that commit,
but that's more of an incidental happenstance than anything else. "git
log --follow" is kind of a special case, where git stops doing some of
the pathname-based simplifying, because if the file shows up from
nothing, git will try to then figure out where it came from. The fact
that "--follow" this ends up not pruning irrelevant history as
aggressively is more of an implementation artifact than anything else.

So generally, don't use "--follow". It's kind of a hack to emulate
"track changes to a file", but it is a hack, and it fundamentally is a
bogus operation (for all the same reasons that the CVS/SCCS/SVN/etc
notion of a "file identity" is complete garbage and leads to
fundamental problems).

So "--follow" also can't handle multiple paths (or directories), and
is generally just a "placate people who don't understand why SVN is
wrong" option. It can be very useful in practice for the simple cases,
but it can also end up missing real changes in other situations.

              Linus

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

[Sasha Levin]

On Sun, May 24, 2020 at 10:05:28AM -0700, Linus Torvalds wrote:
>Sasha mentioned "--follow", which also happens to show that commit,
>but that's more of an incidental happenstance than anything else. "git
>log --follow" is kind of a special case, where git stops doing some of
>the pathname-based simplifying, because if the file shows up from
>nothing, git will try to then figure out where it came from. The fact
>that "--follow" this ends up not pruning irrelevant history as
>aggressively is more of an implementation artifact than anything else.
>
>So generally, don't use "--follow". It's kind of a hack to emulate
>"track changes to a file", but it is a hack, and it fundamentally is a
>bogus operation (for all the same reasons that the CVS/SCCS/SVN/etc
>notion of a "file identity" is complete garbage and leads to
>fundamental problems).

Interesting. My thinking around --follow was that it's like
--full-history in the sense that it won't prune history, but it would
also keep listing history beyond file renames.

The --follow functionality is quite useful when looking at older
branches and trying to understand where changes should go into on those
older branches.

We also do have some notion of "file identity" in the kernel; it's
prevalent with "quirk files". Look at these for example:

  - drivers/mmc/core/quirks.h
  - sound/pci/hda/patch_*.c
  - drivers/hid/hid-quirks.c

We know that patches to those files are likely to contain quirks (which
we usually want to take into the Stable branches) so I might have a
script that monitors a list of these "special" files in which case I
need to see a complete list of commits that went into those files.

-- 
Thanks,
Sasha

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

[Sasha Levin]

On Sun, May 24, 2020 at 03:45:50PM -0400, Sasha Levin wrote:
>On Sun, May 24, 2020 at 10:05:28AM -0700, Linus Torvalds wrote:
>>Sasha mentioned "--follow", which also happens to show that commit,
>>but that's more of an incidental happenstance than anything else. "git
>>log --follow" is kind of a special case, where git stops doing some of
>>the pathname-based simplifying, because if the file shows up from
>>nothing, git will try to then figure out where it came from. The fact
>>that "--follow" this ends up not pruning irrelevant history as
>>aggressively is more of an implementation artifact than anything else.
>>
>>So generally, don't use "--follow". It's kind of a hack to emulate
>>"track changes to a file", but it is a hack, and it fundamentally is a
>>bogus operation (for all the same reasons that the CVS/SCCS/SVN/etc
>>notion of a "file identity" is complete garbage and leads to
>>fundamental problems).
>
>Interesting. My thinking around --follow was that it's like
>--full-history in the sense that it won't prune history, but it would
>also keep listing history beyond file renames.
>
>The --follow functionality is quite useful when looking at older
>branches and trying to understand where changes should go into on those
>older branches.
>
>We also do have some notion of "file identity" in the kernel; it's
>prevalent with "quirk files". Look at these for example:
>
> - drivers/mmc/core/quirks.h
> - sound/pci/hda/patch_*.c
> - drivers/hid/hid-quirks.c
>
>We know that patches to those files are likely to contain quirks (which
>we usually want to take into the Stable branches) so I might have a
>script that monitors a list of these "special" files in which case I
>need to see a complete list of commits that went into those files.

(and I'd like to see the reverts too, so that I could apply that revert
to Stable trees as well. If a revert doesn't show up in git log we might
miss doing a backport of it).

-- 
Thanks,
Sasha

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

[Linus Torvalds]

On Sun, May 24, 2020 at 12:45 PM Sasha Levin <sashal@kernel.org> wrote:
>
> Interesting. My thinking around --follow was that it's like
> --full-history in the sense that it won't prune history, but it would
> also keep listing history beyond file renames.

No. It's only completely accidentally like full-history because it
sets the flag that basically says "give me the whole diff" - so that
if the file goes away, you see where it came from.

And because it wants the whole diff and doesn't limit it to just the
one file that is tracked, it ends up following both sides of the merge
because _other_ files changed in that merge.

> The --follow functionality is quite useful when looking at older
> branches and trying to understand where changes should go into on those
> older branches.

It is useful, but it is ambiguous. What happens if the file came to be
two different ways in two different branches? Or what happens if two
files were combined into one?

So "git log --follow" is not _wrong_, but the operation of trying to
follow a file identity is basically broken. In git, it's not a
fundamental operation (because git isn't broken), it's just an
emulation of that broken concept that often works in practice.

It's a "let's give people what they are used to", but it really isn't
very well-defined in the general case. You think it works, because for
the simple cases it gives the "obviously correct" answer.

> We also do have some notion of "file identity" in the kernel;

No, we really really don't.

The CVS/SVN kind of "file identity" is more like an "inode". Nothing
in the kernel sources cares about the inode number of a file. The
inode will be different depending on how something was created, and
when you rename what previously were two different files to one single
path (as a result of a merge), you have to pick one at random, and
lose the other.

So you end up with the crazy random "Attic" model of stale files in
CVS, exactly because the thing is based on a file identity that is
completely fundamentally broken.

Note how you've never seen anything like that in git. Because the
whole concept is garbage, and git isn't garbage.

Yes, I still hate CVS with a passion, almost two decades after I had
to use that horrid horrid thing. Some mental scars will  not go away.

>i t's prevalent with "quirk files". Look at these for example:
> [ deleted]
> We know that patches to those files are likely to contain quirks

No, those are not file identities AT ALL.

Those are just pathnames with some meaning. You can throw away the
file, and start a new one, and the meaning doesn't go away - because
it's attached to the path.

And yes, certain paths in the repository can be special, although
that's irrelevant to a SCM, of course. Git won't care. It's just
"contents with a name".

Which is exactly what git tracks, and is *not* what the SVN/CVS kind
of completely broken file identity is all about.

          Linus

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

[Linus Torvalds]

On Sun, May 24, 2020 at 2:12 PM Sasha Levin <sashal@kernel.org> wrote:
>
> (and I'd like to see the reverts too, so that I could apply that revert
> to Stable trees as well. If a revert doesn't show up in git log we might
> miss doing a backport of it).

Plain "git log" never simplifies anything at all.

Only when you ask for simplification will "git log" start skipping things.

That's things like "--grep=XYZ" to only ask for something that has a
pattern in the commit log. Or asking for a certain author. That's the
simplest kind of log simplification.

But saying "I'm only interested in changes to this pathname" is
another "please give me simplified history, only as it is relevant to
this pathname". And then it does exactly that. Including pruning out
whole branches that aren't relevant.

Trust me, it's the behavior you want. There's a reason we have
"--full-history", but it's not enabled by default.

But if you do want full history, you can still say so.

              Linus

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

[Greg KH]

On Sun, May 24, 2020 at 11:42:19AM -0400, Sasha Levin wrote:
> 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@linuxfoundation.org> 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"'

And the sha1 is in the commit, showing which patch was reverted.

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

Sometimes, yes, but sometimes, as in this case, a totally different
patch will be submitted for the problem :)

But, even if it was there, the sha1 in the revert should allow us to
track this properly.  I can't remember a time where this has caused
problems in the past, can you?

> 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 :)

Time and sha1 should show them being different :)

thanks,

greg k-h

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@linuxfoundation.org> 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