On Fri, 10 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@xxxxxxxxxx>
> ---
> v2: remove the copyright info
> v3: change fsg_unbind() to use FSG_STATE_DISCONNECT
> ---
> drivers/usb/gadget/function/f_mass_storage.c | 21 +++++++++++++++------
> drivers/usb/gadget/function/storage_common.h | 1 +
> 2 files changed, 16 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/usb/gadget/function/f_mass_storage.c b/drivers/usb/gadget/function/f_mass_storage.c
> index 043f97a..982c3e8 100644
> --- a/drivers/usb/gadget/function/f_mass_storage.c
> +++ b/drivers/usb/gadget/function/f_mass_storage.c
> @@ -2293,8 +2293,7 @@ static int fsg_set_alt(struct usb_function *f, unsigned intf, unsigned alt)
> 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 +2306,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 +2413,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:
> @@ -2989,8 +2999,7 @@ static void fsg_unbind(struct usb_configuration *c, struct usb_function *f)
>
> DBG(fsg, "unbind\n");
> if (fsg->common->fsg == fsg) {
> - fsg->common->new_fsg = NULL;
> - raise_exception(fsg->common, FSG_STATE_CONFIG_CHANGE);
> + raise_exception(fsg->common, FSG_STATE_DISCONNECT);
> /* FIXME: make interruptible or killable somehow? */
> wait_event(common->fsg_wait, common->fsg != fsg);
> }
> 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
> };
Acked-by: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>
Although at this point the comment you have added to the CONFIG_CHANGE
case and the following test are useless. Since common->new_fsg doesn't
get cleared any more, it will never be NULL at this point.
What really matters is that the FSG_STATE_DISCONNECT case doesn't call
usb_composite_setup_continue().
Alan Stern
