[PATCH 3/5] nvme: move result handling into nvme_execute_rq()

[Sagi Grimberg]

On 2/13/23 12:04, Hannes Reinecke wrote:
> On 2/13/23 10:59, Sagi Grimberg wrote:
>>
>>
>> On 2/9/23 16:38, Hannes Reinecke wrote:
>>> and simplify the calling convention.
>>>
>>> Signed-off-by: Hannes Reinecke <hare at suse.de>
>>> ---
>>>   drivers/nvme/host/core.c       | 16 ++++++++++------
>>>   drivers/nvme/host/ioctl.c      |  5 +++--
>>>   drivers/nvme/host/nvme.h       |  3 ++-
>>>   drivers/nvme/target/passthru.c |  3 +--
>>>   4 files changed, 16 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
>>> index 78da9c6cbba8..11faebe87764 100644
>>> --- a/drivers/nvme/host/core.c
>>> +++ b/drivers/nvme/host/core.c
>>> @@ -1004,15 +1004,21 @@ EXPORT_SYMBOL_GPL(nvme_setup_cmd);
>>>    * >0: nvme controller's cqe status response
>>>    * <0: kernel error in lieu of controller response
>>>    */
>>> -int nvme_execute_rq(struct request *rq, bool at_head)
>>> +int nvme_execute_rq(struct request *rq, union nvme_result *result,
>>> +            bool at_head)
>>>   {
>>>       blk_status_t status;
>>> +    int ret;
>>>       status = blk_execute_rq(rq, at_head);
>>>       if (nvme_req(rq)->flags & NVME_REQ_CANCELLED)
>>>           return -EINTR;
>>> -    if (nvme_req(rq)->status)
>>> -        return nvme_req(rq)->status;
>>> +    ret = nvme_req(rq)->status;
>>> +    if (ret) {
>>> +        if (result && ret >= 0)
>>
>> How can ret be < 0 here?
>>
>> Why not always take the result, unconditionally?
>>
> Yeah, right. Will be fixing it up.
> 
>> Is this really necessary though?
>>
> ??
> Yes, it is necessary to return the status (if there is one).
> Quite some callers expect that.
> And no, it's probably not necessary to check if there is a status;
> we might always return a status here. But for the result I guess we
> should be checking the status first; I'm slightly hesitant to blindly
> return the result even if we have a zero status.

I meant the simplification altogether. it really forces callers
to send a nvme_result (or NULL) and convert back. Didn't see that
it dramatically simplifies the code.