Re: [PATCH 6/7] dsa: marvell: Correct value of max_frame_size variable after validation

[Lukasz Majewski]

Hi Russell,

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

Yes. I do agree.

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

Ok. I will restructure patches to follow above scheme.

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

The "peace" of changes for this code is rather slow, so the risk is
minimal.

Moreover, next ICs added would _require_ to have the max_frame_size
field set (the WARN_ON() clause).

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

Ok. I hope that we will avoid this threat.


Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@xxxxxxx

Attachment: pgp90EnQHrXCq.pgp
Description: OpenPGP digital signature