| Message ID | bea9a9dcecbaef5a56e13adc1323a98cb22b0091.1341942485.git.stefan@herbrechtsmeier.net |
|---|---|
| State | Not Applicable |
| Delegated to: | Marek Vasut |
| Headers | show |
Dear Stefan Herbrechtsmeier, > The commit 5dd95cf93dfffa1d19a1928990852aac9f55b9d9 'usb_storage: > Fix EHCI "out of buffer pointers" with CD-ROM' introduce a bug in > usb_storage as it wrongly assumes that every transfer can use > 4096 bytes per qt_buffer. This is wrong if the start address of > the data is not page aligned to 4096 bytes and leads to 'EHCI > timed out on TD' messages because of 'out of buffer pointers' > in ehci_td_buffer function. > > The bug appears during load of a fragmented file and > read from or write to an unaligned memory address. > > Cc: Marek Vasut <marex@denx.de> > Signed-off-by: Stefan Herbrechtsmeier <stefan@herbrechtsmeier.net> This one is good :-) Thank you, Stefan! For your contribution and your patience. I'll test it on friday and I'll talk to WD to include it in current release. The fix looks very reasonable. Though I'd like to get some tests done. Tom, Ilya, can you possibly try this on your toys please? [...] Best regards, Marek Vasut
Hi, The patch is good in the sense it does fix the real problem. But I wonder if it's a good idea to expose lower layer details (like size/number of buffers per EHCI TD) to upper layer driver? I know EHCI is most common USB HCD but we have drivers for a bunch of others... How about them? Regards, Ilya.
Dear Ilya Yanok, > Hi, > > The patch is good in the sense it does fix the real problem. But I wonder > if it's a good idea to expose lower layer details (like size/number of > buffers per EHCI TD) to upper layer driver? I know EHCI is most common USB > HCD but we have drivers for a bunch of others... How about them? No, it's not ... it's yet another usb-is-full-of-crap-in-uboot kind of bugs :-( I'd like to queue this for the current release, we can work on proper rework for -next. Agreed? > Regards, Ilya. Best regards, Marek Vasut
Dear Marek, On Sun, Jul 15, 2012 at 12:14 PM, Marek Vasut <marex@denx.de> wrote: > > The patch is good in the sense it does fix the real problem. But I wonder > > if it's a good idea to expose lower layer details (like size/number of > > buffers per EHCI TD) to upper layer driver? I know EHCI is most common > USB > > HCD but we have drivers for a bunch of others... How about them? > > > No, it's not ... it's yet another usb-is-full-of-crap-in-uboot kind of > bugs :-( > > I'd like to queue this for the current release, we can work on proper > rework for > -next. Agreed? > Agreed, Regards, Ilya.
Dear Wolfgang Denk, > The commit 5dd95cf93dfffa1d19a1928990852aac9f55b9d9 'usb_storage: > Fix EHCI "out of buffer pointers" with CD-ROM' introduce a bug in > usb_storage as it wrongly assumes that every transfer can use > 4096 bytes per qt_buffer. This is wrong if the start address of > the data is not page aligned to 4096 bytes and leads to 'EHCI > timed out on TD' messages because of 'out of buffer pointers' > in ehci_td_buffer function. > > The bug appears during load of a fragmented file and > read from or write to an unaligned memory address. > > Cc: Marek Vasut <marex@denx.de> > Signed-off-by: Stefan Herbrechtsmeier <stefan@herbrechtsmeier.net> I'd like to get this patch: http://patchwork.ozlabs.org/patch/170256/ applied into current release. If you consider a pullRQ is better, I'll prepare one. Best regards, Marek Vasut
Dear Ilya Yanok, > Dear Marek, > > On Sun, Jul 15, 2012 at 12:14 PM, Marek Vasut <marex@denx.de> wrote: > > > The patch is good in the sense it does fix the real problem. But I > > > wonder if it's a good idea to expose lower layer details (like > > > size/number of buffers per EHCI TD) to upper layer driver? I know EHCI > > > is most common > > > > USB > > > > > HCD but we have drivers for a bunch of others... How about them? > > > > No, it's not ... it's yet another usb-is-full-of-crap-in-uboot kind of > > bugs :-( > > > > I'd like to queue this for the current release, we can work on proper > > rework for > > -next. Agreed? > > Agreed, Thanks! I'm marking this important in my mailbox to avoid missing that we need to fix this. Ilya, can you check the driver model papers and see if we can somehow integrate this into that? > Regards, Ilya. Best regards, Marek Vasut
Dear Marek, On Sun, Jul 15, 2012 at 1:51 PM, Marek Vasut <marex@denx.de> wrote: > > Thanks! I'm marking this important in my mailbox to avoid missing that we > need > to fix this. Ilya, can you check the driver model papers and see if we can > somehow integrate this into that? > > I will try to. Regards, Ilya.
Dear Stefan Herbrechtsmeier, > The commit 5dd95cf93dfffa1d19a1928990852aac9f55b9d9 'usb_storage: > Fix EHCI "out of buffer pointers" with CD-ROM' introduce a bug in > usb_storage as it wrongly assumes that every transfer can use > 4096 bytes per qt_buffer. This is wrong if the start address of > the data is not page aligned to 4096 bytes and leads to 'EHCI > timed out on TD' messages because of 'out of buffer pointers' > in ehci_td_buffer function. > > The bug appears during load of a fragmented file and > read from or write to an unaligned memory address. > > Cc: Marek Vasut <marex@denx.de> > Signed-off-by: Stefan Herbrechtsmeier <stefan@herbrechtsmeier.net> [...] I applied this and pushed into u-boot-usb. Thanks again for finding and fixing this. I'm still uncertain if we should add it into current release so late, probably not though. Therefore I'll queue it for next, ok? Best regards, Marek Vasut
Am 18.07.2012 14:50, schrieb Marek Vasut: >> The commit 5dd95cf93dfffa1d19a1928990852aac9f55b9d9 'usb_storage: >> Fix EHCI "out of buffer pointers" with CD-ROM' introduce a bug in >> usb_storage as it wrongly assumes that every transfer can use >> 4096 bytes per qt_buffer. This is wrong if the start address of >> the data is not page aligned to 4096 bytes and leads to 'EHCI >> timed out on TD' messages because of 'out of buffer pointers' >> in ehci_td_buffer function. >> >> The bug appears during load of a fragmented file and >> read from or write to an unaligned memory address. >> >> Cc: Marek Vasut <marex@denx.de> >> Signed-off-by: Stefan Herbrechtsmeier <stefan@herbrechtsmeier.net> > [...] > > I applied this and pushed into u-boot-usb. Thanks again for finding and fixing > this. > > I'm still uncertain if we should add it into current release so late, probably > not though. Therefore I'll queue it for next, ok? Given that there was nobody other than me with the problem until now, it should be okay. Regards, Stefan
diff --git a/common/usb_storage.c b/common/usb_storage.c index faad237..c528979 100644 --- a/common/usb_storage.c +++ b/common/usb_storage.c @@ -150,12 +150,18 @@ struct us_data { unsigned int irqpipe; /* pipe for release_irq */ unsigned char irqmaxp; /* max packed for irq Pipe */ unsigned char irqinterval; /* Intervall for IRQ Pipe */ - unsigned long max_xfer_blk; /* Max blocks per xfer */ ccb *srb; /* current srb */ trans_reset transport_reset; /* reset routine */ trans_cmnd transport; /* transport routine */ }; +/* + * The U-Boot EHCI driver cannot handle more than 5 page aligned buffers + * of 4096 bytes in a transfer without running itself out of qt_buffers + */ +#define USB_MAX_XFER_BLK(start, blksz) \ + (((4096 * 5) - ((start) % 4096)) / (blksz)) + static struct us_data usb_stor[USB_MAX_STOR_DEV]; @@ -1041,7 +1047,7 @@ static void usb_bin_fixup(struct usb_device_descriptor descriptor, unsigned long usb_stor_read(int device, unsigned long blknr, unsigned long blkcnt, void *buffer) { - unsigned long start, blks, buf_addr; + unsigned long start, blks, buf_addr, max_xfer_blk; unsigned short smallblks; struct usb_device *dev; struct us_data *ss; @@ -1083,12 +1089,14 @@ unsigned long usb_stor_read(int device, unsigned long blknr, /* XXX need some comment here */ retry = 2; srb->pdata = (unsigned char *)buf_addr; - if (blks > ss->max_xfer_blk) - smallblks = ss->max_xfer_blk; + max_xfer_blk = USB_MAX_XFER_BLK(buf_addr, + usb_dev_desc[device].blksz); + if (blks > max_xfer_blk) + smallblks = (unsigned short) max_xfer_blk; else smallblks = (unsigned short) blks; retry_it: - if (smallblks == ss->max_xfer_blk) + if (smallblks == max_xfer_blk) usb_show_progress(); srb->datalen = usb_dev_desc[device].blksz * smallblks; srb->pdata = (unsigned char *)buf_addr; @@ -1109,7 +1117,7 @@ retry_it: start, smallblks, buf_addr); usb_disable_asynch(0); /* asynch transfer allowed */ - if (blkcnt >= ss->max_xfer_blk) + if (blkcnt >= max_xfer_blk) debug("\n"); return blkcnt; } @@ -1117,7 +1125,7 @@ retry_it: unsigned long usb_stor_write(int device, unsigned long blknr, unsigned long blkcnt, const void *buffer) { - unsigned long start, blks, buf_addr; + unsigned long start, blks, buf_addr, max_xfer_blk; unsigned short smallblks; struct usb_device *dev; struct us_data *ss; @@ -1162,12 +1170,14 @@ unsigned long usb_stor_write(int device, unsigned long blknr, */ retry = 2; srb->pdata = (unsigned char *)buf_addr; - if (blks > ss->max_xfer_blk) - smallblks = ss->max_xfer_blk; + max_xfer_blk = USB_MAX_XFER_BLK(buf_addr, + usb_dev_desc[device].blksz); + if (blks > max_xfer_blk) + smallblks = (unsigned short) max_xfer_blk; else smallblks = (unsigned short) blks; retry_it: - if (smallblks == ss->max_xfer_blk) + if (smallblks == max_xfer_blk) usb_show_progress(); srb->datalen = usb_dev_desc[device].blksz * smallblks; srb->pdata = (unsigned char *)buf_addr; @@ -1188,7 +1198,7 @@ retry_it: start, smallblks, buf_addr); usb_disable_asynch(0); /* asynch transfer allowed */ - if (blkcnt >= ss->max_xfer_blk) + if (blkcnt >= max_xfer_blk) debug("\n"); return blkcnt; @@ -1415,12 +1425,6 @@ int usb_stor_get_info(struct usb_device *dev, struct us_data *ss, USB_STOR_PRINTF(" address %d\n", dev_desc->target); USB_STOR_PRINTF("partype: %d\n", dev_desc->part_type); - /* - * The U-Boot EHCI driver cannot handle more than 4096 * 5 bytes in a - * transfer without running itself out of qt_buffers. - */ - ss->max_xfer_blk = (4096 * 5) / dev_desc->blksz; - init_part(dev_desc); USB_STOR_PRINTF("partype: %d\n", dev_desc->part_type);
The commit 5dd95cf93dfffa1d19a1928990852aac9f55b9d9 'usb_storage: Fix EHCI "out of buffer pointers" with CD-ROM' introduce a bug in usb_storage as it wrongly assumes that every transfer can use 4096 bytes per qt_buffer. This is wrong if the start address of the data is not page aligned to 4096 bytes and leads to 'EHCI timed out on TD' messages because of 'out of buffer pointers' in ehci_td_buffer function. The bug appears during load of a fragmented file and read from or write to an unaligned memory address. Cc: Marek Vasut <marex@denx.de> Signed-off-by: Stefan Herbrechtsmeier <stefan@herbrechtsmeier.net> --- Changes for v2: - Replace fixed worst case calculation with dynamic computation based on start address of transfer Changes for v3: - Add parentheses within macro around parameters common/usb_storage.c | 38 +++++++++++++++++++++----------------- 1 file changed, 21 insertions(+), 17 deletions(-)