On Thu, May 26 2005, Jeff Garzik wrote:
> 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.
I'll get it cleaned up and tested.
> >>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).
Reserving one for error handling makes a lot of sense, that's a good
point. I still don't buy your argument for not using the full 32 slots
in total, though. But in the end I don't suppose it matters a lot, 31 or
30 for queue depth is very much at the end of the spectrum of
diminishing return in difference.
> >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...
Of course it is possible, but I would rather trade 4-bytes of space to
get clearer code than switching a flag on/off all the time and counting
a single depth. But hey that's trivial detail, compared to what else is
missing. When this becomes the most important point, we can take it up
again :-)
--
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]
