On 09/07/2025 12:14, Philippe Mathieu-Daudé wrote:
> On 18/6/25 08:12, Mark Cave-Ayland wrote:
>> In the cases where mixed DMA/non-DMA transfers are used or no data is
>> available, it is possible to for the calculated transfer length to be
>> zero. Only call the dma_memory_write function where the transfer length
>> is non-zero to avoid invoking the DMA engine for a zero length transfer
>> which can have side-effects (along with generating additional tracing
>> noise).
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>> hw/scsi/esp.c | 6 +++++-
>> 1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
>> index ec9fcbeddf..1c7bad8fc0 100644
>> --- a/hw/scsi/esp.c
>> +++ b/hw/scsi/esp.c
>> @@ -631,7 +631,9 @@ static void esp_do_dma(ESPState *s)
>> switch (s->rregs[ESP_CMD]) {
>> case CMD_TI | CMD_DMA:
>> if (s->dma_memory_write) {
>> - s->dma_memory_write(s->dma_opaque, s->async_buf, len);
>> + if (len) {
>> + s->dma_memory_write(s->dma_opaque, s->async_buf, len);
>> + }
>> } else {
>> /* Copy device data to FIFO */
>> len = MIN(len, fifo8_num_free(&s->fifo));
>> @@ -681,6 +683,7 @@ static void esp_do_dma(ESPState *s)
>
> As future cleanup, indent could be simplified using 'if (!len) break;'.
I remember when I did the large rewrite of esp.c, I found the logic easier to follow
with the indentation that way. But of course that is something that could always be
revisited later if required.
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>
>> buf[0] = s->status;
>> if (s->dma_memory_write) {
>> + /* Length already non-zero */
>> s->dma_memory_write(s->dma_opaque, buf, len);
>> } else {
>> esp_fifo_push_buf(s, buf, len);
>> @@ -715,6 +718,7 @@ static void esp_do_dma(ESPState *s)
>> buf[0] = 0;
>> if (s->dma_memory_write) {
>> + /* Length already non-zero */
>> s->dma_memory_write(s->dma_opaque, buf, len);
>> } else {
>> esp_fifo_push_buf(s, buf, len);
ATB,
Mark.