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

2020-10-27 Thread Mark Cave-Ayland

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


Mark Cave-Ayland, le lun. 26 oct. 2020 13:40:05 +, a ecrit:

On 26/10/2020 13:00, Jason Andryuk wrote:

On Mon, Oct 26, 2020 at 7:21 AM Samuel Thibault  wrote:

Aurelien, you introduced the "| 1" in

commit abb8a13918ecc1e8160aa78582de9d5224ea70df
Author: Aurelien Jarno 
Date:   Wed Aug 13 04:23:17 2008 +

  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:
95a4bf2dd300 2366831506 S Ci:4:004:0 s c0 05   0002 2 <
95a4bf2dd300 2366831607 C Ci:4:004:0 0 2 = 0160

usbmon when adapter plugged in and remote connected to minicom:
95a4452a79c0 2457273895 S Ci:4:004:0 s c0 05   0002 2 <
95a4452a79c0 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.



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

2020-10-27 Thread Jason Andryuk
On Tue, Oct 27, 2020 at 9:18 AM Mark Cave-Ayland
 wrote:
>
> 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:
> 95a4bf2dd300 2366831506 S Ci:4:004:0 s c0 05   0002 2 <
> 95a4bf2dd300 2366831607 C Ci:4:004:0 0 2 = 0160
>
> usbmon when adapter plugged in and remote connected to minicom:
> 95a4452a79c0 2457273895 S Ci:4:004:0 s c0 05   0002 2 <
> 95a4452a79c0 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.

No worries.  Thanks for investigating.

(I had the thought that maybe reserve bit 0 differentiates one and two
byte responses? Bit 0 clear indicates a 1-byte response and set
indicates a 2 bit response.  But I'm just guessing.)

Regards,
Jason



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

2020-10-26 Thread Samuel Thibault
Mark Cave-Ayland, le lun. 26 oct. 2020 13:40:05 +, a ecrit:
> On 26/10/2020 13:00, Jason Andryuk wrote:
> > On Mon, Oct 26, 2020 at 7:21 AM Samuel Thibault  
> > wrote:
> > > Aurelien, you introduced the "| 1" in
> > > 
> > > commit abb8a13918ecc1e8160aa78582de9d5224ea70df
> > > Author: Aurelien Jarno 
> > > Date:   Wed Aug 13 04:23:17 2008 +
> > > 
> > >  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.

Samuel



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

2020-10-26 Thread Jason Andryuk
On Mon, Oct 26, 2020 at 9:40 AM Mark Cave-Ayland
 wrote:
>
> On 26/10/2020 13:00, Jason Andryuk wrote:
>
> > On Mon, Oct 26, 2020 at 7:21 AM Samuel Thibault  
> > wrote:
> >>
> >> Mark Cave-Ayland, le lun. 26 oct. 2020 10:58:43 +, a ecrit:
> >>> On 26/10/2020 09:54, Samuel Thibault wrote:
>  Mark Cave-Ayland, le lun. 26 oct. 2020 08:34:00 +, 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 
> > ---
> >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 
> >> Date:   Wed Aug 13 04:23:17 2008 +
> >>
> >>  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



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

2020-10-26 Thread Mark Cave-Ayland

On 26/10/2020 13:00, Jason Andryuk wrote:


On Mon, Oct 26, 2020 at 7:21 AM Samuel Thibault  wrote:


Mark Cave-Ayland, le lun. 26 oct. 2020 10:58:43 +, a ecrit:

On 26/10/2020 09:54, Samuel Thibault wrote:

Mark Cave-Ayland, le lun. 26 oct. 2020 08:34:00 +, 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 
---
   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 
Date:   Wed Aug 13 04:23:17 2008 +

 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.


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



ATB,

Mark.



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

2020-10-26 Thread Jason Andryuk
On Mon, Oct 26, 2020 at 7:21 AM Samuel Thibault  wrote:
>
> Mark Cave-Ayland, le lun. 26 oct. 2020 10:58:43 +, a ecrit:
> > On 26/10/2020 09:54, Samuel Thibault wrote:
> > > Mark Cave-Ayland, le lun. 26 oct. 2020 08:34:00 +, 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 
> > > > ---
> > > >   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 
> Date:   Wed Aug 13 04:23:17 2008 +
>
> 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?

Regards,
Jason



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

2020-10-26 Thread Samuel Thibault
Mark Cave-Ayland, le lun. 26 oct. 2020 10:58:43 +, a ecrit:
> On 26/10/2020 09:54, Samuel Thibault wrote:
> > Mark Cave-Ayland, le lun. 26 oct. 2020 08:34:00 +, 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 
> > > ---
> > >   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 
Date:   Wed Aug 13 04:23:17 2008 +

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



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

2020-10-26 Thread Mark Cave-Ayland

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 +, 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 
---
  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.



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

2020-10-26 Thread Samuel Thibault
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 +, 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 
> ---
>  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



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

2020-10-26 Thread Mark Cave-Ayland
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 
---
 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;
 break;
 case VendorDeviceOutRequest | FTDI_SET_EVENT_CHR:
 /* TODO: handle it */
-- 
2.20.1