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

[<Jerry.Ray@microchip.com>]

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