[Cbe-oss-dev] [PATCH] OProfile fix call to kzalloc

[Arnd Bergmann]

On Friday 22 June 2007, Bob Nelson wrote:
> Ok, try again without any Windows programs involved this time...
> 
> Fix OProfile kernel module to check pointer returned from kzalloc for 
> success/failure.  Eliminated unnecessary cast.
> Added some better error handling and cleanup in the related area of the 
> code.

Actually, on closer look I found a few style issues, but I
fixed them up myself, no need to resend another time.

> @@ -22,7 +22,7 @@
>  #define TRACE_ARRAY_SIZE 1024
>  #define SCALE_SHIFT 14
>  
> -static u32 * samples;
> +static u32 * samples = 0;
>  
>  static int spu_prof_running = 0;
>  static unsigned int profiling_interval = 0;

This is functionally irrelevant, but stylewise wrong on two accounts:

- static variables are automatically initialized to zero, so
  we don't add an explicit initialization for those, by convention.

- In the kernel, the convention is to write NULL instead of 0
  when meaning null pointers. The sparse tool warns about your
  code, so I assume you have not run sparse on it.
  Ideally, you should always build with sparse enabled, at least
  before submitting a patch.

> @@ -192,9 +192,10 @@ static struct hrtimer timer;
>   * cycles_reset is the count value specified by the user when
>   * setting up OProfile to count SPU_CYCLES.
>   */
> -void start_spu_profiling(unsigned int cycles_reset) {
> +int start_spu_profiling(unsigned int cycles_reset) {
>  
>  	ktime_t kt;
> +	int ret = 0;
>  
>  	pr_debug("timer resolution: %lu\n",
>  		 TICK_NSEC);

The opening braces in a function belong to the start of the line.
I know it's not introduced by you, but if you touch a line, you
can just as well fix it.

Initializing local variables in the declaration is usually bad
style because it prevents the compiler from warning about potential
uninitialized uses. Better initialize it as late as possible.

> @@ -204,17 +205,25 @@ void start_spu_profiling(unsigned int cy
>  	timer.function = profile_spus;
>  
>  	/* Allocate arrays for collecting SPU PC samples */
> -	samples = (u32 *) kzalloc(SPUS_PER_NODE *
> -				  TRACE_ARRAY_SIZE * sizeof(u32), GFP_KERNEL);
> +	samples = kzalloc(SPUS_PER_NODE *
> +			  TRACE_ARRAY_SIZE * sizeof(u32), GFP_KERNEL);
>  
> -	spu_prof_running = 1;
> -	hrtimer_start(&timer, kt, HRTIMER_MODE_REL);
> +	if (unlikely(samples == 0)) {
> +		ret = -ENOMEM;

Here, again, you should use 'NULL' instead of '0'. Even better, write
'if (!samples)' instead of 'if (samples == NULL)'. Yes, benh disagrees
with me on that, but this is my position on it.

Also, no need for 'likely' and 'unlikely' in code that is not
extremely performance critical. Using those too much can actually
blow up the code size, which makes it slower in the end than getting
the occasional mispredicted branch.

>  void stop_spu_profiling(void)
>  {
>  	spu_prof_running = 0;
>  	hrtimer_cancel(&timer);
> -	kfree(samples);
> +	if (samples != 0) {
> +		kfree(samples);
> +	}
>  	pr_debug("SPU_PROF: stop_spu_profiling issued\n");
>  }

- kfree(NULL) is fine, no need to test explicitly.
- you should not use curly braces when the basic block is
  a single source line
- 'if (!samples)' instead of 'if (samples != NULL)'

>  
> +static void cell_global_stop_spu(void);
> +

Please avoid forward declarations of static funcions. Instead reorder
the functions so you don't need them. It will also result in more
readable code, because you can assume that static functions can only
be used in code that come later in the same file.

> -		if (unlikely(rtn_value != 0)) {
> +		if (unlikely(ret != 0)) {

If you change this line, just change it to 'if (ret)' in the process.

> -	start_spu_profiling(spu_cycle_reset);
> +	ret = start_spu_profiling(spu_cycle_reset);
> +	if (unlikely(ret != 0)) {
> +		cell_global_stop_spu();		/* clean up the PMU/debug bus */
> +		rtas_error = ret;
> +		goto out;
> +	}

same here. Also, cell_global_stop_spu() should probably be called after the
out: label, to make the cleanup more obvious.

	Arnd <><