| Message ID | 1642415361-140388-1-git-send-email-kwmad.kim@samsung.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v1] scsi: ufs: use an generic error code in ufshcd_set_dev_pwr_mode | expand |
On 1/17/22 02:29, Kiwoong Kim wrote: > The return value of ufshcd_set_dev_pwr_mode is given to > device pm core. However, the function currently returns a result > in scsi command and the device pm core doesn't understand it. > It might lead to unexpected behaviors of user land. I found > the return value led to platform reset in Android. > > This patch is to use an generic code for SSU failures. > > Signed-off-by: Kiwoong Kim <kwmad.kim@samsung.com> > --- > drivers/scsi/ufs/ufshcd.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c > index 1049e41..a60816c 100644 > --- a/drivers/scsi/ufs/ufshcd.c > +++ b/drivers/scsi/ufs/ufshcd.c > @@ -8669,6 +8669,7 @@ static int ufshcd_set_dev_pwr_mode(struct ufs_hba *hba, > pwr_mode, ret); > if (ret > 0 && scsi_sense_valid(&sshdr)) > scsi_print_sense_hdr(sdp, NULL, &sshdr); > + ret = -EIO; > } > > if (!ret) Shouldn't "ret = -EIO" only be executed if ret > 0? Additionally, please update the documentation of ufshcd_set_dev_pwr_mode(). I'm referring to the following comment: "Returns non-zero if failed to set the requested power mode". Thanks, Bart.
> > @@ -8669,6 +8669,7 @@ static int ufshcd_set_dev_pwr_mode(struct ufs_hba > *hba, > > pwr_mode, ret); > > if (ret > 0 && scsi_sense_valid(&sshdr)) > > scsi_print_sense_hdr(sdp, NULL, &sshdr); > > + ret = -EIO; > > } > > > > if (!ret) > > Shouldn't "ret = -EIO" only be executed if ret > 0? Additionally, please > update the documentation of ufshcd_set_dev_pwr_mode(). I'm referring to > the following comment: "Returns non-zero if failed to set the requested > power mode". > > Thanks, > > Bart. scsi_execute returns cmd->result(int type) but I think there is no case that the valaue is negative because all values defined for its most significant byte, i.e. driver byte, are smaller than 0x80. And I understand the 'ret > 0' presents that something wrong happens during the process. So I'm not sure if 'ret = -EIO;' should be executed under 'ret > 0'. -- #define DRIVER_BUSY 0x01 #define DRIVER_SOFT 0x02 And for the comment, I got it. Thanks Kiwoong
On 1/19/22 18:51, Kiwoong Kim wrote: >>> @@ -8669,6 +8669,7 @@ static int ufshcd_set_dev_pwr_mode(struct ufs_hba >> *hba, >>> pwr_mode, ret); >>> if (ret > 0 && scsi_sense_valid(&sshdr)) >>> scsi_print_sense_hdr(sdp, NULL, &sshdr); >>> + ret = -EIO; >>> } >>> >>> if (!ret) >> >> Shouldn't "ret = -EIO" only be executed if ret > 0? Additionally, please >> update the documentation of ufshcd_set_dev_pwr_mode(). I'm referring to >> the following comment: "Returns non-zero if failed to set the requested >> power mode". >> >> Thanks, >> >> Bart. > > scsi_execute returns cmd->result(int type) but I think there is no case that the valaue is negative > because all values defined for its most significant byte, i.e. driver byte, are smaller than 0x80. > And I understand the 'ret > 0' presents that something wrong happens during the process. > > So I'm not sure if 'ret = -EIO;' should be executed under 'ret > 0'. I think that scsi_execute() can return a negative value. From __scsi_execute(): req = scsi_alloc_request(sdev->request_queue, data_direction == DMA_TO_DEVICE ? REQ_OP_DRV_OUT : REQ_OP_DRV_IN, rq_flags & RQF_PM ? BLK_MQ_REQ_PM : 0); if (IS_ERR(req)) return PTR_ERR(req); As you probably know PTR_ERR() returns a negative error code if IS_ERR() returns true. Thanks, Bart.
> I think that scsi_execute() can return a negative value. From > __scsi_execute(): > > req = scsi_alloc_request(sdev->request_queue, > data_direction == DMA_TO_DEVICE ? > REQ_OP_DRV_OUT : REQ_OP_DRV_IN, > rq_flags & RQF_PM ? BLK_MQ_REQ_PM : 0); > if (IS_ERR(req)) > return PTR_ERR(req); > > As you probably know PTR_ERR() returns a negative error code if IS_ERR() > returns true. Right, Thanks.
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index 1049e41..a60816c 100644 --- a/drivers/scsi/ufs/ufshcd.c +++ b/drivers/scsi/ufs/ufshcd.c @@ -8669,6 +8669,7 @@ static int ufshcd_set_dev_pwr_mode(struct ufs_hba *hba, pwr_mode, ret); if (ret > 0 && scsi_sense_valid(&sshdr)) scsi_print_sense_hdr(sdp, NULL, &sshdr); + ret = -EIO; } if (!ret)
The return value of ufshcd_set_dev_pwr_mode is given to device pm core. However, the function currently returns a result in scsi command and the device pm core doesn't understand it. It might lead to unexpected behaviors of user land. I found the return value led to platform reset in Android. This patch is to use an generic code for SSU failures. Signed-off-by: Kiwoong Kim <kwmad.kim@samsung.com> --- drivers/scsi/ufs/ufshcd.c | 1 + 1 file changed, 1 insertion(+)