Messages in this thread | | | Date | Thu, 7 Aug 2003 03:11:46 +0200 | From | Andries Brouwer <> | Subject | Re: [PATCH] ide-disk.c rev 1.13 killed CONFIG_IDEDISK_STROKE |
| |
On Wed, Aug 06, 2003 at 08:32:23PM +0200, Bartlomiej Zolnierkiewicz wrote:
> diff -puN drivers/ide/ide-disk.c~ide-disk-capacity-init-cleanup drivers/ide/ide-disk.c > --- linux-2.6.0-test2-bk5/drivers/ide/ide-disk.c~ide-disk-capacity-init-cleanup 2003-08-06 02:48:33.000000000 +0200
Ha - and you didnt even tell me you had this patch out.
Looks good. You forgot to correct the do_div. The part I most object to are things like
> + id->lba_capacity_2 = capacity_2 = set_max_ext;
There have been many problems in the past, and it is a bad idea to add more of this. We should be eliminating all cases.
I mean:
We have info from BIOS, user, disk etc and conclude to a certain geometry. Sneakily changing what the disk reported is very ugly. I recall a case where a disk bounced between two capacities because the value that this computation concluded to was not a fixed point. Also, the user gets an incorrect report from HDIO_GET_IDENTITY.
So, the clean way is to examine what the disk reported, never change it (except for the part
-------------------------------------------------------------- void ide_fix_driveid (struct hd_driveid *id) { #ifndef __LITTLE_ENDIAN # ifdef __BIG_ENDIAN int i; u16 *shortcast;
shortcast = (u16 *) id; for (i = 0; i < (sizeof(struct hd_driveid) / 2); i++) shortcast[i] = __le16_to_cpu(shortcast[i]); # else # error "Please fix <asm/byteorder.h>" # endif #endif } --------------------------------------------------------------
that brings shorts into CPU order) and store the results elsewhere. In particular, our conclusions should go into drive->capacity and should not overwrite id->lba_capacity.
Andries
[will try to find the appropriate fragment of my patch again for comparison purposes]
- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |