On Fri, Mar 10, 2023 at 02:17:19PM +0100, Lukasz Majewski wrote:

quoted

quoted

For example mv88e6185 supports max 1632 bytes, which is now
in-driver standard value.  

What is the criterion based on which 1632 is the "in-driver standard
value"?

It comes from the documentation I suppose. Moreover, this might be the
the "first" used value when set_max_mtu callback was introduced.

I'm not playing dumb, I just don't understand what is meant by
"in-driver standard value". Is it the return value of mv88e6xxx_get_max_mtu()
for the MV88E6185 chip? Because I understood it to be somehow the value
returned by default, for chips which don't have a way to change the MTU
(because of the "standard" word).

quoted

quoted

On the other hand - mv88e6250 supports 2048 bytes.  

What you mean to suggest here is that, using the current
classification from mv88e6xxx_get_max_mtu(), mv88e6250 falls into the
"none of the above" bucket, for which the driver returns 1522 -
VLAN_ETH_HLEN - EDSA_HLEN - ETH_FCS_LEN // 1492. But it truly
supports a maximum frame length of 2048, per your research.

And this cannot be easily fixed.

I could just provide callback to .set_max_frame_size in mv88e6250_ops
and the mv88e6250 would use 1632 bytes which would prevent errors.

However, when I dig deeper - it turned out that this value is larger.
And hence I wanted to do it in "right way".

Correct, I'm not debating this. I'm just saying, as a reader of this
patch set in linear order, that the justification is not obvious.

quoted

I have also noticed that you have not acted upon my previous review
comment:
https://patchwork.kernel.org/project/netdevbpf/patch/20230106101651.1137755-1-lukma@denx.de/

| 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?

I guess I will have to fix this now, since you haven't done it.

I do agree with Russel's reply here.

It's possible that Russell might have slightly misunderstood my quoted
reply here, because he said something about a PHY.

Moreover, as 6250 and 6220 also have max frame size equal to 2048
bytes, this would be fixed in v6 after getting the "validation"
function run.

The problem with this kind of fix is that it should go to the "net" tree;
it removes a user-visible warning that could have been avoided.

OTOH, the kind of "fix" for 6250 and 6220 is different. It is
sub-optimal use of hardware capabilities. That classifies as net-next
material.

The 2 go to different kernel branches.

This is the "patch 4" in the comment sent by Russel (to fix stuff which
is already broken, but it has been visible after running the validation
code):

https://lists.openwall.net/netdev/2023/03/09/233

I will get there.. eventually.