Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754118Ab3EJT2R (ORCPT ); Fri, 10 May 2013 15:28:17 -0400 Received: from mail-vc0-f175.google.com ([209.85.220.175]:35523 "EHLO mail-vc0-f175.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753996Ab3EJT2P (ORCPT ); Fri, 10 May 2013 15:28:15 -0400 MIME-Version: 1.0 In-Reply-To: <20130510190954.GS12292@phenom.ffwll.local> References: <97346b603822ffd8ab446316fc08fbf7686c9d50.1368128020.git.luto@amacapital.net> <20130510091923.GH12292@phenom.ffwll.local> <20130510190954.GS12292@phenom.ffwll.local> From: Andy Lutomirski Date: Fri, 10 May 2013 12:27:54 -0700 Message-ID: Subject: Re: [RFC/PATCH v2 1/8] Add arch_phys_wc_{add,del} to manipulate WC MTRRs if needed To: Andy Lutomirski , linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, linux-fbdev@vger.kernel.org, Jerome Glisse , Alex Deucher , Dave Airlie Cc: Daniel Vetter Content-Type: text/plain; charset=ISO-8859-1 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5851 Lines: 133 On Fri, May 10, 2013 at 12:09 PM, Daniel Vetter wrote: > On Fri, May 10, 2013 at 11:00:56AM -0700, Andy Lutomirski wrote: >> On Fri, May 10, 2013 at 2:19 AM, Daniel Vetter wrote: >> > On Thu, May 09, 2013 at 12:46:20PM -0700, Andy Lutomirski wrote: >> >> Several drivers currently use mtrr_add through various #ifdef guards >> >> and/or drm wrappers. The vast majority of them want to add WC MTRRs >> >> on x86 systems and don't actually need the MTRR if PAT (i.e. >> >> ioremap_wc, etc) are working. >> >> >> >> arch_phys_wc_add and arch_phys_wc_del are new functions, available >> >> on all architectures and configurations, that add WC MTRRs on x86 if >> >> needed (and handle errors) and do nothing at all otherwise. They're >> >> also easier to use than mtrr_add and mtrr_del, so the call sites can >> >> be simplified. >> >> >> >> As an added benefit, this will avoid wasting MTRRs and possibly >> >> warning pointlessly on PAT-supporting systems. >> >> >> >> Signed-off-by: Andy Lutomirski >> >> --- >> >> arch/x86/include/asm/io.h | 7 ++++++ >> >> arch/x86/include/asm/mtrr.h | 5 ++++- >> >> arch/x86/kernel/cpu/mtrr/main.c | 48 +++++++++++++++++++++++++++++++++++++++++ >> >> include/linux/io.h | 25 +++++++++++++++++++++ >> >> 4 files changed, 84 insertions(+), 1 deletion(-) >> >> >> >> diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h >> >> index d8e8eef..34f69cb 100644 >> >> --- a/arch/x86/include/asm/io.h >> >> +++ b/arch/x86/include/asm/io.h >> >> @@ -345,4 +345,11 @@ extern bool xen_biovec_phys_mergeable(const struct bio_vec *vec1, >> >> >> >> #define IO_SPACE_LIMIT 0xffff >> >> >> >> +#ifdef CONFIG_MTRR >> >> +extern int __must_check arch_phys_wc_add(unsigned long base, >> >> + unsigned long size); >> >> +extern void arch_phys_wc_del(int handle); >> >> +#define arch_phys_wc_add arch_phys_wc_add >> >> +#endif >> >> + >> >> #endif /* _ASM_X86_IO_H */ >> >> diff --git a/arch/x86/include/asm/mtrr.h b/arch/x86/include/asm/mtrr.h >> >> index e235582..10d0fba 100644 >> >> --- a/arch/x86/include/asm/mtrr.h >> >> +++ b/arch/x86/include/asm/mtrr.h >> >> @@ -26,7 +26,10 @@ >> >> #include >> >> >> >> >> >> -/* The following functions are for use by other drivers */ >> >> +/* >> >> + * The following functions are for use by other drivers that cannot use >> >> + * arch_phys_wc_add and arch_phys_wc_del. >> >> + */ >> >> # ifdef CONFIG_MTRR >> >> extern u8 mtrr_type_lookup(u64 addr, u64 end); >> >> extern void mtrr_save_fixed_ranges(void *); >> >> diff --git a/arch/x86/kernel/cpu/mtrr/main.c b/arch/x86/kernel/cpu/mtrr/main.c >> >> index 726bf96..23bd49a 100644 >> >> --- a/arch/x86/kernel/cpu/mtrr/main.c >> >> +++ b/arch/x86/kernel/cpu/mtrr/main.c >> >> @@ -51,6 +51,7 @@ >> >> #include >> >> #include >> >> #include >> >> +#include >> >> >> >> #include "mtrr.h" >> >> >> >> @@ -524,6 +525,53 @@ int mtrr_del(int reg, unsigned long base, unsigned long size) >> >> } >> >> EXPORT_SYMBOL(mtrr_del); >> >> >> >> +/** >> >> + * arch_phys_wc_add - add a WC MTRR and handle errors if PAT is unavailable >> >> + * @base: Physical base address >> >> + * @size: Size of region >> >> + * >> >> + * If PAT is available, this does nothing. If PAT is unavailable, it >> >> + * attempts to add a WC MTRR covering size bytes starting at base and >> >> + * logs an error if this fails. >> >> + * >> >> + * Drivers must store the return value to pass to mtrr_del_wc_if_needed, >> >> + * but drivers should not try to interpret that return value. >> >> + */ >> >> +int arch_phys_wc_add(unsigned long base, unsigned long size) >> >> +{ >> >> + int ret; >> >> + >> >> + if (pat_enabled) >> >> + return 0; /* Success! (We don't need to do anything.) */ >> > >> > Shouldn't we #define a big number for this case since mtrr_add returns >> > 0-based mtrr indices? Rather unlikely that the very first mtrr is unused I >> > know, but still feels like a cleaner interface. And we don't need to leak >> > that #define out at all to users of this interface. >> >> I did something more like that in v1, but I like having the property >> that arch_phys_wc_del(0) does nothing as opposed to possibly breaking >> the system. Given that almost all of these things are stored in >> kzalloced space, this seems safer to me. The return value here could >> just as easily be -ENOSYS, but nothing should care. >> >> There is an issue, though: the drm maps interface is leaking the >> return values to userspace via a couple of ioctls, so I should fix it >> to at least return the correct value. (Why that interface includes an >> mtrr number is a mystery to me.) > > Old drm interfaces are horrible. Best approach is to simply stay far away > from them ... > >> Have I convinced you that my bike shed color is okay? I'll send out a >> v3 later today with a fix for the leaking-to-userspace issue and I'll >> fix the i915 thing. > > A comment in your arch_phys_wc_add/del functions that we do have an mtrr > with index 0, but won't ever get that would be fine with me. Still feels a > bit irky though ;-) Hmm. Maybe you missed the hack I ended up using a couple of lines below. I'm always using strictly positive values to indicate a real MTRR -- I'm adding 1000 to the result if we actually do anything. So a return value of 0 is genuinely safe. (I still have to fix up the two places in the drm code that pass that hacked-up value back to userspace.) --Andy -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/