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@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.
Please make this clear in the patch description. I think that will make
this patch easier to review.
Thanks,
Bart.
