Re: [PATCH 1/1] hw/ide/core.c: fix handling of unsupported commands

[Mateusz Albecki]

Certainly seems like my patch is wrong as it will make the abort path execute ide_cmd_done twice. During debug I came to the conclusion that ide_cmd_done is not called at all as I was getting timeouts on the driver side while waiting for D2H FIS. I am still not sure how I was getting this behavior if the problem was actually with setting correct error bits. Even so I think it can be safely assumed that Niklas' change will solve the issue, I will try to verify it in a couple of days and if I see any problem I will come back to you.

Mateusz

On Wed, 17 May 2023 at 23:33, John Snow <jsnow@redhat.com> wrote:

On Sun, Apr 16, 2023 at 6:29 PM Mateusz Albecki
<mateusz.p.albecki@gmail.com> wrote:
>
> From: Mateusz Albecki <mateusz.albecki@outlook.com>
>
> Current code will not call ide_cmd_done when aborting the unsupported
> command which will lead to the command timeout on the driver side instead
> of getting a D2H FIS with ABRT indication. This can lead to problems on the
> driver side as the spec mandates that device should return a D2H FIS with
> ABRT bit set in ERR register(from SATA 3.1 section 16.3.3.8.6)
>
> Signed-off-by: Mateusz Albecki <mateusz.p.albecki@gmail.com>
> ---
>  hw/ide/core.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index 45d14a25e9..d7027bbd4d 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -2146,6 +2146,7 @@ void ide_bus_exec_cmd(IDEBus *bus, uint32_t val)
>
>      if (!ide_cmd_permitted(s, val)) {
>          ide_abort_command(s);
> +        ide_cmd_done(s);
>          ide_bus_set_irq(s->bus);
>          return;
>      }
> --
> 2.40.0
>

I recently noticed that Niklas Cassel sent a patch to fix unsupported
command handling:
https://lists.gnu.org/archive/html/qemu-devel/2023-04/msg05552.html

I suspect that his approach is the more technically correct one and
that calling ide_cmd_done here is a heavy cudgel that may have
unintended consequences. Am I mistaken?
Can you check that Niklas's patch solves your issue? I think you're
both solving the same problem. I've CC'd him on this patch as well.

--js