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

[Lukasz Majewski]

Hi Russell,

> On Fri, Mar 10, 2023 at 12:53:46PM +0100, Lukasz Majewski wrote:
> > Hi Andrew,
> >   
> > > > > 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?    
> > > 
> > > The driver takes care with its namespace in order to add per
> > > switch family defines. So you can add
> > > MV88E6250_G1_CTL1_MAX_FRAME_2048. It does not matter if it is the
> > > same bit. You can also add a mv88e6250_g1_set_max_frame_size()
> > > and it also does not matter if it is in effect the same as
> > > mv88e6185_g1_set_max_frame_size().
> > > 
> > > We should always make the driver understandably first, compact and
> > > without redundancy second. We are then less likely to get into
> > > situations like this again where it is not clear what MTU a device
> > > actually supports because the code is cryptic.  
> > 
> > Ok, I will add new function.
> > 
> > Thanks for hints.  
> 
> It may be worth doing:
> 
> static int mv88e6xxx_g1_modify(struct mv88e6xxx_chip *chip, int reg,
> 			       u16 mask, u16 val)
> {
> 	int addr = chip->info->global1_addr;
> 	int err;
> 	u16 v;
> 
> 	err = mv88e6xxx_read(chip, addr, reg, &v);
> 	if (err < 0)
> 		return err;
> 
> 	v = (v & ~mask) | val;
> 
> 	return mv88e6xxx_write(chip, addr, reg, v);
> }
> 
> Then, mv88e6185_g1_set_max_frame_size() becomes:
> 
> int mv88e6185_g1_set_max_frame_size(struct mv88e6xxx_chip *chip, int
> mtu) {
> 	u16 val = 0;
> 
> 	if (mtu + ETH_HLEN + ETH_FCS_LEN > 1518)
> 		val = MV88E6185_G1_CTL1_MAX_FRAME_1632;
> 
> 	return mv88e6xxx_g1_modify(chip, MV88E6XXX_G1_CTL1,
> 				   MV88E6185_G1_CTL1_MAX_FRAME_1632,
> val); }
> 

Yes, correct.

> The 6250 variant becomes similar.
> 
> We can also think about converting all those other read-modify-writes
> to use mv88e6xxx_g1_modify().
> 
> The strange thing is... we already have mv88e6xxx_g1_ctl2_mask() which
> is an implementation of mv88e6xxx_g1_modify() specifically for
> MV88E6XXX_G1_CTL2 register, although it uses (val & mask) rather than
> just val. That wouldn't be necessary if the bitfield macros (e.g.
> FIELD_PREP() were used rather than explicit __bf_shf().
> 

I do have the impression that major refactoring of the mv6xxx driver
would be welcome...


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