Re: [PATCH] KVM: Unregister IRQ ACK notifier with in-kernel irqchip

[Sheng Yang <sheng@linux.intel.com>]

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 <sheng@linux.intel.com>
> > > ---
> > >  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. :)
>
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. 

I will update the patch to say we need in kernel irqchip when call the 
function explicitly. 

Thanks!
--
regards
Yang, Sheng