[#upstream-fixes] libahci: fix result_tf handling after an ATA PIO data-in command

Submitted by Tejun Heo on Oct. 14, 2010, 8:53 a.m.

Details

Message ID 4CB6C50F.60609@kernel.org
State Not Applicable
Delegated to: David Miller
Headers show

Commit Message

Tejun Heo Oct. 14, 2010, 8:53 a.m.
ATA devices don't send D2H Reg FIS after an successful ATA PIO data-in
command.  The host is supposed to take the E_Status of the preceding
PIO Setup FIS.  Update ahci_qc_fill_rtf() such that it takes E_Status
after a successful ATA PIO data-in command.

Without this patch, result_tf for such a command is filled with the
content of the previous D2H Reg FIS which belongs to a previous
command, which can make the command incorrectly seen as failed.

Signed-off-by: Tejun Heo <tj@kernel.org>
Reported-by: Mark Lord <kernel@teksavvy.com>
Cc: Robert Hancock <hancockrwd@gmail.com>
Cc: stable@kernel.org
---
 drivers/ata/ahci.h    |    1 +
 drivers/ata/libahci.c |   17 ++++++++++++++---
 2 files changed, 15 insertions(+), 3 deletions(-)

--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Mark Lord Oct. 14, 2010, 9:38 p.m.
On 10-10-14 04:53 AM, Tejun Heo wrote:
> ATA devices don't send D2H Reg FIS after an successful ATA PIO data-in
> command.  The host is supposed to take the E_Status of the preceding
> PIO Setup FIS.  Update ahci_qc_fill_rtf() such that it takes E_Status
> after a successful ATA PIO data-in command.
>
> Without this patch, result_tf for such a command is filled with the
> content of the previous D2H Reg FIS which belongs to a previous
> command, which can make the command incorrectly seen as failed.
>
> Signed-off-by: Tejun Heo<tj@kernel.org>
> Reported-by: Mark Lord<kernel@teksavvy.com>
> Cc: Robert Hancock<hancockrwd@gmail.com>
> Cc: stable@kernel.org
> ---
>   drivers/ata/ahci.h    |    1 +
>   drivers/ata/libahci.c |   17 ++++++++++++++---
>   2 files changed, 15 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
> index e5fdeeb..d1a0f5b 100644
> --- a/drivers/ata/ahci.h
> +++ b/drivers/ata/ahci.h
> @@ -72,6 +72,7 @@ enum {
>   	AHCI_CMD_RESET		= (1<<  8),
>   	AHCI_CMD_CLR_BUSY	= (1<<  10),
>
> +	RX_FIS_PIO_SETUP	= 0x20,	/* offset of PIO Setup FIS data */
>   	RX_FIS_D2H_REG		= 0x40,	/* offset of D2H Register FIS data */
>   	RX_FIS_SDB		= 0x58, /* offset of SDB FIS data */
>   	RX_FIS_UNK		= 0x60, /* offset of Unknown FIS data */
> diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
> index 8eea309..0c482a0 100644
> --- a/drivers/ata/libahci.c
> +++ b/drivers/ata/libahci.c
> @@ -1830,12 +1830,23 @@ static unsigned int ahci_qc_issue(struct ata_queued_cmd *qc)
>   static bool ahci_qc_fill_rtf(struct ata_queued_cmd *qc)
>   {
>   	struct ahci_port_priv *pp = qc->ap->private_data;
> -	u8 *d2h_fis = pp->rx_fis + RX_FIS_D2H_REG;
> +	u8 *rx_fis = pp->rx_fis;
>
>   	if (pp->fbs_enabled)
> -		d2h_fis += qc->dev->link->pmp * AHCI_RX_FIS_SZ;
> +		rx_fis += qc->dev->link->pmp * AHCI_RX_FIS_SZ;
> +
> +	/*
> +	 * After a successful execution of an ATA PIO data-in command,
> +	 * the device doesn't send D2H Reg FIS to update the TF and
> +	 * the host should take Status from E_Status of the preceding
> +	 * PIO Setup FIS.
> +	 */
> +	if (qc->tf.protocol == ATA_PROT_PIO&&  qc->dma_dir == DMA_FROM_DEVICE&&
> +	    !(qc->flags&  ATA_QCFLAG_FAILED))
..

I'm not certain, but doesn't this problem also affect DMA_TO_DEVICE PIO commands?
Eg. SECURITY_ERASE_PREPARE and PIO_WRITE ?

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Robert Hancock Oct. 15, 2010, 12:24 a.m.
On Thu, Oct 14, 2010 at 3:38 PM, Mark Lord <kernel@teksavvy.com> wrote:
> On 10-10-14 04:53 AM, Tejun Heo wrote:
>>
>> ATA devices don't send D2H Reg FIS after an successful ATA PIO data-in
>> command.  The host is supposed to take the E_Status of the preceding
>> PIO Setup FIS.  Update ahci_qc_fill_rtf() such that it takes E_Status
>> after a successful ATA PIO data-in command.
>>
>> Without this patch, result_tf for such a command is filled with the
>> content of the previous D2H Reg FIS which belongs to a previous
>> command, which can make the command incorrectly seen as failed.
>>
>> Signed-off-by: Tejun Heo<tj@kernel.org>
>> Reported-by: Mark Lord<kernel@teksavvy.com>
>> Cc: Robert Hancock<hancockrwd@gmail.com>
>> Cc: stable@kernel.org
>> ---
>>  drivers/ata/ahci.h    |    1 +
>>  drivers/ata/libahci.c |   17 ++++++++++++++---
>>  2 files changed, 15 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
>> index e5fdeeb..d1a0f5b 100644
>> --- a/drivers/ata/ahci.h
>> +++ b/drivers/ata/ahci.h
>> @@ -72,6 +72,7 @@ enum {
>>        AHCI_CMD_RESET          = (1<<  8),
>>        AHCI_CMD_CLR_BUSY       = (1<<  10),
>>
>> +       RX_FIS_PIO_SETUP        = 0x20, /* offset of PIO Setup FIS data */
>>        RX_FIS_D2H_REG          = 0x40, /* offset of D2H Register FIS data
>> */
>>        RX_FIS_SDB              = 0x58, /* offset of SDB FIS data */
>>        RX_FIS_UNK              = 0x60, /* offset of Unknown FIS data */
>> diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
>> index 8eea309..0c482a0 100644
>> --- a/drivers/ata/libahci.c
>> +++ b/drivers/ata/libahci.c
>> @@ -1830,12 +1830,23 @@ static unsigned int ahci_qc_issue(struct
>> ata_queued_cmd *qc)
>>  static bool ahci_qc_fill_rtf(struct ata_queued_cmd *qc)
>>  {
>>        struct ahci_port_priv *pp = qc->ap->private_data;
>> -       u8 *d2h_fis = pp->rx_fis + RX_FIS_D2H_REG;
>> +       u8 *rx_fis = pp->rx_fis;
>>
>>        if (pp->fbs_enabled)
>> -               d2h_fis += qc->dev->link->pmp * AHCI_RX_FIS_SZ;
>> +               rx_fis += qc->dev->link->pmp * AHCI_RX_FIS_SZ;
>> +
>> +       /*
>> +        * After a successful execution of an ATA PIO data-in command,
>> +        * the device doesn't send D2H Reg FIS to update the TF and
>> +        * the host should take Status from E_Status of the preceding
>> +        * PIO Setup FIS.
>> +        */
>> +       if (qc->tf.protocol == ATA_PROT_PIO&&  qc->dma_dir ==
>> DMA_FROM_DEVICE&&
>> +           !(qc->flags&  ATA_QCFLAG_FAILED))
>
> ..
>
> I'm not certain, but doesn't this problem also affect DMA_TO_DEVICE PIO
> commands?
> Eg. SECURITY_ERASE_PREPARE and PIO_WRITE ?

No, PIO data-out commands send a D2H register FIS after completion. It
appears that PIO data-in is the only special case.
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Robert Hancock Oct. 15, 2010, 12:32 a.m.
On Thu, Oct 14, 2010 at 2:53 AM, Tejun Heo <tj@kernel.org> wrote:
> ATA devices don't send D2H Reg FIS after an successful ATA PIO data-in
> command.  The host is supposed to take the E_Status of the preceding
> PIO Setup FIS.  Update ahci_qc_fill_rtf() such that it takes E_Status
> after a successful ATA PIO data-in command.
>
> Without this patch, result_tf for such a command is filled with the
> content of the previous D2H Reg FIS which belongs to a previous
> command, which can make the command incorrectly seen as failed.
>
> Signed-off-by: Tejun Heo <tj@kernel.org>
> Reported-by: Mark Lord <kernel@teksavvy.com>
> Cc: Robert Hancock <hancockrwd@gmail.com>
> Cc: stable@kernel.org
> ---
>  drivers/ata/ahci.h    |    1 +
>  drivers/ata/libahci.c |   17 ++++++++++++++---
>  2 files changed, 15 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
> index e5fdeeb..d1a0f5b 100644
> --- a/drivers/ata/ahci.h
> +++ b/drivers/ata/ahci.h
> @@ -72,6 +72,7 @@ enum {
>        AHCI_CMD_RESET          = (1 << 8),
>        AHCI_CMD_CLR_BUSY       = (1 << 10),
>
> +       RX_FIS_PIO_SETUP        = 0x20, /* offset of PIO Setup FIS data */
>        RX_FIS_D2H_REG          = 0x40, /* offset of D2H Register FIS data */
>        RX_FIS_SDB              = 0x58, /* offset of SDB FIS data */
>        RX_FIS_UNK              = 0x60, /* offset of Unknown FIS data */
> diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
> index 8eea309..0c482a0 100644
> --- a/drivers/ata/libahci.c
> +++ b/drivers/ata/libahci.c
> @@ -1830,12 +1830,23 @@ static unsigned int ahci_qc_issue(struct ata_queued_cmd *qc)
>  static bool ahci_qc_fill_rtf(struct ata_queued_cmd *qc)
>  {
>        struct ahci_port_priv *pp = qc->ap->private_data;
> -       u8 *d2h_fis = pp->rx_fis + RX_FIS_D2H_REG;
> +       u8 *rx_fis = pp->rx_fis;
>
>        if (pp->fbs_enabled)
> -               d2h_fis += qc->dev->link->pmp * AHCI_RX_FIS_SZ;
> +               rx_fis += qc->dev->link->pmp * AHCI_RX_FIS_SZ;
> +
> +       /*
> +        * After a successful execution of an ATA PIO data-in command,
> +        * the device doesn't send D2H Reg FIS to update the TF and
> +        * the host should take Status from E_Status of the preceding
> +        * PIO Setup FIS.
> +        */
> +       if (qc->tf.protocol == ATA_PROT_PIO && qc->dma_dir == DMA_FROM_DEVICE &&
> +           !(qc->flags & ATA_QCFLAG_FAILED))
> +               qc->result_tf.command = (rx_fis + RX_FIS_PIO_SETUP)[15];

Shouldn't we be reading the entire TF from the PIO setup FIS and then
just replacing the command register value with the E_Status field? It
looks like that's the only field in the TF that's being set here.

> +       else
> +               ata_tf_from_fis(rx_fis + RX_FIS_D2H_REG, &qc->result_tf);
>
> -       ata_tf_from_fis(d2h_fis, &qc->result_tf);
>        return true;
>  }
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tejun Heo Oct. 15, 2010, 8:45 a.m.
Hello,

On 10/15/2010 02:32 AM, Robert Hancock wrote:
>> +       if (qc->tf.protocol == ATA_PROT_PIO && qc->dma_dir == DMA_FROM_DEVICE &&
>> +           !(qc->flags & ATA_QCFLAG_FAILED))
>> +               qc->result_tf.command = (rx_fis + RX_FIS_PIO_SETUP)[15];
> 
> Shouldn't we be reading the entire TF from the PIO setup FIS and then
> just replacing the command register value with the E_Status field? It
> looks like that's the only field in the TF that's being set here.

Hmmm, yeah, probably that would be better.  I'll post the updated
version soon.

Thanks.
Tejun Heo Oct. 15, 2010, 8:45 a.m.
On 10/15/2010 02:24 AM, Robert Hancock wrote:
>> I'm not certain, but doesn't this problem also affect DMA_TO_DEVICE PIO
>> commands?
>> Eg. SECURITY_ERASE_PREPARE and PIO_WRITE ?
> 
> No, PIO data-out commands send a D2H register FIS after completion. It
> appears that PIO data-in is the only special case.

Yeap, it's an exception only for PIO data-in's.

Thanks.
Mark Lord Oct. 15, 2010, 10:14 p.m.
On 10-10-15 04:45 AM, Tejun Heo wrote:
> On 10/15/2010 02:24 AM, Robert Hancock wrote:
>>> I'm not certain, but doesn't this problem also affect DMA_TO_DEVICE PIO
>>> commands?
>>> Eg. SECURITY_ERASE_PREPARE and PIO_WRITE ?
>>
>> No, PIO data-out commands send a D2H register FIS after completion. It
>> appears that PIO data-in is the only special case.
>
> Yeap, it's an exception only for PIO data-in's.


Peachy.  Thanks for working on this.
I'm away from my setup right now, but will test as soon as I get back.

In the meanwhile, feel free to nag if you don't hear back by Mon/Tues.

Cheers
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch hide | download patch | download mbox

diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
index e5fdeeb..d1a0f5b 100644
--- a/drivers/ata/ahci.h
+++ b/drivers/ata/ahci.h
@@ -72,6 +72,7 @@  enum {
 	AHCI_CMD_RESET		= (1 << 8),
 	AHCI_CMD_CLR_BUSY	= (1 << 10),

+	RX_FIS_PIO_SETUP	= 0x20,	/* offset of PIO Setup FIS data */
 	RX_FIS_D2H_REG		= 0x40,	/* offset of D2H Register FIS data */
 	RX_FIS_SDB		= 0x58, /* offset of SDB FIS data */
 	RX_FIS_UNK		= 0x60, /* offset of Unknown FIS data */
diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
index 8eea309..0c482a0 100644
--- a/drivers/ata/libahci.c
+++ b/drivers/ata/libahci.c
@@ -1830,12 +1830,23 @@  static unsigned int ahci_qc_issue(struct ata_queued_cmd *qc)
 static bool ahci_qc_fill_rtf(struct ata_queued_cmd *qc)
 {
 	struct ahci_port_priv *pp = qc->ap->private_data;
-	u8 *d2h_fis = pp->rx_fis + RX_FIS_D2H_REG;
+	u8 *rx_fis = pp->rx_fis;

 	if (pp->fbs_enabled)
-		d2h_fis += qc->dev->link->pmp * AHCI_RX_FIS_SZ;
+		rx_fis += qc->dev->link->pmp * AHCI_RX_FIS_SZ;
+
+	/*
+	 * After a successful execution of an ATA PIO data-in command,
+	 * the device doesn't send D2H Reg FIS to update the TF and
+	 * the host should take Status from E_Status of the preceding
+	 * PIO Setup FIS.
+	 */
+	if (qc->tf.protocol == ATA_PROT_PIO && qc->dma_dir == DMA_FROM_DEVICE &&
+	    !(qc->flags & ATA_QCFLAG_FAILED))
+		qc->result_tf.command = (rx_fis + RX_FIS_PIO_SETUP)[15];
+	else
+		ata_tf_from_fis(rx_fis + RX_FIS_D2H_REG, &qc->result_tf);

-	ata_tf_from_fis(d2h_fis, &qc->result_tf);
 	return true;
 }