On 01:38 Thu 16 Aug 2007, Waskiewicz Jr, Peter P wrote:
> > - err = -ENOMEM;
> > - netdev = alloc_etherdev(sizeof(struct e1000_adapter));
> > + netdev = devm_alloc_etherdev(&pdev->dev, sizeof(struct
> > +e1000_adapter));
> > if (!netdev)
> > - goto err_alloc_etherdev;
> > + return -ENOMEM;
>
> I'm a bit confused why you removed the goto's, and then removed all the
> target unwinding code at the bottom of e1000_probe(). Those labels
> clean up resources if something fails, like the err_sw_init label. I
> don't see anything in the devres code that jumps out at me that explains
> why we can do away with these cleanup routines. Thoughts?
Have you read Documentation/driver-model/devres.txt? That has a good
explanation. Here is a practical explanation on how it works too.
This is the output from a normal modprobe then rmmod of e1000 with
devres debugging on.
# modprobe
DEVRES ADD f7fd6cc0 pcim_release (8 bytes)
DEVRES ADD f7a80fe0 devm_free_netdev (4 bytes) # netdev **p for free_netdev
DEVRES ADD f7dbe780 pcim_iomap_release (24 bytes) # adapter->hw.hw_addr
DEVRES ADD f7dbe9c0 devm_kzalloc_release (40 bytes) # adapter->tx_ring
DEVRES ADD f7dbe8c0 devm_kzalloc_release (44 bytes) # adapter->rx_ring
# rmmod
DEVRES REL f7dbe8c0 devm_kzalloc_release (44 bytes) # adapter->tx_ring
DEVRES REL f7dbe9c0 devm_kzalloc_release (40 bytes) # adapter->rx_ring
DEVRES REL f7dbe780 pcim_iomap_release (24 bytes) # adapter->hw.hw_addr
DEVRES REL f7a80fe0 devm_free_netdev (4 bytes) # called free_netdev
DEVRES REL f7fd6cc0 pcim_release (8 bytes)
Now if I insert a return -ENOMEM right after allocating tx_ring:
--- a/drivers/net/e1000/e1000_main.c
+++ b/drivers/net/e1000/e1000_main.c
@@ -1356,6 +1356,8 @@ e1000_alloc_queues(struct e1000_adapter *adapter)
{
adapter->tx_ring = kcalloc(adapter->num_tx_queues,
sizeof(struct e1000_tx_ring),
GFP_KERNEL);
+
+ return -ENOMEM;
if (!adapter->tx_ring)
return -ENOMEM;
#insmod
DEVRES ADD f7a80e80 pcim_release (8 bytes)
DEVRES ADD f7a80ca0 devm_free_netdev (4 bytes)
DEVRES ADD eb7f0080 pcim_iomap_release (24 bytes)
DEVRES ADD eb7f0000 devm_kzalloc_release (40 bytes)
e1000_sw_init: Unable to allocate memory for queues
DEVRES REL eb7f0000 devm_kzalloc_release (40 bytes)
DEVRES REL eb7f0080 pcim_iomap_release (24 bytes)
DEVRES REL f7a80ca0 devm_free_netdev (4 bytes)
DEVRES REL f7a80e80 pcim_release (8 bytes)
ACPI: PCI interrupt for device 0000:02:00.0 disabled
e1000: probe of 0000:02:00.0 failed with error -12
Since we are returning an error from probe the driver core calls
devres_release_all(dev) which releases all of the resources in the right
order. See really_probe() in drivers/base/dd.c.
SIDE NOTE
---------
I ran into a possible e1000 bug with the little -ENOMEM patch above both
with and without the devres patches. The driver seems to leave the
EEPROM in a bad state on error because I get this error after trying to
insert the module again:
e1000_probe: The EEPROM Checksum Is Not Valid
A power cycle but not a reboot fixes it.
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
