#59 incorrect HOB/LOB bit field union definition of struct hdio_taskfile

v1.0 (example)
closed-fixed
nobody
None
5
2014-12-18
2014-08-25
Lucas Magasweran
No

Not sure if this was addressed with the fix for [#49] but the definition of the bit field unions in struct hdio_taskfile are incorrect. In order to match the kernel's union ide_reg_valid_s the HOB and LOB unions should be inside a struct.

--- hdparm-9.43-orig/sgio.h 2014-08-25 15:07:08.461172023 -0700
+++ hdparm-9.43/sgio.h  2014-08-25 14:26:19.721131780 -0700
@@ -118,6 +118,7 @@

 union reg_flags {
    unsigned all                :16;
+    struct {
    union {
        unsigned lob_all        : 8;
        struct {
@@ -144,6 +145,7 @@
            unsigned command    : 1;
        } hob;
    };
+    } bits;
 } __attribute__((packed));

 struct taskfile_regs {

The complete set of patches for hdparm 9.43:

diff -w -b -u hdparm-9.43-orig/fwdownload.c hdparm-9.43/fwdownload.c
--- hdparm-9.43-orig/fwdownload.c   2009-07-23 05:53:35.000000000 -0700
+++ hdparm-9.43/fwdownload.c    2014-08-25 12:16:19.353003589 -0700
@@ -49,8 +49,8 @@
    init_hdio_taskfile(r, ATA_OP_DOWNLOAD_MICROCODE, RW_WRITE, LBA28_OK, lba, blockcount & 0xff, bytecount);

    r->lob.feat = xfer_mode;
-   r->oflags.lob.feat  = 1;
-   r->iflags.lob.nsect = 1;
+   r->oflags.bits.lob.feat  = 1;
+   r->iflags.bits.lob.nsect = 1;

    if (data && bytecount)
        memcpy(r->data, data, bytecount);
diff -w -b -u hdparm-9.43-orig/hdparm.c hdparm-9.43/hdparm.c
--- hdparm-9.43-orig/hdparm.c   2012-11-15 14:08:51.000000000 -0800
+++ hdparm-9.43/hdparm.c    2014-08-25 15:11:59.525176806 -0700
@@ -752,14 +752,14 @@
    r->dphase   = TASKFILE_DPHASE_PIO_OUT;
    r->obytes   = 512;
    r->lob.command  = security_command;
-   r->oflags.lob.nsect = 1;
+   r->oflags.bits.lob.nsect = 1;
    r->lob.nsect        = 1;
    data        = (__u8*)r->data;
    data[0]     = security_master & 0x01;
    memcpy(data+2, security_password, 32);

-   r->oflags.lob.command = 1;
-   r->oflags.lob.feat    = 1;
+   r->oflags.bits.lob.command = 1;
+   r->oflags.bits.lob.feat    = 1;

    switch (security_command) {
        case ATA_OP_SECURITY_ERASE_UNIT:
@@ -1010,18 +1010,18 @@
    memset(&r, 0, sizeof(r));
    r.cmd_req = TASKFILE_CMD_REQ_NODATA;
    r.dphase  = TASKFILE_DPHASE_NONE;
-   r.oflags.lob.dev      = 1;
-   r.oflags.lob.command  = 1;
-   r.iflags.lob.command  = 1;
-   r.iflags.lob.lbal     = 1;
-   r.iflags.lob.lbam     = 1;
-   r.iflags.lob.lbah     = 1;
+   r.oflags.bits.lob.dev      = 1;
+   r.oflags.bits.lob.command  = 1;
+   r.iflags.bits.lob.command  = 1;
+   r.iflags.bits.lob.lbal     = 1;
+   r.iflags.bits.lob.lbam     = 1;
+   r.iflags.bits.lob.lbah     = 1;
    r.lob.dev = 0x40;

    if (((id[83] & 0xc400) == 0x4400) && (id[86] & 0x0400)) {
-       r.iflags.hob.lbal  = 1;
-       r.iflags.hob.lbam  = 1;
-       r.iflags.hob.lbah  = 1;
+       r.iflags.bits.hob.lbal  = 1;
+       r.iflags.bits.hob.lbam  = 1;
+       r.iflags.bits.hob.lbah  = 1;
        r.lob.command = ATA_OP_READ_NATIVE_MAX_EXT;
        if (do_taskfile_cmd(fd, &r, 10)) {
            err = errno;
@@ -1034,7 +1034,7 @@
                     | ((r.lob.lbah << 16) | (r.lob.lbam << 8) | r.lob.lbal)) + 1;
        }
    } else {
-       r.iflags.lob.dev = 1;
+       r.iflags.bits.lob.dev = 1;
        r.lob.command = ATA_OP_READ_NATIVE_MAX;
        if (do_taskfile_cmd(fd, &r, 0)) {
            err = errno;
@@ -1072,7 +1072,7 @@
                "This operation will probably fail (continuing regardless).\n");
        }
        init_hdio_taskfile(r, ATA_OP_WRITE_UNC_EXT, RW_READ, LBA48_FORCE, lba, 1, 0);
-       r->oflags.lob.feat = 1;
+       r->oflags.bits.lob.feat = 1;
        r->lob.feat = make_bad_sector_flagged ? 0xaa : 0x55;
        flagged     = make_bad_sector_flagged ? "flagged" : "pseudo";
        printf("Corrupting sector %llu (WRITE_UNC_EXT as %s): ", lba, flagged);
@@ -1392,7 +1392,7 @@

    abort_if_not_full_device(fd, 0, devname, NULL);
    init_hdio_taskfile(&r, ATA_OP_IDLEIMMEDIATE, RW_READ, LBA28_OK, 0x0554e4c, 0, 0);
-   r.oflags.lob.feat = 1;
+   r.oflags.bits.lob.feat = 1;
    r.lob.feat = 0x44;

    if (do_taskfile_cmd(fd, &r, 0)) {
@@ -1415,7 +1415,7 @@
        init_hdio_taskfile(&r, ATA_OP_SET_MAX_EXT, RW_READ, LBA48_FORCE, max_lba, nsect, 0);
    } else {
        init_hdio_taskfile(&r, ATA_OP_SET_MAX, RW_READ, LBA28_OK, max_lba, nsect, 0);
-       r.oflags.lob.feat = 1;  /* this ATA op requires feat==0 */
+       r.oflags.bits.lob.feat = 1;  /* this ATA op requires feat==0 */
    }

    /* spec requires that we do this immediately in front.. racey */
diff -w -b -u hdparm-9.43-orig/sgio.c hdparm-9.43/sgio.c
--- hdparm-9.43-orig/sgio.c 2012-09-28 10:29:24.000000000 -0700
+++ hdparm-9.43/sgio.c  2014-08-25 15:14:55.557179699 -0700
@@ -430,38 +430,38 @@
    tf_init(&tf, 0, 0, 0);
 #if 1 /* debugging */
    if (verbose) {
-       printf("oflags.lob_all=0x%02x, flags={", r->oflags.lob_all);
-       if (r->oflags.lob.feat) printf(" feat");
-       if (r->oflags.lob.nsect)printf(" nsect");
-       if (r->oflags.lob.lbal) printf(" lbal");
-       if (r->oflags.lob.lbam) printf(" lbam");
-       if (r->oflags.lob.lbah) printf(" lbah");
-       if (r->oflags.lob.dev)  printf(" dev");
-       if (r->oflags.lob.command) printf(" command");
+       printf("oflags.bits.lob_all=0x%02x, flags={", r->oflags.bits.lob_all);
+       if (r->oflags.bits.lob.feat)    printf(" feat");
+       if (r->oflags.bits.lob.nsect)printf(" nsect");
+       if (r->oflags.bits.lob.lbal)    printf(" lbal");
+       if (r->oflags.bits.lob.lbam)    printf(" lbam");
+       if (r->oflags.bits.lob.lbah)    printf(" lbah");
+       if (r->oflags.bits.lob.dev) printf(" dev");
+       if (r->oflags.bits.lob.command) printf(" command");
        printf(" }\n");
-       printf("oflags.hob_all=0x%02x, flags={", r->oflags.hob_all);
-       if (r->oflags.hob.feat) printf(" feat");
-       if (r->oflags.hob.nsect)printf(" nsect");
-       if (r->oflags.hob.lbal) printf(" lbal");
-       if (r->oflags.hob.lbam) printf(" lbam");
-       if (r->oflags.hob.lbah) printf(" lbah");
+       printf("oflags.bits.hob_all=0x%02x, flags={", r->oflags.bits.hob_all);
+       if (r->oflags.bits.hob.feat)    printf(" feat");
+       if (r->oflags.bits.hob.nsect)printf(" nsect");
+       if (r->oflags.bits.hob.lbal)    printf(" lbal");
+       if (r->oflags.bits.hob.lbam)    printf(" lbam");
+       if (r->oflags.bits.hob.lbah)    printf(" lbah");
        printf(" }\n");
    }
 #endif
-   if (r->oflags.lob.feat)     tf.lob.feat  = r->lob.feat;
-   if (r->oflags.lob.lbal)     tf.lob.lbal  = r->lob.lbal;
-   if (r->oflags.lob.nsect)    tf.lob.nsect = r->lob.nsect;
-   if (r->oflags.lob.lbam)     tf.lob.lbam  = r->lob.lbam;
-   if (r->oflags.lob.lbah)     tf.lob.lbah  = r->lob.lbah;
-   if (r->oflags.lob.dev)      tf.dev       = r->lob.dev;
-   if (r->oflags.lob.command)  tf.command   = r->lob.command;
-   if (needs_lba48(tf.command,0,0) || r->oflags.hob_all || r->iflags.hob_all) {
+   if (r->oflags.bits.lob.feat)        tf.lob.feat  = r->lob.feat;
+   if (r->oflags.bits.lob.lbal)        tf.lob.lbal  = r->lob.lbal;
+   if (r->oflags.bits.lob.nsect)   tf.lob.nsect = r->lob.nsect;
+   if (r->oflags.bits.lob.lbam)        tf.lob.lbam  = r->lob.lbam;
+   if (r->oflags.bits.lob.lbah)        tf.lob.lbah  = r->lob.lbah;
+   if (r->oflags.bits.lob.dev)     tf.dev       = r->lob.dev;
+   if (r->oflags.bits.lob.command) tf.command   = r->lob.command;
+   if (needs_lba48(tf.command,0,0) || r->oflags.bits.hob_all || r->iflags.bits.hob_all) {
        tf.is_lba48 = 1;
-       if (r->oflags.hob.feat) tf.hob.feat  = r->hob.feat;
-       if (r->oflags.hob.lbal) tf.hob.lbal  = r->hob.lbal;
-       if (r->oflags.hob.nsect)tf.hob.nsect = r->hob.nsect;
-       if (r->oflags.hob.lbam) tf.hob.lbam  = r->hob.lbam;
-       if (r->oflags.hob.lbah) tf.hob.lbah  = r->hob.lbah;
+       if (r->oflags.bits.hob.feat)    tf.hob.feat  = r->hob.feat;
+       if (r->oflags.bits.hob.lbal)    tf.hob.lbal  = r->hob.lbal;
+       if (r->oflags.bits.hob.nsect)   tf.hob.nsect = r->hob.nsect;
+       if (r->oflags.bits.hob.lbam)    tf.hob.lbam  = r->hob.lbam;
+       if (r->oflags.bits.hob.lbah)    tf.hob.lbah  = r->hob.lbah;
        if (verbose)
            fprintf(stderr, "using LBA48 taskfile\n");
    }
@@ -485,18 +485,18 @@
    }

    if (rc == 0 || errno == EIO) {
-       if (r->iflags.lob.feat)     r->lob.feat  = tf.error;
-       if (r->iflags.lob.lbal)     r->lob.lbal  = tf.lob.lbal;
-       if (r->iflags.lob.nsect)    r->lob.nsect = tf.lob.nsect;
-       if (r->iflags.lob.lbam)     r->lob.lbam  = tf.lob.lbam;
-       if (r->iflags.lob.lbah)     r->lob.lbah  = tf.lob.lbah;
-       if (r->iflags.lob.dev)      r->lob.dev   = tf.dev;
-       if (r->iflags.lob.command)  r->lob.command = tf.status;
-       if (r->iflags.hob.feat)     r->hob.feat  = tf.hob.feat;
-       if (r->iflags.hob.lbal)     r->hob.lbal  = tf.hob.lbal;
-       if (r->iflags.hob.nsect)    r->hob.nsect = tf.hob.nsect;
-       if (r->iflags.hob.lbam)     r->hob.lbam  = tf.hob.lbam;
-       if (r->iflags.hob.lbah)     r->hob.lbah  = tf.hob.lbah;
+       if (r->iflags.bits.lob.feat)        r->lob.feat  = tf.error;
+       if (r->iflags.bits.lob.lbal)        r->lob.lbal  = tf.lob.lbal;
+       if (r->iflags.bits.lob.nsect)   r->lob.nsect = tf.lob.nsect;
+       if (r->iflags.bits.lob.lbam)        r->lob.lbam  = tf.lob.lbam;
+       if (r->iflags.bits.lob.lbah)        r->lob.lbah  = tf.lob.lbah;
+       if (r->iflags.bits.lob.dev)     r->lob.dev   = tf.dev;
+       if (r->iflags.bits.lob.command) r->lob.command = tf.status;
+       if (r->iflags.bits.hob.feat)        r->hob.feat  = tf.hob.feat;
+       if (r->iflags.bits.hob.lbal)        r->hob.lbal  = tf.hob.lbal;
+       if (r->iflags.bits.hob.nsect)   r->hob.nsect = tf.hob.nsect;
+       if (r->iflags.bits.hob.lbam)        r->hob.lbam  = tf.hob.lbam;
+       if (r->iflags.bits.hob.lbah)        r->hob.lbah  = tf.hob.lbah;
    }
    return rc;

@@ -512,18 +512,18 @@
    if (verbose) {
        int err = errno;
        fprintf(stderr, "rc=%d, errno=%d, returned ATA registers: ", rc, err);
-       if (r->iflags.lob.feat)     fprintf(stderr, " er=%02x", r->lob.feat);
-       if (r->iflags.lob.nsect)    fprintf(stderr, " ns=%02x", r->lob.nsect);
-       if (r->iflags.lob.lbal)     fprintf(stderr, " ll=%02x", r->lob.lbal);
-       if (r->iflags.lob.lbam)     fprintf(stderr, " lm=%02x", r->lob.lbam);
-       if (r->iflags.lob.lbah)     fprintf(stderr, " lh=%02x", r->lob.lbah);
-       if (r->iflags.lob.dev)      fprintf(stderr, " dh=%02x", r->lob.dev);
-       if (r->iflags.lob.command)  fprintf(stderr, " st=%02x", r->lob.command);
-       if (r->iflags.hob.feat)     fprintf(stderr, " err=%02x", r->hob.feat);
-       if (r->iflags.hob.nsect)    fprintf(stderr, " err=%02x", r->hob.nsect);
-       if (r->iflags.hob.lbal)     fprintf(stderr, " err=%02x", r->hob.lbal);
-       if (r->iflags.hob.lbam)     fprintf(stderr, " err=%02x", r->hob.lbam);
-       if (r->iflags.hob.lbah)     fprintf(stderr, " err=%02x", r->hob.lbah);
+       if (r->iflags.bits.lob.feat)        fprintf(stderr, " er=%02x", r->lob.feat);
+       if (r->iflags.bits.lob.nsect)   fprintf(stderr, " ns=%02x", r->lob.nsect);
+       if (r->iflags.bits.lob.lbal)        fprintf(stderr, " ll=%02x", r->lob.lbal);
+       if (r->iflags.bits.lob.lbam)        fprintf(stderr, " lm=%02x", r->lob.lbam);
+       if (r->iflags.bits.lob.lbah)        fprintf(stderr, " lh=%02x", r->lob.lbah);
+       if (r->iflags.bits.lob.dev)     fprintf(stderr, " dh=%02x", r->lob.dev);
+       if (r->iflags.bits.lob.command) fprintf(stderr, " st=%02x", r->lob.command);
+       if (r->iflags.bits.hob.feat)        fprintf(stderr, " err=%02x", r->hob.feat);
+       if (r->iflags.bits.hob.nsect)   fprintf(stderr, " err=%02x", r->hob.nsect);
+       if (r->iflags.bits.hob.lbal)        fprintf(stderr, " err=%02x", r->hob.lbal);
+       if (r->iflags.bits.hob.lbam)        fprintf(stderr, " err=%02x", r->hob.lbam);
+       if (r->iflags.bits.hob.lbah)        fprintf(stderr, " err=%02x", r->hob.lbah);
        fprintf(stderr, "\n");
        errno = err;
    }
@@ -551,15 +551,15 @@
        r->ibytes  = data_bytes;
    }
    r->lob.command      = ata_op;
-   r->oflags.lob.command = 1;
-   r->oflags.lob.dev     = 1;
-   r->oflags.lob.lbal    = 1;
-   r->oflags.lob.lbam    = 1;
-   r->oflags.lob.lbah    = 1;
-   r->oflags.lob.nsect   = 1;
+   r->oflags.bits.lob.command = 1;
+   r->oflags.bits.lob.dev     = 1;
+   r->oflags.bits.lob.lbal    = 1;
+   r->oflags.bits.lob.lbam    = 1;
+   r->oflags.bits.lob.lbah    = 1;
+   r->oflags.bits.lob.nsect   = 1;

-   r->iflags.lob.command = 1;
-   r->iflags.lob.feat    = 1;
+   r->iflags.bits.lob.command = 1;
+   r->iflags.bits.lob.feat    = 1;

    r->lob.nsect = nsect;
    r->lob.lbal  = lba;
@@ -572,10 +572,10 @@
        r->hob.lbal  = lba   >> 24;
        r->hob.lbam  = lba   >> 32;
        r->hob.lbah  = lba   >> 40;
-       r->oflags.hob.nsect = 1;
-       r->oflags.hob.lbal  = 1;
-       r->oflags.hob.lbam  = 1;
-       r->oflags.hob.lbah  = 1;
+       r->oflags.bits.hob.nsect = 1;
+       r->oflags.bits.hob.lbal  = 1;
+       r->oflags.bits.hob.lbam  = 1;
+       r->oflags.bits.hob.lbah  = 1;
    } else {
        r->lob.dev |= (lba >> 24) & 0x0f;
    }
diff -w -b -u hdparm-9.43-orig/sgio.h hdparm-9.43/sgio.h
--- hdparm-9.43-orig/sgio.h 2014-08-25 15:07:08.461172023 -0700
+++ hdparm-9.43/sgio.h  2014-08-25 14:26:19.721131780 -0700
@@ -118,6 +118,7 @@

 union reg_flags {
    unsigned all                :16;
+    struct {
    union {
        unsigned lob_all        : 8;
        struct {
@@ -144,6 +145,7 @@
            unsigned command    : 1;
        } hob;
    };
+    } bits;
 } __attribute__((packed));

 struct taskfile_regs {
1 Attachments

Related

Bugs: #49

Discussion

  • Mark Lord
    Mark Lord
    2014-09-27

    This patch incorporated into hdparm-9.44.

     
  • Mark Lord
    Mark Lord
    2014-09-27

    • status: unread --> closed-fixed