| Message ID | 1330525309-21333-1-git-send-email-puneets@nvidia.com |
|---|---|
| State | Superseded |
| Delegated to: | Marek Vasut |
| Headers | show |
> As DMA expects the buffers to be equal and larger then > cache lines, This aligns buffers at cacheline. > > Signed-off-by: Puneet Saxena <puneets@nvidia.com> > --- > > Changes for V2: > - Use "ARCH_DMA_MINALIGN" directly > - Use "ALIGN" to align size as cacheline > - Removed headers from usb.h > - Send 8 bytes of device descriptor size to read > Max packet size > scsi.h header is needed to avoid extra memcpy from local buffer > to global buffer. > > Changes for V3: > - Removed local descriptor elements copy to global descriptor elements > - Removed "Signed-off-by: Jim Lin <jilin@nvidia.com>" from commit > message > > common/cmd_usb.c | 3 +- > common/usb.c | 57 > ++++++++++++++++++++++------------------- common/usb_storage.c | > 59 ++++++++++++++++++++---------------------- disk/part_dos.c > | 2 +- > drivers/usb/host/ehci-hcd.c | 8 ++++++ > include/scsi.h | 4 ++- > 6 files changed, 73 insertions(+), 60 deletions(-) > > diff --git a/common/cmd_usb.c b/common/cmd_usb.c > index 320667f..bca9d94 100644 > --- a/common/cmd_usb.c > +++ b/common/cmd_usb.c > @@ -150,7 +150,8 @@ void usb_display_class_sub(unsigned char dclass, > unsigned char subclass, > > void usb_display_string(struct usb_device *dev, int index) > { > - char buffer[256]; > + ALLOC_CACHE_ALIGN_BUFFER(char, buffer, 256); > + > if (index != 0) { > if (usb_string(dev, index, &buffer[0], 256) > 0) > printf("String: \"%s\"", buffer); > diff --git a/common/usb.c b/common/usb.c > index 63a11c8..191bc5b 100644 > --- a/common/usb.c > +++ b/common/usb.c > @@ -73,7 +73,6 @@ static struct usb_device usb_dev[USB_MAX_DEVICE]; > static int dev_index; > static int running; > static int asynch_allowed; > -static struct devrequest setup_packet; > > char usb_started; /* flag for the started/stopped USB status */ > > @@ -185,23 +184,25 @@ int usb_control_msg(struct usb_device *dev, unsigned > int pipe, unsigned short value, unsigned short index, > void *data, unsigned short size, int timeout) > { > + ALLOC_CACHE_ALIGN_BUFFER(struct devrequest, setup_packet, > + sizeof(struct devrequest)); > if ((timeout == 0) && (!asynch_allowed)) { > /* request for a asynch control pipe is not allowed */ > return -1; > } > > /* set setup command */ > - setup_packet.requesttype = requesttype; > - setup_packet.request = request; > - setup_packet.value = cpu_to_le16(value); > - setup_packet.index = cpu_to_le16(index); > - setup_packet.length = cpu_to_le16(size); > + setup_packet->requesttype = requesttype; > + setup_packet->request = request; > + setup_packet->value = cpu_to_le16(value); > + setup_packet->index = cpu_to_le16(index); > + setup_packet->length = cpu_to_le16(size); > USB_PRINTF("usb_control_msg: request: 0x%X, requesttype: 0x%X, " \ > "value 0x%X index 0x%X length 0x%X\n", > request, requesttype, value, index, size); > dev->status = USB_ST_NOT_PROC; /*not yet processed */ > > - submit_control_msg(dev, pipe, data, size, &setup_packet); > + submit_control_msg(dev, pipe, data, size, setup_packet); > if (timeout == 0) > return (int)size; > > @@ -694,7 +695,7 @@ static int usb_string_sub(struct usb_device *dev, > unsigned int langid, */ > int usb_string(struct usb_device *dev, int index, char *buf, size_t size) > { > - unsigned char mybuf[USB_BUFSIZ]; > + ALLOC_CACHE_ALIGN_BUFFER(unsigned char, mybuf, USB_BUFSIZ); > unsigned char *tbuf; > int err; > unsigned int u, idx; > @@ -794,7 +795,7 @@ int usb_new_device(struct usb_device *dev) > { > int addr, err; > int tmp; > - unsigned char tmpbuf[USB_BUFSIZ]; > + ALLOC_CACHE_ALIGN_BUFFER(unsigned char, tmpbuf, USB_BUFSIZ); > > /* We still haven't set the Address yet */ > addr = dev->devnum; > @@ -842,7 +843,10 @@ int usb_new_device(struct usb_device *dev) > dev->epmaxpacketin[0] = 64; > dev->epmaxpacketout[0] = 64; > > - err = usb_get_descriptor(dev, USB_DT_DEVICE, 0, desc, 64); > + desc->bMaxPacketSize0 = 0; > + /*8 bytes of the descriptor to read Max packet size*/ > + err = usb_get_descriptor(dev, USB_DT_DEVICE, 0, desc, > + 8); Did some unrelated addition/change creep in here? > if (err < 0) { > USB_PRINTF("usb_new_device: usb_get_descriptor() failed\n"); > return 1; > @@ -905,7 +909,7 @@ int usb_new_device(struct usb_device *dev) > tmp = sizeof(dev->descriptor); > > err = usb_get_descriptor(dev, USB_DT_DEVICE, 0, > - &dev->descriptor, sizeof(dev->descriptor)); > + desc, sizeof(dev->descriptor)); Won't this change (desc) break anything? > if (err < tmp) { > if (err < 0) > printf("unable to get device descriptor (error=%d)\n", The rest seems fine, from now on it seems to be only matter of trivial fix. Thanks for your effort so far! M
On Thursday 01 March 2012 03:05 AM, Marek Vasut wrote: >> As DMA expects the buffers to be equal and larger then >> cache lines, This aligns buffers at cacheline. >> >> Signed-off-by: Puneet Saxena<puneets@nvidia.com> >> --- >> >> Changes for V2: >> - Use "ARCH_DMA_MINALIGN" directly >> - Use "ALIGN" to align size as cacheline >> - Removed headers from usb.h >> - Send 8 bytes of device descriptor size to read >> Max packet size >> scsi.h header is needed to avoid extra memcpy from local buffer >> to global buffer. >> >> Changes for V3: >> - Removed local descriptor elements copy to global descriptor elements >> - Removed "Signed-off-by: Jim Lin<jilin@nvidia.com>" from commit >> message >> >> common/cmd_usb.c | 3 +- >> common/usb.c | 57 >> ++++++++++++++++++++++------------------- common/usb_storage.c | >> 59 ++++++++++++++++++++---------------------- disk/part_dos.c >> | 2 +- >> drivers/usb/host/ehci-hcd.c | 8 ++++++ >> include/scsi.h | 4 ++- >> 6 files changed, 73 insertions(+), 60 deletions(-) >> >> diff --git a/common/cmd_usb.c b/common/cmd_usb.c >> index 320667f..bca9d94 100644 >> --- a/common/cmd_usb.c >> +++ b/common/cmd_usb.c >> @@ -150,7 +150,8 @@ void usb_display_class_sub(unsigned char dclass, >> unsigned char subclass, >> >> void usb_display_string(struct usb_device *dev, int index) >> { >> - char buffer[256]; >> + ALLOC_CACHE_ALIGN_BUFFER(char, buffer, 256); >> + >> if (index != 0) { >> if (usb_string(dev, index,&buffer[0], 256)> 0) >> printf("String: \"%s\"", buffer); >> diff --git a/common/usb.c b/common/usb.c >> index 63a11c8..191bc5b 100644 >> --- a/common/usb.c >> +++ b/common/usb.c >> @@ -73,7 +73,6 @@ static struct usb_device usb_dev[USB_MAX_DEVICE]; >> static int dev_index; >> static int running; >> static int asynch_allowed; >> -static struct devrequest setup_packet; >> >> char usb_started; /* flag for the started/stopped USB status */ >> >> @@ -185,23 +184,25 @@ int usb_control_msg(struct usb_device *dev, unsigned >> int pipe, unsigned short value, unsigned short index, >> void *data, unsigned short size, int timeout) >> { >> + ALLOC_CACHE_ALIGN_BUFFER(struct devrequest, setup_packet, >> + sizeof(struct devrequest)); >> if ((timeout == 0)&& (!asynch_allowed)) { >> /* request for a asynch control pipe is not allowed */ >> return -1; >> } >> >> /* set setup command */ >> - setup_packet.requesttype = requesttype; >> - setup_packet.request = request; >> - setup_packet.value = cpu_to_le16(value); >> - setup_packet.index = cpu_to_le16(index); >> - setup_packet.length = cpu_to_le16(size); >> + setup_packet->requesttype = requesttype; >> + setup_packet->request = request; >> + setup_packet->value = cpu_to_le16(value); >> + setup_packet->index = cpu_to_le16(index); >> + setup_packet->length = cpu_to_le16(size); >> USB_PRINTF("usb_control_msg: request: 0x%X, requesttype: 0x%X, " \ >> "value 0x%X index 0x%X length 0x%X\n", >> request, requesttype, value, index, size); >> dev->status = USB_ST_NOT_PROC; /*not yet processed */ >> >> - submit_control_msg(dev, pipe, data, size,&setup_packet); >> + submit_control_msg(dev, pipe, data, size, setup_packet); >> if (timeout == 0) >> return (int)size; >> >> @@ -694,7 +695,7 @@ static int usb_string_sub(struct usb_device *dev, >> unsigned int langid, */ >> int usb_string(struct usb_device *dev, int index, char *buf, size_t size) >> { >> - unsigned char mybuf[USB_BUFSIZ]; >> + ALLOC_CACHE_ALIGN_BUFFER(unsigned char, mybuf, USB_BUFSIZ); >> unsigned char *tbuf; >> int err; >> unsigned int u, idx; >> @@ -794,7 +795,7 @@ int usb_new_device(struct usb_device *dev) >> { >> int addr, err; >> int tmp; >> - unsigned char tmpbuf[USB_BUFSIZ]; >> + ALLOC_CACHE_ALIGN_BUFFER(unsigned char, tmpbuf, USB_BUFSIZ); >> >> /* We still haven't set the Address yet */ >> addr = dev->devnum; >> @@ -842,7 +843,10 @@ int usb_new_device(struct usb_device *dev) >> dev->epmaxpacketin[0] = 64; >> dev->epmaxpacketout[0] = 64; >> >> - err = usb_get_descriptor(dev, USB_DT_DEVICE, 0, desc, 64); >> + desc->bMaxPacketSize0 = 0; >> + /*8 bytes of the descriptor to read Max packet size*/ >> + err = usb_get_descriptor(dev, USB_DT_DEVICE, 0, desc, >> + 8); > Did some unrelated addition/change creep in here? No, This is the fix for the similar issue as is discussed for string fetch(). When the device partially fills the passed buffer and we try to invalidate the partial buffer the cache alignment error crops up. The code in "ehci_submit_async() " invalidates *partially* the passed buffer though we pass aligned buffer after "STD_ASS" is received. Actually it should invalidate only the cached line which is equal(~32) to device desc length. If we pass actual device desc length the cache alignment error does not spew. Note that here we are aligning the buffer still the error comes. >> if (err< 0) { >> USB_PRINTF("usb_new_device: usb_get_descriptor() failed\n"); >> return 1; >> @@ -905,7 +909,7 @@ int usb_new_device(struct usb_device *dev) >> tmp = sizeof(dev->descriptor); >> >> err = usb_get_descriptor(dev, USB_DT_DEVICE, 0, >> - &dev->descriptor, sizeof(dev->descriptor)); >> + desc, sizeof(dev->descriptor)); > Won't this change (desc) break anything? Its not breaking any thing. For safer side we could add memcpy to copy from local desc to global desc. What you say? >> if (err< tmp) { >> if (err< 0) >> printf("unable to get device descriptor (error=%d)\n", > The rest seems fine, from now on it seems to be only matter of trivial fix. > Thanks for your effort so far! > > M If rest of the code is fine in [Patch V3 1/2] except these two issue can it be acknowledged for up-streaming? Thanks, Puneet ----------------------------------------------------------------------------------- This email message is for the sole use of the intended recipient(s) and may contain confidential information. Any unauthorized review, use, disclosure or distribution is prohibited. If you are not the intended recipient, please contact the sender by reply email and destroy all copies of the original message. -----------------------------------------------------------------------------------
> On Thursday 01 March 2012 03:05 AM, Marek Vasut wrote: > >> As DMA expects the buffers to be equal and larger then > >> cache lines, This aligns buffers at cacheline. > >> > >> Signed-off-by: Puneet Saxena<puneets@nvidia.com> > >> --- > >> > >> Changes for V2: > >> - Use "ARCH_DMA_MINALIGN" directly > >> - Use "ALIGN" to align size as cacheline > >> - Removed headers from usb.h > >> - Send 8 bytes of device descriptor size to read > >> > >> Max packet size > >> > >> scsi.h header is needed to avoid extra memcpy from local buffer > >> to global buffer. > >> > >> Changes for V3: > >> - Removed local descriptor elements copy to global descriptor > >> elements - Removed "Signed-off-by: Jim Lin<jilin@nvidia.com>" from > >> commit > >> > >> message > >> > >> common/cmd_usb.c | 3 +- > >> common/usb.c | 57 > >> > >> ++++++++++++++++++++++------------------- common/usb_storage.c | > >> 59 ++++++++++++++++++++---------------------- disk/part_dos.c > >> > >> | 2 +- > >> > >> drivers/usb/host/ehci-hcd.c | 8 ++++++ > >> include/scsi.h | 4 ++- > >> 6 files changed, 73 insertions(+), 60 deletions(-) > >> > >> diff --git a/common/cmd_usb.c b/common/cmd_usb.c > >> index 320667f..bca9d94 100644 > >> --- a/common/cmd_usb.c > >> +++ b/common/cmd_usb.c > >> @@ -150,7 +150,8 @@ void usb_display_class_sub(unsigned char dclass, > >> unsigned char subclass, > >> > >> void usb_display_string(struct usb_device *dev, int index) > >> { > >> > >> - char buffer[256]; > >> + ALLOC_CACHE_ALIGN_BUFFER(char, buffer, 256); > >> + > >> > >> if (index != 0) { > >> > >> if (usb_string(dev, index,&buffer[0], 256)> 0) > >> > >> printf("String: \"%s\"", buffer); > >> > >> diff --git a/common/usb.c b/common/usb.c > >> index 63a11c8..191bc5b 100644 > >> --- a/common/usb.c > >> +++ b/common/usb.c > >> @@ -73,7 +73,6 @@ static struct usb_device usb_dev[USB_MAX_DEVICE]; > >> > >> static int dev_index; > >> static int running; > >> static int asynch_allowed; > >> > >> -static struct devrequest setup_packet; > >> > >> char usb_started; /* flag for the started/stopped USB status */ > >> > >> @@ -185,23 +184,25 @@ int usb_control_msg(struct usb_device *dev, > >> unsigned int pipe, unsigned short value, unsigned short index, > >> > >> void *data, unsigned short size, int timeout) > >> > >> { > >> > >> + ALLOC_CACHE_ALIGN_BUFFER(struct devrequest, setup_packet, > >> + sizeof(struct devrequest)); > >> > >> if ((timeout == 0)&& (!asynch_allowed)) { > >> > >> /* request for a asynch control pipe is not allowed */ > >> return -1; > >> > >> } > >> > >> /* set setup command */ > >> > >> - setup_packet.requesttype = requesttype; > >> - setup_packet.request = request; > >> - setup_packet.value = cpu_to_le16(value); > >> - setup_packet.index = cpu_to_le16(index); > >> - setup_packet.length = cpu_to_le16(size); > >> + setup_packet->requesttype = requesttype; > >> + setup_packet->request = request; > >> + setup_packet->value = cpu_to_le16(value); > >> + setup_packet->index = cpu_to_le16(index); > >> + setup_packet->length = cpu_to_le16(size); > >> > >> USB_PRINTF("usb_control_msg: request: 0x%X, requesttype: 0x%X, " \ > >> > >> "value 0x%X index 0x%X length 0x%X\n", > >> request, requesttype, value, index, size); > >> > >> dev->status = USB_ST_NOT_PROC; /*not yet processed */ > >> > >> - submit_control_msg(dev, pipe, data, size,&setup_packet); > >> + submit_control_msg(dev, pipe, data, size, setup_packet); > >> > >> if (timeout == 0) > >> > >> return (int)size; > >> > >> @@ -694,7 +695,7 @@ static int usb_string_sub(struct usb_device *dev, > >> unsigned int langid, */ > >> > >> int usb_string(struct usb_device *dev, int index, char *buf, size_t > >> size) { > >> > >> - unsigned char mybuf[USB_BUFSIZ]; > >> + ALLOC_CACHE_ALIGN_BUFFER(unsigned char, mybuf, USB_BUFSIZ); > >> > >> unsigned char *tbuf; > >> int err; > >> unsigned int u, idx; > >> > >> @@ -794,7 +795,7 @@ int usb_new_device(struct usb_device *dev) > >> > >> { > >> > >> int addr, err; > >> int tmp; > >> > >> - unsigned char tmpbuf[USB_BUFSIZ]; > >> + ALLOC_CACHE_ALIGN_BUFFER(unsigned char, tmpbuf, USB_BUFSIZ); > >> > >> /* We still haven't set the Address yet */ > >> addr = dev->devnum; > >> > >> @@ -842,7 +843,10 @@ int usb_new_device(struct usb_device *dev) > >> > >> dev->epmaxpacketin[0] = 64; > >> dev->epmaxpacketout[0] = 64; > >> > >> - err = usb_get_descriptor(dev, USB_DT_DEVICE, 0, desc, 64); > >> + desc->bMaxPacketSize0 = 0; > >> + /*8 bytes of the descriptor to read Max packet size*/ > >> + err = usb_get_descriptor(dev, USB_DT_DEVICE, 0, desc, > >> + 8); > > > > Did some unrelated addition/change creep in here? > > No, This is the fix for the similar issue as is discussed for string > fetch(). > When the device partially fills the passed buffer and we try to > invalidate the partial buffer > the cache alignment error crops up. > > The code in "ehci_submit_async() " invalidates *partially* the passed > buffer though we pass aligned buffer after "STD_ASS" > is received. Actually it should invalidate only the cached line which is > equal(~32) to device desc length. > > If we pass actual device desc length the cache alignment error does not > spew. > Note that here we are aligning the buffer still the error comes. Then please send this fix as a separate patch. And I think ehci_hcd is what should be fixed then as I said in the other email, or am I wrong? > > >> if (err< 0) { > >> > >> USB_PRINTF("usb_new_device: usb_get_descriptor() failed\n"); > >> return 1; > >> > >> @@ -905,7 +909,7 @@ int usb_new_device(struct usb_device *dev) > >> > >> tmp = sizeof(dev->descriptor); > >> > >> err = usb_get_descriptor(dev, USB_DT_DEVICE, 0, > >> > >> - &dev->descriptor, sizeof(dev->descriptor)); > >> + desc, sizeof(dev->descriptor)); > > > > Won't this change (desc) break anything? > > Its not breaking any thing. For safer side we could add memcpy to copy > from local desc > to global desc. What you say? What do you mean? So you changed the use from some global variable to different (local) variable? This might break stuff I fear :-( > > >> if (err< tmp) { > >> > >> if (err< 0) > >> > >> printf("unable to get device descriptor (error=%d)\n", > > > > The rest seems fine, from now on it seems to be only matter of trivial > > fix. Thanks for your effort so far! > > > > M > > If rest of the code is fine in [Patch V3 1/2] except these two issue can > it be acknowledged for up-streaming? Well, there are those two issues which I'd really prefer to be fixed before accepting the code. I believe you can understand why. Thanks! M
Hi, On Friday 02 March 2012 12:08 AM, Marek Vasut wrote: >> On Thursday 01 March 2012 03:05 AM, Marek Vasut wrote: >>>> As DMA expects the buffers to be equal and larger then >>>> cache lines, This aligns buffers at cacheline. >>>> >>>> Signed-off-by: Puneet Saxena<puneets@nvidia.com> >>>> --- >>>> >>>> Changes for V2: >>>> - Use "ARCH_DMA_MINALIGN" directly >>>> - Use "ALIGN" to align size as cacheline >>>> - Removed headers from usb.h >>>> - Send 8 bytes of device descriptor size to read >>>> >>>> Max packet size >>>> >>>> scsi.h header is needed to avoid extra memcpy from local buffer >>>> to global buffer. >>>> >>>> Changes for V3: >>>> - Removed local descriptor elements copy to global descriptor >>>> elements - Removed "Signed-off-by: Jim Lin<jilin@nvidia.com>" from >>>> commit >>>> >>>> message >>>> >>>> common/cmd_usb.c | 3 +- >>>> common/usb.c | 57 >>>> >>>> ++++++++++++++++++++++------------------- common/usb_storage.c | >>>> 59 ++++++++++++++++++++---------------------- disk/part_dos.c >>>> >>>> | 2 +- >>>> >>>> drivers/usb/host/ehci-hcd.c | 8 ++++++ >>>> include/scsi.h | 4 ++- >>>> 6 files changed, 73 insertions(+), 60 deletions(-) >>>> >>>> diff --git a/common/cmd_usb.c b/common/cmd_usb.c >>>> index 320667f..bca9d94 100644 >>>> --- a/common/cmd_usb.c >>>> +++ b/common/cmd_usb.c >>>> @@ -150,7 +150,8 @@ void usb_display_class_sub(unsigned char dclass, >>>> unsigned char subclass, >>>> >>>> void usb_display_string(struct usb_device *dev, int index) >>>> { >>>> >>>> - char buffer[256]; >>>> + ALLOC_CACHE_ALIGN_BUFFER(char, buffer, 256); >>>> + >>>> >>>> if (index != 0) { >>>> >>>> if (usb_string(dev, index,&buffer[0], 256)> 0) >>>> >>>> printf("String: \"%s\"", buffer); >>>> >>>> diff --git a/common/usb.c b/common/usb.c >>>> index 63a11c8..191bc5b 100644 >>>> --- a/common/usb.c >>>> +++ b/common/usb.c >>>> @@ -73,7 +73,6 @@ static struct usb_device usb_dev[USB_MAX_DEVICE]; >>>> >>>> static int dev_index; >>>> static int running; >>>> static int asynch_allowed; >>>> >>>> -static struct devrequest setup_packet; >>>> >>>> char usb_started; /* flag for the started/stopped USB status */ >>>> >>>> @@ -185,23 +184,25 @@ int usb_control_msg(struct usb_device *dev, >>>> unsigned int pipe, unsigned short value, unsigned short index, >>>> >>>> void *data, unsigned short size, int timeout) >>>> >>>> { >>>> >>>> + ALLOC_CACHE_ALIGN_BUFFER(struct devrequest, setup_packet, >>>> + sizeof(struct devrequest)); >>>> >>>> if ((timeout == 0)&& (!asynch_allowed)) { >>>> >>>> /* request for a asynch control pipe is not allowed */ >>>> return -1; >>>> >>>> } >>>> >>>> /* set setup command */ >>>> >>>> - setup_packet.requesttype = requesttype; >>>> - setup_packet.request = request; >>>> - setup_packet.value = cpu_to_le16(value); >>>> - setup_packet.index = cpu_to_le16(index); >>>> - setup_packet.length = cpu_to_le16(size); >>>> + setup_packet->requesttype = requesttype; >>>> + setup_packet->request = request; >>>> + setup_packet->value = cpu_to_le16(value); >>>> + setup_packet->index = cpu_to_le16(index); >>>> + setup_packet->length = cpu_to_le16(size); >>>> >>>> USB_PRINTF("usb_control_msg: request: 0x%X, requesttype: 0x%X, " \ >>>> >>>> "value 0x%X index 0x%X length 0x%X\n", >>>> request, requesttype, value, index, size); >>>> >>>> dev->status = USB_ST_NOT_PROC; /*not yet processed */ >>>> >>>> - submit_control_msg(dev, pipe, data, size,&setup_packet); >>>> + submit_control_msg(dev, pipe, data, size, setup_packet); >>>> >>>> if (timeout == 0) >>>> >>>> return (int)size; >>>> >>>> @@ -694,7 +695,7 @@ static int usb_string_sub(struct usb_device *dev, >>>> unsigned int langid, */ >>>> >>>> int usb_string(struct usb_device *dev, int index, char *buf, size_t >>>> size) { >>>> >>>> - unsigned char mybuf[USB_BUFSIZ]; >>>> + ALLOC_CACHE_ALIGN_BUFFER(unsigned char, mybuf, USB_BUFSIZ); >>>> >>>> unsigned char *tbuf; >>>> int err; >>>> unsigned int u, idx; >>>> >>>> @@ -794,7 +795,7 @@ int usb_new_device(struct usb_device *dev) >>>> >>>> { >>>> >>>> int addr, err; >>>> int tmp; >>>> >>>> - unsigned char tmpbuf[USB_BUFSIZ]; >>>> + ALLOC_CACHE_ALIGN_BUFFER(unsigned char, tmpbuf, USB_BUFSIZ); >>>> >>>> /* We still haven't set the Address yet */ >>>> addr = dev->devnum; >>>> >>>> @@ -842,7 +843,10 @@ int usb_new_device(struct usb_device *dev) >>>> >>>> dev->epmaxpacketin[0] = 64; >>>> dev->epmaxpacketout[0] = 64; >>>> >>>> - err = usb_get_descriptor(dev, USB_DT_DEVICE, 0, desc, 64); >>>> + desc->bMaxPacketSize0 = 0; >>>> + /*8 bytes of the descriptor to read Max packet size*/ >>>> + err = usb_get_descriptor(dev, USB_DT_DEVICE, 0, desc, >>>> + 8); >>> Did some unrelated addition/change creep in here? >> No, This is the fix for the similar issue as is discussed for string >> fetch(). >> When the device partially fills the passed buffer and we try to >> invalidate the partial buffer >> the cache alignment error crops up. >> >> The code in "ehci_submit_async() " invalidates *partially* the passed >> buffer though we pass aligned buffer after "STD_ASS" >> is received. Actually it should invalidate only the cached line which is >> equal(~32) to device desc length. >> >> If we pass actual device desc length the cache alignment error does not >> spew. >> Note that here we are aligning the buffer still the error comes. > Then please send this fix as a separate patch. And I think ehci_hcd is what > should be fixed then as I said in the other email, or am I wrong? > Yes, I will send this fix in separate patch. To address partial invalidate issue will send another patch in "ehci_hcd()". >>>> if (err< 0) { >>>> >>>> USB_PRINTF("usb_new_device: usb_get_descriptor() failed\n"); >>>> return 1; >>>> >>>> @@ -905,7 +909,7 @@ int usb_new_device(struct usb_device *dev) >>>> >>>> tmp = sizeof(dev->descriptor); >>>> >>>> err = usb_get_descriptor(dev, USB_DT_DEVICE, 0, >>>> >>>> - &dev->descriptor, sizeof(dev->descriptor)); >>>> + desc, sizeof(dev->descriptor)); >>> Won't this change (desc) break anything? >> Its not breaking any thing. For safer side we could add memcpy to copy >> from local desc >> to global desc. What you say? > What do you mean? So you changed the use from some global variable to different > (local) variable? This might break stuff I fear :-( > Actually in previous comments it was said to not cache align "struct usb_device_descriptor descriptor; /* Device Descriptor */ Line:112 usb.h, in header file. So another way is to define cache aligned local variable and pass it to "usb_get_descriptor". After returning from "usb_get_descriptor", memcpy this buffer to global variable "dev->descriptor". I verified the devices, usb mouse, mass-storage etc... that without this memcpy to global variable, its not breaking anything. That's why I avoided memcpy. >>>> if (err< tmp) { >>>> >>>> if (err< 0) >>>> >>>> printf("unable to get device descriptor (error=%d)\n", >>> The rest seems fine, from now on it seems to be only matter of trivial >>> fix. Thanks for your effort so far! >>> >>> M >> If rest of the code is fine in [Patch V3 1/2] except these two issue can >> it be acknowledged for up-streaming? > Well, there are those two issues which I'd really prefer to be fixed before > accepting the code. I believe you can understand why. > > Thanks! > > M Thanx, Puneet ----------------------------------------------------------------------------------- This email message is for the sole use of the intended recipient(s) and may contain confidential information. Any unauthorized review, use, disclosure or distribution is prohibited. If you are not the intended recipient, please contact the sender by reply email and destroy all copies of the original message. -----------------------------------------------------------------------------------
> Hi, > > On Friday 02 March 2012 12:08 AM, Marek Vasut wrote: > >> On Thursday 01 March 2012 03:05 AM, Marek Vasut wrote: > >>>> As DMA expects the buffers to be equal and larger then > >>>> cache lines, This aligns buffers at cacheline. > >>>> > >>>> Signed-off-by: Puneet Saxena<puneets@nvidia.com> > >>>> --- > >>>> > >>>> Changes for V2: > >>>> - Use "ARCH_DMA_MINALIGN" directly > >>>> - Use "ALIGN" to align size as cacheline > >>>> - Removed headers from usb.h > >>>> - Send 8 bytes of device descriptor size to read > >>>> > >>>> Max packet size > >>>> > >>>> scsi.h header is needed to avoid extra memcpy from local buffer > >>>> to global buffer. > >>>> > >>>> Changes for V3: > >>>> - Removed local descriptor elements copy to global descriptor > >>>> elements - Removed "Signed-off-by: Jim Lin<jilin@nvidia.com>" > >>>> from commit > >>>> > >>>> message > >>>> > >>>> common/cmd_usb.c | 3 +- > >>>> common/usb.c | 57 > >>>> > >>>> ++++++++++++++++++++++------------------- common/usb_storage.c > >>>> | 59 ++++++++++++++++++++---------------------- disk/part_dos.c > >>>> > >>>> | 2 +- > >>>> > >>>> drivers/usb/host/ehci-hcd.c | 8 ++++++ > >>>> include/scsi.h | 4 ++- > >>>> 6 files changed, 73 insertions(+), 60 deletions(-) > >>>> > >>>> diff --git a/common/cmd_usb.c b/common/cmd_usb.c > >>>> index 320667f..bca9d94 100644 > >>>> --- a/common/cmd_usb.c > >>>> +++ b/common/cmd_usb.c > >>>> @@ -150,7 +150,8 @@ void usb_display_class_sub(unsigned char dclass, > >>>> unsigned char subclass, > >>>> > >>>> void usb_display_string(struct usb_device *dev, int index) > >>>> { > >>>> > >>>> - char buffer[256]; > >>>> + ALLOC_CACHE_ALIGN_BUFFER(char, buffer, 256); > >>>> + > >>>> > >>>> if (index != 0) { > >>>> > >>>> if (usb_string(dev, index,&buffer[0], 256)> 0) > >>>> > >>>> printf("String: \"%s\"", buffer); > >>>> > >>>> diff --git a/common/usb.c b/common/usb.c > >>>> index 63a11c8..191bc5b 100644 > >>>> --- a/common/usb.c > >>>> +++ b/common/usb.c > >>>> @@ -73,7 +73,6 @@ static struct usb_device usb_dev[USB_MAX_DEVICE]; > >>>> > >>>> static int dev_index; > >>>> static int running; > >>>> static int asynch_allowed; > >>>> > >>>> -static struct devrequest setup_packet; > >>>> > >>>> char usb_started; /* flag for the started/stopped USB status */ > >>>> > >>>> @@ -185,23 +184,25 @@ int usb_control_msg(struct usb_device *dev, > >>>> unsigned int pipe, unsigned short value, unsigned short index, > >>>> > >>>> void *data, unsigned short size, int timeout) > >>>> > >>>> { > >>>> > >>>> + ALLOC_CACHE_ALIGN_BUFFER(struct devrequest, setup_packet, > >>>> + sizeof(struct devrequest)); > >>>> > >>>> if ((timeout == 0)&& (!asynch_allowed)) { > >>>> > >>>> /* request for a asynch control pipe is not allowed */ > >>>> return -1; > >>>> > >>>> } > >>>> > >>>> /* set setup command */ > >>>> > >>>> - setup_packet.requesttype = requesttype; > >>>> - setup_packet.request = request; > >>>> - setup_packet.value = cpu_to_le16(value); > >>>> - setup_packet.index = cpu_to_le16(index); > >>>> - setup_packet.length = cpu_to_le16(size); > >>>> + setup_packet->requesttype = requesttype; > >>>> + setup_packet->request = request; > >>>> + setup_packet->value = cpu_to_le16(value); > >>>> + setup_packet->index = cpu_to_le16(index); > >>>> + setup_packet->length = cpu_to_le16(size); > >>>> > >>>> USB_PRINTF("usb_control_msg: request: 0x%X, requesttype: 0x%X, " \ > >>>> > >>>> "value 0x%X index 0x%X length 0x%X\n", > >>>> request, requesttype, value, index, size); > >>>> > >>>> dev->status = USB_ST_NOT_PROC; /*not yet processed */ > >>>> > >>>> - submit_control_msg(dev, pipe, data, size,&setup_packet); > >>>> + submit_control_msg(dev, pipe, data, size, setup_packet); > >>>> > >>>> if (timeout == 0) > >>>> > >>>> return (int)size; > >>>> > >>>> @@ -694,7 +695,7 @@ static int usb_string_sub(struct usb_device *dev, > >>>> unsigned int langid, */ > >>>> > >>>> int usb_string(struct usb_device *dev, int index, char *buf, size_t > >>>> size) { > >>>> > >>>> - unsigned char mybuf[USB_BUFSIZ]; > >>>> + ALLOC_CACHE_ALIGN_BUFFER(unsigned char, mybuf, USB_BUFSIZ); > >>>> > >>>> unsigned char *tbuf; > >>>> int err; > >>>> unsigned int u, idx; > >>>> > >>>> @@ -794,7 +795,7 @@ int usb_new_device(struct usb_device *dev) > >>>> > >>>> { > >>>> > >>>> int addr, err; > >>>> int tmp; > >>>> > >>>> - unsigned char tmpbuf[USB_BUFSIZ]; > >>>> + ALLOC_CACHE_ALIGN_BUFFER(unsigned char, tmpbuf, USB_BUFSIZ); > >>>> > >>>> /* We still haven't set the Address yet */ > >>>> addr = dev->devnum; > >>>> > >>>> @@ -842,7 +843,10 @@ int usb_new_device(struct usb_device *dev) > >>>> > >>>> dev->epmaxpacketin[0] = 64; > >>>> dev->epmaxpacketout[0] = 64; > >>>> > >>>> - err = usb_get_descriptor(dev, USB_DT_DEVICE, 0, desc, 64); > >>>> + desc->bMaxPacketSize0 = 0; > >>>> + /*8 bytes of the descriptor to read Max packet size*/ > >>>> + err = usb_get_descriptor(dev, USB_DT_DEVICE, 0, desc, > >>>> + 8); > >>> > >>> Did some unrelated addition/change creep in here? > >> > >> No, This is the fix for the similar issue as is discussed for string > >> fetch(). > >> When the device partially fills the passed buffer and we try to > >> invalidate the partial buffer > >> the cache alignment error crops up. > >> > >> The code in "ehci_submit_async() " invalidates *partially* the passed > >> buffer though we pass aligned buffer after "STD_ASS" > >> is received. Actually it should invalidate only the cached line which is > >> equal(~32) to device desc length. > >> > >> If we pass actual device desc length the cache alignment error does not > >> spew. > >> Note that here we are aligning the buffer still the error comes. > > > > Then please send this fix as a separate patch. And I think ehci_hcd is > > what should be fixed then as I said in the other email, or am I wrong? > > Yes, I will send this fix in separate patch. To address partial > invalidate issue > will send another patch in "ehci_hcd()". Very good! I'm really glad we're working towards the proper solution, thanks for all the effort you put into it! > > >>>> if (err< 0) { > >>>> > >>>> USB_PRINTF("usb_new_device: usb_get_descriptor() failed\n"); > >>>> return 1; > >>>> > >>>> @@ -905,7 +909,7 @@ int usb_new_device(struct usb_device *dev) > >>>> > >>>> tmp = sizeof(dev->descriptor); > >>>> > >>>> err = usb_get_descriptor(dev, USB_DT_DEVICE, 0, > >>>> > >>>> - &dev->descriptor, sizeof(dev- >descriptor)); > >>>> + desc, sizeof(dev->descriptor)); > >>> > >>> Won't this change (desc) break anything? > >> > >> Its not breaking any thing. For safer side we could add memcpy to copy > >> from local desc > >> to global desc. What you say? > > > > What do you mean? So you changed the use from some global variable to > > different (local) variable? This might break stuff I fear :-( > > Actually in previous comments it was said to not cache align "struct > usb_device_descriptor descriptor; /* Device Descriptor */ Line:112 > usb.h, in header file. So another way is to define cache aligned local > variable and pass it to "usb_get_descriptor". After returning from > "usb_get_descriptor", memcpy this buffer to global variable > "dev->descriptor". > I verified the devices, usb mouse, mass-storage etc... that without this > memcpy to global variable, its not breaking anything. > That's why I avoided memcpy. Oh ... maybe the global variable is unneeded at all and using the local only is OK? > > >>>> if (err< tmp) { > >>>> > >>>> if (err< 0) > >>>> > >>>> printf("unable to get device descriptor (error=%d)\n", > >>> > >>> The rest seems fine, from now on it seems to be only matter of trivial > >>> fix. Thanks for your effort so far! > >>> > >>> M > >> > >> If rest of the code is fine in [Patch V3 1/2] except these two issue can > >> it be acknowledged for up-streaming? > > > > Well, there are those two issues which I'd really prefer to be fixed > > before accepting the code. I believe you can understand why. > > > > Thanks! > > > > M > > Thanx, > Puneet No, thank you ! M
Hi, On Friday 02 March 2012 04:13 PM, Marek Vasut wrote: >> Hi, >> >> On Friday 02 March 2012 12:08 AM, Marek Vasut wrote: >>>> On Thursday 01 March 2012 03:05 AM, Marek Vasut wrote: >>>>>> As DMA expects the buffers to be equal and larger then >>>>>> cache lines, This aligns buffers at cacheline. >>>>>> >>>>>> Signed-off-by: Puneet Saxena<puneets@nvidia.com> >>>>>> --- >>>>>> >>>>>> Changes for V2: >>>>>> - Use "ARCH_DMA_MINALIGN" directly >>>>>> - Use "ALIGN" to align size as cacheline >>>>>> - Removed headers from usb.h >>>>>> - Send 8 bytes of device descriptor size to read >>>>>> >>>>>> Max packet size >>>>>> >>>>>> scsi.h header is needed to avoid extra memcpy from local buffer >>>>>> to global buffer. >>>>>> >>>>>> Changes for V3: >>>>>> - Removed local descriptor elements copy to global descriptor >>>>>> elements - Removed "Signed-off-by: Jim Lin<jilin@nvidia.com>" >>>>>> from commit >>>>>> >>>>>> message >>>>>> >>>>>> common/cmd_usb.c | 3 +- >>>>>> common/usb.c | 57 >>>>>> >>>>>> ++++++++++++++++++++++------------------- common/usb_storage.c >>>>>> | 59 ++++++++++++++++++++---------------------- disk/part_dos.c >>>>>> >>>>>> | 2 +- >>>>>> >>>>>> drivers/usb/host/ehci-hcd.c | 8 ++++++ >>>>>> include/scsi.h | 4 ++- >>>>>> 6 files changed, 73 insertions(+), 60 deletions(-) >>>>>> >>>>>> diff --git a/common/cmd_usb.c b/common/cmd_usb.c >>>>>> index 320667f..bca9d94 100644 >>>>>> --- a/common/cmd_usb.c >>>>>> +++ b/common/cmd_usb.c >>>>>> @@ -150,7 +150,8 @@ void usb_display_class_sub(unsigned char dclass, >>>>>> unsigned char subclass, >>>>>> >>>>>> void usb_display_string(struct usb_device *dev, int index) >>>>>> { >>>>>> >>>>>> - char buffer[256]; >>>>>> + ALLOC_CACHE_ALIGN_BUFFER(char, buffer, 256); >>>>>> + >>>>>> >>>>>> if (index != 0) { >>>>>> >>>>>> if (usb_string(dev, index,&buffer[0], 256)> 0) >>>>>> >>>>>> printf("String: \"%s\"", buffer); >>>>>> >>>>>> diff --git a/common/usb.c b/common/usb.c >>>>>> index 63a11c8..191bc5b 100644 >>>>>> --- a/common/usb.c >>>>>> +++ b/common/usb.c >>>>>> @@ -73,7 +73,6 @@ static struct usb_device usb_dev[USB_MAX_DEVICE]; >>>>>> >>>>>> static int dev_index; >>>>>> static int running; >>>>>> static int asynch_allowed; >>>>>> >>>>>> -static struct devrequest setup_packet; >>>>>> >>>>>> char usb_started; /* flag for the started/stopped USB status */ >>>>>> >>>>>> @@ -185,23 +184,25 @@ int usb_control_msg(struct usb_device *dev, >>>>>> unsigned int pipe, unsigned short value, unsigned short index, >>>>>> >>>>>> void *data, unsigned short size, int timeout) >>>>>> >>>>>> { >>>>>> >>>>>> + ALLOC_CACHE_ALIGN_BUFFER(struct devrequest, setup_packet, >>>>>> + sizeof(struct devrequest)); >>>>>> >>>>>> if ((timeout == 0)&& (!asynch_allowed)) { >>>>>> >>>>>> /* request for a asynch control pipe is not allowed */ >>>>>> return -1; >>>>>> >>>>>> } >>>>>> >>>>>> /* set setup command */ >>>>>> >>>>>> - setup_packet.requesttype = requesttype; >>>>>> - setup_packet.request = request; >>>>>> - setup_packet.value = cpu_to_le16(value); >>>>>> - setup_packet.index = cpu_to_le16(index); >>>>>> - setup_packet.length = cpu_to_le16(size); >>>>>> + setup_packet->requesttype = requesttype; >>>>>> + setup_packet->request = request; >>>>>> + setup_packet->value = cpu_to_le16(value); >>>>>> + setup_packet->index = cpu_to_le16(index); >>>>>> + setup_packet->length = cpu_to_le16(size); >>>>>> >>>>>> USB_PRINTF("usb_control_msg: request: 0x%X, requesttype: 0x%X, " > \ >>>>>> >>>>>> "value 0x%X index 0x%X length 0x%X\n", >>>>>> request, requesttype, value, index, size); >>>>>> >>>>>> dev->status = USB_ST_NOT_PROC; /*not yet processed */ >>>>>> >>>>>> - submit_control_msg(dev, pipe, data, size,&setup_packet); >>>>>> + submit_control_msg(dev, pipe, data, size, setup_packet); >>>>>> >>>>>> if (timeout == 0) >>>>>> >>>>>> return (int)size; >>>>>> >>>>>> @@ -694,7 +695,7 @@ static int usb_string_sub(struct usb_device *dev, >>>>>> unsigned int langid, */ >>>>>> >>>>>> int usb_string(struct usb_device *dev, int index, char *buf, size_t >>>>>> size) { >>>>>> >>>>>> - unsigned char mybuf[USB_BUFSIZ]; >>>>>> + ALLOC_CACHE_ALIGN_BUFFER(unsigned char, mybuf, USB_BUFSIZ); >>>>>> >>>>>> unsigned char *tbuf; >>>>>> int err; >>>>>> unsigned int u, idx; >>>>>> >>>>>> @@ -794,7 +795,7 @@ int usb_new_device(struct usb_device *dev) >>>>>> >>>>>> { >>>>>> >>>>>> int addr, err; >>>>>> int tmp; >>>>>> >>>>>> - unsigned char tmpbuf[USB_BUFSIZ]; >>>>>> + ALLOC_CACHE_ALIGN_BUFFER(unsigned char, tmpbuf, USB_BUFSIZ); >>>>>> >>>>>> /* We still haven't set the Address yet */ >>>>>> addr = dev->devnum; >>>>>> >>>>>> @@ -842,7 +843,10 @@ int usb_new_device(struct usb_device *dev) >>>>>> >>>>>> dev->epmaxpacketin[0] = 64; >>>>>> dev->epmaxpacketout[0] = 64; >>>>>> >>>>>> - err = usb_get_descriptor(dev, USB_DT_DEVICE, 0, desc, 64); >>>>>> + desc->bMaxPacketSize0 = 0; >>>>>> + /*8 bytes of the descriptor to read Max packet size*/ >>>>>> + err = usb_get_descriptor(dev, USB_DT_DEVICE, 0, desc, >>>>>> + 8); >>>>> Did some unrelated addition/change creep in here? >>>> No, This is the fix for the similar issue as is discussed for string >>>> fetch(). >>>> When the device partially fills the passed buffer and we try to >>>> invalidate the partial buffer >>>> the cache alignment error crops up. >>>> >>>> The code in "ehci_submit_async() " invalidates *partially* the passed >>>> buffer though we pass aligned buffer after "STD_ASS" >>>> is received. Actually it should invalidate only the cached line which is >>>> equal(~32) to device desc length. >>>> >>>> If we pass actual device desc length the cache alignment error does not >>>> spew. >>>> Note that here we are aligning the buffer still the error comes. >>> Then please send this fix as a separate patch. And I think ehci_hcd is >>> what should be fixed then as I said in the other email, or am I wrong? >> Yes, I will send this fix in separate patch. To address partial >> invalidate issue >> will send another patch in "ehci_hcd()". > Very good! I'm really glad we're working towards the proper solution, thanks for > all the effort you put into it! >>>>>> if (err< 0) { >>>>>> >>>>>> USB_PRINTF("usb_new_device: usb_get_descriptor() > failed\n"); >>>>>> return 1; >>>>>> >>>>>> @@ -905,7 +909,7 @@ int usb_new_device(struct usb_device *dev) >>>>>> >>>>>> tmp = sizeof(dev->descriptor); >>>>>> >>>>>> err = usb_get_descriptor(dev, USB_DT_DEVICE, 0, >>>>>> >>>>>> - &dev->descriptor, sizeof(dev- >> descriptor)); >>>>>> + desc, sizeof(dev->descriptor)); >>>>> Won't this change (desc) break anything? >>>> Its not breaking any thing. For safer side we could add memcpy to copy >>>> from local desc >>>> to global desc. What you say? >>> What do you mean? So you changed the use from some global variable to >>> different (local) variable? This might break stuff I fear :-( >> Actually in previous comments it was said to not cache align "struct >> usb_device_descriptor descriptor; /* Device Descriptor */ Line:112 >> usb.h, in header file. So another way is to define cache aligned local >> variable and pass it to "usb_get_descriptor". After returning from >> "usb_get_descriptor", memcpy this buffer to global variable >> "dev->descriptor". >> I verified the devices, usb mouse, mass-storage etc... that without this >> memcpy to global variable, its not breaking anything. >> That's why I avoided memcpy. > Oh ... maybe the global variable is unneeded at all and using the local only is > OK? > Investigated further and found that "memcpy" is needed to configure Usb version, vendor id, prod Id ..etc of the device. Its also useful to hook actual device driver and detect no. of configuration supported. I strongly feel we could avoid "memcpy" by making global device desc in usb.h Line:112, cache align as done in scsi.h,Line:30 in this patch and this is accepted also? >>>>>> if (err< tmp) { >>>>>> >>>>>> if (err< 0) >>>>>> >>>>>> printf("unable to get device descriptor > (error=%d)\n", >>>>> The rest seems fine, from now on it seems to be only matter of trivial >>>>> fix. Thanks for your effort so far! >>>>> >>>>> M >>>> If rest of the code is fine in [Patch V3 1/2] except these two issue can >>>> it be acknowledged for up-streaming? >>> Well, there are those two issues which I'd really prefer to be fixed >>> before accepting the code. I believe you can understand why. >>> >>> Thanks! >>> >>> M >> Thanx, >> Puneet > No, thank you ! > > M Thanx, Puneet ----------------------------------------------------------------------------------- This email message is for the sole use of the intended recipient(s) and may contain confidential information. Any unauthorized review, use, disclosure or distribution is prohibited. If you are not the intended recipient, please contact the sender by reply email and destroy all copies of the original message. -----------------------------------------------------------------------------------
> Hi, > > On Friday 02 March 2012 04:13 PM, Marek Vasut wrote: > >> Hi, > >> > >> On Friday 02 March 2012 12:08 AM, Marek Vasut wrote: > >>>> On Thursday 01 March 2012 03:05 AM, Marek Vasut wrote: > >>>>>> As DMA expects the buffers to be equal and larger then > >>>>>> cache lines, This aligns buffers at cacheline. > >>>>>> > >>>>>> Signed-off-by: Puneet Saxena<puneets@nvidia.com> > >>>>>> --- > >>>>>> > >>>>>> Changes for V2: > >>>>>> - Use "ARCH_DMA_MINALIGN" directly > >>>>>> - Use "ALIGN" to align size as cacheline > >>>>>> - Removed headers from usb.h > >>>>>> - Send 8 bytes of device descriptor size to read > >>>>>> > >>>>>> Max packet size > >>>>>> > >>>>>> scsi.h header is needed to avoid extra memcpy from local > >>>>>> buffer to global buffer. > >>>>>> > >>>>>> Changes for V3: > >>>>>> - Removed local descriptor elements copy to global descriptor > >>>>>> elements - Removed "Signed-off-by: Jim Lin<jilin@nvidia.com>" > >>>>>> from commit > >>>>>> > >>>>>> message > >>>>>> > >>>>>> common/cmd_usb.c | 3 +- > >>>>>> common/usb.c | 57 > >>>>>> > >>>>>> ++++++++++++++++++++++------------------- common/usb_storage.c > >>>>>> > >>>>>> | 59 ++++++++++++++++++++---------------------- disk/part_dos.c > >>>>>> | > >>>>>> | 2 +- > >>>>>> > >>>>>> drivers/usb/host/ehci-hcd.c | 8 ++++++ > >>>>>> include/scsi.h | 4 ++- > >>>>>> 6 files changed, 73 insertions(+), 60 deletions(-) > >>>>>> > >>>>>> diff --git a/common/cmd_usb.c b/common/cmd_usb.c > >>>>>> index 320667f..bca9d94 100644 > >>>>>> --- a/common/cmd_usb.c > >>>>>> +++ b/common/cmd_usb.c > >>>>>> @@ -150,7 +150,8 @@ void usb_display_class_sub(unsigned char dclass, > >>>>>> unsigned char subclass, > >>>>>> > >>>>>> void usb_display_string(struct usb_device *dev, int index) > >>>>>> { > >>>>>> > >>>>>> - char buffer[256]; > >>>>>> + ALLOC_CACHE_ALIGN_BUFFER(char, buffer, 256); > >>>>>> + > >>>>>> > >>>>>> if (index != 0) { > >>>>>> > >>>>>> if (usb_string(dev, index,&buffer[0], 256)> 0) > >>>>>> > >>>>>> printf("String: \"%s\"", buffer); > >>>>>> > >>>>>> diff --git a/common/usb.c b/common/usb.c > >>>>>> index 63a11c8..191bc5b 100644 > >>>>>> --- a/common/usb.c > >>>>>> +++ b/common/usb.c > >>>>>> @@ -73,7 +73,6 @@ static struct usb_device usb_dev[USB_MAX_DEVICE]; > >>>>>> > >>>>>> static int dev_index; > >>>>>> static int running; > >>>>>> static int asynch_allowed; > >>>>>> > >>>>>> -static struct devrequest setup_packet; > >>>>>> > >>>>>> char usb_started; /* flag for the started/stopped USB status */ > >>>>>> > >>>>>> @@ -185,23 +184,25 @@ int usb_control_msg(struct usb_device *dev, > >>>>>> unsigned int pipe, unsigned short value, unsigned short index, > >>>>>> > >>>>>> void *data, unsigned short size, int timeout) > >>>>>> > >>>>>> { > >>>>>> > >>>>>> + ALLOC_CACHE_ALIGN_BUFFER(struct devrequest, setup_packet, > >>>>>> + sizeof(struct devrequest)); > >>>>>> > >>>>>> if ((timeout == 0)&& (!asynch_allowed)) { > >>>>>> > >>>>>> /* request for a asynch control pipe is not allowed */ > >>>>>> return -1; > >>>>>> > >>>>>> } > >>>>>> > >>>>>> /* set setup command */ > >>>>>> > >>>>>> - setup_packet.requesttype = requesttype; > >>>>>> - setup_packet.request = request; > >>>>>> - setup_packet.value = cpu_to_le16(value); > >>>>>> - setup_packet.index = cpu_to_le16(index); > >>>>>> - setup_packet.length = cpu_to_le16(size); > >>>>>> + setup_packet->requesttype = requesttype; > >>>>>> + setup_packet->request = request; > >>>>>> + setup_packet->value = cpu_to_le16(value); > >>>>>> + setup_packet->index = cpu_to_le16(index); > >>>>>> + setup_packet->length = cpu_to_le16(size); > >>>>>> > >>>>>> USB_PRINTF("usb_control_msg: request: 0x%X, requesttype: 0x%X, > >>>>>> " > > > > \ > > > >>>>>> "value 0x%X index 0x%X length 0x%X\n", > >>>>>> > >>>>>> request, requesttype, value, index, size); > >>>>>> > >>>>>> dev->status = USB_ST_NOT_PROC; /*not yet processed */ > >>>>>> > >>>>>> - submit_control_msg(dev, pipe, data, size,&setup_packet); > >>>>>> + submit_control_msg(dev, pipe, data, size, setup_packet); > >>>>>> > >>>>>> if (timeout == 0) > >>>>>> > >>>>>> return (int)size; > >>>>>> > >>>>>> @@ -694,7 +695,7 @@ static int usb_string_sub(struct usb_device > >>>>>> *dev, unsigned int langid, */ > >>>>>> > >>>>>> int usb_string(struct usb_device *dev, int index, char *buf, > >>>>>> size_t size) { > >>>>>> > >>>>>> - unsigned char mybuf[USB_BUFSIZ]; > >>>>>> + ALLOC_CACHE_ALIGN_BUFFER(unsigned char, mybuf, USB_BUFSIZ); > >>>>>> > >>>>>> unsigned char *tbuf; > >>>>>> int err; > >>>>>> unsigned int u, idx; > >>>>>> > >>>>>> @@ -794,7 +795,7 @@ int usb_new_device(struct usb_device *dev) > >>>>>> > >>>>>> { > >>>>>> > >>>>>> int addr, err; > >>>>>> int tmp; > >>>>>> > >>>>>> - unsigned char tmpbuf[USB_BUFSIZ]; > >>>>>> + ALLOC_CACHE_ALIGN_BUFFER(unsigned char, tmpbuf, USB_BUFSIZ); > >>>>>> > >>>>>> /* We still haven't set the Address yet */ > >>>>>> addr = dev->devnum; > >>>>>> > >>>>>> @@ -842,7 +843,10 @@ int usb_new_device(struct usb_device *dev) > >>>>>> > >>>>>> dev->epmaxpacketin[0] = 64; > >>>>>> dev->epmaxpacketout[0] = 64; > >>>>>> > >>>>>> - err = usb_get_descriptor(dev, USB_DT_DEVICE, 0, desc, 64); > >>>>>> + desc->bMaxPacketSize0 = 0; > >>>>>> + /*8 bytes of the descriptor to read Max packet size*/ > >>>>>> + err = usb_get_descriptor(dev, USB_DT_DEVICE, 0, desc, > >>>>>> + 8); > >>>>> > >>>>> Did some unrelated addition/change creep in here? > >>>> > >>>> No, This is the fix for the similar issue as is discussed for string > >>>> fetch(). > >>>> When the device partially fills the passed buffer and we try to > >>>> invalidate the partial buffer > >>>> the cache alignment error crops up. > >>>> > >>>> The code in "ehci_submit_async() " invalidates *partially* the passed > >>>> buffer though we pass aligned buffer after "STD_ASS" > >>>> is received. Actually it should invalidate only the cached line which > >>>> is equal(~32) to device desc length. > >>>> > >>>> If we pass actual device desc length the cache alignment error does > >>>> not spew. > >>>> Note that here we are aligning the buffer still the error comes. > >>> > >>> Then please send this fix as a separate patch. And I think ehci_hcd is > >>> what should be fixed then as I said in the other email, or am I wrong? > >> > >> Yes, I will send this fix in separate patch. To address partial > >> invalidate issue > >> will send another patch in "ehci_hcd()". > > > > Very good! I'm really glad we're working towards the proper solution, > > thanks for all the effort you put into it! > > > >>>>>> if (err< 0) { > >>>>>> > >>>>>> USB_PRINTF("usb_new_device: usb_get_descriptor() > > > > failed\n"); > > > >>>>>> return 1; > >>>>>> > >>>>>> @@ -905,7 +909,7 @@ int usb_new_device(struct usb_device *dev) > >>>>>> > >>>>>> tmp = sizeof(dev->descriptor); > >>>>>> > >>>>>> err = usb_get_descriptor(dev, USB_DT_DEVICE, 0, > >>>>>> > >>>>>> - &dev->descriptor, sizeof(dev- > >> > >> descriptor)); > >> > >>>>>> + desc, sizeof(dev->descriptor)); > >>>>> > >>>>> Won't this change (desc) break anything? > >>>> > >>>> Its not breaking any thing. For safer side we could add memcpy to copy > >>>> from local desc > >>>> to global desc. What you say? > >>> > >>> What do you mean? So you changed the use from some global variable to > >>> different (local) variable? This might break stuff I fear :-( > >> > >> Actually in previous comments it was said to not cache align "struct > >> usb_device_descriptor descriptor; /* Device Descriptor */ Line:112 > >> usb.h, in header file. So another way is to define cache aligned local > >> variable and pass it to "usb_get_descriptor". After returning from > >> "usb_get_descriptor", memcpy this buffer to global variable > >> "dev->descriptor". > >> I verified the devices, usb mouse, mass-storage etc... that without this > >> memcpy to global variable, its not breaking anything. > >> That's why I avoided memcpy. > > > > Oh ... maybe the global variable is unneeded at all and using the local > > only is OK? > > Investigated further and found that "memcpy" is needed to configure Usb > version, vendor id, prod Id ..etc > of the device. Its also useful to hook actual device driver and detect > no. of configuration supported. > > I strongly feel we could avoid "memcpy" by making global device desc in > usb.h Line:112, cache align as done in > scsi.h,Line:30 in this patch and this is accepted also? We try to make as many things local as possible. M
diff --git a/common/cmd_usb.c b/common/cmd_usb.c index 320667f..bca9d94 100644 --- a/common/cmd_usb.c +++ b/common/cmd_usb.c @@ -150,7 +150,8 @@ void usb_display_class_sub(unsigned char dclass, unsigned char subclass, void usb_display_string(struct usb_device *dev, int index) { - char buffer[256]; + ALLOC_CACHE_ALIGN_BUFFER(char, buffer, 256); + if (index != 0) { if (usb_string(dev, index, &buffer[0], 256) > 0) printf("String: \"%s\"", buffer); diff --git a/common/usb.c b/common/usb.c index 63a11c8..191bc5b 100644 --- a/common/usb.c +++ b/common/usb.c @@ -73,7 +73,6 @@ static struct usb_device usb_dev[USB_MAX_DEVICE]; static int dev_index; static int running; static int asynch_allowed; -static struct devrequest setup_packet; char usb_started; /* flag for the started/stopped USB status */ @@ -185,23 +184,25 @@ int usb_control_msg(struct usb_device *dev, unsigned int pipe, unsigned short value, unsigned short index, void *data, unsigned short size, int timeout) { + ALLOC_CACHE_ALIGN_BUFFER(struct devrequest, setup_packet, + sizeof(struct devrequest)); if ((timeout == 0) && (!asynch_allowed)) { /* request for a asynch control pipe is not allowed */ return -1; } /* set setup command */ - setup_packet.requesttype = requesttype; - setup_packet.request = request; - setup_packet.value = cpu_to_le16(value); - setup_packet.index = cpu_to_le16(index); - setup_packet.length = cpu_to_le16(size); + setup_packet->requesttype = requesttype; + setup_packet->request = request; + setup_packet->value = cpu_to_le16(value); + setup_packet->index = cpu_to_le16(index); + setup_packet->length = cpu_to_le16(size); USB_PRINTF("usb_control_msg: request: 0x%X, requesttype: 0x%X, " \ "value 0x%X index 0x%X length 0x%X\n", request, requesttype, value, index, size); dev->status = USB_ST_NOT_PROC; /*not yet processed */ - submit_control_msg(dev, pipe, data, size, &setup_packet); + submit_control_msg(dev, pipe, data, size, setup_packet); if (timeout == 0) return (int)size; @@ -694,7 +695,7 @@ static int usb_string_sub(struct usb_device *dev, unsigned int langid, */ int usb_string(struct usb_device *dev, int index, char *buf, size_t size) { - unsigned char mybuf[USB_BUFSIZ]; + ALLOC_CACHE_ALIGN_BUFFER(unsigned char, mybuf, USB_BUFSIZ); unsigned char *tbuf; int err; unsigned int u, idx; @@ -794,7 +795,7 @@ int usb_new_device(struct usb_device *dev) { int addr, err; int tmp; - unsigned char tmpbuf[USB_BUFSIZ]; + ALLOC_CACHE_ALIGN_BUFFER(unsigned char, tmpbuf, USB_BUFSIZ); /* We still haven't set the Address yet */ addr = dev->devnum; @@ -842,7 +843,10 @@ int usb_new_device(struct usb_device *dev) dev->epmaxpacketin[0] = 64; dev->epmaxpacketout[0] = 64; - err = usb_get_descriptor(dev, USB_DT_DEVICE, 0, desc, 64); + desc->bMaxPacketSize0 = 0; + /*8 bytes of the descriptor to read Max packet size*/ + err = usb_get_descriptor(dev, USB_DT_DEVICE, 0, desc, + 8); if (err < 0) { USB_PRINTF("usb_new_device: usb_get_descriptor() failed\n"); return 1; @@ -905,7 +909,7 @@ int usb_new_device(struct usb_device *dev) tmp = sizeof(dev->descriptor); err = usb_get_descriptor(dev, USB_DT_DEVICE, 0, - &dev->descriptor, sizeof(dev->descriptor)); + desc, sizeof(dev->descriptor)); if (err < tmp) { if (err < 0) printf("unable to get device descriptor (error=%d)\n", @@ -921,8 +925,8 @@ int usb_new_device(struct usb_device *dev) le16_to_cpus(&dev->descriptor.idProduct); le16_to_cpus(&dev->descriptor.bcdDevice); /* only support for one config for now */ - usb_get_configuration_no(dev, &tmpbuf[0], 0); - usb_parse_config(dev, &tmpbuf[0], 0); + usb_get_configuration_no(dev, tmpbuf, 0); + usb_parse_config(dev, tmpbuf, 0); usb_set_maxpacket(dev); /* we set the default configuration here */ if (usb_set_configuration(dev, dev->config.desc.bConfigurationValue)) { @@ -1076,7 +1080,7 @@ static int hub_port_reset(struct usb_device *dev, int port, unsigned short *portstat) { int tries; - struct usb_port_status portsts; + ALLOC_CACHE_ALIGN_BUFFER(struct usb_port_status, portsts, 1); unsigned short portstatus, portchange; USB_HUB_PRINTF("hub_port_reset: resetting port %d...\n", port); @@ -1085,13 +1089,13 @@ static int hub_port_reset(struct usb_device *dev, int port, usb_set_port_feature(dev, port + 1, USB_PORT_FEAT_RESET); wait_ms(200); - if (usb_get_port_status(dev, port + 1, &portsts) < 0) { + if (usb_get_port_status(dev, port + 1, portsts) < 0) { USB_HUB_PRINTF("get_port_status failed status %lX\n", dev->status); return -1; } - portstatus = le16_to_cpu(portsts.wPortStatus); - portchange = le16_to_cpu(portsts.wPortChange); + portstatus = le16_to_cpu(portsts->wPortStatus); + portchange = le16_to_cpu(portsts->wPortChange); USB_HUB_PRINTF("portstatus %x, change %x, %s\n", portstatus, portchange, @@ -1129,19 +1133,19 @@ static int hub_port_reset(struct usb_device *dev, int port, void usb_hub_port_connect_change(struct usb_device *dev, int port) { struct usb_device *usb; - struct usb_port_status portsts; + ALLOC_CACHE_ALIGN_BUFFER(struct usb_port_status, portsts, 1); unsigned short portstatus; /* Check status */ - if (usb_get_port_status(dev, port + 1, &portsts) < 0) { + if (usb_get_port_status(dev, port + 1, portsts) < 0) { USB_HUB_PRINTF("get_port_status failed\n"); return; } - portstatus = le16_to_cpu(portsts.wPortStatus); + portstatus = le16_to_cpu(portsts->wPortStatus); USB_HUB_PRINTF("portstatus %x, change %x, %s\n", portstatus, - le16_to_cpu(portsts.wPortChange), + le16_to_cpu(portsts->wPortChange), portspeed(portstatus)); /* Clear the connection change status */ @@ -1190,7 +1194,8 @@ void usb_hub_port_connect_change(struct usb_device *dev, int port) int usb_hub_configure(struct usb_device *dev) { int i; - unsigned char buffer[USB_BUFSIZ], *bitmap; + ALLOC_CACHE_ALIGN_BUFFER(unsigned char, buffer, USB_BUFSIZ); + unsigned char *bitmap; struct usb_hub_descriptor *descriptor; struct usb_hub_device *hub; #ifdef USB_HUB_DEBUG @@ -1312,16 +1317,16 @@ int usb_hub_configure(struct usb_device *dev) usb_hub_power_on(hub); for (i = 0; i < dev->maxchild; i++) { - struct usb_port_status portsts; + ALLOC_CACHE_ALIGN_BUFFER(struct usb_port_status, portsts, 1); unsigned short portstatus, portchange; - if (usb_get_port_status(dev, i + 1, &portsts) < 0) { + if (usb_get_port_status(dev, i + 1, portsts) < 0) { USB_HUB_PRINTF("get_port_status failed\n"); continue; } - portstatus = le16_to_cpu(portsts.wPortStatus); - portchange = le16_to_cpu(portsts.wPortChange); + portstatus = le16_to_cpu(portsts->wPortStatus); + portchange = le16_to_cpu(portsts->wPortChange); USB_HUB_PRINTF("Port %d Status %X Change %X\n", i + 1, portstatus, portchange); diff --git a/common/usb_storage.c b/common/usb_storage.c index de84c8d..88ca390 100644 --- a/common/usb_storage.c +++ b/common/usb_storage.c @@ -79,8 +79,7 @@ static const unsigned char us_direction[256/8] = { }; #define US_DIRECTION(x) ((us_direction[x>>3] >> (x & 7)) & 1) -static unsigned char usb_stor_buf[512]; -static ccb usb_ccb; +static ccb usb_ccb __attribute__((aligned(ARCH_DMA_MINALIGN))); /* * CBI style @@ -210,17 +209,17 @@ int usb_stor_info(void) static unsigned int usb_get_max_lun(struct us_data *us) { int len; - unsigned char result; + ALLOC_CACHE_ALIGN_BUFFER(unsigned char, result, 1); len = usb_control_msg(us->pusb_dev, usb_rcvctrlpipe(us->pusb_dev, 0), US_BBB_GET_MAX_LUN, USB_TYPE_CLASS | USB_RECIP_INTERFACE | USB_DIR_IN, 0, us->ifnum, - &result, sizeof(result), + result, sizeof(char), USB_CNTL_TIMEOUT * 5); USB_STOR_PRINTF("Get Max LUN -> len = %i, result = %i\n", - len, (int) result); - return (len > 0) ? result : 0; + len, (int) *result); + return (len > 0) ? *result : 0; } /******************************************************************************* @@ -233,9 +232,6 @@ int usb_stor_scan(int mode) unsigned char i; struct usb_device *dev; - /* GJ */ - memset(usb_stor_buf, 0, sizeof(usb_stor_buf)); - if (mode == 1) printf(" scanning bus for storage devices... "); @@ -499,7 +495,7 @@ int usb_stor_BBB_comdat(ccb *srb, struct us_data *us) int actlen; int dir_in; unsigned int pipe; - umass_bbb_cbw_t cbw; + ALLOC_CACHE_ALIGN_BUFFER(umass_bbb_cbw_t, cbw, 1); dir_in = US_DIRECTION(srb->cmd[0]); @@ -522,16 +518,16 @@ int usb_stor_BBB_comdat(ccb *srb, struct us_data *us) /* always OUT to the ep */ pipe = usb_sndbulkpipe(us->pusb_dev, us->ep_out); - cbw.dCBWSignature = cpu_to_le32(CBWSIGNATURE); - cbw.dCBWTag = cpu_to_le32(CBWTag++); - cbw.dCBWDataTransferLength = cpu_to_le32(srb->datalen); - cbw.bCBWFlags = (dir_in ? CBWFLAGS_IN : CBWFLAGS_OUT); - cbw.bCBWLUN = srb->lun; - cbw.bCDBLength = srb->cmdlen; + cbw->dCBWSignature = cpu_to_le32(CBWSIGNATURE); + cbw->dCBWTag = cpu_to_le32(CBWTag++); + cbw->dCBWDataTransferLength = cpu_to_le32(srb->datalen); + cbw->bCBWFlags = (dir_in ? CBWFLAGS_IN : CBWFLAGS_OUT); + cbw->bCBWLUN = srb->lun; + cbw->bCDBLength = srb->cmdlen; /* copy the command data into the CBW command data buffer */ /* DST SRC LEN!!! */ - memcpy(cbw.CBWCDB, srb->cmd, srb->cmdlen); - result = usb_bulk_msg(us->pusb_dev, pipe, &cbw, UMASS_BBB_CBW_SIZE, + memcpy(cbw->CBWCDB, srb->cmd, srb->cmdlen); + result = usb_bulk_msg(us->pusb_dev, pipe, cbw, UMASS_BBB_CBW_SIZE, &actlen, USB_CNTL_TIMEOUT * 5); if (result < 0) USB_STOR_PRINTF("usb_stor_BBB_comdat:usb_bulk_msg error\n"); @@ -675,7 +671,7 @@ int usb_stor_BBB_transport(ccb *srb, struct us_data *us) int dir_in; int actlen, data_actlen; unsigned int pipe, pipein, pipeout; - umass_bbb_csw_t csw; + ALLOC_CACHE_ALIGN_BUFFER(umass_bbb_csw_t, csw, 1); #ifdef BBB_XPORT_TRACE unsigned char *ptr; int index; @@ -733,7 +729,7 @@ st: retry = 0; again: USB_STOR_PRINTF("STATUS phase\n"); - result = usb_bulk_msg(us->pusb_dev, pipein, &csw, UMASS_BBB_CSW_SIZE, + result = usb_bulk_msg(us->pusb_dev, pipein, csw, UMASS_BBB_CSW_SIZE, &actlen, USB_CNTL_TIMEOUT*5); /* special handling of STALL in STATUS phase */ @@ -753,28 +749,28 @@ again: return USB_STOR_TRANSPORT_FAILED; } #ifdef BBB_XPORT_TRACE - ptr = (unsigned char *)&csw; + ptr = (unsigned char *)csw; for (index = 0; index < UMASS_BBB_CSW_SIZE; index++) printf("ptr[%d] %#x ", index, ptr[index]); printf("\n"); #endif /* misuse pipe to get the residue */ - pipe = le32_to_cpu(csw.dCSWDataResidue); + pipe = le32_to_cpu(csw->dCSWDataResidue); if (pipe == 0 && srb->datalen != 0 && srb->datalen - data_actlen != 0) pipe = srb->datalen - data_actlen; - if (CSWSIGNATURE != le32_to_cpu(csw.dCSWSignature)) { + if (CSWSIGNATURE != le32_to_cpu(csw->dCSWSignature)) { USB_STOR_PRINTF("!CSWSIGNATURE\n"); usb_stor_BBB_reset(us); return USB_STOR_TRANSPORT_FAILED; - } else if ((CBWTag - 1) != le32_to_cpu(csw.dCSWTag)) { + } else if ((CBWTag - 1) != le32_to_cpu(csw->dCSWTag)) { USB_STOR_PRINTF("!Tag\n"); usb_stor_BBB_reset(us); return USB_STOR_TRANSPORT_FAILED; - } else if (csw.bCSWStatus > CSWSTATUS_PHASE) { + } else if (csw->bCSWStatus > CSWSTATUS_PHASE) { USB_STOR_PRINTF(">PHASE\n"); usb_stor_BBB_reset(us); return USB_STOR_TRANSPORT_FAILED; - } else if (csw.bCSWStatus == CSWSTATUS_PHASE) { + } else if (csw->bCSWStatus == CSWSTATUS_PHASE) { USB_STOR_PRINTF("=PHASE\n"); usb_stor_BBB_reset(us); return USB_STOR_TRANSPORT_FAILED; @@ -782,7 +778,7 @@ again: USB_STOR_PRINTF("transferred %dB instead of %ldB\n", data_actlen, srb->datalen); return USB_STOR_TRANSPORT_FAILED; - } else if (csw.bCSWStatus == CSWSTATUS_FAILED) { + } else if (csw->bCSWStatus == CSWSTATUS_FAILED) { USB_STOR_PRINTF("FAILED\n"); return USB_STOR_TRANSPORT_FAILED; } @@ -1343,7 +1339,8 @@ int usb_stor_get_info(struct usb_device *dev, struct us_data *ss, block_dev_desc_t *dev_desc) { unsigned char perq, modi; - unsigned long cap[2]; + ALLOC_CACHE_ALIGN_BUFFER(unsigned long, cap, 2); + ALLOC_CACHE_ALIGN_BUFFER(unsigned char, usb_stor_buf, 36); unsigned long *capacity, *blksz; ccb *pccb = &usb_ccb; @@ -1367,9 +1364,9 @@ int usb_stor_get_info(struct usb_device *dev, struct us_data *ss, /* drive is removable */ dev_desc->removable = 1; } - memcpy(&dev_desc->vendor[0], &usb_stor_buf[8], 8); - memcpy(&dev_desc->product[0], &usb_stor_buf[16], 16); - memcpy(&dev_desc->revision[0], &usb_stor_buf[32], 4); + memcpy(&dev_desc->vendor[0], (const void *) &usb_stor_buf[8], 8); + memcpy(&dev_desc->product[0], (const void *) &usb_stor_buf[16], 16); + memcpy(&dev_desc->revision[0], (const void *) &usb_stor_buf[32], 4); dev_desc->vendor[8] = 0; dev_desc->product[16] = 0; dev_desc->revision[4] = 0; diff --git a/disk/part_dos.c b/disk/part_dos.c index b5bcb37..70211ee 100644 --- a/disk/part_dos.c +++ b/disk/part_dos.c @@ -87,7 +87,7 @@ static int test_block_type(unsigned char *buffer) int test_part_dos (block_dev_desc_t *dev_desc) { - unsigned char buffer[dev_desc->blksz]; + ALLOC_CACHE_ALIGN_BUFFER(unsigned char, buffer, dev_desc->blksz); if ((dev_desc->block_read(dev_desc->dev, 0, 1, (ulong *) buffer) != 1) || (buffer[DOS_PART_MAGIC_OFFSET + 0] != 0x55) || diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c index d893b2a..eb5220b 100644 --- a/drivers/usb/host/ehci-hcd.c +++ b/drivers/usb/host/ehci-hcd.c @@ -120,6 +120,14 @@ static struct descriptor { */ static void flush_invalidate(u32 addr, int size, int flush) { + /* + * Size is the bytes actually moved during transaction, + * which may not equal to the cache line. This results + * stop address passed for invalidating cache may not be aligned. + * Therfore making size as multiple of cache line size. + */ + size = ALIGN(size, ARCH_DMA_MINALIGN); + if (flush) flush_dcache_range(addr, addr + size); else diff --git a/include/scsi.h b/include/scsi.h index c52759c..89ae45f 100644 --- a/include/scsi.h +++ b/include/scsi.h @@ -26,7 +26,9 @@ typedef struct SCSI_cmd_block{ unsigned char cmd[16]; /* command */ - unsigned char sense_buf[64]; /* for request sense */ + /* for request sense */ + unsigned char sense_buf[64] + __attribute__((aligned(ARCH_DMA_MINALIGN))); unsigned char status; /* SCSI Status */ unsigned char target; /* Target ID */ unsigned char lun; /* Target LUN */
As DMA expects the buffers to be equal and larger then cache lines, This aligns buffers at cacheline. Signed-off-by: Puneet Saxena <puneets@nvidia.com> --- Changes for V2: - Use "ARCH_DMA_MINALIGN" directly - Use "ALIGN" to align size as cacheline - Removed headers from usb.h - Send 8 bytes of device descriptor size to read Max packet size scsi.h header is needed to avoid extra memcpy from local buffer to global buffer. Changes for V3: - Removed local descriptor elements copy to global descriptor elements - Removed "Signed-off-by: Jim Lin <jilin@nvidia.com>" from commit message common/cmd_usb.c | 3 +- common/usb.c | 57 ++++++++++++++++++++++------------------- common/usb_storage.c | 59 ++++++++++++++++++++---------------------- disk/part_dos.c | 2 +- drivers/usb/host/ehci-hcd.c | 8 ++++++ include/scsi.h | 4 ++- 6 files changed, 73 insertions(+), 60 deletions(-)