Am 07.11.2023 um 19:14 hat Kevin Wolf geschrieben:
> Am 05.10.2023 um 12:04 hat Niklas Cassel geschrieben:
> > From: Niklas Cassel <niklas.cas...@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.juszkiew...@linaro.org>
> > Signed-off-by: Niklas Cassel <niklas.cas...@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?
The other thing I wondered and forgot to actually write is if we should
extend ahci-test to include port and software resets.
Kevin
