Re: [PATCH v3] USB:gadget: Fix a warning while loading g_mass_storage

[Michal Nazarewicz <mina86@mina86.com>]

On Fri, Jun 13 2014, Alan Stern <stern@rowland.harvard.edu> wrote:
> 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.

Doing the read under the lock may give an impression that the value is
indeed protected by the lock.  Doing it outside of the lock creates no
such confusion.  Therefore, I would prefer having the read outside.  But
no strong feelings.

>> >  		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.

No strong feelings on my side.

-- 
Best regards,                                         _     _
.o. | Liege of Serenely Enlightened Majesty of      o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz    (o o)
ooo +--<mpn@google.com>--<xmpp:mina86@jabber.org>--ooO--(_)--Ooo--