Re: [PATCH 2/2] Add a driver for the Winbond WPCD376I IR functionality.

[David Härdeman]

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-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/