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
| ||
|
Date: Tue, 28 Jan 2014 11:40:57 -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, Jan 27, 2014 at 6:30 PM, David Cohen <david.a.cohen@...ux.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@...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. > > 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@...ux.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. 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/
Powered by blists - more mailing lists