On Mon, Dec 05, 2011 at 11:32:09AM -0800, Sarah Sharp wrote: > On Mon, Dec 05, 2011 at 09:15:13AM -0800, Paul E. McKenney wrote: > > On Fri, Dec 02, 2011 at 11:55:58AM -0800, Sarah Sharp wrote: > > Looks good, especially given that it is a first posting! > > > > A few questions and comments interspersed below. > > Thanks for the review! Thank you for the responses -- answer to your question below. > > > + /* Make sure that the CPU doesn't reorder writes such that clearing the > > > + * reset flag happens before setting up the streams > > > + */ > > > + smp_wmb(); > > > + atomic_set(&devinfo->resetting, 0); > > > + /* Make sure that all future readers will see the cleared flag before > > > + * returning from post-reset. Otherwise the SCSI core could schedule a > > > + * command queue on a separate CPU where the flag might be stale. > > > + */ > > > + synchronize_srcu(devinfo->srcu); > > > > Do we also need to handle workqueue items that were scheduled > > by pre-existing readers? > > Once uas_prereset returns, the workqueue will be empty and all commands > will have been killed. OK, good. > > (My guess is that the checks I see in > > uas_xfer_data() will have prevented pre-existing readers from scheduling > > workqueue items, but just in case there are a few schedule_work() calls > > that aren't visible in the patch...) > > No, you didn't miss any. All instances of schedule_work() are covered > by the SRCU reader lock, except the one in the workqueue function, > uas_do_work(). > > > If the answer to this question is "no", please expand the comment to > > document the answer. > > What sort of expansion do you want? A note that the workqueue will have > been stopped by pre-reset and thus doesn't need to be a reader? That would be good! Thanx, Paul > Sarah Sharp > -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
