diff mbox series

lpfc: use NVMe error codes for LS request done callback

Message ID 20200916085059.27206-1-hare@suse.de
State New
Headers show
Series lpfc: use NVMe error codes for LS request done callback | expand

Commit Message

Hannes Reinecke Sept. 16, 2020, 8:50 a.m. UTC
The LS request callback requires a 'status' argument, but that one
should be an NVMe error code, not a driver specific one which has
no meaning in the nvme layer.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/scsi/lpfc/lpfc_nvme.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

James Smart Sept. 16, 2020, 9:23 p.m. UTC | #1
On 9/16/2020 1:50 AM, Hannes Reinecke wrote:
> The LS request callback requires a 'status' argument, but that one

> should be an NVMe error code, not a driver specific one which has

> no meaning in the nvme layer.

>

> Signed-off-by: Hannes Reinecke <hare@suse.de>

> ---

>   drivers/scsi/lpfc/lpfc_nvme.c | 4 +++-

>   1 file changed, 3 insertions(+), 1 deletion(-)

>

> diff --git a/drivers/scsi/lpfc/lpfc_nvme.c b/drivers/scsi/lpfc/lpfc_nvme.c

> index e5be334d6a11..4b007a28014b 100644

> --- a/drivers/scsi/lpfc/lpfc_nvme.c

> +++ b/drivers/scsi/lpfc/lpfc_nvme.c

> @@ -498,7 +498,9 @@ __lpfc_nvme_ls_req_cmp(struct lpfc_hba *phba,  struct lpfc_vport *vport,

>   		cmdwqe->context3 = NULL;

>   	}

>   	if (pnvme_lsreq->done)

> -		pnvme_lsreq->done(pnvme_lsreq, status);

> +		pnvme_lsreq->done(pnvme_lsreq,

> +				  status == IOSTAT_SUCCESS ?

> +				  NVME_SC_SUCCESS : NVME_SC_INTERNAL);

>   	else

>   		lpfc_printf_vlog(vport, KERN_ERR, LOG_TRACE_EVENT,

>   				 "6046 NVMEx cmpl without done call back? "


No - it's not a nvme command, so doesn't need a nvme status code. It 
should be a -Exxx  value or a 0 (success).  nvme_fc_send_ls_req() for 
example calls __nvme_fc_send_ls_req(), which can return any number of 
-Exxx values, and the routine returns the value returned by the done 
call after waiting for it to complete - so they should all follow the 
same form.

-- james
Hannes Reinecke Sept. 17, 2020, 5:37 a.m. UTC | #2
On 9/16/20 11:23 PM, James Smart wrote:
> 

> 

> On 9/16/2020 1:50 AM, Hannes Reinecke wrote:

>> The LS request callback requires a 'status' argument, but that one

>> should be an NVMe error code, not a driver specific one which has

>> no meaning in the nvme layer.

>>

>> Signed-off-by: Hannes Reinecke <hare@suse.de>

>> ---

>>   drivers/scsi/lpfc/lpfc_nvme.c | 4 +++-

>>   1 file changed, 3 insertions(+), 1 deletion(-)

>>

>> diff --git a/drivers/scsi/lpfc/lpfc_nvme.c 

>> b/drivers/scsi/lpfc/lpfc_nvme.c

>> index e5be334d6a11..4b007a28014b 100644

>> --- a/drivers/scsi/lpfc/lpfc_nvme.c

>> +++ b/drivers/scsi/lpfc/lpfc_nvme.c

>> @@ -498,7 +498,9 @@ __lpfc_nvme_ls_req_cmp(struct lpfc_hba *phba,  

>> struct lpfc_vport *vport,

>>           cmdwqe->context3 = NULL;

>>       }

>>       if (pnvme_lsreq->done)

>> -        pnvme_lsreq->done(pnvme_lsreq, status);

>> +        pnvme_lsreq->done(pnvme_lsreq,

>> +                  status == IOSTAT_SUCCESS ?

>> +                  NVME_SC_SUCCESS : NVME_SC_INTERNAL);

>>       else

>>           lpfc_printf_vlog(vport, KERN_ERR, LOG_TRACE_EVENT,

>>                    "6046 NVMEx cmpl without done call back? "

> 

> No - it's not a nvme command, so doesn't need a nvme status code. It 

> should be a -Exxx  value or a 0 (success).  nvme_fc_send_ls_req() for 

> example calls __nvme_fc_send_ls_req(), which can return any number of 

> -Exxx values, and the routine returns the value returned by the done 

> call after waiting for it to complete - so they should all follow the 

> same form.

> 

Right. But returning IOSTAT definitions is still wrong :->

Will be updating the patch.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer
James Smart Sept. 18, 2020, 5:26 p.m. UTC | #3
On 9/16/2020 10:37 PM, Hannes Reinecke wrote:
> On 9/16/20 11:23 PM, James Smart wrote:

>>

>>

>> On 9/16/2020 1:50 AM, Hannes Reinecke wrote:

>>> The LS request callback requires a 'status' argument, but that one

>>> should be an NVMe error code, not a driver specific one which has

>>> no meaning in the nvme layer.

>>>

>>> Signed-off-by: Hannes Reinecke <hare@suse.de>

>>> ---

>>>   drivers/scsi/lpfc/lpfc_nvme.c | 4 +++-

>>>   1 file changed, 3 insertions(+), 1 deletion(-)

>>>

>>> diff --git a/drivers/scsi/lpfc/lpfc_nvme.c 

>>> b/drivers/scsi/lpfc/lpfc_nvme.c

>>> index e5be334d6a11..4b007a28014b 100644

>>> --- a/drivers/scsi/lpfc/lpfc_nvme.c

>>> +++ b/drivers/scsi/lpfc/lpfc_nvme.c

>>> @@ -498,7 +498,9 @@ __lpfc_nvme_ls_req_cmp(struct lpfc_hba *phba,  

>>> struct lpfc_vport *vport,

>>>           cmdwqe->context3 = NULL;

>>>       }

>>>       if (pnvme_lsreq->done)

>>> -        pnvme_lsreq->done(pnvme_lsreq, status);

>>> +        pnvme_lsreq->done(pnvme_lsreq,

>>> +                  status == IOSTAT_SUCCESS ?

>>> +                  NVME_SC_SUCCESS : NVME_SC_INTERNAL);

>>>       else

>>>           lpfc_printf_vlog(vport, KERN_ERR, LOG_TRACE_EVENT,

>>>                    "6046 NVMEx cmpl without done call back? "

>>

>> No - it's not a nvme command, so doesn't need a nvme status code. It 

>> should be a -Exxx  value or a 0 (success). nvme_fc_send_ls_req() for 

>> example calls __nvme_fc_send_ls_req(), which can return any number of 

>> -Exxx values, and the routine returns the value returned by the done 

>> call after waiting for it to complete - so they should all follow the 

>> same form.

>>

> Right. But returning IOSTAT definitions is still wrong :->


true..  :)

-- james
diff mbox series

Patch

diff --git a/drivers/scsi/lpfc/lpfc_nvme.c b/drivers/scsi/lpfc/lpfc_nvme.c
index e5be334d6a11..4b007a28014b 100644
--- a/drivers/scsi/lpfc/lpfc_nvme.c
+++ b/drivers/scsi/lpfc/lpfc_nvme.c
@@ -498,7 +498,9 @@  __lpfc_nvme_ls_req_cmp(struct lpfc_hba *phba,  struct lpfc_vport *vport,
 		cmdwqe->context3 = NULL;
 	}
 	if (pnvme_lsreq->done)
-		pnvme_lsreq->done(pnvme_lsreq, status);
+		pnvme_lsreq->done(pnvme_lsreq,
+				  status == IOSTAT_SUCCESS ?
+				  NVME_SC_SUCCESS : NVME_SC_INTERNAL);
 	else
 		lpfc_printf_vlog(vport, KERN_ERR, LOG_TRACE_EVENT,
 				 "6046 NVMEx cmpl without done call back? "