Re: [PATCH 3/4] net: dsa: realtek: rtl8365mb: Add setting MTU

[Andrew Lunn <andrew@lunn.ch>]

> > +static int rtl8365mb_port_change_mtu(struct dsa_switch *ds, int port,
> > +                                    int new_mtu)
> > +{
> > +       struct dsa_port *dp = dsa_to_port(ds, port);
> > +       struct realtek_priv *priv = ds->priv;
> > +       int length;
> > +
> > +       /* When a new MTU is set, DSA always set the CPU port's MTU to the
> > +        * largest MTU of the slave ports. Because the switch only has a global
> > +        * RX length register, only allowing CPU port here is enough.
> > +        */
> > +       if (!dsa_is_cpu_port(ds, port))
> > +               return 0;
> > +
> > +       length = new_mtu + ETH_HLEN + ETH_FCS_LEN;
> > +       length += dp->tag_ops->needed_headroom;
> > +       length += dp->tag_ops->needed_tailroom;
> 
> Isn't it better to keep that within the driver? No matter the tag
> position, it will be either 4 (RTL8365MB_CPU_FORMAT_4BYTES) or 8
> (RTL8365MB_CPU_FORMAT_8BYTES) bytes. You can retrieve that from
> priv->chip_data->cpu->format, but the driver will probably never
> support RTL8365MB_CPU_FORMAT_4BYTES. Until someone does implement the
> 4-bytes tag (for some mysterious reason), I believe we could simply
> use a constant here (using a proper new macro).

Another option is to simply always use the bigger header length. I
doubt there are many people actually using jumbo frames, and do they
really care about 0x3FFF-4, vs 0x3FFF-8?
 
> > +
> > +       if (length > RTL8365MB_CFG0_MAX_LEN_MASK)
> > +               return -EINVAL;
> > +
> > +       return regmap_update_bits(priv->map, RTL8365MB_CFG0_MAX_LEN_REG,
> > +                                 RTL8365MB_CFG0_MAX_LEN_MASK,
> > +                                 FIELD_PREP(RTL8365MB_CFG0_MAX_LEN_MASK,
> > +                                            length));
> > +}
> > +
> > +static int rtl8365mb_port_max_mtu(struct dsa_switch *ds, int port)
> > +{
> > +       return RTL8365MB_CFG0_MAX_LEN_MASK - ETH_HLEN - ETH_FCS_LEN - 8;
> 
> What is this magic 8? RTL8_4_TAG_LEN?

There are some DSA headers in include/linux/dsa, probably a new one
should be added with this RTL8_4_TAG_LEN.
 
	Andrew