From: "Kweh, Hock Leong" <redacted>
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 <redacted>
---
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(-)
@@ -3345,8 +3345,14 @@ int stmmac_dvr_probe(struct device *device,ndev->max_mtu=JUMBO_LEN;elsendev->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;+elseif(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 */
@@ -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+*parameterisnotdefinedforthedevice.+*/+plat->maxmtu=JUMBO_LEN;if(info){info->pdev=pdev;if(info->setup){
Hi Wilson,
Às 12:49 AM de 1/7/2017, Kweh, Hock Leong escreveu:
quoted hunk
From: "Kweh, Hock Leong" <redacted>
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 <redacted>
---
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(-)
@@ -3345,8 +3345,14 @@ int stmmac_dvr_probe(struct device *device,ndev->max_mtu=JUMBO_LEN;elsendev->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;+elseif(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 */
@@ -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+*parameterisnotdefinedforthedevice.+*/+plat->maxmtu=JUMBO_LEN;
I suggest to put this configuration in one of the default config functions.
Tahnks.
From: Andy Shevchenko <hidden> Date: 2017-01-06 17:06:48
On Sat, Jan 7, 2017 at 2:49 AM, Kweh, Hock Leong
[off-list ref] wrote:
From: "Kweh, Hock Leong" <redacted>
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.
@@ -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+*parameterisnotdefinedforthedevice.+*/+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.
--
With Best Regards,
Andy Shevchenko
-----Original Message-----
From: Joao Pinto [mailto:Joao.Pinto@synopsys.com]
Sent: Saturday, January 07, 2017 12:58 AM
To: Kweh, Hock Leong <redacted>; David S. Miller
[off-list ref]; Joao Pinto [off-list ref]; Giuseppe
CAVALLARO [off-list ref]; seraphin.bonnaffe@st.com; Jarod
Wilson [off-list ref]; Andy Shevchenko [off-list ref]
Cc: Alexandre TORGUE <redacted>; Joachim Eastwood
[off-list ref]; Niklas Cassel [off-list ref]; Johan Hovold
[off-list ref]; pavel@ucw.cz; lars.persson@axis.com; netdev
[off-list ref]; LKML [off-list ref]
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:
quoted
From: "Kweh, Hock Leong" <redacted>
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 <redacted>
---
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(-)
@@ -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+*parameterisnotdefinedforthedevice.+*/+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.
-----Original Message-----
From: Andy Shevchenko [mailto:andy.shevchenko@gmail.com]
Sent: Saturday, January 07, 2017 1:07 AM
To: Kweh, Hock Leong <redacted>
Cc: David S. Miller <davem@davemloft.net>; Joao Pinto
[off-list ref]; Giuseppe CAVALLARO [off-list ref];
seraphin.bonnaffe@st.com; Jarod Wilson [off-list ref]; Alexandre
TORGUE [off-list ref]; Joachim Eastwood
[off-list ref]; Niklas Cassel [off-list ref]; Johan Hovold
[off-list ref]; Pavel Machek [off-list ref]; lars.persson@axis.com;
netdev [off-list ref]; LKML [off-list ref]
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
[off-list ref] wrote:
quoted
From: "Kweh, Hock Leong" <redacted>
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.
@@ -3345,8 +3345,14 @@ int stmmac_dvr_probe(struct device *device,ndev->max_mtu=JUMBO_LEN;elsendev->max_mtu=SKB_MAX_HEAD(NET_SKB_PAD+NET_IP_ALIGN);-if(priv->plat->maxmtu<ndev->max_mtu)
quoted
+
The lines are logically grouped here. No need to split it. Thus,
remove this extra line.
Ok noted.
quoted
+ if ((priv->plat->maxmtu < ndev->max_mtu) &&
+ (priv->plat->maxmtu >= ndev->min_mtu))
quoted
ndev->max_mtu = priv->plat->maxmtu;
quoted
+ 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?
quoted
+ netdev_warn(priv->dev,
+ "%s: warning: maxmtu having invalid value (%d)\n",
+ __func__, priv->plat->maxmtu);
@@ -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+*parameterisnotdefinedforthedevice.+*/+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.
@@ -3345,8 +3345,14 @@ int stmmac_dvr_probe(struct device *device,ndev->max_mtu=JUMBO_LEN;elsendev->max_mtu=SKB_MAX_HEAD(NET_SKB_PAD+NET_IP_ALIGN);-if(priv->plat->maxmtu<ndev->max_mtu)
quoted
+ if ((priv->plat->maxmtu < ndev->max_mtu) &&
+ (priv->plat->maxmtu >= ndev->min_mtu))
quoted
ndev->max_mtu = priv->plat->maxmtu;
quoted
+ 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?
quoted
quoted
+ netdev_warn(priv->dev,
+ "%s: warning: maxmtu having invalid value (%d)\n",
+ __func__, priv->plat->maxmtu);
quoted
quoted
+ /* 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();
...
}
--
With Best Regards,
Andy Shevchenko
-----Original Message-----
From: Andy Shevchenko [mailto:andy.shevchenko@gmail.com]
Sent: Saturday, January 07, 2017 8:07 AM
To: Kweh, Hock Leong <redacted>
Cc: David S. Miller <davem@davemloft.net>; Joao Pinto
[off-list ref]; Giuseppe CAVALLARO [off-list ref];
seraphin.bonnaffe@st.com; Jarod Wilson [off-list ref]; Alexandre
TORGUE [off-list ref]; Joachim Eastwood
[off-list ref]; Niklas Cassel [off-list ref]; Johan Hovold
[off-list ref]; Pavel Machek [off-list ref]; lars.persson@axis.com;
netdev [off-list ref]; LKML [off-list ref]
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
[off-list ref] wrote:
@@ -3345,8 +3345,14 @@ int stmmac_dvr_probe(struct device *device,ndev->max_mtu=JUMBO_LEN;elsendev->max_mtu=SKB_MAX_HEAD(NET_SKB_PAD+
NET_IP_ALIGN);
quoted
quoted
quoted
- if (priv->plat->maxmtu < ndev->max_mtu)
quoted
+ if ((priv->plat->maxmtu < ndev->max_mtu) &&
+ (priv->plat->maxmtu >= ndev->min_mtu))
quoted
ndev->max_mtu = priv->plat->maxmtu;
quoted
+ 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
quoted
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
quoted
follow it in stmmac_pci.c.
Or do you mean only take maxmtu = JUMBO_LEN for the option to use driver
itself
quoted
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.
quoted
quoted
quoted
+ netdev_warn(priv->dev,
+ "%s: warning: maxmtu having invalid value (%d)\n",
+ __func__, priv->plat->maxmtu);
quoted
quoted
quoted
+ /* 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