Re: [patch 2/2] Add a driver for the Winbond WPCD376I Consumer IRhardware

[David Härdeman]

On Wed, Aug 12, 2009 at 11:58:22PM -0700, Dmitry Torokhov wrote:
> > +/* Registers and other state is protected by wbcir_lock */
> > +struct wbcir_data {
> > +	unsigned long wbase;        /* Wake-Up Baseaddr		*/
> > +	unsigned long ebase;        /* Enhanced Func. Baseaddr	*/
> > +	unsigned long sbase;        /* Serial Port Baseaddr	*/
> > +	unsigned int  irq;          /* Serial Port IRQ		*/
> > +
> > +	struct input_dev *input_dev;
> > +	struct timer_list timer_keyup;
> > +	struct led_trigger *rxtrigger;
> > +	struct led_trigger *txtrigger;
> > +	struct led_classdev led;
> > +
> > +	u32 last_scancode;
> > +	unsigned int last_keycode;
> > +	u8 last_toggle;
> > +	u8 keypressed;
> > +	unsigned long keyup_jiffies;
> > +	unsigned int idle_count;
> > +
> > +	/* RX irdata and parsing state */
> > +	unsigned long irdata[30];
> > +	unsigned int irdata_count;
> > +	unsigned int irdata_idle;
> > +	unsigned int irdata_off;
> > +	unsigned int irdata_error;
> > +
> > +	/* Protected by keytable_lock */
> > +	struct list_head keytable;
>
> I think this has a potential to deadlock... Set and get keycodes are
> called with event lock taken, and then your implementations acquire
> keytable lock. When you emit input events the opposite happens - you
> take the keytable lock and then input core takes event lock.

I've taken a look at it, and I think it's ok. set/getkeycode take the 
event lock and then the keytable lock.

The irqhandler call sequence (which generates the input events) will 
take the keytable lock, copy the keycode, release the lock and report 
the event (which will take the event lock).

--
David Härdeman
--
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/