RE: [PATCH v3] net: stmmac: fix maxmtu assignment to be within valid range

[Kweh, Hock Leong]

> -----Original Message-----
> From: Andy Shevchenko [mailto:andy.shevchenko@xxxxxxxxx]
> Sent: Saturday, January 07, 2017 8:07 AM
> To: Kweh, Hock Leong <hock.leong.kweh@xxxxxxxxx>
> Cc: David S. Miller <davem@xxxxxxxxxxxxx>; Joao Pinto
> <Joao.Pinto@xxxxxxxxxxxx>; Giuseppe CAVALLARO <peppe.cavallaro@xxxxxx>;
> seraphin.bonnaffe@xxxxxx; Jarod Wilson <jarod@xxxxxxxxxx>; Alexandre
> TORGUE <alexandre.torgue@xxxxxxxxx>; Joachim Eastwood
> <manabian@xxxxxxxxx>; Niklas Cassel <niklas.cassel@xxxxxxxx>; Johan Hovold
> <johan@xxxxxxxxxx>; Pavel Machek <pavel@xxxxxx>; lars.persson@xxxxxxxx;
> netdev <netdev@xxxxxxxxxxxxxxx>; LKML <linux-kernel@xxxxxxxxxxxxxxx>
> Subject: Re: [PATCH v3] net: stmmac: fix maxmtu assignment to be within valid
> range
> 
> On Sat, Jan 7, 2017 at 1:47 AM, Kweh, Hock Leong
> <hock.leong.kweh@xxxxxxxxx> wrote:
> 
> >> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> >> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> >> > @@ -3345,8 +3345,14 @@ int stmmac_dvr_probe(struct device *device,
> >> >                 ndev->max_mtu = JUMBO_LEN;
> >> >         else
> >> >                 ndev->max_mtu = SKB_MAX_HEAD(NET_SKB_PAD +
> NET_IP_ALIGN);
> >> > -       if (priv->plat->maxmtu < ndev->max_mtu)
> >>
> >> > +       if ((priv->plat->maxmtu < ndev->max_mtu) &&
> >> > +           (priv->plat->maxmtu >= ndev->min_mtu))
> >>
> >> >                 ndev->max_mtu = priv->plat->maxmtu;
> >>
> >> > +       else if (priv->plat->maxmtu < ndev->min_mtu)
> >>
> >> And if it > ndev->max_mtu?..
> >>
> >
> > Base on my understanding to the original code, the "maxmtu >= ndev-
> >max_mtu"
> > is meant for products that would want to use the value from logic which is just
> above
> > this statement where you just ask me not to add new line. That the reason the
> > stmmac_platform.c put in "plat->maxmtu = JUMBO_LEN;" as generic and I
> also
> > follow it in stmmac_pci.c.
> >
> > Or do you mean only take maxmtu = JUMBO_LEN for the option to use driver
> itself
> > assignment statement above and all the > max_mtu consider invalid?
> 
> So, just answer to the simple question: is it a valid case to have
> plat->maxmtu > ndev->max_mtu? If it so, how is it used?
> Otherwise we need a warning in such case. What did I miss?
> 

it is a valid case for priv->plat->maxmtu > ndev->max_mtu if referring
to the statement above it:

	/* MTU range: 46 - hw-specific max */
	ndev->min_mtu = ETH_ZLEN - ETH_HLEN;
	if ((priv->plat->enh_desc) || (priv->synopsys_id >= DWMAC_CORE_4_00))
		ndev->max_mtu = JUMBO_LEN;
	else
		ndev->max_mtu = SKB_MAX_HEAD(NET_SKB_PAD + NET_IP_ALIGN);

When the ndev->max_mtu go into the else statement, then the assignment in
stmmac_platform.c & stammac_pci.c plat->maxmtu = JUMBO_LEN is actually 
greater than ndev->max_mtu. That is what I understanding that maxmtu > max_mtu
is an option trick to allow driver assign value through the logic above instead of getting
it from of_property_read_u32(np, "max-frame-size", &plat->maxmtu); or *_default_data().

I need to revert back the V4 and submit V5.

> >
> >> > +               netdev_warn(priv->dev,
> >> > +                           "%s: warning: maxmtu having invalid value (%d)\n",
> >> > +                           __func__, priv->plat->maxmtu);
> 
> 
> >> > +       /* Set the maxmtu to a default of JUMBO_LEN in case the
> >> > +        * parameter is not defined for the device.
> >> > +        */
> >> > +       plat->maxmtu = JUMBO_LEN;
> >>
> >> Please, use *_default_data() hooks for that.
> >>
> >> At some point it might make sense to extract
> >> static int common_default_data() {...}
> >> and call it at the beginning of the rest of *_defautl_data() hooks.
> >>
> >
> > Just try to understand, are you referring changing the code something
> > like this:
> >
> >         stmmac_default_data(plat);
> >         if (info) {
> >                 info->pdev = pdev;
> >                 if (info->setup) {
> >                         ret = info->setup(plat, info);
> >                         if (ret)
> >                                 return ret;
> >                 }
> >         }
> >
> > Where all the common code is inside the stmmac_default_data()?
> 
> No.
> 
> common_default_data()
> {
>  ... common defaults among *_default_data() ...
> }
> 
> *_default_data()
> {
> ...
>  common_default_data();
>  ...
> }
> 

Ok noted. Will be a separate patch. Thanks.

Regards,
Wilson

> --
> With Best Regards,
> Andy Shevchenko