Messages in this thread | | | Subject | Re: [PATCH] usbnet: Fix two races between usbnet_stop() and the BH | From | Oliver Neukum <> | Date | Tue, 25 Aug 2015 14:31:51 +0200 |
| |
On Mon, 2015-08-24 at 15:29 +0200, Bjørn Mork wrote: > Eugene Shatokhin <eugene.shatokhin@rosalab.ru> writes: > > > 19.08.2015 15:31, Bjørn Mork пишет: > >> Eugene Shatokhin <eugene.shatokhin@rosalab.ru> writes:
> >> Stopping the tasklet rescheduling etc depends only on netif_running(), > >> which will be false when usbnet_stop is called. There is no need to > >> touch dev->flags for this to happen. > > > > That was one of the first ideas we discussed here. Unfortunately, it > > is probably not so simple. > > > > Setting dev->flags to 0 makes some delayed operations do nothing and, > > among other things, not to reschedule usbnet_bh(). > > Yes, but I believe that is merely a side effect. You should never need > to clear multiple flags to get the desired behaviour.
Why? Is there any reason you cannot have a TX and an RX halt at the same time?
> > As you can see in drivers/net/usb/usbnet.c, usbnet_bh() can be called > > as a tasklet function and as a timer function in a number of > > situations (look for the usage of dev->bh and dev->delay there). > > > > netif_running() is indeed false when usbnet_stop() runs, usbnet_stop() > > also disables Tx. This seems to be enough for many cases where > > usbnet_bh() is scheduled, but I am not so sure about the remaining > > ones, namely: > > > > 1. A work function, usbnet_deferred_kevent(), may reschedule > > usbnet_bh(). Looks like the workqueue is only stopped in > > usbnet_disconnect(), so a work item might be processed while > > usbnet_stop() works. Setting dev->flags to 0 makes the work function > > do nothing, by the way. See also the comment in usbnet_stop() about > > this.
Yes, this is the main reason the flags are collectively cleared. We could do them all with clear_bit(). Ugly though.
> > A work item may be placed to this workqueue in a number of ways, by > > both usbnet module and the mini-drivers. It is not too easy to track > > all these situations. > > That's an understatement :)
Yes.
> So FLAG_AVOID_UNLINK_URBS should probably be removed and replaced calls > to usbnet_status_start() and usbnet_status_stop(). This will require > testing on some of the devices with the original firmware problem > however.
And there you point out the main problem.
> In any case: I do not think this flag should be considered when trying > to make usbnet_stop behaviour saner. It's only purpose is to > deliberately break usbnet_stop by not actually stopping.
Yes.
Regards Oliver
| |