On Tue, June 30, 2009 23:23, Andrew Morton wrote:
> On Sat, 27 Jun 2009 07:18:32 +0200
> David Härdeman <david@xxxxxxxxxxx> wrote:
>> +static void
>> +wbcir_set_bits(unsigned long addr, u8 bits, u8 mask)
>> +{
>> + u8 val;
>> +
>> + val = inb(addr);
>> + val = ((val & ~mask) | (bits & mask));
>> + outb(val, addr);
>> +}
>
> What locking prevents the races which could occur here?
>
> Whatever it is, it would be good to document that here in a code
> comment.
I should probably clarify the comment in "struct wbcir_data" to mention
that wbcir_lock is supposed to protect registers against race conditions.
>>
>> ...
>>
>> +static u8
>> +wbcir_revbyte(u8 byte)
>> +{
>> + byte = ((byte >> 1) & 0x55) | ((byte << 1) & 0xAA);
>> + byte = ((byte >> 2) & 0x33) | ((byte << 2) & 0xCC);
>> + return (byte >> 4) | (byte<<4);
>> +}
>
> Can this use the facilities in include/linux/bitrev.h?
Yes, I grep:ed for a suitable function but must have missed it, thanks for
pointing it out.
>> + 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?
>> + 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.
>> +{
>> + 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? :)
>> +static void
>> +add_irdata_bit(struct wbcir_data *data, int set)
>> +{
>> + if (set)
>> + data->irdata[data->irdata_count / 8] |=
>> + 0x01 << (data->irdata_count % 8);
>
> Can/should this use generic___set_le_bit() or similar, rather than
> open-coding it?
Will look into it...
You also had various code-comment objections which I'll take care of.
Thanks for the prompt review.
Regards,
David
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
