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