netdev - Re: [patch 4/4] Update e1000 driver to use devres.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 16 Aug 2007 10:05:22 -0700
From:	Brandon Philips <bphilips@...e.de>
To:	"Waskiewicz Jr, Peter P" <peter.p.waskiewicz.jr@...el.com>
Cc:	netdev@...r.kernel.org, e1000-devel@...ts.sourceforge.net,
	teheo@...e.de
Subject: Re: [patch 4/4] Update e1000 driver to use devres.

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 majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html