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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAErSpo4ymfSdZ5an4zt+C=5asUW=vz000d32mPZkF-S5N_uynA@mail.gmail.com>
Date:	Mon, 27 Jan 2014 17:52:30 -0700
From:	Bjorn Helgaas <bhelgaas@...gle.com>
To:	David Cohen <david.a.cohen@...ux.intel.com>
Cc:	Thomas Gleixner <tglx@...utronix.de>,
	Ingo Molnar <mingo@...hat.com>,
	"H. Peter Anvin" <hpa@...or.com>,
	"x86@...nel.org" <x86@...nel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
	Fei Yang <fei.yang@...el.com>,
	"Mark F. Brown" <mark.f.brown@...el.com>,
	Kuppuswamy Sathyanarayanan 
	<sathyanarayanan.kuppuswamy@...ux.intel.com>
Subject: Re: [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@...ux.intel.com> wrote:
> This code was partially based on Mark Brown's previous work.
>
> Signed-off-by: David Cohen <david.a.cohen@...ux.intel.com>
> Signed-off-by: Fei Yang <fei.yang@...el.com>
> Cc: Mark F. Brown <mark.f.brown@...el.com>
> Cc: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@...ux.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/