LinuxLists.cc - [PATCH v4 1/3] dsa: marvell: Provide per device information about max frame size

[permalink] [raw]

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

On Fri, Jan 06, 2023 at 11:16:49AM +0100, Lukasz Majewski wrote:
> 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 <[email protected]>
>
> ---
> 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 dsa_switch *ds, int port)
> {
> struct mv88e6xxx_chip *chip = ds->priv;
>
> - 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 1522 - VLAN_ETH_HLEN - EDSA_HLEN - ETH_FCS_LEN;
> + WARN_ON_ONCE(!chip->info->max_frame_size);
> +
> + return chip->info->max_frame_size - VLAN_ETH_HLEN - EDSA_HLEN
> + - ETH_FCS_LEN;

VLAN_ETH_HLEN (18) + EDSA_HLEN (8) + ETH_FCS_LEN (4) = 30

> }
>
> static int mv88e6xxx_change_mtu(struct dsa_switch *ds, int port, int new_mtu)
> @@ -4955,6 +4954,7 @@ static const struct mv88e6xxx_ops mv88e6250_ops = {
> .avb_ops = &mv88e6352_avb_ops,
> .ptp_ops = &mv88e6250_ptp_ops,
> .phylink_get_caps = mv88e6250_phylink_get_caps,
> + .set_max_frame_size = mv88e6185_g1_set_max_frame_size,
> };
>
> static const struct mv88e6xxx_ops mv88e6290_ops = {
> @@ -5543,6 +5543,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
> .num_internal_phys = 5,
> .max_vid = 4095,
> .max_sid = 63,
> + .max_frame_size = 1522,

1522 - 30 = 1492.

I don't believe that there are switches which don't support the standard
MTU of 1500 ?!

> .port_base_addr = 0x10,
> .phy_base_addr = 0x0,
> .global1_addr = 0x1b,

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?

2023-01-09 09:53:36

[permalink] [raw]

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

Hi Andrew,

> On Fri, Jan 06, 2023 at 11:16:49AM +0100, Lukasz Majewski wrote:
> > 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 <[email protected]>
>
> Reviewed-by: Andrew Lunn <[email protected]>
>
> FYI: It is normal to include a patch 0/X for a patchset, which
> explains the big picture of the patchset. Please try to remember this
> for your next patchset.

Ok. I will. Thanks for the tip.

>
> Andrew
>
>

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: [email protected]

Attachments:

2023-01-13 10:50:41

[permalink] [raw]

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

Hi Andrew, Vladimir,

> On Fri, Jan 06, 2023 at 11:16:49AM +0100, Lukasz Majewski wrote:
> > 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 <[email protected]>
>
> Reviewed-by: Andrew Lunn <[email protected]>
>
> FYI: It is normal to include a patch 0/X for a patchset, which
> explains the big picture of the patchset. Please try to remember this
> for your next patchset.
>
> Andrew
>
>

Are there any more comments, or is this patch set eligible for pulling
into net-next tree?

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: [email protected]

Attachments:

2023-01-13 10:54:45

[permalink] [raw]

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

On Fri, Jan 13, 2023 at 11:39:08AM +0100, Lukasz Majewski wrote:
> Are there any more comments, or is this patch set eligible for pulling
> into net-next tree?

How about responding to the comment that was already posted first?

2023-01-13 11:21:49

[permalink] [raw]

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

Hi Vladimir,

> On Fri, Jan 13, 2023 at 11:39:08AM +0100, Lukasz Majewski wrote:
> > Are there any more comments, or is this patch set eligible for
> > pulling into net-next tree?
>
> How about responding to the comment that was already posted first?

Could you be more specific?

On the beginning (first posted version) the patch included 9 patches
(which included work for ADDR4 for some mv88e6020 setup).

But after the discussion, I've decided to split this patch set to
smaller pieces;

First to add the set_max_frame size with basic definition for mv88e6020
and mv88e6071 and then follow with more complicated changes (for which
there is no agreement on how to tackle them).

For the 'set_max_frame' feature Alexander Dyuck had some comments
regarding defensive programming approach, but finally he agreed with
Andrew's approach.

As of now - the v4 has been Acked by Andrew, so it looks like at least
this "part" of the work is eligible for upstreaming.

Or there are any more issues about which I've forgotten ?

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: [email protected]

Attachments:

2023-01-13 11:53:19

[permalink] [raw]

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

On Fri, Jan 13, 2023 at 12:02:19PM +0100, Lukasz Majewski wrote:
> Hi Vladimir,
>
> > On Fri, Jan 13, 2023 at 11:39:08AM +0100, Lukasz Majewski wrote:
> > > Are there any more comments, or is this patch set eligible for
> > > pulling into net-next tree?
> >
> > How about responding to the comment that was already posted first?
>
> Could you be more specific?
>
>
> On the beginning (first posted version) the patch included 9 patches
> (which included work for ADDR4 for some mv88e6020 setup).
>
> But after the discussion, I've decided to split this patch set to
> smaller pieces;
>
> First to add the set_max_frame size with basic definition for mv88e6020
> and mv88e6071 and then follow with more complicated changes (for which
> there is no agreement on how to tackle them).
>
> For the 'set_max_frame' feature Alexander Dyuck had some comments
> regarding defensive programming approach, but finally he agreed with
> Andrew's approach.
>
> As of now - the v4 has been Acked by Andrew, so it looks like at least
> this "part" of the work is eligible for upstreaming.
>
>
> Or there are any more issues about which I've forgotten ?

Do you agree that for the chip families which neither implement
port_set_jumbo_size() nor set_max_frame_size(), a max MTU of 1492 will
be returned, which currently produces warnings at probe time and should
be fixed first, prior to refactoring the code?
https://patchwork.kernel.org/project/netdevbpf/patch/[email protected]/#25149891

2023-01-13 12:20:54

[permalink] [raw]

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

Hi Vladimir,

> On Fri, Jan 13, 2023 at 12:02:19PM +0100, Lukasz Majewski wrote:
> > Hi Vladimir,
> >
> > > On Fri, Jan 13, 2023 at 11:39:08AM +0100, Lukasz Majewski wrote:
> > > > Are there any more comments, or is this patch set eligible for
> > > > pulling into net-next tree?
> > >
> > > How about responding to the comment that was already posted
> > > first?
> >
> > Could you be more specific?
> >
> >
> > On the beginning (first posted version) the patch included 9 patches
> > (which included work for ADDR4 for some mv88e6020 setup).
> >
> > But after the discussion, I've decided to split this patch set to
> > smaller pieces;
> >
> > First to add the set_max_frame size with basic definition for
> > mv88e6020 and mv88e6071 and then follow with more complicated
> > changes (for which there is no agreement on how to tackle them).
> >
> > For the 'set_max_frame' feature Alexander Dyuck had some comments
> > regarding defensive programming approach, but finally he agreed with
> > Andrew's approach.
> >
> > As of now - the v4 has been Acked by Andrew, so it looks like at
> > least this "part" of the work is eligible for upstreaming.
> >
> >
> > Or there are any more issues about which I've forgotten ?
>
> Do you agree that for the chip families which neither implement
> port_set_jumbo_size() nor set_max_frame_size(), a max MTU of 1492 will
> be returned, which currently produces warnings at probe time and
> should be fixed first, prior to refactoring the code?
> https://patchwork.kernel.org/project/netdevbpf/patch/[email protected]/#25149891

Sorry, but I've overlooked your reply.

I will write my comments as a reply to it.

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: [email protected]

Attachments:

2023-01-13 12:34:33

[permalink] [raw]

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

On Fri, Jan 13, 2023 at 01:13:31PM +0100, Lukasz Majewski wrote:
> I think that this commit [1], made the adjustment to fix yet another
> issue.
> [1] -
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=b9c587fed61cf88bd45822c3159644445f6d5aa6

It appears that this is the commit to blame, indeed.

> It looks like the missing 8 bytes are added in the
> mv88e6xxx_change_mtu() function.

Only for DSA and CPU ports. The driver still behaves as if the max MTU
on user ports is 1492 bytes.

> > I wonder, shouldn't we first fix that, and apply this patch set
> > afterwards?
>
> IMHO, it is up to Andrew to decide how to proceed, as the
> aforementioned patch [1] is an attempt to fix yet another issue [2].
> [2] -
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=1baf0fac10fbe3084975d7cb0a4378eb18871482

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.

2023-01-13 13:00:42

[permalink] [raw]

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

Hi Vladimir,

> On Fri, Jan 06, 2023 at 11:16:49AM +0100, Lukasz Majewski wrote:
> > 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 <[email protected]>
> >
> > ---
> > 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
> > dsa_switch *ds, int port) {
> > struct mv88e6xxx_chip *chip = ds->priv;
> >
> > - 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 1522 - VLAN_ETH_HLEN - EDSA_HLEN - ETH_FCS_LEN;
> > + WARN_ON_ONCE(!chip->info->max_frame_size);
> > +
> > + return chip->info->max_frame_size - VLAN_ETH_HLEN -
> > EDSA_HLEN
> > + - ETH_FCS_LEN;
>
> VLAN_ETH_HLEN (18) + EDSA_HLEN (8) + ETH_FCS_LEN (4) = 30
>
> > }
> >
> > static int mv88e6xxx_change_mtu(struct dsa_switch *ds, int port,
> > int new_mtu) @@ -4955,6 +4954,7 @@ static const struct
> > mv88e6xxx_ops mv88e6250_ops = { .avb_ops = &mv88e6352_avb_ops,
> > .ptp_ops = &mv88e6250_ptp_ops,
> > .phylink_get_caps = mv88e6250_phylink_get_caps,
> > + .set_max_frame_size = mv88e6185_g1_set_max_frame_size,
> > };
> >
> > static const struct mv88e6xxx_ops mv88e6290_ops = {
> > @@ -5543,6 +5543,7 @@ static const struct mv88e6xxx_info
> > mv88e6xxx_table[] = { .num_internal_phys = 5,
> > .max_vid = 4095,
> > .max_sid = 63,
> > + .max_frame_size = 1522,
>
> 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.

>
> > .port_base_addr = 0x10,
> > .phy_base_addr = 0x0,
> > .global1_addr = 0x1b,
>
> 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?

IMHO, it is up to Andrew to decide how to proceed, as the
aforementioned patch [1] is an attempt to fix yet another issue [2].

Links:

[1] -
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=b9c587fed61cf88bd45822c3159644445f6d5aa6

[2] -
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=1baf0fac10fbe3084975d7cb0a4378eb18871482

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: [email protected]

Attachments:

2023-01-13 13:56:52

[permalink] [raw]

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

Hi Vladimir,

> On Fri, Jan 13, 2023 at 01:13:31PM +0100, Lukasz Majewski wrote:
> > I think that this commit [1], made the adjustment to fix yet another
> > issue.
> > [1] -
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=b9c587fed61cf88bd45822c3159644445f6d5aa6
> >
>
> It appears that this is the commit to blame, indeed.
>
> > It looks like the missing 8 bytes are added in the
> > mv88e6xxx_change_mtu() function.
>
> Only for DSA and CPU ports. The driver still behaves as if the max MTU
> on user ports is 1492 bytes.
>

It looks so...

> > > I wonder, shouldn't we first fix that, and apply this patch set
> > > afterwards?
> >
> > IMHO, it is up to Andrew to decide how to proceed, as the
> > aforementioned patch [1] is an attempt to fix yet another issue [2].
> > [2] -
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=1baf0fac10fbe3084975d7cb0a4378eb18871482
> >
>
> 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: [email protected]

Attachments:

2023-01-13 14:15:04

[permalink] [raw]

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

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.

2023-01-13 14:27:26

by Andrew Lunn

[permalink] [raw]

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

> 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

2023-01-13 14:53:31

by Russell King (Oracle)

[permalink] [raw]

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

2023-01-16 10:41:54

[permalink] [raw]

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

Hi Russell,

> On Fri, Jan 06, 2023 at 11:16:49AM +0100, Lukasz Majewski wrote:
> > 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).
>
> I don't think this patch is correct.
>
> One of the things that mv88e6xxx_setup_port() does when initialising
> each port is:
>
> if (chip->info->ops->port_set_jumbo_size) {
> err = chip->info->ops->port_set_jumbo_size(chip,
> port, 10218); if (err)
> return err;
> }
>
> There is one implementation of this, which is
> mv88e6165_port_set_jumbo_size() and that has the effect of setting
> port register 8 to the largest size. So any chip that supports the
> port_set_jumbo_size() method will be programmed on initialisation to
> support this larger size.
>
> However, you seem to be listing e.g. the 88e6190 (if I'm interpreting
> the horrid mv88e6xxx_table changes correctly)

Those changes were requested by the community. Previous versions of
this patch were just changing things to allow correct operation of the
switch ICs on which I do work (i.e. 88e6020 and 88e6071).

And yes, for 88e6190 the max_frame_size = 10240, but (by mistake) the
same value was not updated for 88e6190X.

The question is - how shall I proceed?

After the discussion about this code - it looks like approach from v3
[1] seems to be the most non-intrusive for other ICs.

> as having a maximum
> frame size of 1522, but it implements this method, supports 10240, and
> thus is programmed to support frames of that size rather than 1522.
>

Links:

[1] - https://lore.kernel.org/netdev/[email protected]/T/

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: [email protected]

Attachments:

2023-01-25 11:24:33

[permalink] [raw]

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

Hi,

> Hi Russell,
>
> > On Fri, Jan 06, 2023 at 11:16:49AM +0100, Lukasz Majewski wrote:
> > > 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).
> >
> > I don't think this patch is correct.
> >
> > One of the things that mv88e6xxx_setup_port() does when initialising
> > each port is:
> >
> > if (chip->info->ops->port_set_jumbo_size) {
> > err = chip->info->ops->port_set_jumbo_size(chip,
> > port, 10218); if (err)
> > return err;
> > }
> >
> > There is one implementation of this, which is
> > mv88e6165_port_set_jumbo_size() and that has the effect of setting
> > port register 8 to the largest size. So any chip that supports the
> > port_set_jumbo_size() method will be programmed on initialisation to
> > support this larger size.
> >
> > However, you seem to be listing e.g. the 88e6190 (if I'm
> > interpreting the horrid mv88e6xxx_table changes correctly)
>
> Those changes were requested by the community. Previous versions of
> this patch were just changing things to allow correct operation of the
> switch ICs on which I do work (i.e. 88e6020 and 88e6071).
>
> And yes, for 88e6190 the max_frame_size = 10240, but (by mistake) the
> same value was not updated for 88e6190X.
>
> The question is - how shall I proceed?
>
> After the discussion about this code - it looks like approach from v3
> [1] seems to be the most non-intrusive for other ICs.
>

I would appreciate _any_ hints on how shall I proceed to prepare those
patches, so the community will accept them...

Thanks in advance.

> > as having a maximum
> > frame size of 1522, but it implements this method, supports 10240,
> > and thus is programmed to support frames of that size rather than
> > 1522.
>
> Links:
>
> [1] - https://lore.kernel.org/netdev/[email protected]/T/
>
>
> 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:
> [email protected]

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: [email protected]

Attachments:

(No filename) (488.00 B)
OpenPGP digital signature

2023-01-25 15:13:20

by Russell King (Oracle)

[permalink] [raw]

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

On Wed, Jan 25, 2023 at 12:24:12PM +0100, Lukasz Majewski wrote:
> Hi,
>
> > Hi Russell,
> >
> > > On Fri, Jan 06, 2023 at 11:16:49AM +0100, Lukasz Majewski wrote:
> > > > 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).
> > >
> > > I don't think this patch is correct.
> > >
> > > One of the things that mv88e6xxx_setup_port() does when initialising
> > > each port is:
> > >
> > > if (chip->info->ops->port_set_jumbo_size) {
> > > err = chip->info->ops->port_set_jumbo_size(chip,
> > > port, 10218); if (err)
> > > return err;
> > > }
> > >
> > > There is one implementation of this, which is
> > > mv88e6165_port_set_jumbo_size() and that has the effect of setting
> > > port register 8 to the largest size. So any chip that supports the
> > > port_set_jumbo_size() method will be programmed on initialisation to
> > > support this larger size.
> > >
> > > However, you seem to be listing e.g. the 88e6190 (if I'm
> > > interpreting the horrid mv88e6xxx_table changes correctly)
> >
> > Those changes were requested by the community. Previous versions of
> > this patch were just changing things to allow correct operation of the
> > switch ICs on which I do work (i.e. 88e6020 and 88e6071).
> >
> > And yes, for 88e6190 the max_frame_size = 10240, but (by mistake) the
> > same value was not updated for 88e6190X.
> >
> > The question is - how shall I proceed?
> >
> > After the discussion about this code - it looks like approach from v3
> > [1] seems to be the most non-intrusive for other ICs.
> >
>
> I would appreciate _any_ hints on how shall I proceed to prepare those
> patches, so the community will accept them...

What I'm concerned about, and why I replied, is that setting the devices
to have a max frame size of 1522 when we program them to use a larger
frame size means we break those switches for normal sized packets.

The current logic in mv88e6xxx_get_max_mtu() is:

If the chip implements port_set_jumbo_size, then packet sizes of
up to 10240 are supported.
(ops: 6131, 6141, 6171, 6172, 6175, 6176, 6190, 6190x, 6240, 6320,
6321, 6341, 6350, 6351, 6352, 6390, 6390x, 6393x)
If the chip implements set_max_frame_size, then packet sizes of
up to 1632 are supported.
(ops: 6085, 6095, 6097, 6123, 6161, 6185)
Otherwise, packets of up to 1522 are supported.

Now, going through the patch, I see:

88e6085 has 10240 but currently has 1632
88e6095 has 1632 (no change)
88e6097 has 1632 (no change)
88e6123 has 10240 but currently has 1632
88e6131 has 10240 (no change)
88e6141 has 10240 (no change)
88e6161 has 1632 but currently has 10240
88e6165 has 1632 but currently has 1522
88e6171 has 1522 but currently has 10240
88e6172 has 10240 (no change)
88e6175 has 1632 but currently has 10240
88e6176 has 10240 (no change)
88e6185 has 1632 (no change)
88e6190 has 10240 (no change)
88e6190x has 10240 (no change)
88e6191 has 10240 but currently has 1522
88e6191x has 1522 but currently has 10240
88e6193x has 1522 but currently has 10240
88e6220 has 2048 but currently has 1522
88e6240 has 10240 (no change)
88e6250 has 2048 but currently has 1522
88e6290 has 10240 but currently has 1522
88e6320 has 10240 (no change)
88e6321 has 10240 (no change)
88e6341 has 10240 (no change)
88e6350 has 10240 (no change)
88e6351 has 10240 (no change)
88e6352 has 10240 (no change)
88e6390 has 1522 but currently has 10240
88e6390x has 1522 but currently has 10240
88e6393x has 1522 but currently has 10240

My point is that based on the above, there's an awful lot of changes
that this one patch brings, and I'm not sure many of them are intended.

All the ones with "but currently has 10240", it seems they implement
port_set_jumbo_size() which, although the switch may default to a
smaller frame size, we configure it to be higher. Maybe these don't
implement the field that configures those? Maybe your patch is wrong?
I don't know.

Similarly for the ones with "but currently has 1632", it seems they
implement set_max_frame_size(), but this is only called via
mv88e6xxx_change_mtu(), and I haven't worked out whether that will
be called during initialisation by the networking layer.

Now, what really concerns me is the difficulty in making this change.
As we can see from the above, there's a lot of changes going on here,
and it's not obvious which are intentional and which may be bugs.

So, I think it would be far better to introduce the "max_frame_size"
field using the existing values, and then verify that value during
initialisation time for every entry in mv88e6xxx_table[] using the
rules that mv88e6xxx_get_max_mtu() was using. Boot that kernel, and
have it run that verification, and state that's what's happened and
was successful in the commit message.

In the next commit, change mv88e6xxx_get_max_mtu() to use those
verified values and remove the verification code.

Then in the following commit, update the "max_frame_size" values with
the changes you intend to make.

Then, we can (a) have confidence that each of the new members were
properly initialised, and (b) we can also see what changes you're
intentionally making.

Right now, given that at least two of the "max_frame_size" values are
wrong in this patch, I think we can say for certain that we've proven
that trying to introduce this new member and use it in a single patch
is too error prone.

Thanks.

--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

2023-01-30 11:58:23