linux-kernel - Re: [PATCH RFC v3 04/22] scsi: core: Add support to send reserved commands

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <c30e421e-9652-ebb8-9733-b286cb0f9f19@opensource.wdc.com>
Date:   Thu, 27 Oct 2022 10:21:40 +0900
From:   Damien Le Moal <damien.lemoal@...nsource.wdc.com>
To:     John Garry <john.garry@...wei.com>, axboe@...nel.dk,
        jejb@...ux.ibm.com, martin.petersen@...cle.com,
        jinpu.wang@...ud.ionos.com, hare@...e.de, bvanassche@....org,
        hch@....de, ming.lei@...hat.com, niklas.cassel@....com
Cc:     linux-block@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-ide@...r.kernel.org, linux-scsi@...r.kernel.org,
        linuxarm@...wei.com
Subject: Re: [PATCH RFC v3 04/22] scsi: core: Add support to send reserved
 commands

On 10/25/22 19:17, John Garry wrote:
> Add a method to queue reserved commands.
> 
> Signed-off-by: John Garry <john.garry@...wei.com>
> ---
>  drivers/scsi/hosts.c     |  6 ++++++
>  drivers/scsi/scsi_lib.c  | 25 +++++++++++++++++++++++++
>  include/scsi/scsi_host.h |  1 +
>  3 files changed, 32 insertions(+)
> 
> diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
> index db89afc37bc9..78968553089f 100644
> --- a/drivers/scsi/hosts.c
> +++ b/drivers/scsi/hosts.c
> @@ -230,6 +230,12 @@ int scsi_add_host_with_dma(struct Scsi_Host *shost, struct device *dev,
>  		goto fail;
>  	}
>  
> +	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;
> +	}
> +
>  	/* 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 a8c4e7c037ae..08015c42c326 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -1428,6 +1428,16 @@ static void scsi_complete(struct request *rq)
>  	struct scsi_cmnd *cmd = blk_mq_rq_to_pdu(rq);
>  	enum scsi_disposition disposition;
>  
> +	if (blk_mq_is_reserved_rq(rq)) {
> +		struct scsi_device *sdev = cmd->device;

This variable is not really needed. You can call:
		
		scsi_device_unbusy(cmd->device, cmd);

No ?

> +
> +		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);
> @@ -1718,6 +1728,21 @@ static blk_status_t scsi_queue_rq(struct blk_mq_hw_ctx *hctx,
>  	blk_status_t ret;
>  	int reason;
>  
> +	if (blk_mq_is_reserved_rq(req)) {
> +		if (!(req->rq_flags & RQF_DONTPREP)) {
> +			ret = scsi_prepare_cmd(req);
> +			if (ret != BLK_STS_OK)
> +				goto out_dec_host_busy;
> +
> +			req->rq_flags |= RQF_DONTPREP;
> +		} else {
> +			clear_bit(SCMD_STATE_COMPLETE, &cmd->state);
> +		}
> +		blk_mq_start_request(req);
> +
> +		return shost->hostt->reserved_queuecommand(shost, cmd);
> +	}
> +
>  	WARN_ON_ONCE(cmd->budget_token < 0);
>  
>  	/*
> diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
> index 91678c77398e..a39f36aa0b0d 100644
> --- a/include/scsi/scsi_host.h
> +++ b/include/scsi/scsi_host.h
> @@ -73,6 +73,7 @@ struct scsi_host_template {
>  	 * STATUS: REQUIRED
>  	 */
>  	int (* queuecommand)(struct Scsi_Host *, struct scsi_cmnd *);
> +	int (*reserved_queuecommand)(struct Scsi_Host *, struct scsi_cmnd *);

Nit: This op name sound like something returning a bool... May be a
straight "queue_reserved_command" name would be clearer ?

>  
>  	/*
>  	 * The commit_rqs function is used to trigger a hardware

-- 
Damien Le Moal
Western Digital Research