From: Vladimir Oltean <olteanv@gmail.com> Date: 2023-01-06 14:52:20
On Fri, Jan 06, 2023 at 11:16:49AM +0100, Lukasz Majewski wrote:
quoted hunk
Different Marvell DSA switches support different size of max frame
bytes to be sent. This value corresponds to the memory allocated
in switch to store single frame.
For example mv88e6185 supports max 1632 bytes, which is now in-driver
standard value. On the other hand - mv88e6250 supports 2048 bytes.
To be more interresting - devices supporting jumbo frames - use yet
another value (10240 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.
This commit doesn't change the code functionality - it just provides
the max frame size value explicitly - up till now it has been
assigned depending on the callback provided by the IC driver
(e.g. .set_max_frame_size, .port_set_jumbo_size).
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
Changes for v3:
- Add default value for 1632B of the max frame size (to avoid problems
with not defined values)
Changes for v4:
- Rework the mv88e6xxx_get_max_mtu() by using per device defined
max_frame_size value
- Add WARN_ON_ONCE() when max_frame_size is not defined
- Add description for the new 'max_frame_size' member of mv88e6xxx_info
---
drivers/net/dsa/mv88e6xxx/chip.c | 41 ++++++++++++++++++++++++++++----
drivers/net/dsa/mv88e6xxx/chip.h | 6 +++++
2 files changed, 42 insertions(+), 5 deletions(-)
Note that I see this behavior isn't new. But I've simulated it, and it
will produce the following messages on probe:
[ 7.425752] mscc_felix 0000:00:00.5 swp0 (uninitialized): PHY [0000:00:00.3:10] driver [Microsemi GE VSC8514 SyncE] (irq=POLL)
[ 7.437516] mscc_felix 0000:00:00.5: nonfatal error -34 setting MTU to 1500 on port 0
[ 7.588585] mscc_felix 0000:00:00.5 swp1 (uninitialized): PHY [0000:00:00.3:11] driver [Microsemi GE VSC8514 SyncE] (irq=POLL)
[ 7.600433] mscc_felix 0000:00:00.5: nonfatal error -34 setting MTU to 1500 on port 1
[ 7.752613] mscc_felix 0000:00:00.5 swp2 (uninitialized): PHY [0000:00:00.3:12] driver [Microsemi GE VSC8514 SyncE] (irq=POLL)
[ 7.764457] mscc_felix 0000:00:00.5: nonfatal error -34 setting MTU to 1500 on port 2
[ 7.900771] mscc_felix 0000:00:00.5 swp3 (uninitialized): PHY [0000:00:00.3:13] driver [Microsemi GE VSC8514 SyncE] (irq=POLL)
[ 7.912501] mscc_felix 0000:00:00.5: nonfatal error -34 setting MTU to 1500 on port 3
I wonder, shouldn't we first fix that, and apply this patch set afterwards?
On Fri, Jan 06, 2023 at 11:16:49AM +0100, Lukasz Majewski wrote:
quoted
Different Marvell DSA switches support different size of max frame
bytes to be sent. This value corresponds to the memory allocated
in switch to store single frame.
For example mv88e6185 supports max 1632 bytes, which is now
in-driver standard value. On the other hand - mv88e6250 supports
2048 bytes. To be more interresting - devices supporting jumbo
frames - use yet another value (10240 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.
This commit doesn't change the code functionality - it just provides
the max frame size value explicitly - up till now it has been
assigned depending on the callback provided by the IC driver
(e.g. .set_max_frame_size, .port_set_jumbo_size).
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
Changes for v3:
- Add default value for 1632B of the max frame size (to avoid
problems with not defined values)
Changes for v4:
- Rework the mv88e6xxx_get_max_mtu() by using per device defined
max_frame_size value
- Add WARN_ON_ONCE() when max_frame_size is not defined
- Add description for the new 'max_frame_size' member of
mv88e6xxx_info ---
drivers/net/dsa/mv88e6xxx/chip.c | 41
++++++++++++++++++++++++++++---- drivers/net/dsa/mv88e6xxx/chip.h |
6 +++++ 2 files changed, 42 insertions(+), 5 deletions(-)
diff --git a/drivers/net/dsa/mv88e6xxx/chip.c
b/drivers/net/dsa/mv88e6xxx/chip.c index 242b8b325504..fc6d98c4a029
100644 --- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -3545,11 +3545,10 @@ static int mv88e6xxx_get_max_mtu(struct
1522 - 30 = 1492.
I don't believe that there are switches which don't support the
standard MTU of 1500 ?!
I think that this commit [1], made the adjustment to fix yet another
issue.
It looks like the missing 8 bytes are added in the
mv88e6xxx_change_mtu() function.
Note that I see this behavior isn't new. But I've simulated it, and it
will produce the following messages on probe:
[ 7.425752] mscc_felix 0000:00:00.5 swp0 (uninitialized): PHY
[0000:00:00.3:10] driver [Microsemi GE VSC8514 SyncE] (irq=POLL) [
7.437516] mscc_felix 0000:00:00.5: nonfatal error -34 setting MTU to
1500 on port 0 [ 7.588585] mscc_felix 0000:00:00.5 swp1
(uninitialized): PHY [0000:00:00.3:11] driver [Microsemi GE VSC8514
SyncE] (irq=POLL) [ 7.600433] mscc_felix 0000:00:00.5: nonfatal
error -34 setting MTU to 1500 on port 1 [ 7.752613] mscc_felix
0000:00:00.5 swp2 (uninitialized): PHY [0000:00:00.3:12] driver
[Microsemi GE VSC8514 SyncE] (irq=POLL) [ 7.764457] mscc_felix
0000:00:00.5: nonfatal error -34 setting MTU to 1500 on port 2 [
7.900771] mscc_felix 0000:00:00.5 swp3 (uninitialized): PHY
[0000:00:00.3:13] driver [Microsemi GE VSC8514 SyncE] (irq=POLL) [
7.912501] mscc_felix 0000:00:00.5: nonfatal error -34 setting MTU to
1500 on port 3
I wonder, shouldn't we first fix that, and apply this patch set
afterwards?
I think the handling for those switches were neither port_set_jumbo_size()
nor set_max_frame_size() is present is just a roundabout way of saying
"hey, I only support ETH_DATA_LEN MTU and can't change it, leave me alone".
But it isn't what the code does.
I think the handling for those switches were neither
port_set_jumbo_size() nor set_max_frame_size() is present is just a
roundabout way of saying "hey, I only support ETH_DATA_LEN MTU and
can't change it, leave me alone". But it isn't what the code does.
I tend to agree... The number of switched which suppor 1522 B max frame
is only six. This may be why the problem was not noticed.
The fixed function maybe should look like below:
static int mv88e6xxx_get_max_mtu(struct dsa_switch *ds, int port)
{
....
int max_mtu;
max_mtu = chip->info->max_frame_size - VLAN_ETH_HLEN -
ETH_FCS_LE;
if (dsa_is_dsa_port(ds, port) || dsa_is_cpu_port(ds, port))
max_mtu -= EDSA_HLEN;
return max_mtu;
}
Comments more than 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@denx.de
From: Andrew Lunn <andrew@lunn.ch> Date: 2023-01-13 13:57:17
I tend to agree... The number of switched which suppor 1522 B max frame
is only six. This may be why the problem was not noticed.
The fixed function maybe should look like below:
static int mv88e6xxx_get_max_mtu(struct dsa_switch *ds, int port)
{
....
int max_mtu;
max_mtu = chip->info->max_frame_size - VLAN_ETH_HLEN -
ETH_FCS_LE;
if (dsa_is_dsa_port(ds, port) || dsa_is_cpu_port(ds, port))
max_mtu -= EDSA_HLEN;
return max_mtu;
}
Comments more than welcome.
I would suggest some comments are added, explaining what is going on
here. Given the number of Fixes: tags in this area, it is clearly
tricky to get right, given how different switches operate.
I've not looked back to the email archive, but i have a vague
recollection that it could be some switches don't impose the MTU limit
on CPU and DSA ports, just the user ports. So the value reported for
those ports can actually be bigger than then the max mtu, in order to
accommodate the DSA header.
Andrew
From: Vladimir Oltean <olteanv@gmail.com> Date: 2023-01-13 14:03:05
On Fri, Jan 13, 2023 at 02:20:17PM +0100, Lukasz Majewski wrote:
The fixed function maybe should look like below:
static int mv88e6xxx_get_max_mtu(struct dsa_switch *ds, int port)
{
....
int max_mtu;
max_mtu = chip->info->max_frame_size - VLAN_ETH_HLEN -
ETH_FCS_LE;
if (dsa_is_dsa_port(ds, port) || dsa_is_cpu_port(ds, port))
max_mtu -= EDSA_HLEN;
return max_mtu;
}
Comments more than welcome.
I suspect that looking at the DSA code which calls these methods will
answer a lot of your questions. ds->ops->port_max_mtu() is only called
for user ports. As for ds->ops->port_change_mtu(), this will always be
called with the requested L2 payload length (default 1500) on user ports,
and with the maximum among user ports for DSA and CPU ports.