Re: [RFT][PATCH] ide-disk.c: more write cache fixes

[Rene Herman]

Bartlomiej Zolnierkiewicz wrote:

> Comments, suggestions, complains?

Yes, this works to stop it from complaining (on 6Y120P0). It comes up 
with write cache enabled, and hdparm -W0/-W1 work to disable/enable 
write cache as evidenced by the tiobench results. Not as evidenced by 
/proc/ide/hda/settings (drive->wcache) which is always 1 and which will 
probably confuse more users than just me -- I believe I saw hdparm just 
pushes a drive command through an ioctl?

Question though:

> @@ -1678,8 +1683,12 @@ static void idedisk_setup (ide_drive_t *
>  #endif	/* CONFIG_IDEDISK_MULTI_MODE */
>  	}
>  	drive->no_io_32bit = id->dword_io ? 1 : 0;
> -	if (drive->id->cfs_enable_2 & 0x3000)
> -		write_cache(drive, (id->cfs_enable_2 & 0x3000));
> +
> +	/* write cache enabled? */
> +	if ((id->csfo & 1) || (id->cfs_enable_1 & (1 << 5)))
> +		drive->wcache = 1;
> +
> +	write_cache(drive, 1);

write_cache() also sets drive->wcache (to the argument, 1 in this case) 
and you call that unconditionally, so the "if (foo) drive->wcache = 1" 
seems superfluous. If the idea indeed is to unconditionally enable write 
cache, it seems just

write_cache(drive, 1);

would be equivalent. Or if that wasn't the intention, maybe:

if (foo)
	write_cache(drive, 1);

or if it should in fact be disabled if (!foo):

write_cache(drive, (id->csfo & 1) || (id->cfs_enable_1 & (1 << 5)));

or ...

Ignore me if I completely missed the point, just looks odd.

Rene.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/