Re: [PATCH v4 2/3] media: rc: introduce BPF_PROG_LIRC_MODE2

[Sean Young]

On Fri, May 25, 2018 at 01:45:11PM -0700, Alexei Starovoitov wrote:
> On Fri, May 18, 2018 at 03:07:29PM +0100, Sean Young wrote:
> > Add support for BPF_PROG_LIRC_MODE2. This type of BPF program can call
> > rc_keydown() to reported decoded IR scancodes, or rc_repeat() to report
> > that the last key should be repeated.
> > 
> > The bpf program can be attached to using the bpf(BPF_PROG_ATTACH) syscall;
> > the target_fd must be the /dev/lircN device.
> > 
> > Signed-off-by: Sean Young 
> ...
> >  enum bpf_attach_type {
> > @@ -158,6 +159,7 @@ enum bpf_attach_type {
> > BPF_CGROUP_INET6_CONNECT,
> > BPF_CGROUP_INET4_POST_BIND,
> > BPF_CGROUP_INET6_POST_BIND,
> > +   BPF_LIRC_MODE2,
> > __MAX_BPF_ATTACH_TYPE
> >  };
> >  
> > @@ -1902,6 +1904,53 @@ union bpf_attr {
> >   * egress otherwise). This is the only flag supported for now.
> >   * Return
> >   * **SK_PASS** on success, or **SK_DROP** on error.
> > + *
> > + * int bpf_rc_keydown(void *ctx, u32 protocol, u64 scancode, u32 toggle)
> > + * Description
> > + * This helper is used in programs implementing IR decoding, to
> > + * report a successfully decoded key press with *scancode*,
> > + * *toggle* value in the given *protocol*. The scancode will be
> > + * translated to a keycode using the rc keymap, and reported as
> > + * an input key down event. After a period a key up event is
> > + * generated. This period can be extended by calling either
> > + * **bpf_rc_keydown** () with the same values, or calling
> > + * **bpf_rc_repeat** ().
> > + *
> > + * Some protocols include a toggle bit, in case the button
> > + * was released and pressed again between consecutive scancodes
> > + *
> > + * The *ctx* should point to the lirc sample as passed into
> > + * the program.
> > + *
> > + * The *protocol* is the decoded protocol number (see
> > + * **enum rc_proto** for some predefined values).
> > + *
> > + * This helper is only available is the kernel was compiled with
> > + * the **CONFIG_BPF_LIRC_MODE2** configuration option set to
> > + * "**y**".
> > + *
> > + * Return
> > + * 0
> > + *
> > + * int bpf_rc_repeat(void *ctx)
> > + * Description
> > + * This helper is used in programs implementing IR decoding, to
> > + * report a successfully decoded repeat key message. This delays
> > + * the generation of a key up event for previously generated
> > + * key down event.
> > + *
> > + * Some IR protocols like NEC have a special IR message for
> > + * repeating last button, for when a button is held down.
> > + *
> > + * The *ctx* should point to the lirc sample as passed into
> > + * the program.
> > + *
> > + * This helper is only available is the kernel was compiled with
> > + * the **CONFIG_BPF_LIRC_MODE2** configuration option set to
> > + * "**y**".
> 
> Hi Sean,
> 
> thank you for working on this. The patch set looks good to me.
> I'd only ask to change above two helper names to something more specific.
> Since BPF_PROG_TYPE_LIRC_MODE2 is the name of new prog type and kconfig.
> May be bpf_lirc2_keydown() and bpf_lirc2_repeat() ?

A little history might help here.

lirc and rc-core have non-obvious meanings. So, lirc was the original project
that dealt with IR. That project was rejected from mainline because it did
not send translated keycodes to input devices (it exposed its own interface
for keypresses).

Then rc-core was written which maps IR scancodes to keycodes (using rc
keymaps) and sends them to the input layer. The original lirc userspace ABI
for receiving and sending raw IR pulses and spaces was retained (mode2 as
it was called in lirc).

Reusing parts of the lirc ABI for BPF decoding raw IR makes sense, however
dispatching decoded scancodes was never part of lirc, only rc-core. In fact,
rc-core is reused in hdmi-cec for cec commands, which does not use lirc
at all. So for example, if we want to process cec messages in bpf, it would
want call rc_keydown().

I don't think this lirc/rc-core duality is particularly great, but I'm
not sure what the right answer to that is.

> > @@ -1576,6 +1577,8 @@ static int bpf_prog_attach(const union bpf_attr *attr)
> > case BPF_SK_SKB_STREAM_PARSER:
> > case BPF_SK_SKB_STREAM_VERDICT:
> > return sockmap_get_from_fd(attr, BPF_PROG_TYPE_SK_SKB, true);
> > +   case BPF_LIRC_MODE2:
> > +   return rc_dev_prog_attach(attr);
> ...
> > +   case BPF_LIRC_MODE2:
> > +   return rc_dev_prog_detach(attr);
> 
> and similar rename for internal function names that go into bpf core.

I agree with this.

> Please add accumulated acks when you respin.

Good point, will do.

Thanks,

Sean

Re: [PATCH v4 2/3] media: rc: introduce BPF_PROG_LIRC_MODE2

[Alexei Starovoitov]

On Fri, May 18, 2018 at 03:07:29PM +0100, Sean Young wrote:
> Add support for BPF_PROG_LIRC_MODE2. This type of BPF program can call
> rc_keydown() to reported decoded IR scancodes, or rc_repeat() to report
> that the last key should be repeated.
> 
> The bpf program can be attached to using the bpf(BPF_PROG_ATTACH) syscall;
> the target_fd must be the /dev/lircN device.
> 
> Signed-off-by: Sean Young 
...
>  enum bpf_attach_type {
> @@ -158,6 +159,7 @@ enum bpf_attach_type {
>   BPF_CGROUP_INET6_CONNECT,
>   BPF_CGROUP_INET4_POST_BIND,
>   BPF_CGROUP_INET6_POST_BIND,
> + BPF_LIRC_MODE2,
>   __MAX_BPF_ATTACH_TYPE
>  };
>  
> @@ -1902,6 +1904,53 @@ union bpf_attr {
>   *   egress otherwise). This is the only flag supported for now.
>   *   Return
>   *   **SK_PASS** on success, or **SK_DROP** on error.
> + *
> + * int bpf_rc_keydown(void *ctx, u32 protocol, u64 scancode, u32 toggle)
> + *   Description
> + *   This helper is used in programs implementing IR decoding, to
> + *   report a successfully decoded key press with *scancode*,
> + *   *toggle* value in the given *protocol*. The scancode will be
> + *   translated to a keycode using the rc keymap, and reported as
> + *   an input key down event. After a period a key up event is
> + *   generated. This period can be extended by calling either
> + *   **bpf_rc_keydown** () with the same values, or calling
> + *   **bpf_rc_repeat** ().
> + *
> + *   Some protocols include a toggle bit, in case the button
> + *   was released and pressed again between consecutive scancodes
> + *
> + *   The *ctx* should point to the lirc sample as passed into
> + *   the program.
> + *
> + *   The *protocol* is the decoded protocol number (see
> + *   **enum rc_proto** for some predefined values).
> + *
> + *   This helper is only available is the kernel was compiled with
> + *   the **CONFIG_BPF_LIRC_MODE2** configuration option set to
> + *   "**y**".
> + *
> + *   Return
> + *   0
> + *
> + * int bpf_rc_repeat(void *ctx)
> + *   Description
> + *   This helper is used in programs implementing IR decoding, to
> + *   report a successfully decoded repeat key message. This delays
> + *   the generation of a key up event for previously generated
> + *   key down event.
> + *
> + *   Some IR protocols like NEC have a special IR message for
> + *   repeating last button, for when a button is held down.
> + *
> + *   The *ctx* should point to the lirc sample as passed into
> + *   the program.
> + *
> + *   This helper is only available is the kernel was compiled with
> + *   the **CONFIG_BPF_LIRC_MODE2** configuration option set to
> + *   "**y**".

Hi Sean,

thank you for working on this. The patch set looks good to me.
I'd only ask to change above two helper names to something more specific.
Since BPF_PROG_TYPE_LIRC_MODE2 is the name of new prog type and kconfig.
May be bpf_lirc2_keydown() and bpf_lirc2_repeat() ?

> @@ -1576,6 +1577,8 @@ static int bpf_prog_attach(const union bpf_attr *attr)
>   case BPF_SK_SKB_STREAM_PARSER:
>   case BPF_SK_SKB_STREAM_VERDICT:
>   return sockmap_get_from_fd(attr, BPF_PROG_TYPE_SK_SKB, true);
> + case BPF_LIRC_MODE2:
> + return rc_dev_prog_attach(attr);
...
> + case BPF_LIRC_MODE2:
> + return rc_dev_prog_detach(attr);

and similar rename for internal function names that go into bpf core.

Please add accumulated acks when you respin.

Thanks

Re: [PATCH v4 2/3] media: rc: introduce BPF_PROG_LIRC_MODE2

[Y Song]

On Fri, May 18, 2018 at 7:07 AM, Sean Young  wrote:
> Add support for BPF_PROG_LIRC_MODE2. This type of BPF program can call
> rc_keydown() to reported decoded IR scancodes, or rc_repeat() to report
> that the last key should be repeated.
>
> The bpf program can be attached to using the bpf(BPF_PROG_ATTACH) syscall;
> the target_fd must be the /dev/lircN device.
>
> Signed-off-by: Sean Young 

Acked-by: Yonghong Song 

> ---
>  drivers/media/rc/Kconfig|  13 ++
>  drivers/media/rc/Makefile   |   1 +
>  drivers/media/rc/bpf-lirc.c | 308 
>  drivers/media/rc/lirc_dev.c |  30 
>  drivers/media/rc/rc-core-priv.h |  22 +++
>  drivers/media/rc/rc-ir-raw.c|  12 +-
>  include/linux/bpf_rcdev.h   |  30 
>  include/linux/bpf_types.h   |   3 +
>  include/uapi/linux/bpf.h|  53 +-
>  kernel/bpf/syscall.c|   7 +
>  10 files changed, 476 insertions(+), 3 deletions(-)
>  create mode 100644 drivers/media/rc/bpf-lirc.c
>  create mode 100644 include/linux/bpf_rcdev.h
>
> diff --git a/drivers/media/rc/Kconfig b/drivers/media/rc/Kconfig
> index eb2c3b6eca7f..d5b35a6ba899 100644
> --- a/drivers/media/rc/Kconfig
> +++ b/drivers/media/rc/Kconfig
> @@ -25,6 +25,19 @@ config LIRC
>passes raw IR to and from userspace, which is needed for
>IR transmitting (aka "blasting") and for the lirc daemon.
>
> +config BPF_LIRC_MODE2
> +   bool "Support for eBPF programs attached to lirc devices"
> +   depends on BPF_SYSCALL
> +   depends on RC_CORE=y
> +   depends on LIRC
> +   help
> +  Allow attaching eBPF programs to a lirc device using the bpf(2)
> +  syscall command BPF_PROG_ATTACH. This is supported for raw IR
> +  receivers.
> +
> +  These eBPF programs can be used to decode IR into scancodes, for
> +  IR protocols not supported by the kernel decoders.
> +
>  menuconfig RC_DECODERS
> bool "Remote controller decoders"
> depends on RC_CORE
> diff --git a/drivers/media/rc/Makefile b/drivers/media/rc/Makefile
> index 2e1c87066f6c..e0340d043fe8 100644
> --- a/drivers/media/rc/Makefile
> +++ b/drivers/media/rc/Makefile
> @@ -5,6 +5,7 @@ obj-y += keymaps/
>  obj-$(CONFIG_RC_CORE) += rc-core.o
>  rc-core-y := rc-main.o rc-ir-raw.o
>  rc-core-$(CONFIG_LIRC) += lirc_dev.o
> +rc-core-$(CONFIG_BPF_LIRC_MODE2) += bpf-lirc.o
>  obj-$(CONFIG_IR_NEC_DECODER) += ir-nec-decoder.o
>  obj-$(CONFIG_IR_RC5_DECODER) += ir-rc5-decoder.o
>  obj-$(CONFIG_IR_RC6_DECODER) += ir-rc6-decoder.o
> diff --git a/drivers/media/rc/bpf-lirc.c b/drivers/media/rc/bpf-lirc.c
> new file mode 100644
> index ..c9673df2d9cd
> --- /dev/null
> +++ b/drivers/media/rc/bpf-lirc.c
> @@ -0,0 +1,308 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// bpf-lirc.c - handles bpf
> +//
> +// Copyright (C) 2018 Sean Young 
> +
> +#include 
> +#include 
> +#include 
> +#include "rc-core-priv.h"
> +
> +/*
> + * BPF interface for raw IR
> + */
> +const struct bpf_prog_ops lirc_mode2_prog_ops = {
> +};
> +
> +BPF_CALL_1(bpf_rc_repeat, u32*, sample)
> +{
> +   struct ir_raw_event_ctrl *ctrl;
> +
> +   ctrl = container_of(sample, struct ir_raw_event_ctrl, bpf_sample);
> +
> +   rc_repeat(ctrl->dev);
> +
> +   return 0;
> +}
> +
> +static const struct bpf_func_proto rc_repeat_proto = {
> +   .func  = bpf_rc_repeat,
> +   .gpl_only  = true, /* rc_repeat is EXPORT_SYMBOL_GPL */
> +   .ret_type  = RET_INTEGER,
> +   .arg1_type = ARG_PTR_TO_CTX,
> +};
> +
> +/*
> + * Currently rc-core does not support 64-bit scancodes, but there are many
> + * known protocols with more than 32 bits. So, define the interface as u64
> + * as a future-proof.
> + */
> +BPF_CALL_4(bpf_rc_keydown, u32*, sample, u32, protocol, u64, scancode,
> +  u32, toggle)
> +{
> +   struct ir_raw_event_ctrl *ctrl;
> +
> +   ctrl = container_of(sample, struct ir_raw_event_ctrl, bpf_sample);
> +
> +   rc_keydown(ctrl->dev, protocol, scancode, toggle != 0);
> +
> +   return 0;
> +}
> +
> +static const struct bpf_func_proto rc_keydown_proto = {
> +   .func  = bpf_rc_keydown,
> +   .gpl_only  = true, /* rc_keydown is EXPORT_SYMBOL_GPL */
> +   .ret_type  = RET_INTEGER,
> +   .arg1_type = ARG_PTR_TO_CTX,
> +   .arg2_type = ARG_ANYTHING,
> +   .arg3_type = ARG_ANYTHING,
> +   .arg4_type = ARG_ANYTHING,
> +};
> +
> +static const struct bpf_func_proto *
> +lirc_mode2_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
> +{
> +   switch (func_id) {
> +   case BPF_FUNC_rc_repeat:
> +   return _repeat_proto;
> +   case BPF_FUNC_rc_keydown:
> +   return _keydown_proto;
> +   case BPF_FUNC_map_lookup_elem:
> +   return _map_lookup_elem_proto;
> +   case BPF_FUNC_map_update_elem:
> +   return _map_update_elem_proto;
> +