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

[Felipe Balbi]

Hi Jim,

Jim Lin  writes:
> On 2016年05月04日 18:37, Felipe Balbi wrote:
>> * PGP Signed by an unknown key
>>
>>
>> Hi,
>>
>> Jim Lin  writes:
>>
>> 
>>
> 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


signature.asc
Description: PGP signature

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  writes:




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
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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  writes:




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

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Felipe Balbi]

Hi,

Jim Lin  writes:



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


signature.asc
Description: PGP signature

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  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 
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
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Felipe Balbi]

Hi,

Mathias Nyman  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=, idProduct=
>>> [17908.013656] usb 6-2: New USB device strings: Mfr=1, Product=2,
>>> SerialNumber=3
>>> [17908.013658] usb 6-2: Product: xxx
>>> [17908.013661] usb 6-2: Manufacturer: xx
>>> [17908.013664] usb 6-2: SerialNumber: 1234567890
>>> [17908.014680] xhci_hcd :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


signature.asc
Description: PGP signature

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=, idProduct=
[17908.013656] usb 6-2: New USB device strings: Mfr=1, Product=2,
SerialNumber=3
[17908.013658] usb 6-2: Product: xxx
[17908.013661] usb 6-2: Manufacturer: xx
[17908.013664] usb 6-2: SerialNumber: 1234567890
[17908.014680] xhci_hcd :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

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Felipe Balbi]

Hi,

Jim Lin  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 
>>> 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


signature.asc
Description: PGP signature

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 
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
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Felipe Balbi]

Hi,

Jim Lin  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=, idProduct=
> [17908.013656] usb 6-2: New USB device strings: Mfr=1, Product=2, 
> SerialNumber=3
> [17908.013658] usb 6-2: Product: xxx
> [17908.013661] usb 6-2: Manufacturer: xx
> [17908.013664] usb 6-2: SerialNumber: 1234567890
> [17908.014680] xhci_hcd :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 
> 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.
>
> 

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=, 
idProduct=
[17908.013656] usb 6-2: New USB device strings: Mfr=1, Product=2, 
SerialNumber=3

[17908.013658] usb 6-2: Product: xxx
[17908.013661] usb 6-2: Manufacturer: xx
[17908.013664] usb 6-2: SerialNumber: 1234567890
[17908.014680] xhci_hcd :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 <bad...@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
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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=, idProduct=
[17908.013656] usb 6-2: New USB device strings: Mfr=1, Product=2, 
SerialNumber=3

[17908.013658] usb 6-2: Product: xxx
[17908.013661] usb 6-2: Manufacturer: xx
[17908.013664] usb 6-2: SerialNumber: 1234567890
[17908.014680] xhci_hcd :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 
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
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Felipe Balbi]

Hi,

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


signature.asc
Description: PGP signature

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  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
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Felipe Balbi]

Hi Jim,

Jim Lin  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


signature.asc
Description: PGP signature

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.

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html