* [PATCH v2 0/2] libata: A tagging improvement and related code reorg
@ 2022-04-04 14:53 John Garry
2022-04-04 14:53 ` [PATCH v2 1/2] libata: Use scsi cmnd budget token for qc tag for SAS host John Garry
2022-04-04 14:53 ` [PATCH v2 2/2] libata: Inline ata_qc_new_init() in ata_scsi_qc_new() John Garry
0 siblings, 2 replies; 12+ messages in thread
From: John Garry @ 2022-04-04 14:53 UTC (permalink / raw)
To: damien.lemoal, hch
Cc: linux-kernel, linux-ide, linux-doc, linux-scsi, 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
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2 1/2] libata: Use scsi cmnd budget token for qc tag for SAS host
2022-04-04 14:53 [PATCH v2 0/2] libata: A tagging improvement and related code reorg John Garry
@ 2022-04-04 14:53 ` John Garry
2022-04-06 1:39 ` Damien Le Moal
2022-04-04 14:53 ` [PATCH v2 2/2] libata: Inline ata_qc_new_init() in ata_scsi_qc_new() John Garry
1 sibling, 1 reply; 12+ messages in thread
From: John Garry @ 2022-04-04 14:53 UTC (permalink / raw)
To: damien.lemoal, hch
Cc: linux-kernel, linux-ide, linux-doc, linux-scsi, 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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v2 2/2] libata: Inline ata_qc_new_init() in ata_scsi_qc_new()
2022-04-04 14:53 [PATCH v2 0/2] libata: A tagging improvement and related code reorg John Garry
2022-04-04 14:53 ` [PATCH v2 1/2] libata: Use scsi cmnd budget token for qc tag for SAS host John Garry
@ 2022-04-04 14:53 ` John Garry
2022-04-05 5:52 ` Christoph Hellwig
2022-04-06 1:47 ` Damien Le Moal
1 sibling, 2 replies; 12+ messages in thread
From: John Garry @ 2022-04-04 14:53 UTC (permalink / raw)
To: damien.lemoal, hch
Cc: linux-kernel, linux-ide, linux-doc, linux-scsi, 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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/2] libata: Inline ata_qc_new_init() in ata_scsi_qc_new()
2022-04-04 14:53 ` [PATCH v2 2/2] libata: Inline ata_qc_new_init() in ata_scsi_qc_new() John Garry
@ 2022-04-05 5:52 ` Christoph Hellwig
2022-04-06 1:48 ` Damien Le Moal
2022-04-06 1:47 ` Damien Le Moal
1 sibling, 1 reply; 12+ messages in thread
From: Christoph Hellwig @ 2022-04-05 5:52 UTC (permalink / raw)
To: John Garry
Cc: damien.lemoal, hch, linux-kernel, linux-ide, linux-doc, linux-scsi
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.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/2] libata: Use scsi cmnd budget token for qc tag for SAS host
2022-04-04 14:53 ` [PATCH v2 1/2] libata: Use scsi cmnd budget token for qc tag for SAS host John Garry
@ 2022-04-06 1:39 ` Damien Le Moal
2022-04-06 7:12 ` John Garry
0 siblings, 1 reply; 12+ messages in thread
From: Damien Le Moal @ 2022-04-06 1:39 UTC (permalink / raw)
To: John Garry, hch; +Cc: linux-kernel, linux-ide, linux-doc, linux-scsi
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/2] libata: Inline ata_qc_new_init() in ata_scsi_qc_new()
2022-04-04 14:53 ` [PATCH v2 2/2] libata: Inline ata_qc_new_init() in ata_scsi_qc_new() John Garry
2022-04-05 5:52 ` Christoph Hellwig
@ 2022-04-06 1:47 ` Damien Le Moal
1 sibling, 0 replies; 12+ messages in thread
From: Damien Le Moal @ 2022-04-06 1:47 UTC (permalink / raw)
To: John Garry, hch; +Cc: linux-kernel, linux-ide, linux-doc, linux-scsi
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/2] libata: Inline ata_qc_new_init() in ata_scsi_qc_new()
2022-04-05 5:52 ` Christoph Hellwig
@ 2022-04-06 1:48 ` Damien Le Moal
2022-04-06 7:16 ` John Garry
0 siblings, 1 reply; 12+ messages in thread
From: Damien Le Moal @ 2022-04-06 1:48 UTC (permalink / raw)
To: Christoph Hellwig, John Garry
Cc: linux-kernel, linux-ide, linux-doc, linux-scsi
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/2] libata: Use scsi cmnd budget token for qc tag for SAS host
2022-04-06 1:39 ` Damien Le Moal
@ 2022-04-06 7:12 ` John Garry
2022-04-06 8:10 ` Damien Le Moal
0 siblings, 1 reply; 12+ messages in thread
From: John Garry @ 2022-04-06 7:12 UTC (permalink / raw)
To: Damien Le Moal, hch; +Cc: linux-kernel, linux-ide, linux-doc, linux-scsi
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/2] libata: Inline ata_qc_new_init() in ata_scsi_qc_new()
2022-04-06 1:48 ` Damien Le Moal
@ 2022-04-06 7:16 ` John Garry
2022-04-06 8:10 ` Damien Le Moal
2022-04-06 8:11 ` Christoph Hellwig
0 siblings, 2 replies; 12+ messages in thread
From: John Garry @ 2022-04-06 7:16 UTC (permalink / raw)
To: Damien Le Moal, Christoph Hellwig
Cc: linux-kernel, linux-ide, linux-doc, linux-scsi
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/2] libata: Use scsi cmnd budget token for qc tag for SAS host
2022-04-06 7:12 ` John Garry
@ 2022-04-06 8:10 ` Damien Le Moal
0 siblings, 0 replies; 12+ messages in thread
From: Damien Le Moal @ 2022-04-06 8:10 UTC (permalink / raw)
To: John Garry, hch; +Cc: linux-kernel, linux-ide, linux-doc, linux-scsi
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/2] libata: Inline ata_qc_new_init() in ata_scsi_qc_new()
2022-04-06 7:16 ` John Garry
@ 2022-04-06 8:10 ` Damien Le Moal
2022-04-06 8:11 ` Christoph Hellwig
1 sibling, 0 replies; 12+ messages in thread
From: Damien Le Moal @ 2022-04-06 8:10 UTC (permalink / raw)
To: John Garry, Christoph Hellwig
Cc: linux-kernel, linux-ide, linux-doc, linux-scsi
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/2] libata: Inline ata_qc_new_init() in ata_scsi_qc_new()
2022-04-06 7:16 ` John Garry
2022-04-06 8:10 ` Damien Le Moal
@ 2022-04-06 8:11 ` Christoph Hellwig
1 sibling, 0 replies; 12+ messages in thread
From: Christoph Hellwig @ 2022-04-06 8:11 UTC (permalink / raw)
To: John Garry
Cc: Damien Le Moal, Christoph Hellwig, linux-kernel, linux-ide,
linux-doc, linux-scsi
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.
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2022-04-06 14:52 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-04 14:53 [PATCH v2 0/2] libata: A tagging improvement and related code reorg John Garry
2022-04-04 14:53 ` [PATCH v2 1/2] libata: Use scsi cmnd budget token for qc tag for SAS host John Garry
2022-04-06 1:39 ` Damien Le Moal
2022-04-06 7:12 ` John Garry
2022-04-06 8:10 ` Damien Le Moal
2022-04-04 14:53 ` [PATCH v2 2/2] libata: Inline ata_qc_new_init() in ata_scsi_qc_new() John Garry
2022-04-05 5:52 ` Christoph Hellwig
2022-04-06 1:48 ` Damien Le Moal
2022-04-06 7:16 ` John Garry
2022-04-06 8:10 ` Damien Le Moal
2022-04-06 8:11 ` Christoph Hellwig
2022-04-06 1:47 ` Damien Le Moal
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.