On 10/4/22 04:46, Niklas Cassel wrote:
> Currently, for PIO commands, ahci_qc_fill_rtf() reads the status and
> error fields from the PIO Setup FIS area of the Received FIS Structure.
>
> For any non-PIO command, ahci_qc_fill_rtf() currently reads the status
> and error fields from the D2H Register FIS area of the Received FIS
> Structure. This is simply not correct.
>
> According to the SATA 3.5a specification:
> 11.10 DMA DATA-IN command protocol and 11.11 DMA DATA-OUT command
> protocol: READ DMA and WRITE DMA (non-NCQ commands) will end with
> the Send_status state, which transmits a Register D2H FIS.
>
> Likewise, in:
> 11.15 FPDMA QUEUED command protocol: READ FPDMA QUEUED and WRITE FPDMA
> QUEUED (NCQ commands) will end with the SendStatus state, which
> transmits a Set Device Bits FIS.
>
> So, for NCQ commands, there is never a D2H Register FIS sent.
> Reading the status and error fields from the D2H Register FIS area
> for a NCQ command, will result in us returning the status and error
> values for the last non-NCQ command, which is incorrect.
>
> Update ahci_qc_fill_rtf() to read the status and error fields from
> the correct area in the Received FIS Structure for NCQ commands.
>
> Once reason why this has not been detected before, could be because,
> in case of an NCQ error, ata_eh_analyze_ncq_error() will overwrite
> the (incorrect) status and error values set by ahci_qc_fill_rtf().
>
> However, even successful NCQ commands can have bits set in the status
> field (e.g. the sense data available bit), so it is mandatory to read
> the status from the correct area also for NCQ commands.
>
> Signed-off-by: Niklas Cassel <niklas.cassel@xxxxxxx>
Given that this does not seem to create issues currently, I applied this
one to for-6.2 on top of your autosense series. This will get more testing
in linux-next this way.
Thanks !
> ---
> drivers/ata/libahci.c | 14 ++++++++++++++
> 1 file changed, 14 insertions(+)
>
> diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
> index cf8c7fd59ada..d30c1a4c11fe 100644
> --- a/drivers/ata/libahci.c
> +++ b/drivers/ata/libahci.c
> @@ -2034,6 +2034,20 @@ static bool ahci_qc_fill_rtf(struct ata_queued_cmd *qc)
> !(qc->flags & ATA_QCFLAG_FAILED)) {
> ata_tf_from_fis(rx_fis + RX_FIS_PIO_SETUP, &qc->result_tf);
> qc->result_tf.status = (rx_fis + RX_FIS_PIO_SETUP)[15];
> +
> + /*
> + * For NCQ commands, we never get a D2H FIS, so reading the D2H Register
> + * FIS area of the Received FIS Structure (which contains a copy of the
> + * last D2H FIS received) will contain an outdated status code.
> + * For NCQ commands, we instead get a SDB FIS, so read the SDB FIS area
> + * instead. However, the SDB FIS does not contain the LBA, so we can't
> + * use the ata_tf_from_fis() helper.
> + */
> + } else if (ata_is_ncq(qc->tf.protocol)) {
> + const u8 *fis = rx_fis + RX_FIS_SDB;
> +
> + qc->result_tf.status = fis[2];
> + qc->result_tf.error = fis[3];
> } else
> ata_tf_from_fis(rx_fis + RX_FIS_D2H_REG, &qc->result_tf);
>
--
Damien Le Moal
Western Digital Research
