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

From: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
To: Samuel Thibault <samuel.thibault@gnu.org>
Cc: Jason Andryuk <jandryuk@gmail.com>,
	Gerd Hoffmann <kraxel@redhat.com>,
	Aurelien Jarno <aurelien@aurel32.net>,
	QEMU <qemu-devel@nongnu.org>
Subject: Re: [PATCH 8/9] dev-serial: fix FTDI_GET_MDM_ST response
Date: Tue, 27 Oct 2020 13:18:44 +0000	[thread overview]
Message-ID: <a141714c-158f-0099-34ab-92fc27be6975@ilande.co.uk> (raw)
In-Reply-To: <20201026150432.yqkyceke5uppqom5@function>

On 26/10/2020 15:04, Samuel Thibault wrote:

> Mark Cave-Ayland, le lun. 26 oct. 2020 13:40:05 +0000, a ecrit:
>> On 26/10/2020 13:00, Jason Andryuk wrote:
>>> On Mon, Oct 26, 2020 at 7:21 AM Samuel Thibault <samuel.thibault@gnu.org> wrote:
>>>> 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;
>>>
>>> I'm not particularly familiar with the FTDI USB serial devices.  I
>>> found setting FTDI_THRE | FTDI_TEMT by comparing with real hardware.
>>>
>>> A little searching found this:
>>> https://elixir.bootlin.com/linux/latest/source/drivers/usb/serial/ftdi_sio.h#L541
>>>
>>> That shows "B0   Reserved - must be 1", so maybe that is why "| 1" was added?
>>
>> Right - that's for the modem status returned as part of the first 2 status
>> bytes for incoming data which is slightly different from modem status
>> returned directly from FTDI_SIO_GET_MODEM_STATUS: https://elixir.bootlin.com/linux/latest/source/drivers/usb/serial/ftdi_sio.h#L423.
>>
>> It is the latter which this patch changes and appears to match what I see on
>> my Chipi-X hardware here.
> 
> Aurelien, do you remember the reason for the addition of 1 here? It does
> look like the confusion between the incoming data bytes and the modem
> status bytes.

I spent a bit of time this morning doing some further tests on Linux using 2 machines 
and a test program to check CTS and usbmon:

usbmon when adapter unplugged:
ffff95a4bf2dd300 2366831506 S Ci:4:004:0 s c0 05 0000 0000 0002 2 <
ffff95a4bf2dd300 2366831607 C Ci:4:004:0 0 2 = 0160

usbmon when adapter plugged in and remote connected to minicom:
ffff95a4452a79c0 2457273895 S Ci:4:004:0 s c0 05 0000 0000 0002 2 <
ffff95a4452a79c0 2457274057 C Ci:4:004:0 0 2 = 3160

It seems I made a mistake here since the response is interpreted as 2 bytes rather 
than a single little-endian word: with CTS asserted on the remote the first byte is 
0x31 == FTDI_CTS | FTDI_DSR | 1, whilst the 2nd byte is 0x60 == FTDI_THRE | FTDI_TEMT 
which matches the existing QEMU code rather than the comments in ftdi_sio.h.

So sorry for the noise: I'll drop this patch from the series and post a v2 shortly.

ATB,

Mark.