[v2 PATCH] ucc_geth: fix ethtool set ring param bug

[Ben Hutchings]

On Fri, 2010-09-03 at 00:02 +0800, Liang Li wrote:
> It's common sense that when we should do change to driver ring
> desc/buffer etc only after 'stop/shutdown' the device. When we
> do change while devices/driver is running, kernel oops occur:
[...]
> +		printk(KERN_INFO "Reactivating interface %s.\n", netdev->name);
> +		ret = ucc_geth_open(netdev);
> +		if (ret) {
> +			printk(KERN_WARNING "uec_set_ringparam: set ring param for running"
> +					" interface %s failed. Please try again.\n", netdev->name);
> +			dev_close(netdev);
[...]

If ucc_geth_open() failed you MUST NOT call ucc_geth_close(), but that
is what dev_close() is going to do.  But the device is still flagged as
running so 'ifconfig down' is going to call dev_close() as well.  There
is no way out.

This is why I said you must call dev_close() and then dev_open()
instead.  Then if dev_open() fails, just print the error, e.g.:

               dev_close(netdev);
               ret = dev_open(netdev);
               if (ret)
                       netdev_err(netdev,
                                  "uec_set_ringparam: failed to restart"
                                  " interface with new ring parameters\n");

(And I think this really is a serious error, hence the 'err' rather than
'warning' severity.)

(By the way, I noticed there are other places where ucc_geth_close() and
ucc_geth_open() are called, without error checking.  These are also
bugs, but that doesn't justify adding new bugs.)

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.