2021-10-26 05:52:01

by Wesley Cheng

Subject: [PATCH] usb: gadget: f_mass_storage: Disable eps during disconnect

From: Wesley Cheng <[email protected]>

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 <[email protected]>
---
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

2021-10-26 20:08:47

by Alan Stern

Subject: Re: [PATCH] usb: gadget: f_mass_storage: Disable eps during disconnect

On Mon, Oct 25, 2021 at 05:44:56PM -0700, Wesley Cheng wrote:
> From: Wesley Cheng <[email protected]>
>
> 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 <[email protected]>
> ---
> 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

2021-10-27 09:53:06

by Wesley Cheng

Subject: Re: [PATCH] usb: gadget: f_mass_storage: Disable eps during disconnect

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 <[email protected]>
>>
>> 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 <[email protected]>
>> ---
>> 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

2021-10-27 10:24:41

by Alan Stern

Subject: Re: [PATCH] usb: gadget: f_mass_storage: Disable eps during disconnect

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 <[email protected]>
> >>
> >> 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 <[email protected]>
> >> ---
> >> 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

2021-10-27 14:51:53

by Wesley Cheng

Subject: Re: [PATCH] usb: gadget: f_mass_storage: Disable eps during disconnect

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 <[email protected]>
>>>>
>>>> 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 <[email protected]>
>>>> ---
>>>> 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