[patch 0/5][RFC] Update network drivers to use devres

[patch 0/5][RFC] Update network drivers to use devres

[Brandon Philips]

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

-- 

Re: [patch 0/5][RFC] Update network drivers to use devres

[Stephen Hemminger]

On Thu, 2 Aug 2007 15:42:06 -0700
Brandon Philips <brandon@ifup.org> 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.

Re: [patch 0/5][RFC] Update network drivers to use devres

[Tejun Heo]

On Fri, Aug 03, 2007 at 09:58:57AM +0100, Stephen Hemminger wrote:
> On Thu, 2 Aug 2007 15:42:06 -0700
> Brandon Philips <brandon@ifup.org> 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

Re: [patch 0/5][RFC] Update network drivers to use devres

[Stephen Hemminger]

On Fri, 3 Aug 2007 19:26:45 +0900
Tejun Heo <htejun@gmail.com> 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 <brandon@ifup.org> 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.

Re: [patch 0/5][RFC] Update network drivers to use devres

[Tejun Heo]

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

Re: [patch 0/5][RFC] Update network drivers to use devres

[Stephen Hemminger]

On Fri, 03 Aug 2007 20:33:04 +0900
Tejun Heo <htejun@gmail.com> 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

Re: [patch 0/5][RFC] Update network drivers to use devres

[Brandon Philips]

On 14:44 Fri 03 Aug 2007, Stephen Hemminger wrote:
> On Fri, 03 Aug 2007 20:33:04 +0900 Tejun Heo <htejun@gmail.com> 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