Re: [PATCH] IDE disks show invalid geometries in /proc/ide/hd*/geometry

[Mark Bellon <mbellon@mvista.com>]

Bartlomiej Zolnierkiewicz wrote:

>On 8/3/05, Mark Bellon <mbellon@mvista.com> wrote:
>  
>
>>Bartlomiej Zolnierkiewicz wrote:
>>
>>    
>>
>>>Hi,
>>>
>>>The topic was discussed to death on linux-kernel.
>>>
>>>Mark, you need to fix your applications and stop using /proc/ide/hd*/geometry
>>>or/and HDIO_GET_GEO ioctl (which BTW your patch also affects).
>>>
>>>
>>>      
>>>
>>Fixing the applications I can understand but the patch still seems
>>necessary, to me, so the
>>HDIO_GET_GEO returns "rational" values.
>>
>>I tested  HDIO_GET_GEO and it returns the same broken values as go into
>>/proc (no surprises) without my patch.
>>    
>>
>
>Please explain.
>  
>
Let me explain more below. Thanks for the response as now I can 
understand better the issue (which was what I was fishing for).

>  
>
>>If a drive is in LBA mode (28 or 48 bit) the existing code doesn't
>>always "fix up" the geometry properly for some value returns. It only
>>tries with 48 bit mode and it fails there for some values.  My patch
>>forces a complete geometry and appears (to me) to preserve the side
>>efefcts of the existing code.
>>
>>Am I missing something?
>>    
>>
>
>diff -Naur linux-2.6.13-rc3-git9-orig/drivers/ide/ide-disk.c
>linux-2.6.13-rc3-git9/drivers/ide/ide-disk.c
>--- linux-2.6.13-rc3-git9-orig/drivers/ide/ide-disk.c	2005-08-01
>13:48:21.000000000 -0700
>+++ linux-2.6.13-rc3-git9/drivers/ide/ide-disk.c	2005-08-02
>12:14:43.000000000 -0700
>@@ -884,10 +884,17 @@
>  	ide_add_setting(drive,	"max_failures",		SETTING_RW,					-1,			-1,			TYPE_INT,	0,	65535,				1,	1,	&drive->max_failures,		NULL);
> }
> 
>+static uint32_t do_div64_32 (__u64 n, uint32_t d)
>+{
>+	do_div(n, d);
>+
>+	return n;
>+}
>+
> static void idedisk_setup (ide_drive_t *drive)
> {
> 	struct hd_driveid *id = drive->id;
>-	unsigned long long capacity;
>+	__u64 capacity;
> 	int barrier;
> 
> 	idedisk_add_settings(drive);
>@@ -949,27 +956,32 @@
> 	 */
> 	capacity = idedisk_capacity (drive);
> 	if (!drive->forced_geom) {
>+		uint32_t cylsz, cyl;
> 
>-		if (idedisk_supports_lba48(drive->id)) {
>-			/* compatibility */
>-			drive->bios_sect = 63;
>-			drive->bios_head = 255;
>+		/*
>+		 * In LBA mode the geometry isn't used to talk to the drive
>+		 * so always create a "rational" geometry from the capacity.
>+		 */
>+		if (drive->select.b.lba) {
>
>why are you forcing drive->bios_sect and drive->bios_head values for LBA28 drives?
>  
>
Any LBA 28 drive can return fixed geometry (regardless of size) just as 
well as an LBA 48 drive can. I've got drives all over the place that do 
just that. In any event there are machines which do not have have an 
BIOS, like a PPC target, and the BIOS values can be "fixed up".

The ioctl returns the BIOS values and these are totally wrong on some 
targets. Comments?

>+			drive->bios_sect = drive->sect = 63;
>+			drive->bios_head = drive->head = 255;
>
>changing drive->sect and drive->head is just wrong,
>these are values reported by the device - do not touch them
>  
>
If drive->cyl, drive-head amd drive->sect are to be left alone in all 
cases I have no problem.

>+
>+			cylsz = drive->bios_sect * drive->bios_head;
>+			cyl = do_div64_32(capacity, cylsz);
>
>division by zero now is possible (yes, 0/0/0 is possible)
>  
>
Then the original code is prefered.

>+
>+			/* "fake out" works up to ~500 GB */
>+			cyl = (cyl > 65535) ? 65535 : cyl;
>+			drive->bios_cyl = drive->cyl = cyl;
>
>drive->cyl shouldn't be changed
>  
>
Same comment as above. Only change BIOS values.

> 		}
> 
> 		if (drive->bios_sect && drive->bios_head) {
>-			unsigned int cap0 = capacity; /* truncate to 32 bits */
>-			unsigned int cylsz, cyl;
>+			cylsz = drive->bios_sect * drive->bios_head;
>+			cyl = do_div64_32(capacity, cylsz);
> 
>-			if (cap0 != capacity)
>-				drive->bios_cyl = 65535;
>
>cap0 != capacity => set the max possible drive->bios_cyl,
>no need for adjusting it
>
>-			else {
>
>cap0 == capacity => no need for do_div(), only lower 32-bits are used,
>adjust drive->bios_cyl
>
>-				cylsz = drive->bios_sect * drive->bios_head;
>-				cyl = cap0 / cylsz;
>-				if (cyl > 65535)
>-					cyl = 65535;
>-				if (cyl > drive->bios_cyl)
>-					drive->bios_cyl = cyl;
>-			}
>+			if (cyl > 65535)
>+				cyl = 65535;
>+			if (cyl > drive->bios_cyl)
>+				drive->bios_cyl = cyl;
> 		}
> 	}
> 	printk(KERN_INFO "%s: %llu sectors (%llu MB)",
>  
>
I understand clearly now...

mark

>  
>
>>mark
>>
>>    
>>
>>>Bartlomiej
>>>
>>>On 8/3/05, Andre Hedrick <andre@linux-ide.org> wrote:
>>>
>>>
>>>      
>>>
>>>>Did you read ATA-1 through ATA-7 to understand all the variations?
>>>>
>>>>On Tue, 2 Aug 2005, Mark Bellon wrote:
>>>>
>>>>
>>>>
>>>>        
>>>>
>>>>>The ATA specification tells large disk drives to return C/H/S data of
>>>>>16383/16/63 regardless of their actual size (other variations on this
>>>>>return include 15 heads and/or 4092 cylinders). Unfortunately these CHS
>>>>>data confuse the existing IDE code and cause it to report invalid
>>>>>geometries in /proc when the disk runs in LBA mode.
>>>>>
>>>>>The invalid geometries can cause failures in the partitioning tools;
>>>>>partitioning may be impossible or illogical size limitations occur. This
>>>>>also leads to various forms of human confusion.
>>>>>
>>>>>I attach a patch that fixes this problem while strongly attempting to
>>>>>not break any existing side effects and await any comments.
>>>>>
>>>>>mark
>>>>>
>>>>>Signed-off-by: Mark Bellon <mbellon@mvista.com>
>>>>>          
>>>>>