| Message ID | 20201026083401.13231-9-mark.cave-ayland@ilande.co.uk |
|---|---|
| State | New |
| Headers | show |
| Series | dev-serial: minor fixes and improvements | expand |
Hello, (Cc-ing Aurelien who introduced the support for modem control, and Jason who added the missing THRE and TEMT flags). Mark Cave-Ayland, le lun. 26 oct. 2020 08:34:00 +0000, a ecrit: > The FTDI_GET_MDM_ST response should only return a single byte indicating the > modem status with bit 0 cleared (as documented in the Linux ftdi_sio.h header > file). > > Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> > --- > hw/usb/dev-serial.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > diff --git a/hw/usb/dev-serial.c b/hw/usb/dev-serial.c > index 4c374d0790..fa734bcf54 100644 > --- a/hw/usb/dev-serial.c > +++ b/hw/usb/dev-serial.c > @@ -360,9 +360,8 @@ static void usb_serial_handle_control(USBDevice *dev, USBPacket *p, > /* TODO: TX ON/OFF */ > break; > case VendorDeviceRequest | FTDI_GET_MDM_ST: > - data[0] = usb_get_modem_lines(s) | 1; > - data[1] = FTDI_THRE | FTDI_TEMT; > - p->actual_length = 2; > + data[0] = usb_get_modem_lines(s); > + p->actual_length = 1; Err, but Linux' drivers/usb/serial/ftdi_sio.c:ftdi_process_packet() contradicts this: if (len < 2) { dev_dbg(&port->dev, "malformed packet\n"); return 0; } status = buf[0] & FTDI_STATUS_B0_MASK; if (status != priv->prev_status) { char diff_status = status ^ priv->prev_status; if (diff_status & FTDI_RS0_CTS) port->icount.cts++; [...] /* save if the transmitter is empty or not */ if (buf[1] & FTDI_RS_TEMT) priv->transmit_empty = 1; else priv->transmit_empty = 0; Did you actually get an issue with seeing this packet contain two bytes? Samuel
On 26/10/2020 09:54, Samuel Thibault wrote: > Hello, > > (Cc-ing Aurelien who introduced the support for modem control, and Jason > who added the missing THRE and TEMT flags). > > Mark Cave-Ayland, le lun. 26 oct. 2020 08:34:00 +0000, a ecrit: >> The FTDI_GET_MDM_ST response should only return a single byte indicating the >> modem status with bit 0 cleared (as documented in the Linux ftdi_sio.h header >> file). >> >> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> >> --- >> hw/usb/dev-serial.c | 5 ++--- >> 1 file changed, 2 insertions(+), 3 deletions(-) >> >> diff --git a/hw/usb/dev-serial.c b/hw/usb/dev-serial.c >> index 4c374d0790..fa734bcf54 100644 >> --- a/hw/usb/dev-serial.c >> +++ b/hw/usb/dev-serial.c >> @@ -360,9 +360,8 @@ static void usb_serial_handle_control(USBDevice *dev, USBPacket *p, >> /* TODO: TX ON/OFF */ >> break; >> case VendorDeviceRequest | FTDI_GET_MDM_ST: >> - data[0] = usb_get_modem_lines(s) | 1; >> - data[1] = FTDI_THRE | FTDI_TEMT; >> - p->actual_length = 2; >> + data[0] = usb_get_modem_lines(s); >> + p->actual_length = 1; > > Err, but Linux' drivers/usb/serial/ftdi_sio.c:ftdi_process_packet() > contradicts this: > > if (len < 2) { > dev_dbg(&port->dev, "malformed packet\n"); > return 0; > } > > status = buf[0] & FTDI_STATUS_B0_MASK; > if (status != priv->prev_status) { > char diff_status = status ^ priv->prev_status; > > if (diff_status & FTDI_RS0_CTS) > port->icount.cts++; > > [...] > > /* save if the transmitter is empty or not */ > if (buf[1] & FTDI_RS_TEMT) > priv->transmit_empty = 1; > else > priv->transmit_empty = 0; > > Did you actually get an issue with seeing this packet contain two bytes? Hi Samuel, Thanks for the review! There are 2 different places where the status is read: the one above in ftdi_process_packet() relates to the control packet header for incoming data which is always 2 bytes, whereas this relates to FTDI_SIO_GET_MODEM_STATUS_REQUEST in https://elixir.bootlin.com/linux/latest/source/drivers/usb/serial/ftdi_sio.c#L2825. I have a FTDI Chipi-X adapter to compare with and that returns 2 bytes, but it looks like I mis-read the code and it's the SIO chipsets that return 1 byte which are older than the chipset being emulated by QEMU. This came from reading https://elixir.bootlin.com/linux/latest/source/drivers/usb/serial/ftdi_sio.h#L415 which states that only 1 byte should be returned, but of course that comment could be out of date. A quick test shows my Chipi-X returns 0x1 0x60 with nothing attached in response to FTDI_SIO_GET_MODEM_STATUS_REQUEST: assuming the reply length should be 2 bytes, the comment about B0-B3 being zero and the response from my Chip-X above suggests that the "| 1" should still be dropped from the response. ATB, Mark.
Mark Cave-Ayland, le lun. 26 oct. 2020 10:58:43 +0000, a ecrit: > On 26/10/2020 09:54, Samuel Thibault wrote: > > Mark Cave-Ayland, le lun. 26 oct. 2020 08:34:00 +0000, a ecrit: > > > The FTDI_GET_MDM_ST response should only return a single byte indicating the > > > modem status with bit 0 cleared (as documented in the Linux ftdi_sio.h header > > > file). > > > > > > Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> > > > --- > > > hw/usb/dev-serial.c | 5 ++--- > > > 1 file changed, 2 insertions(+), 3 deletions(-) > > > > > > diff --git a/hw/usb/dev-serial.c b/hw/usb/dev-serial.c > > > index 4c374d0790..fa734bcf54 100644 > > > --- a/hw/usb/dev-serial.c > > > +++ b/hw/usb/dev-serial.c > > > @@ -360,9 +360,8 @@ static void usb_serial_handle_control(USBDevice *dev, USBPacket *p, > > > /* TODO: TX ON/OFF */ > > > break; > > > case VendorDeviceRequest | FTDI_GET_MDM_ST: > > > - data[0] = usb_get_modem_lines(s) | 1; > > > - data[1] = FTDI_THRE | FTDI_TEMT; > > > - p->actual_length = 2; > > > + data[0] = usb_get_modem_lines(s); > > > + p->actual_length = 1; > > [...] > A quick test shows my Chipi-X returns 0x1 0x60 with nothing attached in > response to FTDI_SIO_GET_MODEM_STATUS_REQUEST: assuming the reply length > should be 2 bytes, the comment about B0-B3 being zero and the response from > my Chip-X above suggests that the "| 1" should still be dropped from the > response. Aurelien, you introduced the "| 1" in commit abb8a13918ecc1e8160aa78582de9d5224ea70df Author: Aurelien Jarno <aurelien@aurel32.net> Date: Wed Aug 13 04:23:17 2008 +0000 usb-serial: add support for modem lines [...] @@ -357,9 +393,9 @@ static int usb_serial_handle_control(USBDevice *dev, int request, int value, /* TODO: TX ON/OFF */ break; case DeviceInVendor | FTDI_GET_MDM_ST: - /* TODO: return modem status */ - data[0] = 0; - ret = 1; + data[0] = usb_get_modem_lines(s) | 1; + data[1] = 0; + ret = 2; break; do you know exactly what it is for? Samuel
diff --git a/hw/usb/dev-serial.c b/hw/usb/dev-serial.c index 4c374d0790..fa734bcf54 100644 --- a/hw/usb/dev-serial.c +++ b/hw/usb/dev-serial.c @@ -360,9 +360,8 @@ static void usb_serial_handle_control(USBDevice *dev, USBPacket *p, /* TODO: TX ON/OFF */ break; case VendorDeviceRequest | FTDI_GET_MDM_ST: - data[0] = usb_get_modem_lines(s) | 1; - data[1] = FTDI_THRE | FTDI_TEMT; - p->actual_length = 2; + data[0] = usb_get_modem_lines(s); + p->actual_length = 1; break; case VendorDeviceOutRequest | FTDI_SET_EVENT_CHR: /* TODO: handle it */
The FTDI_GET_MDM_ST response should only return a single byte indicating the modem status with bit 0 cleared (as documented in the Linux ftdi_sio.h header file). Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> --- hw/usb/dev-serial.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)