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.
> {
> + 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.
> /* 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.
--
tejun
-
: 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
