Re: [PATCH 5/9] MIPS: Remove "weak" from get_c0_perfcount_int() declaration

[James Hogan]

On 13/07/15 00:11, Bjorn Helgaas wrote:
> Weak header file declarations are error-prone because they make every
> definition weak, and the linker chooses one based on link order (see
> 10629d711ed7 ("PCI: Remove __weak annotation from pcibios_get_phb_of_node
> decl")).
> 
> get_c0_perfcount_int() is defined in several files.  Every definition is
> weak, so I assume Kconfig prevents two or more from being included.  The
> callers contain identical default code used when get_c0_perfcount_int()
> isn't defined at all.
> 
> Add a weak get_c0_perfcount_int() definition with the default code and
> remove the weak annotation from the declaration.
> 
> Then the platform implementations will be strong and will override the weak
> default.  If multiple platforms are ever configured in, we'll get a link
> error instead of calling a random platform's implementation.
> 
> Signed-off-by: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>
> CC: Andrew Bresticker <abrestic@xxxxxxxxxxxx>
> ---
>  arch/mips/include/asm/time.h         |    2 +-
>  arch/mips/kernel/perf_event_mipsxx.c |    7 +------
>  arch/mips/kernel/time.c              |   10 +++++++++-
>  arch/mips/oprofile/op_model_mipsxx.c |    8 +-------
>  4 files changed, 12 insertions(+), 15 deletions(-)
> 
> diff --git a/arch/mips/include/asm/time.h b/arch/mips/include/asm/time.h
> index 8ab2874..ce6a7d5 100644
> --- a/arch/mips/include/asm/time.h
> +++ b/arch/mips/include/asm/time.h
> @@ -46,7 +46,7 @@ extern unsigned int mips_hpt_frequency;
>   * so it lives here.
>   */
>  extern int (*perf_irq)(void);
> -extern int __weak get_c0_perfcount_int(void);
> +extern int get_c0_perfcount_int(void);
>  
>  /*
>   * Initialize the calling CPU's compare interrupt as clockevent device
> diff --git a/arch/mips/kernel/perf_event_mipsxx.c b/arch/mips/kernel/perf_event_mipsxx.c
> index cc1b6fa..c126b1c 100644
> --- a/arch/mips/kernel/perf_event_mipsxx.c
> +++ b/arch/mips/kernel/perf_event_mipsxx.c
> @@ -1682,12 +1682,7 @@ init_hw_perf_events(void)
>  		counters = counters_total_to_per_cpu(counters);
>  #endif
>  
> -	if (get_c0_perfcount_int)
> -		irq = get_c0_perfcount_int();
> -	else if (cp0_perfcount_irq >= 0)
> -		irq = MIPS_CPU_IRQ_BASE + cp0_perfcount_irq;
> -	else
> -		irq = -1;
> +	irq = get_c0_perfcount_int();
>  
>  	mipspmu.map_raw_event = mipsxx_pmu_map_raw_event;
>  
> diff --git a/arch/mips/kernel/time.c b/arch/mips/kernel/time.c
> index 8d01709..ec7082d 100644
> --- a/arch/mips/kernel/time.c
> +++ b/arch/mips/kernel/time.c
> @@ -55,9 +55,17 @@ static int null_perf_irq(void)
>  }
>  
>  int (*perf_irq)(void) = null_perf_irq;
> -
>  EXPORT_SYMBOL(perf_irq);
>  
> +#ifdef MIPS_CPU_IRQ_BASE

why the ifdef? This would be the only such ifdef in the kernel, and
arch/mips/include/asm/mach-generic/irq.h seems to ensure it is always
defined anyway.

Aside from that the patch looks good.

Cheers
James

> +int __weak get_c0_perfcount_int(void)
> +{
> +	if (cp0_perfcount_irq >= 0)
> +		return MIPS_CPU_IRQ_BASE + cp0_perfcount_irq;
> +	return -1;
> +}
> +#endif
> +
>  /*
>   * time_init() - it does the following things.
>   *
> diff --git a/arch/mips/oprofile/op_model_mipsxx.c b/arch/mips/oprofile/op_model_mipsxx.c
> index 6a6e2cc..c0cffa9 100644
> --- a/arch/mips/oprofile/op_model_mipsxx.c
> +++ b/arch/mips/oprofile/op_model_mipsxx.c
> @@ -438,13 +438,7 @@ static int __init mipsxx_init(void)
>  	save_perf_irq = perf_irq;
>  	perf_irq = mipsxx_perfcount_handler;
>  
> -	if (get_c0_perfcount_int)
> -		perfcount_irq = get_c0_perfcount_int();
> -	else if (cp0_perfcount_irq >= 0)
> -		perfcount_irq = MIPS_CPU_IRQ_BASE + cp0_perfcount_irq;
> -	else
> -		perfcount_irq = -1;
> -
> +	perfcount_irq = get_c0_perfcount_int();
>  	if (perfcount_irq >= 0)
>  		return request_irq(perfcount_irq, mipsxx_perfcount_int,
>  				   IRQF_PERCPU | IRQF_NOBALANCING |
>

Attachment: signature.asc
Description: OpenPGP digital signature