Re: [PATCH 2/2] usb: gadget serial: Honour termios CLOCAL on disconnect

Re: [PATCH 2/2] usb: gadget serial: Honour termios CLOCAL on disconnect

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1566 bytes --]

Hi,

On Sun, Oct 26, 2014 at 08:01:30PM +0200, Kyösti Mälkki wrote:
> There are applications where it is desirable to not hangup ttyGS* when
> USB disconnect is detected. USB host side of communication may
> power-cycle periodically or there may be the actual need to physically
> disconnect and reconnect USB cable temporarily.
> 
> USB disconnects on serial gadget are comparable to loss of Carrier Detect
> of conventional UARTs. With the change, if ttyGS* has termios CLOCAL flag
> set, disconnect on USB does not hangup the TTY.
> 
> Signed-off-by: Kyösti Mälkki <kyosti.malkki-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/usb/gadget/function/u_serial.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/gadget/function/u_serial.c b/drivers/usb/gadget/function/u_serial.c
> index 491082a..e68ffd7 100644
> --- a/drivers/usb/gadget/function/u_serial.c
> +++ b/drivers/usb/gadget/function/u_serial.c
> @@ -1254,8 +1254,13 @@ void gserial_disconnect(struct gserial *gser)
>  	gser->ioport = NULL;
>  	if (port->port.count > 0 || port->openclose) {
>  		wake_up_interruptible(&port->drain_wait);
> -		if (port->port.tty)
> -			tty_hangup(port->port.tty);
> +		struct tty_struct *tty = port->port.tty;

declare above as Sergei said.

> +		if (tty) {

is there any situation where tty would be NULL here ?

> +			if (tty->termios.c_cflag & CLOCAL)
> +				stop_tty(tty);
> +			else
> +				tty_hangup(tty);

this I'll defer to Greg who also maintains tty.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 2/2] usb: gadget serial: Honour termios CLOCAL on disconnect

[Kyösti Mälkki]

On 10/27/2014 03:38 PM, Felipe Balbi wrote:
> Hi,
>
> On Sun, Oct 26, 2014 at 08:01:30PM +0200, Kyösti Mälkki wrote:
>> There are applications where it is desirable to not hangup ttyGS* when
>> USB disconnect is detected. USB host side of communication may
>> power-cycle periodically or there may be the actual need to physically
>> disconnect and reconnect USB cable temporarily.
>>
>> USB disconnects on serial gadget are comparable to loss of Carrier Detect
>> of conventional UARTs. With the change, if ttyGS* has termios CLOCAL flag
>> set, disconnect on USB does not hangup the TTY.
>>
>> Signed-off-by: Kyösti Mälkki <kyosti.malkki@gmail.com>
>> ---
>>   drivers/usb/gadget/function/u_serial.c | 9 +++++++--
>>   1 file changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/usb/gadget/function/u_serial.c b/drivers/usb/gadget/function/u_serial.c
>> index 491082a..e68ffd7 100644
>> --- a/drivers/usb/gadget/function/u_serial.c
>> +++ b/drivers/usb/gadget/function/u_serial.c
>> @@ -1254,8 +1254,13 @@ void gserial_disconnect(struct gserial *gser)
>>   	gser->ioport = NULL;
>>   	if (port->port.count > 0 || port->openclose) {
>>   		wake_up_interruptible(&port->drain_wait);
>> -		if (port->port.tty)
>> -			tty_hangup(port->port.tty);
>> +		struct tty_struct *tty = port->port.tty;
>
> declare above as Sergei said.
>
>> +		if (tty) {
>
> is there any situation where tty would be NULL here ?
>
>> +			if (tty->termios.c_cflag & CLOCAL)
>> +				stop_tty(tty);
>> +			else
>> +				tty_hangup(tty);
>
> this I'll defer to Greg who also maintains tty.
>

My main concern is if someone runs getty on ttyACM without explicitly 
clearing CLOCAL. With the patch here login session would no longer get 
SIGHUP if cable is disconnected. And it is not possible to tell if it is 
the same cable that is plugged back in.

KM




--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] usb: gadget serial: Honour termios CLOCAL on disconnect

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2133 bytes --]

On Mon, Oct 27, 2014 at 05:43:06PM +0200, Kyösti Mälkki wrote:
> On 10/27/2014 03:38 PM, Felipe Balbi wrote:
> >Hi,
> >
> >On Sun, Oct 26, 2014 at 08:01:30PM +0200, Kyösti Mälkki wrote:
> >>There are applications where it is desirable to not hangup ttyGS* when
> >>USB disconnect is detected. USB host side of communication may
> >>power-cycle periodically or there may be the actual need to physically
> >>disconnect and reconnect USB cable temporarily.
> >>
> >>USB disconnects on serial gadget are comparable to loss of Carrier Detect
> >>of conventional UARTs. With the change, if ttyGS* has termios CLOCAL flag
> >>set, disconnect on USB does not hangup the TTY.
> >>
> >>Signed-off-by: Kyösti Mälkki <kyosti.malkki-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> >>---
> >>  drivers/usb/gadget/function/u_serial.c | 9 +++++++--
> >>  1 file changed, 7 insertions(+), 2 deletions(-)
> >>
> >>diff --git a/drivers/usb/gadget/function/u_serial.c b/drivers/usb/gadget/function/u_serial.c
> >>index 491082a..e68ffd7 100644
> >>--- a/drivers/usb/gadget/function/u_serial.c
> >>+++ b/drivers/usb/gadget/function/u_serial.c
> >>@@ -1254,8 +1254,13 @@ void gserial_disconnect(struct gserial *gser)
> >>  	gser->ioport = NULL;
> >>  	if (port->port.count > 0 || port->openclose) {
> >>  		wake_up_interruptible(&port->drain_wait);
> >>-		if (port->port.tty)
> >>-			tty_hangup(port->port.tty);
> >>+		struct tty_struct *tty = port->port.tty;
> >
> >declare above as Sergei said.
> >
> >>+		if (tty) {
> >
> >is there any situation where tty would be NULL here ?
> >
> >>+			if (tty->termios.c_cflag & CLOCAL)
> >>+				stop_tty(tty);
> >>+			else
> >>+				tty_hangup(tty);
> >
> >this I'll defer to Greg who also maintains tty.
> >
> 
> My main concern is if someone runs getty on ttyACM without explicitly
> clearing CLOCAL. With the patch here login session would no longer get
> SIGHUP if cable is disconnected. And it is not possible to tell if it is the
> same cable that is plugged back in.

true, but woudln't the same thing happen with a real UART port using
8250.ko ?

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 2/2] usb: gadget serial: Honour termios CLOCAL on disconnect

[Peter Hurley]

On 10/27/2014 09:38 AM, Felipe Balbi wrote:
> Hi,
> 
> On Sun, Oct 26, 2014 at 08:01:30PM +0200, Kyösti Mälkki wrote:
>> There are applications where it is desirable to not hangup ttyGS* when
>> USB disconnect is detected. USB host side of communication may
>> power-cycle periodically or there may be the actual need to physically
>> disconnect and reconnect USB cable temporarily.
>>
>> USB disconnects on serial gadget are comparable to loss of Carrier Detect
>> of conventional UARTs. With the change, if ttyGS* has termios CLOCAL flag
>> set, disconnect on USB does not hangup the TTY.
>>
>> Signed-off-by: Kyösti Mälkki <kyosti.malkki-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> ---
>>  drivers/usb/gadget/function/u_serial.c | 9 +++++++--
>>  1 file changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/usb/gadget/function/u_serial.c b/drivers/usb/gadget/function/u_serial.c
>> index 491082a..e68ffd7 100644
>> --- a/drivers/usb/gadget/function/u_serial.c
>> +++ b/drivers/usb/gadget/function/u_serial.c
>> @@ -1254,8 +1254,13 @@ void gserial_disconnect(struct gserial *gser)
>>  	gser->ioport = NULL;
>>  	if (port->port.count > 0 || port->openclose) {
>>  		wake_up_interruptible(&port->drain_wait);
>> -		if (port->port.tty)
>> -			tty_hangup(port->port.tty);
>> +		struct tty_struct *tty = port->port.tty;
> 
> declare above as Sergei said.
> 
>> +		if (tty) {
> 
> is there any situation where tty would be NULL here ?
> 
>> +			if (tty->termios.c_cflag & CLOCAL)
>> +				stop_tty(tty);
>> +			else
>> +				tty_hangup(tty);
> 
> this I'll defer to Greg who also maintains tty.

I'm curious what happens without stop_tty().

The tty is restartable from userspace with
    tcflow(fd, TCOOFF);
    tcflow(fd, TCOON);

Regards,
Peter Hurley
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

usb: gadget: Fixes to receive from USB EHCI Debug

[Kyösti Mälkki]

USB gadget driver dbgp can be used as an EHCI debug dongle in replacement
for product like Net20DC. With it one can receive early kernel messages
from remote targets over USB. See parameter earlyprintk=dbgp for target
kernel requirements and configuration.
 
The two patches are required for dbgp gadget driver to work. Longterm
plan: I think G_DBGP_PRINTK is an odd hack and could be removed.
Exposing the debug descriptor can be implemented in cdc_acm so that we
could seemlessly reconfigure endpoint from the 8 byte payload limitation
EHCI Debug Port has, to 512 bytes, once ehci-hcd with DMA is available.


Handling of the termios CLOCAL flag calls for some discussion and I am
not that familiar with the tty infra u_serial uses. See the previous
change around gserial_disconnect():
  http://permalink.gmane.org/gmane.linux.usb.general/4260

Regards,
  Kyösti
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 1/2] usb: gadget dbgp: Fix endpoint config after USB disconnect

[Kyösti Mälkki]

SET_FEATURE request with DEBUG_MODE only worked the first time after module
initialisation. Per the USB 2.0 debug device specification, said request
is to be treated as if it were a SET_CONFIGURATION request, i.e. endpoint
must be re-configured.

As configure_endpoints() may now get called multiple times, move it outside
__init and move serial_alloc_tty() call into __init.

Code has assumption that endpoint mapping remains unchanged with consecutive
calls of configure_endpoints().

Signed-off-by: Kyösti Mälkki <kyosti.malkki-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/usb/gadget/legacy/dbgp.c | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/drivers/usb/gadget/legacy/dbgp.c b/drivers/usb/gadget/legacy/dbgp.c
index 1b07513..633683a 100644
--- a/drivers/usb/gadget/legacy/dbgp.c
+++ b/drivers/usb/gadget/legacy/dbgp.c
@@ -237,7 +237,7 @@ static void dbgp_unbind(struct usb_gadget *gadget)
 static unsigned char tty_line;
 #endif
 
-static int __init dbgp_configure_endpoints(struct usb_gadget *gadget)
+static int dbgp_configure_endpoints(struct usb_gadget *gadget)
 {
 	int stp;
 
@@ -273,19 +273,10 @@ static int __init dbgp_configure_endpoints(struct usb_gadget *gadget)
 
 	dbgp.serial->in->desc = &i_desc;
 	dbgp.serial->out->desc = &o_desc;
-
-	if (gserial_alloc_line(&tty_line)) {
-		stp = 3;
-		goto fail_3;
-	}
+#endif
 
 	return 0;
 
-fail_3:
-	dbgp.o_ep->driver_data = NULL;
-#else
-	return 0;
-#endif
 fail_2:
 	dbgp.i_ep->driver_data = NULL;
 fail_1:
@@ -324,10 +315,17 @@ static int __init dbgp_bind(struct usb_gadget *gadget,
 		err = -ENOMEM;
 		goto fail;
 	}
+
+	if (gserial_alloc_line(&tty_line)) {
+		stp = 4;
+		err = -ENODEV;
+		goto fail;
+	}
 #endif
+
 	err = dbgp_configure_endpoints(gadget);
 	if (err < 0) {
-		stp = 4;
+		stp = 5;
 		goto fail;
 	}
 
@@ -383,6 +381,10 @@ static int dbgp_setup(struct usb_gadget *gadget,
 #ifdef CONFIG_USB_G_DBGP_PRINTK
 		err = dbgp_enable_ep();
 #else
+		err = dbgp_configure_endpoints(gadget);
+		if (err < 0) {
+			goto fail;
+		}
 		err = gserial_connect(dbgp.serial, tty_line);
 #endif
 		if (err < 0)
-- 
1.8.1.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/2] usb: gadget dbgp: Fix endpoint config after USB disconnect

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 765 bytes --]

On Mon, Nov 03, 2014 at 05:18:04PM +0200, Kyösti Mälkki wrote:
> SET_FEATURE request with DEBUG_MODE only worked the first time after module
> initialisation. Per the USB 2.0 debug device specification, said request
> is to be treated as if it were a SET_CONFIGURATION request, i.e. endpoint
> must be re-configured.
> 
> As configure_endpoints() may now get called multiple times, move it outside
> __init and move serial_alloc_tty() call into __init.
> 
> Code has assumption that endpoint mapping remains unchanged with consecutive
> calls of configure_endpoints().
> 
> Signed-off-by: Kyösti Mälkki <kyosti.malkki-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---

if there are no comments here, I'll add this one to my testing/next.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH v2 2/2] usb: gadget serial: Honour termios CLOCAL on disconnect

[Kyösti Mälkki]

There are applications where it is desirable to not hangup ttyGS* when
USB disconnect is detected. USB host side of communication may
power-cycle periodically or there may be the actual need to physically
disconnect and reconnect USB cable temporarily.

USB disconnects on serial gadget are comparable to loss of Carrier Detect
of conventional UARTs. With the change, if ttyGS* has termios CLOCAL flag
set, disconnect on USB does not hangup the TTY.

Signed-off-by: Kyösti Mälkki <kyosti.malkki-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/usb/gadget/function/u_serial.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/function/u_serial.c b/drivers/usb/gadget/function/u_serial.c
index 491082a..dabc165 100644
--- a/drivers/usb/gadget/function/u_serial.c
+++ b/drivers/usb/gadget/function/u_serial.c
@@ -1253,9 +1253,16 @@ void gserial_disconnect(struct gserial *gser)
 	port->port_usb = NULL;
 	gser->ioport = NULL;
 	if (port->port.count > 0 || port->openclose) {
+		struct tty_struct *tty;
+
 		wake_up_interruptible(&port->drain_wait);
-		if (port->port.tty)
-			tty_hangup(port->port.tty);
+		tty = port->port.tty;
+		if (tty) {
+			if (tty->termios.c_cflag & CLOCAL)
+				stop_tty(tty);
+			else
+				tty_hangup(tty);
+		}
 	}
 	spin_unlock_irqrestore(&port->port_lock, flags);
 
-- 
1.8.1.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/2] usb: gadget serial: Honour termios CLOCAL on disconnect

[Peter Hurley]

Hi Kyösti,

On 11/03/2014 10:18 AM, Kyösti Mälkki wrote:
> There are applications where it is desirable to not hangup ttyGS* when
> USB disconnect is detected. USB host side of communication may
> power-cycle periodically or there may be the actual need to physically
> disconnect and reconnect USB cable temporarily.
> 
> USB disconnects on serial gadget are comparable to loss of Carrier Detect
> of conventional UARTs. With the change, if ttyGS* has termios CLOCAL flag
> set, disconnect on USB does not hangup the TTY.
> 
> Signed-off-by: Kyösti Mälkki <kyosti.malkki-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/usb/gadget/function/u_serial.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/gadget/function/u_serial.c b/drivers/usb/gadget/function/u_serial.c
> index 491082a..dabc165 100644
> --- a/drivers/usb/gadget/function/u_serial.c
> +++ b/drivers/usb/gadget/function/u_serial.c
> @@ -1253,9 +1253,16 @@ void gserial_disconnect(struct gserial *gser)
>  	port->port_usb = NULL;
>  	gser->ioport = NULL;
>  	if (port->port.count > 0 || port->openclose) {
> +		struct tty_struct *tty;
> +
>  		wake_up_interruptible(&port->drain_wait);
> -		if (port->port.tty)
> -			tty_hangup(port->port.tty);
> +		tty = port->port.tty;
> +		if (tty) {
> +			if (tty->termios.c_cflag & CLOCAL)
> +				stop_tty(tty);
> +			else
> +				tty_hangup(tty);

It seems you missed my earlier email: what happens if you leave
out the stop_tty() call here?

I ask because the tty is still restartable from userspace after you
stop_tty() here. So if your goal is to prevent write() from
happening, this won't work.

Regards,
Peter Hurley

> +		}
>  	}
>  	spin_unlock_irqrestore(&port->port_lock, flags);
>  
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/2] usb: gadget serial: Honour termios CLOCAL on disconnect

[Kyösti Mälkki]

On 11/03/2014 05:33 PM, Peter Hurley wrote:
> Hi Kyösti,
>
> On 11/03/2014 10:18 AM, Kyösti Mälkki wrote:
>> There are applications where it is desirable to not hangup ttyGS* when
>> USB disconnect is detected. USB host side of communication may
>> power-cycle periodically or there may be the actual need to physically
>> disconnect and reconnect USB cable temporarily.
>>
>> USB disconnects on serial gadget are comparable to loss of Carrier Detect
>> of conventional UARTs. With the change, if ttyGS* has termios CLOCAL flag
>> set, disconnect on USB does not hangup the TTY.
>>
>> Signed-off-by: Kyösti Mälkki <kyosti.malkki-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> ---
>>   drivers/usb/gadget/function/u_serial.c | 11 +++++++++--
>>   1 file changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/usb/gadget/function/u_serial.c b/drivers/usb/gadget/function/u_serial.c
>> index 491082a..dabc165 100644
>> --- a/drivers/usb/gadget/function/u_serial.c
>> +++ b/drivers/usb/gadget/function/u_serial.c
>> @@ -1253,9 +1253,16 @@ void gserial_disconnect(struct gserial *gser)
>>   	port->port_usb = NULL;
>>   	gser->ioport = NULL;
>>   	if (port->port.count > 0 || port->openclose) {
>> +		struct tty_struct *tty;
>> +
>>   		wake_up_interruptible(&port->drain_wait);
>> -		if (port->port.tty)
>> -			tty_hangup(port->port.tty);
>> +		tty = port->port.tty;
>> +		if (tty) {
>> +			if (tty->termios.c_cflag & CLOCAL)
>> +				stop_tty(tty);
>> +			else
>> +				tty_hangup(tty);
>
> It seems you missed my earlier email: what happens if you leave
> out the stop_tty() call here?
>

Hi Peter

I did not miss it. My intro to the patch series says I am not familiar 
with tty infra, so I did not realize the question was directed to me.
Also my setup with serial gadget runs kernel 3.12.6 so I cannot really 
tell or test if tty changes since that would make my patch invalid or 
unnecessary.

In the previous change for gserial_disconnect(), where the tty_hangup() 
call was added, commit message described it fixes an endless loop on tty 
device read and select. Would we be back at that situation without call 
to stop_tty()?

> I ask because the tty is still restartable from userspace after you
> stop_tty() here. So if your goal is to prevent write() from
> happening, this won't work.

There was no goal to prevent write() on the tty device, should there be? 
Or should it silently just drop any characters while in the disconnected 
state instead of buffering (some of) them?

Thanks,
KM

>
> Regards,
> Peter Hurley
>
>> +		}
>>   	}
>>   	spin_unlock_irqrestore(&port->port_lock, flags);
>>
>>
>

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html