Re: [PATCH v2 34/42] esp: remove old deferred command completion mechanism

[Laurent Vivier]

Le 09/02/2021 à 20:30, Mark Cave-Ayland a écrit :
> Commit ea84a44250 "scsi: esp: Defer command completion until previous 
> interrupts
> have been handled" provided a mechanism to delay the command completion 
> interrupt
> until ESP_RINTR is read after the command has completed.
> 
> With the previous fixes for latching the ESP_RINTR bits and deferring the 
> setting
> of the command completion interrupt for incoming data to the SCSI callback, 
> this
> workaround is no longer required and can be removed.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/scsi/esp.c         | 33 ++++++++-------------------------
>  include/hw/scsi/esp.h |  4 ++--
>  2 files changed, 10 insertions(+), 27 deletions(-)
> 
> diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
> index ce6a7a1ed0..8b856155d1 100644
> --- a/hw/scsi/esp.c
> +++ b/hw/scsi/esp.c
> @@ -574,8 +574,11 @@ static void esp_do_dma(ESPState *s)
>      esp_lower_drq(s);
>  }
>  
> -static void esp_report_command_complete(ESPState *s, uint32_t status)
> +void esp_command_complete(SCSIRequest *req, uint32_t status,
> +                          size_t resid)
>  {
> +    ESPState *s = req->hba_private;
> +
>      trace_esp_command_complete();
>      if (s->ti_size != 0) {
>          trace_esp_command_complete_unexpected();
> @@ -596,24 +599,6 @@ static void esp_report_command_complete(ESPState *s, 
> uint32_t status)
>      }
>  }
>  
> -void esp_command_complete(SCSIRequest *req, uint32_t status,
> -                          size_t resid)
> -{
> -    ESPState *s = req->hba_private;
> -
> -    if (s->rregs[ESP_RSTAT] & STAT_INT) {
> -        /*
> -         * Defer handling command complete until the previous
> -         * interrupt has been handled.
> -         */
> -        trace_esp_command_complete_deferred();
> -        s->deferred_status = status;
> -        s->deferred_complete = true;
> -        return;
> -    }
> -    esp_report_command_complete(s, status);
> -}
> -
>  void esp_transfer_data(SCSIRequest *req, uint32_t len)
>  {
>      ESPState *s = req->hba_private;
> @@ -740,10 +725,6 @@ uint64_t esp_reg_read(ESPState *s, uint32_t saddr)
>          s->rregs[ESP_RSTAT] &= ~STAT_TC;
>          s->rregs[ESP_RSEQ] = SEQ_0;
>          esp_lower_irq(s);
> -        if (s->deferred_complete) {
> -            esp_report_command_complete(s, s->deferred_status);
> -            s->deferred_complete = false;
> -        }
>          break;
>      case ESP_TCHI:
>          /* Return the unique id if the value has never been written */
> @@ -951,8 +932,10 @@ const VMStateDescription vmstate_esp = {
>          VMSTATE_UINT32(ti_wptr, ESPState),
>          VMSTATE_BUFFER(ti_buf, ESPState),
>          VMSTATE_UINT32(status, ESPState),
> -        VMSTATE_UINT32(deferred_status, ESPState),
> -        VMSTATE_BOOL(deferred_complete, ESPState),
> +        VMSTATE_UINT32_TEST(mig_deferred_status, ESPState,
> +                            esp_is_before_version_5),
> +        VMSTATE_BOOL_TEST(mig_deferred_complete, ESPState,
> +                          esp_is_before_version_5),
>          VMSTATE_UINT32(dma, ESPState),
>          VMSTATE_PARTIAL_BUFFER(cmdbuf, ESPState, 16),
>          VMSTATE_BUFFER_START_MIDDLE_V(cmdbuf, ESPState, 16, 4),
> diff --git a/include/hw/scsi/esp.h b/include/hw/scsi/esp.h
> index 3b69aedebe..5e68908fcb 100644
> --- a/include/hw/scsi/esp.h
> +++ b/include/hw/scsi/esp.h
> @@ -30,8 +30,6 @@ struct ESPState {
>      int32_t ti_size;
>      uint32_t ti_rptr, ti_wptr;
>      uint32_t status;
> -    uint32_t deferred_status;
> -    bool deferred_complete;
>      uint32_t dma;
>      uint8_t ti_buf[TI_BUFSZ];
>      SCSIBus bus;
> @@ -57,6 +55,8 @@ struct ESPState {
>  
>      /* Legacy fields for vmstate_esp version < 5 */
>      uint32_t mig_dma_left;
> +    uint32_t mig_deferred_status;
> +    bool mig_deferred_complete;
>  };
>  
>  #define TYPE_SYSBUS_ESP "sysbus-esp"
> 

Reviewed-by: Laurent Vivier <laurent@vivier.eu>