Re: [Qemu-devel] [PATCH 1/3] ide: don't loose pending dma state

[Pavel Butsykin <pbutsykin@virtuozzo.com>]

On 24.03.2016 14:23, Paolo Bonzini wrote:
>
>
> On 24/03/2016 10:24, Pavel Butsykin wrote:
>>> For extra points, make ide_handle_rw_error convert IDE_DMA_* to
>>> IDE_RETRY_* so that the callers only need to pass in IDE_RETRY_DMA
>>>
>> You mean to do something like that:
>>
>> ide_handle_rw_error(s, -ret, s->dma_cmd | IDE_RETRY_DMA)
>
> Just ide_handle_rw_error(s, -ret, IDE_RETRY_DMA)
>
> and in ide_handle_rw_error
>
>      if (op == IDE_RETRY_DMA) {
>          if (s->dma_cmd == IDE_DMA_READ)
>              op |= IDE_RETRY_READ;
>          else if (s->dma_cmd == IDE_DMA_TRIM)
>              op |= IDE_RETRY_TRIM;
>      }
>
> But I'm not sure anymore this is a good idea.
>
Look at the ide_sector_read_cb function. The ->dma_cmd field is used 
only for the DMA, we can't use the ide_cmd_is_read macro for PIO or
ATAPI. In fact, the ide_handle_rw_error looks like a generic function
and I do not see any good reason to move the handling of the dma_cmd
field(especially in the context of bug fixes).

> However, if it works, using dma_cmd would be better than using the dma_cb.
>
We can't use the dma_cmd field to build the ATAPI ->error_status. Your
proposal is quite logical, but please look at the 3rd patch, there to
build ->error_status we can use the same dma_cb, it is very convenient.

> Paolo
>