LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Quentin Monnet <quentin.monnet@netronome.com>
To: Sean Young <sean@mess.org>,
	linux-media@vger.kernel.org, linux-kernel@vger.kernel.org,
	Alexei Starovoitov <ast@kernel.org>,
	Mauro Carvalho Chehab <mchehab@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	netdev@vger.kernel.org, Matthias Reichl <hias@horus.com>,
	Devin Heitmueller <dheitmueller@kernellabs.com>,
	Y Song <ys114321@gmail.com>
Subject: Re: [PATCH v3 1/2] media: rc: introduce BPF_PROG_RAWIR_EVENT
Date: Thu, 17 May 2018 13:10:56 +0100	[thread overview]
Message-ID: <592262ad-e92f-2b53-f6bb-086257c21db0@netronome.com> (raw)
In-Reply-To: <92785c791057185fa5691f78cecfa4beae7fc336.1526504511.git.sean@mess.org>

2018-05-16 22:04 UTC+0100 ~ Sean Young <sean@mess.org>
> Add support for BPF_PROG_RAWIR_EVENT. 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 <sean@mess.org>
> ---
>  drivers/media/rc/Kconfig           |  13 ++
>  drivers/media/rc/Makefile          |   1 +
>  drivers/media/rc/bpf-rawir-event.c | 363 +++++++++++++++++++++++++++++
>  drivers/media/rc/lirc_dev.c        |  24 ++
>  drivers/media/rc/rc-core-priv.h    |  24 ++
>  drivers/media/rc/rc-ir-raw.c       |  14 +-
>  include/linux/bpf_rcdev.h          |  30 +++
>  include/linux/bpf_types.h          |   3 +
>  include/uapi/linux/bpf.h           |  55 ++++-
>  kernel/bpf/syscall.c               |   7 +
>  10 files changed, 531 insertions(+), 3 deletions(-)
>  create mode 100644 drivers/media/rc/bpf-rawir-event.c
>  create mode 100644 include/linux/bpf_rcdev.h
> 

[...]

Hi Sean,

Please find below some nitpicks on the documentation for the two helpers.

> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index d94d333a8225..243e141e8a5b 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h

[...]

> @@ -1902,6 +1904,35 @@ 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, u32 scancode, u32 toggle)
> + *	Description
> + *		Report decoded scancode with toggle value. For use in
> + *		BPF_PROG_TYPE_RAWIR_EVENT, to report a successfully

Could you please use bold RST markup for constants and function names?
Typically for BPF_PROG_TYPE_RAWIR_EVENT here and the enum below.

> + *		decoded scancode. This is will generate a keydown event,

s/This is will/This will/?

> + *		and a keyup event once the scancode is no longer repeated.
> + *
> + *		*ctx* pointer to bpf_rawir_event, *protocol* is decoded
> + *		protocol (see RC_PROTO_* enum).

This documentation is intended to be compiled as a man page. Could you
please use a complete sentence here?
Also, this could do with additional markup as well: **struct
bpf_rawir_event**.

> + *
> + *		Some protocols include a toggle bit, in case the button
> + *		was released and pressed again between consecutive scancodes,
> + *		copy this bit into *toggle* if it exists, else set to 0.
> + *
> + *     Return

The "Return" lines here and in the second helper use space indent
instead as tabs (as all other lines do). Would you mind fixing it for
consistency?

> + *		Always return 0 (for now)

Other helpers use just "0" in that case, but I do not really mind.
Out of curiosity, do you have anything specific in mind for changing the
return value here in the future?

> + *
> + * int bpf_rc_repeat(void *ctx)
> + *	Description
> + *		Repeat the last decoded scancode; some IR protocols like
> + *		NEC have a special IR message for repeat last button,

s/repeat/repeating/?

> + *		in case user is holding a button down; the scancode is
> + *		not repeated.
> + *
> + *		*ctx* pointer to bpf_rawir_event.

Please use a complete sentence here as well, if you do not mind.

> + *
> + *     Return
> + *		Always return 0 (for now)
>   */
Thanks,
Quentin

  reply	other threads:[~2018-05-17 12:11 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-05-16 21:04 [PATCH v3 0/2] IR decoding using BPF Sean Young
2018-05-16 21:04 ` [PATCH v3 1/2] media: rc: introduce BPF_PROG_RAWIR_EVENT Sean Young
2018-05-17 12:10   ` Quentin Monnet [this message]
2018-05-17 13:44     ` Sean Young
2018-05-17 17:02   ` Y Song
2018-05-17 21:45     ` Sean Young
2018-05-18  5:31       ` Y Song
2018-05-16 21:04 ` [PATCH v3 2/2] bpf: add selftest for rawir_event type program Sean Young
2018-05-17 17:17   ` Y Song
2018-05-17 21:01     ` Sean Young
2018-05-18 10:13       ` Quentin Monnet
2018-05-18 13:33         ` Sean Young
2018-05-18 13:48           ` Quentin Monnet

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=592262ad-e92f-2b53-f6bb-086257c21db0@netronome.com \
    --to=quentin.monnet@netronome.com \
    --cc=ast@kernel.org \
    --cc=daniel@iogearbox.net \
    --cc=dheitmueller@kernellabs.com \
    --cc=hias@horus.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-media@vger.kernel.org \
    --cc=mchehab@kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=sean@mess.org \
    --cc=ys114321@gmail.com \
    --subject='Re: [PATCH v3 1/2] media: rc: introduce BPF_PROG_RAWIR_EVENT' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).