[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 4/5] lsi: use SCSI phase names instead of number
From: |
Philippe Mathieu-Daudé |
Subject: |
Re: [Qemu-devel] [PATCH 4/5] lsi: use SCSI phase names instead of numbers in trace |
Date: |
Tue, 5 Mar 2019 00:10:31 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.5.1 |
Hi Sven,
On 3/4/19 7:09 PM, Sven Schnelle wrote:
> Signed-off-by: Sven Schnelle <address@hidden>
> ---
> hw/scsi/lsi53c895a.c | 24 ++++++++++++++++++------
> hw/scsi/trace-events | 4 ++--
> 2 files changed, 20 insertions(+), 8 deletions(-)
>
> diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c
> index d1eb2cf074..f635d56e0f 100644
> --- a/hw/scsi/lsi53c895a.c
> +++ b/hw/scsi/lsi53c895a.c
> @@ -184,6 +184,17 @@ static const char *names[] = {
> /* Flag set if this is a tagged command. */
> #define LSI_TAG_VALID (1 << 16)
>
> +static const char *scsi_phases[] = {
> + "DOUT",
> + "DIN",
> + "CMD",
> + "STATUS",
> + "RSVIN",
> + "RSVOUT",
> + "MSGOUT",
> + "MSGIN"
> +};
Some developper might use your code but forget to use value &
PHASE_MASK, trying on small values, then another user uses bigger values
and this array will return unexpected result.
static const char *scsi_phase_name(int phase)
{
static const char *scsi_phases[8] = {
[PHASE_DO] = "DOUT",
[PHASE_DI] = "DIN",
[PHASE_CMD] = "CMD",
[PHASE_ST] = "STATUS",
[PHASE_MO] = "MSGOUT",
[PHASE_MI] = "MSGIN",
};
phase &= PHASE_MASK;
return scsi_phases[phase] ? scsi_phases[phase] : "RESERVED";
}
You can also keep your RSVIN/RSVOUT:
static const char *scsi_phase_name(int phase)
{
static const char *scsi_phases[8] = {
"DOUT",
"DIN",
"CMD",
"STATUS",
"RSVIN",
"RSVOUT",
"MSGOUT",
"MSGIN"
};
return scsi_phases[phase & PHASE_MASK];
}
In both cases, calls are easier to read:
trace_lsi_execute_script_blockmove_badphase(
scsi_phase_name(s->sstat1),
scsi_phase_name(insn >> 24));
> +
> typedef struct lsi_request {
> SCSIRequest *req;
> uint32_t tag;
> @@ -1201,8 +1212,9 @@ again:
> s->ia = s->dsp - 12;
> }
> if ((s->sstat1 & PHASE_MASK) != ((insn >> 24) & 7)) {
> - trace_lsi_execute_script_blockmove_badphase(s->sstat1 &
> PHASE_MASK,
> - (insn >> 24) & 7);
> + trace_lsi_execute_script_blockmove_badphase(
> + scsi_phases[s->sstat1 & PHASE_MASK],
> + scsi_phases[(insn >> 24) & 7]);
> lsi_script_scsi_interrupt(s, LSI_SIST0_MA, 0);
> break;
> }
> @@ -1234,8 +1246,8 @@ again:
> lsi_do_msgin(s);
> break;
> default:
> - qemu_log_mask(LOG_UNIMP, "lsi_scsi: Unimplemented phase %d\n",
> - s->sstat1 & PHASE_MASK);
> + qemu_log_mask(LOG_UNIMP, "lsi_scsi: Unimplemented phase %s\n",
> + scsi_phases[s->sstat1 & PHASE_MASK]);
> }
> s->dfifo = s->dbc & 0xff;
> s->ctest5 = (s->ctest5 & 0xfc) | ((s->dbc >> 8) & 3);
> @@ -1462,9 +1474,9 @@ again:
> }
> if (cond == jmp && (insn & (1 << 17))) {
> trace_lsi_execute_script_tc_compp(
> - (s->sstat1 & PHASE_MASK),
> + scsi_phases[(s->sstat1 & PHASE_MASK)],
No need for parenthesis here,
> jmp ? '=' : '!',
> - ((insn >> 24) & 7));
> + scsi_phases[((insn >> 24) & 7)]);
Ditto.
> cond = (s->sstat1 & PHASE_MASK) == ((insn >> 24) & 7);
> }
> if (cond == jmp && (insn & (1 << 18))) {
> diff --git a/hw/scsi/trace-events b/hw/scsi/trace-events
> index 29aaa752d1..34feb6f42d 100644
> --- a/hw/scsi/trace-events
> +++ b/hw/scsi/trace-events
> @@ -268,7 +268,7 @@ lsi_memcpy(uint32_t dest, uint32_t src, int count)
> "memcpy dest 0x%"PRIx32" src
> lsi_wait_reselect(void) "Wait Reselect"
> lsi_execute_script(uint32_t dsp, uint32_t insn, uint32_t addr) "SCRIPTS
> dsp=0x%"PRIx32" opcode 0x%"PRIx32" arg 0x%"PRIx32
> lsi_execute_script_blockmove_delayed(void) "Delayed select timeout"
> -lsi_execute_script_blockmove_badphase(uint8_t phase, uint8_t expected)
> "Wrong phase got %d expected %d"
> +lsi_execute_script_blockmove_badphase(const char *phase, const char
> *expected) "Wrong phase got %s expected %s"
> lsi_execute_script_io_alreadyreselected(void) "Already reselected, jumping
> to alternative address"
> lsi_execute_script_io_selected(uint8_t id, const char *atn) "Selected target
> %d%s"
> lsi_execute_script_io_disconnect(void) "Wait Disconnect"
> @@ -278,7 +278,7 @@ lsi_execute_script_io_opcode(const char *opcode, int reg,
> const char *opname, ui
> lsi_execute_script_tc_nop(void) "NOP"
> lsi_execute_script_tc_delayedselect_timeout(void) "Delayed select timeout"
> lsi_execute_script_tc_compc(int result) "Compare carry %d"
> -lsi_execute_script_tc_compp(uint8_t phase, int op, uint8_t insn_phase)
> "Compare phase %d %c= %d"
> +lsi_execute_script_tc_compp(const char *phase, int op, const char
> *insn_phase) "Compare phase %s %c= %s"
Since you change this line, can you fix 'int op' -> 'char op'?
Or as a previous cleanup patch... There are other '%c' misused.
> lsi_execute_script_tc_compd(uint32_t sfbr, uint8_t mask, int op, int result)
> "Compare data 0x%"PRIx32" & 0x%x %c= 0x%x"
> lsi_execute_script_tc_jump(uint32_t addr) "Jump to 0x%"PRIx32
> lsi_execute_script_tc_call(uint32_t addr) "Call 0x%"PRIx32
>
Regards,
Phil.
- [Qemu-devel] [PATCH 1/5] lsi: use ldn_le_p()/stn_le_p(), Sven Schnelle, 2019/03/04
- [Qemu-devel] [PATCH 3/5] lsi: use enum type for s->msg_action, Sven Schnelle, 2019/03/04
- [Qemu-devel] [PATCH 5/5] lsi: return dfifo value, Sven Schnelle, 2019/03/04
- [Qemu-devel] [PATCH 4/5] lsi: use SCSI phase names instead of numbers in trace, Sven Schnelle, 2019/03/04
- Re: [Qemu-devel] [PATCH 4/5] lsi: use SCSI phase names instead of numbers in trace,
Philippe Mathieu-Daudé <=
- Re: [Qemu-devel] [PATCH 1/5] lsi: use ldn_le_p()/stn_le_p(), Eric Blake, 2019/03/04
- Re: [Qemu-devel] [PATCH 1/5] lsi: use ldn_le_p()/stn_le_p(), Philippe Mathieu-Daudé, 2019/03/04