Re: [PATCH v4] cxlflash: Base support for IBM CXL Flash Adapter

[Manoj Kumar <manoj@linux.vnet.ibm.com>]

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.