Re: [PATCH 2.6.9-libata1-dev1] libata-scsi: hdparm support

[Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>]

Hi!

On Thu, 04 Nov 2004 17:55:34 +0100, Tobias Lorenz <tobias.lorenz@gmx.net> wrote:
> Hi,
> 
> Bartlomiej Zolnierkiewicz wrote:
> > Speaking about correctness...
> >
> > HDIO_GET_IDENTIFY returns ID block in CPU order (not LE).
> >
> > If you decide to add it sooner or later you will need something ala
> > ide_fix_driveid() from ide-iops.c (bloat).
> 
> Okay, I am wondering, who defined, that hd_driveid has to be little
> endian and not cpu order. Does that really has to be platform

ATA spec ;-)

HDIO_DRIVE_CMD: LE order
HDIO_GET_IDENTIFY : CPU order

> independent ? But anyway...

HDIO_GET_IDENTIFY - yes

> There are two possible solutions:
> 
> The quick way is attached and just adds the missing conversions by
> copy&paste from ide-iops:ide_fix_driveid.

This is not needed (yet)...

ata_std_bios_params() returns things in CPU order arleady,
model and fw_rev are also in CPU order (libata driver handles them).

> The better and less redundant way would be to use the original function:
> - by setting a Kconfig dependency on CONFIG_IDE.
> - or by moving ide_fix_driveid to a seperate file (header file ?).

There was recent discussion about this wrt to isd200.c driver.
IMHO the correct solution is to not need ide_fix_driveid() et all.

> What do you mean ?

My point is that once you decide to support HDIO_GET_IDENTIFY
in libata you will have to take care of this issue (in the future)...

The result will be the same crap as ide_fix_driveid() etc.

I think that is much easier to teach blktool about SG_IO ioctl
and do the stuff right than to try to be "smart" and support all
badly designed IDE driver ioctls used by hdparm...

Bartlomiej

>   Toby
> 
> 
> Signed-off-by: Tobias Lorenz <tobias.lorenz@gmx.net>
> 
> --- a/drivers/scsi/libata-scsi.c        2004-10-30 16:31:01.000000000 +0200
> +++ b/drivers/scsi/libata-scsi.c        2004-11-04 17:39:58.000000000 +0100
> @@ -31,6 +31,7 @@
>  #include <linux/libata.h>
>  #include <linux/hdreg.h>
>  #include <asm/uaccess.h>
> +#include <asm/byteorder.h>
> 
>  #include "libata.h"
> 
> @@ -215,6 +216,17 @@
> 
> 
>         struct ata_port *ap;
>         struct ata_device *dev;
>         int val = -EINVAL, rc = -EINVAL;
> +       struct hd_driveid drv_id = {
> +               .cyls           = 0,
> +               .sectors        = 0,
> +               .heads          = 0,
> +               .fw_rev         = "",
> +               .model          = "",
> +               .cur_cyls       = 0,
> +               .cur_heads      = 0,
> +               .cur_sectors    = 0,
> +       };
> +       int geom[3];
> 
>         ap = (struct ata_port *) &scsidev->host->hostdata[0];
>         if (!ap)
> @@ -249,6 +261,40 @@
> 
> 
>                         return -EACCES;
>                 return ata_task_ioctl(scsidev, arg);
> 
> +       case HDIO_GET_IDENTITY:
> +               ata_std_bios_param(scsidev, NULL, dev->n_sectors, geom);
> +               drv_id.cur_heads        = drv_id.heads          = geom[0];
> +               drv_id.cur_sectors      = drv_id.sectors        = geom[1];
> +               drv_id.cur_cyls         = drv_id.cyls           = geom[2];
> +               strncpy((char *) &drv_id.model, scsidev->model, sizeof(drv_id.model));
> +               strncpy((char *) &drv_id.fw_rev, scsidev->rev, sizeof(drv_id.fw_rev));
> +#ifndef __LITTLE_ENDIAN
> +# ifdef __BIG_ENDIAN
> +               {
> +                       int i;
> +                       u16 *stringcast;
> +
> +                       drv_id.cyls           = __le16_to_cpu(drv_id.cyls);
> +                       drv_id.heads          = __le16_to_cpu(drv_id.heads);
> +                       drv_id.sectors        = __le16_to_cpu(drv_id.sectors);
> +                       stringcast = (u16 *)&drv_id.fw_rev[0];
> +                       for (i = 0; i < (8/2); i++)
> +                               stringcast[i] = __le16_to_cpu(stringcast[i]);
> +                       stringcast = (u16 *)&drv_id.model[0];
> +                       for (i = 0; i < (40/2); i++)
> +                               stringcast[i] = __le16_to_cpu(stringcast[i]);
> +                       drv_id.cur_cyls       = __le16_to_cpu(drv_id.cur_cyls);
> +                       drv_id.cur_heads      = __le16_to_cpu(drv_id.cur_heads);
> +                       drv_id.cur_sectors    = __le16_to_cpu(drv_id.cur_sectors);
> +               }
> +# else
> +#  error "Please fix <asm/byteorder.h>"
> +# endif
> +#endif
> 
> 
> +               if(copy_to_user((char *) arg, (char *) &drv_id, sizeof(drv_id)))
> +                       return(-EFAULT);
> +               return 0;
> +
>         default:
>                 rc = -EOPNOTSUPP;
>                 break;
> 
>