| Message ID | 1480696696-29833-1-git-send-email-olekstysh@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, 2 Dec 2016, Oleksandr Tyshchenko wrote: > Call irq_get_domain for the IRQ we are interested in > only after making sure that it is the guest IRQ to avoid > ASSERT(test_bit(_IRQ_GUEST, &desc->status)) triggering. > > Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> > Signed-off-by: Andrii Anisov <andrii_anisov@epam.com> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> Applied to xen-arm-next (queued for the next release). > xen/arch/arm/irq.c | 32 +++++++++++++++++++------------- > 1 file changed, 19 insertions(+), 13 deletions(-) > > diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c > index 06d4843..508028b 100644 > --- a/xen/arch/arm/irq.c > +++ b/xen/arch/arm/irq.c > @@ -477,26 +477,32 @@ int route_irq_to_guest(struct domain *d, unsigned int virq, > */ > if ( desc->action != NULL ) > { > + if ( test_bit(_IRQ_GUEST, &desc->status) ) > { > + struct domain *ad = irq_get_domain(desc); > + > + if ( d == ad ) > + { > + if ( irq_get_guest_info(desc)->virq != virq ) > + { > + printk(XENLOG_G_ERR > + "d%u: IRQ %u is already assigned to vIRQ %u\n", > + d->domain_id, irq, irq_get_guest_info(desc)->virq); > + retval = -EBUSY; > + } > + } > + else > { > + printk(XENLOG_G_ERR "IRQ %u is already used by domain %u\n", > + irq, ad->domain_id); > retval = -EBUSY; > } > } > else > + { > printk(XENLOG_G_ERR "IRQ %u is already used by Xen\n", irq); > - retval = -EBUSY; > + retval = -EBUSY; > + } > goto out; > } > > -- > 2.7.4 >
Hi Oleksandr, On 02/12/16 16:38, Oleksandr Tyshchenko wrote: > Call irq_get_domain for the IRQ we are interested in > only after making sure that it is the guest IRQ to avoid > ASSERT(test_bit(_IRQ_GUEST, &desc->status)) triggering. > > Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> > Signed-off-by: Andrii Anisov <andrii_anisov@epam.com> > --- > xen/arch/arm/irq.c | 32 +++++++++++++++++++------------- > 1 file changed, 19 insertions(+), 13 deletions(-) > > diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c > index 06d4843..508028b 100644 > --- a/xen/arch/arm/irq.c > +++ b/xen/arch/arm/irq.c > @@ -477,26 +477,32 @@ int route_irq_to_guest(struct domain *d, unsigned int virq, > */ > if ( desc->action != NULL ) > { > - struct domain *ad = irq_get_domain(desc); > - > - if ( test_bit(_IRQ_GUEST, &desc->status) && d == ad ) > + if ( test_bit(_IRQ_GUEST, &desc->status) ) > { > - if ( irq_get_guest_info(desc)->virq != virq ) > + struct domain *ad = irq_get_domain(desc); > + > + if ( d == ad ) > + { > + if ( irq_get_guest_info(desc)->virq != virq ) I know that Stefano already reviewed and queued this patch. But 4 layer of if seems a bit too much and could have been avoided by re-ordering the check. if ( d != ad ) .... else if ( irq_get_guest_info(desc->virq != virq) ) .... Can you please send a follow-up to remove one layer of 1? Regards,
Hi Oleksandr, On 02/12/16 16:38, Oleksandr Tyshchenko wrote: > Call irq_get_domain for the IRQ we are interested in > only after making sure that it is the guest IRQ to avoid > ASSERT(test_bit(_IRQ_GUEST, &desc->status)) triggering. > > Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> I forgot to mention that your Signed-off-by does not match the From. In the future, please make sure that the author e-mail (the From) is listed in the signed-off. Regards,
On Mon, Dec 5, 2016 at 3:50 PM, Julien Grall <julien.grall@arm.com> wrote: > Hi Oleksandr, Hi Julien > > > On 02/12/16 16:38, Oleksandr Tyshchenko wrote: > >> Call irq_get_domain for the IRQ we are interested in >> only after making sure that it is the guest IRQ to avoid >> ASSERT(test_bit(_IRQ_GUEST, &desc->status)) triggering. >> >> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >> Signed-off-by: Andrii Anisov <andrii_anisov@epam.com> >> --- >> xen/arch/arm/irq.c | 32 +++++++++++++++++++------------- >> 1 file changed, 19 insertions(+), 13 deletions(-) >> >> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c >> index 06d4843..508028b 100644 >> --- a/xen/arch/arm/irq.c >> +++ b/xen/arch/arm/irq.c >> @@ -477,26 +477,32 @@ int route_irq_to_guest(struct domain *d, unsigned >> int virq, >> */ >> if ( desc->action != NULL ) >> { >> - struct domain *ad = irq_get_domain(desc); >> - >> - if ( test_bit(_IRQ_GUEST, &desc->status) && d == ad ) >> + if ( test_bit(_IRQ_GUEST, &desc->status) ) >> { >> - if ( irq_get_guest_info(desc)->virq != virq ) >> + struct domain *ad = irq_get_domain(desc); >> + >> + if ( d == ad ) >> + { >> + if ( irq_get_guest_info(desc)->virq != virq ) >> > > I know that Stefano already reviewed and queued this patch. But 4 layer of > if seems a bit too much and could have been avoided by re-ordering the > check. > > if ( d != ad ) > .... > else if ( irq_get_guest_info(desc->virq != virq) ) > .... > > Can you please send a follow-up to remove one layer of 1? > Sure > > Regards, > > -- > Julien Grall >
On Mon, Dec 5, 2016 at 3:52 PM, Julien Grall <julien.grall@arm.com> wrote: > Hi Oleksandr, > Hi Julien, > > On 02/12/16 16:38, Oleksandr Tyshchenko wrote: > >> Call irq_get_domain for the IRQ we are interested in >> only after making sure that it is the guest IRQ to avoid >> ASSERT(test_bit(_IRQ_GUEST, &desc->status)) triggering. >> >> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >> > > I forgot to mention that your Signed-off-by does not match the From. > In the future, please make sure that the author e-mail (the From) is > listed in the signed-off. > Got it > > Regards, > > -- > Julien Grall >
On Mon, 5 Dec 2016, Julien Grall wrote: > Hi Oleksandr, > > On 02/12/16 16:38, Oleksandr Tyshchenko wrote: > > Call irq_get_domain for the IRQ we are interested in > > only after making sure that it is the guest IRQ to avoid > > ASSERT(test_bit(_IRQ_GUEST, &desc->status)) triggering. > > > > Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> > > I forgot to mention that your Signed-off-by does not match the From. > In the future, please make sure that the author e-mail (the From) is listed in > the signed-off. CC'ing Lars. Actually I don't think this is required: Signed-off-by is about copyright ownership, that is where using the company email is important. But I don't think it is necessary for the authorship, that is what From: is about. Authorship is just to give credit to the author, it could be a pseudonym, I think. I am pretty sure that I have seen other instances of this on the LKML. For example: Alice works for Bob Alice writes a patch, Bob has copyright ownership Chris takes Alice's patch and sends it to xen-devel Chris should use its own email address in the email From: field; he uses Alice's email address (doesn't matter which) in the From: field within the patch; he should use Bob's email as Signed-off-by: From: Chris Subject: [PATCH] fix something From: Alice This patch fixes something Signed-off-by: Bob
diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c index 06d4843..508028b 100644 --- a/xen/arch/arm/irq.c +++ b/xen/arch/arm/irq.c @@ -477,26 +477,32 @@ int route_irq_to_guest(struct domain *d, unsigned int virq, */ if ( desc->action != NULL ) { - struct domain *ad = irq_get_domain(desc); - - if ( test_bit(_IRQ_GUEST, &desc->status) && d == ad ) + if ( test_bit(_IRQ_GUEST, &desc->status) ) { - if ( irq_get_guest_info(desc)->virq != virq ) + struct domain *ad = irq_get_domain(desc); + + if ( d == ad ) + { + if ( irq_get_guest_info(desc)->virq != virq ) + { + printk(XENLOG_G_ERR + "d%u: IRQ %u is already assigned to vIRQ %u\n", + d->domain_id, irq, irq_get_guest_info(desc)->virq); + retval = -EBUSY; + } + } + else { - printk(XENLOG_G_ERR - "d%u: IRQ %u is already assigned to vIRQ %u\n", - d->domain_id, irq, irq_get_guest_info(desc)->virq); + printk(XENLOG_G_ERR "IRQ %u is already used by domain %u\n", + irq, ad->domain_id); retval = -EBUSY; } - goto out; } - - if ( test_bit(_IRQ_GUEST, &desc->status) ) - printk(XENLOG_G_ERR "IRQ %u is already used by domain %u\n", - irq, ad->domain_id); else + { printk(XENLOG_G_ERR "IRQ %u is already used by Xen\n", irq); - retval = -EBUSY; + retval = -EBUSY; + } goto out; }