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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 9 May 2022 13:55:53 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Luiz Angelo Daros de Luca <luizluca@...il.com>
Cc:     Hauke Mehrtens <hauke@...ke-m.de>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Linus Walleij <linus.walleij@...aro.org>,
        Alvin Šipraga <alsi@...g-olufsen.dk>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Vladimir Oltean <olteanv@...il.com>,
        "open list:NETWORKING DRIVERS" <netdev@...r.kernel.org>
Subject: Re: [PATCH 3/4] net: dsa: realtek: rtl8365mb: Add setting MTU

> > +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