| Message ID | 20200916085059.27206-1-hare@suse.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | lpfc: use NVMe error codes for LS request done callback | expand |
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
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
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 --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? "
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(-)