On Thu, May 26 2005, Jeff Garzik wrote:
> > static void ahci_qc_prep(struct ata_queued_cmd *qc)
> > {
> > struct ahci_port_priv *pp = qc->ap->private_data;
> >- u32 opts;
> >+ void *port_mmio = (void *) qc->ap->ioaddr.cmd_addr;
> > const u32 cmd_fis_len = 5; /* five dwords */
> >+ dma_addr_t cmd_tbl_dma;
> >+ u32 opts;
> >+ int offset;
> >+
> >+ if (qc->flags & ATA_QCFLAG_NCQ) {
> >+ pp->sactive |= (1 << qc->tag);
> >+
> >+ writel(1 << qc->tag, port_mmio + PORT_SCR_ACT);
> >+ readl(port_mmio + PORT_SCR_ACT); /* flush */
> >+ }
>
> Wrong, you should do this in ahci_qc_issue not here.
Are you sure, I moved this on purpose? I think the reason I did this was
the wording at the back of the the sata-ii spec (appendix b) that says
something ala 'preset the active bit and transmit a register FIS'. Feel
free to point me at the authoritative wording in the ACHI spec.
One thing that I definitely think _was_ wrong with the sactive bit
before, is that you set it unconditionally of whether this was an NCQ
command or not. The maxtor drives don't clear sactive on non-fpdma
commands, which confused me at first.
> >+static void ahci_host_ncq_intr_err(struct ata_port *ap)
> >+{
> >+ struct ahci_port_priv *pp = ap->private_data;
> >+ void *mmio = ap->host_set->mmio_base;
> >+ void *port_mmio = ahci_port_base(mmio, ap->port_no);
> >+ unsigned long flags;
> >+ char *buffer;
> >+
> >+ printk(KERN_ERR "ata%u: ncq interrupt error\n", ap->id);
> >+
> >+ /*
> >+ * error all io first
> >+ */
> >+ spin_lock_irqsave(&ap->host_set->lock, flags);
> >+
> >+ while (pp->sactive) {
> >+ struct ata_queued_cmd *qc;
> >+ int tag = ffs(pp->sactive) - 1;
> >+
> >+ pp->sactive &= ~(1 << tag);
> >+ qc = ata_qc_from_tag(ap, tag);
> >+ if (qc)
> >+ ata_qc_complete(qc, ATA_ERR);
>
> else printk error "I couldn't find the tag!"
Agree.
> >@@ -632,18 +767,27 @@
> > status = readl(port_mmio + PORT_IRQ_STAT);
> > writel(status, port_mmio + PORT_IRQ_STAT);
> >
> >- ci = readl(port_mmio + PORT_CMD_ISSUE);
> >- if (likely((ci & 0x1) == 0)) {
> >- if (qc) {
> >- ata_qc_complete(qc, 0);
> >- qc = NULL;
> >+ if (ap->ncq_depth) {
> >+ if (!serr)
>
> incorrect test. serr is not the only error indicator.
Yes, I'm aware of that. There are still quite a few loose ends wrt error
handling, as mentioned. I also don't check the irq fatal stat in the ncq
interrupt handler.
> >@@ -1139,6 +1143,10 @@
> > goto err_out_nosup;
> > }
> >
> >+ if (ap->flags & ATA_FLAG_NCQ)
> >+ if (ata_id_queue_depth(dev->id) > 1)
> >+ dev->flags |= ATA_DFLAG_NCQ;
>
> This will turn on queueing for older TCQ devices that are plugged into
> an NCQ-capable board.
Yup, that is an error. At that time, I could not easily find what
feature bit encompassed NCQ, that needs to be added of course.
> >+int ata_read_log_page(struct ata_port *ap, unsigned int device, char page,
> >+ char *buffer, unsigned int sectors)
> >+{
> >+ struct ata_device *dev = &ap->device[device];
> >+ DECLARE_COMPLETION(wait);
> >+ struct ata_queued_cmd *qc;
> >+ unsigned long flags;
> >+ u8 status;
> >+ int rc;
> >+
> >+ assert(dev->class == ATA_DEV_ATA);
> >+
> >+ ata_dev_select(ap, device, 1, 1);
>
> is this needed? These types of calls need to be removed, in general, as
> they don't make sense on FIS-based hardware at all.
You tell me, this read_log_page() was mainly copy-pasted from the pio
driven function above it. I'll try and kill the select when doing error
testing.
> >+ printk("RLP issue\n");
> >+ spin_lock_irqsave(&ap->host_set->lock, flags);
> >+ rc = ata_qc_issue(qc);
> >+ spin_unlock_irqrestore(&ap->host_set->lock, flags);
> >+ printk("RLP issue done\n");
> >+
> >+ if (rc)
> >+ return -EIO;
> >+
> >+ wait_for_completion(&wait);
> >+
> >+ printk("RLP wait done\n");
> >+
> >+ status = ata_chk_status(ap);
> >+ if (status & (ATA_ERR | ATA_ABORTED))
> >+ return -EIO;
>
> we need to get rid of this too for AHCI-like devices
Can you expand on that?
> >@@ -2753,6 +2830,16 @@
> > struct ata_port *ap = qc->ap;
> > unsigned int tag, do_clear = 0;
> >
> >+ if (likely(qc->flags & ATA_QCFLAG_ACCOUNT)) {
> >+ if (qc->flags & ATA_QCFLAG_NCQ) {
> >+ assert(ap->ncq_depth);
> >+ ap->ncq_depth--;
> >+ } else {
> >+ assert(ap->depth);
> >+ ap->depth--;
> >+ }
> >+ }
>
> why is this accounting conditional?
Because if you free a qc before it has gone to the device, you don't
want to account it. Hmm, perhaps ACTIVE would work fine in fact. I'll
double check!
> > #ifdef CONFIG_PCI
> > EXPORT_SYMBOL_GPL(pci_test_config_bits);
> >Index: drivers/scsi/libata-scsi.c
> >===================================================================
> >--- f5c58b6b0cfd2a92fb3b1d1f4cbfdfb3df6f45d6/drivers/scsi/libata-scsi.c
> >(mode:100644)
> >+++ uncommitted/drivers/scsi/libata-scsi.c (mode:100644)
> >@@ -336,6 +336,7 @@
> > if (sdev->id < ATA_MAX_DEVICES) {
> > struct ata_port *ap;
> > struct ata_device *dev;
> >+ int depth;
> >
> > ap = (struct ata_port *) &sdev->host->hostdata[0];
> > dev = &ap->device[sdev->id];
> >@@ -353,6 +354,13 @@
> > */
> > blk_queue_max_sectors(sdev->request_queue, 2048);
> > }
> >+
> >+ if (dev->flags & ATA_DFLAG_NCQ) {
> >+ int ddepth = ata_id_queue_depth(dev->id) + 1;
> >+
> >+ depth = min(sdev->host->can_queue, ddepth);
> >+ scsi_adjust_queue_depth(sdev, MSG_ORDERED_TAG,
> >depth);
>
> For all hardware that uses SActive (all NCQ), the max is 31 not 32.
That's not true, the max is 32 counting 0 as a valid tag. So 31 is
indeed th max tag value, but 32 is the depth.
> 32 tags == 0xffffffff SActive, which is the same value as that which
> occurs when the hardware is dead/unplugged/etc.
Well you would think you would notice the hardware going dead by other
means than just SActive, right? Seems silly to cap the depth because of
that.
> Also... NCQ does not provide ordered tags. I think MSG_SIMPLE_TAG is
> more appropriate.
Yep indeed, thanks.
> >+ if (ncq)
> >+ qc->flags |= ATA_QCFLAG_NCQ;
> >+
> > if (scsicmd[0] == READ_10 || scsicmd[0] == WRITE_10) {
> >- if (lba48) {
> >+ if (ncq) {
> >+ tf->hob_feature = scsicmd[7];
> >+ tf->feature = scsicmd[8];
> >+ tf->nsect = qc->tag << 3;
> >+ tf->hob_lbal = scsicmd[2];
> >+ qc->nsect = ((unsigned int)scsicmd[7] << 8) |
> >+ scsicmd[8];
> >+ } else if (lba48) {
> > tf->hob_nsect = scsicmd[7];
> > tf->hob_lbal = scsicmd[2];
> >
> >@@ -569,7 +588,8 @@
> > qc->nsect = scsicmd[8];
> > }
> >
> >- tf->nsect = scsicmd[8];
> >+ if (!ncq)
> >+ tf->nsect = scsicmd[8];
>
> the other changes seem fine, but this seem strange
How so? FPDMA has the tag in nsect, others store the bottom 8 bits of
lba.
> >@@ -308,6 +317,11 @@
> > struct ata_queued_cmd qcmd[ATA_MAX_QUEUE];
> > unsigned long qactive;
> > unsigned int active_tag;
> >+ int ncq_depth;
> >+ int depth;
>
> I don't think we need two depths
The two depths were added because we need to differentiate between the
two for issuing new commands. ncq_depth > 0 is fine for issuing a new
FPDMA request, where as non-FPDMA commands need both !ncq_depth and
!depth.
Thanks for your comments!
--
Jens Axboe
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
