Re: Playing with SATA NCQ

[Jeff Garzik]

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


-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/