On Wed, Sep 07, 2016 at 10:31:12PM +0200, Andrew Lunn wrote:
> Hi Jarod
>
> > - /* MTU must be positive. */
> > - if (new_mtu < 0)
> > + if (new_mtu < dev->min_mtu) {
> > + netdev_err(dev, "Invalid MTU %d requested, hw min %d\n",
> > + new_mtu, dev->min_mtu);
> > return -EINVAL;
> > + }
> > +
> > + if (new_mtu > dev->max_mtu) {
> > + netdev_err(dev, "Invalid MTU %d requested, hw max %d\n",
> > + new_mtu, dev->min_mtu);
> > + return -EINVAL;
> > + }
>
> I doubt you can make such a big change like this in one go. Can you
> really guarantee all interfaces, of what ever type, will have some
> value for dev->min_mtu and dev->max_mtu? What may fly is something
> more like:
>
> > + if (dev->max_mtu && new_mtu > dev->max_mtu) {
> > + netdev_err(dev, "Invalid MTU %d requested, hw max %d\n",
> > + new_mtu, dev->min_mtu);
> > + return -EINVAL;
> > + }
>
> Maybe in a few cycles you can add a WARN_ON(!dev->max_mtu), and a few
> cycles after that go with (new_mtu > dev->max_mtu).
My local tree actually has if (dev->max_mtu > 0 && new_mtu > dev->max_mtu)
since just after I'd sent my mail for exactly that reason, though looking
at alloc_netdev_mqs(), it does seem we're at least guaranteed the value
will be 0 if not otherwise initialized, so your version looks perfectly
fine, and in the min_mtu case, without any additional handling, things
behave exactly as they did before. This is definitely going to require a
few passes... (Working my way through every driver with an ndo_change_mtu
wired up right now to see just how crazy this might get).
--
Jarod Wilson
ja...@redhat.com
