[PATCH 0/9] MIPS: Remove "weak" usage

[PATCH 0/9] MIPS: Remove "weak" usage

[Bjorn Helgaas]

These patches don't fix any problem I'm aware of, but I think they make the
code easier to analyze, and they reduce the likelihood of issues if MIPS
ever builds a multi-platform kernel.

Weak function declarations in header files are hard to use safely because
they make every definition weak.  If the linker sees multiple weak
definitions, it silently chooses one based on link order.  That's not a
very obvious criterion, and it can easily lead to running the wrong
version.

These patches remove the weak attribute from function declarations and 
rework the code to match.  I don't have any of these platforms, so I can't
test them, but my intent is that these should cause no functional change.

---

Bjorn Helgaas (9):
      MIPS: CPC: Remove "weak" from mips_cpc_phys_base() and make it static
      MIPS: Remove "weak" from platform_maar_init() declaration
      MIPS: VPE: Exit vpe_release() early if vpe_run() isn't defined
      MIPS: MT: Remove "weak" from vpe_run() declaration
      MIPS: Remove "weak" from get_c0_perfcount_int() declaration
      MIPS: Remove "weak" from get_c0_compare_int() declaration
      MIPS: Remove "weak" from get_c0_fdc_int() declaration
      MIPS: Remove "weak" from mips_cdmm_phys_base() declaration
      MIPS: Remove "__weak" definition from arch-specific linkage.h


 arch/mips/include/asm/cdmm.h         |    4 ++--
 arch/mips/include/asm/irq.h          |    2 +-
 arch/mips/include/asm/linkage.h      |    1 -
 arch/mips/include/asm/maar.h         |    2 +-
 arch/mips/include/asm/mips-cpc.h     |   10 ----------
 arch/mips/include/asm/time.h         |    4 ++--
 arch/mips/include/asm/vpe.h          |    2 +-
 arch/mips/kernel/cevt-r4k.c          |   11 +++++++----
 arch/mips/kernel/mips-cpc.c          |    9 ++++++++-
 arch/mips/kernel/perf_event_mipsxx.c |    7 +------
 arch/mips/kernel/time.c              |   10 +++++++++-
 arch/mips/kernel/vpe.c               |    7 ++++++-
 arch/mips/oprofile/op_model_mipsxx.c |    8 +-------
 drivers/bus/mips_cdmm.c              |   14 +++++++++++++-
 drivers/tty/mips_ejtag_fdc.c         |    9 ++++++---
 15 files changed, 58 insertions(+), 42 deletions(-)

[PATCH 1/9] MIPS: CPC: Remove "weak" from mips_cpc_phys_base() and make it static

[Bjorn Helgaas]

There's only one implementation of mips_cpc_phys_base(), and it's only used
within the same file, so it doesn't need to be weak, and it doesn't need an
extern declaration.

Remove the extern mips_cpc_phys_base() declaration and make it static.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
CC: linux-mips@linux-mips.org
---
 arch/mips/include/asm/mips-cpc.h |   10 ----------
 arch/mips/kernel/mips-cpc.c      |    9 ++++++++-
 2 files changed, 8 insertions(+), 11 deletions(-)

diff --git a/arch/mips/include/asm/mips-cpc.h b/arch/mips/include/asm/mips-cpc.h
index 1cebe8c..f386f32 100644
--- a/arch/mips/include/asm/mips-cpc.h
+++ b/arch/mips/include/asm/mips-cpc.h
@@ -28,16 +28,6 @@ extern void __iomem *mips_cpc_base;
 extern phys_addr_t mips_cpc_default_phys_base(void);
 
 /**
- * mips_cpc_phys_base - retrieve the physical base address of the CPC
- *
- * This function returns the physical base address of the Cluster Power
- * Controller memory mapped registers, or 0 if no Cluster Power Controller
- * is present. It may be overriden by individual platforms which determine
- * this address in a different way.
- */
-extern phys_addr_t __weak mips_cpc_phys_base(void);
-
-/**
  * mips_cpc_probe - probe for a Cluster Power Controller
  *
  * Attempt to detect the presence of a Cluster Power Controller. Returns 0 if
diff --git a/arch/mips/kernel/mips-cpc.c b/arch/mips/kernel/mips-cpc.c
index 1196450..7e9ea9b 100644
--- a/arch/mips/kernel/mips-cpc.c
+++ b/arch/mips/kernel/mips-cpc.c
@@ -21,7 +21,14 @@ static DEFINE_PER_CPU_ALIGNED(spinlock_t, cpc_core_lock);
 
 static DEFINE_PER_CPU_ALIGNED(unsigned long, cpc_core_lock_flags);
 
-phys_addr_t __weak mips_cpc_phys_base(void)
+/**
+ * mips_cpc_phys_base - retrieve the physical base address of the CPC
+ *
+ * This function returns the physical base address of the Cluster Power
+ * Controller memory mapped registers, or 0 if no Cluster Power Controller
+ * is present.
+ */
+static phys_addr_t mips_cpc_phys_base(void)
 {
 	u32 cpc_base;
 

[PATCH 2/9] MIPS: Remove "weak" from platform_maar_init() declaration

[Bjorn Helgaas]

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")).

platform_maar_init() is defined in:

  - arch/mips/mm/init.c (where it is marked "weak")
  - arch/mips/mti-malta/malta-memory.c (without annotation)

The "weak" attribute on the platform_maar_init() extern declaration applies
to the platform-specific definition in arch/mips/mti-malta/malta-memory.c,
so both definitions are weak, and which one we get depends on link order.

Remove the "weak" attribute from the declaration.  That makes the malta
definition strong, so it will always be preferred if it is present.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
CC: linux-mips@linux-mips.org
---
 arch/mips/include/asm/maar.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/mips/include/asm/maar.h b/arch/mips/include/asm/maar.h
index 6c62b0f..b02891f 100644
--- a/arch/mips/include/asm/maar.h
+++ b/arch/mips/include/asm/maar.h
@@ -26,7 +26,7 @@
  *
  * Return:	The number of MAAR pairs configured.
  */
-unsigned __weak platform_maar_init(unsigned num_pairs);
+unsigned platform_maar_init(unsigned num_pairs);
 
 /**
  * write_maar_pair() - write to a pair of MAARs

Re: [PATCH 2/9] MIPS: Remove "weak" from platform_maar_init() declaration

[James Hogan]

[-- Attachment #1: Type: text/plain, Size: 1552 bytes --]

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")).
> 
> platform_maar_init() is defined in:
> 
>   - arch/mips/mm/init.c (where it is marked "weak")
>   - arch/mips/mti-malta/malta-memory.c (without annotation)
> 
> The "weak" attribute on the platform_maar_init() extern declaration applies
> to the platform-specific definition in arch/mips/mti-malta/malta-memory.c,
> so both definitions are weak, and which one we get depends on link order.
> 
> Remove the "weak" attribute from the declaration.  That makes the malta
> definition strong, so it will always be preferred if it is present.
> 
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> CC: linux-mips@linux-mips.org

Reviewed-by: James Hogan <james.hogan@imgtec.com>

Cheers
James

> ---
>  arch/mips/include/asm/maar.h |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/mips/include/asm/maar.h b/arch/mips/include/asm/maar.h
> index 6c62b0f..b02891f 100644
> --- a/arch/mips/include/asm/maar.h
> +++ b/arch/mips/include/asm/maar.h
> @@ -26,7 +26,7 @@
>   *
>   * Return:	The number of MAAR pairs configured.
>   */
> -unsigned __weak platform_maar_init(unsigned num_pairs);
> +unsigned platform_maar_init(unsigned num_pairs);
>  
>  /**
>   * write_maar_pair() - write to a pair of MAARs
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH 3/9] MIPS: VPE: Exit vpe_release() early if vpe_run() isn't defined

[Bjorn Helgaas]

vpe_run() is a weak symbol.  If there's no definition of it, its value is
zero.

If vpe_run is zero, return failure early.  We're going to fail anyway, so
there's no point in getting a VPE and attempting to load it.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 arch/mips/kernel/vpe.c |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/arch/mips/kernel/vpe.c b/arch/mips/kernel/vpe.c
index 11da314..72cae9f 100644
--- a/arch/mips/kernel/vpe.c
+++ b/arch/mips/kernel/vpe.c
@@ -821,13 +821,18 @@ static int vpe_release(struct inode *inode, struct file *filp)
 	Elf_Ehdr *hdr;
 	int ret = 0;
 
+	if (!vpe_run) {
+		pr_warn("VPE loader: ELF load failed.\n");
+		return -ENOEXEC;
+	}
+
 	v = get_vpe(aprp_cpu_index());
 	if (v == NULL)
 		return -ENODEV;
 
 	hdr = (Elf_Ehdr *) v->pbuffer;
 	if (memcmp(hdr->e_ident, ELFMAG, SELFMAG) == 0) {
-		if ((vpe_elfload(v) >= 0) && vpe_run) {
+		if (vpe_elfload(v) >= 0) {
 			vpe_run(v);
 		} else {
 			pr_warn("VPE loader: ELF load failed.\n");

[PATCH 4/9] MIPS: MT: Remove "weak" from vpe_run() declaration

[Bjorn Helgaas]

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")).

That's not a problem for vpe_run() because Kconfig ensures there's never
more than one definition:

  - vpe_run() is defined in arch/mips/kernel/vpe-mt.c if
    CONFIG_MIPS_VPE_LOADER_MT=y

  - vpe_run() is defined in arch/mips/mti-malta/malta-amon.c if
    CONFIG_MIPS_CMP=y

  - CONFIG_MIPS_VPE_LOADER_MT cannot be set if CONFIG_MIPS_CMP=y

But it's simpler to verify correctness if we remove "weak" from the picture
and test the config symbols directly.

Remove "weak" from the vpe_run() declaration and use #if to test whether a
definition should be present.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 arch/mips/include/asm/vpe.h |    2 +-
 arch/mips/kernel/vpe.c      |   10 +++++-----
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/arch/mips/include/asm/vpe.h b/arch/mips/include/asm/vpe.h
index 7849f39..80e70db 100644
--- a/arch/mips/include/asm/vpe.h
+++ b/arch/mips/include/asm/vpe.h
@@ -122,7 +122,7 @@ void release_vpe(struct vpe *v);
 void *alloc_progmem(unsigned long len);
 void release_progmem(void *ptr);
 
-int __weak vpe_run(struct vpe *v);
+int vpe_run(struct vpe *v);
 void cleanup_tc(struct tc *tc);
 
 int __init vpe_module_init(void);
diff --git a/arch/mips/kernel/vpe.c b/arch/mips/kernel/vpe.c
index 72cae9f..04539d6 100644
--- a/arch/mips/kernel/vpe.c
+++ b/arch/mips/kernel/vpe.c
@@ -817,15 +817,11 @@ static int vpe_open(struct inode *inode, struct file *filp)
 
 static int vpe_release(struct inode *inode, struct file *filp)
 {
+#if defined(CONFIG_MIPS_VPE_LOADER_MT) || defined(CONFIG_MIPS_CMP)
 	struct vpe *v;
 	Elf_Ehdr *hdr;
 	int ret = 0;
 
-	if (!vpe_run) {
-		pr_warn("VPE loader: ELF load failed.\n");
-		return -ENOEXEC;
-	}
-
 	v = get_vpe(aprp_cpu_index());
 	if (v == NULL)
 		return -ENODEV;
@@ -855,6 +851,10 @@ static int vpe_release(struct inode *inode, struct file *filp)
 	v->plen = 0;
 
 	return ret;
+#else
+	pr_warn("VPE loader: ELF load failed.\n");
+	return -ENOEXEC;
+#endif
 }
 
 static ssize_t vpe_write(struct file *file, const char __user *buffer,

Re: [PATCH 4/9] MIPS: MT: Remove "weak" from vpe_run() declaration

[James Hogan]

[-- Attachment #1: Type: text/plain, Size: 3072 bytes --]

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")).
> 
> That's not a problem for vpe_run() because Kconfig ensures there's never
> more than one definition:
> 
>   - vpe_run() is defined in arch/mips/kernel/vpe-mt.c if
>     CONFIG_MIPS_VPE_LOADER_MT=y
> 
>   - vpe_run() is defined in arch/mips/mti-malta/malta-amon.c if
>     CONFIG_MIPS_CMP=y
> 
>   - CONFIG_MIPS_VPE_LOADER_MT cannot be set if CONFIG_MIPS_CMP=y
> 
> But it's simpler to verify correctness if we remove "weak" from the picture
> and test the config symbols directly.
> 
> Remove "weak" from the vpe_run() declaration and use #if to test whether a
> definition should be present.
> 
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
>  arch/mips/include/asm/vpe.h |    2 +-
>  arch/mips/kernel/vpe.c      |   10 +++++-----
>  2 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/mips/include/asm/vpe.h b/arch/mips/include/asm/vpe.h
> index 7849f39..80e70db 100644
> --- a/arch/mips/include/asm/vpe.h
> +++ b/arch/mips/include/asm/vpe.h
> @@ -122,7 +122,7 @@ void release_vpe(struct vpe *v);
>  void *alloc_progmem(unsigned long len);
>  void release_progmem(void *ptr);
>  
> -int __weak vpe_run(struct vpe *v);
> +int vpe_run(struct vpe *v);
>  void cleanup_tc(struct tc *tc);
>  
>  int __init vpe_module_init(void);
> diff --git a/arch/mips/kernel/vpe.c b/arch/mips/kernel/vpe.c
> index 72cae9f..04539d6 100644
> --- a/arch/mips/kernel/vpe.c
> +++ b/arch/mips/kernel/vpe.c
> @@ -817,15 +817,11 @@ static int vpe_open(struct inode *inode, struct file *filp)
>  
>  static int vpe_release(struct inode *inode, struct file *filp)
>  {
> +#if defined(CONFIG_MIPS_VPE_LOADER_MT) || defined(CONFIG_MIPS_CMP)

That should be CONFIG_MIPS_VPE_LOADER_CMP, in which case the error case
in the #else bit is always dead code. This file is built only if
CONFIG_MIPS_VPE_LOADER, and the other ones are defined without prompts:

config MIPS_VPE_LOADER_CMP
	bool
	default "y"
	depends on MIPS_VPE_LOADER && MIPS_CMP

config MIPS_VPE_LOADER_MT
	bool
	default "y"
	depends on MIPS_VPE_LOADER && !MIPS_CMP

I.e. one xor the other must be "y" when MIPS_VPE_LOADER=y.

Maybe its worth just removing the weak and the vpe_run check?

Cheers
James

>  	struct vpe *v;
>  	Elf_Ehdr *hdr;
>  	int ret = 0;
>  
> -	if (!vpe_run) {
> -		pr_warn("VPE loader: ELF load failed.\n");
> -		return -ENOEXEC;
> -	}
> -
>  	v = get_vpe(aprp_cpu_index());
>  	if (v == NULL)
>  		return -ENODEV;
> @@ -855,6 +851,10 @@ static int vpe_release(struct inode *inode, struct file *filp)
>  	v->plen = 0;
>  
>  	return ret;
> +#else
> +	pr_warn("VPE loader: ELF load failed.\n");
> +	return -ENOEXEC;
> +#endif
>  }
>  
>  static ssize_t vpe_write(struct file *file, const char __user *buffer,
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 4/9] MIPS: MT: Remove "weak" from vpe_run() declaration

[Bjorn Helgaas]

On Mon, Jul 13, 2015 at 5:27 AM, James Hogan <james.hogan@imgtec.com> wrote:
> 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")).
>>
>> That's not a problem for vpe_run() because Kconfig ensures there's never
>> more than one definition:
>>
>>   - vpe_run() is defined in arch/mips/kernel/vpe-mt.c if
>>     CONFIG_MIPS_VPE_LOADER_MT=y
>>
>>   - vpe_run() is defined in arch/mips/mti-malta/malta-amon.c if
>>     CONFIG_MIPS_CMP=y
>>
>>   - CONFIG_MIPS_VPE_LOADER_MT cannot be set if CONFIG_MIPS_CMP=y
>>
>> But it's simpler to verify correctness if we remove "weak" from the picture
>> and test the config symbols directly.
>>
>> Remove "weak" from the vpe_run() declaration and use #if to test whether a
>> definition should be present.
>>
>> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>> ---
>>  arch/mips/include/asm/vpe.h |    2 +-
>>  arch/mips/kernel/vpe.c      |   10 +++++-----
>>  2 files changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/arch/mips/include/asm/vpe.h b/arch/mips/include/asm/vpe.h
>> index 7849f39..80e70db 100644
>> --- a/arch/mips/include/asm/vpe.h
>> +++ b/arch/mips/include/asm/vpe.h
>> @@ -122,7 +122,7 @@ void release_vpe(struct vpe *v);
>>  void *alloc_progmem(unsigned long len);
>>  void release_progmem(void *ptr);
>>
>> -int __weak vpe_run(struct vpe *v);
>> +int vpe_run(struct vpe *v);
>>  void cleanup_tc(struct tc *tc);
>>
>>  int __init vpe_module_init(void);
>> diff --git a/arch/mips/kernel/vpe.c b/arch/mips/kernel/vpe.c
>> index 72cae9f..04539d6 100644
>> --- a/arch/mips/kernel/vpe.c
>> +++ b/arch/mips/kernel/vpe.c
>> @@ -817,15 +817,11 @@ static int vpe_open(struct inode *inode, struct file *filp)
>>
>>  static int vpe_release(struct inode *inode, struct file *filp)
>>  {
>> +#if defined(CONFIG_MIPS_VPE_LOADER_MT) || defined(CONFIG_MIPS_CMP)
>
> That should be CONFIG_MIPS_VPE_LOADER_CMP, in which case the error case
> in the #else bit is always dead code. This file is built only if
> CONFIG_MIPS_VPE_LOADER, and the other ones are defined without prompts:
>
> config MIPS_VPE_LOADER_CMP
>         bool
>         default "y"
>         depends on MIPS_VPE_LOADER && MIPS_CMP
>
> config MIPS_VPE_LOADER_MT
>         bool
>         default "y"
>         depends on MIPS_VPE_LOADER && !MIPS_CMP
>
> I.e. one xor the other must be "y" when MIPS_VPE_LOADER=y.
>
> Maybe its worth just removing the weak and the vpe_run check?

Yes, thanks a lot for clearing up my Kconfig confusion!

>>       struct vpe *v;
>>       Elf_Ehdr *hdr;
>>       int ret = 0;
>>
>> -     if (!vpe_run) {
>> -             pr_warn("VPE loader: ELF load failed.\n");
>> -             return -ENOEXEC;
>> -     }
>> -
>>       v = get_vpe(aprp_cpu_index());
>>       if (v == NULL)
>>               return -ENODEV;
>> @@ -855,6 +851,10 @@ static int vpe_release(struct inode *inode, struct file *filp)
>>       v->plen = 0;
>>
>>       return ret;
>> +#else
>> +     pr_warn("VPE loader: ELF load failed.\n");
>> +     return -ENOEXEC;
>> +#endif
>>  }
>>
>>  static ssize_t vpe_write(struct file *file, const char __user *buffer,
>>
>

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

[Bjorn Helgaas]

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@google.com>
CC: Andrew Bresticker <abrestic@chromium.org>
---
 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
+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 |

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

[James Hogan]

[-- Attachment #1: Type: text/plain, Size: 3947 bytes --]

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@google.com>
> CC: Andrew Bresticker <abrestic@chromium.org>
> ---
>  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 #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Bjorn Helgaas]

On Mon, Jul 13, 2015 at 10:43:54AM +0100, James Hogan wrote:
> 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@google.com>
> > CC: Andrew Bresticker <abrestic@chromium.org>
> > ---
> >  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.

I added the ifdef because I got a build error in
arch/mips/kernel/time.c.  But maybe I can fix that by including
<asm/irq.h> instead; that would be much nicer.

> > +int __weak get_c0_perfcount_int(void)
> > +{
> > + if (cp0_perfcount_irq >= 0)
> > + return MIPS_CPU_IRQ_BASE + cp0_perfcount_irq;
> > + return -1;
> > +}
> > +#endif

[PATCH 6/9] MIPS: Remove "weak" from get_c0_compare_int() declaration

[Bjorn Helgaas]

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_compare_int() is defined in several files.  Each definition is weak,
so I assume Kconfig prevents two or more from being included.  The caller
contains default code used when get_c0_compare_int() isn't defined at all.

Add a weak get_c0_compare_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@google.com>
---
 arch/mips/include/asm/time.h |    2 +-
 arch/mips/kernel/cevt-r4k.c  |   11 +++++++----
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/arch/mips/include/asm/time.h b/arch/mips/include/asm/time.h
index ce6a7d5..44a9c3a 100644
--- a/arch/mips/include/asm/time.h
+++ b/arch/mips/include/asm/time.h
@@ -51,7 +51,7 @@ extern int get_c0_perfcount_int(void);
 /*
  * Initialize the calling CPU's compare interrupt as clockevent device
  */
-extern unsigned int __weak get_c0_compare_int(void);
+extern unsigned int get_c0_compare_int(void);
 extern int r4k_clockevent_init(void);
 
 static inline int mips_clockevent_init(void)
diff --git a/arch/mips/kernel/cevt-r4k.c b/arch/mips/kernel/cevt-r4k.c
index d70c4d8..cc7cc46 100644
--- a/arch/mips/kernel/cevt-r4k.c
+++ b/arch/mips/kernel/cevt-r4k.c
@@ -174,6 +174,11 @@ int c0_compare_int_usable(void)
 	return 1;
 }
 
+unsigned int __weak get_c0_compare_int(void)
+{
+	return MIPS_CPU_IRQ_BASE + cp0_compare_irq;
+}
+
 int r4k_clockevent_init(void)
 {
 	unsigned int cpu = smp_processor_id();
@@ -189,11 +194,9 @@ int r4k_clockevent_init(void)
 	/*
 	 * With vectored interrupts things are getting platform specific.
 	 * get_c0_compare_int is a hook to allow a platform to return the
-	 * interrupt number of it's liking.
+	 * interrupt number of its liking.
 	 */
-	irq = MIPS_CPU_IRQ_BASE + cp0_compare_irq;
-	if (get_c0_compare_int)
-		irq = get_c0_compare_int();
+	irq = get_c0_compare_int();
 
 	cd = &per_cpu(mips_clockevent_device, cpu);
 

Re: [PATCH 6/9] MIPS: Remove "weak" from get_c0_compare_int() declaration

[James Hogan]

[-- Attachment #1: Type: text/plain, Size: 2656 bytes --]

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_compare_int() is defined in several files.  Each definition is weak,
> so I assume Kconfig prevents two or more from being included.  The caller
> contains default code used when get_c0_compare_int() isn't defined at all.
> 
> Add a weak get_c0_compare_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@google.com>

Reviewed-by: James Hogan <james.hogan@imgtec.com>

Cheers
James

> ---
>  arch/mips/include/asm/time.h |    2 +-
>  arch/mips/kernel/cevt-r4k.c  |   11 +++++++----
>  2 files changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/mips/include/asm/time.h b/arch/mips/include/asm/time.h
> index ce6a7d5..44a9c3a 100644
> --- a/arch/mips/include/asm/time.h
> +++ b/arch/mips/include/asm/time.h
> @@ -51,7 +51,7 @@ extern int get_c0_perfcount_int(void);
>  /*
>   * Initialize the calling CPU's compare interrupt as clockevent device
>   */
> -extern unsigned int __weak get_c0_compare_int(void);
> +extern unsigned int get_c0_compare_int(void);
>  extern int r4k_clockevent_init(void);
>  
>  static inline int mips_clockevent_init(void)
> diff --git a/arch/mips/kernel/cevt-r4k.c b/arch/mips/kernel/cevt-r4k.c
> index d70c4d8..cc7cc46 100644
> --- a/arch/mips/kernel/cevt-r4k.c
> +++ b/arch/mips/kernel/cevt-r4k.c
> @@ -174,6 +174,11 @@ int c0_compare_int_usable(void)
>  	return 1;
>  }
>  
> +unsigned int __weak get_c0_compare_int(void)
> +{
> +	return MIPS_CPU_IRQ_BASE + cp0_compare_irq;
> +}
> +
>  int r4k_clockevent_init(void)
>  {
>  	unsigned int cpu = smp_processor_id();
> @@ -189,11 +194,9 @@ int r4k_clockevent_init(void)
>  	/*
>  	 * With vectored interrupts things are getting platform specific.
>  	 * get_c0_compare_int is a hook to allow a platform to return the
> -	 * interrupt number of it's liking.
> +	 * interrupt number of its liking.
>  	 */
> -	irq = MIPS_CPU_IRQ_BASE + cp0_compare_irq;
> -	if (get_c0_compare_int)
> -		irq = get_c0_compare_int();
> +	irq = get_c0_compare_int();
>  
>  	cd = &per_cpu(mips_clockevent_device, cpu);
>  
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH 7/9] MIPS: Remove "weak" from get_c0_fdc_int() declaration

[Bjorn Helgaas]

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_fdc_int() is defined only in arch/mips/mti-malta/malta-time.c so
there's no problem with multiple definitions.  But it works better to have
a weak default implementation and allow a strong function to override it.
Then we don't have to test whether a definition is present, and if there
are ever multiple strong definitions, we get a link error instead of
calling a random definition.

Add a weak get_c0_fdc_int() definition with the default code and remove the
weak annotation from the declaration.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
CC: James Hogan <james.hogan@imgtec.com>
---
 arch/mips/include/asm/irq.h  |    2 +-
 drivers/tty/mips_ejtag_fdc.c |    9 ++++++---
 2 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/arch/mips/include/asm/irq.h b/arch/mips/include/asm/irq.h
index f0db99f8..15e0fec 100644
--- a/arch/mips/include/asm/irq.h
+++ b/arch/mips/include/asm/irq.h
@@ -49,7 +49,7 @@ extern int cp0_compare_irq_shift;
 extern int cp0_perfcount_irq;
 extern int cp0_fdc_irq;
 
-extern int __weak get_c0_fdc_int(void);
+extern int get_c0_fdc_int(void);
 
 void arch_trigger_all_cpu_backtrace(bool);
 #define arch_trigger_all_cpu_backtrace arch_trigger_all_cpu_backtrace
diff --git a/drivers/tty/mips_ejtag_fdc.c b/drivers/tty/mips_ejtag_fdc.c
index 358323c..a8c8cfd 100644
--- a/drivers/tty/mips_ejtag_fdc.c
+++ b/drivers/tty/mips_ejtag_fdc.c
@@ -879,6 +879,11 @@ static const struct tty_operations mips_ejtag_fdc_tty_ops = {
 	.chars_in_buffer	= mips_ejtag_fdc_tty_chars_in_buffer,
 };
 
+int __weak get_c0_fdc_int(void)
+{
+	return -1;
+}
+
 static int mips_ejtag_fdc_tty_probe(struct mips_cdmm_device *dev)
 {
 	int ret, nport;
@@ -967,9 +972,7 @@ static int mips_ejtag_fdc_tty_probe(struct mips_cdmm_device *dev)
 	wake_up_process(priv->thread);
 
 	/* Look for an FDC IRQ */
-	priv->irq = -1;
-	if (get_c0_fdc_int)
-		priv->irq = get_c0_fdc_int();
+	priv->irq = get_c0_fdc_int();
 
 	/* Try requesting the IRQ */
 	if (priv->irq >= 0) {

Re: [PATCH 7/9] MIPS: Remove "weak" from get_c0_fdc_int() declaration

[James Hogan]

[-- Attachment #1: Type: text/plain, Size: 2616 bytes --]

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_fdc_int() is defined only in arch/mips/mti-malta/malta-time.c so

well, until 6b5e741e9a83 ("MIPS: Pistachio: Support CDMM & Fast Debug
Channel") in v4.2-rc2 ;-)

> there's no problem with multiple definitions.  But it works better to have
> a weak default implementation and allow a strong function to override it.
> Then we don't have to test whether a definition is present, and if there
> are ever multiple strong definitions, we get a link error instead of
> calling a random definition.
> 
> Add a weak get_c0_fdc_int() definition with the default code and remove the
> weak annotation from the declaration.
> 
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> CC: James Hogan <james.hogan@imgtec.com>

Reviewed-by: James Hogan <james.hogan@imgtec.com>

Thanks
James

> ---
>  arch/mips/include/asm/irq.h  |    2 +-
>  drivers/tty/mips_ejtag_fdc.c |    9 ++++++---
>  2 files changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/mips/include/asm/irq.h b/arch/mips/include/asm/irq.h
> index f0db99f8..15e0fec 100644
> --- a/arch/mips/include/asm/irq.h
> +++ b/arch/mips/include/asm/irq.h
> @@ -49,7 +49,7 @@ extern int cp0_compare_irq_shift;
>  extern int cp0_perfcount_irq;
>  extern int cp0_fdc_irq;
>  
> -extern int __weak get_c0_fdc_int(void);
> +extern int get_c0_fdc_int(void);
>  
>  void arch_trigger_all_cpu_backtrace(bool);
>  #define arch_trigger_all_cpu_backtrace arch_trigger_all_cpu_backtrace
> diff --git a/drivers/tty/mips_ejtag_fdc.c b/drivers/tty/mips_ejtag_fdc.c
> index 358323c..a8c8cfd 100644
> --- a/drivers/tty/mips_ejtag_fdc.c
> +++ b/drivers/tty/mips_ejtag_fdc.c
> @@ -879,6 +879,11 @@ static const struct tty_operations mips_ejtag_fdc_tty_ops = {
>  	.chars_in_buffer	= mips_ejtag_fdc_tty_chars_in_buffer,
>  };
>  
> +int __weak get_c0_fdc_int(void)
> +{
> +	return -1;
> +}
> +
>  static int mips_ejtag_fdc_tty_probe(struct mips_cdmm_device *dev)
>  {
>  	int ret, nport;
> @@ -967,9 +972,7 @@ static int mips_ejtag_fdc_tty_probe(struct mips_cdmm_device *dev)
>  	wake_up_process(priv->thread);
>  
>  	/* Look for an FDC IRQ */
> -	priv->irq = -1;
> -	if (get_c0_fdc_int)
> -		priv->irq = get_c0_fdc_int();
> +	priv->irq = get_c0_fdc_int();
>  
>  	/* Try requesting the IRQ */
>  	if (priv->irq >= 0) {
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH 8/9] MIPS: Remove "weak" from mips_cdmm_phys_base() declaration

[Bjorn Helgaas]

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")).

mips_cdmm_phys_base() is defined only in arch/mips/mti-malta/malta-memory.c
so there's no problem with multiple definitions.  But it works better to
have a weak default implementation and allow a strong function to override
it.  Then we don't have to test whether a definition is present, and if
there are ever multiple strong definitions, we get a link error instead of
calling a random definition.

Add a weak mips_cdmm_phys_base() definition and remove the weak annotation
from the declaration in arch/mips/include/asm/cdmm.h.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
CC: James Hogan <james.hogan@imgtec.com>
---
 arch/mips/include/asm/cdmm.h |    4 ++--
 drivers/bus/mips_cdmm.c      |   14 +++++++++++++-
 2 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/arch/mips/include/asm/cdmm.h b/arch/mips/include/asm/cdmm.h
index 16e22ce..bece206 100644
--- a/arch/mips/include/asm/cdmm.h
+++ b/arch/mips/include/asm/cdmm.h
@@ -53,7 +53,7 @@ struct mips_cdmm_driver {
  * mips_cdmm_phys_base() - Choose a physical base address for CDMM region.
  *
  * Picking a suitable physical address at which to map the CDMM region is
- * platform specific, so this weak function can be defined by platform code to
+ * platform specific, so this function can be defined by platform code to
  * pick a suitable value if none is configured by the bootloader.
  *
  * This address must be 32kB aligned, and the region occupies a maximum of 32kB
@@ -61,7 +61,7 @@ struct mips_cdmm_driver {
  *
  * Returns:	Physical base address for CDMM region, or 0 on failure.
  */
-phys_addr_t __weak mips_cdmm_phys_base(void);
+phys_addr_t mips_cdmm_phys_base(void);
 
 extern struct bus_type mips_cdmm_bustype;
 void __iomem *mips_cdmm_early_probe(unsigned int dev_type);
diff --git a/drivers/bus/mips_cdmm.c b/drivers/bus/mips_cdmm.c
index ab3bde1..1c543ef 100644
--- a/drivers/bus/mips_cdmm.c
+++ b/drivers/bus/mips_cdmm.c
@@ -332,6 +332,18 @@ static phys_addr_t mips_cdmm_cur_base(void)
 }
 
 /**
+ * mips_cdmm_phys_base() - Choose a physical base address for CDMM region.
+ *
+ * Picking a suitable physical address at which to map the CDMM region is
+ * platform specific, so this weak function can be overridden by platform
+ * code to pick a suitable value if none is configured by the bootloader.
+ */
+phys_addr_t __weak mips_cdmm_phys_base(void)
+{
+	return 0;
+}
+
+/**
  * mips_cdmm_setup() - Ensure the CDMM bus is initialised and usable.
  * @bus:	Pointer to bus information for current CPU.
  *		IS_ERR(bus) is checked, so no need for caller to check.
@@ -368,7 +380,7 @@ static int mips_cdmm_setup(struct mips_cdmm_bus *bus)
 	if (!bus->phys)
 		bus->phys = mips_cdmm_cur_base();
 	/* Otherwise, ask platform code for suggestions */
-	if (!bus->phys && mips_cdmm_phys_base)
+	if (!bus->phys)
 		bus->phys = mips_cdmm_phys_base();
 	/* Otherwise, copy what other CPUs have done */
 	if (!bus->phys)

Re: [PATCH 8/9] MIPS: Remove "weak" from mips_cdmm_phys_base() declaration

[James Hogan]

[-- Attachment #1: Type: text/plain, Size: 3752 bytes --]

Hi Bjorn,

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")).
> 
> mips_cdmm_phys_base() is defined only in arch/mips/mti-malta/malta-memory.c
> so there's no problem with multiple definitions.  But it works better to
> have a weak default implementation and allow a strong function to override
> it.  Then we don't have to test whether a definition is present, and if
> there are ever multiple strong definitions, we get a link error instead of
> calling a random definition.
> 
> Add a weak mips_cdmm_phys_base() definition and remove the weak annotation
> from the declaration in arch/mips/include/asm/cdmm.h.
> 
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> CC: James Hogan <james.hogan@imgtec.com>

Reviewed-by: James Hogan <james.hogan@imgtec.com>

Thanks for pointing out these problems. Hopefully eventually some of
these uses of weak symbols can be moved to device tree (its almost like
we need a DT binding for "this physical address isn't used for anything,
and would be suitable for an overlay").

Cheers
James

> ---
>  arch/mips/include/asm/cdmm.h |    4 ++--
>  drivers/bus/mips_cdmm.c      |   14 +++++++++++++-
>  2 files changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/mips/include/asm/cdmm.h b/arch/mips/include/asm/cdmm.h
> index 16e22ce..bece206 100644
> --- a/arch/mips/include/asm/cdmm.h
> +++ b/arch/mips/include/asm/cdmm.h
> @@ -53,7 +53,7 @@ struct mips_cdmm_driver {
>   * mips_cdmm_phys_base() - Choose a physical base address for CDMM region.
>   *
>   * Picking a suitable physical address at which to map the CDMM region is
> - * platform specific, so this weak function can be defined by platform code to
> + * platform specific, so this function can be defined by platform code to
>   * pick a suitable value if none is configured by the bootloader.
>   *
>   * This address must be 32kB aligned, and the region occupies a maximum of 32kB
> @@ -61,7 +61,7 @@ struct mips_cdmm_driver {
>   *
>   * Returns:	Physical base address for CDMM region, or 0 on failure.
>   */
> -phys_addr_t __weak mips_cdmm_phys_base(void);
> +phys_addr_t mips_cdmm_phys_base(void);
>  
>  extern struct bus_type mips_cdmm_bustype;
>  void __iomem *mips_cdmm_early_probe(unsigned int dev_type);
> diff --git a/drivers/bus/mips_cdmm.c b/drivers/bus/mips_cdmm.c
> index ab3bde1..1c543ef 100644
> --- a/drivers/bus/mips_cdmm.c
> +++ b/drivers/bus/mips_cdmm.c
> @@ -332,6 +332,18 @@ static phys_addr_t mips_cdmm_cur_base(void)
>  }
>  
>  /**
> + * mips_cdmm_phys_base() - Choose a physical base address for CDMM region.
> + *
> + * Picking a suitable physical address at which to map the CDMM region is
> + * platform specific, so this weak function can be overridden by platform
> + * code to pick a suitable value if none is configured by the bootloader.
> + */
> +phys_addr_t __weak mips_cdmm_phys_base(void)
> +{
> +	return 0;
> +}
> +
> +/**
>   * mips_cdmm_setup() - Ensure the CDMM bus is initialised and usable.
>   * @bus:	Pointer to bus information for current CPU.
>   *		IS_ERR(bus) is checked, so no need for caller to check.
> @@ -368,7 +380,7 @@ static int mips_cdmm_setup(struct mips_cdmm_bus *bus)
>  	if (!bus->phys)
>  		bus->phys = mips_cdmm_cur_base();
>  	/* Otherwise, ask platform code for suggestions */
> -	if (!bus->phys && mips_cdmm_phys_base)
> +	if (!bus->phys)
>  		bus->phys = mips_cdmm_phys_base();
>  	/* Otherwise, copy what other CPUs have done */
>  	if (!bus->phys)
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH 9/9] MIPS: Remove "__weak" definition from arch-specific linkage.h

[Bjorn Helgaas]

"__weak" is defined in include/linux/compiler-gcc.h.  We shouldn't need an
arch-specific definition.

Remove the "__weak" definition from arch/mips/include/asm/linkage.h.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 arch/mips/include/asm/linkage.h |    1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/mips/include/asm/linkage.h b/arch/mips/include/asm/linkage.h
index 2767dda..99651b0 100644
--- a/arch/mips/include/asm/linkage.h
+++ b/arch/mips/include/asm/linkage.h
@@ -5,7 +5,6 @@
 #include <asm/asm.h>
 #endif
 
-#define __weak __attribute__((weak))
 #define cond_syscall(x) asm(".weak\t" #x "\n" #x "\t=\tsys_ni_syscall")
 #define SYSCALL_ALIAS(alias, name)					\
 	asm ( #alias " = " #name "\n\t.globl " #alias)

Re: [PATCH 9/9] MIPS: Remove "__weak" definition from arch-specific linkage.h

[James Hogan]

[-- Attachment #1: Type: text/plain, Size: 1067 bytes --]

On 13/07/15 00:12, Bjorn Helgaas wrote:
> "__weak" is defined in include/linux/compiler-gcc.h.  We shouldn't need an
> arch-specific definition.
> 
> Remove the "__weak" definition from arch/mips/include/asm/linkage.h.
> 
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

asm/linkage.h is only included from linux/linkage.h, which includes
linux/compiler.h first, so no chance of build problems AFAICT, therefore:

Reviewed-by: James Hogan <james.hogan@imgtec.com>

Thanks
James

> ---
>  arch/mips/include/asm/linkage.h |    1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/arch/mips/include/asm/linkage.h b/arch/mips/include/asm/linkage.h
> index 2767dda..99651b0 100644
> --- a/arch/mips/include/asm/linkage.h
> +++ b/arch/mips/include/asm/linkage.h
> @@ -5,7 +5,6 @@
>  #include <asm/asm.h>
>  #endif
>  
> -#define __weak __attribute__((weak))
>  #define cond_syscall(x) asm(".weak\t" #x "\n" #x "\t=\tsys_ni_syscall")
>  #define SYSCALL_ALIAS(alias, name)					\
>  	asm ( #alias " = " #name "\n\t.globl " #alias)
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]