Re: [PATCH 08/12] Drivers: hv: vmbus: Implement Direct Mode for stimer0
On Wed, Feb 14, 2018 at 02:58:41AM +, Michael Kelley (EOSG) wrote: > > -Original Message- > > From: Dan Carpenter > > Sent: Monday, February 12, 2018 12:42 AM > > To: KY Srinivasan ; Stephen Hemminger > > > > Cc: gre...@linuxfoundation.org; linux-kernel@vger.kernel.org; > > de...@linuxdriverproject.org; > > o...@aepfle.de; a...@canonical.com; vkuzn...@redhat.com; > > jasow...@redhat.com; > > leann.ogasaw...@canonical.com; marcelo.ce...@canonical.com; Stephen > > Hemminger > > ; Michael Kelley (EOSG) > > > > Subject: Re: [PATCH 08/12] Drivers: hv: vmbus: Implement Direct Mode for > > stimer0 > > > > On Sun, Feb 11, 2018 at 05:33:16PM -0700, k...@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... > > I'll fix the indenting. Old habits > > The " timer_cfg.as_uint64 = 0" statement already zero's out .apic_vector > along with all the other unused fields in the 64-bit value, as required by > the Hyper-V spec. Ah, you're right, of course. regards, dan carpenter
RE: [PATCH 08/12] Drivers: hv: vmbus: Implement Direct Mode for stimer0
> -Original Message- > From: Dan Carpenter > Sent: Monday, February 12, 2018 12:42 AM > To: KY Srinivasan ; Stephen Hemminger > > Cc: gre...@linuxfoundation.org; linux-kernel@vger.kernel.org; > de...@linuxdriverproject.org; > o...@aepfle.de; a...@canonical.com; vkuzn...@redhat.com; jasow...@redhat.com; > leann.ogasaw...@canonical.com; marcelo.ce...@canonical.com; Stephen Hemminger > ; Michael Kelley (EOSG) > > Subject: Re: [PATCH 08/12] Drivers: hv: vmbus: Implement Direct Mode for > stimer0 > > On Sun, Feb 11, 2018 at 05:33:16PM -0700, k...@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... I'll fix the indenting. Old habits The " timer_cfg.as_uint64 = 0" statement already zero's out .apic_vector along with all the other unused fields in the 64-bit value, as required by the Hyper-V spec. > > > 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; OK -- will change. > > > [ 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. We'll fix this is a separate patch. There are a couple other minor things that should be cleaned up in hv.c as well, and can do them together. Michael > > Anyway... Sorry for the long rant. Summary: Always distrust vague > label names. > > regards, > dan carpenter
Re: [PATCH 08/12] Drivers: hv: vmbus: Implement Direct Mode for stimer0
On Sun, Feb 11, 2018 at 05:33:16PM -0700, k...@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