Re: [PATCH v2 3/4] x86: intel-mid: add Merrifield platform support

[David Cohen <david.a.cohen@linux.intel.com>]

On Tue, Jan 28, 2014 at 11:40:57AM -0700, Bjorn Helgaas wrote:
> On Mon, Jan 27, 2014 at 6:30 PM, David Cohen
> <david.a.cohen@linux.intel.com> 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
> >> <david.a.cohen@linux.intel.com> wrote:
> >> > This code was partially based on Mark Brown's previous work.
> >> >
> >> > Signed-off-by: David Cohen <david.a.cohen@linux.intel.com>
> >> > Signed-off-by: Fei Yang <fei.yang@intel.com>
> >> > Cc: Mark F. Brown <mark.f.brown@intel.com>
> >> > Cc: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> >>
> >> 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 <david.a.cohen@linux.intel.com>
> > 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