Re: [PATCH 08/12] Drivers: hv: vmbus: Implement Direct Mode for stimer0

[Dan Carpenter <dan.carpenter@oracle.com>]

On Sun, Feb 11, 2018 at 05:33:16PM -0700, kys@exchange.microsoft.com wrote:
> @@ -116,9 +146,29 @@ static int hv_ce_set_oneshot(struct clock_event_device *evt)
>  {
>  	union hv_timer_config timer_cfg;
>  
> +	timer_cfg.as_uint64 = 0;
>  	timer_cfg.enable = 1;
>  	timer_cfg.auto_enable = 1;
> -	timer_cfg.sintx = VMBUS_MESSAGE_SINT;
> +	if (direct_mode_enabled)
> +		/*
> +		 * When it expires, the timer will directly interrupt
> +		 * on the specified hardware vector/IRQ.
> +		 */
> +		{
> +		timer_cfg.direct_mode = 1;
> +		timer_cfg.apic_vector = stimer0_vector;
> +		hv_enable_stimer0_percpu_irq(stimer0_irq);
> +		}
> +	else
> +		/*
> +		 * When it expires, the timer will generate a VMbus message,
> +		 * to be handled by the normal VMbus interrupt handler.
> +		 */
> +		{
> +		timer_cfg.direct_mode = 0;
> +		timer_cfg.sintx = VMBUS_MESSAGE_SINT;
> +		}
> +

This indenting isn't right.  We should probably zero out .apic_vector
if .direct_mode is zero.  Or maybe it's fine.  I don't know if any
static analysis tools will complain...

>  	hv_init_timer_config(HV_X64_MSR_STIMER0_CONFIG, timer_cfg.as_uint64);
>  
>  	return 0;
> @@ -191,6 +241,10 @@ int hv_synic_alloc(void)
>  		INIT_LIST_HEAD(&hv_cpu->chan_list);
>  	}
>  
> +	if (direct_mode_enabled && hv_setup_stimer0_irq(
> +		&stimer0_irq, &stimer0_vector, hv_stimer0_isr))
> +		goto err;


Can you indent it like this:

	if (direct_mode_enabled &&
	    hv_setup_stimer0_irq(&stimer0_irq, &stimer0_vector,
				 hv_stimer0_isr))
		goto err;


[ What follows is a long rant not directed at you ]

It's annoying because as soon as I see the "goto err;", I know the error
handling for this function is going to be buggy...

Some rules of error handling are:

1)  Each function should clean up after itself instead returning
    partially allocated structures.
2)  Each allocation function should have a matching free function.
3)  Give meaningful label names based on what the label location so that
    we can tell what the goto does just by looking at it, such as,
    "goto free_some_variable".  This way we can just keep a mental tally
    of the most recently allocated resource and verify based on the
    "goto free_resource;" statemetn that it frees the correct thing.  We
    don't need to scroll to the bottom of the function.

    Using good names means that we should avoid do-nothing gotos
    because, by definition, the label name for a do-nothing goto is
    going to be vague.

In this case the label looks like this:

> +
>  	return 0;
>  err:
>  	return -ENOMEM;

We allocate a bunch of stuff in this function so at first glance this
looks like we leak everything but, actually, the cleanup is done in
vmbus_bus_init().  This is a layering violation.

Later on, we changed hv_synic_alloc() in 37cdd991fac8 ("vmbus: put
related per-cpu variable together") and we started allocating:

	hv_cpu->clk_evt = kzalloc(...

but we forgot to update the error handling because it was in the wrong
place.  It's a very predictable, avoidable bug if we just use proper
error handling style.

Anyway...  Sorry for the long rant.  Summary:  Always distrust vague
label names.

regards,
dan carpenter