| Message ID | 20190304180920.21534-2-svens@stackframe.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/5] lsi: use ldn_le_p()/stn_le_p() | expand |
On 3/4/19 7:09 PM, Sven Schnelle wrote: > This makes the code easier to read - no functional change. > > Signed-off-by: Sven Schnelle <svens@stackframe.org> > --- > hw/scsi/lsi53c895a.c | 42 +++++++++++++++++++++++------------------- > 1 file changed, 23 insertions(+), 19 deletions(-) > > diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c > index 6d280f8b77..fb9c6db4b2 100644 > --- a/hw/scsi/lsi53c895a.c > +++ b/hw/scsi/lsi53c895a.c > @@ -194,6 +194,13 @@ typedef struct lsi_request { > QTAILQ_ENTRY(lsi_request) next; > } lsi_request; > > +enum { > + LSI_NOWAIT, You forgot the comment for NOWAIT. > + LSI_WAIT_RESELECT, /* Wait Reselect instruction has been issued */ > + LSI_DMA_SCRIPTS, /* processing DMA from lsi_execute_script */ > + LSI_DMA_IN_PROGRESS, /* DMA operation is in progress */ > +}; > + > typedef struct { > /*< private >*/ > PCIDevice parent_obj; > @@ -212,10 +219,6 @@ typedef struct { > int msg_action; > int msg_len; > uint8_t msg[LSI_MAX_MSGIN_LEN]; > - /* 0 if SCRIPTS are running or stopped. > - * 1 if a Wait Reselect instruction has been issued. > - * 2 if processing DMA from lsi_execute_script. > - * 3 if a DMA operation is in progress. */ > int waiting; When a field is not used by migration, you can declare it as enum: enum { LSI_NOWAIT = 0, /* SCRIPTS are running or stopped */ LSI_WAIT_RESELECT = 1, /* Wait Reselect instruction has been issued */ LSI_DMA_SCRIPTS = 2, /* processing DMA from lsi_execute_script */ LSI_DMA_IN_PROGRESS = 3, /* DMA operation is in progress */ } waiting; This gives hints to the compiler about values to check. > SCSIBus bus; > int current_lun; > @@ -322,7 +325,7 @@ static void lsi_soft_reset(LSIState *s) > > s->msg_action = 0; > s->msg_len = 0; > - s->waiting = 0; > + s->waiting = LSI_NOWAIT; > s->dsa = 0; > s->dnad = 0; > s->dbc = 0; > @@ -564,10 +567,10 @@ static void lsi_bad_phase(LSIState *s, int out, int new_phase) > static void lsi_resume_script(LSIState *s) > { > if (s->waiting != 2) { > - s->waiting = 0; > + s->waiting = LSI_NOWAIT; > lsi_execute_script(s); > } else { > - s->waiting = 0; > + s->waiting = LSI_NOWAIT; > } > } > > @@ -744,7 +747,7 @@ static int lsi_queue_req(LSIState *s, SCSIRequest *req, uint32_t len) > Since no interrupt stacking is implemented in the emulation, it > is also required that there are no pending interrupts waiting > for service from the device driver. */ > - if (s->waiting == 1 || > + if (s->waiting == LSI_WAIT_RESELECT || > (lsi_irq_on_rsl(s) && !(s->scntl1 & LSI_SCNTL1_CON) && > !(s->istat0 & (LSI_ISTAT0_SIP | LSI_ISTAT0_DIP)))) { > /* Reselect device. */ > @@ -789,7 +792,7 @@ static void lsi_transfer_data(SCSIRequest *req, uint32_t len) > int out; > > assert(req->hba_private); > - if (s->waiting == 1 || req->hba_private != s->current || > + if (s->waiting == LSI_WAIT_RESELECT || req->hba_private != s->current || > (lsi_irq_on_rsl(s) && !(s->scntl1 & LSI_SCNTL1_CON))) { > if (lsi_queue_req(s, req, len)) { > return; > @@ -803,7 +806,7 @@ static void lsi_transfer_data(SCSIRequest *req, uint32_t len) > s->current->dma_len = len; > s->command_complete = 1; > if (s->waiting) { > - if (s->waiting == 1 || s->dbc == 0) { > + if (s->waiting == LSI_WAIT_RESELECT || s->dbc == 0) { > lsi_resume_script(s); > } else { > lsi_do_dma(s, out); > @@ -1093,7 +1096,7 @@ static void lsi_wait_reselect(LSIState *s) > lsi_reselect(s, p); > } > if (s->current == NULL) { > - s->waiting = 1; > + s->waiting = LSI_WAIT_RESELECT; > } > } > > @@ -1202,16 +1205,16 @@ again: > s->dnad64 = addr_high; > switch (s->sstat1 & 0x7) { > case PHASE_DO: > - s->waiting = 2; > + s->waiting = LSI_DMA_SCRIPTS; > lsi_do_dma(s, 1); > if (s->waiting) > - s->waiting = 3; > + s->waiting = LSI_DMA_IN_PROGRESS; > break; > case PHASE_DI: > - s->waiting = 2; > + s->waiting = LSI_DMA_SCRIPTS; > lsi_do_dma(s, 0); > if (s->waiting) > - s->waiting = 3; > + s->waiting = LSI_DMA_IN_PROGRESS; > break; > case PHASE_CMD: > lsi_do_command(s); > @@ -1278,6 +1281,7 @@ again: > } > s->sbcl |= LSI_SBCL_BSY; > lsi_set_phase(s, PHASE_MO); > + s->waiting = LSI_NOWAIT; > break; > case 1: /* Disconnect */ > trace_lsi_execute_script_io_disconnect(); > @@ -1542,7 +1546,7 @@ again: > } > } > } > - if (insn_processed > 10000 && !s->waiting) { > + if (insn_processed > 10000 && s->waiting == LSI_NOWAIT) { > /* Some windows drivers make the device spin waiting for a memory > location to change. If we have been executed a lot of code then > assume this is the case and force an unexpected device disconnect. > @@ -1554,7 +1558,7 @@ again: > } > lsi_script_scsi_interrupt(s, LSI_SIST0_UDC, 0); > lsi_disconnect(s); > - } else if (s->istat1 & LSI_ISTAT1_SRUN && !s->waiting) { > + } else if (s->istat1 & LSI_ISTAT1_SRUN && s->waiting == LSI_NOWAIT) { > if (s->dcntl & LSI_DCNTL_SSM) { > lsi_script_dma_interrupt(s, LSI_DSTAT_SSI); > } else { > @@ -1885,9 +1889,9 @@ static void lsi_reg_writeb(LSIState *s, int offset, uint8_t val) > s->istat0 &= ~LSI_ISTAT0_INTF; > lsi_update_irq(s); > } > - if (s->waiting == 1 && val & LSI_ISTAT0_SIGP) { > + if (s->waiting == LSI_WAIT_RESELECT && val & LSI_ISTAT0_SIGP) { > trace_lsi_awoken(); > - s->waiting = 0; > + s->waiting = LSI_NOWAIT; > s->dsp = s->dnad; > lsi_execute_script(s); > } >
Hi Philippe, On Tue, Mar 05, 2019 at 12:18:01AM +0100, Philippe Mathieu-Daudé wrote: > > > > +enum { > > + LSI_NOWAIT, > > You forgot the comment for NOWAIT. I thought LSI_NOWAIT is self-explaining, but will add that. > > int waiting; > > When a field is not used by migration, you can declare it as enum: > > enum { > LSI_NOWAIT = 0, /* SCRIPTS are running or stopped */ > LSI_WAIT_RESELECT = 1, /* Wait Reselect instruction has been > issued */ > LSI_DMA_SCRIPTS = 2, /* processing DMA from lsi_execute_script */ > LSI_DMA_IN_PROGRESS = 3, /* DMA operation is in progress */ > } waiting; > > This gives hints to the compiler about values to check. But it is used by migration, so this doesn't apply here? I had a typedef enum before, but this doesn't compile. Otherwise thanks for reviewing. Regards Sven
On 3/5/19 8:17 AM, Sven Schnelle wrote: > Hi Philippe, > > On Tue, Mar 05, 2019 at 12:18:01AM +0100, Philippe Mathieu-Daudé wrote: >>> >>> +enum { >>> + LSI_NOWAIT, >> >> You forgot the comment for NOWAIT. > > I thought LSI_NOWAIT is self-explaining, but will add that. > >>> int waiting; >> >> When a field is not used by migration, you can declare it as enum: >> >> enum { >> LSI_NOWAIT = 0, /* SCRIPTS are running or stopped */ >> LSI_WAIT_RESELECT = 1, /* Wait Reselect instruction has been >> issued */ >> LSI_DMA_SCRIPTS = 2, /* processing DMA from lsi_execute_script */ >> LSI_DMA_IN_PROGRESS = 3, /* DMA operation is in progress */ >> } waiting; >> >> This gives hints to the compiler about values to check. > > But it is used by migration, so this doesn't apply here? I had a typedef enum before, > but this doesn't compile. Oh you are right, I didn't check... So with the updated comment: Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> > > Otherwise thanks for reviewing. > > Regards > Sven > >
diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c index 6d280f8b77..fb9c6db4b2 100644 --- a/hw/scsi/lsi53c895a.c +++ b/hw/scsi/lsi53c895a.c @@ -194,6 +194,13 @@ typedef struct lsi_request { QTAILQ_ENTRY(lsi_request) next; } lsi_request; +enum { + LSI_NOWAIT, + LSI_WAIT_RESELECT, /* Wait Reselect instruction has been issued */ + LSI_DMA_SCRIPTS, /* processing DMA from lsi_execute_script */ + LSI_DMA_IN_PROGRESS, /* DMA operation is in progress */ +}; + typedef struct { /*< private >*/ PCIDevice parent_obj; @@ -212,10 +219,6 @@ typedef struct { int msg_action; int msg_len; uint8_t msg[LSI_MAX_MSGIN_LEN]; - /* 0 if SCRIPTS are running or stopped. - * 1 if a Wait Reselect instruction has been issued. - * 2 if processing DMA from lsi_execute_script. - * 3 if a DMA operation is in progress. */ int waiting; SCSIBus bus; int current_lun; @@ -322,7 +325,7 @@ static void lsi_soft_reset(LSIState *s) s->msg_action = 0; s->msg_len = 0; - s->waiting = 0; + s->waiting = LSI_NOWAIT; s->dsa = 0; s->dnad = 0; s->dbc = 0; @@ -564,10 +567,10 @@ static void lsi_bad_phase(LSIState *s, int out, int new_phase) static void lsi_resume_script(LSIState *s) { if (s->waiting != 2) { - s->waiting = 0; + s->waiting = LSI_NOWAIT; lsi_execute_script(s); } else { - s->waiting = 0; + s->waiting = LSI_NOWAIT; } } @@ -744,7 +747,7 @@ static int lsi_queue_req(LSIState *s, SCSIRequest *req, uint32_t len) Since no interrupt stacking is implemented in the emulation, it is also required that there are no pending interrupts waiting for service from the device driver. */ - if (s->waiting == 1 || + if (s->waiting == LSI_WAIT_RESELECT || (lsi_irq_on_rsl(s) && !(s->scntl1 & LSI_SCNTL1_CON) && !(s->istat0 & (LSI_ISTAT0_SIP | LSI_ISTAT0_DIP)))) { /* Reselect device. */ @@ -789,7 +792,7 @@ static void lsi_transfer_data(SCSIRequest *req, uint32_t len) int out; assert(req->hba_private); - if (s->waiting == 1 || req->hba_private != s->current || + if (s->waiting == LSI_WAIT_RESELECT || req->hba_private != s->current || (lsi_irq_on_rsl(s) && !(s->scntl1 & LSI_SCNTL1_CON))) { if (lsi_queue_req(s, req, len)) { return; @@ -803,7 +806,7 @@ static void lsi_transfer_data(SCSIRequest *req, uint32_t len) s->current->dma_len = len; s->command_complete = 1; if (s->waiting) { - if (s->waiting == 1 || s->dbc == 0) { + if (s->waiting == LSI_WAIT_RESELECT || s->dbc == 0) { lsi_resume_script(s); } else { lsi_do_dma(s, out); @@ -1093,7 +1096,7 @@ static void lsi_wait_reselect(LSIState *s) lsi_reselect(s, p); } if (s->current == NULL) { - s->waiting = 1; + s->waiting = LSI_WAIT_RESELECT; } } @@ -1202,16 +1205,16 @@ again: s->dnad64 = addr_high; switch (s->sstat1 & 0x7) { case PHASE_DO: - s->waiting = 2; + s->waiting = LSI_DMA_SCRIPTS; lsi_do_dma(s, 1); if (s->waiting) - s->waiting = 3; + s->waiting = LSI_DMA_IN_PROGRESS; break; case PHASE_DI: - s->waiting = 2; + s->waiting = LSI_DMA_SCRIPTS; lsi_do_dma(s, 0); if (s->waiting) - s->waiting = 3; + s->waiting = LSI_DMA_IN_PROGRESS; break; case PHASE_CMD: lsi_do_command(s); @@ -1278,6 +1281,7 @@ again: } s->sbcl |= LSI_SBCL_BSY; lsi_set_phase(s, PHASE_MO); + s->waiting = LSI_NOWAIT; break; case 1: /* Disconnect */ trace_lsi_execute_script_io_disconnect(); @@ -1542,7 +1546,7 @@ again: } } } - if (insn_processed > 10000 && !s->waiting) { + if (insn_processed > 10000 && s->waiting == LSI_NOWAIT) { /* Some windows drivers make the device spin waiting for a memory location to change. If we have been executed a lot of code then assume this is the case and force an unexpected device disconnect. @@ -1554,7 +1558,7 @@ again: } lsi_script_scsi_interrupt(s, LSI_SIST0_UDC, 0); lsi_disconnect(s); - } else if (s->istat1 & LSI_ISTAT1_SRUN && !s->waiting) { + } else if (s->istat1 & LSI_ISTAT1_SRUN && s->waiting == LSI_NOWAIT) { if (s->dcntl & LSI_DCNTL_SSM) { lsi_script_dma_interrupt(s, LSI_DSTAT_SSI); } else { @@ -1885,9 +1889,9 @@ static void lsi_reg_writeb(LSIState *s, int offset, uint8_t val) s->istat0 &= ~LSI_ISTAT0_INTF; lsi_update_irq(s); } - if (s->waiting == 1 && val & LSI_ISTAT0_SIGP) { + if (s->waiting == LSI_WAIT_RESELECT && val & LSI_ISTAT0_SIGP) { trace_lsi_awoken(); - s->waiting = 0; + s->waiting = LSI_NOWAIT; s->dsp = s->dnad; lsi_execute_script(s); }
This makes the code easier to read - no functional change. Signed-off-by: Sven Schnelle <svens@stackframe.org> --- hw/scsi/lsi53c895a.c | 42 +++++++++++++++++++++++------------------- 1 file changed, 23 insertions(+), 19 deletions(-)