Re: [Qemu-devel] [PATCH 3/3] ide: really restart pending and in-flight atapi dma

From: John Snow <jsnow@redhat.com>
To: Pavel Butsykin <pbutsykin@virtuozzo.com>,
	Eric Blake <eblake@redhat.com>, "Denis V. Lunev" <den@openvz.org>,
	qemu-devel@nongnu.org
Subject: Re: [Qemu-devel] [PATCH 3/3] ide: really restart pending and in-flight atapi dma
Date: Tue, 12 Apr 2016 12:47:23 -0400	[thread overview]
Message-ID: <570D269B.4010802@redhat.com> (raw)
In-Reply-To: <570CE751.4050401@virtuozzo.com>

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 <pbutsykin@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 <pbutsykin@virtuozzo.com>>

and these seem prone to false positives; where it might be better to do:

>>> Signed-off-by: Denis V. Lunev <den@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