Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
IronPort-SDR: oqwoWS6Aa3yKn9FWdqfzW+BQFITi9fatvRfx2w6kwVtUxAUAIqpadYN//mglpE+z1wAk7REJ59
 HO5WAHhmX2ZEXTA1v/99luheoiMDtS/B0TEMuH6i76BQH8hYLOi/LCa1Z2YzNzkEdm8J9Bkag7
 /kyly5Effzdkkv3aIx+XoE97xlAeqhT7gHJdD0t0c2EnfE/szek2vXdh2jXlGy2t2/wATy45WT
 O2NChwqnWOV3gaLjOr2uNxFfNi0pIMPsx2KRxud74SnSIabrhBw3FUoA9lNEZ9PiaXIOM8iYe/
 mTs8J3tYc7cgoKVYlykN71Z/
IronPort-SDR: TnNJ+eOBcZlzyTghWp58f1M5Pc5BQAW4Z80aPlU0NiLWimWTB4seDdmy8fAys42LOLi39rK9N3
 C7N8PW1Gos1Roxy0BZS9qSo0rXaMcOIVW9z03hl2dU+Uyy9EAcbKqjxlByTJbaZ/MwNsDjtlhx
 kRlIx/VgVKGTOLME+6wbfEfmZO1PQvFFVR/ZsKoMoqWGJRNQ0btDAYcEEPuz81z8CVLStdeNP1
 VYa2S4NXWGBv3kIV83yi1N13UkwrLxs+tlsY2RNHn4snpLVf9Rvk42J6dq/B0siHSE72rw3lH0
 PlM=
WDCIronportException: Internal
Message-ID: <06820834-409e-5a53-b7b2-e3787a16d329@opensource.wdc.com>
Date:   Wed, 6 Apr 2022 10:47:27 +0900
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.7.0
Subject: Re: [PATCH v2 2/2] libata: Inline ata_qc_new_init() in
 ata_scsi_qc_new()
Content-Language: en-US
To:     John Garry <john.garry@huawei.com>, hch@lst.de
Cc:     linux-kernel@vger.kernel.org, linux-ide@vger.kernel.org,
        linux-doc@vger.kernel.org, linux-scsi@vger.kernel.org
References: <1649083990-207133-1-git-send-email-john.garry@huawei.com>
 <1649083990-207133-3-git-send-email-john.garry@huawei.com>
From:   Damien Le Moal <damien.lemoal@opensource.wdc.com>
Organization: Western Digital Research
In-Reply-To: <1649083990-207133-3-git-send-email-john.garry@huawei.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 4/4/22 23:53, John Garry wrote:
> From: Christoph Hellwig <hch@lst.de>
> 
> It is a bit pointless to have ata_qc_new_init() in libata-core.c since it
> pokes scsi internals, so inline it in ata_scsi_qc_new() (in libata-scsi.c).
> 
> <Christoph, please provide signed-off-by>
> [jpg, Take Christoph's change from list and form into a patch]
> Signed-off-by: John Garry <john.garry@huawei.com>
> ---
>   Documentation/driver-api/libata.rst | 11 -------
>   drivers/ata/libata-core.c           | 44 +---------------------------
>   drivers/ata/libata-sata.c           |  8 -----
>   drivers/ata/libata-scsi.c           | 45 ++++++++++++++++++++++-------
>   drivers/ata/libata.h                | 12 --------
>   5 files changed, 35 insertions(+), 85 deletions(-)
> 
> diff --git a/Documentation/driver-api/libata.rst b/Documentation/driver-api/libata.rst
> index d477e296bda5..311af516a3fd 100644
> --- a/Documentation/driver-api/libata.rst
> +++ b/Documentation/driver-api/libata.rst
> @@ -424,12 +424,6 @@ How commands are issued
>   -----------------------
>   
>   Internal commands
> -    First, qc is allocated and initialized using :c:func:`ata_qc_new_init`.
> -    Although :c:func:`ata_qc_new_init` doesn't implement any wait or retry
> -    mechanism when qc is not available, internal commands are currently
> -    issued only during initialization and error recovery, so no other
> -    command is active and allocation is guaranteed to succeed.
> -
>       Once allocated qc's taskfile is initialized for the command to be
>       executed. qc currently has two mechanisms to notify completion. One
>       is via ``qc->complete_fn()`` callback and the other is completion
> @@ -447,11 +441,6 @@ SCSI commands
>       translated. No qc is involved in processing a simulated scmd. The
>       result is computed right away and the scmd is completed.
>   
> -    For a translated scmd, :c:func:`ata_qc_new_init` is invoked to allocate a
> -    qc and the scmd is translated into the qc. SCSI midlayer's
> -    completion notification function pointer is stored into
> -    ``qc->scsidone``.
> -
>       ``qc->complete_fn()`` callback is used for completion notification. ATA
>       commands use :c:func:`ata_scsi_qc_complete` while ATAPI commands use
>       :c:func:`atapi_qc_complete`. Both functions end up calling ``qc->scsidone``
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 1067b2e2be28..5e7d6ccad5da 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -4563,43 +4563,6 @@ void swap_buf_le16(u16 *buf, unsigned int buf_words)
>   #endif /* __BIG_ENDIAN */
>   }
>   
> -/**
> - *	ata_qc_new_init - Request an available ATA command, and initialize it
> - *	@dev: Device from whom we request an available command structure
> - *	@scmd: scmd for which to get qc
> - *
> - *	LOCKING:
> - *	None.
> - */
> -
> -struct ata_queued_cmd *ata_qc_new_init(struct ata_device *dev, struct scsi_cmnd *scmd)
> -{
> -	int tag = scsi_cmd_to_rq(scmd)->tag;
> -	struct ata_port *ap = dev->link->ap;
> -	struct ata_queued_cmd *qc;
> -
> -	/* no command while frozen */
> -	if (unlikely(ap->pflags & ATA_PFLAG_FROZEN))
> -		return NULL;
> -
> -	/* libsas case */
> -	if (ap->flags & ATA_FLAG_SAS_HOST) {
> -		tag = ata_sas_get_tag(scmd);
> -		if (tag < 0)
> -			return NULL;
> -	}
> -
> -	qc = __ata_qc_from_tag(ap, tag);
> -	qc->tag = qc->hw_tag = tag;
> -	qc->scsicmd = NULL;
> -	qc->ap = ap;
> -	qc->dev = dev;
> -
> -	ata_qc_reinit(qc);
> -
> -	return qc;
> -}
> -
>   /**
>    *	ata_qc_free - free unused ata_queued_cmd
>    *	@qc: Command to complete
> @@ -4612,13 +4575,8 @@ struct ata_queued_cmd *ata_qc_new_init(struct ata_device *dev, struct scsi_cmnd
>    */
>   void ata_qc_free(struct ata_queued_cmd *qc)
>   {
> -	unsigned int tag;
> -
> -	WARN_ON_ONCE(qc == NULL); /* ata_qc_from_tag _might_ return NULL */
> -
>   	qc->flags = 0;
> -	tag = qc->tag;
> -	if (ata_tag_valid(tag))
> +	if (ata_tag_valid(qc->tag))
>   		qc->tag = ATA_TAG_POISON;
>   }
>   
> diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
> index c3e9fd7d920c..7a5fe41aa5ae 100644
> --- a/drivers/ata/libata-sata.c
> +++ b/drivers/ata/libata-sata.c
> @@ -1268,14 +1268,6 @@ int ata_sas_queuecmd(struct scsi_cmnd *cmd, struct ata_port *ap)
>   }
>   EXPORT_SYMBOL_GPL(ata_sas_queuecmd);
>   
> -int ata_sas_get_tag(struct scsi_cmnd *scmd)
> -{
> -	if (WARN_ON_ONCE(scmd->budget_token >= ATA_MAX_QUEUE))
> -		return -1;
> -
> -	return scmd->budget_token;
> -}
> -
>   /**
>    *	sata_async_notification - SATA async notification handler
>    *	@ap: ATA port where async notification is received
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index 61dd7f7c7743..50ef132ec48c 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -638,24 +638,47 @@ EXPORT_SYMBOL_GPL(ata_scsi_ioctl);
>   static struct ata_queued_cmd *ata_scsi_qc_new(struct ata_device *dev,
>   					      struct scsi_cmnd *cmd)
>   {
> +	struct ata_port *ap = dev->link->ap;
>   	struct ata_queued_cmd *qc;
> +	int tag;
>   
> -	qc = ata_qc_new_init(dev, cmd);
> -	if (qc) {
> -		qc->scsicmd = cmd;
> -		qc->scsidone = scsi_done;
> -
> -		qc->sg = scsi_sglist(cmd);
> -		qc->n_elem = scsi_sg_count(cmd);
> +	if (unlikely(ap->pflags & ATA_PFLAG_FROZEN))
> +		goto fail;
>   
> -		if (scsi_cmd_to_rq(cmd)->rq_flags & RQF_QUIET)
> -			qc->flags |= ATA_QCFLAG_QUIET;
> +	if (ap->flags & ATA_FLAG_SAS_HOST) {
> +		/*
> +		 * SAS hosts may queue > ATA_MAX_QUEUE commands so use
> +		 * unique per-device budget token as a tag.
> +		 */
> +		if (WARN_ON_ONCE(cmd->budget_token >= ATA_MAX_QUEUE))
> +			goto fail;
> +		tag = cmd->budget_token;
>   	} else {
> -		cmd->result = (DID_OK << 16) | SAM_STAT_TASK_SET_FULL;
> -		scsi_done(cmd);
> +		tag = scsi_cmd_to_rq(cmd)->tag;
>   	}
>   
> +	qc = __ata_qc_from_tag(ap, tag);
> +	qc->tag = qc->hw_tag = tag;
> +	qc->scsicmd = NULL;
> +	qc->ap = ap;
> +	qc->dev = dev;
> +
> +	ata_qc_reinit(qc);
> +
> +	qc->scsicmd = cmd;
> +	qc->scsidone = scsi_done;
> +
> +	qc->sg = scsi_sglist(cmd);
> +	qc->n_elem = scsi_sg_count(cmd);
> +
> +	if (scsi_cmd_to_rq(cmd)->rq_flags & RQF_QUIET)
> +		qc->flags |= ATA_QCFLAG_QUIET;

Please add a blank line here.
I like to have return statements stand out :)

>   	return qc;
> +
> +fail:
> +	cmd->result = (DID_OK << 16) | SAM_STAT_TASK_SET_FULL;

While at it, it may be better to use:

	set_host_byte(cmd, DID_OK);
	set_status_byte(cmd, SAM_STAT_TASK_SET_FULL);

> +	scsi_done(cmd);
> +	return NULL;
>   }
>   
>   static void ata_qc_set_pc_nbytes(struct ata_queued_cmd *qc)
> diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
> index 92e52090165b..926a7f41303d 100644
> --- a/drivers/ata/libata.h
> +++ b/drivers/ata/libata.h
> @@ -44,7 +44,6 @@ static inline void ata_force_cbl(struct ata_port *ap) { }
>   #endif
>   extern u64 ata_tf_to_lba(const struct ata_taskfile *tf);
>   extern u64 ata_tf_to_lba48(const struct ata_taskfile *tf);
> -extern struct ata_queued_cmd *ata_qc_new_init(struct ata_device *dev, struct scsi_cmnd *scmd);
>   extern int ata_build_rw_tf(struct ata_taskfile *tf, struct ata_device *dev,
>   			   u64 block, u32 n_block, unsigned int tf_flags,
>   			   unsigned int tag, int class);
> @@ -91,17 +90,6 @@ extern unsigned int ata_read_log_page(struct ata_device *dev, u8 log,
>   
>   #define to_ata_port(d) container_of(d, struct ata_port, tdev)
>   
> -/* libata-sata.c */
> -#ifdef CONFIG_SATA_HOST
> -int ata_sas_get_tag(struct scsi_cmnd *scmd);
> -#else
> -static inline int ata_sas_get_tag(struct scsi_cmnd *scmd)
> -{
> -	return -EOPNOTSUPP;
> -}
> -static inline void ata_sas_free_tag(unsigned int tag, struct ata_port *ap) { }
> -#endif
> -
>   /* libata-acpi.c */
>   #ifdef CONFIG_ATA_ACPI
>   extern unsigned int ata_acpi_gtf_filter;


-- 
Damien Le Moal
Western Digital Research