libata: cleanup ata_sff_interrupt()

Traditional IDE interface sucks in that it doesn't have a reliable IRQ pending bit, so if the controller raises IRQ while the driver is expecting it not to, the IRQ won't be cleared and eventually the IRQ line will be killed by interrupt subsystem. Some controllers have non-standard mechanism to indicate IRQ pending so that this condition can be detected and worked around.

This patch adds an optional operation ->sff_irq_check() which will be called for each port from the ata_sff_interrupt() if an unexpected interrupt is received. If the operation returns %true, ->sff_check_status() and ->sff_irq_clear() will be cleared for the port. Note that this doesn't mark the interrupt as handled so it won't prevent IRQ subsystem from killing the IRQ if this mechanism fails to clear the spurious IRQ.

This patch also implements ->sff_irq_check() for ata_piix. Note that this adds slight overhead to shared IRQ operation as IRQs which are destined for other controllers will trigger extra register accesses to check whether IDE interrupt is pending but this solves rare screaming IRQ cases and for some curious reason also helps weird BIOS related glitch on Samsung n130 as reported in bko#14314.

   http://bugzilla.kernel.org/show_bug.cgi?id=14314

* piix_base_ops dropped as suggested by Sergei.

* Spurious IRQ detection doesn't kick in anymore if polling qc is in progress. This provides less protection but some controllers have possible data corruption issues if the wrong register is accessed while a command is in progress.

Signed-off-by Tejun Heo<[email protected]>

Reported-by Johannes Stezenbach<[email protected]>

Reported-by Hans Werner<[email protected]>

Cc Alan Cox<[email protected]>

Cc Sergei Shtylyov<[email protected]>

---
Jeff, these two are for #upstream but generated on top of #upstream-fixes because the piix sata 32bit bmdma patch is currently only in #upstream-fixes. Please pull -fixes into #upstream before applying these two. Thanks.

  drivers/ata/ata_piix.c   |   20 +++++++++++++++-----
  drivers/ata/libata-sff.c |   35 ++++++++++++++++++++++++++++++++---
  include/linux/libata.h   |    1 +
  3 files changed, 48 insertions(+), 8 deletions(-)

Overall, as long as the drive is in Bus-Idle mode, it should be safe to go ahead and read Status, for pretty much every controller and drive.

I would make exception only for the new SATA FIS-based controllers, where we know that hitting Status is likely both pointless and wasteful, as well as being superfluous because the newer FIS-based controllers all have irq status registers.

Additionally, I think we should have a "fast-timeout" and "slow-timeout", whereby we check Status after a short period (5 seconds?) to make sure we did not lose an interrupt. If Status is !BSY, then we can proceed with handling qc success/failure immediately.

Hmmm... I was a bit worried about the case Alan mentioned several times where access to AltStatus while data transfer is going on can lead to silent data corruption.

Hello,

Quoted 2 lines · Jeff Garzik

Overall, as long as the drive is in Bus-Idle mode, it should be safe to go ahead and read Status, for pretty much every controller and drive.

Hmmm... I was a bit worried about the case Alan mentioned several times where access to AltStatus while data transfer is going on can lead to silent data corruption.

Quoted 4 lines

I would make exception only for the new SATA FIS-based controllers, where we know that hitting Status is likely both pointless and wasteful, as well as being superfluous because the newer FIS-based controllers all have irq status registers.

FIS-based ones need their own interrupt handlers anyway so, fortunately, things like irq_check callback isn't necessary to begin with. :-)

Quoted 4 lines

Additionally, I think we should have a "fast-timeout" and "slow-timeout", whereby we check Status after a short period (5 seconds?) to make sure we did not lose an interrupt. If Status is !BSY, then we can proceed with handling qc success/failure immediately.

Does this happen often? What I find more common is just plain timeouts, so I think it would improve our exception latency if we apply different timeouts for each trial. ie. For the first RW try, set the timeout to 7 secs. For the second, 15 and then to 30. This wouldn't harm the correctness while allowing libata to react much faster to transient failures.

Another thing is I can think of which can improve our robustness is dynamic irqpoll support such that when screaming IRQ happens, IRQ subsystem not only shuts down the IRQ line but also begins selectively irqpolling it.

Quoted 3 lines

Hmmm... I was a bit worried about the case Alan mentioned several times where access to AltStatus while data transfer is going on can lead to silent data corruption.

If a drive is in Bus-Idle, as I mentioned, then there is no active data transfer.

Quoted 6 lines

Does this happen often? What I find more common is just plain timeouts, so I think it would improve our exception latency if we apply different timeouts for each trial. ie. For the first RW try, set the timeout to 7 secs. For the second, 15 and then to 30. This wouldn't harm the correctness while allowing libata to react much faster to transient failures.

Lost interrupts do not happen often, but they do happen. Google finds plenty of examples.

Quoted 4 lines

Another thing is I can think of which can improve our robustness is dynamic irqpoll support such that when screaming IRQ happens, IRQ subsystem not only shuts down the IRQ line but also begins selectively irqpolling it.

Does this ever happen when data transfer is active? AFAIK this happens during probe or reset or set-xfer or bus-idle or some other auxiliary moment in time.

host->ports[i] is never NULL if i< host->n_ports and non-NULL return from ata_qc_from_tag() guarantees that the returned qc is active. Drop unnecessary tests.

Superflous () dropped as suggested by Sergei.

Signed-off-by Tejun Heo<[email protected]>

Cc Sergei Shtylyov<[email protected]>

---
  drivers/ata/libata-sff.c |   17 +++++++----------
  1 file changed, 7 insertions(+), 10 deletions(-)