lkml.org 
[lkml]   [2014]   [Jan]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH v2 3/4] x86: intel-mid: add Merrifield platform support
    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.

    Bjorn


    \
     
     \ /
      Last update: 2014-01-28 02:21    [W:4.027 / U:0.008 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site