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 <js...@redhat.com> wrote:
> On Sun, Apr 16, 2023 at 6:29 PM Mateusz Albecki
> <mateusz.p.albe...@gmail.com> wrote:
> >
> > From: Mateusz Albecki <mateusz.albe...@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.albe...@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
>
>
