From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58738) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1apxUT-0006eZ-6b for qemu-devel@nongnu.org; Tue, 12 Apr 2016 08:32:22 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1apxUN-0000HU-0q for qemu-devel@nongnu.org; Tue, 12 Apr 2016 08:32:21 -0400 Received: from mail-db3on0118.outbound.protection.outlook.com ([157.55.234.118]:17193 helo=emea01-db3-obe.outbound.protection.outlook.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1apxUM-0000Gk-Gk for qemu-devel@nongnu.org; Tue, 12 Apr 2016 08:32:14 -0400 References: <1459924806-306-1-git-send-email-den@openvz.org> <1459924806-306-4-git-send-email-den@openvz.org> <570C22D2.8060401@redhat.com> From: Pavel Butsykin Message-ID: <570CE751.4050401@virtuozzo.com> Date: Tue, 12 Apr 2016 15:17:21 +0300 MIME-Version: 1.0 In-Reply-To: <570C22D2.8060401@redhat.com> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 3/3] ide: really restart pending and in-flight atapi dma List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake , "Denis V. Lunev" , qemu-devel@nongnu.org On 12.04.2016 01:18, Eric Blake wrote: > On 04/06/2016 12:40 AM, Denis V. Lunev wrote: >> From: Pavel Butsykin >> >> 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 >> Signed-off-by: Denis V. Lunev >> --- > > 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.