Re: GNU Mach: IDE disk drives with unusual C/H/S

[Ivan Shmakov]

>>>>> Thomas Schwinge <thomas@codesourcery.com> writes:

[…]

 > Does anyone spot any problems, or does this need more testing, or is
 > it OK to commit right away?

        I see none (but then, I'm not really a Mach or ATA expert),
        apart from a few style and formatting issues.

[…]

 > +    if (id->cyls == 16383 && id->sectors == 63 &&
 > +        (id->heads == 15 || id->heads == 16) &&
 > +        id->lba_capacity >= 16383*63*id->heads)
 >              return 1;       /* lba_capacity is our only option */
 > -    }
 > +

        Here, a diff line could've been saved by adding an opening { to
        the if above.  (IOW, no need to drop the C grouping construct
        from here.)

[…]

 > @@ -2568,8 +2581,7 @@ static inline void do_identify (ide_drive_t *drive, 
 > byte cmd)
 >      }
 >      /* Handle logical geometry translation by the drive */
 >      if ((id->field_valid & 1) && id->cur_cyls && id->cur_heads
 > -     && (id->cur_heads <= 16) && id->cur_sectors)
 > -    {
 > +        && (id->cur_heads <= 16) && id->cur_sectors) {
 >              /*
 >               * Extract the physical drive geometry for our use.
 >               * Note that we purposely do *not* update the bios info.
 > @@ -2594,7 +2606,8 @@ static inline void do_identify (ide_drive_t *drive, 
 > byte cmd)
 >              }
 >      }
 >      /* Use physical geometry if what we have still makes no sense */
 > -    if ((!drive->head || drive->head > 16) && id->heads && id->heads <= 16) 
 > {
 > +    if ((!drive->head || drive->head > 16) &&
 > +        id->heads && id->heads <= 16) {
 >              drive->cyl  = id->cyls;
 >              drive->head = id->heads;
 >              drive->sect = id->sectors;

        These two are only stylistic changes, aren't they?  For clarity,
        it may be reasonable to put them into a separate patch.

        Also, for consistency with the style of the code above, my
        suggestion would be to put && to the second line instead:

+       if ((!drive->head || drive->head > 16)
+           && id->heads && id->heads <= 16) {

[…]

 > @@ -3346,7 +3360,7 @@ done:
 >   * This routine is called from the partition-table code in genhd.c
 >   * to "convert" a drive to a logical geometry with fewer than 1024 cyls.
 >   *
 > - * The second parameter, "xparm", determines exactly how the translation
 > + * The second parameter, "xparm", determines exactly how the translation 
 >   * will be handled:
 >   *           0 = convert to CHS with fewer than 1024 cyls
 >   *                  using the same method as Ontrack DiskManager.
 > @@ -3354,10 +3368,11 @@ done:
 >   *          -1 = similar to "0", plus redirect sector 0 to sector 1.
 >   *          >1 = convert to a CHS geometry with "xparm" heads.
 >   *
 > - * Returns 0 if the translation was not possible, if the device was not
 > + * Returns 0 if the translation was not possible, if the device was not 

        In the two instances above, a (presumably unwanted) trailing
        blank was added.

 >   * an IDE disk drive, or if a geometry was "forced" on the commandline.
 >   * Returns 1 if the geometry translation was successful.
 >   */
 > +

        It seems that no blank lines separate the function's description
        from its definition (at least as well as the rest of the diff is
        considered.)

 >  int ide_xlate_1024 (kdev_t i_rdev, int xparm, const char *msg)
 >  {
 >      ide_drive_t *drive;
 > @@ -3365,7 +3380,11 @@ int ide_xlate_1024 (kdev_t i_rdev, int xparm, const 
 > char *msg)
 >      const byte *heads = head_vals;
 >      unsigned long tracks;
 >  
 > -    if ((drive = get_info_ptr(i_rdev)) == NULL || drive->forced_geom)
 > +    drive = get_info_ptr(i_rdev);
 > +    if (!drive)
 > +            return 0;
 > +
 > +    if (drive->forced_geom)
 >              return 0;
 >  
 >      if (xparm > 1 && xparm <= drive->bios_head && drive->bios_sect == 63)

        This looks like a purely stylistic change as well?

[…]

-- 
FSF associate member #7257