[PATCH 0/3] ide: fix SRST (again)

[PATCH 0/3] ide: fix SRST (again)

[John Snow]

My last fix for SRST wasn't entirely correct and caused a regression;
namely we need to mime execution of the diagnostic command. If we don't,
Linux bins the IDE device as 'horked'.

For detailed reading on the SRST protocol, I recommend checking out the
ATA8-APT specification.

John Snow (3):
  ide: run diagnostic after SRST
  ide: perform SRST as early as possible
  ide: clear SRST after SRST finishes

 hw/ide/core.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

-- 
2.26.2

[PATCH 1/3] ide: run diagnostic after SRST

[John Snow]

Software reset (SRST) should cause the diagnostic command to be run. Make an
explicit call to that routine.

Reported-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Fixes: 55adb3c45620c31f29978f209e2a44a08d34e2da
Fixes: https://bugs.launchpad.net/bugs/1900155
Tested-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Signed-off-by: John Snow <jsnow@redhat.com>
---
 hw/ide/core.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index 693b352d5e..84e887d426 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -2254,10 +2254,8 @@ static void ide_perform_srst(IDEState *s)
     /* Cancel PIO callback, reset registers/signature, etc */
     ide_reset(s);
 
-    if (s->drive_kind == IDE_CD) {
-        /* ATAPI drives do not set READY or SEEK */
-        s->status = 0x00;
-    }
+    /* perform diagnostic */
+    cmd_exec_dev_diagnostic(s, WIN_DIAGNOSE);
 }
 
 static void ide_bus_perform_srst(void *opaque)
-- 
2.26.2

[PATCH 2/3] ide: perform SRST as early as possible

[John Snow]

We don't need to wait for the falling edge. We can set BSY as
soon as possible and begin immediately resetting the drive. Devices
don't appear to need to take any specific action on the falling edge.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 hw/ide/core.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index 84e887d426..98cea7ad45 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -2280,9 +2280,7 @@ void ide_ctrl_write(void *opaque, uint32_t addr, uint32_t val)
 
     /* Device0 and Device1 each have their own control register,
      * but QEMU models it as just one register in the controller. */
-    if ((bus->cmd & IDE_CTRL_RESET) &&
-        !(val & IDE_CTRL_RESET)) {
-        /* SRST triggers on falling edge */
+    if (!(bus->cmd & IDE_CTRL_RESET) && (val & IDE_CTRL_RESET)) {
         for (i = 0; i < 2; i++) {
             s = &bus->ifs[i];
             s->status |= BUSY_STAT;
-- 
2.26.2

[PATCH 3/3] ide: clear SRST after SRST finishes

[John Snow]

The SRST protocol states that after diagnostics are complete and the
status is posted, we should clear the SRST bit if it should so happen to
be set.

The reset method itself should handle this, but just in case -- make our
intention explicit here.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 hw/ide/core.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index 98cea7ad45..e85821637c 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -2268,6 +2268,8 @@ static void ide_bus_perform_srst(void *opaque)
         s = &bus->ifs[i];
         ide_perform_srst(s);
     }
+
+    bus->cmd &= ~IDE_CTRL_RESET;
 }
 
 void ide_ctrl_write(void *opaque, uint32_t addr, uint32_t val)
-- 
2.26.2

Re: [PATCH 0/3] ide: fix SRST (again)

[Mark Cave-Ayland]

On 20/10/2020 21:02, John Snow wrote:

> My last fix for SRST wasn't entirely correct and caused a regression;
> namely we need to mime execution of the diagnostic command. If we don't,
> Linux bins the IDE device as 'horked'.
> 
> For detailed reading on the SRST protocol, I recommend checking out the
> ATA8-APT specification.
> 
> John Snow (3):
>    ide: run diagnostic after SRST
>    ide: perform SRST as early as possible
>    ide: clear SRST after SRST finishes
> 
>   hw/ide/core.c | 12 +++++-------
>   1 file changed, 5 insertions(+), 7 deletions(-)

I tried this in 3 combinations: HD only, HD + CD, and CD only and I didn't notice any 
error messages during my boot tests on qemu-system-sparc64 so:

Tested-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>


ATB,

Mark.