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@xxxxxxxxxxxxxxxxxx>
>> ---
>> 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
