[U-Boot] [PATCH v3 1/2] usb: align buffers at cacheline

[puneets <puneets@nvidia.com>]

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.
-----------------------------------------------------------------------------------