Re: [PATCH] Re: [PATCH][IDE] update qd65xx driver

Re: [PATCH] Re: [PATCH][IDE] update qd65xx driver

[Andries.Brouwer]

	From B.Zolnierkiewicz@elka.pw.edu.pl  Fri Sep 12 00:44:48 2003

	> That reminds me, did I ever send you this?
	>
	> Andries

	No, only similar patch for hpt366.c.

	I think the (almost) correct scheme is following ...

Yes, larger changes are possible - and in fact I have a directory
full of IDE stuff, polishing, cleanup, non-urgent.

I sent this mainly because the hpt366.c analog was needed to
prevent filesystem corruption (on my own system). Similarly,
I imagine this patch is needed to prevent filesystem corruption -
no need to wait until someone actually reports a corrupted filesystem.

Patches that allow people to set lower PIO modes than the max
may be nice, but are less urgent than preventing modes higher
than the max.

Andries

	> -		pio = ide_get_best_pio_mode(drive, pio, 255, &d);
	> +		pio = ide_get_best_pio_mode(drive, 255, pio, &d);

Re: [PATCH] Re: [PATCH][IDE] update qd65xx driver

[Bartlomiej Zolnierkiewicz]

On Saturday 13 of September 2003 01:09, Andries.Brouwer@cwi.nl wrote:
> From B.Zolnierkiewicz@elka.pw.edu.pl  Fri Sep 12 00:44:48 2003
>
> 	> That reminds me, did I ever send you this?
> 	>
> 	> Andries
>
> 	No, only similar patch for hpt366.c.
>
> 	I think the (almost) correct scheme is following ...
>
> Yes, larger changes are possible - and in fact I have a directory
> full of IDE stuff, polishing, cleanup, non-urgent.

Great!  Please push'em.

> I sent this mainly because the hpt366.c analog was needed to
> prevent filesystem corruption (on my own system). Similarly,
> I imagine this patch is needed to prevent filesystem corruption -
> no need to wait until someone actually reports a corrupted filesystem.
>
> Patches that allow people to set lower PIO modes than the max
> may be nice, but are less urgent than preventing modes higher
> than the max.

Eeeh... please re-read my mail.

As stated before you got corruption with hpt366.c because it was calling
hpt3xx_tune_drive() *internally* with pio argument equal to 5 instead of 255.

qd65xx.c is not calling qd*_tune_drive() internally et all -> no possibility
of corruption unless user *manually* sets mode higher than supported.

--bartlomiej

> Andries
>
> 	> -		pio = ide_get_best_pio_mode(drive, pio, 255, &d);
> 	> +		pio = ide_get_best_pio_mode(drive, 255, pio, &d);

Re: [PATCH] Re: [PATCH][IDE] update qd65xx driver

[Andries.Brouwer]

> As stated before you got corruption with hpt366.c because it was calling
> hpt3xx_tune_drive() *internally* with pio argument equal to 5 instead of 255.
>
> qd65xx.c is not calling qd*_tune_drive() internally et all -> no possibility
> of corruption unless user *manually* sets mode higher than supported.

OK, agreed.

[PATCH] Re: [PATCH][IDE] update qd65xx driver

[Andries.Brouwer]

That reminds me, did I ever send you this?

Andries

diff -u --recursive --new-file -X /linux/dontdiff a/drivers/ide/legacy/qd65xx.c b/drivers/ide/legacy/qd65xx.c
--- a/drivers/ide/legacy/qd65xx.c	Mon Sep  8 23:44:59 2003
+++ b/drivers/ide/legacy/qd65xx.c	Thu Sep 11 23:20:26 2003
@@ -261,7 +261,7 @@
 	int recovery_time = 415; /* worst case values from the dos driver */
 
 	if (drive->id && !qd_find_disk_type(drive, &active_time, &recovery_time)) {
-		pio = ide_get_best_pio_mode(drive, pio, 255, &d);
+		pio = ide_get_best_pio_mode(drive, 255, pio, &d);
 		pio = IDE_MIN(pio,4);
 
 		switch (pio) {

Re: [PATCH] Re: [PATCH][IDE] update qd65xx driver

[Bartlomiej Zolnierkiewicz]

On Thursday 11 of September 2003 23:51, Andries.Brouwer@cwi.nl wrote:
> That reminds me, did I ever send you this?
>
> Andries

No, only similar patch for hpt366.c.

I think the (almost) correct scheme is following (some drivers get it wrong):

config_chipset_pio() style functions are used for driver auto-tuning
and  "255, pio" is used to find best PIO mode supported by device.
"pio" argument should be set to highest mode allowed by controller.

tune_proc()/tune_drive() style functions are used for direct mode setting
and "pio, max_pio" is used to set desired mode.
"pio" is desired mode, "max_pio" is max mode supported by controller.

It's almost correct because we should also check if desired PIO mode
is supported by device, currently we let user shoot herself/himself.

IDE driver itself calls ->tuneproc() with argument == 255, so always
highest supported PIO mode is chosen.  User-space can call ->tuneproc()
with smaller argument.  With you patch smaller than highest possible
PIO mode can't be chosen.

Your patch for hpt366.c has also this side effect that you can now only
set highest possible PIO mode.  You should revert this change
and fix hpt3xx_tune_drive() call inside hpt366_config_drive_xfer_rate()
to pass 255 instead of 5.

--bartlomiej

> diff -u --recursive --new-file -X /linux/dontdiff
> a/drivers/ide/legacy/qd65xx.c b/drivers/ide/legacy/qd65xx.c ---
> a/drivers/ide/legacy/qd65xx.c	Mon Sep  8 23:44:59 2003
> +++ b/drivers/ide/legacy/qd65xx.c	Thu Sep 11 23:20:26 2003
> @@ -261,7 +261,7 @@
>  	int recovery_time = 415; /* worst case values from the dos driver */
>
>  	if (drive->id && !qd_find_disk_type(drive, &active_time, &recovery_time))
> { -		pio = ide_get_best_pio_mode(drive, pio, 255, &d);
> +		pio = ide_get_best_pio_mode(drive, 255, pio, &d);
>  		pio = IDE_MIN(pio,4);
>
>  		switch (pio) {