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

[David Cohen]

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@xxxxxxxxxxxxxxx> wrote:
> > This code was partially based on Mark Brown's previous work.
> >
> > Signed-off-by: David Cohen <david.a.cohen@xxxxxxxxxxxxxxx>
> > Signed-off-by: Fei Yang <fei.yang@xxxxxxxxx>
> > Cc: Mark F. Brown <mark.f.brown@xxxxxxxxx>
> > Cc: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@xxxxxxxxxxxxxxx>
> 
> 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@xxxxxxxxxxxxxxx>
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/