Herbert Xu wrote: > 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?
Yes, and to make sure the RTNL is held. > 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). The change_rx_flags function was intended to be used by software devices that want to synchronize flags to a different device, in which case they shouldn't interfere since set_multicast_list would only be used for the multicast list and not the 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 sounds like a really good idea to get rid of all the confusion. > 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? Sounds great :) - 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
