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> > 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.
> };
>
> #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)
> +
> +/*
> + * 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)
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
