Message-ID: <20110825170112.GA27146@core.coreip.homeip.net>
Date:	Thu, 25 Aug 2011 10:01:12 -0700
From:	Dmitry Torokhov <dmitry.torokhov@...il.com>
To:	Alan Cox <alan@...rguk.ukuu.org.uk>
Cc:	Dudley Du <dudl@...ress.com>,
	"rubini@...vis.unipv.it" <rubini@...l.unipv.it>,
	"rydberg@...omail.se" <rydberg@...omail.se>,
	"ccross@...roid.com" <ccross@...roid.com>,
	"konkers@...roid.com" <konkers@...roid.com>,
	"olof@...om.net" <olof@...om.net>,
	"linux-input@...r.kernel.org" <linux-input@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-tegra@...r.kernel.org" <linux-tegra@...r.kernel.org>
Subject: Re: [PATCH] input:cyapa: Add new I2C-based input touchpad driver for
 Cypress I2C touchpad devices

On Thu, Aug 25, 2011 at 03:01:46PM +0100, Alan Cox wrote:
> > +
> > +static void cyapa_disable_irq(struct cyapa_i2c *touch)
> > +{
> > +       unsigned long flags;
> > +
> > +       spin_lock_irqsave(&touch->miscdev_spinlock, flags);
> > +       if (!touch->polling_mode_enabled &&
> > +               touch->bl_irq_enable &&
> > +               touch->irq_enabled) {
> > +               touch->irq_enabled = false;
> > +               disable_irq(touch->irq);
> > +       }
> > +       spin_unlock_irqrestore(&touch->miscdev_spinlock, flags);
> 
> This doesn't do what you think. disable_irq does not guarantee that the
> IRQ is not executing on another CPU core. disable_irq_sync does but then
> you will deadlock.

Actually disable_irq() does guarantee that IRQ is not executing once the
call completes. We have disable_irq_nosync() that does not wait.

> +
> > +static int cyapa_i2c_open(struct input_dev *input)
> > +{
> > +       struct cyapa_i2c *touch = input_get_drvdata(input);
> > +       int ret;
> > +       unsigned long flags;
> > +
> > +       spin_lock_irqsave(&touch->lock, flags);
> > +       if (touch->open_count == 0) {
> > +               ret = cyapa_i2c_reset_config(touch);
> > +               if (ret < 0) {
> > +                       pr_err("reset i2c trackpad error code, %d.\n", ret);
> > +                       return ret;
> > +               }
> > +       }
> > +       spin_unlock_irqrestore(&touch->lock, flags);
> > +
> > +       spin_lock_irqsave(&touch->lock, flags);
> > +       touch->open_count++;
> > +       spin_unlock_irqrestore(&touch->lock, flags);
> 
> You've dropped the lock and taken it and dropped it again in sequential
> lines. That's nonsensical and also means you've added a race !
> 

Also please note that input core ensures that open() and close() methods
are called only when needed (first user opens the device or last user
closes it), you do not need to count yourself.

> 
> > +static void cyapa_i2c_close(struct input_dev *input)
> > +{
> > +       unsigned long flags;
> > +       struct cyapa_i2c *touch = input_get_drvdata(input);
> > +
> > +       spin_lock_irqsave(&touch->lock, flags);
> > +
> > +       if (touch->open_count == 0) {
> 
> Wouldn't this be an error ?
> 
> 
> > +       ret = request_irq(touch->irq,
> > +                       cyapa_i2c_irq,
> > +                       IRQF_TRIGGER_FALLING,
> > +                       CYAPA_I2C_NAME,
> > +                       touch);
> 
> This IRQ can happen from the moment it is registered which means it can
> occur before the variables you set up further down...
> 
> > +       if (ret) {
> > +               pr_warning("IRQ request failed: %d, "
> > +                       "falling back to polling mode.\n", ret);
> > +
> > +               spin_lock_irqsave(&touch->miscdev_spinlock, flags);
> > +               touch->polling_mode_enabled = true;
> > +               touch->bl_irq_enable = false;
> > +               touch->irq_enabled = false;
> > +               spin_unlock_irqrestore(&touch->miscdev_spinlock, flags);
> > +       } else {
> > +               spin_lock_irqsave(&touch->miscdev_spinlock, flags);
> > +               touch->polling_mode_enabled = false;
> > +               touch->bl_irq_enable = false;
> > +               touch->irq_enabled = true;
> > +               enable_irq_wake(touch->irq);
> > +               spin_unlock_irqrestore(&touch->miscdev_spinlock, flags);
> > +       }
> > +
> > +       /* create an input_dev instance for trackpad device. */
> > +       ret = cyapa_create_input_dev(touch);
> > +       if (ret) {
> > +               free_irq(touch->irq, touch);
> 
> But what if it was polling ?
> 
> 
> In general I think
> 
> - use the threaded_irq interfaces for your IRQ paths (if you look at the
>   current kernel you'll see a lot of drivers do this)
> - use the polled input device interface for the no IRQ case, so it does
>   all the polling work for you

Polling mode for a touchscreen is unusable in real product (unlike
accelerometers/magnetometers/etc touchscreens need constant polling with
relatively high rate) so I'd rather not have it at all.

> - tidy up the locking (and the fact you've got locking in there is a lot
>   better than many first submissions we see)
> 
> I would think about how the various states and handlers work. Right now
> the code is quite convoluted, and maybe a state machine of some kind would
> clean it up ?
> 

Also the miscdevice inteface should go away and be replaced with
sysfs/debugfs solution. For firmware update request_firmware() interface
can be used (see atmel_mxt_ts driver).

Thanks.

-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists