* [PATCH] usb: gadget: f_mass_storage: Disable eps during disconnect
@ 2021-10-26 0:44 Wesley Cheng
2021-10-26 14:41 ` Alan Stern
0 siblings, 1 reply; 5+ messages in thread
From: Wesley Cheng @ 2021-10-26 0:44 UTC (permalink / raw)
To: balbi, gregkh; +Cc: linux-usb, linux-kernel, Wesley Cheng
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);
+ 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);
}
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] usb: gadget: f_mass_storage: Disable eps during disconnect
2021-10-26 0:44 [PATCH] usb: gadget: f_mass_storage: Disable eps during disconnect Wesley Cheng
@ 2021-10-26 14:41 ` Alan Stern
2021-10-26 19:40 ` Wesley Cheng
0 siblings, 1 reply; 5+ messages in thread
From: Alan Stern @ 2021-10-26 14:41 UTC (permalink / raw)
To: Wesley Cheng; +Cc: balbi, gregkh, linux-usb, linux-kernel, Wesley Cheng
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] usb: gadget: f_mass_storage: Disable eps during disconnect
2021-10-26 14:41 ` Alan Stern
@ 2021-10-26 19:40 ` Wesley Cheng
2021-10-26 20:57 ` Alan Stern
0 siblings, 1 reply; 5+ messages in thread
From: Wesley Cheng @ 2021-10-26 19:40 UTC (permalink / raw)
To: Alan Stern; +Cc: balbi, gregkh, linux-usb, linux-kernel, Wesley Cheng
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] usb: gadget: f_mass_storage: Disable eps during disconnect
2021-10-26 19:40 ` Wesley Cheng
@ 2021-10-26 20:57 ` Alan Stern
2021-10-27 1:18 ` Wesley Cheng
0 siblings, 1 reply; 5+ messages in thread
From: Alan Stern @ 2021-10-26 20:57 UTC (permalink / raw)
To: Wesley Cheng; +Cc: balbi, gregkh, linux-usb, linux-kernel, 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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] usb: gadget: f_mass_storage: Disable eps during disconnect
2021-10-26 20:57 ` Alan Stern
@ 2021-10-27 1:18 ` Wesley Cheng
0 siblings, 0 replies; 5+ messages in thread
From: Wesley Cheng @ 2021-10-27 1:18 UTC (permalink / raw)
To: Alan Stern; +Cc: balbi, gregkh, linux-usb, linux-kernel, Wesley Cheng
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-10-27 1:18 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-26 0:44 [PATCH] usb: gadget: f_mass_storage: Disable eps during disconnect Wesley Cheng
2021-10-26 14:41 ` Alan Stern
2021-10-26 19:40 ` Wesley Cheng
2021-10-26 20:57 ` Alan Stern
2021-10-27 1:18 ` Wesley Cheng
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).