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 <[email protected]>
---
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
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.
Hi Jim,
Jim Lin <[email protected]> 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
On 2016年04月22日 19:52, Felipe Balbi wrote:
> * PGP Signed by an unknown key
>
>
> Hi Jim,
>
> Jim Lin <[email protected]> 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
Hi,
Jim Lin <[email protected]> writes:
> On 2016年04月22日 19:52, Felipe Balbi wrote:
>> * PGP Signed by an unknown key
>>
>>
>> Hi Jim,
>>
>> Jim Lin <[email protected]> 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
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 <[email protected]>
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
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 <[email protected]>
> 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
Hi,
Jim Lin <[email protected]> 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 <[email protected]>
> 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
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 <[email protected]>
>> 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
Hi,
Jim Lin <[email protected]> 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 <[email protected]>
>>> 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
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