| Message ID | 20191021095322.137969-19-hare@suse.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | scsi: Revamp result values | expand |
On 10/21/19 2:53 AM, Hannes Reinecke wrote: > We should return the actual error code in st_scsi_execute(), > avoiding the need to use DRIVER_ERROR. > > Signed-off-by: Hannes Reinecke <hare@suse.de> > --- > drivers/scsi/st.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/scsi/st.c b/drivers/scsi/st.c > index e3266a64a477..5f38369cc62f 100644 > --- a/drivers/scsi/st.c > +++ b/drivers/scsi/st.c > @@ -549,7 +549,7 @@ static int st_scsi_execute(struct st_request *SRpnt, const unsigned char *cmd, > data_direction == DMA_TO_DEVICE ? > REQ_OP_SCSI_OUT : REQ_OP_SCSI_IN, 0); > if (IS_ERR(req)) > - return DRIVER_ERROR << 24; > + return PTR_ERR(req); > rq = scsi_req(req); > req->rq_flags |= RQF_QUIET; > > @@ -560,7 +560,7 @@ static int st_scsi_execute(struct st_request *SRpnt, const unsigned char *cmd, > GFP_KERNEL); > if (err) { > blk_put_request(req); > - return DRIVER_ERROR << 24; > + return err; > } > } The patch description looks confusing to me. Is it perhaps because the caller compares the st_scsi_execute() return value with zero and doesn't use the return value in any other way that it is fine to return an integer error code instead of a SCSI status? Thanks, Bart.
On 10/21/19 6:41 PM, Bart Van Assche wrote: > On 10/21/19 2:53 AM, Hannes Reinecke wrote: >> We should return the actual error code in st_scsi_execute(), >> avoiding the need to use DRIVER_ERROR. >> >> Signed-off-by: Hannes Reinecke <hare@suse.de> >> --- >> drivers/scsi/st.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/scsi/st.c b/drivers/scsi/st.c >> index e3266a64a477..5f38369cc62f 100644 >> --- a/drivers/scsi/st.c >> +++ b/drivers/scsi/st.c >> @@ -549,7 +549,7 @@ static int st_scsi_execute(struct st_request >> *SRpnt, const unsigned char *cmd, >> data_direction == DMA_TO_DEVICE ? >> REQ_OP_SCSI_OUT : REQ_OP_SCSI_IN, 0); >> if (IS_ERR(req)) >> - return DRIVER_ERROR << 24; >> + return PTR_ERR(req); >> rq = scsi_req(req); >> req->rq_flags |= RQF_QUIET; >> @@ -560,7 +560,7 @@ static int st_scsi_execute(struct st_request >> *SRpnt, const unsigned char *cmd, >> GFP_KERNEL); >> if (err) { >> blk_put_request(req); >> - return DRIVER_ERROR << 24; >> + return err; >> } >> } > > The patch description looks confusing to me. Is it perhaps because the > caller compares the st_scsi_execute() return value with zero and doesn't > use the return value in any other way that it is fine to return an > integer error code instead of a SCSI status? > Yes. The caller does: ret = st_scsi_execute(SRpnt, cmd, direction, NULL, bytes, timeout, retries); if (ret) { /* could not allocate the buffer or request was too large */ (STp->buffer)->syscall_result = (-EBUSY); (STp->buffer)->last_SRpnt = NULL; So it's immaterial _what_ we return here as long as it's non-zero. Cheers, Hannes
On 2019-10-21 23:28, Hannes Reinecke wrote: > On 10/21/19 6:41 PM, Bart Van Assche wrote: >> On 10/21/19 2:53 AM, Hannes Reinecke wrote: >>> We should return the actual error code in st_scsi_execute(), >>> avoiding the need to use DRIVER_ERROR. >>> >>> Signed-off-by: Hannes Reinecke <hare@suse.de> >>> --- >>> drivers/scsi/st.c | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/scsi/st.c b/drivers/scsi/st.c >>> index e3266a64a477..5f38369cc62f 100644 >>> --- a/drivers/scsi/st.c >>> +++ b/drivers/scsi/st.c >>> @@ -549,7 +549,7 @@ static int st_scsi_execute(struct st_request >>> *SRpnt, const unsigned char *cmd, >>> data_direction == DMA_TO_DEVICE ? >>> REQ_OP_SCSI_OUT : REQ_OP_SCSI_IN, 0); >>> if (IS_ERR(req)) >>> - return DRIVER_ERROR << 24; >>> + return PTR_ERR(req); >>> rq = scsi_req(req); >>> req->rq_flags |= RQF_QUIET; >>> @@ -560,7 +560,7 @@ static int st_scsi_execute(struct st_request >>> *SRpnt, const unsigned char *cmd, >>> GFP_KERNEL); >>> if (err) { >>> blk_put_request(req); >>> - return DRIVER_ERROR << 24; >>> + return err; >>> } >>> } >> >> The patch description looks confusing to me. Is it perhaps because the >> caller compares the st_scsi_execute() return value with zero and doesn't >> use the return value in any other way that it is fine to return an >> integer error code instead of a SCSI status? >> > Yes. The caller does: > > ret = st_scsi_execute(SRpnt, cmd, direction, NULL, bytes, timeout, > retries); > if (ret) { > /* could not allocate the buffer or request was too large */ > (STp->buffer)->syscall_result = (-EBUSY); > (STp->buffer)->last_SRpnt = NULL; > > So it's immaterial _what_ we return here as long as it's non-zero. Please make this clear in the patch description. I think that will make this patch easier to review. Thanks, Bart.
diff --git a/drivers/scsi/st.c b/drivers/scsi/st.c index e3266a64a477..5f38369cc62f 100644 --- a/drivers/scsi/st.c +++ b/drivers/scsi/st.c @@ -549,7 +549,7 @@ static int st_scsi_execute(struct st_request *SRpnt, const unsigned char *cmd, data_direction == DMA_TO_DEVICE ? REQ_OP_SCSI_OUT : REQ_OP_SCSI_IN, 0); if (IS_ERR(req)) - return DRIVER_ERROR << 24; + return PTR_ERR(req); rq = scsi_req(req); req->rq_flags |= RQF_QUIET; @@ -560,7 +560,7 @@ static int st_scsi_execute(struct st_request *SRpnt, const unsigned char *cmd, GFP_KERNEL); if (err) { blk_put_request(req); - return DRIVER_ERROR << 24; + return err; } }
We should return the actual error code in st_scsi_execute(), avoiding the need to use DRIVER_ERROR. Signed-off-by: Hannes Reinecke <hare@suse.de> --- drivers/scsi/st.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)