RE: [PATCH v4 2/2] ufs: mcq: poll OCS after sq cleanup for device management commands

["Kiwoong Kim" <kwmad.kim@samsung.com>]

> > +static enum utp_ocs ufshcd_mcq_read_ocs(struct ufs_hw_queue *hwq,
> > +					struct ufshcd_lrb *lrbp)
> > +{
> > +	guard(spinlock_irqsave)(&hwq->cq_lock);
> > +	return READ_ONCE(lrbp->utr_descriptor_ptr->header.ocs);
> > +}
> 
> READ_ONCE() is sufficient. I don't think that the spinlock helps here.
> Additionally, if the spinlock operations are left out, the function
> ufshcd_mcq_read_ocs() is no longer necessary and can be inlined in its
> only caller.
> 

It seems like your proposal works in terms of reading OCS,
but I guess there could be a race at a nearly same time among different CPUs
because UFS driver uses the same tag # for device commands.

Of course, it would take CPU C long to reach up to clearing cmd->state,
so that what the timeline shows will be unlikely to happen, I think.
I was just concerned about the possibility.

Anyway, let me take your proposal to revise this.


WITHOUT cq_lock

 CPU A: ISR                      CPU B: Error handler     CPU C: IO thread
 ----------                      ---------------           -------------------
 set OCS 
                                   READ_ONCE sees OCS
                                                                scsi_prepare_cmd()
                                                                -> clears cmd->state

 scsi_done(cmd)
 -> cmd->state is set to SCMD_STATE_COMPLETE and test_and_set_bit returns non zero
(cmd->state is already set when the cmd is timed-out)
 -> blk_mq_complete_request called (Unexpected)


WITH cq_lock

 CPU A: ISR                      CPU B: Error handler     CPU C: IO thread
 --------------------------   ---------------           -------------------
 set OCS
                                   lock(cq_lock) ...waits...
 scsi_done(cmd)
 -> cmd->state is set to SCMD_STATE_COMPLETE and test_and_set_bit returns 0
 -> just return
 unlock(cq_lock) 
                                   lock acquired
                                   READ_ONCE sees OCS
                                                                scsi_prepare_cmd()
                                                                 -> clears cmd->state

> > +		err = read_poll_timeout(ufshcd_mcq_read_ocs, ocs,
> > +					ocs != OCS_INVALID_COMMAND_STATUS, 20,
> > +					MCQ_POLL_US, false, hwq, lrbp);
> > +	}
> 
> An error message should be reported if the above loop times out.
Got it.

> 
> Thanks,
> 
> Bart.

Kiwoong Kim
BRs.