Date:   Thu, 9 Mar 2023 16:42:27 +0100
From:   Andrew Lunn <andrew@lunn.ch>
To:     Lukasz Majewski <lukma@denx.de>
Cc:     "Russell King (Oracle)" <linux@armlinux.org.uk>,
        Vladimir Oltean <olteanv@gmail.com>,
        Eric Dumazet <edumazet@google.com>,
        Florian Fainelli <f.fainelli@gmail.com>,
        "David S. Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        Alexander Duyck <alexander.duyck@gmail.com>,
        netdev@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 6/7] dsa: marvell: Correct value of max_frame_size
 variable after validation
Message-ID: <0959097a-35cb-48c1-8e88-5e6c1269852d@lunn.ch>
References: <20230309125421.3900962-1-lukma@denx.de>
 <20230309125421.3900962-7-lukma@denx.de>
 <ZAnnk5MZc0w4VkDE@shell.armlinux.org.uk>
 <20230309154350.0bdc54c8@wsk>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230309154350.0bdc54c8@wsk>
Precedence: bulk

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

	 Andrew