Re: [PATCH 03/19] x86, io_apic: Introduce x86_io_apic_ops.disable()

[Joerg Roedel]

On Thu, Aug 23, 2012 at 11:11:40PM +0200, Sebastian Andrzej Siewior wrote:
> On Mon, Aug 20, 2012 at 03:55:49PM +0200, Joerg Roedel wrote:
> What I miss here is the fact that you extract irq-mapping specific bits from
> disable_IO_APIC() and put them in a separate function named
> irq_remapping_disable_io_apic(). And having a big if in this function probably
> didn't look that sexy so you decided to use function op. Nice move, no
> question but not abvious from the description here and it took a while to figure
> it out based on the code. Well, maybe it is lateâ?¦

Well, yeah, the confusing thing with this patch is, that it does not only
introduce the x86_io_apic_ops.disable call-back but also adds the
infrastructure to change this op (and others). So the infrastructure
change should probably be in a seperate patch. But on the other side the
infrastructure change is small and the patch-set is already really
large.
Is it worth it to split that out?

> I don't want sound to picky here but the term 'modify' is bad I think. Would
> 'overwrite' fit better here? Is 'modify' used someplace else in x86 so you
> follow a common pattern here? Maybe it is just me.

Probably it's just me, but overwrite sounds like 'replace' (== overwrite
all ops with own values). But the code changes only a few of the ops
which need different behavior for interrupt remapping. I think 'modify'
describes that better, no?


	Joerg

-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

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