Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 7 Dec 2022 13:39:58 +0200
From:   Vladimir Oltean <olteanv@gmail.com>
To:     Jerry.Ray@microchip.com
Cc:     andrew@lunn.ch, f.fainelli@gmail.com, davem@davemloft.net,
        edumazet@google.com, kuba@kernel.org, pabeni@redhat.com,
        netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux@armlinux.org.uk
Subject: Re: [PATCH net-next v3 1/2] dsa: lan9303: Add port_max_mtu API
Message-ID: <20221207113958.6xp64kcp5ekwmlaa@skbuf>
References: <20221206183500.6898-1-jerry.ray@microchip.com>
 <20221206183500.6898-2-jerry.ray@microchip.com>
 <20221206185616.2ksuvlcmgelsfvw5@skbuf>
 <MWHPR11MB16930878ED42D3F4CC95A015EF1B9@MWHPR11MB1693.namprd11.prod.outlook.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <MWHPR11MB16930878ED42D3F4CC95A015EF1B9@MWHPR11MB1693.namprd11.prod.outlook.com>
Precedence: bulk

On Tue, Dec 06, 2022 at 11:44:58PM +0000, Jerry.Ray@microchip.com wrote:
> > > +/* For non-cpu ports, the max frame size is 1518.
> > > + * The CPU port supports a max frame size of 1522.
> > > + * There is a JUMBO flag to make the max size 2048, but this driver
> > > + * presently does not support using it.
> > > + */
> > > +static int lan9303_port_max_mtu(struct dsa_switch *ds, int port)
> > > +{
> > > +     struct net_device *p = dsa_port_to_master(dsa_to_port(ds, port));
> > 
> > You can put debugging prints in the code, but please, in the code that
> > you submit, do remove gratuitous poking in the master net_device.
> > 
> > > +     struct lan9303 *chip = ds->priv;
> > > +
> > > +     dev_dbg(chip->dev, "%s(%d) entered. NET max_mtu is %d",
> > > +             __func__, port, p->max_mtu);
> > > +
> > > +     if (dsa_port_is_cpu(dsa_to_port(ds, port)))
> > 
> > The ds->ops->port_max_mtu() function is never called for the CPU port.
> > You must know this, you put a debugging print right above. If this would
> > have been called for anything other than user ports, dsa_port_to_master()
> > would have triggered a NULL pointer dereference (dp->cpu_dp is set to
> > NULL for CPU ports).
> > 
> > So please remove dead code.
> > 
> 
> I've written the function to handle being called with any port.  While I
> couldn't directly exercise calling the port_max_mtu with the cpu port, I did
> simulate it to verify it would work.
> 
> I'm using the dsa_to_port() rather than the dsa_port_to_master() function.

No, you're using the dsa_to_port() *and* the dsa_port_to_master() functions.
See? It's in the code you posted:

static int lan9303_port_max_mtu(struct dsa_switch *ds, int port)
{
	struct net_device *p = dsa_port_to_master(dsa_to_port(ds, port));
			       ~~~~~~~~~~~~~~~~~~

> I'd rather include support for calling the api with the cpu port. I didn't
> want to assume otherwise.  That's why I don't consider this dead code.
> 
> > > +             return 1522 - ETH_HLEN - ETH_FCS_LEN;
> > > +     else
> > > +             return 1518 - ETH_HLEN - ETH_FCS_LEN;
> > 
> > Please replace "1518 - ETH_HLEN - ETH_FCS_LEN" with "ETH_DATA_LEN".
> > 
> > Which brings me to a more serious question. If you say that the max_mtu
> > is equal to the default interface MTU (1500), and you provide no means
> > for the user to change the MTU to a different value, then why write the
> > patch? What behaves differently with and without it?
> > 
> 
> I began adding the port_max_mtu api to attempt to get rid of the following
> error message:
> "macb f802c000.ethernet eth0: error -22 setting MTU to 1504 to include DSA overhead"

And how well did that go? That error message is saying that the macb driver
(drivers/net/ethernet/cadence/macb_main.c) does not accept the MTU of 1504.
Maybe because it doesn't have MACB_CAPS_JUMBO, I don't know. But this
patch is clearly unrelated to the problem you've observed.

> If someone were to check the max_mtu supported on the CPU port of the LAN9303,
> they would see that 1504 is okay.

No, they would not see that 1504 is okay. They would get a NULL pointer
dereference in your function, if port_max_mtu() was ever called for a
CPU port.

Don't believe me? You don't even have to. Please look at this patch,
study it, run it, and see what happens with your port_max_mtu()
implementation.

diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
index e5f156940c67..636e4b4df79a 100644
--- a/net/dsa/dsa.c
+++ b/net/dsa/dsa.c
@@ -473,6 +473,12 @@ static int dsa_port_setup(struct dsa_port *dp)
 			break;
 		dsa_port_enabled = true;
 
+		if (ds->ops->port_max_mtu) {
+			dev_info(ds->dev, "max MTU of CPU port %d is %d\n",
+				 dp->index,
+				 ds->ops->port_max_mtu(ds, dp->index));
+		}
+
 		break;
 	case DSA_PORT_TYPE_DSA:
 		if (dp->dn) {

The max_mtu of the CPU port is simply a question that the DSA core does
not ask, so there's no reason to report it. How things are supposed to
work is that the max_mtu of the user ports is propagated to their
net_devices, and when the MTU of any user port is changed, the
port_change_mtu() of that user port is called, and the maximum MTU of
all user ports is recalculated and all CPU and DSA ports also get a
port_change_mtu() call with that maximum value. If those ports need to
program their hardware with something that also includes their tagging
protocol overhead, they do so privately.