Jan Beulich
2009-Apr-01 09:33 UTC
[Xen-devel] [PATCH] fix irq_vector[] update of c/s 19419
The updating of irq_vector[] looks very suspicious - the array is IRQ-
indexed, hence you potentially overwrite an already existing entry, or
risk your entry to be overwritten later.
Also, what is the reason for overwriting the vector_irq[] initialization
assign_irq_vector() already did?
Likewise you should be calling free_irq_vector() in the error path rather
than doing any of the updates yourself.
Finally, assign_irq_vector() does not return zero on failure, but a negative
error code.
If there is nothing I''m overlooking, here''s a patch -
untested, as I don''t
have the needed hardware.
Signed-off-by: Jan Beulich <jbeulich@novell.com>
--- 2009-03-27.orig/xen/arch/x86/hpet.c 2009-03-24 09:04:02.000000000 +0100
+++ 2009-03-27/xen/arch/x86/hpet.c 2009-04-01 11:28:25.000000000 +0200
@@ -346,17 +346,14 @@ static int hpet_assign_irq(struct hpet_e
unsigned int vector;
vector = assign_irq_vector(AUTO_ASSIGN_IRQ);
- if ( !vector )
- return -EINVAL;
+ if ( (int)vector < 0 )
+ return vector;
- irq_vector[vector] = vector;
- vector_irq[vector] = vector;
vector_channel[vector] = ch - &hpet_events[0];
if ( hpet_setup_msi_irq(vector) )
{
- irq_vector[vector] = 0;
- vector_irq[vector] = FREE_TO_ASSIGN_IRQ;
+ free_irq_vector(vector);
vector_channel[vector] = -1;
return -EINVAL;
}
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Wei, Gang
2009-Apr-01 10:50 UTC
[Xen-devel] RE: [PATCH] fix irq_vector[] update of c/s 19419
All things you found are absolutely right. I used to think some code paths may need a vector to irq converting, but I can''t find a way to dynamically assign a irq, so I did the assignment manually. Careful checking shows it is not necessary, so your correction should be ok. As to the assign_irq_vector() return code, it should be my incaution. Thanks for this correction patch. Jimmy On Wednesday, April 01, 2009 5:34 PM, Jan Beulich wrote:> The updating of irq_vector[] looks very suspicious - the array is IRQ- > indexed, hence you potentially overwrite an already existing entry, or > risk your entry to be overwritten later. > > Also, what is the reason for overwriting the vector_irq[] initialization > assign_irq_vector() already did? > > Likewise you should be calling free_irq_vector() in the error path rather > than doing any of the updates yourself. > > Finally, assign_irq_vector() does not return zero on failure, but a negative > error code. > > If there is nothing I''m overlooking, here''s a patch - untested, as I don''t > have the needed hardware. > > Signed-off-by: Jan Beulich <jbeulich@novell.com> > > --- 2009-03-27.orig/xen/arch/x86/hpet.c 2009-03-24 09:04:02.000000000 +0100 > +++ 2009-03-27/xen/arch/x86/hpet.c 2009-04-01 11:28:25.000000000 +0200 > @@ -346,17 +346,14 @@ static int hpet_assign_irq(struct hpet_e > unsigned int vector; > > vector = assign_irq_vector(AUTO_ASSIGN_IRQ); > - if ( !vector ) > - return -EINVAL; > + if ( (int)vector < 0 ) > + return vector; > > - irq_vector[vector] = vector; > - vector_irq[vector] = vector; > vector_channel[vector] = ch - &hpet_events[0]; > > if ( hpet_setup_msi_irq(vector) ) > { > - irq_vector[vector] = 0; > - vector_irq[vector] = FREE_TO_ASSIGN_IRQ; > + free_irq_vector(vector); > vector_channel[vector] = -1; > return -EINVAL; > }_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel