[V2] usb: gadget: storage: Remove warning message

[V2] usb: gadget: storage: Remove warning message

[EJ Hsu]

This change is to fix below warning message in following scenario:
usb_composite_setup_continue: Unexpected call

When system tried to enter suspend, the fsg_disable() will be called to
disable fsg driver and send a signal to fsg_main_thread. However, at
this point, the fsg_main_thread has already been frozen and can not
respond to this signal. So, this signal will be pended until
fsg_main_thread wakes up.

Once system resumes from suspend, fsg_main_thread will detect a signal
pended and do some corresponding action (in handle_exception()). Then,
host will send some setup requests (get descriptor, set configuration...)
to UDC driver trying to enumerate this device. During the handling of "set
configuration" request, it will try to sync up with fsg_main_thread by
sending a signal (which is the same as the signal sent by fsg_disable)
to it. In a similar manner, once the fsg_main_thread receives this
signal, it will call handle_exception() to handle the request.

However, if the fsg_main_thread wakes up from suspend a little late and
"set configuration" request from Host arrives a little earlier,
fsg_main_thread might come across the request from "set configuration"
when it handles the signal from fsg_disable(). In this case, it will
handle this request as well. So, when fsg_main_thread tries to handle
the signal sent from "set configuration" later, there will nothing left
to do and warning message "Unexpected call" is printed.

Signed-off-by: EJ Hsu <ejh@nvidia.com>
---
v2: remove the copyright info
---
 drivers/usb/gadget/function/f_mass_storage.c | 17 ++++++++++++++---
 drivers/usb/gadget/function/storage_common.h |  1 +
 2 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/gadget/function/f_mass_storage.c b/drivers/usb/gadget/function/f_mass_storage.c
index 043f97a..ba1d827 100644
--- a/drivers/usb/gadget/function/f_mass_storage.c
+++ b/drivers/usb/gadget/function/f_mass_storage.c
@@ -2294,7 +2294,7 @@ static void fsg_disable(struct usb_function *f)
 {
 	struct fsg_dev *fsg = fsg_from_func(f);
 	fsg->common->new_fsg = NULL;
-	raise_exception(fsg->common, FSG_STATE_CONFIG_CHANGE);
+	raise_exception(fsg->common, FSG_STATE_DISCONNECT);
 }
 
 
@@ -2307,6 +2307,7 @@ static void handle_exception(struct fsg_common *common)
 	enum fsg_state		old_state;
 	struct fsg_lun		*curlun;
 	unsigned int		exception_req_tag;
+	struct fsg_dev		*fsg;
 
 	/*
 	 * Clear the existing signals.  Anything but SIGUSR1 is converted
@@ -2413,9 +2414,19 @@ 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)
+		fsg = common->new_fsg;
+		/*
+		 * Add a check here to double confirm if a disconnect event
+		 * occurs and common->new_fsg has been cleared.
+		 */
+		if (fsg) {
+			do_set_interface(common, fsg);
 			usb_composite_setup_continue(common->cdev);
+		}
+		break;
+
+	case FSG_STATE_DISCONNECT:
+		do_set_interface(common, NULL);
 		break;
 
 	case FSG_STATE_EXIT:
diff --git a/drivers/usb/gadget/function/storage_common.h b/drivers/usb/gadget/function/storage_common.h
index e5e3a25..12687f7 100644
--- a/drivers/usb/gadget/function/storage_common.h
+++ b/drivers/usb/gadget/function/storage_common.h
@@ -161,6 +161,7 @@ enum fsg_state {
 	FSG_STATE_ABORT_BULK_OUT,
 	FSG_STATE_PROTOCOL_RESET,
 	FSG_STATE_CONFIG_CHANGE,
+	FSG_STATE_DISCONNECT,
 	FSG_STATE_EXIT,
 	FSG_STATE_TERMINATED
 };
-- 
2.1.4

Re: [V2] usb: gadget: storage: Remove warning message

[Alan Stern]

On Wed, 8 May 2019, EJ Hsu wrote:

> This change is to fix below warning message in following scenario:
> usb_composite_setup_continue: Unexpected call
> 
> When system tried to enter suspend, the fsg_disable() will be called to
> disable fsg driver and send a signal to fsg_main_thread. However, at
> this point, the fsg_main_thread has already been frozen and can not
> respond to this signal. So, this signal will be pended until
> fsg_main_thread wakes up.
> 
> Once system resumes from suspend, fsg_main_thread will detect a signal
> pended and do some corresponding action (in handle_exception()). Then,
> host will send some setup requests (get descriptor, set configuration...)
> to UDC driver trying to enumerate this device. During the handling of "set
> configuration" request, it will try to sync up with fsg_main_thread by
> sending a signal (which is the same as the signal sent by fsg_disable)
> to it. In a similar manner, once the fsg_main_thread receives this
> signal, it will call handle_exception() to handle the request.
> 
> However, if the fsg_main_thread wakes up from suspend a little late and
> "set configuration" request from Host arrives a little earlier,
> fsg_main_thread might come across the request from "set configuration"
> when it handles the signal from fsg_disable(). In this case, it will
> handle this request as well. So, when fsg_main_thread tries to handle
> the signal sent from "set configuration" later, there will nothing left
> to do and warning message "Unexpected call" is printed.
> 
> Signed-off-by: EJ Hsu <ejh@nvidia.com>
> ---
> v2: remove the copyright info
> ---
>  drivers/usb/gadget/function/f_mass_storage.c | 17 ++++++++++++++---
>  drivers/usb/gadget/function/storage_common.h |  1 +
>  2 files changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/gadget/function/f_mass_storage.c b/drivers/usb/gadget/function/f_mass_storage.c
> index 043f97a..ba1d827 100644
> --- a/drivers/usb/gadget/function/f_mass_storage.c
> +++ b/drivers/usb/gadget/function/f_mass_storage.c
> @@ -2294,7 +2294,7 @@ static void fsg_disable(struct usb_function *f)
>  {
>  	struct fsg_dev *fsg = fsg_from_func(f);
>  	fsg->common->new_fsg = NULL;

That line is no longer needed.

> -	raise_exception(fsg->common, FSG_STATE_CONFIG_CHANGE);
> +	raise_exception(fsg->common, FSG_STATE_DISCONNECT);
>  }
>  
>  
> @@ -2307,6 +2307,7 @@ static void handle_exception(struct fsg_common *common)
>  	enum fsg_state		old_state;
>  	struct fsg_lun		*curlun;
>  	unsigned int		exception_req_tag;
> +	struct fsg_dev		*fsg;
>  
>  	/*
>  	 * Clear the existing signals.  Anything but SIGUSR1 is converted
> @@ -2413,9 +2414,19 @@ 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)
> +		fsg = common->new_fsg;
> +		/*
> +		 * Add a check here to double confirm if a disconnect event
> +		 * occurs and common->new_fsg has been cleared.
> +		 */
> +		if (fsg) {
> +			do_set_interface(common, fsg);
>  			usb_composite_setup_continue(common->cdev);
> +		}
> +		break;
> +
> +	case FSG_STATE_DISCONNECT:
> +		do_set_interface(common, NULL);
>  		break;
>  
>  	case FSG_STATE_EXIT:
> diff --git a/drivers/usb/gadget/function/storage_common.h b/drivers/usb/gadget/function/storage_common.h
> index e5e3a25..12687f7 100644
> --- a/drivers/usb/gadget/function/storage_common.h
> +++ b/drivers/usb/gadget/function/storage_common.h
> @@ -161,6 +161,7 @@ enum fsg_state {
>  	FSG_STATE_ABORT_BULK_OUT,
>  	FSG_STATE_PROTOCOL_RESET,
>  	FSG_STATE_CONFIG_CHANGE,
> +	FSG_STATE_DISCONNECT,
>  	FSG_STATE_EXIT,
>  	FSG_STATE_TERMINATED
>  };

You forgot to change fsg_unbind() to use FSG_STATE_DISCONNECT.  And
when that's done, it will no longer need to set common->new_fsg to NULL
either.

This is sort of a band-aid approach.  The real problem is that the
original design of the driver never intended for there to be more than
one outstanding CONFIG_CHANGE event, so naturally there are scenarios
where it doesn't work right when that assumption is violated.  This
patch addresses one of those scenarios, but there may be others
remaining.

Ultimately the problem boils down to synchronization with the composite
core.  Some of the callbacks made by the core take time to fully
process, so what should happen if the core makes a second callback 
before the first one is completely processed?

On the other hand, I can't think of anything better at the moment.

Alan Stern

RE: [V2] usb: gadget: storage: Remove warning message

[EJ Hsu]

> 
> You forgot to change fsg_unbind() to use FSG_STATE_DISCONNECT.  And when
> that's done, it will no longer need to set common->new_fsg to NULL either.
> 

Yes, we should change fsg_unbind() as well.

> This is sort of a band-aid approach.  The real problem is that the original design
> of the driver never intended for there to be more than one outstanding
> CONFIG_CHANGE event, so naturally there are scenarios where it doesn't work
> right when that assumption is violated.  This patch addresses one of those
> scenarios, but there may be others remaining.
> 
> Ultimately the problem boils down to synchronization with the composite core.

Thanks for your reviewing, I agree with your point.

> Some of the callbacks made by the core take time to fully process, so what
> should happen if the core makes a second callback before the first one is
> completely processed?
> 
> On the other hand, I can't think of anything better at the moment.
> 
> Alan Stern

Actually, composite core have tried to deal with this case by using a spinlock. 
(please refer to the cdev->lock)
The problem here is that the callback of fsg will delegate the request to 
fsg_main_thread. That is, the handling of fsg request will run in parallel with 
composite core.
In my opinion, this issue can be avoided if we handle these request directly 
in the callback of fsg instead of handing it over to fsg_main_thread. But this 
might take too much time in the interrupt context, which is not good for 
system performance.

Please correct me if there is anything wrong. Thanks

EJ

--nvpublic

RE: [V2] usb: gadget: storage: Remove warning message

[Alan Stern]

On Thu, 9 May 2019, EJ Hsu wrote:

> > You forgot to change fsg_unbind() to use FSG_STATE_DISCONNECT.  And when
> > that's done, it will no longer need to set common->new_fsg to NULL either.
> > 
> 
> Yes, we should change fsg_unbind() as well.
> 
> > This is sort of a band-aid approach.  The real problem is that the original design
> > of the driver never intended for there to be more than one outstanding
> > CONFIG_CHANGE event, so naturally there are scenarios where it doesn't work
> > right when that assumption is violated.  This patch addresses one of those
> > scenarios, but there may be others remaining.
> > 
> > Ultimately the problem boils down to synchronization with the composite core.
> 
> Thanks for your reviewing, I agree with your point.
> 
> > Some of the callbacks made by the core take time to fully process, so what
> > should happen if the core makes a second callback before the first one is
> > completely processed?
> > 
> > On the other hand, I can't think of anything better at the moment.
> > 
> > Alan Stern
> 
> Actually, composite core have tried to deal with this case by using a spinlock. 
> (please refer to the cdev->lock)
> The problem here is that the callback of fsg will delegate the request to 
> fsg_main_thread. That is, the handling of fsg request will run in parallel with 
> composite core.
> In my opinion, this issue can be avoided if we handle these request directly 
> in the callback of fsg instead of handing it over to fsg_main_thread. But this 
> might take too much time in the interrupt context, which is not good for 
> system performance.
> 
> Please correct me if there is anything wrong. Thanks

In theory, the mass-storage function could also be fixed to be more
careful about locking and synchronization.  For example, never set or
read common->next_fsg unless you're holding the fsg lock, and don't
raise a CONFIG_CHANGE exception if another one is already pending.

But I think your patch will be good enough for now, once you have fixed 
up the two issues mentioned earlier.

Alan Stern