| Message ID | 20211026004456.23054-1-quic_wcheng@quicinc.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 9fff139aeb11186fd8e75860c959c86cb43ab2f6 |
| Headers | show |
| Series | usb: gadget: f_mass_storage: Disable eps during disconnect | expand |
On Mon, Oct 25, 2021 at 05:44:56PM -0700, Wesley Cheng wrote: > From: Wesley Cheng <wcheng@codeaurora.org> > > When receiving a disconnect event from the UDC, the mass storage > function driver currently runs the handle_exception() routine > asynchronously. For UDCs that support runtime PM, there is a > possibility the UDC is already suspended by the time the > do_set_interface() is executed. This can lead to HW register access > while the UDC is already suspended. > > Signed-off-by: Wesley Cheng <wcheng@codeaurora.org> > --- > drivers/usb/gadget/function/f_mass_storage.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/drivers/usb/gadget/function/f_mass_storage.c b/drivers/usb/gadget/function/f_mass_storage.c > index 3cabf7692ee1..752439690fda 100644 > --- a/drivers/usb/gadget/function/f_mass_storage.c > +++ b/drivers/usb/gadget/function/f_mass_storage.c > @@ -2342,6 +2342,16 @@ static void fsg_disable(struct usb_function *f) > { > struct fsg_dev *fsg = fsg_from_func(f); > > + /* Disable the endpoints */ > + if (fsg->bulk_in_enabled) { > + usb_ep_disable(fsg->bulk_in); According to the kerneldoc, this routine must be called in process context. > + fsg->bulk_in_enabled = 0; > + } > + if (fsg->bulk_out_enabled) { > + usb_ep_disable(fsg->bulk_out); > + fsg->bulk_out_enabled = 0; > + } > + > __raise_exception(fsg->common, FSG_STATE_CONFIG_CHANGE, NULL); > } Looks like you'll have to find a different way to avoid the problem. For example, if an exception is pending then you might prevent the gadget from going into runtime suspend until the exception has been handled. Alan Stern
Hi Alan, On 10/26/2021 7:41 AM, Alan Stern wrote: > On Mon, Oct 25, 2021 at 05:44:56PM -0700, Wesley Cheng wrote: >> From: Wesley Cheng <wcheng@codeaurora.org> >> >> When receiving a disconnect event from the UDC, the mass storage >> function driver currently runs the handle_exception() routine >> asynchronously. For UDCs that support runtime PM, there is a >> possibility the UDC is already suspended by the time the >> do_set_interface() is executed. This can lead to HW register access >> while the UDC is already suspended. >> >> Signed-off-by: Wesley Cheng <wcheng@codeaurora.org> >> --- >> drivers/usb/gadget/function/f_mass_storage.c | 10 ++++++++++ >> 1 file changed, 10 insertions(+) >> >> diff --git a/drivers/usb/gadget/function/f_mass_storage.c b/drivers/usb/gadget/function/f_mass_storage.c >> index 3cabf7692ee1..752439690fda 100644 >> --- a/drivers/usb/gadget/function/f_mass_storage.c >> +++ b/drivers/usb/gadget/function/f_mass_storage.c >> @@ -2342,6 +2342,16 @@ static void fsg_disable(struct usb_function *f) >> { >> struct fsg_dev *fsg = fsg_from_func(f); >> >> + /* Disable the endpoints */ >> + if (fsg->bulk_in_enabled) { >> + usb_ep_disable(fsg->bulk_in); > > According to the kerneldoc, this routine must be called in process > context. > >> + fsg->bulk_in_enabled = 0; >> + } >> + if (fsg->bulk_out_enabled) { >> + usb_ep_disable(fsg->bulk_out); >> + fsg->bulk_out_enabled = 0; >> + } >> + >> __raise_exception(fsg->common, FSG_STATE_CONFIG_CHANGE, NULL); >> } > > Looks like you'll have to find a different way to avoid the problem. > For example, if an exception is pending then you might prevent the > gadget from going into runtime suspend until the exception has been > handled. > Thanks for the suggestion. I noticed that a lot of other FDs currently call usb_ep_disable() in the disable/disconnect path as well. Actually, f_mass_storage seems to be the only one that doesn't do so. Maybe we should change the kerneldoc :) Thanks Wesley Cheng
On Tue, Oct 26, 2021 at 12:40:04PM -0700, Wesley Cheng wrote: > Hi Alan, > > On 10/26/2021 7:41 AM, Alan Stern wrote: > > On Mon, Oct 25, 2021 at 05:44:56PM -0700, Wesley Cheng wrote: > >> From: Wesley Cheng <wcheng@codeaurora.org> > >> > >> When receiving a disconnect event from the UDC, the mass storage > >> function driver currently runs the handle_exception() routine > >> asynchronously. For UDCs that support runtime PM, there is a > >> possibility the UDC is already suspended by the time the > >> do_set_interface() is executed. This can lead to HW register access > >> while the UDC is already suspended. > >> > >> Signed-off-by: Wesley Cheng <wcheng@codeaurora.org> > >> --- > >> drivers/usb/gadget/function/f_mass_storage.c | 10 ++++++++++ > >> 1 file changed, 10 insertions(+) > >> > >> diff --git a/drivers/usb/gadget/function/f_mass_storage.c b/drivers/usb/gadget/function/f_mass_storage.c > >> index 3cabf7692ee1..752439690fda 100644 > >> --- a/drivers/usb/gadget/function/f_mass_storage.c > >> +++ b/drivers/usb/gadget/function/f_mass_storage.c > >> @@ -2342,6 +2342,16 @@ static void fsg_disable(struct usb_function *f) > >> { > >> struct fsg_dev *fsg = fsg_from_func(f); > >> > >> + /* Disable the endpoints */ > >> + if (fsg->bulk_in_enabled) { > >> + usb_ep_disable(fsg->bulk_in); > > > > According to the kerneldoc, this routine must be called in process > > context. > > > >> + fsg->bulk_in_enabled = 0; > >> + } > >> + if (fsg->bulk_out_enabled) { > >> + usb_ep_disable(fsg->bulk_out); > >> + fsg->bulk_out_enabled = 0; > >> + } > >> + > >> __raise_exception(fsg->common, FSG_STATE_CONFIG_CHANGE, NULL); > >> } > > > > Looks like you'll have to find a different way to avoid the problem. > > For example, if an exception is pending then you might prevent the > > gadget from going into runtime suspend until the exception has been > > handled. > > > Thanks for the suggestion. I noticed that a lot of other FDs currently > call usb_ep_disable() in the disable/disconnect path as well. Actually, > f_mass_storage seems to be the only one that doesn't do so. Maybe we > should change the kerneldoc :) You're right; I don't see any real reason why the usb_ep_{en,dis}able routines can't run in an atomic context. So if you to make this change to f_mass_storage, you should first submit a patch changing the kerneldoc. Alan Stern
Hi Alan, On 10/26/2021 1:57 PM, Alan Stern wrote: > On Tue, Oct 26, 2021 at 12:40:04PM -0700, Wesley Cheng wrote: >> Hi Alan, >> >> On 10/26/2021 7:41 AM, Alan Stern wrote: >>> On Mon, Oct 25, 2021 at 05:44:56PM -0700, Wesley Cheng wrote: >>>> From: Wesley Cheng <wcheng@codeaurora.org> >>>> >>>> When receiving a disconnect event from the UDC, the mass storage >>>> function driver currently runs the handle_exception() routine >>>> asynchronously. For UDCs that support runtime PM, there is a >>>> possibility the UDC is already suspended by the time the >>>> do_set_interface() is executed. This can lead to HW register access >>>> while the UDC is already suspended. >>>> >>>> Signed-off-by: Wesley Cheng <wcheng@codeaurora.org> >>>> --- >>>> drivers/usb/gadget/function/f_mass_storage.c | 10 ++++++++++ >>>> 1 file changed, 10 insertions(+) >>>> >>>> diff --git a/drivers/usb/gadget/function/f_mass_storage.c b/drivers/usb/gadget/function/f_mass_storage.c >>>> index 3cabf7692ee1..752439690fda 100644 >>>> --- a/drivers/usb/gadget/function/f_mass_storage.c >>>> +++ b/drivers/usb/gadget/function/f_mass_storage.c >>>> @@ -2342,6 +2342,16 @@ static void fsg_disable(struct usb_function *f) >>>> { >>>> struct fsg_dev *fsg = fsg_from_func(f); >>>> >>>> + /* Disable the endpoints */ >>>> + if (fsg->bulk_in_enabled) { >>>> + usb_ep_disable(fsg->bulk_in); >>> >>> According to the kerneldoc, this routine must be called in process >>> context. >>> >>>> + fsg->bulk_in_enabled = 0; >>>> + } >>>> + if (fsg->bulk_out_enabled) { >>>> + usb_ep_disable(fsg->bulk_out); >>>> + fsg->bulk_out_enabled = 0; >>>> + } >>>> + >>>> __raise_exception(fsg->common, FSG_STATE_CONFIG_CHANGE, NULL); >>>> } >>> >>> Looks like you'll have to find a different way to avoid the problem. >>> For example, if an exception is pending then you might prevent the >>> gadget from going into runtime suspend until the exception has been >>> handled. >>> >> Thanks for the suggestion. I noticed that a lot of other FDs currently >> call usb_ep_disable() in the disable/disconnect path as well. Actually, >> f_mass_storage seems to be the only one that doesn't do so. Maybe we >> should change the kerneldoc :) > > You're right; I don't see any real reason why the usb_ep_{en,dis}able > routines can't run in an atomic context. So if you to make this change > to f_mass_storage, you should first submit a patch changing the > kerneldoc. > Sure, will do that and include it in the next rev. Thanks Wesley Cheng
diff --git a/drivers/usb/gadget/function/f_mass_storage.c b/drivers/usb/gadget/function/f_mass_storage.c index 3cabf7692ee1..752439690fda 100644 --- a/drivers/usb/gadget/function/f_mass_storage.c +++ b/drivers/usb/gadget/function/f_mass_storage.c @@ -2342,6 +2342,16 @@ static void fsg_disable(struct usb_function *f) { struct fsg_dev *fsg = fsg_from_func(f); + /* Disable the endpoints */ + if (fsg->bulk_in_enabled) { + usb_ep_disable(fsg->bulk_in); + fsg->bulk_in_enabled = 0; + } + if (fsg->bulk_out_enabled) { + usb_ep_disable(fsg->bulk_out); + fsg->bulk_out_enabled = 0; + } + __raise_exception(fsg->common, FSG_STATE_CONFIG_CHANGE, NULL); }