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

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

[Jim Lin]

Android N adds os_desc_compat in v2_descriptor by init_functionfs()
(system/core/adb/usb_linux_client.cpp) to support automatic install
of MTP driver on Windows for USB device mode.

Current __ffs_data_do_os_desc() of f_fs.c will check reserved1 field
and return -EINVAL.
This results in a second adb_write of usb_linux_client.cpp
(system/core/adb/) which doesn't have ss_descriptors filled.
Then later kernel_panic (composite.c) occurs when ss_descriptors
as a pointer with NULL is being accessed.

Fix is to ignore the checking on reserved1 field so that first
adb_write goes successfully with v2_descriptor which has
ss_descriptors filled.

Signed-off-by: Jim Lin <jilin@nvidia.com>
---
 drivers/usb/gadget/function/f_fs.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
index 73515d5..f5ea3df 100644
--- a/drivers/usb/gadget/function/f_fs.c
+++ b/drivers/usb/gadget/function/f_fs.c
@@ -2050,8 +2050,7 @@ static int __ffs_data_do_os_desc(enum ffs_os_desc_type type,
 		int i;
 
 		if (len < sizeof(*d) ||
-		    d->bFirstInterfaceNumber >= ffs->interfaces_count ||
-		    d->Reserved1)
+		    d->bFirstInterfaceNumber >= ffs->interfaces_count)
 			return -EINVAL;
 		for (i = 0; i < ARRAY_SIZE(d->Reserved2); ++i)
 			if (d->Reserved2[i])
-- 
1.9.1

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

[Lars-Peter Clausen]

On 04/22/2016 12:43 PM, Jim Lin wrote:
> Android N adds os_desc_compat in v2_descriptor by init_functionfs()
> (system/core/adb/usb_linux_client.cpp) to support automatic install
> of MTP driver on Windows for USB device mode.
> 
> Current __ffs_data_do_os_desc() of f_fs.c will check reserved1 field
> and return -EINVAL.
> This results in a second adb_write of usb_linux_client.cpp
> (system/core/adb/) which doesn't have ss_descriptors filled.
> Then later kernel_panic (composite.c) occurs when ss_descriptors
> as a pointer with NULL is being accessed.
> 
> Fix is to ignore the checking on reserved1 field so that first
> adb_write goes successfully with v2_descriptor which has
> ss_descriptors filled.

That sounds like the wrong approach. The kernel should not crash if
ss_descriptors is not filled. I think the right fix is to make sure that the
NULL pointer deref can never happen regardless of which input is supplied by
userspace.

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

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2716 bytes --]


Hi Jim,

Jim Lin <jilin@nvidia.com> writes:
> Android N adds os_desc_compat in v2_descriptor by init_functionfs()
> (system/core/adb/usb_linux_client.cpp) to support automatic install
> of MTP driver on Windows for USB device mode.
>
> Current __ffs_data_do_os_desc() of f_fs.c will check reserved1 field
> and return -EINVAL.
> This results in a second adb_write of usb_linux_client.cpp
> (system/core/adb/) which doesn't have ss_descriptors filled.
> Then later kernel_panic (composite.c) occurs when ss_descriptors
> as a pointer with NULL is being accessed.

where exactly in composite.c are we dereferencing a NULL pointer ?

Is this happening on set_config() ? If that's the case, why is
gadget->speed set to USB_SPEED_SUPER to start with ? Your controller
should already have negotiated highspeed which means
function_descriptors() should have returned highspeed descriptors, not a
NULL superspeed.

Care to explain why you haven't negotiated Highspeed ? The only thing I
can think of is that you're using a Superspeed-capable peripheral
controller (dwc3?) with maximum-speed set to Superspeed, with a
Superspeed-capable cable connected to an XHCI PC, but loading a
high-speed gadget driver (which you got from Android, written with f_fs)
and this gadget doesn't tell composite that its maximum speed is
Highspeed, instead of super-speed.

We can add a check, sure, to avoid a kernel oops; however, you should
really fix up the gadget implementation and/or set dwc3's maximum-speed
property accordingly.

Can you check if this patch makes your scenario work while still being
fully functional ?

diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
index de9ffd60fcfa..3d3cdc5ed20d 100644
--- a/drivers/usb/gadget/composite.c
+++ b/drivers/usb/gadget/composite.c
@@ -66,20 +66,36 @@ function_descriptors(struct usb_function *f,
 {
 	struct usb_descriptor_header **descriptors;
 
+	/*
+	 * NOTE: we try to help gadget drivers which might not be setting
+	 * max_speed appropriately.
+	 */
+
 	switch (speed) {
 	case USB_SPEED_SUPER_PLUS:
 		descriptors = f->ssp_descriptors;
-		break;
+		if (descriptors)
+			break;
+		/* FALLTHROUGH */
 	case USB_SPEED_SUPER:
 		descriptors = f->ss_descriptors;
-		break;
+		if (descriptors)
+			break;
+		/* FALLTHROUGH */
 	case USB_SPEED_HIGH:
 		descriptors = f->hs_descriptors;
-		break;
+		if (descriptors)
+			break;
+		/* FALLTHROUGH */
 	default:
 		descriptors = f->fs_descriptors;
 	}
 
+	/*
+	 * if we can't find any descriptors at all, then this gadget deserves to
+	 * Oops with a NULL pointer dereference
+	 */
+
 	return descriptors;
 }
 
-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

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

[Jim Lin]

On 2016年04月22日 19:52, Felipe Balbi wrote:
> * PGP Signed by an unknown key
>
>
> Hi Jim,
>
> Jim Lin <jilin@nvidia.com> writes:
>> Android N adds os_desc_compat in v2_descriptor by init_functionfs()
>> (system/core/adb/usb_linux_client.cpp) to support automatic install
>> of MTP driver on Windows for USB device mode.
>>
>> Current __ffs_data_do_os_desc() of f_fs.c will check reserved1 field
>> and return -EINVAL.
>> This results in a second adb_write of usb_linux_client.cpp
>> (system/core/adb/) which doesn't have ss_descriptors filled.
>> Then later kernel_panic (composite.c) occurs when ss_descriptors
>> as a pointer with NULL is being accessed.
> where exactly in composite.c are we dereferencing a NULL pointer ?
In set_config

for (; *descriptors; ++descriptors) {



Here is the link which shows reserved (at offset 1, e.g., Function 
Section 2) as  1.
https://msdn.microsoft.com/en-us/library/windows/hardware/gg463179.aspx


>
> Is this happening on set_config() ? If that's the case, why is
> gadget->speed set to USB_SPEED_SUPER to start with ? Your controller
> should already have negotiated highspeed which means
> function_descriptors() should have returned highspeed descriptors, not a
> NULL superspeed.
>
> Care to explain why you haven't negotiated Highspeed ? The only thing I
> can think of is that you're using a Superspeed-capable peripheral
> controller (dwc3?) with maximum-speed set to Superspeed, with a
> Superspeed-capable cable connected to an XHCI PC, but loading a
> high-speed gadget driver (which you got from Android, written with f_fs)
> and this gadget doesn't tell composite that its maximum speed is
> Highspeed, instead of super-speed.
>
> We can add a check, sure, to avoid a kernel oops; however, you should
> really fix up the gadget implementation and/or set dwc3's maximum-speed
> property accordingly.
>
> Can you check if this patch makes your scenario work while still being
> fully functional ?
>
> diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
> index de9ffd60fcfa..3d3cdc5ed20d 100644
> --- a/drivers/usb/gadget/composite.c
> +++ b/drivers/usb/gadget/composite.c
> @@ -66,20 +66,36 @@ function_descriptors(struct usb_function *f,
>   {
>   	struct usb_descriptor_header **descriptors;
>   
> +	/*
> +	 * NOTE: we try to help gadget drivers which might not be setting
> +	 * max_speed appropriately.
> +	 */
> +
>   	switch (speed) {
>   	case USB_SPEED_SUPER_PLUS:
>   		descriptors = f->ssp_descriptors;
> -		break;
> +		if (descriptors)
> +			break;
> +		/* FALLTHROUGH */
>   	case USB_SPEED_SUPER:
>   		descriptors = f->ss_descriptors;
> -		break;
> +		if (descriptors)
> +			break;
> +		/* FALLTHROUGH */
>   	case USB_SPEED_HIGH:
>   		descriptors = f->hs_descriptors;
> -		break;
> +		if (descriptors)
> +			break;
> +		/* FALLTHROUGH */
>   	default:
>   		descriptors = f->fs_descriptors;
>   	}
>   
> +	/*
> +	 * if we can't find any descriptors at all, then this gadget deserves to
> +	 * Oops with a NULL pointer dereference
> +	 */
> +
>   	return descriptors;
>   }
>   
After trying your change, no kernel panic, but SuperSpeed device (device 
mode) is not enumerated by ubuntu 14.04 USB 3.0 host controller as MTP 
device with USB3.0 cable.

--nvpublic

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

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 3939 bytes --]


Hi,

Jim Lin <jilin@nvidia.com> writes:
> On 2016年04月22日 19:52, Felipe Balbi wrote:
>> * PGP Signed by an unknown key
>>
>>
>> Hi Jim,
>>
>> Jim Lin <jilin@nvidia.com> writes:
>>> Android N adds os_desc_compat in v2_descriptor by init_functionfs()
>>> (system/core/adb/usb_linux_client.cpp) to support automatic install
>>> of MTP driver on Windows for USB device mode.
>>>
>>> Current __ffs_data_do_os_desc() of f_fs.c will check reserved1 field
>>> and return -EINVAL.
>>> This results in a second adb_write of usb_linux_client.cpp
>>> (system/core/adb/) which doesn't have ss_descriptors filled.
>>> Then later kernel_panic (composite.c) occurs when ss_descriptors
>>> as a pointer with NULL is being accessed.
>> where exactly in composite.c are we dereferencing a NULL pointer ?
> In set_config
>
> for (; *descriptors; ++descriptors) {
>
>
>
> Here is the link which shows reserved (at offset 1, e.g., Function 
> Section 2) as  1.
> https://msdn.microsoft.com/en-us/library/windows/hardware/gg463179.aspx

thanks

>> Is this happening on set_config() ? If that's the case, why is
>> gadget->speed set to USB_SPEED_SUPER to start with ? Your controller
>> should already have negotiated highspeed which means
>> function_descriptors() should have returned highspeed descriptors, not a
>> NULL superspeed.
>>
>> Care to explain why you haven't negotiated Highspeed ? The only thing I
>> can think of is that you're using a Superspeed-capable peripheral
>> controller (dwc3?) with maximum-speed set to Superspeed, with a
>> Superspeed-capable cable connected to an XHCI PC, but loading a
>> high-speed gadget driver (which you got from Android, written with f_fs)
>> and this gadget doesn't tell composite that its maximum speed is
>> Highspeed, instead of super-speed.
>>
>> We can add a check, sure, to avoid a kernel oops; however, you should
>> really fix up the gadget implementation and/or set dwc3's maximum-speed
>> property accordingly.
>>
>> Can you check if this patch makes your scenario work while still being
>> fully functional ?
>>
>> diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
>> index de9ffd60fcfa..3d3cdc5ed20d 100644
>> --- a/drivers/usb/gadget/composite.c
>> +++ b/drivers/usb/gadget/composite.c
>> @@ -66,20 +66,36 @@ function_descriptors(struct usb_function *f,
>>   {
>>   	struct usb_descriptor_header **descriptors;
>>   
>> +	/*
>> +	 * NOTE: we try to help gadget drivers which might not be setting
>> +	 * max_speed appropriately.
>> +	 */
>> +
>>   	switch (speed) {
>>   	case USB_SPEED_SUPER_PLUS:
>>   		descriptors = f->ssp_descriptors;
>> -		break;
>> +		if (descriptors)
>> +			break;
>> +		/* FALLTHROUGH */
>>   	case USB_SPEED_SUPER:
>>   		descriptors = f->ss_descriptors;
>> -		break;
>> +		if (descriptors)
>> +			break;
>> +		/* FALLTHROUGH */
>>   	case USB_SPEED_HIGH:
>>   		descriptors = f->hs_descriptors;
>> -		break;
>> +		if (descriptors)
>> +			break;
>> +		/* FALLTHROUGH */
>>   	default:
>>   		descriptors = f->fs_descriptors;
>>   	}
>>   
>> +	/*
>> +	 * if we can't find any descriptors at all, then this gadget deserves to
>> +	 * Oops with a NULL pointer dereference
>> +	 */
>> +
>>   	return descriptors;
>>   }
>>   
> After trying your change, no kernel panic, but SuperSpeed device (device 
> mode) is not enumerated by ubuntu 14.04 USB 3.0 host controller as MTP 
> device with USB3.0 cable.

what do you get on dmesg on host side ? Are you running dwc3 ? If you
are, please capture trace logs of the failure:

# mount -t debugfs none /sys/kernel/debug
# cd /sys/kernel/debug/tracing
# echo 2048 > buffer_size_kb
# echo 1 > events/dwc3/enable

(now connect your cable to host pc)

# cp trace /path/to/non-volatile/storage/trace.txt

Please reply with this trace.txt file and dmesg from host side.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

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

[Jim Lin]

On 2016年04月25日 20:01, Felipe Balbi wrote:
> * PGP Signed by an unknown key
>
>
>
>
>>> Is this happening on set_config() ? If that's the case, why is
>>> gadget->speed set to USB_SPEED_SUPER to start with ? Your controller
>>> should already have negotiated highspeed which means
>>> function_descriptors() should have returned highspeed descriptors, not a
>>> NULL superspeed.
>>>
>>> Care to explain why you haven't negotiated Highspeed ? The only thing I
>>> can think of is that you're using a Superspeed-capable peripheral
>>> controller (dwc3?) with maximum-speed set to Superspeed, with a
>>> Superspeed-capable cable connected to an XHCI PC, but loading a
>>> high-speed gadget driver (which you got from Android, written with f_fs)
>>> and this gadget doesn't tell composite that its maximum speed is
>>> Highspeed, instead of super-speed.
>>>
>>> We can add a check, sure, to avoid a kernel oops; however, you should
>>> really fix up the gadget implementation and/or set dwc3's maximum-speed
>>> property accordingly.
>>>
>>> Can you check if this patch makes your scenario work while still being
>>> fully functional ?
>>>
>>> diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
>>> index de9ffd60fcfa..3d3cdc5ed20d 100644
>>> --- a/drivers/usb/gadget/composite.c
>>> +++ b/drivers/usb/gadget/composite.c
>>> @@ -66,20 +66,36 @@ function_descriptors(struct usb_function *f,
>>>    {
>>>    	struct usb_descriptor_header **descriptors;
>>>    
>>> +	/*
>>> +	 * NOTE: we try to help gadget drivers which might not be setting
>>> +	 * max_speed appropriately.
>>> +	 */
>>> +
>>>    	switch (speed) {
>>>    	case USB_SPEED_SUPER_PLUS:
>>>    		descriptors = f->ssp_descriptors;
>>> -		break;
>>> +		if (descriptors)
>>> +			break;
>>> +		/* FALLTHROUGH */
>>>    	case USB_SPEED_SUPER:
>>>    		descriptors = f->ss_descriptors;
>>> -		break;
>>> +		if (descriptors)
>>> +			break;
>>> +		/* FALLTHROUGH */
>>>    	case USB_SPEED_HIGH:
>>>    		descriptors = f->hs_descriptors;
>>> -		break;
>>> +		if (descriptors)
>>> +			break;
>>> +		/* FALLTHROUGH */
>>>    	default:
>>>    		descriptors = f->fs_descriptors;
>>>    	}
>>>    
>>> +	/*
>>> +	 * if we can't find any descriptors at all, then this gadget deserves to
>>> +	 * Oops with a NULL pointer dereference
>>> +	 */
>>> +
>>>    	return descriptors;
>>>    }
>>>    
>> After trying your change, no kernel panic, but SuperSpeed device (device
>> mode) is not enumerated by ubuntu 14.04 USB 3.0 host controller as MTP
>> device with USB3.0 cable.
> what do you get on dmesg on host side ? Are you running dwc3 ? If you
> are, please capture trace logs of the failure:
>
> # mount -t debugfs none /sys/kernel/debug
> # cd /sys/kernel/debug/tracing
> # echo 2048 > buffer_size_kb
> # echo 1 > events/dwc3/enable
>
> (now connect your cable to host pc)
>
> # cp trace /path/to/non-volatile/storage/trace.txt
>
> Please reply with this trace.txt file and dmesg from host side.
This is not running with dwc3.

dmesg from PC host side (after adding your change without my patch):

[17907.984647] usb 6-2: new SuperSpeed USB device number 54 using xhci_hcd
[17908.012036] usb 6-2: No SuperSpeed endpoint companion for config 1  
interface 1 altsetting 0 ep 2: using minimum values
[17908.012040] usb 6-2: No SuperSpeed endpoint companion for config 1  
interface 1 altsetting 0 ep 131: using minimum values
[17908.013652] usb 6-2: New USB device found, idVendor=xxxx, idProduct=xxxx
[17908.013656] usb 6-2: New USB device strings: Mfr=1, Product=2, 
SerialNumber=3
[17908.013658] usb 6-2: Product: xxxxxxx
[17908.013661] usb 6-2: Manufacturer: xxxxxx
[17908.013664] usb 6-2: SerialNumber: 1234567890
[17908.014680] xhci_hcd 0000:05:00.0: ERROR: unexpected command 
completion code 0x11.
[17908.014690] usb 6-2: can't set config #1, error -22

I also attach git log of system/core/adb/usb_linux_client.cpp of Android 
N for your reference.
"
Author: Badhri Jagan Sridharan <Badhri@google.com>
Date:   Mon Oct 5 13:04:03 2015 -0700

     adbd: Add os descriptor support for adb.

     Eventhough windows does not rely on extended os
     descriptor for adbd, when android usb device is
     configures as a composite device such as mtp+adb,
     windows discards the extended os descriptor even
     if one of the USB function fails to send
     the extended compat descriptor. This results in automatic
     install of MTP driverto fail when Android device is in
     "File Transfer" mode with adb enabled.

https://msdn.microsoft.com/en-us/library/windows/hardware/gg463179.aspx
"

--nvpublic

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

[Jim Lin]

On 2016年04月26日 16:49, Jim Lin wrote:
> On 2016年04月25日 20:01, Felipe Balbi wrote:
>> * PGP Signed by an unknown key
>>
>>
>>
>>
>>>> Is this happening on set_config() ? If that's the case, why is
>>>> gadget->speed set to USB_SPEED_SUPER to start with ? Your controller
>>>> should already have negotiated highspeed which means
>>>> function_descriptors() should have returned highspeed descriptors, 
>>>> not a
>>>> NULL superspeed.
>>>>
>>>> Care to explain why you haven't negotiated Highspeed ? The only 
>>>> thing I
>>>> can think of is that you're using a Superspeed-capable peripheral
>>>> controller (dwc3?) with maximum-speed set to Superspeed, with a
>>>> Superspeed-capable cable connected to an XHCI PC, but loading a
>>>> high-speed gadget driver (which you got from Android, written with 
>>>> f_fs)
>>>> and this gadget doesn't tell composite that its maximum speed is
>>>> Highspeed, instead of super-speed.
>>>>
>>>> We can add a check, sure, to avoid a kernel oops; however, you should
>>>> really fix up the gadget implementation and/or set dwc3's 
>>>> maximum-speed
>>>> property accordingly.
>>>>
>>>> Can you check if this patch makes your scenario work while still being
>>>> fully functional ?
>>>>
>>>> diff --git a/drivers/usb/gadget/composite.c 
>>>> b/drivers/usb/gadget/composite.c
>>>> index de9ffd60fcfa..3d3cdc5ed20d 100644
>>>> --- a/drivers/usb/gadget/composite.c
>>>> +++ b/drivers/usb/gadget/composite.c
>>>> @@ -66,20 +66,36 @@ function_descriptors(struct usb_function *f,
>>>>    {
>>>>        struct usb_descriptor_header **descriptors;
>>>>    +    /*
>>>> +     * NOTE: we try to help gadget drivers which might not be setting
>>>> +     * max_speed appropriately.
>>>> +     */
>>>> +
>>>>        switch (speed) {
>>>>        case USB_SPEED_SUPER_PLUS:
>>>>            descriptors = f->ssp_descriptors;
>>>> -        break;
>>>> +        if (descriptors)
>>>> +            break;
>>>> +        /* FALLTHROUGH */
>>>>        case USB_SPEED_SUPER:
>>>>            descriptors = f->ss_descriptors;
>>>> -        break;
>>>> +        if (descriptors)
>>>> +            break;
>>>> +        /* FALLTHROUGH */
>>>>        case USB_SPEED_HIGH:
>>>>            descriptors = f->hs_descriptors;
>>>> -        break;
>>>> +        if (descriptors)
>>>> +            break;
>>>> +        /* FALLTHROUGH */
>>>>        default:
>>>>            descriptors = f->fs_descriptors;
>>>>        }
>>>>    +    /*
>>>> +     * if we can't find any descriptors at all, then this gadget 
>>>> deserves to
>>>> +     * Oops with a NULL pointer dereference
>>>> +     */
>>>> +
>>>>        return descriptors;
>>>>    }
>>> After trying your change, no kernel panic, but SuperSpeed device 
>>> (device
>>> mode) is not enumerated by ubuntu 14.04 USB 3.0 host controller as MTP
>>> device with USB3.0 cable.
>> what do you get on dmesg on host side ? Are you running dwc3 ? If you
>> are, please capture trace logs of the failure:
>>
>> # mount -t debugfs none /sys/kernel/debug
>> # cd /sys/kernel/debug/tracing
>> # echo 2048 > buffer_size_kb
>> # echo 1 > events/dwc3/enable
>>
>> (now connect your cable to host pc)
>>
>> # cp trace /path/to/non-volatile/storage/trace.txt
>>
>> Please reply with this trace.txt file and dmesg from host side.
> This is not running with dwc3.
>
> dmesg from PC host side (after adding your change without my patch):
>
> [17907.984647] usb 6-2: new SuperSpeed USB device number 54 using 
> xhci_hcd
> [17908.012036] usb 6-2: No SuperSpeed endpoint companion for config 1  
> interface 1 altsetting 0 ep 2: using minimum values
> [17908.012040] usb 6-2: No SuperSpeed endpoint companion for config 1  
> interface 1 altsetting 0 ep 131: using minimum values
> [17908.013652] usb 6-2: New USB device found, idVendor=xxxx, 
> idProduct=xxxx
> [17908.013656] usb 6-2: New USB device strings: Mfr=1, Product=2, 
> SerialNumber=3
> [17908.013658] usb 6-2: Product: xxxxxxx
> [17908.013661] usb 6-2: Manufacturer: xxxxxx
> [17908.013664] usb 6-2: SerialNumber: 1234567890
> [17908.014680] xhci_hcd 0000:05:00.0: ERROR: unexpected command 
> completion code 0x11.
> [17908.014690] usb 6-2: can't set config #1, error -22
>
> I also attach git log of system/core/adb/usb_linux_client.cpp of 
> Android N for your reference.
> "
> Author: Badhri Jagan Sridharan <Badhri@google.com>
> Date:   Mon Oct 5 13:04:03 2015 -0700
>
>     adbd: Add os descriptor support for adb.
>
>     Eventhough windows does not rely on extended os
>     descriptor for adbd, when android usb device is
>     configures as a composite device such as mtp+adb,
>     windows discards the extended os descriptor even
>     if one of the USB function fails to send
>     the extended compat descriptor. This results in automatic
>     install of MTP driverto fail when Android device is in
>     "File Transfer" mode with adb enabled.
>
> https://msdn.microsoft.com/en-us/library/windows/hardware/gg463179.aspx
> "
>
How do we proceed on this patch?
  [PATCH] usb: gadget: f_fs: Fix kernel panic for SuperSpeed

Thanks,
--nvpublic

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

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 5047 bytes --]


Hi,

Jim Lin <jilin@nvidia.com> writes:
> On 2016年04月25日 20:01, Felipe Balbi wrote:
>>>> Is this happening on set_config() ? If that's the case, why is
>>>> gadget->speed set to USB_SPEED_SUPER to start with ? Your controller
>>>> should already have negotiated highspeed which means
>>>> function_descriptors() should have returned highspeed descriptors, not a
>>>> NULL superspeed.
>>>>
>>>> Care to explain why you haven't negotiated Highspeed ? The only thing I
>>>> can think of is that you're using a Superspeed-capable peripheral
>>>> controller (dwc3?) with maximum-speed set to Superspeed, with a
>>>> Superspeed-capable cable connected to an XHCI PC, but loading a
>>>> high-speed gadget driver (which you got from Android, written with f_fs)
>>>> and this gadget doesn't tell composite that its maximum speed is
>>>> Highspeed, instead of super-speed.
>>>>
>>>> We can add a check, sure, to avoid a kernel oops; however, you should
>>>> really fix up the gadget implementation and/or set dwc3's maximum-speed
>>>> property accordingly.
>>>>
>>>> Can you check if this patch makes your scenario work while still being
>>>> fully functional ?
>>>>
>>>> diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
>>>> index de9ffd60fcfa..3d3cdc5ed20d 100644
>>>> --- a/drivers/usb/gadget/composite.c
>>>> +++ b/drivers/usb/gadget/composite.c
>>>> @@ -66,20 +66,36 @@ function_descriptors(struct usb_function *f,
>>>>    {
>>>>    	struct usb_descriptor_header **descriptors;
>>>>    
>>>> +	/*
>>>> +	 * NOTE: we try to help gadget drivers which might not be setting
>>>> +	 * max_speed appropriately.
>>>> +	 */
>>>> +
>>>>    	switch (speed) {
>>>>    	case USB_SPEED_SUPER_PLUS:
>>>>    		descriptors = f->ssp_descriptors;
>>>> -		break;
>>>> +		if (descriptors)
>>>> +			break;
>>>> +		/* FALLTHROUGH */
>>>>    	case USB_SPEED_SUPER:
>>>>    		descriptors = f->ss_descriptors;
>>>> -		break;
>>>> +		if (descriptors)
>>>> +			break;
>>>> +		/* FALLTHROUGH */
>>>>    	case USB_SPEED_HIGH:
>>>>    		descriptors = f->hs_descriptors;
>>>> -		break;
>>>> +		if (descriptors)
>>>> +			break;
>>>> +		/* FALLTHROUGH */
>>>>    	default:
>>>>    		descriptors = f->fs_descriptors;
>>>>    	}
>>>>    
>>>> +	/*
>>>> +	 * if we can't find any descriptors at all, then this gadget deserves to
>>>> +	 * Oops with a NULL pointer dereference
>>>> +	 */
>>>> +
>>>>    	return descriptors;
>>>>    }
>>>>    
>>> After trying your change, no kernel panic, but SuperSpeed device (device
>>> mode) is not enumerated by ubuntu 14.04 USB 3.0 host controller as MTP
>>> device with USB3.0 cable.
>> what do you get on dmesg on host side ? Are you running dwc3 ? If you
>> are, please capture trace logs of the failure:
>>
>> # mount -t debugfs none /sys/kernel/debug
>> # cd /sys/kernel/debug/tracing
>> # echo 2048 > buffer_size_kb
>> # echo 1 > events/dwc3/enable
>>
>> (now connect your cable to host pc)
>>
>> # cp trace /path/to/non-volatile/storage/trace.txt
>>
>> Please reply with this trace.txt file and dmesg from host side.
> This is not running with dwc3.

okay

> dmesg from PC host side (after adding your change without my patch):
>
> [17907.984647] usb 6-2: new SuperSpeed USB device number 54 using xhci_hcd
> [17908.012036] usb 6-2: No SuperSpeed endpoint companion for config 1  
> interface 1 altsetting 0 ep 2: using minimum values
> [17908.012040] usb 6-2: No SuperSpeed endpoint companion for config 1  
> interface 1 altsetting 0 ep 131: using minimum values
> [17908.013652] usb 6-2: New USB device found, idVendor=xxxx, idProduct=xxxx
> [17908.013656] usb 6-2: New USB device strings: Mfr=1, Product=2, 
> SerialNumber=3
> [17908.013658] usb 6-2: Product: xxxxxxx
> [17908.013661] usb 6-2: Manufacturer: xxxxxx
> [17908.013664] usb 6-2: SerialNumber: 1234567890
> [17908.014680] xhci_hcd 0000:05:00.0: ERROR: unexpected command 
> completion code 0x11.

hmmm... completed with unknown code ? Odd. Mathias, any idea what this
is ?

> [17908.014690] usb 6-2: can't set config #1, error -22
>
> I also attach git log of system/core/adb/usb_linux_client.cpp of Android 
> N for your reference.
> "
> Author: Badhri Jagan Sridharan <Badhri@google.com>
> Date:   Mon Oct 5 13:04:03 2015 -0700
>
>      adbd: Add os descriptor support for adb.
>
>      Eventhough windows does not rely on extended os
>      descriptor for adbd, when android usb device is
>      configures as a composite device such as mtp+adb,
>      windows discards the extended os descriptor even
>      if one of the USB function fails to send
>      the extended compat descriptor. This results in automatic
>      install of MTP driverto fail when Android device is in
>      "File Transfer" mode with adb enabled.
>
> https://msdn.microsoft.com/en-us/library/windows/hardware/gg463179.aspx
> "

Okay, cool. Can you check that you're limitting your controller's speed
to high-speed ?

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

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

[Jim Lin]

On 2016年04月28日 20:21, Felipe Balbi wrote:
>>
>> I also attach git log of system/core/adb/usb_linux_client.cpp of Android
>> N for your reference.
>> "
>> Author: Badhri Jagan Sridharan <Badhri@google.com>
>> Date:   Mon Oct 5 13:04:03 2015 -0700
>>
>>       adbd: Add os descriptor support for adb.
>>
>>       Eventhough windows does not rely on extended os
>>       descriptor for adbd, when android usb device is
>>       configures as a composite device such as mtp+adb,
>>       windows discards the extended os descriptor even
>>       if one of the USB function fails to send
>>       the extended compat descriptor. This results in automatic
>>       install of MTP driverto fail when Android device is in
>>       "File Transfer" mode with adb enabled.
>>
>> https://msdn.microsoft.com/en-us/library/windows/hardware/gg463179.aspx
>> "
> Okay, cool. Can you check that you're limitting your controller's speed
> to high-speed ?
>
Let's focus on original patch.
Could you help to explain why we need below d->Reserved1 checking?
Now the question is that

https://msdn.microsoft.com/en-us/library/windows/hardware/gg463179.aspx

Page 7 of OS_Desc_CompatID.doc
defines reserved field to be 1 and
below code will think that os_desc is invalid because d->Reserved1 is 1.


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;
"

--nvpublic

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

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2174 bytes --]


Hi,

Jim Lin <jilin@nvidia.com> writes:
> On 2016年04月28日 20:21, Felipe Balbi wrote:
>>>
>>> I also attach git log of system/core/adb/usb_linux_client.cpp of Android
>>> N for your reference.
>>> "
>>> Author: Badhri Jagan Sridharan <Badhri@google.com>
>>> Date:   Mon Oct 5 13:04:03 2015 -0700
>>>
>>>       adbd: Add os descriptor support for adb.
>>>
>>>       Eventhough windows does not rely on extended os
>>>       descriptor for adbd, when android usb device is
>>>       configures as a composite device such as mtp+adb,
>>>       windows discards the extended os descriptor even
>>>       if one of the USB function fails to send
>>>       the extended compat descriptor. This results in automatic
>>>       install of MTP driverto fail when Android device is in
>>>       "File Transfer" mode with adb enabled.
>>>
>>> https://msdn.microsoft.com/en-us/library/windows/hardware/gg463179.aspx
>>> "
>> Okay, cool. Can you check that you're limitting your controller's speed
>> to high-speed ?
>>
> Let's focus on original patch.
> Could you help to explain why we need below d->Reserved1 checking?
> Now the question is that
>
> https://msdn.microsoft.com/en-us/library/windows/hardware/gg463179.aspx
>
> Page 7 of OS_Desc_CompatID.doc
> defines reserved field to be 1 and
> below code will think that os_desc is invalid because d->Reserved1 is 1.
>
>
> 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.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

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

[Mathias Nyman]

On 28.04.2016 15:21, Felipe Balbi wrote:
>
> Hi,
>
>> dmesg from PC host side (after adding your change without my patch):
>>
>> [17907.984647] usb 6-2: new SuperSpeed USB device number 54 using xhci_hcd
>> [17908.012036] usb 6-2: No SuperSpeed endpoint companion for config 1
>> interface 1 altsetting 0 ep 2: using minimum values
>> [17908.012040] usb 6-2: No SuperSpeed endpoint companion for config 1
>> interface 1 altsetting 0 ep 131: using minimum values
>> [17908.013652] usb 6-2: New USB device found, idVendor=xxxx, idProduct=xxxx
>> [17908.013656] usb 6-2: New USB device strings: Mfr=1, Product=2,
>> SerialNumber=3
>> [17908.013658] usb 6-2: Product: xxxxxxx
>> [17908.013661] usb 6-2: Manufacturer: xxxxxx
>> [17908.013664] usb 6-2: SerialNumber: 1234567890
>> [17908.014680] xhci_hcd 0000:05:00.0: ERROR: unexpected command
>> completion code 0x11.
>
> hmmm... completed with unknown code ? Odd. Mathias, any idea what this
> is ?
>

Parameter error, xhci doesn't like one of the values in one of the contexts we
give it (slot context, endpoint context etc)

-Mathias

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

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1166 bytes --]


Hi,

Mathias Nyman <mathias.nyman@linux.intel.com> writes:
>>> dmesg from PC host side (after adding your change without my patch):
>>>
>>> [17907.984647] usb 6-2: new SuperSpeed USB device number 54 using xhci_hcd
>>> [17908.012036] usb 6-2: No SuperSpeed endpoint companion for config 1
>>> interface 1 altsetting 0 ep 2: using minimum values
>>> [17908.012040] usb 6-2: No SuperSpeed endpoint companion for config 1
>>> interface 1 altsetting 0 ep 131: using minimum values
>>> [17908.013652] usb 6-2: New USB device found, idVendor=xxxx, idProduct=xxxx
>>> [17908.013656] usb 6-2: New USB device strings: Mfr=1, Product=2,
>>> SerialNumber=3
>>> [17908.013658] usb 6-2: Product: xxxxxxx
>>> [17908.013661] usb 6-2: Manufacturer: xxxxxx
>>> [17908.013664] usb 6-2: SerialNumber: 1234567890
>>> [17908.014680] xhci_hcd 0000:05:00.0: ERROR: unexpected command
>>> completion code 0x11.
>>
>> hmmm... completed with unknown code ? Odd. Mathias, any idea what this
>> is ?
>>
>
> Parameter error, xhci doesn't like one of the values in one of the contexts we
> give it (slot context, endpoint context etc)

okay, thanks :-)

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

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

[Jim Lin]

On 2016年04月29日 19:57, Felipe Balbi wrote:
> * PGP Signed by an unknown key
>
>
> Hi,
>
> Jim Lin <jilin@nvidia.com> writes:
>> On 2016年04月28日 20:21, Felipe Balbi wrote:
>>>> I also attach git log of system/core/adb/usb_linux_client.cpp of Android
>>>> N for your reference.
>>>> "
>>>> Author: Badhri Jagan Sridharan <Badhri@google.com>
>>>> Date:   Mon Oct 5 13:04:03 2015 -0700
>>>>
>>>>        adbd: Add os descriptor support for adb.
>>>>
>>>>        Eventhough windows does not rely on extended os
>>>>        descriptor for adbd, when android usb device is
>>>>        configures as a composite device such as mtp+adb,
>>>>        windows discards the extended os descriptor even
>>>>        if one of the USB function fails to send
>>>>        the extended compat descriptor. This results in automatic
>>>>        install of MTP driverto fail when Android device is in
>>>>        "File Transfer" mode with adb enabled.
>>>>
>>>> https://msdn.microsoft.com/en-us/library/windows/hardware/gg463179.aspx
>>>> "
>>> Okay, cool. Can you check that you're limitting your controller's speed
>>> to high-speed ?
>>>
>> Let's focus on original patch.
>> Could you help to explain why we need below d->Reserved1 checking?
>> Now the question is that
>>
>> https://msdn.microsoft.com/en-us/library/windows/hardware/gg463179.aspx
>>
>> Page 7 of OS_Desc_CompatID.doc
>> defines reserved field to be 1 and
>> below code will think that os_desc is invalid because d->Reserved1 is 1.
>>
>>
>> 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.

--nvpublic

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

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1282 bytes --]


Hi,

Jim Lin <jilin@nvidia.com> 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 ? 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 ?

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

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

[Jim Lin]

On 2016年05月04日 18:37, Felipe Balbi wrote:
> * PGP Signed by an unknown key
>
>
> Hi,
>
> Jim Lin <jilin@nvidia.com> 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
https://android.googlesource.com/device/google/dragon/+/android-6.0.1_r4/init.dragon.usb.rc
https://android.googlesource.com/platform/system/core/+/master/rootdir/init.usb.configfs.rc


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

bFirstInterfaceNumber This field specifies the interface or function 
that is associated with the IDs in this section. To use this function 
section to associate a single-function group of interfaces with a single 
pair of IDs, set bFirstInterfaceNumber to the first interface in the 
group. Then use an IAD in that interface’s interface descriptor to 
specify which additional interfaces should be included in the group. The 
interfaces in the group must be consecutively numbered. For details, see 
“Support for USB Interface Association Descriptor in Windows.”

RESERVED Reserved for system use. Set this value to 0x01.

compatibleID This field contains the value of the compatible ID to be 
associated with this function. Any unused bytes should be filled with 
NULLs. If the function does not have a compatible ID, fill the entire 
field with NULLs.

subCompatibleID This field contains the value of the subcompatible ID to 
be associated with this function. Any remaining bytes should be filled 
with NULLs. If the function does not have a subcompatible ID, fill the 
entire field with NULLs.

RESERVED Reserved. Fill this value with NULLs.
"

--nvpublic

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

[Jim Lin]

On 2016年05月04日 18:37, Felipe Balbi wrote:
> * PGP Signed by an unknown key
>
>
> Hi,
>
> Jim Lin <jilin@nvidia.com> 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 ? 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 ?
>
For some reason I have to put this patch on hold.

Thanks,
--nvpublic

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

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 3577 bytes --]


Hi Jim,

Jim Lin <jilin@nvidia.com> writes:
> On 2016年05月04日 18:37, Felipe Balbi wrote:
>> * PGP Signed by an unknown key
>>
>>
>> Hi,
>>
>> Jim Lin <jilin@nvidia.com> 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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]