mbox series

[v3,0/3] IR driver for USB-UIRT device

Message ID cover.1620304986.git.sean@mess.org
Headers show
Series IR driver for USB-UIRT device | expand

Message

Sean Young May 6, 2021, 12:44 p.m. UTC
This is a new rc-core driver for the USB-UIRT which you can see here
http://www.usbuirt.com/

This device is supported in lirc, via the usb serial kernel driver. This
driver is both for rc-core, which means it can use kernel/BPF decoding
ec. Also this implement is superior because it can:
 - support learning mode
 - setting transmit carrier
 - larger transmits using streaming tx command

Changes since v2:
 - Fixed race condition is disconnect
 - Removed superfluous kmalloc in short tx

Changes since v1:
 - Review comments from Oliver Neukum
 - Simplified wideband read function

Sean Young (3):
  USB: serial: move ftdi_sio.h into include directories
  media: rc: new driver for USB-UIRT device
  USB: serial: blacklist USB-UIRT when driver is selected

 drivers/media/rc/Kconfig                      |  11 +
 drivers/media/rc/Makefile                     |   1 +
 drivers/media/rc/uirt.c                       | 723 ++++++++++++++++++
 drivers/usb/serial/ftdi_sio.c                 |  10 +-
 .../serial => include/linux/usb}/ftdi_sio.h   |   0
 5 files changed, 744 insertions(+), 1 deletion(-)
 create mode 100644 drivers/media/rc/uirt.c
 rename {drivers/usb/serial => include/linux/usb}/ftdi_sio.h (100%)

Comments

Johan Hovold May 10, 2021, 8:15 a.m. UTC | #1
On Thu, May 06, 2021 at 01:44:52PM +0100, Sean Young wrote:
> This is a new rc-core driver for the USB-UIRT which you can see here

> http://www.usbuirt.com/

> 

> This device is supported in lirc, via the usb serial kernel driver. This

> driver is both for rc-core, which means it can use kernel/BPF decoding

> ec. Also this implement is superior because it can:

>  - support learning mode

>  - setting transmit carrier

>  - larger transmits using streaming tx command


This looks like something which should have been implemented as a
line-discipline or serdev driver instead of reimplementing a minimal
on-off ftdi driver and tying it closely to the RC subsystem.

Why can't you just add support for the above features to whatever
subsystem is managing this device today?

Serdev still doesn't support hotplugging unfortunately so that route may
take a bit more work.

Johan
Sean Young May 11, 2021, 10:32 a.m. UTC | #2
On Mon, May 10, 2021 at 10:15:14AM +0200, Johan Hovold wrote:
> On Thu, May 06, 2021 at 01:44:52PM +0100, Sean Young wrote:

> > This is a new rc-core driver for the USB-UIRT which you can see here

> > http://www.usbuirt.com/

> > 

> > This device is supported in lirc, via the usb serial kernel driver. This

> > driver is both for rc-core, which means it can use kernel/BPF decoding

> > ec. Also this implement is superior because it can:

> >  - support learning mode

> >  - setting transmit carrier

> >  - larger transmits using streaming tx command

> 

> This looks like something which should have been implemented as a

> line-discipline or serdev driver instead of reimplementing a minimal

> on-off ftdi driver and tying it closely to the RC subsystem.


The device is an infrared device, I'm not sure what it is lost by
doing it this way. The "minimal on-off ftdi driver" is super trivial.

> Why can't you just add support for the above features to whatever

> subsystem is managing this device today?

> 

> Serdev still doesn't support hotplugging unfortunately so that route may

> take a bit more work.


There seems to be at least three ways of attaching drivers to serial
devices: serio, serdev, and line-discipline. All seem to have limitations,
as you say none of them provide a way of hotplugging devices without
user-space attaching them through an ioctl or so.

If you want to go down this route, then ideally you'd want a quirk on
fdti saying "attach usb-uirt serdev device to this pid/vid". Considering
module dependencies, I don't know how that could work without again
userspace getting involved.

Getting userspace involved seem like a big song and dance because the
device uses an fdti device, even though it's not a serial port because
it's hardwired for infrared functions, no db9 connector in sight.


Sean
Johan Hovold May 14, 2021, 11:16 a.m. UTC | #3
On Tue, May 11, 2021 at 11:32:19AM +0100, Sean Young wrote:
> On Mon, May 10, 2021 at 10:15:14AM +0200, Johan Hovold wrote:

> > On Thu, May 06, 2021 at 01:44:52PM +0100, Sean Young wrote:

> > > This is a new rc-core driver for the USB-UIRT which you can see here

> > > http://www.usbuirt.com/

> > > 

> > > This device is supported in lirc, via the usb serial kernel driver. This

> > > driver is both for rc-core, which means it can use kernel/BPF decoding

> > > ec. Also this implement is superior because it can:

> > >  - support learning mode

> > >  - setting transmit carrier

> > >  - larger transmits using streaming tx command

> > 

> > This looks like something which should have been implemented as a

> > line-discipline or serdev driver instead of reimplementing a minimal

> > on-off ftdi driver and tying it closely to the RC subsystem.

> 

> The device is an infrared device, I'm not sure what it is lost by

> doing it this way. The "minimal on-off ftdi driver" is super trivial.


It's still code duplication (and I meant to say "one-off" above").

What is preventing you from supporting the above functionality through
lirc?

> > Why can't you just add support for the above features to whatever

> > subsystem is managing this device today?

> > 

> > Serdev still doesn't support hotplugging unfortunately so that route may

> > take a bit more work.

> 

> There seems to be at least three ways of attaching drivers to serial

> devices: serio, serdev, and line-discipline. All seem to have limitations,

> as you say none of them provide a way of hotplugging devices without

> user-space attaching them through an ioctl or so.


serio is also a line-discipline driver, which unlike serdev needs to be
set up by user space.

And the problem with serdev is that it does not (yet) support
hotplugging (specifically hangups) so it can't be enabled for USB serial
just yet.

> If you want to go down this route, then ideally you'd want a quirk on

> fdti saying "attach usb-uirt serdev device to this pid/vid". Considering

> module dependencies, I don't know how that could work without again

> userspace getting involved.


We'd just reuse or add another matching mechanism for USB devices. This
can be handled without user-space interaction just fine as long as you
have a dedicated device id as you do here.

> Getting userspace involved seem like a big song and dance because the

> device uses an fdti device, even though it's not a serial port because

> it's hardwired for infrared functions, no db9 connector in sight.


Far from every USB serial device have a db9 connector (e.g. modems,
barcode scanners, development board consoles, etc.) and you still have a
UART in your device.

In principle reimplementing a one-off ftdi driver is wrong but since
parts of the infrastructure needed to avoid this is still missing it may
be acceptable, especially if you can't get this to work with lirc.

Johan
Sean Young May 15, 2021, 9:22 a.m. UTC | #4
On Fri, May 14, 2021 at 01:16:47PM +0200, Johan Hovold wrote:
> On Tue, May 11, 2021 at 11:32:19AM +0100, Sean Young wrote:
> > On Mon, May 10, 2021 at 10:15:14AM +0200, Johan Hovold wrote:
> > > On Thu, May 06, 2021 at 01:44:52PM +0100, Sean Young wrote:
> > > > This is a new rc-core driver for the USB-UIRT which you can see here
> > > > http://www.usbuirt.com/
> > > > 
> > > > This device is supported in lirc, via the usb serial kernel driver. This
> > > > driver is both for rc-core, which means it can use kernel/BPF decoding
> > > > ec. Also this implement is superior because it can:
> > > >  - support learning mode
> > > >  - setting transmit carrier
> > > >  - larger transmits using streaming tx command
> > > 
> > > This looks like something which should have been implemented as a
> > > line-discipline or serdev driver instead of reimplementing a minimal
> > > on-off ftdi driver and tying it closely to the RC subsystem.
> > 
> > The device is an infrared device, I'm not sure what it is lost by
> > doing it this way. The "minimal on-off ftdi driver" is super trivial.
> 
> It's still code duplication (and I meant to say "one-off" above").
> 
> What is preventing you from supporting the above functionality through
> lirc?

I guess you mean the userspace lirc daemon, as opposed to the /dev/lirc
chardev. If you use the lirc daemon, you don't use rc-core which comes with
IR decoding using BPF IR decoding or in-kernel decoders, automatic setup of
rc keymaps via udev. None of the modern ir-ctl/ir-keytable tooling will
work, including the IRP encoder/BPF compiler I'm working on (very slowly).

The other nice thing is that IR TX feeds data from an urb interrupt handler,
so you don't need to rely userspace being scheduled quickly enough to feed
more data before the device runs out.

> 
> > > Why can't you just add support for the above features to whatever
> > > subsystem is managing this device today?
> > > 
> > > Serdev still doesn't support hotplugging unfortunately so that route may
> > > take a bit more work.
> > 
> > There seems to be at least three ways of attaching drivers to serial
> > devices: serio, serdev, and line-discipline. All seem to have limitations,
> > as you say none of them provide a way of hotplugging devices without
> > user-space attaching them through an ioctl or so.
> 
> serio is also a line-discipline driver, which unlike serdev needs to be
> set up by user space.

serio is unusable for this case. serio does not allow more than one byte
to be written nor does it have callbacks for CTS readiness.

> And the problem with serdev is that it does not (yet) support
> hotplugging (specifically hangups) so it can't be enabled for USB serial
> just yet.
> 
> > If you want to go down this route, then ideally you'd want a quirk on
> > fdti saying "attach usb-uirt serdev device to this pid/vid". Considering
> > module dependencies, I don't know how that could work without again
> > userspace getting involved.
> 
> We'd just reuse or add another matching mechanism for USB devices. This
> can be handled without user-space interaction just fine as long as you
> have a dedicated device id as you do here.

Right, ok. I don't quite follow what you have in mind. If at all possible
keep me in the loop for any patches for this, I'm happy to test/re-write
this driver and the drivers/media/rc/ir_toy.c driver on top of any such
patches.

There are a bunch old serial usb device IR devices and even older non-usb
serial devices that would be nice to have supported, if anyone is still
using them.

> > Getting userspace involved seem like a big song and dance because the
> > device uses an fdti device, even though it's not a serial port because
> > it's hardwired for infrared functions, no db9 connector in sight.
> 
> Far from every USB serial device have a db9 connector (e.g. modems,
> barcode scanners, development board consoles, etc.) and you still have a
> UART in your device.
> 
> In principle reimplementing a one-off ftdi driver is wrong but since
> parts of the infrastructure needed to avoid this is still missing it may
> be acceptable, especially if you can't get this to work with lirc.

Thanks -- that's a good compromise.


Sean
Johan Hovold May 17, 2021, 9:30 a.m. UTC | #5
On Sat, May 15, 2021 at 10:22:26AM +0100, Sean Young wrote:
> On Fri, May 14, 2021 at 01:16:47PM +0200, Johan Hovold wrote:
> > On Tue, May 11, 2021 at 11:32:19AM +0100, Sean Young wrote:
> > > On Mon, May 10, 2021 at 10:15:14AM +0200, Johan Hovold wrote:
> > > > On Thu, May 06, 2021 at 01:44:52PM +0100, Sean Young wrote:
> > > > > This is a new rc-core driver for the USB-UIRT which you can see here
> > > > > http://www.usbuirt.com/
> > > > > 
> > > > > This device is supported in lirc, via the usb serial kernel driver. This
> > > > > driver is both for rc-core, which means it can use kernel/BPF decoding
> > > > > ec. Also this implement is superior because it can:
> > > > >  - support learning mode
> > > > >  - setting transmit carrier
> > > > >  - larger transmits using streaming tx command
> > > > 
> > > > This looks like something which should have been implemented as a
> > > > line-discipline or serdev driver instead of reimplementing a minimal
> > > > on-off ftdi driver and tying it closely to the RC subsystem.
> > > 
> > > The device is an infrared device, I'm not sure what it is lost by
> > > doing it this way. The "minimal on-off ftdi driver" is super trivial.
> > 
> > It's still code duplication (and I meant to say "one-off" above").
> > 
> > What is preventing you from supporting the above functionality through
> > lirc?
> 
> I guess you mean the userspace lirc daemon, as opposed to the /dev/lirc
> chardev. If you use the lirc daemon, you don't use rc-core which comes with
> IR decoding using BPF IR decoding or in-kernel decoders, automatic setup of
> rc keymaps via udev. None of the modern ir-ctl/ir-keytable tooling will
> work, including the IRP encoder/BPF compiler I'm working on (very slowly).

Ok, but apart from BPF that sound like other stuff and not the three
items you list above? Is there anything preventing those items from
being implemented in user space?

Obviously a user-space implementation (e.g. accessing the device through
/dev/ttyUSB0) is not going to be able to use in-kernel RC functionality.
For that you'd need to use either a line-discipline or serdev driver,
that is, a kernel driver, but not everything has to live in the kernel.

> The other nice thing is that IR TX feeds data from an urb interrupt handler,
> so you don't need to rely userspace being scheduled quickly enough to feed
> more data before the device runs out.

The tty layer and tty drivers provide write buffering so that need not
be an issue.

> > > > Why can't you just add support for the above features to whatever
> > > > subsystem is managing this device today?
> > > > 
> > > > Serdev still doesn't support hotplugging unfortunately so that route may
> > > > take a bit more work.
> > > 
> > > There seems to be at least three ways of attaching drivers to serial
> > > devices: serio, serdev, and line-discipline. All seem to have limitations,
> > > as you say none of them provide a way of hotplugging devices without
> > > user-space attaching them through an ioctl or so.
> > 
> > serio is also a line-discipline driver, which unlike serdev needs to be
> > set up by user space.
> 
> serio is unusable for this case. serio does not allow more than one byte
> to be written nor does it have callbacks for CTS readiness.

Ok, but you could still implement this as an rc-core line-discipline or
serdev driver. And when you use hardware flow control as you do here,
you shouldn't need any callbacks for CTS events either, right?
 
> > And the problem with serdev is that it does not (yet) support
> > hotplugging (specifically hangups) so it can't be enabled for USB serial
> > just yet.
> > 
> > > If you want to go down this route, then ideally you'd want a quirk on
> > > fdti saying "attach usb-uirt serdev device to this pid/vid". Considering
> > > module dependencies, I don't know how that could work without again
> > > userspace getting involved.
> > 
> > We'd just reuse or add another matching mechanism for USB devices. This
> > can be handled without user-space interaction just fine as long as you
> > have a dedicated device id as you do here.
> 
> Right, ok. I don't quite follow what you have in mind. If at all possible
> keep me in the loop for any patches for this, I'm happy to test/re-write
> this driver and the drivers/media/rc/ir_toy.c driver on top of any such
> patches.

Thanks for that pointer. Judging from a quick look, the new driver
appears to based on this one. By abstracting the serial interface bits
in a generic RC serdev/ldisc driver you may be able reuse more code,
even if I'm not in a position to judge how much of the IR protocol bits
that can be shared.

> There are a bunch old serial usb device IR devices and even older non-usb
> serial devices that would be nice to have supported, if anyone is still
> using them.

I noticed that drivers/media/rc/serial_ir.c also appears to use a
similar approach of implementing a minimal one-off serial (8250?) driver
and tying it closely to RC core. This one might also benefit from using
the standard serial drivers for the transport and having an RC layer on
top.

Currently it appears to use module-parameters for configuration instead
of devicetree or some to-be-implemented interface for instantiating
serdev devices from user space.

Johan
Sean Young May 17, 2021, 10:35 a.m. UTC | #6
On Mon, May 17, 2021 at 11:30:39AM +0200, Johan Hovold wrote:
> On Sat, May 15, 2021 at 10:22:26AM +0100, Sean Young wrote:

> > On Fri, May 14, 2021 at 01:16:47PM +0200, Johan Hovold wrote:

> > > On Tue, May 11, 2021 at 11:32:19AM +0100, Sean Young wrote:

> > > > On Mon, May 10, 2021 at 10:15:14AM +0200, Johan Hovold wrote:

> > > > > On Thu, May 06, 2021 at 01:44:52PM +0100, Sean Young wrote:

> > > > > > This is a new rc-core driver for the USB-UIRT which you can see here

> > > > > > http://www.usbuirt.com/

> > > > > > 

> > > > > > This device is supported in lirc, via the usb serial kernel driver. This

> > > > > > driver is both for rc-core, which means it can use kernel/BPF decoding

> > > > > > ec. Also this implement is superior because it can:

> > > > > >  - support learning mode

> > > > > >  - setting transmit carrier

> > > > > >  - larger transmits using streaming tx command

> > > > > 

> > > > > This looks like something which should have been implemented as a

> > > > > line-discipline or serdev driver instead of reimplementing a minimal

> > > > > on-off ftdi driver and tying it closely to the RC subsystem.

> > > > 

> > > > The device is an infrared device, I'm not sure what it is lost by

> > > > doing it this way. The "minimal on-off ftdi driver" is super trivial.

> > > 

> > > It's still code duplication (and I meant to say "one-off" above").

> > > 

> > > What is preventing you from supporting the above functionality through

> > > lirc?

> > 

> > I guess you mean the userspace lirc daemon, as opposed to the /dev/lirc

> > chardev. If you use the lirc daemon, you don't use rc-core which comes with

> > IR decoding using BPF IR decoding or in-kernel decoders, automatic setup of

> > rc keymaps via udev. None of the modern ir-ctl/ir-keytable tooling will

> > work, including the IRP encoder/BPF compiler I'm working on (very slowly).

> 

> Ok, but apart from BPF that sound like other stuff and not the three

> items you list above? Is there anything preventing those items from

> being implemented in user space?


Well, after IR is decoded, you want to send decoded scancodes/key codes
to any input device, so your remote works just like any input device.

> Obviously a user-space implementation (e.g. accessing the device through

> /dev/ttyUSB0) is not going to be able to use in-kernel RC functionality.

> For that you'd need to use either a line-discipline or serdev driver,

> that is, a kernel driver, but not everything has to live in the kernel.


No, of course not. A lot of kernel functionality could live in user space,
for sure. But it doesn't.

Even if the input problem can be resolved, the lirc daemon is pretty outdated.
All the existing functionality in-kernel would have to be re-written for
userspace, and it would be total duplication of code, which you do not like.
You end up with a userspace implementation and a kernel space implementation.

There are many other IR devices that can be controlled through libusb in
userspace, which could work entirely in userspace. Same for i2c IR
devices, those could work entirely from userspace too. I don't know what
the state is of pci userspace drivers, but there certainly have been patches
for that; the line is not so clear.

I do think that the monolithic approach to kernels necessarily invokes
discussions like these, and there are no perfect answers. 

> > The other nice thing is that IR TX feeds data from an urb interrupt handler,

> > so you don't need to rely userspace being scheduled quickly enough to feed

> > more data before the device runs out.

> 

> The tty layer and tty drivers provide write buffering so that need not

> be an issue.

 
Ok. I don't know what the size of the write buffer is or what the maximum
TX size is; the IR device supports infinite streaming.

> > > > > Why can't you just add support for the above features to whatever

> > > > > subsystem is managing this device today?

> > > > > 

> > > > > Serdev still doesn't support hotplugging unfortunately so that route may

> > > > > take a bit more work.

> > > > 

> > > > There seems to be at least three ways of attaching drivers to serial

> > > > devices: serio, serdev, and line-discipline. All seem to have limitations,

> > > > as you say none of them provide a way of hotplugging devices without

> > > > user-space attaching them through an ioctl or so.

> > > 

> > > serio is also a line-discipline driver, which unlike serdev needs to be

> > > set up by user space.

> > 

> > serio is unusable for this case. serio does not allow more than one byte

> > to be written nor does it have callbacks for CTS readiness.

> 

> Ok, but you could still implement this as an rc-core line-discipline or

> serdev driver. And when you use hardware flow control as you do here,

> you shouldn't need any callbacks for CTS events either, right?


I like the idea of serdev, but like you say it's not ready yet because of
absence of hotplugging. See above about streaming.

> > > And the problem with serdev is that it does not (yet) support

> > > hotplugging (specifically hangups) so it can't be enabled for USB serial

> > > just yet.

> > > 

> > > > If you want to go down this route, then ideally you'd want a quirk on

> > > > fdti saying "attach usb-uirt serdev device to this pid/vid". Considering

> > > > module dependencies, I don't know how that could work without again

> > > > userspace getting involved.

> > > 

> > > We'd just reuse or add another matching mechanism for USB devices. This

> > > can be handled without user-space interaction just fine as long as you

> > > have a dedicated device id as you do here.

> > 

> > Right, ok. I don't quite follow what you have in mind. If at all possible

> > keep me in the loop for any patches for this, I'm happy to test/re-write

> > this driver and the drivers/media/rc/ir_toy.c driver on top of any such

> > patches.

> 

> Thanks for that pointer. Judging from a quick look, the new driver

> appears to based on this one. By abstracting the serial interface bits

> in a generic RC serdev/ldisc driver you may be able reuse more code,

> even if I'm not in a position to judge how much of the IR protocol bits

> that can be shared.


Yes, I agree. Once hotplugging is in place. If you have patches for that,
please CC me and I can see if will work for IR drivers.

> > There are a bunch old serial usb device IR devices and even older non-usb

> > serial devices that would be nice to have supported, if anyone is still

> > using them.

> 

> I noticed that drivers/media/rc/serial_ir.c also appears to use a

> similar approach of implementing a minimal one-off serial (8250?) driver

> and tying it closely to RC core. This one might also benefit from using

> the standard serial drivers for the transport and having an RC layer on

> top.

> 

> Currently it appears to use module-parameters for configuration instead

> of devicetree or some to-be-implemented interface for instantiating

> serdev devices from user space.


serial_ir.c (called lirc_serial in the past) is a bit special. It uses
an IR sensor connected directly to DCD and an output led connected to DTR,
(depending on the configuration used). I don't think this can be done with
a standard serial port driver. If it is possible, I'd like to know.

It's a bit of an insane way of doing things, but it's super cheap to build.


Sean
Sean Young May 17, 2021, 12:35 p.m. UTC | #7
On Mon, May 17, 2021 at 11:35:22AM +0100, Sean Young wrote:
> On Mon, May 17, 2021 at 11:30:39AM +0200, Johan Hovold wrote:

> > On Sat, May 15, 2021 at 10:22:26AM +0100, Sean Young wrote:

> > > On Fri, May 14, 2021 at 01:16:47PM +0200, Johan Hovold wrote:

> > > > On Tue, May 11, 2021 at 11:32:19AM +0100, Sean Young wrote:

> > > > > On Mon, May 10, 2021 at 10:15:14AM +0200, Johan Hovold wrote:

> > > > > > On Thu, May 06, 2021 at 01:44:52PM +0100, Sean Young wrote:

> > > > > > > This is a new rc-core driver for the USB-UIRT which you can see here

> > > > > > > http://www.usbuirt.com/

> > > > > > > 

> > > > > > > This device is supported in lirc, via the usb serial kernel driver. This

> > > > > > > driver is both for rc-core, which means it can use kernel/BPF decoding

> > > > > > > ec. Also this implement is superior because it can:

> > > > > > >  - support learning mode

> > > > > > >  - setting transmit carrier

> > > > > > >  - larger transmits using streaming tx command

> > > > > > 

> > > > > > This looks like something which should have been implemented as a

> > > > > > line-discipline or serdev driver instead of reimplementing a minimal

> > > > > > on-off ftdi driver and tying it closely to the RC subsystem.

> > > > > 

> > > > > The device is an infrared device, I'm not sure what it is lost by

> > > > > doing it this way. The "minimal on-off ftdi driver" is super trivial.

> > > > 

> > > > It's still code duplication (and I meant to say "one-off" above").

> > > > 

> > > > What is preventing you from supporting the above functionality through

> > > > lirc?

> > > 

> > > I guess you mean the userspace lirc daemon, as opposed to the /dev/lirc

> > > chardev. If you use the lirc daemon, you don't use rc-core which comes with

> > > IR decoding using BPF IR decoding or in-kernel decoders, automatic setup of

> > > rc keymaps via udev. None of the modern ir-ctl/ir-keytable tooling will

> > > work, including the IRP encoder/BPF compiler I'm working on (very slowly).

> > 

> > Ok, but apart from BPF that sound like other stuff and not the three

> > items you list above? Is there anything preventing those items from

> > being implemented in user space?

> 

> Well, after IR is decoded, you want to send decoded scancodes/key codes

> to any input device, so your remote works just like any input device.


There is another advantage. IR decoding in userspace involves a lot more
context switches/scheduling, and it can feel laggy when the cpu is under
load (e.g. video decoding on the CPU). When you press pause/play/stop or
so you expect the response the instantatiously. A 100ms delay is noticable.

Alternatively the key-up events get delayed and you end up with multiple
un-intended button repeats. None of this happens with kernel decoding and
it feels very snappy.


Sean
Johan Hovold May 20, 2021, 1:31 p.m. UTC | #8
On Mon, May 17, 2021 at 11:35:22AM +0100, Sean Young wrote:
> On Mon, May 17, 2021 at 11:30:39AM +0200, Johan Hovold wrote:

> > On Sat, May 15, 2021 at 10:22:26AM +0100, Sean Young wrote:

> > > On Fri, May 14, 2021 at 01:16:47PM +0200, Johan Hovold wrote:

> > > > On Tue, May 11, 2021 at 11:32:19AM +0100, Sean Young wrote:

> > > > > On Mon, May 10, 2021 at 10:15:14AM +0200, Johan Hovold wrote:

> > > > > > On Thu, May 06, 2021 at 01:44:52PM +0100, Sean Young wrote:

> > > > > > > This is a new rc-core driver for the USB-UIRT which you can see here

> > > > > > > http://www.usbuirt.com/

> > > > > > > 

> > > > > > > This device is supported in lirc, via the usb serial kernel driver. This

> > > > > > > driver is both for rc-core, which means it can use kernel/BPF decoding

> > > > > > > ec. Also this implement is superior because it can:

> > > > > > >  - support learning mode

> > > > > > >  - setting transmit carrier

> > > > > > >  - larger transmits using streaming tx command

> > > > > > 

> > > > > > This looks like something which should have been implemented as a

> > > > > > line-discipline or serdev driver instead of reimplementing a minimal

> > > > > > on-off ftdi driver and tying it closely to the RC subsystem.

> > > > > 

> > > > > The device is an infrared device, I'm not sure what it is lost by

> > > > > doing it this way. The "minimal on-off ftdi driver" is super trivial.

> > > > 

> > > > It's still code duplication (and I meant to say "one-off" above").

> > > > 

> > > > What is preventing you from supporting the above functionality through

> > > > lirc?

> > > 

> > > I guess you mean the userspace lirc daemon, as opposed to the /dev/lirc

> > > chardev. If you use the lirc daemon, you don't use rc-core which comes with

> > > IR decoding using BPF IR decoding or in-kernel decoders, automatic setup of

> > > rc keymaps via udev. None of the modern ir-ctl/ir-keytable tooling will

> > > work, including the IRP encoder/BPF compiler I'm working on (very slowly).

> > 

> > Ok, but apart from BPF that sound like other stuff and not the three

> > items you list above? Is there anything preventing those items from

> > being implemented in user space?

> 

> Well, after IR is decoded, you want to send decoded scancodes/key codes

> to any input device, so your remote works just like any input device.


Isn't that already handled by lircd using uinput?
 
> > Obviously a user-space implementation (e.g. accessing the device through

> > /dev/ttyUSB0) is not going to be able to use in-kernel RC functionality.

> > For that you'd need to use either a line-discipline or serdev driver,

> > that is, a kernel driver, but not everything has to live in the kernel.

> 

> No, of course not. A lot of kernel functionality could live in user space,

> for sure. But it doesn't.

> 

> Even if the input problem can be resolved, the lirc daemon is pretty outdated.

> All the existing functionality in-kernel would have to be re-written for

> userspace, and it would be total duplication of code, which you do not like.

> You end up with a userspace implementation and a kernel space implementation.

> 

> There are many other IR devices that can be controlled through libusb in

> userspace, which could work entirely in userspace. Same for i2c IR

> devices, those could work entirely from userspace too. I don't know what

> the state is of pci userspace drivers, but there certainly have been patches

> for that; the line is not so clear.

> 

> I do think that the monolithic approach to kernels necessarily invokes

> discussions like these, and there are no perfect answers. 


I hear you, but we still need to have those discussions from time to
time to make sure our architecture is sane. One of the problems today
with the kernel development process appears to be that too few questions
are asked. If it builds, ship it...

In this case the device in question can already be handled in user space
by lirqd (at least to some degree) and we have infrastructure for
writing in-kernel serial client drivers (i.e. ldisc/serdev). While
neither option may support everything we need today, adding further
one-off serial-device + client combo drivers is still a step in the
wrong direction.

But I think I've got that point across by now.

> > > The other nice thing is that IR TX feeds data from an urb interrupt handler,

> > > so you don't need to rely userspace being scheduled quickly enough to feed

> > > more data before the device runs out.

> > 

> > The tty layer and tty drivers provide write buffering so that need not

> > be an issue.

>  

> Ok. I don't know what the size of the write buffer is or what the maximum

> TX size is; the IR device supports infinite streaming.


Our tty drivers typically have at least a 4k buffer for transmission.
Surely that should be enough for a remote control but perhaps there are
other more demanding applications?

> > Thanks for that pointer. Judging from a quick look, the new driver

> > appears to based on this one. By abstracting the serial interface bits

> > in a generic RC serdev/ldisc driver you may be able reuse more code,

> > even if I'm not in a position to judge how much of the IR protocol bits

> > that can be shared.

> 

> Yes, I agree. Once hotplugging is in place. If you have patches for that,

> please CC me and I can see if will work for IR drivers.


Let's hope someone steps up to fund that work then.

> > > There are a bunch old serial usb device IR devices and even older non-usb

> > > serial devices that would be nice to have supported, if anyone is still

> > > using them.

> > 

> > I noticed that drivers/media/rc/serial_ir.c also appears to use a

> > similar approach of implementing a minimal one-off serial (8250?) driver

> > and tying it closely to RC core. This one might also benefit from using

> > the standard serial drivers for the transport and having an RC layer on

> > top.

> > 

> > Currently it appears to use module-parameters for configuration instead

> > of devicetree or some to-be-implemented interface for instantiating

> > serdev devices from user space.

> 

> serial_ir.c (called lirc_serial in the past) is a bit special. It uses

> an IR sensor connected directly to DCD and an output led connected to DTR,

> (depending on the configuration used). I don't think this can be done with

> a standard serial port driver. If it is possible, I'd like to know.

> 

> It's a bit of an insane way of doing things, but it's super cheap to build.


Heh, ok. Perhaps we'll just have to live with this one then.

Johan
Johan Hovold May 20, 2021, 1:40 p.m. UTC | #9
On Mon, May 17, 2021 at 01:35:09PM +0100, Sean Young wrote:
> On Mon, May 17, 2021 at 11:35:22AM +0100, Sean Young wrote:

> > On Mon, May 17, 2021 at 11:30:39AM +0200, Johan Hovold wrote:


> > > Ok, but apart from BPF that sound like other stuff and not the three

> > > items you list above? Is there anything preventing those items from

> > > being implemented in user space?

> > 

> > Well, after IR is decoded, you want to send decoded scancodes/key codes

> > to any input device, so your remote works just like any input device.

> 

> There is another advantage. IR decoding in userspace involves a lot more

> context switches/scheduling, and it can feel laggy when the cpu is under

> load (e.g. video decoding on the CPU). When you press pause/play/stop or

> so you expect the response the instantatiously. A 100ms delay is noticable.


RT scheduling? Sounds like you should be able to handle this way faster
than 100 ms.

> Alternatively the key-up events get delayed and you end up with multiple

> un-intended button repeats. None of this happens with kernel decoding and

> it feels very snappy.


Yeah, perhaps it's best handled in-kernel, but it seems we should be
able to handle a simple key press within 20 ms or whatever the critical
latency is here using either option (kernel or user-space driver).

Johan
Sean Young May 21, 2021, 11:39 a.m. UTC | #10
On Thu, May 20, 2021 at 03:31:34PM +0200, Johan Hovold wrote:
> On Mon, May 17, 2021 at 11:35:22AM +0100, Sean Young wrote:
> > On Mon, May 17, 2021 at 11:30:39AM +0200, Johan Hovold wrote:
> > > On Sat, May 15, 2021 at 10:22:26AM +0100, Sean Young wrote:
> > > > The other nice thing is that IR TX feeds data from an urb interrupt handler,
> > > > so you don't need to rely userspace being scheduled quickly enough to feed
> > > > more data before the device runs out.
> > > 
> > > The tty layer and tty drivers provide write buffering so that need not
> > > be an issue.
> >  
> > Ok. I don't know what the size of the write buffer is or what the maximum
> > TX size is; the IR device supports infinite streaming.
> 
> Our tty drivers typically have at least a 4k buffer for transmission.
> Surely that should be enough for a remote control but perhaps there are
> other more demanding applications?

That's more than enough. The most demanding consumer IR I know of, is
an air conditioner which encodes temperature and a few others things. That's
a maximum of 439 pulse/spaces which should into 1k.
> 
> > > Thanks for that pointer. Judging from a quick look, the new driver
> > > appears to based on this one. By abstracting the serial interface bits
> > > in a generic RC serdev/ldisc driver you may be able reuse more code,
> > > even if I'm not in a position to judge how much of the IR protocol bits
> > > that can be shared.
> > 
> > Yes, I agree. Once hotplugging is in place. If you have patches for that,
> > please CC me and I can see if will work for IR drivers.
> 
> Let's hope someone steps up to fund that work then.

I'm just a volunteer. I've literally never heard anything about kernel work
being funded by anyone.

Would you mind giving a brief summary what is needed to implement hotplugging
for serdev? I get the impression it's not a lot of work, but I'm probably
missing something obvious.


Sean
Johan Hovold June 23, 2021, 12:48 p.m. UTC | #11
Sorry about the late reply on this one.

On Fri, May 21, 2021 at 12:39:54PM +0100, Sean Young wrote:
> On Thu, May 20, 2021 at 03:31:34PM +0200, Johan Hovold wrote:

> > On Mon, May 17, 2021 at 11:35:22AM +0100, Sean Young wrote:

> > > On Mon, May 17, 2021 at 11:30:39AM +0200, Johan Hovold wrote:


> > > > Thanks for that pointer. Judging from a quick look, the new driver

> > > > appears to based on this one. By abstracting the serial interface bits

> > > > in a generic RC serdev/ldisc driver you may be able reuse more code,

> > > > even if I'm not in a position to judge how much of the IR protocol bits

> > > > that can be shared.

> > > 

> > > Yes, I agree. Once hotplugging is in place. If you have patches for that,

> > > please CC me and I can see if will work for IR drivers.

> > 

> > Let's hope someone steps up to fund that work then.

> 

> I'm just a volunteer. I've literally never heard anything about kernel work

> being funded by anyone.


Someone always pays whether it's a client, an employer or you yourself
with your spare time.

> Would you mind giving a brief summary what is needed to implement hotplugging

> for serdev? I get the impression it's not a lot of work, but I'm probably

> missing something obvious.


First, it's the matching bits we already touched on where we would like
to be able to use something like devicetree overlays to avoid rolling a
new mechanism for every bus. But devicetree overlays has its issues
currently (e.g. theres no user-space interface for providing them and
last time I checked they could not be reverted).

Second, serdev does not use the file abstraction and does not support
hangups which is used to implement tty hotplugging (e.g. by signalling
the consumer and making all file operations become noops after a
disconnect).

This would take some thinking-through to get right, and hopefully it can
be done without having to update every current serdev driver.

Retrofitting serdev into the tty layer wasn't painless and broke things
here and there. Supporting hotplugging should be doable but it's not as
straight-forward as it may sound.

Johan
Sean Young June 24, 2021, 9:13 a.m. UTC | #12
On Wed, Jun 23, 2021 at 03:10:20PM +0200, Johan Hovold wrote:
> Sorry about the late reply on this one too.

> 

> On Tue, May 25, 2021 at 02:25:49PM +0200, Oliver Neukum wrote:

> > Am Donnerstag, den 20.05.2021, 15:31 +0200 schrieb Johan Hovold:

> 

> > > Isn't that already handled by lircd using uinput?

> > 

> > The problem with that reasoning, though it is true, is

> > 

> > 1) We would need to remove a lot of subsystems if we took that

> > to the logical conclusion. 

> 

> Removing code is always nice. ;)


So rather than adding hotplug to serdev, we should remove line-discipline,
serdev, and serio and all its drivers from the kernel? This is taking
your own argument and applying it your code.

> > 2) It makes runtime PM much harder

> 

> Possibly, depends on the bus and device.

> 

> > 3) We end up with two classes of LIRC devices

> 

> We already do, right? That's kind of my point since we have lircd

> supporting uinput.


This is not an either-or situation, lircd is the "old" solution which is
slowing being supplanted with rc-core. All the new keymaps are rc-core and
do not work with lircd. The new rc-core tooling (in the v4l-utils package) 
does not work with lircd. lircd hasn't had any real patches merged for years
now.

There is whole new tooling in the works for rc-core which is not compatible
with lircd.

> > > I hear you, but we still need to have those discussions from time to

> > > time to make sure our architecture is sane. One of the problems today

> > > with the kernel development process appears to be that too few

> > > questions

> > > are asked. If it builds, ship it...

> > 

> > Indeed, so, could we force a line discipline on a device on the kernel

> > level? Code duplication is bad.

> 

> Not sure I understand what you have mind here. serdev is sort of a

> line-discipline which we'd "force" on a device if there's a matching

> description in devicetree, while line disciplines always need to be

> instantiated by user space. Or are you referring to ldisc/serdev code

> reuse?


I am pretty sure Oliver is suggesting that all ldisc/serdev code in
the kernel is duplication of code which can be done in userspace, by your
own argument.

> > > But I think I've got that point across by now.

> > 

> > Yes and and we need to think about the conclusion we draw from

> > that point. It seems to me that an architecture that pushes data

> > through the whole tty layer into a demon, then through uinput

> > is definitely not elegant.

> 

> The elegant answer is serdev, but it does not yet support the features

> needed in this case (i.e. hotplugging).

> 

> Since we already support user-space drivers for these devices, I see

> nothing wrong with implementing support for another one in user space

> unless there are strong reasons against doing so (e.g. performance,

> pm or usability). But if uinput works then great, we're done.


As discussed lircd has terrible latency, and lircd is out of date and
unmaintained and does not work with modern tooling and keymaps.

Also essentially your saying that any input device that connects to a
serial port should be done in user space. There are a ton of kernel
drivers doing exactly that, and that is why serio exists in the first
place.


Sean
Johan Hovold June 24, 2021, 9:41 a.m. UTC | #13
On Thu, Jun 24, 2021 at 10:13:49AM +0100, Sean Young wrote:
> On Wed, Jun 23, 2021 at 03:10:20PM +0200, Johan Hovold wrote:

> > Sorry about the late reply on this one too.

> > 

> > On Tue, May 25, 2021 at 02:25:49PM +0200, Oliver Neukum wrote:

> > > Am Donnerstag, den 20.05.2021, 15:31 +0200 schrieb Johan Hovold:

> > 

> > > > Isn't that already handled by lircd using uinput?

> > > 

> > > The problem with that reasoning, though it is true, is

> > > 

> > > 1) We would need to remove a lot of subsystems if we took that

> > > to the logical conclusion. 

> > 

> > Removing code is always nice. ;)

> 

> So rather than adding hotplug to serdev, we should remove line-discipline,

> serdev, and serio and all its drivers from the kernel? This is taking

> your own argument and applying it your code.


Not at all. Not everything can be done in user space, but some things
can.

> > > 3) We end up with two classes of LIRC devices

> > 

> > We already do, right? That's kind of my point since we have lircd

> > supporting uinput.

> 

> This is not an either-or situation, lircd is the "old" solution which is

> slowing being supplanted with rc-core. All the new keymaps are rc-core and

> do not work with lircd. The new rc-core tooling (in the v4l-utils package) 

> does not work with lircd. lircd hasn't had any real patches merged for years

> now.

> 

> There is whole new tooling in the works for rc-core which is not compatible

> with lircd.


Sure, you already explained that. I was just asking (earlier) why you
didn't use the infrastructure that's already in place. If there are good
reasons for not doing so then fine. 

> > > > I hear you, but we still need to have those discussions from time to

> > > > time to make sure our architecture is sane. One of the problems today

> > > > with the kernel development process appears to be that too few

> > > > questions

> > > > are asked. If it builds, ship it...

> > > 

> > > Indeed, so, could we force a line discipline on a device on the kernel

> > > level? Code duplication is bad.

> > 

> > Not sure I understand what you have mind here. serdev is sort of a

> > line-discipline which we'd "force" on a device if there's a matching

> > description in devicetree, while line disciplines always need to be

> > instantiated by user space. Or are you referring to ldisc/serdev code

> > reuse?

> 

> I am pretty sure Oliver is suggesting that all ldisc/serdev code in

> the kernel is duplication of code which can be done in userspace, by your

> own argument.


See above.

> > > > But I think I've got that point across by now.

> > > 

> > > Yes and and we need to think about the conclusion we draw from

> > > that point. It seems to me that an architecture that pushes data

> > > through the whole tty layer into a demon, then through uinput

> > > is definitely not elegant.

> > 

> > The elegant answer is serdev, but it does not yet support the features

> > needed in this case (i.e. hotplugging).

> > 

> > Since we already support user-space drivers for these devices, I see

> > nothing wrong with implementing support for another one in user space

> > unless there are strong reasons against doing so (e.g. performance,

> > pm or usability). But if uinput works then great, we're done.

> 

> As discussed lircd has terrible latency, and lircd is out of date and

> unmaintained and does not work with modern tooling and keymaps.

> 

> Also essentially your saying that any input device that connects to a

> serial port should be done in user space. There are a ton of kernel

> drivers doing exactly that, and that is why serio exists in the first

> place.


I'm not, again see above. I'm saying that we should not make one-off
copies of serial drivers if we can avoid it.

In this case the limitations of lircd and the lack of hotplugging in
serdev may be a sufficient reason for making an exception. As we've
already discussed.

Johan
Sean Young June 25, 2021, 8:08 a.m. UTC | #14
On Thu, Jun 24, 2021 at 11:41:28AM +0200, Johan Hovold wrote:
> I'm not, again see above. I'm saying that we should not make one-off

> copies of serial drivers if we can avoid it.

> 

> In this case the limitations of lircd and the lack of hotplugging in

> serdev may be a sufficient reason for making an exception. As we've

> already discussed.


Great, thanks very much. I totally agree a serdev solution would be
preferable.

In that case, can have your Signed-off-by for the ftdi_sio.c change, or
could this be merged through the usb tree please?

Thanks

Sean
Johan Hovold July 2, 2021, 10:44 a.m. UTC | #15
On Fri, Jun 25, 2021 at 09:08:00AM +0100, Sean Young wrote:
> On Thu, Jun 24, 2021 at 11:41:28AM +0200, Johan Hovold wrote:
> > I'm not, again see above. I'm saying that we should not make one-off
> > copies of serial drivers if we can avoid it.
> > 
> > In this case the limitations of lircd and the lack of hotplugging in
> > serdev may be a sufficient reason for making an exception. As we've
> > already discussed.
> 
> Great, thanks very much. I totally agree a serdev solution would be
> preferable.
> 
> In that case, can have your Signed-off-by for the ftdi_sio.c change, or
> could this be merged through the usb tree please?

Sure, I can take the ftdi change (for the current -rc) once the ir
driver hits the media tree.

Johan