[PATCH #upstream-fixes] pata_cmd64x: revert commit d62f5576

[PATCH #upstream-fixes] pata_cmd64x: revert commit d62f5576

[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 <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);
-			}
 		}
 	}

Re: [PATCH #upstream-fixes] pata_cmd64x: revert commit d62f5576

[Bartlomiej Zolnierkiewicz]

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);
> -			}
>  		}
>  	}

Re: [PATCH #upstream-fixes] pata_cmd64x: revert commit d62f5576

[Tejun Heo]

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.

-- 
tejun

Re: [PATCH #upstream-fixes] pata_cmd64x: revert commit d62f5576

[Alan Cox]

> 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

Re: [PATCH #upstream-fixes] pata_cmd64x: revert commit d62f5576

[Bartlomiej Zolnierkiewicz]

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

Re: [PATCH #upstream-fixes] pata_cmd64x: revert commit d62f5576

[Sergei Shtylyov]

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

Re: [PATCH #upstream-fixes] pata_cmd64x: revert commit d62f5576

[Jeff Garzik]

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.

Re: [PATCH #upstream-fixes] pata_cmd64x: revert commit d62f5576

[Milan Kocian]

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

-- 
Milan Kocian