Re: [PATCH 0/3] Improve CEC autorepeat handling

[Dmitry Torokhov]

On Mon, Nov 27, 2017 at 09:47:24AM +, Sean Young wrote:
> Hi Hans,
> 
> On Mon, Nov 27, 2017 at 10:13:51AM +0100, Hans Verkuil wrote:
> > On 11/26/2017 12:47 AM, Dmitry Torokhov wrote:
> > > On Fri, Nov 24, 2017 at 11:43:58AM +, Sean Young wrote:
> > >> Due to the slowness of the CEC bus, autorepeat handling rather special
> > >> on CEC. If the repeated user control pressed message is received, a 
> > >> keydown repeat should be sent immediately.
> > > 
> > > This sounds like you want to have hardware autorepeat combined with
> > > software one. This seems fairly specific to CEC and I do not think that
> > > this should be in input core; but stay in the driver.
> > > 
> > > Another option just to decide what common delay for CEC autorepeat is
> > > and rely on the standard autorepeat handling. The benefit is that users
> > > can control the delay before autorepeat kicks in.
> > 
> > They are not allowed to. Autorepeat is only allowed to start when a second
> > keydown message arrives within 550 ms as per the spec. After that autorepeat
> > continues as long as keydown messages are received within 550ms from the
> > previous one. The actual REP_PERIOD time is unrelated to the frequency of
> > the CEC messages but should be that of the local system.

Not allowed by whom? If I, as a user, want my remote to start
autorepeating after 400 msec instead of 550, will the police come and
fine me? Please do not confuse the default behavior with allowed one.
The only restriction is that if you have not seen messages for longer
than 550 msecs you should "release" the key.

> > 
> > The thing to remember here is that CEC is slooow (400 bits/s) so you cannot
> > send messages at REP_PERIOD rate. You should see it as messages that tell
> > you to enter/stay in autorepeat mode. Not as actual autorepeat messages.

Right, and they do not have to match autorepeat timings, just control
whether we should continue repeating or generate release event.

> > 
> > > 
> > >>
> > >> By handling this in the input layer, we can remove some ugly code from
> > >> cec, which also sends a keyup event after the first keydown, to prevent
> > >> autorepeat.
> > > 
> > > If driver does not want input core to handle autorepeat (but handle
> > > autorepeat by themselves) they should indicate it by setting appropriate
> > > dev->rep[REP_DELAY] and dev->rep[REP_PERIOD] before calling
> > > input_register_device(). This will let input core know that it should
> > > not setup its autorepeat timer.
> > 
> > That only means that I have to setup the autorepeat timer myself, there
> > is no benefit in that :-)
> > 
> > Sean, I kind of agree with Dmitry here. The way autorepeat works for CEC
> > is pretty specific to that protocol and unlikely to be needed for other
> > protocols.
> 
> That's a valid point, I guess. The only downside is special case handling
> outside the input layer, which would be much simpler in the input layer.
> 
> > It is also no big deal to keep knowledge of that within cec-adap.c.
> 
> So first of all, the sii8620 uses the CEC protocol as well (see commit
> e25f1f7c94e1 drm/bridge/sii8620: add remote control support), so this
> will have to go into rc-core, not cec-adap.c. There was a discussion about
> some time ago.
> 
> The current implementation has an ugly key up event which would be nice
> to do without.
> 
> > The only thing that would be nice to have control over is that with CEC
> > userspace shouldn't be able to change REP_DELAY and that REP_DELAY should
> > always be identical to REP_PERIOD. If this can be done easily, then that
> > would be nice, but it's a nice-to-have in my opinion.

You could do that by catching EV_REP events in your driver, but I do not
see why you want to remove this flexibility from users. Again, if I want
to wait for autorepeat to start for 10 seconds, or 10 msecs, it is my
choice. You are to provide the default, I am to override it if I want.

> 
> The REP_DELAY must be equal to REP_PERIOD seems a bit odd to me. In fact,
> I propose something different. If REP_DELAY != 0 then the input layer
> produces autorepeats as normal. If REP_DELAY == 0, then generate repeats
> on the second key down event.

I have no idea why you simply not rely on current input core autorepeat
handling, set REP_DELAY to be 550 msec and call it a day.

Thanks.

-- 
Dmitry

Re: [PATCH 0/3] Improve CEC autorepeat handling

[Dmitry Torokhov]

Hi Sean,

On Fri, Nov 24, 2017 at 11:43:58AM +, Sean Young wrote:
> Due to the slowness of the CEC bus, autorepeat handling rather special
> on CEC. If the repeated user control pressed message is received, a 
> keydown repeat should be sent immediately.

This sounds like you want to have hardware autorepeat combined with
software one. This seems fairly specific to CEC and I do not think that
this should be in input core; but stay in the driver.

Another option just to decide what common delay for CEC autorepeat is
and rely on the standard autorepeat handling. The benefit is that users
can control the delay before autorepeat kicks in.

> 
> By handling this in the input layer, we can remove some ugly code from
> cec, which also sends a keyup event after the first keydown, to prevent
> autorepeat.

If driver does not want input core to handle autorepeat (but handle
autorepeat by themselves) they should indicate it by setting appropriate
dev->rep[REP_DELAY] and dev->rep[REP_PERIOD] before calling
input_register_device(). This will let input core know that it should
not setup its autorepeat timer.

Thanks.

-- 
Dmitry

Re: [PATCH v2] media: ttpci: remove autorepeat handling and use timer_setup

[Dmitry Torokhov]

On Thu, Nov 02, 2017 at 10:16:58PM -0200, Mauro Carvalho Chehab wrote:
> Em Thu, 2 Nov 2017 16:50:37 -0700
> Dmitry Torokhov <dmitry.torok...@gmail.com> escreveu:
> 
> > On Thu, Nov 02, 2017 at 04:24:27PM -0700, Kees Cook wrote:
> > > On Tue, Oct 31, 2017 at 1:11 PM, Sean Young <s...@mess.org> wrote:  
> > > > Leave the autorepeat handling up to the input layer, and move
> > > > to the new timer API.
> > > >
> > > > Compile tested only.
> > > >
> > > > Signed-off-by: Sean Young <s...@mess.org>  
> > > 
> > > Hi! Just checking up on this... the input timer conversion is blocked
> > > by getting this sorted out, so I'd love to have something either
> > > media, input, or timer tree can carry. :)  
> > 
> > Acked-by: Dmitry Torokhov <dmitry.torok...@gmail.com>
> > 
> > From my POV the patch is good. Mauro, do you want me to take it through
> > my tree, or maybe you could create an immutable branch off 4.14-rc5 (or
> > 6) with this commit and I will pull it in and then can apply Kees input
> > core conversion patch?
> 
> Feel free to apply it into your tree with my ack:
> 
> Acked-by: Mauro Carvalho Chehab <mche...@s-opensource.com>

Applied and pulled Kees' patch to the input core (dropping the timer_data
business) on top.

Thanks.

-- 
Dmitry

Re: [PATCH v2] media: ttpci: remove autorepeat handling and use timer_setup

[Dmitry Torokhov]

On Thu, Nov 02, 2017 at 04:24:27PM -0700, Kees Cook wrote:
> On Tue, Oct 31, 2017 at 1:11 PM, Sean Young <s...@mess.org> wrote:
> > Leave the autorepeat handling up to the input layer, and move
> > to the new timer API.
> >
> > Compile tested only.
> >
> > Signed-off-by: Sean Young <s...@mess.org>
> 
> Hi! Just checking up on this... the input timer conversion is blocked
> by getting this sorted out, so I'd love to have something either
> media, input, or timer tree can carry. :)

Acked-by: Dmitry Torokhov <dmitry.torok...@gmail.com>

>From my POV the patch is good. Mauro, do you want me to take it through
my tree, or maybe you could create an immutable branch off 4.14-rc5 (or
6) with this commit and I will pull it in and then can apply Kees input
core conversion patch?

Thanks!

> 
> Thanks!
> 
> -Kees
> 
> > ---
> > v2:
> >  - fixes and improvements from Dmitry Torokhov
> >
> >  drivers/media/pci/ttpci/av7110.h|  2 +-
> >  drivers/media/pci/ttpci/av7110_ir.c | 56 
> > +
> >  2 files changed, 21 insertions(+), 37 deletions(-)
> >
> > diff --git a/drivers/media/pci/ttpci/av7110.h 
> > b/drivers/media/pci/ttpci/av7110.h
> > index 347827925c14..bcb72ecbedc0 100644
> > --- a/drivers/media/pci/ttpci/av7110.h
> > +++ b/drivers/media/pci/ttpci/av7110.h
> > @@ -93,7 +93,7 @@ struct infrared {
> > u8  inversion;
> > u16 last_key;
> > u16 last_toggle;
> > -   u8  delay_timer_finished;
> > +   boolkeypressed;
> >  };
> >
> >
> > diff --git a/drivers/media/pci/ttpci/av7110_ir.c 
> > b/drivers/media/pci/ttpci/av7110_ir.c
> > index ca05198de2c2..ee414803e6b5 100644
> > --- a/drivers/media/pci/ttpci/av7110_ir.c
> > +++ b/drivers/media/pci/ttpci/av7110_ir.c
> > @@ -84,15 +84,16 @@ static u16 default_key_map [256] = {
> >
> >
> >  /* key-up timer */
> > -static void av7110_emit_keyup(unsigned long parm)
> > +static void av7110_emit_keyup(struct timer_list *t)
> >  {
> > -   struct infrared *ir = (struct infrared *) parm;
> > +   struct infrared *ir = from_timer(ir, t, keyup_timer);
> >
> > -   if (!ir || !test_bit(ir->last_key, ir->input_dev->key))
> > +   if (!ir || !ir->keypressed)
> > return;
> >
> > input_report_key(ir->input_dev, ir->last_key, 0);
> > input_sync(ir->input_dev);
> > +   ir->keypressed = false;
> >  }
> >
> >
> > @@ -152,29 +153,18 @@ static void av7110_emit_key(unsigned long parm)
> > return;
> > }
> >
> > -   if (timer_pending(>keyup_timer)) {
> > -   del_timer(>keyup_timer);
> > -   if (ir->last_key != keycode || toggle != ir->last_toggle) {
> > -   ir->delay_timer_finished = 0;
> > -   input_event(ir->input_dev, EV_KEY, ir->last_key, 0);
> > -   input_event(ir->input_dev, EV_KEY, keycode, 1);
> > -   input_sync(ir->input_dev);
> > -   } else if (ir->delay_timer_finished) {
> > -   input_event(ir->input_dev, EV_KEY, keycode, 2);
> > -   input_sync(ir->input_dev);
> > -   }
> > -   } else {
> > -   ir->delay_timer_finished = 0;
> > -   input_event(ir->input_dev, EV_KEY, keycode, 1);
> > -   input_sync(ir->input_dev);
> > -   }
> > +   if (ir->keypressed &&
> > +   (ir->last_key != keycode || toggle != ir->last_toggle))
> > +   input_event(ir->input_dev, EV_KEY, ir->last_key, 0);
> >
> > +   input_event(ir->input_dev, EV_KEY, keycode, 1);
> > +   input_sync(ir->input_dev);
> > +
> > +   ir->keypressed = true;
> > ir->last_key = keycode;
> > ir->last_toggle = toggle;
> >
> > -   ir->keyup_timer.expires = jiffies + UP_TIMEOUT;
> > -   add_timer(>keyup_timer);
> > -
> > +   mod_timer(>keyup_timer, jiffies + UP_TIMEOUT);
> >  }
> >
> >
> > @@ -204,16 +194,6 @@ static void input_register_keys(struct infrared *ir)
> > ir->input_dev->keycodemax = ARRAY_SIZE(ir->key_map);
> >  }
> >
> > -
> > -/* called by the input driver after

Re: [PATCH] media: ttpci: remove autorepeat handling and use timer_setup

[Dmitry Torokhov]

Hi Sean,

On Tue, Oct 31, 2017 at 05:45:58PM +, Sean Young wrote:
> Leave the autorepeat handling up to the input layer, and move
> to the new timer API.
> 
> Compile tested only.
> 
> Signed-off-by: Sean Young 
> ---
>  drivers/media/pci/ttpci/av7110.h|  2 +-
>  drivers/media/pci/ttpci/av7110_ir.c | 54 
> ++---
>  2 files changed, 21 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/media/pci/ttpci/av7110.h 
> b/drivers/media/pci/ttpci/av7110.h
> index 347827925c14..bcb72ecbedc0 100644
> --- a/drivers/media/pci/ttpci/av7110.h
> +++ b/drivers/media/pci/ttpci/av7110.h
> @@ -93,7 +93,7 @@ struct infrared {
>   u8  inversion;
>   u16 last_key;
>   u16 last_toggle;
> - u8  delay_timer_finished;
> + boolkeypressed;
>  };
>  
>  
> diff --git a/drivers/media/pci/ttpci/av7110_ir.c 
> b/drivers/media/pci/ttpci/av7110_ir.c
> index ca05198de2c2..8207bead2224 100644
> --- a/drivers/media/pci/ttpci/av7110_ir.c
> +++ b/drivers/media/pci/ttpci/av7110_ir.c
> @@ -84,15 +84,16 @@ static u16 default_key_map [256] = {
>  
>  
>  /* key-up timer */
> -static void av7110_emit_keyup(unsigned long parm)
> +static void av7110_emit_keyup(struct timer_list *t)
>  {
> - struct infrared *ir = (struct infrared *) parm;
> + struct infrared *ir = from_timer(ir, t, keyup_timer);
>  
> - if (!ir || !test_bit(ir->last_key, ir->input_dev->key))
> + if (!ir || !ir->keypressed)
>   return;
>  
>   input_report_key(ir->input_dev, ir->last_key, 0);
>   input_sync(ir->input_dev);
> + ir->keypressed = false;
>  }
>  
>  
> @@ -105,6 +106,7 @@ static void av7110_emit_key(unsigned long parm)
>   u8 addr;
>   u16 toggle;
>   u16 keycode;
> + bool new_event;
>  
>   /* extract device address and data */
>   switch (ir->protocol) {
> @@ -152,29 +154,22 @@ static void av7110_emit_key(unsigned long parm)
>   return;
>   }
>  
> - if (timer_pending(>keyup_timer)) {
> - del_timer(>keyup_timer);
> - if (ir->last_key != keycode || toggle != ir->last_toggle) {
> - ir->delay_timer_finished = 0;
> - input_event(ir->input_dev, EV_KEY, ir->last_key, 0);
> - input_event(ir->input_dev, EV_KEY, keycode, 1);
> - input_sync(ir->input_dev);
> - } else if (ir->delay_timer_finished) {
> - input_event(ir->input_dev, EV_KEY, keycode, 2);
> - input_sync(ir->input_dev);
> - }
> - } else {
> - ir->delay_timer_finished = 0;
> - input_event(ir->input_dev, EV_KEY, keycode, 1);
> + new_event = !ir->keypressed || ir->last_key != keycode ||
> +toggle != ir->last_toggle;
> +
> + if (new_event && ir->keypressed)
> + input_event(ir->input_dev, EV_KEY, ir->last_key, 1);
> +
> + if (new_event) {
> + input_event(ir->input_dev, EV_KEY, keycode, 0);

I do not think this is correct. You want to release the old button, and
press the new one, not the other way around.

Given that we are reworking the code, and input core actually filters
out duplicate events for you, you can probably simplify it all further:


/* Release old key/button */
if (ir->keypressed && ir->last_key != keycode)
input_event(ir->input_dev, EV_KEY, ir->last_key, 0);

input_event(ir->input_dev, EV_KEY, keycode, 1);
input_sync(ir->input_dev);

and get rid of last_toggle member.

>   input_sync(ir->input_dev);
>   }
>  
> + ir->keypressed = true;
>   ir->last_key = keycode;
>   ir->last_toggle = toggle;
>  
> - ir->keyup_timer.expires = jiffies + UP_TIMEOUT;
> - add_timer(>keyup_timer);
> -
> + mod_timer(>keyup_timer, jiffies + UP_TIMEOUT);
>  }
>  
>  
> @@ -204,16 +199,6 @@ static void input_register_keys(struct infrared *ir)
>   ir->input_dev->keycodemax = ARRAY_SIZE(ir->key_map);
>  }
>  
> -
> -/* called by the input driver after rep[REP_DELAY] ms */
> -static void input_repeat_key(unsigned long parm)
> -{
> - struct infrared *ir = (struct infrared *) parm;
> -
> - ir->delay_timer_finished = 1;
> -}
> -
> -
>  /* check for configuration changes */
>  int av7110_check_ir_config(struct av7110 *av7110, int force)
>  {
> @@ -333,8 +318,7 @@ int av7110_ir_init(struct av7110 *av7110)
>   av_list[av_cnt++] = av7110;
>   av7110_check_ir_config(av7110, true);
>  
> - setup_timer(>ir.keyup_timer, av7110_emit_keyup,
> - (unsigned long)>ir);
> + timer_setup(>ir.keyup_timer, av7110_emit_keyup, 0);
>  
>   input_dev = input_allocate_device();
>   if (!input_dev)
> @@ -365,8 +349,10 @@ int av7110_ir_init(struct av7110 *av7110)
>   input_free_device(input_dev);
>   

[PATCH] media: av7110: switch to useing timer_setup()

[Dmitry Torokhov]

In preparation for unconditionally passing the struct timer_list pointer to
all timer callbacks, switch to using the new timer_setup() and from_timer()
to pass the timer pointer explicitly.

Also stop poking into input core internals and override its autorepeat
timer function. I am not sure why we have such convoluted autorepeat
handling in this driver instead of letting input core handle autorepeat,
but this preserves current behavior of allowing controlling autorepeat
delay and forcing autorepeat period to be whatever the hardware has.

Signed-off-by: Dmitry Torokhov <dmitry.torok...@gmail.com>
---

Note that this has not been tested on the hardware. But it should
compile, so I have that going for me.

 drivers/media/pci/ttpci/av7110.h|  4 ++--
 drivers/media/pci/ttpci/av7110_ir.c | 40 +
 2 files changed, 20 insertions(+), 24 deletions(-)

diff --git a/drivers/media/pci/ttpci/av7110.h b/drivers/media/pci/ttpci/av7110.h
index 347827925c14..0aa3c6f01853 100644
--- a/drivers/media/pci/ttpci/av7110.h
+++ b/drivers/media/pci/ttpci/av7110.h
@@ -80,10 +80,11 @@ struct av7110;
 
 /* infrared remote control */
 struct infrared {
-   u16 key_map[256];
+   u16 key_map[256];
struct input_dev*input_dev;
charinput_phys[32];
struct timer_list   keyup_timer;
+   unsigned long   keydown_time;
struct tasklet_struct   ir_tasklet;
void(*ir_handler)(struct av7110 *av7110, u32 ircom);
u32 ir_command;
@@ -93,7 +94,6 @@ struct infrared {
u8  inversion;
u16 last_key;
u16 last_toggle;
-   u8  delay_timer_finished;
 };
 
 
diff --git a/drivers/media/pci/ttpci/av7110_ir.c 
b/drivers/media/pci/ttpci/av7110_ir.c
index ca05198de2c2..b602e64b3412 100644
--- a/drivers/media/pci/ttpci/av7110_ir.c
+++ b/drivers/media/pci/ttpci/av7110_ir.c
@@ -84,9 +84,9 @@ static u16 default_key_map [256] = {
 
 
 /* key-up timer */
-static void av7110_emit_keyup(unsigned long parm)
+static void av7110_emit_keyup(struct timer_list *t)
 {
-   struct infrared *ir = (struct infrared *) parm;
+   struct infrared *ir = from_timer(ir, keyup_timer, t);
 
if (!ir || !test_bit(ir->last_key, ir->input_dev->key))
return;
@@ -152,19 +152,20 @@ static void av7110_emit_key(unsigned long parm)
return;
}
 
-   if (timer_pending(>keyup_timer)) {
-   del_timer(>keyup_timer);
+   if (del_timer(>keyup_timer)) {
if (ir->last_key != keycode || toggle != ir->last_toggle) {
-   ir->delay_timer_finished = 0;
+   ir->keydown_time = jiffies;
input_event(ir->input_dev, EV_KEY, ir->last_key, 0);
input_event(ir->input_dev, EV_KEY, keycode, 1);
input_sync(ir->input_dev);
-   } else if (ir->delay_timer_finished) {
+   } else if (time_after(jiffies, ir->keydown_time +
+   msecs_to_jiffies(
+   ir->input_dev->rep[REP_PERIOD]))) {
input_event(ir->input_dev, EV_KEY, keycode, 2);
input_sync(ir->input_dev);
}
} else {
-   ir->delay_timer_finished = 0;
+   ir->keydown_time = jiffies;
input_event(ir->input_dev, EV_KEY, keycode, 1);
input_sync(ir->input_dev);
}
@@ -172,9 +173,7 @@ static void av7110_emit_key(unsigned long parm)
ir->last_key = keycode;
ir->last_toggle = toggle;
 
-   ir->keyup_timer.expires = jiffies + UP_TIMEOUT;
-   add_timer(>keyup_timer);
-
+   mod_timer(>keyup_timer, jiffies + UP_TIMEOUT);
 }
 
 
@@ -184,12 +183,19 @@ static void input_register_keys(struct infrared *ir)
int i;
 
set_bit(EV_KEY, ir->input_dev->evbit);
-   set_bit(EV_REP, ir->input_dev->evbit);
set_bit(EV_MSC, ir->input_dev->evbit);
 
set_bit(MSC_RAW, ir->input_dev->mscbit);
set_bit(MSC_SCAN, ir->input_dev->mscbit);
 
+   set_bit(EV_REP, ir->input_dev->evbit);
+   /*
+* By setting the delay before registering input device we
+* indicate that we will be implementing the autorepeat
+* ourselves.
+*/
+   ir->input_dev->rep[REP_DELAY] = 250;
+
memset(ir->input_dev->keybit, 0, sizeof(ir->input_dev->keybit));
 
for (i = 0; i < ARRAY_SIZE(ir->key_map); i++) {
@@ -205,15 +211,6 @@ static void input_register_keys(struct infrared *ir)
 }
 
 
-/* called by the input driver after rep[REP_DELAY] ms */
-static void

Re: [PATCH] media: input: Convert timers to use timer_setup()

[Dmitry Torokhov]

On Thu, Oct 19, 2017 at 03:45:38PM -0700, Kees Cook wrote:
> On Thu, Oct 19, 2017 at 3:32 PM, Dmitry Torokhov
> <dmitry.torok...@gmail.com> wrote:
> > On Mon, Oct 16, 2017 at 04:14:43PM -0700, Kees Cook wrote:
> >> In preparation for unconditionally passing the struct timer_list pointer to
> >> all timer callbacks, switch to using the new timer_setup() and from_timer()
> >> to pass the timer pointer explicitly.
> >>
> >> One input_dev user hijacks the input_dev software autorepeat timer to
> >> perform its own repeat management. However, there is no path back to the
> >> existing status variable, so add a generic one to the input structure and
> >> use that instead.
> >
> > That is too bad and it should not be doing this. I'd rather av7110 used
> > its own private timer for that.
> 
> Yeah, that was a pretty weird case. I couldn't see how to avoid it,
> though. I didn't see a way to hook the autorepeat, but I'm not too
> familiar with the code here.

You just need to manage the private timer in the driver and not mess up
with the input core if input core's autorepeat does not provide the
desired behavior...

Thanks.

-- 
Dmitry

Re: [PATCH] media: input: Convert timers to use timer_setup()

[Dmitry Torokhov]

On Mon, Oct 16, 2017 at 04:14:43PM -0700, Kees Cook wrote:
> In preparation for unconditionally passing the struct timer_list pointer to
> all timer callbacks, switch to using the new timer_setup() and from_timer()
> to pass the timer pointer explicitly.
> 
> One input_dev user hijacks the input_dev software autorepeat timer to
> perform its own repeat management. However, there is no path back to the
> existing status variable, so add a generic one to the input structure and
> use that instead.

That is too bad and it should not be doing this. I'd rather av7110 used
its own private timer for that.

> 
> Cc: Dmitry Torokhov <dmitry.torok...@gmail.com>
> Cc: Mauro Carvalho Chehab <mche...@kernel.org>
> Cc: Hans Verkuil <hans.verk...@cisco.com>
> Cc: Sakari Ailus <sakari.ai...@linux.intel.com>
> Cc: Geliang Tang <geliangt...@gmail.com>
> Cc: linux-in...@vger.kernel.org
> Cc: linux-media@vger.kernel.org
> Signed-off-by: Kees Cook <keesc...@chromium.org>
> Acked-by: Pali Rohár <pali.ro...@gmail.com>
> ---
>  drivers/input/input.c   | 12 ++--
>  drivers/media/pci/ttpci/av7110.h|  1 -
>  drivers/media/pci/ttpci/av7110_ir.c | 16 
>  include/linux/input.h   |  2 ++
>  4 files changed, 16 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/input/input.c b/drivers/input/input.c
> index d268fdc23c64..497ad2dcb699 100644
> --- a/drivers/input/input.c
> +++ b/drivers/input/input.c
> @@ -76,7 +76,7 @@ static void input_start_autorepeat(struct input_dev *dev, 
> int code)
>  {
>   if (test_bit(EV_REP, dev->evbit) &&
>   dev->rep[REP_PERIOD] && dev->rep[REP_DELAY] &&
> - dev->timer.data) {
> + dev->timer.function) {
>   dev->repeat_key = code;
>   mod_timer(>timer,
> jiffies + msecs_to_jiffies(dev->rep[REP_DELAY]));
> @@ -179,9 +179,9 @@ static void input_pass_event(struct input_dev *dev,
>   * dev->event_lock here to avoid racing with input_event
>   * which may cause keys get "stuck".
>   */
> -static void input_repeat_key(unsigned long data)
> +static void input_repeat_key(struct timer_list *t)
>  {
> - struct input_dev *dev = (void *) data;
> + struct input_dev *dev = from_timer(dev, t, timer);
>   unsigned long flags;
>  
>   spin_lock_irqsave(>event_lock, flags);
> @@ -1790,7 +1790,7 @@ struct input_dev *input_allocate_device(void)
>   device_initialize(>dev);
>   mutex_init(>mutex);
>   spin_lock_init(>event_lock);
> - init_timer(>timer);
> + timer_setup(>timer, NULL, 0);
>   INIT_LIST_HEAD(>h_list);
>   INIT_LIST_HEAD(>node);
>  
> @@ -2053,8 +2053,8 @@ static void devm_input_device_unregister(struct device 
> *dev, void *res)
>   */
>  void input_enable_softrepeat(struct input_dev *dev, int delay, int period)
>  {
> - dev->timer.data = (unsigned long) dev;
> - dev->timer.function = input_repeat_key;
> + dev->timer.function = (TIMER_FUNC_TYPE)input_repeat_key;
> + dev->timer_data = 0;
>   dev->rep[REP_DELAY] = delay;
>   dev->rep[REP_PERIOD] = period;
>  }
> diff --git a/drivers/media/pci/ttpci/av7110.h 
> b/drivers/media/pci/ttpci/av7110.h
> index 347827925c14..b98a4f3006df 100644
> --- a/drivers/media/pci/ttpci/av7110.h
> +++ b/drivers/media/pci/ttpci/av7110.h
> @@ -93,7 +93,6 @@ struct infrared {
>   u8  inversion;
>   u16 last_key;
>   u16 last_toggle;
> - u8  delay_timer_finished;
>  };
>  
>  
> diff --git a/drivers/media/pci/ttpci/av7110_ir.c 
> b/drivers/media/pci/ttpci/av7110_ir.c
> index ca05198de2c2..a883caa6488c 100644
> --- a/drivers/media/pci/ttpci/av7110_ir.c
> +++ b/drivers/media/pci/ttpci/av7110_ir.c
> @@ -155,16 +155,16 @@ static void av7110_emit_key(unsigned long parm)
>   if (timer_pending(>keyup_timer)) {
>   del_timer(>keyup_timer);
>   if (ir->last_key != keycode || toggle != ir->last_toggle) {
> - ir->delay_timer_finished = 0;
> + ir->input_dev->timer_data = 0;
>   input_event(ir->input_dev, EV_KEY, ir->last_key, 0);
>   input_event(ir->input_dev, EV_KEY, keycode, 1);
>   input_sync(ir->input_dev);
> - } else if (ir->delay_timer_finished) {
> + } else if (ir->input_dev->timer_data) {
>   

Re: [PATCH 08/14] Input: adxl34x - fix gcc-7 -Wint-in-bool-context warning

[Dmitry Torokhov]

On Fri, Jul 14, 2017 at 10:17:10PM +0200, Arnd Bergmann wrote:
> On Fri, Jul 14, 2017 at 9:24 PM, Linus Torvalds
>  wrote:
> > On Fri, Jul 14, 2017 at 2:25 AM, Arnd Bergmann  wrote:
> >> FIFO_MODE is an macro expression with a '<<' operator, which
> >> gcc points out could be misread as a '<':
> >
> > Yeah, no, NAK again.
> >
> > We don't make the code look worse just because gcc is being a f*cking
> > moron about things.
> >
> > This warning is clearly pure garbage.
> >
> 
> I looked at this one again and found a better approach, matching the
> check that is done a few lines later. Unless you find something wrong
> with that one, I'd resubmit it with the fixup below.
> 
>   Arnd
> 
> --- a/drivers/input/misc/adxl34x.c
> +++ b/drivers/input/misc/adxl34x.c
> @@ -789,21 +789,21 @@ struct adxl34x *adxl34x_probe(struct device *dev, int 
> irq,
> __set_bit(pdata->ev_code_ff, input_dev->keybit);
> }
> 
> if (pdata->ev_code_act_inactivity)
> __set_bit(pdata->ev_code_act_inactivity, input_dev->keybit);
> 
> ac->int_mask |= ACTIVITY | INACTIVITY;
> 
> if (pdata->watermark) {
> ac->int_mask |= WATERMARK;
> -   if (FIFO_MODE(pdata->fifo_mode) == 0)
> +   if (FIFO_MODE(pdata->fifo_mode) == FIFO_BYPASS)

This is better, not because of GCC, but it makes sense logically; 0 is
not a special value here.

Still, I am not sure that GCC is being that helpful here. Checking
result of shift for 0/non 0 with "!" is very common pattern.

Thanks.

-- 
Dmitry

Re: [PATCH 2/2] rainshadow-cec: new RainShadow Tech HDMI CEC driver

[Dmitry Torokhov]

Hi Hans,

On Fri, Feb 03, 2017 at 03:46:01PM +0100, Hans Verkuil wrote:
> Hi Dmitry,
> 
> Thanks for the feedback. See my comments below:
> 
> On 03/01/17 07:51, Dmitry Torokhov wrote:
> >Hi Hans,
> >
> >On Thu, Dec 15, 2016 at 02:02:07PM +0100, Hans Verkuil wrote:
> >>From: Hans Verkuil <hans.verk...@cisco.com>
> >>
> >>This driver supports the RainShadow Tech USB HDMI CEC adapter.
> >>
> >>See: http://rainshadowtech.com/HdmiCecUsb.html
> >>
> >>Signed-off-by: Hans Verkuil <hans.verk...@cisco.com>
> >>---
> >> MAINTAINERS   |   7 +
> >> drivers/media/usb/Kconfig |   1 +
> >> drivers/media/usb/Makefile|   1 +
> >> drivers/media/usb/rainshadow-cec/Kconfig  |  10 +
> >> drivers/media/usb/rainshadow-cec/Makefile |   1 +
> >> drivers/media/usb/rainshadow-cec/rainshadow-cec.c | 344 
> >> ++
> >> 6 files changed, 364 insertions(+)
> >> create mode 100644 drivers/media/usb/rainshadow-cec/Kconfig
> >> create mode 100644 drivers/media/usb/rainshadow-cec/Makefile
> >> create mode 100644 drivers/media/usb/rainshadow-cec/rainshadow-cec.c
> >>
> >>diff --git a/MAINTAINERS b/MAINTAINERS
> >>index 52cc077..78ebc5d 100644
> >>--- a/MAINTAINERS
> >>+++ b/MAINTAINERS
> >>@@ -10069,6 +10069,13 @@ L: linux-fb...@vger.kernel.org
> >> S: Maintained
> >> F: drivers/video/fbdev/aty/aty128fb.c
> >>
> >>+RAINSHADOW-CEC DRIVER
> >>+M: Hans Verkuil <hverk...@xs4all.nl>
> >>+L: linux-media@vger.kernel.org
> >>+T: git git://linuxtv.org/media_tree.git
> >>+S: Maintained
> >>+F: drivers/media/usb/rainshadow-cec/*
> >>+
> >> RALINK MIPS ARCHITECTURE
> >> M: John Crispin <j...@phrozen.org>
> >> L: linux-m...@linux-mips.org
> >>diff --git a/drivers/media/usb/Kconfig b/drivers/media/usb/Kconfig
> >>index c9644b6..b24e753 100644
> >>--- a/drivers/media/usb/Kconfig
> >>+++ b/drivers/media/usb/Kconfig
> >>@@ -63,6 +63,7 @@ endif
> >> if MEDIA_CEC_SUPPORT
> >>comment "USB HDMI CEC adapters"
> >> source "drivers/media/usb/pulse8-cec/Kconfig"
> >>+source "drivers/media/usb/rainshadow-cec/Kconfig"
> >> endif
> >>
> >> endif #MEDIA_USB_SUPPORT
> >>diff --git a/drivers/media/usb/Makefile b/drivers/media/usb/Makefile
> >>index 0f15e33..738b993 100644
> >>--- a/drivers/media/usb/Makefile
> >>+++ b/drivers/media/usb/Makefile
> >>@@ -25,3 +25,4 @@ obj-$(CONFIG_VIDEO_USBTV) += usbtv/
> >> obj-$(CONFIG_VIDEO_GO7007) += go7007/
> >> obj-$(CONFIG_DVB_AS102) += as102/
> >> obj-$(CONFIG_USB_PULSE8_CEC) += pulse8-cec/
> >>+obj-$(CONFIG_USB_RAINSHADOW_CEC) += rainshadow-cec/
> >>diff --git a/drivers/media/usb/rainshadow-cec/Kconfig 
> >>b/drivers/media/usb/rainshadow-cec/Kconfig
> >>new file mode 100644
> >>index 000..447291b
> >>--- /dev/null
> >>+++ b/drivers/media/usb/rainshadow-cec/Kconfig
> >>@@ -0,0 +1,10 @@
> >>+config USB_RAINSHADOW_CEC
> >>+   tristate "RainShadow Tech HDMI CEC"
> >>+   depends on USB_ACM && MEDIA_CEC_SUPPORT
> >>+   select SERIO
> >>+   select SERIO_SERPORT
> >>+   ---help---
> >>+ This is a cec driver for the RainShadow Tech HDMI CEC device.
> >>+
> >>+ To compile this driver as a module, choose M here: the
> >>+ module will be called rainshadow-cec.
> >>diff --git a/drivers/media/usb/rainshadow-cec/Makefile 
> >>b/drivers/media/usb/rainshadow-cec/Makefile
> >>new file mode 100644
> >>index 000..a79fbc7
> >>--- /dev/null
> >>+++ b/drivers/media/usb/rainshadow-cec/Makefile
> >>@@ -0,0 +1 @@
> >>+obj-$(CONFIG_USB_RAINSHADOW_CEC) += rainshadow-cec.o
> >>diff --git a/drivers/media/usb/rainshadow-cec/rainshadow-cec.c 
> >>b/drivers/media/usb/rainshadow-cec/rainshadow-cec.c
> >>new file mode 100644
> >>index 000..dc7f287
> >>--- /dev/null
> >>+++ b/drivers/media/usb/rainshadow-cec/rainshadow-cec.c
> >>@@ -0,0 +1,344 @@
> >>+/*
> >>+ * RainShadow Tech HDMI CEC driver
> >>+ *
> >>+ * Copyright 2016 Hans Verkuil <hverk...@xs4all.nl
> >>+ *
> >>+ * This program is free software; you can redistribute it and/or modify it
> >>+ 

Re: [PATCHv2 1/2] serio.h: add SERIO_RAINSHADOW_CEC ID

[Dmitry Torokhov]

On Fri, Feb 03, 2017 at 04:26:32PM +0100, Hans Verkuil wrote:
> From: Hans Verkuil <hans.verk...@cisco.com>
> 
> Add a new serio ID for the RainShadow Tech USB HDMI CEC adapter.
> 
> Signed-off-by: Hans Verkuil <hans.verk...@cisco.com>

Acked-by: Dmitry Torokhov <dmitry.torok...@gmail.com>

> ---
>  include/uapi/linux/serio.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/include/uapi/linux/serio.h b/include/uapi/linux/serio.h
> index f2447a8..f42e919 100644
> --- a/include/uapi/linux/serio.h
> +++ b/include/uapi/linux/serio.h
> @@ -79,5 +79,6 @@
>  #define SERIO_WACOM_IV   0x3e
>  #define SERIO_EGALAX 0x3f
>  #define SERIO_PULSE8_CEC 0x40
> +#define SERIO_RAINSHADOW_CEC 0x41
>  
>  #endif /* _UAPI_SERIO_H */
> -- 
> 2.10.2
> 

-- 
Dmitry

[PATCH] [media] Staging: media: radio-bcm2048: remove incorrect __exit markups

[Dmitry Torokhov]

Even if bus is not hot-pluggable, devices can be unbound from the
driver via sysfs, so we should not be using __exit annotations on
remove() methods. The only exception is drivers registered with
platform_driver_probe() which specifically disables sysfs bind/unbind
attributes.

Signed-off-by: Dmitry Torokhov <dmitry.torok...@gmail.com>
---
 drivers/staging/media/bcm2048/radio-bcm2048.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/media/bcm2048/radio-bcm2048.c 
b/drivers/staging/media/bcm2048/radio-bcm2048.c
index 37bd439ee08b..1fba377f816b 100644
--- a/drivers/staging/media/bcm2048/radio-bcm2048.c
+++ b/drivers/staging/media/bcm2048/radio-bcm2048.c
@@ -2634,7 +2634,7 @@ static int bcm2048_i2c_driver_probe(struct i2c_client 
*client,
return err;
 }
 
-static int __exit bcm2048_i2c_driver_remove(struct i2c_client *client)
+static int bcm2048_i2c_driver_remove(struct i2c_client *client)
 {
struct bcm2048_device *bdev = i2c_get_clientdata(client);
 
@@ -2673,7 +2673,7 @@ static struct i2c_driver bcm2048_i2c_driver = {
.name   = BCM2048_DRIVER_NAME,
},
.probe  = bcm2048_i2c_driver_probe,
-   .remove = __exit_p(bcm2048_i2c_driver_remove),
+   .remove = bcm2048_i2c_driver_remove,
.id_table   = bcm2048_id,
 };
 
-- 
2.12.0.rc1.440.g5b76565f74-goog


-- 
Dmitry

[PATCH] [media] ad5820: remove incorrect __exit markups

[Dmitry Torokhov]

Even if bus is not hot-pluggable, devices can be unbound from the
driver via sysfs, so we should not be using __exit annotations on
remove() methods. The only exception is drivers registered with
platform_driver_probe() which specifically disables sysfs bind/unbind
attributes.

Signed-off-by: Dmitry Torokhov <dmitry.torok...@gmail.com>
---
 drivers/media/i2c/ad5820.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/ad5820.c b/drivers/media/i2c/ad5820.c
index a9026a91855e..3d2a3c6b67d8 100644
--- a/drivers/media/i2c/ad5820.c
+++ b/drivers/media/i2c/ad5820.c
@@ -336,7 +336,7 @@ static int ad5820_probe(struct i2c_client *client,
return ret;
 }
 
-static int __exit ad5820_remove(struct i2c_client *client)
+static int ad5820_remove(struct i2c_client *client)
 {
struct v4l2_subdev *subdev = i2c_get_clientdata(client);
struct ad5820_device *coil = to_ad5820_device(subdev);
@@ -362,7 +362,7 @@ static struct i2c_driver ad5820_i2c_driver = {
.pm = _pm,
},
.probe  = ad5820_probe,
-   .remove = __exit_p(ad5820_remove),
+   .remove = ad5820_remove,
.id_table   = ad5820_id_table,
 };
 
-- 
2.12.0.rc1.440.g5b76565f74-goog


-- 
Dmitry

Re: [PATCH 2/2] rainshadow-cec: new RainShadow Tech HDMI CEC driver

[Dmitry Torokhov]

Hi Hans,

On Thu, Dec 15, 2016 at 02:02:07PM +0100, Hans Verkuil wrote:
> From: Hans Verkuil 
> 
> This driver supports the RainShadow Tech USB HDMI CEC adapter.
> 
> See: http://rainshadowtech.com/HdmiCecUsb.html
> 
> Signed-off-by: Hans Verkuil 
> ---
>  MAINTAINERS   |   7 +
>  drivers/media/usb/Kconfig |   1 +
>  drivers/media/usb/Makefile|   1 +
>  drivers/media/usb/rainshadow-cec/Kconfig  |  10 +
>  drivers/media/usb/rainshadow-cec/Makefile |   1 +
>  drivers/media/usb/rainshadow-cec/rainshadow-cec.c | 344 
> ++
>  6 files changed, 364 insertions(+)
>  create mode 100644 drivers/media/usb/rainshadow-cec/Kconfig
>  create mode 100644 drivers/media/usb/rainshadow-cec/Makefile
>  create mode 100644 drivers/media/usb/rainshadow-cec/rainshadow-cec.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 52cc077..78ebc5d 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -10069,6 +10069,13 @@ L:   linux-fb...@vger.kernel.org
>  S:   Maintained
>  F:   drivers/video/fbdev/aty/aty128fb.c
>  
> +RAINSHADOW-CEC DRIVER
> +M:   Hans Verkuil 
> +L:   linux-media@vger.kernel.org
> +T:   git git://linuxtv.org/media_tree.git
> +S:   Maintained
> +F:   drivers/media/usb/rainshadow-cec/*
> +
>  RALINK MIPS ARCHITECTURE
>  M:   John Crispin 
>  L:   linux-m...@linux-mips.org
> diff --git a/drivers/media/usb/Kconfig b/drivers/media/usb/Kconfig
> index c9644b6..b24e753 100644
> --- a/drivers/media/usb/Kconfig
> +++ b/drivers/media/usb/Kconfig
> @@ -63,6 +63,7 @@ endif
>  if MEDIA_CEC_SUPPORT
>   comment "USB HDMI CEC adapters"
>  source "drivers/media/usb/pulse8-cec/Kconfig"
> +source "drivers/media/usb/rainshadow-cec/Kconfig"
>  endif
>  
>  endif #MEDIA_USB_SUPPORT
> diff --git a/drivers/media/usb/Makefile b/drivers/media/usb/Makefile
> index 0f15e33..738b993 100644
> --- a/drivers/media/usb/Makefile
> +++ b/drivers/media/usb/Makefile
> @@ -25,3 +25,4 @@ obj-$(CONFIG_VIDEO_USBTV) += usbtv/
>  obj-$(CONFIG_VIDEO_GO7007) += go7007/
>  obj-$(CONFIG_DVB_AS102) += as102/
>  obj-$(CONFIG_USB_PULSE8_CEC) += pulse8-cec/
> +obj-$(CONFIG_USB_RAINSHADOW_CEC) += rainshadow-cec/
> diff --git a/drivers/media/usb/rainshadow-cec/Kconfig 
> b/drivers/media/usb/rainshadow-cec/Kconfig
> new file mode 100644
> index 000..447291b
> --- /dev/null
> +++ b/drivers/media/usb/rainshadow-cec/Kconfig
> @@ -0,0 +1,10 @@
> +config USB_RAINSHADOW_CEC
> + tristate "RainShadow Tech HDMI CEC"
> + depends on USB_ACM && MEDIA_CEC_SUPPORT
> + select SERIO
> + select SERIO_SERPORT
> + ---help---
> +   This is a cec driver for the RainShadow Tech HDMI CEC device.
> +
> +   To compile this driver as a module, choose M here: the
> +   module will be called rainshadow-cec.
> diff --git a/drivers/media/usb/rainshadow-cec/Makefile 
> b/drivers/media/usb/rainshadow-cec/Makefile
> new file mode 100644
> index 000..a79fbc7
> --- /dev/null
> +++ b/drivers/media/usb/rainshadow-cec/Makefile
> @@ -0,0 +1 @@
> +obj-$(CONFIG_USB_RAINSHADOW_CEC) += rainshadow-cec.o
> diff --git a/drivers/media/usb/rainshadow-cec/rainshadow-cec.c 
> b/drivers/media/usb/rainshadow-cec/rainshadow-cec.c
> new file mode 100644
> index 000..dc7f287
> --- /dev/null
> +++ b/drivers/media/usb/rainshadow-cec/rainshadow-cec.c
> @@ -0,0 +1,344 @@
> +/*
> + * RainShadow Tech HDMI CEC driver
> + *
> + * Copyright 2016 Hans Verkuil  + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License as published by the
> + * Free Software Foundation; either version of 2 of the License, or (at your
> + * option) any later version. See the file COPYING in the main directory of
> + * this archive for more details.
> + */
> +
> +/*
> + * Notes:
> + *
> + * The higher level protocols are currently disabled. This can be added
> + * later, similar to how this is done for the Pulse Eight CEC driver.
> + *
> + * Documentation of the protocol is available here:
> + *
> + * http://rainshadowtech.com/doc/HDMICECtoUSBandRS232v2.0.pdf
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +
> +MODULE_AUTHOR("Hans Verkuil ");
> +MODULE_DESCRIPTION("RainShadow Tech HDMI CEC driver");
> +MODULE_LICENSE("GPL");
> +
> +static int debug;
> +module_param(debug, int, 0644);
> +MODULE_PARM_DESC(debug, "debug level (0-1)");
> +
> +#define DATA_SIZE 256
> +
> +struct rain {
> + struct device *dev;
> + struct serio *serio;
> + struct cec_adapter *adap;
> + struct completion cmd_done;
> + struct work_struct work;
> + struct cec_msg rx_msg;
> + char data[DATA_SIZE];
> + char buf[DATA_SIZE];
> + unsigned int idx;
> + 

Re: [RFC] Input: synaptics-rmi4 - fix out-of-bounds memory access

[Dmitry Torokhov]

Hi Guenter,

On Sat, Nov 19, 2016 at 07:46:58PM -0800, Guenter Roeck wrote:
> Kasan reports:
> 
> BUG: KASAN: slab-out-of-bounds in __fill_v4l2_buffer+0xc3/0x540
>   [videobuf2_v4l2] at addr 8806c5e0c6cc
> Read of size 4 by task heatmap/14414
> CPU: 2 PID: 14414 Comm: heatmap Tainted: GB  OE 4.9.0-rc5+ #1
> Hardware name: MSI MS-7924/H97M-G43(MS-7924), BIOS V2.0 04/15/2014
> 88010cf57940 81606978 8806fe803080 8806c5e0c500
> 88010cf57968 812fd131 88010cf579f8 8806c5e0c500
> 8806fe803080 88010cf579e8 812fd3ca 0010
> Call Trace:
> [] dump_stack+0x63/0x8b
> [] kasan_object_err+0x21/0x70
> [] kasan_report_error+0x1fa/0x4d0
> [] kasan_report+0x39/0x40
> [] ? __asan_load4+0x80/0x80
> [] ? __fill_v4l2_buffer+0xc3/0x540 [videobuf2_v4l2]
> [] __asan_load4+0x61/0x80
> [] __fill_v4l2_buffer+0xc3/0x540 [videobuf2_v4l2]
> [] ? __enqueue_in_driver+0xed/0x180 [videobuf2_core]
> [] vb2_core_qbuf+0x191/0x320 [videobuf2_core]
> [] vb2_qbuf+0x69/0x90 [videobuf2_v4l2]
> [] vb2_ioctl_qbuf+0x73/0x80 [videobuf2_v4l2]
> [] v4l_qbuf+0x50/0x60 [videodev]
> [] __video_do_ioctl+0x46f/0x4f0 [videodev]
> [] ? video_ioctl2+0x20/0x20 [videodev]
> [] ? __wake_up+0x44/0x50
> [] video_usercopy+0x3b4/0x710 [videodev]
> [] ? video_ioctl2+0x20/0x20 [videodev]
> [] ? v4l_enum_fmt+0x1290/0x1290 [videodev]
> [] ? do_loop_readv_writev+0x130/0x130
> [] video_ioctl2+0x15/0x20 [videodev]
> [] v4l2_ioctl+0x123/0x160 [videodev]
> [] do_vfs_ioctl+0x12e/0x8c0
> [] ? ioctl_preallocate+0x140/0x140
> [] ? __fsnotify_parent+0x2c/0x130
> [] ? SyS_rt_sigaction+0xfa/0x160
> [] ? SyS_sigprocmask+0x1f0/0x1f0
> [] ? __fget_light+0xa7/0xc0
> [] SyS_ioctl+0x79/0x90
> [] entry_SYSCALL_64_fastpath+0x1e/0xad
> Object at 8806c5e0c500, in cache kmalloc-512 size: 512
> Allocated:
> PID = 14414
> [] save_stack_trace+0x1b/0x20
> [] save_stack+0x46/0xd0
> [] kasan_kmalloc+0xad/0xe0
> [] __kmalloc+0x12f/0x210
> [] __vb2_queue_alloc+0x9d/0x6a0 [videobuf2_core]
> [] vb2_core_reqbufs+0x2da/0x640 [videobuf2_core]
> [] vb2_ioctl_reqbufs+0xd8/0x130 [videobuf2_v4l2]
> [] v4l_reqbufs+0x60/0x70 [videodev]
> [] __video_do_ioctl+0x46f/0x4f0 [videodev]
> [] video_usercopy+0x3b4/0x710 [videodev]
> [] video_ioctl2+0x15/0x20 [videodev]
> [] v4l2_ioctl+0x123/0x160 [videodev]
> [] do_vfs_ioctl+0x12e/0x8c0
> [] SyS_ioctl+0x79/0x90
> [] entry_SYSCALL_64_fastpath+0x1e/0xad
> Freed:
> PID = 1717
> [] save_stack_trace+0x1b/0x20
> [] save_stack+0x46/0xd0
> [] kasan_slab_free+0x71/0xb0
> [] kfree+0x94/0x190
> [] kvfree+0x2a/0x40
> [] i915_gem_execbuffer2+0x1d6/0x2e0 [i915]
> [] drm_ioctl+0x35b/0x640 [drm]
> [] do_vfs_ioctl+0x12e/0x8c0
> [] SyS_ioctl+0x79/0x90
> [] entry_SYSCALL_64_fastpath+0x1e/0xad
> 
> The problematic code is
>   b->flags = vbuf->flags;
> and all other code in __fill_v4l2_buffer() accessing variables from
> struct vb2_v4l2_buffer. That buffer is actually allocated as struct
> vb2_buffer. Accessing the flags in struct vb2_v4l2_buffer beyond struct
> vb2_buffer is causing the KASAN report.
> 
> Fixes: 3a762dbd5347 ("[media] Input: synaptics-rmi4 - add support for F54 
> ...")
> Cc: Nick Dyer 
> Signed-off-by: Guenter Roeck 
> ---
> RFC because if I have no idea if the fix is correct. Sure, KASAN is silent 
> with
> this fix, but what bothers me is that the error is reported when copying
> variables from struct vb2_v4l2_buffer to struct v4l2_buffer, not earlier.
> This suggests that those variables (flags, field, timecode, sequence) are 
> never
> written in the first place. Maybe that is as expected, and the variables are
> not supposed to be written, but I don't know that subsystem well enough to
> know the expected behavior.

I think we best ask media folks (CCed).

> 
>  drivers/input/rmi4/rmi_f54.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/input/rmi4/rmi_f54.c b/drivers/input/rmi4/rmi_f54.c
> index cf805b960866..ef91633acb6b 100644
> --- a/drivers/input/rmi4/rmi_f54.c
> +++ b/drivers/input/rmi4/rmi_f54.c
> @@ -363,7 +363,7 @@ static const struct vb2_ops rmi_f54_queue_ops = {
>  static const struct vb2_queue rmi_f54_queue = {
>   .type = V4L2_BUF_TYPE_VIDEO_CAPTURE,
>   .io_modes = VB2_MMAP | VB2_USERPTR | VB2_DMABUF | VB2_READ,
> - .buf_struct_size = sizeof(struct vb2_buffer),
> + .buf_struct_size = sizeof(struct vb2_v4l2_buffer),
>   .ops = _f54_queue_ops,
>   .mem_ops = _vmalloc_memops,
>   .timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC,
> -- 
> 2.5.0
> 

-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v8 03/10] Input: atmel_mxt_ts - add support for T37 diagnostic data

[Dmitry Torokhov]

On Tue, Aug 23, 2016 at 04:30:47PM -0300, Mauro Carvalho Chehab wrote:
> Hi Dmitry,
> 
> Em Mon, 18 Jul 2016 22:10:31 +0100
> Nick Dyer <n...@shmanahar.org> escreveu:
> 
> > Atmel maXTouch devices have a T37 object which can be used to read raw
> > touch deltas from the device. This consists of an array of 16-bit
> > integers, one for each node on the touchscreen matrix.
> 
> Is it ok to merge this patch (and the other patches on this series)
> via my tree?


Yes, please (I do not think I have any other Atmel changes pending).
Feel free to add my

Acked-by: Dmitry Torokhov <dmitry.torok...@gmail.com>

Thanks.

-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv2] serio: add hangup support

[Dmitry Torokhov]

On Wed, Aug 03, 2016 at 01:00:44PM +0200, Hans Verkuil wrote:
> 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 
> Tested-by: Hans Verkuil 
> ---
> 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(>lock, flags);
>   clear_bit(SERPORT_ACTIVE, >flags);
> - set_bit(SERPORT_DEAD, >flags);
>   spin_unlock_irqrestore(>lock, flags);
> 
>   wake_up_interruptible(>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.

> @@ -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(>lock, flags);
> + set_bit(SERPORT_DEAD, >flags);
> + spin_unlock_irqrestore(>lock, flags);
> +
> + wake_up_interruptible(>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.

-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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.verk...@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(>lock, flags);
> > set_bit(SERPORT_DEAD, >flags);
> > spin_unlock_irqrestore(>lock, flags);
> > 
> > wake_up_interruptible(>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
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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 
> 
> ---
> 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(>lock, flags);
set_bit(SERPORT_DEAD, >flags);
spin_unlock_irqrestore(>lock, flags);

wake_up_interruptible(>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
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/5] serio.h: add new define for the Pulse-Eight USB-CEC Adapter

[Dmitry Torokhov]

On Sun, Jul 10, 2016 at 03:11:18PM +0200, Hans Verkuil wrote:
> From: Hans Verkuil <hans.verk...@cisco.com>
> 
> This is for the new pulse8-cec staging driver.
> 
> Signed-off-by: Hans Verkuil <hans.verk...@cisco.com>
> Cc: Dmitry Torokhov <dmitry.torok...@gmail.com>


Acked-by: Dmitry Torokhov <dmitry.torok...@gmail.com>

Please feel free to merge through media tree. If you could change the
subject to read:

Input: serio - add new protocol for the Pulse-Eight USB-CEC Adapter

That would be great.

Thanks!

> ---
>  include/uapi/linux/serio.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/include/uapi/linux/serio.h b/include/uapi/linux/serio.h
> index c2ea169..f2447a8 100644
> --- a/include/uapi/linux/serio.h
> +++ b/include/uapi/linux/serio.h
> @@ -78,5 +78,6 @@
>  #define SERIO_TSC40  0x3d
>  #define SERIO_WACOM_IV   0x3e
>  #define SERIO_EGALAX 0x3f
> +#define SERIO_PULSE8_CEC 0x40
>  
>  #endif /* _UAPI_SERIO_H */
> -- 
> 2.8.1
> 

-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 1/9] [media] v4l2-core: Add support for touch devices

[Dmitry Torokhov]

On Wed, Jun 22, 2016 at 11:08:25PM +0100, Nick Dyer wrote:
> Some touch controllers send out touch data in a similar way to a
> greyscale frame grabber.
> 
> Use a new device prefix v4l-touch for these devices, to stop generic
> capture software from treating them as webcams.
> 
> Add formats:
> - V4L2_TCH_FMT_DELTA_TD16 for signed 16-bit touch deltas
> - V4L2_TCH_FMT_DELTA_TD08 for signed 16-bit touch deltas
> - V4L2_TCH_FMT_TU16 for unsigned 16-bit touch data
> - V4L2_TCH_FMT_TU08 for unsigned 8-bit touch data
> 
> This support will be used by:
> * Atmel maXTouch (atmel_mxt_ts)
> * Synaptics RMI4.
> * sur40
> 
> Signed-off-by: Nick Dyer 

Hans/Mauro, when you merge this can you make a stable branch (maybe
based on 4.6) so I can pull it in as well and then merge the rest?

Thanks!

-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 8/9] Input: synaptics-rmi4 - add support for F54 diagnostics

[Dmitry Torokhov]

Hi Nick,

On Wed, Jun 22, 2016 at 11:08:32PM +0100, Nick Dyer wrote:
> Function 54 implements access to various RMI4 diagnostic features.
> 
> This patch adds support for retrieving this data. It registers a V4L2
> device to output the data to user space.
> 
> Signed-off-by: Nick Dyer 
> ---
>  drivers/input/rmi4/Kconfig  |  11 +
>  drivers/input/rmi4/Makefile |   1 +
>  drivers/input/rmi4/rmi_bus.c|   3 +
>  drivers/input/rmi4/rmi_driver.h |   1 +
>  drivers/input/rmi4/rmi_f54.c| 730 
> 
>  5 files changed, 746 insertions(+)
>  create mode 100644 drivers/input/rmi4/rmi_f54.c
> 
> diff --git a/drivers/input/rmi4/Kconfig b/drivers/input/rmi4/Kconfig
> index f73df24..f3418b6 100644
> --- a/drivers/input/rmi4/Kconfig
> +++ b/drivers/input/rmi4/Kconfig
> @@ -61,3 +61,14 @@ config RMI4_F30
>  
> Function 30 provides GPIO and LED support for RMI4 devices. This
> includes support for buttons on TouchPads and ClickPads.
> +
> +config RMI4_F54
> + bool "RMI4 Function 54 (Analog diagnostics)"
> + depends on RMI4_CORE
> + depends on VIDEO_V4L2
> + select VIDEOBUF2_VMALLOC
> + help
> +   Say Y here if you want to add support for RMI4 function 54
> +
> +   Function 54 provides access to various diagnostic features in certain
> +   RMI4 touch sensors.
> diff --git a/drivers/input/rmi4/Makefile b/drivers/input/rmi4/Makefile
> index 95c00a7..0bafc85 100644
> --- a/drivers/input/rmi4/Makefile
> +++ b/drivers/input/rmi4/Makefile
> @@ -7,6 +7,7 @@ rmi_core-$(CONFIG_RMI4_2D_SENSOR) += rmi_2d_sensor.o
>  rmi_core-$(CONFIG_RMI4_F11) += rmi_f11.o
>  rmi_core-$(CONFIG_RMI4_F12) += rmi_f12.o
>  rmi_core-$(CONFIG_RMI4_F30) += rmi_f30.o
> +rmi_core-$(CONFIG_RMI4_F54) += rmi_f54.o
>  
>  # Transports
>  obj-$(CONFIG_RMI4_I2C) += rmi_i2c.o
> diff --git a/drivers/input/rmi4/rmi_bus.c b/drivers/input/rmi4/rmi_bus.c
> index b368b05..3aedc65 100644
> --- a/drivers/input/rmi4/rmi_bus.c
> +++ b/drivers/input/rmi4/rmi_bus.c
> @@ -315,6 +315,9 @@ static struct rmi_function_handler *fn_handlers[] = {
>  #ifdef CONFIG_RMI4_F30
>   _f30_handler,
>  #endif
> +#ifdef CONFIG_RMI4_F54
> + _f54_handler,
> +#endif
>  };
>  
>  static void __rmi_unregister_function_handlers(int start_idx)
> diff --git a/drivers/input/rmi4/rmi_driver.h b/drivers/input/rmi4/rmi_driver.h
> index 6e140fa..8dfbebe 100644
> --- a/drivers/input/rmi4/rmi_driver.h
> +++ b/drivers/input/rmi4/rmi_driver.h
> @@ -102,4 +102,5 @@ extern struct rmi_function_handler rmi_f01_handler;
>  extern struct rmi_function_handler rmi_f11_handler;
>  extern struct rmi_function_handler rmi_f12_handler;
>  extern struct rmi_function_handler rmi_f30_handler;
> +extern struct rmi_function_handler rmi_f54_handler;
>  #endif
> diff --git a/drivers/input/rmi4/rmi_f54.c b/drivers/input/rmi4/rmi_f54.c
> new file mode 100644
> index 000..df4c821
> --- /dev/null
> +++ b/drivers/input/rmi4/rmi_f54.c
> @@ -0,0 +1,730 @@
> +/*
> + * Copyright (c) 2012-2015 Synaptics Incorporated
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published 
> by
> + * the Free Software Foundation.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include "rmi_driver.h"
> +
> +#define F54_NAME "rmi4_f54"
> +
> +/* F54 data offsets */
> +#define F54_REPORT_DATA_OFFSET  3
> +#define F54_FIFO_OFFSET 1
> +#define F54_NUM_TX_OFFSET   1
> +#define F54_NUM_RX_OFFSET   0
> +
> +/* F54 commands */
> +#define F54_GET_REPORT  1
> +#define F54_FORCE_CAL   2
> +
> +/* Fixed sizes of reports */
> +#define F54_QUERY_LEN27
> +
> +/* F54 capabilities */
> +#define F54_CAP_BASELINE (1 << 2)
> +#define F54_CAP_IMAGE8   (1 << 3)
> +#define F54_CAP_IMAGE16  (1 << 6)
> +
> +enum rmi_f54_report_type {
> + F54_REPORT_NONE = 0,
> + F54_8BIT_IMAGE = 1,
> + F54_16BIT_IMAGE = 2,
> + F54_RAW_16BIT_IMAGE = 3,
> + F54_TRUE_BASELINE = 9,
> + F54_FULL_RAW_CAP = 19,
> + F54_FULL_RAW_CAP_RX_COUPLING_COMP = 20,
> + F54_MAX_REPORT_TYPE,
> +};
> +
> +const char *rmi_f54_report_type_names[] = {
> + [F54_REPORT_NONE]   = "No report",
> + [F54_8BIT_IMAGE]= "8 bit image",
> + [F54_16BIT_IMAGE]   = "16 bit image",
> + [F54_RAW_16BIT_IMAGE]   = "Raw 16 bit image",
> + [F54_TRUE_BASELINE] = "True baseline",
> + [F54_FULL_RAW_CAP]  = "Full raw cap",
> + [F54_FULL_RAW_CAP_RX_COUPLING_COMP]
> + = "Full raw cap RX coupling comp",
> + [F54_MAX_REPORT_TYPE]   = "Max report type",
> +};
> +
> +#define f54_reptype_name(a) (((unsigned)(a)) < 
> 

Re: [PATCH 0/2] input: add support for HDMI CEC

[Dmitry Torokhov]

On Sat, Jun 18, 2016 at 02:44:08PM -0300, Mauro Carvalho Chehab wrote:
> Em Sat, 18 Jun 2016 18:56:24 +0200
> Hans Verkuil <hverk...@xs4all.nl> escreveu:
> 
> > On 06/18/2016 06:26 PM, Dmitry Torokhov wrote:
> > > Hi Hans,
> > > 
> > > On Sat, Jun 18, 2016 at 04:50:26PM +0200, Hans Verkuil wrote:  
> > >> From: Hans Verkuil <hans.verk...@cisco.com>
> > >>
> > >> Hi Dmitry,
> > >>
> > >> This patch series adds input support for the HDMI CEC bus through which
> > >> remote control keys can be passed from one HDMI device to another.
> > >>
> > >> This has been posted before as part of the HDMI CEC patch series. We are
> > >> going to merge that in linux-media for 4.8, but these two patches have to
> > >> go through linux-input.
> > >>
> > >> Only the rc-cec keymap file depends on this, and we will take care of 
> > >> that
> > >> dependency (we'll postpone merging that until both these input patches 
> > >> and
> > >> our own CEC patches have been merged in mainline).  
> > > 
> > > If it would be easier for you I am perfectly fine with these patches
> > > going through media tree; you have my acks on them.  
> > 
> > You're not expecting any changes to these headers for 4.8 that might
> > cause merge conflicts? That was Mauro's concern.
> > 
> > If not, then I would prefer it to go through the media tree to simplify
> > the dependencies, but it's up to Mauro.
> 
> Hi Dmitry,
> 
> My main concern is with patch 2, as it could conflict with some other
> patch on your tree, as I suspect it should be somewhat common to add
> new keystrokes from time to time, on your tree. As there's just one patch 
> affected by it from Hans CEC patch series, with adds the keymap to be 
> used by the remote controller CEC patch, it won't be a big issue to 
> delay just that patch to be sent upstream after both your and my trees 
> would be merged.

Mauro,

I created an immutable branch "cec-defines" based on 4.6; I'll also
merge it into 4.7.

Thanks.

-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/2] input: add support for HDMI CEC

[Dmitry Torokhov]

Hi Hans,

On Sat, Jun 18, 2016 at 04:50:26PM +0200, Hans Verkuil wrote:
> From: Hans Verkuil 
> 
> Hi Dmitry,
> 
> This patch series adds input support for the HDMI CEC bus through which
> remote control keys can be passed from one HDMI device to another.
> 
> This has been posted before as part of the HDMI CEC patch series. We are
> going to merge that in linux-media for 4.8, but these two patches have to
> go through linux-input.
> 
> Only the rc-cec keymap file depends on this, and we will take care of that
> dependency (we'll postpone merging that until both these input patches and
> our own CEC patches have been merged in mainline).

If it would be easier for you I am perfectly fine with these patches
going through media tree; you have my acks on them.

Thanks.

-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv16 08/13] DocBook/media: add CEC documentation

[Dmitry Torokhov]

On Fri, Jun 17, 2016 at 08:37:38AM -0300, Mauro Carvalho Chehab wrote:
> Em Fri, 17 Jun 2016 13:09:10 +0200
> Hans Verkuil  escreveu:
> 
> > On 06/17/2016 11:50 AM, Mauro Carvalho Chehab wrote:
> > One area where I am uncertain is when remote control messages are received 
> > and
> > passed on by the framework to the RC input device.
> > 
> > Suppose the application is the one receiving a password, then that password 
> > appears
> > both in the input device and the cec device. What I think will be useful is 
> > if the
> > application can prevent the use of an input device to pass on remote 
> > control messages.
> > 
> > CEC_ADAP_S_LOG_ADDRS has a flags field that I intended for just that 
> > purpose.
> > 
> > Note that RC messages are always passed on to CEC followers even if there 
> > is an
> > input device since some RC messages have additional arguments that the rc 
> > subsystem
> > can't handle. Also I think that it is often easier to handle all messages 
> > from the
> > same CEC device instead of having to read from two devices (cec and input). 
> > I
> > actually considered removing the input support, but it turned out to be 
> > useful in
> > existing video streaming apps since they don't need to add special cec 
> > support to
> > handle remote control presses.
> > 
> > Question: is there a way for applications to get exclusive access to an 
> > input device?
> > Or can anyone always read from it?
> 
> That's a very good question. I did a quick test to check how this is
> currently protected, by running:
> 
> $ strace ir-keytable -t
> ...
> open("/dev/input/event12", O_RDONLY)= -1 EACCES (Permission denied)
> ...
> 
> It turns that the input device was created by udev with those
> permissions:
> 
> crw-rw 1 root input 13, 76 Jun 17 08:26 /dev/input/event12
> 
> Changing access to 666 allowed to run ir-keytable -t without the
> need of being root.
> 
> Yet, maybe there's a way to get exclusive access to input/event
> device, but I never needed to go that deep at the input subsystem.
> Maybe Dmitry could shed some light on that. Adding him in the loop.

EVIOCGRAB ioctl will do what you want.

Thanks.

-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 0/8] Input: atmel_mxt_ts - output raw touch diagnostic data via V4L

[Dmitry Torokhov]

On Wed, Jun 01, 2016 at 05:39:44PM +0100, Nick Dyer wrote:
> This is a series of patches to add diagnostic data support to the Atmel
> maXTouch driver. It's a rewrite of the previous implementation which output 
> via
> debugfs: it now uses a V4L2 device in a similar way to the sur40 driver.
> 
> There are significant performance advantages to putting this code into the
> driver.  The algorithm for retrieving the data has been fairly consistent
> across a range of chips, with the exception of the mXT1386 series (see patch).
> 
> We have a utility which can read the data and display it in a useful format:
> https://github.com/ndyer/heatmap/commits/heatmap-v4l
> 
> These patches are also available from
> https://github.com/ndyer/linux/commits/diagnostic-v4l
> 
> Changes in v3:
> - Address V4L2 review comments from Hans Verkuil
> - Run v4l-compliance and fix all issues - needs minor patch here:
>   https://github.com/ndyer/v4l-utils/commit/cf50469773f
> 
> Changes in v2:
> - Split pixfmt changes into separate commit and add DocBook
> - Introduce VFL_TYPE_TOUCH_SENSOR and /dev/v4l-touch
> - Remove "single node" support for now, it may be better to treat it as 
> metadata later
> - Explicitly set VFL_DIR_RX
> - Fix Kconfig
> 

I do not have any objections other than some nits form the input side;
majority of the review should come from V4L2 side here...

-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 4/8] Input: atmel_mxt_ts - output diagnostic debug via v4l2 device

[Dmitry Torokhov]

On Wed, Jun 01, 2016 at 05:39:48PM +0100, Nick Dyer wrote:
> Register a video device to output T37 diagnostic data.
> 
> Signed-off-by: Nick Dyer 
> ---
>  drivers/input/touchscreen/Kconfig|   2 +
>  drivers/input/touchscreen/atmel_mxt_ts.c | 247 
> +++
>  2 files changed, 249 insertions(+)
> 
> diff --git a/drivers/input/touchscreen/Kconfig 
> b/drivers/input/touchscreen/Kconfig
> index 8ecdc38..73154b6 100644
> --- a/drivers/input/touchscreen/Kconfig
> +++ b/drivers/input/touchscreen/Kconfig
> @@ -105,6 +105,8 @@ config TOUCHSCREEN_AR1021_I2C
>  config TOUCHSCREEN_ATMEL_MXT
>   tristate "Atmel mXT I2C Touchscreen"
>   depends on I2C
> + depends on VIDEO_V4L2

Hmm.. can we possibly stub this out? I do not think we want to depend on
V4L2 unconditionally.

> + select VIDEOBUF2_VMALLOC
>   select FW_LOADER
>   help
> Say Y here if you have Atmel mXT series I2C touchscreen,
> diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c 
> b/drivers/input/touchscreen/atmel_mxt_ts.c
> index 21f42ed..8608cb1 100644
> --- a/drivers/input/touchscreen/atmel_mxt_ts.c
> +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
> @@ -28,6 +28,10 @@
>  #include 
>  #include 
>  #include 
> +#include 
> +#include 
> +#include 
> +#include 
>  
>  /* Firmware files */
>  #define MXT_FW_NAME  "maxtouch.fw"
> @@ -222,6 +226,23 @@ struct mxt_dbg {
>   struct t37_debug *t37_buf;
>   unsigned int t37_pages;
>   unsigned int t37_nodes;
> +
> + struct v4l2_device v4l2;
> + struct v4l2_pix_format format;
> + struct video_device vdev;
> + struct vb2_queue queue;
> + struct mutex lock;
> + int input;
> +};
> +
> +static const struct v4l2_file_operations mxt_video_fops = {
> + .owner = THIS_MODULE,
> + .open = v4l2_fh_open,
> + .release = vb2_fop_release,
> + .unlocked_ioctl = video_ioctl2,
> + .read = vb2_fop_read,
> + .mmap = vb2_fop_mmap,
> + .poll = vb2_fop_poll,
>  };
>  
>  /* Each client has this additional data */
> @@ -277,6 +298,11 @@ struct mxt_data {
>   struct completion crc_completion;
>  };
>  
> +struct mxt_vb2_buffer {
> + struct vb2_buffer   vb;
> + struct list_headlist;
> +};
> +
>  static size_t mxt_obj_size(const struct mxt_object *obj)
>  {
>   return obj->size_minus_one + 1;
> @@ -1523,6 +1549,9 @@ static void mxt_free_input_device(struct mxt_data *data)
>  
>  static void mxt_free_object_table(struct mxt_data *data)
>  {
> + video_unregister_device(>dbg.vdev);
> + v4l2_device_unregister(>dbg.v4l2);
> +
>   kfree(data->object_table);
>   data->object_table = NULL;
>   kfree(data->msg_buf);
> @@ -2154,10 +2183,191 @@ wait_cmd:
>   return mxt_convert_debug_pages(data, outbuf);
>  }
>  
> +static int mxt_queue_setup(struct vb2_queue *q,
> +unsigned int *nbuffers, unsigned int *nplanes,
> +unsigned int sizes[], void *alloc_ctxs[])
> +{
> + struct mxt_data *data = q->drv_priv;
> + size_t size = data->dbg.t37_nodes * sizeof(u16);
> +
> + if (*nplanes)
> + return sizes[0] < size ? -EINVAL : 0;
> +
> + *nplanes = 1;
> + sizes[0] = size;
> +
> + return 0;
> +}
> +
> +static void mxt_buffer_queue(struct vb2_buffer *vb)
> +{
> + struct mxt_data *data = vb2_get_drv_priv(vb->vb2_queue);
> + u16 *ptr;
> + int ret;
> +
> + ptr = vb2_plane_vaddr(vb, 0);
> + if (!ptr) {
> + dev_err(>client->dev, "Error acquiring frame ptr\n");
> + goto fault;
> + }
> +
> + ret = mxt_read_diagnostic_debug(data, MXT_DIAGNOSTIC_DELTAS, ptr);
> + if (ret)
> + goto fault;
> +
> + vb2_set_plane_payload(vb, 0, data->dbg.t37_nodes * sizeof(u16));
> + vb2_buffer_done(vb, VB2_BUF_STATE_DONE);
> + return;
> +
> +fault:
> + vb2_buffer_done(vb, VB2_BUF_STATE_ERROR);
> +}
> +
> +/* V4L2 structures */
> +static const struct vb2_ops mxt_queue_ops = {
> + .queue_setup= mxt_queue_setup,
> + .buf_queue  = mxt_buffer_queue,
> + .wait_prepare   = vb2_ops_wait_prepare,
> + .wait_finish= vb2_ops_wait_finish,
> +};
> +
> +static const struct vb2_queue mxt_queue = {
> + .type = V4L2_BUF_TYPE_VIDEO_CAPTURE,
> + .io_modes = VB2_MMAP | VB2_USERPTR | VB2_DMABUF | VB2_READ,
> + .buf_struct_size = sizeof(struct mxt_vb2_buffer),
> + .ops = _queue_ops,
> + .mem_ops = _vmalloc_memops,
> + .timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC,
> + .min_buffers_needed = 1,
> +};
> +
> +static int mxt_vidioc_querycap(struct file *file, void *priv,
> +  struct v4l2_capability *cap)
> +{
> + struct mxt_data *data = video_drvdata(file);
> +
> + strlcpy(cap->driver, "atmel_mxt_ts", sizeof(cap->driver));
> + strlcpy(cap->card, "atmel_mxt_ts touch", sizeof(cap->card));
> + snprintf(cap->bus_info, 

Re: [PATCH v3 7/8] Input: atmel_mxt_ts - add diagnostic data support for mXT1386

[Dmitry Torokhov]

On Wed, Jun 01, 2016 at 05:39:51PM +0100, Nick Dyer wrote:
> The mXT1386 family of chips have a different architecture which splits
> the diagnostic data into 3 columns.
> 
> Signed-off-by: Nick Dyer 
> ---
>  drivers/input/touchscreen/atmel_mxt_ts.c | 29 ++---
>  1 file changed, 26 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c 
> b/drivers/input/touchscreen/atmel_mxt_ts.c
> index eced661..7d8002d 100644
> --- a/drivers/input/touchscreen/atmel_mxt_ts.c
> +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
> @@ -137,6 +137,10 @@ struct t9_range {
>  #define MXT_DIAGNOSTIC_DELTAS0x10
>  #define MXT_DIAGNOSTIC_SIZE  128
>  
> +#define MXT_FAMILY_1386  160
> +#define MXT1386_COLUMNS  3
> +#define MXT1386_PAGES_PER_COLUMN 8
> +
>  struct t37_debug {
>   u8 mode;
>   u8 page;
> @@ -2135,13 +2139,27 @@ recheck:
>  static u16 mxt_get_debug_value(struct mxt_data *data, unsigned int x,
>  unsigned int y)
>  {
> + struct mxt_info *info = >info;
>   struct mxt_dbg *dbg = >dbg;
>   unsigned int ofs, page;
> + unsigned int col = 0;
> + unsigned int col_width;
> +
> + if (info->family_id == MXT_FAMILY_1386) {
> + col_width = info->matrix_ysize / MXT1386_COLUMNS;
> + col = y / col_width;
> + y = y % col_width;
> + } else {
> + col_width = info->matrix_ysize;
> + }
>  
> - ofs = (y + (x * data->info.matrix_ysize)) * sizeof(u16);
> + ofs = (y + (x * col_width)) * sizeof(u16);
>   page = ofs / MXT_DIAGNOSTIC_SIZE;
>   ofs %= MXT_DIAGNOSTIC_SIZE;
>  
> + if (info->family_id == MXT_FAMILY_1386)
> + page += col * MXT1386_PAGES_PER_COLUMN;
> +
>   return get_unaligned_le16(>t37_buf[page].data[ofs]);
>  }
>  
> @@ -2411,6 +2429,7 @@ static const struct video_device mxt_video_device = {
>  
>  static void mxt_debug_init(struct mxt_data *data)
>  {
> + struct mxt_info *info = >info;
>   struct mxt_dbg *dbg = >dbg;
>   struct mxt_object *object;
>   int error;
> @@ -2434,9 +2453,13 @@ static void mxt_debug_init(struct mxt_data *data)
>  
>   /* Calculate size of data and allocate buffer */
>   dbg->t37_nodes = data->xsize * data->ysize;
> - dbg->t37_pages = ((data->xsize * data->info.matrix_ysize)
> -   * sizeof(u16) / sizeof(dbg->t37_buf->data)) + 1;
>  
> + if (info->family_id == MXT_FAMILY_1386)
> + dbg->t37_pages = MXT1386_COLUMNS * MXT1386_PAGES_PER_COLUMN;
> + else
> + dbg->t37_pages = ((data->xsize * info->matrix_ysize)
> +* sizeof(u16) / sizeof(dbg->t37_buf->data))
> ++ 1;

Won't this result in an extra page in some cases. DIV_ROUND_UP instead?

>  
>   dbg->t37_buf = devm_kzalloc(>client->dev,
>sizeof(struct t37_debug) * dbg->t37_pages,
> -- 
> 2.5.0
> 

-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 1/8] Input: atmel_mxt_ts - add support for T37 diagnostic data

[Dmitry Torokhov]

Hi Nick,

On Wed, Jun 01, 2016 at 05:39:45PM +0100, Nick Dyer wrote:
> Atmel maXTouch devices have a T37 object which can be used to read raw
> touch deltas from the device. This consists of an array of 16-bit
> integers, one for each node on the touchscreen matrix.
> 
> Signed-off-by: Nick Dyer 
> ---
>  drivers/input/touchscreen/atmel_mxt_ts.c | 152 
> +++
>  1 file changed, 152 insertions(+)
> 
> diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c 
> b/drivers/input/touchscreen/atmel_mxt_ts.c
> index 5af7907..21f42ed 100644
> --- a/drivers/input/touchscreen/atmel_mxt_ts.c
> +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
> @@ -124,6 +124,17 @@ struct t9_range {
>  #define MXT_COMMS_CTRL   0
>  #define MXT_COMMS_CMD1
>  
> +/* MXT_DEBUG_DIAGNOSTIC_T37 */
> +#define MXT_DIAGNOSTIC_PAGEUP0x01
> +#define MXT_DIAGNOSTIC_DELTAS0x10
> +#define MXT_DIAGNOSTIC_SIZE  128
> +
> +struct t37_debug {
> + u8 mode;
> + u8 page;
> + u8 data[MXT_DIAGNOSTIC_SIZE];
> +};
> +
>  /* Define for MXT_GEN_COMMAND_T6 */
>  #define MXT_BOOT_VALUE   0xa5
>  #define MXT_RESET_VALUE  0x01
> @@ -205,6 +216,14 @@ struct mxt_object {
>   u8 num_report_ids;
>  } __packed;
>  
> +struct mxt_dbg {
> + u16 t37_address;
> + u16 diag_cmd_address;
> + struct t37_debug *t37_buf;
> + unsigned int t37_pages;
> + unsigned int t37_nodes;
> +};
> +
>  /* Each client has this additional data */
>  struct mxt_data {
>   struct i2c_client *client;
> @@ -233,6 +252,7 @@ struct mxt_data {
>   u8 num_touchids;
>   u8 multitouch;
>   struct t7_config t7_cfg;
> + struct mxt_dbg dbg;
>  
>   /* Cached parameters from object table */
>   u16 T5_address;
> @@ -2043,6 +2063,136 @@ recheck:
>   return 0;
>  }
>  
> +static u16 mxt_get_debug_value(struct mxt_data *data, unsigned int x,
> +unsigned int y)
> +{
> + struct mxt_dbg *dbg = >dbg;
> + unsigned int ofs, page;
> +
> + ofs = (y + (x * data->info.matrix_ysize)) * sizeof(u16);
> + page = ofs / MXT_DIAGNOSTIC_SIZE;
> + ofs %= MXT_DIAGNOSTIC_SIZE;
> +
> + return get_unaligned_le16(>t37_buf[page].data[ofs]);
> +}
> +
> +static int mxt_convert_debug_pages(struct mxt_data *data, u16 *outbuf)
> +{
> + struct mxt_dbg *dbg = >dbg;
> + unsigned int x = 0;
> + unsigned int y = 0;
> + unsigned int i;
> +
> + for (i = 0; i < dbg->t37_nodes; i++) {
> + outbuf[i] = mxt_get_debug_value(data, x, y);
> +
> + /* Next value */
> + if (++x >= data->info.matrix_xsize) {
> + x = 0;
> + y++;
> + }
> + }
> +
> + return 0;
> +}
> +
> +static int mxt_read_diagnostic_debug(struct mxt_data *data, u8 mode,
> +  u16 *outbuf)
> +{
> + struct mxt_dbg *dbg = >dbg;
> + int retries = 0;
> + int page;
> + int ret;
> + u8 cmd = mode;
> + struct t37_debug *p;
> + u8 cmd_poll;
> +
> + for (page = 0; page < dbg->t37_pages; page++) {
> + p = dbg->t37_buf + page;
> +
> + ret = mxt_write_reg(data->client, dbg->diag_cmd_address,
> + cmd);
> + if (ret)
> + return ret;
> +
> + retries = 0;
> + msleep(20);
> +wait_cmd:
> + /* Read back command byte */
> + ret = __mxt_read_reg(data->client, dbg->diag_cmd_address,
> +  sizeof(cmd_poll), _poll);
> + if (ret)
> + return ret;
> +
> + /* Field is cleared once the command has been processed */
> + if (cmd_poll) {
> + if (retries++ > 100)
> + return -EINVAL;
> +
> + msleep(20);
> + goto wait_cmd;
> + }
> +
> + /* Read T37 page */
> + ret = __mxt_read_reg(data->client, dbg->t37_address,
> + sizeof(struct t37_debug), p);
> + if (ret)
> + return ret;
> +
> + if ((p->mode != mode) || (p->page != page)) {

No need for extra parenthesis.

> + dev_err(>client->dev, "T37 page mismatch\n");
> + return -EINVAL;
> + }
> +
> + dev_dbg(>client->dev, "%s page:%d retries:%d\n",
> + __func__, page, retries);
> +
> + /* For remaining pages, write PAGEUP rather than mode */
> + cmd = MXT_DIAGNOSTIC_PAGEUP;
> + }
> +
> + return mxt_convert_debug_pages(data, outbuf);
> +}
> +
> +static void mxt_debug_init(struct mxt_data *data)
> +{
> + struct mxt_dbg *dbg = >dbg;
> + struct mxt_object *object;
> +
> + object = mxt_get_object(data, MXT_GEN_COMMAND_T6);
> + if (!object)
> +  

Re: [PATCHv14 05/18] HID: add HDMI CEC specific keycodes

[Dmitry Torokhov]

On Fri, Mar 25, 2016 at 02:10:03PM +0100, Hans Verkuil wrote:
> From: Kamil Debski <ka...@wypas.org>
> 
> Add HDMI CEC specific keycodes to the keycodes definition.
> 
> Signed-off-by: Kamil Debski <ka...@wypas.org>
> Signed-off-by: Hans Verkuil <hans.verk...@cisco.com>

Acked-by: Dmitry Torokhov <dmitry.torok...@gmail.com>

> ---
>  include/uapi/linux/input-event-codes.h | 30 ++
>  1 file changed, 30 insertions(+)
> 
> diff --git a/include/uapi/linux/input-event-codes.h 
> b/include/uapi/linux/input-event-codes.h
> index 87cf351..02b7b3a 100644
> --- a/include/uapi/linux/input-event-codes.h
> +++ b/include/uapi/linux/input-event-codes.h
> @@ -611,6 +611,36 @@
>  #define KEY_KBDINPUTASSIST_ACCEPT0x264
>  #define KEY_KBDINPUTASSIST_CANCEL0x265
>  
> +/* Diagonal movement keys */
> +#define KEY_RIGHT_UP 0x266
> +#define KEY_RIGHT_DOWN   0x267
> +#define KEY_LEFT_UP  0x268
> +#define KEY_LEFT_DOWN0x269
> +
> +#define KEY_ROOT_MENU0x26a /* Show Device's Root 
> Menu */
> +#define KEY_MEDIA_TOP_MENU   0x26b /* Show Top Menu of the Media 
> (e.g. DVD) */
> +#define KEY_NUMERIC_11   0x26c
> +#define KEY_NUMERIC_12   0x26d
> +/*
> + * Toggle Audio Description: refers to an audio service that helps blind and
> + * visually impaired consumers understand the action in a program. Note: in
> + * some countries this is referred to as "Video Description".
> + */
> +#define KEY_AUDIO_DESC   0x26e
> +#define KEY_3D_MODE  0x26f
> +#define KEY_NEXT_FAVORITE0x270
> +#define KEY_STOP_RECORD  0x271
> +#define KEY_PAUSE_RECORD 0x272
> +#define KEY_VOD  0x273 /* Video on Demand */
> +#define KEY_UNMUTE   0x274
> +#define KEY_FASTREVERSE  0x275
> +#define KEY_SLOWREVERSE  0x276
> +/*
> + * Control a data application associated with the currently viewed channel,
> + * e.g. teletext or data broadcast application (MHEG, MHP, HbbTV, etc.)
> + */
> +#define KEY_DATA 0x275
> +
>  #define BTN_TRIGGER_HAPPY0x2c0
>  #define BTN_TRIGGER_HAPPY1   0x2c0
>  #define BTN_TRIGGER_HAPPY2   0x2c1
> -- 
> 2.7.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv12 05/17] HID: add HDMI CEC specific keycodes

[Dmitry Torokhov]

On Sat, Mar 05, 2016 at 09:44:24AM +0100, Hans Verkuil wrote:
> Hi Dmitry,
> 
> On 03/04/2016 08:58 PM, Dmitry Torokhov wrote:
> > On Wed, Feb 10, 2016 at 01:51:39PM +0100, Hans Verkuil wrote:
> >> +#define KEY_AUDIO_DESC0x26e
> >> +#define KEY_3D_MODE   0x26f
> >> +#define KEY_NEXT_FAVORITE 0x270
> >> +#define KEY_STOP_RECORD   0x271
> >> +#define KEY_PAUSE_RECORD  0x272
> >> +#define KEY_VOD   0x273 /* Video on Demand */
> >> +#define KEY_UNMUTE0x274
> >> +#define KEY_FASTREVERSE   0x275
> >> +#define KEY_SLOWREVERSE   0x276
> > 
> > KEY_FRAMEBACK maybe?
> 
> FRAMEBACK suggests to me that it goes back a single frame and then pauses
> at that frame. Whereas SLOWREVERSE is continual slow reverse playback.
> 
> So I would prefer to keep it, unless you think otherwise.

OK, fair enough.

Thanks.

-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv12 05/17] HID: add HDMI CEC specific keycodes

[Dmitry Torokhov]

On Wed, Feb 10, 2016 at 01:51:39PM +0100, Hans Verkuil wrote:
> From: Kamil Debski 
> 
> Add HDMI CEC specific keycodes to the keycodes definition.
> 
> Signed-off-by: Kamil Debski 
> Signed-off-by: Hans Verkuil 
> ---
>  include/uapi/linux/input-event-codes.h | 28 
>  1 file changed, 28 insertions(+)
> 
> diff --git a/include/uapi/linux/input-event-codes.h 
> b/include/uapi/linux/input-event-codes.h
> index 87cf351..2662500 100644
> --- a/include/uapi/linux/input-event-codes.h
> +++ b/include/uapi/linux/input-event-codes.h
> @@ -611,6 +611,34 @@
>  #define KEY_KBDINPUTASSIST_ACCEPT0x264
>  #define KEY_KBDINPUTASSIST_CANCEL0x265
>  
> +#define KEY_RIGHT_UP 0x266
> +#define KEY_RIGHT_DOWN   0x267
> +#define KEY_LEFT_UP  0x268
> +#define KEY_LEFT_DOWN0x269

Can you please add some comments to these and how they are different
form normal KEY_UP/KEY_DOWN or KEY_DPAD* or BTN_A/B/X/Y.

> +#define KEY_ROOT_MENU0x26a /* Show Device's Root 
> Menu */
> +#define KEY_MEDIA_TOP_MENU   0x26b /* Show Top Menu of the Media 
> (e.g. DVD) */
> +#define KEY_NUMERIC_11   0x26c
> +#define KEY_NUMERIC_12   0x26d
> +/*
> + * Toggle Audio Description: refers to an audio service that helps blind and
> + * visually impaired consumers understand the action in a program. Note: in
> + * some countries this is referred to as "Video Description".
> + */
> +#define KEY_AUDIO_DESC   0x26e
> +#define KEY_3D_MODE  0x26f
> +#define KEY_NEXT_FAVORITE0x270
> +#define KEY_STOP_RECORD  0x271
> +#define KEY_PAUSE_RECORD 0x272
> +#define KEY_VOD  0x273 /* Video on Demand */
> +#define KEY_UNMUTE   0x274
> +#define KEY_FASTREVERSE  0x275
> +#define KEY_SLOWREVERSE  0x276

KEY_FRAMEBACK maybe?

> +/*
> + * Control a data application associated with the currently viewed channel,
> + * e.g. teletext or data broadcast application (MHEG, MHP, HbbTV, etc.)
> + */
> +#define KEY_DATA 0x275
> +
>  #define BTN_TRIGGER_HAPPY0x2c0
>  #define BTN_TRIGGER_HAPPY1   0x2c0
>  #define BTN_TRIGGER_HAPPY2   0x2c1
> -- 
> 2.7.0
> 

Thanks.

-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] android: fix warning when releasing active sync point

[Dmitry Torokhov]

On Tue, Dec 15, 2015 at 1:26 AM, Daniel Vetter <dan...@ffwll.ch> wrote:
> On Mon, Dec 14, 2015 at 05:29:55PM -0800, Dmitry Torokhov wrote:
>> Userspace can close the sync device while there are still active fence
>> points, in which case kernel produces the following warning:
>>
>> [   43.853176] [ cut here ]
>> [   43.857834] WARNING: CPU: 0 PID: 892 at 
>> /mnt/host/source/src/third_party/kernel/v3.18/drivers/staging/android/sync.c:439
>>  android_fence_release+0x88/0x104()
>> [   43.871741] CPU: 0 PID: 892 Comm: Binder_5 Tainted: G U 
>> 3.18.0-07661-g0550ce9 #1
>> [   43.880176] Hardware name: Google Tegra210 Smaug Rev 1+ (DT)
>> [   43.885834] Call trace:
>> [   43.888294] [] dump_backtrace+0x0/0x10c
>> [   43.893697] [] show_stack+0x10/0x1c
>> [   43.898756] [] dump_stack+0x74/0xb8
>> [   43.903814] [] warn_slowpath_common+0x84/0xb0
>> [   43.909736] [] warn_slowpath_null+0x14/0x20
>> [   43.915482] [] android_fence_release+0x84/0x104
>> [   43.921582] [] fence_release+0x104/0x134
>> [   43.927066] [] sync_fence_free+0x74/0x9c
>> [   43.932552] [] sync_fence_release+0x34/0x48
>> [   43.938304] [] __fput+0x100/0x1b8
>> [   43.943185] [] fput+0x8/0x14
>> [   43.947982] [] task_work_run+0xb0/0xe4
>> [   43.953297] [] do_notify_resume+0x44/0x5c
>> [   43.958867] ---[ end trace 5a2aa4027cc5d171 ]---
>>
>> Let's fix it by introducing a new optional callback (disable_signaling)
>> to fence operations so that drivers can do proper clean ups when we
>> remove last callback for given fence.
>>
>> Reviewed-by: Andrew Bresticker <abres...@chromium.org>
>> Signed-off-by: Dmitry Torokhov <d...@chromium.org>
>> ---
>>  drivers/dma-buf/fence.c| 6 +-
>>  drivers/staging/android/sync.c | 8 
>>  include/linux/fence.h  | 2 ++
>>  3 files changed, 15 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/dma-buf/fence.c b/drivers/dma-buf/fence.c
>> index 7b05dbe..0ed73ad 100644
>> --- a/drivers/dma-buf/fence.c
>> +++ b/drivers/dma-buf/fence.c
>> @@ -304,8 +304,12 @@ fence_remove_callback(struct fence *fence, struct 
>> fence_cb *cb)
>>   spin_lock_irqsave(fence->lock, flags);
>>
>>   ret = !list_empty(>node);
>> - if (ret)
>> + if (ret) {
>>   list_del_init(>node);
>> + if (list_empty(>cb_list))
>> + if (fence->ops->disable_signaling)
>> + fence->ops->disable_signaling(fence);
>
> What exactly is the bug here? A fence with no callbacks registered any
> more shouldn't have any problem. Why exactly does this blow up?
>
> I guess I don't really understand the bug ... we do seem to remove the
> callback already.
>

The issue is that when enabling signalling in sync driver we put fence
on an internal list in the driver and there is no way of taking it off
this list, except when it is signalled. The driver, when destroying
the fence, checks if the fence is not on this list (as a sanity
measure) and that produces the backtrace in the commit log.

IOW for some drivers we need an "undo" for enable_signaling() callback
so that drivers can maintain consistent internal state.

Thanks.

-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] android: fix warning when releasing active sync point

[Dmitry Torokhov]

On Tue, Dec 15, 2015 at 2:01 AM, Maarten Lankhorst
<maarten.lankho...@linux.intel.com> wrote:
> Op 15-12-15 om 02:29 schreef Dmitry Torokhov:
>> Userspace can close the sync device while there are still active fence
>> points, in which case kernel produces the following warning:
>>
>> [   43.853176] [ cut here ]
>> [   43.857834] WARNING: CPU: 0 PID: 892 at 
>> /mnt/host/source/src/third_party/kernel/v3.18/drivers/staging/android/sync.c:439
>>  android_fence_release+0x88/0x104()
>> [   43.871741] CPU: 0 PID: 892 Comm: Binder_5 Tainted: G U 
>> 3.18.0-07661-g0550ce9 #1
>> [   43.880176] Hardware name: Google Tegra210 Smaug Rev 1+ (DT)
>> [   43.885834] Call trace:
>> [   43.888294] [] dump_backtrace+0x0/0x10c
>> [   43.893697] [] show_stack+0x10/0x1c
>> [   43.898756] [] dump_stack+0x74/0xb8
>> [   43.903814] [] warn_slowpath_common+0x84/0xb0
>> [   43.909736] [] warn_slowpath_null+0x14/0x20
>> [   43.915482] [] android_fence_release+0x84/0x104
>> [   43.921582] [] fence_release+0x104/0x134
>> [   43.927066] [] sync_fence_free+0x74/0x9c
>> [   43.932552] [] sync_fence_release+0x34/0x48
>> [   43.938304] [] __fput+0x100/0x1b8
>> [   43.943185] [] fput+0x8/0x14
>> [   43.947982] [] task_work_run+0xb0/0xe4
>> [   43.953297] [] do_notify_resume+0x44/0x5c
>> [   43.958867] ---[ end trace 5a2aa4027cc5d171 ]---
>>
>> Let's fix it by introducing a new optional callback (disable_signaling)
>> to fence operations so that drivers can do proper clean ups when we
>> remove last callback for given fence.
>>
>> Reviewed-by: Andrew Bresticker <abres...@chromium.org>
>> Signed-off-by: Dmitry Torokhov <d...@chromium.org>
>>
> NACK! There's no way to do this race free.

Can you please explain the race because as far as I can see there is not one.

> The driver should hold a refcount until fence is signaled.

If we are no longer interested in fence why do we need to wait for the
fence to be signaled?

Thanks.

-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] android: fix warning when releasing active sync point

[Dmitry Torokhov]

On Tue, Dec 15, 2015 at 5:50 AM, Frank Binns  wrote:
> Is this not the issue fixed by 8e43c9c75?

No because if we start teardown without waiting for the fence to be
signaled it will still be on the active_list.

Thanks.

-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] android: fix warning when releasing active sync point

[Dmitry Torokhov]

On Tue, Dec 15, 2015 at 5:30 AM, Gustavo Padovan <gust...@padovan.org> wrote:
> 2015-12-14 Dmitry Torokhov <d...@chromium.org>:
>
>> Userspace can close the sync device while there are still active fence
>> points, in which case kernel produces the following warning:
>>
>> [   43.853176] [ cut here ]
>> [   43.857834] WARNING: CPU: 0 PID: 892 at 
>> /mnt/host/source/src/third_party/kernel/v3.18/drivers/staging/android/sync.c:439
>>  android_fence_release+0x88/0x104()
>> [   43.871741] CPU: 0 PID: 892 Comm: Binder_5 Tainted: G U 
>> 3.18.0-07661-g0550ce9 #1
>> [   43.880176] Hardware name: Google Tegra210 Smaug Rev 1+ (DT)
>> [   43.885834] Call trace:
>> [   43.888294] [] dump_backtrace+0x0/0x10c
>> [   43.893697] [] show_stack+0x10/0x1c
>> [   43.898756] [] dump_stack+0x74/0xb8
>> [   43.903814] [] warn_slowpath_common+0x84/0xb0
>> [   43.909736] [] warn_slowpath_null+0x14/0x20
>> [   43.915482] [] android_fence_release+0x84/0x104
>> [   43.921582] [] fence_release+0x104/0x134
>> [   43.927066] [] sync_fence_free+0x74/0x9c
>> [   43.932552] [] sync_fence_release+0x34/0x48
>> [   43.938304] [] __fput+0x100/0x1b8
>> [   43.943185] [] fput+0x8/0x14
>> [   43.947982] [] task_work_run+0xb0/0xe4
>> [   43.953297] [] do_notify_resume+0x44/0x5c
>> [   43.958867] ---[ end trace 5a2aa4027cc5d171 ]---
>
> This crash report seems to be for a 3.18 kernel. Can you reproduce it
> on upstream kernel as well?

Unfortunately this board does not run upsrteam just yet, but looking
at the sync driver and fence code we are pretty much in sync with
upstream.

Thanks.

-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] android: fix warning when releasing active sync point

[Dmitry Torokhov]

On Tue, Dec 15, 2015 at 11:00 AM, Gustavo Padovan <gust...@padovan.org> wrote:
> 2015-12-15 Daniel Vetter <dan...@ffwll.ch>:
>
>> On Mon, Dec 14, 2015 at 05:29:55PM -0800, Dmitry Torokhov wrote:
>> > Userspace can close the sync device while there are still active fence
>> > points, in which case kernel produces the following warning:
>> >
>> > [   43.853176] [ cut here ]
>> > [   43.857834] WARNING: CPU: 0 PID: 892 at 
>> > /mnt/host/source/src/third_party/kernel/v3.18/drivers/staging/android/sync.c:439
>> >  android_fence_release+0x88/0x104()
>> > [   43.871741] CPU: 0 PID: 892 Comm: Binder_5 Tainted: G U 
>> > 3.18.0-07661-g0550ce9 #1
>> > [   43.880176] Hardware name: Google Tegra210 Smaug Rev 1+ (DT)
>> > [   43.885834] Call trace:
>> > [   43.888294] [] dump_backtrace+0x0/0x10c
>> > [   43.893697] [] show_stack+0x10/0x1c
>> > [   43.898756] [] dump_stack+0x74/0xb8
>> > [   43.903814] [] warn_slowpath_common+0x84/0xb0
>> > [   43.909736] [] warn_slowpath_null+0x14/0x20
>> > [   43.915482] [] android_fence_release+0x84/0x104
>> > [   43.921582] [] fence_release+0x104/0x134
>> > [   43.927066] [] sync_fence_free+0x74/0x9c
>> > [   43.932552] [] sync_fence_release+0x34/0x48
>> > [   43.938304] [] __fput+0x100/0x1b8
>> > [   43.943185] [] fput+0x8/0x14
>> > [   43.947982] [] task_work_run+0xb0/0xe4
>> > [   43.953297] [] do_notify_resume+0x44/0x5c
>> > [   43.958867] ---[ end trace 5a2aa4027cc5d171 ]---
>> >
>> > Let's fix it by introducing a new optional callback (disable_signaling)
>> > to fence operations so that drivers can do proper clean ups when we
>> > remove last callback for given fence.
>> >
>> > Reviewed-by: Andrew Bresticker <abres...@chromium.org>
>> > Signed-off-by: Dmitry Torokhov <d...@chromium.org>
>> > ---
>> >  drivers/dma-buf/fence.c| 6 +-
>> >  drivers/staging/android/sync.c | 8 
>> >  include/linux/fence.h  | 2 ++
>> >  3 files changed, 15 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/drivers/dma-buf/fence.c b/drivers/dma-buf/fence.c
>> > index 7b05dbe..0ed73ad 100644
>> > --- a/drivers/dma-buf/fence.c
>> > +++ b/drivers/dma-buf/fence.c
>> > @@ -304,8 +304,12 @@ fence_remove_callback(struct fence *fence, struct 
>> > fence_cb *cb)
>> > spin_lock_irqsave(fence->lock, flags);
>> >
>> > ret = !list_empty(>node);
>> > -   if (ret)
>> > +   if (ret) {
>> > list_del_init(>node);
>> > +   if (list_empty(>cb_list))
>> > +   if (fence->ops->disable_signaling)
>> > +   fence->ops->disable_signaling(fence);
>>
>> What exactly is the bug here? A fence with no callbacks registered any
>> more shouldn't have any problem. Why exactly does this blow up?
>
> The WARN_ON is probably this one:
> https://android.googlesource.com/kernel/common/+/android-3.18/drivers/staging/android/sync.c#433
>
> I've been wondering in the last few days if this warning is really
> necessary. If the user is closing a sync_timeline that has unsignalled
> fences it should probably be aware of that already. Then I think it is
> okay to remove the the sync_pt from the active_list at the release-time.
> In fact I've already prepared a patch doing that. Thoughts?
>

Maybe, but you need to make sure that you only affecting your fences.

My main objection is that still leaves fence_remove_callback() being
not mirror image of fence_add_callback().

-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] android: fix warning when releasing active sync point

[Dmitry Torokhov]

Userspace can close the sync device while there are still active fence
points, in which case kernel produces the following warning:

[   43.853176] [ cut here ]
[   43.857834] WARNING: CPU: 0 PID: 892 at 
/mnt/host/source/src/third_party/kernel/v3.18/drivers/staging/android/sync.c:439
 android_fence_release+0x88/0x104()
[   43.871741] CPU: 0 PID: 892 Comm: Binder_5 Tainted: G U 
3.18.0-07661-g0550ce9 #1
[   43.880176] Hardware name: Google Tegra210 Smaug Rev 1+ (DT)
[   43.885834] Call trace:
[   43.888294] [] dump_backtrace+0x0/0x10c
[   43.893697] [] show_stack+0x10/0x1c
[   43.898756] [] dump_stack+0x74/0xb8
[   43.903814] [] warn_slowpath_common+0x84/0xb0
[   43.909736] [] warn_slowpath_null+0x14/0x20
[   43.915482] [] android_fence_release+0x84/0x104
[   43.921582] [] fence_release+0x104/0x134
[   43.927066] [] sync_fence_free+0x74/0x9c
[   43.932552] [] sync_fence_release+0x34/0x48
[   43.938304] [] __fput+0x100/0x1b8
[   43.943185] [] fput+0x8/0x14
[   43.947982] [] task_work_run+0xb0/0xe4
[   43.953297] [] do_notify_resume+0x44/0x5c
[   43.958867] ---[ end trace 5a2aa4027cc5d171 ]---

Let's fix it by introducing a new optional callback (disable_signaling)
to fence operations so that drivers can do proper clean ups when we
remove last callback for given fence.

Reviewed-by: Andrew Bresticker <abres...@chromium.org>
Signed-off-by: Dmitry Torokhov <d...@chromium.org>
---
 drivers/dma-buf/fence.c| 6 +-
 drivers/staging/android/sync.c | 8 
 include/linux/fence.h  | 2 ++
 3 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/dma-buf/fence.c b/drivers/dma-buf/fence.c
index 7b05dbe..0ed73ad 100644
--- a/drivers/dma-buf/fence.c
+++ b/drivers/dma-buf/fence.c
@@ -304,8 +304,12 @@ fence_remove_callback(struct fence *fence, struct fence_cb 
*cb)
spin_lock_irqsave(fence->lock, flags);
 
ret = !list_empty(>node);
-   if (ret)
+   if (ret) {
list_del_init(>node);
+   if (list_empty(>cb_list))
+   if (fence->ops->disable_signaling)
+   fence->ops->disable_signaling(fence);
+   }
 
spin_unlock_irqrestore(fence->lock, flags);
 
diff --git a/drivers/staging/android/sync.c b/drivers/staging/android/sync.c
index e0c1acb..f8566c1 100644
--- a/drivers/staging/android/sync.c
+++ b/drivers/staging/android/sync.c
@@ -465,6 +465,13 @@ static bool android_fence_enable_signaling(struct fence 
*fence)
return true;
 }
 
+static void android_fence_disable_signaling(struct fence *fence)
+{
+   struct sync_pt *pt = container_of(fence, struct sync_pt, base);
+
+   list_del_init(>active_list);
+}
+
 static int android_fence_fill_driver_data(struct fence *fence,
  void *data, int size)
 {
@@ -508,6 +515,7 @@ static const struct fence_ops android_fence_ops = {
.get_driver_name = android_fence_get_driver_name,
.get_timeline_name = android_fence_get_timeline_name,
.enable_signaling = android_fence_enable_signaling,
+   .disable_signaling = android_fence_disable_signaling,
.signaled = android_fence_signaled,
.wait = fence_default_wait,
.release = android_fence_release,
diff --git a/include/linux/fence.h b/include/linux/fence.h
index bb52201..ce44348 100644
--- a/include/linux/fence.h
+++ b/include/linux/fence.h
@@ -107,6 +107,7 @@ struct fence_cb {
  * @get_driver_name: returns the driver name.
  * @get_timeline_name: return the name of the context this fence belongs to.
  * @enable_signaling: enable software signaling of fence.
+ * @disable_signaling: disable software signaling of fence (optional).
  * @signaled: [optional] peek whether the fence is signaled, can be null.
  * @wait: custom wait implementation, or fence_default_wait.
  * @release: [optional] called on destruction of fence, can be null
@@ -166,6 +167,7 @@ struct fence_ops {
const char * (*get_driver_name)(struct fence *fence);
const char * (*get_timeline_name)(struct fence *fence);
bool (*enable_signaling)(struct fence *fence);
+   void (*disable_signaling)(struct fence *fence);
bool (*signaled)(struct fence *fence);
signed long (*wait)(struct fence *fence, bool intr, signed long 
timeout);
void (*release)(struct fence *fence);
-- 
2.6.0.rc2.230.g3dd15c0


-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: ERROR: "vb2_ops_wait_finish" [drivers/input/touchscreen/sur40.ko] undefined!

[Dmitry Torokhov]

On Sat, Oct 17, 2015 at 06:19:31PM -0700, Randy Dunlap wrote:
> On 10/17/15 17:38, kbuild test robot wrote:
> > Hi Florian,
> > 
> > FYI, the error/warning still remains.
> > 
> > tree:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 
> > master
> > head:   81429a6dbcbf3a01830de42dbdf0d9acbe68e1c1
> > commit: e831cd251fb91d6c25352d322743db0d17ea11dd [media] add raw video 
> > stream support for Samsung SUR40
> > date:   7 months ago
> > config: x86_64-randconfig-s5-10180707 (attached as .config)
> > reproduce:
> > git checkout e831cd251fb91d6c25352d322743db0d17ea11dd
> > # save the attached .config to linux build tree
> > make ARCH=x86_64 
> > 
> > All errors (new ones prefixed by >>):
> > 
> >>> ERROR: "vb2_ops_wait_finish" [drivers/input/touchscreen/sur40.ko] 
> >>> undefined!
> >>> ERROR: "vb2_ops_wait_prepare" [drivers/input/touchscreen/sur40.ko] 
> >>> undefined!
> >>> ERROR: "vb2_fop_release" [drivers/input/touchscreen/sur40.ko] undefined!
> >>> ERROR: "v4l2_fh_open" [drivers/input/touchscreen/sur40.ko] undefined!
> >>> ERROR: "vb2_fop_mmap" [drivers/input/touchscreen/sur40.ko] undefined!
> >>> ERROR: "video_ioctl2" [drivers/input/touchscreen/sur40.ko] undefined!
> >>> ERROR: "vb2_fop_poll" [drivers/input/touchscreen/sur40.ko] undefined!
> >>> ERROR: "vb2_fop_read" [drivers/input/touchscreen/sur40.ko] undefined!
> >>> ERROR: "vb2_ioctl_streamoff" [drivers/input/touchscreen/sur40.ko] 
> >>> undefined!
> >>> ERROR: "vb2_ioctl_streamon" [drivers/input/touchscreen/sur40.ko] 
> >>> undefined!
> >>> ERROR: "vb2_ioctl_create_bufs" [drivers/input/touchscreen/sur40.ko] 
> >>> undefined!
> >>> ERROR: "vb2_ioctl_dqbuf" [drivers/input/touchscreen/sur40.ko] undefined!
> >>> ERROR: "vb2_ioctl_expbuf" [drivers/input/touchscreen/sur40.ko] undefined!
> >>> ERROR: "vb2_ioctl_qbuf" [drivers/input/touchscreen/sur40.ko] undefined!
> >>> ERROR: "vb2_ioctl_querybuf" [drivers/input/touchscreen/sur40.ko] 
> >>> undefined!
> >>> ERROR: "vb2_ioctl_reqbufs" [drivers/input/touchscreen/sur40.ko] undefined!
> >>> ERROR: "__video_register_device" [drivers/input/touchscreen/sur40.ko] 
> >>> undefined!
> >>> ERROR: "video_device_release_empty" [drivers/input/touchscreen/sur40.ko] 
> >>> undefined!
> >>> ERROR: "vb2_dma_sg_init_ctx" [drivers/input/touchscreen/sur40.ko] 
> >>> undefined!
> >>> ERROR: "vb2_queue_init" [drivers/input/touchscreen/sur40.ko] undefined!
> > 
> > ---
> > 0-DAY kernel test infrastructureOpen Source Technology 
> > Center
> > https://lists.01.org/pipermail/kbuild-all   Intel 
> > Corporation
> > 
> 
> patch is here:  https://lkml.org/lkml/2015/7/10/507

I picked it up.

Thanks.

-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv8 07/15] cec: add HDMI CEC framework

[Dmitry Torokhov]

On Tue, Aug 18, 2015 at 11:00:20AM +0100, Russell King - ARM Linux wrote:
 On Tue, Aug 18, 2015 at 10:26:32AM +0200, Hans Verkuil wrote:
  +   /* Part 2: Initialize and register the character device */
  +   cdev_init(cecdev-cdev, cec_devnode_fops);
  +   cecdev-cdev.owner = owner;
  +
  +   ret = cdev_add(cecdev-cdev, MKDEV(MAJOR(cec_dev_t), cecdev-minor),
  +   1);
  +   if (ret  0) {
  +   pr_err(%s: cdev_add failed\n, __func__);
  +   goto error;
  +   }
  +
  +   /* Part 3: Register the cec device */
  +   cecdev-dev.bus = cec_bus_type;
  +   cecdev-dev.devt = MKDEV(MAJOR(cec_dev_t), cecdev-minor);
  +   cecdev-dev.release = cec_devnode_release;
  +   if (cecdev-parent)
  +   cecdev-dev.parent = cecdev-parent;
  +   dev_set_name(cecdev-dev, cec%d, cecdev-minor);
  +   ret = device_register(cecdev-dev);
 
 It's worth pointing out that you can greatly simplify the lifetime
 handling (you don't need to get and put cecdev-dev) if you make
 the cdev a child of the cecdev-dev.
 
 If you grep for kobj.parent in drivers/ you'll see many drivers are
 doing this.
 
   cecdev-cdev.kobj.parent = cecdev-dev.kobj;
 
 but you will need to call device_initialize() on cecdev-dev first,
 and use device_add() here.

This is basically a requirement if one embeds both device and a cdev
into the same structure. Trying to do get/put in the driver is racy,
you need to let framework know (by setting cdve's parent to the device
structure).

Thanks.

-- 
Dmitry
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 00/27] Export I2C and OF module aliases in missing drivers

[Dmitry Torokhov]

On Thu, Jul 30, 2015 at 09:35:17AM -0700, Dmitry Torokhov wrote:
 Hi Javier,
 
 On Thu, Jul 30, 2015 at 06:18:25PM +0200, Javier Martinez Canillas wrote:
  Hello,
  
  Short version:
  
  This series add the missing MODULE_DEVICE_TABLE() for OF and I2C tables
  to export that information so modules have the correct aliases built-in
  and autoloading works correctly.
  
  Longer version:
  
  Currently it's mandatory for I2C drivers to have an I2C device ID table
  regardless if the device was registered using platform data or OF. This
  is because the I2C core needs an I2C device ID table for two reasons:
  
  1) Match the I2C client with a I2C device ID so a struct i2c_device_id
 is passed to the I2C driver probe() function.
  
  2) Export the module aliases from the I2C device ID table so userspace
 can auto-load the correct module. This is because i2c_device_uevent
 always reports a MODALIAS of the form i2c:client-name.
 
 Why are we not fixing this? We emit specially carved uevent for
 ACPI-based devices, why not the same for OF? Platform bus does this...

Ah, now I see the 27/27 patch. I think it is exactly what we need. And
probably for SPI bus as well.

Thanks.

-- 
Dmitry
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 00/27] Export I2C and OF module aliases in missing drivers

[Dmitry Torokhov]

Hi Javier,

On Thu, Jul 30, 2015 at 06:18:25PM +0200, Javier Martinez Canillas wrote:
 Hello,
 
 Short version:
 
 This series add the missing MODULE_DEVICE_TABLE() for OF and I2C tables
 to export that information so modules have the correct aliases built-in
 and autoloading works correctly.
 
 Longer version:
 
 Currently it's mandatory for I2C drivers to have an I2C device ID table
 regardless if the device was registered using platform data or OF. This
 is because the I2C core needs an I2C device ID table for two reasons:
 
 1) Match the I2C client with a I2C device ID so a struct i2c_device_id
is passed to the I2C driver probe() function.
 
 2) Export the module aliases from the I2C device ID table so userspace
can auto-load the correct module. This is because i2c_device_uevent
always reports a MODALIAS of the form i2c:client-name.

Why are we not fixing this? We emit specially carved uevent for
ACPI-based devices, why not the same for OF? Platform bus does this...

Thanks.

-- 
Dmitry
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv7 04/15] HID: add HDMI CEC specific keycodes

[Dmitry Torokhov]

On Tue, Jun 30, 2015 at 09:36:49AM +0200, Hans Verkuil wrote:
 On 06/29/15 21:25, Dmitry Torokhov wrote:
  On Mon, Jun 29, 2015 at 12:14:49PM +0200, Hans Verkuil wrote:
  From: Kamil Debski ka...@wypas.org
 
  Add HDMI CEC specific keycodes to the keycodes definition.
 
  Signed-off-by: Kamil Debski ka...@wypas.org
  Signed-off-by: Hans Verkuil hans.verk...@cisco.com
  
  Could you please describe the intended use for these keycodes for people
  who do not live and breathe CEC specs?
 
 Do you want this as comments in the patch or as the commit description?

In the patch would be preferable.

Thanks.
 
-- 
Dmitry
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv7 04/15] HID: add HDMI CEC specific keycodes

[Dmitry Torokhov]

On Mon, Jun 29, 2015 at 12:14:49PM +0200, Hans Verkuil wrote:
 From: Kamil Debski ka...@wypas.org
 
 Add HDMI CEC specific keycodes to the keycodes definition.
 
 Signed-off-by: Kamil Debski ka...@wypas.org
 Signed-off-by: Hans Verkuil hans.verk...@cisco.com

Could you please describe the intended use for these keycodes for people
who do not live and breathe CEC specs?

Thanks!

 ---
  include/uapi/linux/input.h | 12 
  1 file changed, 12 insertions(+)
 
 diff --git a/include/uapi/linux/input.h b/include/uapi/linux/input.h
 index 731417c..7430a3f 100644
 --- a/include/uapi/linux/input.h
 +++ b/include/uapi/linux/input.h
 @@ -752,6 +752,18 @@ struct input_keymap_entry {
  #define KEY_KBDINPUTASSIST_ACCEPT0x264
  #define KEY_KBDINPUTASSIST_CANCEL0x265
  
 +#define KEY_RIGHT_UP 0x266
 +#define KEY_RIGHT_DOWN   0x267
 +#define KEY_LEFT_UP  0x268
 +#define KEY_LEFT_DOWN0x269
 +
 +#define KEY_NEXT_FAVORITE0x270
 +#define KEY_STOP_RECORD  0x271
 +#define KEY_PAUSE_RECORD 0x272
 +#define KEY_VOD  0x273
 +#define KEY_UNMUTE   0x274
 +#define KEY_DVB  0x275
 +
  #define BTN_TRIGGER_HAPPY0x2c0
  #define BTN_TRIGGER_HAPPY1   0x2c0
  #define BTN_TRIGGER_HAPPY2   0x2c1
 -- 
 2.1.4
 

-- 
Dmitry
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv7 05/15] input.h: add BUS_CEC type

[Dmitry Torokhov]

On Mon, Jun 29, 2015 at 12:14:50PM +0200, Hans Verkuil wrote:
 Inputs can come in over the HDMI CEC bus, so add a new type for this.
 
 Signed-off-by: Hans Verkuil hans.verk...@cisco.com

Acked-by: Dmitry Torokhov dmitry.torok...@gmail.com

 ---
  include/uapi/linux/input.h | 1 +
  1 file changed, 1 insertion(+)
 
 diff --git a/include/uapi/linux/input.h b/include/uapi/linux/input.h
 index 7430a3f..0af80b2 100644
 --- a/include/uapi/linux/input.h
 +++ b/include/uapi/linux/input.h
 @@ -984,6 +984,7 @@ struct input_keymap_entry {
  #define BUS_GSC  0x1A
  #define BUS_ATARI0x1B
  #define BUS_SPI  0x1C
 +#define BUS_CEC  0x1D
  
  /*
   * MT_TOOL types
 -- 
 2.1.4
 

-- 
Dmitry
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: input_polldev interval (was Re: [sur40] Debugging a race condition)?

[Dmitry Torokhov]

On March 24, 2015 11:52:54 PM PDT, Florian Echtler f...@butterbrot.org wrote:
Sorry for the continued noise, but this bug/crash is proving quite
difficult to nail down.

Currently, I'm setting the interval for input_polldev to 10 ms.
However, with video data being retrieved at the same time, it's quite
possible that one iteration of poll() will take longer than that. Could
this ultimately be the reason? What happens if a new poll() call is
scheduled before the previous one completes?

This can't happen as we schedule the next poll only after current one completes.

Hi Florian,
Thanks.

-- 
Dmitry
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] fixp-arith: replace sin/cos table by a better precision one

[Dmitry Torokhov]

On Wed, Feb 04, 2015 at 10:07:30AM +0100, Hans Verkuil wrote:
 From: Mauro Carvalho Chehab mche...@osg.samsung.com
 
 The cos table used at fixp-arith.h has only 8 bits of precision.
 That causes problems if it is reused on other drivers.
 
 As some media drivers require a higher precision sin/cos
 implementation, replace the current implementation by one that
 will provide 32 bits precision.
 
 The values generated by the new implementation matches the
 32 bit precision of glibc's sin for an angle measured in
 integer degrees.
 
 It also provides support for fractional angles via linear
 interpolation. On experimental calculus, when used a table
 with a 0.001 degree angle, the maximum error for sin is
 0.38, which is likely good enough for practical purposes.
 
 There are some logic there that seems to be specific to the
 usage inside ff-memless.c. Move those logic to there, as they're
 not needed elsewhere.
 
 Cc: Hans de Goede hdego...@redhat.com
 Cc: Dmitry Torokhov dmitry.torok...@gmail.com
 Cc: linux-in...@vger.kernel.org linux-in...@vger.kernel.org
 Signed-off-by: Mauro Carvalho Chehab mche...@osg.samsung.com
 Signed-off-by: Prashant Laddha prlad...@cisco.com
 Signed-off-by: Hans Verkuil hans.verk...@cisco.com

For input bit:

Acked-by: Dmitry Torokhov dmitry.torok...@gmail.com

 ---
  drivers/input/ff-memless.c  |  18 -
  drivers/media/usb/gspca/ov534.c |  11 +--
  include/linux/fixp-arith.h  | 145 
 +---
  3 files changed, 125 insertions(+), 49 deletions(-)
 
 diff --git a/drivers/input/ff-memless.c b/drivers/input/ff-memless.c
 index 74c0d8c..fcc6c33 100644
 --- a/drivers/input/ff-memless.c
 +++ b/drivers/input/ff-memless.c
 @@ -237,6 +237,18 @@ static u16 ml_calculate_direction(u16 direction, u16 
 force,
   (force + new_force))  1;
  }
  
 +#define FRAC_N 8
 +static inline s16 fixp_new16(s16 a)
 +{
 + return ((s32)a)  (16 - FRAC_N);
 +}
 +
 +static inline s16 fixp_mult(s16 a, s16 b)
 +{
 + a = ((s32)a * 0x100) / 0x7fff;
 + return ((s32)(a * b))  FRAC_N;
 +}
 +
  /*
   * Combine two effects and apply gain.
   */
 @@ -247,7 +259,7 @@ static void ml_combine_effects(struct ff_effect *effect,
   struct ff_effect *new = state-effect;
   unsigned int strong, weak, i;
   int x, y;
 - fixp_t level;
 + s16 level;
  
   switch (new-type) {
   case FF_CONSTANT:
 @@ -255,8 +267,8 @@ static void ml_combine_effects(struct ff_effect *effect,
   level = fixp_new16(apply_envelope(state,
   new-u.constant.level,
   new-u.constant.envelope));
 - x = fixp_mult(fixp_sin(i), level) * gain / 0x;
 - y = fixp_mult(-fixp_cos(i), level) * gain / 0x;
 + x = fixp_mult(fixp_sin16(i), level) * gain / 0x;
 + y = fixp_mult(-fixp_cos16(i), level) * gain / 0x;
   /*
* here we abuse ff_ramp to hold x and y of constant force
* If in future any driver wants something else than x and y
 diff --git a/drivers/media/usb/gspca/ov534.c b/drivers/media/usb/gspca/ov534.c
 index a9c866d..146071b 100644
 --- a/drivers/media/usb/gspca/ov534.c
 +++ b/drivers/media/usb/gspca/ov534.c
 @@ -816,21 +816,16 @@ static void sethue(struct gspca_dev *gspca_dev, s32 val)
   s16 huesin;
   s16 huecos;
  
 - /* fixp_sin and fixp_cos accept only positive values, while
 -  * our val is between -90 and 90
 -  */
 - val += 360;
 -
   /* According to the datasheet the registers expect HUESIN and
* HUECOS to be the result of the trigonometric functions,
* scaled by 0x80.
*
 -  * The 0x100 here represents the maximun absolute value
 +  * The 0x7fff here represents the maximum absolute value
* returned byt fixp_sin and fixp_cos, so the scaling will
* consider the result like in the interval [-1.0, 1.0].
*/
 - huesin = fixp_sin(val) * 0x80 / 0x100;
 - huecos = fixp_cos(val) * 0x80 / 0x100;
 + huesin = fixp_sin16(val) * 0x80 / 0x7fff;
 + huecos = fixp_cos16(val) * 0x80 / 0x7fff;
  
   if (huesin  0) {
   sccb_reg_write(gspca_dev, 0xab,
 diff --git a/include/linux/fixp-arith.h b/include/linux/fixp-arith.h
 index 3089d73..d4686fe 100644
 --- a/include/linux/fixp-arith.h
 +++ b/include/linux/fixp-arith.h
 @@ -1,6 +1,8 @@
  #ifndef _FIXP_ARITH_H
  #define _FIXP_ARITH_H
  
 +#include linux/math64.h
 +
  /*
   * Simplistic fixed-point arithmetics.
   * Hmm, I'm probably duplicating some code :(
 @@ -29,59 +31,126 @@
  
  #include linux/types.h
  
 -/* The type representing fixed-point values */
 -typedef s16 fixp_t;
 +static const s32 sin_table[] = {
 + 0x, 0x023be165, 0x04779632, 0x06b2f1d2

[PATCH] [media] exynos4-is: fix error handling of irq_of_parse_and_map

[Dmitry Torokhov]

Return value of irq_of_parse_and_map() is unsigned int, with 0
indicating failure, so testing for negative result never works.

Signed-off-by: Dmitry Torokhov d...@chromium.org
---

Not tested, found by casual code inspection.

 drivers/media/platform/exynos4-is/fimc-is.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/exynos4-is/fimc-is.c 
b/drivers/media/platform/exynos4-is/fimc-is.c
index 94c6b47..0fdca86 100644
--- a/drivers/media/platform/exynos4-is/fimc-is.c
+++ b/drivers/media/platform/exynos4-is/fimc-is.c
@@ -814,9 +814,9 @@ static int fimc_is_probe(struct platform_device *pdev)
return -ENOMEM;
 
is-irq = irq_of_parse_and_map(dev-of_node, 0);
-   if (is-irq  0) {
+   if (!is-irq) {
dev_err(dev, no irq found\n);
-   return is-irq;
+   return -EINVAL;
}
 
ret = fimc_is_get_clocks(is);
-- 
2.1.0.rc2.206.gedb03e5


-- 
Dmitry
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] input:keyboard: keycode KEY_MAX changes some keycode value.

[Dmitry Torokhov]

Hi Andrew,

On Sat, Nov 23, 2013 at 09:45:45PM +0800, andrew.liu200...@gmail.com wrote:
 From: Andrew Liu andrew.liu200...@gmail.com
 
 For exmaple, keycode: KEY_OK(0x160) is changed by and operation with
 KEY_MAX(0x2ff) to KEY_KPENTER(96).
 
 Signed-off-by: Andrew Liu andrew.liu200...@gmail.com
 ---
  drivers/input/keyboard/adp5588-keys.c |8 ++--
  drivers/input/keyboard/adp5589-keys.c |8 ++--
  drivers/input/keyboard/bf54x-keys.c   |8 ++--
  drivers/input/misc/pcf8574_keypad.c   |6 +-
  drivers/media/pci/ttpci/av7110_ir.c   |1 +
  5 files changed, 24 insertions(+), 7 deletions(-)
 
 diff --git a/drivers/input/keyboard/adp5588-keys.c 
 b/drivers/input/keyboard/adp5588-keys.c
 index dbd2047..1ce559e 100644
 --- a/drivers/input/keyboard/adp5588-keys.c
 +++ b/drivers/input/keyboard/adp5588-keys.c
 @@ -535,8 +535,12 @@ static int adp5588_probe(struct i2c_client *client,
   if (pdata-repeat)
   __set_bit(EV_REP, input-evbit);
  
 - for (i = 0; i  input-keycodemax; i++)
 - __set_bit(kpad-keycode[i]  KEY_MAX, input-keybit);
 + for (i = 0; i  input-keycodemax; i++) {
 + if (kpad-keycode[i]  KEY_MAX)
 + kpad-keycode[i] = 0;

There is no need to reset keymap entires to 0, not setting keybit is
enough.

 + else if (kpad-keycode[i]  KEY_RESERVED)
 + __set_bit(kpad-keycode[i], input-keybit);
 + }
   __clear_bit(KEY_RESERVED, input-keybit);
  
   if (kpad-gpimapsize)
 diff --git a/drivers/input/keyboard/adp5589-keys.c 
 b/drivers/input/keyboard/adp5589-keys.c
 index 67d12b3..22ca2a5 100644
 --- a/drivers/input/keyboard/adp5589-keys.c
 +++ b/drivers/input/keyboard/adp5589-keys.c
 @@ -991,8 +991,12 @@ static int adp5589_probe(struct i2c_client *client,
   if (pdata-repeat)
   __set_bit(EV_REP, input-evbit);
  
 - for (i = 0; i  input-keycodemax; i++)
 - __set_bit(kpad-keycode[i]  KEY_MAX, input-keybit);
 + for (i = 0; i  input-keycodemax; i++) {
 + if (kpad-keycode[i]  KEY_MAX)
 + kpad-keycode[i] = 0;
 + else if (kpad-keycode[i]  KEY_RESERVED)
 + __set_bit(kpad-keycode[i], input-keybit);
 + }
   __clear_bit(KEY_RESERVED, input-keybit);
  
   if (kpad-gpimapsize)
 diff --git a/drivers/input/keyboard/bf54x-keys.c 
 b/drivers/input/keyboard/bf54x-keys.c
 index fc88fb4..96f54e7 100644
 --- a/drivers/input/keyboard/bf54x-keys.c
 +++ b/drivers/input/keyboard/bf54x-keys.c
 @@ -288,8 +288,12 @@ static int bfin_kpad_probe(struct platform_device *pdev)
   if (pdata-repeat)
   __set_bit(EV_REP, input-evbit);
  
 - for (i = 0; i  input-keycodemax; i++)
 - __set_bit(bf54x_kpad-keycode[i]  KEY_MAX, input-keybit);
 + for (i = 0; i  input-keycodemax; i++) {
 + if (bf54x_kpad-keycode[i]  KEY_MAX)
 + bf54x_kpad-keycode[i] = 0;
 + else if (bf54x_kpad-keycode[i]  KEY_RESERVED)
 + __set_bit(bf54x_kpad-keycode[i], input-keybit);
 + }
   __clear_bit(KEY_RESERVED, input-keybit);
  
   error = input_register_device(input);
 diff --git a/drivers/input/misc/pcf8574_keypad.c 
 b/drivers/input/misc/pcf8574_keypad.c
 index e373929..48829f3 100644
 --- a/drivers/input/misc/pcf8574_keypad.c
 +++ b/drivers/input/misc/pcf8574_keypad.c
 @@ -114,8 +114,12 @@ static int pcf8574_kp_probe(struct i2c_client *client, 
 const struct i2c_device_i
  
   for (i = 0; i  ARRAY_SIZE(pcf8574_kp_btncode); i++) {
   lp-btncode[i] = pcf8574_kp_btncode[i];
 - __set_bit(lp-btncode[i]  KEY_MAX, idev-keybit);
 + if (lp-btncode[i]  KEY_MAX)
 + lp-btncode[i] = 0;
 + else if (lp-btncode[i]  KEY_RESERVED)
 + __set_bit(lp-btncode[i], idev-keybit);
   }
 + __clear_bit(KEY_RESERVED, idev-keybit);
  
   sprintf(lp-name, DRV_NAME);
   sprintf(lp-phys, kp_data/input0);
 diff --git a/drivers/media/pci/ttpci/av7110_ir.c 
 b/drivers/media/pci/ttpci/av7110_ir.c
 index 0e763a7..7fdac45 100644
 --- a/drivers/media/pci/ttpci/av7110_ir.c
 +++ b/drivers/media/pci/ttpci/av7110_ir.c
 @@ -201,6 +201,7 @@ static void input_register_keys(struct infrared *ir)
   else if (ir-key_map[i]  KEY_RESERVED)
   set_bit(ir-key_map[i], ir-input_dev-keybit);
   }
 + __clear_bit(KEY_RESERVED, ir-input_dev-keybit);

Here we only setting bits for keycodes above KEY_RESERVED so we do not
need to clear KEY_RESERVED bit.

  
   ir-input_dev-keycode = ir-key_map;
   ir-input_dev-keycodesize = sizeof(ir-key_map[0]);
 -- 
 1.7.1
 

Edited and applied, thank you.

-- 
Dmitry
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 4/8] drivers: input: use module_platform_driver_probe()

[Dmitry Torokhov]

Hi Fabio,

On Thursday, March 14, 2013 06:09:34 PM Fabio Porcedda wrote:
 This patch converts the drivers to use the
 module_platform_driver_probe() macro which makes the code smaller and
 a bit simpler.

I already have patches from Sachin Kamat for this, I am waiting for -rc3
to sync up with mainline and pick up the macro before applying them.

Thanks.

-- 
Dmitry
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Media: remove incorrect __exit markups

[Dmitry Torokhov]

On Mon, Feb 25, 2013 at 08:49:26AM +0100, Guennadi Liakhovetski wrote:
 Hi Dmitry
 
 On Sun, 24 Feb 2013, Dmitry Torokhov wrote:
 
  Even if bus is not hot-pluggable, the devices can be unbound from the
  driver via sysfs, so we should not be using __exit annotations on
  remove() methods. The only exception is drivers registered with
  platform_driver_probe() which specifically disables sysfs bind/unbind
  attributes.
  
  Signed-off-by: Dmitry Torokhov dmitry.torok...@gmail.com
  ---
   drivers/media/i2c/adp1653.c  | 4 ++--
   drivers/media/i2c/smiapp/smiapp-core.c   | 4 ++--
   drivers/media/platform/soc_camera/omap1_camera.c | 4 ++--
   drivers/media/radio/radio-si4713.c   | 4 ++--
   drivers/media/rc/ir-rx51.c   | 4 ++--
   5 files changed, 10 insertions(+), 10 deletions(-)
 
 [snip]
 
  diff --git a/drivers/media/platform/soc_camera/omap1_camera.c 
  b/drivers/media/platform/soc_camera/omap1_camera.c
  index 39a77f0..5f548ac 100644
  --- a/drivers/media/platform/soc_camera/omap1_camera.c
  +++ b/drivers/media/platform/soc_camera/omap1_camera.c
  @@ -1677,7 +1677,7 @@ exit:
  return err;
   }
   
  -static int __exit omap1_cam_remove(struct platform_device *pdev)
  +static int omap1_cam_remove(struct platform_device *pdev)
   {
  struct soc_camera_host *soc_host = to_soc_camera_host(pdev-dev);
  struct omap1_cam_dev *pcdev = container_of(soc_host,
  @@ -1709,7 +1709,7 @@ static struct platform_driver omap1_cam_driver = {
  .name   = DRIVER_NAME,
  },
  .probe  = omap1_cam_probe,
  -   .remove = __exit_p(omap1_cam_remove),
  +   .remove = omap1_cam_remove,
   };
   
   module_platform_driver(omap1_cam_driver);
 
 This looks correct, but don't we also have to remove __init from 
 omap1_cam_probe()? Or would that be a separate patch?

Thanks Guennadi, I missed that one, will update the patch.

-- 
Dmitry
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2] Media: remove incorrect __init/__exit markups

[Dmitry Torokhov]

Even if bus is not hot-pluggable, the devices can be unbound from the
driver via sysfs, so we should not be using __exit annotations on
remove() methods. The only exception is drivers registered with
platform_driver_probe() which specifically disables sysfs bind/unbind
attributes.

Similarly probe() methods should not be marked __init unless
platform_driver_probe() is used.

Signed-off-by: Dmitry Torokhov dmitry.torok...@gmail.com
---

v1-v2: removed __init markup on omap1_cam_probe() that was pointed out
by Guennadi Liakhovetski.

 drivers/media/i2c/adp1653.c  | 4 ++--
 drivers/media/i2c/smiapp/smiapp-core.c   | 4 ++--
 drivers/media/platform/soc_camera/omap1_camera.c | 6 +++---
 drivers/media/radio/radio-si4713.c   | 4 ++--
 drivers/media/rc/ir-rx51.c   | 4 ++--
 5 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/media/i2c/adp1653.c b/drivers/media/i2c/adp1653.c
index df16380..ef75abe 100644
--- a/drivers/media/i2c/adp1653.c
+++ b/drivers/media/i2c/adp1653.c
@@ -447,7 +447,7 @@ free_and_quit:
return ret;
 }
 
-static int __exit adp1653_remove(struct i2c_client *client)
+static int adp1653_remove(struct i2c_client *client)
 {
struct v4l2_subdev *subdev = i2c_get_clientdata(client);
struct adp1653_flash *flash = to_adp1653_flash(subdev);
@@ -476,7 +476,7 @@ static struct i2c_driver adp1653_i2c_driver = {
.pm = adp1653_pm_ops,
},
.probe  = adp1653_probe,
-   .remove = __exit_p(adp1653_remove),
+   .remove = adp1653_remove,
.id_table   = adp1653_id_table,
 };
 
diff --git a/drivers/media/i2c/smiapp/smiapp-core.c 
b/drivers/media/i2c/smiapp/smiapp-core.c
index 83c7ed7..cae4f46 100644
--- a/drivers/media/i2c/smiapp/smiapp-core.c
+++ b/drivers/media/i2c/smiapp/smiapp-core.c
@@ -2833,7 +2833,7 @@ static int smiapp_probe(struct i2c_client *client,
 sensor-src-pads, 0);
 }
 
-static int __exit smiapp_remove(struct i2c_client *client)
+static int smiapp_remove(struct i2c_client *client)
 {
struct v4l2_subdev *subdev = i2c_get_clientdata(client);
struct smiapp_sensor *sensor = to_smiapp_sensor(subdev);
@@ -2881,7 +2881,7 @@ static struct i2c_driver smiapp_i2c_driver = {
.pm = smiapp_pm_ops,
},
.probe  = smiapp_probe,
-   .remove = __exit_p(smiapp_remove),
+   .remove = smiapp_remove,
.id_table = smiapp_id_table,
 };
 
diff --git a/drivers/media/platform/soc_camera/omap1_camera.c 
b/drivers/media/platform/soc_camera/omap1_camera.c
index 39a77f0..e5091b7 100644
--- a/drivers/media/platform/soc_camera/omap1_camera.c
+++ b/drivers/media/platform/soc_camera/omap1_camera.c
@@ -1546,7 +1546,7 @@ static struct soc_camera_host_ops omap1_host_ops = {
.poll   = omap1_cam_poll,
 };
 
-static int __init omap1_cam_probe(struct platform_device *pdev)
+static int omap1_cam_probe(struct platform_device *pdev)
 {
struct omap1_cam_dev *pcdev;
struct resource *res;
@@ -1677,7 +1677,7 @@ exit:
return err;
 }
 
-static int __exit omap1_cam_remove(struct platform_device *pdev)
+static int omap1_cam_remove(struct platform_device *pdev)
 {
struct soc_camera_host *soc_host = to_soc_camera_host(pdev-dev);
struct omap1_cam_dev *pcdev = container_of(soc_host,
@@ -1709,7 +1709,7 @@ static struct platform_driver omap1_cam_driver = {
.name   = DRIVER_NAME,
},
.probe  = omap1_cam_probe,
-   .remove = __exit_p(omap1_cam_remove),
+   .remove = omap1_cam_remove,
 };
 
 module_platform_driver(omap1_cam_driver);
diff --git a/drivers/media/radio/radio-si4713.c 
b/drivers/media/radio/radio-si4713.c
index 1507c9d..8ae8442d 100644
--- a/drivers/media/radio/radio-si4713.c
+++ b/drivers/media/radio/radio-si4713.c
@@ -328,7 +328,7 @@ exit:
 }
 
 /* radio_si4713_pdriver_remove - remove the device */
-static int __exit radio_si4713_pdriver_remove(struct platform_device *pdev)
+static int radio_si4713_pdriver_remove(struct platform_device *pdev)
 {
struct v4l2_device *v4l2_dev = platform_get_drvdata(pdev);
struct radio_si4713_device *rsdev = container_of(v4l2_dev,
@@ -350,7 +350,7 @@ static struct platform_driver radio_si4713_pdriver = {
.name   = radio-si4713,
},
.probe  = radio_si4713_pdriver_probe,
-   .remove = __exit_p(radio_si4713_pdriver_remove),
+   .remove = radio_si4713_pdriver_remove,
 };
 
 module_platform_driver(radio_si4713_pdriver);
diff --git a/drivers/media/rc/ir-rx51.c b/drivers/media/rc/ir-rx51.c
index 8ead492..31b955b 100644
--- a/drivers/media/rc/ir-rx51.c
+++ b/drivers/media/rc/ir-rx51.c
@@ -464,14 +464,14 @@ static int lirc_rx51_probe(struct platform_device *dev)
return 0;
 }
 
-static int __exit lirc_rx51_remove(struct platform_device *dev)
+static

[PATCH] Media: remove incorrect __exit markups

[Dmitry Torokhov]

Even if bus is not hot-pluggable, the devices can be unbound from the
driver via sysfs, so we should not be using __exit annotations on
remove() methods. The only exception is drivers registered with
platform_driver_probe() which specifically disables sysfs bind/unbind
attributes.

Signed-off-by: Dmitry Torokhov dmitry.torok...@gmail.com
---
 drivers/media/i2c/adp1653.c  | 4 ++--
 drivers/media/i2c/smiapp/smiapp-core.c   | 4 ++--
 drivers/media/platform/soc_camera/omap1_camera.c | 4 ++--
 drivers/media/radio/radio-si4713.c   | 4 ++--
 drivers/media/rc/ir-rx51.c   | 4 ++--
 5 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/media/i2c/adp1653.c b/drivers/media/i2c/adp1653.c
index df16380..ef75abe 100644
--- a/drivers/media/i2c/adp1653.c
+++ b/drivers/media/i2c/adp1653.c
@@ -447,7 +447,7 @@ free_and_quit:
return ret;
 }
 
-static int __exit adp1653_remove(struct i2c_client *client)
+static int adp1653_remove(struct i2c_client *client)
 {
struct v4l2_subdev *subdev = i2c_get_clientdata(client);
struct adp1653_flash *flash = to_adp1653_flash(subdev);
@@ -476,7 +476,7 @@ static struct i2c_driver adp1653_i2c_driver = {
.pm = adp1653_pm_ops,
},
.probe  = adp1653_probe,
-   .remove = __exit_p(adp1653_remove),
+   .remove = adp1653_remove,
.id_table   = adp1653_id_table,
 };
 
diff --git a/drivers/media/i2c/smiapp/smiapp-core.c 
b/drivers/media/i2c/smiapp/smiapp-core.c
index 83c7ed7..cae4f46 100644
--- a/drivers/media/i2c/smiapp/smiapp-core.c
+++ b/drivers/media/i2c/smiapp/smiapp-core.c
@@ -2833,7 +2833,7 @@ static int smiapp_probe(struct i2c_client *client,
 sensor-src-pads, 0);
 }
 
-static int __exit smiapp_remove(struct i2c_client *client)
+static int smiapp_remove(struct i2c_client *client)
 {
struct v4l2_subdev *subdev = i2c_get_clientdata(client);
struct smiapp_sensor *sensor = to_smiapp_sensor(subdev);
@@ -2881,7 +2881,7 @@ static struct i2c_driver smiapp_i2c_driver = {
.pm = smiapp_pm_ops,
},
.probe  = smiapp_probe,
-   .remove = __exit_p(smiapp_remove),
+   .remove = smiapp_remove,
.id_table = smiapp_id_table,
 };
 
diff --git a/drivers/media/platform/soc_camera/omap1_camera.c 
b/drivers/media/platform/soc_camera/omap1_camera.c
index 39a77f0..5f548ac 100644
--- a/drivers/media/platform/soc_camera/omap1_camera.c
+++ b/drivers/media/platform/soc_camera/omap1_camera.c
@@ -1677,7 +1677,7 @@ exit:
return err;
 }
 
-static int __exit omap1_cam_remove(struct platform_device *pdev)
+static int omap1_cam_remove(struct platform_device *pdev)
 {
struct soc_camera_host *soc_host = to_soc_camera_host(pdev-dev);
struct omap1_cam_dev *pcdev = container_of(soc_host,
@@ -1709,7 +1709,7 @@ static struct platform_driver omap1_cam_driver = {
.name   = DRIVER_NAME,
},
.probe  = omap1_cam_probe,
-   .remove = __exit_p(omap1_cam_remove),
+   .remove = omap1_cam_remove,
 };
 
 module_platform_driver(omap1_cam_driver);
diff --git a/drivers/media/radio/radio-si4713.c 
b/drivers/media/radio/radio-si4713.c
index 1507c9d..8ae8442d 100644
--- a/drivers/media/radio/radio-si4713.c
+++ b/drivers/media/radio/radio-si4713.c
@@ -328,7 +328,7 @@ exit:
 }
 
 /* radio_si4713_pdriver_remove - remove the device */
-static int __exit radio_si4713_pdriver_remove(struct platform_device *pdev)
+static int radio_si4713_pdriver_remove(struct platform_device *pdev)
 {
struct v4l2_device *v4l2_dev = platform_get_drvdata(pdev);
struct radio_si4713_device *rsdev = container_of(v4l2_dev,
@@ -350,7 +350,7 @@ static struct platform_driver radio_si4713_pdriver = {
.name   = radio-si4713,
},
.probe  = radio_si4713_pdriver_probe,
-   .remove = __exit_p(radio_si4713_pdriver_remove),
+   .remove = radio_si4713_pdriver_remove,
 };
 
 module_platform_driver(radio_si4713_pdriver);
diff --git a/drivers/media/rc/ir-rx51.c b/drivers/media/rc/ir-rx51.c
index 8ead492..31b955b 100644
--- a/drivers/media/rc/ir-rx51.c
+++ b/drivers/media/rc/ir-rx51.c
@@ -464,14 +464,14 @@ static int lirc_rx51_probe(struct platform_device *dev)
return 0;
 }
 
-static int __exit lirc_rx51_remove(struct platform_device *dev)
+static int lirc_rx51_remove(struct platform_device *dev)
 {
return lirc_unregister_driver(lirc_rx51_driver.minor);
 }
 
 struct platform_driver lirc_rx51_platform_driver = {
.probe  = lirc_rx51_probe,
-   .remove = __exit_p(lirc_rx51_remove),
+   .remove = lirc_rx51_remove,
.suspend= lirc_rx51_suspend,
.resume = lirc_rx51_resume,
.driver = {
-- 
1.7.11.7


-- 
Dmitry
--
To unsubscribe from this list: send the line unsubscribe

Re: [PATCH 5/5] kfifo: log based kfifo API

[Dmitry Torokhov]

Hi Yuanhan,

On Tue, Jan 08, 2013 at 10:57:53PM +0800, Yuanhan Liu wrote:
 The current kfifo API take the kfifo size as input, while it rounds
  _down_ the size to power of 2 at __kfifo_alloc. This may introduce
 potential issue.
 
 Take the code at drivers/hid/hid-logitech-dj.c as example:
 
   if (kfifo_alloc(djrcv_dev-notif_fifo,
DJ_MAX_NUMBER_NOTIFICATIONS * sizeof(struct dj_report),
GFP_KERNEL)) {
 
 Where, DJ_MAX_NUMBER_NOTIFICATIONS is 8, and sizeo of(struct dj_report)
 is 15.
 
 Which means it wants to allocate a kfifo buffer which can store 8
 dj_report entries at once. The expected kfifo buffer size would be
 8 * 15 = 120 then. While, in the end, __kfifo_alloc will turn the
 size to rounddown_power_of_2(120) =  64, and then allocate a buf
 with 64 bytes, which I don't think this is the original author want.
 
 With the new log API, we can do like following:
 
   int kfifo_size_order = order_base_2(DJ_MAX_NUMBER_NOTIFICATIONS *
   sizeof(struct dj_report));
 
   if (kfifo_alloc(djrcv_dev-notif_fifo, kfifo_size_order, GFP_KERNEL)) {
 
 This make sure we will allocate enough kfifo buffer for holding
 DJ_MAX_NUMBER_NOTIFICATIONS dj_report entries.

Why don't you simply change __kfifo_alloc to round the allocation up
instead of down?

Thanks.

-- 
Dmitry
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] Input: move drivers/input/fixp-arith.h to include/linux

[Dmitry Torokhov]

On Mon, Apr 23, 2012 at 03:21:06PM +0200, Antonio Ospite wrote:
 Move drivers/input/fixp-arith.h to include/linux so that the functions
 defined there can be used by other subsystems, for instance some video
 devices ISPs can control the output HUE value by setting registers for
 sin(HUE) and cos(HUE).
 
 Signed-off-by: Antonio Ospite osp...@studenti.unina.it

Acked-by: Dmitry Torokhov dmitry.torok...@gmail.com

 ---
  drivers/input/ff-memless.c|3 +--
  {drivers/input = include/linux}/fixp-arith.h |0
  2 files changed, 1 insertion(+), 2 deletions(-)
  rename {drivers/input = include/linux}/fixp-arith.h (100%)
 
 diff --git a/drivers/input/ff-memless.c b/drivers/input/ff-memless.c
 index 117a59a..5f55885 100644
 --- a/drivers/input/ff-memless.c
 +++ b/drivers/input/ff-memless.c
 @@ -31,8 +31,7 @@
  #include linux/mutex.h
  #include linux/spinlock.h
  #include linux/jiffies.h
 -
 -#include fixp-arith.h
 +#include linux/fixp-arith.h
  
  MODULE_LICENSE(GPL);
  MODULE_AUTHOR(Anssi Hannula anssi.hann...@gmail.com);
 diff --git a/drivers/input/fixp-arith.h b/include/linux/fixp-arith.h
 similarity index 100%
 rename from drivers/input/fixp-arith.h
 rename to include/linux/fixp-arith.h
 -- 
 1.7.10
 

-- 
Dmitry
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/3] gspca - ov534: saturation and hue (using fixp-arith.h)

[Dmitry Torokhov]

On Mon, Apr 23, 2012 at 03:21:04PM +0200, Antonio Ospite wrote:
 Hi,
 
 here are a couple more of controls for the gspca ov534 subdriver.
 
 In order to control the HUE value the sensor expects sin(HUE) and
 cos(HUE) to be set so I decided to reuse the fixed point implementation
 of sine and cosine from drivers/input/fixp-arith.h, see patches 2 and 3.
 
 Dmitry, can the movement of fixp-arith.h in patch 2 go via the media
 tree?  That should ease up the integration of patch 3 in this series
 I think.

Yep, this is fine with me.

 
 Jonathan, maybe fixp_sin() and fixp_cos() can be used in
 drivers/media/video/ov7670.c too where currently ov7670_sine() and
 ov7670_cosine() are defined, but I didn't want to send a patch I could
 not test.
 
 BTW What is the usual way to communicate these cross-subsystem stuff?
 I CC-ed everybody only on the cover letter and on patches 2 and 3 which
 are the ones concerning somehow both input and media.
 
 Thanks,
Antonio
 
 
 Antonio Ospite (3):
   [media] gspca - ov534: Add Saturation control
   Input: move drivers/input/fixp-arith.h to include/linux
   [media] gspca - ov534: Add Hue control
 
  drivers/input/ff-memless.c|3 +-
  drivers/input/fixp-arith.h|   87 
  drivers/media/video/gspca/ov534.c |   89 
 -
  include/linux/fixp-arith.h|   87 
  4 files changed, 176 insertions(+), 90 deletions(-)
  delete mode 100644 drivers/input/fixp-arith.h
  create mode 100644 include/linux/fixp-arith.h
 
 -- 
 Antonio Ospite
 http://ao2.it
 
 A: Because it messes up the order in which people normally read text.
See http://en.wikipedia.org/wiki/Posting_style
 Q: Why is top-posting such a bad thing?

-- 
Dmitry
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/5] Driver core: driver_find() drops reference before returning

[Dmitry Torokhov]

On Tue, Jan 24, 2012 at 01:34:24PM -0500, Alan Stern wrote:
 As part of the removal of get_driver()/put_driver(), this patch
 (as1510) changes driver_find(); it now drops the reference it acquires
 before returning.  The patch also adjusts all the callers of
 driver_find() to remove the now unnecessary calls to put_driver().
 
 In addition, the patch adds a warning to driver_find(): Callers must
 make sure the driver they are searching for does not get unloaded
 while they are using it.  This has always been the case; driver_find()
 has never prevented a driver from being unregistered or unloaded.
 Hence the patch will not introduce any new bugs.  The existing callers
 all seem to be okay in this respect, however I don't understand the
 video drivers well enough to be certain about them.
 
 Signed-off-by: Alan Stern st...@rowland.harvard.edu
 CC: Dmitry Torokhov dmitry.torok...@gmail.com
 CC: Kyungmin Park kyungmin.p...@samsung.com
 CC: Andy Walls awa...@md.metrocast.net
 CC: Martin Schwidefsky schwidef...@de.ibm.com
 

Acked-by: Dmitry Torokhov d...@mail.ru

for serio and gameport parts.

 ---
 
  drivers/base/driver.c   |7 +--
  drivers/input/gameport/gameport.c   |1 -
  drivers/input/serio/serio.c |1 -
  drivers/media/video/cx18/cx18-alsa-main.c   |1 -
  drivers/media/video/ivtv/ivtvfb.c   |2 --
  drivers/media/video/s5p-fimc/fimc-mdevice.c |5 +
  drivers/media/video/s5p-tv/mixer_video.c|1 -
  drivers/s390/net/smsgiucv_app.c |9 -
  8 files changed, 10 insertions(+), 17 deletions(-)
 
 Index: usb-3.3/drivers/base/driver.c
 ===
 --- usb-3.3.orig/drivers/base/driver.c
 +++ usb-3.3/drivers/base/driver.c
 @@ -234,7 +234,6 @@ int driver_register(struct device_driver
  
   other = driver_find(drv-name, drv-bus);
   if (other) {
 - put_driver(other);
   printk(KERN_ERR Error: Driver '%s' is already registered, 
   aborting...\n, drv-name);
   return -EBUSY;
 @@ -275,7 +274,9 @@ EXPORT_SYMBOL_GPL(driver_unregister);
   * Call kset_find_obj() to iterate over list of drivers on
   * a bus to find driver by name. Return driver if found.
   *
 - * Note that kset_find_obj increments driver's reference count.
 + * This routine provides no locking to prevent the driver it returns
 + * from being unregistered or unloaded while the caller is using it.
 + * The caller is responsible for preventing this.
   */
  struct device_driver *driver_find(const char *name, struct bus_type *bus)
  {
 @@ -283,6 +284,8 @@ struct device_driver *driver_find(const
   struct driver_private *priv;
  
   if (k) {
 + /* Drop reference added by kset_find_obj() */
 + kobject_put(k);
   priv = to_driver(k);
   return priv-driver;
   }
 Index: usb-3.3/drivers/input/gameport/gameport.c
 ===
 --- usb-3.3.orig/drivers/input/gameport/gameport.c
 +++ usb-3.3/drivers/input/gameport/gameport.c
 @@ -449,7 +449,6 @@ static ssize_t gameport_rebind_driver(st
   } else if ((drv = driver_find(buf, gameport_bus)) != NULL) {
   gameport_disconnect_port(gameport);
   error = gameport_bind_driver(gameport, to_gameport_driver(drv));
 - put_driver(drv);
   } else {
   error = -EINVAL;
   }
 Index: usb-3.3/drivers/input/serio/serio.c
 ===
 --- usb-3.3.orig/drivers/input/serio/serio.c
 +++ usb-3.3/drivers/input/serio/serio.c
 @@ -441,7 +441,6 @@ static ssize_t serio_rebind_driver(struc
   } else if ((drv = driver_find(buf, serio_bus)) != NULL) {
   serio_disconnect_port(serio);
   error = serio_bind_driver(serio, to_serio_driver(drv));
 - put_driver(drv);
   serio_remove_duplicate_events(serio, SERIO_RESCAN_PORT);
   } else {
   error = -EINVAL;
 Index: usb-3.3/drivers/media/video/cx18/cx18-alsa-main.c
 ===
 --- usb-3.3.orig/drivers/media/video/cx18/cx18-alsa-main.c
 +++ usb-3.3/drivers/media/video/cx18/cx18-alsa-main.c
 @@ -285,7 +285,6 @@ static void __exit cx18_alsa_exit(void)
  
   drv = driver_find(cx18, pci_bus_type);
   ret = driver_for_each_device(drv, NULL, NULL, cx18_alsa_exit_callback);
 - put_driver(drv);
  
   cx18_ext_init = NULL;
   printk(KERN_INFO cx18-alsa: module unload complete\n);
 Index: usb-3.3/drivers/media/video/ivtv/ivtvfb.c
 ===
 --- usb-3.3.orig/drivers/media/video/ivtv/ivtvfb.c
 +++ usb-3.3/drivers/media/video/ivtv/ivtvfb.c
 @@ -1293,7 +1293,6 @@ static int __init ivtvfb_init(void)
  
   drv = driver_find(ivtv, pci_bus_type);
   err

Re: [PATCH 5/7] [media] ati_remote: add keymap for Medion X10 RF remote

[Dmitry Torokhov]

On Sun, Aug 07, 2011 at 01:18:11AM +0300, Anssi Hannula wrote:
 Add keymap for the Medion X10 RF remote which uses the ati_remote
 driver, and default to it based on the usb id.

Since rc-core supports loading custom keytmaps should we ass medion
keymap here?

I think we should keep the original keymap to avoid regressions, but new
keymaps should be offloaded to udev.

Thanks.

 
 The keymap is adapted from an earlier patch by
 Jan Losinski losin...@wh2.tu-dresden.de:
 https://lkml.org/lkml/2011/4/18/104
 with KEY_PLAYPAUSE replaced by KEY_PAUSE.
 
 Signed-off-by: Anssi Hannula anssi.hann...@iki.fi
 ---
  drivers/media/rc/ati_remote.c|   17 +++--
  drivers/media/rc/keymaps/Makefile|1 +
  drivers/media/rc/keymaps/rc-medion-x10.c |  116 
 ++
  include/media/rc-map.h   |1 +
  4 files changed, 128 insertions(+), 7 deletions(-)
  create mode 100644 drivers/media/rc/keymaps/rc-medion-x10.c
 
 diff --git a/drivers/media/rc/ati_remote.c b/drivers/media/rc/ati_remote.c
 index 74cc6b1..4576462 100644
 --- a/drivers/media/rc/ati_remote.c
 +++ b/drivers/media/rc/ati_remote.c
 @@ -151,11 +151,11 @@ MODULE_PARM_DESC(mouse, Enable mouse device, default = 
 yes);
  #define err(format, arg...) printk(KERN_ERR format , ## arg)
  
  static struct usb_device_id ati_remote_table[] = {
 - { USB_DEVICE(ATI_REMOTE_VENDOR_ID, LOLA_REMOTE_PRODUCT_ID) },
 - { USB_DEVICE(ATI_REMOTE_VENDOR_ID, LOLA2_REMOTE_PRODUCT_ID) },
 - { USB_DEVICE(ATI_REMOTE_VENDOR_ID, ATI_REMOTE_PRODUCT_ID) },
 - { USB_DEVICE(ATI_REMOTE_VENDOR_ID, NVIDIA_REMOTE_PRODUCT_ID) },
 - { USB_DEVICE(ATI_REMOTE_VENDOR_ID, MEDION_REMOTE_PRODUCT_ID) },
 + { USB_DEVICE(ATI_REMOTE_VENDOR_ID, LOLA_REMOTE_PRODUCT_ID), 
 .driver_info = (unsigned long)RC_MAP_ATI_X10 },
 + { USB_DEVICE(ATI_REMOTE_VENDOR_ID, LOLA2_REMOTE_PRODUCT_ID),
 .driver_info = (unsigned long)RC_MAP_ATI_X10 },
 + { USB_DEVICE(ATI_REMOTE_VENDOR_ID, ATI_REMOTE_PRODUCT_ID),  
 .driver_info = (unsigned long)RC_MAP_ATI_X10 },
 + { USB_DEVICE(ATI_REMOTE_VENDOR_ID, NVIDIA_REMOTE_PRODUCT_ID),   
 .driver_info = (unsigned long)RC_MAP_ATI_X10 },
 + { USB_DEVICE(ATI_REMOTE_VENDOR_ID, MEDION_REMOTE_PRODUCT_ID),   
 .driver_info = (unsigned long)RC_MAP_MEDION_X10 },
   {}  /* Terminating entry */
  };
  
 @@ -714,8 +714,6 @@ static void ati_remote_rc_init(struct ati_remote 
 *ati_remote)
  
   usb_to_input_id(ati_remote-udev, rdev-input_id);
   rdev-dev.parent = ati_remote-interface-dev;
 -
 - rdev-map_name = RC_MAP_ATI_X10;
  }
  
  static int ati_remote_initialize(struct ati_remote *ati_remote)
 @@ -827,6 +825,11 @@ static int ati_remote_probe(struct usb_interface 
 *interface, const struct usb_de
   snprintf(ati_remote-mouse_name, sizeof(ati_remote-mouse_name),
%s mouse, ati_remote-rc_name);
  
 + if (id-driver_info)
 + rc_dev-map_name = (const char *)id-driver_info;
 + else
 + rc_dev-map_name = RC_MAP_ATI_X10;
 +
   ati_remote_rc_init(ati_remote);
   mutex_init(ati_remote-open_mutex);
   
 diff --git a/drivers/media/rc/keymaps/Makefile 
 b/drivers/media/rc/keymaps/Makefile
 index 3ca9265b7..c3d0f34 100644
 --- a/drivers/media/rc/keymaps/Makefile
 +++ b/drivers/media/rc/keymaps/Makefile
 @@ -48,6 +48,7 @@ obj-$(CONFIG_RC_MAP) += rc-adstech-dvb-t-pci.o \
   rc-lirc.o \
   rc-lme2510.o \
   rc-manli.o \
 + rc-medion-x10.o \
   rc-msi-digivox-ii.o \
   rc-msi-digivox-iii.o \
   rc-msi-tvanywhere.o \
 diff --git a/drivers/media/rc/keymaps/rc-medion-x10.c 
 b/drivers/media/rc/keymaps/rc-medion-x10.c
 new file mode 100644
 index 000..ff62aab
 --- /dev/null
 +++ b/drivers/media/rc/keymaps/rc-medion-x10.c
 @@ -0,0 +1,116 @@
 +/*
 + * Medion X10 RF remote keytable
 + *
 + * Copyright (C) 2011 Anssi Hannula anssi.hannula@ıki.fi
 + *
 + * This file is based on a keytable provided by
 + * Jan Losinski losin...@wh2.tu-dresden.de
 + *
 + * This program is free software; you can redistribute it and/or modify
 + * it under the terms of the GNU General Public License as published by
 + * the Free Software Foundation; either version 2 of the License, or
 + * (at your option) any later version.
 + *
 + * This program is distributed in the hope that it will be useful,
 + * but WITHOUT ANY WARRANTY; without even the implied warranty of
 + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 + * GNU General Public License for more details.
 + *
 + * You should have received a copy of the GNU General Public License along
 + * with this program; if not, write to the Free Software Foundation, Inc.,
 + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
 + */
 +
 +#include media/rc-map.h
 +
 +static struct rc_map_table medion_x10[] = {
 + { 0xf12c, KEY_TV },/* TV */
 

Re: [git:v4l-dvb/for_v3.1] [media] rc: call input_sync after scancode reports

[Dmitry Torokhov]

On Fri, Jul 01, 2011 at 09:34:45PM +0200, Mauro Carvalho Chehab wrote:
 This is an automatic generated email to let you know that the following patch 
 were queued at the 
 http://git.linuxtv.org/media_tree.git tree:
 
 Subject: [media] rc: call input_sync after scancode reports
 Author:  Jarod Wilson ja...@redhat.com
 Date:Thu Jun 23 10:40:55 2011 -0300
 
 Due to commit cdda911c34006f1089f3c87b1a1f31ab3a4722f2, evdev only
 becomes readable when the buffer contains an EV_SYN/SYN_REPORT event. If
 we get a repeat or a scancode we don't have a mapping for, we never call
 input_sync, and thus those events don't get reported in a timely
 fashion.

Hmm, any chance to get it into 3.0?

-- 
Dmitry
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [git:v4l-dvb/for_v3.1] [media] rc: call input_sync after scancode reports

[Dmitry Torokhov]

On Thu, Jul 07, 2011 at 08:28:12PM -0400, Jarod Wilson wrote:
 On Thu, Jul 7, 2011 at 7:58 PM, Dmitry Torokhov
 dmitry.torok...@gmail.com wrote:
  On Fri, Jul 01, 2011 at 09:34:45PM +0200, Mauro Carvalho Chehab wrote:
  This is an automatic generated email to let you know that the following 
  patch were queued at the
  http://git.linuxtv.org/media_tree.git tree:
 
  Subject: [media] rc: call input_sync after scancode reports
  Author:  Jarod Wilson ja...@redhat.com
  Date:    Thu Jun 23 10:40:55 2011 -0300
 
  Due to commit cdda911c34006f1089f3c87b1a1f31ab3a4722f2, evdev only
  becomes readable when the buffer contains an EV_SYN/SYN_REPORT event. If
  we get a repeat or a scancode we don't have a mapping for, we never call
  input_sync, and thus those events don't get reported in a timely
  fashion.
 
  Hmm, any chance to get it into 3.0?
 
 Its actually already there, I think the branch was just mis-named, or
 something along those lines.
 
 http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=98c32bcded0e249fd48726930ae9f393e0e318b4
 

Ah, good then.

Thanks.

-- 
Dmitry
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] [media] rc: call input_sync after scancode reports

[Dmitry Torokhov]

Hi Jarod,

On Thu, Jun 23, 2011 at 01:58:06PM -0400, Jarod Wilson wrote:
 @@ -623,6 +624,7 @@ static void ir_do_keydown(struct rc_dev *dev, int 
 scancode,
 u32 keycode, u8 toggle)
  {
   input_event(dev-input_dev, EV_MSC, MSC_SCAN, scancode);
 + input_sync(dev-input_dev);
  
   /* Repeat event? */
   if (dev-keypressed 

It looks like we would be issuing up to 3 input_sync() for a single
keypress... Order of events is wrong too (we first issue MSC_SCAN for
new key and then release old key). How about we change it a bit like in
the patch below?

-- 
Dmitry


Input: rc - call input_sync after scancode reports

From: Jarod Wilson ja...@redhat.com

Due to commit cdda911c34006f1089f3c87b1a1f31ab3a4722f2, evdev only
becomes readable when the buffer contains an EV_SYN/SYN_REPORT event. If
we get a repeat or a scancode we don't have a mapping for, we never call
input_sync, and thus those events don't get reported in a timely
fashion.

For example, take an mceusb transceiver with a default rc6 keymap. Press
buttons on an rc5 remote while monitoring with ir-keytable, and you'll
see nothing. Now press a button on the rc6 remote matching the keymap.
You'll suddenly get the rc5 key scancodes, the rc6 scancode and the rc6
key spit out all at the same time.

Pressing and holding a button on a remote we do have a keymap for also
works rather unreliably right now, due to repeat events also happening
without a call to input_sync (we bail from ir_do_keydown before getting
to the point where it calls input_sync).

Easy fix though, just add two strategically placed input_sync calls
right after our input_event calls for EV_MSC, and all is well again.
Technically, we probably should have been doing this all along, its just
that it never caused any function difference until the referenced change
went into the input layer.

Reported-by: Stephan Raue sr...@openelec.tv
CC: Mauro Carvalho Chehab mche...@redhat.com
CC: Jeff Brown jeffbr...@android.com
Signed-off-by: Jarod Wilson ja...@redhat.com
Signed-off-by: Dmitry Torokhov d...@mail.ru
---

 drivers/media/rc/rc-main.c |   41 ++---
 1 files changed, 22 insertions(+), 19 deletions(-)


diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
index f57cd56..237cf84 100644
--- a/drivers/media/rc/rc-main.c
+++ b/drivers/media/rc/rc-main.c
@@ -597,6 +597,7 @@ void rc_repeat(struct rc_dev *dev)
spin_lock_irqsave(dev-keylock, flags);
 
input_event(dev-input_dev, EV_MSC, MSC_SCAN, dev-last_scancode);
+   input_sync(dev-input_dev);
 
if (!dev-keypressed)
goto out;
@@ -622,29 +623,31 @@ EXPORT_SYMBOL_GPL(rc_repeat);
 static void ir_do_keydown(struct rc_dev *dev, int scancode,
  u32 keycode, u8 toggle)
 {
-   input_event(dev-input_dev, EV_MSC, MSC_SCAN, scancode);
-
-   /* Repeat event? */
-   if (dev-keypressed 
-   dev-last_scancode == scancode 
-   dev-last_toggle == toggle)
-   return;
+   bool new_event = !dev-keypressed ||
+dev-last_scancode != scancode ||
+dev-last_toggle != toggle;
+
+   if (new_event  dev-keypressed) {
+   /* Release old keypress */
+   IR_dprintk(1, keyup previous key 0x%04x\n, dev-last_keycode);
+   input_report_key(dev-input_dev, dev-last_keycode, 0);
+   dev-keypressed = false;
+   }
 
-   /* Release old keypress */
-   ir_do_keyup(dev);
+   input_event(dev-input_dev, EV_MSC, MSC_SCAN, scancode);
 
-   dev-last_scancode = scancode;
-   dev-last_toggle = toggle;
-   dev-last_keycode = keycode;
+   if (new_event  keycode != KEY_RESERVED) {
+   /* Register a keypress */
+   dev-keypressed = true;
+   IR_dprintk(1, %s: key down event, key 0x%04x, scancode 
0x%04x\n,
+  dev-input_name, keycode, scancode);
+   input_report_key(dev-input_dev, dev-last_keycode, 1);
 
-   if (keycode == KEY_RESERVED)
-   return;
+   dev-last_scancode = scancode;
+   dev-last_toggle = toggle;
+   dev-last_keycode = keycode;
+   }
 
-   /* Register a keypress */
-   dev-keypressed = true;
-   IR_dprintk(1, %s: key down event, key 0x%04x, scancode 0x%04x\n,
-  dev-input_name, keycode, scancode);
-   input_report_key(dev-input_dev, dev-last_keycode, 1);
input_sync(dev-input_dev);
 }
 
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] [media] rc: call input_sync after scancode reports

[Dmitry Torokhov]

On Thu, Jun 23, 2011 at 05:13:24PM -0400, Jarod Wilson wrote:
 Due to commit cdda911c34006f1089f3c87b1a1f31ab3a4722f2, evdev only
 becomes readable when the buffer contains an EV_SYN/SYN_REPORT event. If
 we get a repeat or a scancode we don't have a mapping for, we never call
 input_sync, and thus those events don't get reported in a timely
 fashion.
 
 For example, take an mceusb transceiver with a default rc6 keymap. Press
 buttons on an rc5 remote while monitoring with ir-keytable, and you'll
 see nothing. Now press a button on the rc6 remote matching the keymap.
 You'll suddenly get the rc5 key scancodes, the rc6 scancode and the rc6
 key spit out all at the same time.
 
 Pressing and holding a button on a remote we do have a keymap for also
 works rather unreliably right now, due to repeat events also happening
 without a call to input_sync (we bail from ir_do_keydown before getting
 to the point where it calls input_sync).
 
 Easy fix though, just add two strategically placed input_sync calls
 right after our input_event calls for EV_MSC, and all is well again.
 Technically, we probably should have been doing this all along, its just
 that it never caused any functional difference until the referenced
 change went into the input layer.
 
 v2: rework code a bit, per Dmitry's example, so that we only call
 input_sync once per IR signal. There was another hidden bug in the code
 where we were calling input_report_key using last_keycode instead of our
 just discovered keycode, which manifested with the reordering of calling
 input_report_key and setting last_keycode.
 
 Reported-by: Stephan Raue sr...@openelec.tv
 CC: Stephan Raue sr...@openelec.tv
 CC: Mauro Carvalho Chehab mche...@redhat.com
 CC: Jeff Brown jeffbr...@android.com
 CC: Dmitry Torokhov d...@mail.ru
 Signed-off-by: Jarod Wilson ja...@redhat.com

Acked-by: Dmitry Torokhov d...@mail.ru

 ---
  drivers/media/rc/rc-main.c |   48 ++-
  1 files changed, 25 insertions(+), 23 deletions(-)
 
 diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
 index f57cd56..3186ac7 100644
 --- a/drivers/media/rc/rc-main.c
 +++ b/drivers/media/rc/rc-main.c
 @@ -522,18 +522,20 @@ EXPORT_SYMBOL_GPL(rc_g_keycode_from_table);
  /**
   * ir_do_keyup() - internal function to signal the release of a keypress
   * @dev: the struct rc_dev descriptor of the device
 + * @sync:whether or not to call input_sync
   *
   * This function is used internally to release a keypress, it must be
   * called with keylock held.
   */
 -static void ir_do_keyup(struct rc_dev *dev)
 +static void ir_do_keyup(struct rc_dev *dev, bool sync)
  {
   if (!dev-keypressed)
   return;
  
   IR_dprintk(1, keyup key 0x%04x\n, dev-last_keycode);
   input_report_key(dev-input_dev, dev-last_keycode, 0);
 - input_sync(dev-input_dev);
 + if (sync)
 + input_sync(dev-input_dev);
   dev-keypressed = false;
  }
  
 @@ -549,7 +551,7 @@ void rc_keyup(struct rc_dev *dev)
   unsigned long flags;
  
   spin_lock_irqsave(dev-keylock, flags);
 - ir_do_keyup(dev);
 + ir_do_keyup(dev, true);
   spin_unlock_irqrestore(dev-keylock, flags);
  }
  EXPORT_SYMBOL_GPL(rc_keyup);
 @@ -578,7 +580,7 @@ static void ir_timer_keyup(unsigned long cookie)
*/
   spin_lock_irqsave(dev-keylock, flags);
   if (time_is_before_eq_jiffies(dev-keyup_jiffies))
 - ir_do_keyup(dev);
 + ir_do_keyup(dev, true);
   spin_unlock_irqrestore(dev-keylock, flags);
  }
  
 @@ -597,6 +599,7 @@ void rc_repeat(struct rc_dev *dev)
   spin_lock_irqsave(dev-keylock, flags);
  
   input_event(dev-input_dev, EV_MSC, MSC_SCAN, dev-last_scancode);
 + input_sync(dev-input_dev);
  
   if (!dev-keypressed)
   goto out;
 @@ -622,29 +625,28 @@ EXPORT_SYMBOL_GPL(rc_repeat);
  static void ir_do_keydown(struct rc_dev *dev, int scancode,
 u32 keycode, u8 toggle)
  {
 - input_event(dev-input_dev, EV_MSC, MSC_SCAN, scancode);
 -
 - /* Repeat event? */
 - if (dev-keypressed 
 - dev-last_scancode == scancode 
 - dev-last_toggle == toggle)
 - return;
 + bool new_event = !dev-keypressed ||
 +  dev-last_scancode != scancode ||
 +  dev-last_toggle != toggle;
  
 - /* Release old keypress */
 - ir_do_keyup(dev);
 + if (new_event  dev-keypressed)
 + ir_do_keyup(dev, false);
  
 - dev-last_scancode = scancode;
 - dev-last_toggle = toggle;
 - dev-last_keycode = keycode;
 + input_event(dev-input_dev, EV_MSC, MSC_SCAN, scancode);
  
 - if (keycode == KEY_RESERVED)
 - return;
 + if (new_event  keycode != KEY_RESERVED) {
 + /* Register a keypress */
 + dev-keypressed = true;
 + dev-last_scancode = scancode;
 + dev-last_toggle = toggle;
 + dev-last_keycode

Re: IR remote control autorepeat / evdev

[Dmitry Torokhov]

On Wed, May 11, 2011 at 08:59:16PM +0300, Anssi Hannula wrote:
 
 I meant replacing the softrepeat with native repeat for such devices
 that have native repeats but no native release events:
 
 - keypress from device = keydown + keyup
 - repeat from device = keydown + keyup
 - repeat from device = keydown + keyup
 
 This is what e.g. ati_remote driver now does.
 
 Or alternatively
 
 - keypress from device = keydown
 - repeat from device = repeat
 - repeat from device = repeat
 - nothing for 250ms = keyup
 (doing it this way requires some extra handling in X server to stop its
 softrepeat from triggering, though, as previously noted)
 
 With either of these, if one holds down volumeup, the repeat works, and
 stops volumeup'ing immediately when user releases the button (as it is
 supposed to).
 

Unfortunately this does not work for devices that do not have hardware
autorepeat and also stops users from adjusting autorepeat parameters to
their liking.

It appears that the delay to check whether the key has been released is
too long (almost order of magnitude longer than our typical autorepeat
period). I think we should increase the period for remotes (both in
kernel and in X, and also see if the release check delay can be made
shorter, like 50-100 ms.

Thanks.

-- 
Dmitry
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH] input: add KEY_IMAGES specifically for AL Image Browser

[Dmitry Torokhov]

On Tuesday, April 12, 2011 02:00:53 PM Jarod Wilson wrote:
 Andy Walls wrote:
  Jarod Wilsonja...@redhat.com  wrote:
  Many media center remotes have buttons intended for jumping straight
  to one type of media browser or another -- commonly,
  images/photos/pictures,
  audio/music, television, and movies. At present, remotes with an
  images or photos or pictures button use any number of different
  keycodes which sort of maybe fit. I've seen at least KEY_MEDIA,
  KEY_CAMERA, KEY_GRAPHICSEDITOR and KEY_PRESENTATION. None of those
  seem quite right.
  In my mind, KEY_MEDIA should be something more like a media center
  application launcher (and I'd like to standardize on that for things
  like the windows media center button on the mce remotes). KEY_CAMERA
  is used in a lot of webcams, and typically means take a picture
  now. KEY_GRAPHICSEDITOR implies an editor, not a browser.
  KEY_PRESENTATION might be the closest fit here, if you think photo
  slide show, but it may well be more intended for run application
  in full-screen presentation mode or to launch something like
  magicpoint, I dunno. And thus, I'd like to have a KEY_IMAGES, which
  matches the HID Usage AL Image Browser, the meaning of which I think
  is crystal-clear. I believe AL Audio Browser is already covered by
  KEY_AUDIO, and AL Movie Browser by KEY_VIDEO, so I'm also adding
  appropriate comments next to those keys.
  
  I have follow-on patches for drivers/hid/hid-input.c and for
  drivers/media/rc/* that make use of this new key, if its deemed
  appropriate for addition. To make it simpler to merge the additional
  patches, it would be nice if this could sneak into 2.6.39, and the
  rest can then get queued up for 2.6.40, avoiding any multi-tree
  integration headaches.
  
  CC: Dmitry Torokhovdmitry.torok...@gmail.com
  CC: Jiri Kosinajkos...@suse.cz
  CC: Linux Medialinux-media@vger.kernel.org
  Signed-off-by: Jarod Wilsonja...@redhat.com
  ---
  include/linux/input.h |5 +++--
  1 files changed, 3 insertions(+), 2 deletions(-)
  
  diff --git a/include/linux/input.h b/include/linux/input.h
  index e428382..be082e9 100644
  --- a/include/linux/input.h
  +++ b/include/linux/input.h
  @@ -553,8 +553,8 @@ struct input_keymap_entry {
  #define KEY_DVD0x185   /* Media Select DVD */
  #define KEY_AUX0x186
  #define KEY_MP30x187
  -#define KEY_AUDIO 0x188
  -#define KEY_VIDEO 0x189
  +#define KEY_AUDIO 0x188   /* AL Audio Browser */
  +#define KEY_VIDEO 0x189   /* AL Movie Browser */
  #define KEY_DIRECTORY  0x18a
  #define KEY_LIST   0x18b
  #define KEY_MEMO   0x18c   /* Media Select Messages */
  @@ -605,6 +605,7 @@ struct input_keymap_entry {
  #define KEY_MEDIA_REPEAT   0x1b7   /* Consumer - transport control */
  #define KEY_10CHANNELSUP0x1b8   /* 10 channels up (10+) */
  #define KEY_10CHANNELSDOWN  0x1b9   /* 10 channels down (10-) */
  +#define KEY_IMAGES0x1ba   /* AL Image Browser */
  
  #define KEY_DEL_EOL0x1c0
  #define KEY_DEL_EOS0x1c1
  --
  1.7.1
  
  --
  To unsubscribe from this list: send the line unsubscribe
  linux-media in
  the body of a message to majord...@vger.kernel.org
  More majordomo info at  http://vger.kernel.org/majordomo-info.html
  
  Maybe Launch instead of AL in the comments.  It took me a while to
  figure out AL even with the email context.
 
 Ah, yes, that's an HID Usage Table-ism, short for Application Launch.
 The comment text is taken straight from the HUT's Usage Name column, on
 page 80 of HUT v1.12, and matches a fair number of other comments in
 input.h. It wasn't until I started reading the HUT doc that many of the
 comments next to keys actually started making sense... :)
 
 So I'm fine with either AL or Launch, but if we opt for the latter,
 it might be good to change the rest of the comments to match.

Somewhere above in input.h we have:

/*
 * Keys and buttons
 *
 * Most of the keys/buttons are modeled after USB HUT 1.12
 * (see http://www.usb.org/developers/hidpage).
 * Abbreviations in the comments:
 * AC - Application Control
 * AL - Application Launch Button
 * SC - System Control
 */

Thanks.

-- 
Dmitry
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Bug 30022] Kernel cannot find Flyvideo 98's remote control device.

[Dmitry Torokhov]

On Thursday, March 10, 2011 12:52:00 am bugzilla-dae...@bugzilla.kernel.org 
wrote:
 https://bugzilla.kernel.org/show_bug.cgi?id=30022
 
 
 
 
 
 --- Comment #7 from Mehmet Tekbaş btek...@gmail.com  2011-03-10
 08:51:59 --- Tried with Inca I-TV004 (bt878 analog card) and result
 didn't changed. IR device doesnt place under /dev/input/ . Can it be
 related to inputdev replacement of lirc_gpio?

Let's ask folks on linux-media mailing list...

-- 
Dmitry
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: cx23885-input.c does in fact use a workqueue....

[Dmitry Torokhov]

On Sun, Feb 13, 2011 at 08:35:22PM -0500, Andy Walls wrote:
 Tejun,
 
 I just noticed this commit:
 
 commit 8c71778cbf2c8beaefaa2dee5478aa0622d96682
 Author: Tejun Heo t...@kernel.org
 Date:   Fri Dec 24 16:14:20 2010 +0100
 
 media/video: don't use flush_scheduled_work()
 
 This patch converts the remaining users of flush_scheduled_work() in
 media/video.
 
 * bttv-input.c and cx23885-input.c don't use workqueue at all.  No
   need to flush.
 [...]
 
 
 The cx23885 driver does in fact schedule work for IR input handling:
 
 Here's where it is scheduled for CX23888 chips:
 
 http://git.linuxtv.org/media_tree.git?a=blob;f=drivers/media/video/cx23885/cx23885-ir.c;h=7125247dd25558678c823ee3262675570c9aa630;hb=HEAD#l76
 
 Here's where it is scheduled for CX23885 chips:
 
 http://git.linuxtv.org/media_tree.git?a=blob;f=drivers/media/video/cx23885/cx23885-core.c;h=359882419b7f588b7c698dbcfb6a39ddb1603301;hb=HEAD#l1861
 
 
 The two different chips are handled slightly differently because
 
 a. the CX23888 IR unit is accessable via a PCI register block.  The IR
 IRQ can be acknowledged with direct PCI register accesses in an
 interrupt context, and the IR pulse FIFO serviced later in a workqueue
 context.
 
 b. the CX23885 IR unit is accessed over an I2C bus.  The CX23885 A/V IRQ
 has to be masked in an interrupt context (with PCI registers accesses).
 Then the CX23885 A/V unit's IR IRQ is ack'ed over I2C in a workqueue
 context and the IR pulse FIFO is also serviced over I2C in a workqueue
 context.
 
 
 So what should be done about the flush_scheduled_work()?  I think it
 belongs there.
 

Convert to using threaded irq?

-- 
Dmitry
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] dvb-usb-remote - convert to new style of get/setkeycode

[Dmitry Torokhov]

Input: dvb-usb-remote - convert to new style of get/setkeycode

Signed-off-by: Dmitry Torokhov d...@mail.ru
---

Mauro,

This is needed so that I could rename get/setkeycode_new into
get/setkeycode and get rid of duplicate pointers and compat code in
input core.

Compiled only, not tested.

If you are OK with the patch then I'd like to merge this through my
tree.

Thanks!

 drivers/media/dvb/dvb-usb/dvb-usb-remote.c |  113 +---
 1 files changed, 70 insertions(+), 43 deletions(-)


diff --git a/drivers/media/dvb/dvb-usb/dvb-usb-remote.c 
b/drivers/media/dvb/dvb-usb/dvb-usb-remote.c
index 23005b3..347fbd4 100644
--- a/drivers/media/dvb/dvb-usb/dvb-usb-remote.c
+++ b/drivers/media/dvb/dvb-usb/dvb-usb-remote.c
@@ -8,60 +8,71 @@
 #include dvb-usb-common.h
 #include linux/usb/input.h
 
+static unsigned int
+legacy_dvb_usb_get_keymap_index(const struct input_keymap_entry *ke,
+   struct rc_map_table *keymap,
+   unsigned int keymap_size)
+{
+   unsigned int index;
+   unsigned int scancode;
+
+   if (ke-flags  INPUT_KEYMAP_BY_INDEX) {
+   index = ke-index;
+   } else {
+   if (input_scancode_to_scalar(ke, scancode))
+   return keymap_size;
+
+   /* See if we can match the raw key code. */
+   for (index = 0; index  keymap_size; index++)
+   if (keymap[index].scancode == scancode)
+   break;
+
+   /* See if there is an unused hole in the map */
+   if (index = keymap_size) {
+   for (index = 0; index  keymap_size; index++) {
+   if (keymap[index].keycode == KEY_RESERVED ||
+   keymap[index].keycode == KEY_UNKNOWN) {
+   break;
+   }
+   }
+   }
+   }
+
+   return index;
+}
+
 static int legacy_dvb_usb_getkeycode(struct input_dev *dev,
-   unsigned int scancode, unsigned int *keycode)
+struct input_keymap_entry *ke)
 {
struct dvb_usb_device *d = input_get_drvdata(dev);
-
struct rc_map_table *keymap = d-props.rc.legacy.rc_map_table;
-   int i;
+   unsigned int keymap_size = d-props.rc.legacy.rc_map_size;
+   unsigned int index;
 
-   /* See if we can match the raw key code. */
-   for (i = 0; i  d-props.rc.legacy.rc_map_size; i++)
-   if (keymap[i].scancode == scancode) {
-   *keycode = keymap[i].keycode;
-   return 0;
-   }
+   index = legacy_dvb_usb_get_keymap_index(ke, keymap, keymap_size);
+   if (index = keymap_size)
+   return -EINVAL;
 
-   /*
-* If is there extra space, returns KEY_RESERVED,
-* otherwise, input core won't let legacy_dvb_usb_setkeycode
-* to work
-*/
-   for (i = 0; i  d-props.rc.legacy.rc_map_size; i++)
-   if (keymap[i].keycode == KEY_RESERVED ||
-   keymap[i].keycode == KEY_UNKNOWN) {
-   *keycode = KEY_RESERVED;
-   return 0;
-   }
+   ke-keycode = keymap[index].keycode;
+   if (ke-keycode == KEY_UNKNOWN)
+   ke-keycode = KEY_RESERVED;
+   ke-len = sizeof(keymap[index].scancode);
+   memcpy(ke-scancode, keymap[index].scancode, ke-len);
+   ke-index = index;
 
-   return -EINVAL;
+   return 0;
 }
 
 static int legacy_dvb_usb_setkeycode(struct input_dev *dev,
-   unsigned int scancode, unsigned int keycode)
+const struct input_keymap_entry *ke,
+unsigned int *old_keycode)
 {
struct dvb_usb_device *d = input_get_drvdata(dev);
-
struct rc_map_table *keymap = d-props.rc.legacy.rc_map_table;
-   int i;
-
-   /* Search if it is replacing an existing keycode */
-   for (i = 0; i  d-props.rc.legacy.rc_map_size; i++)
-   if (keymap[i].scancode == scancode) {
-   keymap[i].keycode = keycode;
-   return 0;
-   }
-
-   /* Search if is there a clean entry. If so, use it */
-   for (i = 0; i  d-props.rc.legacy.rc_map_size; i++)
-   if (keymap[i].keycode == KEY_RESERVED ||
-   keymap[i].keycode == KEY_UNKNOWN) {
-   keymap[i].scancode = scancode;
-   keymap[i].keycode = keycode;
-   return 0;
-   }
+   unsigned int keymap_size = d-props.rc.legacy.rc_map_size;
+   unsigned int index;
 
+   index = legacy_dvb_usb_get_keymap_index(ke, keymap, keymap_size);
/*
 * FIXME: Currently, it is not possible to increase the size

[PATCH] Input: switch completely over to the new versions of get/setkeycode

[Dmitry Torokhov]

Input: switch completely over to the new versions of get/setkeycode

All users of old style get/setkeycode methids have been converted so
it is time to retire them.

Signed-off-by: Dmitry Torokhov d...@mail.ru
---

Jiri, Mauro,

There is not a good way to avoid crossing multiple subsystems but the
changes are minimal, so if you are OK with the patch I'd like to move it
through my tree for .39.

Thanks!

 drivers/hid/hid-input.c|4 +-
 drivers/input/input.c  |   55 
 drivers/input/misc/ati_remote2.c   |4 +-
 drivers/input/sparse-keymap.c  |4 +-
 drivers/media/dvb/dvb-usb/dvb-usb-remote.c |4 +-
 drivers/media/rc/rc-main.c |4 +-
 include/linux/input.h  |   12 ++
 7 files changed, 20 insertions(+), 67 deletions(-)


diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index 7f552bf..ba2aeea 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -888,8 +888,8 @@ int hidinput_connect(struct hid_device *hid, unsigned int 
force)
hid-ll_driver-hidinput_input_event;
input_dev-open = hidinput_open;
input_dev-close = hidinput_close;
-   input_dev-setkeycode_new = hidinput_setkeycode;
-   input_dev-getkeycode_new = hidinput_getkeycode;
+   input_dev-setkeycode = hidinput_setkeycode;
+   input_dev-getkeycode = hidinput_getkeycode;
 
input_dev-name = hid-name;
input_dev-phys = hid-phys;
diff --git a/drivers/input/input.c b/drivers/input/input.c
index 11905b6..d6e8bd8 100644
--- a/drivers/input/input.c
+++ b/drivers/input/input.c
@@ -791,22 +791,9 @@ int input_get_keycode(struct input_dev *dev, struct 
input_keymap_entry *ke)
int retval;
 
spin_lock_irqsave(dev-event_lock, flags);
-
-   if (dev-getkeycode) {
-   /*
-* Support for legacy drivers, that don't implement the new
-* ioctls
-*/
-   u32 scancode = ke-index;
-
-   memcpy(ke-scancode, scancode, sizeof(scancode));
-   ke-len = sizeof(scancode);
-   retval = dev-getkeycode(dev, scancode, ke-keycode);
-   } else {
-   retval = dev-getkeycode_new(dev, ke);
-   }
-
+   retval = dev-getkeycode(dev, ke);
spin_unlock_irqrestore(dev-event_lock, flags);
+
return retval;
 }
 EXPORT_SYMBOL(input_get_keycode);
@@ -831,35 +818,7 @@ int input_set_keycode(struct input_dev *dev,
 
spin_lock_irqsave(dev-event_lock, flags);
 
-   if (dev-setkeycode) {
-   /*
-* Support for legacy drivers, that don't implement the new
-* ioctls
-*/
-   unsigned int scancode;
-
-   retval = input_scancode_to_scalar(ke, scancode);
-   if (retval)
-   goto out;
-
-   /*
-* We need to know the old scancode, in order to generate a
-* keyup effect, if the set operation happens successfully
-*/
-   if (!dev-getkeycode) {
-   retval = -EINVAL;
-   goto out;
-   }
-
-   retval = dev-getkeycode(dev, scancode, old_keycode);
-   if (retval)
-   goto out;
-
-   retval = dev-setkeycode(dev, scancode, ke-keycode);
-   } else {
-   retval = dev-setkeycode_new(dev, ke, old_keycode);
-   }
-
+   retval = dev-setkeycode(dev, ke, old_keycode);
if (retval)
goto out;
 
@@ -1846,11 +1805,11 @@ int input_register_device(struct input_dev *dev)
dev-rep[REP_PERIOD] = 33;
}
 
-   if (!dev-getkeycode  !dev-getkeycode_new)
-   dev-getkeycode_new = input_default_getkeycode;
+   if (!dev-getkeycode)
+   dev-getkeycode = input_default_getkeycode;
 
-   if (!dev-setkeycode  !dev-setkeycode_new)
-   dev-setkeycode_new = input_default_setkeycode;
+   if (!dev-setkeycode)
+   dev-setkeycode = input_default_setkeycode;
 
dev_set_name(dev-dev, input%ld,
 (unsigned long) atomic_inc_return(input_no) - 1);
diff --git a/drivers/input/misc/ati_remote2.c b/drivers/input/misc/ati_remote2.c
index 0b0e9be..9ccdb82 100644
--- a/drivers/input/misc/ati_remote2.c
+++ b/drivers/input/misc/ati_remote2.c
@@ -612,8 +612,8 @@ static int ati_remote2_input_init(struct ati_remote2 *ar2)
idev-open = ati_remote2_open;
idev-close = ati_remote2_close;
 
-   idev-getkeycode_new = ati_remote2_getkeycode;
-   idev-setkeycode_new = ati_remote2_setkeycode;
+   idev

Re: [PATCH] Input: switch completely over to the new versions of get/setkeycode

[Dmitry Torokhov]

On Mon, Jan 31, 2011 at 01:32:43PM +0100, Jiri Kosina wrote:
 On Mon, 31 Jan 2011, Mauro Carvalho Chehab wrote:
 
   Input: switch completely over to the new versions of get/setkeycode
   
   All users of old style get/setkeycode methids have been converted so
   it is time to retire them.
   
   Signed-off-by: Dmitry Torokhov d...@mail.ru
   ---
   
   Jiri, Mauro,
   
   There is not a good way to avoid crossing multiple subsystems but the
   changes are minimal, so if you are OK with the patch I'd like to move it
   through my tree for .39.
  
  Acked-by: Mauro Carvalho Chehab mche...@redhat.com
 
 Acked-by: Jiri Kosina jkos...@suse.cz
 

Thanks guys.

-- 
Dmitry
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ?

[Dmitry Torokhov]

On Thu, Jan 27, 2011 at 04:58:57PM -0200, Mauro Carvalho Chehab wrote:
 Em 27-01-2011 15:21, Dmitry Torokhov escreveu:
  On Thu, Jan 27, 2011 at 08:30:00AM -0200, Mauro Carvalho Chehab wrote:
 
  On my tests here, this is working fine, with Fedora and RHEL 6, on my
  usual test devices, so I don't believe that the tool itself is broken, 
  nor I think that the issue is due to the fix patch.
 
  I remember that when Kay added a persistence utility tool that opens a V4L
  device in order to read some capabilities, this caused a race condition
  into a number of drivers that use to register the video device too early.
  The result is that udev were opening the device before the end of the
  register process, causing OOPS and other problems.
  
  Well, this is quite possible. The usev ruls in the v4l-utils reads:
  
  ACTION==add, SUBSYSTEM==rc, RUN+=/usr/bin/ir-keytable -a 
  /etc/rc_maps.cfg -s $name
  
  So we act when we add RC device to the system. The corresponding input
  device has not been registered yet (and will not be for some time
  because before creating input ddevice we invoke request_module() to load
  initial rc map module) so the tool runs simultaneously with kernel
  registering input device and it could very well be it can't find
  something it really wants.
  
  This would explain why Mark sees the segfault only when invoked via
  udev but not when ran manually.
  
  However I still do not understand why Mark does not see the same issue
  without the patch. Like I said, maybe if Mark could recompile with
  debug data and us a core we'd see what is going on.
 
 Race conditions are hard to track... probably the new code added some delay,
 and this allowed the request_module() to finish his job.
 
  BTW, that means that we need to redo udev rules. 
 
 If there's a race condition, then the proper fix is to lock the driver
 until it is ready to receive a fops. Maybe we'll need a mutex to preventing
 opening the device until it is completely initialized.

No, not at all. The devices are ready to handle everything when they are
created, it's just some devices are not there yet. What you do with
current udev rule is similar to trying to mount filesystem as soon as
you discover a PCI SCSI card. The controller is there but disks have not
been discovered, block devices have not been created, and so on.

 
 It is hard to tell, as Mark didn't provide us yet the dmesg info (at least
 on the emails I was c/c), so I don't even know what device he has, and what
 drivers are used.

I belie you have been copied on the mail that had the following snippet:

 kernel: Registered IR keymap rc-rc5-tv
 udevd-event[6438]: run_program: '/usr/bin/ir-keytable' abnormal exit
 kernel: input: i2c IR (Hauppauge) as /devices/virtual/rc/rc0/input7
 kernel: ir-keytable[6439]: segfault at 8 ip 004012d2 sp 
 7fff6d43ca60 error 4 in ir-keytable[40+7000]
 kernel: rc0: i2c IR (Hauppauge) as /devices/virtual/rc/rc0
 kernel: ir-kbd-i2c: i2c IR (Hauppauge) detected at i2c-0/0-0018/ir0 [ivtv i2c 
 driver #0]

 
  Maybe we should split
  the utility into 2 parts - one dealing with rcX device and for keymap
  setting reuse udev's existing utility that adjusts maps on ann input
  devices, not for RCs only.
 
 It could be done, but then we'll need to pollute the existing input tools
 with RC-specific stuff. For IR, there are some additional steps, like
 the need to select the IR protocol, otherwise the keytable is useless.

That should be done by the separate utility that fires up when udev gets
event for /sys/class/rc/rcX device.

 Also, the keytable and persistent info is provided via 
 /sys/class/rc/rc?/uevent.
 So, the tool need to first read the RC class, check what keytable should be
 associated with that device (based on a custom file), and load the proper
 table.

And this could be easily added to the udev's keymap utility that is
fired up when we discover evdevX devices.

 
 Also, I'm currently working on a way to map media keys for remote controllers 
 into X11 (basically, mapping them into the keyspace between 8-255, passing 
 through Xorg evdev.c, and then mapping back into some X11 symbols). This way,
 we don't need to violate the X11 protocol. (Yeah, I know this is hacky, but
 while X11 cannot pass the full evdev keycode, at least the Remote Controllers
 will work). This probably means that we may need to add some DBus logic
 inside ir-keytable, when called via udev, to allow it to announce to X11.

The same issue is present with other types of input devices (multimedia
keyboards emitting codes that X can't consume) and so it again would
make sense to enhance udev's utility instead of confining it all to
ir-keytable.

Thanks.

-- 
Dmitry
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ?

[Dmitry Torokhov]

On Fri, Jan 28, 2011 at 09:55:58AM -0200, Mauro Carvalho Chehab wrote:
 Em 28-01-2011 07:39, Dmitry Torokhov escreveu:
  On Thu, Jan 27, 2011 at 04:58:57PM -0200, Mauro Carvalho Chehab wrote:
  Em 27-01-2011 15:21, Dmitry Torokhov escreveu:
  On Thu, Jan 27, 2011 at 08:30:00AM -0200, Mauro Carvalho Chehab wrote:
 
  On my tests here, this is working fine, with Fedora and RHEL 6, on my
  usual test devices, so I don't believe that the tool itself is broken, 
  nor I think that the issue is due to the fix patch.
 
  I remember that when Kay added a persistence utility tool that opens a 
  V4L
  device in order to read some capabilities, this caused a race condition
  into a number of drivers that use to register the video device too early.
  The result is that udev were opening the device before the end of the
  register process, causing OOPS and other problems.
 
  Well, this is quite possible. The usev ruls in the v4l-utils reads:
 
  ACTION==add, SUBSYSTEM==rc, RUN+=/usr/bin/ir-keytable -a 
  /etc/rc_maps.cfg -s $name
 
  So we act when we add RC device to the system. The corresponding input
  device has not been registered yet (and will not be for some time
  because before creating input ddevice we invoke request_module() to load
  initial rc map module) so the tool runs simultaneously with kernel
  registering input device and it could very well be it can't find
  something it really wants.
 
  This would explain why Mark sees the segfault only when invoked via
  udev but not when ran manually.
 
  However I still do not understand why Mark does not see the same issue
  without the patch. Like I said, maybe if Mark could recompile with
  debug data and us a core we'd see what is going on.
 
  Race conditions are hard to track... probably the new code added some 
  delay,
  and this allowed the request_module() to finish his job.
 
  BTW, that means that we need to redo udev rules. 
 
  If there's a race condition, then the proper fix is to lock the driver
  until it is ready to receive a fops. Maybe we'll need a mutex to preventing
  opening the device until it is completely initialized.
  
  No, not at all. The devices are ready to handle everything when they are
  created, it's just some devices are not there yet. What you do with
  current udev rule is similar to trying to mount filesystem as soon as
  you discover a PCI SCSI card. The controller is there but disks have not
  been discovered, block devices have not been created, and so on.
 
 The rc-core register (and the corresponding input register) is done when
 the device detected a remote controller, so, it should be safe to register
 on that point. If not, IMHO, there's a bug somewhere. 

It is not a matter of safe or unsafe registration. Registration is fine.
The problem is that with the current set up is that utility is fired
when trunk of [sub]tree is created, but the utility wants to operate on
leaves which may not be there yet.

 
 Yet, I agree that udev tries to set devices too fast.

It tries to set devices exacty when you tell it to do so. It's not like
it goes trolling for random devices is sysfs.

 It would be better if
 it would wait for a few milisseconds, to reduce the risk of race conditions.

Gah, I really prefer using properly engineered solutions instead of
adding crutches.

 
  It is hard to tell, as Mark didn't provide us yet the dmesg info (at least
  on the emails I was c/c), so I don't even know what device he has, and what
  drivers are used.
  
  I belie you have been copied on the mail that had the following snippet:
  
  kernel: Registered IR keymap rc-rc5-tv
  udevd-event[6438]: run_program: '/usr/bin/ir-keytable' abnormal exit
  kernel: input: i2c IR (Hauppauge) as /devices/virtual/rc/rc0/input7
  kernel: ir-keytable[6439]: segfault at 8 ip 004012d2 sp 
  7fff6d43ca60 error 4 in ir-keytable[40+7000]
  kernel: rc0: i2c IR (Hauppauge) as /devices/virtual/rc/rc0
  kernel: ir-kbd-i2c: i2c IR (Hauppauge) detected at i2c-0/0-0018/ir0 [ivtv 
  i2c driver #0]
 
 Ok, the last line says it is a ivtv board, using IR. However, it doesn't show
 the I2C detection of other devices that might be racing to gain access to the
 I2C bus, nor if some OOPS were hit by kernel.
 
 I don't have any ivtv boards handy, but there are some developers at
 linux-media ML that may help with this.
 
  Maybe we should split
  the utility into 2 parts - one dealing with rcX device and for keymap
  setting reuse udev's existing utility that adjusts maps on ann input
  devices, not for RCs only.
 
  It could be done, but then we'll need to pollute the existing input tools
  with RC-specific stuff. For IR, there are some additional steps, like
  the need to select the IR protocol, otherwise the keytable is useless.
  
  That should be done by the separate utility that fires up when udev gets
  event for /sys/class/rc/rcX device.
  
  Also, the keytable and persistent info is provided via 
  /sys/class/rc/rc?/uevent.
  So, the tool need

Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ?

[Dmitry Torokhov]

On Thu, Jan 27, 2011 at 11:53:25AM -0800, Dmitry Torokhov wrote:
 On Thu, Jan 27, 2011 at 01:12:48PM -0500, Mark Lord wrote:
  On 11-01-27 11:39 AM, Dmitry Torokhov wrote:
   On Wed, Jan 26, 2011 at 10:18:53PM -0500, Mark Lord wrote:
   No, it does not seem to segfault when I unload/reload ir-kbd-i2c
   and then invoke it by hand with the same parameters.
   Quite possibly the environment is different when udev invokes it,
   and my strace attempt with udev killed the system, so no info there.
  
   
   Hmm, what about compiling with debug and getting a core then?
  
  Sure.  debug is easy, -g, but you'll have to tell me how to get it
  do produce a core dump.
  
 
 See if adjusting /etc/security/limits.conf will enable it to dump core.
 Otherwise you'll have to stick 'ulimit -c unlimited' somewhere...
 

Mark,

Any luck with getting the core? I'd really like to resolve this issue.

Thanks.

-- 
Dmitry
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ?

[Dmitry Torokhov]

On Fri, Jan 28, 2011 at 03:01:58PM -0200, Mauro Carvalho Chehab wrote:
 Em 28-01-2011 14:40, Dmitry Torokhov escreveu:
  On Fri, Jan 28, 2011 at 09:55:58AM -0200, Mauro Carvalho Chehab wrote:
 
  The rc-core register (and the corresponding input register) is done when
  the device detected a remote controller, so, it should be safe to register
  on that point. If not, IMHO, there's a bug somewhere. 
  
  It is not a matter of safe or unsafe registration. Registration is fine.
  The problem is that with the current set up is that utility is fired
  when trunk of [sub]tree is created, but the utility wants to operate on
  leaves which may not be there yet.
 
 I'm not an udev expert. Is there a udev event that hits only after having
 the driver completely loaded?

Define completely loaded? For a PCI SCSI controller does fully loaded
mean all attached devices are discovered and registered with block layer?
For a wireless NIC does it mean that it assocuated with an AP? What if
you have more than one device that driver serves?

So teh answer is no and there should not be.


 Starting an udev rule while modprobe is
 still running is asking for race conditions.

Not if we write stuff properly.

 
 I'm not entirely convinced that this is the bug that Mark is hitting, as

I do not know yet.

 rc-core does all needed setups before registering the evdev device. We
 need the core and the dmesg to be sure about what's happening there.

I will say it again. Your udev rule triggers when you create rcX device.
eventX device may apeear 2 hours after that (I could have evdev as a
module and blacklisted and load it later manually).

You need to split it into 2 separate steps:

1. Triggers when rcX appears, accesses only rcX and it's parents and
   does rcX related stuff.

2. Triggers when eventX appears and loads keymap and what not. Because
   it is a child of rcX (in  specific case of remotes) it may examine
   rcX attributes as well.

 
  Yet, I agree that udev tries to set devices too fast.
  
  It tries to set devices exacty when you tell it to do so. It's not like
  it goes trolling for random devices is sysfs.
  
  It would be better if
  it would wait for a few milisseconds, to reduce the risk of race 
  conditions.
  
  Gah, I really prefer using properly engineered solutions instead of
  adding crutches.
 
 I agree.
 
  And this could be easily added to the udev's keymap utility that is
  fired up when we discover evdevX devices.
 
  Yes, it can, if you add the IR protocol selection on that tool. A remote 
  controller keycode table has both the protocol and the keycodes.
  This basically means to merge 99% of the logic inside ir-keytable into the
  evdev generic tool.
  
  Or just have an utility producing keymap name and feed it as input to
  the generic tools. The way most of utilities work...
 
 I don't like the idea of running a some logic at udev that would generate
 such keymap in runtime just before calling the generic tool. The other

Why? You'd just call something like:

  keymap $name `rc-keymap-name -d $name`

where 'keymap' is udev's utility and 'rc-keymap-name' is new utility
that incorporates map selection logic currently found in rc-keytable.

It looks like format of the keymaps is compatible between 'keymap' and
'ir-keytable' and metadata that is present in your keymaps will not
confuse 'keymap' utility.

 alternative (e. g.) to maintain the RC-protocol dependent keytables separate
 from the RC protocol used by each table will be a maintenance nightmare.

I do not propose splitting keytables, I propose splittign utilities.
ir-keytable is a kitchen sink now. It implements 'keymap', 'evtest' and
bucnch of other stuff and would be much cleaner if split apart.

 
  Also, I'm currently working on a way to map media keys for remote 
  controllers 
  into X11 (basically, mapping them into the keyspace between 8-255, 
  passing 
  through Xorg evdev.c, and then mapping back into some X11 symbols). This 
  way,
  we don't need to violate the X11 protocol. (Yeah, I know this is hacky, 
  but
  while X11 cannot pass the full evdev keycode, at least the Remote 
  Controllers
  will work). This probably means that we may need to add some DBus logic
  inside ir-keytable, when called via udev, to allow it to announce to X11.
 
  The same issue is present with other types of input devices (multimedia
  keyboards emitting codes that X can't consume) and so it again would
  make sense to enhance udev's utility instead of confining it all to
  ir-keytable.
 
  I agree with you, but I'm not sure if we can find a solution that will
  work for both RC and media keyboards, as X11 evdev just maps keyboards
  on the 8-255 range. I was thinking to add a detection there for RC, and
  use a separate map for them, as RC don't need most of the normal keyboard
  keys.
  
  Well, there will always be clashes - there is reason why evdev goes
  beyond 255 keycodes...
 
 Yeah. The most appropriate fix would be for X to just use

Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ?

[Dmitry Torokhov]

On Fri, Jan 28, 2011 at 04:15:51PM -0200, Mauro Carvalho Chehab wrote:
 Em 28-01-2011 15:33, Dmitry Torokhov escreveu:
  On Fri, Jan 28, 2011 at 03:01:58PM -0200, Mauro Carvalho Chehab wrote:
  Em 28-01-2011 14:40, Dmitry Torokhov escreveu:
  On Fri, Jan 28, 2011 at 09:55:58AM -0200, Mauro Carvalho Chehab wrote:
 
  The rc-core register (and the corresponding input register) is done when
  the device detected a remote controller, so, it should be safe to 
  register
  on that point. If not, IMHO, there's a bug somewhere. 
 
  It is not a matter of safe or unsafe registration. Registration is fine.
  The problem is that with the current set up is that utility is fired
  when trunk of [sub]tree is created, but the utility wants to operate on
  leaves which may not be there yet.
 
  I'm not an udev expert. Is there a udev event that hits only after having
  the driver completely loaded?
  
  Define completely loaded? For a PCI SCSI controller does fully loaded
  mean all attached devices are discovered and registered with block layer?
  For a wireless NIC does it mean that it assocuated with an AP? What if
  you have more than one device that driver serves?
  
  So teh answer is no and there should not be.
  
 
  Starting an udev rule while modprobe is
  still running is asking for race conditions.
  
  Not if we write stuff properly.
  
 
  I'm not entirely convinced that this is the bug that Mark is hitting, as
  
  I do not know yet.
  
  rc-core does all needed setups before registering the evdev device. We
  need the core and the dmesg to be sure about what's happening there.
  
  I will say it again. Your udev rule triggers when you create rcX device.
  eventX device may apeear 2 hours after that (I could have evdev as a
  module and blacklisted and load it later manually).
 
 Blacklisting it won't (or shouldn't work).
 
 From rc-main, the registering sequence is:
 
   dev_set_name(dev-dev, rc%ld, dev-devno);
   dev_set_drvdata(dev-dev, dev);
   rc = device_add(dev-dev);
   if (rc)
   return rc;
 
   rc = ir_setkeytable(dev, rc_map);
   if (rc)
   goto out_dev;
 
   dev-input_dev-dev.parent = dev-dev;
   memcpy(dev-input_dev-id, dev-input_id, sizeof(dev-input_id));
   dev-input_dev-phys = dev-input_phys;
   dev-input_dev-name = dev-input_name;
   rc = input_register_device(dev-input_dev);
   if (rc)
   goto out_table;
 
 rc-main will wait for input_register_device() to finish, so even if you
 blacklist it, rc-core will load it, in order to solve the symbol dependency.

No, input_register_device() and input core itself does not have symbol
dependency on evdev (which provides /dev/input/eventX), which is just an
input handler. A very important, but still an optional, handler.

 Btw, there's really a race issue there: device_add is happening before
 input_register_device(), so the udev rule will cause troubles.

That's what I have been saying in the last 3+ emails, yes.

 
  You need to split it into 2 separate steps:
  
  1. Triggers when rcX appears, accesses only rcX and it's parents and
 does rcX related stuff.
  
  2. Triggers when eventX appears and loads keymap and what not. Because
 it is a child of rcX (in  specific case of remotes) it may examine
 rcX attributes as well.
 
 The fix is probably simpler: we need to change the udev rules to work at
 evdev registration and only if the device is a remote controller. This
 should solve the current issue.

The problem I have with it is that it violates layering. You affect
different subsystems/layers, why do you insist on jamming them togetehr?
Don't do the kitchen sink style, pretty please.

 
  Yet, I agree that udev tries to set devices too fast.
 
  It tries to set devices exacty when you tell it to do so. It's not like
  it goes trolling for random devices is sysfs.
 
  It would be better if
  it would wait for a few milisseconds, to reduce the risk of race 
  conditions.
 
  Gah, I really prefer using properly engineered solutions instead of
  adding crutches.
 
  I agree.
 
  And this could be easily added to the udev's keymap utility that is
  fired up when we discover evdevX devices.
 
  Yes, it can, if you add the IR protocol selection on that tool. A remote 
  controller keycode table has both the protocol and the keycodes.
  This basically means to merge 99% of the logic inside ir-keytable into 
  the
  evdev generic tool.
 
  Or just have an utility producing keymap name and feed it as input to
  the generic tools. The way most of utilities work...
 
  I don't like the idea of running a some logic at udev that would generate
  such keymap in runtime just before calling the generic tool. The other
  
  Why? You'd just call something like:
  
keymap $name `rc-keymap-name -d $name`
  
  where 'keymap' is udev's utility and 'rc-keymap-name' is new utility
  that incorporates map selection logic currently found in rc-keytable.
  
  It looks

Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ?

[Dmitry Torokhov]

On Fri, Jan 28, 2011 at 04:03:07PM -0500, Mark Lord wrote:
 On 11-01-28 03:55 PM, Mark Lord wrote:
  On 11-01-28 11:42 AM, Dmitry Torokhov wrote:
  On Thu, Jan 27, 2011 at 11:53:25AM -0800, Dmitry Torokhov wrote:
  On Thu, Jan 27, 2011 at 01:12:48PM -0500, Mark Lord wrote:
  On 11-01-27 11:39 AM, Dmitry Torokhov wrote:
  ..
  Hmm, what about compiling with debug and getting a core then?
 
  Sure.  debug is easy, -g, but you'll have to tell me how to get it
  do produce a core dump.
 
 
  See if adjusting /etc/security/limits.conf will enable it to dump core.
  Otherwise you'll have to stick 'ulimit -c unlimited' somewhere...
  ..
  Any luck with getting the core? I'd really like to resolve this issue.
  ..
  
  I'm upgrading the box to new userspace now.
  But I still have the old installation drive,
  so perhaps I'll go there now and try this.
  
  My plan is to replace /usr/bin/ir-keytable with a script
  that issues the 'ulimit -c unlimited' command and then
  invokes the original /usr/bin/ir-keytable binary.
  
  Should take half an hour or so before I get back here again.
 
 No-go.  According to the syslog, the segfault has not happened
 since I reconfigured the kernel and startup sequence two days
 ago to resolve an XFS mount issue.
 
 Something in there changed the init timing just enough to make
 it go away, I believe.

OK, this reinforces my suspicion that the cause of segfault is the race
we were discussing with Mauro, not the keymap retrieval fix.

I shall be sending the patch to Linus/stable in the next pull then.

Thank you for your help.

-- 
Dmitry
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] Input: rc-keymap - return KEY_RESERVED for unknown mappings

[Dmitry Torokhov]

Do not respond with -EINVAL to EVIOCGKEYCODE for not-yet-mapped scancodes,
but rather return KEY_RESERVED.

This fixes breakage with Ubuntu's input-kbd utility that stopped returning
full keymaps for remote controls.

Tested-by: Mauro Carvalho Chehab mche...@redhat.com
Tested-by: Mark Lord ker...@teksavvy.com
Signed-off-by: Dmitry Torokhov d...@mail.ru
---

Linus,

Due to the fact that contents of drivers/media in my 'for-linus' branch
are quite different from mainline/Mauro's trees and I am not planning on
merging this branch until closer to the next merge window I am sending
this regression fix in patch form instead of pull request.

Please consider applying.

Thanks,
Dmitry

 drivers/media/rc/rc-main.c |   28 +---
 1 files changed, 17 insertions(+), 11 deletions(-)

diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
index 72be8a0..512a2f4 100644
--- a/drivers/media/rc/rc-main.c
+++ b/drivers/media/rc/rc-main.c
@@ -458,21 +458,27 @@ static int ir_getkeycode(struct input_dev *idev,
index = ir_lookup_by_scancode(rc_map, scancode);
}
 
-   if (index = rc_map-len) {
-   if (!(ke-flags  INPUT_KEYMAP_BY_INDEX))
-   IR_dprintk(1, unknown key for scancode 0x%04x\n,
-  scancode);
+   if (index  rc_map-len) {
+   entry = rc_map-scan[index];
+
+   ke-index = index;
+   ke-keycode = entry-keycode;
+   ke-len = sizeof(entry-scancode);
+   memcpy(ke-scancode, entry-scancode, sizeof(entry-scancode));
+
+   } else if (!(ke-flags  INPUT_KEYMAP_BY_INDEX)) {
+   /*
+* We do not really know the valid range of scancodes
+* so let's respond with KEY_RESERVED to anything we
+* do not have mapping for [yet].
+*/
+   ke-index = index;
+   ke-keycode = KEY_RESERVED;
+   } else {
retval = -EINVAL;
goto out;
}
 
-   entry = rc_map-scan[index];
-
-   ke-index = index;
-   ke-keycode = entry-keycode;
-   ke-len = sizeof(entry-scancode);
-   memcpy(ke-scancode, entry-scancode, sizeof(entry-scancode));
-
retval = 0;
 
 out:
-- 
1.7.3.5

-- 
Dmitry
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ?

[Dmitry Torokhov]

On Wed, Jan 26, 2011 at 10:18:53PM -0500, Mark Lord wrote:
 On 11-01-26 09:12 PM, Dmitry Torokhov wrote:
  On Wed, Jan 26, 2011 at 08:07:29PM -0500, Mark Lord wrote:
  On 11-01-26 08:01 PM, Mark Lord wrote:
  On 11-01-26 10:05 AM, Mark Lord wrote:
  On 11-01-25 09:00 PM, Dmitry Torokhov wrote:
  ..
  I wonder if the patch below is all that is needed...
 
  Nope. Does not work here:
 
  $ lsinput
  protocol version mismatch (expected 65536, got 65537)
 
 
  Heh.. I just noticed something *new* in the bootlogs on my system:
 
  kernel: Registered IR keymap rc-rc5-tv
  udevd-event[6438]: run_program: '/usr/bin/ir-keytable' abnormal exit
  kernel: input: i2c IR (Hauppauge) as /devices/virtual/rc/rc0/input7
  kernel: ir-keytable[6439]: segfault at 8 ip 004012d2 sp 
  7fff6d43ca60
  error 4 in ir-keytable[40+7000]
  kernel: rc0: i2c IR (Hauppauge) as /devices/virtual/rc/rc0
  kernel: ir-kbd-i2c: i2c IR (Hauppauge) detected at i2c-0/0-0018/ir0 [ivtv 
  i2c
  driver #0]
 
  That's udev invoking ir-keyboard when the ir-kbd-i2c kernel module is 
  loaded,
  and that is also ir-keyboard (userspace) segfaulting when run.
 
  Note: I tried to capture an strace of ir-keyboard segfaulting during boot
  (as above), but doing so kills the system (hangs on boot).
 
  The command from udev was: /usr/bin/ir-keytable -a /etc/rc_maps.cfg -s rc0
  
  Does it die when you try to invoke the command by hand? Can you see where?
 
 
 No, it does not seem to segfault when I unload/reload ir-kbd-i2c
 and then invoke it by hand with the same parameters.
 Quite possibly the environment is different when udev invokes it,
 and my strace attempt with udev killed the system, so no info there.
 

Hmm, what about compiling with debug and getting a core then?

-- 
Dmitry
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ?

[Dmitry Torokhov]

On Thu, Jan 27, 2011 at 08:30:00AM -0200, Mauro Carvalho Chehab wrote:
 
 On my tests here, this is working fine, with Fedora and RHEL 6, on my
 usual test devices, so I don't believe that the tool itself is broken, 
 nor I think that the issue is due to the fix patch.
 
 I remember that when Kay added a persistence utility tool that opens a V4L
 device in order to read some capabilities, this caused a race condition
 into a number of drivers that use to register the video device too early.
 The result is that udev were opening the device before the end of the
 register process, causing OOPS and other problems.

Well, this is quite possible. The usev ruls in the v4l-utils reads:

ACTION==add, SUBSYSTEM==rc, RUN+=/usr/bin/ir-keytable -a /etc/rc_maps.cfg 
-s $name

So we act when we add RC device to the system. The corresponding input
device has not been registered yet (and will not be for some time
because before creating input ddevice we invoke request_module() to load
initial rc map module) so the tool runs simultaneously with kernel
registering input device and it could very well be it can't find
something it really wants.

This would explain why Mark sees the segfault only when invoked via
udev but not when ran manually.

However I still do not understand why Mark does not see the same issue
without the patch. Like I said, maybe if Mark could recompile with
debug data and us a core we'd see what is going on.

BTW, that means that we need to redo udev rules. Maybe we should split
the utility into 2 parts - one dealing with rcX device and for keymap
setting reuse udev's existing utility that adjusts maps on ann input
devices, not for RCs only.

-- 
Dmitry
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ?

[Dmitry Torokhov]

On Thu, Jan 27, 2011 at 01:12:48PM -0500, Mark Lord wrote:
 On 11-01-27 11:39 AM, Dmitry Torokhov wrote:
  On Wed, Jan 26, 2011 at 10:18:53PM -0500, Mark Lord wrote:
  No, it does not seem to segfault when I unload/reload ir-kbd-i2c
  and then invoke it by hand with the same parameters.
  Quite possibly the environment is different when udev invokes it,
  and my strace attempt with udev killed the system, so no info there.
 
  
  Hmm, what about compiling with debug and getting a core then?
 
 Sure.  debug is easy, -g, but you'll have to tell me how to get it
 do produce a core dump.
 

See if adjusting /etc/security/limits.conf will enable it to dump core.
Otherwise you'll have to stick 'ulimit -c unlimited' somewhere...

-- 
Dmitry
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ?

[Dmitry Torokhov]

On Wed, Jan 26, 2011 at 10:05:57AM -0500, Mark Lord wrote:
 On 11-01-25 09:00 PM, Dmitry Torokhov wrote:
  On Tue, Jan 25, 2011 at 03:29:14PM -0800, Dmitry Torokhov wrote:
  On Tue, Jan 25, 2011 at 05:22:09PM -0500, Mark Lord wrote:
  On 11-01-25 05:00 PM, Mauro Carvalho Chehab wrote:
  Em 25-01-2011 18:54, Dmitry Torokhov escreveu:
 ..
  That has been done as well; we have 2 new ioctls and kept 2 old ioctls.
 
  That's the problem: you did NOT keep the two old ioctls().
  Those got changed too.. so now we have four NEW ioctls(),
  none of which backward compatible with userspace.
 
 
  Please calm down. This, in fact, is not new vs old ioctl problem but
  rather particular driver (or rather set of drivers) implementation
  issue. Even if we drop the new ioctls and convert the RC code to use the
  old ones you'd be observing the same breakage as RC code responds with
  -EINVAL to not-yet-established mappings.
 
  I'll see what can be done for these drivers; I guess we could supply a
  fake KEY_RESERVED entry for not mapped scancodes if there are mapped
  scancodes above current one. That should result in the same behavior
  for RCs as before.
 
 
  I wonder if the patch below is all that is needed...
 
 
 Nope. Does not work here:
 
 $ lsinput
 protocol version mismatch (expected 65536, got 65537)
 

It would be much more helpful if you tried to test what has been fixed
(hint: version change wasn't it).

-- 
Dmitry
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ?

[Dmitry Torokhov]

On Wed, Jan 26, 2011 at 12:18:29PM -0200, Mauro Carvalho Chehab wrote:
 Em 26-01-2011 11:08, Gerd Hoffmann escreveu:
Hi,
  
  Btw, I took some time to take analyse the input-kbd stuff.
  As said at the README:
 
  This is a small collection of input layer utilities.  I wrote them
  mainly for testing and debugging, but maybe others find them useful
  too :-)
  ...
  Gerd Knorrkra...@bytesex.org  [SUSE Labs]
 
  This is an old testing tool written by Gerd Hoffmann probably used for him
  to test the V4L early Remote Controller implementations.
  
  Indeed.
  
  The last official version seems to be this one:
  http://dl.bytesex.org/cvs-snapshots/input-20081014-101501.tar.gz
  
  Just moved the bits to git a few days ago.
  http://bigendian.kraxel.org/cgit/input/
  
  Code is unchanged since 2008 though.
  
  Gerd, if you're still maintaining it, it is a good idea to apply Dmitry's
  patch:
  http://www.spinics.net/lists/linux-input/msg13728.html
  
  Hmm, doesn't apply cleanly ...
 
 I suspect that Dmitry did the patch against the Debian package, based on a 
 2007
 version of it, as it seems that Debian is using an older version of the 
 package.

Actually it was from Ubuntu, so it is based on 2008 checkout, but they
also have more patches, take a peek here:

https://launchpad.net/ubuntu/+archive/primary/+files/input-utils_0.0.20081014-1.debian.tar.gz

Thanks.

-- 
Dmitry
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ?

[Dmitry Torokhov]

On Wed, Jan 26, 2011 at 12:18:29PM -0200, Mauro Carvalho Chehab wrote:
 diff --git a/input.c b/input.c
 index d57a31e..a9bd5e8 100644
 --- a/input.c
 +++ b/input.c
 @@ -101,8 +101,8 @@ int device_open(int nr, int verbose)
   close(fd);
   return -1;
   }
 - if (EV_VERSION != version) {
 - fprintf(stderr, protocol version mismatch (expected %d, got 
 %d)\n,
 + if (EV_VERSION  version) {
 + fprintf(stderr, protocol version mismatch (expected = %d, got 
 %d)\n,
   EV_VERSION, version);

Please do not do this. It causes check to float depending on the
version of kernel headers it was compiled against.

The check should be against concrete version (0x1 in this case).

Thanks.

-- 
Dmitry
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ?

[Dmitry Torokhov]

On Wed, Jan 26, 2011 at 03:41:01PM -0200, Mauro Carvalho Chehab wrote:
 Em 26-01-2011 12:58, Mark Lord escreveu:
  On 11-01-26 06:26 AM, Mauro Carvalho Chehab wrote:
  ..
  However, as said previously in this thread, input-kbd won't work with any
  RC table that uses NEC extended (and there are several devices on the
  current Kernels with those tables), since it only reads up to 16 bits.
 
  ir-keytable works with all RC tables, if you use a kernel equal or upper to
  2.6.36, due to the usage of the new ioctl's.
  
  Is there a way to control the key repeat rate for a device
  controlled by ir-kbd-i2c ?
  
  It appears to be limited to a max of between 4 and 5 repeats/sec somewhere,
  and I'd like to fix that.
 
 It depends on what device do you have. Several I2C chips have the repeat
 logic inside the I2C microcontroller PROM firmware. or at the remote
 controller itself. So, there's nothing we can do to change it.
 
 I have even one device here (I think it is a saa7134-based Kworld device) 
 that doesn't send any repeat event at all for most keys (I think it only
 sends repeat events for volume - Can't remember the specific details anymore -
 too many devices!).
 
 The devices that produce repeat events can be adjusted via the normal
 input layer tools like kbdrate.
 

Unfortunately kbdrate affects all connected devices and I am not sure if
there is a utility allowing to set rate on individual devices. But here
is the main part: 

static int input_set_rate(int fd,
  unsigned int delay, unsigned int period)
{
unsigned int rep[2] = { delay, period };

if (ioctl(fd, EVIOCSREP, rep)  0) {
perror(evdev ioctl);
return -1;
}
return 0;
}

-- 
Dmitry
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ?

[Dmitry Torokhov]

On Wed, Jan 26, 2011 at 03:29:09PM -0200, Mauro Carvalho Chehab wrote:
 Em 26-01-2011 14:51, Dmitry Torokhov escreveu:
  On Wed, Jan 26, 2011 at 12:18:29PM -0200, Mauro Carvalho Chehab wrote:
  diff --git a/input.c b/input.c
  index d57a31e..a9bd5e8 100644
  --- a/input.c
  +++ b/input.c
  @@ -101,8 +101,8 @@ int device_open(int nr, int verbose)
 close(fd);
 return -1;
 }
  -  if (EV_VERSION != version) {
  -  fprintf(stderr, protocol version mismatch (expected %d, got 
  %d)\n,
  +  if (EV_VERSION  version) {
  +  fprintf(stderr, protocol version mismatch (expected = %d, got 
  %d)\n,
 EV_VERSION, version);
  
  Please do not do this. It causes check to float depending on the
  version of kernel headers it was compiled against.
  
  The check should be against concrete version (0x1 in this case).
 
 The idea here is to not prevent it to load if version is 0x10001.
 This is actually the only change that it is really needed (after applying
 your KEY_RESERVED patch to 2.6.37) for the tool to work. Reverting it causes
 the error:

You did not understand. When comparing against EV_VERSION, if you
compile on 2.6.32 you are comparing with 0x1. If you are compiling
on 2.6.37 you are comparing with 0x10001 as EV_VERSION value changes
(not the value returned by EVIOCGVERSION, the value of the _define_
itself).

The proper check is:

#define EVDEV_MIN_VERSION 0x1
if (version  EVDEV_MIN_VERSION) {
fprintf(stderr,
protocol version mismatch (need at least %d, got 
%d)\n,
EVDEV_MIN_VERSION, version);
...
}

-- 
Dmitry
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ?

[Dmitry Torokhov]

On Wed, Jan 26, 2011 at 08:16:09PM +0100, Gerd Hoffmann wrote:Hi,
 
 The check should be against concrete version (0x1 in this case).
 
 Stepping back: what does the version mean?


Nothing, it is just a number.
 
 0x1 == 1.0 ?
 0x10001 == 1.1 ?

No, not really.

 
 Can I expect the interface stay backward compatible if only the
 minor revision changes, i.e. makes it sense to accept 1.x?

I am not planning on breaking backward compatibility.

 
 Will the major revision ever change?  Does it make sense to check
 the version at all?

It depends. We do not have a clear way to see if new ioctls are
supported (and I do not consider try new ioctl and see if data sticks
being a good way) so that facilitated protocol version rev-up. So keymap
manipulating tools might be forced to check protocol version. For the
rest I think doing EVIOCGVERSION just to check that ioctl is supported
is an OK way to validate that we are dealing with an event device, but
that's it.

BTW, maybe we should move lsinput and input-kbd into linuxconsole
package, together with evtest, fftest, etc?

-- 
Dmitry
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ?

[Dmitry Torokhov]

On Wed, Jan 26, 2011 at 02:31:44PM -0500, Mark Lord wrote:
 On 11-01-26 11:44 AM, Dmitry Torokhov wrote:
  On Wed, Jan 26, 2011 at 10:05:57AM -0500, Mark Lord wrote:
 ..
  Nope. Does not work here:
 
  $ lsinput
  protocol version mismatch (expected 65536, got 65537)
 
  
  It would be much more helpful if you tried to test what has been fixed
  (hint: version change wasn't it).
 
 It would be much more helpful if you would revert that which was broken
 in 2.6.36.  (hint: version was part of it).
 

No, version change will not be reverted as we do not have a way to
validate whether new ioctls are supported. The older kernels are
returning -EINVAL for unknown evdev ioctls so userspace can't know
if ioctl failed because it is unsupported or because arguments are
wrong/not applicable for the underlying device.

 The other part does indeed appear to work with the old binary for input-kbd,
 but the binary for lsinput still fails as above.
 

Great, then I'' include the fix for RC keytables in my next pull
request. I guess it should go to stable as well.

Thanks.

-- 
Dmitry
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ?

[Dmitry Torokhov]

On Wed, Jan 26, 2011 at 02:33:17PM -0500, Mark Lord wrote:
 On 11-01-26 12:32 PM, Mauro Carvalho Chehab wrote:
  Em 26-01-2011 13:05, Mark Lord escreveu:
 ..
  Nope. Does not work here:
 
  $ lsinput
  protocol version mismatch (expected 65536, got 65537)
  
  You need to relax the version test at the tree. As I said before, this is
  a development tool from the early RC days, bound to work with one specific
  version of the API, and programmed by purpose to fail if there would by any
  updates at the Input layer.
 ..
 
 As I said before, I personally have done that on my copy here.
 But that's not what this thread is about.
 
 This thread is about broken userspace courtesy of these changes.
 So I am testing with the original userspace binary here,
 and it still fails.  And will continue to fail until that regression is fixed.

I do not consider lsinput refusing to work a regression. The tool
claims to work with particular protocol version and it is tool's choice.

Shall I write a utility that checks kernel version and only works with
2.6.37 and yell when we release 2.6.38?

-- 
Dmitry
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ?

[Dmitry Torokhov]

On Wed, Jan 26, 2011 at 02:47:18PM -0500, Mark Lord wrote:
 On 11-01-26 02:41 PM, Dmitry Torokhov wrote:
 
  I do not consider lsinput refusing to work a regression.
 
 Obviously, since you don't use that tool.
 Those of us who do use it see this as broken userspace compatibility.
 
 Who the hell reviews this crap, anyway?
 Code like that should never have made it upstream in the first place.
 

You are more than welcome spend more time on reviews.

Thanks.

-- 
Dmitry
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ?

[Dmitry Torokhov]

On Wed, Jan 26, 2011 at 04:41:07PM -0500, Mark Lord wrote:
 On 11-01-26 02:50 PM, Dmitry Torokhov wrote:
  On Wed, Jan 26, 2011 at 02:47:18PM -0500, Mark Lord wrote:
  On 11-01-26 02:41 PM, Dmitry Torokhov wrote:
 
  I do not consider lsinput refusing to work a regression.
 
  Obviously, since you don't use that tool.
  Those of us who do use it see this as broken userspace compatibility.
 
  Who the hell reviews this crap, anyway?
  Code like that should never have made it upstream in the first place.
 
  
  You are more than welcome spend more time on reviews.
 
 Somehow I detect a totally lack of sincerity there.

No, not really. If we known about Ubuntu's employ of such utility before
we'd try to come up with workaround or updated the utility proactively
so user-visible changes would be limited.

 
 But thanks for fixing the worst of this regression, at least.
 
 Perhaps you might think about eventually fixing the bad use of -EINVAL
 in future revisions.  One way perhaps to approach that, would be to begin
 fixing it internally,

Yes, that is on my lest (unless somebody beats me to it). Won't help
with the older kernels though, unfortunately.

 but still returning the same things from the actual
 f_ops-ioctl() routine.

Not sure if this is needed.

 
 Then eventually provide new ioctl numbers which return the correct -ENOTTY
 (or whatever is best there), rather than converting to -EVINAL at the 
 interface.
 Then a nice multi-year overlap, with a scheduled removal of the old codes 
 some day.
 
 Then the input subsystem would work more like most other subsystems,
 and make userspace programming simpler and easier to get correct.

I do not believe that such characterization is called for. We did fix
the breakage that was ABI breakage. The version issue is different. If
we go by what you say _none_ of the versions anywhere can be changed
ever because there might be a program that does not expect new version.

-- 
Dmitry
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ?

[Dmitry Torokhov]

On Wed, Jan 26, 2011 at 04:49:14PM -0500, Mark Lord wrote:
 Or perhaps get rid of that unworkable version number thing
 (just freeze it in time with the 2.6.35 value returned),
 and implement a get_feature_flags ioctl or something for going forward.
 Then you can just turn on new bits in the flags as new features are added.


That could be done but I do not expect retire features so far so version
is about the same. Plus, what guarantees that someone in the future
won't write a utility that compares exact capability bitmap and refuse
to work when new ones will be added?

-- 
Dmitry
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ?

[Dmitry Torokhov]

On Wed, Jan 26, 2011 at 08:07:29PM -0500, Mark Lord wrote:
 On 11-01-26 08:01 PM, Mark Lord wrote:
  On 11-01-26 10:05 AM, Mark Lord wrote:
  On 11-01-25 09:00 PM, Dmitry Torokhov wrote:
  ..
  I wonder if the patch below is all that is needed...
 
  Nope. Does not work here:
 
  $ lsinput
  protocol version mismatch (expected 65536, got 65537)
 
  
  Heh.. I just noticed something *new* in the bootlogs on my system:
  
  kernel: Registered IR keymap rc-rc5-tv
  udevd-event[6438]: run_program: '/usr/bin/ir-keytable' abnormal exit
  kernel: input: i2c IR (Hauppauge) as /devices/virtual/rc/rc0/input7
  kernel: ir-keytable[6439]: segfault at 8 ip 004012d2 sp 
  7fff6d43ca60
  error 4 in ir-keytable[40+7000]
  kernel: rc0: i2c IR (Hauppauge) as /devices/virtual/rc/rc0
  kernel: ir-kbd-i2c: i2c IR (Hauppauge) detected at i2c-0/0-0018/ir0 [ivtv 
  i2c
  driver #0]
  
  That's udev invoking ir-keyboard when the ir-kbd-i2c kernel module is 
  loaded,
  and that is also ir-keyboard (userspace) segfaulting when run.
 
 Note: I tried to capture an strace of ir-keyboard segfaulting during boot
 (as above), but doing so kills the system (hangs on boot).
 
 The command from udev was: /usr/bin/ir-keytable -a /etc/rc_maps.cfg -s rc0

Does it die when you try to invoke the command by hand? Can you see
where?

-- 
Dmitry
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ?

[Dmitry Torokhov]

On Wed, Jan 26, 2011 at 10:18:53PM -0500, Mark Lord wrote:
 On 11-01-26 09:12 PM, Dmitry Torokhov wrote:
  On Wed, Jan 26, 2011 at 08:07:29PM -0500, Mark Lord wrote:
  On 11-01-26 08:01 PM, Mark Lord wrote:
  On 11-01-26 10:05 AM, Mark Lord wrote:
  On 11-01-25 09:00 PM, Dmitry Torokhov wrote:
  ..
  I wonder if the patch below is all that is needed...
 
  Nope. Does not work here:
 
  $ lsinput
  protocol version mismatch (expected 65536, got 65537)
 
 
  Heh.. I just noticed something *new* in the bootlogs on my system:
 
  kernel: Registered IR keymap rc-rc5-tv
  udevd-event[6438]: run_program: '/usr/bin/ir-keytable' abnormal exit
  kernel: input: i2c IR (Hauppauge) as /devices/virtual/rc/rc0/input7
  kernel: ir-keytable[6439]: segfault at 8 ip 004012d2 sp 
  7fff6d43ca60
  error 4 in ir-keytable[40+7000]
  kernel: rc0: i2c IR (Hauppauge) as /devices/virtual/rc/rc0
  kernel: ir-kbd-i2c: i2c IR (Hauppauge) detected at i2c-0/0-0018/ir0 [ivtv 
  i2c
  driver #0]
 
  That's udev invoking ir-keyboard when the ir-kbd-i2c kernel module is 
  loaded,
  and that is also ir-keyboard (userspace) segfaulting when run.
 
  Note: I tried to capture an strace of ir-keyboard segfaulting during boot
  (as above), but doing so kills the system (hangs on boot).
 
  The command from udev was: /usr/bin/ir-keytable -a /etc/rc_maps.cfg -s rc0
  
  Does it die when you try to invoke the command by hand? Can you see where?
 
 
 No, it does not seem to segfault when I unload/reload ir-kbd-i2c
 and then invoke it by hand with the same parameters.
 Quite possibly the environment is different when udev invokes it,
 and my strace attempt with udev killed the system, so no info there.
 
 It does NOT segfault on the stock 2.6.37 kernel, without the patch.

I must admit I am baffled. The patch in question only affects the
EVIOCGKEYCODE path whereas '-a' means automatically load appropriate
keymap and as far as I can see it does not call EVIOCGKEYCODE, only
EVIOCSKEYCODE...

Mauro, any ideas?

BTW, I wonder what package ir-keytable is coming from? Ubuntu seems to
have v4l-utils at 0.8.1-2 and you say yours is 0.8.2...

Thanks.

-- 
Dmitry
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ?

[Dmitry Torokhov]

On Tue, Jan 25, 2011 at 09:42:44AM -0200, Mauro Carvalho Chehab wrote:
 Em 25-01-2011 03:31, Dmitry Torokhov escreveu:
  On Tue, Jan 25, 2011 at 12:07:29AM -0500, Mark Lord wrote:
  On 11-01-25 12:04 AM, Mark Lord wrote:
  On 11-01-24 11:55 PM, Dmitry Torokhov wrote:
  On Mon, Jan 24, 2011 at 11:37:06PM -0500, Mark Lord wrote:
  ..
  This results in (map-size==10) for 2.6.36+ (wrong),
  and a much larger map-size for 2.6.35 and earlier.
 
  So perhaps EVIOCGKEYCODE has changed?
 
 
  So the utility expects that all devices have flat scancode space and
  driver might have changed so it does not recognize scancode 10 as valid
  scancode anymore.
 
  The options are:
 
  1. Convert to EVIOCGKEYCODE2
  2. Ignore errors from EVIOCGKEYCODE and go through all 65536 iterations.
 
  or 3. Revert/fix the in-kernel regression.
 
  The EVIOCGKEYCODE ioctl is supposed to return KEY_RESERVED for unmapped
  (but value) keycodes, and only return -EINVAL when the keycode itself
  is out of range.
 
  That's how it worked in all kernels prior to 2.6.36,
  and now it is broken.  It now returns -EINVAL for any unmapped keycode,
  even though keycodes higher than that still have mappings.
 
  This is a bug, a regression, and breaks userspace.
  I haven't identified *where* in the kernel the breakage happened,
  though.. that code confuses me.  :)
 
  Note that this device DOES have flat scancode space,
  and the kernel is now incorrectly signalling an error (-EINVAL)
  in response to a perfectly valid query of a VALID (and mappable)
  keycode on the remote control
 
  The code really is a valid button, it just doesn't have a default mapping
  set by the kernel (I can set a mapping for that code from userspace and it 
  works).
 
  
  OK, in this case let's ping Mauro - I think he done the adjustments to
  IR keymap hanlding.
 
 I lost part of the thread, but a quick search via the Internet showed that 
 you're using
 the input tools to work with a Remote Controller, right? Are you using a 
 vanilla
 kernel, or are you using the media_build backports? There are some distros 
 that are
 using those backports also like Fedora 14.
 
 In the latter case, I found the reason why the backports were not working and 
 I fixed
 it a couple days ago:
   
 http://git.linuxtv.org/media_build.git?a=commit;h=b83dc3e49d90527d8e1016d09e06f4842a6a847a
 
 The issue is simple, and it is related on how the input.c used to handle 
 EVIOSGKEYCODE.
 Basically, before allowing you to change a key, it used to call EVIOCGKEYCODE 
 to check
 it that key exists. However, when you're creating a new association, the key 
 didn't
 exist, and, to be strict with input rules, EVIOCGKEYCODE should return 
 -EINVAL.

We should be able to handle the case where scancode is valid even though
it might be unmapped yet. This is regardless of what version of
EVIOCGKEYCODE we use, 1 or 2, and whether it is sparse keymap or not.

Is it possible to validate the scancode by driver?

-- 
Dmitry
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Extending rc-core/userspace to handle bigger scancodes - Was: Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ?

[Dmitry Torokhov]

On Tue, Jan 25, 2011 at 12:42:57PM -0200, Mauro Carvalho Chehab wrote:
 Em 25-01-2011 04:52, Dmitry Torokhov escreveu:
  On Mon, Jan 24, 2011 at 09:31:17PM -0800, Dmitry Torokhov wrote:
  On Tue, Jan 25, 2011 at 12:07:29AM -0500, Mark Lord wrote:
  On 11-01-25 12:04 AM, Mark Lord wrote:
  On 11-01-24 11:55 PM, Dmitry Torokhov wrote:
  On Mon, Jan 24, 2011 at 11:37:06PM -0500, Mark Lord wrote:
  ..
  This results in (map-size==10) for 2.6.36+ (wrong),
  and a much larger map-size for 2.6.35 and earlier.
 
  So perhaps EVIOCGKEYCODE has changed?
 
 
  So the utility expects that all devices have flat scancode space and
  driver might have changed so it does not recognize scancode 10 as valid
  scancode anymore.
 
  The options are:
 
  1. Convert to EVIOCGKEYCODE2
  2. Ignore errors from EVIOCGKEYCODE and go through all 65536 iterations.
 
  or 3. Revert/fix the in-kernel regression.
 
  The EVIOCGKEYCODE ioctl is supposed to return KEY_RESERVED for unmapped
  (but value) keycodes, and only return -EINVAL when the keycode itself
  is out of range.
 
  That's how it worked in all kernels prior to 2.6.36,
  and now it is broken.  It now returns -EINVAL for any unmapped keycode,
  even though keycodes higher than that still have mappings.
 
  This is a bug, a regression, and breaks userspace.
  I haven't identified *where* in the kernel the breakage happened,
  though.. that code confuses me.  :)
 
  Note that this device DOES have flat scancode space,
  and the kernel is now incorrectly signalling an error (-EINVAL)
  in response to a perfectly valid query of a VALID (and mappable)
  keycode on the remote control
 
  The code really is a valid button, it just doesn't have a default mapping
  set by the kernel (I can set a mapping for that code from userspace and 
  it works).
 
 
  OK, in this case let's ping Mauro - I think he done the adjustments to
  IR keymap hanlding.
 
  Thanks.
 
  
  BTW, could you please try the following patch (it assumes that
  EVIOCGVERSION in input.c is alreday relaxed).
 
 Dmitry,
 
 Thanks for your patch. I used part of his logic to improve the ir-keytable 
 tool at v4l-utils:
   http://git.linuxtv.org/v4l-utils.git
 
 The ir-keytable is a tool that just handles Remote Controller input devices,
 and do it well, allowing all sorts of operations related to it, and using the
 sysfs /sys/class/rc stuff to help its operation. Without any arguments, it
 lists the existing RC devices. Arguments are there to allow enabling/disabling
 RC protocols, reading/writing/cleaning keycode tables and to test if the
 remote is generating events (EV_MSC/EV_KEY/EV_REP/EV_SYN).
 
 Now, it will be using V2 for reads and keycode cleanups, but will still use
 V1 for writes, as, currently with 32 bits scancodes, there's no gain to use
 V2 for it. Also, changing the tool to use more bits will require to rewrite
 part of the code.
 
 Also, writing a rc-core code that can work with an arbitrary large scancode
 is still on our TODO list.
 
 I'm not entirely sure how to extend the scancode size, as there are a
 few options:
   1) Core would always work internally with 32 bytes (1024 bits). Some
 logic will be required to accept entries with .len  32;
   2) Drivers will define the code lengtht, and core will use it,
 returning -EINVAL if userspace uses a len grater than used internally by
 the core. In this case, we'll need a sysfs node to tell userspace what's
 the maximum allowed size;
   3) Drivers will define the max number of bits, and core will use it,
 truncating the number to the max size if userspace tries to write more bits 
 than the internal representation;
   4) Drivers will define the max number of bits, and core will use it,
 returning an error if the number is bigger than the max scancode that can be
 represented internally.
 
 I think that (2) is the best way for doing it, but I'm not yet entirely sure.
 So, it is good to hear some comments about that.

I'd say 4 and userspace utility should normalize scancodes packing them into the
least number of bits possible. Since keymap should be device specific
data in the keymap will not exceed what the driver expects, right?

-- 
Dmitry
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ?

[Dmitry Torokhov]

On Wed, Jan 26, 2011 at 06:09:45AM +1000, Linus Torvalds wrote:
 On Wed, Jan 26, 2011 at 2:48 AM, Dmitry Torokhov
 dmitry.torok...@gmail.com wrote:
 
  We should be able to handle the case where scancode is valid even though
  it might be unmapped yet. This is regardless of what version of
  EVIOCGKEYCODE we use, 1 or 2, and whether it is sparse keymap or not.
 
  Is it possible to validate the scancode by driver?
 
 More appropriately, why not just revert the thing? The version change

Well, then we'll break Ubuntu again as they recompiled their input-utils
package (without fixing the check). And the rest of distros do not seem
to be using that package...

 and the buggy EINVAL return both.

I believe that -EINVAL thing only affects RC devices that Mauro switched
to the new rc-core; input core in itself should be ABI compatible. Thus
I'll leave the decision to him whether he wants to revert or fix
compatibility issue.

 
 As Mark said, breaking user space simply isn't acceptable. And since
 breaking user space isn't acceptable, then incrementing the version is
 stupid too.

It might not have been the best idea to increment, however I maintain
that if there exists version is can be changed. Otherwise there is no
point in having version at all.

As I said, reverting the version bump will cause yet another wave of
breakages so I propose leaving version as is.

 
 The way we add new ioctl's is not by incrementing some ABI version
 crap. It's by adding new ioctl's or system calls or whatever that
 simply used to return -ENOSYS or other error before, while preserving
 the old ABI. That way old binaries don't break (for _ANY_ reason), and
 new binaries can see oh, this doesn't support the new thing.

That has been done as well; we have 2 new ioctls and kept 2 old ioctls.

-- 
Dmitry
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ?

[Dmitry Torokhov]

On Tue, Jan 25, 2011 at 12:54:53PM -0800, Dmitry Torokhov wrote:
 On Wed, Jan 26, 2011 at 06:09:45AM +1000, Linus Torvalds wrote:
  On Wed, Jan 26, 2011 at 2:48 AM, Dmitry Torokhov
  dmitry.torok...@gmail.com wrote:
  
   We should be able to handle the case where scancode is valid even though
   it might be unmapped yet. This is regardless of what version of
   EVIOCGKEYCODE we use, 1 or 2, and whether it is sparse keymap or not.
  
   Is it possible to validate the scancode by driver?
  
  More appropriately, why not just revert the thing? The version change
 
 Well, then we'll break Ubuntu again as they recompiled their input-utils
 package (without fixing the check). And the rest of distros do not seem
 to be using that package...
 
  and the buggy EINVAL return both.
 
 I believe that -EINVAL thing only affects RC devices that Mauro switched
 to the new rc-core; input core in itself should be ABI compatible. Thus
 I'll leave the decision to him whether he wants to revert or fix
 compatibility issue.
 
  
  As Mark said, breaking user space simply isn't acceptable. And since
  breaking user space isn't acceptable, then incrementing the version is
  stupid too.
 
 It might not have been the best idea to increment, however I maintain
 that if there exists version is can be changed. Otherwise there is no
 point in having version at all.
 
 As I said, reverting the version bump will cause yet another wave of
 breakages so I propose leaving version as is.
 
  
  The way we add new ioctl's is not by incrementing some ABI version
  crap. It's by adding new ioctl's or system calls or whatever that
  simply used to return -ENOSYS or other error before, while preserving
  the old ABI. That way old binaries don't break (for _ANY_ reason), and
  new binaries can see oh, this doesn't support the new thing.
 
 That has been done as well; we have 2 new ioctls and kept 2 old ioctls.
 

BTW, another issue is that evdev's ioctl returns -EINVAL for unknown
ioctls so applications would have hard time figuring out whether error
returned because of kernel being too old or because they are trying to
retrieve/establish invalid mapping if they had to go only by the error
code.

As far as I can see EINVAL is a proper error for unknown ioctls:

[dtor@hammer work]$ man 2 ioctl | grep EINVAL
   EINVAL Request or argp is not valid.
[dtor@hammer work]$

-- 
Dmitry
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ?

[Dmitry Torokhov]

On Wed, Jan 26, 2011 at 07:20:07AM +1000, Linus Torvalds wrote:
 On Wed, Jan 26, 2011 at 7:01 AM, Dmitry Torokhov
 dmitry.torok...@gmail.com wrote:
 
  BTW, another issue is that evdev's ioctl returns -EINVAL for unknown
  ioctls so applications would have hard time figuring out whether error
  returned because of kernel being too old or because they are trying to
  retrieve/establish invalid mapping if they had to go only by the error
  code.
 
 So that's just another evdev interface bug.

Huh? I do not have lot of options here as far as error codes go. Invalid
request, invalid data in request - all goes to EINVAL.

 
  As far as I can see EINVAL is a proper error for unknown ioctls:
 
  [dtor@hammer work]$ man 2 ioctl | grep EINVAL
        EINVAL Request or argp is not valid.
 
 Yeah, there's some confusion there.
 
 The unknown ioctl error code is (for traditional reasons) ENOTTY,
 but yes, the EINVAL thing admittedly has a lot of legacy use too.
 
 Inside the kernel, the preferred way to say I don't recognize that
 ioctl number is actually ENOIOCTLCMD.  That's exactly so that various
 nested ioctl handlers can then tell the difference between I didn't
 recognize that ioctl and I understand what you asked me to do, but
 your arguments were crap.
 
 vfs_ioctl() will then turn ENOIOCTLCMD to EINVAL to return to user space.

OK, so I can change evdev to employ ENOIOCTLCMD where needed, bit that
will not change older kernels where such distinction is needed (as never
kernels do support newer ioctl). And even if I could go back it would
not help since userspace still sees EINVAL only.

-- 
Dmitry
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ?

[Dmitry Torokhov]

On Tue, Jan 25, 2011 at 05:22:09PM -0500, Mark Lord wrote:
 On 11-01-25 05:00 PM, Mauro Carvalho Chehab wrote:
  Em 25-01-2011 18:54, Dmitry Torokhov escreveu:
  On Wed, Jan 26, 2011 at 06:09:45AM +1000, Linus Torvalds wrote:
  On Wed, Jan 26, 2011 at 2:48 AM, Dmitry Torokhov
  dmitry.torok...@gmail.com wrote:
 
  We should be able to handle the case where scancode is valid even though
  it might be unmapped yet. This is regardless of what version of
  EVIOCGKEYCODE we use, 1 or 2, and whether it is sparse keymap or not.
 
  Is it possible to validate the scancode by driver?
 
  More appropriately, why not just revert the thing? The version change
  
  Reverting the version increment is a bad thing. I agree with Dmitry that
  an application that fails just because the API version were incremented
  is buggy.
  
  Well, then we'll break Ubuntu again as they recompiled their input-utils
  package (without fixing the check). And the rest of distros do not seem
  to be using that package...
  
  Reverting it will also break the ir-keytable userspace program that it is
  meant to be used by the Remote Controller devices, and uses it to adjust
  its behaviour to support RC's with more than 16 bits of scancodes.
  
  I agree that it is bad that the ABI broke, but reverting it will cause even
  more damage.
 
 There we disagree.  Sure it's a very poorly thought out interface,
 but the way to fix it is to put a new one along side the old,
 and put the old back the way it was before it got broken.
 
 I'm not making a fuss here for myself -- I'm more than capable of working
 around new kernel bugs like these, but for every person like me there are
 likely hundreds of others who simply get frustrated and give up.
 
 If you're worried about Ubuntu's adaptation to the buggy regression,
 then email their developers (kernel and input-utils packagers) explaining
 the revert, and they can coordination their kernel and input-utils updates
 to do the Right Thing.
 
 But for all of the rest of us, our systems are broken by this change.
 
 ...
 
 
  As Mark said, breaking user space simply isn't acceptable. And since
  breaking user space isn't acceptable, then incrementing the version is
  stupid too.
 
  It might not have been the best idea to increment, however I maintain
  that if there exists version is can be changed. Otherwise there is no
  point in having version at all.
  
  Not arguing in favor of the version numbering, but it is easy to read
  the version increment at the beginning of the application, and adjust
  if the code will use EVIOCGKEYCODE or EVIOCGKEYCODE_V2 of the ioctl's,
  depending on what kernel provides.
  
  Ok, we might be just calling the new ioctl and check for -ENOSYS at
  the beginning, using some fake arguments.
  
  As I said, reverting the version bump will cause yet another wave of
  breakages so I propose leaving version as is.
 
 
  The way we add new ioctl's is not by incrementing some ABI version
  crap. It's by adding new ioctl's or system calls or whatever that
  simply used to return -ENOSYS or other error before, while preserving
  the old ABI. That way old binaries don't break (for _ANY_ reason), and
  new binaries can see oh, this doesn't support the new thing.
 
  That has been done as well; we have 2 new ioctls and kept 2 old ioctls.
 
 That's the problem: you did NOT keep the two old ioctls().
 Those got changed too.. so now we have four NEW ioctls(),
 none of which backward compatible with userspace.
 

Please calm down. This, in fact, is not new vs old ioctl problem but
rather particular driver (or rather set of drivers) implementation
issue. Even if we drop the new ioctls and convert the RC code to use the
old ones you'd be observing the same breakage as RC code responds with
-EINVAL to not-yet-established mappings.

I'll see what can be done for these drivers; I guess we could supply a
fake KEY_RESERVED entry for not mapped scancodes if there are mapped
scancodes above current one. That should result in the same behavior
for RCs as before.

-- 
Dmitry
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ?

[Dmitry Torokhov]

On Tue, Jan 25, 2011 at 03:29:14PM -0800, Dmitry Torokhov wrote:
 On Tue, Jan 25, 2011 at 05:22:09PM -0500, Mark Lord wrote:
  On 11-01-25 05:00 PM, Mauro Carvalho Chehab wrote:
   Em 25-01-2011 18:54, Dmitry Torokhov escreveu:
   On Wed, Jan 26, 2011 at 06:09:45AM +1000, Linus Torvalds wrote:
   On Wed, Jan 26, 2011 at 2:48 AM, Dmitry Torokhov
   dmitry.torok...@gmail.com wrote:
  
   We should be able to handle the case where scancode is valid even 
   though
   it might be unmapped yet. This is regardless of what version of
   EVIOCGKEYCODE we use, 1 or 2, and whether it is sparse keymap or not.
  
   Is it possible to validate the scancode by driver?
  
   More appropriately, why not just revert the thing? The version change
   
   Reverting the version increment is a bad thing. I agree with Dmitry that
   an application that fails just because the API version were incremented
   is buggy.
   
   Well, then we'll break Ubuntu again as they recompiled their input-utils
   package (without fixing the check). And the rest of distros do not seem
   to be using that package...
   
   Reverting it will also break the ir-keytable userspace program that it is
   meant to be used by the Remote Controller devices, and uses it to adjust
   its behaviour to support RC's with more than 16 bits of scancodes.
   
   I agree that it is bad that the ABI broke, but reverting it will cause 
   even
   more damage.
  
  There we disagree.  Sure it's a very poorly thought out interface,
  but the way to fix it is to put a new one along side the old,
  and put the old back the way it was before it got broken.
  
  I'm not making a fuss here for myself -- I'm more than capable of working
  around new kernel bugs like these, but for every person like me there are
  likely hundreds of others who simply get frustrated and give up.
  
  If you're worried about Ubuntu's adaptation to the buggy regression,
  then email their developers (kernel and input-utils packagers) explaining
  the revert, and they can coordination their kernel and input-utils updates
  to do the Right Thing.
  
  But for all of the rest of us, our systems are broken by this change.
  
  ...
  
  
   As Mark said, breaking user space simply isn't acceptable. And since
   breaking user space isn't acceptable, then incrementing the version is
   stupid too.
  
   It might not have been the best idea to increment, however I maintain
   that if there exists version is can be changed. Otherwise there is no
   point in having version at all.
   
   Not arguing in favor of the version numbering, but it is easy to read
   the version increment at the beginning of the application, and adjust
   if the code will use EVIOCGKEYCODE or EVIOCGKEYCODE_V2 of the ioctl's,
   depending on what kernel provides.
   
   Ok, we might be just calling the new ioctl and check for -ENOSYS at
   the beginning, using some fake arguments.
   
   As I said, reverting the version bump will cause yet another wave of
   breakages so I propose leaving version as is.
  
  
   The way we add new ioctl's is not by incrementing some ABI version
   crap. It's by adding new ioctl's or system calls or whatever that
   simply used to return -ENOSYS or other error before, while preserving
   the old ABI. That way old binaries don't break (for _ANY_ reason), and
   new binaries can see oh, this doesn't support the new thing.
  
   That has been done as well; we have 2 new ioctls and kept 2 old ioctls.
  
  That's the problem: you did NOT keep the two old ioctls().
  Those got changed too.. so now we have four NEW ioctls(),
  none of which backward compatible with userspace.
  
 
 Please calm down. This, in fact, is not new vs old ioctl problem but
 rather particular driver (or rather set of drivers) implementation
 issue. Even if we drop the new ioctls and convert the RC code to use the
 old ones you'd be observing the same breakage as RC code responds with
 -EINVAL to not-yet-established mappings.
 
 I'll see what can be done for these drivers; I guess we could supply a
 fake KEY_RESERVED entry for not mapped scancodes if there are mapped
 scancodes above current one. That should result in the same behavior
 for RCs as before.
 

I wonder if the patch below is all that is needed...

Thanks!

-- 
Dmitry


Input: ir-keymap - return KEY_RESERVED for unknown mappings

Do not respond with -EINVAL to EVIOCGKEYCODE for not-yet-mapped scancodes,
but rather return KEY_RESERVED.

This fixes breakage with Ubuntu's input-kbd utility that stopped returning
full keymaps for remote controls.

Signed-off-by: Dmitry Torokhov d...@mail.ru
---

 drivers/media/IR/ir-keytable.c |   28 +---
 1 files changed, 17 insertions(+), 11 deletions(-)


diff --git a/drivers/media/IR/ir-keytable.c b/drivers/media/IR/ir-keytable.c
index f60107c..c4645d7 100644
--- a/drivers/media/IR/ir-keytable.c
+++ b/drivers/media/IR/ir-keytable.c
@@ -374,21 +374,27 @@ static int ir_getkeycode(struct input_dev *dev

Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ?

[Dmitry Torokhov]

On Tue, Jan 25, 2011 at 12:07:29AM -0500, Mark Lord wrote:
 On 11-01-25 12:04 AM, Mark Lord wrote:
  On 11-01-24 11:55 PM, Dmitry Torokhov wrote:
  On Mon, Jan 24, 2011 at 11:37:06PM -0500, Mark Lord wrote:
  ..
  This results in (map-size==10) for 2.6.36+ (wrong),
  and a much larger map-size for 2.6.35 and earlier.
 
  So perhaps EVIOCGKEYCODE has changed?
 
 
  So the utility expects that all devices have flat scancode space and
  driver might have changed so it does not recognize scancode 10 as valid
  scancode anymore.
 
  The options are:
 
  1. Convert to EVIOCGKEYCODE2
  2. Ignore errors from EVIOCGKEYCODE and go through all 65536 iterations.
  
  or 3. Revert/fix the in-kernel regression.
  
  The EVIOCGKEYCODE ioctl is supposed to return KEY_RESERVED for unmapped
  (but value) keycodes, and only return -EINVAL when the keycode itself
  is out of range.
  
  That's how it worked in all kernels prior to 2.6.36,
  and now it is broken.  It now returns -EINVAL for any unmapped keycode,
  even though keycodes higher than that still have mappings.
  
  This is a bug, a regression, and breaks userspace.
  I haven't identified *where* in the kernel the breakage happened,
  though.. that code confuses me.  :)
 
 Note that this device DOES have flat scancode space,
 and the kernel is now incorrectly signalling an error (-EINVAL)
 in response to a perfectly valid query of a VALID (and mappable)
 keycode on the remote control
 
 The code really is a valid button, it just doesn't have a default mapping
 set by the kernel (I can set a mapping for that code from userspace and it 
 works).
 

OK, in this case let's ping Mauro - I think he done the adjustments to
IR keymap hanlding.

Thanks.

-- 
Dmitry
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html