Messages in this thread | | | Date | Tue, 20 Dec 2022 18:33:59 +0100 | From | Lukasz Majewski <> | Subject | Re: [PATCH v2 1/3] dsa: marvell: Provide per device information about max frame size |
| |
Hi Alexander,
> Hi Alexander, > > > On Fri, Dec 16, 2022 at 5:05 AM Lukasz Majewski <lukma@denx.de> > > wrote: > > > > > > 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? > > > > That was mostly my concern. I was adding a bit of my own defensive > > programming in the event that somebody forgot to fill out the > > chip->info. If nothing else it might make sense to add a check to > > verify that the max_frame_size is populated before blindly using it. > > So perhaps you could do something similar to the max_t approach I > > had called out earlier but instead of applying it on the last case > > you could apply it for the "set_max_frame_size" case with 1632 > > being the minimum and being overwritten by 2048 if it is set in > > max_frame_size. > > I think that I shall add: > > else if (chip->info->ops->set_max_frame_size) > return max_t(int, chip->info->max_frame_size, 1632) - > (headers) > > So then the "default" value of 1632 will be overwritten by 2048 bytes. >
Is this approach acceptable for you?
> > 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
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] | |