hw/ide/ahci: trace ncq write command as write instead of read

[PATCH] hw/ide/ahci: trace ncq write command as write instead of read

Posted by Fiona Ebner 2 years, 11 months ago

Fixes: e4baa9f00b ("AHCI: Replace DPRINTF with trace-events")
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---

Or should it be sorted alphabetically below execute_ncq_command_unsup?
I felt read and write belong close together and this reflects the
positions in the code.

 hw/ide/ahci.c       | 4 ++--
 hw/ide/trace-events | 1 +
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 7ce001cacd..595a96203f 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -1085,8 +1085,8 @@ static void execute_ncq_command(NCQTransferState *ncq_tfs)
                                       ncq_cb, ncq_tfs);
         break;
     case WRITE_FPDMA_QUEUED:
-        trace_execute_ncq_command_read(ad->hba, port, ncq_tfs->tag,
-                                       ncq_tfs->sector_count, ncq_tfs->lba);
+        trace_execute_ncq_command_write(ad->hba, port, ncq_tfs->tag,
+                                        ncq_tfs->sector_count, ncq_tfs->lba);
         dma_acct_start(ide_state->blk, &ncq_tfs->acct,
                        &ncq_tfs->sglist, BLOCK_ACCT_WRITE);
         ncq_tfs->aiocb = dma_blk_write(ide_state->blk, &ncq_tfs->sglist,
diff --git a/hw/ide/trace-events b/hw/ide/trace-events
index 15d7921f15..5ef344ae73 100644
--- a/hw/ide/trace-events
+++ b/hw/ide/trace-events
@@ -91,6 +91,7 @@ ahci_populate_sglist_short_map(void *s, int port) "ahci(%p)[%d]: mapped less tha
 ahci_populate_sglist_bad_offset(void *s, int port, int off_idx, int64_t off_pos) "ahci(%p)[%d]: Incorrect offset! off_idx: %d, off_pos: %"PRId64
 ncq_finish(void *s, int port, uint8_t tag) "ahci(%p)[%d][tag:%d]: NCQ transfer finished"
 execute_ncq_command_read(void *s, int port, uint8_t tag, int count, int64_t lba) "ahci(%p)[%d][tag:%d]: NCQ reading %d sectors from LBA %"PRId64
+execute_ncq_command_write(void *s, int port, uint8_t tag, int count, int64_t lba) "ahci(%p)[%d][tag:%d]: NCQ writing %d sectors to LBA %"PRId64
 execute_ncq_command_unsup(void *s, int port, uint8_t tag, uint8_t cmd) "ahci(%p)[%d][tag:%d]: error: unsupported NCQ command (0x%02x) received"
 process_ncq_command_mismatch(void *s, int port, uint8_t tag, uint8_t slot) "ahci(%p)[%d][tag:%d]: Warning: NCQ slot (%d) did not match the given tag"
 process_ncq_command_aux(void *s, int port, uint8_t tag) "ahci(%p)[%d][tag:%d]: Warn: Attempt to use NCQ auxiliary fields"
-- 
2.30.2

Re: [PATCH] hw/ide/ahci: trace ncq write command as write instead of read

Posted by Kevin Wolf 2 years, 11 months ago

Am 17.02.2023 um 11:31 hat Fiona Ebner geschrieben:
> Fixes: e4baa9f00b ("AHCI: Replace DPRINTF with trace-events")
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

Re: [PATCH] hw/ide/ahci: trace ncq write command as write instead of read

Posted by Philippe Mathieu-Daudé 2 years, 11 months ago

On 17/2/23 11:31, Fiona Ebner wrote:
> Fixes: e4baa9f00b ("AHCI: Replace DPRINTF with trace-events")
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
> 
> Or should it be sorted alphabetically below execute_ncq_command_unsup?
> I felt read and write belong close together and this reflects the
> positions in the code.
> 
>   hw/ide/ahci.c       | 4 ++--
>   hw/ide/trace-events | 1 +
>   2 files changed, 3 insertions(+), 2 deletions(-)

> diff --git a/hw/ide/trace-events b/hw/ide/trace-events
> index 15d7921f15..5ef344ae73 100644
> --- a/hw/ide/trace-events
> +++ b/hw/ide/trace-events
> @@ -91,6 +91,7 @@ ahci_populate_sglist_short_map(void *s, int port) "ahci(%p)[%d]: mapped less tha
>   ahci_populate_sglist_bad_offset(void *s, int port, int off_idx, int64_t off_pos) "ahci(%p)[%d]: Incorrect offset! off_idx: %d, off_pos: %"PRId64
>   ncq_finish(void *s, int port, uint8_t tag) "ahci(%p)[%d][tag:%d]: NCQ transfer finished"
>   execute_ncq_command_read(void *s, int port, uint8_t tag, int count, int64_t lba) "ahci(%p)[%d][tag:%d]: NCQ reading %d sectors from LBA %"PRId64
> +execute_ncq_command_write(void *s, int port, uint8_t tag, int count, int64_t lba) "ahci(%p)[%d][tag:%d]: NCQ writing %d sectors to LBA %"PRId64

Note that ideally the tag format should be '%u' (pre-existing).

I'll carry this patch along with other QDev IDE patches I plan to merge
(with John's blessing).

Thanks,

Phil.

Re: [PATCH] hw/ide/ahci: trace ncq write command as write instead of read

Posted by John Snow 2 years, 11 months ago

On Mon, Feb 20, 2023 at 3:12 AM Philippe Mathieu-Daudé
<philmd@linaro.org> wrote:
>
> On 17/2/23 11:31, Fiona Ebner wrote:
> > Fixes: e4baa9f00b ("AHCI: Replace DPRINTF with trace-events")
> > Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> > ---
> >
> > Or should it be sorted alphabetically below execute_ncq_command_unsup?
> > I felt read and write belong close together and this reflects the
> > positions in the code.
> >
> >   hw/ide/ahci.c       | 4 ++--
> >   hw/ide/trace-events | 1 +
> >   2 files changed, 3 insertions(+), 2 deletions(-)
>
> > diff --git a/hw/ide/trace-events b/hw/ide/trace-events
> > index 15d7921f15..5ef344ae73 100644
> > --- a/hw/ide/trace-events
> > +++ b/hw/ide/trace-events
> > @@ -91,6 +91,7 @@ ahci_populate_sglist_short_map(void *s, int port) "ahci(%p)[%d]: mapped less tha
> >   ahci_populate_sglist_bad_offset(void *s, int port, int off_idx, int64_t off_pos) "ahci(%p)[%d]: Incorrect offset! off_idx: %d, off_pos: %"PRId64
> >   ncq_finish(void *s, int port, uint8_t tag) "ahci(%p)[%d][tag:%d]: NCQ transfer finished"
> >   execute_ncq_command_read(void *s, int port, uint8_t tag, int count, int64_t lba) "ahci(%p)[%d][tag:%d]: NCQ reading %d sectors from LBA %"PRId64
> > +execute_ncq_command_write(void *s, int port, uint8_t tag, int count, int64_t lba) "ahci(%p)[%d][tag:%d]: NCQ writing %d sectors to LBA %"PRId64
>
> Note that ideally the tag format should be '%u' (pre-existing).
>
> I'll carry this patch along with other QDev IDE patches I plan to merge
> (with John's blessing).

Sounds good to me, thank you Phil.

--js

Re: [PATCH] hw/ide/ahci: trace ncq write command as write instead of read

Posted by Alex Bennée 2 years, 11 months ago

Fiona Ebner <f.ebner@proxmox.com> writes:

> Fixes: e4baa9f00b ("AHCI: Replace DPRINTF with trace-events")
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
<snip>

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH] hw/ide/ahci: trace ncq write command as write instead of read

Posted by Philippe Mathieu-Daudé 2 years, 11 months ago

On 17/2/23 11:31, Fiona Ebner wrote:
> Fixes: e4baa9f00b ("AHCI: Replace DPRINTF with trace-events")

Oops

> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
> 
> Or should it be sorted alphabetically below execute_ncq_command_unsup?

No, there is no convention...

> I felt read and write belong close together and this reflects the
> positions in the code.

probably for this reason.

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

Thanks!

Re: [PATCH] hw/ide/ahci: trace ncq write command as write instead of read

Posted by John Snow 2 years, 11 months ago

On Fri, Feb 17, 2023 at 7:27 AM Philippe Mathieu-Daudé
<philmd@linaro.org> wrote:
>
> On 17/2/23 11:31, Fiona Ebner wrote:
> > Fixes: e4baa9f00b ("AHCI: Replace DPRINTF with trace-events")
>
> Oops

Seconding the whoops.

>
> > Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> > ---
> >
> > Or should it be sorted alphabetically below execute_ncq_command_unsup?
>
> No, there is no convention...
>
> > I felt read and write belong close together and this reflects the
> > positions in the code.
>
> probably for this reason.
>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>
> Thanks!
>

Reviewed-by: John Snow <jsnow@redhat.com>