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

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

On Wed, Sep 08, 2010 at 11:07:57AM +0800, Jason Wang wrote:
> Dmitry Torokhov wrote:
> >On Wed, Sep 01, 2010 at 09:48:18AM -0700, Dmitry Torokhov wrote:
> >>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.
> >>
> >
> >I wonder if the following patch works (or can be made to work)...
> >
> Hi Dmitry,
> 
> I have made a quick test of this patch, currently two issues found:
> 1) missing #include <linux/sched.h> , it will produce a building error.
> 2) touch panel can't work, it seems that the input dev doesn't
> report pen up events.
> 

Yeah, it is kinda hard to get stuff working first time around without
the hardware :)

> Your patch is a good frame work, i will continue working basing off
> your patch.
> Maybe several dates later, i will send out an extra patch to
> strengthen your patch.
> 

Great, let me know how it goes.

Thanks.

-- 
Dmitry