diff mbox series

[v3,2/8] hw/ide/core: set ERR_STAT in unsupported command completion

Message ID 20230609140844.202795-3-nks@flawful.org
State New
Headers show
Series misc AHCI cleanups | expand

Commit Message

Niklas Cassel June 9, 2023, 2:08 p.m. UTC 
From: Niklas Cassel <niklas.cassel@wdc.com>

Currently, the first time sending an unsupported command
(e.g. READ LOG DMA EXT) will not have ERR_STAT set in the completion.
Sending the unsupported command again, will correctly have ERR_STAT set.

When ide_cmd_permitted() returns false, it calls ide_abort_command().
ide_abort_command() first calls ide_transfer_stop(), which will call
ide_transfer_halt() and ide_cmd_done(), after that ide_abort_command()
sets ERR_STAT in status.

ide_cmd_done() for AHCI will call ahci_write_fis_d2h() which writes the
current status in the FIS, and raises an IRQ. (The status here will not
have ERR_STAT set!).

Thus, we cannot call ide_transfer_stop() before setting ERR_STAT, as
ide_transfer_stop() will result in the FIS being written and an IRQ
being raised.

The reason why it works the second time, is that ERR_STAT will still
be set from the previous command, so when writing the FIS, the
completion will correctly have ERR_STAT set.

Set ERR_STAT before writing the FIS (calling cmd_done), so that we will
raise an error IRQ correctly when receiving an unsupported command.

Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
---
 hw/ide/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Philippe Mathieu-Daudé July 19, 2023, 11:43 a.m. UTC | #1 
On 9/6/23 16:08, Niklas Cassel wrote:
> From: Niklas Cassel <niklas.cassel@wdc.com>
> 
> Currently, the first time sending an unsupported command
> (e.g. READ LOG DMA EXT) will not have ERR_STAT set in the completion.
> Sending the unsupported command again, will correctly have ERR_STAT set.
> 
> When ide_cmd_permitted() returns false, it calls ide_abort_command().
> ide_abort_command() first calls ide_transfer_stop(), which will call
> ide_transfer_halt() and ide_cmd_done(), after that ide_abort_command()
> sets ERR_STAT in status.
> 
> ide_cmd_done() for AHCI will call ahci_write_fis_d2h() which writes the
> current status in the FIS, and raises an IRQ. (The status here will not
> have ERR_STAT set!).
> 
> Thus, we cannot call ide_transfer_stop() before setting ERR_STAT, as
> ide_transfer_stop() will result in the FIS being written and an IRQ
> being raised.
> 
> The reason why it works the second time, is that ERR_STAT will still
> be set from the previous command, so when writing the FIS, the
> completion will correctly have ERR_STAT set.
> 
> Set ERR_STAT before writing the FIS (calling cmd_done), so that we will
> raise an error IRQ correctly when receiving an unsupported command.
> 
> Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
> ---
>   hw/ide/core.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
diff mbox series

Patch

diff --git a/hw/ide/core.c b/hw/ide/core.c
index de48ff9f86..07971c0218 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -533,9 +533,9 @@  BlockAIOCB *ide_issue_trim(
 
 void ide_abort_command(IDEState *s)
 {
-    ide_transfer_stop(s);
     s->status = READY_STAT | ERR_STAT;
     s->error = ABRT_ERR;
+    ide_transfer_stop(s);
 }
 
 static void ide_set_retry(IDEState *s)