> -----Original Message-----
> From: Andy Shevchenko [mailto:[email protected]]
> Sent: Saturday, January 07, 2017 8:07 AM
> To: Kweh, Hock Leong <[email protected]>
> Cc: David S. Miller <[email protected]>; Joao Pinto
> <[email protected]>; Giuseppe CAVALLARO <[email protected]>;
> [email protected]; Jarod Wilson <[email protected]>; Alexandre
> TORGUE <[email protected]>; Joachim Eastwood
> <[email protected]>; Niklas Cassel <[email protected]>; Johan Hovold
> <[email protected]>; Pavel Machek <[email protected]>; [email protected];
> netdev <[email protected]>; LKML <[email protected]>
> 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
> <[email protected]> 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
