[PATCH 0/2] Fix sparse warnings in scsi_debug

[PATCH 0/2] Fix sparse warnings in scsi_debug

[Damien Le Moal]

A couple of patches to suppress sparse warnings in scsi_debug. No
functional changes.

Damien Le Moal (2):
  scsi: scsi_debug: silence sparse unexpected unlock warnings
  scsi: scsi_debug: fix sparse lock warnings in sdebug_blk_mq_poll()

 drivers/scsi/scsi_debug.c | 87 ++++++++++++++++++++++++---------------
 1 file changed, 53 insertions(+), 34 deletions(-)

-- 
2.35.1

[PATCH 1/2] scsi: scsi_debug: silence sparse unexpected unlock warnings

[Damien Le Moal]

The return statement inside the sdeb_read_lock(), sdeb_read_unlock(),
sdeb_write_lock() and sdeb_write_unlock() confuse sparse, leading to
many warnings about unexpected unlocks in the resp_xxx() functions.

Modify the lock/unlock functions using the __acquire() and __release()
inline annotations for the sdebug_no_rwlock == true case to avoid these
warnings.

Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
---
 drivers/scsi/scsi_debug.c | 68 +++++++++++++++++++++++++--------------
 1 file changed, 44 insertions(+), 24 deletions(-)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 0d25b30922ef..f4e97f2224b2 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -3167,45 +3167,65 @@ static int prot_verify_read(struct scsi_cmnd *scp, sector_t start_sec,
 static inline void
 sdeb_read_lock(struct sdeb_store_info *sip)
 {
-	if (sdebug_no_rwlock)
-		return;
-	if (sip)
-		read_lock(&sip->macc_lck);
-	else
-		read_lock(&sdeb_fake_rw_lck);
+	if (sdebug_no_rwlock) {
+		if (sip)
+			__acquire(&sip->macc_lck);
+		else
+			__acquire(&sdeb_fake_rw_lck);
+	} else {
+		if (sip)
+			read_lock(&sip->macc_lck);
+		else
+			read_lock(&sdeb_fake_rw_lck);
+	}
 }
 
 static inline void
 sdeb_read_unlock(struct sdeb_store_info *sip)
 {
-	if (sdebug_no_rwlock)
-		return;
-	if (sip)
-		read_unlock(&sip->macc_lck);
-	else
-		read_unlock(&sdeb_fake_rw_lck);
+	if (sdebug_no_rwlock) {
+		if (sip)
+			__release(&sip->macc_lck);
+		else
+			__release(&sdeb_fake_rw_lck);
+	} else {
+		if (sip)
+			read_unlock(&sip->macc_lck);
+		else
+			read_unlock(&sdeb_fake_rw_lck);
+	}
 }
 
 static inline void
 sdeb_write_lock(struct sdeb_store_info *sip)
 {
-	if (sdebug_no_rwlock)
-		return;
-	if (sip)
-		write_lock(&sip->macc_lck);
-	else
-		write_lock(&sdeb_fake_rw_lck);
+	if (sdebug_no_rwlock) {
+		if (sip)
+			__acquire(&sip->macc_lck);
+		else
+			__acquire(&sdeb_fake_rw_lck);
+	} else {
+		if (sip)
+			write_lock(&sip->macc_lck);
+		else
+			write_lock(&sdeb_fake_rw_lck);
+	}
 }
 
 static inline void
 sdeb_write_unlock(struct sdeb_store_info *sip)
 {
-	if (sdebug_no_rwlock)
-		return;
-	if (sip)
-		write_unlock(&sip->macc_lck);
-	else
-		write_unlock(&sdeb_fake_rw_lck);
+	if (sdebug_no_rwlock) {
+		if (sip)
+			__release(&sip->macc_lck);
+		else
+			__release(&sdeb_fake_rw_lck);
+	} else {
+		if (sip)
+			write_unlock(&sip->macc_lck);
+		else
+			write_unlock(&sdeb_fake_rw_lck);
+	}
 }
 
 static int resp_read_dt0(struct scsi_cmnd *scp, struct sdebug_dev_info *devip)
-- 
2.35.1

[PATCH 2/2] scsi: scsi_debug: fix sparse lock warnings in sdebug_blk_mq_poll()

[Damien Le Moal]

The use of the locked boolean variable to control locking and unlocking
of the qc_lock of struct sdebug_queue confuses sparse, leading to a
warning about an unexpected unlock. Simplify the qc_lock lock/unlock
handling code of this function to avoid this warning by removing the
locked boolean variable.

Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
---
 drivers/scsi/scsi_debug.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index f4e97f2224b2..acb32f3e38eb 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -7509,7 +7509,6 @@ static int sdebug_blk_mq_poll(struct Scsi_Host *shost, unsigned int queue_num)
 {
 	bool first;
 	bool retiring = false;
-	bool locked = false;
 	int num_entries = 0;
 	unsigned int qc_idx = 0;
 	unsigned long iflags;
@@ -7525,18 +7524,17 @@ static int sdebug_blk_mq_poll(struct Scsi_Host *shost, unsigned int queue_num)
 	if (qc_idx >= sdebug_max_queue)
 		return 0;
 
+	spin_lock_irqsave(&sqp->qc_lock, iflags);
+
 	for (first = true; first || qc_idx + 1 < sdebug_max_queue; )   {
-		if (!locked) {
-			spin_lock_irqsave(&sqp->qc_lock, iflags);
-			locked = true;
-		}
 		if (first) {
 			first = false;
 			if (!test_bit(qc_idx, sqp->in_use_bm))
 				continue;
-		} else {
-			qc_idx = find_next_bit(sqp->in_use_bm, sdebug_max_queue, qc_idx + 1);
 		}
+
+		qc_idx = find_next_bit(sqp->in_use_bm, sdebug_max_queue,
+				       qc_idx + 1);
 		if (qc_idx >= sdebug_max_queue)
 			break;
 
@@ -7586,14 +7584,15 @@ static int sdebug_blk_mq_poll(struct Scsi_Host *shost, unsigned int queue_num)
 		}
 		WRITE_ONCE(sd_dp->defer_t, SDEB_DEFER_NONE);
 		spin_unlock_irqrestore(&sqp->qc_lock, iflags);
-		locked = false;
 		scsi_done(scp); /* callback to mid level */
 		num_entries++;
+		spin_lock_irqsave(&sqp->qc_lock, iflags);
 		if (find_first_bit(sqp->in_use_bm, sdebug_max_queue) >= sdebug_max_queue)
 			break;	/* if no more then exit without retaking spinlock */
 	}
-	if (locked)
-		spin_unlock_irqrestore(&sqp->qc_lock, iflags);
+
+	spin_unlock_irqrestore(&sqp->qc_lock, iflags);
+
 	if (num_entries > 0)
 		atomic_add(num_entries, &sdeb_mq_poll_count);
 	return num_entries;
-- 
2.35.1

Re: [PATCH 1/2] scsi: scsi_debug: silence sparse unexpected unlock warnings

[Douglas Gilbert]

On 2022-02-25 03:45, Damien Le Moal wrote:
> The return statement inside the sdeb_read_lock(), sdeb_read_unlock(),
> sdeb_write_lock() and sdeb_write_unlock() confuse sparse, leading to
> many warnings about unexpected unlocks in the resp_xxx() functions.
> 
> Modify the lock/unlock functions using the __acquire() and __release()
> inline annotations for the sdebug_no_rwlock == true case to avoid these
> warnings.

I'm confused. The idea with sdebug_no_rwlock was that the application
may know that the protection afforded by the driver's rwlock is not
needed because locking is performed at a higher level (e.g. in the
user space). Hence there is no need to use a read-write lock (or a
full lock) in this driver to protect a read (say) against a co-incident
write to the same memory region. So this was a performance enhancement.

The proposed patch seems to be replacing a read-write lock with a full
lock. That would be going in the opposite direction to what I intended.

If this is the only solution, a better idea might be to drop the
patch (in staging I think) that introduced the sdebug_no_rwlock option.

The sdebug_no_rwlock option has been pretty thoroughly tested (for over
a year) with memory to memory transfers (together with sgl to sgl
additions to lib/scatterlist.h). Haven't seen any unexplained crashes
that I could trace to this lack of locking. OTOH I haven't measured
any improvement of the copy speed either, that may be because my tests
are approaching the copy bandwidth of the ram.


Does sparse understand guard variables (e.g. like 'bool lock_taken')?
 From what I've seen with sg3_utils Coverity doesn't, leading to many false
reports.

Doug Gilbert

> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> ---
>   drivers/scsi/scsi_debug.c | 68 +++++++++++++++++++++++++--------------
>   1 file changed, 44 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
> index 0d25b30922ef..f4e97f2224b2 100644
> --- a/drivers/scsi/scsi_debug.c
> +++ b/drivers/scsi/scsi_debug.c
> @@ -3167,45 +3167,65 @@ static int prot_verify_read(struct scsi_cmnd *scp, sector_t start_sec,
>   static inline void
>   sdeb_read_lock(struct sdeb_store_info *sip)
>   {
> -	if (sdebug_no_rwlock)
> -		return;
> -	if (sip)
> -		read_lock(&sip->macc_lck);
> -	else
> -		read_lock(&sdeb_fake_rw_lck);
> +	if (sdebug_no_rwlock) {
> +		if (sip)
> +			__acquire(&sip->macc_lck);
> +		else
> +			__acquire(&sdeb_fake_rw_lck);
> +	} else {
> +		if (sip)
> +			read_lock(&sip->macc_lck);
> +		else
> +			read_lock(&sdeb_fake_rw_lck);
> +	}
>   }
>   
>   static inline void
>   sdeb_read_unlock(struct sdeb_store_info *sip)
>   {
> -	if (sdebug_no_rwlock)
> -		return;
> -	if (sip)
> -		read_unlock(&sip->macc_lck);
> -	else
> -		read_unlock(&sdeb_fake_rw_lck);
> +	if (sdebug_no_rwlock) {
> +		if (sip)
> +			__release(&sip->macc_lck);
> +		else
> +			__release(&sdeb_fake_rw_lck);
> +	} else {
> +		if (sip)
> +			read_unlock(&sip->macc_lck);
> +		else
> +			read_unlock(&sdeb_fake_rw_lck);
> +	}
>   }
>   
>   static inline void
>   sdeb_write_lock(struct sdeb_store_info *sip)
>   {
> -	if (sdebug_no_rwlock)
> -		return;
> -	if (sip)
> -		write_lock(&sip->macc_lck);
> -	else
> -		write_lock(&sdeb_fake_rw_lck);
> +	if (sdebug_no_rwlock) {
> +		if (sip)
> +			__acquire(&sip->macc_lck);
> +		else
> +			__acquire(&sdeb_fake_rw_lck);
> +	} else {
> +		if (sip)
> +			write_lock(&sip->macc_lck);
> +		else
> +			write_lock(&sdeb_fake_rw_lck);
> +	}
>   }
>   
>   static inline void
>   sdeb_write_unlock(struct sdeb_store_info *sip)
>   {
> -	if (sdebug_no_rwlock)
> -		return;
> -	if (sip)
> -		write_unlock(&sip->macc_lck);
> -	else
> -		write_unlock(&sdeb_fake_rw_lck);
> +	if (sdebug_no_rwlock) {
> +		if (sip)
> +			__release(&sip->macc_lck);
> +		else
> +			__release(&sdeb_fake_rw_lck);
> +	} else {
> +		if (sip)
> +			write_unlock(&sip->macc_lck);
> +		else
> +			write_unlock(&sdeb_fake_rw_lck);
> +	}
>   }
>   
>   static int resp_read_dt0(struct scsi_cmnd *scp, struct sdebug_dev_info *devip)

Re: [PATCH 2/2] scsi: scsi_debug: fix sparse lock warnings in sdebug_blk_mq_poll()

[Douglas Gilbert]

On 2022-02-25 03:45, Damien Le Moal wrote:
> The use of the locked boolean variable to control locking and unlocking
> of the qc_lock of struct sdebug_queue confuses sparse, leading to a
> warning about an unexpected unlock. Simplify the qc_lock lock/unlock
> handling code of this function to avoid this warning by removing the
> locked boolean variable.

See below.

> 
> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> ---
>   drivers/scsi/scsi_debug.c | 19 +++++++++----------
>   1 file changed, 9 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
> index f4e97f2224b2..acb32f3e38eb 100644
> --- a/drivers/scsi/scsi_debug.c
> +++ b/drivers/scsi/scsi_debug.c
> @@ -7509,7 +7509,6 @@ static int sdebug_blk_mq_poll(struct Scsi_Host *shost, unsigned int queue_num)
>   {
>   	bool first;
>   	bool retiring = false;
> -	bool locked = false;
>   	int num_entries = 0;
>   	unsigned int qc_idx = 0;
>   	unsigned long iflags;
> @@ -7525,18 +7524,17 @@ static int sdebug_blk_mq_poll(struct Scsi_Host *shost, unsigned int queue_num)
>   	if (qc_idx >= sdebug_max_queue)
>   		return 0;
>   
> +	spin_lock_irqsave(&sqp->qc_lock, iflags);
> +
>   	for (first = true; first || qc_idx + 1 < sdebug_max_queue; )   {
> -		if (!locked) {
> -			spin_lock_irqsave(&sqp->qc_lock, iflags);
> -			locked = true;
> -		}
>   		if (first) {
>   			first = false;
>   			if (!test_bit(qc_idx, sqp->in_use_bm))
>   				continue;
> -		} else {
> -			qc_idx = find_next_bit(sqp->in_use_bm, sdebug_max_queue, qc_idx + 1);
>   		}
> +
> +		qc_idx = find_next_bit(sqp->in_use_bm, sdebug_max_queue,
> +				       qc_idx + 1);

The original logic is wrong or the above line is wrong. find_next_bit() is not
called on the first iteration in the original, but it is with this patch.

>   		if (qc_idx >= sdebug_max_queue)
>   			break;
>   
> @@ -7586,14 +7584,15 @@ static int sdebug_blk_mq_poll(struct Scsi_Host *shost, unsigned int queue_num)
>   		}
>   		WRITE_ONCE(sd_dp->defer_t, SDEB_DEFER_NONE);
>   		spin_unlock_irqrestore(&sqp->qc_lock, iflags);
> -		locked = false;
>   		scsi_done(scp); /* callback to mid level */
>   		num_entries++;
> +		spin_lock_irqsave(&sqp->qc_lock, iflags);
>   		if (find_first_bit(sqp->in_use_bm, sdebug_max_queue) >= sdebug_max_queue)
>   			break;	/* if no more then exit without retaking spinlock */

See that comment on the line above? That is the reason for the guard variable.
Defying that comment, the modified code does a superfluous spinlock irqsave
and irqrestore.

Sparse could be taken as a comment on the amount of grey matter that tool has.


>   	}
> -	if (locked)
> -		spin_unlock_irqrestore(&sqp->qc_lock, iflags);
> +
> +	spin_unlock_irqrestore(&sqp->qc_lock, iflags);
> +
>   	if (num_entries > 0)
>   		atomic_add(num_entries, &sdeb_mq_poll_count);
>   	return num_entries;

Locking issues are extremely difficult to analyze via a unified diff of
the function. A copy of the original function is required to make any
sense of it.

Doug Gilbert

Re: [PATCH 1/2] scsi: scsi_debug: silence sparse unexpected unlock warnings

[Damien Le Moal]

On 2022/02/28 3:39, Douglas Gilbert wrote:
> On 2022-02-25 03:45, Damien Le Moal wrote:
>> The return statement inside the sdeb_read_lock(), sdeb_read_unlock(),
>> sdeb_write_lock() and sdeb_write_unlock() confuse sparse, leading to
>> many warnings about unexpected unlocks in the resp_xxx() functions.
>>
>> Modify the lock/unlock functions using the __acquire() and __release()
>> inline annotations for the sdebug_no_rwlock == true case to avoid these
>> warnings.
> 
> I'm confused. The idea with sdebug_no_rwlock was that the application
> may know that the protection afforded by the driver's rwlock is not
> needed because locking is performed at a higher level (e.g. in the
> user space). Hence there is no need to use a read-write lock (or a
> full lock) in this driver to protect a read (say) against a co-incident
> write to the same memory region. So this was a performance enhancement.
> 
> The proposed patch seems to be replacing a read-write lock with a full
> lock. That would be going in the opposite direction to what I intended.

Not at all. The __acquire() and __release() calls are not locking functions.
They are annotations for sparse so that we get a correct +/-1 counting of the
lock/unlock calls. So there is no functional change here and no overhead added
when compiling without C=1 since these macros disappear without sparse.

> 
> If this is the only solution, a better idea might be to drop the
> patch (in staging I think) that introduced the sdebug_no_rwlock option.
> 
> The sdebug_no_rwlock option has been pretty thoroughly tested (for over
> a year) with memory to memory transfers (together with sgl to sgl
> additions to lib/scatterlist.h). Haven't seen any unexplained crashes
> that I could trace to this lack of locking. OTOH I haven't measured
> any improvement of the copy speed either, that may be because my tests
> are approaching the copy bandwidth of the ram.
> 
> 
> Does sparse understand guard variables (e.g. like 'bool lock_taken')?
>  From what I've seen with sg3_utils Coverity doesn't, leading to many false
> reports.
> 
> Doug Gilbert
> 
>> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
>> ---
>>   drivers/scsi/scsi_debug.c | 68 +++++++++++++++++++++++++--------------
>>   1 file changed, 44 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
>> index 0d25b30922ef..f4e97f2224b2 100644
>> --- a/drivers/scsi/scsi_debug.c
>> +++ b/drivers/scsi/scsi_debug.c
>> @@ -3167,45 +3167,65 @@ static int prot_verify_read(struct scsi_cmnd *scp, sector_t start_sec,
>>   static inline void
>>   sdeb_read_lock(struct sdeb_store_info *sip)
>>   {
>> -	if (sdebug_no_rwlock)
>> -		return;
>> -	if (sip)
>> -		read_lock(&sip->macc_lck);
>> -	else
>> -		read_lock(&sdeb_fake_rw_lck);
>> +	if (sdebug_no_rwlock) {
>> +		if (sip)
>> +			__acquire(&sip->macc_lck);
>> +		else
>> +			__acquire(&sdeb_fake_rw_lck);
>> +	} else {
>> +		if (sip)
>> +			read_lock(&sip->macc_lck);
>> +		else
>> +			read_lock(&sdeb_fake_rw_lck);
>> +	}
>>   }
>>   
>>   static inline void
>>   sdeb_read_unlock(struct sdeb_store_info *sip)
>>   {
>> -	if (sdebug_no_rwlock)
>> -		return;
>> -	if (sip)
>> -		read_unlock(&sip->macc_lck);
>> -	else
>> -		read_unlock(&sdeb_fake_rw_lck);
>> +	if (sdebug_no_rwlock) {
>> +		if (sip)
>> +			__release(&sip->macc_lck);
>> +		else
>> +			__release(&sdeb_fake_rw_lck);
>> +	} else {
>> +		if (sip)
>> +			read_unlock(&sip->macc_lck);
>> +		else
>> +			read_unlock(&sdeb_fake_rw_lck);
>> +	}
>>   }
>>   
>>   static inline void
>>   sdeb_write_lock(struct sdeb_store_info *sip)
>>   {
>> -	if (sdebug_no_rwlock)
>> -		return;
>> -	if (sip)
>> -		write_lock(&sip->macc_lck);
>> -	else
>> -		write_lock(&sdeb_fake_rw_lck);
>> +	if (sdebug_no_rwlock) {
>> +		if (sip)
>> +			__acquire(&sip->macc_lck);
>> +		else
>> +			__acquire(&sdeb_fake_rw_lck);
>> +	} else {
>> +		if (sip)
>> +			write_lock(&sip->macc_lck);
>> +		else
>> +			write_lock(&sdeb_fake_rw_lck);
>> +	}
>>   }
>>   
>>   static inline void
>>   sdeb_write_unlock(struct sdeb_store_info *sip)
>>   {
>> -	if (sdebug_no_rwlock)
>> -		return;
>> -	if (sip)
>> -		write_unlock(&sip->macc_lck);
>> -	else
>> -		write_unlock(&sdeb_fake_rw_lck);
>> +	if (sdebug_no_rwlock) {
>> +		if (sip)
>> +			__release(&sip->macc_lck);
>> +		else
>> +			__release(&sdeb_fake_rw_lck);
>> +	} else {
>> +		if (sip)
>> +			write_unlock(&sip->macc_lck);
>> +		else
>> +			write_unlock(&sdeb_fake_rw_lck);
>> +	}
>>   }
>>   
>>   static int resp_read_dt0(struct scsi_cmnd *scp, struct sdebug_dev_info *devip)
> 


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 2/2] scsi: scsi_debug: fix sparse lock warnings in sdebug_blk_mq_poll()

[Damien Le Moal]

On 2022/02/28 4:05, Douglas Gilbert wrote:
> On 2022-02-25 03:45, Damien Le Moal wrote:
>> The use of the locked boolean variable to control locking and unlocking
>> of the qc_lock of struct sdebug_queue confuses sparse, leading to a
>> warning about an unexpected unlock. Simplify the qc_lock lock/unlock
>> handling code of this function to avoid this warning by removing the
>> locked boolean variable.
> 
> See below.
> 
>>
>> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
>> ---
>>   drivers/scsi/scsi_debug.c | 19 +++++++++----------
>>   1 file changed, 9 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
>> index f4e97f2224b2..acb32f3e38eb 100644
>> --- a/drivers/scsi/scsi_debug.c
>> +++ b/drivers/scsi/scsi_debug.c
>> @@ -7509,7 +7509,6 @@ static int sdebug_blk_mq_poll(struct Scsi_Host *shost, unsigned int queue_num)
>>   {
>>   	bool first;
>>   	bool retiring = false;
>> -	bool locked = false;
>>   	int num_entries = 0;
>>   	unsigned int qc_idx = 0;
>>   	unsigned long iflags;
>> @@ -7525,18 +7524,17 @@ static int sdebug_blk_mq_poll(struct Scsi_Host *shost, unsigned int queue_num)
>>   	if (qc_idx >= sdebug_max_queue)
>>   		return 0;
>>   
>> +	spin_lock_irqsave(&sqp->qc_lock, iflags);
>> +
>>   	for (first = true; first || qc_idx + 1 < sdebug_max_queue; )   {
>> -		if (!locked) {
>> -			spin_lock_irqsave(&sqp->qc_lock, iflags);
>> -			locked = true;
>> -		}
>>   		if (first) {
>>   			first = false;
>>   			if (!test_bit(qc_idx, sqp->in_use_bm))
>>   				continue;
>> -		} else {
>> -			qc_idx = find_next_bit(sqp->in_use_bm, sdebug_max_queue, qc_idx + 1);
>>   		}
>> +
>> +		qc_idx = find_next_bit(sqp->in_use_bm, sdebug_max_queue,
>> +				       qc_idx + 1);
> 
> The original logic is wrong or the above line is wrong. find_next_bit() is not
> called on the first iteration in the original, but it is with this patch.

Good catch. Indeed I should not have touched that. Will fix this in V2.

> 
>>   		if (qc_idx >= sdebug_max_queue)
>>   			break;
>>   
>> @@ -7586,14 +7584,15 @@ static int sdebug_blk_mq_poll(struct Scsi_Host *shost, unsigned int queue_num)
>>   		}
>>   		WRITE_ONCE(sd_dp->defer_t, SDEB_DEFER_NONE);
>>   		spin_unlock_irqrestore(&sqp->qc_lock, iflags);
>> -		locked = false;
>>   		scsi_done(scp); /* callback to mid level */
>>   		num_entries++;
>> +		spin_lock_irqsave(&sqp->qc_lock, iflags);
>>   		if (find_first_bit(sqp->in_use_bm, sdebug_max_queue) >= sdebug_max_queue)
>>   			break;	/* if no more then exit without retaking spinlock */
> 
> See that comment on the line above? That is the reason for the guard variable.
> Defying that comment, the modified code does a superfluous spinlock irqsave
> and irqrestore.

It does, and I should have removed the comment. Without this extra lock/unlock,
I could not find a way to shut-up sparse. Do you think this is too much of an
overhead ?

> Sparse could be taken as a comment on the amount of grey matter that tool has.

When it comes to conditional locking, most of the time, sparse does not get it
and generates warnings. In this case, I tried the acquire/release annotations,
but the loop+locked variable are too much for sparse.

I really would like to suppress *all* sparse warnings in scsi. There are way too
many right now, which makes this tool nearly useless as new warnings get drowned
in the sea of old ones...

> 
> 
>>   	}
>> -	if (locked)
>> -		spin_unlock_irqrestore(&sqp->qc_lock, iflags);
>> +
>> +	spin_unlock_irqrestore(&sqp->qc_lock, iflags);
>> +
>>   	if (num_entries > 0)
>>   		atomic_add(num_entries, &sdeb_mq_poll_count);
>>   	return num_entries;
> 
> Locking issues are extremely difficult to analyze via a unified diff of
> the function. A copy of the original function is required to make any
> sense of it.

Sorry, but I do not understand what you are trying to say here...

> 
> Doug Gilbert
> 
> 


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 2/2] scsi: scsi_debug: fix sparse lock warnings in sdebug_blk_mq_poll()

[Damien Le Moal]

On 2022/02/28 4:05, Douglas Gilbert wrote:
> On 2022-02-25 03:45, Damien Le Moal wrote:
>> The use of the locked boolean variable to control locking and unlocking
>> of the qc_lock of struct sdebug_queue confuses sparse, leading to a
>> warning about an unexpected unlock. Simplify the qc_lock lock/unlock
>> handling code of this function to avoid this warning by removing the
>> locked boolean variable.
> 
> See below.
> 
>>
>> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
>> ---
>>   drivers/scsi/scsi_debug.c | 19 +++++++++----------
>>   1 file changed, 9 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
>> index f4e97f2224b2..acb32f3e38eb 100644
>> --- a/drivers/scsi/scsi_debug.c
>> +++ b/drivers/scsi/scsi_debug.c
>> @@ -7509,7 +7509,6 @@ static int sdebug_blk_mq_poll(struct Scsi_Host *shost, unsigned int queue_num)
>>   {
>>   	bool first;
>>   	bool retiring = false;
>> -	bool locked = false;
>>   	int num_entries = 0;
>>   	unsigned int qc_idx = 0;
>>   	unsigned long iflags;
>> @@ -7525,18 +7524,17 @@ static int sdebug_blk_mq_poll(struct Scsi_Host *shost, unsigned int queue_num)
>>   	if (qc_idx >= sdebug_max_queue)
>>   		return 0;
>>   
>> +	spin_lock_irqsave(&sqp->qc_lock, iflags);
>> +
>>   	for (first = true; first || qc_idx + 1 < sdebug_max_queue; )   {
>> -		if (!locked) {
>> -			spin_lock_irqsave(&sqp->qc_lock, iflags);
>> -			locked = true;
>> -		}
>>   		if (first) {
>>   			first = false;
>>   			if (!test_bit(qc_idx, sqp->in_use_bm))
>>   				continue;
>> -		} else {
>> -			qc_idx = find_next_bit(sqp->in_use_bm, sdebug_max_queue, qc_idx + 1);
>>   		}
>> +
>> +		qc_idx = find_next_bit(sqp->in_use_bm, sdebug_max_queue,
>> +				       qc_idx + 1);
> 
> The original logic is wrong or the above line is wrong. find_next_bit() is not
> called on the first iteration in the original, but it is with this patch.
> 
>>   		if (qc_idx >= sdebug_max_queue)
>>   			break;
>>   
>> @@ -7586,14 +7584,15 @@ static int sdebug_blk_mq_poll(struct Scsi_Host *shost, unsigned int queue_num)
>>   		}
>>   		WRITE_ONCE(sd_dp->defer_t, SDEB_DEFER_NONE);
>>   		spin_unlock_irqrestore(&sqp->qc_lock, iflags);
>> -		locked = false;
>>   		scsi_done(scp); /* callback to mid level */
>>   		num_entries++;
>> +		spin_lock_irqsave(&sqp->qc_lock, iflags);
>>   		if (find_first_bit(sqp->in_use_bm, sdebug_max_queue) >= sdebug_max_queue)
>>   			break;	/* if no more then exit without retaking spinlock */
> 
> See that comment on the line above? That is the reason for the guard variable.
> Defying that comment, the modified code does a superfluous spinlock irqsave
> and irqrestore.

Rechecking this, there is one point that is bothering me: is it OK to have the
find_first_bit() outside of the sqp lock ? If not, then this is a bug and the
extra lock/unlock that my patch add is a fix...

> 
> Sparse could be taken as a comment on the amount of grey matter that tool has.
> 
> 
>>   	}
>> -	if (locked)
>> -		spin_unlock_irqrestore(&sqp->qc_lock, iflags);
>> +
>> +	spin_unlock_irqrestore(&sqp->qc_lock, iflags);
>> +
>>   	if (num_entries > 0)
>>   		atomic_add(num_entries, &sdeb_mq_poll_count);
>>   	return num_entries;
> 
> Locking issues are extremely difficult to analyze via a unified diff of
> the function. A copy of the original function is required to make any
> sense of it.
> 
> Doug Gilbert
> 
> 


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 1/2] scsi: scsi_debug: silence sparse unexpected unlock warnings

[Douglas Gilbert]

On 2022-02-28 01:58, Damien Le Moal wrote:
> On 2022/02/28 3:39, Douglas Gilbert wrote:
>> On 2022-02-25 03:45, Damien Le Moal wrote:
>>> The return statement inside the sdeb_read_lock(), sdeb_read_unlock(),
>>> sdeb_write_lock() and sdeb_write_unlock() confuse sparse, leading to
>>> many warnings about unexpected unlocks in the resp_xxx() functions.
>>>
>>> Modify the lock/unlock functions using the __acquire() and __release()
>>> inline annotations for the sdebug_no_rwlock == true case to avoid these
>>> warnings.
>>
>> I'm confused. The idea with sdebug_no_rwlock was that the application
>> may know that the protection afforded by the driver's rwlock is not
>> needed because locking is performed at a higher level (e.g. in the
>> user space). Hence there is no need to use a read-write lock (or a
>> full lock) in this driver to protect a read (say) against a co-incident
>> write to the same memory region. So this was a performance enhancement.
>>
>> The proposed patch seems to be replacing a read-write lock with a full
>> lock. That would be going in the opposite direction to what I intended.
> 
> Not at all. The __acquire() and __release() calls are not locking functions.
> They are annotations for sparse so that we get a correct +/-1 counting of the
> lock/unlock calls. So there is no functional change here and no overhead added
> when compiling without C=1 since these macros disappear without sparse.

Grrr. If those functions are dummies then I think it would be
reasonable if their names had a word like "fake" or "dummy" in
them.

That being the case:
Acked-by: Douglas Gilbert <dgilbert@interlog.com>


Note: these patches should probably be against Martin's 5.18/scsi-staging
       tree as he has taken 5 or 6 of my scsi_debug patches in this cycle.

> 
>>
>> If this is the only solution, a better idea might be to drop the
>> patch (in staging I think) that introduced the sdebug_no_rwlock option.
>>
>> The sdebug_no_rwlock option has been pretty thoroughly tested (for over
>> a year) with memory to memory transfers (together with sgl to sgl
>> additions to lib/scatterlist.h). Haven't seen any unexplained crashes
>> that I could trace to this lack of locking. OTOH I haven't measured
>> any improvement of the copy speed either, that may be because my tests
>> are approaching the copy bandwidth of the ram.
>>
>>
>> Does sparse understand guard variables (e.g. like 'bool lock_taken')?
>>   From what I've seen with sg3_utils Coverity doesn't, leading to many false
>> reports.
>>
>> Doug Gilbert
>>
>>> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
>>> ---
>>>    drivers/scsi/scsi_debug.c | 68 +++++++++++++++++++++++++--------------
>>>    1 file changed, 44 insertions(+), 24 deletions(-)
>>>
>>> diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
>>> index 0d25b30922ef..f4e97f2224b2 100644
>>> --- a/drivers/scsi/scsi_debug.c
>>> +++ b/drivers/scsi/scsi_debug.c
>>> @@ -3167,45 +3167,65 @@ static int prot_verify_read(struct scsi_cmnd *scp, sector_t start_sec,
>>>    static inline void
>>>    sdeb_read_lock(struct sdeb_store_info *sip)
>>>    {
>>> -	if (sdebug_no_rwlock)
>>> -		return;
>>> -	if (sip)
>>> -		read_lock(&sip->macc_lck);
>>> -	else
>>> -		read_lock(&sdeb_fake_rw_lck);
>>> +	if (sdebug_no_rwlock) {
>>> +		if (sip)
>>> +			__acquire(&sip->macc_lck);
>>> +		else
>>> +			__acquire(&sdeb_fake_rw_lck);
>>> +	} else {
>>> +		if (sip)
>>> +			read_lock(&sip->macc_lck);
>>> +		else
>>> +			read_lock(&sdeb_fake_rw_lck);
>>> +	}
>>>    }
>>>    
>>>    static inline void
>>>    sdeb_read_unlock(struct sdeb_store_info *sip)
>>>    {
>>> -	if (sdebug_no_rwlock)
>>> -		return;
>>> -	if (sip)
>>> -		read_unlock(&sip->macc_lck);
>>> -	else
>>> -		read_unlock(&sdeb_fake_rw_lck);
>>> +	if (sdebug_no_rwlock) {
>>> +		if (sip)
>>> +			__release(&sip->macc_lck);
>>> +		else
>>> +			__release(&sdeb_fake_rw_lck);
>>> +	} else {
>>> +		if (sip)
>>> +			read_unlock(&sip->macc_lck);
>>> +		else
>>> +			read_unlock(&sdeb_fake_rw_lck);
>>> +	}
>>>    }
>>>    
>>>    static inline void
>>>    sdeb_write_lock(struct sdeb_store_info *sip)
>>>    {
>>> -	if (sdebug_no_rwlock)
>>> -		return;
>>> -	if (sip)
>>> -		write_lock(&sip->macc_lck);
>>> -	else
>>> -		write_lock(&sdeb_fake_rw_lck);
>>> +	if (sdebug_no_rwlock) {
>>> +		if (sip)
>>> +			__acquire(&sip->macc_lck);
>>> +		else
>>> +			__acquire(&sdeb_fake_rw_lck);
>>> +	} else {
>>> +		if (sip)
>>> +			write_lock(&sip->macc_lck);
>>> +		else
>>> +			write_lock(&sdeb_fake_rw_lck);
>>> +	}
>>>    }
>>>    
>>>    static inline void
>>>    sdeb_write_unlock(struct sdeb_store_info *sip)
>>>    {
>>> -	if (sdebug_no_rwlock)
>>> -		return;
>>> -	if (sip)
>>> -		write_unlock(&sip->macc_lck);
>>> -	else
>>> -		write_unlock(&sdeb_fake_rw_lck);
>>> +	if (sdebug_no_rwlock) {
>>> +		if (sip)
>>> +			__release(&sip->macc_lck);
>>> +		else
>>> +			__release(&sdeb_fake_rw_lck);
>>> +	} else {
>>> +		if (sip)
>>> +			write_unlock(&sip->macc_lck);
>>> +		else
>>> +			write_unlock(&sdeb_fake_rw_lck);
>>> +	}
>>>    }
>>>    
>>>    static int resp_read_dt0(struct scsi_cmnd *scp, struct sdebug_dev_info *devip)
>>
> 
> 

Re: [PATCH 2/2] scsi: scsi_debug: fix sparse lock warnings in sdebug_blk_mq_poll()

[Douglas Gilbert]

See below:

On 2022-02-28 08:46, Damien Le Moal wrote:
> On 2022/02/28 4:05, Douglas Gilbert wrote:
>> On 2022-02-25 03:45, Damien Le Moal wrote:
>>> The use of the locked boolean variable to control locking and unlocking
>>> of the qc_lock of struct sdebug_queue confuses sparse, leading to a
>>> warning about an unexpected unlock. Simplify the qc_lock lock/unlock
>>> handling code of this function to avoid this warning by removing the
>>> locked boolean variable.
>>
>> See below.
>>
>>>
>>> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
>>> ---
>>>    drivers/scsi/scsi_debug.c | 19 +++++++++----------
>>>    1 file changed, 9 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
>>> index f4e97f2224b2..acb32f3e38eb 100644
>>> --- a/drivers/scsi/scsi_debug.c
>>> +++ b/drivers/scsi/scsi_debug.c
>>> @@ -7509,7 +7509,6 @@ static int sdebug_blk_mq_poll(struct Scsi_Host *shost, unsigned int queue_num)
>>>    {
>>>    	bool first;
>>>    	bool retiring = false;
>>> -	bool locked = false;
>>>    	int num_entries = 0;
>>>    	unsigned int qc_idx = 0;
>>>    	unsigned long iflags;
>>> @@ -7525,18 +7524,17 @@ static int sdebug_blk_mq_poll(struct Scsi_Host *shost, unsigned int queue_num)
>>>    	if (qc_idx >= sdebug_max_queue)
>>>    		return 0;
>>>    
>>> +	spin_lock_irqsave(&sqp->qc_lock, iflags);
>>> +
>>>    	for (first = true; first || qc_idx + 1 < sdebug_max_queue; )   {
>>> -		if (!locked) {
>>> -			spin_lock_irqsave(&sqp->qc_lock, iflags);
>>> -			locked = true;
>>> -		}
>>>    		if (first) {
>>>    			first = false;
>>>    			if (!test_bit(qc_idx, sqp->in_use_bm))
>>>    				continue;
>>> -		} else {
>>> -			qc_idx = find_next_bit(sqp->in_use_bm, sdebug_max_queue, qc_idx + 1);
>>>    		}
>>> +
>>> +		qc_idx = find_next_bit(sqp->in_use_bm, sdebug_max_queue,
>>> +				       qc_idx + 1);
>>
>> The original logic is wrong or the above line is wrong. find_next_bit() is not
>> called on the first iteration in the original, but it is with this patch.
>>
>>>    		if (qc_idx >= sdebug_max_queue)
>>>    			break;
>>>    
>>> @@ -7586,14 +7584,15 @@ static int sdebug_blk_mq_poll(struct Scsi_Host *shost, unsigned int queue_num)
>>>    		}
>>>    		WRITE_ONCE(sd_dp->defer_t, SDEB_DEFER_NONE);
>>>    		spin_unlock_irqrestore(&sqp->qc_lock, iflags);
>>> -		locked = false;
>>>    		scsi_done(scp); /* callback to mid level */
>>>    		num_entries++;
>>> +		spin_lock_irqsave(&sqp->qc_lock, iflags);
>>>    		if (find_first_bit(sqp->in_use_bm, sdebug_max_queue) >= sdebug_max_queue)
>>>    			break;	/* if no more then exit without retaking spinlock */
>>
>> See that comment on the line above? That is the reason for the guard variable.
>> Defying that comment, the modified code does a superfluous spinlock irqsave
>> and irqrestore.
> 
> Rechecking this, there is one point that is bothering me: is it OK to have the
> find_first_bit() outside of the sqp lock ? If not, then this is a bug and the
> extra lock/unlock that my patch add is a fix...

I think you are correct, please fix it.
You will notice that when the spinlock_irq is dropped to call scsi_done(),
that the iteration is restarted.

>>
>> Sparse could be taken as a comment on the amount of grey matter that tool has.
>>
>>
>>>    	}
>>> -	if (locked)
>>> -		spin_unlock_irqrestore(&sqp->qc_lock, iflags);
>>> +
>>> +	spin_unlock_irqrestore(&sqp->qc_lock, iflags);
>>> +
>>>    	if (num_entries > 0)
>>>    		atomic_add(num_entries, &sdeb_mq_poll_count);
>>>    	return num_entries;
>>
>> Locking issues are extremely difficult to analyze via a unified diff of
>> the function. A copy of the original function is required to make any
>> sense of it.

I was trying to say: it is difficult to understand what diff style output
of a change as shown in a [PATCH] post like this, especially to a function's
locking, will do, without see the __whole__ function.

It is not a criticism of this patchset, but the process in general which loses
important context of the function being patched.