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. _______________________________________________ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev