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

[Douglas Gilbert <dgilbert@interlog.com>]

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