Re: [PATCH RFC] libata: interrupt driven pio

[Jeff Garzik <jgarzik@pobox.com>]

Albert Lee wrote:
> Dear all,
> 
> The interrupt driven PIO draft patch is ready for your review.
> 
> Changes:
> - add PIO_ST_FIRST for the state before sending ATAPI CDB or sending 
> "ATA PIO data out" first data block.
> - remove the ATA_FLAG_NOINTR flag since the interrupt handler is now 
> aware of the states
> - modify ata_pio_sector() and atapi_pio_bytes() to work in the interrupt 
> context
> - modify the ata_host_intr() to handle PIO interrupts
> - modify ata_qc_issue_prot() to initialize states
> - atapi_packet_task() changed to handle "ATA PIO data out" first data block
> - support the pre-ATA4 ATAPI device which raise interrupt when ready to 
> receive CDB
> 
> Patch against 2.6.13 (80ac2912f846c01d702774bb6aa7100ec71e88b9).

Very nice, thanks for working on this.

Comments:

1) With this work, PIO_ST_xxx is no longer an appropriate name.  I 
suggest s/PIO_ST_/HSM_ST_/


2) ditto with s/pio_task_state/hsm_task_state/


3) Don't bother with in_atomic(), since we mix contexts in libata. 
Causes more trouble than its worth.  Just use *map_atomic()


4) prefer 'ata_id_cdb_intr' to 'atapi_...'


5) please don't put braces around single-line C statements:

+	if (status & ATA_BUSY) {
+		goto idle_irq;
+	}


6) the following locking is questionable.  AFAICS this should be 
resolved elsewhere, and if you have to take the lock here, you already 
have problems

+		/* PIO data out protocol.
+		 * send first data block.
+		 */
+		ata_pio_sector(qc);

-		/* PIO commands are handled by polling */
+		spin_lock_irqsave(&ap->host_set->lock, flags);
  		ap->pio_task_state = PIO_ST;
-		queue_work(ata_wq, &ap->pio_task);
+		spin_unlock_irqrestore(&ap->host_set->lock, flags);
+
+		/* interrupt handler takes over from here */

ie. why queue the task at all, if the intr handler takes over?


7) Overall, looks pretty good at first glance.  This patch will require 
much testing before global deployment.  Once you have a final patch, 
we'll let it simmer in libata-dev.git for a while, allowing -mm users 
plenty of time for testing.  Possibly a 2.6.16 version target.