On Mon, Dec 16, 2013 at 1:07 PM, David Cohen
<[email protected]> wrote:
> This code was partially based on Mark Brown's previous work.
>
> Signed-off-by: David Cohen <[email protected]>
> Signed-off-by: Fei Yang <[email protected]>
> Cc: Mark F. Brown <[email protected]>
> Cc: Kuppuswamy Sathyanarayanan <[email protected]>
I know this has already been merged to Linus' tree, but it looks funny to me.
> --- a/arch/x86/platform/intel-mid/intel_mid_weak_decls.h
> +++ b/arch/x86/platform/intel-mid/intel_mid_weak_decls.h
> @@ -16,3 +16,4 @@
> */
> extern void * __cpuinit get_penwell_ops(void) __attribute__((weak));
> extern void * __cpuinit get_cloverview_ops(void) __attribute__((weak));
> +extern void * __init get_tangier_ops(void) __attribute__((weak));
We should use "__weak" instead of the gcc-specific "__attribute__((weak))".
I don't think it's a good idea to use __weak on a declaration in a
header file. If there are ever multiple definitions of the symbol,
they are *all* made weak symbols, and one is chosen based on link
order, which is error-prone. I only see one definition now, but the
whole point of weak is to allow multiple definitions, so this looks
like a problem waiting to happen. See 10629d711ed, for example.
It look me a bit to figure out that these get_*_ops() functions are
used by INTEL_MID_OPS_INIT, which constructs the name using a macro,
so grep/cscope/etc. don't see any users. A comment pointing to
INTEL_MID_OPS_INIT would be helpful.
What's the reason for making these symbols weak? Normally we use weak
to make a generic default version of a function, while allowing
architectures to replace the default with their own version if
necessary. But I don't see that happening here. Maybe I'm just
missing it, like I missed the uses of get_tangier_ops(), et al.
Bjorn
Hi Bjorn,
On Mon, Jan 27, 2014 at 05:52:30PM -0700, Bjorn Helgaas wrote:
> On Mon, Dec 16, 2013 at 1:07 PM, David Cohen
> <[email protected]> wrote:
> > This code was partially based on Mark Brown's previous work.
> >
> > Signed-off-by: David Cohen <[email protected]>
> > Signed-off-by: Fei Yang <[email protected]>
> > Cc: Mark F. Brown <[email protected]>
> > Cc: Kuppuswamy Sathyanarayanan <[email protected]>
>
> I know this has already been merged to Linus' tree, but it looks funny to me.
>
> > --- a/arch/x86/platform/intel-mid/intel_mid_weak_decls.h
> > +++ b/arch/x86/platform/intel-mid/intel_mid_weak_decls.h
> > @@ -16,3 +16,4 @@
> > */
> > extern void * __cpuinit get_penwell_ops(void) __attribute__((weak));
> > extern void * __cpuinit get_cloverview_ops(void) __attribute__((weak));
> > +extern void * __init get_tangier_ops(void) __attribute__((weak));
>
> We should use "__weak" instead of the gcc-specific "__attribute__((weak))".
>
> I don't think it's a good idea to use __weak on a declaration in a
> header file. If there are ever multiple definitions of the symbol,
> they are *all* made weak symbols, and one is chosen based on link
> order, which is error-prone. I only see one definition now, but the
> whole point of weak is to allow multiple definitions, so this looks
> like a problem waiting to happen. See 10629d711ed, for example.
>
> It look me a bit to figure out that these get_*_ops() functions are
> used by INTEL_MID_OPS_INIT, which constructs the name using a macro,
> so grep/cscope/etc. don't see any users. A comment pointing to
> INTEL_MID_OPS_INIT would be helpful.
>
> What's the reason for making these symbols weak? Normally we use weak
> to make a generic default version of a function, while allowing
> architectures to replace the default with their own version if
> necessary. But I don't see that happening here. Maybe I'm just
> missing it, like I missed the uses of get_tangier_ops(), et al.
Intel mid was implemented in such way that we should select which soc to
be used in compilation time. Depending on the selection, mfld.c or
mrfl.c could not be compiled then some symbols wouldn't be available.
But IMHO this is a bad legacy design that exists in there, so I started
to rework it as you can see in this commit:
commit 4cb9b00f42e07830310319a07e6c91413ee8153e
Author: David Cohen <[email protected]>
Date: Mon Dec 16 17:37:26 2013 -0800
x86, intel-mid: Remove deprecated X86_MDFLD and X86_WANT_INTEL_MID
configs
I'm sending more patches soon and getting rid of intel_mid_weak_decls.h
file is in my TODO list.
Br, David
On Mon, Jan 27, 2014 at 6:30 PM, David Cohen
<[email protected]> wrote:
> Hi Bjorn,
>
> On Mon, Jan 27, 2014 at 05:52:30PM -0700, Bjorn Helgaas wrote:
>> On Mon, Dec 16, 2013 at 1:07 PM, David Cohen
>> <[email protected]> wrote:
>> > This code was partially based on Mark Brown's previous work.
>> >
>> > Signed-off-by: David Cohen <[email protected]>
>> > Signed-off-by: Fei Yang <[email protected]>
>> > Cc: Mark F. Brown <[email protected]>
>> > Cc: Kuppuswamy Sathyanarayanan <[email protected]>
>>
>> I know this has already been merged to Linus' tree, but it looks funny to me.
>>
>> > --- a/arch/x86/platform/intel-mid/intel_mid_weak_decls.h
>> > +++ b/arch/x86/platform/intel-mid/intel_mid_weak_decls.h
>> > @@ -16,3 +16,4 @@
>> > */
>> > extern void * __cpuinit get_penwell_ops(void) __attribute__((weak));
>> > extern void * __cpuinit get_cloverview_ops(void) __attribute__((weak));
>> > +extern void * __init get_tangier_ops(void) __attribute__((weak));
>>
>> We should use "__weak" instead of the gcc-specific "__attribute__((weak))".
>>
>> I don't think it's a good idea to use __weak on a declaration in a
>> header file. If there are ever multiple definitions of the symbol,
>> they are *all* made weak symbols, and one is chosen based on link
>> order, which is error-prone. I only see one definition now, but the
>> whole point of weak is to allow multiple definitions, so this looks
>> like a problem waiting to happen. See 10629d711ed, for example.
>>
>> It look me a bit to figure out that these get_*_ops() functions are
>> used by INTEL_MID_OPS_INIT, which constructs the name using a macro,
>> so grep/cscope/etc. don't see any users. A comment pointing to
>> INTEL_MID_OPS_INIT would be helpful.
>>
>> What's the reason for making these symbols weak? Normally we use weak
>> to make a generic default version of a function, while allowing
>> architectures to replace the default with their own version if
>> necessary. But I don't see that happening here. Maybe I'm just
>> missing it, like I missed the uses of get_tangier_ops(), et al.
>
> Intel mid was implemented in such way that we should select which soc to
> be used in compilation time. Depending on the selection, mfld.c or
> mrfl.c could not be compiled then some symbols wouldn't be available.
>
> But IMHO this is a bad legacy design that exists in there, so I started
> to rework it as you can see in this commit:
>
> commit 4cb9b00f42e07830310319a07e6c91413ee8153e
> Author: David Cohen <[email protected]>
> Date: Mon Dec 16 17:37:26 2013 -0800
>
> x86, intel-mid: Remove deprecated X86_MDFLD and X86_WANT_INTEL_MID
> configs
>
> I'm sending more patches soon and getting rid of intel_mid_weak_decls.h
> file is in my TODO list.
Sounds good. While you're looking at it, I have similar questions
about ipc_device_handler() and msic_generic_platform_data(). It's not
clear to me why they should be weak.
Bjorn
On Tue, Jan 28, 2014 at 11:40:57AM -0700, Bjorn Helgaas wrote:
> On Mon, Jan 27, 2014 at 6:30 PM, David Cohen
> <[email protected]> wrote:
> > Hi Bjorn,
> >
> > On Mon, Jan 27, 2014 at 05:52:30PM -0700, Bjorn Helgaas wrote:
> >> On Mon, Dec 16, 2013 at 1:07 PM, David Cohen
> >> <[email protected]> wrote:
> >> > This code was partially based on Mark Brown's previous work.
> >> >
> >> > Signed-off-by: David Cohen <[email protected]>
> >> > Signed-off-by: Fei Yang <[email protected]>
> >> > Cc: Mark F. Brown <[email protected]>
> >> > Cc: Kuppuswamy Sathyanarayanan <[email protected]>
> >>
> >> I know this has already been merged to Linus' tree, but it looks funny to me.
> >>
> >> > --- a/arch/x86/platform/intel-mid/intel_mid_weak_decls.h
> >> > +++ b/arch/x86/platform/intel-mid/intel_mid_weak_decls.h
> >> > @@ -16,3 +16,4 @@
> >> > */
> >> > extern void * __cpuinit get_penwell_ops(void) __attribute__((weak));
> >> > extern void * __cpuinit get_cloverview_ops(void) __attribute__((weak));
> >> > +extern void * __init get_tangier_ops(void) __attribute__((weak));
> >>
> >> We should use "__weak" instead of the gcc-specific "__attribute__((weak))".
> >>
> >> I don't think it's a good idea to use __weak on a declaration in a
> >> header file. If there are ever multiple definitions of the symbol,
> >> they are *all* made weak symbols, and one is chosen based on link
> >> order, which is error-prone. I only see one definition now, but the
> >> whole point of weak is to allow multiple definitions, so this looks
> >> like a problem waiting to happen. See 10629d711ed, for example.
> >>
> >> It look me a bit to figure out that these get_*_ops() functions are
> >> used by INTEL_MID_OPS_INIT, which constructs the name using a macro,
> >> so grep/cscope/etc. don't see any users. A comment pointing to
> >> INTEL_MID_OPS_INIT would be helpful.
> >>
> >> What's the reason for making these symbols weak? Normally we use weak
> >> to make a generic default version of a function, while allowing
> >> architectures to replace the default with their own version if
> >> necessary. But I don't see that happening here. Maybe I'm just
> >> missing it, like I missed the uses of get_tangier_ops(), et al.
> >
> > Intel mid was implemented in such way that we should select which soc to
> > be used in compilation time. Depending on the selection, mfld.c or
> > mrfl.c could not be compiled then some symbols wouldn't be available.
> >
> > But IMHO this is a bad legacy design that exists in there, so I started
> > to rework it as you can see in this commit:
> >
> > commit 4cb9b00f42e07830310319a07e6c91413ee8153e
> > Author: David Cohen <[email protected]>
> > Date: Mon Dec 16 17:37:26 2013 -0800
> >
> > x86, intel-mid: Remove deprecated X86_MDFLD and X86_WANT_INTEL_MID
> > configs
> >
> > I'm sending more patches soon and getting rid of intel_mid_weak_decls.h
> > file is in my TODO list.
>
> Sounds good. While you're looking at it, I have similar questions
> about ipc_device_handler() and msic_generic_platform_data(). It's not
> clear to me why they should be weak.
I'm afraid that's gargabe I missed. It supposed to be removed already.
The original upstreamed patch set needed it, since all platform data
were gathered in a board.c file and some of them could not be compiled.
You can see it here:
http://us.generation-nt.com/answer/patch-v2-00-10-rework-arch-x86-platform-mrst-intel-mid-help-212689892.html
But I reworked this approach and added a sfi_device() macro to let
compiler to gather all the platform data, thus board.c file doesn't
exist. It means it is not necessary anymore to be weak. I can send a
patch right away fixing it.
Thanks for pointing that out.
Br, David
>
> Bjorn
platform_ipc.h and platform_msic.h are wrongly declaring functions as
external and with 'weak' attribute. This patch does a cleanup on those
header files.
It should have no functional change.
Signed-off-by: David Cohen <[email protected]>
Cc: Bjorn Helgaas <[email protected]>
---
arch/x86/platform/intel-mid/device_libs/platform_ipc.h | 5 +++--
arch/x86/platform/intel-mid/device_libs/platform_msic.h | 4 ++--
2 files changed, 5 insertions(+), 4 deletions(-)
diff --git a/arch/x86/platform/intel-mid/device_libs/platform_ipc.h b/arch/x86/platform/intel-mid/device_libs/platform_ipc.h
index 8f568dd79605..79bb09d4f718 100644
--- a/arch/x86/platform/intel-mid/device_libs/platform_ipc.h
+++ b/arch/x86/platform/intel-mid/device_libs/platform_ipc.h
@@ -12,6 +12,7 @@
#ifndef _PLATFORM_IPC_H_
#define _PLATFORM_IPC_H_
-extern void __init ipc_device_handler(struct sfi_device_table_entry *pentry,
- struct devs_id *dev) __attribute__((weak));
+void __init
+ipc_device_handler(struct sfi_device_table_entry *pentry, struct devs_id *dev);
+
#endif
diff --git a/arch/x86/platform/intel-mid/device_libs/platform_msic.h b/arch/x86/platform/intel-mid/device_libs/platform_msic.h
index 917eb56d77da..b7be1d041da2 100644
--- a/arch/x86/platform/intel-mid/device_libs/platform_msic.h
+++ b/arch/x86/platform/intel-mid/device_libs/platform_msic.h
@@ -14,6 +14,6 @@
extern struct intel_msic_platform_data msic_pdata;
-extern void *msic_generic_platform_data(void *info,
- enum intel_msic_block block) __attribute__((weak));
+void *msic_generic_platform_data(void *info, enum intel_msic_block block);
+
#endif
--
1.8.4.2
Commit-ID: 790ed7421a973d9773dda8b4e5300c3f7f6fbcf7
Gitweb: http://git.kernel.org/tip/790ed7421a973d9773dda8b4e5300c3f7f6fbcf7
Author: David Cohen <[email protected]>
AuthorDate: Tue, 28 Jan 2014 15:09:27 -0800
Committer: H. Peter Anvin <[email protected]>
CommitDate: Tue, 28 Jan 2014 15:13:40 -0800
x86, intel-mid: Cleanup some platform code's header files
platform_ipc.h and platform_msic.h are wrongly declaring functions as
external and with 'weak' attribute. This patch does a cleanup on those
header files.
It should have no functional change.
Signed-off-by: David Cohen <[email protected]>
Link: http://lkml.kernel.org/r/[email protected]
Cc: Bjorn Helgaas <[email protected]>
Signed-off-by: H. Peter Anvin <[email protected]>
---
arch/x86/platform/intel-mid/device_libs/platform_ipc.h | 5 +++--
arch/x86/platform/intel-mid/device_libs/platform_msic.h | 4 ++--
2 files changed, 5 insertions(+), 4 deletions(-)
diff --git a/arch/x86/platform/intel-mid/device_libs/platform_ipc.h b/arch/x86/platform/intel-mid/device_libs/platform_ipc.h
index 8f568dd..79bb09d 100644
--- a/arch/x86/platform/intel-mid/device_libs/platform_ipc.h
+++ b/arch/x86/platform/intel-mid/device_libs/platform_ipc.h
@@ -12,6 +12,7 @@
#ifndef _PLATFORM_IPC_H_
#define _PLATFORM_IPC_H_
-extern void __init ipc_device_handler(struct sfi_device_table_entry *pentry,
- struct devs_id *dev) __attribute__((weak));
+void __init
+ipc_device_handler(struct sfi_device_table_entry *pentry, struct devs_id *dev);
+
#endif
diff --git a/arch/x86/platform/intel-mid/device_libs/platform_msic.h b/arch/x86/platform/intel-mid/device_libs/platform_msic.h
index 917eb56..b7be1d0 100644
--- a/arch/x86/platform/intel-mid/device_libs/platform_msic.h
+++ b/arch/x86/platform/intel-mid/device_libs/platform_msic.h
@@ -14,6 +14,6 @@
extern struct intel_msic_platform_data msic_pdata;
-extern void *msic_generic_platform_data(void *info,
- enum intel_msic_block block) __attribute__((weak));
+void *msic_generic_platform_data(void *info, enum intel_msic_block block);
+
#endif