[PATCH] usb: gadget: f_mass_storage: cahnge wait_event to wait_event_timeout

[PATCH] usb: gadget: f_mass_storage: cahnge wait_event to wait_event_timeout

[Oh Eomji]

Changed to return a timeout error if there is no response for a certain
period of time in order to solve the problem of waiting for a event
complete while executing unbind.

Signed-off-by: Oh Eomji <eomji.oh@samsung.com>
---
 drivers/usb/gadget/function/f_mass_storage.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/function/f_mass_storage.c b/drivers/usb/gadget/function/f_mass_storage.c
index 950c943..b474840 100644
--- a/drivers/usb/gadget/function/f_mass_storage.c
+++ b/drivers/usb/gadget/function/f_mass_storage.c
@@ -3000,7 +3000,7 @@ static void fsg_unbind(struct usb_configuration *c, struct usb_function *f)
 	if (fsg->common->fsg == fsg) {
 		__raise_exception(fsg->common, FSG_STATE_CONFIG_CHANGE, NULL);
 		/* FIXME: make interruptible or killable somehow? */
-		wait_event(common->fsg_wait, common->fsg != fsg);
+		wait_event_timeout(common->fsg_wait, common->fsg != fsg, HZ / 4);
 	}
 
 	usb_free_all_descriptors(&fsg->function);
-- 
2.7.4

Re: [PATCH] usb: gadget: f_mass_storage: cahnge wait_event to wait_event_timeout

[Andy Shevchenko]

On Thu, Jan 21, 2021 at 03:56:45PM +0900, Oh Eomji wrote:
> Changed to return a timeout error if there is no response for a certain
> period of time in order to solve the problem of waiting for a event
> complete while executing unbind.

Can you shed a light on the choice of the timeout length?

> Signed-off-by: Oh Eomji <eomji.oh@samsung.com>
> ---
>  drivers/usb/gadget/function/f_mass_storage.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/gadget/function/f_mass_storage.c b/drivers/usb/gadget/function/f_mass_storage.c
> index 950c943..b474840 100644
> --- a/drivers/usb/gadget/function/f_mass_storage.c
> +++ b/drivers/usb/gadget/function/f_mass_storage.c
> @@ -3000,7 +3000,7 @@ static void fsg_unbind(struct usb_configuration *c, struct usb_function *f)
>  	if (fsg->common->fsg == fsg) {
>  		__raise_exception(fsg->common, FSG_STATE_CONFIG_CHANGE, NULL);
>  		/* FIXME: make interruptible or killable somehow? */
> -		wait_event(common->fsg_wait, common->fsg != fsg);
> +		wait_event_timeout(common->fsg_wait, common->fsg != fsg, HZ / 4);
>  	}
>  
>  	usb_free_all_descriptors(&fsg->function);
> -- 
> 2.7.4
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] usb: gadget: f_mass_storage: cahnge wait_event to wait_event_timeout

[Greg Kroah-Hartman]

On Thu, Jan 21, 2021 at 03:56:45PM +0900, Oh Eomji wrote:
> Changed to return a timeout error if there is no response for a certain
> period of time in order to solve the problem of waiting for a event
> complete while executing unbind.
> 
> Signed-off-by: Oh Eomji <eomji.oh@samsung.com>
> ---
>  drivers/usb/gadget/function/f_mass_storage.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/gadget/function/f_mass_storage.c b/drivers/usb/gadget/function/f_mass_storage.c
> index 950c943..b474840 100644
> --- a/drivers/usb/gadget/function/f_mass_storage.c
> +++ b/drivers/usb/gadget/function/f_mass_storage.c
> @@ -3000,7 +3000,7 @@ static void fsg_unbind(struct usb_configuration *c, struct usb_function *f)
>  	if (fsg->common->fsg == fsg) {
>  		__raise_exception(fsg->common, FSG_STATE_CONFIG_CHANGE, NULL);
>  		/* FIXME: make interruptible or killable somehow? */
> -		wait_event(common->fsg_wait, common->fsg != fsg);
> +		wait_event_timeout(common->fsg_wait, common->fsg != fsg, HZ / 4);

That's a random choice of a timeout value.

Please document this really really really well as to why you picked this
number, and what it means.

Also, is the commet above this line still correct now?

thanks,

greg k-h

Re: [PATCH] usb: gadget: f_mass_storage: cahnge wait_event to wait_event_timeout

[Alan Stern]

On Thu, Jan 21, 2021 at 03:56:45PM +0900, Oh Eomji wrote:
> Changed to return a timeout error if there is no response for a certain
> period of time in order to solve the problem of waiting for a event
> complete while executing unbind.
> 
> Signed-off-by: Oh Eomji <eomji.oh@samsung.com>
> ---
>  drivers/usb/gadget/function/f_mass_storage.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/gadget/function/f_mass_storage.c b/drivers/usb/gadget/function/f_mass_storage.c
> index 950c943..b474840 100644
> --- a/drivers/usb/gadget/function/f_mass_storage.c
> +++ b/drivers/usb/gadget/function/f_mass_storage.c
> @@ -3000,7 +3000,7 @@ static void fsg_unbind(struct usb_configuration *c, struct usb_function *f)
>  	if (fsg->common->fsg == fsg) {
>  		__raise_exception(fsg->common, FSG_STATE_CONFIG_CHANGE, NULL);
>  		/* FIXME: make interruptible or killable somehow? */
> -		wait_event(common->fsg_wait, common->fsg != fsg);
> +		wait_event_timeout(common->fsg_wait, common->fsg != fsg, HZ / 4);
>  	}
>  
>  	usb_free_all_descriptors(&fsg->function);

No, no, no!

This patch completely misunderstands the purpose of the wait_event call.  
The reason it isn't interruptible is not because that would be difficult 
-- all it takes is adding a timeout argument, as you did here.

The reason is because the driver will get invalid memory accesses if 
fsg_unbind returns too early.  fsg will be deallocated, but 
fsg_set_interface will continue to use it.  This patch completely 
ignores that issue.

Was there any real reason for writing this patch?  Does it solve a 
real-world problem?  Did you encounter a situation where the wait_event 
call would wait for more than 1/4 second?

Alan Stern