linux-kernel - Re: [PATCH -v2 2/2] genericirq: change ack/mask in irq_chip to take irq

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 2 Mar 2010 15:58:26 +0100 (CET)
From:	Thomas Gleixner <tglx@...utronix.de>
To:	"Eric W. Biederman" <ebiederm@...ssion.com>
cc:	Yinghai Lu <yinghai@...nel.org>, Ingo Molnar <mingo@...e.hu>,
	"H. Peter Anvin" <hpa@...or.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Rusty Russell <rusty@...tcorp.com.au>,
	Suresh Siddha <suresh.b.siddha@...el.com>,
	LKML <linux-kernel@...r.kernel.org>,
	Jeremy Fitzhardinge <jeremy@...p.org>,
	linux-arch@...r.kernel.org
Subject: Re: [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 <yinghai@...nel.org> 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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/