linux-kernel - Re: [GIT PULL] PCI changes for v5.15

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Tue, 7 Sep 2021 19:52:43 -0700
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Bjorn Helgaas <helgaas@...nel.org>
Cc:     David Miller <davem@...emloft.net>,
        Linux PCI <linux-pci@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Lorenzo Pieralisi <lorenzo.pieralisi@....com>
Subject: Re: [GIT PULL] PCI changes for v5.15

On Tue, Sep 7, 2021 at 7:26 PM Bjorn Helgaas <helgaas@...nel.org> wrote:
>
> I was a little mystified about why there was a conflict at all, since
> I expected those patches to apply cleanly on top of the revert, and I
> should have investigated that more instead of just chalking it up to
> my lack of git-fu.

It's because that wasn't the only change to that function.

So for example, the networking tree had that "bnx2: Search VPD with
pci_vpd_find_ro_info_keyword()" commit, and then reverted it.

Fine - that's a no-op, right? So then the fact that the PCI tree had
that change - and other changes - should just merge cleanly.

Except the networking tree *also* had "bnx2: Replace open-coded
version with swab32s()", and that wasn't reverted.

End result: the networkling tree and the PCI tree touched the exact
same code, and did it differently.  So - conflict.

And the bnxt.c case is similar, except there the networking tree _did_
revert the "other" commit too, but it seems to have actively done
something else as well. See how the networking tree actually has that
"This reverts commit 58a9b5d2621e725526a63847ae77b3a4c2c2bf93"
_twice_, because it did it wrong.

Anyway, because of that "revert twice", my tree actually had (before
your pull) that

        i = pci_vpd_find_tag(vpd_data, vpd_size, PCI_VPD_LRDT_RO_DATA);
        if (i < 0) {
                netdev_err(bp->dev, "VPD READ-Only not found\n");
                goto exit;
        }

code duplicated twice, so now the conflict was due to that.

And the thing is, the revert for some merge reason is always the wrong
thing to do, but it's doubly wrong when it's done badly.

I'd *much* rather see a few more conflicts, and then go "oh, tree X
and Y both did Z, but Y also did ABC". That's a very straightforward
conflict, and I can go "I'll take the Y side" because it's clearly and
unambiguously the "superset" of the changes.

The revert actually made things harder. Now neither branch had a
superset of changes, and the networking side in particular looked like
the original change had actually been in error, and should be reverted
entirely (and the PCI side had just missed the problem). See? It
actually technically looked like the networking tree did more, and
knew more.

A revert is additional work, and by default I assume that a revert was
done for a sane reason (ie "that commit was broken, so I'll revert
it").

In this case I had to take the hint from your pull request that it
wasn't that the commit was broken and thus reverted.

So the networking tree did two really horribly bad things: doing extra
work for a bad reason, and then not even *documenting* that bad
reason. Either of them is bad on its own. Together, they are really
really bad.

Anyway. I obviously _think_ my merge is all good (and it wasn't really
a complicated merge despite the annoyance), but considering the
confusion, it's always a good idea to double-check.

Because mistakes do happen. I'm pretty good at merges these days, but
they most definitely happen to me too.

            Linus