On Fri, Apr 28, 2023 at 9:22 AM Niklas Cassel <n...@flawful.org> wrote: > > From: Niklas Cassel <niklas.cas...@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.cas...@wdc.com> > --- > hw/ide/core.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/ide/core.c b/hw/ide/core.c > index 45d14a25e9..c144d1155d 100644 > --- a/hw/ide/core.c > +++ b/hw/ide/core.c > @@ -531,9 +531,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) > -- > 2.40.0 >
Seems OK at a glance. Does this change the behavior of ide_transfer_stop at all? I guess we've been using this order of operations since 2008 at least. I didn't know C then. ACK