Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1400520 > unrolled thread

RE: [PATCH] usb: gadget: f_fs: report error if excess data received

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  RE: [PATCH] usb: gadget: f_fs: report error if excess data received "Du, Changbin" <changbin.du@intel.com> - 2016-05-13 08:00 +0200
    RE: [PATCH] usb: gadget: f_fs: report error if excess data received Felipe Balbi <felipe.balbi@linux.intel.com> - 2016-05-13 08:40 +0200
      RE: [PATCH] usb: gadget: f_fs: report error if excess data received "Du, Changbin" <changbin.du@intel.com> - 2016-05-13 12:40 +0200
      RE: [PATCH] usb: gadget: f_fs: report error if excess data received Alan Stern <stern@rowland.harvard.edu> - 2016-05-13 16:30 +0200
        Re: [PATCH] usb: gadget: f_fs: report error if excess data received Michal Nazarewicz <mina86@mina86.com> - 2016-05-14 22:40 +0200
        RE: [PATCH] usb: gadget: f_fs: report error if excess data received Felipe Balbi <felipe.balbi@linux.intel.com> - 2016-05-16 15:10 +0200
          Re: [PATCH] usb: gadget: f_fs: report error if excess data received Michal Nazarewicz <mina86@mina86.com> - 2016-05-16 15:10 +0200
            Re: [PATCH] usb: gadget: f_fs: report error if excess data received Felipe Balbi <felipe.balbi@linux.intel.com> - 2016-05-16 15:20 +0200
              Re: [PATCH] usb: gadget: f_fs: report error if excess data received Michal Nazarewicz <mina86@mina86.com> - 2016-05-16 21:10 +0200
                RE: [PATCH] usb: gadget: f_fs: report error if excess data received "Du, Changbin" <changbin.du@intel.com> - 2016-05-17 05:00 +0200
                  Re: [PATCH] usb: gadget: f_fs: report error if excess data received Michal Nazarewicz <mina86@mina86.com> - 2016-05-18 11:50 +0200
                    Re: [PATCH] usb: gadget: f_fs: report error if excess data received Felipe Balbi <felipe.balbi@linux.intel.com> - 2016-05-18 12:20 +0200
                      Re: [PATCH] usb: gadget: f_fs: report error if excess data received Michal Nazarewicz <mina86@mina86.com> - 2016-05-18 15:40 +0200
                        RE: [PATCH] usb: gadget: f_fs: report error if excess data received "Du, Changbin" <changbin.du@intel.com> - 2016-05-19 05:00 +0200
                          Re: [PATCH] usb: gadget: f_fs: report error if excess data received Michal Nazarewicz <mina86@mina86.com> - 2016-05-19 09:40 +0200
                            RE: [PATCH] usb: gadget: f_fs: report error if excess data received "Du, Changbin" <changbin.du@intel.com> - 2016-05-19 11:00 +0200
                      RE: [PATCH] usb: gadget: f_fs: report error if excess data received "Du, Changbin" <changbin.du@intel.com> - 2016-05-19 04:40 +0200

#1400520 — RE: [PATCH] usb: gadget: f_fs: report error if excess data received

> Hi,
> 
> "Du, Changbin" <changbin.du@intel.com> 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 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
> 
Ok, if we think this is a problem at host side that the transfer is not device
expected, then device side should not accept the data or deliver the
transferred data to userspace. But now we take part of the data to userspace
and says it is ok.
Do you agree with this point?

IMO, we expose usb transfer as a file on device side. But file read() doesn't
have a requirement that "sorry, you cannot read so little! you need read all
once, else we may drop data for you. :) ".
And some library that may retry read() until get enough data (which is normal
For a general read). Then sometimes the buffer size for sys_read may not as
expected. This is why I think ioctl approach is more appropriate for usb transfer.

> --
> Balbi

Best Regards,
Du, Changbin

[toc] | [next] | [standalone]

#1400536

[Multipart message — attachments visible in raw view] — view raw

Hi,

"Du, Changbin" <changbin.du@intel.com> writes:
>> "Du, Changbin" <changbin.du@intel.com> 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 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
>> 
> Ok, if we think this is a problem at host side that the transfer is not device
> expected, then device side should not accept the data or deliver the
> transferred data to userspace. But now we take part of the data to userspace
> and says it is ok.
> Do you agree with this point?

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.

> IMO, we expose usb transfer as a file on device side. But file read() doesn't
> have a requirement that "sorry, you cannot read so little! you need read all
> once, else we may drop data for you. :) ".

but that's not how read() semantics work. When userspace asks to read(x)
bytes, we have three possible outcomes:

i. We have x bytes to return, so we copy_to_user(x)

ii. We have y < x bytes to return, so we copy_to_user(y)

iii. We have y > x bytes to return, so we copy_to_user(x)

This is exactly how the kernel is behaving. The only "detail" we have is
that, for some reason, host is sending too much data. what I still don't
know is if this extra data is garbage or something userspace genuinely
cares about. Do you know the answer to this?

> And some library that may retry read() until get enough data (which is
> normal For a general read). Then sometimes the buffer size for
> sys_read may not as expected. This is why I think ioctl approach is
> more appropriate for usb transfer.

no, this won't change anything. Besides, it's a pointless discussion as
cannot break userspace ABI. GadgetFS and FunctionFS have been shipping
in kernel for many years.

-- 
balbi

[toc] | [prev] | [next] | [standalone]

#1400688

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
> >>
> > Ok, if we think this is a problem at host side that the transfer is not device
> > expected, then device side should not accept the data or deliver the
> > transferred data to userspace. But now we take part of the data to
> userspace
> > and says it is ok.
> > Do you agree with this point?
> 
> 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.
> 
*babble* error is a good example. As you know, when xhci received more
data than expected, how does it process it? It doesn't deliver to software
the part userspace requested, but just give an error. And the xhci driver
set the urb status to -EOVERFLOW. This is similar to device side between
kernel and userspace.

> > IMO, we expose usb transfer as a file on device side. But file read() doesn't
> > have a requirement that "sorry, you cannot read so little! you need read all
> > once, else we may drop data for you. :) ".
> 
> but that's not how read() semantics work. When userspace asks to read(x)
> bytes, we have three possible outcomes:
> 
> i. We have x bytes to return, so we copy_to_user(x)
> 
> ii. We have y < x bytes to return, so we copy_to_user(y)
> 
> iii. We have y > x bytes to return, so we copy_to_user(x)
> 
I totally agree with these. They are all right. But what if userspace read
Twice? EG. When it want read a its packet, it may first read a head size,
Then read body.
read(20) = read(5) + read(15)
If this normal for a file, and works well, right? But if it happens on 
FunctionFS, the first read success, but the remailing 15 bytes lost because
they dropped by kernel. You may think it is host's bug, which also means
FunctionFS file does be different. You can compare usb with network,
Does network driver drop data? Afaik, You can read any bytes from a
socket, and data never lost.

For example, the host may send "aaabbb" and "cccddd", and device
side app May get "aaaccc" and all read success! Host send also success! 
But "bbb" "ddd" are lost.

The different views we have is on how to process the extra data. So we
can focus discussion here.

> This is exactly how the kernel is behaving. The only "detail" we have is
> that, for some reason, host is sending too much data. what I still don't
> know is if this extra data is garbage or something userspace genuinely
> cares about. Do you know the answer to this?
> 
IMO, FunctionFS should not care about what the data is. That is userspace' s
logic and meaningless for kernel. Kernel should not assume the data is garbage
or not. Is it right?

PS, for adb, it will re-open FunctionFS file after it detect unexpected data.

> > And some library that may retry read() until get enough data (which is
> > normal For a general read). Then sometimes the buffer size for
> > sys_read may not as expected. This is why I think ioctl approach is
> > more appropriate for usb transfer.
> 
> no, this won't change anything. Besides, it's a pointless discussion as
> cannot break userspace ABI. GadgetFS and FunctionFS have been shipping
> in kernel for many years.
> 
That because developers know the special requirement of FunctionFS,
just like adb.

> --
> Balbi

Do you mind if I modify my original patch for print error instead of return
error?

Best Regards,
Du, Changbin

[toc] | [prev] | [next] | [standalone]

#1400829

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 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 the full set of
data using two read() system calls.

Alan Stern

[toc] | [prev] | [next] | [standalone]

#1401146

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 to retrieve the full set of
> data using two read() system calls.

I was wondering about that for a while actually.  So far, f_fs’ model
was: one read, one request.  Splitting requests would certainly be
possible, but is that what f_fs’ users would expect to happen if host
rounds the request up?

-- 
Best regards
ミハウ “𝓶𝓲𝓷𝓪86” ナザレヴイツ
«If at first you don’t succeed, give up skydiving»

[toc] | [prev] | [next] | [standalone]

#1401487

[Multipart message — attachments visible in raw view] — view raw

Hi,

Alan Stern <stern@rowland.harvard.edu> 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 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 the full set of
> data using two read() system calls.

right, but that just means we need to buffer the data instead of bailing
out of the first read() completely.

-- 
balbi

[toc] | [prev] | [next] | [standalone]

#1401488

> Alan Stern <stern@rowland.harvard.edu> 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 the full
>> set of data using two read() system calls.

On Mon, May 16 2016, Felipe Balbi wrote:
> right, but that just means we need to buffer the data instead of bailing
> out of the first read() completely.

Correct.

I have a ~4h bus ride ahead of me so I’ll try to implement it.  If you
don’t hear from me by the end of the day, there probably wasn’t enough
space/comfort in the bus to use a laptop.

-- 
Best regards
ミハウ “𝓶𝓲𝓷𝓪86” ナザレヴイツ
«If at first you don’t succeed, give up skydiving»

[toc] | [prev] | [next] | [standalone]

#1401493

[Multipart message — attachments visible in raw view] — view raw

Michal Nazarewicz <mina86@mina86.com> writes:

>> Alan Stern <stern@rowland.harvard.edu> 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 the full
>>> set of data using two read() system calls.
>
> On Mon, May 16 2016, Felipe Balbi wrote:
>> right, but that just means we need to buffer the data instead of bailing
>> out of the first read() completely.
>
> Correct.
>
> I have a ~4h bus ride ahead of me so I’ll try to implement it.  If you
> don’t hear from me by the end of the day, there probably wasn’t enough
> space/comfort in the bus to use a laptop.

Cool, Michal. Thanks

seems like a kfifo would do well here(?)

-- 
balbi

[toc] | [prev] | [next] | [standalone]

#1401711

On Mon, May 16 2016, Felipe Balbi wrote:
> Michal Nazarewicz <mina86@mina86.com> writes:
>
>>> Alan Stern <stern@rowland.harvard.edu> 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 the full
>>>> set of data using two read() system calls.
>>
>> On Mon, May 16 2016, Felipe Balbi wrote:
>>> right, but that just means we need to buffer the data instead of bailing
>>> out of the first read() completely.
>>
>> Correct.
>>
>> I have a ~4h bus ride ahead of me so I’ll try to implement it.  If you
>> don’t hear from me by the end of the day, there probably wasn’t enough
>> space/comfort in the bus to use a laptop.
>
> Cool, Michal. Thanks
>
> seems like a kfifo would do well here(?)

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 the impression
that the whole rounding up in f_fs was a mistake.  And the more I’m
leaning towards ignoring the excess data set by the host.

---------- >8 ----------------------------------------------------------
Subject: usb: gadget: f_fs: buffer data from ‘oversized’ OUT requests

f_fs rounds up read(2) requests to a multiple of a max packet size
which means that host may provide more data than user has space for.
So far, the excess data has been silently ignored.

This introduces a buffer for a tail of such requests so that they are
returned on next read instead of being ignored.

Signed-off-by: Michal Nazarewicz <mina86@mina86.com>
---
 drivers/usb/gadget/function/f_fs.c | 63 +++++++++++++++++++++++++++++++++-----
 1 file changed, 56 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
index 2c314c1..7d3c51a 100644
--- a/drivers/usb/gadget/function/f_fs.c
+++ b/drivers/usb/gadget/function/f_fs.c
@@ -130,6 +130,12 @@ struct ffs_epfile {
 
 	struct dentry			*dentry;
 
+	/*
+	 * Buffer for holding data from partial reads which may happen since
+	 * we’re rounding user read requests to a multiple of a max packet size.
+	 */
+	struct ffs_buffer		*read_buffer;
+
 	char				name[5];
 
 	unsigned char			in;	/* P: ffs->eps_lock */
@@ -138,6 +144,12 @@ struct ffs_epfile {
 	unsigned char			_pad;
 };
 
+struct ffs_buffer {
+	size_t length;
+	char *data;
+	char storage[];
+};
+
 /*  ffs_io_data structure ***************************************************/
 
 struct ffs_io_data {
@@ -681,6 +693,24 @@ static void ffs_epfile_async_io_complete(struct usb_ep *_ep,
 	schedule_work(&io_data->work);
 }
 
+static ssize_t ffs_epfile_read_buffered(struct ffs_epfile *epfile,
+					struct iov_iter *iter)
+{
+	struct ffs_buffer *buf = epfile->read_buffer;
+	ssize_t ret = 0;
+	if (buf) {
+		ret = copy_to_iter(buf->data, buf->length, iter);
+		buf->length -= ret;
+		if (buf->length) {
+			buf->data += ret;
+		} else {
+			kfree(buf);
+			epfile->read_buffer = NULL;
+		}
+	}
+	return ret;
+}
+
 static ssize_t ffs_epfile_io(struct file *file, struct ffs_io_data *io_data)
 {
 	struct ffs_epfile *epfile = file->private_data;
@@ -710,6 +740,18 @@ static ssize_t ffs_epfile_io(struct file *file, struct ffs_io_data *io_data)
 	if (halt && epfile->isoc)
 		return -EINVAL;
 
+	/*
+	 * Do we have buffered data from previous partial read?  Check that for
+	 * synchronous case only because we do not have facility to ‘wake up’
+	 * a pending asynchronous read and push buffered data to it which we
+	 * would need to make things behave consistently.
+	 */
+	if (!halt && !io_data->aio && io_data->read) {
+		ret = ffs_epfile_read_buffered(epfile, &io_data->data);
+		if (ret)
+			return ret;
+	}
+
 	/* Allocate & copy */
 	if (!halt) {
 		/*
@@ -804,17 +846,24 @@ static ssize_t ffs_epfile_io(struct file *file, struct ffs_io_data *io_data)
 			interrupted = ep->status < 0;
 		}
 
-		/*
-		 * XXX We may end up silently droping data here.  Since data_len
-		 * (i.e. req->length) may be bigger than len (after being
-		 * rounded up to maxpacketsize), we may end up with more data
-		 * then user space has space for.
-		 */
 		ret = interrupted ? -EINTR : ep->status;
 		if (io_data->read && ret > 0) {
+			size_t left;
 			ret = copy_to_iter(data, ret, &io_data->data);
-			if (!ret)
+			left = ep->status - ret;
+			if (!left) {
+				/* nop */
+			} else if (iov_iter_count(&io_data->data)) {
 				ret = -EFAULT;
+			} else {
+				struct ffs_buffer *buf = kmalloc(
+					sizeof(*epfile->read_buffer) + left,
+					GFP_KERNEL);
+				buf->length = left;
+				buf->data = buf->storage;
+				memcpy(buf->storage, data + ret, left);
+				epfile->read_buffer = buf;
+			}
 		}
 		goto error_mutex;
 	} else if (!(req = usb_ep_alloc_request(ep->ep, GFP_KERNEL))) {
-- 
Best regards
ミハウ “𝓶𝓲𝓷𝓪86” ナザレヴイツ
«If at first you don’t succeed, give up skydiving»

[toc] | [prev] | [next] | [standalone]

#1402054

> 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 the impression
> that the whole rounding up in f_fs was a mistake.  And the more I’m
> leaning towards ignoring the excess data set by the host.
> 
> ---------- >8 ----------------------------------------------------------
> Subject: usb: gadget: f_fs: buffer data from ‘oversized’ OUT requests
> 
> f_fs rounds up read(2) requests to a multiple of a max packet size
> which means that host may provide more data than user has space for.
> So far, the excess data has been silently ignored.
> 
> This introduces a buffer for a tail of such requests so that they are
> returned on next read instead of being ignored.
> 

Congratulations finally reach an agreement, 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.

[toc] | [prev] | [next] | [standalone]

#1402841

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.
>> 
>> Regardless, the more I thin about it, the more I’m under the impression
>> that the whole rounding up in f_fs was a mistake.  And the more I’m
>> leaning towards ignoring the excess data set by the host.
>> 
>> ---------- >8 ----------------------------------------------------------
>> Subject: usb: gadget: f_fs: buffer data from ‘oversized’ OUT requests
>> 
>> f_fs rounds up read(2) requests to a multiple of a max packet size
>> which means that host may provide more data than user has space for.
>> So far, the excess data has been silently ignored.
>> 
>> This introduces a buffer for a tail of such requests so that they are
>> returned on next read instead of being ignored.
>> 
>
> Congratulations finally reach an agreement,

To be honest, if it was up to me, I would rip request size rounding up
out of the code.

> 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 buffer be cleared as soon as the
endpoint is disabled?  Or maybe when the endpoint is enabled again?  Or
maybe it should never be cleared?

If the buffer is cleared when endpoint is disabled, we again silently
drop data.  On the other hand, if we don’t do that, read() on the
endpoint will may succeed even if the configuration is disabled which
may be surprising for users.

-- 
Best regards
ミハウ “𝓶𝓲𝓷𝓪86” ナザレヴイツ
«If at first you don’t succeed, give up skydiving»

[toc] | [prev] | [next] | [standalone]

#1402860

[Multipart message — attachments visible in raw view] — view raw

Hi,

Michal Nazarewicz <mina86@mina86.com> 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 does compile at least.
>>> 
>>> Regardless, the more I thin about it, the more I’m under the impression
>>> that the whole rounding up in f_fs was a mistake.  And the more I’m
>>> leaning towards ignoring the excess data set by the host.
>>> 
>>> ---------- >8 ----------------------------------------------------------
>>> Subject: usb: gadget: f_fs: buffer data from ‘oversized’ OUT requests
>>> 
>>> f_fs rounds up read(2) requests to a multiple of a max packet size
>>> which means that host may provide more data than user has space for.
>>> So far, the excess data has been silently ignored.
>>> 
>>> This introduces a buffer for a tail of such requests so that they are
>>> returned on next read instead of being ignored.
>>> 
>>
>> Congratulations finally reach an agreement,
>
> To be honest, if it was up to me, I would rip request size rounding up
> out of the code.

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.

>> 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 buffer be cleared as soon as the
> endpoint is disabled?  Or maybe when the endpoint is enabled again?  Or
> maybe it should never be cleared?
>
> If the buffer is cleared when endpoint is disabled, we again silently
> drop data.  On the other hand, if we don’t do that, read() on the
> endpoint will may succeed even if the configuration is disabled which
> may be surprising for users.

tough decision... but seems like clearing the buffer as soon as ep is
disabled is the way to go.

-- 
balbi

[toc] | [prev] | [next] | [standalone]

#1402982

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 it from a regular USB function point
of view.  If a USB function allocates a request which is not aligned,
UDC will align the buffer and *drop* excess data.  Seeing how ugly
f_fs’s code is becoming, I’m now leaning to letting to f_fs do the same
thing: if user space makes an unaligned read, f_fs aligns the buffer and
then drops excess data.

Any arguments for f_fs to not drop the data apply to UDC, so they should
behave identically.

-- 
Best regards
ミハウ “𝓶𝓲𝓷𝓪86” ナザレヴイツ
«If at first you don’t succeed, give up skydiving»

[toc] | [prev] | [next] | [standalone]

#1403407

> 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 it from a regular USB function point
> of view.  If a USB function allocates a request which is not aligned,
> UDC will align the buffer and *drop* excess data.  Seeing how ugly
>
Do you mean UDC driver align the buffer? I searched the code, currently
only DWC3 needs buffer size to be aligned to MaxPacketSize on ep out.
And the align is done in f_fs driver.

> f_fs’s code is becoming, I’m now leaning to letting to f_fs do the same
> thing: if user space makes an unaligned read, f_fs aligns the buffer and
> then drops excess data.
> 
> Any arguments for f_fs to not drop the data apply to UDC, so they should
> behave identically.
> 
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.

So, now we have 3 choices:
1) buffer the excess data
2) fail the transfer
3) drop the excess data, then print an warning message

Which one do you prefer?

> --
> Best regards
> ミハウ “𝓶𝓲𝓷𝓪86” ナザレヴイツ
> «If at first you don’t succeed, give up skydiving»

Best Regards,
Du, Changbin

[toc] | [prev] | [next] | [standalone]

#1403477

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
>> > the UDC driver level.
>> 
>> Right, all right, so let’s look at it from a regular USB function point
>> of view.  If a USB function allocates a request which is not aligned,
>> UDC will align the buffer and *drop* excess data.  Seeing how ugly

> Do you mean UDC driver align the buffer? I searched the code, currently
> only DWC3 needs buffer size to be aligned to MaxPacketSize on ep out.
> And the align is done in f_fs driver.

I thought that was what was happening based on Felipe’s comment about
avoiding memcpy.  I looked at the code now and dunno what actually
happens.

>> f_fs’s code is becoming, I’m now leaning to letting to f_fs do the same
>> thing: if user space makes an unaligned read, f_fs aligns the buffer and
>> then drops excess data.
>> 
>> Any arguments for f_fs to not drop the data apply to UDC, so they should
>> behave identically.
>> 
> 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.
>
> So, now we have 3 choices:
> 1) buffer the excess data
> 2) fail the transfer

You mean fail when more data has been sent (i.e. drop the whole packet)
or fail at entry to read() if the buffer is not aligned?

> 3) drop the excess data, then print an warning message
>
> Which one do you prefer?

I think f_fs should mimic whatever happens if unaligned request is
queued on dwc3.  As far as I understand, this is not 1.

I’ll be travelling again on Friday so I’ll finish up the patch doing 1
so we will have a choice between 1 (my patch) and 3 (your patch).

-- 
Best regards
ミハウ “𝓶𝓲𝓷𝓪86” ナザレヴイツ
«If at first you don’t succeed, give up skydiving»

[toc] | [prev] | [next] | [standalone]

#1403510

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.
> >
> > So, now we have 3 choices:
> > 1) buffer the excess data
> > 2) fail the transfer
> 
> You mean fail when more data has been sent (i.e. drop the whole packet)
> or fail at entry to read() if the buffer is not aligned?
> 
I mean the first one.

> > 3) drop the excess data, then print an warning message
> >
> > Which one do you prefer?
> 
> I think f_fs should mimic whatever happens if unaligned request is
> queued on dwc3.  As far as I understand, this is not 1.
> 
> I’ll be travelling again on Friday so I’ll finish up the patch doing 1
> so we will have a choice between 1 (my patch) and 3 (your patch).
> 
Great! Prefer your patch if #1 works good.

> --
> Best regards
> ミハウ “𝓶𝓲𝓷𝓪86” ナザレヴイツ
> «If at first you don’t succeed, give up skydiving»

Best Regards,
Du, Changbin

[toc] | [prev] | [next] | [standalone]

#1403404

> >> 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 buffer be cleared as soon as the
> > endpoint is disabled?  Or maybe when the endpoint is enabled again?  Or
> > maybe it should never be cleared?
> >
> > If the buffer is cleared when endpoint is disabled, we again silently
> > drop data.  On the other hand, if we don’t do that, read() on the
> > endpoint will may succeed even if the configuration is disabled which
> > may be surprising for users.
> 
> tough decision... but seems like clearing the buffer as soon as ep is
> disabled is the way to go.
> 
> --
> Balbi

I agree with Balbi, seems it is not easy to maintain the excess buffer... I was to
implement it at the beginning but I am not confident everything is done correctly.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web