On Mon, Aug 14, 2017 at 7:26 AM, Michal Nazarewicz <mina86@xxxxxxxxxx> wrote:
> On Fri, Aug 11 2017, Jerry Zhang wrote:
>> Users can apply i/o in the wrong direction on an
>> endpoint to stall it. In case there is an error
>> that does not allow the endpoint to be stalled,
>> we want the user to know.
>>
>> An operation to stall the endpoint will return
>> EBADMSG if successful, EAGAIN if there are still
>> queued requests, and other errors depending on
>> the underlying implementation.
>>
>> Also remove the conditional since it is always true.
>
> I’m not sure it’s true. ‘ep = epfile->ep;’ is executed prior to taking
> a spin_lock so by the time the spin_lock is taken, epfile->ep could
> change.
spin_lock_irq(&epfile->ffs->eps_lock);
if (epfile->ep != ep) {
/* In the meantime, endpoint got disabled or
changed. */
ret = -ESHUTDOWN;
} else if (halt) {
ret = usb_ep_set_halt(ep->ep);
if (!ret)
ret = -EBADMSG;
}
Yeah, but wouldn't the first if block (epfile->ep != ep) catch all
cases of that,
so in all the else if blocks epfile->ep == ep is always true?
>
>> Signed-off-by: Jerry Zhang <zhangjerry@xxxxxxxxxx>
>> ---
>> Notes for v1:
>> - What's the reasoning behind having this behavior
>> in epfile_io? We could also consider moving this
>> codepath to an ioctl and documenting it.
>
> This was a design in gadgetfs. At this point I don’t think it can be
> changed.
Got it.
>
>> drivers/usb/gadget/function/f_fs.c | 7 +++----
>> 1 file changed, 3 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
>> index d21874b35cf6..9990944a7245 100644
>> --- a/drivers/usb/gadget/function/f_fs.c
>> +++ b/drivers/usb/gadget/function/f_fs.c
>> @@ -961,10 +961,9 @@ static ssize_t ffs_epfile_io(struct file *file, struct ffs_io_data *io_data)
>> /* In the meantime, endpoint got disabled or changed. */
>> ret = -ESHUTDOWN;
>> } else if (halt) {
>> - /* Halt */
>> - if (likely(epfile->ep == ep) && !WARN_ON(!ep->ep))
>> - usb_ep_set_halt(ep->ep);
>> - ret = -EBADMSG;
>> + ret = usb_ep_set_halt(ep->ep);
>> + if (!ret)
>> + ret = -EBADMSG;
>> } else if (unlikely(data_len == -EINVAL)) {
>> /*
>> * Sanity Check: even though data_len can't be used
>
> --
> Best regards
> ミハウ “𝓶𝓲𝓷𝓪86” ナザレヴイツ
> «If at first you don’t succeed, give up skydiving»
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
