Messages in this thread | | | Date | Fri, 16 Dec 2022 14:05:26 +0100 | From | Lukasz Majewski <> | Subject | Re: [PATCH v2 1/3] dsa: marvell: Provide per device information about max frame size |
| |
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 [unhandled content-type:application/pgp-signature] | |