On 04/12/2016 08:17 AM, Pavel Butsykin wrote:
> On 12.04.2016 01:18, Eric Blake wrote:
>> On 04/06/2016 12:40 AM, Denis V. Lunev wrote:
>>> From: Pavel Butsykin <pbutsy...@virtuozzo.com>
>>>
>>> Restart of ATAPI DMA used to be unreachable, because the request to do
>>> so wasn't indicated in bus->error_status due to the lack of spare
>>> bits, and
>>> ide_restart_bh() would return early doing nothing.
>>>
>>> This patch makes use of the observation that not all bit combinations
>>> were
>>> possible in ->error_status. In particular, IDE_RETRY_READ only made
>>> sense
>>> together with IDE_RETRY_DMA or IDE_RETRY_PIO. This allows to re-use
>>> IDE_RETRY_READ alone as an indicator of ATAPI DMA restart request.
>>>
>>> To makes things more uniform, ATAPI DMA gets its own value for
>>> ->dma_cmd.
>>> As a means against confusion, macros are added to test the state of
>>> ->error_status.
>>>
>>> The patch fixes the restart of both in-flight and pending ATAPI DMA,
>>> following the scheme similar to that of IDE DMA.
>>>
>>> Signed-off-by: Pavel Butsykin <pbutsy...@virtuozzo.com>>
and these seem prone to false positives; where it might be better to do:
>>> Signed-off-by: Denis V. Lunev <d...@openvz.org>
>>> ---
>>
>> I'll leave the technical feasibility of this to others, but have some
>> coding style comments:
>>
>>
>>> @@ -783,8 +782,10 @@ static int ide_handle_rw_error(IDEState *s, int
>>> error, int op)
>>> s->bus->error_status = op;
>>> } else if (action == BLOCK_ERROR_ACTION_REPORT) {
>>> block_acct_failed(blk_get_stats(s->blk), &s->acct);
>>> - if (op & IDE_RETRY_DMA) {
>>> + if (IS_IDE_RETRY_DMA(op)) {
>>> ide_dma_error(s);
>>
>> I'd probably have split this into two patches; one adding the accessor
>> macros for existing access, and the other adding the new bit pattern
>> (mixing a conversion along with new code is a bit trickier to review in
>> one patch).
>>
>>
>>> +++ b/hw/ide/internal.h
>>> @@ -338,6 +338,7 @@ enum ide_dma_cmd {
>>> IDE_DMA_READ,
>>> IDE_DMA_WRITE,
>>> IDE_DMA_TRIM,
>>> + IDE_DMA_ATAPI
>>
>> Please keep the trailing comma, so that the next addition to this enum
>> won't have to adjust an existing line.
>>
> ok.
>
>>> };
>>>
>>> #define ide_cmd_is_read(s) \
>>> @@ -508,11 +509,27 @@ struct IDEDevice {
>>> /* These are used for the error_status field of IDEBus */
>>> #define IDE_RETRY_DMA 0x08
>>> #define IDE_RETRY_PIO 0x10
>>> +#define IDE_RETRY_ATAPI 0x20 /* reused IDE_RETRY_READ bit */
>>> #define IDE_RETRY_READ 0x20
>>> #define IDE_RETRY_FLUSH 0x40
>>> #define IDE_RETRY_TRIM 0x80
>>> #define IDE_RETRY_HBA 0x100
>>
>> Seems rather sparse on the comments about which bit patterns are valid
>> together. If IDE_RETRY_READ is always used with at least one other bit,
>> it might make more sense to have an IDE_RETRY_MASK that selects the set
>> of bits being multiplexed, and/or macros that define the bits used in
>> combination. Something along the lines of:
>>
>> #define IDE_RETRY_MASK 0x38
>> #define IDE_RETRY_READ_DMA 0x28
>> #define IDE_RETRY_READ_PIO 0x30
>> #define IDE_RETRY_ATAPI 0x20
>>
>>>
>>> +#define IS_IDE_RETRY_DMA(_status) \
>>> + ((_status) & IDE_RETRY_DMA)
>>> +
>>> +#define IS_IDE_RETRY_PIO(_status) \
>>> + ((_status) & IDE_RETRY_PIO)
>>
>> and these seem prone to false positives; where it might be better to do:
>>
>> #define IS_IDE_RETRY_DMA(_status) \
>> (((_status) & IDE_RETRY_MASK) == IDE_RETRY_READ_DMA)
>>
> This is not entirely true, because IDE_RETRY_DMA can be used for READ or
> WRITE operation.
>
>>> +
>>> +/*
>>> + * The method of the IDE_RETRY_ATAPI determination is to use a
>>> previously
>>> + * impossible bit combination as a new status value.
>>> + */
>>> +#define IS_IDE_RETRY_ATAPI(_status) \
>>> + (((_status) & IDE_RETRY_ATAPI) && \
>>> + !IS_IDE_RETRY_DMA(_status) && \
>>> + !IS_IDE_RETRY_PIO(_status))
>>> +
>>
>> And this evaluates _status more than once, compared to:
>>
>> #define IS_IDE_RETRY_ATAPI(_status) \
>> (((_status) & IDE_RETRY_MASK) == IDE_RETRY_ATAPI)
>>
>>
> Yes, it looks much nicer. I can make the change as a follow-up patch.
>
I can squash the patch in staging.
Thanks,
--js
