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
->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 <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
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.
};
#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)