Messages in this thread | | | Date | Fri, 18 Jul 2003 00:12:10 +0200 (MET DST) | From | Bartlomiej Zolnierkiewicz <> | Subject | Re: disk geometry |
| |
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/
| |