On Wednesday 08 October 2008 17:20:20 Amit Shah wrote: > * 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.
Yeah, so register should got some ASSERT. > > 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. OK. Seems we need a NULL judgement of unregister to keep it safe. -- regards Yang, Sheng > > 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
