Re: [PATCH 8/9] dev-serial: fix FTDI_GET_MDM_ST response

[Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>]

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.