Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1552950 > unrolled thread
| Started by | "Kweh, Hock Leong" <hock.leong.kweh@intel.com> |
|---|---|
| First post | 2017-01-06 17:50 +0100 |
| Last post | 2017-01-07 02:00 +0100 |
| Articles | 7 — 3 participants |
Back to article view | Back to linux.kernel
[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
| From | "Kweh, Hock Leong" <hock.leong.kweh@intel.com> |
|---|---|
| Date | 2017-01-06 17:50 +0100 |
| Subject | [PATCH v3] net: stmmac: fix maxmtu assignment to be within valid range |
| Message-ID | <sWFSh-4A4-15@gated-at.bofh.it> |
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;
if (info) {
info->pdev = pdev;
if (info->setup) {
--
1.7.9.5
[toc] | [next] | [standalone]
| From | Joao Pinto <Joao.Pinto@synopsys.com> |
|---|---|
| Date | 2017-01-06 18:00 +0100 |
| Subject | Re: [PATCH v3] net: stmmac: fix maxmtu assignment to be within valid range |
| Message-ID | <sWG23-4DR-19@gated-at.bofh.it> |
| In reply to | #1552950 |
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) {
>
[toc] | [prev] | [next] | [standalone]
| From | "Kweh, Hock Leong" <hock.leong.kweh@intel.com> |
|---|---|
| Date | 2017-01-07 00:40 +0100 |
| Subject | RE: [PATCH v3] net: stmmac: fix maxmtu assignment to be within valid range |
| Message-ID | <sWMh3-Fc-13@gated-at.bofh.it> |
| In reply to | #1552959 |
> -----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) {
> >
[toc] | [prev] | [next] | [standalone]
| From | Andy Shevchenko <andy.shevchenko@gmail.com> |
|---|---|
| Date | 2017-01-06 18:10 +0100 |
| Message-ID | <sWGbE-4WL-33@gated-at.bofh.it> |
| In reply to | #1552950 |
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.
--
With Best Regards,
Andy Shevchenko
[toc] | [prev] | [next] | [standalone]
| From | "Kweh, Hock Leong" <hock.leong.kweh@intel.com> |
|---|---|
| Date | 2017-01-07 00:50 +0100 |
| Subject | RE: [PATCH v3] net: stmmac: fix maxmtu assignment to be within valid range |
| Message-ID | <sWMqJ-Iz-17@gated-at.bofh.it> |
| In reply to | #1552972 |
> -----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
[toc] | [prev] | [next] | [standalone]
| From | Andy Shevchenko <andy.shevchenko@gmail.com> |
|---|---|
| Date | 2017-01-07 01:10 +0100 |
| Message-ID | <sWMK6-14E-9@gated-at.bofh.it> |
| In reply to | #1553508 |
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();
...
}
--
With Best Regards,
Andy Shevchenko
[toc] | [prev] | [next] | [standalone]
| From | "Kweh, Hock Leong" <hock.leong.kweh@intel.com> |
|---|---|
| Date | 2017-01-07 02:00 +0100 |
| Subject | RE: [PATCH v3] net: stmmac: fix maxmtu assignment to be within valid range |
| Message-ID | <sWNwt-1nt-1@gated-at.bofh.it> |
| In reply to | #1553512 |
> -----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
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web