On Mon, Aug 14 2017, Jerry Zhang wrote:
> 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?
Yeah, you’re right.
>>
>>> Signed-off-by: Jerry Zhang <zhangjerry@xxxxxxxxxx>
Acked-by: Michal Nazarewicz <mina86@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
