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

[Bartlomiej Zolnierkiewicz]

On 8/3/05, Mark Bellon <mbellon@xxxxxxxxxx> wrote:
> Bartlomiej Zolnierkiewicz wrote:
> 
> >On 8/3/05, Mark Bellon <mbellon@xxxxxxxxxx> 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?

Simple: do not use BIOS values.

[ Yes, there should be some warning from kernel. ]

> >+                      drive->bios_sect = drive->sect = 63;
> >+                      drive->bios_head = drive->head = 255;
> >
t> >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@xxxxxxxxxxxxx> 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@xxxxxxxxxx>
> >>>>>
> >>>>>
> 
>
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/