[PATCH 2/5] ide: ->ide_dma_clear_irq() -> ->clear_irq() - Kernel

This is a discussion on [PATCH 2/5] ide: ->ide_dma_clear_irq() -> ->clear_irq() - Kernel ; * Rename ->ide_dma_clear_irq method to ->clear_irq and move it from ide_hwif_t to struct ide_port_ops. * Move ->waiting_for_dma check inside ->clear_irq method. * Move ->dma_base check inside ->clear_irq method. piix.c: * Add ich_port_ops and remove init_hwif_ich() wrapper. There should be no ...

+ Reply to Thread
Results 1 to 6 of 6

Thread: [PATCH 2/5] ide: ->ide_dma_clear_irq() -> ->clear_irq()

  1. [PATCH 2/5] ide: ->ide_dma_clear_irq() -> ->clear_irq()

    * Rename ->ide_dma_clear_irq method to ->clear_irq
    and move it from ide_hwif_t to struct ide_port_ops.

    * Move ->waiting_for_dma check inside ->clear_irq method.

    * Move ->dma_base check inside ->clear_irq method.

    piix.c:
    * Add ich_port_ops and remove init_hwif_ich() wrapper.

    There should be no functional changes caused by this patch.

    Signed-off-by: Bartlomiej Zolnierkiewicz
    ---
    drivers/ide/ide-io.c | 15 ++++-----------
    drivers/ide/pci/piix.c | 39 +++++++++++++++++++++++----------------
    include/linux/ide.h | 4 ++--
    3 files changed, 29 insertions(+), 29 deletions(-)

    Index: b/drivers/ide/ide-io.c
    ================================================== =================
    --- a/drivers/ide/ide-io.c
    +++ b/drivers/ide/ide-io.c
    @@ -1418,23 +1418,16 @@ irqreturn_t ide_intr (int irq, void *dev
    del_timer(&hwgroup->timer);
    spin_unlock(&ide_lock);

    - /* Some controllers might set DMA INTR no matter DMA or PIO;
    - * bmdma status might need to be cleared even for
    - * PIO interrupts to prevent spurious/lost irq.
    - */
    - if (hwif->ide_dma_clear_irq && !(drive->waiting_for_dma))
    - /* ide_dma_end() needs bmdma status for error checking.
    - * So, skip clearing bmdma status here and leave it
    - * to ide_dma_end() if this is dma interrupt.
    - */
    - hwif->ide_dma_clear_irq(drive);
    + if (hwif->port_ops && hwif->port_ops->clear_irq)
    + hwif->port_ops->clear_irq(drive);

    if (drive->dev_flags & IDE_DFLAG_UNMASK)
    local_irq_enable_in_hardirq();
    +
    /* service this interrupt, may set handler for next interrupt */
    startstop = handler(drive);
    - spin_lock_irq(&ide_lock);

    + spin_lock_irq(&ide_lock);
    /*
    * Note that handler() may have set things up for another
    * interrupt to occur soon, but it cannot happen until
    Index: b/drivers/ide/pci/piix.c
    ================================================== =================
    --- a/drivers/ide/pci/piix.c
    +++ b/drivers/ide/pci/piix.c
    @@ -215,17 +215,26 @@ static unsigned int init_chipset_ich(str
    }

    /**
    - * piix_dma_clear_irq - clear BMDMA status
    - * @drive: IDE drive to clear
    + * ich_clear_irq - clear BMDMA status
    + * @drive: IDE drive
    *
    - * Called from ide_intr() for PIO interrupts
    - * to clear BMDMA status as needed by ICHx
    + * ICHx contollers set DMA INTR no matter DMA or PIO.
    + * BMDMA status might need to be cleared even for
    + * PIO interrupts to prevent spurious/lost IRQ.
    */
    -static void piix_dma_clear_irq(ide_drive_t *drive)
    +static void ich_clear_irq(ide_drive_t *drive)
    {
    ide_hwif_t *hwif = HWIF(drive);
    u8 dma_stat;

    + /*
    + * ide_dma_end() needs BMDMA status for error checking.
    + * So, skip clearing BMDMA status here and leave it
    + * to ide_dma_end() if this is DMA interrupt.
    + */
    + if (drive->waiting_for_dma || hwif->dma_base == 0)
    + return;
    +
    /* clear the INTR & ERROR bits */
    dma_stat = inb(hwif->dma_base + ATA_DMA_STATUS);
    /* Should we force the bit as well ? */
    @@ -293,21 +302,19 @@ static void __devinit init_hwif_piix(ide
    hwif->ultra_mask = hwif->mwdma_mask = hwif->swdma_mask = 0;
    }

    -static void __devinit init_hwif_ich(ide_hwif_t *hwif)
    -{
    - init_hwif_piix(hwif);
    -
    - /* ICHx need to clear the BMDMA status for all interrupts */
    - if (hwif->dma_base)
    - hwif->ide_dma_clear_irq = &piix_dma_clear_irq;
    -}
    -
    static const struct ide_port_ops piix_port_ops = {
    .set_pio_mode = piix_set_pio_mode,
    .set_dma_mode = piix_set_dma_mode,
    .cable_detect = piix_cable_detect,
    };

    +static const struct ide_port_ops ich_port_ops = {
    + .set_pio_mode = piix_set_pio_mode,
    + .set_dma_mode = piix_set_dma_mode,
    + .clear_irq = ich_clear_irq,
    + .cable_detect = piix_cable_detect,
    +};
    +
    #ifndef CONFIG_IA64
    #define IDE_HFLAGS_PIIX IDE_HFLAG_LEGACY_IRQS
    #else
    @@ -331,9 +338,9 @@ static const struct ide_port_ops piix_po
    { \
    .name = DRV_NAME, \
    .init_chipset = init_chipset_ich, \
    - .init_hwif = init_hwif_ich, \
    + .init_hwif = init_hwif_piix, \
    .enablebits = {{0x41,0x80,0x80}, {0x43,0x80,0x80}}, \
    - .port_ops = &piix_port_ops, \
    + .port_ops = &ich_port_ops, \
    .host_flags = IDE_HFLAGS_PIIX, \
    .pio_mask = ATA_PIO4, \
    .swdma_mask = ATA_SWDMA2_ONLY, \
    Index: b/include/linux/ide.h
    ================================================== =================
    --- a/include/linux/ide.h
    +++ b/include/linux/ide.h
    @@ -702,6 +702,7 @@ extern const struct ide_tp_ops default_t
    * @resetproc: routine to reset controller after a disk reset
    * @maskproc: special host masking for drive selection
    * @quirkproc: check host's drive quirk list
    + * @clear_irq: clear IRQ
    *
    * @mdma_filter: filter MDMA modes
    * @udma_filter: filter UDMA modes
    @@ -718,6 +719,7 @@ struct ide_port_ops {
    void (*resetproc)(ide_drive_t *);
    void (*maskproc)(ide_drive_t *, int);
    void (*quirkproc)(ide_drive_t *);
    + void (*clear_irq)(ide_drive_t *);

    u8 (*mdma_filter)(ide_drive_t *);
    u8 (*udma_filter)(ide_drive_t *);
    @@ -780,8 +782,6 @@ typedef struct hwif_s {
    const struct ide_port_ops *port_ops;
    const struct ide_dma_ops *dma_ops;

    - void (*ide_dma_clear_irq)(ide_drive_t *drive);
    -
    /* dma physical region descriptor table (cpu view) */
    unsigned int *dmatable_cpu;
    /* dma physical region descriptor table (dma view) */
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  2. Re: [PATCH 2/5] ide: ->ide_dma_clear_irq() -> ->clear_irq()

    Hello.

    Bartlomiej Zolnierkiewicz wrote:

    > * Rename ->ide_dma_clear_irq method to ->clear_irq
    > and move it from ide_hwif_t to struct ide_port_ops.
    >
    > * Move ->waiting_for_dma check inside ->clear_irq method.
    >
    > * Move ->dma_base check inside ->clear_irq method.
    >
    > piix.c:
    > * Add ich_port_ops and remove init_hwif_ich() wrapper.
    >
    > There should be no functional changes caused by this patch.
    >


    Good. I think it's worth implementing this method in at least
    cmd64x.c which actually reads the IDE interrupt latch bits (independent
    from the DMA interrupt status) in the dma_test_irq() methods but never
    clears them, so the latches may reflect a non-current state of the IDE
    interrupt...
    It may also be worth considering turning this method into
    test-and-clear, so that we can get the actual IDE interrupt state on the
    chips that implement this...

    > Signed-off-by: Bartlomiej Zolnierkiewicz


    Acked-by: Sergei Shtylyov

    MBR, Sergei


    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  3. Re: [PATCH 2/5] ide: ->ide_dma_clear_irq() -> ->clear_irq()

    Hello, I wrote:

    >> * Rename ->ide_dma_clear_irq method to ->clear_irq
    >> and move it from ide_hwif_t to struct ide_port_ops.
    >>
    >> * Move ->waiting_for_dma check inside ->clear_irq method.
    >>
    >> * Move ->dma_base check inside ->clear_irq method.
    >>
    >> piix.c:
    >> * Add ich_port_ops and remove init_hwif_ich() wrapper.
    >>
    >> There should be no functional changes caused by this patch.
    >>

    >
    > Good. I think it's worth implementing this method in at least
    > cmd64x.c which actually reads the IDE interrupt latch bits
    > (independent from the DMA interrupt status) in the dma_test_irq()
    > methods but never clears them, so the latches may reflect a
    > non-current state of the IDE interrupt...


    I forgot that it does clear them in its dma_end() methods (which I
    myself have reworked :-).
    It seems however that at least for SFF-8038 compatibles, it makes
    sense to leave it that way since INTRQ might be asserted while BMIDE
    interrupt bit is not, so the interrupt latch would need clearing even on
    DMA timeout...

    > It may also be worth considering turning this method into
    > test-and-clear, so that we can get the actual IDE interrupt state on
    > the chips that implement this...


    Probably might add the test_irq() method to be called on
    !hwif->waiting_for_dma. Cleraing the status at once seems impractical...

    >> Signed-off-by: Bartlomiej Zolnierkiewicz

    >
    > Acked-by: Sergei Shtylyov


    Not feeling sure about this patch -- ->waiting_for_dma probably
    should've been left where it was...

    MBR, Sergei


    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  4. Re: [PATCH 2/5] ide: ->ide_dma_clear_irq() -> ->clear_irq()

    On Monday 15 September 2008 15:11:21 Sergei Shtylyov wrote:
    > Hello, I wrote:
    >
    > >> * Rename ->ide_dma_clear_irq method to ->clear_irq
    > >> and move it from ide_hwif_t to struct ide_port_ops.
    > >>
    > >> * Move ->waiting_for_dma check inside ->clear_irq method.
    > >>
    > >> * Move ->dma_base check inside ->clear_irq method.
    > >>
    > >> piix.c:
    > >> * Add ich_port_ops and remove init_hwif_ich() wrapper.
    > >>
    > >> There should be no functional changes caused by this patch.
    > >>

    > >
    > > Good. I think it's worth implementing this method in at least
    > > cmd64x.c which actually reads the IDE interrupt latch bits
    > > (independent from the DMA interrupt status) in the dma_test_irq()
    > > methods but never clears them, so the latches may reflect a
    > > non-current state of the IDE interrupt...

    >
    > I forgot that it does clear them in its dma_end() methods (which I
    > myself have reworked :-).
    > It seems however that at least for SFF-8038 compatibles, it makes
    > sense to leave it that way since INTRQ might be asserted while BMIDE
    > interrupt bit is not, so the interrupt latch would need clearing even on
    > DMA timeout...
    >
    > > It may also be worth considering turning this method into
    > > test-and-clear, so that we can get the actual IDE interrupt state on
    > > the chips that implement this...

    >
    > Probably might add the test_irq() method to be called on
    > !hwif->waiting_for_dma. Cleraing the status at once seems impractical...


    Or we can test for ->waiting_for_dma inside ->test_irq.

    > >> Signed-off-by: Bartlomiej Zolnierkiewicz

    > >
    > > Acked-by: Sergei Shtylyov

    >
    > Not feeling sure about this patch -- ->waiting_for_dma probably
    > should've been left where it was...


    Well, it doesn't change behavior and I think having ->clear_irq method
    independent from the transfer mode is a preffered approach.

    Thanks,
    Bart
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  5. Re: [PATCH 2/5] ide: ->ide_dma_clear_irq() -> ->clear_irq()

    Hello.

    Bartlomiej Zolnierkiewicz wrote:

    >>>> * Rename ->ide_dma_clear_irq method to ->clear_irq
    >>>> and move it from ide_hwif_t to struct ide_port_ops.
    >>>>
    >>>> * Move ->waiting_for_dma check inside ->clear_irq method.
    >>>>
    >>>> * Move ->dma_base check inside ->clear_irq method.
    >>>>
    >>>> piix.c:
    >>>> * Add ich_port_ops and remove init_hwif_ich() wrapper.
    >>>>
    >>>> There should be no functional changes caused by this patch.
    >>>>
    >>>>
    >>> Good. I think it's worth implementing this method in at least
    >>> cmd64x.c which actually reads the IDE interrupt latch bits
    >>> (independent from the DMA interrupt status) in the dma_test_irq()
    >>> methods but never clears them, so the latches may reflect a
    >>> non-current state of the IDE interrupt...
    >>>

    >> I forgot that it does clear them in its dma_end() methods (which I
    >> myself have reworked :-).
    >> It seems however that at least for SFF-8038 compatibles, it makes
    >> sense to leave it that way since INTRQ might be asserted while BMIDE
    >> interrupt bit is not, so the interrupt latch would need clearing even on
    >> DMA timeout...
    >>
    >>> It may also be worth considering turning this method into
    >>> test-and-clear, so that we can get the actual IDE interrupt state on
    >>> the chips that implement this...
    >>>

    >> Probably might add the test_irq() method to be called on
    >> !hwif->waiting_for_dma. Cleraing the status at once seems impractical...
    >>

    >
    > Or we can test for ->waiting_for_dma inside ->test_irq.
    >


    That also will do...

    >>>> Signed-off-by: Bartlomiej Zolnierkiewicz
    >>>>
    >>> Acked-by: Sergei Shtylyov
    >>>

    >> Not feeling sure about this patch -- ->waiting_for_dma probably
    >> should've been left where it was...
    >>

    >
    > Well, it doesn't change behavior and I think having ->clear_irq method
    > independent from the transfer mode is a preffered approach.
    >


    But its implementations will have to depend on it anyway. And
    clearing the IDE interrupt in general already depends on the transfer
    mode -- the BMIDE interrupt which is a (delayed) reflection of INTRQ is
    cleared implicitly by the dma_end() method -- except in at least sgiioc4
    driver.
    BTW, sgiioc4 seems another candidate for clear_irq() implementation
    -- currently clearing is done implicitly by the read_status() method (I
    don't quite understand why it clears DMA error interrupt there).
    However, since there's no documentation, I'm not sure how the IDE
    interrupt is latched by IOC4.

    > Thanks,
    > Bart
    >


    MBR, Sergei


    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  6. Re: [PATCH 2/5] ide: ->ide_dma_clear_irq() -> ->clear_irq()

    On Tue, Sep 16, 2008 at 01:16:54PM +0400, Sergei Shtylyov wrote:
    > >>>>Signed-off-by: Bartlomiej Zolnierkiewicz
    > >>>>
    > >>>Acked-by: Sergei Shtylyov
    > >>>
    > >> Not feeling sure about this patch -- ->waiting_for_dma probably
    > >>should've been left where it was...
    > >>

    > >
    > >Well, it doesn't change behavior and I think having ->clear_irq method
    > >independent from the transfer mode is a preffered approach.
    > >

    >
    > But its implementations will have to depend on it anyway. And
    > clearing the IDE interrupt in general already depends on the transfer
    > mode -- the BMIDE interrupt which is a (delayed) reflection of INTRQ is
    > cleared implicitly by the dma_end() method -- except in at least sgiioc4
    > driver.
    > BTW, sgiioc4 seems another candidate for clear_irq() implementation
    > -- currently clearing is done implicitly by the read_status() method (I
    > don't quite understand why it clears DMA error interrupt there).
    > However, since there's no documentation, I'm not sure how the IDE
    > interrupt is latched by IOC4.


    The person who originally wrote sgiioc4 has been gone for a long time.

    I'm guessing that the DMA error is cleared there, because any DMA error
    status has already been processed above, and it's easier to clear
    unconditionally than to test and clear, and there's no ill effect to
    clearing something that's already clear.

    jeremy
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

+ Reply to Thread