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

[Douglas Gilbert <dgilbert@interlog.com>]

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.