Re: [PATCH v2] hw/ide/ahci: fix legacy software reset

[Kevin Wolf <kwolf@redhat.com>]

Am 05.10.2023 um 12:04 hat Niklas Cassel geschrieben:
> From: Niklas Cassel <niklas.cassel@wdc.com>
> 
> Legacy software contains a standard mechanism for generating a reset to a
> Serial ATA device - setting the SRST (software reset) bit in the Device
> Control register.
> 
> Serial ATA has a more robust mechanism called COMRESET, also referred to
> as port reset. A port reset is the preferred mechanism for error
> recovery and should be used in place of software reset.
> 
> Commit e2a5d9b3d9c3 ("hw/ide/ahci: simplify and document PxCI handling")
> improved the handling of PxCI, such that PxCI gets cleared after handling
> a non-NCQ, or NCQ command (instead of incorrectly clearing PxCI after
> receiving an arbitrary FIS).
> 
> However, simply clearing PxCI after a non-NCQ, or NCQ command, is not
> enough, we also need to clear PxCI when receiving a SRST in the Device
> Control register.
> 
> This fixes an issue for FreeBSD where the device would fail to reset.
> The problem was not noticed in Linux, because Linux uses a COMRESET
> instead of a legacy software reset by default.
> 
> Fixes: e2a5d9b3d9c3 ("hw/ide/ahci: simplify and document PxCI handling")
> Reported-by: Marcin Juszkiewicz <marcin.juszkiewicz@linaro.org>
> Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
> ---
> Changes since v1: write the D2H FIS before clearing PxCI.
> 
>  hw/ide/ahci.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
> index babdd7b458..7269dabbdb 100644
> --- a/hw/ide/ahci.c
> +++ b/hw/ide/ahci.c
> @@ -1254,10 +1254,26 @@ static void handle_reg_h2d_fis(AHCIState *s, int port,
>          case STATE_RUN:
>              if (cmd_fis[15] & ATA_SRST) {
>                  s->dev[port].port_state = STATE_RESET;
> +                /*
> +                 * When setting SRST in the first H2D FIS in the reset sequence,
> +                 * the device does not send a D2H FIS. Host software thus has to
> +                 * set the "Clear Busy upon R_OK" bit such that PxCI (and BUSY)
> +                 * gets cleared. See AHCI 1.3.1, section 10.4.1 Software Reset.
> +                 */
> +                if (opts & AHCI_CMD_CLR_BUSY) {
> +                    ahci_clear_cmd_issue(ad, slot);
> +                }

I suspect that AHCI_CMD_CLR_BUSY really should be checked in a more
generic place, but this will do for fixing software reset.

>              }
>              break;
>          case STATE_RESET:
>              if (!(cmd_fis[15] & ATA_SRST)) {
> +                /*
> +                 * When clearing SRST in the second H2D FIS in the reset
> +                 * sequence, the device will send a D2H FIS. See SATA 3.5a Gold,
> +                 * section 11.4 Software reset protocol.
> +                 */
> +                ahci_clear_cmd_issue(ad, slot);
> +                ahci_write_fis_d2h(ad, false);
>                  ahci_reset_port(s, port);

This part isn't mentioned in the commit message at all, and I don't see
how it's related to commit e2a5d9b3d9c3 either. Is this supposed to be a
bonus fix?

ahci_reset_port() already calls ahci_init_d2h() -> ahci_write_fis_d2h().
So I think this new ahci_write_fis_d2h() only sets some state that will
immediately be overwritten again. Which is good, because we didn't set
the signature as described in the SATA software reset protocol yet, that
is only done in ahci_reset_port().

Am I missing something? Why do we need this ahci_write_fis_d2h() call
here?

As for the ahci_clear_cmd_issue(), I'm surprised that ahci_reset_port()
doesn't already clear the register. Wouldn't it make more sense there
than just in this one caller?

Kevin