On Wed, Dec 01, 2021 at 04:41:10PM +0000, John Keeping wrote:
> On Wed, Dec 01, 2021 at 02:02:05AM -0800, Wesley Cheng wrote:
> > During device disconnect or composition unbind, applications should be
> > notified that the endpoints are no longer enabled, so that it can take
> > the proper actions to handle its IO threads. Otherwise, they can be
> > left waiting for endpoints until EPs are re-enabled.
> >
> > Signed-off-by: Wesley Cheng <quic_wcheng@xxxxxxxxxxx>
> > ---
> > drivers/usb/gadget/function/f_fs.c | 7 +++++--
> > 1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
> > index 3c584da9118c..0b0747d96378 100644
> > --- a/drivers/usb/gadget/function/f_fs.c
> > +++ b/drivers/usb/gadget/function/f_fs.c
> > @@ -957,10 +957,12 @@ static ssize_t ffs_epfile_io(struct file *file, struct ffs_io_data *io_data)
> > if (file->f_flags & O_NONBLOCK)
> > return -EAGAIN;
> >
> > - ret = wait_event_interruptible(
> > - epfile->ffs->wait, (ep = epfile->ep));
> > + ret = wait_event_interruptible(epfile->ffs->wait,
> > + (ep = epfile->ep) || !epfile->ffs->func);
I looked at this again, and doesn't this totally break the wait
condition?
epfile->ep is set to non-null in ffs_func_eps_enable() which is called
from ffs_func_set_alt() just after ffs->func is set to non-null, and
then those are also set back to null at the same time.
So the condition boils down to a || !a and this never blocks. Or am I
missing something?
> > if (ret)
> > return -EINTR;
> > + if (!epfile->ffs->func)
> > + return -ENODEV;
>
> This seems strange - we are inside the case where the endpoint is not
> initially enabled, if we're returning ENODEV here shouldn't that happen
> in all cases?
>
> Beyond that, there is no locking for accessing ffs->func here;
> modification happens in gadget callbacks so it's guarded by the gadget
> core (the existing case in ffs_ep0_ioctl() looks suspicious as well).
>
> But I can't see why this change is necessary - there are event
> notifications through ep0 when this happens, as can be seen in the hunk
> below from the ffs_event_add(ffs, FUNCTIONFS_DISABLE) line. If
> userspace cares about this, then it can read the events from ep0.
>
> > }
> >
> > /* Do we halt? */
> > @@ -3292,6 +3294,7 @@ static int ffs_func_set_alt(struct usb_function *f,
> > if (alt == (unsigned)-1) {
> > ffs->func = NULL;
> > ffs_event_add(ffs, FUNCTIONFS_DISABLE);
> > + wake_up_interruptible(&ffs->wait);
> > return 0;
> > }
> >
