[PATCH] 2.6.21 fix lba48 bug in libata fill_result_tf()

[PATCH] 2.6.21 fix lba48 bug in libata fill_result_tf()

[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..

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 <mlord@pobox.com>
---
--- 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
@@ -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);
 }
 
 /**

Re: [PATCH] 2.6.21 fix lba48 bug in libata fill_result_tf()

[Mark Lord]

Mark Lord wrote:
>
> 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 <mlord@pobox.com>
> ---
> --- 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
> @@ -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?

Re: [PATCH] 2.6.21 fix lba48 bug in libata fill_result_tf()

[Tejun Heo]

Mark Lord wrote:
> Mark Lord wrote:
>>
>> 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 <mlord@pobox.com>
>> ---
>> --- 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
>> @@ -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?

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

Acked-by: Tejun Heo <htejun@gmail.com>

-- 
tejun

Re: [PATCH] 2.6.21 fix lba48 bug in libata fill_result_tf()

[Mark Lord]

Tejun Heo wrote:
> Mark Lord wrote:
>> Mark Lord wrote:
>>> 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 <mlord@pobox.com>
>>> ---
>>> --- 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
>>> @@ -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?
> 
> Yes, you're right.  Thanks for fixing this.
> 
> Acked-by: Tejun Heo <htejun@gmail.com>

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

Where's Jeff?

Re: [PATCH] 2.6.21 fix lba48 bug in libata fill_result_tf()

[Jeff Garzik]

Mark Lord wrote:
> 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 <mlord@pobox.com>

applied