On Tue, Oct 02, 2007 at 05:29:11PM +0200, Patrick McHardy wrote: > > I think this doesn't completely fix it, when dev_unicast_add is > interrupted by dev_mc_add before the unicast changes are performed, > they will get committed in the dev_mc_add context, so we might still > call change_flags with BH disabled. Taking the TX lock around the > dev->uc_count and dev->uc_promisc checks and changes in __dev_set_rx_mode > should fix this.
Good catch. Digging back in history it seems that you added the change_rx_flags function so that the driver didn't have to do it under TX lock, right? The problem with this is that the stack can now call change_rx_flags and set_multicast_list simultaneously which presents a potential headache for the driver author (if they were to use change_rx_flags). It seems to me what we could do is in fact separate out the part that adds the address and the part that syncs it with hardware. That way we can call the hardware from a process context later and use the RTNL to guarantee that we only enter the driver once. So dev_mc_add would look like: 1) Hold some form of lock L. 2) Modify mc list A (a copy of the current mc list). 3) Drop lock. 4) Schedule an update to the hardware. The update to the hardware would look lie: 1) Hold RTNL. 2) Hold lock L. 3) Copy list A to list B (B would be our current list). 4) Drop lock L. 5) Call the hardware. 6) Drop RTNL. For compatibility, set_multicast_list would still be invoked under the TX lock while set_rx_mode would do exactly the same thing but would only hold the RTNL. What do you think about this approach? Cheers, -- Visit Openswan at http://www.openswan.org/ Email: Herbert Xu ~{PmV>HI~} <[EMAIL PROTECTED]> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt - 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
