Re: Updated if_* attach/detach patches
On Wed, 19 Mar 2003, Nate Lawson wrote: Patches are at: http://www.root.org/~nate/freebsd/if_pci/ I'd like to see calls to mtx_destroy() protected by a test for mtx_initialized(). In most cases this isn't strictly necessary but its not bad practice. -- | Matthew N. Dodd | '78 Datsun 280Z | '75 Volvo 164E | FreeBSD/NetBSD | | [EMAIL PROTECTED] | 2 x '84 Volvo 245DL| ix86,sparc,pmax | | http://www.jurai.net/~winter | For Great Justice! | ISO8802.5 4ever | To Unsubscribe: send mail to [EMAIL PROTECTED] with unsubscribe freebsd-current in the body of the message
Re: Updated if_* attach/detach patches
On Fri, 21 Mar 2003, Matthew N. Dodd wrote: On Wed, 19 Mar 2003, Nate Lawson wrote: Patches are at: http://www.root.org/~nate/freebsd/if_pci/ I'd like to see calls to mtx_destroy() protected by a test for mtx_initialized(). In most cases this isn't strictly necessary but its not bad practice. Perhaps I should add a comment mentioning my assumption: mtx_init is one of the first things called and since detach unconditionally locks the mtx, it should never be called unless the mutex is initialized. -Nate To Unsubscribe: send mail to [EMAIL PROTECTED] with unsubscribe freebsd-current in the body of the message
Re: Updated if_* attach/detach patches
On Fri, 21 Mar 2003, Nate Lawson wrote: Perhaps I should add a comment mentioning my assumption: mtx_init is one of the first things called and since detach unconditionally locks the mtx, it should never be called unless the mutex is initialized. This isn't the case for all drivers and the test would set a good example for people reading the code. -- | Matthew N. Dodd | '78 Datsun 280Z | '75 Volvo 164E | FreeBSD/NetBSD | | [EMAIL PROTECTED] | 2 x '84 Volvo 245DL| ix86,sparc,pmax | | http://www.jurai.net/~winter | For Great Justice! | ISO8802.5 4ever | To Unsubscribe: send mail to [EMAIL PROTECTED] with unsubscribe freebsd-current in the body of the message
Re: Updated if_* attach/detach patches
On Wed, Mar 19, 2003 at 10:37:43AM -0800, Nate Lawson wrote: I have updated my patches for: dc pcn rl sf sis sk ste ti tl vr wb xl They have been compile tested but I only have an rl card so I'd appreciate feedback. - xl: add missed error setting in I/O, memory mapping cases - xl: add missing bzero of softc - xl: remove multi-level goto on attach failure I've tested the xl diff, but only limited internet traffic (i.e., cvsup, browsing, ssh). If high volume traffic is needed to testing, I'm not set up to do that. I haven't seen in problems. -- Steve To Unsubscribe: send mail to [EMAIL PROTECTED] with unsubscribe freebsd-current in the body of the message
Re: Updated if_* attach/detach patches
On Thu, 20 Mar 2003, Steve Kargl wrote: On Wed, Mar 19, 2003 at 10:37:43AM -0800, Nate Lawson wrote: I have updated my patches for: dc pcn rl sf sis sk ste ti tl vr wb xl They have been compile tested but I only have an rl card so I'd appreciate feedback. - xl: add missed error setting in I/O, memory mapping cases - xl: add missing bzero of softc - xl: remove multi-level goto on attach failure I've tested the xl diff, but only limited internet traffic (i.e., cvsup, browsing, ssh). If high volume traffic is needed to testing, I'm not set up to do that. I haven't seen in problems. No, the attach/detach routines are only called at boot and shutdown or if you compile it as a module, load/unload. In fact, for people who would like info on how to test it, this would be helpful (replacing the name with your module of course): #!/bin/sh kldload if_xl kldunload if_xl sleep .2 exec $0 Crashing or growing memory use might indicate problems. -Nate To Unsubscribe: send mail to [EMAIL PROTECTED] with unsubscribe freebsd-current in the body of the message
Re: Updated if_* attach/detach patches
In message: [EMAIL PROTECTED] Nate Lawson [EMAIL PROTECTED] writes: : - dc: move interrupt allocation back where it was before. It was unnecessary : to move it Why's that? If dc is on a shared interrupt line, then dc_intr is going to be called, potentially, before the rest of the attach routine finishes. Keep in mind that attach routines run with interrupts enabled in many interesting cases (including cardbus). You can't call bus_setup_intr until the very end of attach. Since there's no locking here, bad things would happen if an interrupt fired, no? It looks like dc might be safe (since it returns right away if DC_ISR is zero for the bits it knows about)... : - pcn: add missing bzero of softc softc is automatically bzero'd. : - rl: move irq allocation before ether_ifattach. Problems could have been : caused by allocating the irq after enabling interrupts on the card. Same problem as the dc driver. ether_ifattach isn't going to start the interface, so you are safe waiting until after ether_ifattach to do this. And for the rl driver there's no check to see if the RL_ISR has bits set before we acquire the lock... Even with that, it might be safe, but I'm less sure. Why are you checking device_is_alive() in detach? detach won't get called if that isn't the case. Oh, I see, you've added calls... In general, in the drivers I've written I have a foo_alloc() and foo_dealloc() to get and release the resources rather than overloading detach to do this. Well, foo_alloc isn't always possible in the newer world order where locking matters. foo_dalloc can safely be written, however. You want to device_delete_child before calling bus_generic_detatch(). but the old driver does it backwards, so that's not a huge deal. For dc and rl you might want to call bus_child_present(dev) in the detach routines and *NOT* call foo_stop() if the card isn't present. wi actually sets gone in detach: /* check if device was removed */ sc-wi_gone = !bus_child_present(dev); which might not be a horrible idea. Warner To Unsubscribe: send mail to [EMAIL PROTECTED] with unsubscribe freebsd-current in the body of the message
Updated if_* attach/detach patches
I have updated my patches for: dc pcn rl sf sis sk ste ti tl vr wb xl They have been compile tested but I only have an rl card so I'd appreciate feedback. Patches are at: http://www.root.org/~nate/freebsd/if_pci/ Clean up locking and resource management for pci/if_* - Remove locking of the softc in the attach method, instead depending on ether_ifattach being at the end of attach (delaying interrupt enable) - Call *_detach directly in the error case of attach, depending on checking in detach to only free resources that were allocated. This makes all resource freeing in one place, avoiding thinkos that lead to memory leaks. - dc: move interrupt allocation back where it was before. It was unnecessary to move it - pcn: add missing bzero of softc - rl: move irq allocation before ether_ifattach. Problems could have been caused by allocating the irq after enabling interrupts on the card. - rl: call rl_stop before ether_ifdetach - sf: call sf_stop before ether_ifdetach - sis: add missed free of sis_tag - sis: check errors from tag creation - sk: remove duplicate initialization of sk_dev - ste: add missed bus_generic_detach - ti: call ti_stop before ether_ifdetach - ti: add missed error setting in ti_rdata alloc failure - vr: add missed error setting in I/O, memory mapping cases - wb: add missing bzero of softc - xl: add missed error setting in I/O, memory mapping cases - xl: add missing bzero of softc - xl: remove multi-level goto on attach failure -Nate To Unsubscribe: send mail to [EMAIL PROTECTED] with unsubscribe freebsd-current in the body of the message
Re: Updated if_* attach/detach patches
I had forgotten to cvsup before generating the diff so I have updated them in place after resolving conflicts. Only rl and xl have changed. -Nate To Unsubscribe: send mail to [EMAIL PROTECTED] with unsubscribe freebsd-current in the body of the message
Re: Updated if_* attach/detach patches
Apparently, On Wed, Mar 19, 2003 at 10:37:43AM -0800, Nate Lawson said words to the effect of; I have updated my patches for: dc pcn rl sf sis sk ste ti tl vr wb xl They have been compile tested but I only have an rl card so I'd appreciate feedback. Patches are at: http://www.root.org/~nate/freebsd/if_pci/ Clean up locking and resource management for pci/if_* - pcn: add missing bzero of softc - wb: add missing bzero of softc - xl: add missing bzero of softc FWIW, the softc is allocated with M_ZERO, and probably lots of drivers depend on this already. It might be better to just remove all bzero-ing of the softc. Jake To Unsubscribe: send mail to [EMAIL PROTECTED] with unsubscribe freebsd-current in the body of the message
