[PATCH v1 00/10] Remove weak function declarations

[PATCH v1 00/10] Remove weak function declarations

[Bjorn Helgaas]

A common usage of "weak" is for a default implementation of a function.
An architecture that needs something different can supply a non-weak
("strong") implementation, with the expectation that the linker will select
the strong version and discard the weak default version.

We have a few function declarations in header files annotated as "weak".
That causes every *every* definition to be marked "weak", which means there
is no strong version at all.  In this case, the linker selects one of the
weak versions based on link order.  I don't think this is what we want.

These patches remove almost all the weak annotations from header files
(MIPS still uses it for get_c0_compare_int(), apparently relying on the
fact that a weak symbol need not be defined at all).  In most cases, the
default implementation was already marked weak at the definition.  When it
wasn't, I added that.

It might be simplest if I ask Linus to pull these all as a group from my
branch [1].  I'll look for acks from the following people.  If I don't see
an ack, I'll drop the patch and you can take it yourself or ignore it as
you wish.

  Eric:                    audit
  Thomas, Ingo, or Peter:  x86
  Ralf:                    MIPS
  John or Thomas:          clocksource
  Jason:                   kgdb
  Ingo:                    uprobes
  Andrew:                  vmcore, memory-hotplug

I don't know whether these fix any actual bugs.  We *did* have a bug like
this on MIPS a while ago (10629d711ed7 ("PCI: Remove __weak annotation from
pcibios_get_phb_of_node decl")), so it's possible that they do fix
something.

[1] https://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/log/?h=remove-weak-function-declarations

---

Bjorn Helgaas (10):
      audit: Remove "weak" from audit_classify_compat_syscall() declaration
      x86, intel-mid: Remove "weak" from function declarations
      MIPS: CPC: Make mips_cpc_phys_base() static
      MIPS: Remove "weak" from platform_maar_init() declaration
      MIPS: MT: Move "weak" from vpe_run() declaration to definition
      clocksource: Remove "weak" from clocksource_default_clock() declaration
      vmcore: Remove "weak" from function declarations
      kgdb: Remove "weak" from kgdb_arch_pc() declaration
      memory-hotplug: Remove "weak" from memory_block_size_bytes() declaration
      uprobes: Remove "weak" from function declarations


 arch/mips/include/asm/maar.h                       |    2 +-
 arch/mips/include/asm/mips-cpc.h                   |   10 ----------
 arch/mips/include/asm/vpe.h                        |    2 +-
 arch/mips/kernel/mips-cpc.c                        |    2 +-
 arch/mips/kernel/vpe-mt.c                          |    2 +-
 arch/x86/platform/intel-mid/intel_mid_weak_decls.h |    7 +++----
 include/linux/audit.h                              |    2 +-
 include/linux/clocksource.h                        |    2 +-
 include/linux/crash_dump.h                         |   15 +++++++--------
 include/linux/kgdb.h                               |    2 +-
 include/linux/memory.h                             |    2 +-
 include/linux/uprobes.h                            |   14 +++++++-------
 12 files changed, 25 insertions(+), 37 deletions(-)

[PATCH v1 01/10] audit: Remove "weak" from audit_classify_compat_syscall() declaration

[Bjorn Helgaas]

There's only one audit_classify_compat_syscall() definition, so it doesn't
need to be weak.

Remove the "weak" attribute from the audit_classify_compat_syscall()
declaration.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
CC: AKASHI Takahiro <takahiro.akashi@linaro.org>
CC: Richard Guy Briggs <rgb@redhat.com>
---
 include/linux/audit.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/audit.h b/include/linux/audit.h
index 22cfddb75566..1b6098beb46b 100644
--- a/include/linux/audit.h
+++ b/include/linux/audit.h
@@ -86,7 +86,7 @@ extern unsigned compat_dir_class[];
 extern unsigned compat_chattr_class[];
 extern unsigned compat_signal_class[];
 
-extern int __weak audit_classify_compat_syscall(int abi, unsigned syscall);
+extern int audit_classify_compat_syscall(int abi, unsigned syscall);
 
 /* audit_names->type values */
 #define	AUDIT_TYPE_UNKNOWN	0	/* we don't know yet */

Re: [PATCH v1 01/10] audit: Remove "weak" from audit_classify_compat_syscall() declaration

[Bjorn Helgaas]

[+cc AKASHI, Richard; sorry, I botched my "stg mail" so you weren't
included the first time]

On Wed, Oct 15, 2014 at 11:05 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> There's only one audit_classify_compat_syscall() definition, so it doesn't
> need to be weak.
>
> Remove the "weak" attribute from the audit_classify_compat_syscall()
> declaration.
>
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> CC: AKASHI Takahiro <takahiro.akashi@linaro.org>
> CC: Richard Guy Briggs <rgb@redhat.com>
> ---
>  include/linux/audit.h |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index 22cfddb75566..1b6098beb46b 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -86,7 +86,7 @@ extern unsigned compat_dir_class[];
>  extern unsigned compat_chattr_class[];
>  extern unsigned compat_signal_class[];
>
> -extern int __weak audit_classify_compat_syscall(int abi, unsigned syscall);
> +extern int audit_classify_compat_syscall(int abi, unsigned syscall);
>
>  /* audit_names->type values */
>  #define        AUDIT_TYPE_UNKNOWN      0       /* we don't know yet */
>

Re: [PATCH v1 01/10] audit: Remove "weak" from audit_classify_compat_syscall() declaration

[Richard Guy Briggs]

On 14/10/15, Bjorn Helgaas wrote:
> [+cc AKASHI, Richard; sorry, I botched my "stg mail" so you weren't
> included the first time]

Makes sense, ACK.

> On Wed, Oct 15, 2014 at 11:05 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> > There's only one audit_classify_compat_syscall() definition, so it doesn't
> > need to be weak.
> >
> > Remove the "weak" attribute from the audit_classify_compat_syscall()
> > declaration.
> >
> > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> > CC: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > CC: Richard Guy Briggs <rgb@redhat.com>
> > ---
> >  include/linux/audit.h |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/include/linux/audit.h b/include/linux/audit.h
> > index 22cfddb75566..1b6098beb46b 100644
> > --- a/include/linux/audit.h
> > +++ b/include/linux/audit.h
> > @@ -86,7 +86,7 @@ extern unsigned compat_dir_class[];
> >  extern unsigned compat_chattr_class[];
> >  extern unsigned compat_signal_class[];
> >
> > -extern int __weak audit_classify_compat_syscall(int abi, unsigned syscall);
> > +extern int audit_classify_compat_syscall(int abi, unsigned syscall);
> >
> >  /* audit_names->type values */
> >  #define        AUDIT_TYPE_UNKNOWN      0       /* we don't know yet */

- RGB

--
Richard Guy Briggs <rbriggs@redhat.com>
Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545

[PATCH v1 02/10] x86, intel-mid: Remove "weak" from function declarations

[Bjorn Helgaas]

For the following interfaces:

  get_penwell_ops()
  get_cloverview_ops()
  get_tangier_ops()

there is only one implementation, so they do not need to be marked "weak".

Remove the "weak" attribute from their declarations.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
CC: David Cohen <david.a.cohen@linux.intel.com>
CC: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
CC: x86@kernel.org
---
 arch/x86/platform/intel-mid/intel_mid_weak_decls.h |    7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/arch/x86/platform/intel-mid/intel_mid_weak_decls.h b/arch/x86/platform/intel-mid/intel_mid_weak_decls.h
index 46aa25c8ce06..3c1c3866d82b 100644
--- a/arch/x86/platform/intel-mid/intel_mid_weak_decls.h
+++ b/arch/x86/platform/intel-mid/intel_mid_weak_decls.h
@@ -10,10 +10,9 @@
  */
 
 
-/* __attribute__((weak)) makes these declarations overridable */
 /* For every CPU addition a new get_<cpuname>_ops interface needs
  * to be added.
  */
-extern void *get_penwell_ops(void) __attribute__((weak));
-extern void *get_cloverview_ops(void) __attribute__((weak));
-extern void *get_tangier_ops(void) __attribute__((weak));
+extern void *get_penwell_ops(void);
+extern void *get_cloverview_ops(void);
+extern void *get_tangier_ops(void);

Re: [PATCH v1 02/10] x86, intel-mid: Remove "weak" from function declarations

[Bjorn Helgaas]

[+cc David, Kuppuswamy, x86; sorry, I botched my "stg mail" so you
weren't included the first time]

On Wed, Oct 15, 2014 at 11:05 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> For the following interfaces:
>
>   get_penwell_ops()
>   get_cloverview_ops()
>   get_tangier_ops()
>
> there is only one implementation, so they do not need to be marked "weak".
>
> Remove the "weak" attribute from their declarations.
>
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> CC: David Cohen <david.a.cohen@linux.intel.com>
> CC: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> CC: x86@kernel.org
> ---
>  arch/x86/platform/intel-mid/intel_mid_weak_decls.h |    7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/arch/x86/platform/intel-mid/intel_mid_weak_decls.h b/arch/x86/platform/intel-mid/intel_mid_weak_decls.h
> index 46aa25c8ce06..3c1c3866d82b 100644
> --- a/arch/x86/platform/intel-mid/intel_mid_weak_decls.h
> +++ b/arch/x86/platform/intel-mid/intel_mid_weak_decls.h
> @@ -10,10 +10,9 @@
>   */
>
>
> -/* __attribute__((weak)) makes these declarations overridable */
>  /* For every CPU addition a new get_<cpuname>_ops interface needs
>   * to be added.
>   */
> -extern void *get_penwell_ops(void) __attribute__((weak));
> -extern void *get_cloverview_ops(void) __attribute__((weak));
> -extern void *get_tangier_ops(void) __attribute__((weak));
> +extern void *get_penwell_ops(void);
> +extern void *get_cloverview_ops(void);
> +extern void *get_tangier_ops(void);
>

Re: [PATCH v1 02/10] x86, intel-mid: Remove "weak" from function declarations

[sathyanarayanan kuppuswamy]

Hi Bjorn,

On 10/15/2014 04:26 PM, Bjorn Helgaas wrote:
> [+cc David, Kuppuswamy, x86; sorry, I botched my "stg mail" so you
> weren't included the first time]
>
> On Wed, Oct 15, 2014 at 11:05 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>> For the following interfaces:
>>
>>    get_penwell_ops()
>>    get_cloverview_ops()
>>    get_tangier_ops()
>>
>> there is only one implementation, so they do not need to be marked "weak".
>>
>> Remove the "weak" attribute from their declarations.
>>
>> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>> CC: David Cohen <david.a.cohen@linux.intel.com>
>> CC: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>> CC: x86@kernel.org
>> ---
>>   arch/x86/platform/intel-mid/intel_mid_weak_decls.h |    7 +++----
>>   1 file changed, 3 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/x86/platform/intel-mid/intel_mid_weak_decls.h b/arch/x86/platform/intel-mid/intel_mid_weak_decls.h
>> index 46aa25c8ce06..3c1c3866d82b 100644
>> --- a/arch/x86/platform/intel-mid/intel_mid_weak_decls.h
Please remove this file and move the contents to asm/intel-mid.h.
>> +++ b/arch/x86/platform/intel-mid/intel_mid_weak_decls.h
>> @@ -10,10 +10,9 @@
>>    */
>>
>>
>> -/* __attribute__((weak)) makes these declarations overridable */
>>   /* For every CPU addition a new get_<cpuname>_ops interface needs
>>    * to be added.
>>    */
>> -extern void *get_penwell_ops(void) __attribute__((weak));
>> -extern void *get_cloverview_ops(void) __attribute__((weak));
>> -extern void *get_tangier_ops(void) __attribute__((weak));
>> +extern void *get_penwell_ops(void);
>> +extern void *get_cloverview_ops(void);
>> +extern void *get_tangier_ops(void);
>>

-- 
Sathyanarayanan Kuppuswamy
Android kernel developer

Re: [PATCH v1 02/10] x86, intel-mid: Remove "weak" from function declarations

[David Cohen]

Hi Bjorn and Sathya,

On Thu, Oct 16, 2014 at 05:42:11PM -0700, sathyanarayanan kuppuswamy wrote:
> Hi Bjorn,
> 
> On 10/15/2014 04:26 PM, Bjorn Helgaas wrote:
> >[+cc David, Kuppuswamy, x86; sorry, I botched my "stg mail" so you
> >weren't included the first time]
> >
> >On Wed, Oct 15, 2014 at 11:05 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> >>For the following interfaces:
> >>
> >>   get_penwell_ops()
> >>   get_cloverview_ops()
> >>   get_tangier_ops()
> >>
> >>there is only one implementation, so they do not need to be marked "weak".
> >>
> >>Remove the "weak" attribute from their declarations.
> >>
> >>Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> >>CC: David Cohen <david.a.cohen@linux.intel.com>
> >>CC: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> >>CC: x86@kernel.org
> >>---
> >>  arch/x86/platform/intel-mid/intel_mid_weak_decls.h |    7 +++----
> >>  1 file changed, 3 insertions(+), 4 deletions(-)
> >>
> >>diff --git a/arch/x86/platform/intel-mid/intel_mid_weak_decls.h b/arch/x86/platform/intel-mid/intel_mid_weak_decls.h
> >>index 46aa25c8ce06..3c1c3866d82b 100644
> >>--- a/arch/x86/platform/intel-mid/intel_mid_weak_decls.h
> Please remove this file and move the contents to asm/intel-mid.h.

I partially agree :)

Historically, this file was created because we could not build all intel
mid variants at once. So we have to select only one during compilation
time, which was fixed already.

But we don't need to expose those functions outside intel-mid's
directory, which means asm/intel-mid.h isn't the best option IMHO.

If you want, I can send a small re-work instead: we get rid of this
header file completely and simplify a bit what is exposed by
asm/intel-mid.h. Or you can keep this patch and then I send the re-work
on top of it. Anyway I'm fine.

Br, David

> >>+++ b/arch/x86/platform/intel-mid/intel_mid_weak_decls.h
> >>@@ -10,10 +10,9 @@
> >>   */
> >>
> >>
> >>-/* __attribute__((weak)) makes these declarations overridable */
> >>  /* For every CPU addition a new get_<cpuname>_ops interface needs
> >>   * to be added.
> >>   */
> >>-extern void *get_penwell_ops(void) __attribute__((weak));
> >>-extern void *get_cloverview_ops(void) __attribute__((weak));
> >>-extern void *get_tangier_ops(void) __attribute__((weak));
> >>+extern void *get_penwell_ops(void);
> >>+extern void *get_cloverview_ops(void);
> >>+extern void *get_tangier_ops(void);
> >>
> 
> -- 
> Sathyanarayanan Kuppuswamy
> Android kernel developer
> 

Re: [PATCH v1 02/10] x86, intel-mid: Remove "weak" from function declarations

[Bjorn Helgaas]

On Thu, Oct 16, 2014 at 7:41 PM, David Cohen
<david.a.cohen@linux.intel.com> wrote:
> Hi Bjorn and Sathya,
>
> On Thu, Oct 16, 2014 at 05:42:11PM -0700, sathyanarayanan kuppuswamy wrote:
>> Hi Bjorn,
>>
>> On 10/15/2014 04:26 PM, Bjorn Helgaas wrote:
>> >[+cc David, Kuppuswamy, x86; sorry, I botched my "stg mail" so you
>> >weren't included the first time]
>> >
>> >On Wed, Oct 15, 2014 at 11:05 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>> >>For the following interfaces:
>> >>
>> >>   get_penwell_ops()
>> >>   get_cloverview_ops()
>> >>   get_tangier_ops()
>> >>
>> >>there is only one implementation, so they do not need to be marked "weak".
>> >>
>> >>Remove the "weak" attribute from their declarations.
>> >>
>> >>Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>> >>CC: David Cohen <david.a.cohen@linux.intel.com>
>> >>CC: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>> >>CC: x86@kernel.org
>> >>---
>> >>  arch/x86/platform/intel-mid/intel_mid_weak_decls.h |    7 +++----
>> >>  1 file changed, 3 insertions(+), 4 deletions(-)
>> >>
>> >>diff --git a/arch/x86/platform/intel-mid/intel_mid_weak_decls.h b/arch/x86/platform/intel-mid/intel_mid_weak_decls.h
>> >>index 46aa25c8ce06..3c1c3866d82b 100644
>> >>--- a/arch/x86/platform/intel-mid/intel_mid_weak_decls.h
>> Please remove this file and move the contents to asm/intel-mid.h.

I don't know or care enough about intel-mid to do this myself.  Right
now, I'm just trying to remove unnecessary and incorrect usage of
"weak" in header files.

> I partially agree :)
>
> Historically, this file was created because we could not build all intel
> mid variants at once. So we have to select only one during compilation
> time, which was fixed already.
>
> But we don't need to expose those functions outside intel-mid's
> directory, which means asm/intel-mid.h isn't the best option IMHO.
>
> If you want, I can send a small re-work instead: we get rid of this
> header file completely and simplify a bit what is exposed by
> asm/intel-mid.h. Or you can keep this patch and then I send the re-work
> on top of it. Anyway I'm fine.

It's fine with me if you want to rework this to remove the header
completely.  When I pointed this out in January [1], you mentioned
plans for that.  But I think we should merge this patch in the interim
to remove the use of "weak" in a header file.  If we leave bad
examples in the tree, they just proliferate.

[1] http://lkml.kernel.org/r/20140128013013.GA31626@psi-dev26.jf.intel.com


>> >>+++ b/arch/x86/platform/intel-mid/intel_mid_weak_decls.h
>> >>@@ -10,10 +10,9 @@
>> >>   */
>> >>
>> >>
>> >>-/* __attribute__((weak)) makes these declarations overridable */
>> >>  /* For every CPU addition a new get_<cpuname>_ops interface needs
>> >>   * to be added.
>> >>   */
>> >>-extern void *get_penwell_ops(void) __attribute__((weak));
>> >>-extern void *get_cloverview_ops(void) __attribute__((weak));
>> >>-extern void *get_tangier_ops(void) __attribute__((weak));
>> >>+extern void *get_penwell_ops(void);
>> >>+extern void *get_cloverview_ops(void);
>> >>+extern void *get_tangier_ops(void);
>> >>
>>
>> --
>> Sathyanarayanan Kuppuswamy
>> Android kernel developer
>>

Re: [PATCH v1 02/10] x86, intel-mid: Remove "weak" from function declarations

[David Cohen]

On Mon, Oct 20, 2014 at 10:15:17AM -0600, Bjorn Helgaas wrote:
> On Thu, Oct 16, 2014 at 7:41 PM, David Cohen
> <david.a.cohen@linux.intel.com> wrote:
> > Hi Bjorn and Sathya,
> >
> > On Thu, Oct 16, 2014 at 05:42:11PM -0700, sathyanarayanan kuppuswamy wrote:
> >> Hi Bjorn,
> >>
> >> On 10/15/2014 04:26 PM, Bjorn Helgaas wrote:
> >> >[+cc David, Kuppuswamy, x86; sorry, I botched my "stg mail" so you
> >> >weren't included the first time]
> >> >
> >> >On Wed, Oct 15, 2014 at 11:05 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> >> >>For the following interfaces:
> >> >>
> >> >>   get_penwell_ops()
> >> >>   get_cloverview_ops()
> >> >>   get_tangier_ops()
> >> >>
> >> >>there is only one implementation, so they do not need to be marked "weak".
> >> >>
> >> >>Remove the "weak" attribute from their declarations.
> >> >>
> >> >>Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> >> >>CC: David Cohen <david.a.cohen@linux.intel.com>
> >> >>CC: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> >> >>CC: x86@kernel.org
> >> >>---
> >> >>  arch/x86/platform/intel-mid/intel_mid_weak_decls.h |    7 +++----
> >> >>  1 file changed, 3 insertions(+), 4 deletions(-)
> >> >>
> >> >>diff --git a/arch/x86/platform/intel-mid/intel_mid_weak_decls.h b/arch/x86/platform/intel-mid/intel_mid_weak_decls.h
> >> >>index 46aa25c8ce06..3c1c3866d82b 100644
> >> >>--- a/arch/x86/platform/intel-mid/intel_mid_weak_decls.h
> >> Please remove this file and move the contents to asm/intel-mid.h.
> 
> I don't know or care enough about intel-mid to do this myself.  Right
> now, I'm just trying to remove unnecessary and incorrect usage of
> "weak" in header files.
> 
> > I partially agree :)
> >
> > Historically, this file was created because we could not build all intel
> > mid variants at once. So we have to select only one during compilation
> > time, which was fixed already.
> >
> > But we don't need to expose those functions outside intel-mid's
> > directory, which means asm/intel-mid.h isn't the best option IMHO.
> >
> > If you want, I can send a small re-work instead: we get rid of this
> > header file completely and simplify a bit what is exposed by
> > asm/intel-mid.h. Or you can keep this patch and then I send the re-work
> > on top of it. Anyway I'm fine.
> 
> It's fine with me if you want to rework this to remove the header
> completely.  When I pointed this out in January [1], you mentioned
> plans for that.  But I think we should merge this patch in the interim
> to remove the use of "weak" in a header file.  If we leave bad
> examples in the tree, they just proliferate.
> 
> [1] http://lkml.kernel.org/r/20140128013013.GA31626@psi-dev26.jf.intel.com

Yeah, a lot late :(
Please, go ahead with this patch and I send something on top of it.

BR, David

> 
> 
> >> >>+++ b/arch/x86/platform/intel-mid/intel_mid_weak_decls.h
> >> >>@@ -10,10 +10,9 @@
> >> >>   */
> >> >>
> >> >>
> >> >>-/* __attribute__((weak)) makes these declarations overridable */
> >> >>  /* For every CPU addition a new get_<cpuname>_ops interface needs
> >> >>   * to be added.
> >> >>   */
> >> >>-extern void *get_penwell_ops(void) __attribute__((weak));
> >> >>-extern void *get_cloverview_ops(void) __attribute__((weak));
> >> >>-extern void *get_tangier_ops(void) __attribute__((weak));
> >> >>+extern void *get_penwell_ops(void);
> >> >>+extern void *get_cloverview_ops(void);
> >> >>+extern void *get_tangier_ops(void);
> >> >>
> >>
> >> --
> >> Sathyanarayanan Kuppuswamy
> >> Android kernel developer
> >>

Re: [PATCH v1 02/10] x86, intel-mid: Remove "weak" from function declarations

[Ingo Molnar]

* Bjorn Helgaas <bhelgaas@google.com> wrote:

> For the following interfaces:
> 
>   get_penwell_ops()
>   get_cloverview_ops()
>   get_tangier_ops()
> 
> there is only one implementation, so they do not need to be marked "weak".
> 
> Remove the "weak" attribute from their declarations.
> 
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> CC: David Cohen <david.a.cohen@linux.intel.com>
> CC: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> CC: x86@kernel.org
> ---
>  arch/x86/platform/intel-mid/intel_mid_weak_decls.h |    7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/platform/intel-mid/intel_mid_weak_decls.h b/arch/x86/platform/intel-mid/intel_mid_weak_decls.h
> index 46aa25c8ce06..3c1c3866d82b 100644
> --- a/arch/x86/platform/intel-mid/intel_mid_weak_decls.h
> +++ b/arch/x86/platform/intel-mid/intel_mid_weak_decls.h
> @@ -10,10 +10,9 @@
>   */
>  
>  
> -/* __attribute__((weak)) makes these declarations overridable */
>  /* For every CPU addition a new get_<cpuname>_ops interface needs
>   * to be added.
>   */
> -extern void *get_penwell_ops(void) __attribute__((weak));
> -extern void *get_cloverview_ops(void) __attribute__((weak));
> -extern void *get_tangier_ops(void) __attribute__((weak));
> +extern void *get_penwell_ops(void);
> +extern void *get_cloverview_ops(void);
> +extern void *get_tangier_ops(void);

Acked-by: Ingo Molnar <mingo@kernel.org>

Thanks,

	Ingo

[PATCH v1 03/10] MIPS: CPC: Make mips_cpc_phys_base() 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      |    2 +-
 2 files changed, 1 insertion(+), 11 deletions(-)

diff --git a/arch/mips/include/asm/mips-cpc.h b/arch/mips/include/asm/mips-cpc.h
index e139a534e0fd..8ff92cd74bde 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_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_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 ba473608a347..36c20ae509d8 100644
--- a/arch/mips/kernel/mips-cpc.c
+++ b/arch/mips/kernel/mips-cpc.c
@@ -21,7 +21,7 @@ static DEFINE_PER_CPU_ALIGNED(spinlock_t, cpc_core_lock);
 
 static DEFINE_PER_CPU_ALIGNED(unsigned long, cpc_core_lock_flags);
 
-phys_t __weak mips_cpc_phys_base(void)
+static phys_t mips_cpc_phys_base(void)
 {
 	u32 cpc_base;
 

Re: [PATCH v1 03/10] MIPS: CPC: Make mips_cpc_phys_base() static

[Bjorn Helgaas]

[+cc linux-mips@linux-mips.org]

On Wed, Oct 15, 2014 at 11:06 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> 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      |    2 +-
>  2 files changed, 1 insertion(+), 11 deletions(-)
>
> diff --git a/arch/mips/include/asm/mips-cpc.h b/arch/mips/include/asm/mips-cpc.h
> index e139a534e0fd..8ff92cd74bde 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_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_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 ba473608a347..36c20ae509d8 100644
> --- a/arch/mips/kernel/mips-cpc.c
> +++ b/arch/mips/kernel/mips-cpc.c
> @@ -21,7 +21,7 @@ static DEFINE_PER_CPU_ALIGNED(spinlock_t, cpc_core_lock);
>
>  static DEFINE_PER_CPU_ALIGNED(unsigned long, cpc_core_lock_flags);
>
> -phys_t __weak mips_cpc_phys_base(void)
> +static phys_t mips_cpc_phys_base(void)
>  {
>         u32 cpc_base;
>
>

Re: [PATCH v1 03/10] MIPS: CPC: Make mips_cpc_phys_base() static

[Bjorn Helgaas]

[+cc linux-mips for real this time.  sheesh]

On Wed, Oct 15, 2014 at 5:27 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> [+cc linux-mips@linux-mips.org]
>
> On Wed, Oct 15, 2014 at 11:06 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>> 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      |    2 +-
>>  2 files changed, 1 insertion(+), 11 deletions(-)
>>
>> diff --git a/arch/mips/include/asm/mips-cpc.h b/arch/mips/include/asm/mips-cpc.h
>> index e139a534e0fd..8ff92cd74bde 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_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_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 ba473608a347..36c20ae509d8 100644
>> --- a/arch/mips/kernel/mips-cpc.c
>> +++ b/arch/mips/kernel/mips-cpc.c
>> @@ -21,7 +21,7 @@ static DEFINE_PER_CPU_ALIGNED(spinlock_t, cpc_core_lock);
>>
>>  static DEFINE_PER_CPU_ALIGNED(unsigned long, cpc_core_lock_flags);
>>
>> -phys_t __weak mips_cpc_phys_base(void)
>> +static phys_t mips_cpc_phys_base(void)
>>  {
>>         u32 cpc_base;
>>
>>

Re: [PATCH v1 03/10] MIPS: CPC: Make mips_cpc_phys_base() static

[Bjorn Helgaas]

On Wed, Oct 15, 2014 at 5:28 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> [+cc linux-mips for real this time.  sheesh]
>
> On Wed, Oct 15, 2014 at 5:27 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>> [+cc linux-mips@linux-mips.org]
>>
>> On Wed, Oct 15, 2014 at 11:06 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>>> 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

I'm dropping this patch from my series because I haven't seen any
response from the MIPS folks.

>>> ---
>>>  arch/mips/include/asm/mips-cpc.h |   10 ----------
>>>  arch/mips/kernel/mips-cpc.c      |    2 +-
>>>  2 files changed, 1 insertion(+), 11 deletions(-)
>>>
>>> diff --git a/arch/mips/include/asm/mips-cpc.h b/arch/mips/include/asm/mips-cpc.h
>>> index e139a534e0fd..8ff92cd74bde 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_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_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 ba473608a347..36c20ae509d8 100644
>>> --- a/arch/mips/kernel/mips-cpc.c
>>> +++ b/arch/mips/kernel/mips-cpc.c
>>> @@ -21,7 +21,7 @@ static DEFINE_PER_CPU_ALIGNED(spinlock_t, cpc_core_lock);
>>>
>>>  static DEFINE_PER_CPU_ALIGNED(unsigned long, cpc_core_lock_flags);
>>>
>>> -phys_t __weak mips_cpc_phys_base(void)
>>> +static phys_t mips_cpc_phys_base(void)
>>>  {
>>>         u32 cpc_base;
>>>
>>>

[PATCH v1 04/10] MIPS: Remove "weak" from platform_maar_init() declaration

[Bjorn Helgaas]

arch/mips/mm/init.c provides a default platform_maar_init() definition
explicitly marked "weak".  arch/mips/mti-malta/malta-memory.c provides its
own definition intended to override the default, but the "weak" attribute
on the declaration applied to this as well, so the linker chose one based
on link order (see 10629d711ed7 ("PCI: Remove __weak annotation from
pcibios_get_phb_of_node decl")).

Remove the "weak" attribute from the declaration so we always prefer a
non-weak definition over the weak one, independent of link order.

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 6c62b0f899c0..b02891f9caaf 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 v1 04/10] MIPS: Remove "weak" from platform_maar_init() declaration

[Bjorn Helgaas]

[+cc linux-mips]

On Wed, Oct 15, 2014 at 11:06 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> arch/mips/mm/init.c provides a default platform_maar_init() definition
> explicitly marked "weak".  arch/mips/mti-malta/malta-memory.c provides its
> own definition intended to override the default, but the "weak" attribute
> on the declaration applied to this as well, so the linker chose one based
> on link order (see 10629d711ed7 ("PCI: Remove __weak annotation from
> pcibios_get_phb_of_node decl")).
>
> Remove the "weak" attribute from the declaration so we always prefer a
> non-weak definition over the weak one, independent of link order.
>
> 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 6c62b0f899c0..b02891f9caaf 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 v1 04/10] MIPS: Remove "weak" from platform_maar_init() declaration

[Bjorn Helgaas]

On Wed, Oct 15, 2014 at 5:27 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> [+cc linux-mips]
>
> On Wed, Oct 15, 2014 at 11:06 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>> arch/mips/mm/init.c provides a default platform_maar_init() definition
>> explicitly marked "weak".  arch/mips/mti-malta/malta-memory.c provides its
>> own definition intended to override the default, but the "weak" attribute
>> on the declaration applied to this as well, so the linker chose one based
>> on link order (see 10629d711ed7 ("PCI: Remove __weak annotation from
>> pcibios_get_phb_of_node decl")).
>>
>> Remove the "weak" attribute from the declaration so we always prefer a
>> non-weak definition over the weak one, independent of link order.
>>
>> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>> CC: linux-mips@linux-mips.org

Dropping this because no MIPS folks responded.

>> ---
>>  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 6c62b0f899c0..b02891f9caaf 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
>>

[PATCH v1 05/10] MIPS: MT: Move "weak" from vpe_run() declaration to definition

[Bjorn Helgaas]

When the "weak" attribute is on a declaration in a header file, every
definition where the header is included becomes weak, and the linker
chooses one definition based on link order (see 10629d711ed7 ("PCI: Remove
__weak annotation from pcibios_get_phb_of_node decl")).

Move the "weak" attribute from the declaration to the default definition so
we always prefer a non-weak definition over the weak one, independent of
link order.

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

diff --git a/arch/mips/include/asm/vpe.h b/arch/mips/include/asm/vpe.h
index 7849f3978fea..80e70dbd1f64 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-mt.c b/arch/mips/kernel/vpe-mt.c
index 2e003b11a098..0e5899a2cd96 100644
--- a/arch/mips/kernel/vpe-mt.c
+++ b/arch/mips/kernel/vpe-mt.c
@@ -23,7 +23,7 @@ static int major;
 static int hw_tcs, hw_vpes;
 
 /* We are prepared so configure and start the VPE... */
-int vpe_run(struct vpe *v)
+int __weak vpe_run(struct vpe *v)
 {
 	unsigned long flags, val, dmt_flag;
 	struct vpe_notifications *notifier;

Re: [PATCH v1 05/10] MIPS: MT: Move "weak" from vpe_run() declaration to definition

[Bjorn Helgaas]

[+cc linux-mips]

On Wed, Oct 15, 2014 at 11:06 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> When the "weak" attribute is on a declaration in a header file, every
> definition where the header is included becomes weak, and the linker
> chooses one definition based on link order (see 10629d711ed7 ("PCI: Remove
> __weak annotation from pcibios_get_phb_of_node decl")).
>
> Move the "weak" attribute from the declaration to the default definition so
> we always prefer a non-weak definition over the weak one, independent of
> link order.
>
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> CC: linux-mips@linux-mips.org
> ---
>  arch/mips/include/asm/vpe.h |    2 +-
>  arch/mips/kernel/vpe-mt.c   |    2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/mips/include/asm/vpe.h b/arch/mips/include/asm/vpe.h
> index 7849f3978fea..80e70dbd1f64 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-mt.c b/arch/mips/kernel/vpe-mt.c
> index 2e003b11a098..0e5899a2cd96 100644
> --- a/arch/mips/kernel/vpe-mt.c
> +++ b/arch/mips/kernel/vpe-mt.c
> @@ -23,7 +23,7 @@ static int major;
>  static int hw_tcs, hw_vpes;
>
>  /* We are prepared so configure and start the VPE... */
> -int vpe_run(struct vpe *v)
> +int __weak vpe_run(struct vpe *v)
>  {
>         unsigned long flags, val, dmt_flag;
>         struct vpe_notifications *notifier;
>

Re: [PATCH v1 05/10] MIPS: MT: Move "weak" from vpe_run() declaration to definition

[Bjorn Helgaas]

[+cc Stephen]

On Wed, Oct 15, 2014 at 5:28 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> [+cc linux-mips]
>
> On Wed, Oct 15, 2014 at 11:06 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>> When the "weak" attribute is on a declaration in a header file, every
>> definition where the header is included becomes weak, and the linker
>> chooses one definition based on link order (see 10629d711ed7 ("PCI: Remove
>> __weak annotation from pcibios_get_phb_of_node decl")).
>>
>> Move the "weak" attribute from the declaration to the default definition so
>> we always prefer a non-weak definition over the weak one, independent of
>> link order.
>>
>> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>> CC: linux-mips@linux-mips.org
>> ---
>>  arch/mips/include/asm/vpe.h |    2 +-
>>  arch/mips/kernel/vpe-mt.c   |    2 +-
>>  2 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/mips/include/asm/vpe.h b/arch/mips/include/asm/vpe.h
>> index 7849f3978fea..80e70dbd1f64 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-mt.c b/arch/mips/kernel/vpe-mt.c
>> index 2e003b11a098..0e5899a2cd96 100644
>> --- a/arch/mips/kernel/vpe-mt.c
>> +++ b/arch/mips/kernel/vpe-mt.c
>> @@ -23,7 +23,7 @@ static int major;
>>  static int hw_tcs, hw_vpes;
>>
>>  /* We are prepared so configure and start the VPE... */
>> -int vpe_run(struct vpe *v)
>> +int __weak vpe_run(struct vpe *v)
>>  {
>>         unsigned long flags, val, dmt_flag;
>>         struct vpe_notifications *notifier;
>>

Just FYI, this patch was in linux-next today, but I dropped it
temporarily because Fengguang's auto-builder found the following issue
with it:

All error/warnings:

   arch/mips/kernel/vpe.c: In function 'vpe_release':
>> arch/mips/kernel/vpe.c:830:29: error: the address of 'vpe_run' will always evaluate as 'true' [-Werror=address]
      if ((vpe_elfload(v) >= 0) && vpe_run) {
                                ^
   cc1: all warnings being treated as errors

Re: [PATCH v1 05/10] MIPS: MT: Move "weak" from vpe_run() declaration to definition

[Bjorn Helgaas]

On Thu, Oct 16, 2014 at 7:49 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> [+cc Stephen]
>
> On Wed, Oct 15, 2014 at 5:28 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>> [+cc linux-mips]
>>
>> On Wed, Oct 15, 2014 at 11:06 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>>> When the "weak" attribute is on a declaration in a header file, every
>>> definition where the header is included becomes weak, and the linker
>>> chooses one definition based on link order (see 10629d711ed7 ("PCI: Remove
>>> __weak annotation from pcibios_get_phb_of_node decl")).
>>>
>>> Move the "weak" attribute from the declaration to the default definition so
>>> we always prefer a non-weak definition over the weak one, independent of
>>> link order.
>>>
>>> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>>> CC: linux-mips@linux-mips.org

Dropping this permanently since I haven't heard from any MIPS folks.

>>> ---
>>>  arch/mips/include/asm/vpe.h |    2 +-
>>>  arch/mips/kernel/vpe-mt.c   |    2 +-
>>>  2 files changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/arch/mips/include/asm/vpe.h b/arch/mips/include/asm/vpe.h
>>> index 7849f3978fea..80e70dbd1f64 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-mt.c b/arch/mips/kernel/vpe-mt.c
>>> index 2e003b11a098..0e5899a2cd96 100644
>>> --- a/arch/mips/kernel/vpe-mt.c
>>> +++ b/arch/mips/kernel/vpe-mt.c
>>> @@ -23,7 +23,7 @@ static int major;
>>>  static int hw_tcs, hw_vpes;
>>>
>>>  /* We are prepared so configure and start the VPE... */
>>> -int vpe_run(struct vpe *v)
>>> +int __weak vpe_run(struct vpe *v)
>>>  {
>>>         unsigned long flags, val, dmt_flag;
>>>         struct vpe_notifications *notifier;
>>>
>
> Just FYI, this patch was in linux-next today, but I dropped it
> temporarily because Fengguang's auto-builder found the following issue
> with it:
>
> All error/warnings:
>
>    arch/mips/kernel/vpe.c: In function 'vpe_release':
>>> arch/mips/kernel/vpe.c:830:29: error: the address of 'vpe_run' will always evaluate as 'true' [-Werror=address]
>       if ((vpe_elfload(v) >= 0) && vpe_run) {
>                                 ^
>    cc1: all warnings being treated as errors

[PATCH v1 06/10] clocksource: Remove "weak" from clocksource_default_clock() declaration

[Bjorn Helgaas]

kernel/time/jiffies.c provides a default clocksource_default_clock()
definition explicitly marked "weak".  arch/s390 provides its own definition
intended to override the default, but the "weak" attribute on the
declaration applied to the s390 definition as well, so the linker chose one
based on link order (see 10629d711ed7 ("PCI: Remove __weak annotation from
pcibios_get_phb_of_node decl")).

Remove the "weak" attribute from the clocksource_default_clock()
declaration so we always prefer a non-weak definition over the weak one,
independent of link order.

Fixes: f1b82746c1e9 ("clocksource: Cleanup clocksource selection")
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
CC: Daniel Lezcano <daniel.lezcano@linaro.org>
CC: Martin Schwidefsky <schwidefsky@de.ibm.com>
---
 include/linux/clocksource.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/clocksource.h b/include/linux/clocksource.h
index 653f0e2b6ca9..abcafaa20b86 100644
--- a/include/linux/clocksource.h
+++ b/include/linux/clocksource.h
@@ -287,7 +287,7 @@ extern struct clocksource* clocksource_get_next(void);
 extern void clocksource_change_rating(struct clocksource *cs, int rating);
 extern void clocksource_suspend(void);
 extern void clocksource_resume(void);
-extern struct clocksource * __init __weak clocksource_default_clock(void);
+extern struct clocksource * __init clocksource_default_clock(void);
 extern void clocksource_mark_unstable(struct clocksource *cs);
 
 extern u64

Re: [PATCH v1 06/10] clocksource: Remove "weak" from clocksource_default_clock() declaration

[John Stultz]

On Wed, Oct 15, 2014 at 10:06 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> kernel/time/jiffies.c provides a default clocksource_default_clock()
> definition explicitly marked "weak".  arch/s390 provides its own definition
> intended to override the default, but the "weak" attribute on the
> declaration applied to the s390 definition as well, so the linker chose one
> based on link order (see 10629d711ed7 ("PCI: Remove __weak annotation from
> pcibios_get_phb_of_node decl")).
>
> Remove the "weak" attribute from the clocksource_default_clock()
> declaration so we always prefer a non-weak definition over the weak one,
> independent of link order.
>
> Fixes: f1b82746c1e9 ("clocksource: Cleanup clocksource selection")
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> CC: Daniel Lezcano <daniel.lezcano@linaro.org>
> CC: Martin Schwidefsky <schwidefsky@de.ibm.com>

Acked-by: John Stultz <john.stultz@linaro.org>

Re: [PATCH v1 06/10] clocksource: Remove "weak" from clocksource_default_clock() declaration

[Bjorn Helgaas]

[+cc Daniel, Martin, linux-s390; sorry, I botched my "stg mail" so you
weren't included the first time.  s390 could see issues from this.]

On Wed, Oct 15, 2014 at 11:06 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> kernel/time/jiffies.c provides a default clocksource_default_clock()
> definition explicitly marked "weak".  arch/s390 provides its own definition
> intended to override the default, but the "weak" attribute on the
> declaration applied to the s390 definition as well, so the linker chose one
> based on link order (see 10629d711ed7 ("PCI: Remove __weak annotation from
> pcibios_get_phb_of_node decl")).
>
> Remove the "weak" attribute from the clocksource_default_clock()
> declaration so we always prefer a non-weak definition over the weak one,
> independent of link order.
>
> Fixes: f1b82746c1e9 ("clocksource: Cleanup clocksource selection")
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> CC: Daniel Lezcano <daniel.lezcano@linaro.org>
> CC: Martin Schwidefsky <schwidefsky@de.ibm.com>
> ---
>  include/linux/clocksource.h |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/linux/clocksource.h b/include/linux/clocksource.h
> index 653f0e2b6ca9..abcafaa20b86 100644
> --- a/include/linux/clocksource.h
> +++ b/include/linux/clocksource.h
> @@ -287,7 +287,7 @@ extern struct clocksource* clocksource_get_next(void);
>  extern void clocksource_change_rating(struct clocksource *cs, int rating);
>  extern void clocksource_suspend(void);
>  extern void clocksource_resume(void);
> -extern struct clocksource * __init __weak clocksource_default_clock(void);
> +extern struct clocksource * __init clocksource_default_clock(void);
>  extern void clocksource_mark_unstable(struct clocksource *cs);
>
>  extern u64
>

Re: [PATCH v1 06/10] clocksource: Remove "weak" from clocksource_default_clock() declaration

[Martin Schwidefsky]

On Wed, 15 Oct 2014 17:30:33 -0600
Bjorn Helgaas <bhelgaas@google.com> wrote:

> [+cc Daniel, Martin, linux-s390; sorry, I botched my "stg mail" so you
> weren't included the first time.  s390 could see issues from this.]
> 
> On Wed, Oct 15, 2014 at 11:06 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> > kernel/time/jiffies.c provides a default clocksource_default_clock()
> > definition explicitly marked "weak".  arch/s390 provides its own definition
> > intended to override the default, but the "weak" attribute on the
> > declaration applied to the s390 definition as well, so the linker chose one
> > based on link order (see 10629d711ed7 ("PCI: Remove __weak annotation from
> > pcibios_get_phb_of_node decl")).
> >
> > Remove the "weak" attribute from the clocksource_default_clock()
> > declaration so we always prefer a non-weak definition over the weak one,
> > independent of link order.
> >
> > Fixes: f1b82746c1e9 ("clocksource: Cleanup clocksource selection")
> > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> > CC: Daniel Lezcano <daniel.lezcano@linaro.org>
> > CC: Martin Schwidefsky <schwidefsky@de.ibm.com>
> > ---
> >  include/linux/clocksource.h |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/include/linux/clocksource.h b/include/linux/clocksource.h
> > index 653f0e2b6ca9..abcafaa20b86 100644
> > --- a/include/linux/clocksource.h
> > +++ b/include/linux/clocksource.h
> > @@ -287,7 +287,7 @@ extern struct clocksource* clocksource_get_next(void);
> >  extern void clocksource_change_rating(struct clocksource *cs, int rating);
> >  extern void clocksource_suspend(void);
> >  extern void clocksource_resume(void);
> > -extern struct clocksource * __init __weak clocksource_default_clock(void);
> > +extern struct clocksource * __init clocksource_default_clock(void);
> >  extern void clocksource_mark_unstable(struct clocksource *cs);
> >
> >  extern u64
> >
 
s390 compiles and boots without the __weak for clocksource_default_clock.

-- 
blue skies,
   Martin.

"Reality continues to ruin my life." - Calvin.

Re: [PATCH v1 06/10] clocksource: Remove "weak" from clocksource_default_clock() declaration

[Bjorn Helgaas]

On Thu, Oct 16, 2014 at 1:22 AM, Martin Schwidefsky
<schwidefsky@de.ibm.com> wrote:
> On Wed, 15 Oct 2014 17:30:33 -0600
> Bjorn Helgaas <bhelgaas@google.com> wrote:
>
>> [+cc Daniel, Martin, linux-s390; sorry, I botched my "stg mail" so you
>> weren't included the first time.  s390 could see issues from this.]
>>
>> On Wed, Oct 15, 2014 at 11:06 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>> > kernel/time/jiffies.c provides a default clocksource_default_clock()
>> > definition explicitly marked "weak".  arch/s390 provides its own definition
>> > intended to override the default, but the "weak" attribute on the
>> > declaration applied to the s390 definition as well, so the linker chose one
>> > based on link order (see 10629d711ed7 ("PCI: Remove __weak annotation from
>> > pcibios_get_phb_of_node decl")).
>> >
>> > Remove the "weak" attribute from the clocksource_default_clock()
>> > declaration so we always prefer a non-weak definition over the weak one,
>> > independent of link order.
>> >
>> > Fixes: f1b82746c1e9 ("clocksource: Cleanup clocksource selection")
>> > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>> > CC: Daniel Lezcano <daniel.lezcano@linaro.org>
>> > CC: Martin Schwidefsky <schwidefsky@de.ibm.com>
>> > ---
>> >  include/linux/clocksource.h |    2 +-
>> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/include/linux/clocksource.h b/include/linux/clocksource.h
>> > index 653f0e2b6ca9..abcafaa20b86 100644
>> > --- a/include/linux/clocksource.h
>> > +++ b/include/linux/clocksource.h
>> > @@ -287,7 +287,7 @@ extern struct clocksource* clocksource_get_next(void);
>> >  extern void clocksource_change_rating(struct clocksource *cs, int rating);
>> >  extern void clocksource_suspend(void);
>> >  extern void clocksource_resume(void);
>> > -extern struct clocksource * __init __weak clocksource_default_clock(void);
>> > +extern struct clocksource * __init clocksource_default_clock(void);
>> >  extern void clocksource_mark_unstable(struct clocksource *cs);
>> >
>> >  extern u64
>> >
>
> s390 compiles and boots without the __weak for clocksource_default_clock.

I assume this means you've tested this patch and s390 compiles and boots?

I assume you *don't* mean that s390 could drop its
clocksource_default_clock() implementation and use the generic one,
right?

Bjorn

Re: [PATCH v1 06/10] clocksource: Remove "weak" from clocksource_default_clock() declaration

[Martin Schwidefsky]

On Thu, 16 Oct 2014 07:40:16 -0600
Bjorn Helgaas <bhelgaas@google.com> wrote:

> On Thu, Oct 16, 2014 at 1:22 AM, Martin Schwidefsky
> <schwidefsky@de.ibm.com> wrote:
> > On Wed, 15 Oct 2014 17:30:33 -0600
> > Bjorn Helgaas <bhelgaas@google.com> wrote:
> >
> >> [+cc Daniel, Martin, linux-s390; sorry, I botched my "stg mail" so you
> >> weren't included the first time.  s390 could see issues from this.]
> >>
> >> On Wed, Oct 15, 2014 at 11:06 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> >> > kernel/time/jiffies.c provides a default clocksource_default_clock()
> >> > definition explicitly marked "weak".  arch/s390 provides its own definition
> >> > intended to override the default, but the "weak" attribute on the
> >> > declaration applied to the s390 definition as well, so the linker chose one
> >> > based on link order (see 10629d711ed7 ("PCI: Remove __weak annotation from
> >> > pcibios_get_phb_of_node decl")).
> >> >
> >> > Remove the "weak" attribute from the clocksource_default_clock()
> >> > declaration so we always prefer a non-weak definition over the weak one,
> >> > independent of link order.
> >> >
> >> > Fixes: f1b82746c1e9 ("clocksource: Cleanup clocksource selection")
> >> > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> >> > CC: Daniel Lezcano <daniel.lezcano@linaro.org>
> >> > CC: Martin Schwidefsky <schwidefsky@de.ibm.com>
> >> > ---
> >> >  include/linux/clocksource.h |    2 +-
> >> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >> >
> >> > diff --git a/include/linux/clocksource.h b/include/linux/clocksource.h
> >> > index 653f0e2b6ca9..abcafaa20b86 100644
> >> > --- a/include/linux/clocksource.h
> >> > +++ b/include/linux/clocksource.h
> >> > @@ -287,7 +287,7 @@ extern struct clocksource* clocksource_get_next(void);
> >> >  extern void clocksource_change_rating(struct clocksource *cs, int rating);
> >> >  extern void clocksource_suspend(void);
> >> >  extern void clocksource_resume(void);
> >> > -extern struct clocksource * __init __weak clocksource_default_clock(void);
> >> > +extern struct clocksource * __init clocksource_default_clock(void);
> >> >  extern void clocksource_mark_unstable(struct clocksource *cs);
> >> >
> >> >  extern u64
> >> >
> >
> > s390 compiles and boots without the __weak for clocksource_default_clock.
> 
> I assume this means you've tested this patch and s390 compiles and boots?

Correct.
 
> I assume you *don't* mean that s390 could drop its
> clocksource_default_clock() implementation and use the generic one,
> right?

Sorry, but I want to keep the s390 TOD clock as default.
The jiffies clock is not precise enough, even if it is used only at the beginning.

-- 
blue skies,
   Martin.

"Reality continues to ruin my life." - Calvin.

Re: [PATCH v1 06/10] clocksource: Remove "weak" from clocksource_default_clock() declaration

[Ingo Molnar]

* Bjorn Helgaas <bhelgaas@google.com> wrote:

> kernel/time/jiffies.c provides a default clocksource_default_clock()
> definition explicitly marked "weak".  arch/s390 provides its own definition
> intended to override the default, but the "weak" attribute on the
> declaration applied to the s390 definition as well, so the linker chose one
> based on link order (see 10629d711ed7 ("PCI: Remove __weak annotation from
> pcibios_get_phb_of_node decl")).
> 
> Remove the "weak" attribute from the clocksource_default_clock()
> declaration so we always prefer a non-weak definition over the weak one,
> independent of link order.
> 
> Fixes: f1b82746c1e9 ("clocksource: Cleanup clocksource selection")
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> CC: Daniel Lezcano <daniel.lezcano@linaro.org>
> CC: Martin Schwidefsky <schwidefsky@de.ibm.com>
> ---
>  include/linux/clocksource.h |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/linux/clocksource.h b/include/linux/clocksource.h
> index 653f0e2b6ca9..abcafaa20b86 100644
> --- a/include/linux/clocksource.h
> +++ b/include/linux/clocksource.h
> @@ -287,7 +287,7 @@ extern struct clocksource* clocksource_get_next(void);
>  extern void clocksource_change_rating(struct clocksource *cs, int rating);
>  extern void clocksource_suspend(void);
>  extern void clocksource_resume(void);
> -extern struct clocksource * __init __weak clocksource_default_clock(void);
> +extern struct clocksource * __init clocksource_default_clock(void);
>  extern void clocksource_mark_unstable(struct clocksource *cs);

Acked-by: Ingo Molnar <mingo@kernel.org>

Thanks,

	Ingo

[PATCH v1 07/10] vmcore: Remove "weak" from function declarations

[Bjorn Helgaas]

For the following functions:

  elfcorehdr_alloc()
  elfcorehdr_free()
  elfcorehdr_read()
  elfcorehdr_read_notes()
  remap_oldmem_pfn_range()

fs/proc/vmcore.c provides default definitions explicitly marked "weak".
arch/s390 provides its own definitions intended to override the default
ones, but the "weak" attribute on the declarations applied to the s390
definitions as well, so the linker chose one based on link order (see
10629d711ed7 ("PCI: Remove __weak annotation from pcibios_get_phb_of_node
decl")).

Remove the "weak" attribute from the declarations so we always prefer a
non-weak definition over the weak one, independent of link order.

Fixes: be8a8d069e50 ("vmcore: introduce ELF header in new memory feature")
Fixes: 9cb218131de1 ("vmcore: introduce remap_oldmem_pfn_range()")
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
CC: Michael Holzheu <holzheu@linux.vnet.ibm.com>
CC: Vivek Goyal <vgoyal@redhat.com>
---
 include/linux/crash_dump.h |   15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/include/linux/crash_dump.h b/include/linux/crash_dump.h
index 72ab536ad3de..3849fce7ecfe 100644
--- a/include/linux/crash_dump.h
+++ b/include/linux/crash_dump.h
@@ -14,14 +14,13 @@
 extern unsigned long long elfcorehdr_addr;
 extern unsigned long long elfcorehdr_size;
 
-extern int __weak elfcorehdr_alloc(unsigned long long *addr,
-				   unsigned long long *size);
-extern void __weak elfcorehdr_free(unsigned long long addr);
-extern ssize_t __weak elfcorehdr_read(char *buf, size_t count, u64 *ppos);
-extern ssize_t __weak elfcorehdr_read_notes(char *buf, size_t count, u64 *ppos);
-extern int __weak remap_oldmem_pfn_range(struct vm_area_struct *vma,
-					 unsigned long from, unsigned long pfn,
-					 unsigned long size, pgprot_t prot);
+extern int elfcorehdr_alloc(unsigned long long *addr, unsigned long long *size);
+extern void elfcorehdr_free(unsigned long long addr);
+extern ssize_t elfcorehdr_read(char *buf, size_t count, u64 *ppos);
+extern ssize_t elfcorehdr_read_notes(char *buf, size_t count, u64 *ppos);
+extern int remap_oldmem_pfn_range(struct vm_area_struct *vma,
+				  unsigned long from, unsigned long pfn,
+				  unsigned long size, pgprot_t prot);
 
 extern ssize_t copy_oldmem_page(unsigned long, char *, size_t,
 						unsigned long, int);

Re: [PATCH v1 07/10] vmcore: Remove "weak" from function declarations

[Bjorn Helgaas]

[+cc Michael, Vivek; sorry, I botched my "stg mail" so you weren't
included the first time]

On Wed, Oct 15, 2014 at 11:06 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> For the following functions:
>
>   elfcorehdr_alloc()
>   elfcorehdr_free()
>   elfcorehdr_read()
>   elfcorehdr_read_notes()
>   remap_oldmem_pfn_range()
>
> fs/proc/vmcore.c provides default definitions explicitly marked "weak".
> arch/s390 provides its own definitions intended to override the default
> ones, but the "weak" attribute on the declarations applied to the s390
> definitions as well, so the linker chose one based on link order (see
> 10629d711ed7 ("PCI: Remove __weak annotation from pcibios_get_phb_of_node
> decl")).
>
> Remove the "weak" attribute from the declarations so we always prefer a
> non-weak definition over the weak one, independent of link order.
>
> Fixes: be8a8d069e50 ("vmcore: introduce ELF header in new memory feature")
> Fixes: 9cb218131de1 ("vmcore: introduce remap_oldmem_pfn_range()")
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> CC: Michael Holzheu <holzheu@linux.vnet.ibm.com>
> CC: Vivek Goyal <vgoyal@redhat.com>
> ---
>  include/linux/crash_dump.h |   15 +++++++--------
>  1 file changed, 7 insertions(+), 8 deletions(-)
>
> diff --git a/include/linux/crash_dump.h b/include/linux/crash_dump.h
> index 72ab536ad3de..3849fce7ecfe 100644
> --- a/include/linux/crash_dump.h
> +++ b/include/linux/crash_dump.h
> @@ -14,14 +14,13 @@
>  extern unsigned long long elfcorehdr_addr;
>  extern unsigned long long elfcorehdr_size;
>
> -extern int __weak elfcorehdr_alloc(unsigned long long *addr,
> -                                  unsigned long long *size);
> -extern void __weak elfcorehdr_free(unsigned long long addr);
> -extern ssize_t __weak elfcorehdr_read(char *buf, size_t count, u64 *ppos);
> -extern ssize_t __weak elfcorehdr_read_notes(char *buf, size_t count, u64 *ppos);
> -extern int __weak remap_oldmem_pfn_range(struct vm_area_struct *vma,
> -                                        unsigned long from, unsigned long pfn,
> -                                        unsigned long size, pgprot_t prot);
> +extern int elfcorehdr_alloc(unsigned long long *addr, unsigned long long *size);
> +extern void elfcorehdr_free(unsigned long long addr);
> +extern ssize_t elfcorehdr_read(char *buf, size_t count, u64 *ppos);
> +extern ssize_t elfcorehdr_read_notes(char *buf, size_t count, u64 *ppos);
> +extern int remap_oldmem_pfn_range(struct vm_area_struct *vma,
> +                                 unsigned long from, unsigned long pfn,
> +                                 unsigned long size, pgprot_t prot);
>
>  extern ssize_t copy_oldmem_page(unsigned long, char *, size_t,
>                                                 unsigned long, int);
>

Re: [PATCH v1 07/10] vmcore: Remove "weak" from function declarations

[Bjorn Helgaas]

[+cc Martin, Heiko, linux-s390, since s390 could see issues from this.
Sorry for all the spam; I should have gotten this right the first
time.]

On Wed, Oct 15, 2014 at 5:31 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> [+cc Michael, Vivek; sorry, I botched my "stg mail" so you weren't
> included the first time]
>
> On Wed, Oct 15, 2014 at 11:06 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>> For the following functions:
>>
>>   elfcorehdr_alloc()
>>   elfcorehdr_free()
>>   elfcorehdr_read()
>>   elfcorehdr_read_notes()
>>   remap_oldmem_pfn_range()
>>
>> fs/proc/vmcore.c provides default definitions explicitly marked "weak".
>> arch/s390 provides its own definitions intended to override the default
>> ones, but the "weak" attribute on the declarations applied to the s390
>> definitions as well, so the linker chose one based on link order (see
>> 10629d711ed7 ("PCI: Remove __weak annotation from pcibios_get_phb_of_node
>> decl")).
>>
>> Remove the "weak" attribute from the declarations so we always prefer a
>> non-weak definition over the weak one, independent of link order.
>>
>> Fixes: be8a8d069e50 ("vmcore: introduce ELF header in new memory feature")
>> Fixes: 9cb218131de1 ("vmcore: introduce remap_oldmem_pfn_range()")
>> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>> CC: Michael Holzheu <holzheu@linux.vnet.ibm.com>
>> CC: Vivek Goyal <vgoyal@redhat.com>
>> ---
>>  include/linux/crash_dump.h |   15 +++++++--------
>>  1 file changed, 7 insertions(+), 8 deletions(-)
>>
>> diff --git a/include/linux/crash_dump.h b/include/linux/crash_dump.h
>> index 72ab536ad3de..3849fce7ecfe 100644
>> --- a/include/linux/crash_dump.h
>> +++ b/include/linux/crash_dump.h
>> @@ -14,14 +14,13 @@
>>  extern unsigned long long elfcorehdr_addr;
>>  extern unsigned long long elfcorehdr_size;
>>
>> -extern int __weak elfcorehdr_alloc(unsigned long long *addr,
>> -                                  unsigned long long *size);
>> -extern void __weak elfcorehdr_free(unsigned long long addr);
>> -extern ssize_t __weak elfcorehdr_read(char *buf, size_t count, u64 *ppos);
>> -extern ssize_t __weak elfcorehdr_read_notes(char *buf, size_t count, u64 *ppos);
>> -extern int __weak remap_oldmem_pfn_range(struct vm_area_struct *vma,
>> -                                        unsigned long from, unsigned long pfn,
>> -                                        unsigned long size, pgprot_t prot);
>> +extern int elfcorehdr_alloc(unsigned long long *addr, unsigned long long *size);
>> +extern void elfcorehdr_free(unsigned long long addr);
>> +extern ssize_t elfcorehdr_read(char *buf, size_t count, u64 *ppos);
>> +extern ssize_t elfcorehdr_read_notes(char *buf, size_t count, u64 *ppos);
>> +extern int remap_oldmem_pfn_range(struct vm_area_struct *vma,
>> +                                 unsigned long from, unsigned long pfn,
>> +                                 unsigned long size, pgprot_t prot);
>>
>>  extern ssize_t copy_oldmem_page(unsigned long, char *, size_t,
>>                                                 unsigned long, int);
>>

Re: [PATCH v1 07/10] vmcore: Remove "weak" from function declarations

[Martin Schwidefsky]

On Wed, 15 Oct 2014 17:44:29 -0600
Bjorn Helgaas <bhelgaas@google.com> wrote:

> [+cc Martin, Heiko, linux-s390, since s390 could see issues from this.
> Sorry for all the spam; I should have gotten this right the first
> time.]
> 
> On Wed, Oct 15, 2014 at 5:31 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> > [+cc Michael, Vivek; sorry, I botched my "stg mail" so you weren't
> > included the first time]
> >
> > On Wed, Oct 15, 2014 at 11:06 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> >> For the following functions:
> >>
> >>   elfcorehdr_alloc()
> >>   elfcorehdr_free()
> >>   elfcorehdr_read()
> >>   elfcorehdr_read_notes()
> >>   remap_oldmem_pfn_range()
> >>
> >> fs/proc/vmcore.c provides default definitions explicitly marked "weak".
> >> arch/s390 provides its own definitions intended to override the default
> >> ones, but the "weak" attribute on the declarations applied to the s390
> >> definitions as well, so the linker chose one based on link order (see
> >> 10629d711ed7 ("PCI: Remove __weak annotation from pcibios_get_phb_of_node
> >> decl")).
> >>
> >> Remove the "weak" attribute from the declarations so we always prefer a
> >> non-weak definition over the weak one, independent of link order.
> >>
> >> Fixes: be8a8d069e50 ("vmcore: introduce ELF header in new memory feature")
> >> Fixes: 9cb218131de1 ("vmcore: introduce remap_oldmem_pfn_range()")
> >> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> >> CC: Michael Holzheu <holzheu@linux.vnet.ibm.com>
> >> CC: Vivek Goyal <vgoyal@redhat.com>
> >> ---
> >>  include/linux/crash_dump.h |   15 +++++++--------
> >>  1 file changed, 7 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/include/linux/crash_dump.h b/include/linux/crash_dump.h
> >> index 72ab536ad3de..3849fce7ecfe 100644
> >> --- a/include/linux/crash_dump.h
> >> +++ b/include/linux/crash_dump.h
> >> @@ -14,14 +14,13 @@
> >>  extern unsigned long long elfcorehdr_addr;
> >>  extern unsigned long long elfcorehdr_size;
> >>
> >> -extern int __weak elfcorehdr_alloc(unsigned long long *addr,
> >> -                                  unsigned long long *size);
> >> -extern void __weak elfcorehdr_free(unsigned long long addr);
> >> -extern ssize_t __weak elfcorehdr_read(char *buf, size_t count, u64 *ppos);
> >> -extern ssize_t __weak elfcorehdr_read_notes(char *buf, size_t count, u64 *ppos);
> >> -extern int __weak remap_oldmem_pfn_range(struct vm_area_struct *vma,
> >> -                                        unsigned long from, unsigned long pfn,
> >> -                                        unsigned long size, pgprot_t prot);
> >> +extern int elfcorehdr_alloc(unsigned long long *addr, unsigned long long *size);
> >> +extern void elfcorehdr_free(unsigned long long addr);
> >> +extern ssize_t elfcorehdr_read(char *buf, size_t count, u64 *ppos);
> >> +extern ssize_t elfcorehdr_read_notes(char *buf, size_t count, u64 *ppos);
> >> +extern int remap_oldmem_pfn_range(struct vm_area_struct *vma,
> >> +                                 unsigned long from, unsigned long pfn,
> >> +                                 unsigned long size, pgprot_t prot);
> >>
> >>  extern ssize_t copy_oldmem_page(unsigned long, char *, size_t,
> >>                                                 unsigned long, int);
> >>
 
These __weak attributes can be removed as well without ill effect for s390.

-- 
blue skies,
   Martin.

"Reality continues to ruin my life." - Calvin.

Re: [PATCH v1 07/10] vmcore: Remove "weak" from function declarations

[Vivek Goyal]

On Wed, Oct 15, 2014 at 05:44:29PM -0600, Bjorn Helgaas wrote:
> [+cc Martin, Heiko, linux-s390, since s390 could see issues from this.
> Sorry for all the spam; I should have gotten this right the first
> time.]
> 
> On Wed, Oct 15, 2014 at 5:31 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> > [+cc Michael, Vivek; sorry, I botched my "stg mail" so you weren't
> > included the first time]
> >
> > On Wed, Oct 15, 2014 at 11:06 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> >> For the following functions:
> >>
> >>   elfcorehdr_alloc()
> >>   elfcorehdr_free()
> >>   elfcorehdr_read()
> >>   elfcorehdr_read_notes()
> >>   remap_oldmem_pfn_range()
> >>
> >> fs/proc/vmcore.c provides default definitions explicitly marked "weak".
> >> arch/s390 provides its own definitions intended to override the default
> >> ones, but the "weak" attribute on the declarations applied to the s390
> >> definitions as well, so the linker chose one based on link order (see
> >> 10629d711ed7 ("PCI: Remove __weak annotation from pcibios_get_phb_of_node
> >> decl")).
> >>
> >> Remove the "weak" attribute from the declarations so we always prefer a
> >> non-weak definition over the weak one, independent of link order.
> >>
> >> Fixes: be8a8d069e50 ("vmcore: introduce ELF header in new memory feature")
> >> Fixes: 9cb218131de1 ("vmcore: introduce remap_oldmem_pfn_range()")
> >> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> >> CC: Michael Holzheu <holzheu@linux.vnet.ibm.com>
> >> CC: Vivek Goyal <vgoyal@redhat.com>

Looks good to me.

Acked-by: Vivek Goyal <vgoyal@redhat.com>

Thanks
Vivek

> >> ---
> >>  include/linux/crash_dump.h |   15 +++++++--------
> >>  1 file changed, 7 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/include/linux/crash_dump.h b/include/linux/crash_dump.h
> >> index 72ab536ad3de..3849fce7ecfe 100644
> >> --- a/include/linux/crash_dump.h
> >> +++ b/include/linux/crash_dump.h
> >> @@ -14,14 +14,13 @@
> >>  extern unsigned long long elfcorehdr_addr;
> >>  extern unsigned long long elfcorehdr_size;
> >>
> >> -extern int __weak elfcorehdr_alloc(unsigned long long *addr,
> >> -                                  unsigned long long *size);
> >> -extern void __weak elfcorehdr_free(unsigned long long addr);
> >> -extern ssize_t __weak elfcorehdr_read(char *buf, size_t count, u64 *ppos);
> >> -extern ssize_t __weak elfcorehdr_read_notes(char *buf, size_t count, u64 *ppos);
> >> -extern int __weak remap_oldmem_pfn_range(struct vm_area_struct *vma,
> >> -                                        unsigned long from, unsigned long pfn,
> >> -                                        unsigned long size, pgprot_t prot);
> >> +extern int elfcorehdr_alloc(unsigned long long *addr, unsigned long long *size);
> >> +extern void elfcorehdr_free(unsigned long long addr);
> >> +extern ssize_t elfcorehdr_read(char *buf, size_t count, u64 *ppos);
> >> +extern ssize_t elfcorehdr_read_notes(char *buf, size_t count, u64 *ppos);
> >> +extern int remap_oldmem_pfn_range(struct vm_area_struct *vma,
> >> +                                 unsigned long from, unsigned long pfn,
> >> +                                 unsigned long size, pgprot_t prot);
> >>
> >>  extern ssize_t copy_oldmem_page(unsigned long, char *, size_t,
> >>                                                 unsigned long, int);
> >>

[PATCH v1 08/10] kgdb: Remove "weak" from kgdb_arch_pc() declaration

[Bjorn Helgaas]

kernel/debug/debug_core.c provides a default kgdb_arch_pc() definition
explicitly marked "weak".  Several architectures provide their own
definitions intended to override the default, but the "weak" attribute on
the declaration applied to the arch definitions as well, so the linker
chose one based on link order (see 10629d711ed7 ("PCI: Remove __weak
annotation from pcibios_get_phb_of_node decl")).

Remove the "weak" attribute from the declaration so we always prefer a
non-weak definition over the weak one, independent of link order.

Fixes: 688b744d8bc8 ("kgdb: fix signedness mixmatches, add statics, add declaration to header")
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
CC: Harvey Harrison <harvey.harrison@gmail.com>
---
 include/linux/kgdb.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/kgdb.h b/include/linux/kgdb.h
index 6b06d378f3df..e465bb15912d 100644
--- a/include/linux/kgdb.h
+++ b/include/linux/kgdb.h
@@ -283,7 +283,7 @@ struct kgdb_io {
 
 extern struct kgdb_arch		arch_kgdb_ops;
 
-extern unsigned long __weak kgdb_arch_pc(int exception, struct pt_regs *regs);
+extern unsigned long kgdb_arch_pc(int exception, struct pt_regs *regs);
 
 #ifdef CONFIG_SERIAL_KGDB_NMI
 extern int kgdb_register_nmi_console(void);

Re: [PATCH v1 08/10] kgdb: Remove "weak" from kgdb_arch_pc() declaration

[Bjorn Helgaas]

[+cc Harvey, Vineet, linux-sh; arc, sh, and x86 have kgdb_arch_pc()
definitions and could see issues from this]

On Wed, Oct 15, 2014 at 11:06 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> kernel/debug/debug_core.c provides a default kgdb_arch_pc() definition
> explicitly marked "weak".  Several architectures provide their own
> definitions intended to override the default, but the "weak" attribute on
> the declaration applied to the arch definitions as well, so the linker
> chose one based on link order (see 10629d711ed7 ("PCI: Remove __weak
> annotation from pcibios_get_phb_of_node decl")).
>
> Remove the "weak" attribute from the declaration so we always prefer a
> non-weak definition over the weak one, independent of link order.
>
> Fixes: 688b744d8bc8 ("kgdb: fix signedness mixmatches, add statics, add declaration to header")
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> CC: Harvey Harrison <harvey.harrison@gmail.com>
> ---
>  include/linux/kgdb.h |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/linux/kgdb.h b/include/linux/kgdb.h
> index 6b06d378f3df..e465bb15912d 100644
> --- a/include/linux/kgdb.h
> +++ b/include/linux/kgdb.h
> @@ -283,7 +283,7 @@ struct kgdb_io {
>
>  extern struct kgdb_arch                arch_kgdb_ops;
>
> -extern unsigned long __weak kgdb_arch_pc(int exception, struct pt_regs *regs);
> +extern unsigned long kgdb_arch_pc(int exception, struct pt_regs *regs);
>
>  #ifdef CONFIG_SERIAL_KGDB_NMI
>  extern int kgdb_register_nmi_console(void);
>

Re: [PATCH v1 08/10] kgdb: Remove "weak" from kgdb_arch_pc() declaration

[Vineet Gupta]

Hi Bjorn,

On Thursday 16 October 2014 05:05 AM, Bjorn Helgaas wrote:
> [+cc Harvey, Vineet, linux-sh; arc, sh, and x86 have kgdb_arch_pc()
> definitions and could see issues from this]

Thanks for the heads up !

I compile tested arc tree (with kgdb) from linux-next of today, which has this
patch already and things seem to be ok.

As a hack I reverted the patch and see that kgdb_arch_pc() becomes weak, although
ARC version was still being pulled in final - it was recored as weak and that
cause bloat-o-meter to go awry.

bloat-o-meter vmlinux-with-fix vmlinux-before-fix
add/remove: 0/1 grow/shrink: 1/0 up/down: 1/-8 (-7)
function                                     old     new   delta
vermagic                                      36      37      +1
kgdb_arch_pc                                 8       -      -8

So functionally we are same as before - which is good.
If you need it.

Tested-by: Vineet Gupta <vgupta@synopsys.com>   #for ARC build

Thx,
-Vineet
>
> On Wed, Oct 15, 2014 at 11:06 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>> kernel/debug/debug_core.c provides a default kgdb_arch_pc() definition
>> explicitly marked "weak".  Several architectures provide their own
>> definitions intended to override the default, but the "weak" attribute on
>> the declaration applied to the arch definitions as well, so the linker
>> chose one based on link order (see 10629d711ed7 ("PCI: Remove __weak
>> annotation from pcibios_get_phb_of_node decl")).
>>
>> Remove the "weak" attribute from the declaration so we always prefer a
>> non-weak definition over the weak one, independent of link order.
>>
>> Fixes: 688b744d8bc8 ("kgdb: fix signedness mixmatches, add statics, add declaration to header")
>> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>> CC: Harvey Harrison <harvey.harrison@gmail.com>
>> ---
>>  include/linux/kgdb.h |    2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/include/linux/kgdb.h b/include/linux/kgdb.h
>> index 6b06d378f3df..e465bb15912d 100644
>> --- a/include/linux/kgdb.h
>> +++ b/include/linux/kgdb.h
>> @@ -283,7 +283,7 @@ struct kgdb_io {
>>
>>  extern struct kgdb_arch                arch_kgdb_ops;
>>
>> -extern unsigned long __weak kgdb_arch_pc(int exception, struct pt_regs *regs);
>> +extern unsigned long kgdb_arch_pc(int exception, struct pt_regs *regs);
>>
>>  #ifdef CONFIG_SERIAL_KGDB_NMI
>>  extern int kgdb_register_nmi_console(void);
>>

Re: [PATCH v1 08/10] kgdb: Remove "weak" from kgdb_arch_pc() declaration

[Vineet Gupta]

On Thursday 16 October 2014 07:08 PM, Vineet Gupta wrote:
> On Thursday 16 October 2014 05:05 AM, Bjorn Helgaas wrote:
>> > [+cc Harvey, Vineet, linux-sh; arc, sh, and x86 have kgdb_arch_pc()
>> > definitions and could see issues from this]
> Thanks for the heads up !
> 
> I compile tested arc tree (with kgdb) from linux-next of today, which has this
> patch already and things seem to be ok.
> 
> As a hack I reverted the patch and see that kgdb_arch_pc() becomes weak, although
> ARC version was still being pulled in final - it was recored as weak and that
> cause bloat-o-meter to go awry.
> 
> bloat-o-meter vmlinux-with-fix vmlinux-before-fix
> add/remove: 0/1 grow/shrink: 1/0 up/down: 1/-8 (-7)
> function                                     old     new   delta
> vermagic                                      36      37      +1
> kgdb_arch_pc                                 8       -      -8

The ARC definition of kgdb_arch_pc is same as generic one so we might as well drop it.
Can you add the patch below to your series. If not I can add it to ARC changes for
this merge window. Please let me know.

Thx,
-Vineet

---------------------->
>From f01b1db6986924e794eddc038167329d63d1fda9 Mon Sep 17 00:00:00 2001
From: Vineet Gupta <vgupta@synopsys.com>
Date: Fri, 17 Oct 2014 13:27:03 +0530
Subject: [PATCH] ARC: kgdb: generic kgdb_arch_pc() suffices

Signed-off-by: Vineet Gupta <vgupta@synopsys.com>
---
 arch/arc/kernel/kgdb.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/arch/arc/kernel/kgdb.c b/arch/arc/kernel/kgdb.c
index a2ff5c5d1450..ecf6a7869375 100644
--- a/arch/arc/kernel/kgdb.c
+++ b/arch/arc/kernel/kgdb.c
@@ -158,11 +158,6 @@ int kgdb_arch_handle_exception(int e_vector, int signo, int
err_code,
 	return -1;
 }

-unsigned long kgdb_arch_pc(int exception, struct pt_regs *regs)
-{
-	return instruction_pointer(regs);
-}
-
 int kgdb_arch_init(void)
 {
 	single_step_data.armed = 0;
-- 
1.8.3.2

Re: [PATCH v1 08/10] kgdb: Remove "weak" from kgdb_arch_pc() declaration

[Bjorn Helgaas]

On Fri, Oct 17, 2014 at 1:59 AM, Vineet Gupta
<Vineet.Gupta1@synopsys.com> wrote:
> On Thursday 16 October 2014 07:08 PM, Vineet Gupta wrote:
>> On Thursday 16 October 2014 05:05 AM, Bjorn Helgaas wrote:
>>> > [+cc Harvey, Vineet, linux-sh; arc, sh, and x86 have kgdb_arch_pc()
>>> > definitions and could see issues from this]
>> Thanks for the heads up !
>>
>> I compile tested arc tree (with kgdb) from linux-next of today, which has this
>> patch already and things seem to be ok.
>>
>> As a hack I reverted the patch and see that kgdb_arch_pc() becomes weak, although
>> ARC version was still being pulled in final - it was recored as weak and that
>> cause bloat-o-meter to go awry.
>>
>> bloat-o-meter vmlinux-with-fix vmlinux-before-fix
>> add/remove: 0/1 grow/shrink: 1/0 up/down: 1/-8 (-7)
>> function                                     old     new   delta
>> vermagic                                      36      37      +1
>> kgdb_arch_pc                                 8       -      -8
>
> The ARC definition of kgdb_arch_pc is same as generic one so we might as well drop it.
> Can you add the patch below to your series. If not I can add it to ARC changes for
> this merge window. Please let me know.

I added the patch below to my series.  The merge window just closed,
but I'll try to merge these as fixes before v3.18.  Thanks!

Bjorn

> ---------------------->
> From f01b1db6986924e794eddc038167329d63d1fda9 Mon Sep 17 00:00:00 2001
> From: Vineet Gupta <vgupta@synopsys.com>
> Date: Fri, 17 Oct 2014 13:27:03 +0530
> Subject: [PATCH] ARC: kgdb: generic kgdb_arch_pc() suffices
>
> Signed-off-by: Vineet Gupta <vgupta@synopsys.com>
> ---
>  arch/arc/kernel/kgdb.c | 5 -----
>  1 file changed, 5 deletions(-)
>
> diff --git a/arch/arc/kernel/kgdb.c b/arch/arc/kernel/kgdb.c
> index a2ff5c5d1450..ecf6a7869375 100644
> --- a/arch/arc/kernel/kgdb.c
> +++ b/arch/arc/kernel/kgdb.c
> @@ -158,11 +158,6 @@ int kgdb_arch_handle_exception(int e_vector, int signo, int
> err_code,
>         return -1;
>  }
>
> -unsigned long kgdb_arch_pc(int exception, struct pt_regs *regs)
> -{
> -       return instruction_pointer(regs);
> -}
> -
>  int kgdb_arch_init(void)
>  {
>         single_step_data.armed = 0;
> --
> 1.8.3.2

Re: [PATCH v1 08/10] kgdb: Remove "weak" from kgdb_arch_pc() declaration

[Harvey Harrison]

On Wed, Oct 15, 2014 at 10:06 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>
> kernel/debug/debug_core.c provides a default kgdb_arch_pc() definition
> explicitly marked "weak".  Several architectures provide their own
> definitions intended to override the default, but the "weak" attribute on
> the declaration applied to the arch definitions as well, so the linker
> chose one based on link order (see 10629d711ed7 ("PCI: Remove __weak
> annotation from pcibios_get_phb_of_node decl")).
>
> Remove the "weak" attribute from the declaration so we always prefer a
> non-weak definition over the weak one, independent of link order.
>
> Fixes: 688b744d8bc8 ("kgdb: fix signedness mixmatches, add statics, add declaration to header")
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> CC: Harvey Harrison <harvey.harrison@gmail.com>

Reviewed-by: Harvey Harrison <harvey.harrison@gmail.com>

This was likely simply an error in my patch, likely just copied the
function definition and didn't even notice the
weak annotation at the time.

Harvey

[PATCH v1 09/10] memory-hotplug: Remove "weak" from memory_block_size_bytes() declaration

[Bjorn Helgaas]

drivers/base/memory.c provides a default memory_block_size_bytes()
definition explicitly marked "weak".  Several architectures provide their
own definitions intended to override the default, but the "weak" attribute
on the declaration applied to the arch definitions as well, so the linker
chose one based on link order (see 10629d711ed7 ("PCI: Remove __weak
annotation from pcibios_get_phb_of_node decl")).

Remove the "weak" attribute from the declaration so we always prefer a
non-weak definition over the weak one, independent of link order.

Fixes: 41f107266b19 ("drivers: base: Add prototype declaration to the header file")
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
CC: Rashika Kheria <rashika.kheria@gmail.com>
CC: Nathan Fontenot <nfont@austin.ibm.com>
CC: Anton Blanchard <anton@au1.ibm.com>
CC: Heiko Carstens <heiko.carstens@de.ibm.com>
CC: Yinghai Lu <yinghai@kernel.org>
---
 include/linux/memory.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/memory.h b/include/linux/memory.h
index bb7384e3c3d8..8b8d8d12348e 100644
--- a/include/linux/memory.h
+++ b/include/linux/memory.h
@@ -35,7 +35,7 @@ struct memory_block {
 };
 
 int arch_get_memory_phys_device(unsigned long start_pfn);
-unsigned long __weak memory_block_size_bytes(void);
+unsigned long memory_block_size_bytes(void);
 
 /* These states are exposed to userspace as text strings in sysfs */
 #define	MEM_ONLINE		(1<<0) /* exposed to userspace */

Re: [PATCH v1 09/10] memory-hotplug: Remove "weak" from memory_block_size_bytes() declaration

[Bjorn Helgaas]

[+cc Rashika, Nathan, Anton, Blanchard, Heiko, Yinghai, Martin,
linux-s390; sorry, I botched my "stg mail" so you weren't included the
first time.  s390 and x86 define their own memory_block_size_bytes()
and are at risk for this problem.]

On Wed, Oct 15, 2014 at 11:06 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> drivers/base/memory.c provides a default memory_block_size_bytes()
> definition explicitly marked "weak".  Several architectures provide their
> own definitions intended to override the default, but the "weak" attribute
> on the declaration applied to the arch definitions as well, so the linker
> chose one based on link order (see 10629d711ed7 ("PCI: Remove __weak
> annotation from pcibios_get_phb_of_node decl")).
>
> Remove the "weak" attribute from the declaration so we always prefer a
> non-weak definition over the weak one, independent of link order.
>
> Fixes: 41f107266b19 ("drivers: base: Add prototype declaration to the header file")
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> CC: Rashika Kheria <rashika.kheria@gmail.com>
> CC: Nathan Fontenot <nfont@austin.ibm.com>
> CC: Anton Blanchard <anton@au1.ibm.com>
> CC: Heiko Carstens <heiko.carstens@de.ibm.com>
> CC: Yinghai Lu <yinghai@kernel.org>
> ---
>  include/linux/memory.h |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/linux/memory.h b/include/linux/memory.h
> index bb7384e3c3d8..8b8d8d12348e 100644
> --- a/include/linux/memory.h
> +++ b/include/linux/memory.h
> @@ -35,7 +35,7 @@ struct memory_block {
>  };
>
>  int arch_get_memory_phys_device(unsigned long start_pfn);
> -unsigned long __weak memory_block_size_bytes(void);
> +unsigned long memory_block_size_bytes(void);
>
>  /* These states are exposed to userspace as text strings in sysfs */
>  #define        MEM_ONLINE              (1<<0) /* exposed to userspace */
>

Re: [PATCH v1 09/10] memory-hotplug: Remove "weak" from memory_block_size_bytes() declaration

[Martin Schwidefsky]

On Wed, 15 Oct 2014 17:38:02 -0600
Bjorn Helgaas <bhelgaas@google.com> wrote:

> [+cc Rashika, Nathan, Anton, Blanchard, Heiko, Yinghai, Martin,
> linux-s390; sorry, I botched my "stg mail" so you weren't included the
> first time.  s390 and x86 define their own memory_block_size_bytes()
> and are at risk for this problem.]
> 
> On Wed, Oct 15, 2014 at 11:06 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> > drivers/base/memory.c provides a default memory_block_size_bytes()
> > definition explicitly marked "weak".  Several architectures provide their
> > own definitions intended to override the default, but the "weak" attribute
> > on the declaration applied to the arch definitions as well, so the linker
> > chose one based on link order (see 10629d711ed7 ("PCI: Remove __weak
> > annotation from pcibios_get_phb_of_node decl")).
> >
> > Remove the "weak" attribute from the declaration so we always prefer a
> > non-weak definition over the weak one, independent of link order.
> >
> > Fixes: 41f107266b19 ("drivers: base: Add prototype declaration to the header file")
> > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> > CC: Rashika Kheria <rashika.kheria@gmail.com>
> > CC: Nathan Fontenot <nfont@austin.ibm.com>
> > CC: Anton Blanchard <anton@au1.ibm.com>
> > CC: Heiko Carstens <heiko.carstens@de.ibm.com>
> > CC: Yinghai Lu <yinghai@kernel.org>
> > ---
> >  include/linux/memory.h |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/include/linux/memory.h b/include/linux/memory.h
> > index bb7384e3c3d8..8b8d8d12348e 100644
> > --- a/include/linux/memory.h
> > +++ b/include/linux/memory.h
> > @@ -35,7 +35,7 @@ struct memory_block {
> >  };
> >
> >  int arch_get_memory_phys_device(unsigned long start_pfn);
> > -unsigned long __weak memory_block_size_bytes(void);
> > +unsigned long memory_block_size_bytes(void);
> >
> >  /* These states are exposed to userspace as text strings in sysfs */
> >  #define        MEM_ONLINE              (1<<0) /* exposed to userspace */
> >
 
s390 works fine with the __weak on memory_bloc_size_bytes.

-- 
blue skies,
   Martin.

"Reality continues to ruin my life." - Calvin.

[PATCH v1 10/10] uprobes: Remove "weak" from function declarations

[Bjorn Helgaas]

For the following interfaces:

  set_swbp()
  set_orig_insn()
  is_swbp_insn()
  is_trap_insn()
  uprobe_get_swbp_addr()
  arch_uprobe_ignore()
  arch_uprobe_copy_ixol()

kernel/events/uprobes.c provides default definitions explicitly marked
"weak".  Some architectures provide their own definitions intended to
override the defaults, but the "weak" attribute on the declarations applied
to the arch definitions as well, so the linker chose one based on link
order (see 10629d711ed7 ("PCI: Remove __weak annotation from
pcibios_get_phb_of_node decl")).

Remove the "weak" attribute from the declarations so we always prefer a
non-weak definition over the weak one, independent of link order.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
CC: Victor Kamensky <victor.kamensky@linaro.org>
CC: Oleg Nesterov <oleg@redhat.com>
CC: David A. Long <dave.long@linaro.org>
CC: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
CC: Ananth N Mavinakayanahalli <ananth@in.ibm.com>
---
 include/linux/uprobes.h |   14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
index 4f844c6b03ee..60beb5dc7977 100644
--- a/include/linux/uprobes.h
+++ b/include/linux/uprobes.h
@@ -98,11 +98,11 @@ struct uprobes_state {
 	struct xol_area		*xol_area;
 };
 
-extern int __weak set_swbp(struct arch_uprobe *aup, struct mm_struct *mm, unsigned long vaddr);
-extern int __weak set_orig_insn(struct arch_uprobe *aup, struct mm_struct *mm, unsigned long vaddr);
-extern bool __weak is_swbp_insn(uprobe_opcode_t *insn);
-extern bool __weak is_trap_insn(uprobe_opcode_t *insn);
-extern unsigned long __weak uprobe_get_swbp_addr(struct pt_regs *regs);
+extern int set_swbp(struct arch_uprobe *aup, struct mm_struct *mm, unsigned long vaddr);
+extern int set_orig_insn(struct arch_uprobe *aup, struct mm_struct *mm, unsigned long vaddr);
+extern bool is_swbp_insn(uprobe_opcode_t *insn);
+extern bool is_trap_insn(uprobe_opcode_t *insn);
+extern unsigned long uprobe_get_swbp_addr(struct pt_regs *regs);
 extern unsigned long uprobe_get_trap_addr(struct pt_regs *regs);
 extern int uprobe_write_opcode(struct mm_struct *mm, unsigned long vaddr, uprobe_opcode_t);
 extern int uprobe_register(struct inode *inode, loff_t offset, struct uprobe_consumer *uc);
@@ -128,8 +128,8 @@ extern bool arch_uprobe_xol_was_trapped(struct task_struct *tsk);
 extern int  arch_uprobe_exception_notify(struct notifier_block *self, unsigned long val, void *data);
 extern void arch_uprobe_abort_xol(struct arch_uprobe *aup, struct pt_regs *regs);
 extern unsigned long arch_uretprobe_hijack_return_addr(unsigned long trampoline_vaddr, struct pt_regs *regs);
-extern bool __weak arch_uprobe_ignore(struct arch_uprobe *aup, struct pt_regs *regs);
-extern void __weak arch_uprobe_copy_ixol(struct page *page, unsigned long vaddr,
+extern bool arch_uprobe_ignore(struct arch_uprobe *aup, struct pt_regs *regs);
+extern void arch_uprobe_copy_ixol(struct page *page, unsigned long vaddr,
 					 void *src, unsigned long len);
 #else /* !CONFIG_UPROBES */
 struct uprobes_state {

Re: [PATCH v1 10/10] uprobes: Remove "weak" from function declarations

[Bjorn Helgaas]

[+cc Victor, Oleg, David, Srikar, Ananth, Russell, linux-arm-kernel,
Ben, Paul, Michael, linuxppc-dev.  arm and powerpc define some of
these functions and are at risk for this issue.]

On Wed, Oct 15, 2014 at 11:06 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> For the following interfaces:
>
>   set_swbp()
>   set_orig_insn()
>   is_swbp_insn()
>   is_trap_insn()
>   uprobe_get_swbp_addr()
>   arch_uprobe_ignore()
>   arch_uprobe_copy_ixol()
>
> kernel/events/uprobes.c provides default definitions explicitly marked
> "weak".  Some architectures provide their own definitions intended to
> override the defaults, but the "weak" attribute on the declarations applied
> to the arch definitions as well, so the linker chose one based on link
> order (see 10629d711ed7 ("PCI: Remove __weak annotation from
> pcibios_get_phb_of_node decl")).
>
> Remove the "weak" attribute from the declarations so we always prefer a
> non-weak definition over the weak one, independent of link order.
>
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> CC: Victor Kamensky <victor.kamensky@linaro.org>
> CC: Oleg Nesterov <oleg@redhat.com>
> CC: David A. Long <dave.long@linaro.org>
> CC: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
> CC: Ananth N Mavinakayanahalli <ananth@in.ibm.com>
> ---
>  include/linux/uprobes.h |   14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
> index 4f844c6b03ee..60beb5dc7977 100644
> --- a/include/linux/uprobes.h
> +++ b/include/linux/uprobes.h
> @@ -98,11 +98,11 @@ struct uprobes_state {
>         struct xol_area         *xol_area;
>  };
>
> -extern int __weak set_swbp(struct arch_uprobe *aup, struct mm_struct *mm, unsigned long vaddr);
> -extern int __weak set_orig_insn(struct arch_uprobe *aup, struct mm_struct *mm, unsigned long vaddr);
> -extern bool __weak is_swbp_insn(uprobe_opcode_t *insn);
> -extern bool __weak is_trap_insn(uprobe_opcode_t *insn);
> -extern unsigned long __weak uprobe_get_swbp_addr(struct pt_regs *regs);
> +extern int set_swbp(struct arch_uprobe *aup, struct mm_struct *mm, unsigned long vaddr);
> +extern int set_orig_insn(struct arch_uprobe *aup, struct mm_struct *mm, unsigned long vaddr);
> +extern bool is_swbp_insn(uprobe_opcode_t *insn);
> +extern bool is_trap_insn(uprobe_opcode_t *insn);
> +extern unsigned long uprobe_get_swbp_addr(struct pt_regs *regs);
>  extern unsigned long uprobe_get_trap_addr(struct pt_regs *regs);
>  extern int uprobe_write_opcode(struct mm_struct *mm, unsigned long vaddr, uprobe_opcode_t);
>  extern int uprobe_register(struct inode *inode, loff_t offset, struct uprobe_consumer *uc);
> @@ -128,8 +128,8 @@ extern bool arch_uprobe_xol_was_trapped(struct task_struct *tsk);
>  extern int  arch_uprobe_exception_notify(struct notifier_block *self, unsigned long val, void *data);
>  extern void arch_uprobe_abort_xol(struct arch_uprobe *aup, struct pt_regs *regs);
>  extern unsigned long arch_uretprobe_hijack_return_addr(unsigned long trampoline_vaddr, struct pt_regs *regs);
> -extern bool __weak arch_uprobe_ignore(struct arch_uprobe *aup, struct pt_regs *regs);
> -extern void __weak arch_uprobe_copy_ixol(struct page *page, unsigned long vaddr,
> +extern bool arch_uprobe_ignore(struct arch_uprobe *aup, struct pt_regs *regs);
> +extern void arch_uprobe_copy_ixol(struct page *page, unsigned long vaddr,
>                                          void *src, unsigned long len);
>  #else /* !CONFIG_UPROBES */
>  struct uprobes_state {
>

Re: [PATCH v1 10/10] uprobes: Remove "weak" from function declarations

[Ingo Molnar]

* Bjorn Helgaas <bhelgaas@google.com> wrote:

> For the following interfaces:
> 
>   set_swbp()
>   set_orig_insn()
>   is_swbp_insn()
>   is_trap_insn()
>   uprobe_get_swbp_addr()
>   arch_uprobe_ignore()
>   arch_uprobe_copy_ixol()
> 
> kernel/events/uprobes.c provides default definitions explicitly marked
> "weak".  Some architectures provide their own definitions intended to
> override the defaults, but the "weak" attribute on the declarations applied
> to the arch definitions as well, so the linker chose one based on link
> order (see 10629d711ed7 ("PCI: Remove __weak annotation from
> pcibios_get_phb_of_node decl")).
> 
> Remove the "weak" attribute from the declarations so we always prefer a
> non-weak definition over the weak one, independent of link order.
> 
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> CC: Victor Kamensky <victor.kamensky@linaro.org>
> CC: Oleg Nesterov <oleg@redhat.com>
> CC: David A. Long <dave.long@linaro.org>
> CC: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
> CC: Ananth N Mavinakayanahalli <ananth@in.ibm.com>
> ---
>  include/linux/uprobes.h |   14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
> index 4f844c6b03ee..60beb5dc7977 100644
> --- a/include/linux/uprobes.h
> +++ b/include/linux/uprobes.h
> @@ -98,11 +98,11 @@ struct uprobes_state {
>  	struct xol_area		*xol_area;
>  };
>  
> -extern int __weak set_swbp(struct arch_uprobe *aup, struct mm_struct *mm, unsigned long vaddr);
> -extern int __weak set_orig_insn(struct arch_uprobe *aup, struct mm_struct *mm, unsigned long vaddr);
> -extern bool __weak is_swbp_insn(uprobe_opcode_t *insn);
> -extern bool __weak is_trap_insn(uprobe_opcode_t *insn);
> -extern unsigned long __weak uprobe_get_swbp_addr(struct pt_regs *regs);
> +extern int set_swbp(struct arch_uprobe *aup, struct mm_struct *mm, unsigned long vaddr);
> +extern int set_orig_insn(struct arch_uprobe *aup, struct mm_struct *mm, unsigned long vaddr);
> +extern bool is_swbp_insn(uprobe_opcode_t *insn);
> +extern bool is_trap_insn(uprobe_opcode_t *insn);
> +extern unsigned long uprobe_get_swbp_addr(struct pt_regs *regs);
>  extern unsigned long uprobe_get_trap_addr(struct pt_regs *regs);
>  extern int uprobe_write_opcode(struct mm_struct *mm, unsigned long vaddr, uprobe_opcode_t);
>  extern int uprobe_register(struct inode *inode, loff_t offset, struct uprobe_consumer *uc);
> @@ -128,8 +128,8 @@ extern bool arch_uprobe_xol_was_trapped(struct task_struct *tsk);
>  extern int  arch_uprobe_exception_notify(struct notifier_block *self, unsigned long val, void *data);
>  extern void arch_uprobe_abort_xol(struct arch_uprobe *aup, struct pt_regs *regs);
>  extern unsigned long arch_uretprobe_hijack_return_addr(unsigned long trampoline_vaddr, struct pt_regs *regs);
> -extern bool __weak arch_uprobe_ignore(struct arch_uprobe *aup, struct pt_regs *regs);
> -extern void __weak arch_uprobe_copy_ixol(struct page *page, unsigned long vaddr,
> +extern bool arch_uprobe_ignore(struct arch_uprobe *aup, struct pt_regs *regs);
> +extern void arch_uprobe_copy_ixol(struct page *page, unsigned long vaddr,
>  					 void *src, unsigned long len);
>  #else /* !CONFIG_UPROBES */
>  struct uprobes_state {

Acked-by: Ingo Molnar <mingo@kernel.org>

Thanks,

	Ingo

Re: [PATCH v1 10/10] uprobes: Remove "weak" from function declarations

[Srikar Dronamraju]

* Bjorn Helgaas <bhelgaas@google.com> [2014-10-15 11:06:50]:

> For the following interfaces:
> 
>   set_swbp()
>   set_orig_insn()
>   is_swbp_insn()
>   is_trap_insn()
>   uprobe_get_swbp_addr()
>   arch_uprobe_ignore()
>   arch_uprobe_copy_ixol()
> 
> kernel/events/uprobes.c provides default definitions explicitly marked
> "weak".  Some architectures provide their own definitions intended to
> override the defaults, but the "weak" attribute on the declarations applied
> to the arch definitions as well, so the linker chose one based on link
> order (see 10629d711ed7 ("PCI: Remove __weak annotation from
> pcibios_get_phb_of_node decl")).
> 
> Remove the "weak" attribute from the declarations so we always prefer a
> non-weak definition over the weak one, independent of link order.
> 

Acked-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>

> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> CC: Victor Kamensky <victor.kamensky@linaro.org>
> CC: Oleg Nesterov <oleg@redhat.com>
> CC: David A. Long <dave.long@linaro.org>
> CC: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
> CC: Ananth N Mavinakayanahalli <ananth@in.ibm.com>
> ---
>  include/linux/uprobes.h |   14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
> index 4f844c6b03ee..60beb5dc7977 100644
> --- a/include/linux/uprobes.h
> +++ b/include/linux/uprobes.h
> @@ -98,11 +98,11 @@ struct uprobes_state {
>  	struct xol_area		*xol_area;
>  };
>  
> -extern int __weak set_swbp(struct arch_uprobe *aup, struct mm_struct *mm, unsigned long vaddr);
> -extern int __weak set_orig_insn(struct arch_uprobe *aup, struct mm_struct *mm, unsigned long vaddr);
> -extern bool __weak is_swbp_insn(uprobe_opcode_t *insn);
> -extern bool __weak is_trap_insn(uprobe_opcode_t *insn);
> -extern unsigned long __weak uprobe_get_swbp_addr(struct pt_regs *regs);
> +extern int set_swbp(struct arch_uprobe *aup, struct mm_struct *mm, unsigned long vaddr);
> +extern int set_orig_insn(struct arch_uprobe *aup, struct mm_struct *mm, unsigned long vaddr);
> +extern bool is_swbp_insn(uprobe_opcode_t *insn);
> +extern bool is_trap_insn(uprobe_opcode_t *insn);
> +extern unsigned long uprobe_get_swbp_addr(struct pt_regs *regs);
>  extern unsigned long uprobe_get_trap_addr(struct pt_regs *regs);
>  extern int uprobe_write_opcode(struct mm_struct *mm, unsigned long vaddr, uprobe_opcode_t);
>  extern int uprobe_register(struct inode *inode, loff_t offset, struct uprobe_consumer *uc);
> @@ -128,8 +128,8 @@ extern bool arch_uprobe_xol_was_trapped(struct task_struct *tsk);
>  extern int  arch_uprobe_exception_notify(struct notifier_block *self, unsigned long val, void *data);
>  extern void arch_uprobe_abort_xol(struct arch_uprobe *aup, struct pt_regs *regs);
>  extern unsigned long arch_uretprobe_hijack_return_addr(unsigned long trampoline_vaddr, struct pt_regs *regs);
> -extern bool __weak arch_uprobe_ignore(struct arch_uprobe *aup, struct pt_regs *regs);
> -extern void __weak arch_uprobe_copy_ixol(struct page *page, unsigned long vaddr,
> +extern bool arch_uprobe_ignore(struct arch_uprobe *aup, struct pt_regs *regs);
> +extern void arch_uprobe_copy_ixol(struct page *page, unsigned long vaddr,
>  					 void *src, unsigned long len);
>  #else /* !CONFIG_UPROBES */
>  struct uprobes_state {
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

-- 
Thanks and Regards
Srikar Dronamraju

Re: [PATCH v1 00/10] Remove weak function declarations

[Andrew Morton]

On Wed, 15 Oct 2014 11:05:41 -0600 Bjorn Helgaas <bhelgaas@google.com> wrote:

> A common usage of "weak" is for a default implementation of a function.
> An architecture that needs something different can supply a non-weak
> ("strong") implementation, with the expectation that the linker will select
> the strong version and discard the weak default version.
> 
> We have a few function declarations in header files annotated as "weak".
> That causes every *every* definition to be marked "weak", which means there
> is no strong version at all.  In this case, the linker selects one of the
> weak versions based on link order.  I don't think this is what we want.
> 
> These patches remove almost all the weak annotations from header files
> (MIPS still uses it for get_c0_compare_int(), apparently relying on the
> fact that a weak symbol need not be defined at all).  In most cases, the
> default implementation was already marked weak at the definition.  When it
> wasn't, I added that.
> 
> It might be simplest if I ask Linus to pull these all as a group from my
> branch [1].  I'll look for acks from the following people.  If I don't see
> an ack, I'll drop the patch and you can take it yourself or ignore it as
> you wish.
> 
>   Eric:                    audit
>   Thomas, Ingo, or Peter:  x86
>   Ralf:                    MIPS
>   John or Thomas:          clocksource
>   Jason:                   kgdb
>   Ingo:                    uprobes
>   Andrew:                  vmcore, memory-hotplug

Acks, of course..

> I don't know whether these fix any actual bugs.  We *did* have a bug like
> this on MIPS a while ago (10629d711ed7 ("PCI: Remove __weak annotation from
> pcibios_get_phb_of_node decl")), so it's possible that they do fix
> something.

I'm rather astonished that we haven't hit problems with this before
now.

This is pretty rude behaviour from the linker, really - grabbing the
first __weak function and using that is very likely to be the wrong
thing to do.

Still, this is a bit of a hand grenade and we should think up some way
of detecting/preventing recurrences.

I guess a checkpatch rule which warns about __weak and
__attribute__((weak)) in a header file would help.  Is there anything
more robust we can do?  Coccinelle, sparse, etc?