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

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

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