Re: Playing with SATA NCQ

[Jeff Garzik <jgarzik@pobox.com>]

Jens Axboe wrote:
> On Thu, May 26 2005, Jeff Garzik wrote:
>>>+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?

(this covers both quoted questions above)

The PIO function assumes that PCI IDE-like ATA register blocks (command 
registers, control registers) are available.  The read-log-page function 
can make no such assumptions.

dev-select and check-status should both be done by the machinery that 
occurs once you start things in motion by calling ata_qc_issue().

Doing things this way is necessary for FIS-based hardware like AHCI or 
SiI 3124.


>>>#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.

I was talking about depth.  In libata, it's a policy decision to never 
use more than 31 tags at any given time.

You can change it from 31 to 32 in SuSE for value-add, if you wish :)

Note also that error handling occasionally needs a command slot, so the 
limit may even be 30 (or 31 at most).



> 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.

You can definitely handle both FPDMA and non-FPDMA with a single 
variable.  Think harder on this one.  You have flags to work with, you 
know...

	Jeff