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

-----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:

quoted

quoted

quoted

--- 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);

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

--
With Best Regards,
Andy Shevchenko