Re: [PATCH v2] Input: ads7846 - Replace spinlock by mutex to wrap disable()/enable()

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

On Wed, Sep 01, 2010 at 06:07:30PM +0800, Jason Wang wrote:
> Dmitry Torokhov wrote:
> >Hi Jason,
> >
> >On Wed, Aug 25, 2010 at 11:45:27AM +0800, Jason Wang wrote:
> >>Hi dmitry and others,
> >>
> >>could you please to help me review this patch?
> >>
> >>Thanks,
> >>Jason.
> >>
> >>Jason Wang wrote:
> >>>The commit 9114337 introduces regulator operations in the ads7846
> >>>touchscreen driver. Among these operations, some are called in the
> >>>spinlock protected context.
> >>>On most platforms, the regulator operation is achieved through
> >>>i2c/spi bus transfer operations, some bus transfer operations will
> >>>call wait_for_completion function. It isn't allowable  to call
> >>>sleepable function in the atomic context. So replace the spinlock with
> >>>mutex to protect ads7846_disable()/ads7846_enable().
> >>>
> >
> >I am afraid the patch is not correct. ads7846_disable() and
> >ads7846_enable() check and modify flags (such as irq_disabled and
> >pending) that are also accessed form timer/interrupt context. Moving to
> >mutex removes the serialization that used to be there.
> Thanks for your comments. you are right it is dangerous and
> unreasonable to use different locks to protect a critical resource.
> >I wonder if we should start by converting the driver to used threaded
> >IRQ model with "long playing" interrupt handler so all access happens in
> >process context and shutdown sequence is simplified.
> It can solve most issues, but it can't solve this situation. Because
> here the atomic region in which conflicts happened is from spin_lock
> instead of irq handler.
> 

Right, but switching to threaded IRQ will allow to use mutex in place
of the spinlock everywhere.

-- 
Dmitry