| Message ID | 1483750143-11966-1-git-send-email-hock.leong.kweh@intel.com |
|---|---|
| State | Superseded, archived |
| Delegated to: | David Miller |
| Headers | show |
Hi Wilson, Às 12:49 AM de 1/7/2017, Kweh, Hock Leong escreveu: > From: "Kweh, Hock Leong" <hock.leong.kweh@intel.com> > > 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. > > Signed-off-by: Kweh, Hock Leong <hock.leong.kweh@intel.com> > --- > changelog v3: > * print the warning message only if maxmtu < min_mtu > * add maxmtu = JUMBO_LEN at stmmac_pci.c to follow stmmac_platform.c > > changelog v2: > * correction of "devicetree" to "device tree" reported by Andy > * print warning message while maxmtu is not in valid range > > drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 8 +++++++- > drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c | 4 ++++ > 2 files changed, 11 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > index 92ac006..ce74ae6 100644 > --- 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) > + netdev_warn(priv->dev, > + "%s: warning: maxmtu having invalid value (%d)\n", > + __func__, priv->plat->maxmtu); > > if (flow_ctrl) > priv->flow_ctrl = FLOW_AUTO; /* RX/TX pause on */ > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c > index a283177..e539afe 100644 > --- 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; I suggest to put this configuration in one of the default config functions. Tahnks. > if (info) { > info->pdev = pdev; > if (info->setup) { >
On Sat, Jan 7, 2017 at 2:49 AM, Kweh, Hock Leong <hock.leong.kweh@intel.com> wrote: > From: "Kweh, Hock Leong" <hock.leong.kweh@intel.com> > > 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. > + 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?.. > + 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.
> -----Original Message----- > From: Joao Pinto [mailto:Joao.Pinto@synopsys.com] > Sent: Saturday, January 07, 2017 12:58 AM > To: Kweh, Hock Leong <hock.leong.kweh@intel.com>; 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>; Andy Shevchenko <andy.shevchenko@gmail.com> > Cc: Alexandre TORGUE <alexandre.torgue@gmail.com>; Joachim Eastwood > <manabian@gmail.com>; Niklas Cassel <niklas.cassel@axis.com>; Johan Hovold > <johan@kernel.org>; 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 > > > Hi Wilson, > > Às 12:49 AM de 1/7/2017, Kweh, Hock Leong escreveu: > > From: "Kweh, Hock Leong" <hock.leong.kweh@intel.com> > > > > 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. > > > > Signed-off-by: Kweh, Hock Leong <hock.leong.kweh@intel.com> > > --- > > changelog v3: > > * print the warning message only if maxmtu < min_mtu > > * add maxmtu = JUMBO_LEN at stmmac_pci.c to follow stmmac_platform.c > > > > changelog v2: > > * correction of "devicetree" to "device tree" reported by Andy > > * print warning message while maxmtu is not in valid range > > > > drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 8 +++++++- > > drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c | 4 ++++ > > 2 files changed, 11 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > > index 92ac006..ce74ae6 100644 > > --- 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) > > + netdev_warn(priv->dev, > > + "%s: warning: maxmtu having invalid value (%d)\n", > > + __func__, priv->plat->maxmtu); > > > > if (flow_ctrl) > > priv->flow_ctrl = FLOW_AUTO; /* RX/TX pause on */ > > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c > b/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c > > index a283177..e539afe 100644 > > --- 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; > > I suggest to put this configuration in one of the default config functions. > > Tahnks. > My original thought is to have one line for all *_default_data() instead of having multiple same line inside each *_default_data(). If this is not a big concern of future expand, I can do that. Thanks. > > if (info) { > > info->pdev = pdev; > > if (info->setup) { > >
> -----Original Message----- > From: Andy Shevchenko [mailto:andy.shevchenko@gmail.com] > Sent: Saturday, January 07, 2017 1: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 2:49 AM, Kweh, Hock Leong > <hock.leong.kweh@intel.com> wrote: > > From: "Kweh, Hock Leong" <hock.leong.kweh@intel.com> > > > > 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
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? > >> > + 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(); ... }
> -----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
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c index 92ac006..ce74ae6 100644 --- 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) + netdev_warn(priv->dev, + "%s: warning: maxmtu having invalid value (%d)\n", + __func__, priv->plat->maxmtu); if (flow_ctrl) priv->flow_ctrl = FLOW_AUTO; /* RX/TX pause on */ diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c index a283177..e539afe 100644 --- 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; if (info) { info->pdev = pdev; if (info->setup) {