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

login
register
mail settings
Submitter Tejun Heo
Date Oct. 14, 2010, 8:53 a.m.
Message ID <4CB6C50F.60609@kernel.org>
Download mbox | patch
Permalink /patch/67797/
State Not Applicable
Delegated to: David Miller
Headers show

Comments

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

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;
 }