| Message ID | 4C6A7CF6.5090706@kernel.org |
|---|---|
| State | Not Applicable |
| Delegated to: | David Miller |
| Headers | show |
On Tuesday 17 August 2010 02:13:42 pm Tejun Heo wrote: > 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).. > Signed-off-by: Tejun Heo <tj@kernel.org> > Reported-and-root-caused-by: Milan Kocian <milan.kocian@wq.cz> > Cc: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> > Cc: stable@kernel.org > --- > drivers/ata/pata_cmd64x.c | 6 ------ > 1 file changed, 6 deletions(-) > > diff --git a/drivers/ata/pata_cmd64x.c b/drivers/ata/pata_cmd64x.c > index 9f5da1c..905ff76 100644 > --- a/drivers/ata/pata_cmd64x.c > +++ b/drivers/ata/pata_cmd64x.c > @@ -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); > - } > } > } -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello, On 08/17/2010 02:56 PM, Bartlomiej Zolnierkiewicz wrote: > On Tuesday 17 August 2010 02:13:42 pm Tejun Heo wrote: >> 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. > 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.
> 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 > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday 17 August 2010 05:39:17 pm Alan Cox wrote: > > 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 > > > 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. 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) > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Alan Cox wrote: >> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 08/17/2010 08:13 AM, Tejun Heo wrote: > 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<tj@kernel.org> > Reported-and-root-caused-by: Milan Kocian<milan.kocian@wq.cz> > Cc: Bartlomiej Zolnierkiewicz<bzolnier@gmail.com> > Cc: stable@kernel.org > --- > drivers/ata/pata_cmd64x.c | 6 ------ > 1 file changed, 6 deletions(-) > > diff --git a/drivers/ata/pata_cmd64x.c b/drivers/ata/pata_cmd64x.c > index 9f5da1c..905ff76 100644 > --- a/drivers/ata/pata_cmd64x.c > +++ b/drivers/ata/pata_cmd64x.c > @@ -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. -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Aug 17, 2010 at 05:30:07PM -0400, Jeff Garzik wrote: > On 08/17/2010 08:13 AM, Tejun Heo wrote: > >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<tj@kernel.org> > >Reported-and-root-caused-by: Milan Kocian<milan.kocian@wq.cz> > >Cc: Bartlomiej Zolnierkiewicz<bzolnier@gmail.com> > >Cc: stable@kernel.org > >--- > > drivers/ata/pata_cmd64x.c | 6 ------ > > 1 file changed, 6 deletions(-) > > > >diff --git a/drivers/ata/pata_cmd64x.c b/drivers/ata/pata_cmd64x.c > >index 9f5da1c..905ff76 100644 > >--- a/drivers/ata/pata_cmd64x.c > >+++ b/drivers/ata/pata_cmd64x.c > >@@ -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. > > Thank all for solving this. Should I close the bug now ? regards
diff --git a/drivers/ata/pata_cmd64x.c b/drivers/ata/pata_cmd64x.c index 9f5da1c..905ff76 100644 --- a/drivers/ata/pata_cmd64x.c +++ b/drivers/ata/pata_cmd64x.c @@ -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); - } } }
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 <tj@kernel.org> Reported-and-root-caused-by: Milan Kocian <milan.kocian@wq.cz> Cc: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> Cc: stable@kernel.org --- drivers/ata/pata_cmd64x.c | 6 ------ 1 file changed, 6 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html