Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
IronPort-SDR: 1bwGAAb1sidgyaZP8Zwp2xcKHoYl6Q52y2dVp1GDE9vs15YGtszBLp3ZUPRWqsxO34Auqu8PTW
 hxMLhiKLEVlsZ9/liZVbJJ1IawBYo/3X3MIIayQwF4Pmb2c9JO0mzuSyXzUsIL4fPQZFyrSQUY
 wwfUOWfEo/aNq48af/rfRtu9CLz5V+N/ffwPy/WYyQ0V8gTIf6tFJ3zlgKyLKyTyK+UIF56KWA
 xFVOSiJsyq9Zy2t1Y1JMALsaMTOJ2GNpvxijrbQdq8hp+tUHxlfABKHBNG9bVZ4wEPow8v43oG
 zKBM6HZMKqka9+HfuwPGcyz1
IronPort-SDR: +a4mpC1xG4CCidh8NSreLZHKolgi8+NXFiWRtlFOZm1hrBocDza7Pxh+khlAO8WKQmFILHhmll
 bc544oId8i3wHeU8WOYvxAY7W0zYhdeSEdG6QrnR02S2ISz7OBi7HEhU431z9wb5IdmOG7jdn5
 vN0HkbDrdQyzAIwzxjPgc2Rbrhmrx7FOG0MllSEcJ487tjGg8iRInMPUuzUUJ71lCPHcwggSrN
 PQoYfOb/7I2faUsZv47yK+c95+KitQOWSS8ruF7PLhZQzbvGgN3ZjjP3DEdi5yJ/TBIOfb6CEA
 vcA=
WDCIronportException: Internal
Message-ID: <9b55ae8d-30be-ff70-85e0-61616e18f18a@opensource.wdc.com>
Date:   Mon, 13 Jun 2022 18:41:38 +0900
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.10.0
Subject: Re: [PATCH RFC v2 04/18] scsi: core: Add support to send reserved
 commands
Content-Language: en-US
To:     John Garry <john.garry@huawei.com>, axboe@kernel.dk,
        jejb@linux.ibm.com, martin.petersen@oracle.com, brking@us.ibm.com,
        hare@suse.de, hch@lst.de
Cc:     linux-block@vger.kernel.org, linux-ide@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-scsi@vger.kernel.org,
        chenxiang66@hisilicon.com
References: <1654770559-101375-1-git-send-email-john.garry@huawei.com>
 <1654770559-101375-5-git-send-email-john.garry@huawei.com>
 <3f519a72-bb43-b1f0-c85d-a2ea4596f2f2@opensource.wdc.com>
 <04540488-ec07-3bc2-a997-b7f64b0ba606@huawei.com>
From:   Damien Le Moal <damien.lemoal@opensource.wdc.com>
Organization: Western Digital Research
In-Reply-To: <04540488-ec07-3bc2-a997-b7f64b0ba606@huawei.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

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