Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1553521

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

Show all headers | View raw

> -----Original Message-----
> From: Andy Shevchenko [mailto:andy.shevchenko@gmail.com]
> Sent: Saturday, January 07, 2017 8:07 AM
> To: Kweh, Hock Leong <hock.leong.kweh@intel.com>
> Cc: David S. Miller <davem@davemloft.net>; Joao Pinto
> <Joao.Pinto@synopsys.com>; Giuseppe CAVALLARO <peppe.cavallaro@st.com>;
> seraphin.bonnaffe@st.com; Jarod Wilson <jarod@redhat.com>; Alexandre
> TORGUE <alexandre.torgue@gmail.com>; Joachim Eastwood
> <manabian@gmail.com>; Niklas Cassel <niklas.cassel@axis.com>; Johan Hovold
> <johan@kernel.org>; Pavel Machek <pavel@ucw.cz>; lars.persson@axis.com;
> netdev <netdev@vger.kernel.org>; LKML <linux-kernel@vger.kernel.org>
> 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@intel.com> 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

Back to linux.kernel | Previous | Next — Previous in thread | Find similar | Unroll thread

Thread

[PATCH v3] net: stmmac: fix maxmtu assignment to be within valid range "Kweh, Hock Leong" <hock.leong.kweh@intel.com> - 2017-01-06 17:50 +0100
  Re: [PATCH v3] net: stmmac: fix maxmtu assignment to be within valid  range Joao Pinto <Joao.Pinto@synopsys.com> - 2017-01-06 18:00 +0100
    RE: [PATCH v3] net: stmmac: fix maxmtu assignment to be within  valid range "Kweh, Hock Leong" <hock.leong.kweh@intel.com> - 2017-01-07 00:40 +0100
  Re: [PATCH v3] net: stmmac: fix maxmtu assignment to be within valid range Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-01-06 18:10 +0100
    RE: [PATCH v3] net: stmmac: fix maxmtu assignment to be within  valid range "Kweh, Hock Leong" <hock.leong.kweh@intel.com> - 2017-01-07 00:50 +0100
      Re: [PATCH v3] net: stmmac: fix maxmtu assignment to be within valid range Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-01-07 01:10 +0100
        RE: [PATCH v3] net: stmmac: fix maxmtu assignment to be within  valid range "Kweh, Hock Leong" <hock.leong.kweh@intel.com> - 2017-01-07 02:00 +0100

csiph-web