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
