linux-kernel - Re: [PATCH 03/19] x86, io_apic: Introduce x86_io_apic

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 for Android: free password hash cracker in your pocket

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

Message-ID: <20120823211140.GB7120@breakpoint.cc>
Date:	Thu, 23 Aug 2012 23:11:40 +0200
From:	Sebastian Andrzej Siewior <sebastian@...akpoint.cc>
To:	Joerg Roedel <joerg.roedel@....com>
Cc:	x86@...nel.org, linux-kernel@...r.kernel.org, joro@...tes.org,
	Suresh Siddha <suresh.b.siddha@...el.com>,
	Yinghai Lu <yinghai@...nel.org>
Subject: Re: [PATCH 03/19] x86, io_apic: Introduce x86_io_apic_ops.disable()

On Mon, Aug 20, 2012 at 03:55:49PM +0200, Joerg Roedel wrote:
> This function pointer is used to call a system-specific
> function for disabling the IO-APIC. Currently this is used
> for IRQ remapping which has its own disable routine.

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â¦

> diff --git a/drivers/iommu/irq_remapping.c b/drivers/iommu/irq_remapping.c
> index 283a43f..f72a5e8 100644
> --- a/drivers/iommu/irq_remapping.c
> +++ b/drivers/iommu/irq_remapping.c
> @@ -17,6 +21,24 @@ int no_x2apic_optout;
<snip>
> +static void __init irq_remapping_modify_x86_ops(void)

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.

> +{
> +	x86_io_apic_ops.disable	=	irq_remapping_disable_io_apic;
> +}
> +

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