Messages in this thread Patch in this message | | | Subject | Re: [PATCH] irqchip: irq-gic-v4: return real error code | From | Marc Zyngier <> | Date | Mon, 19 Mar 2018 08:30:49 +0000 |
| |
On 19/03/18 11:38, Peng Hao wrote: > __irq_domain_alloc_irqs will return some different error code, so we > should return real error code in its_alloc_vcpu_irqs.
What do we gain by doing so? Do we end-up treating the error in a different way? What does this actually improve? This is the kind of information I'm looking for in a commit message. Not something that describes the patch.
> > Signed-off-by: Peng Hao <peng.hao2@zte.com.cn> > --- > drivers/irqchip/irq-gic-v4.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/drivers/irqchip/irq-gic-v4.c b/drivers/irqchip/irq-gic-v4.c > index dba9d67..ecd170d 100644 > --- a/drivers/irqchip/irq-gic-v4.c > +++ b/drivers/irqchip/irq-gic-v4.c > @@ -99,7 +99,7 @@ > > int its_alloc_vcpu_irqs(struct its_vm *vm) > { > - int vpe_base_irq, i; > + int vpe_base_irq, i, ret = -ENOMEM; > > vm->fwnode = irq_domain_alloc_named_id_fwnode("GICv4-vpe", > task_pid_nr(current)); > @@ -120,8 +120,10 @@ int its_alloc_vcpu_irqs(struct its_vm *vm) > vpe_base_irq = __irq_domain_alloc_irqs(vm->domain, -1, vm->nr_vpes, > NUMA_NO_NODE, vm, > false, NULL); > - if (vpe_base_irq <= 0) > + if (vpe_base_irq <= 0) { > + ret = vpe_base_irq; > goto err;
Given that a return value of zero denotes an actual error, we can now pretend that everything went smoothly, except that none of the doorbells are initialized. KVM will then try and request the interrupts, which will fail (because 0 is not a valid interrupt number).
We thus went from a situation where the error was completely unambiguous (failed to allocate a bunch of interrupts) to a situation where things fail in a bizarre way because we cannot request the interrupts that we just allocated (something that shouldn't really fail).
I cannot call this change a real improvement.
> + } > > for (i = 0; i < vm->nr_vpes; i++) > vm->vpes[i]->irq = vpe_base_irq + i; > @@ -134,7 +136,7 @@ int its_alloc_vcpu_irqs(struct its_vm *vm) > if (vm->fwnode) > irq_domain_free_fwnode(vm->fwnode); > > - return -ENOMEM; > + return ret; > } > > void its_free_vcpu_irqs(struct its_vm *vm) >
Thanks,
M. -- Jazz is not dead. It just smells funny...
| |