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

[Liang Li <liang.li@windriver.com>]

On Thu, Sep 02, 2010 at 07:04:59PM +0100, Ben Hutchings wrote:
> 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.

dev_close is safe enough IMHO. Call dev_close repeatly won't cause
problem though.

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

I checked NIC drivers in drivers/net, there is no such:

	dev_close(netdev);
	ret = dev_open(netdev)
	if (ret)
		netdev_err(...);

Instead, there are:

	nic_driver_close/down(netdev);
	ret = nic_driver_open/restart(netdev);
	if (ret) {
		waring;
		dev_close(netdev);
	}

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

I think I did not invite new bugs, as I mentioned before, can you show
scenario that the reopen fail and perfect cleanup way?

Thanks,
				-Liang Li

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