[PATCHv2] serio: add hangup support

The Pulse-Eight USB CEC adapter is a usb device that shows up as a ttyACM0 device.
It requires that you run inputattach in order to communicate with it via serio.

This all works well, but it would be nice to have a udev rule to automatically
start inputattach. That too works OK, but the problem comes when the USB device
is unplugged: the tty hangup is never handled by the serio framework so the
inputattach utility never exits and you have to kill it manually.

By adding this hangup callback the inputattach utility now properly exits as
soon as the USB device is unplugged.

The udev rule I used on my Debian sid system is:

SUBSYSTEM=="tty", KERNEL=="ttyACM[0-9]*", ATTRS{idVendor}=="2548", ATTRS{idProduct}=="1002", ACTION=="add", TAG+="systemd", ENV{SYSTEMD_WANTS}+="pulse8-cec-inputattach@%k.service"

And pulse8-cec-inputattach@%k.service is as follows:

===============================================================
[Unit]
Description=inputattach for pulse8-cec device on %I

[Service]
Type=simple
ExecStart=/usr/local/bin/inputattach --pulse8-cec /dev/%I
KillMode=process
===============================================================

Signed-off-by: Hans Verkuil <redacted>
Tested-by: Hans Verkuil <redacted>
---
Change since the original RFC patch: don't call close() from the hangup() function,
instead only set the DEAD flag in hangup() instead of in close().
---

diff --git a/drivers/input/serio/serport.c b/drivers/input/serio/serport.c
index 9c927d3..4045e95 100644
--- a/drivers/input/serio/serport.c
+++ b/drivers/input/serio/serport.c

@@ -71,7 +71,6 @@ static void serport_serio_close(struct serio *serio)

 	spin_lock_irqsave(&serport->lock, flags);
 	clear_bit(SERPORT_ACTIVE, &serport->flags);
-	set_bit(SERPORT_DEAD, &serport->flags);
 	spin_unlock_irqrestore(&serport->lock, flags);

 	wake_up_interruptible(&serport->wait);

@@ -248,6 +247,19 @@ static long serport_ldisc_compat_ioctl(struct tty_struct *tty,
 }
 #endif

+static int serport_ldisc_hangup(struct tty_struct * tty)
+{
+	struct serport *serport = (struct serport *) tty->disc_data;
+	unsigned long flags;
+
+	spin_lock_irqsave(&serport->lock, flags);
+	set_bit(SERPORT_DEAD, &serport->flags);
+	spin_unlock_irqrestore(&serport->lock, flags);
+
+	wake_up_interruptible(&serport->wait);
+	return 0;
+}
+
 static void serport_ldisc_write_wakeup(struct tty_struct * tty)
 {
 	struct serport *serport = (struct serport *) tty->disc_data;

@@ -274,6 +286,7 @@ static struct tty_ldisc_ops serport_ldisc = {
 	.compat_ioctl =	serport_ldisc_compat_ioctl,
 #endif
 	.receive_buf =	serport_ldisc_receive,
+	.hangup =	serport_ldisc_hangup,
 	.write_wakeup =	serport_ldisc_write_wakeup
 };

On Wed, Aug 03, 2016 at 01:00:44PM +0200, Hans Verkuil wrote:

I think we should remove this line as well - the waiter is waiting on
SERPORT_DEAD bit, if we are not setting it we do not need to wake up the
waiter either.

I can fix it up on my side.

Thanks.

-- 
Dmitry

On 08/03/2016 05:26 PM, Dmitry Torokhov wrote:

On Wed, Aug 03, 2016 at 01:00:44PM +0200, Hans Verkuil wrote:

quoted

The Pulse-Eight USB CEC adapter is a usb device that shows up as a ttyACM0 device.
It requires that you run inputattach in order to communicate with it via serio.

This all works well, but it would be nice to have a udev rule to automatically
start inputattach. That too works OK, but the problem comes when the USB device
is unplugged: the tty hangup is never handled by the serio framework so the
inputattach utility never exits and you have to kill it manually.

By adding this hangup callback the inputattach utility now properly exits as
soon as the USB device is unplugged.

The udev rule I used on my Debian sid system is:

SUBSYSTEM=="tty", KERNEL=="ttyACM[0-9]*", ATTRS{idVendor}=="2548", ATTRS{idProduct}=="1002", ACTION=="add", TAG+="systemd", ENV{SYSTEMD_WANTS}+="pulse8-cec-inputattach@%k.service"

And pulse8-cec-inputattach@%k.service is as follows:

===============================================================
[Unit]
Description=inputattach for pulse8-cec device on %I

[Service]
Type=simple
ExecStart=/usr/local/bin/inputattach --pulse8-cec /dev/%I
KillMode=process
===============================================================

Signed-off-by: Hans Verkuil <redacted>
Tested-by: Hans Verkuil <redacted>
---
Change since the original RFC patch: don't call close() from the hangup() function,
instead only set the DEAD flag in hangup() instead of in close().
---

diff --git a/drivers/input/serio/serport.c b/drivers/input/serio/serport.c
index 9c927d3..4045e95 100644
--- a/drivers/input/serio/serport.c
+++ b/drivers/input/serio/serport.c

@@ -71,7 +71,6 @@ static void serport_serio_close(struct serio *serio)

 	spin_lock_irqsave(&serport->lock, flags);
 	clear_bit(SERPORT_ACTIVE, &serport->flags);
-	set_bit(SERPORT_DEAD, &serport->flags);
 	spin_unlock_irqrestore(&serport->lock, flags);

 	wake_up_interruptible(&serport->wait);

I think we should remove this line as well - the waiter is waiting on
SERPORT_DEAD bit, if we are not setting it we do not need to wake up the
waiter either.

I can fix it up on my side.

OK, I wasn't sure about that myself, thanks for looking into this and taking care of it.

Regards,

	Hans

Hi Dmitry,

I don't know if you processed this patch yet, but if not, can you update a
small mistake in the commit log? See below.

On 08/03/16 17:26, Dmitry Torokhov wrote:

On Wed, Aug 03, 2016 at 01:00:44PM +0200, Hans Verkuil wrote:

quoted

The Pulse-Eight USB CEC adapter is a usb device that shows up as a ttyACM0 device.
It requires that you run inputattach in order to communicate with it via serio.

This all works well, but it would be nice to have a udev rule to automatically
start inputattach. That too works OK, but the problem comes when the USB device
is unplugged: the tty hangup is never handled by the serio framework so the
inputattach utility never exits and you have to kill it manually.

By adding this hangup callback the inputattach utility now properly exits as
soon as the USB device is unplugged.

The udev rule I used on my Debian sid system is:

SUBSYSTEM=="tty", KERNEL=="ttyACM[0-9]*", ATTRS{idVendor}=="2548", ATTRS{idProduct}=="1002", ACTION=="add", TAG+="systemd", ENV{SYSTEMD_WANTS}+="pulse8-cec-inputattach@%k.service"

And pulse8-cec-inputattach@%k.service is as follows:

^^^^ This line should read:

And /etc/systemd/system/pulse8-cec-inputattach@.service is as follows:

(I had a spurious %k in the name, and it didn't have the full path)

Regards,

	Hans

quoted

===============================================================
[Unit]
Description=inputattach for pulse8-cec device on %I

[Service]
Type=simple
ExecStart=/usr/local/bin/inputattach --pulse8-cec /dev/%I
KillMode=process
===============================================================

Signed-off-by: Hans Verkuil <redacted>
Tested-by: Hans Verkuil <redacted>
---
Change since the original RFC patch: don't call close() from the hangup() function,
instead only set the DEAD flag in hangup() instead of in close().
---

diff --git a/drivers/input/serio/serport.c b/drivers/input/serio/serport.c
index 9c927d3..4045e95 100644
--- a/drivers/input/serio/serport.c
+++ b/drivers/input/serio/serport.c

@@ -71,7 +71,6 @@ static void serport_serio_close(struct serio *serio)

 	spin_lock_irqsave(&serport->lock, flags);
 	clear_bit(SERPORT_ACTIVE, &serport->flags);
-	set_bit(SERPORT_DEAD, &serport->flags);
 	spin_unlock_irqrestore(&serport->lock, flags);

 	wake_up_interruptible(&serport->wait);

I think we should remove this line as well - the waiter is waiting on
SERPORT_DEAD bit, if we are not setting it we do not need to wake up the
waiter either.

I can fix it up on my side.

quoted

@@ -248,6 +247,19 @@ static long serport_ldisc_compat_ioctl(struct tty_struct *tty,
 }
 #endif

+static int serport_ldisc_hangup(struct tty_struct * tty)
+{
+	struct serport *serport = (struct serport *) tty->disc_data;
+	unsigned long flags;
+
+	spin_lock_irqsave(&serport->lock, flags);
+	set_bit(SERPORT_DEAD, &serport->flags);
+	spin_unlock_irqrestore(&serport->lock, flags);
+
+	wake_up_interruptible(&serport->wait);
+	return 0;
+}
+
 static void serport_ldisc_write_wakeup(struct tty_struct * tty)
 {
 	struct serport *serport = (struct serport *) tty->disc_data;

@@ -274,6 +286,7 @@ static struct tty_ldisc_ops serport_ldisc = {
 	.compat_ioctl =	serport_ldisc_compat_ioctl,
 #endif
 	.receive_buf =	serport_ldisc_receive,
+	.hangup =	serport_ldisc_hangup,
 	.write_wakeup =	serport_ldisc_write_wakeup
 };

Thanks.