Date:   Thu, 2 Dec 2021 14:49:09 +0000
From:   John Keeping <john@metanate.com>
To:     Wesley Cheng <quic_wcheng@quicinc.com>
Cc:     balbi@kernel.org, gregkh@linuxfoundation.org,
        linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org,
        quic_jackp@quicinc.com
Subject: Re: [PATCH] usb: gadget: f_fs: Wake up IO thread during disconnect
Message-ID: <Yajc5f3LDm+dSji/@donbot>
References: <20211201100205.25448-1-quic_wcheng@quicinc.com>
 <YaelpmsJXmhTY4A0@donbot>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <YaelpmsJXmhTY4A0@donbot>
Precedence: bulk

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@quicinc.com>
> > ---
> >  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;
> >  	}
> >