Re: [patch 0/5][RFC] Update network drivers to use devres
On Fri, Aug 03, 2007 at 09:58:57AM +0100, Stephen Hemminger wrote: On Thu, 2 Aug 2007 15:42:06 -0700 Brandon Philips [EMAIL PROTECTED] wrote: This patch set adds support for devres in the net core and converts the e100 and e1000 drivers to devres. Devres is a simple resource manager for device drivers, see Documentation/driver-model/devres.txt for more information. The use of devres will remain optional for drivers with this patch set. Drivers can be converted when it makes sense. Just because devres exists is not sufficient motivation to change. It seems that devres was a band-aid rather than fixing storage drivers to have proper DMA lifetimes. I don't really get what you mean by having proper DMA lifetimes but please don't write devres off too fast. devres doesn't solve any problem that you can't fix without it but it does make the 'solving' much easier. IMHO, libata drivers generally have been well maintained and reviewed but I could still find quite a few bugs (resource leaks or occasionally double free) in init failure and removal paths. Init failure paths are especially prone to bugs because they don't get excercised often. It's just very easy to make a mistake and fail to notice and low level drivers don't always get sufficient amount of review or testing. Skimming through drivers... via-rhine doesn't disable PCI device on init failure path but does so on removal. sky2 doesn't free consistent memory if sky2_init() fails. acenic calls iounmap() with NULL parameter which I'm not sure whether it's safe or not. natsemi doesn't disable PCI device on failure or removal. Devres makes low level drivers simpler, easier to get right and maintain. Writing new drivers becomes easier too. So, why not? Network devices seem to work fine thanks, and the resource requirements are different. If ain't broke, don't fix it. Care to enlighten me on how the resource requirments are different from ATA drivers? Thanks. -- tejun - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch 0/5][RFC] Update network drivers to use devres
On Fri, 3 Aug 2007 19:26:45 +0900 Tejun Heo [EMAIL PROTECTED] wrote: On Fri, Aug 03, 2007 at 09:58:57AM +0100, Stephen Hemminger wrote: On Thu, 2 Aug 2007 15:42:06 -0700 Brandon Philips [EMAIL PROTECTED] wrote: This patch set adds support for devres in the net core and converts the e100 and e1000 drivers to devres. Devres is a simple resource manager for device drivers, see Documentation/driver-model/devres.txt for more information. The use of devres will remain optional for drivers with this patch set. Drivers can be converted when it makes sense. Just because devres exists is not sufficient motivation to change. It seems that devres was a band-aid rather than fixing storage drivers to have proper DMA lifetimes. I don't really get what you mean by having proper DMA lifetimes but please don't write devres off too fast. devres doesn't solve any problem that you can't fix without it but it does make the 'solving' much easier. IMHO, libata drivers generally have been well maintained and reviewed but I could still find quite a few bugs (resource leaks or occasionally double free) in init failure and removal paths. Init failure paths are especially prone to bugs because they don't get excercised often. It's just very easy to make a mistake and fail to notice and low level drivers don't always get sufficient amount of review or testing. Skimming through drivers... via-rhine doesn't disable PCI device on init failure path but does so on removal. sky2 doesn't free consistent memory if sky2_init() fails. acenic calls iounmap() with NULL parameter which I'm not sure whether it's safe or not. natsemi doesn't disable PCI device on failure or removal. Did you report these to the developers? Devres makes low level drivers simpler, easier to get right and maintain. Writing new drivers becomes easier too. So, why not? Network devices seem to work fine thanks, and the resource requirements are different. If ain't broke, don't fix it. Care to enlighten me on how the resource requirments are different from ATA drivers? I was thinking of the hot remove (no mod ref counts) and lingering /sys open issues. ATA drivers use ref counts. My take on devres is that it is similar to talloc() for device drivers. Not a bad idea in itself, but the real advantage of hierarchical allocation is that it makes exception handling easier if things are layered deeply. - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch 0/5][RFC] Update network drivers to use devres
On Fri, 03 Aug 2007 20:33:04 +0900 Tejun Heo [EMAIL PROTECTED] wrote: Hello, Stephen Hemminger wrote: Skimming through drivers... via-rhine doesn't disable PCI device on init failure path but does so on removal. sky2 doesn't free consistent memory if sky2_init() fails. acenic calls iounmap() with NULL parameter which I'm not sure whether it's safe or not. natsemi doesn't disable PCI device on failure or removal. Did you report these to the developers? Just skimmed through. I'm pretty sure Brandon will pick those up later. Devres makes low level drivers simpler, easier to get right and maintain. Writing new drivers becomes easier too. So, why not? Network devices seem to work fine thanks, and the resource requirements are different. If ain't broke, don't fix it. Care to enlighten me on how the resource requirments are different from ATA drivers? I was thinking of the hot remove (no mod ref counts) and lingering /sys open issues. ATA drivers use ref counts. I guess the hot removing is done by severing netdev from the actual device, right? I don't see how that affects usage of devres on network drivers. Am I missing something? The issue is that device may be removed at any time. So you can't rely on module ref counts to save you. And netdevice structure must still linger after module is removed, till dev ref count goes to zero. On a separate note, can you explain lingering /sys open issue to me a bit? With recent sysfs changes, sysfs nodes are disconnected immediately on deletion. Would that make any difference to netdevs? Examples are in Documentation/networking/netdevices.txt My take on devres is that it is similar to talloc() for device drivers. Not a bad idea in itself, but the real advantage of hierarchical allocation is that it makes exception handling easier if things are layered deeply. Yeah, devres made layering easier in libata, especially SFF stuff. Dunno how much of that is applicable to netdev but, with or without layering, it'll be a nice cleanup and I don't see much negative side. Conversion would take some work and bugs might be introduced in the process as with any changes but the good thing about devres is that you're very likely to get failure/release paths right if you get the init path right, and if you get the init path wrong, it will stand out like a sore thumb - easy to spot, easy to fix. So, I think using devres on net drivers is a good idea, well, for that matter, for any driver, but me being the devres writer, that isn't really surprising, is it? Thanks. -- tejun - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch 0/5][RFC] Update network drivers to use devres
Hello, Stephen Hemminger wrote: Skimming through drivers... via-rhine doesn't disable PCI device on init failure path but does so on removal. sky2 doesn't free consistent memory if sky2_init() fails. acenic calls iounmap() with NULL parameter which I'm not sure whether it's safe or not. natsemi doesn't disable PCI device on failure or removal. Did you report these to the developers? Just skimmed through. I'm pretty sure Brandon will pick those up later. Devres makes low level drivers simpler, easier to get right and maintain. Writing new drivers becomes easier too. So, why not? Network devices seem to work fine thanks, and the resource requirements are different. If ain't broke, don't fix it. Care to enlighten me on how the resource requirments are different from ATA drivers? I was thinking of the hot remove (no mod ref counts) and lingering /sys open issues. ATA drivers use ref counts. I guess the hot removing is done by severing netdev from the actual device, right? I don't see how that affects usage of devres on network drivers. Am I missing something? On a separate note, can you explain lingering /sys open issue to me a bit? With recent sysfs changes, sysfs nodes are disconnected immediately on deletion. Would that make any difference to netdevs? My take on devres is that it is similar to talloc() for device drivers. Not a bad idea in itself, but the real advantage of hierarchical allocation is that it makes exception handling easier if things are layered deeply. Yeah, devres made layering easier in libata, especially SFF stuff. Dunno how much of that is applicable to netdev but, with or without layering, it'll be a nice cleanup and I don't see much negative side. Conversion would take some work and bugs might be introduced in the process as with any changes but the good thing about devres is that you're very likely to get failure/release paths right if you get the init path right, and if you get the init path wrong, it will stand out like a sore thumb - easy to spot, easy to fix. So, I think using devres on net drivers is a good idea, well, for that matter, for any driver, but me being the devres writer, that isn't really surprising, is it? Thanks. -- tejun - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch 0/5][RFC] Update network drivers to use devres
On Thu, 2 Aug 2007 15:42:06 -0700 Brandon Philips [EMAIL PROTECTED] wrote: This patch set adds support for devres in the net core and converts the e100 and e1000 drivers to devres. Devres is a simple resource manager for device drivers, see Documentation/driver-model/devres.txt for more information. The use of devres will remain optional for drivers with this patch set. Drivers can be converted when it makes sense. Just because devres exists is not sufficient motivation to change. It seems that devres was a band-aid rather than fixing storage drivers to have proper DMA lifetimes. Network devices seem to work fine thanks, and the resource requirements are different. If ain't broke, don't fix it. - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch 0/5][RFC] Update network drivers to use devres
On 14:44 Fri 03 Aug 2007, Stephen Hemminger wrote:
On Fri, 03 Aug 2007 20:33:04 +0900 Tejun Heo [EMAIL PROTECTED] wrote:
Devres makes low level drivers simpler, easier to get right and
maintain. Writing new drivers becomes easier too. So, why not?
Network devices seem to work fine thanks, and the resource requirements
are different. If ain't broke, don't fix it.
Care to enlighten me on how the resource requirments are different
from ATA drivers?
I was thinking of the hot remove (no mod ref counts) and lingering
/sys open issues. ATA drivers use ref counts.
I guess the hot removing is done by severing netdev from the actual
device, right? I don't see how that affects usage of devres on network
drivers. Am I missing something?
The issue is that device may be removed at any time. So you can't rely
on module ref counts to save you. And netdevice structure must still
linger after module is removed, till dev ref count goes to zero.
These patches allow the net_device to linger. The code calls
free_netdev on device removal just as before.
This is how the net_device is handled on device removal by these
patches:
+static void devm_free_netdev(struct device *gendev, void *res)
+{
+ struct net_device *dev = dev_get_drvdata(gendev);
+ free_netdev(dev);
+}
On a separate note, can you explain lingering /sys open issue to me a
bit? With recent sysfs changes, sysfs nodes are disconnected
immediately on deletion. Would that make any difference to netdevs?
Examples are in Documentation/networking/netdevices.txt
Isn't this the same problem as above? The net_device structure must
stay around if there are still references to it and it does.
Or am I missing something?
Thanks,
Brandon
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
[patch 0/5][RFC] Update network drivers to use devres
This patch set adds support for devres in the net core and converts the e100 and e1000 drivers to devres. Devres is a simple resource manager for device drivers, see Documentation/driver-model/devres.txt for more information. The use of devres will remain optional for drivers with this patch set. Drivers can be converted when it makes sense. Builds on top of f0a664bbd1839fbe9f57564983f39bfc6c6f931d in Linus' tree which renames __pci_reenable_device() to pci_reenable_device() -- - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
