On Wed, 1 Jul 2009 09:47:46 +0200 (CEST)
David H__rdeman <david@xxxxxxxxxxx> wrote:
> >> + struct wbcir_data *data = input_get_drvdata(dev);
> >> +
> >> + if (scancode < 0 || scancode > 0xFFFFFFFF)
> >
> > Neither of the comparisons in this expression can ever be true.
>
> Didn't know if I could be certain that int is always 32 bit on all
> platforms which use/might use the chip...can I?
Yep, int and unsigned int are always 32-bit.
It's not a big deal at all - the compiler will optimise the tests away
completely. The tests may have some value as code commentary, and they
provide robustness should someone change the type of `scancode'.
> >> + return -EINVAL;
> >> +
> >> + *keycode = (int)wbcir_do_getkeycode(data, (u32)scancode);
> >
> > uneeded casts.
> >
> > Something has gone wrong with the types here. Where does the fault lie
> > - this driver, or input core?
>
> Two issues:
>
> a) the input layer API confused me
>
> include/linux/input.h provides:
>
> struct input_event {
> struct timeval time;
> __u16 type;
> __u16 code;
> __s32 value;
> };
>
> (keycode is an unsigned 16 bit integer)
>
> int input_get_keycode(struct input_dev *dev, int scancode, int *keycode);
> int input_set_keycode(struct input_dev *dev, int scancode, int keycode);
>
> (keycode is an int)
>
> static inline void input_report_key(struct input_dev *dev,
> unsigned int code,
> int value)
> {
> input_event(dev, EV_KEY, code, !!value);
> }
>
> (keycode is an uint)
>
>
> b) 32 bit scancodes
>
> I wanted to use 32 bit scancodes in my driver since the largest IR message
> supported by the driver is RC6 mode 6A which can potentially have a 1 + 15
> bits "customer" field + 8 bits address + 8 bits command = 32 bits.
>
> Casting the int scancode passed to input_[get__set]_keycode to an uint and
> assuming it would be at least 32 bits on all platforms using the chip was
> the best solution I could come up with without changing the input API.
erp. Hopefully this is all something which Dmitry can help us with.
> >> +{
> >> + struct wbcir_data *data = (struct wbcir_data *)cookie;
> >> + unsigned long flags;
> >> +
> >> + /*
> >> + * data->keyup_jiffies is used to prevent a race condition if a
> >> + * hardware interrupt occurs at this point and the keyup timer
> >> + * event is moved further into the future as a result.
> >> + */
> >
> >hm. I don't see what the race is, nor how the comparison fixes it. If
> >I _did_ understand this, perhaps I could suggest alternative fixes.
> >But I don't so I can't. Oh well.
>
> When the interrupt service routine detects an IR command it reports a
> keydown event and sets a timer to report a keyup event in the future if no
> repeated ir messages are detected (in which case the timer-driven keyup
> should be pushed further into the future to allow the input core to do its
> auto-repeat-handling magic).
>
> What I wanted to protect against was something like this:
>
> Thread 1 Thread 2
> -------- --------
> ISR called, grabs
> wbcir_lock, reports
> keydown for key X,
> sets up keyup timer,
> releases lock, exits
>
> (many ms later)
>
> keyup timer function called
> and preempted before grabbing
> wbcir_lock
>
> ISR called, grabs wbcir_lock,
> notices a repeat event for
> key X, pushes the keyup timer
> further into the future using
> mod_timer (thus reenabling the
> timer), releases lock, exits
> keyup timer function grabs
> wbcir_lock, reports keyup,
> exits.
> (many ms later)
>
> keyup timer function called *again*,
> reports keyup, exits.
>
> The result would be (if I understood the timer implementation correctly)
> that a keyup event is reported immediately after the second ISR even
> though the "first" timer function call should have been cancelled/pushed
> further into the future at that point.
>
> Does this make any sense? :)
yes. The timer will be rescheduledin the scenario which you describe.
Here's my problem: often when I ask a question about some code, what I
_really_ mean is "if I didn't understand this code today, others
probably won't understand it when reading it a year from now. Hence it
perhaps needs additional commentary to prevent this". But I tire of
complaining about code comments ;)
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
