Re: [PATCH 18/24] st: return error code in st_scsi_execute()
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
>>> ---
>>> 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.
Re: [PATCH 18/24] st: return error code in st_scsi_execute()
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
>> ---
>> 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
--
Dr. Hannes Reinecke Teamlead Storage & Networking
h...@suse.de +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 247165 (AG München), GF: Felix Imendörffer
Re: [PATCH 18/24] st: return error code in st_scsi_execute()
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
---
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.
