Re: [PATCH 1/1] nvme: retry security commands if media not ready

[Nilay Shroff <nilay@linux.ibm.com>]

On 10/4/24 11:11, Christoph Hellwig wrote:
> On Thu, Oct 03, 2024 at 06:35:30PM -0500, Greg Joyce wrote:
>> I agree, I wonder about the value/wisdom of the CC.CRIME capability
>> especially since there is no way to read status that indicates that the
>> media is ready for the Figure 103 commands. But it is a defined feature
>> and setting it does cause CSTS.RDY to be asserted before the media is
>> ready.
>>
>> The Kioxia CM7 drive does set both CRWMS and CRIMS (CRMS=11b). And
>> these lines in the NVMe driver thus set CC.CRIME:
>>
>>         if (ctrl->cap & NVME_CAP_CRMS_CRWMS && ctrl->cap &            
>> NVME_CAP_CRMS_CRIMS)
>>                 ctrl->ctrl_config |= NVME_CC_CRIME;
>>
>> After reading more of the spec and driver code and discussions here, I
>> suggest that those two lines be removed. This has the effect of
>> returning to the NVMe version 1.4 behavior and CSTS.RDY will not be
>> asserted until the media is ready for commands.
> 
> Well, it is a useful feature unless random admin commands return
> not ready.  Which got weaseld into the spec, but really should not
> happen to make the feature useful.  So I think we'll need to put the
> workaround in instead of messing up the proper implementations of
> the feature that aren't this silly.  And make sure the big companies
> put that into their purchase specs.
> 

How about retrying the command, from nvme_retry_req(), if the return status
of the security (or for that matter any admin) command suggests to retry 
the request (i.e. DNR bit in the status is cleared to 0 and return status of 
the command is NVME_SC_ADMIN_COMMAND_MEDIA_NOT_READY)?  

Basically, nvme_retry_req() shall calculate the "delay" for retrying request.
We should first mark the timestamp the moment we enable the controller (i.e. 
time when CC.EN bit is set) and later nvme_retry_req() can use it to calculate 
the time remaining for media to be ready. And use this remaining time as "dealy"
for re-queuing the failed request.

Yes we also need to set the flag NVME_SUBMIT_RETRY while submitting request in
nvme_sec_submit() so that nvme_decide_disposition() allow retrying the request
if it fails.

Thanks,
--Nilay