[patch 3/9] input: ads7846.c sparse lock annotation

[patch 3/9] input: ads7846.c sparse lock annotation

[akpm]

From: Harvey Harrison <harvey.harrison@gmail.com>

Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>
Cc: Dmitry Torokhov <dtor@mail.ru>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 drivers/input/touchscreen/ads7846.c |    2 ++
 1 file changed, 2 insertions(+)

diff -puN drivers/input/touchscreen/ads7846.c~input-ads7846c-sparse-lock-annotation drivers/input/touchscreen/ads7846.c
--- a/drivers/input/touchscreen/ads7846.c~input-ads7846c-sparse-lock-annotation
+++ a/drivers/input/touchscreen/ads7846.c
@@ -769,6 +769,8 @@ static irqreturn_t ads7846_irq(int irq, 
 
 /* Must be called with ts->lock held */
 static void ads7846_disable(struct ads7846 *ts)
+__releases(&ts->lock)
+__acquires(&ts->lock)
 {
 	if (ts->disabled)
 		return;
_

Re: [patch 3/9] input: ads7846.c sparse lock annotation

[Dmitry Torokhov]

On Tue, May 12, 2009 at 01:43:06PM -0700, akpm@linux-foundation.org wrote:
> From: Harvey Harrison <harvey.harrison@gmail.com>
> 
> Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>
> Cc: Dmitry Torokhov <dtor@mail.ru>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> ---
> 
>  drivers/input/touchscreen/ads7846.c |    2 ++
>  1 file changed, 2 insertions(+)
> 
> diff -puN drivers/input/touchscreen/ads7846.c~input-ads7846c-sparse-lock-annotation drivers/input/touchscreen/ads7846.c
> --- a/drivers/input/touchscreen/ads7846.c~input-ads7846c-sparse-lock-annotation
> +++ a/drivers/input/touchscreen/ads7846.c
> @@ -769,6 +769,8 @@ static irqreturn_t ads7846_irq(int irq, 
>  
>  /* Must be called with ts->lock held */
>  static void ads7846_disable(struct ads7846 *ts)
> +__releases(&ts->lock)
> +__acquires(&ts->lock)
>  {

I still haven't gotten any explanation why this is needed and also I am
still getting sparce warnings with this patch applied. Please drop.

-- 
Dmitry

Re: [patch 3/9] input: ads7846.c sparse lock annotation

[Harvey Harrison]

On Wed, 2009-05-13 at 20:24 -0700, Dmitry Torokhov wrote:
> On Tue, May 12, 2009 at 01:43:06PM -0700, akpm@linux-foundation.org wrote:
> > From: Harvey Harrison <harvey.harrison@gmail.com>
> > 
> > Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>
> > Cc: Dmitry Torokhov <dtor@mail.ru>
> > Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> > ---
> >  
> >  /* Must be called with ts->lock held */
> >  static void ads7846_disable(struct ads7846 *ts)
> > +__releases(&ts->lock)
> > +__acquires(&ts->lock)
> >  {
> 
> I still haven't gotten any explanation why this is needed and also I am
> still getting sparce warnings with this patch applied. Please drop.
> 

Sorry, I didn't realize I had a local patchset doing extra context checking
of what lock is passed in to spin_lock/unlock...it helps to document when a
function requires a lock held (see the comment)

The culprit in this function:
		while (ts->pending) {
			spin_unlock_irq(&ts->lock);
			msleep(1);
			spin_lock_irq(&ts->lock);
		}

Anyways, as current sparse will still warn please drop.

Harvey

Re: [patch 3/9] input: ads7846.c sparse lock annotation

[Dmitry Torokhov]

On Wed, May 13, 2009 at 08:34:27PM -0700, Harvey Harrison wrote:
> On Wed, 2009-05-13 at 20:24 -0700, Dmitry Torokhov wrote:
> > On Tue, May 12, 2009 at 01:43:06PM -0700, akpm@linux-foundation.org wrote:
> > > From: Harvey Harrison <harvey.harrison@gmail.com>
> > > 
> > > Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>
> > > Cc: Dmitry Torokhov <dtor@mail.ru>
> > > Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> > > ---
> > >  
> > >  /* Must be called with ts->lock held */
> > >  static void ads7846_disable(struct ads7846 *ts)
> > > +__releases(&ts->lock)
> > > +__acquires(&ts->lock)
> > >  {
> > 
> > I still haven't gotten any explanation why this is needed and also I am
> > still getting sparce warnings with this patch applied. Please drop.
> > 
> 
> Sorry, I didn't realize I had a local patchset doing extra context checking
> of what lock is passed in to spin_lock/unlock...it helps to document when a
> function requires a lock held (see the comment)
> 

I understand the comment ;) The annotation syntax is confusing though...
Maybe if instead of __releases and __acquires it said __needs_lock() and
__leaves_locked() that would make more sense... 

> The culprit in this function:
> 		while (ts->pending) {
> 			spin_unlock_irq(&ts->lock);
> 			msleep(1);
> 			spin_lock_irq(&ts->lock);
> 		}
> 
> Anyways, as current sparse will still warn please drop.
> 
> Harvey
> 

-- 
Dmitry

Re: [patch 3/9] input: ads7846.c sparse lock annotation

[Jiri Kosina]

On Wed, 13 May 2009, Dmitry Torokhov wrote:

> > > >  /* Must be called with ts->lock held */
> > > >  static void ads7846_disable(struct ads7846 *ts)
> > > > +__releases(&ts->lock)
> > > > +__acquires(&ts->lock)
> > > >  {
> > > I still haven't gotten any explanation why this is needed and also I am
> > > still getting sparce warnings with this patch applied. Please drop.
> > Sorry, I didn't realize I had a local patchset doing extra context 
> > checking of what lock is passed in to spin_lock/unlock...it helps to 
> > document when a function requires a lock held (see the comment)
> I understand the comment ;) The annotation syntax is confusing though... 
> Maybe if instead of __releases and __acquires it said __needs_lock() and 
> __leaves_locked() that would make more sense...

I fully agree. __acquires() and __releases() is used to annotate a 
completely different thing -- it should be used solely to avoid false 
positives about locking inbalances across functions (i.e. one function 
deliberately not unlocking lock that it has locked), shoudldn't it?

-- 
Jiri Kosina
SUSE Labs