On Thu, 17 Jul 2003 Andries.Brouwer@cwi.nl wrote:
> Dear Bartlomiej,
>
> Long ago I used to take care of disk geometry stuff.
> By some coincidence I looked today at ide-disk.c:init_idedisk_capacity().
> A sad sight. Three times the statement
> drive->cyl = (unsigned int) capacity_2 / (drive->head * drive->sect);
> Why not once? Or not at all?
I would like to now too. It looks as it can be simplified.
> Started making a patch, but it got a bit large.
> If I were Linus or Andrew I would never accept it at this stage.
> [Nevertheless, there are actual bugs here, that will cause devices to fail.]
> [Nevertheless, if Linus/Andrew/you are interested, I can come with a slow
> trickle of obviously correct changes fixing this whole area.]
Please finish this patch, it will go into -ide tree later.
> So, instead of a patch just some minor muttering. Maybe you can
> use some of it in future changes.
>
> Disk geometry does not exist. That makes it very confusing.
>
> Properly written code separates cleanly the source of information
> (is it from the user on the kernel command line?
> is it from the disk, from the IDENTIFY DEVICE command?
> is it from the BIOS? (From which BIOS function?)
> is it a guess based on a DOS-type partition table?
> is it some random number like 63 or 255 that we thought might
> be a good idea?).
>
> Properly written code separates cleanly the use of information
> (is it for CHS addressing the disk? then "heads" can be at most 16.
> is it for telling LILO about what we think the BIOS disk translation is?
> then "heads" can be at most 256, maybe at most 255).
So where is this properly written code? ;-)
> The present driver has drive->{head,sect,cyl} for the data
> used for CHS addressing.
> The present driver has drive->bios_{head,sect,cyl} for the translation
> data we think the BIOS uses.
>
> It follows that lines like "drive->head = 255;" are bugs.
>
> What is the use of
>
> if ((id->command_set_2 & 0x0400) && (id->cfs_enable_2 & 0x0400)) {
> drive->capacity48 = id->lba_capacity_2;
> drive->head = 255;
> drive->sect = 63;
> drive->cyl = (unsigned long)(drive->capacity48)
> / (drive->head * drive->sect);
> }
>
> ? If the disk knows about 48-bit LBA then probably it will not be
> addressed by CHS, but still - why change drive->head? To some
> ridiculous value?
Don't know, I need to check with ATA standard.
Also Andre added to cc:.
> It also follows that lines like "drive->id->lba_capacity_2 = ..."
> are very undesirable. Here the kernel modifies the disk report
> with some of its own inventions. But drive->id should be the
> unmodified IDENTIFY DRIVE output. Whatever the kernel thinks
> about the disk capacity should be written in drive->foocapacity.
> Lines like this make the HDIO_GET_IDENTITY ioctl rather worthless:
> one wants to investigate some disk problem, but the ioctl does not
> tell us what the disk reported, and instead gives some polluted version.
I think HDIO_GET_IDENTIFY should be fixed to read identify data directly
from device, just like /proc/ide/hdx/identify now does.
> Looking at this was prompted by reports on problems with pdcraid.
> It seems its geometry plays some role, so it is necessary to recognize
> precisely which geometry it is that plays a role, and then use the
> proper unmodified one.
Yes, I've read this thread.
Thanks for your comments,
-- Bartlomiej> Andries
- 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/
This archive was generated by hypermail 2b29 : Wed Jul 23 2003 - 22:00:31 EST