[PATCH 0/5] lock warnings cleanup

[PATCH 0/5] lock warnings cleanup

[Jules Irenge]

This patch series adds missing annotations to various functions,
that register warnings of context imbalance when built with Sparse tool.
The adds fix the warnings, improve on readability of the code
and give better insight or directive on what the functions are actually doing.

Jules Irenge (5):
  ipc/msg: Add missing annotation for freeque()
  video: Add missing annotation for cyber2000fb_enable_ddc() and
    cyber2000fb_disable_ddc()
  ocfs2: Add missing annotation for dlm_empty_lockres()
  libata: Add missing annotation for ata_scsi_rbuf_get() and
    ata_scsi_rbuf_fill()
  ipmi: Add missing annotation for ipmi_ssif_lock_cond() and
    ipmi_ssif_unlock_cond()

 drivers/ata/libata-scsi.c         | 2 ++
 drivers/char/ipmi/ipmi_ssif.c     | 2 ++
 drivers/video/fbdev/cyber2000fb.c | 2 ++
 fs/ocfs2/dlm/dlmmaster.c          | 1 +
 ipc/msg.c                         | 2 ++
 5 files changed, 9 insertions(+)

-- 
2.24.1

[PATCH 1/5] ipc/msg: Add missing annotation for freeque()

[Jules Irenge]

Sparse reports a warning at freeque()

warning: context imbalance in freeque() - unexpected unlock

The root cause is the missing annotation at freeque()

Add the missing __releases(RCU) annotation
Add the missing __releases(&msq->q_perm) annotation

Signed-off-by: Jules Irenge <jbi.octave@gmail.com>
---
 ipc/msg.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/ipc/msg.c b/ipc/msg.c
index caca67368cb5..acd1bc7af55a 100644
--- a/ipc/msg.c
+++ b/ipc/msg.c
@@ -268,6 +268,8 @@ static void expunge_all(struct msg_queue *msq, int res,
  * before freeque() is called. msg_ids.rwsem remains locked on exit.
  */
 static void freeque(struct ipc_namespace *ns, struct kern_ipc_perm *ipcp)
+	__releases(RCU)
+	__releases(&msq->q_perm)
 {
 	struct msg_msg *msg, *t;
 	struct msg_queue *msq = container_of(ipcp, struct msg_queue, q_perm);
-- 
2.24.1

[PATCH 2/5] video: Add missing annotation for cyber2000fb_enable_ddc() and cyber2000fb_disable_ddc()

[Jules Irenge]

Sparse reports warnings at cyber2000fb_enable_ddc()
	and cyber2000fb_disable_ddc()

warning: context imbalance in cyber2000fb_enable_ddc()
	- wrong count at exit

warning: context imbalance in cyber2000fb_disable_ddc()
	- unexpected unlock

The root cause is the missing annotation at cyber2000fb_enable_ddc()
	and cyber2000fb_disable_ddc()

Add the missing __acquires(&cfb->reg_b0_lock) annotation
Add the missing __releases(&cfb->reg_b0_lock) annotation

Signed-off-by: Jules Irenge <jbi.octave@gmail.com>
---
 drivers/video/fbdev/cyber2000fb.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/video/fbdev/cyber2000fb.c b/drivers/video/fbdev/cyber2000fb.c
index 460826a7ad55..513f58f28b0f 100644
--- a/drivers/video/fbdev/cyber2000fb.c
+++ b/drivers/video/fbdev/cyber2000fb.c
@@ -1160,12 +1160,14 @@ EXPORT_SYMBOL(cyber2000fb_detach);
 #define DDC_SDA_IN	(1 << 6)
 
 static void cyber2000fb_enable_ddc(struct cfb_info *cfb)
+	__acquires(&cfb->reg_b0_lock)
 {
 	spin_lock(&cfb->reg_b0_lock);
 	cyber2000fb_writew(0x1bf, 0x3ce, cfb);
 }
 
 static void cyber2000fb_disable_ddc(struct cfb_info *cfb)
+	__releases(&cfb->reg_b0_lock)
 {
 	cyber2000fb_writew(0x0bf, 0x3ce, cfb);
 	spin_unlock(&cfb->reg_b0_lock);
-- 
2.24.1

[PATCH 3/5] ocfs2: Add missing annotation for dlm_empty_lockres()

[Jules Irenge]

Sparse reports a warning at dlm_empty_lockres()

warning: context imbalance in dlm_purge_lockres() - unexpected unlock

The root cause is the missing annotation at dlm_purge_lockres()

Add the missing __must_hold(&dlm->spinlock)

Signed-off-by: Jules Irenge <jbi.octave@gmail.com>
---
 fs/ocfs2/dlm/dlmmaster.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/ocfs2/dlm/dlmmaster.c b/fs/ocfs2/dlm/dlmmaster.c
index 900f7e466d11..8cbd46b3509a 100644
--- a/fs/ocfs2/dlm/dlmmaster.c
+++ b/fs/ocfs2/dlm/dlmmaster.c
@@ -2762,6 +2762,7 @@ static int dlm_migrate_lockres(struct dlm_ctxt *dlm,
  * Returns: 1 if dlm->spinlock was dropped/retaken, 0 if never dropped
  */
 int dlm_empty_lockres(struct dlm_ctxt *dlm, struct dlm_lock_resource *res)
+	__must_hold(&dlm->spinlock)
 {
 	int ret;
 	int lock_dropped = 0;
-- 
2.24.1

[PATCH 4/5] libata: Add missing annotation for ata_scsi_rbuf_get() and ata_scsi_rbuf_fill()

[Jules Irenge]

Sparse reports a warning at ata_scsi_rbuf_fill() and ata_scsi_rbuf_get()

 warning: context imbalance in ata_scsi_rbuf_get() - wrong count at exit
warning: context imbalance in ata_scsi_rbuf_fill() - unexpected unlock

The root cause is the missing annotation at ata_scsi_rbuf_fill()
	and ata_scsi_rbuf_get()

Add the missing __acquires(&ata_scsi_rbuf_lock)
Add the missing __releases(&ata_scsi_rbuf_lock)

Signed-off-by: Jules Irenge <jbi.octave@gmail.com>
---
 drivers/ata/libata-scsi.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index eb2eb599e602..3436b782053d 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -2082,6 +2082,7 @@ struct ata_scsi_args {
  */
 static void *ata_scsi_rbuf_get(struct scsi_cmnd *cmd, bool copy_in,
 			       unsigned long *flags)
+	__acquires(&ata_scsi_rbuf_lock)
 {
 	spin_lock_irqsave(&ata_scsi_rbuf_lock, *flags);
 
@@ -2106,6 +2107,7 @@ static void *ata_scsi_rbuf_get(struct scsi_cmnd *cmd, bool copy_in,
  */
 static inline void ata_scsi_rbuf_put(struct scsi_cmnd *cmd, bool copy_out,
 				     unsigned long *flags)
+	__releases(&ata_scsi_rbuf_lock)
 {
 	if (copy_out)
 		sg_copy_from_buffer(scsi_sglist(cmd), scsi_sg_count(cmd),
-- 
2.24.1

[PATCH 5/5] ipmi: Add missing annotation for ipmi_ssif_lock_cond() and ipmi_ssif_unlock_cond()

[Jules Irenge]

Sparse reports a warning at ipmi_ssif_unlock_cond()
	and ipmi_ssif_lock_cond()

warning: context imbalance in ipmi_ssif_lock_cond()
	- wrong count at exit
 warning: context imbalance in ipmi_ssif_unlock_cond()
	- unexpected unlock

The root cause is the missing annotation at ipmi_ssif_unlock_cond()
	and ipmi_ssif_lock_cond()

Add the missing __acquires(&ata_scsi_rbuf_lock)
Add the missing __releases(&ata_scsi_rbuf_lock)

Signed-off-by: Jules Irenge <jbi.octave@gmail.com>
---
 drivers/char/ipmi/ipmi_ssif.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/char/ipmi/ipmi_ssif.c b/drivers/char/ipmi/ipmi_ssif.c
index 22c6a2e61236..030e7c09e44f 100644
--- a/drivers/char/ipmi/ipmi_ssif.c
+++ b/drivers/char/ipmi/ipmi_ssif.c
@@ -313,6 +313,7 @@ static int start_send(struct ssif_info *ssif_info,
 
 static unsigned long *ipmi_ssif_lock_cond(struct ssif_info *ssif_info,
 					  unsigned long *flags)
+	__acquires(&ssif_info->lock)
 {
 	spin_lock_irqsave(&ssif_info->lock, *flags);
 	return flags;
@@ -320,6 +321,7 @@ static unsigned long *ipmi_ssif_lock_cond(struct ssif_info *ssif_info,
 
 static void ipmi_ssif_unlock_cond(struct ssif_info *ssif_info,
 				  unsigned long *flags)
+	__releases(&ssif_info->lock)
 {
 	spin_unlock_irqrestore(&ssif_info->lock, *flags);
 }
-- 
2.24.1

Re: [PATCH 5/5] ipmi: Add missing annotation for ipmi_ssif_lock_cond() and ipmi_ssif_unlock_cond()

[Corey Minyard]

On Fri, Apr 03, 2020 at 05:05:05PM +0100, Jules Irenge wrote:
> Sparse reports a warning at ipmi_ssif_unlock_cond()
> 	and ipmi_ssif_lock_cond()
> 
> warning: context imbalance in ipmi_ssif_lock_cond()
> 	- wrong count at exit
>  warning: context imbalance in ipmi_ssif_unlock_cond()
> 	- unexpected unlock
> 
> The root cause is the missing annotation at ipmi_ssif_unlock_cond()
> 	and ipmi_ssif_lock_cond()
> 
> Add the missing __acquires(&ata_scsi_rbuf_lock)
> Add the missing __releases(&ata_scsi_rbuf_lock)

Yeah, this is good, I've included it in my tree.

-corey

> 
> Signed-off-by: Jules Irenge <jbi.octave@gmail.com>
> ---
>  drivers/char/ipmi/ipmi_ssif.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/char/ipmi/ipmi_ssif.c b/drivers/char/ipmi/ipmi_ssif.c
> index 22c6a2e61236..030e7c09e44f 100644
> --- a/drivers/char/ipmi/ipmi_ssif.c
> +++ b/drivers/char/ipmi/ipmi_ssif.c
> @@ -313,6 +313,7 @@ static int start_send(struct ssif_info *ssif_info,
>  
>  static unsigned long *ipmi_ssif_lock_cond(struct ssif_info *ssif_info,
>  					  unsigned long *flags)
> +	__acquires(&ssif_info->lock)
>  {
>  	spin_lock_irqsave(&ssif_info->lock, *flags);
>  	return flags;
> @@ -320,6 +321,7 @@ static unsigned long *ipmi_ssif_lock_cond(struct ssif_info *ssif_info,
>  
>  static void ipmi_ssif_unlock_cond(struct ssif_info *ssif_info,
>  				  unsigned long *flags)
> +	__releases(&ssif_info->lock)
>  {
>  	spin_unlock_irqrestore(&ssif_info->lock, *flags);
>  }
> -- 
> 2.24.1
> 

Re: [PATCH 3/5] ocfs2: Add missing annotation for dlm_empty_lockres()

[Joseph Qi]

On 2020/4/4 00:05, Jules Irenge wrote:
> Sparse reports a warning at dlm_empty_lockres()
> 
> warning: context imbalance in dlm_purge_lockres() - unexpected unlock
> 
> The root cause is the missing annotation at dlm_purge_lockres()
> 
> Add the missing __must_hold(&dlm->spinlock)
> 
Seems this is duplicate with assert_spin_locked()?

Thanks,
Joseph

> Signed-off-by: Jules Irenge <jbi.octave@gmail.com>
> ---
>  fs/ocfs2/dlm/dlmmaster.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/fs/ocfs2/dlm/dlmmaster.c b/fs/ocfs2/dlm/dlmmaster.c
> index 900f7e466d11..8cbd46b3509a 100644
> --- a/fs/ocfs2/dlm/dlmmaster.c
> +++ b/fs/ocfs2/dlm/dlmmaster.c
> @@ -2762,6 +2762,7 @@ static int dlm_migrate_lockres(struct dlm_ctxt *dlm,
>   * Returns: 1 if dlm->spinlock was dropped/retaken, 0 if never dropped
>   */
>  int dlm_empty_lockres(struct dlm_ctxt *dlm, struct dlm_lock_resource *res)
> +	__must_hold(&dlm->spinlock)
>  {
>  	int ret;
>  	int lock_dropped = 0;
> 

Re: [PATCH 2/5] video: Add missing annotation for cyber2000fb_enable_ddc() and cyber2000fb_disable_ddc()

[Sam Ravnborg]

Hi Jules.

On Fri, Apr 03, 2020 at 05:05:02PM +0100, Jules Irenge wrote:
> Sparse reports warnings at cyber2000fb_enable_ddc()
> 	and cyber2000fb_disable_ddc()
> 
> warning: context imbalance in cyber2000fb_enable_ddc()
> 	- wrong count at exit
> 
> warning: context imbalance in cyber2000fb_disable_ddc()
> 	- unexpected unlock
> 
> The root cause is the missing annotation at cyber2000fb_enable_ddc()
> 	and cyber2000fb_disable_ddc()
> 
> Add the missing __acquires(&cfb->reg_b0_lock) annotation
> Add the missing __releases(&cfb->reg_b0_lock) annotation
> 
> Signed-off-by: Jules Irenge <jbi.octave@gmail.com>

Thanks, added to drm-misc-next, so the patch will hit the kernel
when the merge windows opens the next time.

Was this the only locking relevant warning in fbdev?
I would expect a few more.

	Sam

> ---
>  drivers/video/fbdev/cyber2000fb.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/video/fbdev/cyber2000fb.c b/drivers/video/fbdev/cyber2000fb.c
> index 460826a7ad55..513f58f28b0f 100644
> --- a/drivers/video/fbdev/cyber2000fb.c
> +++ b/drivers/video/fbdev/cyber2000fb.c
> @@ -1160,12 +1160,14 @@ EXPORT_SYMBOL(cyber2000fb_detach);
>  #define DDC_SDA_IN	(1 << 6)
>  
>  static void cyber2000fb_enable_ddc(struct cfb_info *cfb)
> +	__acquires(&cfb->reg_b0_lock)
>  {
>  	spin_lock(&cfb->reg_b0_lock);
>  	cyber2000fb_writew(0x1bf, 0x3ce, cfb);
>  }
>  
>  static void cyber2000fb_disable_ddc(struct cfb_info *cfb)
> +	__releases(&cfb->reg_b0_lock)
>  {
>  	cyber2000fb_writew(0x0bf, 0x3ce, cfb);
>  	spin_unlock(&cfb->reg_b0_lock);
> -- 
> 2.24.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/5] ocfs2: Add missing annotation for dlm_empty_lockres()

[Andrew Morton]

On Sat, 4 Apr 2020 07:45:49 +0800 Joseph Qi <joseph.qi@linux.alibaba.com> wrote:

> On 2020/4/4 00:05, Jules Irenge wrote:
> > Sparse reports a warning at dlm_empty_lockres()
> > 
> > warning: context imbalance in dlm_purge_lockres() - unexpected unlock
> > 
> > The root cause is the missing annotation at dlm_purge_lockres()
> > 
> > Add the missing __must_hold(&dlm->spinlock)
> > 
> Seems this is duplicate with assert_spin_locked()?

I doubt is sparse is smart enough to treat assert_spin_locked() as
equivalent to __must_hold().  It would be neat though.