[lkml]   [2010]   [Mar]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
    SubjectRe: [PATCH -v2 2/2] genericirq: change ack/mask in irq_chip to take irq_desc instead of irq
    On Wed, 24 Feb 2010, Eric W. Biederman wrote:
    > Yinghai Lu <> writes:
    > > will have
    > > void (*ack)(struct irq_desc *desc);
    > > void (*mask)(struct irq_desc *desc);
    > > void (*mask_ack)(struct irq_desc *desc);
    > > void (*unmask)(struct irq_desc *desc);
    > > void (*eoi)(struct irq_desc *desc);
    > >
    > > so for sparseirq with raidix tree, we don't call extra
    > > irq_to_desc, and could use desc directly
    > Overall this looks pretty decent. This look pretty complete.
    > How many platforms did you manage to compile test this on?
    > I have found a couple of issues (see below).
    > A few times you change a bit more than is necessary which is a bit
    > spooky in a patch this far ranging.
    > Reading through this patch to review it took an uncomfortably long
    > time.

    And you didn't even catch all problem spots.

    While I like the idea, I really hate the convert wholesale approach.

    303 files changed, 3127 insertions(+), 2191 deletions(-)

    is fine for a sed script renaming action, but definitely _not_ for
    patches which require semantical changes to the affected code.

    The right way to do that is

    1) add new function pointers, which take irq_desc as their argument

    2) make the generic code use them when the old function pointer is

    3) convert the irq_chip implementations one by one with separate

    4) Remove the old function pointers and switch the generic code
    fully over to use the new ones

    Yes, it's more work, but it's less error prone and easier to review,
    as the maintainers just have to look at the patch which affects their
    particular area.



     \ /
      Last update: 2010-03-02 16:01    [W:0.022 / U:21.032 seconds]
    ©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site