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@xxxxxxx>
>> ---
>> 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
hare@xxxxxxx +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 247165 (AG München), GF: Felix Imendörffer
