* On Wednesday 08 Oct 2008 14:34:05 Sheng Yang wrote: > On Wednesday 08 October 2008 16:54:18 Sheng Yang wrote: > > On Wednesday 08 October 2008 15:08:52 Amit Shah wrote: > > > * On Wednesday 08 Oct 2008 12:09:20 Sheng Yang wrote: > > > > Signed-off-by: Sheng Yang <[EMAIL PROTECTED]> > > > > --- > > > > arch/x86/kvm/x86.c | 4 +++- > > > > 1 files changed, 3 insertions(+), 1 deletions(-) > > > > > > > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > > > > index 675fcc1..c5763d7 100644 > > > > --- a/arch/x86/kvm/x86.c > > > > +++ b/arch/x86/kvm/x86.c > > > > @@ -176,7 +176,9 @@ static void kvm_free_assigned_device(struct kvm > > > > *kvm, if (irqchip_in_kernel(kvm) && assigned_dev->irq_requested) > > > > free_irq(assigned_dev->host_irq, (void *)assigned_dev); > > > > > > > > - kvm_unregister_irq_ack_notifier(kvm, > > > > &assigned_dev->ack_notifier); > > > > + if (irqchip_in_kernel(kvm)) > > > > + kvm_unregister_irq_ack_notifier(kvm, > > > > + &assigned_dev->ack_notifier); > > > > > > The unregister API should perform the check whether the said notifier > > > exists so this shouldn't be necessary. > > > > Yeah, that's more reasonable. But now I just see, > > kvm_register_irq_ack_notifier() go with irqchip_in_kernel() and > > unregister didn't. :)
Yes, because if we don't use the irqchip in the kernel, we don't need it at all. However, there's no need to special-case the unregister path as you note below. > Um... After consider a little more, I think keep it unwrapped by > irqchip_in_kernel() may be a little more reasonable. The reason is we just > register kvm_register_irq_ack_notifier() when we have in kernel irqchip. If > we don't have in kernel irqchip, we shouldn't call them and try to perform > this action. Call the function without in-kernel irqchip is wrong, ensure > the exist of in-kernel irqchip is the caller's responsibility. What we can > do is add a BUG_ON() or ASSERT() to the function, rather let the function > tell if itself need in-kernel irqchip. We ensure our notifier is registered only in the in-kernel irqchip case. Calling the unregister function in all the cases, though, should be fine I think. BUG_ON or ASSERT are conditions used to trap hardware bugs or unexpected paths in software. This doesn't qualify for that. To make the code symmetrical, it might make sense to have the unregister path in the kernel-irqchip case, but I really don't think it's necessary and doesn't hurt readability as well. Also, the lesser the conditionals we have the better. Amit -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
