Hi Russell,

On Thu, Mar 09, 2023 at 03:43:50PM +0100, Lukasz Majewski wrote:

quoted

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.

quoted

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@denx.de