[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Pavel Butsykin
Subject: Re: [Qemu-devel] [PATCH 3/3] ide: really restart pending and in-flight atapi dma
Date: Tue, 12 Apr 2016 15:17:21 +0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0

On 12.04.2016 01:18, Eric Blake wrote:
On 04/06/2016 12:40 AM, Denis V. Lunev wrote:
From: Pavel Butsykin <address@hidden>

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

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

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 
          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)) {

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 {

Please keep the trailing comma, so that the next addition to this enum
won't have to adjust an existing line.



  #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.

reply via email to

[Prev in Thread] Current Thread [Next in Thread]