| 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 |
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
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
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
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
> 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
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
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
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
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
> 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
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 --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 */
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(-)