Tejun Heo wrote:
> On Thu, Mar 09, 2006 at 04:51:36PM +0800, Albert Lee wrote:
> [--- snip ---]
>
>>+
>>+static int ata_hsm_move(struct ata_port *ap, struct ata_queued_cmd *qc,
>>+ u8 status, int in_wq)
>
>
> IMHO, polling would be a slightly better name than in_wq.
>
>
The "in_wq" name helps to differ it from (qc->tf.flags & ATA_TFLAG_POLLING).
>> {
>>+ unsigned long flags = 0;
>>+ int poll_next;
>>+
>> WARN_ON(qc == NULL);
>> WARN_ON((qc->flags & ATA_QCFLAG_ACTIVE) == 0);
>>
>>+ WARN_ON(in_wq != ((qc->tf.flags & ATA_TFLAG_POLLING) ||
>>+ (ap->hsm_task_state == HSM_ST_FIRST &&
>>+ ((qc->tf.protocol == ATA_PROT_PIO &&
>>+ (qc->tf.flags & ATA_TFLAG_WRITE)) ||
>>+ (is_atapi_taskfile(&qc->tf) &&
>>+ !(qc->dev->flags & ATA_DFLAG_CDB_INTR))))));
>>+
>
>
> I think this is a little bit excessive. Can't we get away with
> WARN_ON(in_wq != !in_irq()) or just trust what the caller says? It's
> not like this function has a lot of callers.
>
>
The check prevents me from doing something stupid to ata_qc_issue_prot(), say,
insert a qc with DMA protocol into the workqueue, etc.
Also it helps to show how in_wq compares with (qc->tf.flags & ATA_TFLAG_POLLING).
(I was once confused when wrote the code.) Will add a comment to make it clear next time.
>> /* check error */
>> if (unlikely(status & (ATA_ERR | ATA_DF))) {
>> qc->err_mask |= AC_ERR_DEV;
>>@@ -3643,6 +3664,13 @@ static void ata_hsm_move(struct ata_port
>> fsm_start:
>> switch (ap->hsm_task_state) {
>> case HSM_ST_FIRST:
>>+ /* Send first data block or PACKET CDB */
>>+
>>+ /* if polling, we will stay in the work queue after sending the data.
>>+ * otherwise, interrupt handler takes over after sending the data.
>>+ */
>>+ poll_next = (qc->tf.flags & ATA_TFLAG_POLLING);
>>+
>> /* check device status */
>> if (unlikely((status & (ATA_BUSY | ATA_DRQ)) != ATA_DRQ)) {
>> /* Wrong status. Let EH handle this */
>>@@ -3651,8 +3679,35 @@ fsm_start:
>> goto fsm_start;
>> }
>>
>>- atapi_send_cdb(ap, qc);
>>+ /* Send the CDB (atapi) or the first data block (ata pio out).
>>+ * During the state transition, interrupt handler shouldn't
>>+ * be invoked before the data transfer is complete and
>>+ * hsm_task_state is changed. Hence, the following locking.
>>+ */
>>+ if (in_wq)
>>+ spin_lock_irqsave(&ap->host_set->lock, flags);
>>+
>>+ if (qc->tf.protocol == ATA_PROT_PIO) {
>>+ /* PIO data out protocol.
>>+ * send first data block.
>>+ */
>>+
>>+ /* ata_pio_sectors() might change the state to HSM_ST_LAST.
>>+ * so, the state is changed here before ata_pio_sectors().
>>+ */
>>+ ap->hsm_task_state = HSM_ST;
>>+ ata_pio_sectors(qc);
>>+ ata_altstatus(ap); /* flush */
>>+ } else
>>+ /* send CDB */
>>+ atapi_send_cdb(ap, qc);
>>
>>+ if (in_wq)
>>+ spin_unlock_irqrestore(&ap->host_set->lock, flags);
>>+
>>+ /* if polling, ata_pio_task() handles the rest.
>>+ * otherwise, interrupt handler takes over from here.
>>+ */
>> break;
>>
>
>
> For ATA PIO write transfers, the first transfer and n'th transfers
> aren't really different. The code would be simpler if it handles the
> first ATA PIO write in HSM_ST. And if we do that, HSM_ST_FIRST can be
> renamed to HSM_ST_CDB. Hmmm.. Maybe this should be done in separate
> series of patches.
>
For ATA PIO write transfer, the first transfer is different:
It is always done by polling, even if irq is turned on.
If we treat the first PIO write transfer as HSM_ST, we need to add some
additional logic to HSM_ST and check whether it is first transfer or not. If it is,
and irq is on, the transition from polling to interrupt-driven must be protected
by spinlock, similar to what's done in HSM_ST_FIRST.
HSM_ST is more frequent than HSM_ST_FIRST, so treat the first PIO
write transfer as HSM_ST generates more if() execution than
treat the first PIO write transfer as HSM_ST_FIRST and do the if() there.
--
Thanks for the review,
Albert
-
: 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
