lkml.org 
[lkml]   [2012]   [Aug]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2012-08-24 00:01    [W:0.494 / U:0.464 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site