* Re: [PATCH] usb: gadget: f_mass_storage: cahnge wait_event to wait_event_timeout
2021-01-21 6:56 ` [PATCH] usb: gadget: f_mass_storage: cahnge wait_event to wait_event_timeout Oh Eomji
@ 2021-01-21 10:11 ` Andy Shevchenko
2021-01-21 11:22 ` Greg Kroah-Hartman
2021-01-21 15:45 ` Alan Stern
2 siblings, 0 replies; 4+ messages in thread
From: Andy Shevchenko @ 2021-01-21 10:11 UTC (permalink / raw)
To: Oh Eomji
Cc: balbi, Greg Kroah-Hartman, Gustavo A. R. Silva, Bart Van Assche,
linux-usb, linux-kernel
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
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] usb: gadget: f_mass_storage: cahnge wait_event to wait_event_timeout
2021-01-21 6:56 ` [PATCH] usb: gadget: f_mass_storage: cahnge wait_event to wait_event_timeout Oh Eomji
2021-01-21 10:11 ` Andy Shevchenko
@ 2021-01-21 11:22 ` Greg Kroah-Hartman
2021-01-21 15:45 ` Alan Stern
2 siblings, 0 replies; 4+ messages in thread
From: Greg Kroah-Hartman @ 2021-01-21 11:22 UTC (permalink / raw)
To: Oh Eomji
Cc: balbi, Gustavo A. R. Silva, Andy Shevchenko, Bart Van Assche,
linux-usb, linux-kernel
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
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] usb: gadget: f_mass_storage: cahnge wait_event to wait_event_timeout
2021-01-21 6:56 ` [PATCH] usb: gadget: f_mass_storage: cahnge wait_event to wait_event_timeout Oh Eomji
2021-01-21 10:11 ` Andy Shevchenko
2021-01-21 11:22 ` Greg Kroah-Hartman
@ 2021-01-21 15:45 ` Alan Stern
2 siblings, 0 replies; 4+ messages in thread
From: Alan Stern @ 2021-01-21 15:45 UTC (permalink / raw)
To: Oh Eomji
Cc: balbi, Greg Kroah-Hartman, Gustavo A. R. Silva, Andy Shevchenko,
Bart Van Assche, linux-usb, linux-kernel
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
^ permalink raw reply [flat|nested] 4+ messages in thread