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 1: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 2:49 AM, Kweh, Hock Leong
> <hock.leong.kweh@xxxxxxxxx> wrote:
> > From: "Kweh, Hock Leong" <hock.leong.kweh@xxxxxxxxx>
> >
> > 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