Re: [PATCH -v2 2/2] genericirq: change ack/mask in irq_chip to takeirq_desc instead of irq

[Thomas Gleixner]

On Wed, 24 Feb 2010, Eric W. Biederman wrote:
> Yinghai Lu <yinghai@xxxxxxxxxx> 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
     NULL

  3) convert the irq_chip implementations one by one with separate
     patches

  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.

Thanks,

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