ahci: handle TFES irq correctly

[SeaBIOS] [PATCH] ahci: handle TFES irq correctly

Posted by Niklas Cassel via SeaBIOS 1 year, 4 months ago

According to AHCI 1.3.1, 5.3.8.1 RegFIS:Entry, if ERR_STAT is set in the
received FIS, the HBA shall jump to state ERR:FatalTaskfile, which will
raise a TFES IRQ.

This means that if ERR_STAT is set in the recevied FIS, PxIS.TFES will
be set, without either PxIS.DHRS or PxIS.PSS being set.

SeaBIOS function ahci_port_setup() will try to identify an AHCI device
by sending an ATAPI identify device command. However, such a command
will be aborted with ERR_STAT set for a regular (non-ATAPI) device.

ahci_command() already performs the correct error recovery steps when
status is correctly set, so simply modify ahci_command() to read the
correct status when PxIS.TFES is set.

It is safe to read PxTFD when PxIS.TFES is set, even for systems with a
port multiplier, see AHCI 1.3.1, 9.3.7 PxTFD Register Information:
"When a taskfile error occurs (PxIS.TFES is set to '1'), the host may
refer to the values in PxTFD. The values in PxTFD at this time are
guaranteed to correspond to the device that reported the taskfile error
condition."

Without this, each boot will be delayed by 32 seconds, waiting for the
AHCI command to timeout.

Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
---
 src/hw/ahci.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/src/hw/ahci.c b/src/hw/ahci.c
index d45b430..3fa845a 100644
--- a/src/hw/ahci.c
+++ b/src/hw/ahci.c
@@ -138,6 +138,12 @@ static int ahci_command(struct ahci_port_s *port_gf, int iswrite, int isatapi,
             intbits = ahci_port_readl(ctrl, pnr, PORT_IRQ_STAT);
             if (intbits) {
                 ahci_port_writel(ctrl, pnr, PORT_IRQ_STAT, intbits);
+                if (intbits & 0x40000000) {
+                    u32 tf = ahci_port_readl(ctrl, pnr, PORT_TFDATA);
+                    status = tf & 0xff;
+                    error = (tf & 0xff00) >> 8;
+                    break;
+                }
                 if (intbits & 0x02) {
                     status = GET_LOWFLAT(fis->psfis[2]);
                     error  = GET_LOWFLAT(fis->psfis[3]);
-- 
2.40.1

_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org

[SeaBIOS] Re: [PATCH] ahci: handle TFES irq correctly

Posted by Kevin O'Connor 1 year, 3 months ago

On Tue, May 30, 2023 at 03:44:05PM +0200, Niklas Cassel via SeaBIOS wrote:
> According to AHCI 1.3.1, 5.3.8.1 RegFIS:Entry, if ERR_STAT is set in the
> received FIS, the HBA shall jump to state ERR:FatalTaskfile, which will
> raise a TFES IRQ.
> 
> This means that if ERR_STAT is set in the recevied FIS, PxIS.TFES will
> be set, without either PxIS.DHRS or PxIS.PSS being set.
> 
> SeaBIOS function ahci_port_setup() will try to identify an AHCI device
> by sending an ATAPI identify device command. However, such a command
> will be aborted with ERR_STAT set for a regular (non-ATAPI) device.
> 
> ahci_command() already performs the correct error recovery steps when
> status is correctly set, so simply modify ahci_command() to read the
> correct status when PxIS.TFES is set.
> 
> It is safe to read PxTFD when PxIS.TFES is set, even for systems with a
> port multiplier, see AHCI 1.3.1, 9.3.7 PxTFD Register Information:
> "When a taskfile error occurs (PxIS.TFES is set to '1'), the host may
> refer to the values in PxTFD. The values in PxTFD at this time are
> guaranteed to correspond to the device that reported the taskfile error
> condition."
> 
> Without this, each boot will be delayed by 32 seconds, waiting for the
> AHCI command to timeout.

Thanks.  I committed this change.

-Kevin
_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org

[SeaBIOS] From header field rewrite used as git author (was: Re: [PATCH] ahci: handle TFES irq correctly)

Posted by Paul Menzel 1 year, 3 months ago

Dear Kevin, dear Niklas,


Am 22.06.23 um 04:03 schrieb Kevin O'Connor:
> On Tue, May 30, 2023 at 03:44:05PM +0200, Niklas Cassel via SeaBIOS wrote:

[…]

> Thanks.  I committed this change.

Just a note, Mailman’s rewrite of the `From` header field resulted in 
the author information below in the SeaBIOS git archive [1][2]:

 > Niklas Cassel via SeaBIOS <seabios@seabios.org>

No idea, how to solve this.


Kind regards,

Paul


[1]: https://review.coreboot.org/plugins/gitiles/seabios/
[2]: 
https://review.coreboot.org/plugins/gitiles/seabios/+/1281e340ad1d90c0cc8e8d902bb34f1871eb48cf
_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org

[SeaBIOS] Re: From header field rewrite used as git author (was: Re: [PATCH] ahci: handle TFES irq correctly)

Posted by Philippe Mathieu-Daudé 1 year, 3 months ago

On 3/7/23 12:51, Paul Menzel wrote:

> Just a note, Mailman’s rewrite of the `From` header field resulted in 
> the author information below in the SeaBIOS git archive [1][2]:
> 
>  > Niklas Cassel via SeaBIOS <seabios@seabios.org>
> 
> No idea, how to solve this.

Thanks for reporting. Candidate patch using git .mailmap
(https://git-scm.com/docs/gitmailmap):

https://mail.coreboot.org/hyperkitty/list/seabios@seabios.org/thread/6QUBPHALQ6NURG2553FZJPDUO4I3SRGZ/

It would be better if some pre-apply script checks for this
locally or during merge.

Regards,

Phil.
_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org

[SeaBIOS] Re: [PATCH] ahci: handle TFES irq correctly

Posted by Kevin O'Connor 1 year, 3 months ago

On Tue, May 30, 2023 at 03:44:05PM +0200, Niklas Cassel via SeaBIOS wrote:
> According to AHCI 1.3.1, 5.3.8.1 RegFIS:Entry, if ERR_STAT is set in the
> received FIS, the HBA shall jump to state ERR:FatalTaskfile, which will
> raise a TFES IRQ.
> 
> This means that if ERR_STAT is set in the recevied FIS, PxIS.TFES will
> be set, without either PxIS.DHRS or PxIS.PSS being set.
> 
> SeaBIOS function ahci_port_setup() will try to identify an AHCI device
> by sending an ATAPI identify device command. However, such a command
> will be aborted with ERR_STAT set for a regular (non-ATAPI) device.
> 
> ahci_command() already performs the correct error recovery steps when
> status is correctly set, so simply modify ahci_command() to read the
> correct status when PxIS.TFES is set.
> 
> It is safe to read PxTFD when PxIS.TFES is set, even for systems with a
> port multiplier, see AHCI 1.3.1, 9.3.7 PxTFD Register Information:
> "When a taskfile error occurs (PxIS.TFES is set to '1'), the host may
> refer to the values in PxTFD. The values in PxTFD at this time are
> guaranteed to correspond to the device that reported the taskfile error
> condition."
> 
> Without this, each boot will be delayed by 32 seconds, waiting for the
> AHCI command to timeout.

Gerd, would you be able to review this patch?

Thanks.
-Kevin

> 
> Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
> ---
>  src/hw/ahci.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/src/hw/ahci.c b/src/hw/ahci.c
> index d45b430..3fa845a 100644
> --- a/src/hw/ahci.c
> +++ b/src/hw/ahci.c
> @@ -138,6 +138,12 @@ static int ahci_command(struct ahci_port_s *port_gf, int iswrite, int isatapi,
>              intbits = ahci_port_readl(ctrl, pnr, PORT_IRQ_STAT);
>              if (intbits) {
>                  ahci_port_writel(ctrl, pnr, PORT_IRQ_STAT, intbits);
> +                if (intbits & 0x40000000) {
> +                    u32 tf = ahci_port_readl(ctrl, pnr, PORT_TFDATA);
> +                    status = tf & 0xff;
> +                    error = (tf & 0xff00) >> 8;
> +                    break;
> +                }
>                  if (intbits & 0x02) {
>                      status = GET_LOWFLAT(fis->psfis[2]);
>                      error  = GET_LOWFLAT(fis->psfis[3]);
> -- 
> 2.40.1
> 
> _______________________________________________
> SeaBIOS mailing list -- seabios@seabios.org
> To unsubscribe send an email to seabios-leave@seabios.org
_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org

[SeaBIOS] Re: [PATCH] ahci: handle TFES irq correctly

Posted by Gerd Hoffmann 1 year, 3 months ago

On Tue, Jun 13, 2023 at 11:13:29AM -0400, Kevin O'Connor wrote:
> On Tue, May 30, 2023 at 03:44:05PM +0200, Niklas Cassel via SeaBIOS wrote:
> > According to AHCI 1.3.1, 5.3.8.1 RegFIS:Entry, if ERR_STAT is set in the
> > received FIS, the HBA shall jump to state ERR:FatalTaskfile, which will
> > raise a TFES IRQ.
> > 
> > This means that if ERR_STAT is set in the recevied FIS, PxIS.TFES will
> > be set, without either PxIS.DHRS or PxIS.PSS being set.
> > 
> > SeaBIOS function ahci_port_setup() will try to identify an AHCI device
> > by sending an ATAPI identify device command. However, such a command
> > will be aborted with ERR_STAT set for a regular (non-ATAPI) device.
> > 
> > ahci_command() already performs the correct error recovery steps when
> > status is correctly set, so simply modify ahci_command() to read the
> > correct status when PxIS.TFES is set.
> > 
> > It is safe to read PxTFD when PxIS.TFES is set, even for systems with a
> > port multiplier, see AHCI 1.3.1, 9.3.7 PxTFD Register Information:
> > "When a taskfile error occurs (PxIS.TFES is set to '1'), the host may
> > refer to the values in PxTFD. The values in PxTFD at this time are
> > guaranteed to correspond to the device that reported the taskfile error
> > condition."
> > 
> > Without this, each boot will be delayed by 32 seconds, waiting for the
> > AHCI command to timeout.
> 
> Gerd, would you be able to review this patch?

Description and patch look good to me, testing with qemu works fine.

Tested-by: Gerd Hoffmann <kraxel@redhat.com>
Acked-by: Gerd Hoffmann <kraxel@redhat.com>

take care,
  Gerd

_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org