On Fri, Jan 16, 2015 at 12:23:44AM -0800, Christoph Hellwig wrote:
> > +static bool ata_valid_internal_tag(struct ata_port *ap, struct ata_device *dev,
> > + unsigned int tag)
> > +{
> > + if (!ap->scsi_host)
> > + return !test_and_set_bit(tag, &ap->sas_tag_allocated);
> > + return !dev->sdev ||
> > + !blk_queue_find_tag(dev->sdev->request_queue, tag);
>
> How is this supposed to work with blk-mq?
>
> > +}
> > +
> > /**
> > * ata_exec_internal_sg - execute libata internal command
> > * @dev: Device to which the command is sent
> > @@ -1585,8 +1594,7 @@ unsigned ata_exec_internal_sg(struct ata_device *dev,
> > else
> > tag = 0;
> >
> > - if (test_and_set_bit(tag, &ap->qc_allocated))
> > - BUG();
> > + BUG_ON(!ata_valid_internal_tag(ap, dev, tag));
>
> But given that this is just a single assert we might as well just
> remove that whole thing.
Ok, I delete it
> > +static struct ata_queued_cmd *ata_qc_new(struct ata_port *ap, int blktag)
> > +{
> > + struct ata_queued_cmd *qc;
> > +
> > + /* no command while frozen */
> > + if (unlikely(ap->pflags & ATA_PFLAG_FROZEN))
> > + return NULL;
> > +
> > + /* SATA will directly use block tag. libsas need its own tag management */
> > + if (ap->scsi_host) {
> > + qc = __ata_qc_from_tag(ap, blktag);
> > + qc->tag = blktag;
> > + return qc;
> > + }
> > +
> > + return sas_ata_qc_new(ap);
>
> Why don't you merge tis into ata_qc_new_init?
>
> > +static void sas_ata_qc_free(unsigned int tag, struct ata_port *ap)
> > +{
> > + if (!ap->scsi_host)
>
> Given the sas_ name I'd move the check into the caller.
>
> That beeing said I wonder if you shouldn't do all this at a much
> higher level.
>
> __ata_scsi_queuecmd has a two callers, one for sata, and one for sas.
> If you pass in the tag from that level the low lever code won't
> need any branches for sas vs libata. The only downside would be
> that you now consume a tag for simulated command on SAS, but as that's
> already done for SATA it can't be that bad.
I can do this in __ata_scsi_queuecmd, but don't see a good place to free
the tag, which really should be done at scsi_done. The whole thing
should be done at a higher level. I moved the ata tag allocation code to
libata-scsi.c to make it clearer.
>From 125d41a001b8555fac42ea5b35dae45049655ab0 Mon Sep 17 00:00:00 2001
Message-Id: <125d41a001b8555fac42ea5b35dae45049655ab0.1421446044.git.shli@xxxxxx>
In-Reply-To: <81b8ef84122544faa1f37a404cf2e7c71791d961.1421446044.git.shli@xxxxxx>
References: <81b8ef84122544faa1f37a404cf2e7c71791d961.1421446044.git.shli@xxxxxx>
From: Shaohua Li <shli@xxxxxx>
Date: Thu, 18 Dec 2014 09:04:25 -0800
Subject: [PATCH 3/3] libata: use blk taging
libata uses its own tag management which is duplication and the
implementation is poor. And if we switch to blk-mq, tag is build-in.
It's time to switch to generic taging.
The SAS driver has its own tag management, and looks we can't directly
map the host controler tag to SATA tag. So I just bypassed the SAS case.
I changed the code/variable name for the tag management of libata to
make it self contained. Only sas will use it. Later if libsas implements
its tag management, the tag management code in libata can be deleted
easily.
Cc: Jens Axboe <axboe@xxxxxx>
Cc: Tejun Heo <tj@xxxxxxxxxx>
Cc: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Signed-off-by: Shaohua Li <shli@xxxxxx>
---
drivers/ata/libata-core.c | 67 +++++++++++++----------------------------------
drivers/ata/libata-scsi.c | 30 ++++++++++++++++++++-
drivers/ata/libata.h | 4 ++-
include/linux/libata.h | 5 ++--
4 files changed, 53 insertions(+), 53 deletions(-)
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 5c84fb5..d626605 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -1585,8 +1585,6 @@ unsigned ata_exec_internal_sg(struct ata_device *dev,
else
tag = 0;
- if (test_and_set_bit(tag, &ap->qc_allocated))
- BUG();
qc = __ata_qc_from_tag(ap, tag);
qc->tag = tag;
@@ -4726,66 +4724,36 @@ void swap_buf_le16(u16 *buf, unsigned int buf_words)
}
/**
- * ata_qc_new - Request an available ATA command, for queueing
- * @ap: target port
- *
- * Some ATA host controllers may implement a queue depth which is less
- * than ATA_MAX_QUEUE. So we shouldn't allocate a tag which is beyond
- * the hardware limitation.
+ * ata_qc_new_init - Request an available ATA command, and initialize it
+ * @dev: Device from whom we request an available command structure
*
* LOCKING:
* None.
*/
-static struct ata_queued_cmd *ata_qc_new(struct ata_port *ap)
+struct ata_queued_cmd *ata_qc_new_init(struct ata_device *dev, int tag)
{
- struct ata_queued_cmd *qc = NULL;
- unsigned int max_queue = ap->host->n_tags;
- unsigned int i, 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;
- for (i = 0, tag = ap->last_tag + 1; i < max_queue; i++, tag++) {
- tag = tag < max_queue ? tag : 0;
-
- /* the last tag is reserved for internal command. */
- if (tag == ATA_TAG_INTERNAL)
- continue;
-
- if (!test_and_set_bit(tag, &ap->qc_allocated)) {
- qc = __ata_qc_from_tag(ap, tag);
- qc->tag = tag;
- ap->last_tag = tag;
- break;
- }
+ /* libsas case */
+ if (!ap->scsi_host) {
+ tag = ata_sas_allocate_tag(ap);
+ if (tag < 0)
+ return NULL;
}
- return qc;
-}
-
-/**
- * ata_qc_new_init - Request an available ATA command, and initialize it
- * @dev: Device from whom we request an available command structure
- *
- * LOCKING:
- * None.
- */
-
-struct ata_queued_cmd *ata_qc_new_init(struct ata_device *dev)
-{
- struct ata_port *ap = dev->link->ap;
- struct ata_queued_cmd *qc;
-
- qc = ata_qc_new(ap);
- if (qc) {
- qc->scsicmd = NULL;
- qc->ap = ap;
- qc->dev = dev;
+ qc = __ata_qc_from_tag(ap, tag);
+ qc->tag = tag;
+ qc->scsicmd = NULL;
+ qc->ap = ap;
+ qc->dev = dev;
- ata_qc_reinit(qc);
- }
+ ata_qc_reinit(qc);
return qc;
}
@@ -4812,7 +4780,8 @@ void ata_qc_free(struct ata_queued_cmd *qc)
tag = qc->tag;
if (likely(ata_tag_valid(tag))) {
qc->tag = ATA_TAG_POISON;
- clear_bit(tag, &ap->qc_allocated);
+ if (!ap->scsi_host)
+ ata_sas_free_tag(tag, ap);
}
}
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index e364e86..59c9d72 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -756,7 +756,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);
+ qc = ata_qc_new_init(dev, cmd->request->tag);
if (qc) {
qc->scsicmd = cmd;
qc->scsidone = cmd->scsi_done;
@@ -3666,6 +3666,9 @@ int ata_scsi_add_hosts(struct ata_host *host, struct scsi_host_template *sht)
*/
shost->max_host_blocked = 1;
+ if (scsi_init_shared_tag_map(shost, host->n_tags))
+ goto err_add;
+
rc = scsi_add_host_with_dma(ap->scsi_host,
&ap->tdev, ap->host->dev);
if (rc)
@@ -4228,3 +4231,28 @@ int ata_sas_queuecmd(struct scsi_cmnd *cmd, struct ata_port *ap)
return rc;
}
EXPORT_SYMBOL_GPL(ata_sas_queuecmd);
+
+int ata_sas_allocate_tag(struct ata_port *ap)
+{
+ unsigned int max_queue = ap->host->n_tags;
+ unsigned int i, tag;
+
+ 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 (tag == ATA_TAG_INTERNAL)
+ 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);
+}
diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
index 5f4e0cc..8c491cd 100644
--- a/drivers/ata/libata.h
+++ b/drivers/ata/libata.h
@@ -63,7 +63,7 @@ extern struct ata_link *ata_dev_phys_link(struct ata_device *dev);
extern void ata_force_cbl(struct ata_port *ap);
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);
+extern struct ata_queued_cmd *ata_qc_new_init(struct ata_device *dev, int tag);
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);
@@ -145,6 +145,8 @@ extern void ata_scsi_dev_rescan(struct work_struct *work);
extern int ata_bus_probe(struct ata_port *ap);
extern int ata_scsi_user_scan(struct Scsi_Host *shost, unsigned int channel,
unsigned int id, u64 lun);
+int ata_sas_allocate_tag(struct ata_port *ap);
+void ata_sas_free_tag(unsigned int tag, struct ata_port *ap);
/* libata-eh.c */
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 2d18241..f234547 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -821,10 +821,10 @@ struct ata_port {
unsigned int cbl; /* cable type; ATA_CBL_xxx */
struct ata_queued_cmd qcmd[ATA_MAX_QUEUE];
- unsigned long qc_allocated;
+ unsigned long sas_tag_allocated; /* for sas tag allocation only */
unsigned int qc_active;
int nr_active_links; /* #links with active qcs */
- unsigned int last_tag; /* track next tag hw expects */
+ unsigned int sas_last_tag; /* track next tag hw expects */
struct ata_link link; /* host default link */
struct ata_link *slave_link; /* see ata_slave_link_init() */
@@ -1344,6 +1344,7 @@ extern struct device_attribute *ata_common_sdev_attrs[];
.ioctl = ata_scsi_ioctl, \
.queuecommand = ata_scsi_queuecmd, \
.can_queue = ATA_DEF_QUEUE, \
+ .tag_alloc_policy = BLK_TAG_ALLOC_RR, \
.this_id = ATA_SHT_THIS_ID, \
.cmd_per_lun = ATA_SHT_CMD_PER_LUN, \
.emulated = ATA_SHT_EMULATED, \
--
1.8.1
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
