Re: [PATCH 05/42] scsi: stop using DRIVER_ERROR

Hannes Reinecke
2021-04-22T08:39:23Z

On 4/21/21 11:30 PM, Bart Van Assche wrote:
> On 4/21/21 10:47 AM, Hannes Reinecke wrote:
>> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
>> index c532f9390ae3..2d9b533ef1ec 100644
>> --- a/drivers/scsi/scsi_lib.c
>> +++ b/drivers/scsi/scsi_lib.c
>> @@ -245,20 +245,23 @@ int __scsi_execute(struct scsi_device *sdev,
>> const unsigned char *cmd,
>>   {
>>       struct request *req;
>>       struct scsi_request *rq;
>> -    int ret = DRIVER_ERROR << 24;
>> +    int ret;
>>       req = blk_get_request(sdev->request_queue,
>>               data_direction == DMA_TO_DEVICE ?
>>               REQ_OP_SCSI_OUT : REQ_OP_SCSI_IN,
>>               rq_flags & RQF_PM ? BLK_MQ_REQ_PM : 0);
>>       if (IS_ERR(req))
>> -        return ret;
>> -    rq = scsi_req(req);
>> +        return PTR_ERR(req);
>> -    if (bufflen &&    blk_rq_map_kern(sdev->request_queue, req,
>> -                    buffer, bufflen, GFP_NOIO))
>> -        goto out;
>> +    rq = scsi_req(req);
>> +    if (bufflen) {
>> +        ret = blk_rq_map_kern(sdev->request_queue, req,
>> +                      buffer, bufflen, GFP_NOIO);
>> +        if (ret)
>> +            goto out;
>> +    }
>>       rq->cmd_len = COMMAND_SIZE(cmd[0]);
>>       memcpy(rq->cmd, cmd, rq->cmd_len);
>>       rq->retries = retries;
>
> Please mention in the patch description that this change involves a user
> space ABI change. My understanding is that the current behavior of the
> IOC_PR_* ioctls is as follows:
> * A value <= 0 is returned for NVMe where 0 represents success and a
> negative value represents failure.
> * A value >= 0 is returned for SCSI where 0 represents success and a
> positive value is a four-byte SCSI status code.
>
> This patch changes the behavior for SCSI from returning a value >= 0
> into returning a value that can be negative, zero or positive where only
> the return value 0 represents success.
>
> See also sd_pr_command() in drivers/scsi/sd.c.
>

As indicated: the description for __scsi_execute() explicitly states
that a negative return value is allowed here.
And all functions calling sd_pr_command() might even now return a
negative error value, so userspace _must_ be prepared to handle it.

So while we're changing the error we return, the actual impact to
userspace should be pretty neglible as either error value indicates
a non-retryable failure.

Cheers,

Hannes
--
Dr. Hannes Reinecke Kernel Storage Architect
hare@suse.de +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer