Re: [PATCH RFC v2 04/18] scsi: core: Add support to send reserved commands

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 6/13/22 18:40, John Garry wrote:
> On 13/06/2022 08:03, Damien Le Moal wrote:
>>> +	if (shost->nr_reserved_cmds && !sht->reserved_queuecommand) {
>>> +		shost_printk(KERN_ERR, shost,
>>> +			"nr_reserved_cmds set but no method to queue\n");
>>> +		goto fail;
>> This would be a driver implementation bug.
> 
> It would be a driver bug, but it probably makes the driver utterly 
> useless and there is no point in continuing (to try to add). If the 
> driver supports reserved commands then they are prob essential to make 
> the driver function.
> 
>> So what about a WARN() here ?
> 
> Maybe but I really do not see a point in continuing

Completely agree. My suggestion is to replace the regular shost_printk()
error message with something "heavier" like WARN(). Since that is not an
expected error. Even a BUG_ON() may be acceptable :)

> 
>>
>>> +	}
>>> +
>>>   	/* Use min_t(int, ...) in case shost->can_queue exceeds SHRT_MAX */
>>>   	shost->cmd_per_lun = min_t(int, shost->cmd_per_lun,
>>>   				   shost->can_queue);
>>> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
>>> index f6e53c6d913c..8c8b4c6767d9 100644
>>> --- a/drivers/scsi/scsi_lib.c
>>> +++ b/drivers/scsi/scsi_lib.c
>>> @@ -1422,6 +1422,16 @@ static void scsi_complete(struct request *rq)
>>>   	struct scsi_cmnd *cmd = blk_mq_rq_to_pdu(rq);
>>>   	enum scsi_disposition disposition;
>>>   
>>> +	if (scsi_is_reserved_cmd(cmd)) {
>>> +		struct scsi_device *sdev = cmd->device;
>>> +
>>> +		scsi_mq_uninit_cmd(cmd);
>>> +		scsi_device_unbusy(sdev, cmd);
>>> +		__blk_mq_end_request(rq, 0);
>>> +
>>> +		return;
>>> +	}
>>> +
>>>   	INIT_LIST_HEAD(&cmd->eh_entry);
>>>   
>>>   	atomic_inc(&cmd->device->iodone_cnt);
>>> @@ -1706,6 +1716,28 @@ static blk_status_t scsi_queue_rq(struct blk_mq_hw_ctx *hctx,
>>>   
>>>   	WARN_ON_ONCE(cmd->budget_token < 0);
>>>   
>>> +	if (scsi_is_reserved_cmd(cmd)) {
>>> +		unsigned char *host_scribble = cmd->host_scribble;
>>> +
>>> +		if (!(req->rq_flags & RQF_DONTPREP)) {
>>> +			ret = scsi_prepare_cmd(req);
>>> +			if (ret != BLK_STS_OK) {
>>> +
>> Stray blank line.
> 
> ok
> 
>>
>>> +				goto out_dec_host_busy;
>>> +			}
>> No need for the curly brackets here.
> 
> ok
> 
>>
>>> +
>>> +			req->rq_flags |= RQF_DONTPREP;
>>> +		} else {
>>> +			clear_bit(SCMD_STATE_COMPLETE, &cmd->state);
>>> +		}
> 
> Thanks,
> John


-- 
Damien Le Moal
Western Digital Research



[Index of Archives]     [Linux Filesystems]     [Linux SCSI]     [Linux RAID]     [Git]     [Kernel Newbies]     [Linux Newbie]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Samba]     [Device Mapper]

  Powered by Linux