Re: [PATCH] irqchip: irq-gic-v4: return real error code

[Marc Zyngier]

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@xxxxxxxxxx>
> ---
>  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...