Re: [PATCH] usb: gadget: f_fs: Fix kernel panic for SuperSpeed

[Felipe Balbi]

Hi Jim,

Jim Lin <jilin@xxxxxxxxxx> writes:
> On 2016å05æ04æ 18:37, Felipe Balbi wrote:
>> * PGP Signed by an unknown key
>>
>>
>> Hi,
>>
>> Jim Lin <jilin@xxxxxxxxxx> writes:
>>
>> <snip>
>>
>>>>> In f_fs.c
>>>>> "
>>>>> static int __ffs_data_do_os_desc(enum ffs_os_desc_type type,
>>>>>                     struct usb_os_desc_header *h, void *data,
>>>>>                     unsigned len, void *priv)
>>>>> {
>>>>>        struct ffs_data *ffs = priv;
>>>>>        u8 length;
>>>>>
>>>>>        ENTER();
>>>>>
>>>>>        switch (type) {
>>>>>        case FFS_OS_DESC_EXT_COMPAT: {
>>>>>            struct usb_ext_compat_desc *d = data;
>>>>>            int i;
>>>>>
>>>>>            if (len < sizeof(*d) ||
>>>>>                d->bFirstInterfaceNumber >= ffs->interfaces_count ||
>>>>>                d->Reserved1)
>>>>>                return -EINVAL;
>>>>> "
>>>> that's fine, but this is only failing because something else is
>>>> returning the wrong set of descriptors (SS vs HS). That's the bug we
>>>> want to fix, not work around it.
>>>>
>>> Thanks.
>> you're welcome, but to fix that bug we need more information. Why is
>> composite.c using the wrong set of descriptors ? What is your setup ?
>>
>> Are you using an in-kernel gadget ? which one ?
> No, our gadget driver is on the way to submit.
>> Using configfs or legacy
>> gadgets ? gadgetfs ? f_fs ?
>
>>   How to trigger this ? Can you provide
>> instructions and (in case of gadgetfs/ffs) code to create a gadget that
>> hits this problem ?
>>
> Please refer to
> https://android.googlesource.com/platform/system/core/+/master/adb/usb_linux_client.cpp

according to this, there is a set of SuperSpeed descriptors starting on
linux 169:

https://android.googlesource.com/platform/system/core/+/master/adb/usb_linux_client.cpp#169

I don't get what the problem is. You mentioned something about SS vs HS
descriptors at some point, but that shouldn't be a problem seen that ADB
provides SS descriptors.

> Also this is a thought coming from another engineer for your reference.
> "
>
> I think Microsoft and linux are contradicting the requirements. 
> According MSFT's os descriptor definition, one of the reserved fields 
> needs to be set to 1 whereas seems like f_fs.c expects them to be 0. 
> (copy pasting from the spec downloaded from: 
> https://msdn.microsoft.com/en-us/library/windows/hardware/gg463179.aspx) 

I see..

> What does upstream think ? Requires some conflict resolution I guess !! 
> Since the OS descriptors are from MSFT, I believe upstream has to drop 
> the check and I think this patch might be valid..

If we difer from the spec, we need to remain compliant. I can see adb
sets this to a 1 as the spec requires:

https://android.googlesource.com/platform/system/core/+/master/adb/usb_linux_client.cpp#206

Now I understand the problem, it's not related to SS vs HS, it's just us
using the wrong check for Reserved1. Here's one thing though, the patch
isn't exactly correct. Instead of removing the check completely, we
*must* force the correct check. IOW:

		if (len < sizeof(*d) ||
 		    d->bFirstInterfaceNumber >= ffs->interfaces_count ||
-		    d->Reserved1)
+		    !d->Reserved1)

Heh, now your commit log makes more sense as well, but it could use some
rewording. It appears, from that commit, that the problem is writing
without SS descriptors, which it isn't. The real problem is the wrong
check of the Reserved1 field in MSFT OS Descriptor.

cheers

-- 
balbi

Attachment: signature.asc
Description: PGP signature