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
Date: Wed, 7 Dec 2022 13:39:58 +0200 [thread overview]
Message-ID: <20221207113958.6xp64kcp5ekwmlaa@skbuf> (raw)
In-Reply-To: <MWHPR11MB16930878ED42D3F4CC95A015EF1B9@MWHPR11MB1693.namprd11.prod.outlook.com>
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.
next prev parent reply other threads:[~2022-12-07 11:40 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-12-06 18:34 [PATCH net-next v3 0/2] dsa: lan9303: Move to PHYLINK Jerry Ray
2022-12-06 18:34 ` [PATCH net-next v3 1/2] dsa: lan9303: Add port_max_mtu API Jerry Ray
2022-12-06 18:56 ` Vladimir Oltean
2022-12-06 23:44 ` Jerry.Ray
2022-12-07 11:39 ` Vladimir Oltean [this message]
2022-12-06 18:35 ` [PATCH net-next v3 2/2] dsa: lan9303: Move to PHYLINK Jerry Ray
2022-12-06 19:32 ` Vladimir Oltean
2022-12-06 21:07 ` Russell King (Oracle)
2022-12-06 22:12 ` Vladimir Oltean
2022-12-06 22:58 ` Jerry.Ray
2022-12-07 14:01 ` Vladimir Oltean
2022-12-06 18:57 ` [PATCH net-next v3 0/2] " Vladimir Oltean
2022-12-06 23:58 ` Jerry.Ray
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20221207113958.6xp64kcp5ekwmlaa@skbuf \
--to=olteanv@gmail.com \
--cc=Jerry.Ray@microchip.com \
--cc=andrew@lunn.ch \
--cc=davem@davemloft.net \
--cc=edumazet@google.com \
--cc=f.fainelli@gmail.com \
--cc=kuba@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux@armlinux.org.uk \
--cc=netdev@vger.kernel.org \
--cc=pabeni@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).