| Message ID | 20210420021402.27678-12-bvanassche@acm.org (mailing list archive) |
|---|---|
| State | Deferred |
| Headers | show |
| Series | Make better use of static type checking | expand |
Hi Bart, On 2021-04-20 10:13, Bart Van Assche wrote: > An explanation of the purpose of this patch is available in the patch > "scsi: Introduce the scsi_status union". > > Cc: Can Guo <cang@codeaurora.org> > Cc: Yue Hu <huyue2@yulong.com> > Signed-off-by: Bart Van Assche <bvanassche@acm.org> > --- > drivers/scsi/ufs/ufs_bsg.c | 2 +- > drivers/scsi/ufs/ufshcd.c | 27 +++++++++++++++------------ > 2 files changed, 16 insertions(+), 13 deletions(-) > > diff --git a/drivers/scsi/ufs/ufs_bsg.c b/drivers/scsi/ufs/ufs_bsg.c > index 5b2bc1a6f922..4dcc09136a50 100644 > --- a/drivers/scsi/ufs/ufs_bsg.c > +++ b/drivers/scsi/ufs/ufs_bsg.c > @@ -152,7 +152,7 @@ static int ufs_bsg_request(struct bsg_job *job) > kfree(desc_buff); > > out: > - bsg_reply->result = ret; > + bsg_reply->status.combined = ret; > job->reply_len = sizeof(struct ufs_bsg_reply); > /* complete the job here only if no error */ > if (ret == 0) > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c > index d966d80838fb..cec555d3fcd7 100644 > --- a/drivers/scsi/ufs/ufshcd.c > +++ b/drivers/scsi/ufs/ufshcd.c > @@ -4933,7 +4933,7 @@ ufshcd_scsi_cmd_status(struct ufshcd_lrb *lrbp, > enum sam_status scsi_status) > static inline int > ufshcd_transfer_rsp_status(struct ufs_hba *hba, struct ufshcd_lrb > *lrbp) > { > - int result = 0; > + union scsi_status result; > int ocs; > > /* overall command status of utrd */ > @@ -4951,7 +4951,7 @@ ufshcd_transfer_rsp_status(struct ufs_hba *hba, > struct ufshcd_lrb *lrbp) > switch (ufshcd_get_req_rsp(lrbp->ucd_rsp_ptr)) { > case UPIU_TRANSACTION_RESPONSE: > /* Propagate the SCSI status to the SCSI midlayer. */ > - result = ufshcd_scsi_cmd_status(lrbp, > + result.combined = ufshcd_scsi_cmd_status(lrbp, > ufshcd_get_rsp_upiu_result(lrbp->ucd_rsp_ptr) & > MASK_SCSI_STATUS); > > @@ -4981,23 +4981,23 @@ ufshcd_transfer_rsp_status(struct ufs_hba > *hba, struct ufshcd_lrb *lrbp) > break; > case UPIU_TRANSACTION_REJECT_UPIU: > /* TODO: handle Reject UPIU Response */ > - result = DID_ERROR << 16; > + result.combined = DID_ERROR << 16; > dev_err(hba->dev, > "Reject UPIU not fully implemented\n"); > break; > default: > dev_err(hba->dev, > "Unexpected request response code = %x\n", > - result); > - result = DID_ERROR << 16; > + result.combined); > + result.combined = DID_ERROR << 16; > break; > } > break; > case OCS_ABORTED: > - result |= DID_ABORT << 16; > + result.combined |= DID_ABORT << 16; > break; > case OCS_INVALID_COMMAND_STATUS: > - result |= DID_REQUEUE << 16; > + result.combined |= DID_REQUEUE << 16; > break; > case OCS_INVALID_CMD_TABLE_ATTR: > case OCS_INVALID_PRDT_ATTR: > @@ -5009,7 +5009,7 @@ ufshcd_transfer_rsp_status(struct ufs_hba *hba, > struct ufshcd_lrb *lrbp) > case OCS_INVALID_CRYPTO_CONFIG: > case OCS_GENERAL_CRYPTO_ERROR: > default: > - result |= DID_ERROR << 16; > + result.combined |= DID_ERROR << 16; > dev_err(hba->dev, > "OCS error from controller = %x for tag %d\n", > ocs, lrbp->task_tag); > @@ -5021,7 +5021,7 @@ ufshcd_transfer_rsp_status(struct ufs_hba *hba, > struct ufshcd_lrb *lrbp) > if ((host_byte(result) != DID_OK) && > (host_byte(result) != DID_REQUEUE) && !hba->silence_err_logs) > ufshcd_print_trs(hba, 1 << lrbp->task_tag, true); > - return result; > + return result.combined; > } > > /** > @@ -5083,7 +5083,7 @@ static void __ufshcd_transfer_req_compl(struct > ufs_hba *hba, > ufshcd_add_command_trace(hba, index, UFS_CMD_COMP); > result = ufshcd_transfer_rsp_status(hba, lrbp); > scsi_dma_unmap(cmd); > - cmd->result = result; > + cmd->status.combined = result; > /* Mark completed command as NULL in LRB */ > lrbp->cmd = NULL; > /* Do not touch lrbp after scsi done */ > @@ -8437,6 +8437,7 @@ static int ufshcd_set_dev_pwr_mode(struct ufs_hba > *hba, > struct scsi_sense_hdr sshdr; > struct scsi_device *sdp; > unsigned long flags; > + union scsi_status start_stop_res; > int ret; > > spin_lock_irqsave(hba->host->host_lock, flags); > @@ -8471,13 +8472,15 @@ static int ufshcd_set_dev_pwr_mode(struct > ufs_hba *hba, > * callbacks hence set the RQF_PM flag so that it doesn't resume the > * already suspended childs. > */ > - ret = scsi_execute(sdp, cmd, DMA_NONE, NULL, 0, NULL, &sshdr, > + start_stop_res.combined = > + scsi_execute(sdp, cmd, DMA_NONE, NULL, 0, NULL, &sshdr, > START_STOP_TIMEOUT, 0, 0, RQF_PM, NULL); > + ret = start_stop_res.combined; > if (ret) { > sdev_printk(KERN_WARNING, sdp, > "START_STOP failed for power mode: %d, result %x\n", > pwr_mode, ret); > - if (driver_byte(ret) == DRIVER_SENSE) > + if (driver_byte(start_stop_res) == DRIVER_SENSE) > scsi_print_sense_hdr(sdp, NULL, &sshdr); > } Thanks for the change, do you miss ufshcd_send_request_sense()? Regards, Can Guo.
On 5/6/21 5:09 PM, Can Guo wrote:
> Thanks for the change, do you miss ufshcd_send_request_sense()?
Hi Can,
That's a good question. I can change the type of the
ufshcd_send_request_sense() return value but I'm not sure whether it's
worth it since that function only has one caller and that caller does
not care about the subcomponents of the SCSI status value. All it cares
about is whether or not ufshcd_send_request_sense() returns 0.
Thanks,
Bart.
Hi Bart, On 2021-05-07 11:35, Bart Van Assche wrote: > On 5/6/21 5:09 PM, Can Guo wrote: >> Thanks for the change, do you miss ufshcd_send_request_sense()? > > Hi Can, > > That's a good question. I can change the type of the > ufshcd_send_request_sense() return value but I'm not sure whether it's > worth it since that function only has one caller and that caller does > not care about the subcomponents of the SCSI status value. All it cares > about is whether or not ufshcd_send_request_sense() returns 0. > > Thanks, > > Bart. I agree. Reviewed-by: Can Guo <cang@codeaurora.org>
diff --git a/drivers/scsi/ufs/ufs_bsg.c b/drivers/scsi/ufs/ufs_bsg.c index 5b2bc1a6f922..4dcc09136a50 100644 --- a/drivers/scsi/ufs/ufs_bsg.c +++ b/drivers/scsi/ufs/ufs_bsg.c @@ -152,7 +152,7 @@ static int ufs_bsg_request(struct bsg_job *job) kfree(desc_buff); out: - bsg_reply->result = ret; + bsg_reply->status.combined = ret; job->reply_len = sizeof(struct ufs_bsg_reply); /* complete the job here only if no error */ if (ret == 0) diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index d966d80838fb..cec555d3fcd7 100644 --- a/drivers/scsi/ufs/ufshcd.c +++ b/drivers/scsi/ufs/ufshcd.c @@ -4933,7 +4933,7 @@ ufshcd_scsi_cmd_status(struct ufshcd_lrb *lrbp, enum sam_status scsi_status) static inline int ufshcd_transfer_rsp_status(struct ufs_hba *hba, struct ufshcd_lrb *lrbp) { - int result = 0; + union scsi_status result; int ocs; /* overall command status of utrd */ @@ -4951,7 +4951,7 @@ ufshcd_transfer_rsp_status(struct ufs_hba *hba, struct ufshcd_lrb *lrbp) switch (ufshcd_get_req_rsp(lrbp->ucd_rsp_ptr)) { case UPIU_TRANSACTION_RESPONSE: /* Propagate the SCSI status to the SCSI midlayer. */ - result = ufshcd_scsi_cmd_status(lrbp, + result.combined = ufshcd_scsi_cmd_status(lrbp, ufshcd_get_rsp_upiu_result(lrbp->ucd_rsp_ptr) & MASK_SCSI_STATUS); @@ -4981,23 +4981,23 @@ ufshcd_transfer_rsp_status(struct ufs_hba *hba, struct ufshcd_lrb *lrbp) break; case UPIU_TRANSACTION_REJECT_UPIU: /* TODO: handle Reject UPIU Response */ - result = DID_ERROR << 16; + result.combined = DID_ERROR << 16; dev_err(hba->dev, "Reject UPIU not fully implemented\n"); break; default: dev_err(hba->dev, "Unexpected request response code = %x\n", - result); - result = DID_ERROR << 16; + result.combined); + result.combined = DID_ERROR << 16; break; } break; case OCS_ABORTED: - result |= DID_ABORT << 16; + result.combined |= DID_ABORT << 16; break; case OCS_INVALID_COMMAND_STATUS: - result |= DID_REQUEUE << 16; + result.combined |= DID_REQUEUE << 16; break; case OCS_INVALID_CMD_TABLE_ATTR: case OCS_INVALID_PRDT_ATTR: @@ -5009,7 +5009,7 @@ ufshcd_transfer_rsp_status(struct ufs_hba *hba, struct ufshcd_lrb *lrbp) case OCS_INVALID_CRYPTO_CONFIG: case OCS_GENERAL_CRYPTO_ERROR: default: - result |= DID_ERROR << 16; + result.combined |= DID_ERROR << 16; dev_err(hba->dev, "OCS error from controller = %x for tag %d\n", ocs, lrbp->task_tag); @@ -5021,7 +5021,7 @@ ufshcd_transfer_rsp_status(struct ufs_hba *hba, struct ufshcd_lrb *lrbp) if ((host_byte(result) != DID_OK) && (host_byte(result) != DID_REQUEUE) && !hba->silence_err_logs) ufshcd_print_trs(hba, 1 << lrbp->task_tag, true); - return result; + return result.combined; } /** @@ -5083,7 +5083,7 @@ static void __ufshcd_transfer_req_compl(struct ufs_hba *hba, ufshcd_add_command_trace(hba, index, UFS_CMD_COMP); result = ufshcd_transfer_rsp_status(hba, lrbp); scsi_dma_unmap(cmd); - cmd->result = result; + cmd->status.combined = result; /* Mark completed command as NULL in LRB */ lrbp->cmd = NULL; /* Do not touch lrbp after scsi done */ @@ -8437,6 +8437,7 @@ static int ufshcd_set_dev_pwr_mode(struct ufs_hba *hba, struct scsi_sense_hdr sshdr; struct scsi_device *sdp; unsigned long flags; + union scsi_status start_stop_res; int ret; spin_lock_irqsave(hba->host->host_lock, flags); @@ -8471,13 +8472,15 @@ static int ufshcd_set_dev_pwr_mode(struct ufs_hba *hba, * callbacks hence set the RQF_PM flag so that it doesn't resume the * already suspended childs. */ - ret = scsi_execute(sdp, cmd, DMA_NONE, NULL, 0, NULL, &sshdr, + start_stop_res.combined = + scsi_execute(sdp, cmd, DMA_NONE, NULL, 0, NULL, &sshdr, START_STOP_TIMEOUT, 0, 0, RQF_PM, NULL); + ret = start_stop_res.combined; if (ret) { sdev_printk(KERN_WARNING, sdp, "START_STOP failed for power mode: %d, result %x\n", pwr_mode, ret); - if (driver_byte(ret) == DRIVER_SENSE) + if (driver_byte(start_stop_res) == DRIVER_SENSE) scsi_print_sense_hdr(sdp, NULL, &sshdr); }
An explanation of the purpose of this patch is available in the patch "scsi: Introduce the scsi_status union". Cc: Can Guo <cang@codeaurora.org> Cc: Yue Hu <huyue2@yulong.com> Signed-off-by: Bart Van Assche <bvanassche@acm.org> --- drivers/scsi/ufs/ufs_bsg.c | 2 +- drivers/scsi/ufs/ufshcd.c | 27 +++++++++++++++------------ 2 files changed, 16 insertions(+), 13 deletions(-)