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 01:54:20PM +0100, Lukasz Majewski wrote:
> > Running of the mv88e6xxx_validate_frame_size() function provided
> > following results:
> > 
> > [    1.585565] BUG: Marvell 88E6020 has differing max_frame_size:
> > 1632 != 2048 [    1.592540] BUG: Marvell 88E6071 has differing
> > max_frame_size: 1632 != 2048 ^------ Correct -> mv88e6250 family
> > max frame size = 2048B
> > 
> > [    1.599507] BUG: Marvell 88E6085 has differing max_frame_size:
> > 1632 != 1522 [    1.606476] BUG: Marvell 88E6165 has differing
> > max_frame_size: 1522 != 1632 [    1.613445] BUG: Marvell 88E6190X
> > has differing max_frame_size: 10240 != 1522 [    1.620590] BUG:
> > Marvell 88E6191X has differing max_frame_size: 10240 != 1522 [
> > 1.627730] BUG: Marvell 88E6193X has differing max_frame_size: 10240
> > != 1522 ^------ Needs to be fixed!!!
> > 
> > [    1.634871] BUG: Marvell 88E6220 has differing max_frame_size:
> > 1632 != 2048 [    1.641842] BUG: Marvell 88E6250 has differing
> > max_frame_size: 1632 != 2048 ^------ Correct -> mv88e6250 family
> > max frame size = 2048B  
> 
> If I understand this correctly, in patch 4, you add a call to the 6250
> family to call mv88e6185_g1_set_max_frame_size(), which sets a bit
> called MV88E6185_G1_CTL1_MAX_FRAME_1632 if the frame size is larger
> than 1518.

Yes, correct.

> 
> However, you're saying that 6250 has a frame size of 2048. That's
> fine, but it makes MV88E6185_G1_CTL1_MAX_FRAME_1632 rather misleading
> as a definition. While the bit may increase the frame size, I think
> if we're going to do this, then this definition ought to be renamed.
> 

I thought about rename, but then I've double checked; register offset
and exact bit definition is the same as for 6185, so to avoid
unnecessary code duplication - I've reused the existing function.

Maybe comment would be just enough?

> That said, I would like Andrew and Vladimir's thoughts on this too.
> 

Ok.

> Finally, I would expect, if this series was done the way I suggested,
> that patch 1 should set the max frame size according to how the
> existing code works, which means patch 2, being the validation patch,
> should be completely silent if patch 1 is correct - and that's the
> entire point of validating. It's to make sure that patch 1 is
> correct.

Ok.

> 
> If it isn't correct, then patch 1 is wrong and should be updated.
> 

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.


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.

> Essentially, this patch should only exist if the values we are using
> today are actually incorrect.
> 
> To put this another way, the conversion from our existing way of
> determining the max mtu to using the .max_frame_size method should be
> an entire no-op from the driver operation point of view. Then any
> errors in those values should be fixed and explained in a separate
> commit. Then the new support added.
> 
> At least that's how I see it. Andrew and Vladimir may disagree.
> 




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: pgp_K_DQxlFLX.pgp
Description: OpenPGP digital signature