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

[Pavel Butsykin]

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