Re: [PATCH] io_apic: initialize irq with -EINVAL

[Thomas Gleixner <tglx@linutronix.de>]

Norbert,

On Wed, 28 Nov 2018, Norbert Manthey wrote:

thanks for the patch.

> Subject: [PATCH] io_apic: initialize irq with -EINVAL

io_apic is not a proper subsystem prefix. git log should give you a hint:

   x86/ioapic: ....

Also please start the first word after the colon with an uppercase letter.

Now 'Initialize irq with -EINVAL' is not really informative. It tells what
the patch does, but does not give a consise hint, what this is about and
why you want to do it. Something like:

   x86/ioapic: Prevent uninitialized return value

provides precise information about the scope of the patch.

> To catch the case where the uninitialized variable irq might be
> returned.

I had to read this incomplete sentence 3 times until I discovered that this
is the second part of the subject line. Please don't do that.

> As the path that might lead to this situation can only
> occur based on invalid arguments, we initialize this variable with
> the value -EINVAL, so that callers are notified accordingly, and no
> uninitialized value is returned.
> 
> The path that would allow to return an uninitialized value for the
> variable irq would require legacy IRQs without the ALLOC flag.

Ideally you describe the problem first and not elaborate lengthy on the
solution, because the solution is obvious once the problem is
clear. Something like this:

  mp_map_pin_to_irq() has an exit path which returns an uninitialized
  variable. This is reached, when  called with arguments ......

  Initialize 'irq' with -EINVAL to prevent that.

> Signed-off-by: Norbert Manthey <nmanthey@amazon.de>
> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>

This SOB chain is wrong. How is David involved in this? If he co-developed
the patch, then a 'Co-Developed-by: David...' tag is missing. If not, then
his SOB is just wrong here.

> ---
>  arch/x86/kernel/apic/io_apic.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
> index 2953bbf..219dbc1 100644
> --- a/arch/x86/kernel/apic/io_apic.c
> +++ b/arch/x86/kernel/apic/io_apic.c
> @@ -1031,7 +1031,7 @@ static int alloc_isa_irq_from_domain(struct irq_domain *domain,
>  static int mp_map_pin_to_irq(u32 gsi, int idx, int ioapic, int pin,
>  			     unsigned int flags, struct irq_alloc_info *info)
>  {
> -	int irq;
> +	int irq = -EINVAL;
>  	bool legacy = false;
>  	struct irq_alloc_info tmp;
>  	struct mp_chip_data *data;

Now, lets look at the actual error path:

        if (!(flags & IOAPIC_MAP_ALLOC)) {
                if (!legacy) {
                        irq = irq_find_mapping(domain, pin);
                        if (irq == 0)
                                irq = -ENOENT;
                }
		 <----------- (i.e. if legacy == true)
	}

That looks about right, but to get there something must set legacy to
'true' and the only code path which does so is:

        if (idx >= 0 && test_bit(mp_irqs[idx].srcbus, mp_bus_not_pci)) {
                irq = mp_irqs[idx].srcbusirq;
                legacy = mp_is_legacy_irq(irq);
        }

and this code path actually initializes 'irq'. So returning uninitialized
'irq' cannot happen.

How did you find that? Code inspection, static checker, ... ?

Thanks,

	tglx