Date:   Thu, 9 Mar 2023 15:31:10 +0000
From:   "Russell King (Oracle)" <linux@armlinux.org.uk>
To:     Lukasz Majewski <lukma@denx.de>
Cc:     Andrew Lunn <andrew@lunn.ch>, Vladimir Oltean <olteanv@gmail.com>,
        Eric Dumazet <edumazet@google.com>,
        Florian Fainelli <f.fainelli@gmail.com>,
        "David S. Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        Alexander Duyck <alexander.duyck@gmail.com>,
        netdev@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 6/7] dsa: marvell: Correct value of max_frame_size
 variable after validation
Message-ID: <ZAn7vkjj0bYdZnhz@shell.armlinux.org.uk>
References: <20230309125421.3900962-1-lukma@denx.de>
 <20230309125421.3900962-7-lukma@denx.de>
 <ZAnnk5MZc0w4VkDE@shell.armlinux.org.uk>
 <20230309154350.0bdc54c8@wsk>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230309154350.0bdc54c8@wsk>
Sender: Russell King (Oracle) <linux@armlinux.org.uk>
Precedence: bulk

On Thu, Mar 09, 2023 at 03:43:50PM +0100, Lukasz Majewski wrote:
> Hi Russell,
> 
> Please correct my understanding - I do see two approaches here:
> 
> A. In patch 1 I do set the max_frame_size values (deduced). Then I add
> validation function (patch 2). This function shows "BUG:...." only when
> we do have a mismatch. In patch 3 I do correct the max_frame_size
> values (according to validation function) and remove the validation
> function. This is how it is done in v5 and is going to be done in v6.

I don't see much point in adding the validation, then correcting the
values that were added in patch 1 that were identified by patch 2 in
patch 3 - because that means patch 1's deduction was incorrect in
some way.

If there is any correction to be done, then it should be:

patch 1 - add the max_frame_size values
patch 2 - add validation (which should not produce any errors)
patch 3 - convert to use max_frame_size, and remove validation, stating
  that the validation had no errors
patch 4 (if necessary) - corrections to max_frame_size values if they
  are actually incorrect (in other words, they were buggy before patch
  1.)
patch 5 onwards - the rest of the series.

> B. Having showed the v5 in public, the validation function is known.
> Then I do prepare v6 with only patch 1 having correct values (from the
> outset) and provide in the commit message the code for validation
> function. Then patch 2 and 3 (validation function and the corrected
> values of max_frame_size) can be omitted in v6.
> 
> For me it would be better to choose approach B.

I would suggest that is acceptable for the final round of patches, but
I'm wary about saying "yes" to it because... what if something changes
in that table between the time you've validated it, and when it
eventually gets accepted. Keeping the validation code means that during
the review of the series, and subsequent updates onto net-next (which
should of course include re-running the validation code) we can be
more certain that nothing has changed that would impact it.

What I worry about is if something changes, the patch adding the
values mis-patches (e.g. due to other changes - much of the context
for each hunk is quite similar) then we will have quite a problem to
sort it out.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!