| Message ID | 1557486130-50945-1-git-send-email-ejh@nvidia.com (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | e70b3f5da00119e057b7faa557753fee7f786f17 |
| Headers | show |
| Series | [V3] usb: gadget: storage: Remove warning message | expand |
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@nvidia.com> > --- > 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@rowland.harvard.edu> 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
Hi, Alan Stern wrote: > 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@nvidia.com> >> --- >> 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@rowland.harvard.edu> > > 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 This patch causes a failure in USB CV TD 9.13 Set Configuration Test. Please review and help resolve it. Apologize for the short report description. I'll try to capture more info if you cannot reproduce it. Thanks, Thinh
On Tue, 2 Jul 2019, Thinh Nguyen wrote: > Hi, > > Alan Stern wrote: > > 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@nvidia.com> > >> --- > >> v2: remove the copyright info > >> v3: change fsg_unbind() to use FSG_STATE_DISCONNECT > >> --- > This patch causes a failure in USB CV TD 9.13 Set Configuration Test. > Please review and help resolve it. > Apologize for the short report description. I'll try to capture more > info if you cannot reproduce it. Yes, please provide the complete log and information from the failing USB CV test. Alan Stern
Thinh Nguyen wrote: > Alan Stern wrote: >> On Tue, 2 Jul 2019, Thinh Nguyen wrote: >> >>> Hi, >>> >>> Alan Stern wrote: >>>> 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@nvidia.com> >>>>> --- >>>>> v2: remove the copyright info >>>>> v3: change fsg_unbind() to use FSG_STATE_DISCONNECT >>>>> --- >>> This patch causes a failure in USB CV TD 9.13 Set Configuration Test. >>> Please review and help resolve it. >>> Apologize for the short report description. I'll try to capture more >>> info if you cannot reproduce it. >> Yes, please provide the complete log and information from the failing >> USB CV test. >> > I attached the CV test log. I hope that's sufficient. > We may have issue sending attched HTML file. Here's a text format of it: TEST SUITE: Chapter 9 Tests [USB 3 Gen X devices].cvtests REVISION: 10866 REVISION DATE: 2018-02-28 16:30:43 -0800 (Wed, 28 Feb 2018) OPERATING SYSTEM: Windows 10 Home (Build 18894.1000.amd64fre.rs_prerelease.190503-1728) WORKSTATION: USB-AUTOMATION DATE: Tuesday, July 02, 2019 TIME: 01:52:01 AM OPERATOR: Lab_auto NUMBER OF TESTS: 1 LOG NAME: Chapter 9 Tests - USB 3 Gen X - 2019-07-02 01-51-36 RESULT: failed InitializeTestSuite INFOTest log initialized. INFOLog Level: Normal INFOUser Input module initialized INFOWindows 10 Home (Build 18894.1000.amd64fre.rs_prerelease.190503-1728) INFOCVExe.exe ver 3.0.0.0 INFOBaseUtilities.dll ver 3.0.0.0 INFOCommandVerifierLog.dll ver 3.0.0.0 INFOTSMFCGuiDialogHelperDLL.dll ver 3.0.0.0 INFOTestUtilities.dll ver 3.0.0.0 INFOTestSuiteEngine.dll ver 3.0.0.0 INFOVIFReader.dll ver 3.0.0.0 INFOxhci_DevIOCTL.dll ver 2.1.10.3 INFOxhci_TestServices.dll ver 2.1.10.3 INFOUSBUtilities.dll ver 1.4.5.1 INFOStackSwitcher.dll ver 1.4.5.1 INFOxhci_USBCommandVerifier.dll ver 2.1.10.3 INFOHost selected: xHCI Host: VID=0x8086, PID=0xa36d (PCI bus 0, device 20, function 0) INFODUT selected: SSP Device (MSC/BOT) addr=1: VID=053f, PID=8bd8 INFOTopology: XHCI HC -- DUT INFOSuperSpeedPlus Device. GetNumberOfConfigurations INFOUSB Version number of device: 3.20 INFONumber of configurations: 1 TD 9.13 Set Configuration Test - Device State AddressedFailed (Aborted) INFOStart time: Tue Jul 2 01:51:39 2019 INFO In Address state: INFONow doing a Set Configuration with Configuration Value 0 INFOSet Configuration with Configuration Value 0 succeeded INFONow doing a Get Configuration INFOGet Configuration for the device returned the correct Configuration Value of 0 INFONow doing a Get Descriptor with Configuration Index 0 INFOGet Descriptor succeeded and returned Configuration Value : 1 INFONow doing a Set Configuration with Configuration Value 1 INFOSet Configuration succeeded with Configuration Value : 1 INFO In Configured state: INFONow doing a Get Descriptor with Configuration Index 0 INFOGet Descriptor succeeded and returned Configuration Value : 1 INFONow doing a Set Configuration with Configuration Value 1 ERRORSet Configuration failed for Configuration Value : 1 ERRORSet Configuration failed with Configuration Value : 1 FAIL(9.4.7.1) Devices must support a valid SetConfiguration() request INFONow doing a Set Configuration with Configuration Value 0 ERRORSet Configuration failed for Configuration Value : 0 ERRORCould not unconfigure the device FAIL(9.4.7.4) In the Configured state in response to the SetConfiguration() request, the device must enter the Address state, if the specified configuration is zero. INFONow doing a Get Configuration ERRORGet configuration failed ERRORCould not Get Configuration for the device ERRORGet device descriptor failed ERRORCouldn't get USB Version of Device Under Test ERRORGet number of configurations failed ABORTCould not find an invalid Configuration Value, FindInvalidConfigValue() returned 0 - Internal error INFOPutting device back in Configured state ERRORSet Configuration failed for Configuration Value : 1 ERRORSet Configuration failed with Configuration Value : 1 FAIL(9.4.7.1) Devices must support a valid SetConfiguration() request INFONow doing a Get Configuration ERRORGet configuration failed ERRORCould not Get Configuration for the device FAIL(9.4.2.2) Devices should return the non-zero bConfigurationValue of the current configuration for the GetConfiguration() request in the Configured state. INFONow doing a Get Configuration ERRORGet configuration failed ERRORCould not Get Configuration for the device FAIL(9.4.2.2) Devices should return the non-zero bConfigurationValue of the current configuration for the GetConfiguration() request in the Configured state. INFONow doing a Set Configuration with Configuration Value 0 ERRORSet Configuration failed for Configuration Value : 0 ERRORCould not unconfigure the device FAIL(9.4.7.4) In the Configured state in response to the SetConfiguration() request, the device must enter the Address state, if the specified configuration is zero. FAILTest did not execute all required steps. INFO Stop time: Tue Jul 2 01:52:01 2019 INFODuration: 22 seconds. INFOStopping Test [ TD 9.13 Set Configuration Test - Device State Addressed: Number of: Fails (7); Aborts (1); Warnings (0) ] Summary INFOTEST SUITE SUMMARY: [ Fails (7); Aborts (1); Warnings (0) ] INFOTEST RESULTS: [ Passed (0); Failed (1) ] BR, Thinh
EJ Hsu wrote: > Thinh Nguyen wrote: > > Alan Stern wrote: > >> On Tue, 2 Jul 2019, Thinh Nguyen wrote: > >> > >>> Hi, > >>> > >>> Alan Stern wrote: > >>>> 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@nvidia.com> > >>>>> --- > >>>>> v2: remove the copyright info > >>>>> v3: change fsg_unbind() to use FSG_STATE_DISCONNECT > >>>>> --- > >>> This patch causes a failure in USB CV TD 9.13 Set Configuration Test. > >>> Please review and help resolve it. > >>> Apologize for the short report description. I'll try to capture more > >>> info if you cannot reproduce it. > >> Yes, please provide the complete log and information from the failing > >> USB CV test. > >> > > I attached the CV test log. I hope that's sufficient. > > > > We may have issue sending attched HTML file. Here's a text format of it: > > > TEST SUITE: Chapter 9 Tests [USB 3 Gen X devices].cvtests > REVISION: 10866 > REVISION DATE: 2018-02-28 16:30:43 -0800 (Wed, 28 Feb 2018) > OPERATING SYSTEM: Windows 10 Home (Build > 18894.1000.amd64fre.rs_prerelease.190503-1728) > WORKSTATION: USB-AUTOMATION > DATE: Tuesday, July 02, 2019 > TIME: 01:52:01 AM > OPERATOR: Lab_auto > NUMBER OF TESTS: 1 > LOG NAME: Chapter 9 Tests - USB 3 Gen X - 2019-07-02 01-51-36 > RESULT: failed > > InitializeTestSuite > > INFOTest log initialized. > INFOLog Level: Normal > INFOUser Input module initialized > INFOWindows 10 Home (Build 18894.1000.amd64fre.rs_prerelease.190503- > 1728) > INFOCVExe.exe ver 3.0.0.0 > INFOBaseUtilities.dll ver 3.0.0.0 > INFOCommandVerifierLog.dll ver 3.0.0.0 > INFOTSMFCGuiDialogHelperDLL.dll ver 3.0.0.0 INFOTestUtilities.dll ver 3.0.0.0 > INFOTestSuiteEngine.dll ver 3.0.0.0 INFOVIFReader.dll ver 3.0.0.0 > INFOxhci_DevIOCTL.dll ver 2.1.10.3 INFOxhci_TestServices.dll ver 2.1.10.3 > INFOUSBUtilities.dll ver 1.4.5.1 INFOStackSwitcher.dll ver 1.4.5.1 > INFOxhci_USBCommandVerifier.dll ver 2.1.10.3 INFOHost selected: xHCI Host: > VID=0x8086, PID=0xa36d (PCI bus 0, device 20, function 0) > INFODUT selected: SSP Device (MSC/BOT) addr=1: VID=053f, PID=8bd8 > INFOTopology: XHCI HC -- DUT > INFOSuperSpeedPlus Device. > > GetNumberOfConfigurations > > INFOUSB Version number of device: 3.20 > INFONumber of configurations: 1 > > TD 9.13 Set Configuration Test - Device State AddressedFailed (Aborted) > > INFOStart time: Tue Jul 2 01:51:39 2019 > > INFO In Address state: > INFONow doing a Set Configuration with Configuration Value 0 INFOSet > Configuration with Configuration Value 0 succeeded INFONow doing a Get > Configuration INFOGet Configuration for the device returned the correct > Configuration Value of 0 INFONow doing a Get Descriptor with Configuration > Index 0 INFOGet Descriptor succeeded and returned Configuration Value : 1 > INFONow doing a Set Configuration with Configuration Value 1 INFOSet > Configuration succeeded with Configuration Value : 1 > INFO In Configured state: > INFONow doing a Get Descriptor with Configuration Index 0 INFOGet > Descriptor succeeded and returned Configuration Value : 1 INFONow doing a > Set Configuration with Configuration Value 1 ERRORSet Configuration failed for > Configuration Value : 1 ERRORSet Configuration failed with Configuration > Value : 1 > FAIL(9.4.7.1) Devices must support a valid SetConfiguration() request INFONow > doing a Set Configuration with Configuration Value 0 ERRORSet Configuration > failed for Configuration Value : 0 ERRORCould not unconfigure the device > FAIL(9.4.7.4) In the Configured state in response to the > SetConfiguration() request, the device must enter the Address state, if the > specified configuration is zero. > INFONow doing a Get Configuration > ERRORGet configuration failed > ERRORCould not Get Configuration for the device ERRORGet device descriptor > failed ERRORCouldn't get USB Version of Device Under Test ERRORGet number > of configurations failed ABORTCould not find an invalid Configuration Value, > FindInvalidConfigValue() returned 0 - Internal error INFOPutting device back in > Configured state ERRORSet Configuration failed for Configuration Value : 1 > ERRORSet Configuration failed with Configuration Value : 1 > FAIL(9.4.7.1) Devices must support a valid SetConfiguration() request INFONow > doing a Get Configuration ERRORGet configuration failed ERRORCould not Get > Configuration for the device > FAIL(9.4.2.2) Devices should return the non-zero bConfigurationValue of the > current configuration for the GetConfiguration() request in the Configured > state. > INFONow doing a Get Configuration > ERRORGet configuration failed > ERRORCould not Get Configuration for the device > FAIL(9.4.2.2) Devices should return the non-zero bConfigurationValue of the > current configuration for the GetConfiguration() request in the Configured > state. > INFONow doing a Set Configuration with Configuration Value 0 ERRORSet > Configuration failed for Configuration Value : 0 ERRORCould not unconfigure > the device > FAIL(9.4.7.4) In the Configured state in response to the > SetConfiguration() request, the device must enter the Address state, if the > specified configuration is zero. > FAILTest did not execute all required steps. > INFO > Stop time: Tue Jul 2 01:52:01 2019 > INFODuration: 22 seconds. > INFOStopping Test [ TD 9.13 Set Configuration Test - Device State Addressed: > Number of: Fails (7); Aborts (1); Warnings (0) ] > > Summary > > INFOTEST SUITE SUMMARY: > [ Fails (7); Aborts (1); Warnings (0) ] INFOTEST RESULTS: > [ Passed (0); Failed (1) ] > > > > > BR, > Thinh I can reproduce this issue locally. The reproducing rate is about 1 out of 10. Will look into this issue. Thanks ----------------------------------------------------------------------------------- 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. -----------------------------------------------------------------------------------
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. --nvpublic
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
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. -----------------------------------------------------------------------------------
On Fri, 2019-07-05 at 10:49 +0000, EJ Hsu wrote: > > 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. Correct. This is on purpose. A CONFIG_CHANGE will wipe out the previous config, so if the queued state was CONFIG_CLEAR, and we haven't dequeued it yet, we can skip it. > 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. My patch does just that. If you get a clear and a change fast enough (ie, the clear hasn't been dequeued), then the change will override, which is what we want, since that will cleanup the previous config regardless. The entire point of my patch is to make sure that new_fsg is only ever set in that one place, the config change, and there is no possible confusion with the async continuation. > 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. new_fsg will never be clear if we do FSG_STATE_CONFIG_CHANGE, that's the whole point of my patch. Otherwise we keep having the problem that I described in my cset comment where two parties stomp on that one variable and confusion ensures. Now, there's indeed one remaining issue as you pointed out. If we disconnect before we've dequeued FSG_STATE_CONFIG_CHANGE. Is that an issue in practice however ? There are several ways we could handle that one: - We can do a fully ordered queue of events. But that's more complex and somewhat suboptimal, but would be the most robust I suppose. - Or we could be a bit smarter here, and have additional state information protected by the lock set while queuing FSG_STATE_CONFIG_CHANGE. This would include the fact that we have a pending set_alt and thus a delayed status to complete. Then we could have a flag indicating a disable/disconnect. fsg_disable would set it, fsg_set_alt would clear it. Those would need to be established with the same lock that queues the state and *retreived* in the same lock as well, otherwise we go back to having them change on the fly leading to inconsistent state. But in any case, having more than one agent stomping on new_fsg locklessly from interrupts is going to be a problem and I don't want to get back down that path. As it is, my patch makes things work for me. Does it work for you ? We can look at polishing more later. Cheers, Ben.
On Fri, 2019-07-05 at 10:49 +0000, EJ Hsu wrote: > 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 > }; Is this patch against some other patch ? Please send the whole thing so people don't have to go digging in archives to figure what the code looks like. The above by itself doesn't make sense and can't be reviewed. However, I have a strong suspicion that if you still need to test new_fsg before calling usb_composite_setup_continue(). Then you haven't fixed the bug that I describe. Cheers, Ben.
(following our conversation)
Here's a completely untested alternative patch (it replaces my previous
one) that fixes it a bit differently.
This time it should handle the case of a disconnect happening
before we have dequeued a config change.
This assumes that it's correct to never call
usb_composite_setup_continue() if an fsg_disable() happens after a
fsg_set_alt() and before we have processed the latter.
I will try to test it tomorrow if time permits, otherwise some time
next week:
---
[PATCH] usb: gadget: mass_storage: Fix races between fsg_disable and fsg_set_alt
If fsg_disable() and fsg_set_alt() are called too closely to each
other (for example due to a quick reset/reconnect), what can happen
is that fsg_set_alt sets common->new_fsg from an interrupt while
handle_exception is trying to process the config change caused by
fsg_disable():
fsg_disable()
...
handle_exception()
sets state back to FSG_STATE_NORMAL
hasn't yet called do_set_interface()
or is inside it.
---> interrupt
fsg_set_alt
sets common->new_fsg
queues a new FSG_STATE_CONFIG_CHANGE
<---
Now, the first handle_exception can "see" the updated
new_fsg, treats it as if it was a fsg_set_alt() response,
call usb_composite_setup_continue() etc...
But then, the thread sees the second FSG_STATE_CONFIG_CHANGE,
and goes back down the same path, wipes and reattaches a now
active fsg, and .. calls usb_composite_setup_continue() which
at this point is wrong.
Not only we get a backtrace, but I suspect the second set_interface
wrecks some state causing the host to get upset in my case.
This fixes it by replacing "new_fsg" by a "state argument" (same
principle) which is set in the same lock section as the state
update, and retrieved similarly.
That way, there is never any discrepancy between the dequeued
state and the observed value of it. We keep the ability to have
the latest reconfig operation take precedence, but we guarantee
that once "dequeued" the argument (new_fsg) will not be clobbered
by any new event.
Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
drivers/usb/gadget/function/f_mass_storage.c | 26 ++++++++++++--------
1 file changed, 16 insertions(+), 10 deletions(-)
diff --git a/drivers/usb/gadget/function/f_mass_storage.c b/drivers/usb/gadget/function/f_mass_storage.c
index 043f97ad8f22..2ef029413b01 100644
--- a/drivers/usb/gadget/function/f_mass_storage.c
+++ b/drivers/usb/gadget/function/f_mass_storage.c
@@ -261,7 +261,7 @@ struct fsg_common;
struct fsg_common {
struct usb_gadget *gadget;
struct usb_composite_dev *cdev;
- struct fsg_dev *fsg, *new_fsg;
+ struct fsg_dev *fsg;
wait_queue_head_t io_wait;
wait_queue_head_t fsg_wait;
@@ -290,6 +290,7 @@ struct fsg_common {
unsigned int bulk_out_maxpacket;
enum fsg_state state; /* For exception handling */
unsigned int exception_req_tag;
+ void *exception_arg;
enum data_direction data_dir;
u32 data_size;
@@ -391,7 +392,8 @@ static int fsg_set_halt(struct fsg_dev *fsg, struct usb_ep *ep)
/* These routines may be called in process context or in_irq */
-static void raise_exception(struct fsg_common *common, enum fsg_state new_state)
+static void __raise_exception(struct fsg_common *common, enum fsg_state new_state,
+ void *arg)
{
unsigned long flags;
@@ -404,6 +406,7 @@ static void raise_exception(struct fsg_common *common, enum fsg_state new_state)
if (common->state <= new_state) {
common->exception_req_tag = common->ep0_req_tag;
common->state = new_state;
+ common->exception_arg = arg;
if (common->thread_task)
send_sig_info(SIGUSR1, SEND_SIG_PRIV,
common->thread_task);
@@ -411,6 +414,10 @@ static void raise_exception(struct fsg_common *common, enum fsg_state new_state)
spin_unlock_irqrestore(&common->lock, flags);
}
+static void raise_exception(struct fsg_common *common, enum fsg_state new_state)
+{
+ __raise_exception(common, new_state, NULL);
+}
/*-------------------------------------------------------------------------*/
@@ -2285,16 +2292,14 @@ static int do_set_interface(struct fsg_common *common, struct fsg_dev *new_fsg)
static int fsg_set_alt(struct usb_function *f, unsigned intf, unsigned alt)
{
struct fsg_dev *fsg = fsg_from_func(f);
- fsg->common->new_fsg = fsg;
- raise_exception(fsg->common, FSG_STATE_CONFIG_CHANGE);
+ __raise_exception(fsg->common, FSG_STATE_CONFIG_CHANGE, fsg);
return USB_GADGET_DELAYED_STATUS;
}
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_CONFIG_CHANGE, NULL);
}
@@ -2307,6 +2312,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 *new_fsg;
/*
* Clear the existing signals. Anything but SIGUSR1 is converted
@@ -2360,6 +2366,7 @@ static void handle_exception(struct fsg_common *common)
common->next_buffhd_to_fill = &common->buffhds[0];
common->next_buffhd_to_drain = &common->buffhds[0];
exception_req_tag = common->exception_req_tag;
+ new_fsg = common->exception_arg;
old_state = common->state;
common->state = FSG_STATE_NORMAL;
@@ -2413,8 +2420,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);
break;
@@ -2989,8 +2996,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_CONFIG_CHANGE, NULL);
/* FIXME: make interruptible or killable somehow? */
wait_event(common->fsg_wait, common->fsg != fsg);
}
On Fri, 5 Jul 2019, Benjamin Herrenschmidt wrote: > On Fri, 2019-07-05 at 10:49 +0000, EJ Hsu wrote: > > > > 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. > > Correct. This is on purpose. A CONFIG_CHANGE will wipe out the previous > config, so if the queued state was CONFIG_CLEAR, and we haven't > dequeued it yet, we can skip it. > > > 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. > > My patch does just that. If you get a clear and a change fast enough > (ie, the clear hasn't been dequeued), then the change will override, > which is what we want, since that will cleanup the previous config > regardless. > > The entire point of my patch is to make sure that new_fsg is only ever > set in that one place, the config change, and there is no possible > confusion with the async continuation. > > > 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. > > new_fsg will never be clear if we do FSG_STATE_CONFIG_CHANGE, that's > the whole point of my patch. > > Otherwise we keep having the problem that I described in my cset > comment where two parties stomp on that one variable and confusion > ensures. > > Now, there's indeed one remaining issue as you pointed out. If we > disconnect before we've dequeued FSG_STATE_CONFIG_CHANGE. > > Is that an issue in practice however ? There are several ways we could > handle that one: > > - We can do a fully ordered queue of events. But that's more complex > and somewhat suboptimal, but would be the most robust I suppose. > > - Or we could be a bit smarter here, and have additional state > information protected by the lock set while queuing > FSG_STATE_CONFIG_CHANGE. This would include the fact that we have a > pending set_alt and thus a delayed status to complete. Then we could > have a flag indicating a disable/disconnect. fsg_disable would set it, > fsg_set_alt would clear it. Those would need to be established with the > same lock that queues the state and *retreived* in the same lock as > well, otherwise we go back to having them change on the fly leading to > inconsistent state. > > But in any case, having more than one agent stomping on new_fsg > locklessly from interrupts is going to be a problem and I don't want to > get back down that path. > > As it is, my patch makes things work for me. Does it work for you ? We > can look at polishing more later. I haven't looked at the new patches yet. Still, what I originally had in mind for this situation was that the _last_ event should always take precedence. This goes against the idea of having separate FSG_STATE_* levels for disconnect and config-change, because the driver assumes that higher levels should override lower levels. Also, if the thread has already started processing one of these events when another one occurs, the new exception should cause the thread to restart the handler and thus take care of the new event. And yes, there should be enough locking to ensure that nothing gets stomped on except in situations where it won't matter. That's how I think this should all work, and it doesn't look like we really need a queue to do it properly. Alan Stern
On Fri, 5 Jul 2019, Benjamin Herrenschmidt wrote: > (following our conversation) > > Here's a completely untested alternative patch (it replaces my previous > one) that fixes it a bit differently. > > This time it should handle the case of a disconnect happening > before we have dequeued a config change. > > This assumes that it's correct to never call > usb_composite_setup_continue() if an fsg_disable() happens after a > fsg_set_alt() and before we have processed the latter. That should be handled okay. If it isn't, the composite core needs to be fixed. > I will try to test it tomorrow if time permits, otherwise some time > next week: > --- > > [PATCH] usb: gadget: mass_storage: Fix races between fsg_disable and fsg_set_alt > > If fsg_disable() and fsg_set_alt() are called too closely to each > other (for example due to a quick reset/reconnect), what can happen > is that fsg_set_alt sets common->new_fsg from an interrupt while > handle_exception is trying to process the config change caused by > fsg_disable(): > > fsg_disable() > ... > handle_exception() > sets state back to FSG_STATE_NORMAL > hasn't yet called do_set_interface() > or is inside it. > > ---> interrupt > fsg_set_alt > sets common->new_fsg > queues a new FSG_STATE_CONFIG_CHANGE > <--- > > Now, the first handle_exception can "see" the updated > new_fsg, treats it as if it was a fsg_set_alt() response, > call usb_composite_setup_continue() etc... > > But then, the thread sees the second FSG_STATE_CONFIG_CHANGE, > and goes back down the same path, wipes and reattaches a now > active fsg, and .. calls usb_composite_setup_continue() which > at this point is wrong. > > Not only we get a backtrace, but I suspect the second set_interface > wrecks some state causing the host to get upset in my case. > > This fixes it by replacing "new_fsg" by a "state argument" (same > principle) which is set in the same lock section as the state > update, and retrieved similarly. > > That way, there is never any discrepancy between the dequeued > state and the observed value of it. We keep the ability to have > the latest reconfig operation take precedence, but we guarantee > that once "dequeued" the argument (new_fsg) will not be clobbered > by any new event. > > Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org> Yes, this looks just right. If I had thought about this a little more deeply earlier on, I would have come up with a patch very much like this. My only comments are cosmetic. > --- > drivers/usb/gadget/function/f_mass_storage.c | 26 ++++++++++++-------- > 1 file changed, 16 insertions(+), 10 deletions(-) > > diff --git a/drivers/usb/gadget/function/f_mass_storage.c b/drivers/usb/gadget/function/f_mass_storage.c > index 043f97ad8f22..2ef029413b01 100644 > --- a/drivers/usb/gadget/function/f_mass_storage.c > +++ b/drivers/usb/gadget/function/f_mass_storage.c > @@ -2285,16 +2292,14 @@ static int do_set_interface(struct fsg_common *common, struct fsg_dev *new_fsg) > static int fsg_set_alt(struct usb_function *f, unsigned intf, unsigned alt) > { > struct fsg_dev *fsg = fsg_from_func(f); While you're changing this, it would be nice to add the customary blank line here. > - fsg->common->new_fsg = fsg; > - raise_exception(fsg->common, FSG_STATE_CONFIG_CHANGE); > + __raise_exception(fsg->common, FSG_STATE_CONFIG_CHANGE, fsg); > return USB_GADGET_DELAYED_STATUS; > } > > static void fsg_disable(struct usb_function *f) > { > struct fsg_dev *fsg = fsg_from_func(f); And here. Otherwise: Acked-by: Alan Stern <stern@rowland.harvard.edu> Alan Stern
On Fri, 2019-07-05 at 10:30 -0400, Alan Stern wrote: > I haven't looked at the new patches yet. > > Still, what I originally had in mind for this situation was that the > _last_ event should always take precedence. This goes against the idea > of having separate FSG_STATE_* levels for disconnect and config-change, > because the driver assumes that higher levels should override lower > levels. Right, this is what my new tentative patch does. The main caveat is to ensure that if that last event arrives after the previous one was "dequeued" by the thread but before it was *processed*, then we don't clobber it's argument (new_fsg) and thus end up processing both events each with the appropriate corresponding value of new_fsg. This is the root of the bug in fact: When the second event occurs in that window, we end up processing twice, as expected, but potentially using twice the *new* new_fsg value. > Also, if the thread has already started processing one of these events > when another one occurs, the new exception should cause the thread to > restart the handler and thus take care of the new event. And yes, > there should be enough locking to ensure that nothing gets stomped on > except in situations where it won't matter. > > That's how I think this should all work, and it doesn't look like we > really need a queue to do it properly. Yes, I agree. That's what the patch I posted last night aims at, I need to test it today. Cheers, Ben.
On Fri, 2019-07-05 at 14:28 -0400, Alan Stern wrote: > On Fri, 5 Jul 2019, Benjamin Herrenschmidt wrote: > > > (following our conversation) > > > > Here's a completely untested alternative patch (it replaces my previous > > one) that fixes it a bit differently. > > > > This time it should handle the case of a disconnect happening > > before we have dequeued a config change. > > > > This assumes that it's correct to never call > > usb_composite_setup_continue() if an fsg_disable() happens after a > > fsg_set_alt() and before we have processed the latter. > > That should be handled okay. If it isn't, the composite core needs to > be fixed. Ok. I'll have a quick look to make sure. .../... > Yes, this looks just right. If I had thought about this a little more > deeply earlier on, I would have come up with a patch very much like > this. Right, so as I grow more familiar with that code and its intent, I agree, I'm much happier with this. Hopefully it passes my tests. I'll tidy up as per your comments and repost properly if all goes well along with some other things I piled up. Cheers, Ben. > My only comments are cosmetic. > > > --- > > drivers/usb/gadget/function/f_mass_storage.c | 26 ++++++++++++-------- > > 1 file changed, 16 insertions(+), 10 deletions(-) > > > > diff --git a/drivers/usb/gadget/function/f_mass_storage.c b/drivers/usb/gadget/function/f_mass_storage.c > > index 043f97ad8f22..2ef029413b01 100644 > > --- a/drivers/usb/gadget/function/f_mass_storage.c > > +++ b/drivers/usb/gadget/function/f_mass_storage.c > > > @@ -2285,16 +2292,14 @@ static int do_set_interface(struct fsg_common *common, struct fsg_dev *new_fsg) > > static int fsg_set_alt(struct usb_function *f, unsigned intf, unsigned alt) > > { > > struct fsg_dev *fsg = fsg_from_func(f); > > While you're changing this, it would be nice to add the customary blank > line here. > > > - fsg->common->new_fsg = fsg; > > - raise_exception(fsg->common, FSG_STATE_CONFIG_CHANGE); > > + __raise_exception(fsg->common, FSG_STATE_CONFIG_CHANGE, fsg); > > return USB_GADGET_DELAYED_STATUS; > > } > > > > static void fsg_disable(struct usb_function *f) > > { > > struct fsg_dev *fsg = fsg_from_func(f); > > And here. Otherwise: > > Acked-by: Alan Stern <stern@rowland.harvard.edu> > > Alan Stern
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 };
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 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(-)