irq-pio branch updated with Tejun's patches

irq-pio branch updated with Tejun's patches

[Jeff Garzik]

Albert,

I updated the libata irq-pio branch with Tejun's patches.  I would 
appreciate it if you could check my work, and make sure that everything 
continues to function properly.

	Jeff

Re: irq-pio branch updated with Tejun's patches

[Albert Lee]

Jeff Garzik wrote:
> 
> I updated the libata irq-pio branch with Tejun's patches.  I would
> appreciate it if you could check my work, and make sure that everything
> continues to function properly.
> 

Sorry for the late reply.
Still checking Tejun's patches, especially the EH vs polling PIO racing fix.

Fixed minor compiliation problem in the irq-pio branch with Tejun and your patches.
Will submit those minor patches first.

Albert

[PATCH 0/4] libata-dev: minor fix for irq-pio with Tejun's EH patches

[Albert Lee]

Dear all,

Minor fix for the irq-pio branch with Tejun's EH patches.

1/4: Fix array index value in ata_rwcmd_protocol()
2/4: Use new ata_queue_pio_task() for PIO polling task.
3/4: Use new AC_ERR_* flags
4/4: Minor comment fix

Patch against the irq-pio branch of libata-dev tree
(f6ef65e6d004b77d516037424c7ccc209d0d3509).

For your review, thanks.

Albert

[PATCH 1/4] libata-dev: Fix array index value in ata_rwcmd_protocol()

[Albert Lee]

Patch 1/4:

  - Fix the array index value in ata_rwcmd_protocol() for the added FUA commands.

For your review, thanks.

Albert
Signed-off-by: Albert Lee <albertcc@tw.ibm.com> 

===

--- irq-pio/drivers/scsi/libata-core.c	2006-02-07 16:59:14.000000000 +0800
+++ rwcmd/drivers/scsi/libata-core.c	2006-02-08 13:39:34.000000000 +0800
@@ -611,7 +611,7 @@ int ata_rwcmd_protocol(struct ata_queued
 	} else if (lba48 && (qc->ap->flags & ATA_FLAG_PIO_LBA48)) {
 		/* Unable to use DMA due to host limitation */
 		tf->protocol = ATA_PROT_PIO;
-		index = dev->multi_count ? 0 : 4;
+		index = dev->multi_count ? 0 : 8;
 	} else {
 		tf->protocol = ATA_PROT_DMA;
 		index = 16;

Re: irq-pio branch updated with Tejun's patches

[Tejun Heo]

Albert Lee wrote:
> Jeff Garzik wrote:
> 
>>I updated the libata irq-pio branch with Tejun's patches.  I would
>>appreciate it if you could check my work, and make sure that everything
>>continues to function properly.
>>
> 
> 
> Sorry for the late reply.
> Still checking Tejun's patches, especially the EH vs polling PIO racing fix.
> 
> Fixed minor compiliation problem in the irq-pio branch with Tejun and your patches.
> Will submit those minor patches first.
> 

Hello, Albert.

This is sort of OT, but would it be possible to separate PIO 
issuing/interrupt handling from ata_qc_issue_prot and ata_host_intr?

As PIO implementation currently stands, a lldd must use those standard 
methods to use PIO, but some controllers need PIO handling but the stock 
issue and interrupt routines don't really fit. e.g. not-yet-working 
sata_inic162x.  Something like ata_qc_issue_pio and ata_qc_pio_intr maybe?

-- 
tejun

[PATCH 2/4] libata-dev: Use new ata_queue_pio_task() for PIO polling task

[Albert Lee]

Patch 2/4:

Changes:
  - Use new ata_queue_pio_task() for PIO polling task.
  - Remove the unused ata_queue_packet_task() function.
    (irq-pio had merged the 2 queues into one.)

For your review, thanks.

Albert
Signed-off-by: Albert Lee <albertcc@tw.ibm.com> 

===

--- rwcmd/drivers/scsi/libata-core.c	2006-02-08 13:39:34.000000000 +0800
+++ queuework/drivers/scsi/libata-core.c	2006-02-08 14:06:41.000000000 +0800
@@ -1073,12 +1073,6 @@ static unsigned int ata_pio_modes(const 
 }
 
 static inline void
-ata_queue_packet_task(struct ata_port *ap)
-{
-	queue_work(ata_wq, &ap->packet_task);
-}
-
-static inline void
 ata_queue_pio_task(struct ata_port *ap)
 {
 	queue_work(ata_wq, &ap->pio_task);
@@ -3971,7 +3965,7 @@ unsigned int ata_qc_issue_prot(struct at
 		ap->hsm_task_state = HSM_ST_LAST;
 
 		if (qc->tf.flags & ATA_TFLAG_POLLING)
-			queue_work(ata_wq, &ap->pio_task);
+			ata_queue_pio_task(ap);
 
 		break;
 
@@ -4024,7 +4018,7 @@ unsigned int ata_qc_issue_prot(struct at
 		/* send cdb by polling if no cdb interrupt */
 		if ((!(qc->dev->flags & ATA_DFLAG_CDB_INTR)) ||
 		    (qc->tf.flags & ATA_TFLAG_POLLING))
-			ata_queue_packet_task(ap);
+			ata_queue_pio_task(ap);
 		break;
 
 	case ATA_PROT_ATAPI_DMA:
@@ -4036,7 +4030,7 @@ unsigned int ata_qc_issue_prot(struct at
 
 		/* send cdb by polling if no cdb interrupt */
 		if (!(qc->dev->flags & ATA_DFLAG_CDB_INTR))
-			ata_queue_packet_task(ap);
+			ata_queue_pio_task(ap);
 		break;
 
 	default:

[PATCH 3/4] libata-dev: Use new AC_ERR_* flags

[Albert Lee]

Patch 3/4:

Changes:
  - Use new AC_ERR_* flags as done in Tejun's patches
  - In ata_qc_timeout(), replace ac_err_mask(drv_stat) with AC_ERR_TIMEOUT.
    This makes time out handler always report error to upper layer.
    Otherwise if the drv_stat looks good, libata might falsely report OK to the upper layer.

For your review, thanks.

Albert
Signed-off-by: Albert Lee <albertcc@tw.ibm.com> 

===

--- queuework/drivers/scsi/libata-core.c	2006-02-08 14:06:41.000000000 +0800
+++ errmask/drivers/scsi/libata-core.c	2006-02-08 14:42:34.000000000 +0800
@@ -3315,7 +3315,7 @@ static int ata_pio_first_block(struct at
 	/* sleep-wait for BSY to clear */
 	DPRINTK("busy wait\n");
 	if (ata_busy_sleep(ap, ATA_TMOUT_DATAOUT_QUICK, ATA_TMOUT_DATAOUT)) {
-		qc->err_mask |= AC_ERR_ATA_BUS;
+		qc->err_mask |= AC_ERR_TIMEOUT;
 		ap->hsm_task_state = HSM_ST_TMOUT;
 		goto err_out;
 	}
@@ -3324,7 +3324,7 @@ static int ata_pio_first_block(struct at
 	status = ata_chk_status(ap);
 	if ((status & (ATA_BUSY | ATA_DRQ)) != ATA_DRQ) {
 		/* device status error */
-		qc->err_mask |= AC_ERR_ATA_BUS;
+		qc->err_mask |= AC_ERR_HSM;
 		ap->hsm_task_state = HSM_ST_ERR;
 		goto err_out;
 	}
@@ -3684,7 +3684,7 @@ static void ata_qc_timeout(struct ata_qu
 		ap->hsm_task_state = HSM_ST_IDLE;
 
 		/* complete taskfile transaction */
-		qc->err_mask |= ac_err_mask(drv_stat);
+		qc->err_mask |= AC_ERR_TIMEOUT;
 		break;
 	}
 
@@ -4365,7 +4365,7 @@ fsm_start:
 		/* check device status */
 		if (unlikely((status & (ATA_BUSY | ATA_DRQ)) != ATA_DRQ)) {
 			/* Wrong status. Let EH handle this */
-			qc->err_mask |= AC_ERR_ATA_BUS;
+			qc->err_mask |= AC_ERR_HSM;
 			ap->hsm_task_state = HSM_ST_ERR;
 			goto fsm_start;
 		}
@@ -4394,7 +4394,7 @@ fsm_start:
 			/* ATA PIO protocol */
 			if (unlikely((status & ATA_DRQ) == 0)) {
 				/* handle BSY=0, DRQ=0 as error */
-				qc->err_mask |= AC_ERR_ATA_BUS;
+				qc->err_mask |= AC_ERR_HSM;
 				ap->hsm_task_state = HSM_ST_ERR;
 				goto fsm_start;
 			}
@@ -4416,7 +4416,7 @@ fsm_start:
 	case HSM_ST_LAST:
 		if (unlikely(status & ATA_DRQ)) {
 			/* handle DRQ=1 as error */
-			qc->err_mask |= AC_ERR_ATA_BUS;
+			qc->err_mask |= AC_ERR_HSM;
 			ap->hsm_task_state = HSM_ST_ERR;
 			goto fsm_start;
 		}

[PATCH 4/4] libata-dev: Minor comment fix

[Albert Lee]

Patch 4/4:

  - Minor comment movement

For your review, thanks.

Albert
Signed-off-by: Albert Lee <albertcc@tw.ibm.com> 

===

--- errmask/drivers/scsi/libata-core.c	2006-02-08 14:42:34.000000000 +0800
+++ comments/drivers/scsi/libata-core.c	2006-02-08 14:44:31.000000000 +0800
@@ -4510,19 +4510,6 @@ irqreturn_t ata_interrupt (int irq, void
 	return IRQ_RETVAL(handled);
 }
 
-/**
- *	ata_port_start - Set port up for dma.
- *	@ap: Port to initialize
- *
- *	Called just after data structures for each port are
- *	initialized.  Allocates space for PRD table.
- *
- *	May be used as the port_start() entry in ata_port_operations.
- *
- *	LOCKING:
- *	Inherited from caller.
- */
-
 /*
  * Execute a 'simple' command, that only consists of the opcode 'cmd' itself,
  * without filling any other registers
@@ -4613,6 +4600,19 @@ int ata_device_suspend(struct ata_port *
 	return 0;
 }
 
+/**
+ *	ata_port_start - Set port up for dma.
+ *	@ap: Port to initialize
+ *
+ *	Called just after data structures for each port are
+ *	initialized.  Allocates space for PRD table.
+ *
+ *	May be used as the port_start() entry in ata_port_operations.
+ *
+ *	LOCKING:
+ *	Inherited from caller.
+ */
+
 int ata_port_start (struct ata_port *ap)
 {
 	struct device *dev = ap->host_set->dev;

Re: irq-pio branch updated with Tejun's patches

[Albert Lee]

Hi Tejun,

> 
> 
> This is sort of OT, but would it be possible to separate PIO
> issuing/interrupt handling from ata_qc_issue_prot and ata_host_intr?
> 

Isn't overriding the ->qc_issue() and ->irq_handle() in LLDD good enough?

> As PIO implementation currently stands, a lldd must use those standard
> methods to use PIO, but some controllers need PIO handling but the stock
> issue and interrupt routines don't really fit. e.g. not-yet-working
> sata_inic162x.  Something like ata_qc_issue_pio and ata_qc_pio_intr maybe?
> 

How about refactoring the PCI-IDE specific logic from libata-core to a 
seperate source file, say, ata_pciide.c?

Currently we have many PCI-IDE specific driving logic in libata-core.c.
Maybe we can seperate the different driving logic required by different
hardware interface into different source files?
This could make libata-core.c to be more abstract and generic from the underlying
hardware: libata-core only knows about the ata_port_operations interface.
(ata_port_operations should be generic enough to cover all the hardware types.)
LLDDs can either select default implementation for its hardware interface type
(such as ata_bmdma_setup() from ata_pciide.c) or override harddware specfic functions.

Ex. hardware interface types:
  - PCI IDE (bmdma + PRD tables)
    (covers legacy taskfile registers interface) => ata_pciide.c
  - ADMA     => pdc_adma.c
  - AHCI     => ahci.c
  - Initio   => sata_initio.c (ata_qc_issue_pio and ata_qc_pio_intr implementation here)
  - SAS/SATA => sata_sas.c (ata_sas_port_start/stop implementation here)
  - etc.     


Albert

Re: irq-pio branch updated with Tejun's patches

[Tejun Heo]

Hello, Albert.

Albert Lee wrote:
>>
>>This is sort of OT, but would it be possible to separate PIO
>>issuing/interrupt handling from ata_qc_issue_prot and ata_host_intr?
>>
> 
> Isn't overriding the ->qc_issue() and ->irq_handle() in LLDD good enough?

The driver needs its own ->qc_issue() and ->irq_handler() but it also 
needs PIO support.  AFAICS, the current PIO implementation is rather 
strongly tied into ata_qc_issue_prot() and ata_host_intr() making it 
difficult to use PIO support in drivers which use private ->qc_issue() 
and ->irq_handler().  So, what I was asking was to separate out PIO 
handling from ata_qc_issue_prot() and ata_host_intr() such that other 
issue and intr routines can use PIO.
> 
> How about refactoring the PCI-IDE specific logic from libata-core to a 
> seperate source file, say, ata_pciide.c?
> 
> Currently we have many PCI-IDE specific driving logic in libata-core.c.
> Maybe we can seperate the different driving logic required by different
> hardware interface into different source files?
> This could make libata-core.c to be more abstract and generic from the underlying
> hardware: libata-core only knows about the ata_port_operations interface.
> (ata_port_operations should be generic enough to cover all the hardware types.)
> LLDDs can either select default implementation for its hardware interface type
> (such as ata_bmdma_setup() from ata_pciide.c) or override harddware specfic functions.

I agree.  If for nothing else, the size of libata-core.c is getting too 
large.  Also, we currently have multiple levels of methods in 
ata_port_operations, some operations are only used by other operations, 
which is a bit confusing I think.

Separating out traditional driving logic from libata-core.c would 
require another level of indirection such that traditional low level 
drivers still can mix & match different legacy operations.  I thought 
about it and wasn't really sure whether the added abstraction was worth 
the clean up.  We currently don't have too many controllers which are 
legacy-free, it seems.

> Ex. hardware interface types:
>   - PCI IDE (bmdma + PRD tables)
>     (covers legacy taskfile registers interface) => ata_pciide.c
>   - ADMA     => pdc_adma.c
>   - AHCI     => ahci.c
>   - Initio   => sata_initio.c (ata_qc_issue_pio and ata_qc_pio_intr implementation here)

ata_qc_issue_pio was bad naming probably.  As I wrote before, I was 
asking for a PIO helper which inic_qc_issue() can call to drive PIO 
logic and the same for ata_qc_pio_intr().

>   - SAS/SATA => sata_sas.c (ata_sas_port_start/stop implementation here)
>   - etc.     

Thanks.

-- 
tejun

Re: irq-pio branch updated with Tejun's patches

[Jeff Garzik]

Albert Lee wrote:
> How about refactoring the PCI-IDE specific logic from libata-core to a 
> seperate source file, say, ata_pciide.c?

This is OK with me.


> Currently we have many PCI-IDE specific driving logic in libata-core.c.
> Maybe we can seperate the different driving logic required by different
> hardware interface into different source files?
> This could make libata-core.c to be more abstract and generic from the underlying

> Ex. hardware interface types:
>   - PCI IDE (bmdma + PRD tables)
>     (covers legacy taskfile registers interface) => ata_pciide.c
>   - ADMA     => pdc_adma.c
>   - AHCI     => ahci.c
>   - Initio   => sata_initio.c (ata_qc_issue_pio and ata_qc_pio_intr implementation here)
>   - SAS/SATA => sata_sas.c (ata_sas_port_start/stop implementation here)
>   - etc.     

We don't need abstraction, we just need to keep individual libata*.c 
file size / LOC under control.  A libata-bmdma.c would certainly be fine 
with me.

	Jeff

Re: irq-pio branch updated with Tejun's patches

[Jeff Garzik]

Tejun Heo wrote:
> Hello, Albert.
> 
> Albert Lee wrote:
> 
>>>
>>> This is sort of OT, but would it be possible to separate PIO
>>> issuing/interrupt handling from ata_qc_issue_prot and ata_host_intr?
>>>
>>
>> Isn't overriding the ->qc_issue() and ->irq_handle() in LLDD good enough?
> 
> 
> The driver needs its own ->qc_issue() and ->irq_handler() but it also 
> needs PIO support.  AFAICS, the current PIO implementation is rather 
> strongly tied into ata_qc_issue_prot() and ata_host_intr() making it 
> difficult to use PIO support in drivers which use private ->qc_issue() 
> and ->irq_handler().  So, what I was asking was to separate out PIO 
> handling from ata_qc_issue_prot() and ata_host_intr() such that other 
> issue and intr routines can use PIO.

What needs this separation?  initio?


> Separating out traditional driving logic from libata-core.c would 
> require another level of indirection such that traditional low level 
> drivers still can mix & match different legacy operations.  I thought 
> about it and wasn't really sure whether the added abstraction was worth 
> the clean up.  We currently don't have too many controllers which are 
> legacy-free, it seems.

IMO the solution is time.  Time and evolution will determine what's 
best.  When a particular section of code get particularly large (PCI IDE 
driver helper code), it certainly makes sense to split it out.  "Do what 
you must, and no more" is the Linux maxim :)

	Jeff

Re: [PATCH 1/4] libata-dev: Fix array index value in ata_rwcmd_protocol()

[Jeff Garzik]

Albert Lee wrote:
> Patch 1/4:
> 
>   - Fix the array index value in ata_rwcmd_protocol() for the added FUA commands.
> 
> For your review, thanks.
> 
> Albert
> Signed-off-by: Albert Lee <albertcc@tw.ibm.com> 

applied 1-4

Re: irq-pio branch updated with Tejun's patches

[Tejun]

Jeff Garzik wrote:
> Tejun Heo wrote:
> 
>> Hello, Albert.
>>
>> Albert Lee wrote:
>>
>>>>
>>>> This is sort of OT, but would it be possible to separate PIO
>>>> issuing/interrupt handling from ata_qc_issue_prot and ata_host_intr?
>>>>
>>>
>>> Isn't overriding the ->qc_issue() and ->irq_handle() in LLDD good 
>>> enough?
>>
>>
>>
>> The driver needs its own ->qc_issue() and ->irq_handler() but it also 
>> needs PIO support.  AFAICS, the current PIO implementation is rather 
>> strongly tied into ata_qc_issue_prot() and ata_host_intr() making it 
>> difficult to use PIO support in drivers which use private ->qc_issue() 
>> and ->irq_handler().  So, what I was asking was to separate out PIO 
>> handling from ata_qc_issue_prot() and ata_host_intr() such that other 
>> issue and intr routines can use PIO.
> 
> 
> What needs this separation?  initio?
> 

Yeah, it seems.  Its broken driver currently emulates BMDMA status to 
work with standard issue/intr functions, but it seems fragile.  In 
addition, for NCQ commands, the controller operates in different mode 
and the code will have to be distorted more than now.  So, I think it's 
better to use separate issue/intr handler and calling PIO helpers when 
needed.  Well, if that driver ever works, that is.  I'll contact Initio 
support again.  Hmmm.. I might even call them.  Heh heh. :-)

> 
>> Separating out traditional driving logic from libata-core.c would 
>> require another level of indirection such that traditional low level 
>> drivers still can mix & match different legacy operations.  I thought 
>> about it and wasn't really sure whether the added abstraction was 
>> worth the clean up.  We currently don't have too many controllers 
>> which are legacy-free, it seems.
> 
> IMO the solution is time.  Time and evolution will determine what's 
> best.  When a particular section of code get particularly large (PCI IDE 
> driver helper code), it certainly makes sense to split it out.  "Do what 
> you must, and no more" is the Linux maxim :)

Being a stupid over-designer, I'm sometimes a bit unconformtable with 
the approach but I guess I'll finally adapt.  I'll talk about this more 
in the thread regarding probe_reset component operatons.

Thanks.

-- 
tejun