Alan Stern wrote: > On Thu, 4 Jul 2019, EJ Hsu wrote: > > > Based on my initial debugging, USB CV TD 9.13 will consecutively set device > to configuration #1 by sending "Set Configuration" transfer. > > So, in set_config() function, it will try to disable each interface first and then > set up each interface. That is, the fsg_disable() will be called first and then > fsg_set_alt(). > > There might be a chance that the request (FSG_STATE_DISCONNECT) from > fsg_disabled() has not been handled by fsg_main_thread before fsg_set_alt() is > called. > > In this case, fsg_set_alt() will try to queue its request > (FSG_STATE_CONFIG_CHANGE) to fsg_main_thread, but find that > FSG_STATE_DISCONNECT has not been handled. > > Because the priority of FSG_STATE_DISCONNECT is higher than > FSG_STATE_CONFIG_CHANGE, FSG_STATE_CONFIG_CHANGE will be discarded > accordingly. > > This might lead to the missing of usb_composite_setup_continue() which > result in the failure of "Set Configuration" transfer. > > > > Will push a new patch to fix this issue. > > Have you seen these emails? > > https://marc.info/?l=linux-usb&m=156222739324546&w=2 > https://marc.info/?l=linux-usb&m=156222747024558&w=2 > > They are probably related to this same issue. > > Alan Stern Yes, looks like we are facing the same issue. The change of Ben is similar to mine, but the priority of FSG_STATE_CONFIG_CHANGE in his patch is higher than FSG_STATE_CONFIG_CLEAR. So, it will not hit the USB CV TD 9.13 failure as above. However, in my opinion, I think we should keep the handling of FSG_STATE_CONFIG_CHANGE as it was. FSG_STATE_CONFIG_CHANGE should take care of handling FSG_STATE_CONFIG_CLEAR because of its higher priority. Think about below case: When fsg_main_thread tries to handle the FSG_STATE_CONFIG_CHANGE, a disconnect event arise at the same time and fsg_disable() is called accordingly. In this case, FSG_STATE_CONFIG_CLEAR might not be queued. (depending on if FSG_STATE_CONFIG_CHANGE is cleared in handle_exception() ) If we still call usb_composite_setup_continue() without checking common->new_fsg, the " Unexpected call" message might still be printed (if delayed_status has been cleared in reset_config() ). Please correct me if I have any misunderstanding. The change for my previous patch is as follows, and it works well on my local test. Thanks, EJ diff --git a/drivers/usb/gadget/function/f_mass_storage.c b/drivers/usb/gadget/function/f_mass_storage.c index 982c3e8..b5f1e1e 100644 --- a/drivers/usb/gadget/function/f_mass_storage.c +++ b/drivers/usb/gadget/function/f_mass_storage.c @@ -2306,7 +2306,6 @@ 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,15 +2412,9 @@ static void handle_exception(struct fsg_common *common) break; case FSG_STATE_CONFIG_CHANGE: - 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); + do_set_interface(common, common->new_fsg); + if (common->new_fsg) usb_composite_setup_continue(common->cdev); - } break; case FSG_STATE_DISCONNECT: diff --git a/drivers/usb/gadget/function/storage_common.h b/drivers/usb/gadget/function/storage_common.h index 12687f7..fc13921 100644 --- a/drivers/usb/gadget/function/storage_common.h +++ b/drivers/usb/gadget/function/storage_common.h @@ -160,8 +160,8 @@ enum fsg_state { FSG_STATE_NORMAL, FSG_STATE_ABORT_BULK_OUT, FSG_STATE_PROTOCOL_RESET, - FSG_STATE_CONFIG_CHANGE, FSG_STATE_DISCONNECT, + FSG_STATE_CONFIG_CHANGE, FSG_STATE_EXIT, FSG_STATE_TERMINATED }; ----------------------------------------------------------------------------------- This email message is for the sole use of the intended recipient(s) and may contain confidential information. Any unauthorized review, use, disclosure or distribution is prohibited. If you are not the intended recipient, please contact the sender by reply email and destroy all copies of the original message. -----------------------------------------------------------------------------------
