> -----Original Message-----
> From: Andy Shevchenko [mailto:[email protected]]
> Sent: Saturday, January 07, 2017 1: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 2:49 AM, Kweh, Hock Leong
> <[email protected]> wrote:
> > From: "Kweh, Hock Leong" <[email protected]>
> >
> > There is no checking valid value of maxmtu when getting it from device tree.
> > This resolution added the checking condition to ensure the assignment is
> > made within a valid range.
>
> > --- 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)
>
> > +
>
> The lines are logically grouped here. No need to split it. Thus,
> remove this extra line.
>
Ok noted.
> > + 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?
> > + netdev_warn(priv->dev,
> > + "%s: warning: maxmtu having invalid value
> > (%d)\n",
> > + __func__, priv->plat->maxmtu);
>
>
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c
> > @@ -204,6 +204,10 @@ static int stmmac_pci_probe(struct pci_dev *pdev,
> >
> > pci_set_master(pdev);
> >
> > + /* 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()?
Anyway, this patch is focus for fixing the maxmtu assignment in valid range.
I will submit V4 to put "plat->maxmtu = JUMBO_LEN;" into each *_default_data().
Regarding the common_default_data() would be a new patch for better code
structuring. Thanks.
> --
> With Best Regards,
> Andy Shevchenko
