linux-kernel - Re: [PATCH 1/7] dsa: marvell: Provide per device information about max frame size

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20230310133651.pfqldx6jdgssbe54@skbuf>
Date:   Fri, 10 Mar 2023 15:36:51 +0200
From:   Vladimir Oltean <olteanv@...il.com>
To:     Lukasz Majewski <lukma@...x.de>
Cc:     Andrew Lunn <andrew@...n.ch>, Russell King <linux@...linux.org.uk>,
        Eric Dumazet <edumazet@...gle.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Alexander Duyck <alexander.duyck@...il.com>,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/7] dsa: marvell: Provide per device information about
 max frame size

On Fri, Mar 10, 2023 at 02:17:19PM +0100, Lukasz Majewski wrote:
> > > 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).

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

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