| Message ID | 5CDE927002000078002300BA@prv1-mh.provo.novell.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86: IRQ management adjustments | expand |
On Fri, May 17, 2019 at 04:52:32AM -0600, Jan Beulich wrote: > Use valid_irq_vector() rather than "> 0". > > Also replace an open-coded use of IRQ_VECTOR_UNASSIGNED. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> The question I have below is not directly related to the usage of valid_irq_vector, but rather with the existing code. > --- > v3: New. > > --- a/xen/arch/x86/irq.c > +++ b/xen/arch/x86/irq.c > @@ -342,7 +342,7 @@ void clear_irq_vector(int irq) > > int irq_to_vector(int irq) > { > - int vector = -1; > + int vector = IRQ_VECTOR_UNASSIGNED; > > BUG_ON(irq >= nr_irqs || irq < 0); > > @@ -452,15 +452,18 @@ static vmask_t *irq_get_used_vector_mask > int vector; > > vector = irq_to_vector(irq); > - if ( vector > 0 ) > + if ( valid_irq_vector(vector) ) > { > - printk(XENLOG_INFO "IRQ %d already assigned vector %d\n", > + printk(XENLOG_INFO "IRQ%d already assigned vector %02x\n", > irq, vector); > > ASSERT(!test_bit(vector, ret)); > > set_bit(vector, ret); > } > + else if ( vector != IRQ_VECTOR_UNASSIGNED ) > + printk(XENLOG_WARNING "IRQ%d mapped to bogus vector %02x\n", > + irq, vector); Maybe add an assert_unreachable here? It seems really bogus to call irq_get_used_vector_mask with an unassigned vector. But I'm not sure I fully understand this piece of code, neither why a vector without a IRQ assigned can have a vector assigned. Is this covering up for the lack of cleanup elsewhere? Thanks, Roger.
>>> On 20.05.19 at 16:04, <roger.pau@citrix.com> wrote: > On Fri, May 17, 2019 at 04:52:32AM -0600, Jan Beulich wrote: >> Use valid_irq_vector() rather than "> 0". >> >> Also replace an open-coded use of IRQ_VECTOR_UNASSIGNED. >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > > Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> Thanks. > The question I have below is not directly related to the usage of > valid_irq_vector, but rather with the existing code. > >> @@ -452,15 +452,18 @@ static vmask_t *irq_get_used_vector_mask >> int vector; >> >> vector = irq_to_vector(irq); >> - if ( vector > 0 ) >> + if ( valid_irq_vector(vector) ) >> { >> - printk(XENLOG_INFO "IRQ %d already assigned vector %d\n", >> + printk(XENLOG_INFO "IRQ%d already assigned vector %02x\n", >> irq, vector); >> >> ASSERT(!test_bit(vector, ret)); >> >> set_bit(vector, ret); >> } >> + else if ( vector != IRQ_VECTOR_UNASSIGNED ) >> + printk(XENLOG_WARNING "IRQ%d mapped to bogus vector %02x\n", >> + irq, vector); > > Maybe add an assert_unreachable here? It seems really bogus to call > irq_get_used_vector_mask with an unassigned vector. How that? This would e.g. get called the very first time a vector is to be assigned. But I'm afraid I'm a little confused anyway by the wording you use - after all this is the code path dealing with an IRQ _not_ being marked as having no vector assigned, but also not having a valid vector. > But I'm not sure I fully understand this piece of code, neither why a > vector without a IRQ assigned can have a vector assigned. Is this > covering up for the lack of cleanup elsewhere? I don't think so, no. However, users of irq_to_vector() need to be careful: The function can legitimately return 0 (besides IRQ_VECTOR_UNASSIGNED) as an error indication. I've tried to do away with this, but quickly realized I'd better not do so. I've not seen the printk() trigger, but I'd rather see the printk() log a message telling us that we also need to exclude vector 0 than a wrong assertion to fire. Jan
On Mon, May 20, 2019 at 09:26:37AM -0600, Jan Beulich wrote: > >>> On 20.05.19 at 16:04, <roger.pau@citrix.com> wrote: > > On Fri, May 17, 2019 at 04:52:32AM -0600, Jan Beulich wrote: > >> Use valid_irq_vector() rather than "> 0". > >> > >> Also replace an open-coded use of IRQ_VECTOR_UNASSIGNED. > >> > >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > > > > Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> > > Thanks. > > > The question I have below is not directly related to the usage of > > valid_irq_vector, but rather with the existing code. > > > >> @@ -452,15 +452,18 @@ static vmask_t *irq_get_used_vector_mask > >> int vector; > >> > >> vector = irq_to_vector(irq); > >> - if ( vector > 0 ) > >> + if ( valid_irq_vector(vector) ) > >> { > >> - printk(XENLOG_INFO "IRQ %d already assigned vector %d\n", > >> + printk(XENLOG_INFO "IRQ%d already assigned vector %02x\n", > >> irq, vector); > >> > >> ASSERT(!test_bit(vector, ret)); > >> > >> set_bit(vector, ret); > >> } > >> + else if ( vector != IRQ_VECTOR_UNASSIGNED ) > >> + printk(XENLOG_WARNING "IRQ%d mapped to bogus vector %02x\n", > >> + irq, vector); > > > > Maybe add an assert_unreachable here? It seems really bogus to call > > irq_get_used_vector_mask with an unassigned vector. > > How that? This would e.g. get called the very first time a vector > is to be assigned. But I'm afraid I'm a little confused anyway by > the wording you use - after all this is the code path dealing with > an IRQ _not_ being marked as having no vector assigned, but > also not having a valid vector. Thanks for the clarification, by the name of the function I assumed it must be called with an irq that has a vector assigned, if that's not the case then I think it's fine. Roger.
>>> On 22.05.19 at 18:42, <roger.pau@citrix.com> wrote: > On Mon, May 20, 2019 at 09:26:37AM -0600, Jan Beulich wrote: >> >>> On 20.05.19 at 16:04, <roger.pau@citrix.com> wrote: >> > On Fri, May 17, 2019 at 04:52:32AM -0600, Jan Beulich wrote: >> >> @@ -452,15 +452,18 @@ static vmask_t *irq_get_used_vector_mask >> >> int vector; >> >> >> >> vector = irq_to_vector(irq); >> >> - if ( vector > 0 ) >> >> + if ( valid_irq_vector(vector) ) >> >> { >> >> - printk(XENLOG_INFO "IRQ %d already assigned vector %d\n", >> >> + printk(XENLOG_INFO "IRQ%d already assigned vector %02x\n", >> >> irq, vector); >> >> >> >> ASSERT(!test_bit(vector, ret)); >> >> >> >> set_bit(vector, ret); >> >> } >> >> + else if ( vector != IRQ_VECTOR_UNASSIGNED ) >> >> + printk(XENLOG_WARNING "IRQ%d mapped to bogus vector %02x\n", >> >> + irq, vector); >> > >> > Maybe add an assert_unreachable here? It seems really bogus to call >> > irq_get_used_vector_mask with an unassigned vector. >> >> How that? This would e.g. get called the very first time a vector >> is to be assigned. But I'm afraid I'm a little confused anyway by >> the wording you use - after all this is the code path dealing with >> an IRQ _not_ being marked as having no vector assigned, but >> also not having a valid vector. > > Thanks for the clarification, by the name of the function I assumed it > must be called with an irq that has a vector assigned, if that's not > the case then I think it's fine. > > Roger. Well, the names means "get the object where used vectors are to be tracked for this IRQ", which has no implication on whether a vector was already assigned. Jan
On 17/05/2019 11:52, Jan Beulich wrote: > Use valid_irq_vector() rather than "> 0". > > Also replace an open-coded use of IRQ_VECTOR_UNASSIGNED. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
--- a/xen/arch/x86/irq.c +++ b/xen/arch/x86/irq.c @@ -342,7 +342,7 @@ void clear_irq_vector(int irq) int irq_to_vector(int irq) { - int vector = -1; + int vector = IRQ_VECTOR_UNASSIGNED; BUG_ON(irq >= nr_irqs || irq < 0); @@ -452,15 +452,18 @@ static vmask_t *irq_get_used_vector_mask int vector; vector = irq_to_vector(irq); - if ( vector > 0 ) + if ( valid_irq_vector(vector) ) { - printk(XENLOG_INFO "IRQ %d already assigned vector %d\n", + printk(XENLOG_INFO "IRQ%d already assigned vector %02x\n", irq, vector); ASSERT(!test_bit(vector, ret)); set_bit(vector, ret); } + else if ( vector != IRQ_VECTOR_UNASSIGNED ) + printk(XENLOG_WARNING "IRQ%d mapped to bogus vector %02x\n", + irq, vector); } } else if ( IO_APIC_IRQ(irq) && @@ -491,7 +494,7 @@ static int _assign_irq_vector(struct irq vmask_t *irq_used_vectors = NULL; old_vector = irq_to_vector(irq); - if ( old_vector > 0 ) + if ( valid_irq_vector(old_vector) ) { cpumask_t tmp_mask; @@ -555,7 +558,7 @@ next: current_vector = vector; current_offset = offset; - if ( old_vector > 0 ) + if ( valid_irq_vector(old_vector) ) { cpumask_and(desc->arch.old_cpu_mask, desc->arch.cpu_mask, &cpu_online_map);
Use valid_irq_vector() rather than "> 0". Also replace an open-coded use of IRQ_VECTOR_UNASSIGNED. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- v3: New.