[RFC PATCH] serio: add hangup support

[RFC PATCH] serio: add hangup support

[Hans Verkuil]

For the upcoming 4.8 kernel I made a driver for the Pulse-Eight USB CEC adapter.
This 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 exists as soon as I
unplug the USB device.

Is this the correct approach?

BTW, the new driver is found here:

https://git.linuxtv.org/media_tree.git/tree/drivers/staging/media/pulse8-cec

Regards,

	Hans

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>

---
diff --git a/drivers/input/serio/serport.c b/drivers/input/serio/serport.c
index 9c927d3..a615846 100644
--- a/drivers/input/serio/serport.c
+++ b/drivers/input/serio/serport.c
@@ -248,6 +248,14 @@ 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;
+
+	serport_serio_close(serport->serio);
+	return 0;
+}
+
 static void serport_ldisc_write_wakeup(struct tty_struct * tty)
 {
 	struct serport *serport = (struct serport *) tty->disc_data;
@@ -274,6 +282,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
 };

Re: [RFC PATCH] serio: add hangup support

[Dmitry Torokhov]

Hi Hans,

On Fri, Jul 15, 2016 at 01:27:21PM +0200, Hans Verkuil wrote:
> For the upcoming 4.8 kernel I made a driver for the Pulse-Eight USB CEC adapter.
> This 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 exists as soon as I
> unplug the USB device.
> 
> Is this the correct approach?
> 
> BTW, the new driver is found here:
> 
> https://git.linuxtv.org/media_tree.git/tree/drivers/staging/media/pulse8-cec
> 
> Regards,
> 
> 	Hans
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> 
> ---
> diff --git a/drivers/input/serio/serport.c b/drivers/input/serio/serport.c
> index 9c927d3..a615846 100644
> --- a/drivers/input/serio/serport.c
> +++ b/drivers/input/serio/serport.c
> @@ -248,6 +248,14 @@ 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;
> +
> +	serport_serio_close(serport->serio);

I see what you mean, but this is not quite correct. I think we should
make serport_serio_close() only reset the SERPORT_ACTIVE flag and have
serport_ldisc_hangup() actually do:

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

	wake_up_interruptible(&serport->wait);

i.e. if user (via device-driver - input core - evdev - userspace chain)
stops using serio port we should not kill inputattach instance right
then and there, but wait for the serial port device disconnect or
something else killing inputattach.

Vojtech, do you recall any of this code?

Thanks.

-- 
Dmitry

Re: [RFC PATCH] serio: add hangup support

[Hans Verkuil]

On 07/15/2016 06:31 PM, Dmitry Torokhov wrote:
> Hi Hans,
> 
> On Fri, Jul 15, 2016 at 01:27:21PM +0200, Hans Verkuil wrote:
>> For the upcoming 4.8 kernel I made a driver for the Pulse-Eight USB CEC adapter.
>> This 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 exists as soon as I
>> unplug the USB device.
>>
>> Is this the correct approach?
>>
>> BTW, the new driver is found here:
>>
>> https://git.linuxtv.org/media_tree.git/tree/drivers/staging/media/pulse8-cec
>>
>> Regards,
>>
>> 	Hans
>>
>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
>>
>> ---
>> diff --git a/drivers/input/serio/serport.c b/drivers/input/serio/serport.c
>> index 9c927d3..a615846 100644
>> --- a/drivers/input/serio/serport.c
>> +++ b/drivers/input/serio/serport.c
>> @@ -248,6 +248,14 @@ 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;
>> +
>> +	serport_serio_close(serport->serio);
> 
> I see what you mean, but this is not quite correct. I think we should
> make serport_serio_close() only reset the SERPORT_ACTIVE flag and have
> serport_ldisc_hangup() actually do:
> 
> 	spin_lock_irqsave(&serport->lock, flags);
> 	set_bit(SERPORT_DEAD, &serport->flags);
> 	spin_unlock_irqrestore(&serport->lock, flags);
> 
> 	wake_up_interruptible(&serport->wait);

I'm preparing a v2 of this patch, but I wonder if in this hangup code
I also need to clear the SERPORT_ACTIVE flag. Or is it guaranteed that
close() always precedes hangup()? In which case close() always clears that
flag.

Regards,

	Hans

> 
> i.e. if user (via device-driver - input core - evdev - userspace chain)
> stops using serio port we should not kill inputattach instance right
> then and there, but wait for the serial port device disconnect or
> something else killing inputattach.
> 
> Vojtech, do you recall any of this code?
> 
> Thanks.
> 

Re: [RFC PATCH] serio: add hangup support

[Dmitry Torokhov]

On Mon, Aug 01, 2016 at 03:43:32PM +0200, Hans Verkuil wrote:
> 
> 
> On 07/15/2016 06:31 PM, Dmitry Torokhov wrote:
> > Hi Hans,
> > 
> > On Fri, Jul 15, 2016 at 01:27:21PM +0200, Hans Verkuil wrote:
> >> For the upcoming 4.8 kernel I made a driver for the Pulse-Eight USB CEC adapter.
> >> This 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 exists as soon as I
> >> unplug the USB device.
> >>
> >> Is this the correct approach?
> >>
> >> BTW, the new driver is found here:
> >>
> >> https://git.linuxtv.org/media_tree.git/tree/drivers/staging/media/pulse8-cec
> >>
> >> Regards,
> >>
> >> 	Hans
> >>
> >> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> >>
> >> ---
> >> diff --git a/drivers/input/serio/serport.c b/drivers/input/serio/serport.c
> >> index 9c927d3..a615846 100644
> >> --- a/drivers/input/serio/serport.c
> >> +++ b/drivers/input/serio/serport.c
> >> @@ -248,6 +248,14 @@ 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;
> >> +
> >> +	serport_serio_close(serport->serio);
> > 
> > I see what you mean, but this is not quite correct. I think we should
> > make serport_serio_close() only reset the SERPORT_ACTIVE flag and have
> > serport_ldisc_hangup() actually do:
> > 
> > 	spin_lock_irqsave(&serport->lock, flags);
> > 	set_bit(SERPORT_DEAD, &serport->flags);
> > 	spin_unlock_irqrestore(&serport->lock, flags);
> > 
> > 	wake_up_interruptible(&serport->wait);
> 
> I'm preparing a v2 of this patch, but I wonder if in this hangup code
> I also need to clear the SERPORT_ACTIVE flag. Or is it guaranteed that
> close() always precedes hangup()? In which case close() always clears that
> flag.

No, we could get hangup and that would wake up the reader which will
cause de-registration of serio port. As part of that process
serport_serio_close() will be called (if serio port has been opened by
someone).

You should not need to clear "active" flag in hangup code.

Thanks.

-- 
Dmitry