diff mbox series

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

Message ID 20221215144536.3810578-1-lukma@denx.de (mailing list archive)
State Deferred
Delegated to: Netdev Maintainers
Headers show
Series [v2,1/3] dsa: marvell: Provide per device information about max frame size | expand

Checks

Context Check Description
netdev/tree_selection success Guessing tree name failed - patch did not apply

Commit Message

Lukasz Majewski Dec. 15, 2022, 2:45 p.m. UTC
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(-)

Comments

Alexander Duyck Dec. 15, 2022, 4:48 p.m. UTC | #1
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? Why not just replace:
	else if (chip->info->ops->set_max_frame_size)
with:
	else if (chip->info->max_frame_size)

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.

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)".
Jakub Kicinski Dec. 16, 2022, 4:20 a.m. UTC | #2
On Thu, 15 Dec 2022 15:45:34 +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.

# Form letter - net-next is closed

We have already submitted the networking pull request to Linus
for v6.2 and therefore net-next is closed for new drivers, features,
code refactoring and optimizations. We are currently accepting
bug fixes only.

Please repost when net-next reopens after Jan 2nd.

RFC patches sent for review only are obviously welcome at any time.
Lukasz Majewski Dec. 16, 2022, 11:56 a.m. UTC | #3
Hi Jakub,

> On Thu, 15 Dec 2022 15:45:34 +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.  
> 
> # Form letter - net-next is closed
> 

I see....

> We have already submitted the networking pull request to Linus
> for v6.2 and therefore net-next is closed for new drivers, features,
> code refactoring and optimizations. We are currently accepting
> bug fixes only.

Ok.

> 
> Please repost when net-next reopens after Jan 2nd.
> 
> RFC patches sent for review only are obviously welcome at any time.

I hope that the discussion regarding those patches will be done by this
time.


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
Lukasz Majewski Dec. 16, 2022, 1:05 p.m. UTC | #4
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
Alexander Duyck Dec. 16, 2022, 5:24 p.m. UTC | #5
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.
Lukasz Majewski Dec. 19, 2022, noon UTC | #6
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.


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
Lukasz Majewski Dec. 20, 2022, 5:33 p.m. UTC | #7
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
diff mbox series

Patch

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;
 }
 
@@ -4461,6 +4463,7 @@  static const struct mv88e6xxx_ops mv88e6250_ops = {
 	.avb_ops = &mv88e6352_avb_ops,
 	.ptp_ops = &mv88e6250_ptp_ops,
 	.phylink_validate = mv88e6065_phylink_validate,
+	.set_max_frame_size = mv88e6185_g1_set_max_frame_size,
 };
 
 static const struct mv88e6xxx_ops mv88e6290_ops = {
@@ -5060,6 +5063,7 @@  static const struct mv88e6xxx_info mv88e6xxx_table[] = {
 		.atu_move_port_mask = 0xf,
 		.multi_chip = true,
 		.ops = &mv88e6095_ops,
+		.max_frame_size = 1632,
 	},
 
 	[MV88E6097] = {
@@ -5083,6 +5087,7 @@  static const struct mv88e6xxx_info mv88e6xxx_table[] = {
 		.multi_chip = true,
 		.edsa_support = MV88E6XXX_EDSA_SUPPORTED,
 		.ops = &mv88e6097_ops,
+		.max_frame_size = 1632,
 	},
 
 	[MV88E6123] = {
@@ -5106,6 +5111,7 @@  static const struct mv88e6xxx_info mv88e6xxx_table[] = {
 		.multi_chip = true,
 		.edsa_support = MV88E6XXX_EDSA_SUPPORTED,
 		.ops = &mv88e6123_ops,
+		.max_frame_size = 1632,
 	},
 
 	[MV88E6131] = {
@@ -5174,6 +5180,7 @@  static const struct mv88e6xxx_info mv88e6xxx_table[] = {
 		.edsa_support = MV88E6XXX_EDSA_SUPPORTED,
 		.ptp_support = true,
 		.ops = &mv88e6161_ops,
+		.max_frame_size = 1632,
 	},
 
 	[MV88E6165] = {
@@ -5197,6 +5204,7 @@  static const struct mv88e6xxx_info mv88e6xxx_table[] = {
 		.multi_chip = true,
 		.ptp_support = true,
 		.ops = &mv88e6165_ops,
+		.max_frame_size = 1632,
 	},
 
 	[MV88E6171] = {
@@ -5312,6 +5320,7 @@  static const struct mv88e6xxx_info mv88e6xxx_table[] = {
 		.multi_chip = true,
 		.edsa_support = MV88E6XXX_EDSA_SUPPORTED,
 		.ops = &mv88e6185_ops,
+		.max_frame_size = 1632,
 	},
 
 	[MV88E6190] = {
@@ -5440,6 +5449,7 @@  static const struct mv88e6xxx_info mv88e6xxx_table[] = {
 		.num_internal_phys = 2,
 		.invalid_port_mask = BIT(2) | BIT(3) | BIT(4),
 		.max_vid = 4095,
+		.max_frame_size = 2048,
 		.port_base_addr = 0x08,
 		.phy_base_addr = 0x00,
 		.global1_addr = 0x0f,
@@ -5486,6 +5496,7 @@  static const struct mv88e6xxx_info mv88e6xxx_table[] = {
 		.num_ports = 7,
 		.num_internal_phys = 5,
 		.max_vid = 4095,
+		.max_frame_size = 2048,
 		.port_base_addr = 0x08,
 		.phy_base_addr = 0x00,
 		.global1_addr = 0x0f,
diff --git a/drivers/net/dsa/mv88e6xxx/chip.h b/drivers/net/dsa/mv88e6xxx/chip.h
index 80dc7b549e81..9712b10fc4ed 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.h
+++ b/drivers/net/dsa/mv88e6xxx/chip.h
@@ -130,6 +130,7 @@  struct mv88e6xxx_info {
 	unsigned int num_internal_phys;
 	unsigned int num_gpio;
 	unsigned int max_vid;
+	unsigned int max_frame_size;
 	unsigned int port_base_addr;
 	unsigned int phy_base_addr;
 	unsigned int global1_addr;