On Fri, 13 Jun 2014, Michal Nazarewicz wrote: > > The root cause is that the existing code fails to take into > > account the possibility that common->new_fsg can change while > > do_set_interface() is running, because the spinlock isn't held > > at this point. > > common->new_fsg is not protected by common->lock so this justification > is not valid.
That's true. A better justification would be that the same value of new_fsg should be used in do_set_interface() and in the test that follows. > > @@ -2421,6 +2422,7 @@ static void handle_exception(struct fsg_common > > *common) > > } > > common->state = FSG_STATE_IDLE; > > } > > + new_fsg = common->new_fsg; > > Also, because common->new_fsg is not protected by common->lock, doing > this under a lock is kinda pointless. Yes, but it doesn't hurt. > > spin_unlock_irq(&common->lock); > > > > /* Carry out any extra actions required for the exception */ > > @@ -2460,8 +2462,8 @@ static void handle_exception(struct fsg_common > > *common) > > break; > > > > case FSG_STATE_CONFIG_CHANGE: > > - do_set_interface(common, common->new_fsg); > > - if (common->new_fsg) > > + do_set_interface(common, new_fsg); > > + if (new_fsg) > > usb_composite_setup_continue(common->cdev); > > As far as I can tell, it's safe to move the assignment to new_fsg here, > e.g.: > > new_fsg = common->new_fsg; > do_set_interface(common, new_fsg); > if (new_fsg) > usb_composite_setup_continue(common->cdev); That would be equally correct. I don't see any strong reason for preferring one over the other. > > break; > > But perhaps new_fsg should be protected by the lock. I think valid fix > (which I did not test in *any* way) will be this: No, I think this change is both too big and unnecessary. common->new_fsg does not need protection; in a sense it is "owned" by the composite driver. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
