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

[Jason Andryuk]

On Mon, Oct 26, 2020 at 9:40 AM Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
>
> 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:
> >>
> >> 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?
> >
> > Hi,
> >
> > 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.

Okay, sorry for the confusion there.

I thought your "it's the SIO chipsets that return 1 byte which are older
than the chipset being emulated by QEMU" meant you thought your change
to 1 byte was unnecessary.  You also posted two bytes (0x1 0x60) from
your hardware.

> It is the latter which this patch changes and appears to match what I see on 
> my
> Chipi-X hardware here.

I don't have my hardware readily available, but I must have been
seeing 2 bytes from FTDI_GET_MDM_ST with data[1] = FTDI_THRE |
FTDI_TEMT; to make the change.

I don't have the USB captures anymore to compare the lowest bit value.

So right now you are just interested in dropping the lowest bit
setting but preserving the 2 bytes modem status?

Regards,
Jason