On Mon, Oct 26, 2020 at 9:40 AM Mark Cave-Ayland <mark.cave-ayl...@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.thiba...@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-ayl...@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 <aurel...@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
