Brian:
Thank you for your review. Comments are inline.
- Manoj
On 6/8/2015 12:54 PM, Brian King wrote:
Looking pretty good. A few more comments.
Thanks,
Brian
+ spin_lock_irqsave(&cfg->tmf_waitq.lock, lock_flags);
+ if (cfg->tmf_active)
+ wait_event_interruptible_locked_irq(cfg->tmf_waitq,
+ !cfg->tmf_active);
+ spin_unlock_irqrestore(&cfg->tmf_waitq.lock, lock_flags);
This needs to return SCSI_MLQUEUE_HOST_BUSY instead of sleeping. You can't sleep
in queuecommand.
Okay, will revise in v5 to return an error instead of sleeping.
+ if (atomic64_dec_and_test(&afu->room)) {
If you have two threads executing this code concurrently you could have a problem.
If afu->room = 1 and thread 1 decrements it and the return value is 0, we go into this
leg. If a second thread then comes in right after afu->room goes to zero, afu->room
will then get decremented to -1, and you'll send the command, regardless of whether
the AFU has room or not. Either the AFU will have room and afu->room will then
end up being off by one, or it won't have room and you'll send a command when it
does not have room.
I think if you use atomic_dec_if_positive instead, you can get rid of this race condition.
You'd then need to check the return value. If its positive, there is room, if it zero,
you are out of room and you are the thread that will reset afu->room from the AFU. If
it is negative, then you have to either return host busy, or wait for the other thread to
reset afu->room and simply try the atomic_dec_if_positive again in the loop here
instead of reading from the adapter and trying to set it from two threads.
Good catch. Will switch to atomic_dec_if_positive() in v5 to avoid the race.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
