[PATCH v2 0/2] libata: A tagging improvement and related code reorg

[PATCH v2 0/2] libata: A tagging improvement and related code reorg

[John Garry]

This is a follow-up to the series at [0] to fix SCSI device tagging and
use the per-device token as the libata qc tag.

I added the changes Christoph shared on the list to move
the functionality from ata_qc_new_init() into libata-scsi.c

Please do not apply on a kernel which does not include
"scsi: core: Fix sbitmap depth in scsi_realloc_sdev_budget_map()"

v5.18-rc2 should be the release to include that patch.

[0] https://lore.kernel.org/linux-ide/1647340746-17600-1-git-send-email-john.garry@huawei.com/

Christoph Hellwig (1):
  libata: Inline ata_qc_new_init() in ata_scsi_qc_new()

John Garry (1):
  libata: Use scsi cmnd budget token for qc tag for SAS host

 Documentation/driver-api/libata.rst | 11 -------
 drivers/ata/libata-core.c           | 48 +----------------------------
 drivers/ata/libata-sata.c           | 25 ---------------
 drivers/ata/libata-scsi.c           | 45 ++++++++++++++++++++-------
 drivers/ata/libata.h                | 13 --------
 include/linux/libata.h              |  1 -
 6 files changed, 35 insertions(+), 108 deletions(-)

-- 
2.26.2

[PATCH v2 1/2] libata: Use scsi cmnd budget token for qc tag for SAS host

[John Garry]

For attaining a qc tag for a SAS host we need to allocate a bit in
ata_port.sas_tag_allocated bitmap.

However we already have a unique tag per device in range
[0, ATA_MAX_QUEUE) in the scsi cmnd budget token, so just use that
instead.

Signed-off-by: John Garry <john.garry@huawei.com>
---
 drivers/ata/libata-core.c | 14 +++++---------
 drivers/ata/libata-sata.c | 25 ++++---------------------
 drivers/ata/libata-scsi.c |  2 +-
 drivers/ata/libata.h      |  7 +++----
 include/linux/libata.h    |  1 -
 5 files changed, 13 insertions(+), 36 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index cceedde51126..1067b2e2be28 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -4566,14 +4566,15 @@ void swap_buf_le16(u16 *buf, unsigned int buf_words)
 /**
  *	ata_qc_new_init - Request an available ATA command, and initialize it
  *	@dev: Device from whom we request an available command structure
- *	@tag: tag
+ *	@scmd: scmd for which to get qc
  *
  *	LOCKING:
  *	None.
  */
 
-struct ata_queued_cmd *ata_qc_new_init(struct ata_device *dev, int tag)
+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;
 
@@ -4583,7 +4584,7 @@ struct ata_queued_cmd *ata_qc_new_init(struct ata_device *dev, int tag)
 
 	/* libsas case */
 	if (ap->flags & ATA_FLAG_SAS_HOST) {
-		tag = ata_sas_allocate_tag(ap);
+		tag = ata_sas_get_tag(scmd);
 		if (tag < 0)
 			return NULL;
 	}
@@ -4611,19 +4612,14 @@ struct ata_queued_cmd *ata_qc_new_init(struct ata_device *dev, int tag)
  */
 void ata_qc_free(struct ata_queued_cmd *qc)
 {
-	struct ata_port *ap;
 	unsigned int tag;
 
 	WARN_ON_ONCE(qc == NULL); /* ata_qc_from_tag _might_ return NULL */
-	ap = qc->ap;
 
 	qc->flags = 0;
 	tag = qc->tag;
-	if (ata_tag_valid(tag)) {
+	if (ata_tag_valid(tag))
 		qc->tag = ATA_TAG_POISON;
-		if (ap->flags & ATA_FLAG_SAS_HOST)
-			ata_sas_free_tag(tag, ap);
-	}
 }
 
 void __ata_qc_complete(struct ata_queued_cmd *qc)
diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
index 044a16daa2d4..c3e9fd7d920c 100644
--- a/drivers/ata/libata-sata.c
+++ b/drivers/ata/libata-sata.c
@@ -1268,29 +1268,12 @@ int ata_sas_queuecmd(struct scsi_cmnd *cmd, struct ata_port *ap)
 }
 EXPORT_SYMBOL_GPL(ata_sas_queuecmd);
 
-int ata_sas_allocate_tag(struct ata_port *ap)
+int ata_sas_get_tag(struct scsi_cmnd *scmd)
 {
-	unsigned int max_queue = ap->host->n_tags;
-	unsigned int i, tag;
+	if (WARN_ON_ONCE(scmd->budget_token >= ATA_MAX_QUEUE))
+		return -1;
 
-	for (i = 0, tag = ap->sas_last_tag + 1; i < max_queue; i++, tag++) {
-		tag = tag < max_queue ? tag : 0;
-
-		/* the last tag is reserved for internal command. */
-		if (ata_tag_internal(tag))
-			continue;
-
-		if (!test_and_set_bit(tag, &ap->sas_tag_allocated)) {
-			ap->sas_last_tag = tag;
-			return tag;
-		}
-	}
-	return -1;
-}
-
-void ata_sas_free_tag(unsigned int tag, struct ata_port *ap)
-{
-	clear_bit(tag, &ap->sas_tag_allocated);
+	return scmd->budget_token;
 }
 
 /**
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 06c9d90238d9..61dd7f7c7743 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -640,7 +640,7 @@ static struct ata_queued_cmd *ata_scsi_qc_new(struct ata_device *dev,
 {
 	struct ata_queued_cmd *qc;
 
-	qc = ata_qc_new_init(dev, scsi_cmd_to_rq(cmd)->tag);
+	qc = ata_qc_new_init(dev, cmd);
 	if (qc) {
 		qc->scsicmd = cmd;
 		qc->scsidone = scsi_done;
diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
index c9c2496d91ea..92e52090165b 100644
--- a/drivers/ata/libata.h
+++ b/drivers/ata/libata.h
@@ -44,7 +44,7 @@ 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, int tag);
+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);
@@ -93,10 +93,9 @@ extern unsigned int ata_read_log_page(struct ata_device *dev, u8 log,
 
 /* libata-sata.c */
 #ifdef CONFIG_SATA_HOST
-int ata_sas_allocate_tag(struct ata_port *ap);
-void ata_sas_free_tag(unsigned int tag, struct ata_port *ap);
+int ata_sas_get_tag(struct scsi_cmnd *scmd);
 #else
-static inline int ata_sas_allocate_tag(struct ata_port *ap)
+static inline int ata_sas_get_tag(struct scsi_cmnd *scmd)
 {
 	return -EOPNOTSUPP;
 }
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 9b1d3d8b1252..16107122e587 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -820,7 +820,6 @@ struct ata_port {
 	unsigned int		cbl;	/* cable type; ATA_CBL_xxx */
 
 	struct ata_queued_cmd	qcmd[ATA_MAX_QUEUE + 1];
-	unsigned long		sas_tag_allocated; /* for sas tag allocation only */
 	u64			qc_active;
 	int			nr_active_links; /* #links with active qcs */
 	unsigned int		sas_last_tag;	/* track next tag hw expects */
-- 
2.26.2

Re: [PATCH v2 1/2] libata: Use scsi cmnd budget token for qc tag for SAS host

[Damien Le Moal]

On 4/4/22 23:53, John Garry wrote:
> For attaining a qc tag for a SAS host we need to allocate a bit in
> ata_port.sas_tag_allocated bitmap.
> 
> However we already have a unique tag per device in range
> [0, ATA_MAX_QUEUE) in the scsi cmnd budget token, so just use that
> instead.

The valid range is [0, ATA_MAX_QUEUE - 1]. Tag ATA_MAX_QUEUE is 
ATA_TAG_INTERNAL which is never allocated as a valid device tag but used 
directly in ata_exec_internal().

> 
> Signed-off-by: John Garry <john.garry@huawei.com>
> ---
>   drivers/ata/libata-core.c | 14 +++++---------
>   drivers/ata/libata-sata.c | 25 ++++---------------------
>   drivers/ata/libata-scsi.c |  2 +-
>   drivers/ata/libata.h      |  7 +++----
>   include/linux/libata.h    |  1 -
>   5 files changed, 13 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index cceedde51126..1067b2e2be28 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -4566,14 +4566,15 @@ void swap_buf_le16(u16 *buf, unsigned int buf_words)
>   /**
>    *	ata_qc_new_init - Request an available ATA command, and initialize it
>    *	@dev: Device from whom we request an available command structure
> - *	@tag: tag
> + *	@scmd: scmd for which to get qc
>    *
>    *	LOCKING:
>    *	None.
>    */
>   
> -struct ata_queued_cmd *ata_qc_new_init(struct ata_device *dev, int tag)
> +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;
>   
> @@ -4583,7 +4584,7 @@ struct ata_queued_cmd *ata_qc_new_init(struct ata_device *dev, int tag)
>   
>   	/* libsas case */
>   	if (ap->flags & ATA_FLAG_SAS_HOST) {
> -		tag = ata_sas_allocate_tag(ap);
> +		tag = ata_sas_get_tag(scmd);
>   		if (tag < 0)
>   			return NULL;
>   	}
> @@ -4611,19 +4612,14 @@ struct ata_queued_cmd *ata_qc_new_init(struct ata_device *dev, int tag)
>    */
>   void ata_qc_free(struct ata_queued_cmd *qc)
>   {
> -	struct ata_port *ap;
>   	unsigned int tag;
>   
>   	WARN_ON_ONCE(qc == NULL); /* ata_qc_from_tag _might_ return NULL */
> -	ap = qc->ap;
>   
>   	qc->flags = 0;
>   	tag = qc->tag;
> -	if (ata_tag_valid(tag)) {
> +	if (ata_tag_valid(tag))
>   		qc->tag = ATA_TAG_POISON;
> -		if (ap->flags & ATA_FLAG_SAS_HOST)
> -			ata_sas_free_tag(tag, ap);
> -	}
>   }
>   
>   void __ata_qc_complete(struct ata_queued_cmd *qc)
> diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
> index 044a16daa2d4..c3e9fd7d920c 100644
> --- a/drivers/ata/libata-sata.c
> +++ b/drivers/ata/libata-sata.c
> @@ -1268,29 +1268,12 @@ int ata_sas_queuecmd(struct scsi_cmnd *cmd, struct ata_port *ap)
>   }
>   EXPORT_SYMBOL_GPL(ata_sas_queuecmd);
>   
> -int ata_sas_allocate_tag(struct ata_port *ap)
> +int ata_sas_get_tag(struct scsi_cmnd *scmd)
>   {
> -	unsigned int max_queue = ap->host->n_tags;
> -	unsigned int i, tag;
> +	if (WARN_ON_ONCE(scmd->budget_token >= ATA_MAX_QUEUE))
> +		return -1;
>   
> -	for (i = 0, tag = ap->sas_last_tag + 1; i < max_queue; i++, tag++) {
> -		tag = tag < max_queue ? tag : 0;
> -
> -		/* the last tag is reserved for internal command. */
> -		if (ata_tag_internal(tag))
> -			continue;
> -
> -		if (!test_and_set_bit(tag, &ap->sas_tag_allocated)) {
> -			ap->sas_last_tag = tag;
> -			return tag;
> -		}
> -	}
> -	return -1;
> -}
> -
> -void ata_sas_free_tag(unsigned int tag, struct ata_port *ap)
> -{
> -	clear_bit(tag, &ap->sas_tag_allocated);
> +	return scmd->budget_token;
>   }
>   
>   /**
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index 06c9d90238d9..61dd7f7c7743 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -640,7 +640,7 @@ static struct ata_queued_cmd *ata_scsi_qc_new(struct ata_device *dev,
>   {
>   	struct ata_queued_cmd *qc;
>   
> -	qc = ata_qc_new_init(dev, scsi_cmd_to_rq(cmd)->tag);
> +	qc = ata_qc_new_init(dev, cmd);
>   	if (qc) {
>   		qc->scsicmd = cmd;
>   		qc->scsidone = scsi_done;
> diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
> index c9c2496d91ea..92e52090165b 100644
> --- a/drivers/ata/libata.h
> +++ b/drivers/ata/libata.h
> @@ -44,7 +44,7 @@ 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, int tag);
> +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);
> @@ -93,10 +93,9 @@ extern unsigned int ata_read_log_page(struct ata_device *dev, u8 log,
>   
>   /* libata-sata.c */
>   #ifdef CONFIG_SATA_HOST
> -int ata_sas_allocate_tag(struct ata_port *ap);
> -void ata_sas_free_tag(unsigned int tag, struct ata_port *ap);
> +int ata_sas_get_tag(struct scsi_cmnd *scmd);
>   #else
> -static inline int ata_sas_allocate_tag(struct ata_port *ap)
> +static inline int ata_sas_get_tag(struct scsi_cmnd *scmd)
>   {
>   	return -EOPNOTSUPP;
>   }
> diff --git a/include/linux/libata.h b/include/linux/libata.h
> index 9b1d3d8b1252..16107122e587 100644
> --- a/include/linux/libata.h
> +++ b/include/linux/libata.h
> @@ -820,7 +820,6 @@ struct ata_port {
>   	unsigned int		cbl;	/* cable type; ATA_CBL_xxx */
>   
>   	struct ata_queued_cmd	qcmd[ATA_MAX_QUEUE + 1];
> -	unsigned long		sas_tag_allocated; /* for sas tag allocation only */
>   	u64			qc_active;
>   	int			nr_active_links; /* #links with active qcs */
>   	unsigned int		sas_last_tag;	/* track next tag hw expects */


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2 1/2] libata: Use scsi cmnd budget token for qc tag for SAS host

[John Garry]

On 06/04/2022 02:39, Damien Le Moal wrote:
> On 4/4/22 23:53, John Garry wrote:
>> For attaining a qc tag for a SAS host we need to allocate a bit in
>> ata_port.sas_tag_allocated bitmap.
>>
>> However we already have a unique tag per device in range
>> [0, ATA_MAX_QUEUE) in the scsi cmnd budget token, so just use that
>> instead.
> 
> The valid range is [0, ATA_MAX_QUEUE - 1]. Tag ATA_MAX_QUEUE is 
> ATA_TAG_INTERNAL which is never allocated as a valid device tag but used 
> directly in ata_exec_internal().

But that is what I have in [0, ATA_MAX_QUEUE), which is same as [0, 
ATA_MAX_QUEUE - 1].

Thanks,
john

Re: [PATCH v2 1/2] libata: Use scsi cmnd budget token for qc tag for SAS host

[Damien Le Moal]

On 4/6/22 16:12, John Garry wrote:
> On 06/04/2022 02:39, Damien Le Moal wrote:
>> On 4/4/22 23:53, John Garry wrote:
>>> For attaining a qc tag for a SAS host we need to allocate a bit in
>>> ata_port.sas_tag_allocated bitmap.
>>>
>>> However we already have a unique tag per device in range
>>> [0, ATA_MAX_QUEUE) in the scsi cmnd budget token, so just use that
>>> instead.
>>
>> The valid range is [0, ATA_MAX_QUEUE - 1]. Tag ATA_MAX_QUEUE is 
>> ATA_TAG_INTERNAL which is never allocated as a valid device tag but 
>> used directly in ata_exec_internal().
> 
> But that is what I have in [0, ATA_MAX_QUEUE), which is same as [0, 
> ATA_MAX_QUEUE - 1].

Oh ! I missed the ")" !
I would prefer an explicit [0, ATA_MAX_QUEUE - 1] to be clear :)

> 
> Thanks,
> john


-- 
Damien Le Moal
Western Digital Research

[PATCH v2 2/2] libata: Inline ata_qc_new_init() in ata_scsi_qc_new()

[John Garry]

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;
 	return qc;
+
+fail:
+	cmd->result = (DID_OK << 16) | 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;
-- 
2.26.2

Re: [PATCH v2 2/2] libata: Inline ata_qc_new_init() in ata_scsi_qc_new()

[Christoph Hellwig]

On Mon, Apr 04, 2022 at 10:53:10PM +0800, 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>

Signed-off-by: Christoph Hellwig <hch@lst.de>

Although I still think merging the two patches into one to avoid all
the churn would be much better.

Re: [PATCH v2 2/2] libata: Inline ata_qc_new_init() in ata_scsi_qc_new()

[Damien Le Moal]

On 4/5/22 14:52, Christoph Hellwig wrote:
> On Mon, Apr 04, 2022 at 10:53:10PM +0800, 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>
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> Although I still think merging the two patches into one to avoid all
> the churn would be much better.

I agree. Let's merge these 2 patches.


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2 2/2] libata: Inline ata_qc_new_init() in ata_scsi_qc_new()

[John Garry]

On 06/04/2022 02:48, Damien Le Moal wrote:
> On 4/5/22 14:52, Christoph Hellwig wrote:
>> On Mon, Apr 04, 2022 at 10:53:10PM +0800, 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>
>>
>> Signed-off-by: Christoph Hellwig <hch@lst.de>
>>
>> Although I still think merging the two patches into one to avoid all
>> the churn would be much better.
> 
> I agree. Let's merge these 2 patches.

I'd say that they are distinct changes.

Anyway, if that is the preference then who shall be the author? 
Considering I did most effort I will be and add Christoph as 
co-developed-by - please let me know if not ok.

thanks,
John

Re: [PATCH v2 2/2] libata: Inline ata_qc_new_init() in ata_scsi_qc_new()

[Damien Le Moal]

On 4/6/22 16:16, John Garry wrote:
> On 06/04/2022 02:48, Damien Le Moal wrote:
>> On 4/5/22 14:52, Christoph Hellwig wrote:
>>> On Mon, Apr 04, 2022 at 10:53:10PM +0800, 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>
>>>
>>> Signed-off-by: Christoph Hellwig <hch@lst.de>
>>>
>>> Although I still think merging the two patches into one to avoid all
>>> the churn would be much better.
>>
>> I agree. Let's merge these 2 patches.
> 
> I'd say that they are distinct changes.
> 
> Anyway, if that is the preference then who shall be the author? 
> Considering I did most effort I will be and add Christoph as 
> co-developed-by - please let me know if not ok.

Works for me.

> 
> thanks,
> John


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2 2/2] libata: Inline ata_qc_new_init() in ata_scsi_qc_new()

[Christoph Hellwig]

On Wed, Apr 06, 2022 at 08:16:25AM +0100, John Garry wrote:
> Anyway, if that is the preference then who shall be the author? Considering 
> I did most effort I will be and add Christoph as co-developed-by - please 
> let me know if not ok.

I think the co-developed-by is a bit silly.  Just attribute it to you.

Re: [PATCH v2 2/2] libata: Inline ata_qc_new_init() in ata_scsi_qc_new()

[Damien Le Moal]

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