netdev - Re: [PATCH 6/7] dsa: marvell: Correct value of max_frame

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: <20230310125346.13f93f78@wsk>
Date:   Fri, 10 Mar 2023 12:53:46 +0100
From:   Lukasz Majewski <lukma@...x.de>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     "Russell King (Oracle)" <linux@...linux.org.uk>,
        Vladimir Oltean <olteanv@...il.com>,
        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 6/7] dsa: marvell: Correct value of max_frame_size
 variable after validation

Hi Andrew,

> > > If I understand this correctly, in patch 4, you add a call to the
> > > 6250 family to call mv88e6185_g1_set_max_frame_size(), which sets
> > > a bit called MV88E6185_G1_CTL1_MAX_FRAME_1632 if the frame size
> > > is larger than 1518.  
> > 
> > Yes, correct.
> >   
> > > 
> > > However, you're saying that 6250 has a frame size of 2048. That's
> > > fine, but it makes MV88E6185_G1_CTL1_MAX_FRAME_1632 rather
> > > misleading as a definition. While the bit may increase the frame
> > > size, I think if we're going to do this, then this definition
> > > ought to be renamed. 
> > 
> > I thought about rename, but then I've double checked; register
> > offset and exact bit definition is the same as for 6185, so to avoid
> > unnecessary code duplication - I've reused the existing function.
> > 
> > Maybe comment would be just enough?  
> 
> The driver takes care with its namespace in order to add per switch
> family defines. So you can add MV88E6250_G1_CTL1_MAX_FRAME_2048. It
> does not matter if it is the same bit. You can also add a
> mv88e6250_g1_set_max_frame_size() and it also does not matter if it is
> in effect the same as mv88e6185_g1_set_max_frame_size().
> 
> We should always make the driver understandably first, compact and
> without redundancy second. We are then less likely to get into
> situations like this again where it is not clear what MTU a device
> actually supports because the code is cryptic.

Ok, I will add new function.

Thanks for hints.

> 
> 	 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: lukma@...x.de

Content of type "application/pgp-signature" skipped