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

[Bartlomiej Zolnierkiewicz <B.Zolnierkiewicz@elka.pw.edu.pl>]

On Friday 14 of May 2004 01:28, Rene Herman wrote:
> 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?

Yep, you are right and we shouldn't worry about confusion too much,
we have similar situation till now (wcache was zero even if enabled).

> 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

Please look at write_cache(), it will try to {en,dis}able write cache
and then set drive->wcache *only* if disk has ATA-6 cache flush bits.

I think that we can change write_cache() to always allow enabling of
write cache so hdparm can be fixed to use HDIO_{GET,SET}_WCACHE ioctls
but we should be careful about always enabling write cache in a driver
(there may be some unknown yet side-effects).

Whatever we decide we can do this later after this patch is merged.

> 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.

:-)

Thanks,
Bartlomiej