From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-5.8 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 90EDAC433F5 for ; Wed, 8 Sep 2021 02:53:20 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 616C66113E for ; Wed, 8 Sep 2021 02:53:20 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1347402AbhIHCyQ (ORCPT ); Tue, 7 Sep 2021 22:54:16 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:43106 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1345895AbhIHCyK (ORCPT ); Tue, 7 Sep 2021 22:54:10 -0400 Received: from mail-lf1-x12f.google.com (mail-lf1-x12f.google.com [IPv6:2a00:1450:4864:20::12f]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 942FCC061575 for ; Tue, 7 Sep 2021 19:53:02 -0700 (PDT) Received: by mail-lf1-x12f.google.com with SMTP id f18so1350225lfk.12 for ; Tue, 07 Sep 2021 19:53:02 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux-foundation.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=FjvpMqPdNZFQsYmJZHelUV/clBYJUUkWHVd43X/arJo=; b=OPV6yfb2KvxvQvVoEx2dtPnNZNoGBs6s1nUmz1enVOwBZLubjzkZqjxpzBWalY8UGt icqdafw8DgsyMogRySKd8EoEnE30gQTUJzi0d69tvAdBC1jlE4TwMXN+qDTeglbnd8nU HqLrA1idTwI5PHzQyFj1h4a8p0IzQgUpN3Ks8= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=FjvpMqPdNZFQsYmJZHelUV/clBYJUUkWHVd43X/arJo=; b=BFp+7cg2JNJL2paiXDWsFmGCouVVUymEchYUSGJTrppSfSmta1Evqdv9Zb7OtKEZIG sX0KUcCLM4UtomN7wb98sLxmUv/Y/Qs9Z6L00BjMIbFMhfPvRGB1SgqVou2Ld3+WlrVn ++XgJnZEMMF20WNPPj3y28H59CTSucxz56y0PYF+CWW8JgQarqnJTuZB5xcnFrKDBQ4s +uEPVV/sWRXSLpoSJyoVo9Dz1BByi1S1EfliLZiQkbq0rKg8qne/OnwxaHVwVQ9VXbxs 2z67LdSJDSkVqW53tIkWuh8fv6i29Wdo1ZEGamH7JUhdZfo4SR8RVDSG/Ird32CxZDEk kYlw== X-Gm-Message-State: AOAM5319zLXd6YaDiFBFGWhKM77Z3lRkIjXJ4Nmgw8QUp8bhckQXBE2Z RuOozyb2y5hNflFp+JmWE2xUqawnaxdyvjjhiVw= X-Google-Smtp-Source: ABdhPJz9kelg2vt3l35d4UTnAAtIIl+D5TxqnC7LlwQKw4DWAsg1C7NhjhI+PuZSzfytpt+xdrmSBw== X-Received: by 2002:a05:6512:338f:: with SMTP id h15mr1052384lfg.38.1631069580608; Tue, 07 Sep 2021 19:53:00 -0700 (PDT) Received: from mail-lf1-f44.google.com (mail-lf1-f44.google.com. [209.85.167.44]) by smtp.gmail.com with ESMTPSA id b14sm76981ljr.111.2021.09.07.19.53.00 for (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 07 Sep 2021 19:53:00 -0700 (PDT) Received: by mail-lf1-f44.google.com with SMTP id n2so1942027lfk.0 for ; Tue, 07 Sep 2021 19:53:00 -0700 (PDT) X-Received: by 2002:a05:6512:1112:: with SMTP id l18mr1044974lfg.402.1631069579859; Tue, 07 Sep 2021 19:52:59 -0700 (PDT) MIME-Version: 1.0 References: <20210908022620.GA845134@bjorn-Precision-5520> In-Reply-To: <20210908022620.GA845134@bjorn-Precision-5520> From: Linus Torvalds Date: Tue, 7 Sep 2021 19:52:43 -0700 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [GIT PULL] PCI changes for v5.15 To: Bjorn Helgaas Cc: David Miller , Linux PCI , Linux Kernel Mailing List , Lorenzo Pieralisi Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Sep 7, 2021 at 7:26 PM Bjorn Helgaas 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