diff mbox series

USB: usb-serial-simple: add new device id for OPPO R11

Message ID 20220715142444.4173681-1-gregkh@linuxfoundation.org
State New
Headers show
Series USB: usb-serial-simple: add new device id for OPPO R11 | expand

Commit Message

Greg KH July 15, 2022, 2:24 p.m. UTC
The Oppo R11 diagnostic USB connection needs to be bound to the
usb-serial-simple driver as it just wants to use a dumb pipe to
communicate to the host.

usb-devices output:
 T: Bus=03 Lev=01 Prnt=01 Port=01 Cnt=01 Dev#= 10 Spd=480 MxCh= 0
 D: Ver= 2.00 Cls=00(>ifc ) Sub=00 Prot=00 MxPS=64 #Cfgs= 1
 P: Vendor=22d9 ProdID=276c Rev=04.04
 S: Manufacturer=OPPO
 S: Product=SDM660-MTP _SN:09C6BCA7
 S: SerialNumber=beb2c403
 C: #Ifs= 2 Cfg#= 1 Atr=80 MxPwr=500mA
 I: If#=0x0 Alt= 0 #EPs= 2 Cls=ff(vend.) Sub=ff Prot=30

Reported-by: Yan Xinyu <sdlyyxy@bupt.edu.cn>
Cc: Johan Hovold <johan@kernel.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/usb/serial/usb-serial-simple.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Reinhard Speyerer July 16, 2022, 12:13 p.m. UTC | #1
On Fri, Jul 15, 2022 at 10:59:13PM +0800, sdlyyxy wrote:
> 
> > On Jul 15, 2022, at 22:24, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> > 
> > The Oppo R11 diagnostic USB connection needs to be bound to the
> > usb-serial-simple driver as it just wants to use a dumb pipe to
> > communicate to the host.
> > 
> > usb-devices output:
> > T: Bus=03 Lev=01 Prnt=01 Port=01 Cnt=01 Dev#= 10 Spd=480 MxCh= 0
> > D: Ver= 2.00 Cls=00(>ifc ) Sub=00 Prot=00 MxPS=64 #Cfgs= 1
> > P: Vendor=22d9 ProdID=276c Rev=04.04
> > S: Manufacturer=OPPO
> > S: Product=SDM660-MTP _SN:09C6BCA7
> > S: SerialNumber=beb2c403
> > C: #Ifs= 2 Cfg#= 1 Atr=80 MxPwr=500mA
> > I: If#=0x0 Alt= 0 #EPs= 2 Cls=ff(vend.) Sub=ff Prot=30
> > 
> > Reported-by: Yan Xinyu <sdlyyxy@bupt.edu.cn>
> > Cc: Johan Hovold <johan@kernel.org>
> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > ---
> > drivers/usb/serial/usb-serial-simple.c | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/usb/serial/usb-serial-simple.c b/drivers/usb/serial/usb-serial-simple.c
> > index 4c6747889a19..eb832b94aa3a 100644
> > --- a/drivers/usb/serial/usb-serial-simple.c
> > +++ b/drivers/usb/serial/usb-serial-simple.c
> > @@ -60,7 +60,9 @@ DEVICE(flashloader, FLASHLOADER_IDS);
> > 	{ USB_VENDOR_AND_INTERFACE_INFO(0x18d1,			\
> > 					USB_CLASS_VENDOR_SPEC,	\
> > 					0x50,			\
> > -					0x01) }
> > +					0x01) },		\
> > +	{ USB_DEVICE_AND_INTERFACE_INFO(0x22d9, 0x276c,		\
> > +					0xff, 0xff, 0x30) }
> > DEVICE(google, GOOGLE_IDS);
> > 
> > /* Libtransistor USB console */
> > -- 
> > 2.37.1
> Tested-by: Yan Xinyu <sdlyyxy@bupt.edu.cn>

While this may work sufficiently well for real low-volume diag traffic I'd
expect a significant percentage of diag messages to be lost in practice
with the usb-serial-simple driver.

According to the usb-devices output this looks like the Qualcomm USB gadget
in the DIAG + ADB composition to me.

Since the option driver uses the usb-wwan framework my suggestion would be
for the original patch to be applied instead similar to what has been done
e.g. for the Quectel RM500Q diag port.

Regards,
Reinhard
sdlyyxy July 16, 2022, 1:36 p.m. UTC | #2
Hi Reinhard,

> On Jul 16, 2022, at 20:13, Reinhard Speyerer <rspmn@arcor.de> wrote:
> 
> On Fri, Jul 15, 2022 at 10:59:13PM +0800, sdlyyxy wrote:
>> 
>>> On Jul 15, 2022, at 22:24, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
>>> 
>>> The Oppo R11 diagnostic USB connection needs to be bound to the
>>> usb-serial-simple driver as it just wants to use a dumb pipe to
>>> communicate to the host.
>>> 
>>> usb-devices output:
>>> T: Bus=03 Lev=01 Prnt=01 Port=01 Cnt=01 Dev#= 10 Spd=480 MxCh= 0
>>> D: Ver= 2.00 Cls=00(>ifc ) Sub=00 Prot=00 MxPS=64 #Cfgs= 1
>>> P: Vendor=22d9 ProdID=276c Rev=04.04
>>> S: Manufacturer=OPPO
>>> S: Product=SDM660-MTP _SN:09C6BCA7
>>> S: SerialNumber=beb2c403
>>> C: #Ifs= 2 Cfg#= 1 Atr=80 MxPwr=500mA
>>> I: If#=0x0 Alt= 0 #EPs= 2 Cls=ff(vend.) Sub=ff Prot=30
>>> 
>>> Reported-by: Yan Xinyu <sdlyyxy@bupt.edu.cn>
>>> Cc: Johan Hovold <johan@kernel.org>
>>> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>>> ---
>>> drivers/usb/serial/usb-serial-simple.c | 4 +++-
>>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>> 
>>> diff --git a/drivers/usb/serial/usb-serial-simple.c b/drivers/usb/serial/usb-serial-simple.c
>>> index 4c6747889a19..eb832b94aa3a 100644
>>> --- a/drivers/usb/serial/usb-serial-simple.c
>>> +++ b/drivers/usb/serial/usb-serial-simple.c
>>> @@ -60,7 +60,9 @@ DEVICE(flashloader, FLASHLOADER_IDS);
>>> 	{ USB_VENDOR_AND_INTERFACE_INFO(0x18d1,			\
>>> 					USB_CLASS_VENDOR_SPEC,	\
>>> 					0x50,			\
>>> -					0x01) }
>>> +					0x01) },		\
>>> +	{ USB_DEVICE_AND_INTERFACE_INFO(0x22d9, 0x276c,		\
>>> +					0xff, 0xff, 0x30) }
>>> DEVICE(google, GOOGLE_IDS);
>>> 
>>> /* Libtransistor USB console */
>>> -- 
>>> 2.37.1
>> Tested-by: Yan Xinyu <sdlyyxy@bupt.edu.cn>
> 
> While this may work sufficiently well for real low-volume diag traffic I'd
> expect a significant percentage of diag messages to be lost in practice
> with the usb-serial-simple driver.
> 
> According to the usb-devices output this looks like the Qualcomm USB gadget
> in the DIAG + ADB composition to me.
> 
> Since the option driver uses the usb-wwan framework my suggestion would be
> for the original patch to be applied instead similar to what has been done
> e.g. for the Quectel RM500Q diag port.
> 
> Regards,
> Reinhard
> 
I tested the diag port using two userspace programs: QCSuper[1] 
and scat[2]. Both option and usb-serial-simple drivers generate
similar output, so I cannot comfirm diag message loss. Do you
have any test method suggestions to generate high-volume diag 
traffic and detect message loss?


[1] https://github.com/P1sec/QCSuper
[2] https://github.com/fgsect/scat

Thanks,
sdlyyxy
Reinhard Speyerer July 17, 2022, 3:48 p.m. UTC | #3
Hi Yan,

On Sat, Jul 16, 2022 at 09:36:27PM +0800, sdlyyxy wrote:
> Hi Reinhard,
> 
> > On Jul 16, 2022, at 20:13, Reinhard Speyerer <rspmn@arcor.de> wrote:
> > 
> > On Fri, Jul 15, 2022 at 10:59:13PM +0800, sdlyyxy wrote:
> >> 
> >>> On Jul 15, 2022, at 22:24, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> >>> 
> >>> The Oppo R11 diagnostic USB connection needs to be bound to the
> >>> usb-serial-simple driver as it just wants to use a dumb pipe to
> >>> communicate to the host.
> >>> 
> >>> usb-devices output:
> >>> T: Bus=03 Lev=01 Prnt=01 Port=01 Cnt=01 Dev#= 10 Spd=480 MxCh= 0
> >>> D: Ver= 2.00 Cls=00(>ifc ) Sub=00 Prot=00 MxPS=64 #Cfgs= 1
> >>> P: Vendor=22d9 ProdID=276c Rev=04.04
> >>> S: Manufacturer=OPPO
> >>> S: Product=SDM660-MTP _SN:09C6BCA7
> >>> S: SerialNumber=beb2c403
> >>> C: #Ifs= 2 Cfg#= 1 Atr=80 MxPwr=500mA
> >>> I: If#=0x0 Alt= 0 #EPs= 2 Cls=ff(vend.) Sub=ff Prot=30
> >>> 
> >>> Reported-by: Yan Xinyu <sdlyyxy@bupt.edu.cn>
> >>> Cc: Johan Hovold <johan@kernel.org>
> >>> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> >>> ---
> >>> drivers/usb/serial/usb-serial-simple.c | 4 +++-
> >>> 1 file changed, 3 insertions(+), 1 deletion(-)
> >>> 
> >>> diff --git a/drivers/usb/serial/usb-serial-simple.c b/drivers/usb/serial/usb-serial-simple.c
> >>> index 4c6747889a19..eb832b94aa3a 100644
> >>> --- a/drivers/usb/serial/usb-serial-simple.c
> >>> +++ b/drivers/usb/serial/usb-serial-simple.c
> >>> @@ -60,7 +60,9 @@ DEVICE(flashloader, FLASHLOADER_IDS);
> >>> 	{ USB_VENDOR_AND_INTERFACE_INFO(0x18d1,			\
> >>> 					USB_CLASS_VENDOR_SPEC,	\
> >>> 					0x50,			\
> >>> -					0x01) }
> >>> +					0x01) },		\
> >>> +	{ USB_DEVICE_AND_INTERFACE_INFO(0x22d9, 0x276c,		\
> >>> +					0xff, 0xff, 0x30) }
> >>> DEVICE(google, GOOGLE_IDS);
> >>> 
> >>> /* Libtransistor USB console */
> >>> -- 
> >>> 2.37.1
> >> Tested-by: Yan Xinyu <sdlyyxy@bupt.edu.cn>
> > 
> > While this may work sufficiently well for real low-volume diag traffic I'd
> > expect a significant percentage of diag messages to be lost in practice
> > with the usb-serial-simple driver.
> > 
> > According to the usb-devices output this looks like the Qualcomm USB gadget
> > in the DIAG + ADB composition to me.
> > 
> > Since the option driver uses the usb-wwan framework my suggestion would be
> > for the original patch to be applied instead similar to what has been done
> > e.g. for the Quectel RM500Q diag port.
> > 
> > Regards,
> > Reinhard
> > 
> I tested the diag port using two userspace programs: QCSuper[1] 
> and scat[2]. Both option and usb-serial-simple drivers generate
> similar output, so I cannot comfirm diag message loss. Do you
> have any test method suggestions to generate high-volume diag 
> traffic and detect message loss?
> 

in my experience activating all message logs on the device with a
mask value like 0xf or 0x1f is a good way to generate more diag traffic.
Please refer to https://source.codeaurora.org/quic/imm/imm/sources/diag
(DIAG_CMD_OP_SET_ALL_MSG_MASK) for details.

Regards,
Reinhard
Greg KH July 17, 2022, 8:14 p.m. UTC | #4
On Sat, Jul 16, 2022 at 02:13:34PM +0200, Reinhard Speyerer wrote:
> On Fri, Jul 15, 2022 at 10:59:13PM +0800, sdlyyxy wrote:
> > 
> > > On Jul 15, 2022, at 22:24, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> > > 
> > > The Oppo R11 diagnostic USB connection needs to be bound to the
> > > usb-serial-simple driver as it just wants to use a dumb pipe to
> > > communicate to the host.
> > > 
> > > usb-devices output:
> > > T: Bus=03 Lev=01 Prnt=01 Port=01 Cnt=01 Dev#= 10 Spd=480 MxCh= 0
> > > D: Ver= 2.00 Cls=00(>ifc ) Sub=00 Prot=00 MxPS=64 #Cfgs= 1
> > > P: Vendor=22d9 ProdID=276c Rev=04.04
> > > S: Manufacturer=OPPO
> > > S: Product=SDM660-MTP _SN:09C6BCA7
> > > S: SerialNumber=beb2c403
> > > C: #Ifs= 2 Cfg#= 1 Atr=80 MxPwr=500mA
> > > I: If#=0x0 Alt= 0 #EPs= 2 Cls=ff(vend.) Sub=ff Prot=30
> > > 
> > > Reported-by: Yan Xinyu <sdlyyxy@bupt.edu.cn>
> > > Cc: Johan Hovold <johan@kernel.org>
> > > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > ---
> > > drivers/usb/serial/usb-serial-simple.c | 4 +++-
> > > 1 file changed, 3 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/usb/serial/usb-serial-simple.c b/drivers/usb/serial/usb-serial-simple.c
> > > index 4c6747889a19..eb832b94aa3a 100644
> > > --- a/drivers/usb/serial/usb-serial-simple.c
> > > +++ b/drivers/usb/serial/usb-serial-simple.c
> > > @@ -60,7 +60,9 @@ DEVICE(flashloader, FLASHLOADER_IDS);
> > > 	{ USB_VENDOR_AND_INTERFACE_INFO(0x18d1,			\
> > > 					USB_CLASS_VENDOR_SPEC,	\
> > > 					0x50,			\
> > > -					0x01) }
> > > +					0x01) },		\
> > > +	{ USB_DEVICE_AND_INTERFACE_INFO(0x22d9, 0x276c,		\
> > > +					0xff, 0xff, 0x30) }
> > > DEVICE(google, GOOGLE_IDS);
> > > 
> > > /* Libtransistor USB console */
> > > -- 
> > > 2.37.1
> > Tested-by: Yan Xinyu <sdlyyxy@bupt.edu.cn>
> 
> While this may work sufficiently well for real low-volume diag traffic I'd
> expect a significant percentage of diag messages to be lost in practice
> with the usb-serial-simple driver.
> 
> According to the usb-devices output this looks like the Qualcomm USB gadget
> in the DIAG + ADB composition to me.
> 
> Since the option driver uses the usb-wwan framework my suggestion would be
> for the original patch to be applied instead similar to what has been done
> e.g. for the Quectel RM500Q diag port.

But this is not using the option chip, nor using the option flow control
protocol at all, so it should not be showing up as a device controlled
by the option driver.  It just will not work properly, the simple driver
should be doing the exact same thing here.

thank,

greg k-h
sdlyyxy July 18, 2022, 2:02 p.m. UTC | #5
> On Jul 17, 2022, at 23:48, Reinhard Speyerer <rspmn@arcor.de> wrote:
> 
> Hi Yan,
> 
> On Sat, Jul 16, 2022 at 09:36:27PM +0800, sdlyyxy wrote:
>> Hi Reinhard,
>> 
>>> On Jul 16, 2022, at 20:13, Reinhard Speyerer <rspmn@arcor.de> wrote:
>>> 
>>> On Fri, Jul 15, 2022 at 10:59:13PM +0800, sdlyyxy wrote:
>>>> 
>>>>> On Jul 15, 2022, at 22:24, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
>>>>> 
>>>>> The Oppo R11 diagnostic USB connection needs to be bound to the
>>>>> usb-serial-simple driver as it just wants to use a dumb pipe to
>>>>> communicate to the host.
>>>>> 
>>>>> usb-devices output:
>>>>> T: Bus=03 Lev=01 Prnt=01 Port=01 Cnt=01 Dev#= 10 Spd=480 MxCh= 0
>>>>> D: Ver= 2.00 Cls=00(>ifc ) Sub=00 Prot=00 MxPS=64 #Cfgs= 1
>>>>> P: Vendor=22d9 ProdID=276c Rev=04.04
>>>>> S: Manufacturer=OPPO
>>>>> S: Product=SDM660-MTP _SN:09C6BCA7
>>>>> S: SerialNumber=beb2c403
>>>>> C: #Ifs= 2 Cfg#= 1 Atr=80 MxPwr=500mA
>>>>> I: If#=0x0 Alt= 0 #EPs= 2 Cls=ff(vend.) Sub=ff Prot=30
>>>>> 
>>>>> Reported-by: Yan Xinyu <sdlyyxy@bupt.edu.cn>
>>>>> Cc: Johan Hovold <johan@kernel.org>
>>>>> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>>>>> ---
>>>>> drivers/usb/serial/usb-serial-simple.c | 4 +++-
>>>>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>>>> 
>>>>> diff --git a/drivers/usb/serial/usb-serial-simple.c b/drivers/usb/serial/usb-serial-simple.c
>>>>> index 4c6747889a19..eb832b94aa3a 100644
>>>>> --- a/drivers/usb/serial/usb-serial-simple.c
>>>>> +++ b/drivers/usb/serial/usb-serial-simple.c
>>>>> @@ -60,7 +60,9 @@ DEVICE(flashloader, FLASHLOADER_IDS);
>>>>> 	{ USB_VENDOR_AND_INTERFACE_INFO(0x18d1,			\
>>>>> 					USB_CLASS_VENDOR_SPEC,	\
>>>>> 					0x50,			\
>>>>> -					0x01) }
>>>>> +					0x01) },		\
>>>>> +	{ USB_DEVICE_AND_INTERFACE_INFO(0x22d9, 0x276c,		\
>>>>> +					0xff, 0xff, 0x30) }
>>>>> DEVICE(google, GOOGLE_IDS);
>>>>> 
>>>>> /* Libtransistor USB console */
>>>>> -- 
>>>>> 2.37.1
>>>> Tested-by: Yan Xinyu <sdlyyxy@bupt.edu.cn>
>>> 
>>> While this may work sufficiently well for real low-volume diag traffic I'd
>>> expect a significant percentage of diag messages to be lost in practice
>>> with the usb-serial-simple driver.
>>> 
>>> According to the usb-devices output this looks like the Qualcomm USB gadget
>>> in the DIAG + ADB composition to me.
>>> 
>>> Since the option driver uses the usb-wwan framework my suggestion would be
>>> for the original patch to be applied instead similar to what has been done
>>> e.g. for the Quectel RM500Q diag port.
>>> 
>>> Regards,
>>> Reinhard
>>> 
>> I tested the diag port using two userspace programs: QCSuper[1] 
>> and scat[2]. Both option and usb-serial-simple drivers generate
>> similar output, so I cannot comfirm diag message loss. Do you
>> have any test method suggestions to generate high-volume diag 
>> traffic and detect message loss?
>> 
> 
> in my experience activating all message logs on the device with a
> mask value like 0xf or 0x1f is a good way to generate more diag traffic.
> Please refer to https://source.codeaurora.org/quic/imm/imm/sources/diag
> (DIAG_CMD_OP_SET_ALL_MSG_MASK) for details.
> 
> Regards,
> Reinhard
> 

Thank you very much for your advice! I'll try to understand the diag
protocol and experiment with different USB drivers to figure out their
difference :)

Regards,
sdlyyxy
Reinhard Speyerer July 18, 2022, 8:47 p.m. UTC | #6
Hi Greg,

On Sun, Jul 17, 2022 at 10:14:43PM +0200, Greg Kroah-Hartman wrote:
> On Sat, Jul 16, 2022 at 02:13:34PM +0200, Reinhard Speyerer wrote:
> > On Fri, Jul 15, 2022 at 10:59:13PM +0800, sdlyyxy wrote:
> > > 
> > > > On Jul 15, 2022, at 22:24, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> > > > 
> > > > The Oppo R11 diagnostic USB connection needs to be bound to the
> > > > usb-serial-simple driver as it just wants to use a dumb pipe to
> > > > communicate to the host.
> > > > 
> > > > usb-devices output:
> > > > T: Bus=03 Lev=01 Prnt=01 Port=01 Cnt=01 Dev#= 10 Spd=480 MxCh= 0
> > > > D: Ver= 2.00 Cls=00(>ifc ) Sub=00 Prot=00 MxPS=64 #Cfgs= 1
> > > > P: Vendor=22d9 ProdID=276c Rev=04.04
> > > > S: Manufacturer=OPPO
> > > > S: Product=SDM660-MTP _SN:09C6BCA7
> > > > S: SerialNumber=beb2c403
> > > > C: #Ifs= 2 Cfg#= 1 Atr=80 MxPwr=500mA
> > > > I: If#=0x0 Alt= 0 #EPs= 2 Cls=ff(vend.) Sub=ff Prot=30
> > > > 
> > > > Reported-by: Yan Xinyu <sdlyyxy@bupt.edu.cn>
> > > > Cc: Johan Hovold <johan@kernel.org>
> > > > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > > ---
> > > > drivers/usb/serial/usb-serial-simple.c | 4 +++-
> > > > 1 file changed, 3 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/usb/serial/usb-serial-simple.c b/drivers/usb/serial/usb-serial-simple.c
> > > > index 4c6747889a19..eb832b94aa3a 100644
> > > > --- a/drivers/usb/serial/usb-serial-simple.c
> > > > +++ b/drivers/usb/serial/usb-serial-simple.c
> > > > @@ -60,7 +60,9 @@ DEVICE(flashloader, FLASHLOADER_IDS);
> > > > 	{ USB_VENDOR_AND_INTERFACE_INFO(0x18d1,			\
> > > > 					USB_CLASS_VENDOR_SPEC,	\
> > > > 					0x50,			\
> > > > -					0x01) }
> > > > +					0x01) },		\
> > > > +	{ USB_DEVICE_AND_INTERFACE_INFO(0x22d9, 0x276c,		\
> > > > +					0xff, 0xff, 0x30) }
> > > > DEVICE(google, GOOGLE_IDS);
> > > > 
> > > > /* Libtransistor USB console */
> > > > -- 
> > > > 2.37.1
> > > Tested-by: Yan Xinyu <sdlyyxy@bupt.edu.cn>
> > 
> > While this may work sufficiently well for real low-volume diag traffic I'd
> > expect a significant percentage of diag messages to be lost in practice
> > with the usb-serial-simple driver.
> > 
> > According to the usb-devices output this looks like the Qualcomm USB gadget
> > in the DIAG + ADB composition to me.
> > 
> > Since the option driver uses the usb-wwan framework my suggestion would be
> > for the original patch to be applied instead similar to what has been done
> > e.g. for the Quectel RM500Q diag port.
> 
> But this is not using the option chip, nor using the option flow control
> protocol at all, so it should not be showing up as a device controlled
> by the option driver.  It just will not work properly, the simple driver
> should be doing the exact same thing here.
> 

you seem to have the misconception that devices handled by the option driver
would use a USB to serial converter chip from Option similar to e.g. the
ftdi_sio driver which is not the case.

Quoting from option.c:
  ===
  This driver exists because the "normal" serial driver doesn't work too well
  with GSM modems. Issues:
  - data loss -- one single Receive URB is not nearly enough
  - nonstandard flow (Option devices) control
  - controlling the baud rate doesn't make sense

  This driver is named "option" because the most common device it's
  used for is a PC-Card (with an internal OHCI-USB interface, behind
  which the GSM interface sits), made by Option Inc.
  ===

The GSM/UMTS interface in the Option card actually contains a Qualcomm chip
which exports its serial ports via USB. The data loss mentioned by Matthias
above also applies to the diag port.

As can be seen from the sendsetup code in the qcserial driver the
"nonstandard flow (Option devices) control" mentioned above is actually
a Qualcomm "feature" required for some of their USB serial implementations
on tne device to work properly.

In case you suspect potential problems with the sendsetup code and the OPPO
R11 diag port Yan's option.c patch could be extended like this:
	{ USB_DEVICE_AND_INTERFACE_INFO(OPPO_VENDOR_ID, OPPO_PRODUCT_R11, 0xff, 0xff, 0x30),
	  .driver_info = NCTRL(0) },

With a few more lines of code you could also add a new layout to the qcserial
driver if you prefer for the OPPO R11 to be handled by it for some reason.

Either one would be fine with me.

Please don't give the OPPO R11 diag port on Linux a bad name by letting
the usb-serial-simple driver handle it.

Thanks,
Reinhard
Johan Hovold July 23, 2022, 4:36 p.m. UTC | #7
On Mon, Jul 18, 2022 at 10:47:24PM +0200, Reinhard Speyerer wrote:
> Hi Greg,
> 
> On Sun, Jul 17, 2022 at 10:14:43PM +0200, Greg Kroah-Hartman wrote:
> > On Sat, Jul 16, 2022 at 02:13:34PM +0200, Reinhard Speyerer wrote:
> > > On Fri, Jul 15, 2022 at 10:59:13PM +0800, sdlyyxy wrote:
> > > > 
> > > > > On Jul 15, 2022, at 22:24, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> > > > > 
> > > > > The Oppo R11 diagnostic USB connection needs to be bound to the
> > > > > usb-serial-simple driver as it just wants to use a dumb pipe to
> > > > > communicate to the host.
> > > > > 
> > > > > usb-devices output:
> > > > > T: Bus=03 Lev=01 Prnt=01 Port=01 Cnt=01 Dev#= 10 Spd=480 MxCh= 0
> > > > > D: Ver= 2.00 Cls=00(>ifc ) Sub=00 Prot=00 MxPS=64 #Cfgs= 1
> > > > > P: Vendor=22d9 ProdID=276c Rev=04.04
> > > > > S: Manufacturer=OPPO
> > > > > S: Product=SDM660-MTP _SN:09C6BCA7
> > > > > S: SerialNumber=beb2c403
> > > > > C: #Ifs= 2 Cfg#= 1 Atr=80 MxPwr=500mA
> > > > > I: If#=0x0 Alt= 0 #EPs= 2 Cls=ff(vend.) Sub=ff Prot=30
> > > > > 
> > > > > Reported-by: Yan Xinyu <sdlyyxy@bupt.edu.cn>
> > > > > Cc: Johan Hovold <johan@kernel.org>
> > > > > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > > > ---
> > > > > drivers/usb/serial/usb-serial-simple.c | 4 +++-
> > > > > 1 file changed, 3 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/drivers/usb/serial/usb-serial-simple.c b/drivers/usb/serial/usb-serial-simple.c
> > > > > index 4c6747889a19..eb832b94aa3a 100644
> > > > > --- a/drivers/usb/serial/usb-serial-simple.c
> > > > > +++ b/drivers/usb/serial/usb-serial-simple.c
> > > > > @@ -60,7 +60,9 @@ DEVICE(flashloader, FLASHLOADER_IDS);
> > > > > 	{ USB_VENDOR_AND_INTERFACE_INFO(0x18d1,			\
> > > > > 					USB_CLASS_VENDOR_SPEC,	\
> > > > > 					0x50,			\
> > > > > -					0x01) }
> > > > > +					0x01) },		\
> > > > > +	{ USB_DEVICE_AND_INTERFACE_INFO(0x22d9, 0x276c,		\
> > > > > +					0xff, 0xff, 0x30) }
> > > > > DEVICE(google, GOOGLE_IDS);
> > > > > 
> > > > > /* Libtransistor USB console */
> > > > > -- 
> > > > > 2.37.1
> > > > Tested-by: Yan Xinyu <sdlyyxy@bupt.edu.cn>
> > > 
> > > While this may work sufficiently well for real low-volume diag traffic I'd
> > > expect a significant percentage of diag messages to be lost in practice
> > > with the usb-serial-simple driver.
> > > 
> > > According to the usb-devices output this looks like the Qualcomm USB gadget
> > > in the DIAG + ADB composition to me.
> > > 
> > > Since the option driver uses the usb-wwan framework my suggestion would be
> > > for the original patch to be applied instead similar to what has been done
> > > e.g. for the Quectel RM500Q diag port.
> > 
> > But this is not using the option chip, nor using the option flow control
> > protocol at all, so it should not be showing up as a device controlled
> > by the option driver.  It just will not work properly, the simple driver
> > should be doing the exact same thing here.
> > 
> 
> you seem to have the misconception that devices handled by the option driver
> would use a USB to serial converter chip from Option similar to e.g. the
> ftdi_sio driver which is not the case.
> 
> Quoting from option.c:
>   ===
>   This driver exists because the "normal" serial driver doesn't work too well
>   with GSM modems. Issues:
>   - data loss -- one single Receive URB is not nearly enough

This was written on 2006 and the single-receive URB statement hasn't
been true for the last decade or so.

>   - nonstandard flow (Option devices) control
>   - controlling the baud rate doesn't make sense
> 
>   This driver is named "option" because the most common device it's
>   used for is a PC-Card (with an internal OHCI-USB interface, behind
>   which the GSM interface sits), made by Option Inc.
>   ===
> 
> The GSM/UMTS interface in the Option card actually contains a Qualcomm chip
> which exports its serial ports via USB. The data loss mentioned by Matthias
> above also applies to the diag port.

Not necessarily true anymore even if the buffer sizes may need to be
increased somewhat to support dumping modem traffic.

And note that qcaux has always used the generic implementation (also
when it only used a single URB).
 
> As can be seen from the sendsetup code in the qcserial driver the
> "nonstandard flow (Option devices) control" mentioned above is actually
> a Qualcomm "feature" required for some of their USB serial implementations
> on tne device to work properly.

It's actually only used by some Sierra modems managed by that driver,
but yeah, those should be Qualcomm based.

> In case you suspect potential problems with the sendsetup code and the OPPO
> R11 diag port Yan's option.c patch could be extended like this:
> 	{ USB_DEVICE_AND_INTERFACE_INFO(OPPO_VENDOR_ID, OPPO_PRODUCT_R11, 0xff, 0xff, 0x30),
> 	  .driver_info = NCTRL(0) },
> 
> With a few more lines of code you could also add a new layout to the qcserial
> driver if you prefer for the OPPO R11 to be handled by it for some reason.

I think we decided not to use qcserial unless it matches one of the
fixed GOBI port layouts.

> Either one would be fine with me.
> 
> Please don't give the OPPO R11 diag port on Linux a bad name by letting
> the usb-serial-simple driver handle it.

So while I'm not sure bandwidth is really a problem, I still tend to
agree that we should add this one to the option driver for now as that
is how we handle (non-GOBI) Qualcomm modems and their QCDM ports.

Johan
Greg KH July 24, 2022, 2 p.m. UTC | #8
On Sat, Jul 23, 2022 at 06:36:25PM +0200, Johan Hovold wrote:
> On Mon, Jul 18, 2022 at 10:47:24PM +0200, Reinhard Speyerer wrote:
> > Hi Greg,
> > 
> > On Sun, Jul 17, 2022 at 10:14:43PM +0200, Greg Kroah-Hartman wrote:
> > > On Sat, Jul 16, 2022 at 02:13:34PM +0200, Reinhard Speyerer wrote:
> > > > On Fri, Jul 15, 2022 at 10:59:13PM +0800, sdlyyxy wrote:
> > > > > 
> > > > > > On Jul 15, 2022, at 22:24, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> > > > > > 
> > > > > > The Oppo R11 diagnostic USB connection needs to be bound to the
> > > > > > usb-serial-simple driver as it just wants to use a dumb pipe to
> > > > > > communicate to the host.
> > > > > > 
> > > > > > usb-devices output:
> > > > > > T: Bus=03 Lev=01 Prnt=01 Port=01 Cnt=01 Dev#= 10 Spd=480 MxCh= 0
> > > > > > D: Ver= 2.00 Cls=00(>ifc ) Sub=00 Prot=00 MxPS=64 #Cfgs= 1
> > > > > > P: Vendor=22d9 ProdID=276c Rev=04.04
> > > > > > S: Manufacturer=OPPO
> > > > > > S: Product=SDM660-MTP _SN:09C6BCA7
> > > > > > S: SerialNumber=beb2c403
> > > > > > C: #Ifs= 2 Cfg#= 1 Atr=80 MxPwr=500mA
> > > > > > I: If#=0x0 Alt= 0 #EPs= 2 Cls=ff(vend.) Sub=ff Prot=30
> > > > > > 
> > > > > > Reported-by: Yan Xinyu <sdlyyxy@bupt.edu.cn>
> > > > > > Cc: Johan Hovold <johan@kernel.org>
> > > > > > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > > > > ---
> > > > > > drivers/usb/serial/usb-serial-simple.c | 4 +++-
> > > > > > 1 file changed, 3 insertions(+), 1 deletion(-)
> > > > > > 
> > > > > > diff --git a/drivers/usb/serial/usb-serial-simple.c b/drivers/usb/serial/usb-serial-simple.c
> > > > > > index 4c6747889a19..eb832b94aa3a 100644
> > > > > > --- a/drivers/usb/serial/usb-serial-simple.c
> > > > > > +++ b/drivers/usb/serial/usb-serial-simple.c
> > > > > > @@ -60,7 +60,9 @@ DEVICE(flashloader, FLASHLOADER_IDS);
> > > > > > 	{ USB_VENDOR_AND_INTERFACE_INFO(0x18d1,			\
> > > > > > 					USB_CLASS_VENDOR_SPEC,	\
> > > > > > 					0x50,			\
> > > > > > -					0x01) }
> > > > > > +					0x01) },		\
> > > > > > +	{ USB_DEVICE_AND_INTERFACE_INFO(0x22d9, 0x276c,		\
> > > > > > +					0xff, 0xff, 0x30) }
> > > > > > DEVICE(google, GOOGLE_IDS);
> > > > > > 
> > > > > > /* Libtransistor USB console */
> > > > > > -- 
> > > > > > 2.37.1
> > > > > Tested-by: Yan Xinyu <sdlyyxy@bupt.edu.cn>
> > > > 
> > > > While this may work sufficiently well for real low-volume diag traffic I'd
> > > > expect a significant percentage of diag messages to be lost in practice
> > > > with the usb-serial-simple driver.
> > > > 
> > > > According to the usb-devices output this looks like the Qualcomm USB gadget
> > > > in the DIAG + ADB composition to me.
> > > > 
> > > > Since the option driver uses the usb-wwan framework my suggestion would be
> > > > for the original patch to be applied instead similar to what has been done
> > > > e.g. for the Quectel RM500Q diag port.
> > > 
> > > But this is not using the option chip, nor using the option flow control
> > > protocol at all, so it should not be showing up as a device controlled
> > > by the option driver.  It just will not work properly, the simple driver
> > > should be doing the exact same thing here.
> > > 
> > 
> > you seem to have the misconception that devices handled by the option driver
> > would use a USB to serial converter chip from Option similar to e.g. the
> > ftdi_sio driver which is not the case.
> > 
> > Quoting from option.c:
> >   ===
> >   This driver exists because the "normal" serial driver doesn't work too well
> >   with GSM modems. Issues:
> >   - data loss -- one single Receive URB is not nearly enough
> 
> This was written on 2006 and the single-receive URB statement hasn't
> been true for the last decade or so.
> 
> >   - nonstandard flow (Option devices) control
> >   - controlling the baud rate doesn't make sense
> > 
> >   This driver is named "option" because the most common device it's
> >   used for is a PC-Card (with an internal OHCI-USB interface, behind
> >   which the GSM interface sits), made by Option Inc.
> >   ===
> > 
> > The GSM/UMTS interface in the Option card actually contains a Qualcomm chip
> > which exports its serial ports via USB. The data loss mentioned by Matthias
> > above also applies to the diag port.
> 
> Not necessarily true anymore even if the buffer sizes may need to be
> increased somewhat to support dumping modem traffic.
> 
> And note that qcaux has always used the generic implementation (also
> when it only used a single URB).
>  
> > As can be seen from the sendsetup code in the qcserial driver the
> > "nonstandard flow (Option devices) control" mentioned above is actually
> > a Qualcomm "feature" required for some of their USB serial implementations
> > on tne device to work properly.
> 
> It's actually only used by some Sierra modems managed by that driver,
> but yeah, those should be Qualcomm based.
> 
> > In case you suspect potential problems with the sendsetup code and the OPPO
> > R11 diag port Yan's option.c patch could be extended like this:
> > 	{ USB_DEVICE_AND_INTERFACE_INFO(OPPO_VENDOR_ID, OPPO_PRODUCT_R11, 0xff, 0xff, 0x30),
> > 	  .driver_info = NCTRL(0) },
> > 
> > With a few more lines of code you could also add a new layout to the qcserial
> > driver if you prefer for the OPPO R11 to be handled by it for some reason.
> 
> I think we decided not to use qcserial unless it matches one of the
> fixed GOBI port layouts.
> 
> > Either one would be fine with me.
> > 
> > Please don't give the OPPO R11 diag port on Linux a bad name by letting
> > the usb-serial-simple driver handle it.
> 
> So while I'm not sure bandwidth is really a problem, I still tend to
> agree that we should add this one to the option driver for now as that
> is how we handle (non-GOBI) Qualcomm modems and their QCDM ports.

If you want it to stay on the option driver, that's fine, but I still
think it feels odd as it obviously does not follow the vendor-specific
protocol that the option driver supports.

To be fair, loads of the ids in that driver could move to the simple
driver as they probably do not also support the line setting protocol
that this driver was originally written for, so what's a few more ids
added :)

thanks,

greg k-h
Johan Hovold July 24, 2022, 2:26 p.m. UTC | #9
On Sun, Jul 24, 2022 at 04:00:36PM +0200, Greg Kroah-Hartman wrote:
> On Sat, Jul 23, 2022 at 06:36:25PM +0200, Johan Hovold wrote:
> > On Mon, Jul 18, 2022 at 10:47:24PM +0200, Reinhard Speyerer wrote:

> > > Please don't give the OPPO R11 diag port on Linux a bad name by letting
> > > the usb-serial-simple driver handle it.
> > 
> > So while I'm not sure bandwidth is really a problem, I still tend to
> > agree that we should add this one to the option driver for now as that
> > is how we handle (non-GOBI) Qualcomm modems and their QCDM ports.
> 
> If you want it to stay on the option driver, that's fine, but I still
> think it feels odd as it obviously does not follow the vendor-specific
> protocol that the option driver supports.

But we've been dumping modem device-id entries in there since forever.

The entries added to option have been for devices whose interfaces did
not follow any particular pattern (e.g. unlike the old GOBI modems).

And as Reinhard mentioned, the line-control requests (which follow CDC)
are actually required by some Qualcomm modems so moving things out would
need to be done carefully.

On the other hand, that request likely isn't needed for any QCDM/DIAG
ports, but who knows for sure.

> To be fair, loads of the ids in that driver could move to the simple
> driver as they probably do not also support the line setting protocol
> that this driver was originally written for, so what's a few more ids
> added :)

Not sure about the simple driver, unless we want to handle say NMEA and
MODEM ports using different drivers. But either way, for now I think we
can add a few more to option.

If we start seeing patterns regarding the Qualcomm interface
descriptors, like the QCDM interface using 0xff/0xff/0x30, we could
implement something more generic in qcserial too.

But the lack of documentation and unification is just a pain to deal
with. So someone would need to be motivated enough to try to organise
this mess.

Johan
sdlyyxy July 29, 2022, 6:13 a.m. UTC | #10
> On Jul 24, 2022, at 22:26, Johan Hovold <johan@kernel.org> wrote:
> 
> On Sun, Jul 24, 2022 at 04:00:36PM +0200, Greg Kroah-Hartman wrote:
>> On Sat, Jul 23, 2022 at 06:36:25PM +0200, Johan Hovold wrote:
>>> On Mon, Jul 18, 2022 at 10:47:24PM +0200, Reinhard Speyerer wrote:
> 
>>>> Please don't give the OPPO R11 diag port on Linux a bad name by letting
>>>> the usb-serial-simple driver handle it.
>>> 
>>> So while I'm not sure bandwidth is really a problem, I still tend to
>>> agree that we should add this one to the option driver for now as that
>>> is how we handle (non-GOBI) Qualcomm modems and their QCDM ports.
>> 
>> If you want it to stay on the option driver, that's fine, but I still
>> think it feels odd as it obviously does not follow the vendor-specific
>> protocol that the option driver supports.
> 
> But we've been dumping modem device-id entries in there since forever.
> 
> The entries added to option have been for devices whose interfaces did
> not follow any particular pattern (e.g. unlike the old GOBI modems).
> 
> And as Reinhard mentioned, the line-control requests (which follow CDC)
> are actually required by some Qualcomm modems so moving things out would
> need to be done carefully.
> 
> On the other hand, that request likely isn't needed for any QCDM/DIAG
> ports, but who knows for sure.

Test result for bandwidth problem:
Sending 0x1f mask (diag command: 0x7d0500001f000000) and running LTE
speedtest on the device, both option and simple can dump more than 80Mbps.
The CRC of diag packets is OK at this high speed, so it seems that
there is no message loss. I think this bandwidth is enough.

For the flow control problem, it seems the SetControlLineState request
send by option (usb_wwan) has no effect on the device. Both with and
without this request the diag port works the same.

Hope this can help you decide which driver to choose :)

Thanks,
sdlyyxy
Johan Hovold July 29, 2022, 6:37 a.m. UTC | #11
On Fri, Jul 29, 2022 at 02:13:56PM +0800, sdlyyxy wrote:
> 
> > On Jul 24, 2022, at 22:26, Johan Hovold <johan@kernel.org> wrote:
> > 
> > On Sun, Jul 24, 2022 at 04:00:36PM +0200, Greg Kroah-Hartman wrote:
> >> On Sat, Jul 23, 2022 at 06:36:25PM +0200, Johan Hovold wrote:
> >>> On Mon, Jul 18, 2022 at 10:47:24PM +0200, Reinhard Speyerer wrote:
> > 
> >>>> Please don't give the OPPO R11 diag port on Linux a bad name by letting
> >>>> the usb-serial-simple driver handle it.
> >>> 
> >>> So while I'm not sure bandwidth is really a problem, I still tend to
> >>> agree that we should add this one to the option driver for now as that
> >>> is how we handle (non-GOBI) Qualcomm modems and their QCDM ports.
> >> 
> >> If you want it to stay on the option driver, that's fine, but I still
> >> think it feels odd as it obviously does not follow the vendor-specific
> >> protocol that the option driver supports.
> > 
> > But we've been dumping modem device-id entries in there since forever.
> > 
> > The entries added to option have been for devices whose interfaces did
> > not follow any particular pattern (e.g. unlike the old GOBI modems).
> > 
> > And as Reinhard mentioned, the line-control requests (which follow CDC)
> > are actually required by some Qualcomm modems so moving things out would
> > need to be done carefully.
> > 
> > On the other hand, that request likely isn't needed for any QCDM/DIAG
> > ports, but who knows for sure.
> 
> Test result for bandwidth problem:
> Sending 0x1f mask (diag command: 0x7d0500001f000000) and running LTE
> speedtest on the device, both option and simple can dump more than 80Mbps.
> The CRC of diag packets is OK at this high speed, so it seems that
> there is no message loss. I think this bandwidth is enough.
> 
> For the flow control problem, it seems the SetControlLineState request
> send by option (usb_wwan) has no effect on the device. Both with and
> without this request the diag port works the same.
> 
> Hope this can help you decide which driver to choose :)

Thanks a lot for confirming! I'll try to revisit this next week and get
something merged for 5.20-rcN.

Johan
diff mbox series

Patch

diff --git a/drivers/usb/serial/usb-serial-simple.c b/drivers/usb/serial/usb-serial-simple.c
index 4c6747889a19..eb832b94aa3a 100644
--- a/drivers/usb/serial/usb-serial-simple.c
+++ b/drivers/usb/serial/usb-serial-simple.c
@@ -60,7 +60,9 @@  DEVICE(flashloader, FLASHLOADER_IDS);
 	{ USB_VENDOR_AND_INTERFACE_INFO(0x18d1,			\
 					USB_CLASS_VENDOR_SPEC,	\
 					0x50,			\
-					0x01) }
+					0x01) },		\
+	{ USB_DEVICE_AND_INTERFACE_INFO(0x22d9, 0x276c,		\
+					0xff, 0xff, 0x30) }
 DEVICE(google, GOOGLE_IDS);
 
 /* Libtransistor USB console */