[GIT PULL] USB-serial fix for v4.20-rc6
The following changes since commit 2595646791c319cadfdbf271563aac97d0843dc7: Linux 4.20-rc5 (2018-12-02 15:07:55 -0800) are available in the Git repository at: https://git.kernel.org/pub/scm/linux/kernel/git/johan/usb-serial.git tags/usb-serial-4.20-rc6 for you to fetch changes up to f51ccf46217c28758b1f3b5bc0ccfc00eca658b2: USB: serial: console: fix reported terminal settings (2018-12-05 11:29:10 +0100) USB-serial fix for v4.20-rc6 Here's a fix for a reported USB-console regression in 4.18 which revealed a long-standing bug in the console implementation. The patch has been in linux-next over night with no reported issues. Signed-off-by: Johan Hovold Johan Hovold (1): USB: serial: console: fix reported terminal settings drivers/tty/tty_io.c | 11 +-- drivers/usb/serial/console.c | 2 +- include/linux/tty.h | 1 + 3 files changed, 11 insertions(+), 3 deletions(-)
Re: [PATCH] USB: qcaux: Add Motorola modem UARTs
On Wed, Dec 05, 2018 at 05:54:07PM -0800, Tony Lindgren wrote: > Hi, > > * Johan Hovold [181205 06:17]: > > On Sun, Dec 02, 2018 at 05:34:24PM -0800, Tony Lindgren wrote: > > > On Motorola Mapphone devices such as Droid 4 there are five USB ports > > > that do not use the same layout as Gobi 1K/2K/etc devices listed in > > > qcserial.c. So we should use qcaux.c or option.c as noted by > > > Dan Williams . > > > > > > The ff/ff/ff interfaces seem to always be UARTs on Motorola devices. > > > And we should not add interfaces with 0x0a class (CDC Data) as they > > > are part of a multi-interface function like for example interface > > > 0x22b8:0x4281 as noted by Bjørn Mork . > > > > Can you post the output of usb-devices (or lsusb -v) for these three > > devices (PIDs)? > > Here's two out of three for you to look at. They're all listed in > drivers/usb/serial/mdm6600.c in at least the Motorola Mapphone > Android kernels, see for example the LineageOS kernel at [0] if > you want to look at the USB serial driver. > > I don't have a device with 9600 with 0x2e0a id. > > [0] > https://github.com/LineageOS/android_kernel_motorola_omap4-common/blob/cm-14.1/drivers/usb/serial/mdm6600.c Thanks for the pointer. > > 8< - > Bus 001 Device 002: ID 22b8:4281 > Device Descriptor: > bLength18 > bDescriptorType 1 > bcdUSB 2.00 > bDeviceClass0 > bDeviceSubClass 0 > bDeviceProtocol 0 > bMaxPacketSize064 > idVendor 0x22b8 > idProduct 0x4281 This PID is not included in your patch however. > bcdDevice0.00 > iManufacturer 1 Motorola, Incorporated > iProduct2 Flash MZ600 > iSerial 0 > bNumConfigurations 1 > Configuration Descriptor: > bLength 9 > bDescriptorType 2 > wTotalLength 0x0020 > bNumInterfaces 1 > bConfigurationValue 1 > iConfiguration 3 Motorola Configuration > bmAttributes 0xe0 > Self Powered > Remote Wakeup > MaxPower 500mA > Interface Descriptor: > bLength 9 > bDescriptorType 4 > bInterfaceNumber0 > bAlternateSetting 0 > bNumEndpoints 2 > bInterfaceClass10 > bInterfaceSubClass 0 > bInterfaceProtocol252 And wouldn't match on ff/ff/ff in any case. > iInterface 5 Motorola Flash > Endpoint Descriptor: > bLength 7 > bDescriptorType 5 > bEndpointAddress 0x81 EP 1 IN > bmAttributes2 > Transfer TypeBulk > Synch Type None > Usage Type Data > wMaxPacketSize 0x0040 1x 64 bytes > bInterval 0 > Endpoint Descriptor: > bLength 7 > bDescriptorType 5 > bEndpointAddress 0x01 EP 1 OUT > bmAttributes2 > Transfer TypeBulk > Synch Type None > Usage Type Data > wMaxPacketSize 0x0040 1x 64 bytes > bInterval 0 > Device Status: 0x > (Bus Powered) > > Bus 003 Device 109: ID 22b8:900e > Device Descriptor: > bLength18 > bDescriptorType 1 > bcdUSB 2.00 > bDeviceClass0 > bDeviceSubClass 0 > bDeviceProtocol 0 > bMaxPacketSize064 > idVendor 0x22b8 > idProduct 0x900e > bcdDevice0.00 > iManufacturer 1 Motorola, Incorporated > iProduct2 Flash MZ600 > iSerial 0 > bNumConfigurations 1 > Configuration Descriptor: > bLength 9 > bDescriptorType 2 > wTotalLength 0x0020 > bNumInterfaces 1 > bConfigurationValue 1 > iConfiguration 3 Motorola Configuration > bmAttributes 0xe0 > Self Powered > Remote Wakeup > MaxPower 500mA > Interface Descriptor: > bLength 9 > bDescriptorType 4 > bInterfaceNumber0 > bAlternateSetting 0 > bNumEndpoints 2 > bInterfaceClass 255 > bInterfaceSubCl
Re: [PATCH] USB: qcaux: Add Motorola modem UARTs
On Sun, Dec 02, 2018 at 05:34:24PM -0800, Tony Lindgren wrote: > On Motorola Mapphone devices such as Droid 4 there are five USB ports > that do not use the same layout as Gobi 1K/2K/etc devices listed in > qcserial.c. So we should use qcaux.c or option.c as noted by > Dan Williams . > > The ff/ff/ff interfaces seem to always be UARTs on Motorola devices. > And we should not add interfaces with 0x0a class (CDC Data) as they > are part of a multi-interface function like for example interface > 0x22b8:0x4281 as noted by Bjørn Mork . Can you post the output of usb-devices (or lsusb -v) for these three devices (PIDs)? Thanks, Johan
Re: pl2303 regression after v4.17. Bisected to 7041d9c3f01b
On Mon, Nov 26, 2018 at 02:29:53PM +0200, Jarkko Nikula wrote: > On 11/24/18 7:32 PM, Florian Zumbiehl wrote: > > Hi, > > > >> Requested baud setting looks odd to me. Maybe related to this > >> --keep-baud flag in "/sbin/agetty -o -p -- \u --keep-baud > >> 115200,38400,9600 ttyUSB0 vt220"? That indeed seems to be the case. > > Well, what is the baud rate of the other side? It seems a bit strange > > that ... > > > 115200. > > >> 1. serial-getty@ttyUSB0.service disabled > >> > >> speed 9600 baud; rows 0; columns 0; line = 0; > > > > ... this works ... > > > >> 3. serial-getty@ttyUSB0.service disabled && picocom -b 115200 /dev/ttyUSB0 > >> > >> speed 115200 baud; rows 0; columns 0; line = 0; > > > > ... and this works as well?! > > > > I think before debugging a potential problem with the new flow control > > feature, it would be a good idea to first make sure the baud rate is > > configured correctly. Maybe agetty's baud rate selection is somehow > > influenced by the new flow control support? I don't see how it would, and > > maybe that's still a bug somehow, but I suspect that there is no flow > > control problem at all, but rather it's just a baud rate mismatch that > > prevents communication. > > > I went debugging the commit and pl2303_set_termios() a bit. I noticed > that the tty_termios_hw_change() returns zero when starting the getty > service that keeps the existing baud. Same goes with the plain "picocom > /dev/ttyUSB0" without specifying the baud which also works before commit > 7041d9c3f01b. > > For both cases ixon_change is true so the pl2303_termios_change() > returns true causing the pl2303_set_termios() to go forward doing the > baudrate (incorrectly 9600 instead of 115200) etc. changes which doesn't > happen before commit 7041d9c3f01b. > > This incorrect 9600 baud made me thinking could it be possible that > driver doesn't initialize some structure with the current settings the > kernel console is using? Would this also explain also why > "stty -a -F /dev/ttyUSB0" shows 9600 when kernel console is certainly > using 115200 (after boot before running neither agetty or picocom)? Correct. The USB-serial console code has never reported the actual settings used, but with the pl2303 flow control change this now obviously broke your setup. Judging from your reports, including the stty outputs, it seems like the getty is trying to disable flow control while keeping the baud rate unchanged. Prior to the pl2303 change this was a noop which left the baud rate unchanged, but now you end up with the driver default 9600 baud. > I verified the same setup using legacy 8250 uart and with it the > "stty -a -F /dev/ttyS0" shows correctly 115200 baud. Right, the serial console correctly updates the terminal settings, unlike the USB-serial console code. I've come up with a non-intrusive fix which fixes the incorrectly reported terminal settings. Would you mind giving it a try? Thanks, Johan
Re: [PATCH v2] USB: serial: ftdi_sio: Use rounding instead of truncating when calculating baud rate divisors.
On Thu, Nov 22, 2018 at 09:27:46PM +0100, Nikolaj Fogh wrote:
> Improve baud-rate generation by using rounding-to-closest instead of
> truncation in divisor calculation.
>
> Results have been verified by logic analyzer on an FT232RT (232BM) chip.
> The following table shows the wanted baud rate, the baud rate obtained
> with the old method (truncation), with the new method (rounding) and the
> baud rate generated by the windows 10 driver. The numbers in parentheses
> is the error.
>
> +- Wanted --+-- Old ---+-- New ---+-- Win ---+
> | 9600 | 9600 (0.00%) | 9604 (0.05%) | 9605 (0.05%) |
> | 19200 | 19200 (0.00%) | 19199 (0.01%) | 19198 (0.01%) |
> | 38400 | 38395 (0.01%) | 38431 (0.08%) | 38394 (0.02%) |
> | 57600 | 57725 (0.22%) | 57540 (0.10%) | 57673 (0.13%) |
> | 115200 | 115307 (0.09%) | 115330 (0.11%) | 115320 (0.10%) |
> | 921600 | 919963 (0.18%) | 920386 (0.13%) | 920810 (0.09%) |
> | 961200 | 996512 (3.67%) | 956480 (0.49%) | 956937 (0.44%) |
> +---+--+--+--+
>
> The error due to noise in the measurements is in the order of a few
> tenths of a %. As can be seen, the baud rate for 961200 is significantly
> improved for some rates, and corresponds to the output given by the
> windows driver.
>
> The theoretical baud rate has been calculated for all baud rates from 1
> to 3M, and as expected, the error is centered around 0, with a triangle
> shape instead of a sawtooth, so the maximum error is decreased to half.
>
> Signed-off-by: Nikolaj Fogh
>
> ---
> drivers/usb/serial/ftdi_sio.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c
> index 609198d9594c..0edbd3427548 100644
> --- a/drivers/usb/serial/ftdi_sio.c
> +++ b/drivers/usb/serial/ftdi_sio.c
> @@ -1130,7 +1130,7 @@ static unsigned short int
> ftdi_232am_baud_base_to_divisor(int baud, int base)
> {
> unsigned short int divisor;
> /* divisor shifted 3 bits to the left */
> - int divisor3 = base / 2 / baud;
> + int divisor3 = DIV_ROUND_CLOSEST(base, 2 * baud);
> if ((divisor3 & 0x7) == 7)
> divisor3++; /* round x.7/8 up to x+1 */
> divisor = divisor3 >> 3;
This version turned out to be white-space damaged by gmail (?) with tabs
replaced by spaces.
I fixed it up manually, but next time try sending the patch to yourself
and make sure can apply it (and/or run checkpatch on the result).
I also shortened the summary further and edited the commit message very
slightly. The result can be seen here (soon):
https://git.kernel.org/pub/scm/linux/kernel/git/johan/usb-serial.git/commit/?h=usb-next=6abd837104a3a8e1cda64fc4d7675f6c3ece9d8b
Again, thanks for fixing this.
Johan
Re: pl2303 regression after v4.17. Bisected to 7041d9c3f01b
On Fri, Nov 23, 2018 at 04:19:49AM +0100, Florian Zumbiehl wrote: > Hi, > > > T: Bus=01 Lev=02 Prnt=03 Port=01 Cnt=01 Dev#= 4 Spd=12 MxCh= 0 > > D: Ver= 1.10 Cls=00(>ifc ) Sub=00 Prot=00 MxPS=64 #Cfgs= 1 > > P: Vendor=0557 ProdID=2008 Rev=03.00 > > S: Manufacturer=Prolific Technology Inc. > > S: Product=USB-Serial Controller D > > C: #Ifs= 1 Cfg#= 1 Atr=a0 MxPwr=100mA > > I: If#= 0 Alt= 0 #EPs= 3 Cls=ff(vend.) Sub=00 Prot=00 Driver=pl2303 > > > > and > > > > T: Bus=01 Lev=02 Prnt=02 Port=03 Cnt=03 Dev#= 6 Spd=12 MxCh= 0 > > D: Ver= 1.10 Cls=00(>ifc ) Sub=00 Prot=00 MxPS=64 #Cfgs= 1 > > P: Vendor=067b ProdID=2303 Rev=04.00 > > S: Manufacturer=Prolific Technology Inc. > > S: Product=USB-Serial Controller D > > C: #Ifs= 1 Cfg#= 1 Atr=a0 MxPwr=100mA > > I: If#= 0 Alt= 0 #EPs= 3 Cls=ff(vend.) Sub=00 Prot=00 Driver=pl2303 > > The ones that I have at hand all look like this: > > T: Bus=05 Lev=01 Prnt=01 Port=01 Cnt=02 Dev#= 3 Spd=12 MxCh= 0 > D: Ver= 1.10 Cls=00(>ifc ) Sub=00 Prot=00 MxPS=64 #Cfgs= 1 > P: Vendor=067b ProdID=2303 Rev=03.00 > S: Manufacturer=Prolific Technology Inc. > S: Product=USB-Serial Controller > C: #Ifs= 1 Cfg#= 1 Atr=80 MxPwr=100mA > I: If#= 0 Alt= 0 #EPs= 3 Cls=ff(vend.) Sub=00 Prot=00 Driver=pl2303 > > So, yours and mine are all type 'HX' according to the driver's > categorization ... > > The only common differences would be the product description string having a > "D" suffix in your version and bmAttributes being 0xa0 vs. 0x80, so yours > claim to support remote wakeup. The former seems like a bad idea to base > hardware detection on, but the latter might indicate a different chip with > different features?! > > Can you tell what the physical chip is labeled as, if anything? > > @Johan: What does the USB device info of your adapter(s) say? Almost the same as Jarkko's second device: T: Bus=01 Lev=01 Prnt=01 Port=00 Cnt=01 Dev#= 2 Spd=12 MxCh= 0 D: Ver= 1.10 Cls=00(>ifc ) Sub=00 Prot=00 MxPS=64 #Cfgs= 1 P: Vendor=067b ProdID=2303 Rev=04.00 S: Manufacturer=Prolific Technology Inc. S: Product=USB-Serial Controller D C: #Ifs= 1 Cfg#= 1 Atr=80 MxPwr=100mA I: If#= 0 Alt= 0 #EPs= 3 Cls=ff(vend.) Sub=00 Prot=00 Driver=pl2303 The only difference appears to be bmAttributes not having Remote Wakeup set. I also just confirmed that my device works fine as console and with a (busybox) getty running. I can stop and start the flow using stop and star chars and disabling IXON transmits any buffered chars while stopped. Jarkko, does software flow control work if you run a terminal program instead of a getty? And could you try enabling debugging for pl2303 so we can see what commands are sent when your getty is started? And what is the output of stty -a -F /dev/ttyUSB0 when the getty has been started? Thanks, Johan
Re: [PATCH v2] USB: serial: ftdi_sio: Improve the accuracy of the baud rate generator by using round-to-closest instead of truncating when calculating baud rate divisors.
On Fri, Nov 16, 2018 at 10:44:21PM +0100, Nikolaj Fogh wrote: > Improve baud-rate generation by using rounding-to-closest instead of > truncation in divisor calculation. > > Results have been verified by logic analyzer on an FT232RT (232BM) chip. > The following table shows the wanted baud rate, the baud rate obtained > with the old method (truncation), with the new method (rounding) and the > baud rate generated by the windows 10 driver. The numbers in parentheses > is the error. > > +- Wanted --+-- Old ---+-- New ---+-- Win ---+ > | 9600 | 9600 (0.00%) | 9604 (0.05%) | 9605 (0.05%) | > | 19200 | 19200 (0.00%) | 19199 (0.01%) | 19198 (0.01%) | > | 38400 | 38395 (0.01%) | 38431 (0.08%) | 38394 (0.02%) | > | 57600 | 57725 (0.22%) | 57540 (0.10%) | 57673 (0.13%) | > | 115200 | 115307 (0.09%) | 115330 (0.11%) | 115320 (0.10%) | > | 921600 | 919963 (0.18%) | 920386 (0.13%) | 920810 (0.09%) | > | 961200 | 996512 (3.67%) | 956480 (0.49%) | 956937 (0.44%) | > +---+--+--+--+ > > The error due to noise in the measurements is in the order of a few > tenths of a %. As can be seen, the baud rate for 961200 is significantly > improved for some rates, and corresponds to the output given by the > windows driver. > > The theoretical baud rate has been calculated for all baud rates from 1 > to 3M, and as expected, the error is centered around 0, with a triangle > shape instead of a sawtooth, so the maximum error is decreased to half. > > Signed-off-by: Nikolaj Fogh Thanks for updated commit message. I was just about to apply it, but checkpatch reported that the Signed-off address doesn't match the author address (From). Would you mind fixing that up? Also, please use a less verbose subject (patch summary), try to make it fit within 72 cols or so. Thanks, Johan
Re: pl2303 regression after v4.17. Bisected to 7041d9c3f01b
On Tue, Nov 20, 2018 at 01:17:34PM +0200, Jarkko Nikula wrote:
> Hi
>
> I'm using PL2303 based USB serial adapter for the kernel and getty
> serial console. I noticed getty console stopped working in v4.18 and
> bisected this into commit 7041d9c3f01b ("USB: serial: pl2303: add
> support for tx xon/xoff flow control").
>
> Serial console works up to somewhere after when getty service starts. I
> can sometimes see on the serial console that getty was started but not
> always. After that there is no messages printed from the kernel or
> userspace.
>
> [ OK ] Started Getty on tty1.
> [ OK ] Started Serial Getty on ttyUSB0.
>
> [ OK ] Reached target Login Prompts.
> [ OK ] Started Login Service.
IXON is enabled by default, and it sounds like flow could have been
stopped. What happens if you send a start character (ctrl-q) on the
console?
And what if you disable the getty? Does the port work then?
Johan
Re: [PATCH] Improve the accuracy of the baud rate generator by using round-to-closest instead of truncating when calculating baud rate divisors.
On Thu, Nov 15, 2018 at 03:16:04PM +0100, Nikolaj Fogh wrote: > On 11/15/18 9:24 AM, Johan Hovold wrote: > > On Tue, Nov 13, 2018 at 08:19:44PM +0100, Nikolaj Fogh wrote: > >> On 11/12/18 10:54 AM, Johan Hovold wrote: > >>> On Wed, Oct 31, 2018 at 09:16:48PM +0100, Nikolaj Fogh wrote: > >>>> I have experienced that the ftdi_sio driver gives less-than-optimal > >>>> baud rates as the driver truncates instead of rounds to nearest > >>>> during baud rate divisor calculation. > >>>> This patch improves on the baud rate generation. The generated baud > >>>> rate corresponds to the optimal baud rate achievable with the chip. > >>>> This is what the windows driver gives as well. > >>> How did you verify this? Did you trace and compare the divisors > >>> actually requested by the Windows driver, or did you measure the > >>> resulting rates using a scope? > >> I verified it by scope. Granted, I only verified it for one baud rate > >> (961200). Whether it gives the same as the Windows driver in general, > >> I'm not sure. However, I would think that rounding instead of flooring > >> would always yield the most accurate result. > > I'm not so sure in this case. The driver uses "sub-integer" divisors and > > looks like it depends on truncation rather than rounding. Some > > background here: > > > > > > https://www.ftdichip.com/Support/Knowledgebase/index.html?whatbaudratesareachieveabl.htm > I have had a closer look at this (and the driver code), and it seemsthat > each bit in the divisorcorresponds to 1/8th (0.125) in the calculation. > > It is shuffled around a bit in the code (for legacy reasons I expect), and > put in the higher order bits, but prior to that, I see no reason that > rounding should not be used instead of truncating. I don't see how it > "depends" on truncation. As I mentioned in my follow-up mail, I agree with that; your proposed change looks correct. > > If you want to change these calculations you need to make a stronger > > case for it and verify that we don't mess up some other rate > > inadvertently. > I have done a calculation which compares the error of the baud rate > calculation going all the way from 1 to 3MBaud where it can be seen that > the rounding (as expected) halves the maximum error. Whereas the old method > went up to 12% baud rate error, the new method reaches 6%, so the range > of baud rates where communication will be successful should increase. Also, > the new method is always better or as-accurate as the old. Excellent, thanks for confirming. Just mention something about that in the commit message too. > I guess that image attachments are not welcome in the mailing list, so > I will refrain from attaching it. Let me know if I should send it to > you. Sure, if you want too that'd be great. > I am using it in a system which uses a baud rate of 961200 (and not > the standard 921600). Here the old calculation gave an error of 4.03% > and the new gave 0.12% error. Also good to have in the commit message. > I will try to verify the numbers I have calculated with a logic analyzer to > make sure that it corresponds with the real world. I can also try to compare > it to the windows driver outputs. That would be really good, at least for a few rates. > As I only have a FT232RT (232bm) to test with, the patch should probably be > limited to the changes in the ftdi_232bm_baud_base_to_divisor() function. No, as long as you mention which device you used for testing, and the numbers for the other other types looks similar, I think we can go ahead and round those divisions too. Thanks for doing this! Johan
Re: [PATCH] Improve the accuracy of the baud rate generator by using round-to-closest instead of truncating when calculating baud rate divisors.
On Thu, Nov 15, 2018 at 09:24:49AM +0100, Johan Hovold wrote: > On Tue, Nov 13, 2018 at 08:19:44PM +0100, Nikolaj Fogh wrote: > > On 11/12/18 10:54 AM, Johan Hovold wrote: > > > On Wed, Oct 31, 2018 at 09:16:48PM +0100, Nikolaj Fogh wrote: > > >> I have experienced that the ftdi_sio driver gives less-than-optimal > > >> baud rates as the driver truncates instead of rounds to nearest > > >> during baud rate divisor calculation. > > > >> This patch improves on the baud rate generation. The generated baud > > >> rate corresponds to the optimal baud rate achievable with the chip. > > >> This is what the windows driver gives as well. > > > > > > How did you verify this? Did you trace and compare the divisors > > > actually requested by the Windows driver, or did you measure the > > > resulting rates using a scope? > > > I verified it by scope. Granted, I only verified it for one baud rate > > (961200). Whether it gives the same as the Windows driver in general, > > I'm not sure. However, I would think that rounding instead of flooring > > would always yield the most accurate result. > > I'm not so sure in this case. The driver uses "sub-integer" divisors and > looks like it depends on truncation rather than rounding. Some > background here: > > > https://www.ftdichip.com/Support/Knowledgebase/index.html?whatbaudratesareachieveabl.htm After taking a closer look at the driver, I think your patch may be fine. It shouldn't change 921600 baud, though, but I see now that you wrote *961200* above. Was that intentional? > If you want to change these calculations you need to make a stronger > case for it and verify that we don't mess up some other rate > inadvertently. This still stands though, you need a better description of why this is needed and correct. Mention which device and rate that was off, and by how much. Determining the theoretical difference may be interesting too, while making sure we don't introduce larger errors for other rates. Thanks, Johan
Re: [PATCH] Improve the accuracy of the baud rate generator by using round-to-closest instead of truncating when calculating baud rate divisors.
On Tue, Nov 13, 2018 at 08:19:44PM +0100, Nikolaj Fogh wrote: > On 11/12/18 10:54 AM, Johan Hovold wrote: > > On Wed, Oct 31, 2018 at 09:16:48PM +0100, Nikolaj Fogh wrote: > >> I have experienced that the ftdi_sio driver gives less-than-optimal > >> baud rates as the driver truncates instead of rounds to nearest > >> during baud rate divisor calculation. > >> This patch improves on the baud rate generation. The generated baud > >> rate corresponds to the optimal baud rate achievable with the chip. > >> This is what the windows driver gives as well. > > > > How did you verify this? Did you trace and compare the divisors > > actually requested by the Windows driver, or did you measure the > > resulting rates using a scope? > I verified it by scope. Granted, I only verified it for one baud rate > (961200). Whether it gives the same as the Windows driver in general, > I'm not sure. However, I would think that rounding instead of flooring > would always yield the most accurate result. I'm not so sure in this case. The driver uses "sub-integer" divisors and looks like it depends on truncation rather than rounding. Some background here: https://www.ftdichip.com/Support/Knowledgebase/index.html?whatbaudratesareachieveabl.htm If you want to change these calculations you need to make a stronger case for it and verify that we don't mess up some other rate inadvertently. Thanks, Johan
Re: Adding NovAtel USB vendor & device ID to Kernel
On Thu, Nov 08, 2018 at 01:07:47AM +, SNELL James wrote: > Hello, > We produce extremely high-end GNSS (GPS, etc) receivers that are often > used for a very wide range of applications. Our receivers can be > connected to via USB, which will provide 3 USB-to-serial ports that > can be used to issue commands and get receiver data. Do your products support other serial interfaces as well (e.g. UART or i2c)? > We typically get Linux users to create a udev file so their systems > attach the USB serial ports to /dev. > > I just noticed that when my receiver enumerates, dmesg outputs: > [ 414.374523] usb 1-1.1.3: new full-speed USB device number 8 using dwc_otg > [ 414.508473] usb 1-1.1.3: New USB device found, idVendor=09d7, > idProduct=0100 > [ 414.508488] usb 1-1.1.3: New USB device strings: Mfr=1, Product=2, > SerialNumber=3 > [ 414.508497] usb 1-1.1.3: Product: NovAtel GPS Receiver > [ 414.508505] usb 1-1.1.3: Manufacturer: NovAtel Inc. > [ 414.508514] usb 1-1.1.3: SerialNumber: DMGW18050122R > [ 414.511608] usbserial_generic 1-1.1.3:1.0: The "generic" usb-serial driver > is only for testing and one-off prototypes. > [ 414.511624] usbserial_generic 1-1.1.3:1.0: Tell > mailto:linux-usb@vger.kernel.org to add your device to a proper driver. > [ 414.511636] usbserial_generic 1-1.1.3:1.0: generic converter detected > [ 414.512004] usb 1-1.1.3: generic converter now attached to ttyUSB0 > [ 414.512352] usb 1-1.1.3: generic converter now attached to ttyUSB1 > [ 414.512805] usb 1-1.1.3: generic converter now attached to ttyUSB2 > # lsusb -s 001:008 -v > I think it would be "nice" if our receiver's USB-delivered serial > ports attached to /dev as /dev/novatel0, .. /dev/novatelN or > (/dev/gnss0 .. /dev/gnssN). Though if they continued to appear as > /dev/ttyUSB0 .. /dev/ttyUSBN, that'd also be great. As Oliver mentioned it would be useful to know what USB-serial converter chip you use in the device, if any, or of this is a custom implementation. Since 4.19 we actually have GNSS subsystem in the kernel, which if we implement a driver for your devices would show up as /dev/gnssN with an associated attribute describing the GNSS type (reflecting the supported protocol, e.g. NMEA, UBX or SiRF). What protocols do your devices use (besides NMEA)? Do use any common GNSS receivers chips, or do you have your own? What are the three ports used for, or is that configurable? > I'm not entirely sure if the dmesg output that's directed me here is > really intended for this sort of request. If not, I'm willing to make > my own git merge request, though I've not toyed much with the Linux > Kernel, so tips would be extremely appreciated. > > Can we please get our Vendor ID (0x09d7) and Product ID (0x0100) added > to the Kernel in a sensical manner? Depending a bit on your answers to the above questions, we could also add your VID/PID to a USB-serial driver, which would give you ttyUSBN devices without any need to mess with modprobe. > The information contained in this e-mail may contain confidential or > privileged material and is intended only for the stated addressee(s). > If you are not a valid addressee, the use, disclosure, copying or > distribution of this information is prohibited and may be unlawful. > If you have received this message in error, please notify the sender > immediately and delete all copies of the message from your computer. > Notwithstanding any applicable legislation which may provide for > contracts to be formed from electronic communication, this email does > not create, form part of, or vary any contract, nor is it otherwise > intended to bind any Hexagon group company. You need to remove this footer when dealing with the mailing lists, though. Thanks, Johan
Re: [PATCH] Improve the accuracy of the baud rate generator by using round-to-closest instead of truncating when calculating baud rate divisors.
On Wed, Oct 31, 2018 at 09:16:48PM +0100, Nikolaj Fogh wrote: > I have experienced that the ftdi_sio driver gives less-than-optimal baud > rates as the driver truncates instead of rounds to nearest during baud rate > divisor calculation. Please break your lines at 72 cols or so, and use the common subject prefix (e.g. "USB: serial: ftdi_sio: improve...") with a concise summary. > This patch improves on the baud rate generation. The generated baud rate > corresponds to the optimal baud rate achievable with the chip. This is what > the windows driver gives as well. How did you verify this? Did you trace and compare the divisors actually requested by the Windows driver, or did you measure the resulting rates using a scope? Thanks, Johan
Re: [PATCH -next] USB: serial: quatech2: remove set but not used variable 'port_priv'
On Thu, Oct 11, 2018 at 07:44:33AM +, YueHaibing wrote:
> Fixes gcc '-Wunused-but-set-variable' warning:
>
> drivers/usb/serial/quatech2.c: In function 'qt2_process_read_urb':
> drivers/usb/serial/quatech2.c:503:27: warning:
> variable 'port_priv' set but not used [-Wunused-but-set-variable]
>
> It not used any more after
> commit 2be818a116b2 ("Revert "USB: quatech2: only write to the tty if the
> port is open."")
>
> Signed-off-by: YueHaibing
Now applied. Thanks for including the not-used-since information.
Johan
Re: Prolific: PL2303G Linux driver ( new USB to UART chip)
On Mon, Oct 29, 2018 at 10:24:26AM +, Yeh.Charles [葉榮鑫] wrote: > Hi Greg, > Because the "confidential" message is auto add by Prolific's > email server. I will use "charlesyeh...@gmail.com" to avoid > "confidential message". Sounds good. > And I will use Linux kernel 4.4 (pl2303.c & pl2303.h) to make > patch file, is it OK? No, please use the latest mainline release, which currently is 4.19. Thanks, Johan
[GIT PULL] USB-serial updates for v4.20-rc1
The following changes since commit 7876320f88802b22d4e2daf7eb027dd14175a0f8: Linux 4.19-rc4 (2018-09-16 11:52:37 -0700) are available in the Git repository at: https://git.kernel.org/pub/scm/linux/kernel/git/johan/usb-serial.git tags/usb-serial-4.20-rc1 for you to fetch changes up to 17c42e34997ae172c794f84fefe47f00bec13f9a: USB: serial: cypress_m8: remove set but not used variable 'iflag' (2018-10-05 08:58:42 +0200) USB-serial updates for v4.20-rc1 Here are the USB-serial updates for 4.20-rc1, including: - support for CBUS GPIO on FTDI devices (FTX and FT232R) - fix of a long-standing transfer-length bug Included are also various clean ups. All have been in linux-next with no reported issues. Signed-off-by: Johan Hovold Colin Ian King (1): USB: serial: cypress_m8: fix spelling mistake "retreiving" -> "retrieving" Johan Hovold (3): USB: serial: ftdi_sio: fix gpio name collisions USB: serial: ftdi_sio: add support for FT232R CBUS gpios USB: serial: cypress_m8: fix interrupt-out transfer length Karoly Pados (1): USB: serial: ftdi_sio: implement GPIO support for FT-X devices YueHaibing (1): USB: serial: cypress_m8: remove set but not used variable 'iflag' drivers/usb/serial/cypress_m8.c | 7 +- drivers/usb/serial/ftdi_sio.c | 391 +++- drivers/usb/serial/ftdi_sio.h | 28 ++- 3 files changed, 420 insertions(+), 6 deletions(-)
Re: [PATCH v2 -next] USB: cypress_m8: remove set but not used variables 'iflag'
On Thu, Oct 04, 2018 at 07:09:53AM +, YueHaibing wrote: > Fixes gcc '-Wunused-but-set-variable' warning: > > drivers/usb/serial/cypress_m8.c: In function 'cypress_set_termios': > drivers/usb/serial/cypress_m8.c:866:18: warning: > variable 'iflag' set but not used [-Wunused-but-set-variable] > > Signed-off-by: YueHaibing > --- > v2: only fix 'iflag' warning Now applied, thanks. Johan
[GIT PULL] USB-serial fixes for v4.19-rc7
Hi Greg, Here are some device-id patches for 4.19-rc7 (unless you prefer to hold them off for 4.20). The option patches are a bit more intrusive than the usual device-id patches, but allows us to support a particular Quectel modem in non-standard configurations. To simplify dealing with stable-backports these also carry a stable tag. Thanks, Johan The following changes since commit 5dfdd24eb3d39d815bc952ae98128e967c9bba49: USB: serial: ti_usb_3410_5052: fix array underflow in completion handler (2018-08-27 11:53:19 +0200) are available in the Git repository at: https://git.kernel.org/pub/scm/linux/kernel/git/johan/usb-serial.git tags/usb-serial-4.19-rc7 for you to fetch changes up to f5fad711c06e652f90f581fc7c2caee327c33d31: USB: serial: simple: add Motorola Tetra MTP6550 id (2018-09-24 15:30:16 +0200) USB-serial fixes for v4.19-rc7 Here are some device-id patches for 4.19-rc7. Some Quectel modems have a vendor command which can be used to disable certain interfaces in their configurations, but unlike some other modems this also causes the interface numbers to change. These patches allow us to support all such interface permutations at least for the Quectel EP06. All have been in linux-next with no reported issues. Signed-off-by: Johan Hovold Johan Hovold (2): USB: serial: option: add two-endpoints device-id flag USB: serial: simple: add Motorola Tetra MTP6550 id Kristian Evensen (1): USB: serial: option: improve Quectel EP06 detection drivers/usb/serial/option.c| 15 +-- drivers/usb/serial/usb-serial-simple.c | 3 ++- 2 files changed, 15 insertions(+), 3 deletions(-)
[PATCH] USB: serial: cypress_m8: fix interrupt-out transfer length
Fix interrupt-out transfer length which was being set to the
transfer-buffer length rather than the size of the outgoing packet.
Note that no slab data was leaked as the whole transfer buffer is always
cleared before each transfer.
Fixes: 9aa8dae7b1fa ("cypress_m8: use usb_fill_int_urb where appropriate")
Signed-off-by: Johan Hovold
---
drivers/usb/serial/cypress_m8.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/serial/cypress_m8.c b/drivers/usb/serial/cypress_m8.c
index 31c6091be46a..5aaab8f4dd8f 100644
--- a/drivers/usb/serial/cypress_m8.c
+++ b/drivers/usb/serial/cypress_m8.c
@@ -769,7 +769,7 @@ static void cypress_send(struct usb_serial_port *port)
usb_fill_int_urb(port->interrupt_out_urb, port->serial->dev,
usb_sndintpipe(port->serial->dev,
port->interrupt_out_endpointAddress),
- port->interrupt_out_buffer, port->interrupt_out_size,
+ port->interrupt_out_buffer, actual_size,
cypress_write_int_callback, port, priv->write_urb_interval);
result = usb_submit_urb(port->interrupt_out_urb, GFP_ATOMIC);
if (result) {
--
2.19.0
Re: [PATCH -next] USB: cypress_m8: remove set but not used variables 'actual_size, iflag'
On Sat, Sep 29, 2018 at 09:54:03AM +, YueHaibing wrote:
> Fixes gcc '-Wunused-but-set-variable' warning:
>
> drivers/usb/serial/cypress_m8.c: In function 'cypress_send':
> drivers/usb/serial/cypress_m8.c:689:33: warning:
> variable 'actual_size' set but not used [-Wunused-but-set-variable]
>
> drivers/usb/serial/cypress_m8.c: In function 'cypress_set_termios':
> drivers/usb/serial/cypress_m8.c:866:18: warning:
> variable 'iflag' set but not used [-Wunused-but-set-variable]
>
> Signed-off-by: YueHaibing
> ---
> drivers/usb/serial/cypress_m8.c | 11 ++-
> 1 file changed, 2 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/usb/serial/cypress_m8.c b/drivers/usb/serial/cypress_m8.c
> index 31c6091..98dff12 100644
> --- a/drivers/usb/serial/cypress_m8.c
> +++ b/drivers/usb/serial/cypress_m8.c
> @@ -686,7 +686,7 @@ static int cypress_write(struct tty_struct *tty, struct
> usb_serial_port *port,
>
> static void cypress_send(struct usb_serial_port *port)
> {
> - int count = 0, result, offset, actual_size;
> + int count = 0, result, offset;
> struct cypress_private *priv = usb_get_serial_port_data(port);
> struct device *dev = >dev;
> unsigned long flags;
> @@ -758,12 +758,6 @@ static void cypress_send(struct usb_serial_port *port)
> priv->write_urb_in_use = 1;
> spin_unlock_irqrestore(>lock, flags);
>
> - if (priv->cmd_ctrl)
> - actual_size = 1;
> - else
> - actual_size = count +
> - (priv->pkt_fmt == packet_format_1 ? 2 : 1);
> -
This looks like we have a bug in the driver, and sure enough we do.
Commit 9aa8dae7b1fa ("cypress_m8: use usb_fill_int_urb where
appropriate") incorrectly started setting the transfer length to the
size of the buffer.
I've prepared a fix to this.
> usb_serial_debug_data(dev, __func__, port->interrupt_out_size,
> port->interrupt_out_urb->transfer_buffer);
>
> @@ -863,7 +857,7 @@ static void cypress_set_termios(struct tty_struct *tty,
> struct cypress_private *priv = usb_get_serial_port_data(port);
> struct device *dev = >dev;
> int data_bits, stop_bits, parity_type, parity_enable;
> - unsigned cflag, iflag;
> + unsigned int cflag;
> unsigned long flags;
> __u8 oldlines;
> int linechange = 0;
> @@ -899,7 +893,6 @@ static void cypress_set_termios(struct tty_struct *tty,
> tty->termios.c_cflag &= ~(CMSPAR|CRTSCTS);
>
> cflag = tty->termios.c_cflag;
> - iflag = tty->termios.c_iflag;
>
> /* check if there are new settings */
> if (old_termios) {
Would you mind resending just this chunk as a v2?
Thanks,
Johan
[PATCH] USB: serial: simple: add Motorola Tetra MTP6550 id
Add device-id for the Motorola Tetra radio MTP6550.
Bus 001 Device 004: ID 0cad:9012 Motorola CGISS
Device Descriptor:
bLength18
bDescriptorType 1
bcdUSB 2.00
bDeviceClass0 (Defined at Interface level)
bDeviceSubClass 0
bDeviceProtocol 0
bMaxPacketSize064
idVendor 0x0cad Motorola CGISS
idProduct 0x9012
bcdDevice 24.16
iManufacturer 1 Motorola Solutions, Inc.
iProduct2 TETRA PEI interface
iSerial 0
bNumConfigurations 1
Configuration Descriptor:
bLength 9
bDescriptorType 2
wTotalLength 55
bNumInterfaces 2
bConfigurationValue 1
iConfiguration 3 Generic Serial config
bmAttributes 0x80
(Bus Powered)
MaxPower 500mA
Interface Descriptor:
bLength 9
bDescriptorType 4
bInterfaceNumber0
bAlternateSetting 0
bNumEndpoints 2
bInterfaceClass 255 Vendor Specific Class
bInterfaceSubClass 0
bInterfaceProtocol 0
iInterface 0
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x81 EP 1 IN
bmAttributes2
Transfer TypeBulk
Synch Type None
Usage Type Data
wMaxPacketSize 0x0200 1x 512 bytes
bInterval 0
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x01 EP 1 OUT
bmAttributes2
Transfer TypeBulk
Synch Type None
Usage Type Data
wMaxPacketSize 0x0200 1x 512 bytes
Interface Descriptor:
bLength 9
bDescriptorType 4
bInterfaceNumber1
bAlternateSetting 0
bNumEndpoints 2
bInterfaceClass 255 Vendor Specific Class
bInterfaceSubClass 0
bInterfaceProtocol 0
iInterface 0
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x82 EP 2 IN
bmAttributes2
Transfer TypeBulk
Synch Type None
Usage Type Data
wMaxPacketSize 0x0200 1x 512 bytes
bInterval 0
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x02 EP 2 OUT
bmAttributes2
Transfer TypeBulk
Synch Type None
Usage Type Data
wMaxPacketSize 0x0200 1x 512 bytes
bInterval 0
Device Qualifier (for other device speed):
bLength10
bDescriptorType 6
bcdUSB 2.00
bDeviceClass0 (Defined at Interface level)
bDeviceSubClass 0
bDeviceProtocol 0
bMaxPacketSize064
bNumConfigurations 1
Device Status: 0x
(Bus Powered)
Reported-by: Hans Hult
Cc: stable
Signed-off-by: Johan Hovold
---
drivers/usb/serial/usb-serial-simple.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/serial/usb-serial-simple.c
b/drivers/usb/serial/usb-serial-simple.c
index 40864c2bd9dc..4d0273508043 100644
--- a/drivers/usb/serial/usb-serial-simple.c
+++ b/drivers/usb/serial/usb-serial-simple.c
@@ -84,7 +84,8 @@ DEVICE(moto_modem, MOTO_IDS);
/* Motorola Tetra driver */
#define MOTOROLA_TETRA_IDS() \
- { USB_DEVICE(0x0cad, 0x9011) } /* Motorola Solutions TETRA PEI */
+ { USB_DEVICE(0x0cad, 0x9011) }, /* Motorola Solutions TETRA PEI */ \
+ { USB_DEVICE(0x0cad, 0x9012) } /* MTP6550 */
DEVICE(motorola_tetra, MOTOROLA_TETRA_IDS);
/* Novatel Wireless GPS driver */
--
2.19.0
[GIT PULL] USB-serial fixes for v4.19-rc3
The following changes since commit 5b394b2ddf0347bef56e50c69a58773c94343ff3: Linux 4.19-rc1 (2018-08-26 14:11:59 -0700) are available in the Git repository at: https://git.kernel.org/pub/scm/linux/kernel/git/johan/usb-serial.git tags/usb-serial-4.19-rc3 for you to fetch changes up to 5dfdd24eb3d39d815bc952ae98128e967c9bba49: USB: serial: ti_usb_3410_5052: fix array underflow in completion handler (2018-08-27 11:53:19 +0200) USB-serial fixes for v4.19-rc3 Here are two fixes for array-underflow bugs in completion handlers due to insufficient sanity checks. All have been in linux-next with no reported issues. Signed-off-by: Johan Hovold Johan Hovold (2): USB: serial: io_ti: fix array underflow in completion handler USB: serial: ti_usb_3410_5052: fix array underflow in completion handler drivers/usb/serial/io_ti.h| 2 +- drivers/usb/serial/ti_usb_3410_5052.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
Re: [PATCH v3] USB: serial: ftdi_sio: Add support for CBUS GPIO
On Wed, Sep 05, 2018 at 08:44:12AM +0200, Loic Poulain wrote: > Thanks for review, I'm fine with that and I'll review the other patch. > Karoly can use chunks of my patch if useful. Thanks a lot. And if chunks can be reused, that would be great. Johan
Re: [PATCH v3] USB: serial: ftdi_sio: Add support for CBUS GPIO
On Sat, Aug 04, 2018 at 12:17:15PM +0200, Loic Poulain wrote: > Some FTDI devices like FTX or FT232R support CBUS Bit Bang mode on CBUS > pins, allowing host to control them via simple USB control transfers. > To make use of a CBUS pin in Bit Bang mode, the pin must be configured > to I/O mode in the FTDI EEPROM. This mode perfectly coexists with regular > USB to Serial function. > > In this implementation, a GPIO controller is registered on FTDI probe > if at least one CBUS pin is configured for I/O mode. For now, only > FTX devices are supported. > > This patch is based on previous Stefan Agner implementation tentative > on LKML [1]. > > [1] Message-Id: 1434838377-8042-1-git-send-email-ste...@agner.ch > > Signed-off-by: Loic Poulain > --- > v2: Use message-id for LKML reference > Rework read_eeprom according to Andy's comment and return read count > Remove noisy messages > Comment style alignment > Add defines for magic values > Cannot use devm, because gpiochip is linked to the port not to the udev > v3: >Fix typo in CBUS mask description comment First, thanks for looking into this; seems like we're finally about to get support for the CBUS gpios. But now I get not one, but two, competing implementations for this posted in one month: https://lkml.kernel.org/r/20180825204744.2307-1-pa...@pados.hu And it looks like you both have been considering at least some of the earlier attempts, which is great. I've reviewed both patches and it seems to me that Karoly's version is closer to what I'd like to see as the end-result. Specifically this means having: - fixed offsets for the physical pins rather than having the offsets depend on eeprom configuration - better type abstraction (we want to add support for ft232r and ft232h eventually as well) The other patch is also more complete in that it: - considers the initial pin state - implements a request() callback - implements a get_direction() callback In contrast, with this implementation as it stands, we could end up with a driven pin being reported as an input (corner case, but still). Implementation-wise, the other patch is also closer to what I'd like to see (e.g. not using atomic bit ops, and getting the error handling right from the start). There are some issues that needs to be addressed in the other patch as well, and in doing so it would be wise to look at your patch for inspiration (e.g. naming issues and adding an eeprom helper to only read the couple of configuration words needed). In the end, going with the other patch means less work for me, even if you both (by unfortunate timing) have forced me to do more than twice the amount of review work already. Hopefully we can all work together on getting the missing bits, including further device support, in place. For completeness, I've included my review comments below. Thanks, Johan > drivers/usb/serial/Kconfig| 9 ++ > drivers/usb/serial/ftdi_sio.c | 238 > ++ > drivers/usb/serial/ftdi_sio.h | 83 +++ > 3 files changed, 330 insertions(+) > > diff --git a/drivers/usb/serial/Kconfig b/drivers/usb/serial/Kconfig > index 533f127..64c9f2e 100644 > --- a/drivers/usb/serial/Kconfig > +++ b/drivers/usb/serial/Kconfig > @@ -181,6 +181,15 @@ config USB_SERIAL_FTDI_SIO > To compile this driver as a module, choose M here: the > module will be called ftdi_sio. > > +config USB_SERIAL_FTDI_SIO_CBUS_GPIO > + bool "USB FDTI CBUS GPIO support" > + depends on USB_SERIAL_FTDI_SIO > + depends on GPIOLIB > + help > + Say yes here to add support for the CBUS bit-bang mode, allowing CBUS > + pins to act as GPIOs. Note that pins must first be configured for GPIO > + in the device's EEPROM. The FT232R and FT-X series support this mode. I understand you're referring to hardware, but this is ambiguous as you don't implement support for FT232R in the driver. > + > config USB_SERIAL_VISOR > tristate "USB Handspring Visor / Palm m50x / Sony Clie Driver" > help > diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c > index b5cef32..e90cae6 100644 > --- a/drivers/usb/serial/ftdi_sio.c > +++ b/drivers/usb/serial/ftdi_sio.c > @@ -33,6 +33,7 @@ > #include > #include > #include > +#include > #include > #include > #include > @@ -40,12 +41,21 @@ > #include > #include > #include > + Unrelated change. > #include "ftdi_sio.h" > #include "ftdi_sio_ids.h" > > #define DRIVER_AUTHOR "Greg Kroah-Hartman , Bill Ryder > , Kuba Ober , Andreas Mohr, Johan > Hovold " > #define DRIVER_DESC "USB FTDI Serial Converters Driver" >
Re: [PATCH] USB: change dev_WARN to dev_err triggerable from user space
On Tue, Sep 04, 2018 at 12:21:09PM +0200, Oliver Neukum wrote:
> On Di, 2018-09-04 at 11:31 +0200, Johan Hovold wrote:
> > On Tue, Sep 04, 2018 at 10:44:41AM +0200, Oliver Neukum wrote:
> > > For those people who run with panic_on_warn a WARN() triggered
> > > from user space is a DOS. It is worth returning to dev_err()
> >
> > I think this should be dev_warn() unless you want to bring back the
> > returning of errors on these conditions as well (i.e. as was the case
> > prior to 0cb54a3e47cb ("USB: debugging code shouldn't alter control
> > flow")).
>
> Should I? A warning in syslog is pretty hardcore, so I have no idea
> whether dev_warn() is enough.
Perhaps there are two sides to this. If something really should not be
happening and needs to be addressed (i.e. it's a driver bug) that
dev_WARN is warranted. If user space can be pass in bogus flags that
gets propagated to USB core, perhaps those need to be sanitised sooner
(in the vain of "don't trust anything coming from user space").
In general though, I believe dev_warn() is more appropriate for cases
where something odd is happening, but we try to recover and proceed
anyway (e.g. by sanitising the flags without bailing out as is the case
here) instead of aborting.
Johan
Re: [PATCH] USB: change dev_WARN to dev_err triggerable from user space
On Tue, Sep 04, 2018 at 10:44:41AM +0200, Oliver Neukum wrote:
> For those people who run with panic_on_warn a WARN() triggered
> from user space is a DOS. It is worth returning to dev_err()
I think this should be dev_warn() unless you want to bring back the
returning of errors on these conditions as well (i.e. as was the case
prior to 0cb54a3e47cb ("USB: debugging code shouldn't alter control
flow")).
> Signed-off-by: Oliver Neukum
> Fixes: 0cb54a3e47cb4baf0bc7463f0a64cfeae5e35697
This should be:
Fixes: 0cb54a3e47cb ("USB: debugging code shouldn't alter control flow")
> Reported-by: syzbot+843efa30c8821bd69...@syzkaller.appspotmail.com
> ---
> drivers/usb/core/urb.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/usb/core/urb.c b/drivers/usb/core/urb.c
> index f51750bcd152..3fe65a774e6c 100644
> --- a/drivers/usb/core/urb.c
> +++ b/drivers/usb/core/urb.c
> @@ -475,7 +475,7 @@ int usb_submit_urb(struct urb *urb, gfp_t mem_flags)
>
> /* Check that the pipe's type matches the endpoint's type */
> if (usb_urb_ep_type_check(urb))
> - dev_WARN(>dev, "BOGUS urb xfer, pipe %x != type %x\n",
> + dev_err(>dev, "BOGUS urb xfer, pipe %x != type %x\n",
> usb_pipetype(urb->pipe), pipetypes[xfertype]);
>
> /* Check against a simple/standard policy */
> @@ -499,7 +499,7 @@ int usb_submit_urb(struct urb *urb, gfp_t mem_flags)
>
> /* warn if submitter gave bogus flags */
> if (allowed != urb->transfer_flags)
> - dev_WARN(>dev, "BOGUS urb flags, %x --> %x\n",
> + dev_err(>dev, "BOGUS urb flags, %x --> %x\n",
> urb->transfer_flags, allowed);
>
> /*
Johan
[PATCH 1/2] USB: serial: io_ti: fix array underflow in completion handler
As reported by Dan Carpenter, a malicious USB device could set
port_number to -3 and we would underflow the port array in the interrupt
completion handler.
As these devices only have one or two ports, fix this by making sure we
only consider the seventh bit when determining the port number (and
ignore bits 0xb0 which are typically set to 0x30).
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable
Reported-by: Dan Carpenter
Signed-off-by: Johan Hovold
---
drivers/usb/serial/io_ti.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/serial/io_ti.h b/drivers/usb/serial/io_ti.h
index e53c68261017..9bbcee37524e 100644
--- a/drivers/usb/serial/io_ti.h
+++ b/drivers/usb/serial/io_ti.h
@@ -173,7 +173,7 @@ struct ump_interrupt {
} __attribute__((packed));
-#define TIUMP_GET_PORT_FROM_CODE(c)(((c) >> 4) - 3)
+#define TIUMP_GET_PORT_FROM_CODE(c)(((c) >> 6) & 0x01)
#define TIUMP_GET_FUNC_FROM_CODE(c)((c) & 0x0f)
#define TIUMP_INTERRUPT_CODE_LSR 0x03
#define TIUMP_INTERRUPT_CODE_MSR 0x04
--
2.18.0
[PATCH 2/2] USB: serial: ti_usb_3410_5052: fix array underflow in completion handler
Similarly to a recently reported bug in io_ti, a malicious USB device
could set port_number to -3 and we would underflow the port array in the
interrupt completion handler.
As these devices only have one or two ports, fix this by making sure we
only consider the seventh bit when determining the port number (and
ignore bits 0xb0 which are typically set to 0x30).
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable
Signed-off-by: Johan Hovold
---
drivers/usb/serial/ti_usb_3410_5052.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/serial/ti_usb_3410_5052.c
b/drivers/usb/serial/ti_usb_3410_5052.c
index 3010878f7f8e..e3c5832337e0 100644
--- a/drivers/usb/serial/ti_usb_3410_5052.c
+++ b/drivers/usb/serial/ti_usb_3410_5052.c
@@ -1119,7 +1119,7 @@ static void ti_break(struct tty_struct *tty, int
break_state)
static int ti_get_port_from_code(unsigned char code)
{
- return (code >> 4) - 3;
+ return (code >> 6) & 0x01;
}
static int ti_get_func_from_code(unsigned char code)
--
2.18.0
Re: [PATCH] USB: serial: io_ti: array underflow in edge_interrupt_callback()
On Tue, Aug 14, 2018 at 12:07:15PM +0300, Dan Carpenter wrote:
> A malicious USB device could set port_number to -3 and we would
> underflow the edge_serial->serial->port[] array.
Good catch!
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Dan Carpenter
>
> diff --git a/drivers/usb/serial/io_ti.c b/drivers/usb/serial/io_ti.c
> index 6d1d6efa3055..fa2af18c6efe 100644
> --- a/drivers/usb/serial/io_ti.c
> +++ b/drivers/usb/serial/io_ti.c
> @@ -1629,7 +1629,7 @@ static void edge_interrupt_callback(struct urb *urb)
> struct device *dev;
> unsigned char *data = urb->transfer_buffer;
> int length = urb->actual_length;
> - int port_number;
> + unsigned int port_number;
I'd prefer fixing the macro to never return a negative port number in
the first place instead of relying on conversion rules. These devices
only come with one or two ports and, while the protocol documentation is
hard to come by, it seems what we really want to do is just to check the
7th bit and thus change:
-#define TIUMP_GET_PORT_FROM_CODE(c)(((c) >> 4) - 3)
+#define TIUMP_GET_PORT_FROM_CODE(c)(((c) >> 6) & 0x01)
I only have a one-port device to test with, but I can at least confirm
that bits 0x30 are always set and that that's probably why subtraction
was used instead of masking out the relevant bit (i.e. it happened to
work).
This also avoids error messages such as
bad port number 4294967293
should the ignored bits (0xb0) have unexpected values (e.g. 0).
> int function;
> int retval;
> __u8 lsr;
Turns out we had the same issue in ti_usb_3410_5052 which is based on
io_ti, but where a recent change converted the port macro to a static
helper. In case you found this using your static checkers, you may want
to look into why it didn't catch that one.
I'll submit two fixes to address this as per above.
Thanks,
Johan
Re: [PATCH v3] USB: serial: ftdi_sio: Add support for CBUS GPIO
On Mon, Aug 20, 2018 at 05:19:40PM +0200, Loic Poulain wrote: > Hi Johan, > > On 4 August 2018 at 12:17, Loic Poulain wrote: > > Some FTDI devices like FTX or FT232R support CBUS Bit Bang mode on CBUS > > pins, allowing host to control them via simple USB control transfers. > > To make use of a CBUS pin in Bit Bang mode, the pin must be configured > > to I/O mode in the FTDI EEPROM. This mode perfectly coexists with regular > > USB to Serial function. > > > > In this implementation, a GPIO controller is registered on FTDI probe > > if at least one CBUS pin is configured for I/O mode. For now, only > > FTX devices are supported. > > > > This patch is based on previous Stefan Agner implementation tentative > > on LKML [1]. > > > > [1] Message-Id: 1434838377-8042-1-git-send-email-ste...@agner.ch > > > > Signed-off-by: Loic Poulain > > Any comments on this v3? I'm just back from vacation and haven't had time to look at this one yet at all. Sorry. Johan
Re: cdc-acm linux kernel
A: No. Q: Should I include quotations after my reply? On Mon, Aug 20, 2018 at 05:57:57PM +0300, Алексей Болдырев wrote: > > Ok, and what makes you believe this chip exposes serial ports? Why > > did you think CDC-ACM would work in the first place? > > This chipset avaible from mobile phome Mikromax x806. The phone is not > android. > https://market.yandex.ru/product--telefon-micromax-x806/13874918 Ok, but I have no idea what those USB interfaces are used for. They are clearly not CDC (e.g. exposing a modem). If you have some documentation suggesting they are used for a serial-type interface, you'd need to determine what protocol is used so we could possibly add an entry to an existing driver. Johan
Re: cdc-acm linux kernel
A: Because it messes up the order in which people normally read text. Q: Why is top-posting such a bad thing? http://daringfireball.net/2007/07/on_top On Mon, Aug 20, 2018 at 04:57:23PM +0300, Алексей Болдырев wrote: > 20.08.2018, 16:54, "Johan Hovold" : > > [ Please respond inline instead of top-posting. ] > > > > On Mon, Aug 20, 2018 at 04:50:25PM +0300, Алексей Болдырев wrote: > >> 20.08.2018, 16:46, "Johan Hovold" : > >> > [ Reshuffling your reply, and responding inline below ] > >> > > >> > On Mon, Aug 20, 2018 at 04:27:37PM +0300, Алексей Болдырев wrote: > >> >> > 20.08.2018, 11:42, "Johan Hovold" : > >> >> >> [ Adding linux-usb on CC. ] > >> >> >> > >> >> >> On Fri, Aug 17, 2018 at 10:41:20PM +0300, Алексей Болдырев wrote: > >> >> >>> please add support to device from cdc-acm: > >> >> >>> > >> >> >>> Bus 004 Device 003: ID 1782:3d00 Spreadtrum Communications Inc. > >> >> >> > >> >> >> Can you please post the output of "lsusb -v" for this device? > >> > > >> >> Bus 004 Device 004: ID 1782:3d00 Spreadtrum Communications Inc. > > > > [...] > > > >> > Thanks for the details. This isn't a CDC device, so this probably needs > >> > to be handled by a USB serial driver. What kind of device is it? Does it > >> > have more than one port? > >> > > >> > Judging from the above you should get two ttyUSBx devices if you do: > >> > > >> > # modprobe usbserial > >> > # echo 1782 3d00 > /sys/bus/usb-serial/drivers/generic/new_id > >> > > >> > as root. > >> > > >> > Are those two ports usable? > > > >> I tried to do so, but for some reason I did not answer the serial > >> port? > > > > Ok, so the generic driver does not work. What kind of device is this? A > > USB-serial adapter? > > > > Do you have any way of figuring out what chip is used (e.g. an FTDI or > > pl2303 chip?), for example, by opening the device? > The device is mobile phone used chipset qualcomm snapdragon. Ok, and what makes you believe this chip exposes serial ports? Why did you think CDC-ACM would work in the first place? Perhaps what you're really after is something like adb? Johan
Re: cdc-acm linux kernel
[ Please respond inline instead of top-posting. ] On Mon, Aug 20, 2018 at 04:50:25PM +0300, Алексей Болдырев wrote: > 20.08.2018, 16:46, "Johan Hovold" : > > [ Reshuffling your reply, and responding inline below ] > > > > On Mon, Aug 20, 2018 at 04:27:37PM +0300, Алексей Болдырев wrote: > >> > 20.08.2018, 11:42, "Johan Hovold" : > >> >> [ Adding linux-usb on CC. ] > >> >> > >> >> On Fri, Aug 17, 2018 at 10:41:20PM +0300, Алексей Болдырев wrote: > >> >>> please add support to device from cdc-acm: > >> >>> > >> >>> Bus 004 Device 003: ID 1782:3d00 Spreadtrum Communications Inc. > >> >> > >> >> Can you please post the output of "lsusb -v" for this device? > > > >> Bus 004 Device 004: ID 1782:3d00 Spreadtrum Communications Inc. [...] > > Thanks for the details. This isn't a CDC device, so this probably needs > > to be handled by a USB serial driver. What kind of device is it? Does it > > have more than one port? > > > > Judging from the above you should get two ttyUSBx devices if you do: > > > > # modprobe usbserial > > # echo 1782 3d00 > /sys/bus/usb-serial/drivers/generic/new_id > > > > as root. > > > > Are those two ports usable? > I tried to do so, but for some reason I did not answer the serial > port? Ok, so the generic driver does not work. What kind of device is this? A USB-serial adapter? Do you have any way of figuring out what chip is used (e.g. an FTDI or pl2303 chip?), for example, by opening the device? Thanks, Johan
Re: cdc-acm linux kernel
[ Reshuffling your reply, and responding inline below ] On Mon, Aug 20, 2018 at 04:27:37PM +0300, Алексей Болдырев wrote: > > 20.08.2018, 11:42, "Johan Hovold" : > >> [ Adding linux-usb on CC. ] > >> > >> On Fri, Aug 17, 2018 at 10:41:20PM +0300, Алексей Болдырев wrote: > >>> please add support to device from cdc-acm: > >>> > >>> Bus 004 Device 003: ID 1782:3d00 Spreadtrum Communications Inc. > >> > >> Can you please post the output of "lsusb -v" for this device? > Bus 004 Device 004: ID 1782:3d00 Spreadtrum Communications Inc. > Couldn't open device, some information will be missing > Device Descriptor: > bLength18 > bDescriptorType 1 > bcdUSB 1.10 > bDeviceClass0 (Defined at Interface level) > bDeviceSubClass 0 > bDeviceProtocol 0 > bMaxPacketSize064 > idVendor 0x1782 Spreadtrum Communications Inc. > idProduct 0x3d00 > bcdDevice0.01 > iManufacturer 0 > iProduct0 > iSerial 0 > bNumConfigurations 1 > Configuration Descriptor: > bLength 9 > bDescriptorType 2 > wTotalLength 55 > bNumInterfaces 2 > bConfigurationValue 1 > iConfiguration 0 > bmAttributes 0xc0 > Self Powered > MaxPower 100mA > Interface Descriptor: > bLength 9 > bDescriptorType 4 > bInterfaceNumber0 > bAlternateSetting 0 > bNumEndpoints 2 > bInterfaceClass 255 Vendor Specific Class > bInterfaceSubClass 0 > bInterfaceProtocol 0 > iInterface 0 > Endpoint Descriptor: > bLength 7 > bDescriptorType 5 > bEndpointAddress 0x81 EP 1 IN > bmAttributes2 > Transfer TypeBulk > Synch Type None > Usage Type Data > wMaxPacketSize 0x0040 1x 64 bytes > bInterval 0 > Endpoint Descriptor: > bLength 7 > bDescriptorType 5 > bEndpointAddress 0x02 EP 2 OUT > bmAttributes2 > Transfer TypeBulk > Synch Type None > Usage Type Data > wMaxPacketSize 0x0040 1x 64 bytes > bInterval 0 > Interface Descriptor: > bLength 9 > bDescriptorType 4 > bInterfaceNumber1 > bAlternateSetting 0 > bNumEndpoints 2 > bInterfaceClass 255 Vendor Specific Class > bInterfaceSubClass 0 > bInterfaceProtocol 0 > iInterface 0 > Endpoint Descriptor: > bLength 7 > bDescriptorType 5 > bEndpointAddress 0x83 EP 3 IN > bmAttributes2 > Transfer TypeBulk > Synch Type None > Usage Type Data > wMaxPacketSize 0x0040 1x 64 bytes > bInterval 0 > Endpoint Descriptor: > bLength 7 > bDescriptorType 5 > bEndpointAddress 0x04 EP 4 OUT > bmAttributes2 > Transfer TypeBulk > Synch Type None > Usage Type Data > wMaxPacketSize 0x0040 1x 64 bytes > bInterval 0 Thanks for the details. This isn't a CDC device, so this probably needs to be handled by a USB serial driver. What kind of device is it? Does it have more than one port? Judging from the above you should get two ttyUSBx devices if you do: # modprobe usbserial # echo 1782 3d00 > /sys/bus/usb-serial/drivers/generic/new_id as root. Are those two ports usable? Thanks, Johan
Re: cdc-acm linux kernel
[ Adding linux-usb on CC. ] On Fri, Aug 17, 2018 at 10:41:20PM +0300, Алексей Болдырев wrote: > please add support to device from cdc-acm: > > Bus 004 Device 003: ID 1782:3d00 Spreadtrum Communications Inc. Can you please post the output of "lsusb -v" for this device? Thanks, Johan
[GIT PULL] USB-serial updates for v4.19-rc1
The following changes since commit 7daf201d7fe8334e2d2364d4e8ed3394ec9af819: Linux 4.18-rc2 (2018-06-24 20:54:29 +0800) are available in the Git repository at: https://git.kernel.org/pub/scm/linux/kernel/git/johan/usb-serial.git tags/usb-serial-4.19-rc1 for you to fetch changes up to c8acfe0aadbeb78f65826959891be15cc0a709a3: USB: serial: cp210x: implement GPIO support for CP2102N (2018-07-20 18:22:52 +0200) USB-serial updates for v4.19-rc1 Here are the USB-serial updates for 4.19-rc1, including: - gpio support for CP2102N devices - improved line-speed handling for cp210x - conversion to spin_lock_irqsave() in completion handlers - dropped kl5kusb105 support from the kl5kusb105 driver (sic!) Included are also various lower-priority fixes and clean ups. All but the final commit have been in linux-next, and with no reported issues. Signed-off-by: Johan Hovold Chengguang Xu (1): USB: serial: cast sizeof() to int when comparing with error code Colin Ian King (1): USB: serial: mos7720: remove redundant variables iflag, mask and serial Johan Hovold (11): USB: serial: digi_acceleport: rename tty flag variable USB: serial: kobil_sct: fix modem-status error handling USB: serial: kobil_sct: add missing version error handling USB: serial: kl5kusb105: remove KLSI device id USB: serial: clean up kl5kusb105 documentation USB: serial: iuu_phoenix: drop unused driver-data baud rate USB: serial: iuu_phoenix: drop redundant input-speed re-encoding USB: serial: cp210x: make line-speed quantisation data driven USB: serial: cp210x: honour device-type maximum line speed USB: serial: cp210x: generalise CP2102N line-speed handling USB: serial: cp210x: improve line-speed handling for CP2104 and CP2105 John Ogness (12): USB: serial: cyberjack: use irqsave() in USB's complete callback USB: serial: digi_acceleport: use irqsave() in USB's complete callback USB: serial: io_edgeport: use irqsave() in USB's complete callback USB: serial: io_ti: use irqsave() in USB's complete callback USB: serial: mos7720: use irqsave() in USB's complete callback USB: serial: mos7840: use irqsave() in USB's complete callback USB: serial: quatech2: use irqsave() in USB's complete callback USB: serial: sierra: fix potential deadlock at close USB: serial: sierra: use irqsave() in USB's complete callback USB: serial: symbolserial: use irqsave() in USB's complete callback USB: serial: ti_usb_3410_5052: use irqsave() in USB's complete callback USB: serial: usb_wwan: use irqsave() in USB's complete callback Karoly Pados (2): USB: serial: cp210x: improve baudrate support for CP2102N USB: serial: cp210x: implement GPIO support for CP2102N Documentation/usb/usb-serial.txt | 9 - drivers/usb/serial/cp210x.c | 421 +++--- drivers/usb/serial/cyberjack.c| 17 +- drivers/usb/serial/digi_acceleport.c | 35 +-- drivers/usb/serial/io_edgeport.c | 17 +- drivers/usb/serial/io_ti.c| 5 +- drivers/usb/serial/ir-usb.c | 2 +- drivers/usb/serial/iuu_phoenix.c | 5 - drivers/usb/serial/kl5kusb105.c | 1 - drivers/usb/serial/kl5kusb105.h | 3 - drivers/usb/serial/kobil_sct.c| 24 +- drivers/usb/serial/mos7720.c | 14 +- drivers/usb/serial/mos7840.c | 5 +- drivers/usb/serial/quatech2.c | 7 +- drivers/usb/serial/sierra.c | 13 +- drivers/usb/serial/ssu100.c | 2 +- drivers/usb/serial/symbolserial.c | 5 +- drivers/usb/serial/ti_usb_3410_5052.c | 9 +- drivers/usb/serial/usb_wwan.c | 5 +- 19 files changed, 431 insertions(+), 168 deletions(-) -- 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 5/5] USB: serial: cp210x: improve line-speed handling for CP2104 and CP2105
On Wed, Jul 18, 2018 at 03:26:30PM +0200, Greg Kroah-Hartman wrote: > On Wed, Jul 18, 2018 at 02:25:01PM +0200, Johan Hovold wrote: > > CP2104 and the ECI interface of CP2105 support further baud rates than > > the ones specified in AN205 table 1, and we can use the same equations > > as for CP2102N to determine and report back the actual baud rates used. > > > > Note that this could eventually be generalised also to CP2108, which > > uses a different base clock. There appears to be an error in the CP2108 > > equations which needs to be confirmed on actual hardware first however > > (specifically, the subtraction of one from the divisor appears to be > > incorrect as it introduces larger errors). > > > > Signed-off-by: Johan Hovold > > --- > > drivers/usb/serial/cp210x.c | 9 +++-- > > 1 file changed, 7 insertions(+), 2 deletions(-) > > Reviewed-by: Greg Kroah-Hartman Thanks for reviewing, all now applied. Johan -- 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 4/5] USB: serial: cp210x: generalise CP2102N line-speed handling
On Wed, Jul 18, 2018 at 03:34:34PM +0300, Andy Shevchenko wrote:
> On Wed, Jul 18, 2018 at 3:25 PM, Johan Hovold wrote:
> > The CP2102N equations for determining the actual baud rate can be used
> > also for other device types, so let's factor it out.
> >
> > Note that this removes the now unused cp210x_is_cp2102n() helper.
>
>
> > +static speed_t cp210x_get_actual_rate(struct usb_serial *serial, speed_t
> > baud)
> > +{
> > + struct cp210x_serial_private *priv = usb_get_serial_data(serial);
> > + unsigned int prescale = 1;
> > + unsigned int div;
> > +
> > + baud = clamp(baud, 300u, priv->max_speed);
> > +
> > + if (baud <= 365)
> > + prescale = 4;
> > +
> > + div = DIV_ROUND_CLOSEST(4800, 2 * prescale * baud);
> > + baud = 4800 / (2 * prescale * div);
> > +
> > + return baud;
> > +}
>
> > - if (cp210x_is_cp2102n(serial)) {
> > - int clk_div;
> > - int prescaler;
> > -
> > - baud = clamp(baud, 300u, priv->max_speed);
> > - prescaler = (baud <= 365) ? 4 : 1;
> > - clk_div = DIV_ROUND_CLOSEST(4800, 2 * prescaler * baud);
> > - baud = 4800 / (2 * prescaler * clk_div);
> > - }
>
> Looks like ping-pong type of changes.
> I think the factoring of this particular piece of code can be done in
> patch 3 in somewhat similar way.
Indeed, and it was done like this on purpose this time to save time (and
I did not want to rewrite Karoly's patch beyond recognition).
Johan
--
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 0/5] USB: serial: cp210x: improve line-speed handling
This was all triggered by a patch from Karoly to enable line-speeds higher than 2 Mbaud for CP2102N devices. The end result is support for further baud rates for CP2104, CP2105 and CP2102N devices with the actual baud rates used now being properly reported back to user space. Johan Johan Hovold (4): USB: serial: cp210x: make line-speed quantisation data driven USB: serial: cp210x: honour device-type maximum line speed USB: serial: cp210x: generalise CP2102N line-speed handling USB: serial: cp210x: improve line-speed handling for CP2104 and CP2105 Karoly Pados (1): USB: serial: cp210x: improve baudrate support for CP2102N drivers/usb/serial/cp210x.c | 176 +++- 1 file changed, 131 insertions(+), 45 deletions(-) -- 2.18.0 -- 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 2/5] USB: serial: cp210x: honour device-type maximum line speed
Newer cp210x devices support higher line speeds than the older ones
which supported a discrete set of speeds up to 921.6 kbaud.
To support these higher speeds, we have for some time mapped speeds
lower than 1 Mbaud to the speeds supported by older devices, while
allowing the device to pick the closest possible rate for higher speeds
(without trying to guess and report back what rate was actually chosen).
As this implementation can lead to undefined behaviour for older devices
which do not support the higher rates, let's use the later-added
device-type detection to determine the maximum supported speed.
This will also be useful when adding support for cp2102n which can
handle rates up to 3 Mbaud.
As per the data sheets the following maximum speeds are used
cp2101 921.6 kbaud
cp2102/31 Mbaud
cp2104/82 Mbaud
cp2105
- ECI port 2 Mbaud
- SCI port 921.6 kbaud
while keeping the maximum 2 Mbaud for unknown device types in order to
avoid any regressions.
Signed-off-by: Johan Hovold
---
drivers/usb/serial/cp210x.c | 41 ++---
1 file changed, 38 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
index 1b380309f653..4281f2bfe0e1 100644
--- a/drivers/usb/serial/cp210x.c
+++ b/drivers/usb/serial/cp210x.c
@@ -229,6 +229,7 @@ struct cp210x_serial_private {
boolgpio_registered;
#endif
u8 partnum;
+ speed_t max_speed;
};
struct cp210x_port_private {
@@ -1052,19 +1053,20 @@ static speed_t cp210x_get_an205_rate(speed_t baud)
static void cp210x_change_speed(struct tty_struct *tty,
struct usb_serial_port *port, struct ktermios *old_termios)
{
+ struct cp210x_serial_private *priv = usb_get_serial_data(port->serial);
u32 baud;
baud = tty->termios.c_ospeed;
/* This maps the requested rate to a rate valid on cp2102 or cp2103,
-* or to an arbitrary rate in [1M,2M].
+* or to an arbitrary rate in [1M, max_speed]
*
* NOTE: B0 is not implemented.
*/
if (baud < 100)
baud = cp210x_get_an205_rate(baud);
- else if (baud > 200)
- baud = 200;
+ else if (baud > priv->max_speed)
+ baud = priv->max_speed;
dev_dbg(>dev, "%s - setting baud rate to %u\n", __func__, baud);
if (cp210x_write_u32_reg(port, CP210X_SET_BAUDRATE, baud)) {
@@ -1495,6 +1497,37 @@ static int cp210x_port_remove(struct usb_serial_port
*port)
return 0;
}
+static void cp210x_init_max_speed(struct usb_serial *serial)
+{
+ struct cp210x_serial_private *priv = usb_get_serial_data(serial);
+ speed_t max;
+
+ switch (priv->partnum) {
+ case CP210X_PARTNUM_CP2101:
+ max = 921600;
+ break;
+ case CP210X_PARTNUM_CP2102:
+ case CP210X_PARTNUM_CP2103:
+ max = 100;
+ break;
+ case CP210X_PARTNUM_CP2104:
+ case CP210X_PARTNUM_CP2108:
+ max = 200;
+ break;
+ case CP210X_PARTNUM_CP2105:
+ if (cp210x_interface_num(serial) == 0)
+ max = 200; /* ECI */
+ else
+ max = 921600; /* SCI */
+ break;
+ default:
+ max = 200;
+ break;
+ }
+
+ priv->max_speed = max;
+}
+
static int cp210x_attach(struct usb_serial *serial)
{
int result;
@@ -1515,6 +1548,8 @@ static int cp210x_attach(struct usb_serial *serial)
usb_set_serial_data(serial, priv);
+ cp210x_init_max_speed(serial);
+
if (priv->partnum == CP210X_PARTNUM_CP2105) {
result = cp2105_shared_gpio_init(serial);
if (result < 0) {
--
2.18.0
--
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 5/5] USB: serial: cp210x: improve line-speed handling for CP2104 and CP2105
CP2104 and the ECI interface of CP2105 support further baud rates than
the ones specified in AN205 table 1, and we can use the same equations
as for CP2102N to determine and report back the actual baud rates used.
Note that this could eventually be generalised also to CP2108, which
uses a different base clock. There appears to be an error in the CP2108
equations which needs to be confirmed on actual hardware first however
(specifically, the subtraction of one from the divisor appears to be
incorrect as it introduces larger errors).
Signed-off-by: Johan Hovold
---
drivers/usb/serial/cp210x.c | 9 +++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
index 957406aac9bd..4a118eb13590 100644
--- a/drivers/usb/serial/cp210x.c
+++ b/drivers/usb/serial/cp210x.c
@@ -1537,14 +1537,19 @@ static void cp210x_init_max_speed(struct usb_serial
*serial)
max = 100;
break;
case CP210X_PARTNUM_CP2104:
+ use_actual_rate = true;
+ max = 200;
+ break;
case CP210X_PARTNUM_CP2108:
max = 200;
break;
case CP210X_PARTNUM_CP2105:
- if (cp210x_interface_num(serial) == 0)
+ if (cp210x_interface_num(serial) == 0) {
+ use_actual_rate = true;
max = 200; /* ECI */
- else
+ } else {
max = 921600; /* SCI */
+ }
break;
case CP210X_PARTNUM_CP2102N_QFN28:
case CP210X_PARTNUM_CP2102N_QFN24:
--
2.18.0
--
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 4/5] USB: serial: cp210x: generalise CP2102N line-speed handling
The CP2102N equations for determining the actual baud rate can be used
also for other device types, so let's factor it out.
Note that this removes the now unused cp210x_is_cp2102n() helper.
Signed-off-by: Johan Hovold
---
drivers/usb/serial/cp210x.c | 50 -
1 file changed, 27 insertions(+), 23 deletions(-)
diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
index 3778685c7b99..957406aac9bd 100644
--- a/drivers/usb/serial/cp210x.c
+++ b/drivers/usb/serial/cp210x.c
@@ -230,6 +230,7 @@ struct cp210x_serial_private {
#endif
u8 partnum;
speed_t max_speed;
+ booluse_actual_rate;
};
struct cp210x_port_private {
@@ -457,15 +458,6 @@ struct cp210x_gpio_write {
u8 state;
} __packed;
-static bool cp210x_is_cp2102n(struct usb_serial *serial)
-{
- struct cp210x_serial_private *priv = usb_get_serial_data(serial);
-
- return (priv->partnum == CP210X_PARTNUM_CP2102N_QFN28) ||
- (priv->partnum == CP210X_PARTNUM_CP2102N_QFN24) ||
- (priv->partnum == CP210X_PARTNUM_CP2102N_QFN20);
-}
-
/*
* Helper to get interface number when we only have struct usb_serial.
*/
@@ -1036,6 +1028,23 @@ static speed_t cp210x_get_an205_rate(speed_t baud)
return cp210x_an205_table1[i].rate;
}
+static speed_t cp210x_get_actual_rate(struct usb_serial *serial, speed_t baud)
+{
+ struct cp210x_serial_private *priv = usb_get_serial_data(serial);
+ unsigned int prescale = 1;
+ unsigned int div;
+
+ baud = clamp(baud, 300u, priv->max_speed);
+
+ if (baud <= 365)
+ prescale = 4;
+
+ div = DIV_ROUND_CLOSEST(4800, 2 * prescale * baud);
+ baud = 4800 / (2 * prescale * div);
+
+ return baud;
+}
+
/*
* CP2101 supports the following baud rates:
*
@@ -1072,25 +1081,17 @@ static void cp210x_change_speed(struct tty_struct *tty,
baud = tty->termios.c_ospeed;
/*
-* This maps the requested rate to the actual rate on cp2102n, a valid
-* rate on cp2102 or cp2103, or to an arbitrary rate in
-* [1M, max_speed].
+* This maps the requested rate to the actual rate, a valid rate on
+* cp2102 or cp2103, or to an arbitrary rate in [1M, max_speed].
*
* NOTE: B0 is not implemented.
*/
- if (cp210x_is_cp2102n(serial)) {
- int clk_div;
- int prescaler;
-
- baud = clamp(baud, 300u, priv->max_speed);
- prescaler = (baud <= 365) ? 4 : 1;
- clk_div = DIV_ROUND_CLOSEST(4800, 2 * prescaler * baud);
- baud = 4800 / (2 * prescaler * clk_div);
- } else if (baud < 100) {
+ if (priv->use_actual_rate)
+ baud = cp210x_get_actual_rate(serial, baud);
+ else if (baud < 100)
baud = cp210x_get_an205_rate(baud);
- } else if (baud > priv->max_speed) {
+ else if (baud > priv->max_speed)
baud = priv->max_speed;
- }
dev_dbg(>dev, "%s - setting baud rate to %u\n", __func__, baud);
if (cp210x_write_u32_reg(port, CP210X_SET_BAUDRATE, baud)) {
@@ -1524,6 +1525,7 @@ static int cp210x_port_remove(struct usb_serial_port
*port)
static void cp210x_init_max_speed(struct usb_serial *serial)
{
struct cp210x_serial_private *priv = usb_get_serial_data(serial);
+ bool use_actual_rate = false;
speed_t max;
switch (priv->partnum) {
@@ -1547,6 +1549,7 @@ static void cp210x_init_max_speed(struct usb_serial
*serial)
case CP210X_PARTNUM_CP2102N_QFN28:
case CP210X_PARTNUM_CP2102N_QFN24:
case CP210X_PARTNUM_CP2102N_QFN20:
+ use_actual_rate = true;
max = 300;
break;
default:
@@ -1555,6 +1558,7 @@ static void cp210x_init_max_speed(struct usb_serial
*serial)
}
priv->max_speed = max;
+ priv->use_actual_rate = use_actual_rate;
}
static int cp210x_attach(struct usb_serial *serial)
--
2.18.0
--
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 1/5] USB: serial: cp210x: make line-speed quantisation data driven
Older cp210x devices only support a fixed set of line speeds to which a
requested speed is mapped. Reimplement this mapping using a table
instead of a long if-else construct.
Signed-off-by: Johan Hovold
---
drivers/usb/serial/cp210x.c | 99 +
1 file changed, 56 insertions(+), 43 deletions(-)
diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
index eb6c26cbe579..1b380309f653 100644
--- a/drivers/usb/serial/cp210x.c
+++ b/drivers/usb/serial/cp210x.c
@@ -752,48 +752,6 @@ static int cp210x_get_line_ctl(struct usb_serial_port
*port, u16 *ctl)
return 0;
}
-/*
- * cp210x_quantise_baudrate
- * Quantises the baud rate as per AN205 Table 1
- */
-static unsigned int cp210x_quantise_baudrate(unsigned int baud)
-{
- if (baud <= 300)
- baud = 300;
- else if (baud <= 600) baud = 600;
- else if (baud <= 1200) baud = 1200;
- else if (baud <= 1800) baud = 1800;
- else if (baud <= 2400) baud = 2400;
- else if (baud <= 4000) baud = 4000;
- else if (baud <= 4803) baud = 4800;
- else if (baud <= 7207) baud = 7200;
- else if (baud <= 9612) baud = 9600;
- else if (baud <= 14428)baud = 14400;
- else if (baud <= 16062)baud = 16000;
- else if (baud <= 19250)baud = 19200;
- else if (baud <= 28912)baud = 28800;
- else if (baud <= 38601)baud = 38400;
- else if (baud <= 51558)baud = 51200;
- else if (baud <= 56280)baud = 56000;
- else if (baud <= 58053)baud = 57600;
- else if (baud <= 64111)baud = 64000;
- else if (baud <= 77608)baud = 76800;
- else if (baud <= 117028) baud = 115200;
- else if (baud <= 129347) baud = 128000;
- else if (baud <= 156868) baud = 153600;
- else if (baud <= 237832) baud = 230400;
- else if (baud <= 254234) baud = 25;
- else if (baud <= 273066) baud = 256000;
- else if (baud <= 491520) baud = 460800;
- else if (baud <= 567138) baud = 50;
- else if (baud <= 670254) baud = 576000;
- else if (baud < 100)
- baud = 921600;
- else if (baud > 200)
- baud = 200;
- return baud;
-}
-
static int cp210x_open(struct tty_struct *tty, struct usb_serial_port *port)
{
int result;
@@ -1013,6 +971,58 @@ static void cp210x_get_termios_port(struct
usb_serial_port *port,
*cflagp = cflag;
}
+struct cp210x_rate {
+ speed_t rate;
+ speed_t high;
+};
+
+static const struct cp210x_rate cp210x_an205_table1[] = {
+ { 300, 300 },
+ { 600, 600 },
+ { 1200, 1200 },
+ { 1800, 1800 },
+ { 2400, 2400 },
+ { 4000, 4000 },
+ { 4800, 4803 },
+ { 7200, 7207 },
+ { 9600, 9612 },
+ { 14400, 14428 },
+ { 16000, 16062 },
+ { 19200, 19250 },
+ { 28800, 28912 },
+ { 38400, 38601 },
+ { 51200, 51558 },
+ { 56000, 56280 },
+ { 57600, 58053 },
+ { 64000, 64111 },
+ { 76800, 77608 },
+ { 115200, 117028 },
+ { 128000, 129347 },
+ { 153600, 156868 },
+ { 230400, 237832 },
+ { 25, 254234 },
+ { 256000, 273066 },
+ { 460800, 491520 },
+ { 50, 567138 },
+ { 576000, 670254 },
+ { 921600, UINT_MAX }
+};
+
+/*
+ * Quantises the baud rate as per AN205 Table 1
+ */
+static speed_t cp210x_get_an205_rate(speed_t baud)
+{
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(cp210x_an205_table1); ++i) {
+ if (baud <= cp210x_an205_table1[i].high)
+ break;
+ }
+
+ return cp210x_an205_table1[i].rate;
+}
+
/*
* CP2101 supports the following baud rates:
*
@@ -1051,7 +1061,10 @@ static void cp210x_change_speed(struct tty_struct *tty,
*
* NOTE: B0 is not implemented.
*/
- baud = cp210x_quantise_baudrate(baud);
+ if (baud < 100)
+ baud = cp210x_get_an205_rate(baud);
+ else if (baud > 200)
+ baud = 200;
dev_dbg(>dev, "%s - setting baud rate to %u\n", __func__, baud);
if (cp210x_write_u32_reg(port, CP210X_SET_BAUDRATE, baud)) {
--
2.18.0
--
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 3/5] USB: serial: cp210x: improve baudrate support for CP2102N
From: Karoly Pados
CP2102N devices support a lot more baudrates than earlier chips by
SiLabs. These devices are not constrained anymore by the table in AN205,
and are able to generate almost any baudrate in the supported range
with only minimal errors. This has also been verified with a scope on
a physical device. This patch adds support for all baudrates supported
by the CP2102N.
Signed-off-by: Karoly Pados
[ johan: rework on top of an205 and max-speed patches ]
Signed-off-by: Johan Hovold
---
drivers/usb/serial/cp210x.c | 39 -
1 file changed, 34 insertions(+), 5 deletions(-)
diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
index 4281f2bfe0e1..3778685c7b99 100644
--- a/drivers/usb/serial/cp210x.c
+++ b/drivers/usb/serial/cp210x.c
@@ -355,6 +355,9 @@ static struct usb_serial_driver * const serial_drivers[] = {
#define CP210X_PARTNUM_CP2104 0x04
#define CP210X_PARTNUM_CP2105 0x05
#define CP210X_PARTNUM_CP2108 0x08
+#define CP210X_PARTNUM_CP2102N_QFN28 0x20
+#define CP210X_PARTNUM_CP2102N_QFN24 0x21
+#define CP210X_PARTNUM_CP2102N_QFN20 0x22
#define CP210X_PARTNUM_UNKNOWN 0xFF
/* CP210X_GET_COMM_STATUS returns these 0x13 bytes */
@@ -454,6 +457,15 @@ struct cp210x_gpio_write {
u8 state;
} __packed;
+static bool cp210x_is_cp2102n(struct usb_serial *serial)
+{
+ struct cp210x_serial_private *priv = usb_get_serial_data(serial);
+
+ return (priv->partnum == CP210X_PARTNUM_CP2102N_QFN28) ||
+ (priv->partnum == CP210X_PARTNUM_CP2102N_QFN24) ||
+ (priv->partnum == CP210X_PARTNUM_CP2102N_QFN20);
+}
+
/*
* Helper to get interface number when we only have struct usb_serial.
*/
@@ -1053,20 +1065,32 @@ static speed_t cp210x_get_an205_rate(speed_t baud)
static void cp210x_change_speed(struct tty_struct *tty,
struct usb_serial_port *port, struct ktermios *old_termios)
{
- struct cp210x_serial_private *priv = usb_get_serial_data(port->serial);
+ struct usb_serial *serial = port->serial;
+ struct cp210x_serial_private *priv = usb_get_serial_data(serial);
u32 baud;
baud = tty->termios.c_ospeed;
- /* This maps the requested rate to a rate valid on cp2102 or cp2103,
-* or to an arbitrary rate in [1M, max_speed]
+ /*
+* This maps the requested rate to the actual rate on cp2102n, a valid
+* rate on cp2102 or cp2103, or to an arbitrary rate in
+* [1M, max_speed].
*
* NOTE: B0 is not implemented.
*/
- if (baud < 100)
+ if (cp210x_is_cp2102n(serial)) {
+ int clk_div;
+ int prescaler;
+
+ baud = clamp(baud, 300u, priv->max_speed);
+ prescaler = (baud <= 365) ? 4 : 1;
+ clk_div = DIV_ROUND_CLOSEST(4800, 2 * prescaler * baud);
+ baud = 4800 / (2 * prescaler * clk_div);
+ } else if (baud < 100) {
baud = cp210x_get_an205_rate(baud);
- else if (baud > priv->max_speed)
+ } else if (baud > priv->max_speed) {
baud = priv->max_speed;
+ }
dev_dbg(>dev, "%s - setting baud rate to %u\n", __func__, baud);
if (cp210x_write_u32_reg(port, CP210X_SET_BAUDRATE, baud)) {
@@ -1520,6 +1544,11 @@ static void cp210x_init_max_speed(struct usb_serial
*serial)
else
max = 921600; /* SCI */
break;
+ case CP210X_PARTNUM_CP2102N_QFN28:
+ case CP210X_PARTNUM_CP2102N_QFN24:
+ case CP210X_PARTNUM_CP2102N_QFN20:
+ max = 300;
+ break;
default:
max = 200;
break;
--
2.18.0
--
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 0/2] USB: serial: cp210x: clean up line-speed handling
Hi Karoly, and sorry for not getting back to you sooner on this. On Thu, Jul 05, 2018 at 04:12:17PM +, Karoly Pados wrote: > Hi, > > > Karoly, how did your line-speed tests with cp2102n go? > > I indeed tested this. I first built a version of the module where I skip > calling cp210x_quantise_baudrate(). Then I attached a scope to the TX > line of my UART adapter and looked at various non-standard values in both > low (<10k) and high (>2M) ranges. In all cases it looks like you can set > any custom value to the cp2102n, assuming you use the right program to > set the baudrate (most I've tried either do not allow non-standard > rates or give an IOCTL error). > > So basically I can confirm that the chip does not snap to values in > table 1 of AN205. I was able to set very odd rates, and measurements with > the scope verified that they were actually applied correctly by the > cp2102n (sometimes ofc with a few percent error here and there). That's great, thanks for verifying. > > ... I think we should > > just handle the higher cp2102n rates as we do with cp2104/8, that is by > > mapping the lower rates to the rates supported by legacy devices, while > > allowing any higher rates (up to the device maximum) to be requested > > without trying to report back the actual rate chosen (for now). > > Based on the test above, my proposal for the cp2102n only is to not do any > kind of software snapping / quantisation in the driver module, except for > capping out at the maximum of 3Mbaud. Otherwise we'd be limiting the > capabilities of the chip in the software artificially. Indeed, I was only only suggesting that we need not solve every issue at once. :) > As for reporting the actual baudrate back to userspace, the formula is > documented clearly by SiLabs, so if that's possible somehow, I'm in favor > of it. The question is, how? Right, and it's documented in the driver as well, but no one has gotten around to implementing yet. And as you already noticed, you can report back the actual rate used through tty_encode_baud_rate(). I'll be sending my 4.19 updates to Greg this week and want to have this included. To save some time, I've reworked your v2 patch on top of this series and generalised your implementation so that it can be used also for CP2104 and CP2105 (ECI). Thanks, Johan -- 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 v6] USB: serial: ftdi_sio: Add MTP NVM support
On Sun, Jul 15, 2018 at 10:30:39PM +0200, Loic Poulain wrote: > Hi Johan, > > Thanks for the review. > > On 10 July 2018 at 11:19, Johan Hovold wrote: > > The CBUS configuration is stored in just a couple of words at a known > > offset and could easily be retrieved without depending on the nvmem > > subsystem when needed. > > > > As I assume fiddling with the EEPROM should be rare (e.g. done during > > development or by hobbyists) and would still depend user-space tools > > (e.g. for layouts, checksums and external EEPROMs), I think we should > > just leave it all to user space to deal with. > > I'm fine with that, to be honest I'm interested in adding GPIO control > but wanted to progress step by step, EEPROM access being the first one. > I thought it could have been valuable to expose it via nvmem, but like you > said, > we can keep this internal for now. > > So, I'll come back with new patche(s) for CBUS GPIOs. Sounds good! Thanks, Johan -- 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 2/2] USB: iuu_phoenix: drop redundant input-speed re-encoding
On Mon, Jul 16, 2018 at 01:56:46PM +0200, Greg Kroah-Hartman wrote: > On Mon, Jul 16, 2018 at 01:51:56PM +0200, Johan Hovold wrote: > > Drop redundant input-speed re-encoding at every open(). The output and > > input speeds are initialised to the same value and are kept in sync on > > termios updates. > > > > Signed-off-by: Johan Hovold > > Reviewed-by: Greg Kroah-Hartman Thanks for reviewing. Both now applied. Johan -- 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 2/2] USB: iuu_phoenix: drop redundant input-speed re-encoding
Drop redundant input-speed re-encoding at every open(). The output and input speeds are initialised to the same value and are kept in sync on termios updates. Signed-off-by: Johan Hovold --- drivers/usb/serial/iuu_phoenix.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/drivers/usb/serial/iuu_phoenix.c b/drivers/usb/serial/iuu_phoenix.c index 87c8dd064205..449e89db9cea 100644 --- a/drivers/usb/serial/iuu_phoenix.c +++ b/drivers/usb/serial/iuu_phoenix.c @@ -962,9 +962,6 @@ static int iuu_open(struct tty_struct *tty, struct usb_serial_port *port) struct iuu_private *priv = usb_get_serial_port_data(port); baud = tty->termios.c_ospeed; - tty->termios.c_ispeed = baud; - /* Re-encode speed */ - tty_encode_baud_rate(tty, baud, baud); dev_dbg(dev, "%s - baud %d\n", __func__, baud); usb_clear_halt(serial->dev, port->write_urb->pipe); -- 2.18.0 -- 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 1/2] USB: serial: iuu_phoenix: drop unused driver-data baud rate
Drop unused driver-data baud rate.
Signed-off-by: Johan Hovold
---
drivers/usb/serial/iuu_phoenix.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/usb/serial/iuu_phoenix.c b/drivers/usb/serial/iuu_phoenix.c
index 2fb71303ec3a..87c8dd064205 100644
--- a/drivers/usb/serial/iuu_phoenix.c
+++ b/drivers/usb/serial/iuu_phoenix.c
@@ -58,7 +58,6 @@ struct iuu_private {
u8 *buf;/* used for initialize speed */
u8 len;
int vcc;/* vcc (either 3 or 5 V) */
- u32 baud;
u32 boost;
u32 clk;
};
@@ -991,7 +990,6 @@ static int iuu_open(struct tty_struct *tty, struct
usb_serial_port *port)
if (boost < 100)
boost = 100;
priv->boost = boost;
- priv->baud = baud;
switch (clockmode) {
case 2: /* 3.680 Mhz */
priv->clk = IUU_CLK_368;
--
2.18.0
--
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 v6] USB: serial: ftdi_sio: Add MTP NVM support
Hi, I finally found some time to dig into the ftdi eeprom handling. On Tue, Jun 26, 2018 at 02:54:48PM +0200, Loic Poulain wrote: > Most of FTDI's devices have an EEPROM which records FTDI devices > configuration setting (e.g. the VID, PID, I/O config...) and user > data. For example, FT232R and FTX chips have 128-byte and 2048-byte > internal EEPROM respectively. The "internal" qualifier here is crucial; only FT232R and FTX have internal EEPROMs of a known size. Other device types use external EEPROMs of varying sizes, and which need not even be populated. And judging from the heuristics used by libftdi, there's no good (known) way to determine the size of the external EEPROMs, which means that this cannot be generalised. Note that handling external EEPROMS also implies dealing with explicit EEPROM erase commands, which does not seem to fit the proposed interface. > This patch adds support for FTDI EEPROM read/write via USB control > transfers and register a new nvm device to the nvmem core. > > This permits to expose the EEPROM as a sysfs file, allowing userspace > to read/modify FTDI configuration and its user data without having to > rely on a specific userspace USB driver. So I have doubts about the usefulness of all of this. - You cannot just write the EEPROM from user space without knowing the device-specific layout. - You also need to make sure to recalculate and update the checksum stored in the final word. - And for that you obviously need to know the EEPROM size (known for R and FTX). So in the end you still need something like libftdi to be able to do anything useful with this. Also note that the layout and protocol for handling the EEPROM is only available under NDA or is based on reverse-engineering guess work. For example, it seems like for FT232R you need to set a specific latency timer to be able to write the EEPROM (reliably). And so on. And as the consequences of getting this wrong may result in a bricked device, I'm even more sceptical about this. > Moreover, any upcoming new tentative to add CBUS GPIO support could > integrate CBUS EEPROM configuration reading in order to determine > which of the CBUS pins are available as GPIO. The CBUS configuration is stored in just a couple of words at a known offset and could easily be retrieved without depending on the nvmem subsystem when needed. As I assume fiddling with the EEPROM should be rare (e.g. done during development or by hobbyists) and would still depend user-space tools (e.g. for layouts, checksums and external EEPROMs), I think we should just leave it all to user space to deal with. Johan -- 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
[GIT PULL] USB-serial fixes for v4.18-rc4
The following changes since commit 021c91791a5e7e85c567452f1be3e4c2c6cb6063: Linux 4.18-rc3 (2018-07-01 16:04:53 -0700) are available in the Git repository at: https://git.kernel.org/pub/scm/linux/kernel/git/johan/usb-serial.git tags/usb-serial-4.18-rc4 for you to fetch changes up to 794744abfffef8b1f3c0c8a4896177d6d13d653d: USB: serial: mos7840: fix status-register error handling (2018-07-06 10:32:28 +0200) USB-serial fixes for v4.18-rc4 Here are three fixes for broken control-transfer error handling, which could lead to uninitialised slab data leaking to user space. Included is also a new device id for cp210x. All but the final two patches have been in linux-next with no reported issues. Signed-off-by: Johan Hovold Dan Carpenter (1): USB: serial: ch341: fix type promotion bug in ch341_control_in() Johan Hovold (2): USB: serial: keyspan_pda: fix modem-status error handling USB: serial: mos7840: fix status-register error handling Olli Salonen (1): USB: serial: cp210x: add another USB ID for Qivicon ZigBee stick drivers/usb/serial/ch341.c | 2 +- drivers/usb/serial/cp210x.c | 1 + drivers/usb/serial/keyspan_pda.c | 4 +++- drivers/usb/serial/mos7840.c | 3 +++ 4 files changed, 8 insertions(+), 2 deletions(-) -- 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 2/2] USB: serial: cp210x: honour device-type maximum line speed
Newer cp210x devices support higher line speeds than the older ones
which supported a discrete set of speeds up to 921.6 kbaud.
To support these higher speeds, we have for some time mapped speeds
lower than 1 Mbaud to the speeds supported by older devices, while
allowing the device to pick the closest possible rate for higher speeds
(without trying to guess and report back what rate was actually chosen).
As this implementation can lead to undefined behaviour for older devices
which do not support the higher rates, let's use the later-added
device-type detection to determine the maximum supported speed.
This will also be useful when adding support for cp2102n which can
handle rates up to 3 Mbaud.
As per the data sheets the following maximum speeds are used
cp2101 921.6 kbaud
cp2102/31 Mbaud
cp2104/82 Mbaud
cp2105
- ECI port 2 Mbaud
- SCI port 921.6 kbaud
while keeping the maximum 2 Mbaud for unknown device types in order to
avoid any regressions.
Signed-off-by: Johan Hovold
---
drivers/usb/serial/cp210x.c | 41 ++---
1 file changed, 38 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
index e1a4d4b3d3aa..7772c4ce45a6 100644
--- a/drivers/usb/serial/cp210x.c
+++ b/drivers/usb/serial/cp210x.c
@@ -244,6 +244,7 @@ struct cp210x_serial_private {
boolgpio_registered;
#endif
u8 partnum;
+ speed_t max_speed;
};
struct cp210x_port_private {
@@ -1067,19 +1068,20 @@ static speed_t cp210x_get_an205_rate(speed_t baud)
static void cp210x_change_speed(struct tty_struct *tty,
struct usb_serial_port *port, struct ktermios *old_termios)
{
+ struct cp210x_serial_private *priv = usb_get_serial_data(port->serial);
u32 baud;
baud = tty->termios.c_ospeed;
/* This maps the requested rate to a rate valid on cp2102 or cp2103,
-* or to an arbitrary rate in [1M,2M].
+* or to an arbitrary rate in [1M, max_speed]
*
* NOTE: B0 is not implemented.
*/
if (baud < 100)
baud = cp210x_get_an205_rate(baud);
- else if (baud > 200)
- baud = 200;
+ else if (baud > priv->max_speed)
+ baud = priv->max_speed;
dev_dbg(>dev, "%s - setting baud rate to %u\n", __func__, baud);
if (cp210x_write_u32_reg(port, CP210X_SET_BAUDRATE, baud)) {
@@ -1510,6 +1512,37 @@ static int cp210x_port_remove(struct usb_serial_port
*port)
return 0;
}
+static void cp210x_init_max_speed(struct usb_serial *serial)
+{
+ struct cp210x_serial_private *priv = usb_get_serial_data(serial);
+ speed_t max;
+
+ switch (priv->partnum) {
+ case CP210X_PARTNUM_CP2101:
+ max = 921600;
+ break;
+ case CP210X_PARTNUM_CP2102:
+ case CP210X_PARTNUM_CP2103:
+ max = 100;
+ break;
+ case CP210X_PARTNUM_CP2104:
+ case CP210X_PARTNUM_CP2108:
+ max = 200;
+ break;
+ case CP210X_PARTNUM_CP2105:
+ if (cp210x_interface_num(serial) == 0)
+ max = 200; /* ECI */
+ else
+ max = 921600; /* SCI */
+ break;
+ default:
+ max = 200;
+ break;
+ }
+
+ priv->max_speed = max;
+}
+
static int cp210x_attach(struct usb_serial *serial)
{
int result;
@@ -1530,6 +1563,8 @@ static int cp210x_attach(struct usb_serial *serial)
usb_set_serial_data(serial, priv);
+ cp210x_init_max_speed(serial);
+
if (priv->partnum == CP210X_PARTNUM_CP2105) {
result = cp2105_shared_gpio_init(serial);
if (result < 0) {
--
2.18.0
--
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 0/2] USB: serial: cp210x: clean up line-speed handling
This cleans up the current line-speed handling somewhat, while avoiding requesting an unsupported line speed (which can lead to undefined behaviour) by determining the maximum speed supported by the device type in question. Karoly, how did your line-speed tests with cp2102n go? I think we should just handle the higher cp2102n rates as we do with cp2104/8, that is by mapping the lower rates to the rates supported by legacy devices, while allowing any higher rates (up to the device maximum) to be requested without trying to report back the actual rate chosen (for now). Johan Johan Hovold (2): USB: serial: cp210x: make line-speed quantisation data driven USB: serial: cp210x: honour device-type maximum line speed drivers/usb/serial/cp210x.c | 136 1 file changed, 92 insertions(+), 44 deletions(-) -- 2.18.0 -- 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 1/2] USB: serial: cp210x: make line-speed quantisation data driven
Older cp210x devices only support a fixed set of line speeds to which a
requested speed is mapped. Reimplement this mapping using a table
instead of a long if-else construct.
Signed-off-by: Johan Hovold
---
drivers/usb/serial/cp210x.c | 99 +
1 file changed, 56 insertions(+), 43 deletions(-)
diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
index 626a29d9aa58..e1a4d4b3d3aa 100644
--- a/drivers/usb/serial/cp210x.c
+++ b/drivers/usb/serial/cp210x.c
@@ -767,48 +767,6 @@ static int cp210x_get_line_ctl(struct usb_serial_port
*port, u16 *ctl)
return 0;
}
-/*
- * cp210x_quantise_baudrate
- * Quantises the baud rate as per AN205 Table 1
- */
-static unsigned int cp210x_quantise_baudrate(unsigned int baud)
-{
- if (baud <= 300)
- baud = 300;
- else if (baud <= 600) baud = 600;
- else if (baud <= 1200) baud = 1200;
- else if (baud <= 1800) baud = 1800;
- else if (baud <= 2400) baud = 2400;
- else if (baud <= 4000) baud = 4000;
- else if (baud <= 4803) baud = 4800;
- else if (baud <= 7207) baud = 7200;
- else if (baud <= 9612) baud = 9600;
- else if (baud <= 14428)baud = 14400;
- else if (baud <= 16062)baud = 16000;
- else if (baud <= 19250)baud = 19200;
- else if (baud <= 28912)baud = 28800;
- else if (baud <= 38601)baud = 38400;
- else if (baud <= 51558)baud = 51200;
- else if (baud <= 56280)baud = 56000;
- else if (baud <= 58053)baud = 57600;
- else if (baud <= 64111)baud = 64000;
- else if (baud <= 77608)baud = 76800;
- else if (baud <= 117028) baud = 115200;
- else if (baud <= 129347) baud = 128000;
- else if (baud <= 156868) baud = 153600;
- else if (baud <= 237832) baud = 230400;
- else if (baud <= 254234) baud = 25;
- else if (baud <= 273066) baud = 256000;
- else if (baud <= 491520) baud = 460800;
- else if (baud <= 567138) baud = 50;
- else if (baud <= 670254) baud = 576000;
- else if (baud < 100)
- baud = 921600;
- else if (baud > 200)
- baud = 200;
- return baud;
-}
-
static int cp210x_open(struct tty_struct *tty, struct usb_serial_port *port)
{
int result;
@@ -1028,6 +986,58 @@ static void cp210x_get_termios_port(struct
usb_serial_port *port,
*cflagp = cflag;
}
+struct cp210x_rate {
+ speed_t rate;
+ speed_t high;
+};
+
+static const struct cp210x_rate cp210x_an205_table1[] = {
+ { 300, 300 },
+ { 600, 600 },
+ { 1200, 1200 },
+ { 1800, 1800 },
+ { 2400, 2400 },
+ { 4000, 4000 },
+ { 4800, 4803 },
+ { 7200, 7207 },
+ { 9600, 9612 },
+ { 14400, 14428 },
+ { 16000, 16062 },
+ { 19200, 19250 },
+ { 28800, 28912 },
+ { 38400, 38601 },
+ { 51200, 51558 },
+ { 56000, 56280 },
+ { 57600, 58053 },
+ { 64000, 64111 },
+ { 76800, 77608 },
+ { 115200, 117028 },
+ { 128000, 129347 },
+ { 153600, 156868 },
+ { 230400, 237832 },
+ { 25, 254234 },
+ { 256000, 273066 },
+ { 460800, 491520 },
+ { 50, 567138 },
+ { 576000, 670254 },
+ { 921600, UINT_MAX }
+};
+
+/*
+ * Quantises the baud rate as per AN205 Table 1
+ */
+static speed_t cp210x_get_an205_rate(speed_t baud)
+{
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(cp210x_an205_table1); ++i) {
+ if (baud <= cp210x_an205_table1[i].high)
+ break;
+ }
+
+ return cp210x_an205_table1[i].rate;
+}
+
/*
* CP2101 supports the following baud rates:
*
@@ -1066,7 +1076,10 @@ static void cp210x_change_speed(struct tty_struct *tty,
*
* NOTE: B0 is not implemented.
*/
- baud = cp210x_quantise_baudrate(baud);
+ if (baud < 100)
+ baud = cp210x_get_an205_rate(baud);
+ else if (baud > 200)
+ baud = 200;
dev_dbg(>dev, "%s - setting baud rate to %u\n", __func__, baud);
if (cp210x_write_u32_reg(port, CP210X_SET_BAUDRATE, baud)) {
--
2.18.0
--
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: cp20x GPIO from user application
[ Adding linux-usb as other may be interested in this. ] On Thu, Jul 05, 2018 at 01:57:29PM +0100, Antonio Santagiuliana wrote: > Thank you for the information. > What I don't understand is that if I use the CP2105 are its GPIOs usable by > this driver ? Yes, they should be. But if I remember correctly, the pins in question are muxed with different functions, so you need to make sure that the device is configured correctly (e.g. using some silabs tool). > If so, which is the interface to the user space ? I can open the driver > usbserial but from the Raspberry pi user space I don't understand where the > GPIOs from this chip are mapped to or how I should map them by creating a > node with mknod ? You can access them from /sys/class/gpio or using the new gpiolib chardev interface as any other gpios. If the gpios are recognised and registered, they should also show up in sysfs as a child device to the USB interface in question, for example as: /sys/bus/usb/devices/1-1.1\:1.0/gpio/gpiochipN Good luck, Johan -- 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: Incorrect PID in drivers/usb/serial/kl5kusb105.h
0x80 > (Bus Powered) > MaxPower 94mA > Interface Descriptor: > bLength 9 > bDescriptorType 4 > bInterfaceNumber0 > bAlternateSetting 0 > bNumEndpoints 3 > bInterfaceClass 255 Vendor Specific Class > bInterfaceSubClass 0 > bInterfaceProtocol 0 > iInterface 0 > Endpoint Descriptor: > bLength 7 > bDescriptorType 5 > bEndpointAddress 0x81 EP 1 IN > bmAttributes2 > Transfer TypeBulk > Synch Type None > Usage Type Data > wMaxPacketSize 0x0040 1x 64 bytes > bInterval 0 > Endpoint Descriptor: > bLength 7 > bDescriptorType 5 > bEndpointAddress 0x02 EP 2 OUT > bmAttributes2 > Transfer TypeBulk > Synch Type None > Usage Type Data > wMaxPacketSize 0x0040 1x 64 bytes > bInterval 0 > Endpoint Descriptor: > bLength 7 > bDescriptorType 5 > bEndpointAddress 0x83 EP 3 IN > bmAttributes 3 > Transfer TypeInterrupt > Synch Type None > Usage Type Data > wMaxPacketSize 0x0008 1x 8 bytes > bInterval 1 > Device Status: 0x28b0 > (Bus Powered) > On 4 Jul 2018, at 16:09, Johan Hovold wrote: > > [ Please avoid using html when posting the lists. Including full mail > below in case this did not reach the list. ] > > On Wed, Jul 04, 2018 at 03:05:24PM +0100, Chris Jakob wrote: > The PID definitions fro VID 05e9 are listed in > http://www.linux-usb.org/usb.ids as: > > > 05e9 Kawasaki LSI > 0008 KL5KUSB101B Ethernet [klsi] > 0009 Sony 10Mbps Ethernet [pegasus] > 000c USB-to-RS-232 > 000d USB-to-RS-232 > 0014 RS-232 J104 > 0040 Ethernet Adapter > 2008 Ethernet Adapter > > > However drivers/usb/serial/kl5kusb105.h defines the following: > > #define KLSI_KL5KUSB105D_PID 0x00c0 > > My KLSI serial adapter is not recognized with the current kernel, however > if I rebuild KL5KUSB105 with > > #define KLSI_KL5KUSB105D_PID 0x000c > > the serial adapter is recognized. > > So my hardware matches the definitions in http://www.linux-usb.org/usb.ids. > > Can this be changed for future kernels? > > Absolutely, and we should probably fix that in the active stable trees > as well. > > Could just provide the output of usb-devices (or lsusb -v) for your > device for future reference? > > Thanks, > Johan > -- 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: Incorrect PID in drivers/usb/serial/kl5kusb105.h
[ Please avoid using html when posting the lists. Including full mail below in case this did not reach the list. ] On Wed, Jul 04, 2018 at 03:05:24PM +0100, Chris Jakob wrote: > The PID definitions fro VID 05e9 are listed in > http://www.linux-usb.org/usb.ids as: > > > 05e9 Kawasaki LSI > 0008 KL5KUSB101B Ethernet [klsi] > 0009 Sony 10Mbps Ethernet [pegasus] > 000c USB-to-RS-232 > 000d USB-to-RS-232 > 0014 RS-232 J104 > 0040 Ethernet Adapter > 2008 Ethernet Adapter > > > However drivers/usb/serial/kl5kusb105.h defines the following: > > #define KLSI_KL5KUSB105D_PID 0x00c0 > > My KLSI serial adapter is not recognized with the current kernel, however > if I rebuild KL5KUSB105 with > > #define KLSI_KL5KUSB105D_PID 0x000c > > the serial adapter is recognized. > > So my hardware matches the definitions in http://www.linux-usb.org/usb.ids. > > Can this be changed for future kernels? Absolutely, and we should probably fix that in the active stable trees as well. Could just provide the output of usb-devices (or lsusb -v) for your device for future reference? Thanks, Johan -- 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 1/4] USB: serial: keyspan_pda: fix modem-status error handling
Fix broken modem-status error handling which could lead to bits of slab
data leaking to user space.
Fixes: 3b36a8fd6777 ("usb: fix uninitialized variable warning in keyspan_pda")
Cc: stable # 2.6.27
Cc: Benny Halevy
Signed-off-by: Johan Hovold
---
drivers/usb/serial/keyspan_pda.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/serial/keyspan_pda.c b/drivers/usb/serial/keyspan_pda.c
index 5169624d8b11..38d43c4b7ce5 100644
--- a/drivers/usb/serial/keyspan_pda.c
+++ b/drivers/usb/serial/keyspan_pda.c
@@ -369,8 +369,10 @@ static int keyspan_pda_get_modem_info(struct usb_serial
*serial,
3, /* get pins */
USB_TYPE_VENDOR|USB_RECIP_INTERFACE|USB_DIR_IN,
0, 0, data, 1, 2000);
- if (rc >= 0)
+ if (rc == 1)
*value = *data;
+ else if (rc >= 0)
+ rc = -EIO;
kfree(data);
return rc;
--
2.18.0
--
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 2/4] USB: serial: mos7840: fix status-register error handling
Add missing transfer-length sanity check to the status-register
completion handler to avoid leaking bits of uninitialised slab data to
user space.
Fixes: 3f5429746d91 ("USB: Moschip 7840 USB-Serial Driver")
Cc: stable # 2.6.19
Cc: Paul B Schroeder
Signed-off-by: Johan Hovold
---
drivers/usb/serial/mos7840.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/usb/serial/mos7840.c b/drivers/usb/serial/mos7840.c
index fdceb46d9fc6..b580b4c7fa48 100644
--- a/drivers/usb/serial/mos7840.c
+++ b/drivers/usb/serial/mos7840.c
@@ -468,6 +468,9 @@ static void mos7840_control_callback(struct urb *urb)
}
dev_dbg(dev, "%s urb buffer size is %d\n", __func__,
urb->actual_length);
+ if (urb->actual_length < 1)
+ goto out;
+
dev_dbg(dev, "%s mos7840_port->MsrLsr is %d port %d\n", __func__,
mos7840_port->MsrLsr, mos7840_port->port_num);
data = urb->transfer_buffer;
--
2.18.0
--
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 4/4] USB: serial: kobil_sct: add missing version error handling
Add missing version-request error handling and suppress printing of the
(zeroed) transfer-buffer content in case of errors.
Signed-off-by: Johan Hovold
---
drivers/usb/serial/kobil_sct.c | 12
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/serial/kobil_sct.c b/drivers/usb/serial/kobil_sct.c
index a6ebed1e0f20..e9882ba20933 100644
--- a/drivers/usb/serial/kobil_sct.c
+++ b/drivers/usb/serial/kobil_sct.c
@@ -190,8 +190,10 @@ static int kobil_open(struct tty_struct *tty, struct
usb_serial_port *port)
KOBIL_TIMEOUT
);
dev_dbg(dev, "%s - Send get_HW_version URB returns: %i\n", __func__,
result);
- dev_dbg(dev, "Hardware version: %i.%i.%i\n", transfer_buffer[0],
- transfer_buffer[1], transfer_buffer[2]);
+ if (result >= 3) {
+ dev_dbg(dev, "Hardware version: %i.%i.%i\n", transfer_buffer[0],
+ transfer_buffer[1], transfer_buffer[2]);
+ }
/* get firmware version */
result = usb_control_msg(port->serial->dev,
@@ -205,8 +207,10 @@ static int kobil_open(struct tty_struct *tty, struct
usb_serial_port *port)
KOBIL_TIMEOUT
);
dev_dbg(dev, "%s - Send get_FW_version URB returns: %i\n", __func__,
result);
- dev_dbg(dev, "Firmware version: %i.%i.%i\n", transfer_buffer[0],
- transfer_buffer[1], transfer_buffer[2]);
+ if (result >= 3) {
+ dev_dbg(dev, "Firmware version: %i.%i.%i\n", transfer_buffer[0],
+ transfer_buffer[1], transfer_buffer[2]);
+ }
if (priv->device_type == KOBIL_ADAPTER_B_PRODUCT_ID ||
priv->device_type == KOBIL_ADAPTER_K_PRODUCT_ID) {
--
2.18.0
--
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] cp210x: add another USB ID for Qivicon ZigBee stick
On Wed, Jul 04, 2018 at 02:07:42PM +0300, Olli Salonen wrote: > There are two versions of the Qivicon Zigbee stick in circulation. This adds > the second USB ID to the cp210x driver. > > Signed-off-by: Olli Salonen Now applied, thanks. Johan -- 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: serial: ch341: type promotion bug in ch341_control_in()
On Wed, Jul 04, 2018 at 12:29:38PM +0300, Dan Carpenter wrote:
> The "r" variable is an int and "bufsize" is an unsigned int so the
> comparison is type promoted to unsigned. If usb_control_msg() returns a
> negative that is treated as a high positive value and the error handling
> doesn't work.
>
> Fixes: 2d5a9c72d0c4 ("USB: serial: ch341: fix control-message error handling")
> Signed-off-by: Dan Carpenter
Thanks for catching this.
Now applied with a stable tag as this could have security implications.
Johan
--
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
[GIT PULL] USB-serial fixes for v4.18-rc3
The following changes since commit ce397d215ccd07b8ae3f71db689aedb85d56ab40: Linux 4.18-rc1 (2018-06-17 08:04:49 +0900) are available in the Git repository at: https://git.kernel.org/pub/scm/linux/kernel/git/johan/usb-serial.git tags/usb-serial-4.18-rc3 for you to fetch changes up to 24160628a34af962ac99f2f58e547ac3c4cbd26f: USB: serial: cp210x: add CESINEL device ids (2018-06-18 11:25:09 +0200) USB-serial fixes for v4.18-rc3 Here are bunch of new device ids for cp210x. All have been in linux-next with no reported issues. Signed-off-by: Johan Hovold Johan Hovold (1): USB: serial: cp210x: add CESINEL device ids Karoly Pados (1): USB: serial: cp210x: add Silicon Labs IDs for Windows Update drivers/usb/serial/cp210x.c | 14 ++ 1 file changed, 14 insertions(+) -- 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: serial: digi_acceleport: rename tty flag variable
On Tue, Jun 26, 2018 at 10:12:28PM +0800, Greg Kroah-Hartman wrote: > On Tue, Jun 26, 2018 at 03:43:13PM +0200, Johan Hovold wrote: > > Add a "tty_" prefix to the tty "flag" variable to avoid any future > > mixups with the recently added irq-mask "flags" one. > > > > Signed-off-by: Johan Hovold > > Reviewed-by: Greg Kroah-Hartman Thanks for reviewing, now applied. Johan -- 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 3/3] Fixed simple typo from ELCOM to ELECOM.
Where did patches 2 and 3 go? If this is the only USB serial patch, you can send it on it's own. Also, please use the common prefix for the subsystem you're changing (see git log). In this case, add "USB: serial: pl2303: " to your Subject. On Tue, Jun 26, 2018 at 07:07:59PM -0500, Guy Chronister wrote: Also add at least one line here describing your change to avoid having an empty commit message, even if this is a trivial change. > Signed-off-by: Guy Chronister Apart from the above, everything looks fine. Care to send a v2? Thanks, Johan -- 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: serial: digi_acceleport: rename tty flag variable
Add a "tty_" prefix to the tty "flag" variable to avoid any future
mixups with the recently added irq-mask "flags" one.
Signed-off-by: Johan Hovold
---
drivers/usb/serial/digi_acceleport.c | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/usb/serial/digi_acceleport.c
b/drivers/usb/serial/digi_acceleport.c
index ae512fed08af..e7f244cf2c07 100644
--- a/drivers/usb/serial/digi_acceleport.c
+++ b/drivers/usb/serial/digi_acceleport.c
@@ -1387,7 +1387,7 @@ static int digi_read_inb_callback(struct urb *urb)
int len;
int port_status;
unsigned char *data;
- int flag, throttled;
+ int tty_flag, throttled;
/* short/multiple packet check */
if (urb->actual_length < 2) {
@@ -1423,7 +1423,7 @@ static int digi_read_inb_callback(struct urb *urb)
data = [3];
/* get flag from port_status */
- flag = 0;
+ tty_flag = 0;
/* overrun is special, not associated with a char */
if (port_status & DIGI_OVERRUN_ERROR)
@@ -1432,17 +1432,17 @@ static int digi_read_inb_callback(struct urb *urb)
/* break takes precedence over parity, */
/* which takes precedence over framing errors */
if (port_status & DIGI_BREAK_ERROR)
- flag = TTY_BREAK;
+ tty_flag = TTY_BREAK;
else if (port_status & DIGI_PARITY_ERROR)
- flag = TTY_PARITY;
+ tty_flag = TTY_PARITY;
else if (port_status & DIGI_FRAMING_ERROR)
- flag = TTY_FRAME;
+ tty_flag = TTY_FRAME;
/* data length is len-1 (one byte of len is port_status) */
--len;
if (len > 0) {
tty_insert_flip_string_fixed_flag(>port, data,
- flag, len);
+ tty_flag, len);
tty_flip_buffer_push(>port);
}
}
--
2.18.0
--
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 08/12] usb: serial: sierra: disable irq's for portdata lock
On Sun, Jun 24, 2018 at 12:32:11AM +0200, Sebastian Andrzej Siewior wrote: > From: John Ogness > > The portdata spinlock can be taken in interrupt context (via > sierra_outdat_callback()). > Disable interrupts when taking the portdata spinlock. Good catch! As this fixes a potential deadlock, I've reworded the commit message and added a Fixes and stable-CC tag. But as no one appears to have hit this since it was introduced over four years ago, I think this can go into -next along with the rest of the series. All patches now applied. Nice work! Thanks, Johan -- 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 02/12] usb: serial: digi_acceleport: use irqsave() in USB's complete callback
On Sun, Jun 24, 2018 at 12:32:05AM +0200, Sebastian Andrzej Siewior wrote: > From: John Ogness > > The USB completion callback does not disable interrupts while acquiring > the lock. We want to remove the local_irq_disable() invocation from > __usb_hcd_giveback_urb() and therefore it is required for the callback > handler to disable the interrupts while acquiring the lock. > The callback may be invoked either in IRQ or BH context depending on the > USB host controller. > Use the _irqsave() variant of the locking primitives. > > Cc: Johan Hovold > Cc: Greg Kroah-Hartman > Signed-off-by: John Ogness > Signed-off-by: Sebastian Andrzej Siewior > --- > drivers/usb/serial/digi_acceleport.c | 23 +-- > 1 file changed, 13 insertions(+), 10 deletions(-) > > diff --git a/drivers/usb/serial/digi_acceleport.c > b/drivers/usb/serial/digi_acceleport.c > index b0526786fb02..ae512fed08af 100644 > --- a/drivers/usb/serial/digi_acceleport.c > +++ b/drivers/usb/serial/digi_acceleport.c > @@ -1381,6 +1382,7 @@ static int digi_read_inb_callback(struct urb *urb) > struct usb_serial_port *port = urb->context; > struct digi_port *priv = usb_get_serial_port_data(port); > unsigned char *buf = urb->transfer_buffer; > + unsigned long flags; > int opcode; > int len; > int port_status; We already had a "flag" variable in this function, which could now possibly get mixed up with "flags". I'll add a "tty_" prefix to the former in a follow-up patch. Thanks, Johan -- 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: serial: qcserial: add support for the Dell DW5821e module
> > So this doesn't really match the Sierra layout in qcserial, where the > > QMI interface is documented as interface 8 (but perhaps we have other > > examples of that already). > > > > Do you know what these serial ports are used for? > > You know what, I kind of missed that. This Dell device is not based on > Sierra Wireless, so using the Sierra layout is not a good idea. Let me > try to confirm with the manufacturer the purpose of each interface. Sounds good. > > Just based on the above, perhaps using option and matching on the vendor > > class, while blacklisting interface 1 would be more appropriate? > > Being a Qualcomm based chipset, I believe qcserial would be more appropriate. > I'll send an updated patch, including usb-devices output, once I have > an explanation for all questions. Note that we have other qualcomm based devices being handled by option already. The qcserial driver was intended for a special subset of Gobi modems with a particular interface layout and firmware loading requirements. So unless your device falls in any of those categories, option may still be preferred. Thanks, Johan -- 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: serial: qcserial: add support for the Dell DW5821e module
On Tue, Jun 26, 2018 at 09:40:24AM +0200, Bjørn Mork wrote:
> Johan Hovold writes:
> > On Sat, Jun 23, 2018 at 11:24:08PM +0200, Aleksander Morgado wrote:
> >> This module exposes two USB configurations: a QMI+AT capable setup on
> >> USB config #1 and a MBIM capable setup on USB config #2.
> >>
> >> By default the kernel will choose the MBIM capable configuration as
> >> long as the cdc_mbim driver is available. This patch adds support for
> >> the serial ports in the secondary configuration.
> >
> > Could you please post the usb-devices output for this device?
> >
> > Depending on another driver to select a specific configuration seems
> > fragile (and that behaviour is even configurable).
>
>
> I believe Aleksander might be referring to usb_choose_configuration() in
> drivers/usb/core/generic.c? It does some confusing things with
> multi-function/multi-configuration devices, explained by this comment:
>
> /* From the remaining configs, choose the first one whose
>* first interface is for a non-vendor-specific class.
>* Reason: Linux is more likely to have a class driver
>* than a vendor-specific driver. */
>
>
> This code can make Linux default to a MBIM configuration if the MBIM
> function uses the first interface in that configuration, even if this
> configuration is not the first one. Availability of a driver is not
> considered. Except for RNDIS, just to make it the whole mess even more
> confusing
Ah, that may be the case. Aleksander, would you mind updating the commit
message and drop the cdc_mbim driver bit?
Please also include the usb-devices output in the commit message for
future reference.
> The result is of course completely arbitrary on any multi-function
> device, where vendor-specific functions may be mixed with class
> functions in any order. The result will also change if you e.g. enable
> a vendor specific debug function in the MBIM configuration, and this
> function happens to use the first interface.
>
> The logic in usb_choose_configuration() is obviously outdated. I assume
> it was created for single function devices, where that single function
> was exposed as either a class function or a vendor specific function
> using separate configurations. This sort of device is still quite
> common for a few device classes, like for example ethernet NICs. But
> things have changed a lot for these as well. Linux now has extensive
> support for vendor sepcific USB functions of all sorts. Not that I
> need to tell you that :-) The legacy code in usb_choose_configuration()
> is now often an obstacle making it more difficult for drivers providing
> the best possible support in Linux. And we end up with horrible hacks
> like this config-dependent blacklist entry in cdc_ether:
>
> #if IS_ENABLED(CONFIG_USB_RTL8152)
> /* Linksys USB3GIGV1 Ethernet Adapter */
> {
> USB_DEVICE_AND_INTERFACE_INFO(LINKSYS_VENDOR_ID, 0x0041, USB_CLASS_COMM,
> USB_CDC_SUBCLASS_ETHERNET, USB_CDC_PROTO_NONE),
> .driver_info = 0,
> },
> #endif
>
> matched with code in the rtl8152 driver to switch the device back to its
> default configuration:
>
> static int rtl8152_probe(struct usb_interface *intf,
>const struct usb_device_id *id)
> {
> struct usb_device *udev = interface_to_usbdev(intf);
> u8 version = rtl_get_version(intf);
> struct r8152 *tp;
> struct net_device *netdev;
> int ret;
>
> if (version == RTL_VER_UNKNOWN)
> return -ENODEV;
>
> if (udev->actconfig->desc.bConfigurationValue != 1) {
> usb_driver_set_configuration(udev, 1);
> return -ENODEV;
> }
> ..
Wow. But from a quick glance at the corresponding commit
10c3271712f5 ("r8152: disable the ECM mode")
it looks like this may at least partly have been due to firmware issues
when switching configurations.
> Extremely ugly, and completely unnecessary if it weren't for that extra
> mess in usb_choose_configuration(). And the net effect is that the
> users end up losing the option of using the class driver for this device,
> since that will be black listed if they (or the distro) built the vendor
> specific driver.
Yeah, not nice.
> Can we fix this? I've thought about it more than once for a few years,
> but so far I've rejected the thought. The Linux defaults coming out of
> usb_choose_configuration() are arbitrary, and often different from any
> other OS, confusing end users. But the Linux defaults are stable as
> long as the device configration is stable. Changing
> usb_choose_configurati
Re: [PATCH] usb: serial: qcserial: add support for the Dell DW5821e module
On Tue, Jun 26, 2018 at 09:32:32AM +0200, Aleksander Morgado wrote: > On Tue, Jun 26, 2018 at 8:09 AM, Johan Hovold wrote: > > On Sat, Jun 23, 2018 at 11:24:08PM +0200, Aleksander Morgado wrote: > >> This module exposes two USB configurations: a QMI+AT capable setup on > >> USB config #1 and a MBIM capable setup on USB config #2. > >> > >> By default the kernel will choose the MBIM capable configuration as > >> long as the cdc_mbim driver is available. This patch adds support for > >> the serial ports in the secondary configuration. > > > > Could you please post the usb-devices output for this device? > > > > Depending on another driver to select a specific configuration seems > > fragile (and that behaviour is even configurable). > > This would be when running on configuration #1: > > T: Bus=04 Lev=03 Prnt=04 Port=02 Cnt=01 Dev#= 7 Spd=5000 MxCh= 0 > D: Ver= 3.10 Cls=ef(misc ) Sub=02 Prot=01 MxPS= 9 #Cfgs= 2 > P: Vendor=413c ProdID=81d7 Rev=03.18 > S: Manufacturer=DELL > S: Product=DW5821e Snapdragon X20 LTE > S: SerialNumber=0123456789ABCDEF > C: #Ifs= 5 Cfg#= 1 Atr=a0 MxPwr=896mA > I: If#=0x0 Alt= 0 #EPs= 2 Cls=ff(vend.) Sub=ff Prot=ff Driver=qcserial > I: If#=0x1 Alt= 0 #EPs= 3 Cls=ff(vend.) Sub=ff Prot=ff Driver=qmi_wwan > I: If#=0x2 Alt= 0 #EPs= 3 Cls=ff(vend.) Sub=00 Prot=00 Driver=qcserial > I: If#=0x3 Alt= 0 #EPs= 3 Cls=ff(vend.) Sub=00 Prot=00 Driver=qcserial > I: If#=0x4 Alt= 0 #EPs= 3 Cls=ff(vend.) Sub=00 Prot=00 Driver=(none) So this doesn't really match the Sierra layout in qcserial, where the QMI interface is documented as interface 8 (but perhaps we have other examples of that already). Do you know what these serial ports are used for? > This would be when running on configuration #2: > > T: Bus=04 Lev=03 Prnt=04 Port=02 Cnt=01 Dev#= 6 Spd=5000 MxCh= 0 > D: Ver= 3.10 Cls=ef(misc ) Sub=02 Prot=01 MxPS= 9 #Cfgs= 2 > P: Vendor=413c ProdID=81d7 Rev=03.18 > S: Manufacturer=DELL > S: Product=DW5821e Snapdragon X20 LTE > S: SerialNumber=0123456789ABCDEF > C: #Ifs= 3 Cfg#= 2 Atr=a0 MxPwr=896mA > I: If#=0x0 Alt= 0 #EPs= 1 Cls=02(commc) Sub=0e Prot=00 Driver=cdc_mbim > I: If#=0x1 Alt= 1 #EPs= 2 Cls=0a(data ) Sub=00 Prot=02 Driver=cdc_mbim > I: If#=0x2 Alt= 0 #EPs= 1 Cls=ff(vend.) Sub=ff Prot=ff Driver=qcserial And here you happen to bind interface 2, but is that intentional? What is that port used for? Just based on the above, perhaps using option and matching on the vendor class, while blacklisting interface 1 would be more appropriate? Johan -- 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: serial: qcserial: add support for the Dell DW5821e module
On Sat, Jun 23, 2018 at 11:24:08PM +0200, Aleksander Morgado wrote: > This module exposes two USB configurations: a QMI+AT capable setup on > USB config #1 and a MBIM capable setup on USB config #2. > > By default the kernel will choose the MBIM capable configuration as > long as the cdc_mbim driver is available. This patch adds support for > the serial ports in the secondary configuration. Could you please post the usb-devices output for this device? Depending on another driver to select a specific configuration seems fragile (and that behaviour is even configurable). Thanks, Johan -- 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 v3] USB: serial: ftdi_sio: Add MTP NVM support
On Mon, Jun 25, 2018 at 10:35:31AM +0200, Loic Poulain wrote:
> Hi Andy,
>
> On 25 June 2018 at 09:36, Andy Shevchenko wrote:
> > On Fri, Jun 22, 2018 at 5:22 PM, Loic Poulain
> > wrote:
> >> +#define EEPROM_SZ_FTX 2048 /* cf FTDI AN_201 */
> >> +#define EEPROM_SZ_FT232RL 128 /* cf FTDI AN_121 */
> >
> > These defines looks too particular, shouldn't be this a part of driver
> > data / platform data / device properties?
>
> I'm not sure where to move this since it depends on chip type which is
> detected at runtime. I don't think ftdi_sio_quirk is the right place.
> So maybe I could just skip the defines for now and come back to:
>
> switch (priv->chip_type) {
> case FTX:
> nvmconf.size = SZ_2K;
> break;
> ...
Yes, please do that. The EEPROM_SZ_ defines you added in this (?)
revision add no value.
Thanks,
Johan
--
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: cast sizeof() to int when comparing with error code
On Mon, Jun 25, 2018 at 03:35:18PM +0800, Chengguang Xu wrote: > Negative error code will be larger than sizeof(). Good catch! I was gonna ask you to submit this as three separate patches to facilitate stable backports, but fortunately it appears none of these have any bad implications (besides possibly the ir-usb one printing a less descriptive debug message). So I'll queue this one of up for -next. Thanks, Johan -- 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: dwc3: of-simple: fix use-after-free on remove
On Thu, Jun 21, 2018 at 11:17:36AM +0300, Roger Quadros wrote: > On 21/06/18 01:55, Rafael J. Wysocki wrote: > > On Thu, Jun 21, 2018 at 12:32 AM, Rafael J. Wysocki > > wrote: > >> On Wed, Jun 20, 2018 at 5:46 PM, Johan Hovold wrote: > >>> On Wed, Jun 20, 2018 at 02:54:10PM +0200, Rafael J. Wysocki wrote: > >>>> On Wednesday, June 20, 2018 2:23:46 PM CEST Johan Hovold wrote: > >>>>> On Wed, Jun 20, 2018 at 02:16:59AM -0700, Tony Lindgren wrote: > >>>>>> Hi, > >>>>>> > >>>>>> Adding Rafael and linux-pm to Cc as well. > >>>>>> > >>>>>> * Felipe Balbi [180619 01:23]: > >>>>>>> This is a direct consequence of not paying attention to the order of > >>>>>>> things. If driver were to assume that pm_domain->activate() would do > >>>>>>> the > >>>>>>> right thing for the device -- meaning that probe would run with an > >>>>>>> active device --, then we wouldn't need that pm_runtime_get() call on > >>>>>>> probe at all. Rather we would follow the sequence: > >>>>>>> > >>>>>>> pm_runtime_forbid() > >>>>>>> pm_runtime_set_active() > >>>>>>> pm_runtime_enable() > >>>>>>> > >>>>>>> /* do your probe routine */ > >>>>>>> > >>>>>>> pm_runtime_put_noidle() > >>>>>>> > >>>>>>> Then you remove you would need to call pm_runtime_get_noresume() to > >>>>>>> balance out the pm_runtime_put_noidle() there. > >>>>> > >>>>>>> (If you need to know why the pm_runtime_put_noidle(), remember that > >>>>>>> pm_runtime_set_active() increments the usage counter, so > >>>>>>> pm_runtime_put_noidle is basically allowing pm_runtime to happen as > >>>>>>> soon > >>>>>>> as userspace writes "auto" to /sys//power/control) > >>>>> > >>>>> That's not correct; pm_runtime_set_active() only increments the usage > >>>>> counter of a parent (under some circumstances), so unless you have bus > >>>>> code incrementing the usage counter before probe, the above > >>>>> pm_runtime_put_noidle() would actually introduce an imbalance. > >>>> > >>>> No, it wouldn't. It balances the incrementation in pm_runtime_forbid(). > >>> > >>> Right, but even if you take the whole sequence, which included > >>> pm_runtime_forbid(), consider what happens when pm_runtime_allow() is > >>> later called through sysfs (see below). > >>> > >>>>> And note that that's also the case even if you meant to say that > >>>>> *pm_runtime_forbid()* increments the usage counter (which it does). > >>>> > >>>> Why is it? > >>>> > >>>> Surely, after > >>>> > >>>> pm_runtime_forbid(dev); > >>>> pm_runtime_put_noidle(dev); > >>>> > >>>> the runtime PM usage counter of dev will be the same as before, won't it? > >>> > >>> Sure, but the imbalance, or rather inconsistent state, has already been > >>> introduced. > >>> > >>> Consider the following sequence of events: > >>> > >>> usage count > >>> 0 > >>> probe() > >>> pm_runtime_forbid() 1 > > Can you call pm_runtime_forbid() before pm_runtime_enable()? > Wouldn't it fail with -EACCES as dev->power.disable_depth > 0? No, pm_runtime_forbid() manipulates the usage count directly and doesn't check for errors from rpm_resume(), so this actually "works". That doesn't mean anyone should be doing it, though (e.g. for the below reasons). > >>> pm_runtime_set_active() > >>> pm_runtime_enable() > >>> pm_runtime_put_noidle() 0 > >>> > >>> Here nothing is preventing the device from runtime suspending, despite > >>> runtime PM being forbidden. In fact, it will typically be suspended due > >>> to the pm_request_idle() in driver_probe_device(). If later we have: > >>> > >>> echo auto > power/control > >>> pm_runtime_allow() -1 > >> > >> OK, you have a point. > >> > >> After calling pm_runtime_forbid() the driver should allow user space > >> to unblock runtime PM for the device - or call pm_runtime_allow() > >> itself. > > > > The confusion regarding the pm_runtime_put_noidle() at the end may > > come from the special requirement of the PCI bus type as per the > > comment in local_pci_probe(). > > OK. So it is the PCI bus which is behaving odd here and > pm_runtime_put_noidle() needs to be done only if its a PCI device, correct? Yeah, the pm_runtime_put_noidle() would be used to balance the unconditional runtime resume by the bus code in case a PCI driver implements runtime pm. Johan -- 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: dwc3: of-simple: fix use-after-free on remove
On Thu, Jun 21, 2018 at 12:55:26AM +0200, Rafael J. Wysocki wrote: > On Thu, Jun 21, 2018 at 12:32 AM, Rafael J. Wysocki wrote: > > On Wed, Jun 20, 2018 at 5:46 PM, Johan Hovold wrote: > >> On Wed, Jun 20, 2018 at 02:54:10PM +0200, Rafael J. Wysocki wrote: > >>> On Wednesday, June 20, 2018 2:23:46 PM CEST Johan Hovold wrote: > >>> > On Wed, Jun 20, 2018 at 02:16:59AM -0700, Tony Lindgren wrote: > >>> > > Hi, > >>> > > > >>> > > Adding Rafael and linux-pm to Cc as well. > >>> > > > >>> > > * Felipe Balbi [180619 01:23]: > >>> > > > This is a direct consequence of not paying attention to the order of > >>> > > > things. If driver were to assume that pm_domain->activate() would > >>> > > > do the > >>> > > > right thing for the device -- meaning that probe would run with an > >>> > > > active device --, then we wouldn't need that pm_runtime_get() call > >>> > > > on > >>> > > > probe at all. Rather we would follow the sequence: > >>> > > > > >>> > > > pm_runtime_forbid() > >>> > > > pm_runtime_set_active() > >>> > > > pm_runtime_enable() > >>> > > > > >>> > > > /* do your probe routine */ > >>> > > > > >>> > > > pm_runtime_put_noidle() > >>> > > > > >>> > > > Then you remove you would need to call pm_runtime_get_noresume() to > >>> > > > balance out the pm_runtime_put_noidle() there. > >>> > > >>> > > > (If you need to know why the pm_runtime_put_noidle(), remember that > >>> > > > pm_runtime_set_active() increments the usage counter, so > >>> > > > pm_runtime_put_noidle is basically allowing pm_runtime to happen as > >>> > > > soon > >>> > > > as userspace writes "auto" to /sys//power/control) > >>> > > >>> > That's not correct; pm_runtime_set_active() only increments the usage > >>> > counter of a parent (under some circumstances), so unless you have bus > >>> > code incrementing the usage counter before probe, the above > >>> > pm_runtime_put_noidle() would actually introduce an imbalance. > The confusion regarding the pm_runtime_put_noidle() at the end may > come from the special requirement of the PCI bus type as per the > comment in local_pci_probe(). That seems to be the case, but I'm not sure of much of what PCI is doing that can be applied here (e.g. OMAP platform devices), where unbound devices should always be powered off and were I assume we want to have runtime pm allowed by default, for example. Johan -- 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: dwc3: of-simple: fix use-after-free on remove
On Thu, Jun 21, 2018 at 12:32:59AM +0200, Rafael J. Wysocki wrote: > On Wed, Jun 20, 2018 at 5:46 PM, Johan Hovold wrote: > > On Wed, Jun 20, 2018 at 02:54:10PM +0200, Rafael J. Wysocki wrote: > >> On Wednesday, June 20, 2018 2:23:46 PM CEST Johan Hovold wrote: > >> > On Wed, Jun 20, 2018 at 02:16:59AM -0700, Tony Lindgren wrote: > >> > > Hi, > >> > > > >> > > Adding Rafael and linux-pm to Cc as well. > >> > > > >> > > * Felipe Balbi [180619 01:23]: > >> > > > This is a direct consequence of not paying attention to the order of > >> > > > things. If driver were to assume that pm_domain->activate() would do > >> > > > the > >> > > > right thing for the device -- meaning that probe would run with an > >> > > > active device --, then we wouldn't need that pm_runtime_get() call on > >> > > > probe at all. Rather we would follow the sequence: > >> > > > > >> > > > pm_runtime_forbid() > >> > > > pm_runtime_set_active() > >> > > > pm_runtime_enable() > >> > > > > >> > > > /* do your probe routine */ > >> > > > > >> > > > pm_runtime_put_noidle() > >> > > > > >> > > > Then you remove you would need to call pm_runtime_get_noresume() to > >> > > > balance out the pm_runtime_put_noidle() there. > >> > > >> > > > (If you need to know why the pm_runtime_put_noidle(), remember that > >> > > > pm_runtime_set_active() increments the usage counter, so > >> > > > pm_runtime_put_noidle is basically allowing pm_runtime to happen as > >> > > > soon > >> > > > as userspace writes "auto" to /sys//power/control) > >> > > >> > That's not correct; pm_runtime_set_active() only increments the usage > >> > counter of a parent (under some circumstances), so unless you have bus > >> > code incrementing the usage counter before probe, the above > >> > pm_runtime_put_noidle() would actually introduce an imbalance. > >> > >> No, it wouldn't. It balances the incrementation in pm_runtime_forbid(). > > > > Right, but even if you take the whole sequence, which included > > pm_runtime_forbid(), consider what happens when pm_runtime_allow() is > > later called through sysfs (see below). > > > >> > And note that that's also the case even if you meant to say that > >> > *pm_runtime_forbid()* increments the usage counter (which it does). > >> > >> Why is it? > >> > >> Surely, after > >> > >> pm_runtime_forbid(dev); > >> pm_runtime_put_noidle(dev); > >> > >> the runtime PM usage counter of dev will be the same as before, won't it? > > > > Sure, but the imbalance, or rather inconsistent state, has already been > > introduced. > > > > Consider the following sequence of events: > > > > usage count > > 0 > > probe() > > pm_runtime_forbid() 1 > > pm_runtime_set_active() > > pm_runtime_enable() > > pm_runtime_put_noidle() 0 > > > > Here nothing is preventing the device from runtime suspending, despite > > runtime PM being forbidden. In fact, it will typically be suspended due > > to the pm_request_idle() in driver_probe_device(). If later we have: > > > > echo auto > power/control > > pm_runtime_allow() -1 > > OK, you have a point. > > After calling pm_runtime_forbid() the driver should allow user space > to unblock runtime PM for the device - or call pm_runtime_allow() > itself. Right, the usage count increment done by forbid() should only be balanced by allow(). > [cut] > > > > > And if runtime pm is later again forbidden: > > > > echo on > power/control > > pm_runtime_forbid() 0 > > > > then the device will not be resumed. > > But I don't quite see why that will be the case. rpm_resume() will > still be called and it doesn't look at the usage counter. You're right, I left out some important details and jumped to the conclusion. As you point out, the device will be unconditionally resumed, but due to the usage counter being zero the idle notification queued by rpm_resume() will typically suspend it straight away despite runtime pm being forbidden (cf. the idle notification issued by driver core after probe() returns). Johan -- 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: dwc3: of-simple: fix use-after-free on remove
On Wed, Jun 20, 2018 at 02:54:10PM +0200, Rafael J. Wysocki wrote: > On Wednesday, June 20, 2018 2:23:46 PM CEST Johan Hovold wrote: > > On Wed, Jun 20, 2018 at 02:16:59AM -0700, Tony Lindgren wrote: > > > Hi, > > > > > > Adding Rafael and linux-pm to Cc as well. > > > > > > * Felipe Balbi [180619 01:23]: > > > > This is a direct consequence of not paying attention to the order of > > > > things. If driver were to assume that pm_domain->activate() would do the > > > > right thing for the device -- meaning that probe would run with an > > > > active device --, then we wouldn't need that pm_runtime_get() call on > > > > probe at all. Rather we would follow the sequence: > > > > > > > > pm_runtime_forbid() > > > > pm_runtime_set_active() > > > > pm_runtime_enable() > > > > > > > > /* do your probe routine */ > > > > > > > > pm_runtime_put_noidle() > > > > > > > > Then you remove you would need to call pm_runtime_get_noresume() to > > > > balance out the pm_runtime_put_noidle() there. > > > > > > (If you need to know why the pm_runtime_put_noidle(), remember that > > > > pm_runtime_set_active() increments the usage counter, so > > > > pm_runtime_put_noidle is basically allowing pm_runtime to happen as soon > > > > as userspace writes "auto" to /sys//power/control) > > > > That's not correct; pm_runtime_set_active() only increments the usage > > counter of a parent (under some circumstances), so unless you have bus > > code incrementing the usage counter before probe, the above > > pm_runtime_put_noidle() would actually introduce an imbalance. > > No, it wouldn't. It balances the incrementation in pm_runtime_forbid(). Right, but even if you take the whole sequence, which included pm_runtime_forbid(), consider what happens when pm_runtime_allow() is later called through sysfs (see below). > > And note that that's also the case even if you meant to say that > > *pm_runtime_forbid()* increments the usage counter (which it does). > > Why is it? > > Surely, after > > pm_runtime_forbid(dev); > pm_runtime_put_noidle(dev); > > the runtime PM usage counter of dev will be the same as before, won't it? Sure, but the imbalance, or rather inconsistent state, has already been introduced. Consider the following sequence of events: usage count 0 probe() pm_runtime_forbid() 1 pm_runtime_set_active() pm_runtime_enable() pm_runtime_put_noidle() 0 Here nothing is preventing the device from runtime suspending, despite runtime PM being forbidden. In fact, it will typically be suspended due to the pm_request_idle() in driver_probe_device(). If later we have: echo auto > power/control pm_runtime_allow() -1 then the device remains suspended, but we get a negative usage count. This does not seem to play well with neither runtime pm (consider a synchronous get followed by a put, which will fail to again suspend the device) or, for example, system suspend, which tries to prevent runtime pm by incrementing the usage count. And if runtime pm is later again forbidden: echo on > power/control pm_runtime_forbid() 0 then the device will not be resumed. Johan -- 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: dwc3: of-simple: fix use-after-free on remove
On Wed, Jun 20, 2018 at 02:16:59AM -0700, Tony Lindgren wrote: > Hi, > > Adding Rafael and linux-pm to Cc as well. > > * Felipe Balbi [180619 01:23]: > > This is a direct consequence of not paying attention to the order of > > things. If driver were to assume that pm_domain->activate() would do the > > right thing for the device -- meaning that probe would run with an > > active device --, then we wouldn't need that pm_runtime_get() call on > > probe at all. Rather we would follow the sequence: > > > > pm_runtime_forbid() > > pm_runtime_set_active() > > pm_runtime_enable() > > > > /* do your probe routine */ > > > > pm_runtime_put_noidle() > > > > Then you remove you would need to call pm_runtime_get_noresume() to > > balance out the pm_runtime_put_noidle() there. > > (If you need to know why the pm_runtime_put_noidle(), remember that > > pm_runtime_set_active() increments the usage counter, so > > pm_runtime_put_noidle is basically allowing pm_runtime to happen as soon > > as userspace writes "auto" to /sys//power/control) That's not correct; pm_runtime_set_active() only increments the usage counter of a parent (under some circumstances), so unless you have bus code incrementing the usage counter before probe, the above pm_runtime_put_noidle() would actually introduce an imbalance. And note that that's also the case even if you meant to say that *pm_runtime_forbid()* increments the usage counter (which it does). Johan -- 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: serial: ftdi_sio: Add MTP NVM support
On Tue, Jun 19, 2018 at 02:32:11PM +0200, Loic Poulain wrote: > Hi Johan, Srini, > > On 18 June 2018 at 11:47, Srinivas Kandagatla > wrote: > > On 18/06/18 09:46, Johan Hovold wrote: > >> I'm not necessarily against the idea, but nvmem core needs to be fixed > >> so that it can handle hotplugging before this can be considered for > >> merging. > >> > >> Right now it just returns -EBUSY from nvmem_unregister(), which results > >> in all kinds of memory leaks, use-after-frees and crashes when user > >> space holds the character device open while the device is being > >> unplugged. > > Correct me if I'm wrong, but nvmem is just exposed to userspace as a > simple sysfs device attribute (nvmem), removing a device and its attribute(s) > dynamically is well managed by sysfs, even if userspace has file open. My bad, I misremembered what interface nvmem was using and only saw the deregistration refusal in nvmem_unregister() during a quick check. > The only risk here is when a kernel internal consumer still has a reference to > the nvmem device on removal, which is not the case in our context. Right. > > I can also suggest you to try devm_nvmem_register(). > > Yes, I'm going to send a v2. I'll take a closer look soonish too. Johan -- 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: dwc3: of-simple: fix use-after-free on remove
On Mon, Jun 18, 2018 at 12:33:44PM +0300, Felipe Balbi wrote: > Johan Hovold writes: > > I suggest merging this fix for 4.18-rc, and then Roger can rework the > > driver so that it works also on OMAP. > > omap has its own glue layer for several reasons. If you're talking about > Keystone devices, then okay, I understand. But in that case, this would > mean Keystone is copying the same arguably broken PM domain design from > OMAP and it would be best not to propagate that idea. Maybe so. I'm not sure what Roger's use case is, but perhaps the omap glue driver could be used instead. Thanks, Johan -- 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: Inquiry for adding PID to Cp2102 driver
On Mon, Jun 18, 2018 at 11:29:51AM +0200, Carlos Barcala Lara wrote: > Thanks again, Johan. > > Should I have a user to do the git-am (if needed in the future)? That would only be needed if you ever want to add further device ids. Then you can create a patch (e.g. by committing you changes to a local git tree and use git-format-patch), submit it to yourself for testing purposes (e.g. using git-send-email), and finally apply it using git-am. The patch submission process is documented in the kernel source tree: Documentation/process/submitting-patches.rst But nothing more is needed for the device ids you reported in this thread. The mainline and stable tree drivers will soon support them out of the box. Thanks, Johan -- 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: Inquiry for adding PID to Cp2102 driver
On Mon, Jun 18, 2018 at 10:53:35AM +0200, Carlos Barcala Lara wrote: > Thank you very much, Johan. > > The PIDs are all right. Thanks for checking. > I´m sorry, but I don´t know how to use the patch, if needed in the > future. It would need to be applied (e.g. using git-am) to the kernel source tree so that a cp210x driver which includes the new device ids can be compiled. I've applied the patch for 4.18-rc with a stable tag so that any kernel based on the stable trees will soon support your devices as well. Thanks, Johan -- 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: dwc3: of-simple: fix use-after-free on remove
On Mon, Jun 18, 2018 at 11:15:41AM +0300, Felipe Balbi wrote: > I don't use this glue layer, actually. As long as there are no > regressions, you can change it to your heart's content. I still it's > best to start with pm runtime blocked and let userspace decide what and > when should have pm runtime enabled. I don't use it either, I just noticed the use-after-free in remove() (and that probe was returning with a positive runtime PM usage count). I suggest merging this fix for 4.18-rc, and then Roger can rework the driver so that it works also on OMAP. Thanks, Johan -- 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: serial: ftdi_sio: Add MTP NVM support
On Thu, Jun 14, 2018 at 10:08:46PM +0200, Loic Poulain wrote:
> Most of FTDI's devices have an EEPROM which records FTDI devices
> configuration setting (e.g. the VID, PID, I/O config...) and user
> data. FT230R chip integrates a 128-byte eeprom, FT230X a 2048-byte
> eeprom...
>
> This patch adds support for FTDI EEPROM read/write via USB control
> transfers and register a new nvm device to the nvmem core.
>
> This permits to expose the eeprom as a sysfs file, allowing userspace
> to read/modify FTDI configuration and its user data without having to
> rely on a specific userspace USB driver.
>
> Moreover, any upcoming new tentative to add CBUS GPIO support could
> integrate CBUS EEPROM configuration reading in order to determine
> which of the CBUS pins are available as GPIO.
I'm not necessarily against the idea, but nvmem core needs to be fixed
so that it can handle hotplugging before this can be considered for
merging.
Right now it just returns -EBUSY from nvmem_unregister(), which results
in all kinds of memory leaks, use-after-frees and crashes when user
space holds the character device open while the device is being
unplugged.
> +static void unregister_eeprom(struct usb_serial_port *port)
> +{
> + struct ftdi_private *priv = usb_get_serial_port_data(port);
> +
> + if (priv->eeprom)
> + nvmem_unregister(priv->eeprom);
> +}
> @@ -1931,6 +2036,9 @@ static int ftdi_sio_port_remove(struct usb_serial_port
> *port)
> {
> struct ftdi_private *priv = usb_get_serial_port_data(port);
>
> +#if IS_ENABLED(CONFIG_USB_SERIAL_FTDI_SIO_NVMEM)
> + unregister_eeprom(port);
> +#endif
> remove_sysfs_attrs(port);
>
> kfree(priv);
Thanks,
Johan
--
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: Inquiry for adding PID to Cp2102 driver
On Fri, Jun 15, 2018 at 05:05:38PM +0200, Carlos Barcala Lara wrote:
> Dear Johan (and rest of the team)
>
> This is Carlos Barcala, from CESINEL SL, in Spain. We are a little
> manufacturer
> of Power Quality Analyzers and Energy Meters. We include in all our products
> the Silabs CP2102 chip for USB communications and we would like to include
> support for our instruments in Linux.
>
> Somebody gave me your contact regarding this matter, so I beg you to include
> the following items in the Silabs CP2102 Linux kernel´s driver:
>
> USB\VID_10C4_817C --> "CESINEL MEDCAL N Power Quality Monitor"
> USB\VID_10C4_817D --> "CESINEL MEDCAL NT Power Quality Monitor"
> USB\VID_10C4_817E --> "CESINEL MEDCAL S Power Quality Monitor"
> USB\VID_10C4_82EF --> "CESINEL FALCO 6105 AC Power Supply"
> USB\VID_10C4_82F1 --> "CESINEL MEDCAL EFD Earth Fault Detector"
> USB\VID_10C4_82F2 --> "CESINEL MEDCAL ST Network Analyzer"
> USB\VID_10C4_851E --> "CESINEL MEDCAL PT Network Analyzer"
> USB\VID_10C4_85B8 --> "CESINEL ReCon T Energy Logger"
> USB\VID_10C4_88FB --> "CESINEL MEDCAL STII Network Analyzer"
> USB\VID_10C4_8938 --> "CESINEL MEDCAL S II Network Analyzer"
> USB\VID_10C4_89A4 --> "CESINEL FTBC Flexible Thyristor Bridge Controller"
Would you mind taking a look at the below patch and double check that
I got all the entries right?
If you ever need to add more ids you you could use that patch as a
basis.
Thanks,
Johan
>From c4e9e567fadd76cfe9f70fc9377598a6b7187c38 Mon Sep 17 00:00:00 2001
From: Johan Hovold
Date: Mon, 18 Jun 2018 10:09:44 +0200
Subject: [PATCH] USB: serial: cp210x: add CESINEL device ids
Add device ids for CESINEL products.
Reported-by: Carlos Barcala Lara
Cc: stable
Signed-off-by: Johan Hovold
---
drivers/usb/serial/cp210x.c | 11 +++
1 file changed, 11 insertions(+)
diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
index 14cf657247b6..ee0cc1d90b51 100644
--- a/drivers/usb/serial/cp210x.c
+++ b/drivers/usb/serial/cp210x.c
@@ -95,6 +95,9 @@ static const struct usb_device_id id_table[] = {
{ USB_DEVICE(0x10C4, 0x8156) }, /* B H3000 link cable */
{ USB_DEVICE(0x10C4, 0x815E) }, /* Helicomm IP-Link 1220-DVM */
{ USB_DEVICE(0x10C4, 0x815F) }, /* Timewave HamLinkUSB */
+ { USB_DEVICE(0x10C4, 0x817C) }, /* CESINEL MEDCAL N Power Quality
Monitor */
+ { USB_DEVICE(0x10C4, 0x817D) }, /* CESINEL MEDCAL NT Power Quality
Monitor */
+ { USB_DEVICE(0x10C4, 0x817E) }, /* CESINEL MEDCAL S Power Quality
Monitor */
{ USB_DEVICE(0x10C4, 0x818B) }, /* AVIT Research USB to TTL */
{ USB_DEVICE(0x10C4, 0x819F) }, /* MJS USB Toslink Switcher */
{ USB_DEVICE(0x10C4, 0x81A6) }, /* ThinkOptics WavIt */
@@ -112,6 +115,9 @@ static const struct usb_device_id id_table[] = {
{ USB_DEVICE(0x10C4, 0x826B) }, /* Cygnal Integrated Products, Inc.,
Fasttrax GPS demonstration module */
{ USB_DEVICE(0x10C4, 0x8281) }, /* Nanotec Plug & Drive */
{ USB_DEVICE(0x10C4, 0x8293) }, /* Telegesis ETRX2USB */
+ { USB_DEVICE(0x10C4, 0x82EF) }, /* CESINEL FALCO 6105 AC Power Supply */
+ { USB_DEVICE(0x10C4, 0x82F1) }, /* CESINEL MEDCAL EFD Earth Fault
Detector */
+ { USB_DEVICE(0x10C4, 0x82F2) }, /* CESINEL MEDCAL ST Network Analyzer */
{ USB_DEVICE(0x10C4, 0x82F4) }, /* Starizona MicroTouch */
{ USB_DEVICE(0x10C4, 0x82F9) }, /* Procyon AVS */
{ USB_DEVICE(0x10C4, 0x8341) }, /* Siemens MC35PU GPRS Modem */
@@ -124,7 +130,9 @@ static const struct usb_device_id id_table[] = {
{ USB_DEVICE(0x10C4, 0x8470) }, /* Juniper Networks BX Series System
Console */
{ USB_DEVICE(0x10C4, 0x8477) }, /* Balluff RFID */
{ USB_DEVICE(0x10C4, 0x84B6) }, /* Starizona Hyperion */
+ { USB_DEVICE(0x10C4, 0x851E) }, /* CESINEL MEDCAL PT Network Analyzer */
{ USB_DEVICE(0x10C4, 0x85A7) }, /* LifeScan OneTouch Verio IQ */
+ { USB_DEVICE(0x10C4, 0x85B8) }, /* CESINEL ReCon T Energy Logger */
{ USB_DEVICE(0x10C4, 0x85EA) }, /* AC-Services IBUS-IF */
{ USB_DEVICE(0x10C4, 0x85EB) }, /* AC-Services CIS-IBUS */
{ USB_DEVICE(0x10C4, 0x85F8) }, /* Virtenio Preon32 */
@@ -134,10 +142,13 @@ static const struct usb_device_id id_table[] = {
{ USB_DEVICE(0x10C4, 0x8857) }, /* CEL EM357 ZigBee USB Stick */
{ USB_DEVICE(0x10C4, 0x88A4) }, /* MMB Networks ZigBee USB Device */
{ USB_DEVICE(0x10C4, 0x88A5) }, /* Planet Innovation Ingeni ZigBee USB
Device */
+ { USB_DEVICE(0x10C4, 0x88FB) }, /* CESINEL MEDCAL STII Network Analyzer
*/
+ { USB_DEVICE(0x10C4, 0x8938) }, /* CESINEL MEDCAL S II Network Analyzer
*/
{ USB_DEVICE(0x10C4, 0x8946) }, /* Ketra N1 Wireless Interface */
{ USB_DEVIC
Re: [PATCH] USB: serial: cp210x: add Silicon Labs IDs for Windows Update
On Mon, Jun 18, 2018 at 07:26:24AM +, Karoly Pados wrote: > The way this works is that the chips still come with the standard > SiLabs IDs. When the chip is integrated into an end-user product, the > product manufacturer has the choice to reprogram the IDs. If they are > left on the factory standard setting, the user will have to install > drivers manually (go to SiLabs website, search, download, install by > hand). Alternatively, the product manufacturer can reprogram the IDs > to the "new" ones for Windows Update before shipping it to the > customer, in which case Windows will automatically find, download, and > install the drivers when plugged in the first time. > > More (official) info here: > https://www.silabs.com/community/interface/knowledge-base.entry.html/2016/12/30/downloading_cp210xd-ek07 Thanks for the info. > About the cutoff line: Sorry about that, now I know (first kernel > patch for me). It seems you prefer to just adjust the commit message > for now, otherwise let me know if you'd like me to resubmit. Yeah, no worries. I've applied the patch for 4.17-rc with a stable tag now. Thanks, Johan -- 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: serial: cp210x: add Silicon Labs IDs for Windows Update
On Sat, Jun 09, 2018 at 01:26:08PM +0200, Karoly Pados wrote:
> Silicon Labs defines alternative VID/PID pairs for some chips that when
> used will automatically install drivers for Windows users without manual
> intervention. Unfortunately, these IDs are not recognized by the Linux
> module, so using these IDs improves user experience on one platform but
> degrades it on Linux. This patch addresses this problem.
How does this work; do these chips now come with the "windows update"
PIDs, and the silabs drivers then reprogram them to use other PIDs once
installed?
Is this documented somewhere?
> Now with mailing list in CC.
This kind of revision information should go below the cut-off line (---)
below. I'll just remove it this time unless there's a resend.
> Signed-off-by: Karoly Pados
> ---
> drivers/usb/serial/cp210x.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
> index eb6c26cbe579..b1849f657e01 100644
> --- a/drivers/usb/serial/cp210x.c
> +++ b/drivers/usb/serial/cp210x.c
> @@ -143,8 +143,11 @@ static const struct usb_device_id id_table[] = {
> { USB_DEVICE(0x10C4, 0x8B34) }, /* Qivicon ZigBee USB Radio Stick */
> { USB_DEVICE(0x10C4, 0xEA60) }, /* Silicon Labs factory default */
> { USB_DEVICE(0x10C4, 0xEA61) }, /* Silicon Labs factory default */
> + { USB_DEVICE(0x10C4, 0xEA63) }, /* Silicon Labs values for Windows
> Update support (CP2101-4/CP2102N) */
I may shorten this somewhat.
> { USB_DEVICE(0x10C4, 0xEA70) }, /* Silicon Labs factory default */
> { USB_DEVICE(0x10C4, 0xEA71) }, /* Infinity GPS-MIC-1 Radio Monophone */
> + { USB_DEVICE(0x10C4, 0xEA7A) }, /* Silicon Labs values for Windows
> Update support (CP2105) */
> + { USB_DEVICE(0x10C4, 0xEA7B) }, /* Silicon Labs values for Windows
> Update support (CP2108) */
> { USB_DEVICE(0x10C4, 0xF001) }, /* Elan Digital Systems USBscope50 */
> { USB_DEVICE(0x10C4, 0xF002) }, /* Elan Digital Systems USBwave12 */
> { USB_DEVICE(0x10C4, 0xF003) }, /* Elan Digital Systems USBpulse100 */
Thanks,
Johan
--
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: dwc3: of-simple: fix use-after-free on remove
On Wed, Jun 13, 2018 at 12:39:18PM +0300, Felipe Balbi wrote:
> Roger Quadros writes:
> > I'm still trying to get my head around this.
> >
> > in probe() we do
> > {
> > enable all clocks;
> > pm_runtime_set_active(dev);
> > pm_runtime_enable(dev);
> > pm_runtime_get_sync(dev);
> > }
> >
> > How will runtime suspend work at all?
> > We're holding a positive RPM count in probe().
>
> echo auto > /path/to/dwc3/power/control
That makes no difference, user space cannot modify the always-active
behaviour given that probe returns with a positive usage count.
> Granted, that get_sync() would've been better as a pm_runtime_forbid()
Yeah, that would allow user space some control, albeit in a way that
may override a user space configuration (as the platform device would
already have been registered).
Why are you trying to prevent runtime pm in the first place? Shouldn't
the device be allowed to suspend when it has no active child by default?
Johan
--
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: 4.16.14: kernel tried to execute NX-protected page [after USB device went to charging state]
[ +CC: linux-usb, even if this does not look like a USB issue ] On Sat, Jun 09, 2018 at 11:50:34AM +0200, Udo van den Heuvel wrote: > Hello, > > My Holus GPSport 245 was used to download a gpx track. Afterwards I > turned the device off while it was attached to USB so it could charge. > Later I found these messages you can find below. > Is this an actual bug? Well, you've got some kind of corruption going on somewhere. > [223632.768623] usb 1-7: cp210x converter now attached to ttyUSB0 > [225389.048501] usb 1-7: USB disconnect, device number 6 > [225389.048758] cp210x ttyUSB0: cp210x converter now disconnected from > ttyUSB0 > [225389.048785] kernel tried to execute NX-protected page - exploit > attempt? (uid: 0) > [225389.048788] BUG: unable to handle kernel paging request at > c08b64e0 > [225389.048797] IP: usb_serial_exit+0x35df/0xff [usbserial] > [225389.048799] PGD 2ea00c067 P4D 2ea00c067 PUD 2ea00e067 PMD 408590067 > PTE 800109510163 > [225389.048807] Oops: 0011 [#1] PREEMPT SMP NOPTI > [225389.048809] Modules linked in: cp210x usbserial it87(O) hwmon_vid First, please try and reproduce this after blacklisting this out-of-tree it87 module. > fuse ipt_REJECT nf_reject_ipv4 xt_u32 xt_multiport iptable_filter > ipt_MASQUERADE nf_nat_masquerade_ipv4 iptable_nat nf_conntrack_ipv4 > nf_defrag_ipv4 nf_nat_ipv4 nf_nat cpufreq_userspace > nf_conntrack_netbios_ns nf_conntrack_broadcast ip6t_REJECT > nf_reject_ipv6 xt_tcpudp nf_conntrack_ipv6 nf_defrag_ipv6 xt_conntrack > msr nf_conntrack ip6table_filter ip6_tables eeprom uvcvideo > videobuf2_vmalloc videobuf2_memops videobuf2_v4l2 snd_usb_audio videodev > snd_hwdep videobuf2_common cdc_acm snd_usbmidi_lib snd_rawmidi amdgpu > snd_hda_codec_realtek snd_hda_codec_generic snd_hda_intel snd_hda_codec > snd_hda_core snd_seq snd_seq_device snd_pcm chash snd_timer gpu_sched > backlight snd ttm i2c_piix4 evdev acpi_cpufreq k10temp nfsd auth_rpcgss > nfs_acl > [225389.048857] lockd grace sunrpc binfmt_misc ip_tables x_tables > hid_generic sr_mod cdrom usbhid i2c_dev autofs4 [last unloaded: hwmon_vid] > [225389.048871] CPU: 1 PID: 5717 Comm: kworker/1:2 Tainted: G > O 4.16.14 #5 > [225389.048873] Hardware name: Gigabyte Technology Co., Ltd. X470 AORUS > ULTRA GAMING/X470 AORUS ULTRA GAMING-CF, BIOS F3g 05/10/2018 > [225389.048880] Workqueue: usb_hub_wq hub_event > [225389.048886] RIP: 0010:usb_serial_exit+0x35df/0xff [usbserial] > [225389.048889] RSP: 0018:90d3c8c27be8 EFLAGS: 00010282 > [225389.048892] RAX: c08b64e0 RBX: 8bd5d2190ae8 RCX: > > [225389.048895] RDX: 8001 RSI: 0282 RDI: > 8bd5d2190ad8 > [225389.048897] RBP: 8bd5d2190ad8 R08: R09: > > [225389.048899] R10: R11: R12: > 8bd392029480 > [225389.048902] R13: 8bd64b4d4e00 R14: 8bd64d2fc030 R15: > 8bd64d2fc030 > [225389.048905] FS: () GS:8bd65ee4() > knlGS: > [225389.048908] CS: 0010 DS: ES: CR0: 80050033 > [225389.048910] CR2: c08b64e0 CR3: 0003f0b5 CR4: > 003406e0 > [225389.048912] Call Trace: > [225389.048918] ? device_release+0x39/0xa0 > [225389.048924] ? kobject_put+0xa1/0x1c0 > [225389.048929] ? usb_serial_put+0x4c/0xf0 [usbserial] > [225389.048933] ? usb_serial_disconnect+0xdd/0x100 [usbserial] > [225389.048938] ? usb_unbind_interface+0x66/0x1e0 > [225389.048942] ? device_release_driver_internal+0x17a/0x230 > [225389.048946] ? bus_remove_device+0xe0/0x150 > [225389.048950] ? device_del+0x129/0x330 > [225389.048954] ? usb_disable_device+0x8d/0x230 > [225389.048958] ? usb_disconnect+0xb1/0x270 > [225389.048962] ? hub_event+0x5f5/0x13b0 > [225389.048967] ? SyS_uname+0x11/0xa0 > [225389.048971] ? process_one_work+0x1a1/0x2f0 > [225389.048974] ? worker_thread+0x26/0x3f0 > [225389.048978] ? process_one_work+0x2f0/0x2f0 > [225389.048982] ? kthread+0x109/0x120 > [225389.048986] ? kthread_create_on_node+0x60/0x60 > [225389.048991] ? ret_from_fork+0x22/0x40 > [225389.048994] Code: ff ff ff 29 1a 8b c0 ff ff ff ff 50 73 8b c0 ff ff > ff ff 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > 00 00 34 8b c0 ff ff ff ff 00 00 00 00 00 00 00 00 00 65 8b c0 ff > [225389.049043] RIP: usb_serial_exit+0x35df/0xff [usbserial] RSP: > 90d3c8c27be8 > [225389.049045] CR2: c08b64e0 > [225389.049048] ---[ end trace 43c4e5674b0ca81f ]--- This looks to me like you've got a struct device whose release pointer is pointing into a non-executable page. The IP symbol looks weird usb_serial_exit+0x35df/0xff but this could correspond with usb_serial_port_release (check /proc/kallsyms as root). Enabling dynamic debugging for usbserial might give some indication of how far you get in usb_serial_put(), but this smells like an x86/mem (or hardware?) issue. Did you say you could reproduce this
Re: [PATCH] usb: dwc3: of-simple: fix use-after-free on remove
On Thu, May 31, 2018 at 04:45:52PM +0200, Johan Hovold wrote:
> The clocks have already been explicitly disabled and put as part of
> remove() so the runtime suspend callback must not be run when balancing
> the runtime PM usage count before returning.
>
> Fixes: 16adc674d0d6 ("usb: dwc3: add generic OF glue layer")
> Signed-off-by: Johan Hovold
> ---
>
> Changes in v2
Forgot to add a v2 prefix to the subject, sorry.
> - balance usage count only after disabling runtime PM to avoid racing
>with pm_runtime_suspend() as suggested by Alan
And this really should be a s/pm_runtime_suspend()/a runtime suspend
request/.
I blame the north European heat wave. ;)
Johan
--
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: dwc3: of-simple: fix use-after-free on remove
The clocks have already been explicitly disabled and put as part of
remove() so the runtime suspend callback must not be run when balancing
the runtime PM usage count before returning.
Fixes: 16adc674d0d6 ("usb: dwc3: add generic OF glue layer")
Signed-off-by: Johan Hovold
---
Changes in v2
- balance usage count only after disabling runtime PM to avoid racing
with pm_runtime_suspend() as suggested by Alan
drivers/usb/dwc3/dwc3-of-simple.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/dwc3/dwc3-of-simple.c
b/drivers/usb/dwc3/dwc3-of-simple.c
index cb2ee96fd3e8..048922d549dd 100644
--- a/drivers/usb/dwc3/dwc3-of-simple.c
+++ b/drivers/usb/dwc3/dwc3-of-simple.c
@@ -165,8 +165,9 @@ static int dwc3_of_simple_remove(struct platform_device
*pdev)
reset_control_put(simple->resets);
- pm_runtime_put_sync(dev);
pm_runtime_disable(dev);
+ pm_runtime_put_noidle(dev);
+ pm_runtime_set_suspended(dev);
return 0;
}
--
2.17.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
Re: [PATCH] ftdi_sio: add Id for Physik Instrumente E-870
On Thu, May 31, 2018 at 12:39:41PM +0200, Greg Kroah-Hartman wrote:
> On Thu, Mar 29, 2018 at 08:39:37AM +0200, Teichmann, Martin wrote:
> > This adds support for the Physik Instrumente E-870 PIShift Drive
> > Electronics, a Piezo motor driver.
> >
> > Signed-off-by: Martin Teichmann
> > ---
> > drivers/usb/serial/ftdi_sio.c | 1 +
> > drivers/usb/serial/ftdi_sio_ids.h | 1 +
> > 2 files changed, 2 insertions(+)
>
> Johan, want me to pick this one up? I think you missed it in your last
> pull request.
No, this one was actually already applied and later reverted when it
turned out it was not really an ftdi device at all:
5267c5e09c25 ("Revert "USB: serial: ftdi_sio: add Id for Physik
Instrumente E-870"")
Thanks,
Johan
--
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
[GIT PULL] USB-serial updates for v4.18-rc1
The following changes since commit 6da6c0db5316275015e8cc2959f12a17584aeb64: Linux v4.17-rc3 (2018-04-29 14:17:42 -0700) are available in the Git repository at: https://git.kernel.org/pub/scm/linux/kernel/git/johan/usb-serial.git tags/usb-serial-4.18-rc1 for you to fetch changes up to 7041d9c3f01b365daa50340a5d2dce84a09ea813: USB: serial: pl2303: add support for tx xon/xoff flow control (2018-05-22 10:08:05 +0200) USB-serial updates for v4.18-rc1 Here are the USB-serial updates for 4.18-rc1, including: - support for hardware-assisted XON/XOFF output flow control for pl2303 - fix for a long-standing IXON/IXOFF mixup in ftdi_sio - blacklist of two apparently unused dwm-158 modem interfaces that confused some user space daemon (option) - add missing const to a tty helper currently used by USB serial only Included are also various clean ups. All have been in linux-next with no reported issues. Signed-off-by: Johan Hovold Florian Zumbiehl (1): USB: serial: pl2303: add support for tx xon/xoff flow control Giuseppe Lippolis (1): USB: serial: option: blacklist unused dwm-158 interfaces Johan Hovold (6): USB: serial: use tty_port_register_device() USB: serial: ftdi_sio: fix IXON/IXOFF mixup USB: serial: ftdi_sio: use non-underscore fixed types USB: serial: ftdi_sio: drop unnecessary urb_ variable prefixes USB: serial: ftdi_sio: clean up flow control management tty: add missing const to termios hw-change helper drivers/tty/tty_ioctl.c | 2 +- drivers/usb/serial/bus.c| 3 +- drivers/usb/serial/ftdi_sio.c | 194 drivers/usb/serial/option.c | 3 +- drivers/usb/serial/pl2303.c | 16 +++- drivers/usb/serial/usb-serial.c | 2 +- include/linux/tty.h | 2 +- 7 files changed, 99 insertions(+), 123 deletions(-) -- 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 2/2] usb: dwc3: of_simple: don't call pm_runtime_set_active()
On Mon, May 28, 2018 at 05:36:14PM +0300, Roger Quadros wrote:
> Don't call pm_runtime_set_active() as it will prevent the device
> from being activated in the next pm_runtime_get_sync() call.
>
> Also call pm_runtime_get_sync() before of_platform_populate().
This paragraph describes what you do, but not why do it.
> Signed-off-by: Roger Quadros <rog...@ti.com>
> ---
> drivers/usb/dwc3/dwc3-of-simple.c | 7 +++
> 1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/usb/dwc3/dwc3-of-simple.c
> b/drivers/usb/dwc3/dwc3-of-simple.c
> index e98d221..2cbb5c0 100644
> --- a/drivers/usb/dwc3/dwc3-of-simple.c
> +++ b/drivers/usb/dwc3/dwc3-of-simple.c
> @@ -121,6 +121,9 @@ static int dwc3_of_simple_probe(struct platform_device
> *pdev)
> if (ret)
> goto err_resetc_assert;
>
> + pm_runtime_enable(dev);
> + pm_runtime_get_sync(dev);
This breaks runtime pm as you now get a second round of clock enables
which are never balanced on runtime suspend (the clocks are first
enabled in dwc3_of_simple_clk_init() above and with your change again in
dwc3_of_simple_runtime_resume()).
On the other hand, we currently return from probe() with a positive RPM
count so perhaps the RPM callbacks can just be removed altogether (i.e.
unless some other entity drops that count at some point before
remove()).
> ret = of_platform_populate(np, NULL, NULL, dev);
> if (ret) {
> for (i = 0; i < simple->num_clocks; i++) {
> @@ -131,10 +134,6 @@ static int dwc3_of_simple_probe(struct platform_device
> *pdev)
> goto err_resetc_assert;
> }
>
> - pm_runtime_set_active(dev);
> - pm_runtime_enable(dev);
> - pm_runtime_get_sync(dev);
> -
> return 0;
>
> err_resetc_assert:
Also note that there's currently a use-after-free in remove(), where
pm_runtime_put_sync() is called after the clocks have been put.
Something like the below (untested) patch should fix it.
Johan
>From 35c384c31010c344d403c26fc0a1dde0fd68ef4a Mon Sep 17 00:00:00 2001
From: Johan Hovold <jo...@kernel.org>
Date: Mon, 28 May 2018 17:31:45 +0200
Subject: [PATCH] usb: dwc3: of-simple: fix use-after-free on remove
The clocks have already been explicitly disabled and put as part of
remove() so the runtime suspend callback must not be run when balancing
the runtime PM usage count before returning.
Fixes: 16adc674d0d6 ("usb: dwc3: add generic OF glue layer")
Signed-off-by: Johan Hovold <jo...@kernel.org>
---
drivers/usb/dwc3/dwc3-of-simple.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/dwc3/dwc3-of-simple.c
b/drivers/usb/dwc3/dwc3-of-simple.c
index cb2ee96fd3e8..b9c869cd6585 100644
--- a/drivers/usb/dwc3/dwc3-of-simple.c
+++ b/drivers/usb/dwc3/dwc3-of-simple.c
@@ -165,8 +165,9 @@ static int dwc3_of_simple_remove(struct platform_device
*pdev)
reset_control_put(simple->resets);
- pm_runtime_put_sync(dev);
+ pm_runtime_put_noidle(dev);
pm_runtime_disable(dev);
+ pm_runtime_set_suspended(dev);
return 0;
}
--
2.17.0
--
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: Serdev: USB device and sysdev probing ala i2c
On Fri, May 25, 2018 at 10:17:20AM -0500, Rob Herring wrote: > On Fri, May 25, 2018 at 7:08 AM, Johan Hovold <jo...@kernel.org> wrote: > > On Thu, May 24, 2018 at 11:49:24AM -0500, Rob Herring wrote: > >> On Thu, May 24, 2018 at 7:18 AM, Ricardo Ribalda Delgado > >> <ricardo.riba...@gmail.com> wrote: > >> > Hi Johan, > >> > > >> > On Thu, May 24, 2018 at 2:07 PM Johan Hovold <jo...@kernel.org> wrote: > > > >> >> Serdev currently only supports device tree and ACPI. Using out-of-tree > >> >> code, you could load a device tree fragment during runtime to describe > >> >> your serial bus (or you just amend the device tree). > >> > > >> >> Using device tree overlays would have the benefit of being able to > >> >> describe associated resources (e.g. reset gpios) which a simple > >> >> compatible string (or equivalent) would not. > >> > > >> >> But there are examples where a simple compatible string would do, for > >> >> example an existing CEC device presenting itself as a generic USB CDC > >> >> device (hopefully with a dedicated VID/PID so that no user-space > >> >> configuration is needed at all). > > > >> The fundamental problem here is you need a parent device node to apply > >> a DT overlay to and a USB device hotplugged has no DT device node. The > >> system you are running on may not even have a DT (like a PC). If you > >> have an overlay of the downstream devices, they have to be a child of > >> something for the overlay to apply to. We could just create virtual > >> device nodes for the purposes of applying overlays to. Another option > >> would be allowing multiple DTs. Then you aren't even using overlays > >> (what's the point of an overlay when a system has no real DT to begin > >> with). That also would mean they are completely independent from any > >> real DT or other instances (you may want to plug in multiple of the > >> same device). > > > > Right, there's more (than just a DT overlay loader) that needs to be in > > place before this could be used for the generic hotplug case (and even > > more than that if this was to be usable for ACPI systems). > > > > I think generating DT nodes during enumeration is preferable to having > > detached trees, if only to deal with the case where a loaded overlay do > > overlap with the "real" static tree. > > Generating nodes effectively means we're implementing the full USB > tree as defined for OpenFirmware[1]. I'd prefer to not go there. That > seems a bit pointless as for most of the devices we don't care and > there's really nothing related to USB we care about. We just need to > describe downstream functions. The USB device (or interface) just > happens to be the controller/provider for those functions. > > There should only be overlap with a real tree if devices are soldered > onto a board or use a custom connector and you have other sideband > signals. Right, we may not want to do it, but having too many mechanisms for doing the same thing can get messy (but so are overlays to begin with, I guess). What we have today (and with the USB serial device tree patches) gets us a long way by covering static setups. And before supporting the generic hotplug case with sideband signals (where the descriptive power of device-tree overlays would be handy) it may be better to focus on the subset where just a compatible string (or equivalent) would do. The USB CEC device I mentioned above would be a good example; and an always-on or DTR/RTS power-controlled GPS could be another. > > Another problem is that we need to deregister any tty device and > > essentially reprobe the underlying serial controller whenever user space > > tells us what can be found on the other end of wire in order to register > > a serdev controller and device instead. IIRC this is something we would > > get for free if using DT overlays (i.e. any device with a modified DT > > node would get removed and readded, or at least notified that something > > changed). > > I did some experiments in this area. Marcel wanted to make all BT > drivers serdev only and then make the BT ldisc just create serdev > devices. And all this would be transparent to existing BT userspace. > As part of this I modified registration to register both serdev ctrlr > and tty char device and allow adding slave devices later. It's up on > my serdev-ldisc-v2 branch in all its hacky glory. Yeah, I remember you mentioning something about that, but I never dared to look at the code. ;) > I don't think we
[RFC PATCH 2/3] USB: serial: enable serdev support
Enable serdev support by using the serdev opt-in tty-port registration
helpers.
FIXME: serdev core always allocates and registers a serdev controller
during port registration only to immediately roll back in the common
case when there is no serdev slave defined in firmware
FIXME: serdev does not support hotplugging (e.g. tty port hangups)
Not-signed-off-by: Johan Hovold <jo...@kernel.org>
---
drivers/usb/serial/bus.c | 7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/serial/bus.c b/drivers/usb/serial/bus.c
index eb0195cf37dd..5f574a418c52 100644
--- a/drivers/usb/serial/bus.c
+++ b/drivers/usb/serial/bus.c
@@ -60,8 +60,9 @@ static int usb_serial_device_probe(struct device *dev)
}
minor = port->minor;
- tty_dev = tty_port_register_device(>port, usb_serial_tty_driver,
- minor, dev);
+ tty_dev = tty_port_register_device_serdev(>port,
+ usb_serial_tty_driver,
+ minor, dev);
if (IS_ERR(tty_dev)) {
retval = PTR_ERR(tty_dev);
goto err_port_remove;
@@ -105,7 +106,7 @@ static int usb_serial_device_remove(struct device *dev)
autopm_err = usb_autopm_get_interface(port->serial->interface);
minor = port->minor;
- tty_unregister_device(usb_serial_tty_driver, minor);
+ tty_port_unregister_device(>port, usb_serial_tty_driver, minor);
driver = port->serial->type;
if (driver->port_remove)
--
2.17.0
--
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
[RFC PATCH 0/3] USB: serial: add device tree (and serdev) support
These are some patches I've been using to test out using serdev with USB
serial. Adding device-tree support to USB serial is really a
separate matter and could be merged before the remaining issues related
to hotplug are addressed for serdev.
Note that this has been a low-intensity on-going effort where most
prerequisites are already upstream including USB device tree support
(4.15), device-tree node sharing (4.13), musb device-node propagation
(linux-next) and various fixes along the way (driver core, usb core,
musb).
What left to be decided is how to deal with multi-port devices. This
series uses child nodes to represent each port, which may be a little
counter-intuitive for devices (or rather interfaces) with just a single
port:
_interface {
#address-cells = <1>;
#size-cells = <0>;
serial@0 {
reg = <0>;
};
};
but I still think this it how it needs to be implemented.
Another thing that's currently lacking is binding documentation.
For completeness (and per request), the second patch enables serdev
support and can be used for testing purposes. The third patch can be
used as a base for testing this on a BBB and describes two USB serial
devices attached to an external hub.
Note that this series depends on a couple of patches (for usb-serial and
musb) that are still in linux-next. For convenience, I've prepared a
branch here:
https://git.kernel.org/pub/scm/linux/kernel/git/johan/usb-serial.git/log/?h=usb-serial-of
Johan
Johan Hovold (3):
USB: serial: add device-tree support
USB: serial: enable serdev support
dbg: ARM: dts: boneblack: add USB topology and serdev nodes
arch/arm/boot/dts/am335x-boneblack.dts | 57 ++
drivers/usb/serial/bus.c | 7 ++--
drivers/usb/serial/usb-serial.c| 28 -
3 files changed, 88 insertions(+), 4 deletions(-)
--
2.17.0
--
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
[RFC PATCH 3/3] dbg: ARM: dts: boneblack: add USB topology and serdev nodes
Add a hub device and two USB devices, of which one has a combined node.
Note that we need to represent the serial ports as well -- consider
devices with multiple ports per interface; which one should serdev
use? Sibling devices can also be described this way (e.g. gpio@0), and
would need to use the same address size.
Also note that serial ports have a standardised node name in ePAPR.
Not-signed-off-by: Johan Hovold <jo...@kernel.org>
---
arch/arm/boot/dts/am335x-boneblack.dts | 57 ++
1 file changed, 57 insertions(+)
diff --git a/arch/arm/boot/dts/am335x-boneblack.dts
b/arch/arm/boot/dts/am335x-boneblack.dts
index d154d3133c16..d5f4c78efa53 100644
--- a/arch/arm/boot/dts/am335x-boneblack.dts
+++ b/arch/arm/boot/dts/am335x-boneblack.dts
@@ -26,3 +26,60 @@
opp-supported-hw = <0x06 0x0100>;
};
};
+
+ {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ dlink_hub: hub@1 {
+ compatible = "usb2101,8501";
+ reg = <1>;
+
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ ft232r: device@3 {
+ compatible = "usb403,6001";
+ reg = <3>;
+
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ serial@0 {
+ reg = <0>;
+
+ serdev {
+ compatible = "none,serdev-mockup";
+ };
+ };
+ };
+
+ mos7820: device@5 {
+ compatible = "usb9710,7840";
+ reg = <5>;
+
+ #address-cells = <2>;
+ #size-cells = <0>;
+
+ interface@0 {
+ compatible = "usbif9710,7840.config1.0";
+ reg = <0 1>;
+
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ serial@0 {
+ reg = <0>;
+
+ gnss {
+ compatible = "u-blox,neo-8";
+ };
+ };
+
+ serial@1 {
+ reg = <1>;
+ };
+ };
+ };
+ };
+};
--
2.17.0
--
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
[RFC PATCH 1/3] USB: serial: add device-tree support
Lookup and associate serial-port device-tree nodes given a parent
USB-interface node during probe.
Note that a serial-port node must be named "serial" and have a "reg"
property so that ports on multi-port interfaces can be distinguished.
_interface {
#address-cells = <1>;
#size-cells = <0>;
serial@0 {
reg = <0>;
};
};
FIXME: binding doc
Not-signed-off-by: Johan Hovold <jo...@kernel.org>
---
drivers/usb/serial/usb-serial.c | 28 +++-
1 file changed, 27 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/serial/usb-serial.c b/drivers/usb/serial/usb-serial.c
index 44ecf0e2be9d..5a7ebe1e9fd6 100644
--- a/drivers/usb/serial/usb-serial.c
+++ b/drivers/usb/serial/usb-serial.c
@@ -35,6 +35,7 @@
#include
#include
#include
+#include
#define DRIVER_AUTHOR "Greg Kroah-Hartman <gre...@linuxfoundation.org>"
#define DRIVER_DESC "USB Serial Driver core"
@@ -97,7 +98,6 @@ static int allocate_minors(struct usb_serial *serial, int
num_ports)
if (minor < 0)
goto error;
port->minor = minor;
- port->port_number = i;
}
serial->minors_reserved = 1;
mutex_unlock(_lock);
@@ -589,6 +589,7 @@ static void usb_serial_port_release(struct device *dev)
kfifo_free(>write_fifo);
kfree(port->interrupt_in_buffer);
kfree(port->interrupt_out_buffer);
+ of_node_put(dev->of_node);
tty_port_destroy(>port);
kfree(port);
}
@@ -857,6 +858,29 @@ static int setup_port_interrupt_out(struct usb_serial_port
*port,
return 0;
}
+/* FIXME: move to separate compilation unit? */
+static struct device_node *find_port_node(struct usb_interface *intf, int port)
+{
+ struct device_node *node;
+ u32 reg;
+
+ for_each_child_of_node(intf->dev.of_node, node) {
+ if (!node->name || of_node_cmp(node->name, "serial") != 0)
+ continue;
+
+ if (of_property_read_u32(node, "reg", ))
+ continue;
+
+ if (reg == port)
+ break;
+ }
+
+ dev_dbg(>dev, "node %pOF, port %d: %pOFP\n", intf->dev.of_node,
+ port, node);
+
+ return node;
+}
+
static int usb_serial_probe(struct usb_interface *interface,
const struct usb_device_id *id)
{
@@ -963,6 +987,7 @@ static int usb_serial_probe(struct usb_interface *interface,
retval = -ENOMEM;
goto err_free_epds;
}
+ port->port_number = i;
tty_port_init(>port);
port->port.ops = _port_ops;
port->serial = serial;
@@ -976,6 +1001,7 @@ static int usb_serial_probe(struct usb_interface
*interface,
port->dev.bus = _serial_bus_type;
port->dev.release = _serial_port_release;
port->dev.groups = usb_serial_port_groups;
+ port->dev.of_node = find_port_node(interface,
port->port_number);
device_initialize(>dev);
}
--
2.17.0
--
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
