pata_cmd64x: revert commit d62f5576

PATCH Applied Tejun Heo 6 participants 8 messages Aug 17, 2010 – Aug 19, 2010 libata

Share & tools

Tejun Heo

author

2010-08-17 12:17 UTC #0 lore raw

PATCH 1/1 pata_cmd64x: revert commit d62f5576

Commit d62f5576 (pata_cmd64x: fix handling of address setup timings) incorrectly called ata_timing_compute() on UDMA mode on 0 @UT leading to devide by zero fault. Revert it until better fix is available. This is reported in bko#16607 by Milan Kocian who also root caused it.

https://bugzilla.kernel.org/show_bug.cgi?id=16607

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

Reported-and-root-caused-by Milan Kocian <[email protected]>

Cc Bartlomiej Zolnierkiewicz <[email protected]>

Cc [email protected]

---
 drivers/ata/pata_cmd64x.c |    6 ------
 1 file changed, 6 deletions(-)

drivers/ata/pata_cmd64x.c +0 -6

index 9f5da1c..905ff76 100644
@@ -121,14 +121,8 @@ static void cmd64x_set_timing(struct ata_port *ap, struct ata_device *adev, u8 m
 
 		if (pair) {
 			struct ata_timing tp;
-
 			ata_timing_compute(pair, pair->pio_mode, &tp, T, 0);
 			ata_timing_merge(&t, &tp, &t, ATA_TIMING_SETUP);
-			if (pair->dma_mode) {
-				ata_timing_compute(pair, pair->dma_mode,
-						&tp, T, 0);
-				ata_timing_merge(&tp, &t, &t, ATA_TIMING_SETUP);
-			}
 		}
 	}

Bartlomiej Zolnierkiewicz

2010-08-17 12:57 UTC #1 lore raw

Quoted 6 lines · Tejun Heo

Commit d62f5576 (pata_cmd64x: fix handling of address setup timings) incorrectly called ata_timing_compute() on UDMA mode on 0 @UT leading to devide by zero fault. Revert it until better fix is available. This is reported in bko#16607 by Milan Kocian who also root caused it.
https://bugzilla.kernel.org/show_bug.cgi?id=16607

How's about giving the ATA maintainer some time to resolve the issue properly?

The revert should be a last resort action (+ the issue was reported only yesterday) as it introduces another regression (it just replaces new bug with the old bug while adding noise in the process)..

Quoted 26 lines

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

Reported-and-root-caused-by Milan Kocian <[email protected]>

Cc Bartlomiej Zolnierkiewicz <[email protected]>

Cc [email protected]

---
 drivers/ata/pata_cmd64x.c |    6 ------
 1 file changed, 6 deletions(-)

drivers/ata/pata_cmd64x.c +0 -6

index 9f5da1c..905ff76 100644
@@ -121,14 +121,8 @@ static void cmd64x_set_timing(struct ata_port *ap, struct ata_device *adev, u8 m
 
 		if (pair) {
 			struct ata_timing tp;
-
 			ata_timing_compute(pair, pair->pio_mode, &tp, T, 0);
 			ata_timing_merge(&t, &tp, &t, ATA_TIMING_SETUP);
-			if (pair->dma_mode) {
-				ata_timing_compute(pair, pair->dma_mode,
-						&tp, T, 0);
-				ata_timing_merge(&tp, &t, &t, ATA_TIMING_SETUP);
-			}
 		}
 	}

Tejun Heo

author

2010-08-17 15:04 UTC #2 lore raw

Hello,

Quoted 10 lines · Bartlomiej Zolnierkiewicz

Quoted 6 lines · Tejun Heo
Commit d62f5576 (pata_cmd64x: fix handling of address setup timings) incorrectly called ata_timing_compute() on UDMA mode on 0 @UT leading to devide by zero fault. Revert it until better fix is available. This is reported in bko#16607 by Milan Kocian who also root caused it.
https://bugzilla.kernel.org/show_bug.cgi?id=16607
How's about giving the ATA maintainer some time to resolve the issue properly?

Yeap, sure.

Quoted 3 lines

The revert should be a last resort action (+ the issue was reported only yesterday) as it introduces another regression (it just replaces new bug with the old bug while adding noise in the process)..

The old problem is much less severe tho. The introduced regression causes oops while the old bug probably doesn't show itself too often. Does it really need to merge the DMA timings too? If the device can't do certain timing, it's PIO configuration should reflect that so merging PIO part only should be enough for PIO configuration, no?

Thanks.

Signature · 1 line

tejun

Alan Cox

2010-08-17 15:25 UTC #3 lore raw

Quoted 2 lines

The old problem is much less severe tho. The introduced regression causes oops while the old bug probably doesn't show itself too often.

If ever

Quoted 3 lines

Does it really need to merge the DMA timings too? If the device can't do certain timing, it's PIO configuration should reflect that so merging PIO part only should be enough for PIO configuration, no?

The timing code knows about DMA/PIO constraints itself. The whole mucking around passing both timings is just bogus.

Pass pair->dma_mode only and it'll work out the PIO for you. Not that it matters - no real hardware has a DMA mode with a slower address setup than its PIO mode.

and for UT just stick in some value (eg 33000) so it works in PCI clocks

Bartlomiej Zolnierkiewicz

2010-08-17 16:39 UTC #4 lore raw

Quoted 9 lines · Alan Cox

Quoted 2 lines
The old problem is much less severe tho. The introduced regression causes oops while the old bug probably doesn't show itself too often.
If ever
Quoted 3 lines
Does it really need to merge the DMA timings too? If the device can't do certain timing, it's PIO configuration should reflect that so merging PIO part only should be enough for PIO configuration, no?

Quoted 6 lines

The timing code knows about DMA/PIO constraints itself. The whole mucking around passing both timings is just bogus.
Pass pair->dma_mode only and it'll work out the PIO for you. Not that it matters - no real hardware has a DMA mode with a slower address setup than its PIO mode.

There were some PIO3/MWDMA1 devices out in the wild (Iomega ZIP IIRC)..

Anyway the patch has never been submitted upstream:

*) I have absolutely no time to support it or related changes

*) it was not critical enough and/or important for other related work
(it is more important in atang/ide2libata context)

Quoted 1 line

and for UT just stick in some value (eg 33000) so it works in PCI clocks

Exactly, ->udma is never used by the driver itself..

Thanks.
--
Bartlomiej Zolnierkiewicz

Sergei Shtylyov

2010-08-17 17:42 UTC #5 lore raw

In reply to Alan Cox

Quoted 10 lines · Alan Cox

Quoted 3 lines
Does it really need to merge the DMA timings too? If the device can't do certain timing, it's PIO configuration should reflect that so merging PIO part only should be enough for PIO configuration, no?
The timing code knows about DMA/PIO constraints itself. The whole mucking around passing both timings is just bogus.
Pass pair->dma_mode only and it'll work out the PIO for you. Not that it matters - no real hardware has a DMA mode with a slower address setup than its PIO mode.

The address setup timing means *exactly nothing* for DMA modes. I keep
wondering why it has been added to the libata timing table...

MBR, Sergei

Jeff Garzik

Applied

2010-08-17 21:30 UTC #6 lore raw

In reply to Tejun Heo

Quoted 20 lines · Tejun Heo

Commit d62f5576 (pata_cmd64x: fix handling of address setup timings) incorrectly called ata_timing_compute() on UDMA mode on 0 @UT leading to devide by zero fault. Revert it until better fix is available. This is reported in bko#16607 by Milan Kocian who also root caused it.
   https://bugzilla.kernel.org/show_bug.cgi?id=16607
Signed-off-by Tejun Heo<[email protected]>
Reported-and-root-caused-by Milan Kocian<[email protected]>
Cc Bartlomiej Zolnierkiewicz<[email protected]>
Cc [email protected]
---
  drivers/ata/pata_cmd64x.c |    6 ------
  1 file changed, 6 deletions(-)
drivers/ata/pata_cmd64x.c +0 -0
index 9f5da1c..905ff76 100644

drivers/ata/pata_cmd64x.c static void cmd64x_set_timing(struct ata_port *ap, struct ata_device *adev, u8 m · line 130

Quoted 10 lines

drivers/ata/pata_cmd64x.c +0 -5

@@ -121,14 +121,8 @@ static void cmd64x_set_timing(struct ata_port *ap, struct ata_device *adev, u8 m
 
  		if (pair) {
  			struct ata_timing tp;
-
  			ata_timing_compute(pair, pair->pio_mode,&tp, T, 0);
  			ata_timing_merge(&t,&tp,&t, ATA_TIMING_SETUP);
-			if (pair->dma_mode) {
-				ata_timing_compute(pair, pair->dma_mode,
-						&tp, T, 0);
-				ata_timing_merge(&tp,&t,&t, ATA_TIMING_SETUP);

applied; even if not perfect, this gets us into a better state.

Milan Kocian

2010-08-19 10:51 UTC #7 lore raw

Quoted 22 lines · Jeff Garzik

Quoted 21 lines · Tejun Heo
Commit d62f5576 (pata_cmd64x: fix handling of address setup timings) incorrectly called ata_timing_compute() on UDMA mode on 0 @UT leading to devide by zero fault. Revert it until better fix is available. This is reported in bko#16607 by Milan Kocian who also root caused it.
https://bugzilla.kernel.org/show_bug.cgi?id=16607
Signed-off-by Tejun Heo<[email protected]>
Reported-and-root-caused-by Milan Kocian<[email protected]>
Cc Bartlomiej Zolnierkiewicz<[email protected]>
Cc [email protected]
---
 drivers/ata/pata_cmd64x.c |    6 ------
 1 file changed, 6 deletions(-)
drivers/ata/pata_cmd64x.c +0 -0
index 9f5da1c..905ff76 100644
@@ -121,14 +121,8 @@ static void cmd64x_set_timing(struct ata_port *ap, struct ata_device *adev, u8 m
 

Quoted 13 lines

drivers/ata/pata_cmd64x.c static void cmd64x_set_timing(struct ata_port *ap, struct ata_device *adev, u8 m
Quoted 9 lines
drivers/ata/pata_cmd64x.c +0 -5
 		if (pair) {
 			struct ata_timing tp;
-
 			ata_timing_compute(pair, pair->pio_mode,&tp, T, 0);
 			ata_timing_merge(&t,&tp,&t, ATA_TIMING_SETUP);
-			if (pair->dma_mode) {
-				ata_timing_compute(pair, pair->dma_mode,
-						&tp, T, 0);
-				ata_timing_merge(&tp,&t,&t, ATA_TIMING_SETUP);
applied; even if not perfect, this gets us into a better state.

Thank all for solving this. Should I close the bug now ?

regards

Signature · 1 line

Milan Kocian