Patchwork [U-Boot,v3,2/2] usb: Add CONFIG to fetch string descriptor

login
register
mail settings
Submitter Puneet Saxena
Date Feb. 29, 2012, 2:21 p.m.
Message ID <1330525309-21333-2-git-send-email-puneets@nvidia.com>
Download mbox | patch
Permalink /patch/143737/
State Not Applicable
Delegated to: Marek Vasut
Headers show

Comments

Puneet Saxena - Feb. 29, 2012, 2:21 p.m.
Add "CONFIG_USB_STRING_FETCH" to fetch first string descriptor length
and then pass this length to fetch string descriptor.

Signed-off-by: Puneet Saxena <puneets@nvidia.com>
---

Changes for V2:
   - Change existing config by "CONFIG_USB_STRING_FETCH"

Changes for V3:
    - Removed extra new line
    - Explained "CONFIG_USB_STRING_FETCH" in top level README

 README                          |    4 ++++
 common/usb.c                    |    4 ++++
 include/configs/tegra2-common.h |    2 ++
 3 files changed, 10 insertions(+), 0 deletions(-)
Marek Vasut - Feb. 29, 2012, 9:29 p.m.
> Add "CONFIG_USB_STRING_FETCH" to fetch first string descriptor length
> and then pass this length to fetch string descriptor.
> 
> Signed-off-by: Puneet Saxena <puneets@nvidia.com>
> ---
> 
> Changes for V2:
>    - Change existing config by "CONFIG_USB_STRING_FETCH"
> 
> Changes for V3:
>     - Removed extra new line
>     - Explained "CONFIG_USB_STRING_FETCH" in top level README
> 
>  README                          |    4 ++++
>  common/usb.c                    |    4 ++++
>  include/configs/tegra2-common.h |    2 ++
>  3 files changed, 10 insertions(+), 0 deletions(-)
> 
> diff --git a/README b/README
> index 7adf7c7..c045a37 100644
> --- a/README
> +++ b/README
> @@ -1138,6 +1138,10 @@ The following options need to be configured:
>  		CONFIG_USB_EHCI_TXFIFO_THRESH enables setting of the
>  		txfilltuning field in the EHCI controller on reset.
> 
> +		CONFIG_USB_STRING_FETCH
> +		Enables settings to USB core to handle string issues which
> +		few devices can not handle.
> +
>  - USB Device:
>  		Define the below if you wish to use the USB console.
>  		Once firmware is rebuilt from a serial console issue the
> diff --git a/common/usb.c b/common/usb.c
> index 191bc5b..a73cb60 100644
> --- a/common/usb.c
> +++ b/common/usb.c
> @@ -658,9 +658,13 @@ static int usb_string_sub(struct usb_device *dev,
> unsigned int langid, {
>  	int rc;
> 
> +#ifdef CONFIG_USB_STRING_FETCH

Shouldn't this be something like ... CONFIG_USB_AVOID_STRING_FETCH then?

Anyway, how come some devices can't handle it? What happens then? What devices 
are those (exact type etc)?

I believe the bug is deeper and adding extra config options can be avoided, what 
do you think?

Thanks!

M

> +	rc = -1;
> +#else
>  	/* Try to read the string descriptor by asking for the maximum
>  	 * possible number of bytes */
>  	rc = usb_get_string(dev, langid, index, buf, 255);
> +#endif
> 
>  	/* If that failed try to read the descriptor length, then
>  	 * ask for just that many bytes */
> diff --git a/include/configs/tegra2-common.h
> b/include/configs/tegra2-common.h index 266d0e5..d20b49c 100644
> --- a/include/configs/tegra2-common.h
> +++ b/include/configs/tegra2-common.h
> @@ -93,6 +93,8 @@
>  #define CONFIG_USB_EHCI_TXFIFO_THRESH	10
>  #define CONFIG_EHCI_IS_TDI
>  #define CONFIG_EHCI_DCACHE
> +/* string descriptors must not be fetched using a 255-byte read */
> +#define CONFIG_USB_STRING_FETCH
> 
>  /* include default commands */
>  #include <config_cmd_default.h>
Puneet Saxena - March 1, 2012, 11:07 a.m.
Hi Marek,
On Thursday 01 March 2012 02:59 AM, Marek Vasut wrote:
>> Add "CONFIG_USB_STRING_FETCH" to fetch first string descriptor length
>> and then pass this length to fetch string descriptor.
>>
>> Signed-off-by: Puneet Saxena<puneets@nvidia.com>
>> ---
>>
>> Changes for V2:
>>     - Change existing config by "CONFIG_USB_STRING_FETCH"
>>
>> Changes for V3:
>>      - Removed extra new line
>>      - Explained "CONFIG_USB_STRING_FETCH" in top level README
>>
>>   README                          |    4 ++++
>>   common/usb.c                    |    4 ++++
>>   include/configs/tegra2-common.h |    2 ++
>>   3 files changed, 10 insertions(+), 0 deletions(-)
>>
>> diff --git a/README b/README
>> index 7adf7c7..c045a37 100644
>> --- a/README
>> +++ b/README
>> @@ -1138,6 +1138,10 @@ The following options need to be configured:
>>   		CONFIG_USB_EHCI_TXFIFO_THRESH enables setting of the
>>   		txfilltuning field in the EHCI controller on reset.
>>
>> +		CONFIG_USB_STRING_FETCH
>> +		Enables settings to USB core to handle string issues which
>> +		few devices can not handle.
>> +
>>   - USB Device:
>>   		Define the below if you wish to use the USB console.
>>   		Once firmware is rebuilt from a serial console issue the
>> diff --git a/common/usb.c b/common/usb.c
>> index 191bc5b..a73cb60 100644
>> --- a/common/usb.c
>> +++ b/common/usb.c
>> @@ -658,9 +658,13 @@ static int usb_string_sub(struct usb_device *dev,
>> unsigned int langid, {
>>   	int rc;
>>
>> +#ifdef CONFIG_USB_STRING_FETCH
> Shouldn't this be something like ... CONFIG_USB_AVOID_STRING_FETCH then?
>
> Anyway, how come some devices can't handle it? What happens then? What devices
> are those (exact type etc)?
>
> I believe the bug is deeper and adding extra config options can be avoided, what
> do you think?
>
> Thanks!
>
> M
>
It does not avoid string fetch.
I checked with few mass storage devices that they does not handle string 
descriptor request correctly and so we get
start/stop Cache alignment error. One way is, blacklist those devices by 
vendor id/product id..
etc. This is done in Linux kernel. Plz see Message.c (drivers\usb\core) 
Line: 722.
Blacklisting the device requires a framework to detect the 
device...However this could be achieved
simply with this implementation.

>> +	rc = -1;
>> +#else
>>   	/* Try to read the string descriptor by asking for the maximum
>>   	 * possible number of bytes */
>>   	rc = usb_get_string(dev, langid, index, buf, 255);
>> +#endif
>>
>>   	/* If that failed try to read the descriptor length, then
>>   	 * ask for just that many bytes */
>> diff --git a/include/configs/tegra2-common.h
>> b/include/configs/tegra2-common.h index 266d0e5..d20b49c 100644
>> --- a/include/configs/tegra2-common.h
>> +++ b/include/configs/tegra2-common.h
>> @@ -93,6 +93,8 @@
>>   #define CONFIG_USB_EHCI_TXFIFO_THRESH	10
>>   #define CONFIG_EHCI_IS_TDI
>>   #define CONFIG_EHCI_DCACHE
>> +/* string descriptors must not be fetched using a 255-byte read */
>> +#define CONFIG_USB_STRING_FETCH
>>
>>   /* include default commands */
>>   #include<config_cmd_default.h>


-----------------------------------------------------------------------------------
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.
-----------------------------------------------------------------------------------
Marek Vasut - March 1, 2012, 11:45 a.m.
Hi!

> Hi Marek,
> 
> On Thursday 01 March 2012 02:59 AM, Marek Vasut wrote:
> >> Add "CONFIG_USB_STRING_FETCH" to fetch first string descriptor length
> >> and then pass this length to fetch string descriptor.
> >> 
> >> Signed-off-by: Puneet Saxena<puneets@nvidia.com>
> >> ---
> >> 
> >> Changes for V2:
> >>     - Change existing config by "CONFIG_USB_STRING_FETCH"
> >> 
> >> Changes for V3:
> >>      - Removed extra new line
> >>      - Explained "CONFIG_USB_STRING_FETCH" in top level README
> >>   
> >>   README                          |    4 ++++
> >>   common/usb.c                    |    4 ++++
> >>   include/configs/tegra2-common.h |    2 ++
> >>   3 files changed, 10 insertions(+), 0 deletions(-)
> >> 
> >> diff --git a/README b/README
> >> index 7adf7c7..c045a37 100644
> >> --- a/README
> >> +++ b/README
> >> 
> >> @@ -1138,6 +1138,10 @@ The following options need to be configured:
> >>   		CONFIG_USB_EHCI_TXFIFO_THRESH enables setting of the
> >>   		txfilltuning field in the EHCI controller on reset.
> >> 
> >> +		CONFIG_USB_STRING_FETCH
> >> +		Enables settings to USB core to handle string issues which
> >> +		few devices can not handle.
> >> +
> >> 
> >>   - USB Device:
> >>   		Define the below if you wish to use the USB console.
> >>   		Once firmware is rebuilt from a serial console issue the
> >> 
> >> diff --git a/common/usb.c b/common/usb.c
> >> index 191bc5b..a73cb60 100644
> >> --- a/common/usb.c
> >> +++ b/common/usb.c
> >> @@ -658,9 +658,13 @@ static int usb_string_sub(struct usb_device *dev,
> >> unsigned int langid, {
> >> 
> >>   	int rc;
> >> 
> >> +#ifdef CONFIG_USB_STRING_FETCH
> > 
> > Shouldn't this be something like ... CONFIG_USB_AVOID_STRING_FETCH then?
> > 
> > Anyway, how come some devices can't handle it? What happens then? What
> > devices are those (exact type etc)?
> > 
> > I believe the bug is deeper and adding extra config options can be
> > avoided, what do you think?
> > 
> > Thanks!
> > 
> > M
> 
> It does not avoid string fetch.

Well it does certainly not call usb_get_string()

> I checked with few mass storage devices that they does not handle string
> descriptor request correctly and so we get
> start/stop Cache alignment error.

Cache alignment error ? Wow, how's that actually related to SUB string fetching? 
Maybe we should manually realign the result then? I still don't really 
understand what you're seeing, sorry ... can you please elaborate?

> One way is, blacklist those devices by
> vendor id/product id..
> etc. This is done in Linux kernel. Plz see Message.c (drivers\usb\core)
> Line: 722.
> Blacklisting the device requires a framework to detect the
> device...However this could be achieved
> simply with this implementation.

Sure, I see your intention, but I still don't get the problem, see above. Can 
you also tell me if there's particular device that's broken and which one is it 
(precise type etc)?

Thanks a lot for your cooperation on this matter!

M

> 
> >> +	rc = -1;
> >> +#else
> >> 
> >>   	/* Try to read the string descriptor by asking for the maximum
> >>   	
> >>   	 * possible number of bytes */
> >>   	
> >>   	rc = usb_get_string(dev, langid, index, buf, 255);
> >> 
> >> +#endif
> >> 
> >>   	/* If that failed try to read the descriptor length, then
> >>   	
> >>   	 * ask for just that many bytes */
> >> 
> >> diff --git a/include/configs/tegra2-common.h
> >> b/include/configs/tegra2-common.h index 266d0e5..d20b49c 100644
> >> --- a/include/configs/tegra2-common.h
> >> +++ b/include/configs/tegra2-common.h
> >> @@ -93,6 +93,8 @@
> >> 
> >>   #define CONFIG_USB_EHCI_TXFIFO_THRESH	10
> >>   #define CONFIG_EHCI_IS_TDI
> >>   #define CONFIG_EHCI_DCACHE
> >> 
> >> +/* string descriptors must not be fetched using a 255-byte read */
> >> +#define CONFIG_USB_STRING_FETCH
> >> 
> >>   /* include default commands */
> >>   #include<config_cmd_default.h>
Puneet Saxena - March 1, 2012, 12:59 p.m.
Hi Marek,

On Thursday 01 March 2012 05:15 PM, Marek Vasut wrote:
> Hi!
>
>> Hi Marek,
>>
>> On Thursday 01 March 2012 02:59 AM, Marek Vasut wrote:
>>>> Add "CONFIG_USB_STRING_FETCH" to fetch first string descriptor length
>>>> and then pass this length to fetch string descriptor.
>>>>
>>>> Signed-off-by: Puneet Saxena<puneets@nvidia.com>
>>>> ---
>>>>
>>>> Changes for V2:
>>>>      - Change existing config by "CONFIG_USB_STRING_FETCH"
>>>>
>>>> Changes for V3:
>>>>       - Removed extra new line
>>>>       - Explained "CONFIG_USB_STRING_FETCH" in top level README
>>>>
>>>>    README                          |    4 ++++
>>>>    common/usb.c                    |    4 ++++
>>>>    include/configs/tegra2-common.h |    2 ++
>>>>    3 files changed, 10 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/README b/README
>>>> index 7adf7c7..c045a37 100644
>>>> --- a/README
>>>> +++ b/README
>>>>
>>>> @@ -1138,6 +1138,10 @@ The following options need to be configured:
>>>>    		CONFIG_USB_EHCI_TXFIFO_THRESH enables setting of the
>>>>    		txfilltuning field in the EHCI controller on reset.
>>>>
>>>> +		CONFIG_USB_STRING_FETCH
>>>> +		Enables settings to USB core to handle string issues which
>>>> +		few devices can not handle.
>>>> +
>>>>
>>>>    - USB Device:
>>>>    		Define the below if you wish to use the USB console.
>>>>    		Once firmware is rebuilt from a serial console issue the
>>>>
>>>> diff --git a/common/usb.c b/common/usb.c
>>>> index 191bc5b..a73cb60 100644
>>>> --- a/common/usb.c
>>>> +++ b/common/usb.c
>>>> @@ -658,9 +658,13 @@ static int usb_string_sub(struct usb_device *dev,
>>>> unsigned int langid, {
>>>>
>>>>    	int rc;
>>>>
>>>> +#ifdef CONFIG_USB_STRING_FETCH
>>> Shouldn't this be something like ... CONFIG_USB_AVOID_STRING_FETCH then?
>>>
>>> Anyway, how come some devices can't handle it? What happens then? What
>>> devices are those (exact type etc)?
>>>
>>> I believe the bug is deeper and adding extra config options can be
>>> avoided, what do you think?
>>>
>>> Thanks!
>>>
>>> M
>> It does not avoid string fetch.
> Well it does certainly not call usb_get_string()
>
Precisely, it avoids fetching string desc of 255 bytes. so better name 
could be
"CONFIG_USB_AVOID_STRING_FETCH_255".  Thanks for your comment.

>> I checked with few mass storage devices that they does not handle string
>> descriptor request correctly and so we get
>> start/stop Cache alignment error.
> Cache alignment error ? Wow, how's that actually related to SUB string fetching?
> Maybe we should manually realign the result then? I still don't really
> understand what you're seeing, sorry ... can you please elaborate?
>
The particular mass storage device is -

Vendor: Kingston Rev: PMAP Prod: DataTraveler 2.0

Type: Removable Hard Disk

Capacity: 1906.0 MB = 1.8 GB (3903488 x 512)

When code tries to read Manufacturing Id..prod id...etc..it passes cache 
aligned buffer "tbuf" in
"usb_string()" @usb.c. Inside "usb_string_sub()", usb_get_string() 
passes as default 255 bytes to fetch string
descriptor.
The code in "ehci_submit_async() " invalidates *partially* the passed 
buffer though we pass aligned buffer and "STD_ASS"
is received. Though it should invalidate only the cached line which is 
equal(~32)  to string desc length.

If we give delay of 1 ms after handshake() the cache alignment warning 
does not spew...but delay of 1 ms is not acceptable.
So I enabled the code to fetch first string desc length and then fetch 
string desc fetch, in this way the issue is resolved.
It makes sense also to fetch string desc length and then actual string 
desc....

Thanks,
Puneet

   One way is, blacklist those devices by
>> vendor id/product id..
>> etc. This is done in Linux kernel. Plz see Message.c (drivers\usb\core)
>> Line: 722.
>> Blacklisting the device requires a framework to detect the
>> device...However this could be achieved
>> simply with this implementation.
> Sure, I see your intention, but I still don't get the problem, see above. Can
> you also tell me if there's particular device that's broken and which one is it
> (precise type etc)?
>
> Thanks a lot for your cooperation on this matter!
>
> M
>
>>>> +	rc = -1;
>>>> +#else
>>>>
>>>>    	/* Try to read the string descriptor by asking for the maximum
>>>>    	
>>>>    	 * possible number of bytes */
>>>>    	
>>>>    	rc = usb_get_string(dev, langid, index, buf, 255);
>>>>
>>>> +#endif
>>>>
>>>>    	/* If that failed try to read the descriptor length, then
>>>>    	
>>>>    	 * ask for just that many bytes */
>>>>
>>>> diff --git a/include/configs/tegra2-common.h
>>>> b/include/configs/tegra2-common.h index 266d0e5..d20b49c 100644
>>>> --- a/include/configs/tegra2-common.h
>>>> +++ b/include/configs/tegra2-common.h
>>>> @@ -93,6 +93,8 @@
>>>>
>>>>    #define CONFIG_USB_EHCI_TXFIFO_THRESH	10
>>>>    #define CONFIG_EHCI_IS_TDI
>>>>    #define CONFIG_EHCI_DCACHE
>>>>
>>>> +/* string descriptors must not be fetched using a 255-byte read */
>>>> +#define CONFIG_USB_STRING_FETCH
>>>>
>>>>    /* include default commands */
>>>>    #include<config_cmd_default.h>


-----------------------------------------------------------------------------------
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.
-----------------------------------------------------------------------------------
Marek Vasut - March 1, 2012, 1:13 p.m.
> Hi Marek,
> 
> On Thursday 01 March 2012 05:15 PM, Marek Vasut wrote:
> > Hi!
> > 
> >> Hi Marek,
> >> 
> >> On Thursday 01 March 2012 02:59 AM, Marek Vasut wrote:
> >>>> Add "CONFIG_USB_STRING_FETCH" to fetch first string descriptor length
> >>>> and then pass this length to fetch string descriptor.
> >>>> 
> >>>> Signed-off-by: Puneet Saxena<puneets@nvidia.com>
> >>>> ---
> >>>> 
> >>>> Changes for V2:
> >>>>      - Change existing config by "CONFIG_USB_STRING_FETCH"
> >>>> 
> >>>> Changes for V3:
> >>>>       - Removed extra new line
> >>>>       - Explained "CONFIG_USB_STRING_FETCH" in top level README
> >>>>    
> >>>>    README                          |    4 ++++
> >>>>    common/usb.c                    |    4 ++++
> >>>>    include/configs/tegra2-common.h |    2 ++
> >>>>    3 files changed, 10 insertions(+), 0 deletions(-)
> >>>> 
> >>>> diff --git a/README b/README
> >>>> index 7adf7c7..c045a37 100644
> >>>> --- a/README
> >>>> +++ b/README
> >>>> 
> >>>> @@ -1138,6 +1138,10 @@ The following options need to be configured:
> >>>>    		CONFIG_USB_EHCI_TXFIFO_THRESH enables setting of the
> >>>>    		txfilltuning field in the EHCI controller on reset.
> >>>> 
> >>>> +		CONFIG_USB_STRING_FETCH
> >>>> +		Enables settings to USB core to handle string issues 
which
> >>>> +		few devices can not handle.
> >>>> +
> >>>> 
> >>>>    - USB Device:
> >>>>    		Define the below if you wish to use the USB console.
> >>>>    		Once firmware is rebuilt from a serial console issue the
> >>>> 
> >>>> diff --git a/common/usb.c b/common/usb.c
> >>>> index 191bc5b..a73cb60 100644
> >>>> --- a/common/usb.c
> >>>> +++ b/common/usb.c
> >>>> @@ -658,9 +658,13 @@ static int usb_string_sub(struct usb_device *dev,
> >>>> unsigned int langid, {
> >>>> 
> >>>>    	int rc;
> >>>> 
> >>>> +#ifdef CONFIG_USB_STRING_FETCH
> >>> 
> >>> Shouldn't this be something like ... CONFIG_USB_AVOID_STRING_FETCH
> >>> then?
> >>> 
> >>> Anyway, how come some devices can't handle it? What happens then? What
> >>> devices are those (exact type etc)?
> >>> 
> >>> I believe the bug is deeper and adding extra config options can be
> >>> avoided, what do you think?
> >>> 
> >>> Thanks!
> >>> 
> >>> M
> >> 
> >> It does not avoid string fetch.
> > 
> > Well it does certainly not call usb_get_string()
> 
> Precisely, it avoids fetching string desc of 255 bytes. so better name
> could be
> "CONFIG_USB_AVOID_STRING_FETCH_255".  Thanks for your comment.
> 
> >> I checked with few mass storage devices that they does not handle string
> >> descriptor request correctly and so we get
> >> start/stop Cache alignment error.
> > 
> > Cache alignment error ? Wow, how's that actually related to SUB string
> > fetching? Maybe we should manually realign the result then? I still
> > don't really understand what you're seeing, sorry ... can you please
> > elaborate?
> 
> The particular mass storage device is -
> 
> Vendor: Kingston Rev: PMAP Prod: DataTraveler 2.0
> 
> Type: Removable Hard Disk
> 
> Capacity: 1906.0 MB = 1.8 GB (3903488 x 512)
> 
> When code tries to read Manufacturing Id..prod id...etc..it passes cache
> aligned buffer "tbuf" in
> "usb_string()" @usb.c. Inside "usb_string_sub()", usb_get_string()
> passes as default 255 bytes to fetch string
> descriptor.
> The code in "ehci_submit_async() " invalidates *partially* the passed
> buffer though we pass aligned buffer and "STD_ASS"
> is received. Though it should invalidate only the cached line which is
> equal(~32)  to string desc length.

Hm ... so this means the bug is in ehci_hcd? Maybe the ehci_hcd should be fixed 
to correctly handle caches then?
> 
> If we give delay of 1 ms after handshake() the cache alignment warning
> does not spew...but delay of 1 ms is not acceptable.
> So I enabled the code to fetch first string desc length and then fetch
> string desc fetch, in this way the issue is resolved.
> It makes sense also to fetch string desc length and then actual string
> desc....
> 

I see, I  understand now just about everything but the ehci problem, see above. 
Your explanation was very helpful.

Let's work this out together!

Cheers!

M
Marek Vasut - March 5, 2012, 12:48 p.m.
Dear Puneet Saxena,

> > Hi Marek,
> > 
> > On Thursday 01 March 2012 05:15 PM, Marek Vasut wrote:
> > > Hi!
> > > 
> > >> Hi Marek,
> > >> 
> > >> On Thursday 01 March 2012 02:59 AM, Marek Vasut wrote:
> > >>>> Add "CONFIG_USB_STRING_FETCH" to fetch first string descriptor
> > >>>> length and then pass this length to fetch string descriptor.
> > >>>> 
> > >>>> Signed-off-by: Puneet Saxena<puneets@nvidia.com>
> > >>>> ---
> > >>>> 
> > >>>> Changes for V2:
> > >>>>      - Change existing config by "CONFIG_USB_STRING_FETCH"
> > >>>> 
> > >>>> Changes for V3:
> > >>>>       - Removed extra new line
> > >>>>       - Explained "CONFIG_USB_STRING_FETCH" in top level README
> > >>>>    
> > >>>>    README                          |    4 ++++
> > >>>>    common/usb.c                    |    4 ++++
> > >>>>    include/configs/tegra2-common.h |    2 ++
> > >>>>    3 files changed, 10 insertions(+), 0 deletions(-)
> > >>>> 
> > >>>> diff --git a/README b/README
> > >>>> index 7adf7c7..c045a37 100644
> > >>>> --- a/README
> > >>>> +++ b/README
> > >>>> 
> > >>>> @@ -1138,6 +1138,10 @@ The following options need to be configured:
> > >>>>    		CONFIG_USB_EHCI_TXFIFO_THRESH enables setting of the
> > >>>>    		txfilltuning field in the EHCI controller on reset.
> > >>>> 
> > >>>> +		CONFIG_USB_STRING_FETCH
> > >>>> +		Enables settings to USB core to handle string issues
> 
> which
> 
> > >>>> +		few devices can not handle.
> > >>>> +
> > >>>> 
> > >>>>    - USB Device:
> > >>>>    		Define the below if you wish to use the USB console.
> > >>>>    		Once firmware is rebuilt from a serial console issue the
> > >>>> 
> > >>>> diff --git a/common/usb.c b/common/usb.c
> > >>>> index 191bc5b..a73cb60 100644
> > >>>> --- a/common/usb.c
> > >>>> +++ b/common/usb.c
> > >>>> @@ -658,9 +658,13 @@ static int usb_string_sub(struct usb_device
> > >>>> *dev, unsigned int langid, {
> > >>>> 
> > >>>>    	int rc;
> > >>>> 
> > >>>> +#ifdef CONFIG_USB_STRING_FETCH
> > >>> 
> > >>> Shouldn't this be something like ... CONFIG_USB_AVOID_STRING_FETCH
> > >>> then?
> > >>> 
> > >>> Anyway, how come some devices can't handle it? What happens then?
> > >>> What devices are those (exact type etc)?
> > >>> 
> > >>> I believe the bug is deeper and adding extra config options can be
> > >>> avoided, what do you think?
> > >>> 
> > >>> Thanks!
> > >>> 
> > >>> M
> > >> 
> > >> It does not avoid string fetch.
> > > 
> > > Well it does certainly not call usb_get_string()
> > 
> > Precisely, it avoids fetching string desc of 255 bytes. so better name
> > could be
> > "CONFIG_USB_AVOID_STRING_FETCH_255".  Thanks for your comment.
> > 
> > >> I checked with few mass storage devices that they does not handle
> > >> string descriptor request correctly and so we get
> > >> start/stop Cache alignment error.
> > > 
> > > Cache alignment error ? Wow, how's that actually related to SUB string
> > > fetching? Maybe we should manually realign the result then? I still
> > > don't really understand what you're seeing, sorry ... can you please
> > > elaborate?
> > 
> > The particular mass storage device is -
> > 
> > Vendor: Kingston Rev: PMAP Prod: DataTraveler 2.0
> > 
> > Type: Removable Hard Disk
> > 
> > Capacity: 1906.0 MB = 1.8 GB (3903488 x 512)
> > 
> > When code tries to read Manufacturing Id..prod id...etc..it passes cache
> > aligned buffer "tbuf" in
> > "usb_string()" @usb.c. Inside "usb_string_sub()", usb_get_string()
> > passes as default 255 bytes to fetch string
> > descriptor.
> > The code in "ehci_submit_async() " invalidates *partially* the passed
> > buffer though we pass aligned buffer and "STD_ASS"
> > is received. Though it should invalidate only the cached line which is
> > equal(~32)  to string desc length.
> 
> Hm ... so this means the bug is in ehci_hcd? Maybe the ehci_hcd should be
> fixed to correctly handle caches then?
> 
> > If we give delay of 1 ms after handshake() the cache alignment warning
> > does not spew...but delay of 1 ms is not acceptable.
> > So I enabled the code to fetch first string desc length and then fetch
> > string desc fetch, in this way the issue is resolved.
> > It makes sense also to fetch string desc length and then actual string
> > desc....
> 
> I see, I  understand now just about everything but the ehci problem, see
> above. Your explanation was very helpful.
> 
> Let's work this out together!
> 
> Cheers!
> 
> M

Is this issue fixed or is this patch still needed? Thanks in advance!

Best regards,
Marek Vasut
Puneet Saxena - March 5, 2012, 1:14 p.m.
Hi Marek,
On Monday 05 March 2012 06:18 PM, Marek Vasut wrote:
> Dear Puneet Saxena,
>
>>> Hi Marek,
>>>
>>> On Thursday 01 March 2012 05:15 PM, Marek Vasut wrote:
>>>> Hi!
>>>>
>>>>> Hi Marek,
>>>>>
>>>>> On Thursday 01 March 2012 02:59 AM, Marek Vasut wrote:
>>>>>>> Add "CONFIG_USB_STRING_FETCH" to fetch first string descriptor
>>>>>>> length and then pass this length to fetch string descriptor.
>>>>>>>
>>>>>>> Signed-off-by: Puneet Saxena<puneets@nvidia.com>
>>>>>>> ---
>>>>>>>
>>>>>>> Changes for V2:
>>>>>>>       - Change existing config by "CONFIG_USB_STRING_FETCH"
>>>>>>>
>>>>>>> Changes for V3:
>>>>>>>        - Removed extra new line
>>>>>>>        - Explained "CONFIG_USB_STRING_FETCH" in top level README
>>>>>>>
>>>>>>>     README                          |    4 ++++
>>>>>>>     common/usb.c                    |    4 ++++
>>>>>>>     include/configs/tegra2-common.h |    2 ++
>>>>>>>     3 files changed, 10 insertions(+), 0 deletions(-)
>>>>>>>
>>>>>>> diff --git a/README b/README
>>>>>>> index 7adf7c7..c045a37 100644
>>>>>>> --- a/README
>>>>>>> +++ b/README
>>>>>>>
>>>>>>> @@ -1138,6 +1138,10 @@ The following options need to be configured:
>>>>>>>     		CONFIG_USB_EHCI_TXFIFO_THRESH enables setting of the
>>>>>>>     		txfilltuning field in the EHCI controller on reset.
>>>>>>>
>>>>>>> +		CONFIG_USB_STRING_FETCH
>>>>>>> +		Enables settings to USB core to handle string issues
>> which
>>
>>>>>>> +		few devices can not handle.
>>>>>>> +
>>>>>>>
>>>>>>>     - USB Device:
>>>>>>>     		Define the below if you wish to use the USB console.
>>>>>>>     		Once firmware is rebuilt from a serial console issue the
>>>>>>>
>>>>>>> diff --git a/common/usb.c b/common/usb.c
>>>>>>> index 191bc5b..a73cb60 100644
>>>>>>> --- a/common/usb.c
>>>>>>> +++ b/common/usb.c
>>>>>>> @@ -658,9 +658,13 @@ static int usb_string_sub(struct usb_device
>>>>>>> *dev, unsigned int langid, {
>>>>>>>
>>>>>>>     	int rc;
>>>>>>>
>>>>>>> +#ifdef CONFIG_USB_STRING_FETCH
>>>>>> Shouldn't this be something like ... CONFIG_USB_AVOID_STRING_FETCH
>>>>>> then?
>>>>>>
>>>>>> Anyway, how come some devices can't handle it? What happens then?
>>>>>> What devices are those (exact type etc)?
>>>>>>
>>>>>> I believe the bug is deeper and adding extra config options can be
>>>>>> avoided, what do you think?
>>>>>>
>>>>>> Thanks!
>>>>>>
>>>>>> M
>>>>> It does not avoid string fetch.
>>>> Well it does certainly not call usb_get_string()
>>> Precisely, it avoids fetching string desc of 255 bytes. so better name
>>> could be
>>> "CONFIG_USB_AVOID_STRING_FETCH_255".  Thanks for your comment.
>>>
>>>>> I checked with few mass storage devices that they does not handle
>>>>> string descriptor request correctly and so we get
>>>>> start/stop Cache alignment error.
>>>> Cache alignment error ? Wow, how's that actually related to SUB string
>>>> fetching? Maybe we should manually realign the result then? I still
>>>> don't really understand what you're seeing, sorry ... can you please
>>>> elaborate?
>>> The particular mass storage device is -
>>>
>>> Vendor: Kingston Rev: PMAP Prod: DataTraveler 2.0
>>>
>>> Type: Removable Hard Disk
>>>
>>> Capacity: 1906.0 MB = 1.8 GB (3903488 x 512)
>>>
>>> When code tries to read Manufacturing Id..prod id...etc..it passes cache
>>> aligned buffer "tbuf" in
>>> "usb_string()" @usb.c. Inside "usb_string_sub()", usb_get_string()
>>> passes as default 255 bytes to fetch string
>>> descriptor.
>>> The code in "ehci_submit_async() " invalidates *partially* the passed
>>> buffer though we pass aligned buffer and "STD_ASS"
>>> is received. Though it should invalidate only the cached line which is
>>> equal(~32)  to string desc length.
>> Hm ... so this means the bug is in ehci_hcd? Maybe the ehci_hcd should be
>> fixed to correctly handle caches then?
>>
>>> If we give delay of 1 ms after handshake() the cache alignment warning
>>> does not spew...but delay of 1 ms is not acceptable.
>>> So I enabled the code to fetch first string desc length and then fetch
>>> string desc fetch, in this way the issue is resolved.
>>> It makes sense also to fetch string desc length and then actual string
>>> desc....
>> I see, I  understand now just about everything but the ehci problem, see
>> above. Your explanation was very helpful.
>>
>> Let's work this out together!
>>
>> Cheers!
>>
>> M
> Is this issue fixed or is this patch still needed? Thanks in advance!
This issue is not fixed and still need a patch to fix root cause, 
explained above.
Note that this issue is observed even though we pass aligned address and
expects something from the device.
> Best regards,
> Marek Vasut
Thanx & Regards,
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.
-----------------------------------------------------------------------------------
Marek Vasut - March 5, 2012, 9:15 p.m.
Dear Puneet Saxena,

> Hi Marek,
> 
> On Monday 05 March 2012 06:18 PM, Marek Vasut wrote:
> > Dear Puneet Saxena,
> > 
> >>> Hi Marek,
> >>> 
> >>> On Thursday 01 March 2012 05:15 PM, Marek Vasut wrote:
> >>>> Hi!
> >>>> 
> >>>>> Hi Marek,
> >>>>> 
> >>>>> On Thursday 01 March 2012 02:59 AM, Marek Vasut wrote:
> >>>>>>> Add "CONFIG_USB_STRING_FETCH" to fetch first string descriptor
> >>>>>>> length and then pass this length to fetch string descriptor.
> >>>>>>> 
> >>>>>>> Signed-off-by: Puneet Saxena<puneets@nvidia.com>
> >>>>>>> ---
> >>>>>>> 
> >>>>>>> Changes for V2:
> >>>>>>>       - Change existing config by "CONFIG_USB_STRING_FETCH"
> >>>>>>> 
> >>>>>>> Changes for V3:
> >>>>>>>        - Removed extra new line
> >>>>>>>        - Explained "CONFIG_USB_STRING_FETCH" in top level README
> >>>>>>>     
> >>>>>>>     README                          |    4 ++++
> >>>>>>>     common/usb.c                    |    4 ++++
> >>>>>>>     include/configs/tegra2-common.h |    2 ++
> >>>>>>>     3 files changed, 10 insertions(+), 0 deletions(-)
> >>>>>>> 
> >>>>>>> diff --git a/README b/README
> >>>>>>> index 7adf7c7..c045a37 100644
> >>>>>>> --- a/README
> >>>>>>> +++ b/README
> >>>>>>> 
> >>>>>>> @@ -1138,6 +1138,10 @@ The following options need to be configured:
> >>>>>>>     		CONFIG_USB_EHCI_TXFIFO_THRESH enables setting of the
> >>>>>>>     		txfilltuning field in the EHCI controller on reset.
> >>>>>>> 
> >>>>>>> +		CONFIG_USB_STRING_FETCH
> >>>>>>> +		Enables settings to USB core to handle string issues
> >> 
> >> which
> >> 
> >>>>>>> +		few devices can not handle.
> >>>>>>> +
> >>>>>>> 
> >>>>>>>     - USB Device:
> >>>>>>>     		Define the below if you wish to use the USB console.
> >>>>>>>     		Once firmware is rebuilt from a serial console issue the
> >>>>>>> 
> >>>>>>> diff --git a/common/usb.c b/common/usb.c
> >>>>>>> index 191bc5b..a73cb60 100644
> >>>>>>> --- a/common/usb.c
> >>>>>>> +++ b/common/usb.c
> >>>>>>> @@ -658,9 +658,13 @@ static int usb_string_sub(struct usb_device
> >>>>>>> *dev, unsigned int langid, {
> >>>>>>> 
> >>>>>>>     	int rc;
> >>>>>>> 
> >>>>>>> +#ifdef CONFIG_USB_STRING_FETCH
> >>>>>> 
> >>>>>> Shouldn't this be something like ... CONFIG_USB_AVOID_STRING_FETCH
> >>>>>> then?
> >>>>>> 
> >>>>>> Anyway, how come some devices can't handle it? What happens then?
> >>>>>> What devices are those (exact type etc)?
> >>>>>> 
> >>>>>> I believe the bug is deeper and adding extra config options can be
> >>>>>> avoided, what do you think?
> >>>>>> 
> >>>>>> Thanks!
> >>>>>> 
> >>>>>> M
> >>>>> 
> >>>>> It does not avoid string fetch.
> >>>> 
> >>>> Well it does certainly not call usb_get_string()
> >>> 
> >>> Precisely, it avoids fetching string desc of 255 bytes. so better name
> >>> could be
> >>> "CONFIG_USB_AVOID_STRING_FETCH_255".  Thanks for your comment.
> >>> 
> >>>>> I checked with few mass storage devices that they does not handle
> >>>>> string descriptor request correctly and so we get
> >>>>> start/stop Cache alignment error.
> >>>> 
> >>>> Cache alignment error ? Wow, how's that actually related to SUB string
> >>>> fetching? Maybe we should manually realign the result then? I still
> >>>> don't really understand what you're seeing, sorry ... can you please
> >>>> elaborate?
> >>> 
> >>> The particular mass storage device is -
> >>> 
> >>> Vendor: Kingston Rev: PMAP Prod: DataTraveler 2.0
> >>> 
> >>> Type: Removable Hard Disk
> >>> 
> >>> Capacity: 1906.0 MB = 1.8 GB (3903488 x 512)
> >>> 
> >>> When code tries to read Manufacturing Id..prod id...etc..it passes
> >>> cache aligned buffer "tbuf" in
> >>> "usb_string()" @usb.c. Inside "usb_string_sub()", usb_get_string()
> >>> passes as default 255 bytes to fetch string
> >>> descriptor.
> >>> The code in "ehci_submit_async() " invalidates *partially* the passed
> >>> buffer though we pass aligned buffer and "STD_ASS"
> >>> is received. Though it should invalidate only the cached line which is
> >>> equal(~32)  to string desc length.
> >> 
> >> Hm ... so this means the bug is in ehci_hcd? Maybe the ehci_hcd should
> >> be fixed to correctly handle caches then?
> >> 
> >>> If we give delay of 1 ms after handshake() the cache alignment warning
> >>> does not spew...but delay of 1 ms is not acceptable.
> >>> So I enabled the code to fetch first string desc length and then fetch
> >>> string desc fetch, in this way the issue is resolved.
> >>> It makes sense also to fetch string desc length and then actual string
> >>> desc....
> >> 
> >> I see, I  understand now just about everything but the ehci problem, see
> >> above. Your explanation was very helpful.
> >> 
> >> Let's work this out together!
> >> 
> >> Cheers!
> >> 
> >> M
> > 
> > Is this issue fixed or is this patch still needed? Thanks in advance!
> 
> This issue is not fixed and still need a patch to fix root cause,
> explained above.
> Note that this issue is observed even though we pass aligned address and
> expects something from the device.

I see, I finally understand the issue. Can I expect a patch from you for this 
issue then?

Thanks in advance!

Patch

diff --git a/README b/README
index 7adf7c7..c045a37 100644
--- a/README
+++ b/README
@@ -1138,6 +1138,10 @@  The following options need to be configured:
 		CONFIG_USB_EHCI_TXFIFO_THRESH enables setting of the
 		txfilltuning field in the EHCI controller on reset.
 
+		CONFIG_USB_STRING_FETCH
+		Enables settings to USB core to handle string issues which
+		few devices can not handle.
+
 - USB Device:
 		Define the below if you wish to use the USB console.
 		Once firmware is rebuilt from a serial console issue the
diff --git a/common/usb.c b/common/usb.c
index 191bc5b..a73cb60 100644
--- a/common/usb.c
+++ b/common/usb.c
@@ -658,9 +658,13 @@  static int usb_string_sub(struct usb_device *dev, unsigned int langid,
 {
 	int rc;
 
+#ifdef CONFIG_USB_STRING_FETCH
+	rc = -1;
+#else
 	/* Try to read the string descriptor by asking for the maximum
 	 * possible number of bytes */
 	rc = usb_get_string(dev, langid, index, buf, 255);
+#endif
 
 	/* If that failed try to read the descriptor length, then
 	 * ask for just that many bytes */
diff --git a/include/configs/tegra2-common.h b/include/configs/tegra2-common.h
index 266d0e5..d20b49c 100644
--- a/include/configs/tegra2-common.h
+++ b/include/configs/tegra2-common.h
@@ -93,6 +93,8 @@ 
 #define CONFIG_USB_EHCI_TXFIFO_THRESH	10
 #define CONFIG_EHCI_IS_TDI
 #define CONFIG_EHCI_DCACHE
+/* string descriptors must not be fetched using a 255-byte read */
+#define CONFIG_USB_STRING_FETCH
 
 /* include default commands */
 #include <config_cmd_default.h>