Re: [git pull request] ACPI patches for 2.6.36.merge

Previous thread: [GIT PULL] sound fixes for 2.6.36 by Takashi Iwai on Sunday, August 15, 2010 - 5:45 am. (1 message)

Next thread: [RFD] Mechanical synchronization algorithms proofs (was: Re: [patch 1/2] x86_64 page fault NMI-safe) by Mathieu Desnoyers on Sunday, August 15, 2010 - 7:10 am. (2 messages)
From: Sedat Dilek
Date: Sunday, August 15, 2010 - 3:41 am

I pulled in release GIT-branch on top of 2.6.35-git16 (commit
5d7cb157025b3b4852f38e6e5e97d06ef12c1d78)

   $ git://git.kernel.org/pub/scm/linux/kernel/git/lenb/linux-acpi-2.6.git
release

Unfortunately, the build breaks:

[ build.log ]
drivers/acpi/power.c: In function ‘acpi_power_off_device’:
drivers/acpi/power.c:252: error: ‘ref’ undeclared (first use in this function)
drivers/acpi/power.c:252: error: (Each undeclared identifier is
reported only once
drivers/acpi/power.c:252: error: for each function it appears in.)
make[5]: *** [drivers/acpi/power.o] Error 1
make[4]: *** [drivers/acpi] Error 2
make[3]: *** [drivers] Error 2


- Sedat -
--

From: Linus Torvalds
Date: Sunday, August 15, 2010 - 11:15 am

What the heck is going on? That thing cannot have been tested AT ALL.
It comes from commit cfa806f05980 ("gcc-4.6: ACPI: fix unused but set
variables in ACPI"), and there is no way that code has ever been
compiled. There's no conditional compilation (except for not enabling
ACPI at all), and the declaration of 'ref' that the commit removes is
followed just a few lines later by the use.

So WTF?

I can merge this and fix it up, but I'm not going to. This thing
should never have been sent to me. It clearly had no testing at all. I
even looked at whether it could _possibly_ be some kind of odd "patch
applied with fuzz at the wrong place" issue, but that looks impossible
too (not to mention _still_ not being an excuse for not even trying to
compile the thing).

I understand when people don't notice compile errors that don't happen
for them (due to being architecture- or configuration-specific), but I
really don't see how that could _ever_ have been the case here.

I see Andrew in the sign-off chain, which surprises me. Maybe he just
passed on the patch blindly. But seriously, what the _hell_ is going
on here?

                              Linus
--

From: Andrew Morton
Date: Sunday, August 15, 2010 - 1:30 pm

I'd be suspecting that we have two patches both of which worked
separately but which broke when combined.  Is there some other patch in
that tree which adds a new reference to `ref' in acpi_power_seq_show()?
--

From: Linus Torvalds
Date: Sunday, August 15, 2010 - 2:04 pm

On Sun, Aug 15, 2010 at 1:30 PM, Andrew Morton

The offending patch isn't about acpi_power_seq_show(), it's about
acpi_power_off_device().

But that may well explain the breakage: there does seem to be an
unused ref in acpi_power_seq_show() (around line 556). It's just that
the patch in question doesn't remove that one, it removes the
very-much-used one in acpi_power_off_device() (line 256 or so).

And the contexts don't even match. Well, they do match to 2 lines. But
they're not very close. However, Len's tree does have a patch to just
remove all the procfs crap, so acpi_power_seq_show() has gone away,
which I guess explains how the subsequent patch was then applied to
something it should never have been applied to. Probably with GNU
patch that doesn't mind the fact that it doesn't match exactly (or
somebody used git and explicitly said "apply it with fuzz").

So that seems to explain how the patch got corrupted.

But nothing explains the fact that clearly _zero_ testing was done.
The tree was clearly never even compiled. Len apparently did a blind
patch application (or rebase, or whatever), and never bothered to
compile the end result afterwards, just sent it to me sight-unseen.

What does that say about the _rest_ of the patches? What does that say
about (lack of) -next testing?

                         Linus
--

From: Andrew Morton
Date: Sunday, August 15, 2010 - 6:21 pm

That code compiled OK in -mm which includes linux-next.  I assume that
some last-minute merging broke things.

--

From: Linus Torvalds
Date: Sunday, August 15, 2010 - 6:35 pm

On Sun, Aug 15, 2010 at 6:21 PM, Andrew Morton

Not tested at all. They were apparently just applied from some other
source. And not even safely, but with "they can apply with fuzz",
which is downright dangerous. And then the result saw neither a

There are _zero_ merge errors there. Go take a look at Len's tree.
It's a linear progression of patches AND THE PATCH IN LEN'S TREE IS
BUGGY.

It's that simple. It clearly never ever saw -next, because it _cannot_
have seen next. That particular commit can never have compiled.

Now, if you sent a patch that touched acpi_power_seq_show(), then the
error happened at Len's side. He took a patch, applied it with fuzz at
the wrong place, and sent the result to me without ever even compiling
it. And that doesn't make me happy.

I decided I should probably pull anyway, because at least the reason
for the bug was explained. And I want to get -rc1 out today. But I'm
grumbling, and judging from your emails, you didn't even look at what
Len sent me. There really are _no_ excuses.

                       Linus
--

From: Stephen Rothwell
Date: Sunday, August 15, 2010 - 7:43 pm

Yeah, it turned up in Len's tree that I fetch over the weekend.

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/
From: Len Brown
Date: Tuesday, August 24, 2010 - 3:35 pm

My bad.

Andrew was right -- I botched a modify/delete
merge conflict of two correct patches.
I did it at the last minute and then fed the wrong branch
("idle-release" instead of "release") to my build test,
send the code, and dropped of the net.  multiple mistakes.

Sorry for the sloppiness, I'll do better.

Upon the git merge conflict, it looks like I resorted to
pulling andi's original patch out of git, pushing it into
quilt on top of rui's delete; and then got fooled
by the refresh.  Should have caught that both by inspection
and by build...

On a positive note, the underlying reason for the conflict
was that we are succeeding in deleting a bunch of crufty code
that should not need to be maintained at all:-)

thanks,
Len Brown, Intel Open Source Technology Center
--

Previous thread: [GIT PULL] sound fixes for 2.6.36 by Takashi Iwai on Sunday, August 15, 2010 - 5:45 am. (1 message)

Next thread: [RFD] Mechanical synchronization algorithms proofs (was: Re: [patch 1/2] x86_64 page fault NMI-safe) by Mathieu Desnoyers on Sunday, August 15, 2010 - 7:10 am. (2 messages)