irq-pio branch updated with Tejun's patches

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.

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 <[email protected]>

Quoted 4 lines · Jeff Garzik

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.

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?

Quoted 4 lines

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?

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.

Hello, Albert.

Quoted 1 line · Albert Lee

Quoted 1 line

Quoted 5 lines

Quoted 3 lines

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.

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.

Quoted 9 lines · Tejun Heo

Hello, Albert.

Quoted 5 lines · Albert Lee

Quoted 4 lines

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?

Quoted 5 lines

Quoted 1 line

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?

Quoted 2 lines

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 :)

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

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.