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 -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
