2.6.21 fix lba48 bug in libata fill_result_tf()

PATCH Applied Mark Lord 3 participants 5 messages Apr 2, 2007 – Apr 4, 2007 libata

lore

Share & tools

lore

Mark Lord

author

2007-04-02 20:20 UTC #0 lore raw

PATCH 1/1 2.6.21 fix lba48 bug in libata fill_result_tf()

Tejun,

Current 2.6.21 libata does the following:

void ata_tf_read(struct ata_port *ap, struct ata_taskfile *tf)

{
       struct ata_ioports *ioaddr = &ap->ioaddr;

       tf->command = ata_check_status(ap);
	...
       if (tf->flags & ATA_TFLAG_LBA48) {
               iowrite8(tf->ctl | ATA_HOB, ioaddr->ctl_addr);
               tf->hob_feature = ioread8(ioaddr->error_addr);
               ...
       }
}
...
static void fill_result_tf(struct ata_queued_cmd *qc)
{
       struct ata_port *ap = qc->ap;

       ap->ops->tf_read(ap, &qc->result_tf);
       qc->result_tf.flags = qc->tf.flags;
}

Based on this, those last two statements fill_result_tf() appear to me to be in the wrong order, in that the tf->flags are uninitialized at the point where tf_read() is invoked. So for lba48 commands, tf_read() won't be reading back the full lba48 register contents..

Correct?

This patch corrects fill_result_tf() so that the flags get copied to result_tf before they are used by tf_read().

Signed-off-by Mark Lord <[email protected]>

---

drivers/ata/libata-core.c renamed from drivers/ata/libata-core.c.orig +0 -0

@@ -4763,8 +4763,8 @@

{
	struct ata_port *ap = qc->ap;

-	ap->ops->tf_read(ap, &qc->result_tf);
	qc->result_tf.flags = qc->tf.flags;

+ ap->ops->tf_read(ap, &qc->result_tf);
}

/**

Mark Lord

author

2007-04-02 21:44 UTC #1 lore raw

Quoted 11 lines · Mark Lord

This patch corrects fill_result_tf() so that the flags get copied to result_tf before they are used by tf_read().
Signed-off-by Mark Lord <[email protected]>
---
--- linux/drivers/ata/libata-core.c.orig 2007-03-21
12:46:06.000000000 -0400
+++ linux/drivers/ata/libata-core.c 2007-04-02 16:16:28.000000000 -0400

Review line 4770

Quoted 9 lines

(quoted diff) +1 -1

@@ -4763,8 +4763,8 @@
 {
    struct ata_port *ap = qc->ap;
 
-    ap->ops->tf_read(ap, &qc->result_tf);
    qc->result_tf.flags = qc->tf.flags;
+    ap->ops->tf_read(ap, &qc->result_tf);
 }
 
 /**

If I understand this correctly, the impact of (not patching) this is that error-handling for LBA48 accesses is broken. Right, Tejun?

Tejun Heo

Acked-by

2007-04-03 11:54 UTC #2 lore raw

Quoted 20 lines · Mark Lord

Quoted 19 lines · Mark Lord
This patch corrects fill_result_tf() so that the flags get copied to result_tf before they are used by tf_read().
Signed-off-by Mark Lord <[email protected]>
---
--- linux/drivers/ata/libata-core.c.orig 2007-03-21
12:46:06.000000000 -0400
+++ linux/drivers/ata/libata-core.c 2007-04-02 16:16:28.000000000
-0400
(quoted diff) +1 -1
@@ -4763,8 +4763,8 @@
 {
    struct ata_port *ap = qc->ap;
 
-    ap->ops->tf_read(ap, &qc->result_tf);
    qc->result_tf.flags = qc->tf.flags;
+    ap->ops->tf_read(ap, &qc->result_tf);
 }
 

Quoted 5 lines

Review line 4770
Quoted 2 lines
(quoted diff) +0 -0
 /**
If I understand this correctly, the impact of (not patching) this is that error-handling for LBA48 accesses is broken. Right, Tejun?

Yes, you're right. Thanks for fixing this.

Acked-by Tejun Heo <[email protected]>

Signature · 1 line

tejun

Mark Lord

author

2007-04-03 15:29 UTC #3 lore raw

Quoted 20 lines · Tejun Heo

Quoted 19 lines · Mark Lord
Quoted 18 lines · Mark Lord
This patch corrects fill_result_tf() so that the flags get copied to result_tf before they are used by tf_read().
Signed-off-by Mark Lord <[email protected]>
---
--- linux/drivers/ata/libata-core.c.orig 2007-03-21
12:46:06.000000000 -0400
+++ linux/drivers/ata/libata-core.c 2007-04-02 16:16:28.000000000
-0400
(quoted diff) +1 -1
@@ -4763,8 +4763,8 @@
 {
    struct ata_port *ap = qc->ap;
 
-    ap->ops->tf_read(ap, &qc->result_tf);
    qc->result_tf.flags = qc->tf.flags;
+    ap->ops->tf_read(ap, &qc->result_tf);
 }
 

Quoted 8 lines

Quoted 4 lines
Review line 4770
Quoted 2 lines
(quoted diff) +0 -0
 /**
If I understand this correctly, the impact of (not patching) this is that error-handling for LBA48 accesses is broken. Right, Tejun?
Yes, you're right. Thanks for fixing this.
Acked-by Tejun Heo <[email protected]>

This should really go into 2.6.21, as well as be backported to the 2.6.20.xx point releases.

Where's Jeff?

Jeff Garzik

Applied

2007-04-04 06:08 UTC #4 lore raw

In reply to Mark Lord

Quoted 31 lines · Mark Lord

Tejun,
Current 2.6.21 libata does the following:
void ata_tf_read(struct ata_port *ap, struct ata_taskfile *tf)
{
       struct ata_ioports *ioaddr = &ap->ioaddr;

       tf->command = ata_check_status(ap);
    ...
       if (tf->flags & ATA_TFLAG_LBA48) {
               iowrite8(tf->ctl | ATA_HOB, ioaddr->ctl_addr);
               tf->hob_feature = ioread8(ioaddr->error_addr);
               ...
       }
}
...
static void fill_result_tf(struct ata_queued_cmd *qc)
{
       struct ata_port *ap = qc->ap;

       ap->ops->tf_read(ap, &qc->result_tf);
       qc->result_tf.flags = qc->tf.flags;
}
Based on this, those last two statements fill_result_tf() appear to me to be in the wrong order, in that the tf->flags are uninitialized at the point where tf_read() is invoked. So for lba48 commands, tf_read() won't be reading back the full lba48 register contents..

Quoted 6 lines

Correct?
This patch corrects fill_result_tf() so that the flags get copied to result_tf before they are used by tf_read().
Signed-off-by Mark Lord <[email protected]>

applied