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

2018-02-13 Thread Dan Carpenter
On Wed, Feb 14, 2018 at 02:58:41AM +, Michael Kelley (EOSG) wrote:
> > -Original Message-
> > From: Dan Carpenter <dan.carpen...@oracle.com>
> > Sent: Monday, February 12, 2018 12:42 AM
> > To: KY Srinivasan <k...@microsoft.com>; Stephen Hemminger
> > <step...@networkplumber.org>
> > Cc: gre...@linuxfoundation.org; linux-ker...@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
> > <sthem...@microsoft.com>; Michael Kelley (EOSG) 
> > <michael.h.kel...@microsoft.com>
> > 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

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


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

2018-02-13 Thread Michael Kelley (EOSG)
> -Original Message-
> From: Dan Carpenter <dan.carpen...@oracle.com>
> Sent: Monday, February 12, 2018 12:42 AM
> To: KY Srinivasan <k...@microsoft.com>; Stephen Hemminger
> <step...@networkplumber.org>
> Cc: gre...@linuxfoundation.org; linux-ker...@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
> <sthem...@microsoft.com>; Michael Kelley (EOSG) 
> <michael.h.kel...@microsoft.com>
> 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(_cpu->chan_list);
> > }
> >
> > +   if (direct_mode_enabled && hv_setup_stimer0_irq(
> > +   _irq, _vector, hv_stimer0_isr))
> > +   goto err;
> 
> 
> Can you indent it like this:
> 
>   if (direct_mode_enabled &&
>   hv_setup_stimer0_irq(_irq, _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

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


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

2018-02-12 Thread Dan Carpenter
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(_cpu->chan_list);
>   }
>  
> + if (direct_mode_enabled && hv_setup_stimer0_irq(
> + _irq, _vector, hv_stimer0_isr))
> + goto err;


Can you indent it like this:

if (direct_mode_enabled &&
hv_setup_stimer0_irq(_irq, _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

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel