Re: [PATCH v2 1/3] dsa: marvell: Provide per device information about max frame size

[Lukasz Majewski <lukma@denx.de>]

[-- Attachment #1: Type: text/plain, Size: 3447 bytes --]

Hi Alexander,

> On Thu, 2022-12-15 at 15:45 +0100, Lukasz Majewski wrote:
> > Different Marvell DSA switches support different size of max frame
> > bytes to be sent.
> > 
> > For example mv88e6185 supports max 1632 bytes, which is now
> > in-driver standard value. On the other hand - mv88e6250 supports
> > 2048 bytes.
> > 
> > As this value is internal and may be different for each switch IC,
> > new entry in struct mv88e6xxx_info has been added to store it.
> > 
> > Signed-off-by: Lukasz Majewski <lukma@denx.de>
> > ---
> > Changes for v2:
> > - Define max_frame_size with default value of 1632 bytes,
> > - Set proper value for the mv88e6250 switch SoC (linkstreet) family
> > ---
> >  drivers/net/dsa/mv88e6xxx/chip.c | 13 ++++++++++++-
> >  drivers/net/dsa/mv88e6xxx/chip.h |  1 +
> >  2 files changed, 13 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/net/dsa/mv88e6xxx/chip.c
> > b/drivers/net/dsa/mv88e6xxx/chip.c index 2ca3cbba5764..7ae4c389ce50
> > 100644 --- a/drivers/net/dsa/mv88e6xxx/chip.c
> > +++ b/drivers/net/dsa/mv88e6xxx/chip.c
> > @@ -3093,7 +3093,9 @@ static int mv88e6xxx_get_max_mtu(struct
> > dsa_switch *ds, int port) if (chip->info->ops->port_set_jumbo_size)
> >  		return 10240 - VLAN_ETH_HLEN - EDSA_HLEN -
> > ETH_FCS_LEN; else if (chip->info->ops->set_max_frame_size)
> > -		return 1632 - VLAN_ETH_HLEN - EDSA_HLEN -
> > ETH_FCS_LEN;
> > +		return (chip->info->max_frame_size  - VLAN_ETH_HLEN
> > +			- EDSA_HLEN - ETH_FCS_LEN);
> > +
> >  	return 1522 - VLAN_ETH_HLEN - EDSA_HLEN - ETH_FCS_LEN;
> >  }
> > 
> >   
> 
> Is there any specific reason for triggering this based on the
> existance of the function call? 

This was the original code in this driver.

This value (1632 or 2048 bytes) is SoC (family) specific.

By checking which device defines set_max_frame_size callback, I could
fill the chip->info->max_frame_size with 1632 value.

> Why not just replace:
> 	else if (chip->info->ops->set_max_frame_size)
> with:
> 	else if (chip->info->max_frame_size)
> 

I think that the callback check is a bit "defensive" approach -> 1522B
is the default value and 1632 (or 10240 - jumbo) can be set only when
proper callback is defined.

> Otherwise my concern is one gets defined without the other leading to
> a future issue as 0 - extra headers will likely wrap and while the
> return value may be a signed int, it is usually stored in an unsigned
> int so it would effectively uncap the MTU.

Please correct me if I misunderstood something:

The problem is with new mv88eXXXX devices, which will not provide
max_frame_size information to their chip->info struct?

Or is there any other issue?

> 
> Actually you could take this one step further since all values should
> be 1522 or greater you could just drop the else/if and replace the
> last line with "max_t(int, chip->info->max_frame_size, 1522) -
> (headers)".

This would be possible, yes.

However, then we will not check if the proper callback (e.g.
chip->info->ops->set_max_frame_size) is available for specific SoC.

If this is OK for maintainers for this driver, then I don't mind.


Best regards,

Lukasz Majewski

--

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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]