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.
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
