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