RE: [PATCH net-next v3 1/2] dsa: lan9303: Add port_max_mtu API

From: Jerry.Ray
Date: Tue Dec 06 2022 - 18:45:07 EST


> > +/* 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.

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"

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

> > +}
> > +
> > static const struct dsa_switch_ops lan9303_switch_ops = {
> > .get_tag_protocol = lan9303_get_tag_protocol,
> > .setup = lan9303_setup,
> > @@ -1299,6 +1318,7 @@ static const struct dsa_switch_ops lan9303_switch_ops = {
> > .port_fdb_dump = lan9303_port_fdb_dump,
> > .port_mdb_add = lan9303_port_mdb_add,
> > .port_mdb_del = lan9303_port_mdb_del,
> > + .port_max_mtu = lan9303_port_max_mtu,
> > };
>

Regards,
Jerry.