1. Patchew
  2. QEMU
  3. [PATCH v2 0/7] esp.c: only allow ESP commands permitted in the current mode
  4. View patch

[PATCH v2 1/7] esp.c: only raise IRQ in esp_transfer_data() for CMD_SEL, CMD_SELATN and CMD_TI commands

Mark Cave-Ayland posted 7 patches 5 months ago
Maintainers: Paolo Bonzini <pbonzini@redhat.com>, Fam Zheng <fam@euphon.net>
There is a newer version of this series
[PATCH v2 1/7] esp.c: only raise IRQ in esp_transfer_data() for CMD_SEL, CMD_SELATN and CMD_TI commands
Posted by Mark Cave-Ayland 5 months ago 
Clarify the logic in esp_transfer_data() to ensure that the deferred interrupt code
can only be triggered for CMD_SEL, CMD_SELATN and CMD_TI commands. This should already
be the case, but make it explicit to ensure the logic isn't triggered unexpectedly.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/scsi/esp.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index f24991fd16..9181c8810f 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -1012,6 +1012,7 @@ void esp_transfer_data(SCSIRequest *req, uint32_t len)
              */
              s->rregs[ESP_RINTR] |= INTR_BS | INTR_FC;
              s->rregs[ESP_RSEQ] = SEQ_CD;
+             esp_raise_irq(s);
              break;
 
         case CMD_SELATNS | CMD_DMA:
@@ -1022,6 +1023,7 @@ void esp_transfer_data(SCSIRequest *req, uint32_t len)
              */
              s->rregs[ESP_RINTR] |= INTR_BS;
              s->rregs[ESP_RSEQ] = SEQ_MO;
+             esp_raise_irq(s);
              break;
 
         case CMD_TI | CMD_DMA:
@@ -1032,10 +1034,9 @@ void esp_transfer_data(SCSIRequest *req, uint32_t len)
              */
             s->rregs[ESP_CMD] = 0;
             s->rregs[ESP_RINTR] |= INTR_BS;
+            esp_raise_irq(s);
             break;
         }
-
-        esp_raise_irq(s);
     }
 
     /*
-- 
2.39.5
Re: [PATCH v2 1/7] esp.c: only raise IRQ in esp_transfer_data() for CMD_SEL, CMD_SELATN and CMD_TI commands
Posted by Philippe Mathieu-Daudé 4 months, 1 week ago 
On 18/6/25 08:12, Mark Cave-Ayland wrote:
> Clarify the logic in esp_transfer_data() to ensure that the deferred interrupt code
> can only be triggered for CMD_SEL, CMD_SELATN and CMD_TI commands. This should already
> be the case, but make it explicit to ensure the logic isn't triggered unexpectedly.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>   hw/scsi/esp.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
> index f24991fd16..9181c8810f 100644
> --- a/hw/scsi/esp.c
> +++ b/hw/scsi/esp.c
> @@ -1012,6 +1012,7 @@ void esp_transfer_data(SCSIRequest *req, uint32_t len)
>                */
>                s->rregs[ESP_RINTR] |= INTR_BS | INTR_FC;
>                s->rregs[ESP_RSEQ] = SEQ_CD;
> +             esp_raise_irq(s);
>                break;
>   
>           case CMD_SELATNS | CMD_DMA:
> @@ -1022,6 +1023,7 @@ void esp_transfer_data(SCSIRequest *req, uint32_t len)
>                */
>                s->rregs[ESP_RINTR] |= INTR_BS;
>                s->rregs[ESP_RSEQ] = SEQ_MO;
> +             esp_raise_irq(s);
>                break;
>   
>           case CMD_TI | CMD_DMA:
> @@ -1032,10 +1034,9 @@ void esp_transfer_data(SCSIRequest *req, uint32_t len)
>                */
>               s->rregs[ESP_CMD] = 0;
>               s->rregs[ESP_RINTR] |= INTR_BS;
> +            esp_raise_irq(s);
>               break;

Should we log unexpected CMDs? Regardless,

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

>           }
> -
> -        esp_raise_irq(s);
>       }
>   
>       /*
Re: [PATCH v2 1/7] esp.c: only raise IRQ in esp_transfer_data() for CMD_SEL, CMD_SELATN and CMD_TI commands
Posted by Mark Cave-Ayland 4 months ago 
On 09/07/2025 12:09, Philippe Mathieu-Daudé wrote:

> On 18/6/25 08:12, Mark Cave-Ayland wrote:
>> Clarify the logic in esp_transfer_data() to ensure that the deferred interrupt code
>> can only be triggered for CMD_SEL, CMD_SELATN and CMD_TI commands. This should already
>> be the case, but make it explicit to ensure the logic isn't triggered unexpectedly.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>>   hw/scsi/esp.c | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
>> index f24991fd16..9181c8810f 100644
>> --- a/hw/scsi/esp.c
>> +++ b/hw/scsi/esp.c
>> @@ -1012,6 +1012,7 @@ void esp_transfer_data(SCSIRequest *req, uint32_t len)
>>                */
>>                s->rregs[ESP_RINTR] |= INTR_BS | INTR_FC;
>>                s->rregs[ESP_RSEQ] = SEQ_CD;
>> +             esp_raise_irq(s);
>>                break;
>>           case CMD_SELATNS | CMD_DMA:
>> @@ -1022,6 +1023,7 @@ void esp_transfer_data(SCSIRequest *req, uint32_t len)
>>                */
>>                s->rregs[ESP_RINTR] |= INTR_BS;
>>                s->rregs[ESP_RSEQ] = SEQ_MO;
>> +             esp_raise_irq(s);
>>                break;
>>           case CMD_TI | CMD_DMA:
>> @@ -1032,10 +1034,9 @@ void esp_transfer_data(SCSIRequest *req, uint32_t len)
>>                */
>>               s->rregs[ESP_CMD] = 0;
>>               s->rregs[ESP_RINTR] |= INTR_BS;
>> +            esp_raise_irq(s);
>>               break;
> 
> Should we log unexpected CMDs? Regardless,

All of the valid information transfer commands are included in the switch() statement 
above, and once patch 7 is applied any command issued in the wrong mode will generate 
an interrupt. So I think we're good.

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

Thanks!

>>           }
>> -
>> -        esp_raise_irq(s);
>>       }
>>       /*


ATB,

Mark.

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.