Hi,
> > I'd prefer fail the request at all, and it is better done in HW.
> > Because per the USB Spec that device can return NAK if a function was
> > unable to accept data From the host. The DWC3 has not been design as
> > this, if software fail the transfer, it is a little weird for host.
> >
>
On Thu, May 19 2016, Changbin Du wrote:
>> On Wed, May 18 2016, Felipe Balbi wrote:
>> > we've been through this before. This needs to be done at the gadget
>> > layer. Gadget driver can over-allocate ahead of time if
>> > gadget->quirk_ep_out_aligned_size is true, then we avoid memcpy() at
>> > th
> On Wed, May 18 2016, Felipe Balbi wrote:
> > we've been through this before. This needs to be done at the gadget
> > layer. Gadget driver can over-allocate ahead of time if
> > gadget->quirk_ep_out_aligned_size is true, then we avoid memcpy() at
> > the UDC driver level.
>
> Right, all right, so
> >> thanks Alan Stern and Michal.
> >> Here just have a comment - the buffered data need be dropped when
> the
> >> epfile is closed, because it means the session is terminated.
> >
> > I blame that on sleep deprivation. Another issue is what to do when
> > endpoint is disabled. Should the buffe
On Wed, May 18 2016, Felipe Balbi wrote:
> we've been through this before. This needs to be done at the gadget
> layer. Gadget driver can over-allocate ahead of time if
> gadget->quirk_ep_out_aligned_size is true, then we avoid memcpy() at
> the UDC driver level.
Right, all right, so let’s look at
Hi,
Michal Nazarewicz writes:
> On Tue, May 17 2016, Changbin Du wrote:
>>> There appears to be no kfifo support for iov_iter though, so I just went
>>> with a simple buffer.
>>>
>>> I haven’t looked at the patch too carefully so this is an RFC rather
>>> than an actual patch at this point. It
On Tue, May 17 2016, Changbin Du wrote:
>> There appears to be no kfifo support for iov_iter though, so I just went
>> with a simple buffer.
>>
>> I haven’t looked at the patch too carefully so this is an RFC rather
>> than an actual patch at this point. It does compile at least.
>>
>> Regardles
> There appears to be no kfifo support for iov_iter though, so I just went
> with a simple buffer.
>
> I haven’t looked at the patch too carefully so this is an RFC rather
> than an actual patch at this point. It does compile at least.
>
> Regardless, the more I thin about it, the more I’m under
On Mon, May 16 2016, Felipe Balbi wrote:
> Michal Nazarewicz writes:
>
>>> Alan Stern writes:
The point is that you don't know whether the host sent more data than
expected. All you know is that the host sent more data than the user
asked the kernel for -- but maybe the user didn'
> On 05/16/2016 06:05 PM, Michal Nazarewicz wrote:
>> So I’ve been looking at AIO handling in f_fs and either I’m stupid or
>> the code is broken.
On Mon, May 16 2016, Lars-Peter Clausen wrote:
> The code was broken. Fixed in commit 332a5b446b791 ("usb: gadget:
> f_fs: Fix EFAULT generation for as
Hi Michal,
On 05/16/2016 06:05 PM, Michal Nazarewicz wrote:
> So I’ve been looking at AIO handling in f_fs and either I’m stupid or
> the code is broken. Here’s part of ffs_user_copy_worker:
>
> int ret = io_data->req->status ? io_data->req->status :
>
On 05/16/2016 06:05 PM, Michal Nazarewicz wrote:
> So I’ve been looking at AIO handling in f_fs and either I’m stupid or
> the code is broken.
The code was broken. Fixed in commit 332a5b446b791 ("usb: gadget: f_fs: Fix
EFAULT generation for async read operations:).
--
To unsubscribe from this list
So I’ve been looking at AIO handling in f_fs and either I’m stupid or
the code is broken. Here’s part of ffs_user_copy_worker:
int ret = io_data->req->status ? io_data->req->status :
io_data->req->actual;
if (io_data->read && ret > 0) {
Michal Nazarewicz writes:
>> Alan Stern writes:
>>> The point is that you don't know whether the host sent more data than
>>> expected. All you know is that the host sent more data than the user
>>> asked the kernel for -- but maybe the user didn't ask for all the
>>> data that he expected. Ma
> Alan Stern writes:
>> The point is that you don't know whether the host sent more data than
>> expected. All you know is that the host sent more data than the user
>> asked the kernel for -- but maybe the user didn't ask for all the
>> data that he expected. Maybe the user wanted to retrieve t
Hi,
Alan Stern writes:
> On Fri, 13 May 2016, Felipe Balbi wrote:
>
>> We deliver to userspace the part userspace requested, right? So that's
>> okay. The USB details WRT e.g. babble or host trying to send more data
>> than expected, needs to be handled within the kernel.
>
> The point is that y
On Fri, May 13 2016, Alan Stern wrote:
> The point is that you don't know whether the host sent more data than
> expected. All you know is that the host sent more data than the user
> asked the kernel for -- but maybe the user didn't ask for all the data
> that he expected. Maybe the user wanted
On Fri, 13 May 2016, Felipe Balbi wrote:
> We deliver to userspace the part userspace requested, right? So that's
> okay. The USB details WRT e.g. babble or host trying to send more data
> than expected, needs to be handled within the kernel.
The point is that you don't know whether the host sent
Hi,
> >> Yeah, it probably deserves a pr_err() or pr_debug(), but host sending
> >> more data than it should, is another problem altogether which needs to
> >> be addressed at the host.
> >>
> >> Adding a print to aid debugging is a good idea, but bailing out on the
> >> peripheral side is not :-s
Hi,
"Du, Changbin" writes:
>> "Du, Changbin" writes:
>> >> right, and that was my point: if we copy more to userspace, then we have
>> >> a real big problem.
>> >>
>> > Yes, we drop the data because we userspace buffer is not enough this time.
>> > The problem here is that really can we just dr
> Hi,
>
> "Du, Changbin" writes:
> >> right, and that was my point: if we copy more to userspace, then we have
> >> a real big problem.
> >>
> > Yes, we drop the data because we userspace buffer is not enough this time.
> > The problem here is that really can we just drop it silently? Maybe not.
Hi,
"Du, Changbin" writes:
>> right, and that was my point: if we copy more to userspace, then we have
>> a real big problem.
>>
> Yes, we drop the data because we userspace buffer is not enough this time.
> The problem here is that really can we just drop it silently? Maybe not.
Yeah, it prob
> Hi,
>
> "Du, Changbin" writes:
> >> >> > These all can lead host send more than device wanted bytes. For
> sure
> >> >> > it wrong at host side, but device side don't know.
> >> >>
> >> >> but none of this means we have a bug at device side. In fact, by
> >> >> allowing these extra bytes to rea
Hi,
"Du, Changbin" writes:
>> >> > These all can lead host send more than device wanted bytes. For sure
>> >> > it wrong at host side, but device side don't know.
>> >>
>> >> but none of this means we have a bug at device side. In fact, by
>> >> allowing these extra bytes to reach userspace, we
Hi,
"Du, Changbin" writes:
>> >> > These all can lead host send more than device wanted bytes. For sure
>> >> > it wrong at host side, but device side don't know.
>> >>
>> >> but none of this means we have a bug at device side. In fact, by
>> >> allowing these extra bytes to reach userspace, we
Hi,
>
> we need a min() here. Better version below:
No need. copy_to_iter will do it for us.
Best Regards,
Du, Changbin
>
> diff --git a/drivers/usb/gadget/function/f_fs.c
> b/drivers/usb/gadget/function/f_fs.c
> index 73515d54e1cc..6c49b152f46e 100644
> --- a/drivers/usb/gadget/function/f_fs.c
> >> > These all can lead host send more than device wanted bytes. For sure
> >> > it wrong at host side, but device side don't know.
> >>
> >> but none of this means we have a bug at device side. In fact, by
> >> allowing these extra bytes to reach userspace, we could be creating a
> >> possible a
Hi again,
Felipe Balbi writes:
> @@ -811,7 +815,12 @@ static ssize_t ffs_epfile_io(struct file *file, struct
> ffs_io_data *io_data)
>*/
> ret = interrupted ? -EINTR : ep->status;
> if (io_data->read && ret > 0) {
> - ret = copy_to
Hi,
"Du, Changbin" writes:
>> >> > The problem is device side app sometimes received incorrect data
>> caused
>> >> > by the dropping. Most times the error can be detected by APP itself, but
>> >>
>> >> why ? app did e.g. read(5), that caused driver to queue a usb_request
>> >> with length set t
> > The extra bytes can be anything(random), they just data from APP layer.
> > It doesn't make sense for you to check. So I will not dump them, sorry.
>
> interesting, so you claim to have found a bug, but when asked to provide
> more information your answer is "no" ? Thanks :-)
>
Do you really
Hi,
"Du, Changbin" writes:
> Hi,
>
>> >> and when has this actually happened ? Host should not send more data in
>> >> this case, if it does, it's an error on the host side. Also, returning
>> >> -EOVERFLOW is not exactly correct here, because you'd violate POSIX
>> >> specification of read(), r
Hi,
> >> and when has this actually happened ? Host should not send more data in
> >> this case, if it does, it's an error on the host side. Also, returning
> >> -EOVERFLOW is not exactly correct here, because you'd violate POSIX
> >> specification of read(), right ?
> >>
> > This can happen if th
Hi,
"Du, Changbin" writes:
>> > If it happen, we can keep the excess data for next i/o, or
>> > report an error. But we cannot silently drop data, because
>> > USB layer should ensure the data integrality it has transferred,
>> > otherwise applications may get corrupt data if it doesn't
>> > det
> On Wed, May 11 2016, Felipe Balbi wrote:
> > Also, returning -EOVERFLOW is not exactly correct here, because you'd
> > violate POSIX specification of read(), right ?
>
> Maybe we could piggyback on:
>
>EINVAL fd was created via a call to timerfd_create(2) and the
> wrong s
> Hi,
>
> changbin...@intel.com writes:
> > From: "Du, Changbin"
> >
> > Since the buffer size for req is rounded up to maxpacketsize,
> > then we may end up with more data then user space has space
> > for.
>
> only for OUT direction with the controller you're using ;-)
For sure.
>
> > If it
On Wed, May 11 2016, Felipe Balbi wrote:
> Also, returning -EOVERFLOW is not exactly correct here, because you'd
> violate POSIX specification of read(), right ?
Maybe we could piggyback on:
EINVAL fd was created via a call to timerfd_create(2) and the
wrong size buffer was g
Hi,
changbin...@intel.com writes:
> From: "Du, Changbin"
>
> Since the buffer size for req is rounded up to maxpacketsize,
> then we may end up with more data then user space has space
> for.
only for OUT direction with the controller you're using ;-)
> If it happen, we can keep the excess dat
From: "Du, Changbin"
Since the buffer size for req is rounded up to maxpacketsize,
then we may end up with more data then user space has space
for.
If it happen, we can keep the excess data for next i/o, or
report an error. But we cannot silently drop data, because
USB layer should ensure the da
38 matches
Mail list logo
