[PATCH] cdc-acm: acm_init: Set initial BAUD to B0

[PATCH] cdc-acm: acm_init: Set initial BAUD to B0

[Joakim Tjernlund]

BO will disable USB input until the device opens. This will
avoid garbage chars waiting flood the TTY. This mimics a real UART
device better.
For initial termios to reach USB core, USB driver has to be
registered before TTY driver.

Signed-off-by: Joakim Tjernlund <joakim.tjernlund@infinera.com>
Cc: stable@vger.kernel.org
---

 I hope this change makes sense to you, if so I belive
 ttyUSB could do the same.

 drivers/usb/class/cdc-acm.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
index 751f00285ee6..5680f71200e5 100644
--- a/drivers/usb/class/cdc-acm.c
+++ b/drivers/usb/class/cdc-acm.c
@@ -1999,19 +1999,19 @@ static int __init acm_init(void)
 	acm_tty_driver->subtype = SERIAL_TYPE_NORMAL,
 	acm_tty_driver->flags = TTY_DRIVER_REAL_RAW | TTY_DRIVER_DYNAMIC_DEV;
 	acm_tty_driver->init_termios = tty_std_termios;
-	acm_tty_driver->init_termios.c_cflag = B9600 | CS8 | CREAD |
+	acm_tty_driver->init_termios.c_cflag = B0 | CS8 | CREAD |
 								HUPCL | CLOCAL;
 	tty_set_operations(acm_tty_driver, &acm_ops);
 
-	retval = tty_register_driver(acm_tty_driver);
+	retval = usb_register(&acm_driver);
 	if (retval) {
 		put_tty_driver(acm_tty_driver);
 		return retval;
 	}
 
-	retval = usb_register(&acm_driver);
+	retval = tty_register_driver(acm_tty_driver);
 	if (retval) {
-		tty_unregister_driver(acm_tty_driver);
+		usb_deregister(&acm_driver);
 		put_tty_driver(acm_tty_driver);
 		return retval;
 	}
-- 
2.26.2

Re: [PATCH] cdc-acm: acm_init: Set initial BAUD to B0

[Johan Hovold]

On Fri, Jul 10, 2020 at 11:35:18AM +0200, Joakim Tjernlund wrote:
> BO will disable USB input until the device opens. This will
> avoid garbage chars waiting flood the TTY. This mimics a real UART
> device better.
> For initial termios to reach USB core, USB driver has to be
> registered before TTY driver.
> 
> Signed-off-by: Joakim Tjernlund <joakim.tjernlund@infinera.com>
> Cc: stable@vger.kernel.org
> ---
> 
>  I hope this change makes sense to you, if so I belive
>  ttyUSB could do the same.

No, this doesn't make sense. B0 is used to hang up an already open tty.

Furthermore, this change only affects the initial terminal settings and
won't have any effect the next time you open the same port.

Why not fix your firmware so that it doesn't transmit before DTR is
asserted during open()?

>  drivers/usb/class/cdc-acm.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
> index 751f00285ee6..5680f71200e5 100644
> --- a/drivers/usb/class/cdc-acm.c
> +++ b/drivers/usb/class/cdc-acm.c
> @@ -1999,19 +1999,19 @@ static int __init acm_init(void)
>  	acm_tty_driver->subtype = SERIAL_TYPE_NORMAL,
>  	acm_tty_driver->flags = TTY_DRIVER_REAL_RAW | TTY_DRIVER_DYNAMIC_DEV;
>  	acm_tty_driver->init_termios = tty_std_termios;
> -	acm_tty_driver->init_termios.c_cflag = B9600 | CS8 | CREAD |
> +	acm_tty_driver->init_termios.c_cflag = B0 | CS8 | CREAD |
>  								HUPCL | CLOCAL;
>  	tty_set_operations(acm_tty_driver, &acm_ops);
>  
> -	retval = tty_register_driver(acm_tty_driver);
> +	retval = usb_register(&acm_driver);
>  	if (retval) {
>  		put_tty_driver(acm_tty_driver);
>  		return retval;
>  	}
>  
> -	retval = usb_register(&acm_driver);
> +	retval = tty_register_driver(acm_tty_driver);
>  	if (retval) {
> -		tty_unregister_driver(acm_tty_driver);
> +		usb_deregister(&acm_driver);
>  		put_tty_driver(acm_tty_driver);
>  		return retval;
>  	}

Johan

Re: [PATCH] cdc-acm: acm_init: Set initial BAUD to B0

[Joakim Tjernlund]

On Fri, 2020-07-10 at 11:54 +0200, Johan Hovold wrote:
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.
> 
> 
> On Fri, Jul 10, 2020 at 11:35:18AM +0200, Joakim Tjernlund wrote:
> > BO will disable USB input until the device opens. This will
> > avoid garbage chars waiting flood the TTY. This mimics a real UART
> > device better.
> > For initial termios to reach USB core, USB driver has to be
> > registered before TTY driver.
> > 
> > Signed-off-by: Joakim Tjernlund <joakim.tjernlund@infinera.com>
> > Cc: stable@vger.kernel.org
> > ---
> > 
> >  I hope this change makes sense to you, if so I belive
> >  ttyUSB could do the same.
> 
> No, this doesn't make sense. B0 is used to hang up an already open tty.

This is at module init so there is no tty yet.
acm_probe() will later set:
        acm->line.dwDTERate = cpu_to_le32(9600);
	acm->line.bDataBits = 8;
	acm_set_line(acm, &acm->line);

> 
> Furthermore, this change only affects the initial terminal settings and
> won't have any effect the next time you open the same port.

hmm, it is not ideal but acm_probe() will later set:
        acm->line.dwDTERate = cpu_to_le32(9600);
	acm->line.bDataBits = 8;
	acm_set_line(acm, &acm->line);

But, would it not make sense to not accept input until TTY is opened ?
That would be more inline with a real RS232, would it not?
> 
> Why not fix your firmware so that it doesn't transmit before DTR is
> asserted during open()?

I would but it is not my firmware, it is a ready made USB to UART chip. will talk
to the manufacturer though.


> >  drivers/usb/class/cdc-acm.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
> > index 751f00285ee6..5680f71200e5 100644
> > --- a/drivers/usb/class/cdc-acm.c
> > +++ b/drivers/usb/class/cdc-acm.c
> > @@ -1999,19 +1999,19 @@ static int __init acm_init(void)
> >       acm_tty_driver->subtype = SERIAL_TYPE_NORMAL,
> >       acm_tty_driver->flags = TTY_DRIVER_REAL_RAW | TTY_DRIVER_DYNAMIC_DEV;
> >       acm_tty_driver->init_termios = tty_std_termios;
> > -     acm_tty_driver->init_termios.c_cflag = B9600 | CS8 | CREAD |
> > +     acm_tty_driver->init_termios.c_cflag = B0 | CS8 | CREAD |
> >                                                               HUPCL | CLOCAL;
> >       tty_set_operations(acm_tty_driver, &acm_ops);
> > 
> > -     retval = tty_register_driver(acm_tty_driver);
> > +     retval = usb_register(&acm_driver);
> >       if (retval) {
> >               put_tty_driver(acm_tty_driver);
> >               return retval;
> >       }
> > 
> > -     retval = usb_register(&acm_driver);
> > +     retval = tty_register_driver(acm_tty_driver);
> >       if (retval) {
> > -             tty_unregister_driver(acm_tty_driver);
> > +             usb_deregister(&acm_driver);
> >               put_tty_driver(acm_tty_driver);
> >               return retval;
> >       }
> 
> Johan

Re: [PATCH] cdc-acm: acm_init: Set initial BAUD to B0

[Greg KH]

On Fri, Jul 10, 2020 at 11:35:18AM +0200, Joakim Tjernlund wrote:
> BO will disable USB input until the device opens. This will
> avoid garbage chars waiting flood the TTY. This mimics a real UART
> device better.
> For initial termios to reach USB core, USB driver has to be
> registered before TTY driver.

You are doing two different things here, please break this up into 2
patches, with good documentation for both of them.

And any reason you didn't send this to the people listed in
scripts/get_maintainers.pl when run on this patch?

> 
> Signed-off-by: Joakim Tjernlund <joakim.tjernlund@infinera.com>
> Cc: stable@vger.kernel.org
> ---
> 
>  I hope this change makes sense to you, if so I belive
>  ttyUSB could do the same.
> 
>  drivers/usb/class/cdc-acm.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
> index 751f00285ee6..5680f71200e5 100644
> --- a/drivers/usb/class/cdc-acm.c
> +++ b/drivers/usb/class/cdc-acm.c
> @@ -1999,19 +1999,19 @@ static int __init acm_init(void)
>  	acm_tty_driver->subtype = SERIAL_TYPE_NORMAL,
>  	acm_tty_driver->flags = TTY_DRIVER_REAL_RAW | TTY_DRIVER_DYNAMIC_DEV;
>  	acm_tty_driver->init_termios = tty_std_termios;
> -	acm_tty_driver->init_termios.c_cflag = B9600 | CS8 | CREAD |
> +	acm_tty_driver->init_termios.c_cflag = B0 | CS8 | CREAD |
>  								HUPCL | CLOCAL;

Huh?  Are you sure this works?

>  	tty_set_operations(acm_tty_driver, &acm_ops);
>  
> -	retval = tty_register_driver(acm_tty_driver);
> +	retval = usb_register(&acm_driver);
>  	if (retval) {
>  		put_tty_driver(acm_tty_driver);
>  		return retval;
>  	}
>  
> -	retval = usb_register(&acm_driver);
> +	retval = tty_register_driver(acm_tty_driver);
>  	if (retval) {
> -		tty_unregister_driver(acm_tty_driver);
> +		usb_deregister(&acm_driver);

Why are you switching these around?  I think I know, but you don't
really say...

thanks,

greg k-h

Re: [PATCH] cdc-acm: acm_init: Set initial BAUD to B0

[Greg KH]

On Fri, Jul 10, 2020 at 10:16:33AM +0000, Joakim Tjernlund wrote:
> On Fri, 2020-07-10 at 11:54 +0200, Johan Hovold wrote:
> > CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.
> > 
> > 
> > On Fri, Jul 10, 2020 at 11:35:18AM +0200, Joakim Tjernlund wrote:
> > > BO will disable USB input until the device opens. This will
> > > avoid garbage chars waiting flood the TTY. This mimics a real UART
> > > device better.
> > > For initial termios to reach USB core, USB driver has to be
> > > registered before TTY driver.
> > > 
> > > Signed-off-by: Joakim Tjernlund <joakim.tjernlund@infinera.com>
> > > Cc: stable@vger.kernel.org
> > > ---
> > > 
> > >  I hope this change makes sense to you, if so I belive
> > >  ttyUSB could do the same.
> > 
> > No, this doesn't make sense. B0 is used to hang up an already open tty.
> 
> This is at module init so there is no tty yet.
> acm_probe() will later set:
>         acm->line.dwDTERate = cpu_to_le32(9600);
> 	acm->line.bDataBits = 8;
> 	acm_set_line(acm, &acm->line);
> 
> > 
> > Furthermore, this change only affects the initial terminal settings and
> > won't have any effect the next time you open the same port.
> 
> hmm, it is not ideal but acm_probe() will later set:
>         acm->line.dwDTERate = cpu_to_le32(9600);
> 	acm->line.bDataBits = 8;
> 	acm_set_line(acm, &acm->line);
> 
> But, would it not make sense to not accept input until TTY is opened ?
> That would be more inline with a real RS232, would it not?

You can't keep the chip in the usb-to-serial device from accepting data
before you do anything with it, that requires firmware to not do this.
Are you sure your firmware is correct?

> > Why not fix your firmware so that it doesn't transmit before DTR is
> > asserted during open()?
> 
> I would but it is not my firmware, it is a ready made USB to UART chip. will talk
> to the manufacturer though.

What chip is this?

thanks,

greg k-h

Re: [PATCH] cdc-acm: acm_init: Set initial BAUD to B0

[Joakim Tjernlund]

On Fri, 2020-07-10 at 12:34 +0200, Greg KH wrote:

> On Fri, Jul 10, 2020 at 11:35:18AM +0200, Joakim Tjernlund wrote:
> > BO will disable USB input until the device opens. This will
> > avoid garbage chars waiting flood the TTY. This mimics a real UART
> > device better.
> > For initial termios to reach USB core, USB driver has to be
> > registered before TTY driver.
> 
> You are doing two different things here, please break this up into 2
> patches, with good documentation for both of them.

OK

> 
> And any reason you didn't send this to the people listed in
> scripts/get_maintainers.pl when run on this patch?

hmm, no. I just didn't check, sorry.

> 
> > 
> > Signed-off-by: Joakim Tjernlund <joakim.tjernlund@infinera.com>
> > Cc: stable@vger.kernel.org
> > ---
> > 
> >  I hope this change makes sense to you, if so I belive
> >  ttyUSB could do the same.
> > 
> >  drivers/usb/class/cdc-acm.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
> > index 751f00285ee6..5680f71200e5 100644
> > --- a/drivers/usb/class/cdc-acm.c
> > +++ b/drivers/usb/class/cdc-acm.c
> > @@ -1999,19 +1999,19 @@ static int __init acm_init(void)
> >       acm_tty_driver->subtype = SERIAL_TYPE_NORMAL,
> >       acm_tty_driver->flags = TTY_DRIVER_REAL_RAW | TTY_DRIVER_DYNAMIC_DEV;
> >       acm_tty_driver->init_termios = tty_std_termios;
> > -     acm_tty_driver->init_termios.c_cflag = B9600 | CS8 | CREAD |
> > +     acm_tty_driver->init_termios.c_cflag = B0 | CS8 | CREAD |
> >                                                               HUPCL | CLOCAL;
> 
> Huh?  Are you sure this works?
> 
> >       tty_set_operations(acm_tty_driver, &acm_ops);
> > 
> > -     retval = tty_register_driver(acm_tty_driver);
> > +     retval = usb_register(&acm_driver);
> >       if (retval) {
> >               put_tty_driver(acm_tty_driver);
> >               return retval;
> >       }
> > 
> > -     retval = usb_register(&acm_driver);
> > +     retval = tty_register_driver(acm_tty_driver);
> >       if (retval) {
> > -             tty_unregister_driver(acm_tty_driver);
> > +             usb_deregister(&acm_driver);
> 
> Why are you switching these around?  I think I know, but you don't
> really say...
> 
> thanks,
> 
> greg k-h

Re: [PATCH] cdc-acm: acm_init: Set initial BAUD to B0

[Joakim Tjernlund]

On Fri, 2020-07-10 at 12:34 +0200, Greg KH wrote:
> 
> On Fri, Jul 10, 2020 at 11:35:18AM +0200, Joakim Tjernlund wrote:
> > BO will disable USB input until the device opens. This will
> > avoid garbage chars waiting flood the TTY. This mimics a real UART
> > device better.
> > For initial termios to reach USB core, USB driver has to be
> > registered before TTY driver.
> 
> You are doing two different things here, please break this up into 2
> patches, with good documentation for both of them.
> 
> And any reason you didn't send this to the people listed in
> scripts/get_maintainers.pl when run on this patch?
> 
> > 
> > Signed-off-by: Joakim Tjernlund <joakim.tjernlund@infinera.com>
> > Cc: stable@vger.kernel.org
> > ---
> > 
> >  I hope this change makes sense to you, if so I belive
> >  ttyUSB could do the same.
> > 
> >  drivers/usb/class/cdc-acm.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
> > index 751f00285ee6..5680f71200e5 100644
> > --- a/drivers/usb/class/cdc-acm.c
> > +++ b/drivers/usb/class/cdc-acm.c
> > @@ -1999,19 +1999,19 @@ static int __init acm_init(void)
> >       acm_tty_driver->subtype = SERIAL_TYPE_NORMAL,
> >       acm_tty_driver->flags = TTY_DRIVER_REAL_RAW | TTY_DRIVER_DYNAMIC_DEV;
> >       acm_tty_driver->init_termios = tty_std_termios;
> > -     acm_tty_driver->init_termios.c_cflag = B9600 | CS8 | CREAD |
> > +     acm_tty_driver->init_termios.c_cflag = B0 | CS8 | CREAD |
> >                                                               HUPCL | CLOCAL;
> 
> Huh?  Are you sure this works?

Yes, sort of. I didn't see prehistory chars when changed to B0

> 
> >       tty_set_operations(acm_tty_driver, &acm_ops);
> > 
> > -     retval = tty_register_driver(acm_tty_driver);
> > +     retval = usb_register(&acm_driver);
> >       if (retval) {
> >               put_tty_driver(acm_tty_driver);
> >               return retval;
> >       }
> > 
> > -     retval = usb_register(&acm_driver);
> > +     retval = tty_register_driver(acm_tty_driver);
> >       if (retval) {
> > -             tty_unregister_driver(acm_tty_driver);
> > +             usb_deregister(&acm_driver);
> 
> Why are you switching these around?  I think I know, but you don't
> really say...

I wrote:
   For initial termios to reach USB core, USB driver has to be
   registered before TTY driver.
Found out that by trial and error. Isn't that clear enough?

I could change to:
    cdc-acm: acm_init: register USB before TTY driver
    
    For initial termios to reach USB core, USB driver has to be
    registered before TTY driver.

and then just have that change:
@@ -2003,15 +2003,15 @@ static int __init acm_init(void)
 								HUPCL | CLOCAL;
 	tty_set_operations(acm_tty_driver, &acm_ops);
 
-	retval = tty_register_driver(acm_tty_driver);
+	retval = usb_register(&acm_driver);
 	if (retval) {
 		put_tty_driver(acm_tty_driver);
 		return retval;
 	}
 
-	retval = usb_register(&acm_driver);
+	retval = tty_register_driver(acm_tty_driver);
 	if (retval) {
-		tty_unregister_driver(acm_tty_driver);
+		usb_deregister(&acm_driver);
 		put_tty_driver(acm_tty_driver);
 		return retval;
 	}

Re: [PATCH] cdc-acm: acm_init: Set initial BAUD to B0

[Joakim Tjernlund]

On Fri, 2020-07-10 at 12:36 +0200, Greg KH wrote:
> On Fri, Jul 10, 2020 at 10:16:33AM +0000, Joakim Tjernlund wrote:
> > On Fri, 2020-07-10 at 11:54 +0200, Johan Hovold wrote:
> > > CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.
> > > 
> > > 
> > > On Fri, Jul 10, 2020 at 11:35:18AM +0200, Joakim Tjernlund wrote:
> > > > BO will disable USB input until the device opens. This will
> > > > avoid garbage chars waiting flood the TTY. This mimics a real UART
> > > > device better.
> > > > For initial termios to reach USB core, USB driver has to be
> > > > registered before TTY driver.
> > > > 
> > > > Signed-off-by: Joakim Tjernlund <joakim.tjernlund@infinera.com>
> > > > Cc: stable@vger.kernel.org
> > > > ---
> > > > 
> > > >  I hope this change makes sense to you, if so I belive
> > > >  ttyUSB could do the same.
> > > 
> > > No, this doesn't make sense. B0 is used to hang up an already open tty.
> > 
> > This is at module init so there is no tty yet.
> > acm_probe() will later set:
> >         acm->line.dwDTERate = cpu_to_le32(9600);
> > 	acm->line.bDataBits = 8;
> > 	acm_set_line(acm, &acm->line);
> > 
> > > 
> > > Furthermore, this change only affects the initial terminal settings and
> > > won't have any effect the next time you open the same port.
> > 
> > hmm, it is not ideal but acm_probe() will later set:
> >         acm->line.dwDTERate = cpu_to_le32(9600);
> > 	acm->line.bDataBits = 8;
> > 	acm_set_line(acm, &acm->line);
> > 
> > But, would it not make sense to not accept input until TTY is opened ?
> > That would be more inline with a real RS232, would it not?
> 
> You can't keep the chip in the usb-to-serial device from accepting data
> before you do anything with it, that requires firmware to not do this.
> Are you sure your firmware is correct?

No, I don't thin it is correct. I am working both ends here :)

> 
> > > Why not fix your firmware so that it doesn't transmit before DTR is
> > > asserted during open()?
> > 
> > I would but it is not my firmware, it is a ready made USB to UART chip. will talk
> > to the manufacturer though.
> 
> What chip is this?

Microchip USB5734

> 
> thanks,
> 
> greg k-h

Re: [PATCH] cdc-acm: acm_init: Set initial BAUD to B0

[Johan Hovold]

On Fri, Jul 10, 2020 at 10:16:33AM +0000, Joakim Tjernlund wrote:
> On Fri, 2020-07-10 at 11:54 +0200, Johan Hovold wrote:
> > CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.
> > 
> > 
> > On Fri, Jul 10, 2020 at 11:35:18AM +0200, Joakim Tjernlund wrote:
> > > BO will disable USB input until the device opens. This will
> > > avoid garbage chars waiting flood the TTY. This mimics a real UART
> > > device better.
> > > For initial termios to reach USB core, USB driver has to be
> > > registered before TTY driver.
> > > 
> > > Signed-off-by: Joakim Tjernlund <joakim.tjernlund@infinera.com>
> > > Cc: stable@vger.kernel.org
> > > ---
> > > 
> > >  I hope this change makes sense to you, if so I belive
> > >  ttyUSB could do the same.
> > 
> > No, this doesn't make sense. B0 is used to hang up an already open tty.
> 
> This is at module init so there is no tty yet.

Right, but init_termios is used for each tty later registered.

And B0 is supposed to be set after opening a tty if you want to trigger
a modem disconnect (deassert DTR).

> acm_probe() will later set:
>         acm->line.dwDTERate = cpu_to_le32(9600);
> 	acm->line.bDataBits = 8;
> 	acm_set_line(acm, &acm->line);
> 
> > 
> > Furthermore, this change only affects the initial terminal settings and
> > won't have any effect the next time you open the same port.
> 
> hmm, it is not ideal but acm_probe() will later set:
>         acm->line.dwDTERate = cpu_to_le32(9600);
> 	acm->line.bDataBits = 8;
> 	acm_set_line(acm, &acm->line);

That's only during probe and won't affect a second open.

> But, would it not make sense to not accept input until TTY is opened ?
> That would be more inline with a real RS232, would it not?

It's a quirk of some of these devices that some will accept input before
the tty has been opened. It should be possible to handle using flow
control if you control the other side of the application.

I don't see how this patch would work at all as B0 only deasserts DTR
when initialising the line settings in set_termios() (for example)
during open but then the tty layer will assert it again shortly after.
And your firmware ignores DTR anyway...

Ahh, I see now that the driver is passing zero as dwDTERate to the
device in set_termios() if B0 is requested, which is non-standard
(drivers typically keep the current rate or set 9600 baud). Perhaps your
firmware happens to disable the UART, but I can't seem to find anything
in the spec that says that this is what is supposed to happen.

> > Why not fix your firmware so that it doesn't transmit before DTR is
> > asserted during open()?
> 
> I would but it is not my firmware, it is a ready made USB to UART
> chip. will talk to the manufacturer though.

Hope that works out.

Johan

Re: [PATCH] cdc-acm: acm_init: Set initial BAUD to B0

[Johan Hovold]

On Fri, Jul 10, 2020 at 10:46:19AM +0000, Joakim Tjernlund wrote:
> On Fri, 2020-07-10 at 12:34 +0200, Greg KH wrote:
> > 
> > On Fri, Jul 10, 2020 at 11:35:18AM +0200, Joakim Tjernlund wrote:

> > >       tty_set_operations(acm_tty_driver, &acm_ops);
> > > 
> > > -     retval = tty_register_driver(acm_tty_driver);
> > > +     retval = usb_register(&acm_driver);
> > >       if (retval) {
> > >               put_tty_driver(acm_tty_driver);
> > >               return retval;
> > >       }
> > > 
> > > -     retval = usb_register(&acm_driver);
> > > +     retval = tty_register_driver(acm_tty_driver);
> > >       if (retval) {
> > > -             tty_unregister_driver(acm_tty_driver);
> > > +             usb_deregister(&acm_driver);
> > 
> > Why are you switching these around?  I think I know, but you don't
> > really say...
> 
> I wrote:
>    For initial termios to reach USB core, USB driver has to be
>    registered before TTY driver.
> Found out that by trial and error. Isn't that clear enough?

No, that makes no sense at all since USB core does not care about
init_termios.

Johan

Re: [PATCH] cdc-acm: acm_init: Set initial BAUD to B0

[Joakim Tjernlund]

On Fri, 2020-07-10 at 14:41 +0200, Johan Hovold wrote:
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.
> 
> 
> On Fri, Jul 10, 2020 at 10:46:19AM +0000, Joakim Tjernlund wrote:
> > On Fri, 2020-07-10 at 12:34 +0200, Greg KH wrote:
> > > 
> > > On Fri, Jul 10, 2020 at 11:35:18AM +0200, Joakim Tjernlund wrote:
> 
> > > >       tty_set_operations(acm_tty_driver, &acm_ops);
> > > > 
> > > > -     retval = tty_register_driver(acm_tty_driver);
> > > > +     retval = usb_register(&acm_driver);
> > > >       if (retval) {
> > > >               put_tty_driver(acm_tty_driver);
> > > >               return retval;
> > > >       }
> > > > 
> > > > -     retval = usb_register(&acm_driver);
> > > > +     retval = tty_register_driver(acm_tty_driver);
> > > >       if (retval) {
> > > > -             tty_unregister_driver(acm_tty_driver);
> > > > +             usb_deregister(&acm_driver);
> > > 
> > > Why are you switching these around?  I think I know, but you don't
> > > really say...
> > 
> > I wrote:
> >    For initial termios to reach USB core, USB driver has to be
> >    registered before TTY driver.
> > Found out that by trial and error. Isn't that clear enough?
> 
> No, that makes no sense at all since USB core does not care about
> init_termios.

But you install acm_ops into tty:
	tty_set_operations(acm_tty_driver, &acm_ops);
Perhaps there is a call into acm_ops?

Anyhow, does it not make sense to have usb before tty as tty uses usb?

Can I ask this too:
 what is the difference between acm_tty_install and acm_tty_open ? Both seems to be called at open(2)
seems to me that install could be folded into open ?

 Jocke

Re: [PATCH] cdc-acm: acm_init: Set initial BAUD to B0

[Joakim Tjernlund]

On Fri, 2020-07-10 at 18:05 +0200, Joakim Tjernlund wrote:
> On Fri, 2020-07-10 at 14:41 +0200, Johan Hovold wrote:
> > CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.
> > 
> > 
> > On Fri, Jul 10, 2020 at 10:46:19AM +0000, Joakim Tjernlund wrote:
> > > On Fri, 2020-07-10 at 12:34 +0200, Greg KH wrote:
> > > > 
> > > > On Fri, Jul 10, 2020 at 11:35:18AM +0200, Joakim Tjernlund wrote:
> > 
> > > > >       tty_set_operations(acm_tty_driver, &acm_ops);
> > > > > 
> > > > > -     retval = tty_register_driver(acm_tty_driver);
> > > > > +     retval = usb_register(&acm_driver);
> > > > >       if (retval) {
> > > > >               put_tty_driver(acm_tty_driver);
> > > > >               return retval;
> > > > >       }
> > > > > 
> > > > > -     retval = usb_register(&acm_driver);
> > > > > +     retval = tty_register_driver(acm_tty_driver);
> > > > >       if (retval) {
> > > > > -             tty_unregister_driver(acm_tty_driver);
> > > > > +             usb_deregister(&acm_driver);
> > > > 
> > > > Why are you switching these around?  I think I know, but you don't
> > > > really say...
> > > 
> > > I wrote:
> > >    For initial termios to reach USB core, USB driver has to be
> > >    registered before TTY driver.
> > > Found out that by trial and error. Isn't that clear enough?
> > 
> > No, that makes no sense at all since USB core does not care about
> > init_termios.
> 
> But you install acm_ops into tty:
> 	tty_set_operations(acm_tty_driver, &acm_ops);
> Perhaps there is a call into acm_ops?
> 
> Anyhow, does it not make sense to have usb before tty as tty uses usb?

Forgot to mention, I can remove:
//	acm->line.dwDTERate = cpu_to_le32(9600);
//	acm->line.bDataBits = 8;
//	acm_set_line(acm, &acm->line);
in  acm_probe() without noticing any change

 Jocke

Re: [PATCH] cdc-acm: acm_init: Set initial BAUD to B0

[Oliver Neukum]

Am Freitag, den 10.07.2020, 11:35 +0200 schrieb Joakim Tjernlund:
> 


Hi,

> --- a/drivers/usb/class/cdc-acm.c
> +++ b/drivers/usb/class/cdc-acm.c
> @@ -1999,19 +1999,19 @@ static int __init acm_init(void)
>  	acm_tty_driver->subtype = SERIAL_TYPE_NORMAL,
>  	acm_tty_driver->flags = TTY_DRIVER_REAL_RAW | TTY_DRIVER_DYNAMIC_DEV;
>  	acm_tty_driver->init_termios = tty_std_termios;
> -	acm_tty_driver->init_termios.c_cflag = B9600 | CS8 | CREAD |
> +	acm_tty_driver->init_termios.c_cflag = B0 | CS8 | CREAD |
>  								HUPCL | CLOCAL;
>  	tty_set_operations(acm_tty_driver, &acm_ops);
>  
> -	retval = tty_register_driver(acm_tty_driver);
> +	retval = usb_register(&acm_driver);


No,

you cannot do that. This means that probe() is now live.
Probe() in turn does this:

        tty_dev = tty_port_register_device(&acm->port, acm_tty_driver, minor,
                        &control_interface->dev);
        if (IS_ERR(tty_dev)) {
                rv = PTR_ERR(tty_dev);
                goto alloc_fail6;
        }


That is just not a good idea when the tty is not already registered.
You are opening up a race.

	Regards
		Oliver

Re: [PATCH] cdc-acm: acm_init: Set initial BAUD to B0

[Johan Hovold]

On Fri, Jul 10, 2020 at 04:05:29PM +0000, Joakim Tjernlund wrote:
> On Fri, 2020-07-10 at 14:41 +0200, Johan Hovold wrote:
> > CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.
> > 
> > 
> > On Fri, Jul 10, 2020 at 10:46:19AM +0000, Joakim Tjernlund wrote:
> > > On Fri, 2020-07-10 at 12:34 +0200, Greg KH wrote:
> > > > 
> > > > On Fri, Jul 10, 2020 at 11:35:18AM +0200, Joakim Tjernlund wrote:
> > 
> > > > >       tty_set_operations(acm_tty_driver, &acm_ops);
> > > > > 
> > > > > -     retval = tty_register_driver(acm_tty_driver);
> > > > > +     retval = usb_register(&acm_driver);
> > > > >       if (retval) {
> > > > >               put_tty_driver(acm_tty_driver);
> > > > >               return retval;
> > > > >       }
> > > > > 
> > > > > -     retval = usb_register(&acm_driver);
> > > > > +     retval = tty_register_driver(acm_tty_driver);
> > > > >       if (retval) {
> > > > > -             tty_unregister_driver(acm_tty_driver);
> > > > > +             usb_deregister(&acm_driver);
> > > > 
> > > > Why are you switching these around?  I think I know, but you don't
> > > > really say...
> > > 
> > > I wrote:
> > >    For initial termios to reach USB core, USB driver has to be
> > >    registered before TTY driver.
> > > Found out that by trial and error. Isn't that clear enough?
> > 
> > No, that makes no sense at all since USB core does not care about
> > init_termios.
> 
> But you install acm_ops into tty:
> 	tty_set_operations(acm_tty_driver, &acm_ops);
> Perhaps there is a call into acm_ops?

No, not until the tty device has been registered by the USB driver.

> Anyhow, does it not make sense to have usb before tty as tty uses usb?

Nope, it's the other way round, and your change is therefore broken.

> Can I ask this too:
>  what is the difference between acm_tty_install and acm_tty_open ?
>  Both seems to be called at open(2)
> seems to me that install could be folded into open ?

No, their purposes are distinct and they cannot be merged.

Johan

Re: [PATCH] cdc-acm: acm_init: Set initial BAUD to B0

[Joakim Tjernlund]

On Mon, 2020-07-13 at 12:08 +0200, Oliver Neukum wrote:
> 
> Am Freitag, den 10.07.2020, 11:35 +0200 schrieb Joakim Tjernlund:
> > 
> 
> 
> Hi,
> 
> > --- a/drivers/usb/class/cdc-acm.c
> > +++ b/drivers/usb/class/cdc-acm.c
> > @@ -1999,19 +1999,19 @@ static int __init acm_init(void)
> >       acm_tty_driver->subtype = SERIAL_TYPE_NORMAL,
> >       acm_tty_driver->flags = TTY_DRIVER_REAL_RAW |
> > TTY_DRIVER_DYNAMIC_DEV;
> >       acm_tty_driver->init_termios = tty_std_termios;
> > -     acm_tty_driver->init_termios.c_cflag = B9600 | CS8 | CREAD |
> > +     acm_tty_driver->init_termios.c_cflag = B0 | CS8 | CREAD |
> >                                                               HUPCL
> > | CLOCAL;
> >       tty_set_operations(acm_tty_driver, &acm_ops);
> > 
> > -     retval = tty_register_driver(acm_tty_driver);
> > +     retval = usb_register(&acm_driver);
> 
> 
> No,
> 
> you cannot do that. This means that probe() is now live.
> Probe() in turn does this:
> 
>         tty_dev = tty_port_register_device(&acm->port,
> acm_tty_driver, minor,
>                         &control_interface->dev);
>         if (IS_ERR(tty_dev)) {
>                 rv = PTR_ERR(tty_dev);
>                 goto alloc_fail6;
>         }
> 
> 
> That is just not a good idea when the tty is not already registered.
> You are opening up a race.
> 

OK, but it is strange that init_termios.c_cflag does not take/do
anything unless I change order. Perhaps termios should move to probe
instead?

Also, the handling of DTR came up. It seems to me that ACM
deassert DTR until open time which is fine/good.
!DTR signals to the other end that ACM is not ready and don't
want input but ACM still accepts input if received.

Would it make sense to actually enforce DTR locally too?
If input is unwanted, don't accept any either.

 Jocke

Re: [PATCH] cdc-acm: acm_init: Set initial BAUD to B0

[Oliver Neukum]

Am Montag, den 13.07.2020, 20:26 +0000 schrieb Joakim Tjernlund:

Good morning,

> 
> OK, but it is strange that init_termios.c_cflag does not take/do
> anything unless I change order. Perhaps termios should move to probe
> instead?

Are you saying that the tty layer does not set termios when a new
tty is created? This looks strange to me and I do not want to paper
over a wider issue.

> Also, the handling of DTR came up. It seems to me that ACM
> deassert DTR until open time which is fine/good.

ACM does not really control DTR by itself. The earliest time
we can touch it would be probe. And again setting sane defaults
for DTR looks like a job for the tty layer.
acm_set_control() AFAICT does its job correctly.

> !DTR signals to the other end that ACM is not ready and don't
> want input but ACM still accepts input if received.
> 
> Would it make sense to actually enforce DTR locally too?
> If input is unwanted, don't accept any either.

This looks difficult. Basically we can call acm_set_control()
which will execute a control request. Yet there is inevitably
a race between setting such a control line and data getting in
and between clearing a control signal and data getting into the buffer.
We could stop reading once we are sure the control signal is
effective, but we have no operation for clearing the buffers
in the device. We cannot tell whether data in them is stale
in the sense of being accepted without DTR or newly arrived.

Could you make a concrete proposal of how to achieve this
short of a device reset? The mapping between CDC-ACM and
a physical RS-232 is only as good as the device makes it.
CDC-ACM is only secondarily a serial driver. The common
association between modems and serial devices is historical,
not technical.

	Regards
		Oliver

Re: [PATCH] cdc-acm: acm_init: Set initial BAUD to B0

[Joakim Tjernlund]

On Tue, 2020-07-14 at 09:02 +0200, Oliver Neukum wrote:
> 
> Am Montag, den 13.07.2020, 20:26 +0000 schrieb Joakim Tjernlund:
> 
> Good morning,
> 
> > 
> > OK, but it is strange that init_termios.c_cflag does not take/do
> > anything unless I change order. Perhaps termios should move to
> > probe
> > instead?
> 
> Are you saying that the tty layer does not set termios when a new
> tty is created? This looks strange to me and I do not want to paper
> over a wider issue.

Not sure. all I can say that when I was playing with B0 baud, I had
to change order to see any effects of B0. 

> 
> > Also, the handling of DTR came up. It seems to me that ACM
> > deassert DTR until open time which is fine/good.
> 
> ACM does not really control DTR by itself. The earliest time
> we can touch it would be probe. And again setting sane defaults
> for DTR looks like a job for the tty layer.
> acm_set_control() AFAICT does its job correctly.

Yes, so it seems.

> 
> > !DTR signals to the other end that ACM is not ready and don't
> > want input but ACM still accepts input if received.
> > 
> > Would it make sense to actually enforce DTR locally too?
> > If input is unwanted, don't accept any either.
> 
> This looks difficult. Basically we can call acm_set_control()
> which will execute a control request. Yet there is inevitably
> a race between setting such a control line and data getting in
> and between clearing a control signal and data getting into the
> buffer.
> We could stop reading once we are sure the control signal is
> effective, but we have no operation for clearing the buffers
> in the device. We cannot tell whether data in them is stale
> in the sense of being accepted without DTR or newly arrived.

Does this race really matter? There will always be a race, even when
DTR is HW driven. I guess what is really missing is way
to control/clear input buffers.
Some sort of flag to the tty layer would be best/minimize race I think,
maybe DTR_NO_INPUT which could be set at init time.

> Could you make a concrete proposal of how to achieve this
> short of a device reset? The mapping between CDC-ACM and
> a physical RS-232 is only as good as the device makes it.

I am just getting started with USB/ACM and I got here due to odd
USB chip we just started to use so a concrete proposal is a bit too
much for me ATM

> CDC-ACM is only secondarily a serial driver. The common
> association between modems and serial devices is historical,
> not technical.

Yes, ttyUSB seems better suited for this but ATM I don't
have that choice.

  Regards
          Joakim