usb: ffs: fix regression when quirk_ep_out_aligned_size flag is set
Subsystems (2)
PATCH 1/1 usb: ffs: fix regression when quirk_ep_out_aligned_size flag is set
The commit '2e4c7553cd usb: gadget: f_fs: add aio support' broke the quirk implemented to align buffer size to maxpacketsize on out endpoint. As result, functionfs does not work on Intel platforms using dwc3 driver (i.e. Bay Trail and Merrifield). This patch fixes the issue.
This code is based on a previous Qiuxu's patch.
---
Hi,
Since this is a feature that worked in past, this patch is meant for 3.17 kernel. If/when we pass review and accept it on 3.17, I'll send a version for stable 3.16 kernel too. It is not required for 3.14, since the regression came later.
Br, David Cohen
--- drivers/usb/gadget/function/f_fs.c | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-)
drivers/usb/gadget/function/f_fs.c +16 -5
@@ -648,15 +648,26 @@ static void ffs_user_copy_worker(struct work_struct *work)if (io_data->read && ret >0 ) {int i;size_t pos =0 ; + +/* +* Since req->length may be bigger than io_data->len (after +* being rounded up to maxpacketsize), we may end up with more +* data then user space has space for. +*/ + ret =min_t (int, ret, io_data->len); +use_mm (io_data->mm);for (i =0 ; i < io_data->nr_segs; i++) { +size_t len =min_t (size_t, ret - pos, + io_data->iovec[i].iov_len); +if (!len) +break ;if (unlikely (copy_to_user (io_data->iovec[i].iov_base, - &io_data->buf[pos], - io_data->iovec[i].iov_len))) { + &io_data->buf[pos], len))) { ret = -EFAULT ;break ; } - pos += io_data->iovec[i].iov_len; + pos += len; }unuse_mm (io_data->mm); }
@@ -794,7 +805,7 @@ static ssize_t ffs_epfile_io(struct file *file, struct ffs_io_data *io_data) goto error_lock; req->buf = data; - req->length = io_data->len; + req->length = data_len; io_data->buf = data; io_data->ep = ep->ep;
@@ -816,7 +827,7 @@ static ssize_t ffs_epfile_io(struct file *file, struct ffs_io_data *io_data) req = ep->req; req->buf = data; - req->length = io_data->len; + req->length = data_len; req->context = &done; req->complete = ffs_epfile_io_complete;
Signature · 1 line
2.1.0
Hi,
Quoted 1 line · David Cohen
The commit '
2e4c7553cdusb: gadget:f_fs: add aio support' broke the
did that commit non-aio or is only aio broken ?
Quoted 3 lines
quirk implemented to align buffer size to maxpacketsize on out endpoint. As result, functionfs does not work on Intel platforms using dwc3 driver (i.e. Bay Trail and Merrifield). This patch fixes the issue.
and Braswell.
Quoted 1 line
This code is based on a previous Qiuxu's patch.
How have you tested this ? Do you have a test-case to share ? Then I can add it to my tests which I have been running on my platforms too (they include USB20CV and USB30CV where applicable - quite a few fixes coming soon).
cheers
Signature · 1 line
balbi
Quoted 1 line · Felipe Balbi
Hi,
Hi Felipe,
Quoted 5 lines · David Cohen
Quoted 1 line
The commit '
2e4c7553cdusb: gadget:f_fs: add aio support' broke thedid that commit non-aio or is only aio broken ?
That commit broke the quirk on both cases.
Quoted 5 lines
Quoted 4 lines
quirk implemented to align buffer size to maxpacketsize on out endpoint. As result, functionfs does not work on Intel platforms using dwc3 driver (i.e. Bay Trail and Merrifield). This patch fixes the issue.
Quoted 8 lines
and Braswell.
Quoted 1 line
This code is based on a previous Qiuxu's patch.
How have you tested this ? Do you have a test-case to share ? Then I can add it to my tests which I have been running on my platforms too (they include USB20CV and USB30CV where applicable - quite a few fixes coming soon).
My testcase is to use Android's adbd service via function_fs (no out-of-tree android gadget). Without the quitk, it doesn't work.
Br, David
Quoted 5 lines
cheers
Signature · 1 line
balbi
Hi again,
Quoted 7 lines · David Cohen
The commit '
2e4c7553cdusb: gadget:f_fs: add aio support' broke the quirk implemented to align buffer size to maxpacketsize on out endpoint. As result, functionfs does not work on Intel platforms using dwc3 driver (i.e. Bay Trail and Merrifield). This patch fixes the issue.This code is based on a previous Qiuxu's patch.
btw, please resend and add below right here:
Quoted 2 lines
Signed-off-by David Cohen <[email protected]>Signed-off-by Qiuxu Zhuo <[email protected]>
Signature · 1 line
balbi
Quoted 8 lines · Felipe Balbi
Hi again,
Quoted 5 lines · David Cohen
The commit '
2e4c7553cdusb: gadget:f_fs: add aio support' broke the quirk implemented to align buffer size to maxpacketsize on out endpoint. As result, functionfs does not work on Intel platforms using dwc3 driver (i.e. Bay Trail and Merrifield). This patch fixes the issue.
Quoted 7 lines
Quoted 2 lines
This code is based on a previous Qiuxu's patch.
btw, please resend and add below right here:
Fixes 2e4c7553cd (usb: gadget: f_fs: add aio support)Cc <[email protected]> # v3.16+
Sure. But v3.16 doesn't have the new 'function' directory under usb/gadget. Not sure if same patch is useful for v3.16+.
The patch I sent is intended for v3.17.1 and v3.18-rc1.
Br, David
Quoted 6 lines
Quoted 3 lines
Signed-off-by David Cohen <[email protected]>Signed-off-by Qiuxu Zhuo <[email protected]>Signature · 1 line
balbi
Hi,
Quoted 12 lines · David Cohen
Quoted 11 lines · Felipe Balbi
Hi again,
Quoted 7 lines · David Cohen
The commit '
2e4c7553cdusb: gadget:f_fs: add aio support' broke the quirk implemented to align buffer size to maxpacketsize on out endpoint. As result, functionfs does not work on Intel platforms using dwc3 driver (i.e. Bay Trail and Merrifield). This patch fixes the issue.This code is based on a previous Qiuxu's patch.
Quoted 8 lines
Quoted 4 lines
btw, please resend and add below right here:
Fixes 2e4c7553cd (usb: gadget: f_fs: add aio support)Cc <[email protected]> # v3.16+Sure. But v3.16 doesn't have the new 'function' directory under usb/gadget. Not sure if same patch is useful for v3.16+.
The patch I sent is intended for v3.17.1 and v3.18-rc1.
code is the same anyway right ? It's just a path change :-)
Signature · 1 line
balbi
Quoted 9 lines · David Cohen
The commit '
2e4c7553cdusb: gadget:f_fs: add aio support' broke the quirk implemented to align buffer size to maxpacketsize on out endpoint. As result, functionfs does not work on Intel platforms using dwc3 driver (i.e. Bay Trail and Merrifield). This patch fixes the issue.This code is based on a previous Qiuxu's patch.
Signed-off-by David Cohen <[email protected]>Signed-off-by Qiuxu Zhuo <[email protected]>
Quoted 70 lines
---
Hi,
Since this is a feature that worked in past, this patch is meant for 3.17 kernel. If/when we pass review and accept it on 3.17, I'll send a version for stable 3.16 kernel too. It is not required for 3.14, since the regression came later.
Br, David Cohen
--- drivers/usb/gadget/function/f_fs.c | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-)drivers/usb/gadget/function/f_fs.c +16
-5@@ -648,15 +648,26 @@ static void ffs_user_copy_worker(struct work_struct *work)if (io_data->read && ret >0 ) {int i;size_t pos =0 ; + +/* +* Since req->length may be bigger than io_data->len (after +* being rounded up to maxpacketsize), we may end up with more +* data then user space has space for. +*/ + ret =min_t (int, ret, io_data->len); +use_mm (io_data->mm);for (i =0 ; i < io_data->nr_segs; i++) { +size_t len =min_t (size_t, ret - pos, + io_data->iovec[i].iov_len); +if (!len) +break ;if (unlikely (copy_to_user (io_data->iovec[i].iov_base, - &io_data->buf[pos], - io_data->iovec[i].iov_len))) { + &io_data->buf[pos], len))) { ret = -EFAULT ;break ; } - pos += io_data->iovec[i].iov_len; + pos += len; }unuse_mm (io_data->mm); }@@ -794,7 +805,7 @@ static ssize_t ffs_epfile_io(struct file *file, struct ffs_io_data *io_data) goto error_lock; req->buf = data; - req->length = io_data->len; + req->length = data_len; io_data->buf = data; io_data->ep = ep->ep;@@ -816,7 +827,7 @@ static ssize_t ffs_epfile_io(struct file *file, struct ffs_io_data *io_data) req = ep->req; req->buf = data; - req->length = io_data->len; + req->length = data_len; req->context = &done; req->complete = ffs_epfile_io_complete;Signature · 1 line
2.1.0
Signature · 4 lines
Best regards, _ _ .o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o ..o | Computer Science, Michał “mina86” Nazarewicz (o o) ooo +--<[email protected]>--<xmpp:[email protected]>--ooO--(_)--Ooo--
PATCH v2 1/1 usb: ffs: fix regression when quirk_ep_out_aligned_size flag is set
In reply to David CohenThe commit '2e4c7553cd usb: gadget: f_fs: add aio support' broke the quirk implemented to align buffer size to maxpacketsize on out endpoint. As result, functionfs does not work on Intel platforms using dwc3 driver (i.e. Bay Trail and Merrifield). This patch fixes the issue.
This code is based on a previous Qiuxu's patch.
---
Hi,
Since this is a feature that worked in past, this is meant for stable versions >= 3.16 too.
v1 to v2: just added Fixes, Cc and Acked-by lines on patch description.
Br, David Cohen
--- drivers/usb/gadget/function/f_fs.c | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-)
drivers/usb/gadget/function/f_fs.c +16 -5
@@ -648,15 +648,26 @@ static void ffs_user_copy_worker(struct work_struct *work)if (io_data->read && ret >0 ) {int i;size_t pos =0 ; + +/* +* Since req->length may be bigger than io_data->len (after +* being rounded up to maxpacketsize), we may end up with more +* data then user space has space for. +*/ + ret =min_t (int, ret, io_data->len); +use_mm (io_data->mm);for (i =0 ; i < io_data->nr_segs; i++) { +size_t len =min_t (size_t, ret - pos, + io_data->iovec[i].iov_len); +if (!len) +break ;if (unlikely (copy_to_user (io_data->iovec[i].iov_base, - &io_data->buf[pos], - io_data->iovec[i].iov_len))) { + &io_data->buf[pos], len))) { ret = -EFAULT ;break ; } - pos += io_data->iovec[i].iov_len; + pos += len; }unuse_mm (io_data->mm); }
@@ -794,7 +805,7 @@ static ssize_t ffs_epfile_io(struct file *file, struct ffs_io_data *io_data) goto error_lock; req->buf = data; - req->length = io_data->len; + req->length = data_len; io_data->buf = data; io_data->ep = ep->ep;
@@ -816,7 +827,7 @@ static ssize_t ffs_epfile_io(struct file *file, struct ffs_io_data *io_data) req = ep->req; req->buf = data; - req->length = io_data->len; + req->length = data_len; req->context = &done; req->complete = ffs_epfile_io_complete;
Signature · 1 line
2.1.0
Quoted 5 lines · David Cohen
drivers/usb/gadget/function/f_fs.c +3
-0use_mm (io_data->mm);for (i =0 ; i < io_data->nr_segs; i++) { +size_t len =min_t (size_t, ret - pos, + io_data->iovec[i].iov_len); +if (!len)
drivers/usb/gadget/function/f_fs.c static void ffs_user_copy_worker(struct work_struct *work) · line 670
Quoted 10 lines
drivers/usb/gadget/function/f_fs.c +3
-3+break ;if (unlikely (copy_to_user (io_data->iovec[i].iov_base, - &io_data->buf[pos], - io_data->iovec[i].iov_len))) { + &io_data->buf[pos], len))) { ret = -EFAULT ;break ; } - pos += io_data->iovec[i].iov_len; + pos += len;
Hmm... This is really asking for something like
if (copy_to_iter(io_data->buf, ret, <something>) != ret) ret = -EFAULT;
with <something> being an iov_iter instead of iovec. It would be really nice to have that thing switched to ->read_iter/->write_iter, dropping ->read/->write/->aio_read/->aio_write; I'm not familiar enough with that code to do it on my own, though, so it would require some help from maintainers...
Quoted 18 lines · Al Viro
Quoted 5 lines · David Cohen
drivers/usb/gadget/function/f_fs.c +3
-0use_mm (io_data->mm);for (i =0 ; i < io_data->nr_segs; i++) { +size_t len =min_t (size_t, ret - pos, + io_data->iovec[i].iov_len); +if (!len)drivers/usb/gadget/function/f_fs.c static void ffs_user_copy_worker(struct work_struct *work) · line 670
Quoted 10 lines
drivers/usb/gadget/function/f_fs.c +3
-3+break ;if (unlikely (copy_to_user (io_data->iovec[i].iov_base, - &io_data->buf[pos], - io_data->iovec[i].iov_len))) { + &io_data->buf[pos], len))) { ret = -EFAULT ;break ; } - pos += io_data->iovec[i].iov_len; + pos += len;Hmm... This is really asking for something like
Quoted 7 lines
if (copy_to_iter(io_data->buf, ret, <something>) != ret) ret = -EFAULT;with <something> being an
iov_iterinstead of iovec. It would be really nice to have that thing switched to ->read_iter/->write_iter, dropping ->read/->write/->aio_read/->aio_write; I'm not familiar enough with that code to do it on my own, though, so it would require some help from maintainers...
cool, Michal, if you're too busy lately, I can look at that one myself. I suppose the test application we have under tool/usb is good enough for validation ?
cheers
Signature · 1 line
balbi
Quoted 12 lines · David Cohen
The commit '
2e4c7553cdusb: gadget:f_fs: add aio support' broke the quirk implemented to align buffer size to maxpacketsize on out endpoint. As result, functionfs does not work on Intel platforms using dwc3 driver (i.e. Bay Trail and Merrifield). This patch fixes the issue.This code is based on a previous Qiuxu's patch.
Fixes 2e4c7553cd (usb: gadget: f_fs: add aio support)Cc <[email protected]> # v3.16+Signed-off-by David Cohen <[email protected]>Signed-off-by Qiuxu Zhuo <[email protected]>
Quoted 8 lines
---
Hi,
Since this is a feature that worked in past, this is meant for stable versions >= 3.16 too.
v1 to v2: just added Fixes, Cc and Acked-by lines on patch description.
this adds a build warning for use of maybe unitialized data_len. Plese fix.
Signature · 1 line
balbi
Quoted 17 lines · Felipe Balbi
Quoted 16 lines · David Cohen
The commit '
2e4c7553cdusb: gadget:f_fs: add aio support' broke the quirk implemented to align buffer size to maxpacketsize on out endpoint. As result, functionfs does not work on Intel platforms using dwc3 driver (i.e. Bay Trail and Merrifield). This patch fixes the issue.This code is based on a previous Qiuxu's patch.
Fixes 2e4c7553cd (usb: gadget: f_fs: add aio support)Cc <[email protected]> # v3.16+Signed-off-by David Cohen <[email protected]>Signed-off-by Qiuxu Zhuo <[email protected]>---
Hi,
Quoted 7 lines
Quoted 4 lines
Since this is a feature that worked in past, this is meant for stable versions >= 3.16 too.
v1 to v2: just added Fixes, Cc and Acked-by lines on patch description.
this adds a build warning for use of maybe unitialized
data_len. Plese fix.
It's a false-positive warning. data_len is only initialized if halt != 0 and it's only used if halt != 0 too.
Do you prefer to initialize it to 0 during the declaration to silent the compiler?
BR, David
Quoted 3 lines
Signature · 2 lines
-- balbi
Quoted 23 lines · David Cohen
Quoted 22 lines · Felipe Balbi
Quoted 20 lines · David Cohen
The commit '
2e4c7553cdusb: gadget:f_fs: add aio support' broke the quirk implemented to align buffer size to maxpacketsize on out endpoint. As result, functionfs does not work on Intel platforms using dwc3 driver (i.e. Bay Trail and Merrifield). This patch fixes the issue.This code is based on a previous Qiuxu's patch.
Fixes 2e4c7553cd (usb: gadget: f_fs: add aio support)Cc <[email protected]> # v3.16+Signed-off-by David Cohen <[email protected]>Signed-off-by Qiuxu Zhuo <[email protected]>---
Hi,
Since this is a feature that worked in past, this is meant for stable versions >= 3.16 too.
v1 to v2: just added Fixes, Cc and Acked-by lines on patch description.
Quoted 8 lines
Quoted 2 lines
this adds a build warning for use of maybe unitialized
data_len. Plese fix.It's a false-positive warning.
data_lenis only initialized if halt != 0 and it's only used if halt != 0 too.Do you prefer to initialize it to 0 during the declaration to silent the compiler?
yup.
Signature · 1 line
balbi
PATCH v3 1/1 usb: ffs: fix regression when quirk_ep_out_aligned_size flag is set
In reply to David CohenThe commit '2e4c7553cd usb: gadget: f_fs: add aio support' broke the quirk implemented to align buffer size to maxpacketsize on out endpoint. As result, functionfs does not work on Intel platforms using dwc3 driver (i.e. Bay Trail and Merrifield). This patch fixes the issue.
This code is based on a previous Qiuxu's patch.
---
Hi,
Since this is a feature that worked in past, this is meant for stable versions >= 3.16 too.
v1 to v2: just added Fixes, Cc and Acked-by lines on patch description. v2 to v3: fixed compiler warning about data_len being used unitialized
Br, David Cohen
--- drivers/usb/gadget/function/f_fs.c | 40 ++++++++++++++++++++++++++++++++------ 1 file changed, 34 insertions(+), 6 deletions(-)
drivers/usb/gadget/function/f_fs.c +34 -6
@@ -648,15 +648,26 @@ static void ffs_user_copy_worker(struct work_struct *work)if (io_data->read && ret >0 ) {int i;size_t pos =0 ; + +/* +* Since req->length may be bigger than io_data->len (after +* being rounded up to maxpacketsize), we may end up with more +* data then user space has space for. +*/ + ret =min_t (int, ret, io_data->len); +use_mm (io_data->mm);for (i =0 ; i < io_data->nr_segs; i++) { +size_t len =min_t (size_t, ret - pos, + io_data->iovec[i].iov_len); +if (!len) +break ;if (unlikely (copy_to_user (io_data->iovec[i].iov_base, - &io_data->buf[pos], - io_data->iovec[i].iov_len))) { + &io_data->buf[pos], len))) { ret = -EFAULT ;break ; } - pos += io_data->iovec[i].iov_len; + pos += len; }unuse_mm (io_data->mm); }
@@ -688,7 +699,7 @@ static ssize_t ffs_epfile_io(struct file *file, struct ffs_io_data *io_data)struct ffs_epfile *epfile = file->private_data;struct ffs_ep *ep;char *data =NULL ; -ssize_t ret, data_len; +ssize_t ret, data_len = -EINVAL ;int halt;/* Are we still active? */
@@ -788,13 +799,30 @@ static ssize_t ffs_epfile_io(struct file *file, struct ffs_io_data *io_data)/* Fire the request */ struct usb_request *req; +/* +* Sanity Check: even though data_len can't be used +* uninitialized at the time I write this comment, some +* compilers complain about this situation. +* In order to keep the code clean from warnings, data_len is +* being initialized to -EINVAL during its declaration, which +* means we can't rely on compiler anymore to warn no future +* changes won't result in data_len being used uninitialized. +* For such reason, we're adding this redundant sanity check +* here. +*/ +if (unlikely (data_len == -EINVAL )) { +WARN (1 ,"%s: data_len == -EINVAL\n" , __func__); + ret = -EINVAL ; + goto error_lock; + } +if (io_data->aio) { req =usb_ep_alloc_request (ep->ep,GFP_KERNEL );if (unlikely (!req)) goto error_lock; req->buf = data; - req->length = io_data->len; + req->length = data_len; io_data->buf = data; io_data->ep = ep->ep;
@@ -816,7 +844,7 @@ static ssize_t ffs_epfile_io(struct file *file, struct ffs_io_data *io_data) req = ep->req; req->buf = data; - req->length = io_data->len; + req->length = data_len; req->context = &done; req->complete = ffs_epfile_io_complete;
Signature · 1 line
2.1.0