| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAFBinCC9ftXxkyoiY=3ia6UubTeG-cHXa40ddd7WMNUhvVjr+g@mail.gmail.com>
Date: Mon, 22 Jul 2024 23:22:32 +0200
From: Martin Blumenstingl <martin.blumenstingl@...glemail.com>
To: Johan Hovold <johan@...nel.org>
Cc: Corentin Labbe <clabbe@...libre.com>, gregkh@...uxfoundation.org,
linux-kernel@...r.kernel.org, linux-usb@...r.kernel.org, david@...t.cz
Subject: Re: [PATCH 1/1 v7] usb: serial: add support for CH348
Hello Johan,
thanks for taking the time to go through this patch and providing feedback!
On Mon, Jul 22, 2024 at 4:21 PM Johan Hovold <johan@...nel.org> wrote:
>
> On Tue, May 07, 2024 at 01:15:22PM +0000, Corentin Labbe wrote:
> > The CH348 is an USB octo port serial adapter.
> > The device multiplexes all 8 ports in the same pair of Bulk endpoints.
> > Since there is no public datasheet, unfortunately it remains some magic values
>
> Could you please include a pointer to the vendor driver (which I assumed
> you based this on)?
We can do that, in case you want/need some info upfront you can find
the reference code here: [0]
[...]
> > @@ -0,0 +1,725 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * USB serial driver for USB to Octal UARTs chip ch348.
> > + *
> > + * Copyright (C) 2022 Corentin Labbe <clabbe@...libre.com>
>
> Do you need to include any copyrights from the vendor driver? It looks
> like at least some of the code below was copied from somewhere (but
> perhaps not that much).
If you - or someone else - have any advice on this I'd be happy to go with that!
[...]
> > +#define R_II_B1 0x06
> > +#define R_II_B2 0x02
> > +#define R_II_B3 0x00
>
> This look like standard IIR masks (e.g. R_II_B2 would be Transmit
> holding register empty).
I didn't know about it until now - thanks for the hint.
note to self: git grep UART_IIR_ include/
[...]
> > + struct urb *status_urb;
> > + u8 status_buffer[];
>
> This buffer should be allocated separately as it is used for DMA.
I assume it's because of alignment reasons.
Lucky me that I didn't hit any issues on ARM or x86_64.
[...}
> > + u8 reg_iir;
> > + union {
> > + u8 lsr_signal;
> > + u8 modem_signal;
> > + u8 init_data[10];
>
> This init_data field is never used and looks a bit odd. What is it used
> for?
I think the firmware is sending back the configuration that was
applied (see ch348_set_termios())
> You mentioned that that the modem lines are not yet used either.
My understanding is: we neither use init_data nor modem_signal in the
driver so let's drop them for now (and add them back when they're
actually needed).
[...]
> > + if (status_entry->lsr_signal & CH348_LO)
> > + port->icount.overrun++;
> > + if (status_entry->lsr_signal & CH348_LP)
> > + port->icount.parity++;
> > + if (status_entry->lsr_signal & CH348_LF)
> > + port->icount.frame++;
> > + if (status_entry->lsr_signal & CH348_LF)
>
> Should you really count every framing error as a break? Looks like this
> should have been CH348_LB.
Excellent catch - thank you!
Also note to self: git grep UART_LSR_ include
We can probably replace the custom #defines with the standard ones.
[...]
> > +static int ch348_configure(struct ch348 *ch348, int portnum)
> > +{
> > + int ret;
> > +
> > + ret = ch348_do_magic(ch348, portnum, CMD_W_R, R_C2, 0x87);
>
> This looks like it could be a write to the standard Fifo Control
> Register.
And again, thank you! 0x87 translates to (which also makes sense):
UART_FCR_ENABLE_FIFO (0x01) |
UART_FCR_CLEAR_RCVR (0x02) |
UART_FCR_CLEAR_XMIT (0x04) |
UART_FCR_R_TRIG_10 (0x80) |
UART_FCR_T_TRIG_00 (0x00)
[...]
> > +static int ch348_write(struct tty_struct *tty, struct usb_serial_port *port,
> > + const unsigned char *buf, int count)
> > +{
> > + struct ch348 *ch348 = usb_get_serial_data(port->serial);
> > + struct ch348_port *ch348_port = &ch348->ports[port->port_number];
> > + int ret, max_tx_size;
> > +
> > + if (tty_get_baud_rate(tty) < 9600 && count >= 128)
>
> You don't hold the termios lock here so this needs to be handled
> differently. Perhaps you can set a flag in set_termios if this is
> really needed.
>
> > + /*
> > + * Writing larger buffers can take longer than the hardware
> > + * allows before discarding the write buffer. Limit the
> > + * transfer size in such cases.
> > + * These values have been found by empirical testing.
> > + */
> > + max_tx_size = 128;
>
> Can you elaborate on you findings here? According to the vendor homepage
> this device has a 1024 byte TX fifo per port. Are you really saying that
> for some reason you can only fill it with 128 b when the line speed is
> below 9600?
For slow speeds I never receive the "Transmitter holding register
empty" interrupt/signal when using the full TX buffer.
It's not that the interrupt/signal is late - it just never arrives.
I don't know why that is (whether the firmware tries to keep things
"fair" for other ports, ...) though.
> > + else
> > + /*
> > + * Only ingest as many bytes as we can transfer with one URB at
> > + * a time. Once an URB has been written we need to wait for the
> > + * R_II_B2 status event before we are allowed to send more data.
>
> As I mentioned above R_II_B2 appears to be the transfer holding register
> empty flag in IIR. So you write one endpoint size worth of data and then
> wait for all of it to be processed on the device before sending more.
>
> How big is the endpoint (please post lsusb -v)?
Bus 003 Device 022: ID 1a86:55d9 QinHeng Electronics USB2.0 To Multi
Serial Ports
Device Descriptor:
bLength 18
bDescriptorType 1
bcdUSB 2.00
bDeviceClass 255 Vendor Specific Class
bDeviceSubClass 128 [unknown]
bDeviceProtocol 55
bMaxPacketSize0 64
idVendor 0x1a86 QinHeng Electronics
idProduct 0x55d9 USB2.0 To Multi Serial Ports
bcdDevice 1.36
iManufacturer 1 wch.cn
iProduct 2 USB2.0 To Multi Serial Ports
iSerial 0
bNumConfigurations 1
Configuration Descriptor:
bLength 9
bDescriptorType 2
wTotalLength 0x002e
bNumInterfaces 1
bConfigurationValue 1
iConfiguration 0
bmAttributes 0x80
(Bus Powered)
MaxPower 200mA
Interface Descriptor:
bLength 9
bDescriptorType 4
bInterfaceNumber 0
bAlternateSetting 0
bNumEndpoints 4
bInterfaceClass 255 Vendor Specific Class
bInterfaceSubClass 128 [unknown]
bInterfaceProtocol 55
iInterface 0
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x82 EP 2 IN
bmAttributes 2
Transfer Type Bulk
Synch Type None
Usage Type Data
wMaxPacketSize 0x0200 1x 512 bytes
bInterval 0
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x02 EP 2 OUT
bmAttributes 2
Transfer Type Bulk
Synch Type None
Usage Type Data
wMaxPacketSize 0x0200 1x 512 bytes
bInterval 0
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x81 EP 1 IN
bmAttributes 2
Transfer Type Bulk
Synch Type None
Usage Type Data
wMaxPacketSize 0x0200 1x 512 bytes
bInterval 0
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x01 EP 1 OUT
bmAttributes 2
Transfer Type Bulk
Synch Type None
Usage Type Data
wMaxPacketSize 0x0200 1x 512 bytes
bInterval 0
Device Qualifier (for other device speed):
bLength 10
bDescriptorType 6
bcdUSB 2.00
bDeviceClass 255 Vendor Specific Class
bDeviceSubClass 0 [unknown]
bDeviceProtocol 255
bMaxPacketSize0 64
bNumConfigurations 1
can't get debug descriptor: Resource temporarily unavailable
Device Status: 0x0000
(Bus Powered)
> > + * If we ingest more data then usb_serial_generic_write() will
> > + * internally try to process as much data as possible with any
> > + * number of URBs without giving us the chance to wait in
> > + * between transfers.
>
> If the hardware really works this way, then perhaps you should not use
> the generic write implementation. Just maintain a single urb per port
> and don't submit it until the device fifo is empty.
I tried to avoid having to copy & paste (which then also means having
to maintain it down the line) most of the generic write
implementation.
This whole dance with waiting for the "Transmitter holding register
empty" by the way was the reason why parts of the transmit buffer got
lost, see the report from Nicolas in v6 [1]
> > + */
> > + max_tx_size = port->bulk_out_size - CH348_TX_HDRSIZE;
> > +
> > + reinit_completion(&ch348_port->write_completion);
>
> This is broken as write can be called at any time and may clear any
> previous completion.
I never hit that issue in my tests but I get your point.
[...]
> > +static struct usb_serial_driver ch348_device = {
> > + .driver = {
> > + .owner = THIS_MODULE,
> > + .name = "ch348",
> > + },
> > + .id_table = ch348_ids,
> > + .num_ports = CH348_MAXPORT,
> > + .num_bulk_in = 1,
> > + .num_bulk_out = 1,
>
> Set both of these to 2 so that core verifies that you have all four
> endpoints.
I will have to test this because I thought that:
- using 2 here makes usb-serial allocate an URB as well and by default
assign it to the first and second port
- usb-serial should not touch the second bulk in / bulk out endpoint
(as they're entirely vendor / chip specific)
I omitted replies to some of your comments because I agree with them
and repeatedly writing "ACK" didn't seem to add much value.
Best regards,
Martin
[0] https://github.com/WCHSoftGroup/ch9344ser_linux/tree/dcbe60d12ead4622b7fdd6dec24e62facee23663/driver
[1] https://lore.kernel.org/lkml/2595072.9XhBIDAVAK@archbook/
Powered by blists - more mailing lists