[PATCH v6 0/4] pci: add and use pci_iomap_wc()

[PATCH v6 0/4] pci: add and use pci_iomap_wc()

[Luis R. Rodriguez]

From: "Luis R. Rodriguez" <mcgrof@suse.com>

Bjorn, this v6 respins the introduction of pci_iomap_wc() and
its users onto Bjorn's pci tree [0] on the next branch. It goes
with your own Acked-by on the PCI part as well as the framebuffer
subsystem maintainer's own Acked-bys for the framebuffer driver
specific changes.

The driver changes are being submitted through the PCI tree
to make the caller have users as soon as its merged otherwise
we'd have to wait 2 release before full integration.

Bjorn, if you rather pull instead of apply these patches one
by one I've put these changes up on a branch on a tree based
on yours to pull from.

The following changes since commit 9b08ae0dde00c20a75cf15636a08e0901c6fe482:

  Merge branch 'pci/msi' into next (2015-05-08 15:05:10 -0500)

are available in the git repository at:

  https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux-pci.git pci_iomap_wc-v6-20150520

for you to fetch changes up to 480d550fa62bdad1c8f25b16d27d2035f36a283b:

  video: fbdev: vt8623fb: use arch_phys_wc_add() and pci_iomap_wc() (2015-05-20 15:12:04 -0700)

[0] https://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/

Luis R. Rodriguez (4):
  pci: add pci_iomap_wc() variants
  video: fbdev: arkfb: use arch_phys_wc_add() and pci_iomap_wc()
  video: fbdev: s3fb: use arch_phys_wc_add() and pci_iomap_wc()
  video: fbdev: vt8623fb: use arch_phys_wc_add() and pci_iomap_wc()

 drivers/video/fbdev/arkfb.c     | 36 ++++--------------------
 drivers/video/fbdev/s3fb.c      | 35 ++++-------------------
 drivers/video/fbdev/vt8623fb.c  | 31 ++++-----------------
 include/asm-generic/pci_iomap.h | 14 ++++++++++
 lib/pci_iomap.c                 | 61 +++++++++++++++++++++++++++++++++++++++++
 5 files changed, 92 insertions(+), 85 deletions(-)

-- 
2.3.2.209.gd67f9d5.dirty

[PATCH v6 1/4] pci: add pci_iomap_wc() variants

[Luis R. Rodriguez]

From: "Luis R. Rodriguez" <mcgrof@suse.com>

This allows drivers to take advantage of write-combining
when possible. The PCI specification does not allow for us
to automatically identify a memory region which needs
write-combining so drivers have to identify these areas
on their own. There is IORESOURCE_PREFETCH but as clarified
by Michael and confirmed later by Bjorn, PCI prefetch bit
merely means bridges can combine writes and prefetch reads.
Prefetch does not affect ordering rules and does not allow
writes to be collapsed [0]. WC is stronger, it allows collapsing
and changes ordering rules. WC can also hurt latency as small
writes are buffered. Because of all this drivers needs to
know what they are doing, we can't set a write-combining
preference flag in the pci core automatically for drivers.

Lastly although there is also arch_phys_wc_add() this makes
use of architecture specific write-combining *hacks* and
the only one currently defined and used is MTRR for x86.
MTRRs are legacy, limited in number, have restrictive size
constraints, and are known to interact pooly with the BIOS.
MTRRs should only really be considered on old video framebuffer
drivers. If we made ioremap_wc() and similar calls start
automatically adding MTRRs, then performance will vary wildly
with the order of driver loading because we'll run out of MTRRs
part-way through bootup.

There are a few motivations for phasing out of MTRR and
helping driver change over to use write-combining with PAT:

a) Take advantage of PAT when available

b) Help bury MTRR code away, MTRR is architecture specific and on
   x86 its replaced by PAT

c) Help with the goal of eventually using _PAGE_CACHE_UC over
   _PAGE_CACHE_UC_MINUS on x86 on ioremap_nocache() (see commit
   de33c442e titled "x86 PAT: fix performance drop for glx,
   use UC minus for ioremap(), ioremap_nocache() and
   pci_mmap_page_range()")

[0] https://lkml.org/lkml/2015/4/21/714

Cc: Toshi Kani <toshi.kani@hp.com>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Suresh Siddha <sbsiddha@gmail.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Juergen Gross <jgross@suse.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Dave Airlie <airlied@redhat.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Antonino Daplas <adaplas@gmail.com>
Cc: Jean-Christophe Plagniol-Villard <plagnioj@jcrosoft.com>
Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: venkatesh.pallipadi@intel.com
Cc: Stefan Bader <stefan.bader@canonical.com>
Cc: Ville Syrjälä <syrjala@sci.fi>
Cc: Mel Gorman <mgorman@suse.de>
Cc: Vlastimil Babka <vbabka@suse.cz>
Cc: Borislav Petkov <bp@suse.de>
Cc: Davidlohr Bueso <dbueso@suse.de>
Cc: konrad.wilk@oracle.com
Cc: ville.syrjala@linux.intel.com
Cc: david.vrabel@citrix.com
Cc: jbeulich@suse.com
Cc: Roger Pau Monné <roger.pau@citrix.com>
Cc: linux-fbdev@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: xen-devel@lists.xensource.com
Acked-by: Bjorn Helgaas <bhelgaas@google.com>
Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>
---
 include/asm-generic/pci_iomap.h | 14 ++++++++++
 lib/pci_iomap.c                 | 61 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 75 insertions(+)

diff --git a/include/asm-generic/pci_iomap.h b/include/asm-generic/pci_iomap.h
index 7389c87..b1e17fc 100644
--- a/include/asm-generic/pci_iomap.h
+++ b/include/asm-generic/pci_iomap.h
@@ -15,9 +15,13 @@ struct pci_dev;
 #ifdef CONFIG_PCI
 /* Create a virtual mapping cookie for a PCI BAR (memory or IO) */
 extern void __iomem *pci_iomap(struct pci_dev *dev, int bar, unsigned long max);
+extern void __iomem *pci_iomap_wc(struct pci_dev *dev, int bar, unsigned long max);
 extern void __iomem *pci_iomap_range(struct pci_dev *dev, int bar,
 				     unsigned long offset,
 				     unsigned long maxlen);
+extern void __iomem *pci_iomap_wc_range(struct pci_dev *dev, int bar,
+					unsigned long offset,
+					unsigned long maxlen);
 /* Create a virtual mapping cookie for a port on a given PCI device.
  * Do not call this directly, it exists to make it easier for architectures
  * to override */
@@ -34,12 +38,22 @@ static inline void __iomem *pci_iomap(struct pci_dev *dev, int bar, unsigned lon
 	return NULL;
 }
 
+static inline void __iomem *pci_iomap_wc(struct pci_dev *dev, int bar, unsigned long max)
+{
+	return NULL;
+}
 static inline void __iomem *pci_iomap_range(struct pci_dev *dev, int bar,
 					    unsigned long offset,
 					    unsigned long maxlen)
 {
 	return NULL;
 }
+static inline void __iomem *pci_iomap_wc_range(struct pci_dev *dev, int bar,
+					       unsigned long offset,
+					       unsigned long maxlen)
+{
+	return NULL;
+}
 #endif
 
 #endif /* __ASM_GENERIC_IO_H */
diff --git a/lib/pci_iomap.c b/lib/pci_iomap.c
index bcce5f1..9604dcb 100644
--- a/lib/pci_iomap.c
+++ b/lib/pci_iomap.c
@@ -52,6 +52,46 @@ void __iomem *pci_iomap_range(struct pci_dev *dev,
 EXPORT_SYMBOL(pci_iomap_range);
 
 /**
+ * pci_iomap_wc_range - create a virtual WC mapping cookie for a PCI BAR
+ * @dev: PCI device that owns the BAR
+ * @bar: BAR number
+ * @offset: map memory at the given offset in BAR
+ * @maxlen: max length of the memory to map
+ *
+ * Using this function you will get a __iomem address to your device BAR.
+ * You can access it using ioread*() and iowrite*(). These functions hide
+ * the details if this is a MMIO or PIO address space and will just do what
+ * you expect from them in the correct way. When possible write combining
+ * is used.
+ *
+ * @maxlen specifies the maximum length to map. If you want to get access to
+ * the complete BAR from offset to the end, pass %0 here.
+ * */
+void __iomem *pci_iomap_wc_range(struct pci_dev *dev,
+				 int bar,
+				 unsigned long offset,
+				 unsigned long maxlen)
+{
+	resource_size_t start = pci_resource_start(dev, bar);
+	resource_size_t len = pci_resource_len(dev, bar);
+	unsigned long flags = pci_resource_flags(dev, bar);
+
+	if (len <= offset || !start)
+		return NULL;
+	len -= offset;
+	start += offset;
+	if (maxlen && len > maxlen)
+		len = maxlen;
+	if (flags & IORESOURCE_IO)
+		return NULL;
+	if (flags & IORESOURCE_MEM)
+		return ioremap_wc(start, len);
+	/* What? */
+	return NULL;
+}
+EXPORT_SYMBOL_GPL(pci_iomap_wc_range);
+
+/**
  * pci_iomap - create a virtual mapping cookie for a PCI BAR
  * @dev: PCI device that owns the BAR
  * @bar: BAR number
@@ -70,4 +110,25 @@ void __iomem *pci_iomap(struct pci_dev *dev, int bar, unsigned long maxlen)
 	return pci_iomap_range(dev, bar, 0, maxlen);
 }
 EXPORT_SYMBOL(pci_iomap);
+
+/**
+ * pci_iomap_wc - create a virtual WC mapping cookie for a PCI BAR
+ * @dev: PCI device that owns the BAR
+ * @bar: BAR number
+ * @maxlen: length of the memory to map
+ *
+ * Using this function you will get a __iomem address to your device BAR.
+ * You can access it using ioread*() and iowrite*(). These functions hide
+ * the details if this is a MMIO or PIO address space and will just do what
+ * you expect from them in the correct way. When possible write combining
+ * is used.
+ *
+ * @maxlen specifies the maximum length to map. If you want to get access to
+ * the complete BAR without checking for its length first, pass %0 here.
+ * */
+void __iomem *pci_iomap_wc(struct pci_dev *dev, int bar, unsigned long maxlen)
+{
+	return pci_iomap_wc_range(dev, bar, 0, maxlen);
+}
+EXPORT_SYMBOL_GPL(pci_iomap_wc);
 #endif /* CONFIG_PCI */
-- 
2.3.2.209.gd67f9d5.dirty

Re: [PATCH v6 1/4] pci: add pci_iomap_wc() variants

[Bjorn Helgaas]

On Wed, May 20, 2015 at 04:08:10PM -0700, Luis R. Rodriguez wrote:
> ...
> --- a/lib/pci_iomap.c
> +++ b/lib/pci_iomap.c
> @@ -52,6 +52,46 @@ void __iomem *pci_iomap_range(struct pci_dev *dev,
>  EXPORT_SYMBOL(pci_iomap_range);
>  
>  /**
> + * pci_iomap_wc_range - create a virtual WC mapping cookie for a PCI BAR
> + * @dev: PCI device that owns the BAR
> + * @bar: BAR number
> + * @offset: map memory at the given offset in BAR
> + * @maxlen: max length of the memory to map
> + *
> + * Using this function you will get a __iomem address to your device BAR.
> + * You can access it using ioread*() and iowrite*(). These functions hide
> + * the details if this is a MMIO or PIO address space and will just do what
> + * you expect from them in the correct way. When possible write combining
> + * is used.
> + *
> + * @maxlen specifies the maximum length to map. If you want to get access to
> + * the complete BAR from offset to the end, pass %0 here.
> + * */
> +void __iomem *pci_iomap_wc_range(struct pci_dev *dev,
> +				 int bar,
> +				 unsigned long offset,
> +				 unsigned long maxlen)
> +{
> +	resource_size_t start = pci_resource_start(dev, bar);
> +	resource_size_t len = pci_resource_len(dev, bar);
> +	unsigned long flags = pci_resource_flags(dev, bar);
> +
> +	if (len <= offset || !start)
> +		return NULL;
> +	len -= offset;
> +	start += offset;
> +	if (maxlen && len > maxlen)
> +		len = maxlen;
> +	if (flags & IORESOURCE_IO)
> +		return NULL;
> +	if (flags & IORESOURCE_MEM)
> +		return ioremap_wc(start, len);
> +	/* What? */
> +	return NULL;
> +}
> +EXPORT_SYMBOL_GPL(pci_iomap_wc_range);
> +
> +/**
>   * pci_iomap - create a virtual mapping cookie for a PCI BAR
>   * @dev: PCI device that owns the BAR
>   * @bar: BAR number
> @@ -70,4 +110,25 @@ void __iomem *pci_iomap(struct pci_dev *dev, int bar, unsigned long maxlen)
>  	return pci_iomap_range(dev, bar, 0, maxlen);
>  }
>  EXPORT_SYMBOL(pci_iomap);
> +
> +/**
> + * pci_iomap_wc - create a virtual WC mapping cookie for a PCI BAR
> + * @dev: PCI device that owns the BAR
> + * @bar: BAR number
> + * @maxlen: length of the memory to map
> + *
> + * Using this function you will get a __iomem address to your device BAR.
> + * You can access it using ioread*() and iowrite*(). These functions hide
> + * the details if this is a MMIO or PIO address space and will just do what
> + * you expect from them in the correct way. When possible write combining
> + * is used.
> + *
> + * @maxlen specifies the maximum length to map. If you want to get access to
> + * the complete BAR without checking for its length first, pass %0 here.
> + * */
> +void __iomem *pci_iomap_wc(struct pci_dev *dev, int bar, unsigned long maxlen)
> +{
> +	return pci_iomap_wc_range(dev, bar, 0, maxlen);
> +}
> +EXPORT_SYMBOL_GPL(pci_iomap_wc);

Huh.  So you let me talk about marking the unused pcim_iomap_wc()
EXPORT_SYMBOL_GPL(), but didn't remind me that you also proposed to mark
the symbol you really care about, the one you already have a use for, as
EXPORT_SYMBOL_GPL().  Sigh.

In my opinion, if we're going to use EXPORT_SYMBOL_GPL() at all, we should
use it consistently and based on technical considerations.  I base this on
statements like the following:

  - "[EXPORT_SYMBOL_GPL()] implies that the function is considered an
    internal implementation issue, and not really an interface." [Rusty
    Russell, 1]

  - "... using the xxx_GPL() version to show that it's an internal
    interface ..." [Linus Torvalds, 2]

  - "Anything exported via EXPORT_SYMBOL_GPL() is considered by the author
    to be so fundamental to the kernel that using it would be impossible
    without creating a derivative work." [Matthew Garrett, 3]

  - "Linus's initial point for [_GPL symbols] has been so diluted by random
    lobby groups asking for every symbol to be _GPL that they are becoming
    effectively pointless now." [Dave Airlie, 4]

Existing interfaces like these are exported with EXPORT_SYMBOL():

  ioremap()
  ioremap_wc()
  ioremap_prot()
  pci_iomap()
  pci_map_rom()

I would argue that pci_iomap_wc() is similar in spirit and is no more an
internal implementation issue than they are, and should be exported
similarly.

So my *advice* is to use EXPORT_SYMBOL() in this case, because that's a
choice you can defend on technical grounds.  I think it's hard to argue
that pci_iomap_wc() is so fundamental or unique to Linux that a caller
would automatically be a derivative work.

Will I still merge it as EXPORT_SYMBOL_GPL()?  Maybe.  I don't feel *good*
about it because the only explanation I can give is "the author wanted it
that way," and that's unsatisfying.  But I did already ack it (before I
noticed the _GPL() issue), and I won't try to retract that and prevent
somebody else from merging it.  And maybe your proposal to clarify the
kernel-hacking.tmpl language will convince me.

Bjorn

[1] http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=b6c17ea4eff3
[2] http://lkml.kernel.org/r/Pine.LNX.4.64.0510050742550.31407@g5.osdl.org
[3] http://mjg59.dreamwidth.org/31357.html
[4] http://lkml.kernel.org/r/CAPM=9tzsT+nah2P-qZ8iKW=aTZJzYgm18mMWyy2-RVkoOSwyjg@mail.gmail.com

Re: [PATCH v6 1/4] pci: add pci_iomap_wc() variants

[Bjorn Helgaas]

On Wed, May 20, 2015 at 04:08:10PM -0700, Luis R. Rodriguez wrote:
> From: "Luis R. Rodriguez" <mcgrof@suse.com>
> 
> This allows drivers to take advantage of write-combining
> when possible. The PCI specification does not allow for us
> to automatically identify a memory region which needs
> write-combining so drivers have to identify these areas
> on their own. There is IORESOURCE_PREFETCH but as clarified
> by Michael and confirmed later by Bjorn, PCI prefetch bit
> merely means bridges can combine writes and prefetch reads.
> Prefetch does not affect ordering rules and does not allow
> writes to be collapsed [0]. WC is stronger, it allows collapsing
> and changes ordering rules. WC can also hurt latency as small
> writes are buffered. Because of all this drivers needs to
> know what they are doing, we can't set a write-combining
> preference flag in the pci core automatically for drivers.
> 
> Lastly although there is also arch_phys_wc_add() this makes
> use of architecture specific write-combining *hacks* and
> the only one currently defined and used is MTRR for x86.
> MTRRs are legacy, limited in number, have restrictive size
> constraints, and are known to interact pooly with the BIOS.
> MTRRs should only really be considered on old video framebuffer
> drivers. If we made ioremap_wc() and similar calls start
> automatically adding MTRRs, then performance will vary wildly
> with the order of driver loading because we'll run out of MTRRs
> part-way through bootup.
> 
> There are a few motivations for phasing out of MTRR and
> helping driver change over to use write-combining with PAT:
> 
> a) Take advantage of PAT when available
> 
> b) Help bury MTRR code away, MTRR is architecture specific and on
>    x86 its replaced by PAT
> 
> c) Help with the goal of eventually using _PAGE_CACHE_UC over
>    _PAGE_CACHE_UC_MINUS on x86 on ioremap_nocache() (see commit
>    de33c442e titled "x86 PAT: fix performance drop for glx,
>    use UC minus for ioremap(), ioremap_nocache() and
>    pci_mmap_page_range()")
> 
> [0] https://lkml.org/lkml/2015/4/21/714

I tentatively put this (and the rest of the series) on a pci/resource
branch.  I'm hoping you'll propose some clarification about
EXPORT_SYMBOL_GPL().

In the meantime, I tried to make the changelog a bit more concise, as
below.  Let me know if I omitted something essential.  We still have URLs
for the discussion for the fine points.

commit 75387ae87b7aebc2a5b447f4d1b8b17a23c15b08
Author: Luis R. Rodriguez <mcgrof@suse.com>
Date:   Wed May 20 16:08:10 2015 -0700

    PCI: Add pci_iomap_wc() variants
    
    PCI BARs tell us whether prefetching is safe, but they don't say anything
    about write combining (WC).  WC changes ordering rules and allows writes to
    be collapsed, so it's not safe in general to use it on a prefetchable
    region.
    
    Add pci_iomap_wc() and pci_iomap_wc_range() so drivers can take advantage
    of write combining when they know it's safe.
    
    On architectures that don't fully support WC, e.g., x86 without PAT,
    drivers for legacy framebuffers may get some of the benefit by using
    arch_phys_wc_add() in addition to pci_iomap_wc().  But arch_phys_wc_add()
    is unreliable and should be avoided in general.  On x86, it uses MTRRs,
    which are limited in number and size, so the results will vary based on
    driver loading order.
    
    The goals of adding pci_iomap_wc() are to:
    
      - Give drivers an architecture-independent way to use WC so they can stop
        using interfaces like mtrr_add() (on x86, pci_iomap_wc() uses PAT when
        available)
    
      - Move toward using _PAGE_CACHE_MODE_UC, not _PAGE_CACHE_MODE_UC_MINUS,
        on x86 on ioremap_nocache() (see de33c442ed2a ("x86 PAT: fix
        performance drop for glx, use UC minus for ioremap(), ioremap_nocache()
        and pci_mmap_page_range()")
    
    [bhelgaas: changelog]
    Link: http://lkml.kernel.org/r/1426893517-2511-6-git-send-email-mcgrof@do-not-panic.com
    Original-posting: http://lkml.kernel.org/r/1432163293-20965-1-git-send-email-mcgrof@do-not-panic.com
    Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>
    Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

Re: [PATCH v6 1/4] pci: add pci_iomap_wc() variants

[Luis R. Rodriguez]

On Thu, May 21, 2015 at 05:33:21PM -0500, Bjorn Helgaas wrote:
> 
> I tentatively put this (and the rest of the series) on a pci/resource
> branch.  I'm hoping you'll propose some clarification about
> EXPORT_SYMBOL_GPL().

EXPORT_SYMBOL_GPL() also serves to ensure only GPL modules can
only run that code. So for instance although we have "Dual BSD/GPL"
tags for modules pure "BSD" tags do not exist for module tags and
cannot run EXPORT_SYMBOL_GPL() code [0]. Also there is some folks
who do believe tha at run time all kernel modules are GPL [1] [2].
And to be precise even though the FSF may claim a list of licenses
are GPL-compatible we cannot rely on this list alone for our own
goals and if folks want to use our EXPORT_SYMBOL_GPL()s they must
discuss this on lkml [2].

In the past when I've tried to try to clarify EXPORT_SYMBOL_GPL()
requirements, implications, its been said that its best to leave
some things as-is [3] and let attorneys figure things out. In so
far as to what exactly it is and can be used for requires legal
attorney review, but the question of derivative work certainly
comes up [4]. Now folks companies seem to want to obviously use
and abuse our symbols despite of all the things above, for instance
Red Hat once tried to change an EXPORT_SYMBOL_GPL() to
EXPORT_SYMBOL() [5]. Obviously that didn't go so well, and some
folks went off on a good rant about this [6].

What developers do and accept varies, I'm not going into pointing
out specifics and I do not wnat to do homework for folks who wish
to abuse things further, but by no means should a developer be
nack'd entry to new code if their functionality is not replacing
old one [9]. In this case this is new functionality. Also in terms
of preference:

"nobody has said that symbols exported with plain EXPORT_SYMBOL() can be freely
used by proprietary code; indeed, a number of developers claim that all (or
nearly all) loadable modules are derived products of the kernel regardless of
whether they use GPL-only symbols or not" [7].

And spot on:

"In general, the kernel community has long worked to maintain a vague and scary
ambiguity around the legal status of proprietary modules while being unwilling
to attempt to ban such modules outright." [7].

Now, a few maintainers insist on tons of new symbols to be EXPORT_SYMBOL_GPL()
though proactively [8] [9] and the reasons vary, I just happen to also write my
code to be perfectly clear with my goals and intent and you are the first to
ask me to reconsider this, even if you do make me use EXPORT_SYMBOL() my intent
and goal does not change, as with others. No code I ever write should be used
by proprietary shit, and I hope to convince others of the importance to do this
as well.

> In the meantime, I tried to make the changelog a bit more concise, as
> below.  Let me know if I omitted something essential.  We still have URLs
> for the discussion for the fine points.

Looks good.

[0] https://lkml.org/lkml/2012/4/7/102
[1] https://lkml.org/lkml/2012/4/8/71
[2] https://lkml.org/lkml/2012/4/8/71
[3] https://lkml.org/lkml/2009/6/1/385
[4] https://lkml.org/lkml/2009/6/1/376
[5] https://lkml.org/lkml/2012/4/20/328
[6] https://plus.google.com/+AlanCoxLinux/posts/D2feRNc6R4d
[7] https://lwn.net/Articles/603131/
[8] https://lwn.net/Articles/603139/
[9] https://lkml.org/lkml/2015/2/26/379

  Luis

Re: [PATCH v6 1/4] pci: add pci_iomap_wc() variants

[Bjorn Helgaas]

On Fri, May 22, 2015 at 02:23:41AM +0200, Luis R. Rodriguez wrote:
> On Thu, May 21, 2015 at 05:33:21PM -0500, Bjorn Helgaas wrote:
> > 
> > I tentatively put this (and the rest of the series) on a pci/resource
> > branch.  I'm hoping you'll propose some clarification about
> > EXPORT_SYMBOL_GPL().
> 
> EXPORT_SYMBOL_GPL() also serves to ensure only GPL modules can
> only run that code. So for instance although we have "Dual BSD/GPL"
> tags for modules pure "BSD" tags do not exist for module tags and
> cannot run EXPORT_SYMBOL_GPL() code [0]. Also there is some folks
> who do believe tha at run time all kernel modules are GPL [1] [2].
> And to be precise even though the FSF may claim a list of licenses
> are GPL-compatible we cannot rely on this list alone for our own
> goals and if folks want to use our EXPORT_SYMBOL_GPL()s they must
> discuss this on lkml [2].

By "propose some clarification," I meant that I hoped you would propose a
patch to Documentation/ that would give maintainers some guidance.

> ...

Re: [PATCH v6 1/4] pci: add pci_iomap_wc() variants

[Luis R. Rodriguez]

On Tue, May 26, 2015 at 12:40:08PM -0500, Bjorn Helgaas wrote:
> On Fri, May 22, 2015 at 02:23:41AM +0200, Luis R. Rodriguez wrote:
> > On Thu, May 21, 2015 at 05:33:21PM -0500, Bjorn Helgaas wrote:
> > > 
> > > I tentatively put this (and the rest of the series) on a pci/resource
> > > branch.  I'm hoping you'll propose some clarification about
> > > EXPORT_SYMBOL_GPL().
> > 
> > EXPORT_SYMBOL_GPL() also serves to ensure only GPL modules can
> > only run that code. So for instance although we have "Dual BSD/GPL"
> > tags for modules pure "BSD" tags do not exist for module tags and
> > cannot run EXPORT_SYMBOL_GPL() code [0]. Also there is some folks
> > who do believe tha at run time all kernel modules are GPL [1] [2].
> > And to be precise even though the FSF may claim a list of licenses
> > are GPL-compatible we cannot rely on this list alone for our own
> > goals and if folks want to use our EXPORT_SYMBOL_GPL()s they must
> > discuss this on lkml [2].
> 
> By "propose some clarification," I meant that I hoped you would propose a
> patch to Documentation/ that would give maintainers some guidance.

I *really really* would hate to do so but only because you insist, I'll look
into this...

ASDF

  Luis

Re: [PATCH v6 1/4] pci: add pci_iomap_wc() variants

[Luis R. Rodriguez]

On Wed, May 27, 2015 at 1:04 PM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
> On Tue, May 26, 2015 at 12:40:08PM -0500, Bjorn Helgaas wrote:
>> On Fri, May 22, 2015 at 02:23:41AM +0200, Luis R. Rodriguez wrote:
>> > On Thu, May 21, 2015 at 05:33:21PM -0500, Bjorn Helgaas wrote:
>> > >
>> > > I tentatively put this (and the rest of the series) on a pci/resource
>> > > branch.  I'm hoping you'll propose some clarification about
>> > > EXPORT_SYMBOL_GPL().
>> >
>> > EXPORT_SYMBOL_GPL() also serves to ensure only GPL modules can
>> > only run that code. So for instance although we have "Dual BSD/GPL"
>> > tags for modules pure "BSD" tags do not exist for module tags and
>> > cannot run EXPORT_SYMBOL_GPL() code [0]. Also there is some folks
>> > who do believe tha at run time all kernel modules are GPL [1] [2].
>> > And to be precise even though the FSF may claim a list of licenses
>> > are GPL-compatible we cannot rely on this list alone for our own
>> > goals and if folks want to use our EXPORT_SYMBOL_GPL()s they must
>> > discuss this on lkml [2].
>>
>> By "propose some clarification," I meant that I hoped you would propose a
>> patch to Documentation/ that would give maintainers some guidance.
>
> I *really really* would hate to do so but only because you insist, I'll look
> into this...

OK done. Please let me know if there is anything else I can do to
help. Also as per review with Tomi, the framebuffer maintainer, he
would prefer for only the required symbols to go through your tree.
We'd then wait for the next merge window for them to perculate to
Linus' tree and once there I'd send him a pull request for the
framebuffer device driver changes alone. So this does mean we'll have
no users of the symbols for a full release, but again, this is as per
Tomi's preference. This strategy is also the preference then for the
pci_iomap_wc() series as well. With that in mind, perhaps the lib
patch can go in as we'd have no users but we do have a few future
possible expected users.

 Luis

Re: [PATCH v6 1/4] pci: add pci_iomap_wc() variants

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 2457 bytes --]



On 29/05/15 03:36, Luis R. Rodriguez wrote:
> On Wed, May 27, 2015 at 1:04 PM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
>> On Tue, May 26, 2015 at 12:40:08PM -0500, Bjorn Helgaas wrote:
>>> On Fri, May 22, 2015 at 02:23:41AM +0200, Luis R. Rodriguez wrote:
>>>> On Thu, May 21, 2015 at 05:33:21PM -0500, Bjorn Helgaas wrote:
>>>>>
>>>>> I tentatively put this (and the rest of the series) on a pci/resource
>>>>> branch.  I'm hoping you'll propose some clarification about
>>>>> EXPORT_SYMBOL_GPL().
>>>>
>>>> EXPORT_SYMBOL_GPL() also serves to ensure only GPL modules can
>>>> only run that code. So for instance although we have "Dual BSD/GPL"
>>>> tags for modules pure "BSD" tags do not exist for module tags and
>>>> cannot run EXPORT_SYMBOL_GPL() code [0]. Also there is some folks
>>>> who do believe tha at run time all kernel modules are GPL [1] [2].
>>>> And to be precise even though the FSF may claim a list of licenses
>>>> are GPL-compatible we cannot rely on this list alone for our own
>>>> goals and if folks want to use our EXPORT_SYMBOL_GPL()s they must
>>>> discuss this on lkml [2].
>>>
>>> By "propose some clarification," I meant that I hoped you would propose a
>>> patch to Documentation/ that would give maintainers some guidance.
>>
>> I *really really* would hate to do so but only because you insist, I'll look
>> into this...
> 
> OK done. Please let me know if there is anything else I can do to
> help. Also as per review with Tomi, the framebuffer maintainer, he
> would prefer for only the required symbols to go through your tree.
> We'd then wait for the next merge window for them to perculate to
> Linus' tree and once there I'd send him a pull request for the
> framebuffer device driver changes alone. So this does mean we'll have
> no users of the symbols for a full release, but again, this is as per
> Tomi's preference. This strategy is also the preference then for the
> pci_iomap_wc() series as well. With that in mind, perhaps the lib
> patch can go in as we'd have no users but we do have a few future
> possible expected users.

I don't have any issue with fbdev changes going through other trees, but
I'd rather do that only if there are good reasons to go that way.

These changes to fbdev drivers look like cleanups, so they are not
critical, right? Does delaying the fbdev changes until the dependencies
are in prevent some other development?

 Tomi


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v6 1/4] pci: add pci_iomap_wc() variants

[Luis R. Rodriguez]

On Thu, May 28, 2015 at 10:57 PM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>
>
> On 29/05/15 03:36, Luis R. Rodriguez wrote:
>> On Wed, May 27, 2015 at 1:04 PM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
>>> On Tue, May 26, 2015 at 12:40:08PM -0500, Bjorn Helgaas wrote:
>>>> On Fri, May 22, 2015 at 02:23:41AM +0200, Luis R. Rodriguez wrote:
>>>>> On Thu, May 21, 2015 at 05:33:21PM -0500, Bjorn Helgaas wrote:
>>>>>>
>>>>>> I tentatively put this (and the rest of the series) on a pci/resource
>>>>>> branch.  I'm hoping you'll propose some clarification about
>>>>>> EXPORT_SYMBOL_GPL().
>>>>>
>>>>> EXPORT_SYMBOL_GPL() also serves to ensure only GPL modules can
>>>>> only run that code. So for instance although we have "Dual BSD/GPL"
>>>>> tags for modules pure "BSD" tags do not exist for module tags and
>>>>> cannot run EXPORT_SYMBOL_GPL() code [0]. Also there is some folks
>>>>> who do believe tha at run time all kernel modules are GPL [1] [2].
>>>>> And to be precise even though the FSF may claim a list of licenses
>>>>> are GPL-compatible we cannot rely on this list alone for our own
>>>>> goals and if folks want to use our EXPORT_SYMBOL_GPL()s they must
>>>>> discuss this on lkml [2].
>>>>
>>>> By "propose some clarification," I meant that I hoped you would propose a
>>>> patch to Documentation/ that would give maintainers some guidance.
>>>
>>> I *really really* would hate to do so but only because you insist, I'll look
>>> into this...
>>
>> OK done. Please let me know if there is anything else I can do to
>> help. Also as per review with Tomi, the framebuffer maintainer, he
>> would prefer for only the required symbols to go through your tree.
>> We'd then wait for the next merge window for them to perculate to
>> Linus' tree and once there I'd send him a pull request for the
>> framebuffer device driver changes alone. So this does mean we'll have
>> no users of the symbols for a full release, but again, this is as per
>> Tomi's preference. This strategy is also the preference then for the
>> pci_iomap_wc() series as well. With that in mind, perhaps the lib
>> patch can go in as we'd have no users but we do have a few future
>> possible expected users.
>
> I don't have any issue with fbdev changes going through other trees, but
> I'd rather do that only if there are good reasons to go that way.

OK, either way I prefer to go with maintainer's preferences. Most
changes are for framebuffer drivers as that's where MTRR is used most
these days.

> These changes to fbdev drivers look like cleanups, so they are not
> critical, right?

I'll let you make the call, I'll just provide information to you. I
trust your judgement and what you prefer.

This and other series which change use of MTRR to arch_phys enable use
of PAT when available, we want to bury out MTRR from further usage so
all these arch_phys changes will help with that. MTRR, although
supported, should be seen as a first step temporary architectural
evolution to what PAT became. There are known architectural issue with
MTRR, let me list the issues for you to review and consider and
evaluate:

  * MTRR acts on physical addresses and requires power-of-two
alignment, on both the base used and size, this limits the flexibility
of MTRR use
  * MTRR is known to be unreliable, it can at times not work even on
modern systems
  * MTRRs are limited, if using a large number of devices MTRRs will
run out fast, its why Andy ended up adding the arch_phys APIs
  * PAT has been available for quite a long time, since Pentium III
(circa 1999) and newer, but having PAT enabled does not restrict use
of MTRR and because of this some systems may end up then combining
MTRR and PAT. I do not believe this wasn't an original highly expected
wide use situation, it technically should work to combine both but
there might be issues with interactions between both, exactly what
issues can exist or have existed remains quite unclear as MTRR in and
of itself has been known to be unreliable anyway. If possible its best
to just be binary about this and only use MTRR if and only if
necessary because of the other issues known with MTRR.

With all these changes being merged the only use case for MTRR then
would be through the arch_phys APIs, which would just enable use of
MTRR when PAT is not available or a system / driver is known to
require MTRR -- those systems are expected to low numbered, in the end
after all these series Linux will will end up with only two device
drivers which will require MTRR to be enabled always:

  * ipath: this device driver is old, powers the old HTX bus cards
that only work in AMD systems, while the newer IB/qib device driver
powers all PCI-e cards. The ipath device driver is obsolete, hardware
hard to find. In fact the maintainers of this driver have recently
even seriously discussed removing the driver from upstream altogether.
  * ivtv: the hardware is really rare these days, its expected only
some lost souls in some third world country are expected to be using
the feature which requires MTRR. The way this driver uses MTRR is also
quite questionable, but still has been present in the driver for a
long time.

These two device drivers can get PAT disabled by just using the kernel
parameter to disable PAT upon boot. The kernel log will tell them what
to do exactly.

Another use case for still using MTRR are cases where PAT is known to
have errata. We actually want to learn about those users who have
issues with PAT though. Last I heard about this was that the required
errata folks are aware of can be worked around by having PAT entry 0
and entry 4 both map to WB. hpa / andy might be able to elaborate if
needed, but we might even be able to preemptively already deal with
the errata issues for users.

Now let me explain the gains with all these changes being merged:

  * Although there are known PAT errata, we want to learn about those
issues and fix the issues
  * We have a long term goal to change the default behavior of
ioremap_nocache() and pci_mmap_page_range() to use PAT strong UC,
right now we cannot do this, but after all drivers are converted (all
these series I've been posting) we expect to be able to make the
change. Making a change to strong UC on these two calls can only
happen after a period of time of having Linux bake with all these
changes merged and in place. How many kernels we will want Linux baked
with all these transformations to arch_phys before making a change to
ioremap_nocache() and pci_mmap_page_range() is up to x86 folks. There
are other gains possible with this but I welcome others to chime in
here with what gains we can expect from this
  * Xen lacks support for MTRR upstream, but has full PAT support.
These changes will help enable use of the devices which exclusively
used MTRR to work on Xen with write combining moving forward

> Does delaying the fbdev changes until the dependencies are in prevent some other development?

Just the later work on putting the nail on the MTRR coffin, and
delaying the change of default  ioremap_nocache() and
pci_mmap_page_range() from UC- to strong UC. We ideally should want a
few kernel release with all the fbdev changes in place, not sure how
many we'd want, but the sooner we get folks testing kernels with that
the sooner we'd be able to evaluate doing a flip form UC- to strong
UC.

 Luis

Re: [PATCH v6 1/4] pci: add pci_iomap_wc() variants

[Luis R. Rodriguez]

On Thu, May 28, 2015 at 5:36 PM, Luis R. Rodriguez
<mcgrof@do-not-panic.com> wrote:
> On Wed, May 27, 2015 at 1:04 PM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
>> On Tue, May 26, 2015 at 12:40:08PM -0500, Bjorn Helgaas wrote:
>>> On Fri, May 22, 2015 at 02:23:41AM +0200, Luis R. Rodriguez wrote:
>>> > On Thu, May 21, 2015 at 05:33:21PM -0500, Bjorn Helgaas wrote:
>>> > >
>>> > > I tentatively put this (and the rest of the series) on a pci/resource
>>> > > branch.  I'm hoping you'll propose some clarification about
>>> > > EXPORT_SYMBOL_GPL().
>>> >
>>> > EXPORT_SYMBOL_GPL() also serves to ensure only GPL modules can
>>> > only run that code. So for instance although we have "Dual BSD/GPL"
>>> > tags for modules pure "BSD" tags do not exist for module tags and
>>> > cannot run EXPORT_SYMBOL_GPL() code [0]. Also there is some folks
>>> > who do believe tha at run time all kernel modules are GPL [1] [2].
>>> > And to be precise even though the FSF may claim a list of licenses
>>> > are GPL-compatible we cannot rely on this list alone for our own
>>> > goals and if folks want to use our EXPORT_SYMBOL_GPL()s they must
>>> > discuss this on lkml [2].
>>>
>>> By "propose some clarification," I meant that I hoped you would propose a
>>> patch to Documentation/ that would give maintainers some guidance.
>>
>> I *really really* would hate to do so but only because you insist, I'll look
>> into this...
>
> OK done.

Bjorn,

This is now on Jonathan Corbet's tree and visible on linux-next:

https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/?id=582ed8d51e2b6cb8a168c94852bca482685c2509

> Please let me know if there is anything else I can do to
> help.

Please let me know.

> Also as per review with Tomi, the framebuffer maintainer, he
> would prefer for only the required symbols to go through your tree.
> We'd then wait for the next merge window for them to perculate to
> Linus' tree and once there I'd send him a pull request for the
> framebuffer device driver changes alone. So this does mean we'll have
> no users of the symbols for a full release, but again, this is as per
> Tomi's preference. This strategy is also the preference then for the
> pci_iomap_wc() series as well. With that in mind, perhaps the lib
> patch can go in as we'd have no users but we do have a few future
> possible expected users.

I repoked Tomi about this topic with a new context provided, my
expressed hope was to just merge the fbvdev dependent changes for both
series (now both Acked by Tomi) through your tree.

 Luis

Re: [PATCH v6 1/4] pci: add pci_iomap_wc() variants

[Bjorn Helgaas]

On Tue, Jun 16, 2015 at 2:16 PM, Luis R. Rodriguez
<mcgrof@do-not-panic.com> wrote:
> On Thu, May 28, 2015 at 5:36 PM, Luis R. Rodriguez
> <mcgrof@do-not-panic.com> wrote:
>> On Wed, May 27, 2015 at 1:04 PM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
>>> On Tue, May 26, 2015 at 12:40:08PM -0500, Bjorn Helgaas wrote:
>>>> On Fri, May 22, 2015 at 02:23:41AM +0200, Luis R. Rodriguez wrote:
>>>> > On Thu, May 21, 2015 at 05:33:21PM -0500, Bjorn Helgaas wrote:
>>>> > >
>>>> > > I tentatively put this (and the rest of the series) on a pci/resource
>>>> > > branch.  I'm hoping you'll propose some clarification about
>>>> > > EXPORT_SYMBOL_GPL().
>>>> >
>>>> > EXPORT_SYMBOL_GPL() also serves to ensure only GPL modules can
>>>> > only run that code. So for instance although we have "Dual BSD/GPL"
>>>> > tags for modules pure "BSD" tags do not exist for module tags and
>>>> > cannot run EXPORT_SYMBOL_GPL() code [0]. Also there is some folks
>>>> > who do believe tha at run time all kernel modules are GPL [1] [2].
>>>> > And to be precise even though the FSF may claim a list of licenses
>>>> > are GPL-compatible we cannot rely on this list alone for our own
>>>> > goals and if folks want to use our EXPORT_SYMBOL_GPL()s they must
>>>> > discuss this on lkml [2].
>>>>
>>>> By "propose some clarification," I meant that I hoped you would propose a
>>>> patch to Documentation/ that would give maintainers some guidance.
>>>
>>> I *really really* would hate to do so but only because you insist, I'll look
>>> into this...
>>
>> OK done.
>
> Bjorn,
>
> This is now on Jonathan Corbet's tree and visible on linux-next:
>
> https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/?id=582ed8d51e2b6cb8a168c94852bca482685c2509

Sorry, I'm just not comfortable with using EXPORT_SYMBOL_GPL() this
way.  I'm happy to use it when it has a technical justification, e.g.,
for internal interfaces where users of the interface are clearly
derived works.  But pci_iomap_wc() is not in that category, and I
think it should be symmetric with similar interfaces like pci_iomap()
and ioremap_wc().

I don't want to use EXPORT_SYMBOL_GPL() for a random collection of
things depending on the whim of the author.  That makes for a messy
environment to work in, and it's messy enough already.  If we wanted
to remove the EXPORT_SYMBOL/EXPORT_SYMBOL_GPL distinction completely,
that'd be fine with me, too.  But as long as we keep it, I think it
should mean something more than the preference of the author.

I know I did already ack this, and I even said I would merge it, but a
month of thinking about this hasn't made me more comfortable with it,
so I've changed my mind.  I said before that I wouldn't try to stop
you if you want to merge it some other way, but I don't want to ack
it, and I don't want to merge it via my tree.

Bjorn

Re: [PATCH v6 1/4] pci: add pci_iomap_wc() variants

[Luis R. Rodriguez]

On Tue, Jun 16, 2015 at 3:20 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Tue, Jun 16, 2015 at 2:16 PM, Luis R. Rodriguez
> <mcgrof@do-not-panic.com> wrote:
>> On Thu, May 28, 2015 at 5:36 PM, Luis R. Rodriguez
>> <mcgrof@do-not-panic.com> wrote:
>>> On Wed, May 27, 2015 at 1:04 PM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
>>>> On Tue, May 26, 2015 at 12:40:08PM -0500, Bjorn Helgaas wrote:
>>>>> On Fri, May 22, 2015 at 02:23:41AM +0200, Luis R. Rodriguez wrote:
>>>>> > On Thu, May 21, 2015 at 05:33:21PM -0500, Bjorn Helgaas wrote:
>>>>> > >
>>>>> > > I tentatively put this (and the rest of the series) on a pci/resource
>>>>> > > branch.  I'm hoping you'll propose some clarification about
>>>>> > > EXPORT_SYMBOL_GPL().
>>>>> >
>>>>> > EXPORT_SYMBOL_GPL() also serves to ensure only GPL modules can
>>>>> > only run that code. So for instance although we have "Dual BSD/GPL"
>>>>> > tags for modules pure "BSD" tags do not exist for module tags and
>>>>> > cannot run EXPORT_SYMBOL_GPL() code [0]. Also there is some folks
>>>>> > who do believe tha at run time all kernel modules are GPL [1] [2].
>>>>> > And to be precise even though the FSF may claim a list of licenses
>>>>> > are GPL-compatible we cannot rely on this list alone for our own
>>>>> > goals and if folks want to use our EXPORT_SYMBOL_GPL()s they must
>>>>> > discuss this on lkml [2].
>>>>>
>>>>> By "propose some clarification," I meant that I hoped you would propose a
>>>>> patch to Documentation/ that would give maintainers some guidance.
>>>>
>>>> I *really really* would hate to do so but only because you insist, I'll look
>>>> into this...
>>>
>>> OK done.
>>
>> Bjorn,
>>
>> This is now on Jonathan Corbet's tree and visible on linux-next:
>>
>> https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/?id=582ed8d51e2b6cb8a168c94852bca482685c2509
>
> Sorry, I'm just not comfortable with using EXPORT_SYMBOL_GPL() this
> way.  I'm happy to use it when it has a technical justification, e.g.,
> for internal interfaces where users of the interface are clearly
> derived works.

I believe it is completely fair for some maintainers to take the
position of what the documentation used to say prior to the new
documentation patch on queue for v4.2, but note that that is an old
position and I seriously caution against it. A few reasons I caution
against it:

1)  It used to be believed that EXPORT_SYMBOL_GPL() was pretty
pointless by many maintainers and developers -- in particular for all
those who have always written even EXPORT_SYMBOL() code with the same
intent as EXPORT_SYMBOL_GPL(). Experience and comments with attorneys
even on Linus' part reflects that there is a huge legal value to
EXPORT_SYMBOL_GPL() [0].

The skinny: Intent matters a lot and "circumventing a GPL-only export
requires an explicit action, making it clear that the resulting
copyright infringement was a deliberate act".

Naturally lax positions on the matter over use of EXPORT_SYMBOL_GPL()
have evolved over time then, not only about its value but also then as
a consequence about where its used in practice today by maintainers
and developers in different trees.

[0] https://lwn.net/Articles/154602/

2) The old position of use of EXPORT_SYMBOL_GPL() about the
derivatives work punts onto the maintainer the onus over the question
of derivatives work -- such question really should not be taken
lightly and this responsibility should really not fall onto the
developer, it requires attorney involvement and should not be taken
lightly by any means.

3) Most drivers are upstream these days, we want to avoid bug reports
from crappy proprietary drivers, specially as we add new features. We
don't have to be begging vendors to work upstream these days, that's
rather the norm. The landscape has changed dramatically. Even Linus
has told Nvidia to go fuck themselves lately, bravo.

> But pci_iomap_wc() is not in that category, and I
> think it should be symmetric with similar interfaces like pci_iomap()
> and ioremap_wc().

Those are two old APIs, and quite the contrary, as I noted we have a
series of new PAT APIs that old modules did not use that are now being
added and spread into the kernel onto which upstream maintainers have
been insisting on using EXPORT_SYMBOL_GPL() even though older similar
APIs only used EXPORT_SYMBOL(). As a recent example the family of APIs
set_pages_array_xx() are all EXPORT_SYMBOL() but Toshi's new
set_pages_array_wt() was asked to be changed to EXPORT_SYMBOL_GPL()
because as noted by Ingo:

--------------
By default we make new APIs EXPORT_SYMBOL_GPL():
we don't want proprietary modules mucking around with new code
PAT interfaces, we only want modules we can analyze and fix
in detail.
--------------

http://article.gmane.org/gmane.linux.kernel.mm/129104

> I don't want to use EXPORT_SYMBOL_GPL() for a random collection of
> things depending on the whim of the author.

Its not just me as I note above, and the new APIs I'm introducing are
also to help with PAT usage.

> That makes for a messy environment to work in, and it's messy enough already.

You're right about the mismatch over the kernel and even on a set of
family of APIs, that's a fair argument, but *that* is a very different
problem. Also the "messy environment" you describe is for proprietary
drivers and it always has been a messy environment for them, that's on
them and they should be talking to attorneys! The issue is already
super messy for them considering tons of folks using EXPORT_SYMBOL()
with the same intent as EXPORT_SYMBOL_GPL(), and refusing to use
EXPORT_SYMBOL_GPL() because of this.

> If we wanted  to remove the EXPORT_SYMBOL/EXPORT_SYMBOL_GPL distinction
> completely,  that'd be fine with me, too.

You're not the only one who has argued over this and the misfortune
over the naming used for EXPORT_SYMBOL_GPL(). Christoph made a similar
argument recently and provided some technical ideas on how to improve
the situation with symbol groups to reflect more of what was intended.
I also followed up with some further possibilities of how to do that
work [1]. If we really care about this so much perhaps we should talk
about this as a separate topic and address it but I would strongly
prefer to avoid this discussion at this point on this thread. If we
really want to we can talk about it over beers at Plumbers perhaps.

[1] http://lkml.kernel.org/r/20150529174051.GC23057@wotan.suse.de

>  But as long as we keep it, I think it  should mean something more than the preference of the author.

We extend the documentation as things become relevant and as you noted
to help with guidance, we did just that recently upon your request and
I provided basis for why things have changed over time now, as well as
reasoning by other maintainers on PAT interfaces, I hope this helps
clarify things.

> I know I did already ack this, and I even said I would merge it, but a
> month of thinking about this hasn't made me more comfortable with it,
> so I've changed my mind.  I said before that I wouldn't try to stop
> you if you want to merge it some other way, but I don't want to ack
> it, and I don't want to merge it via my tree.

I appreciate your thoroughness over this matter and patience. I hope I
have provided new information to help you reconsider. Contrary to the
old days where we needed more of technical reason to use
EXPORT_SYMBOL_GPL() since its use has proven to have huge value, I
personally will always prefer to *always* use EXPORT_SYMBOL_GPL() for
all *new* exported symbols, unless explicitly told by the maintainer,
and even if they do ask for me to  use EXPORT_SYMBOL_GPL() obviously
my intent remains the same, and no one can change that. These days
maintainer's don't ask me to change EXPORT_SYMBPOL_GPL() to
EXPORT_SYMBOL() though, and if they do they are of the position that
EXPORT_SYBOL() means the same thing as EXPORT_SYMBOL_GPL(), you're the
first to question this but it seems it was for the lack of guidance we
had to reflect new best practices.

I hope to have provided a bit of new information to help you
reconsider this series to go through you but since you seem to be fine
for this to go through another tree and since I failed to notice that
I should also get Arnd's Ack I am in hopes this might be able to go
through Arnd's tree if not through you. Please let me know, in case I
have to resubmit to Arnd.

 Luis
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in

Re: [PATCH v6 1/4] pci: add pci_iomap_wc() variants

[Bjorn Helgaas]

On Fri, Jun 19, 2015 at 4:06 PM, Luis R. Rodriguez
<mcgrof@do-not-panic.com> wrote:

> I hope to have provided a bit of new information to help you
> reconsider this series to go through you but since you seem to be fine
> for this to go through another tree and since I failed to notice that
> I should also get Arnd's Ack I am in hopes this might be able to go
> through Arnd's tree if not through you. Please let me know, in case I
> have to resubmit to Arnd.

If you want to have Arnd merge it, that's fine with me.

Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH v6 2/4] video: fbdev: arkfb: use arch_phys_wc_add() and pci_iomap_wc()

[Luis R. Rodriguez]

From: "Luis R. Rodriguez" <mcgrof@suse.com>

Convert the driver from using the x86 specific MTRR code to
the architecture agnostic arch_phys_wc_add(). arch_phys_wc_add()
will avoid MTRR if write-combining is available, in order to
take advantage of that also ensure the ioremap'd area is requested
as write-combining.

There are a few motivations for this:

a) Take advantage of PAT when available

b) Help bury MTRR code away, MTRR is architecture specific and on
   x86 its replaced by PAT

c) Help with the goal of eventually using _PAGE_CACHE_UC over
   _PAGE_CACHE_UC_MINUS on x86 on ioremap_nocache() (see commit
   de33c442e titled "x86 PAT: fix performance drop for glx,
   use UC minus for ioremap(), ioremap_nocache() and
   pci_mmap_page_range()")

The conversion done is expressed by the following Coccinelle
SmPL patch, it additionally required manual intervention to
address all the #ifdery and removal of redundant things which
arch_phys_wc_add() already addresses such as verbose message
about when MTRR fails and doing nothing when we didn't get
an MTRR.

@ mtrr_found @
expression index, base, size;
@@

-index = mtrr_add(base, size, MTRR_TYPE_WRCOMB, 1);
+index = arch_phys_wc_add(base, size);

@ mtrr_rm depends on mtrr_found @
expression mtrr_found.index, mtrr_found.base, mtrr_found.size;
@@

-mtrr_del(index, base, size);
+arch_phys_wc_del(index);

@ mtrr_rm_zero_arg depends on mtrr_found @
expression mtrr_found.index;
@@

-mtrr_del(index, 0, 0);
+arch_phys_wc_del(index);

@ mtrr_rm_fb_info depends on mtrr_found @
struct fb_info *info;
expression mtrr_found.index;
@@

-mtrr_del(index, info->fix.smem_start, info->fix.smem_len);
+arch_phys_wc_del(index);

@ ioremap_replace_nocache depends on mtrr_found @
struct fb_info *info;
expression base, size;
@@

-info->screen_base = ioremap_nocache(base, size);
+info->screen_base = ioremap_wc(base, size);

@ ioremap_replace_default depends on mtrr_found @
struct fb_info *info;
expression base, size;
@@

-info->screen_base = ioremap(base, size);
+info->screen_base = ioremap_wc(base, size);

Generated-by: Coccinelle SmPL
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: "Lad, Prabhakar" <prabhakar.csengg@gmail.com>
Cc: Suresh Siddha <sbsiddha@gmail.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Juergen Gross <jgross@suse.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Dave Airlie <airlied@redhat.com>
Cc: Antonino Daplas <adaplas@gmail.com>
Cc: Jean-Christophe Plagniol-Villard <plagnioj@jcrosoft.com>
Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
Cc: linux-fbdev@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Acked-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>
---
 drivers/video/fbdev/arkfb.c | 36 +++++-------------------------------
 1 file changed, 5 insertions(+), 31 deletions(-)

diff --git a/drivers/video/fbdev/arkfb.c b/drivers/video/fbdev/arkfb.c
index b305a1e..6a317de 100644
--- a/drivers/video/fbdev/arkfb.c
+++ b/drivers/video/fbdev/arkfb.c
@@ -26,13 +26,9 @@
 #include <linux/console.h> /* Why should fb driver call console functions? because console_lock() */
 #include <video/vga.h>
 
-#ifdef CONFIG_MTRR
-#include <asm/mtrr.h>
-#endif
-
 struct arkfb_info {
 	int mclk_freq;
-	int mtrr_reg;
+	int wc_cookie;
 
 	struct dac_info *dac;
 	struct vgastate state;
@@ -102,10 +98,6 @@ static const struct svga_timing_regs ark_timing_regs     = {
 
 static char *mode_option = "640x480-8@60";
 
-#ifdef CONFIG_MTRR
-static int mtrr = 1;
-#endif
-
 MODULE_AUTHOR("(c) 2007 Ondrej Zajicek <santiago@crfreenet.org>");
 MODULE_LICENSE("GPL");
 MODULE_DESCRIPTION("fbdev driver for ARK 2000PV");
@@ -115,11 +107,6 @@ MODULE_PARM_DESC(mode_option, "Default video mode ('640x480-8@60', etc)");
 module_param_named(mode, mode_option, charp, 0444);
 MODULE_PARM_DESC(mode, "Default video mode ('640x480-8@60', etc) (deprecated)");
 
-#ifdef CONFIG_MTRR
-module_param(mtrr, int, 0444);
-MODULE_PARM_DESC(mtrr, "Enable write-combining with MTRR (1=enable, 0=disable, default=1)");
-#endif
-
 static int threshold = 4;
 
 module_param(threshold, int, 0644);
@@ -1002,7 +989,7 @@ static int ark_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
 	info->fix.smem_len = pci_resource_len(dev, 0);
 
 	/* Map physical IO memory address into kernel space */
-	info->screen_base = pci_iomap(dev, 0, 0);
+	info->screen_base = pci_iomap_wc(dev, 0, 0);
 	if (! info->screen_base) {
 		rc = -ENOMEM;
 		dev_err(info->device, "iomap for framebuffer failed\n");
@@ -1057,14 +1044,8 @@ static int ark_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
 
 	/* Record a reference to the driver data */
 	pci_set_drvdata(dev, info);
-
-#ifdef CONFIG_MTRR
-	if (mtrr) {
-		par->mtrr_reg = -1;
-		par->mtrr_reg = mtrr_add(info->fix.smem_start, info->fix.smem_len, MTRR_TYPE_WRCOMB, 1);
-	}
-#endif
-
+	par->wc_cookie = arch_phys_wc_add(info->fix.smem_start,
+					  info->fix.smem_len);
 	return 0;
 
 	/* Error handling */
@@ -1092,14 +1073,7 @@ static void ark_pci_remove(struct pci_dev *dev)
 
 	if (info) {
 		struct arkfb_info *par = info->par;
-
-#ifdef CONFIG_MTRR
-		if (par->mtrr_reg >= 0) {
-			mtrr_del(par->mtrr_reg, 0, 0);
-			par->mtrr_reg = -1;
-		}
-#endif
-
+		arch_phys_wc_del(par->wc_cookie);
 		dac_release(par->dac);
 		unregister_framebuffer(info);
 		fb_dealloc_cmap(&info->cmap);
-- 
2.3.2.209.gd67f9d5.dirty

[PATCH v6 3/4] video: fbdev: s3fb: use arch_phys_wc_add() and pci_iomap_wc()

[Luis R. Rodriguez]

From: "Luis R. Rodriguez" <mcgrof@suse.com>

This driver uses the same area for MTRR as for the ioremap().
Convert the driver from using the x86 specific MTRR code to
the architecture agnostic arch_phys_wc_add(). arch_phys_wc_add()
will avoid MTRR if write-combining is available, in order to
take advantage of that also ensure the ioremap'd area is requested
as write-combining.

There are a few motivations for this:

a) Take advantage of PAT when available

b) Help bury MTRR code away, MTRR is architecture specific and on
   x86 its replaced by PAT

c) Help with the goal of eventually using _PAGE_CACHE_UC over
   _PAGE_CACHE_UC_MINUS on x86 on ioremap_nocache() (see commit
   de33c442e titled "x86 PAT: fix performance drop for glx,
   use UC minus for ioremap(), ioremap_nocache() and
   pci_mmap_page_range()")

The conversion done is expressed by the following Coccinelle
SmPL patch, it additionally required manual intervention to
address all the #ifdery and removal of redundant things which
arch_phys_wc_add() already addresses such as verbose message
about when MTRR fails and doing nothing when we didn't get
an MTRR.

@ mtrr_found @
expression index, base, size;
@@

-index = mtrr_add(base, size, MTRR_TYPE_WRCOMB, 1);
+index = arch_phys_wc_add(base, size);

@ mtrr_rm depends on mtrr_found @
expression mtrr_found.index, mtrr_found.base, mtrr_found.size;
@@

-mtrr_del(index, base, size);
+arch_phys_wc_del(index);

@ mtrr_rm_zero_arg depends on mtrr_found @
expression mtrr_found.index;
@@

-mtrr_del(index, 0, 0);
+arch_phys_wc_del(index);

@ mtrr_rm_fb_info depends on mtrr_found @
struct fb_info *info;
expression mtrr_found.index;
@@

-mtrr_del(index, info->fix.smem_start, info->fix.smem_len);
+arch_phys_wc_del(index);

@ ioremap_replace_nocache depends on mtrr_found @
struct fb_info *info;
expression base, size;
@@

-info->screen_base = ioremap_nocache(base, size);
+info->screen_base = ioremap_wc(base, size);

@ ioremap_replace_default depends on mtrr_found @
struct fb_info *info;
expression base, size;
@@

-info->screen_base = ioremap(base, size);
+info->screen_base = ioremap_wc(base, size);

Generated-by: Coccinelle SmPL
Cc: Jean-Christophe Plagniol-Villard <plagnioj@jcrosoft.com>
Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
Cc: Jingoo Han <jg1.han@samsung.com>
Cc: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: "Lad, Prabhakar" <prabhakar.csengg@gmail.com>
Cc: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se>
Cc: Suresh Siddha <sbsiddha@gmail.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Juergen Gross <jgross@suse.com>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Dave Airlie <airlied@redhat.com>
Cc: Antonino Daplas <adaplas@gmail.com>
Cc: linux-fbdev@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Acked-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>
---
 drivers/video/fbdev/s3fb.c | 35 ++++++-----------------------------
 1 file changed, 6 insertions(+), 29 deletions(-)

diff --git a/drivers/video/fbdev/s3fb.c b/drivers/video/fbdev/s3fb.c
index f0ae61a..13b1090 100644
--- a/drivers/video/fbdev/s3fb.c
+++ b/drivers/video/fbdev/s3fb.c
@@ -28,13 +28,9 @@
 #include <linux/i2c.h>
 #include <linux/i2c-algo-bit.h>
 
-#ifdef CONFIG_MTRR
-#include <asm/mtrr.h>
-#endif
-
 struct s3fb_info {
 	int chip, rev, mclk_freq;
-	int mtrr_reg;
+	int wc_cookie;
 	struct vgastate state;
 	struct mutex open_lock;
 	unsigned int ref_count;
@@ -154,11 +150,7 @@ static const struct svga_timing_regs s3_timing_regs     = {
 
 
 static char *mode_option;
-
-#ifdef CONFIG_MTRR
 static int mtrr = 1;
-#endif
-
 static int fasttext = 1;
 
 
@@ -170,11 +162,8 @@ module_param(mode_option, charp, 0444);
 MODULE_PARM_DESC(mode_option, "Default video mode ('640x480-8@60', etc)");
 module_param_named(mode, mode_option, charp, 0444);
 MODULE_PARM_DESC(mode, "Default video mode ('640x480-8@60', etc) (deprecated)");
-
-#ifdef CONFIG_MTRR
 module_param(mtrr, int, 0444);
 MODULE_PARM_DESC(mtrr, "Enable write-combining with MTRR (1=enable, 0=disable, default=1)");
-#endif
 
 module_param(fasttext, int, 0644);
 MODULE_PARM_DESC(fasttext, "Enable S3 fast text mode (1=enable, 0=disable, default=1)");
@@ -1168,7 +1157,7 @@ static int s3_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
 	info->fix.smem_len = pci_resource_len(dev, 0);
 
 	/* Map physical IO memory address into kernel space */
-	info->screen_base = pci_iomap(dev, 0, 0);
+	info->screen_base = pci_iomap_wc(dev, 0, 0);
 	if (! info->screen_base) {
 		rc = -ENOMEM;
 		dev_err(info->device, "iomap for framebuffer failed\n");
@@ -1365,12 +1354,9 @@ static int s3_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
 	/* Record a reference to the driver data */
 	pci_set_drvdata(dev, info);
 
-#ifdef CONFIG_MTRR
-	if (mtrr) {
-		par->mtrr_reg = -1;
-		par->mtrr_reg = mtrr_add(info->fix.smem_start, info->fix.smem_len, MTRR_TYPE_WRCOMB, 1);
-	}
-#endif
+	if (mtrr)
+		par->wc_cookie = arch_phys_wc_add(info->fix.smem_start,
+						  info->fix.smem_len);
 
 	return 0;
 
@@ -1405,14 +1391,7 @@ static void s3_pci_remove(struct pci_dev *dev)
 
 	if (info) {
 		par = info->par;
-
-#ifdef CONFIG_MTRR
-		if (par->mtrr_reg >= 0) {
-			mtrr_del(par->mtrr_reg, 0, 0);
-			par->mtrr_reg = -1;
-		}
-#endif
-
+		arch_phys_wc_del(par->wc_cookie);
 		unregister_framebuffer(info);
 		fb_dealloc_cmap(&info->cmap);
 
@@ -1551,10 +1530,8 @@ static int  __init s3fb_setup(char *options)
 
 		if (!*opt)
 			continue;
-#ifdef CONFIG_MTRR
 		else if (!strncmp(opt, "mtrr:", 5))
 			mtrr = simple_strtoul(opt + 5, NULL, 0);
-#endif
 		else if (!strncmp(opt, "fasttext:", 9))
 			fasttext = simple_strtoul(opt + 9, NULL, 0);
 		else
-- 
2.3.2.209.gd67f9d5.dirty

[PATCH v6 4/4] video: fbdev: vt8623fb: use arch_phys_wc_add() and pci_iomap_wc()

[Luis R. Rodriguez]

From: "Luis R. Rodriguez" <mcgrof@suse.com>

This driver uses the same area for MTRR as for the ioremap().
Convert the driver from using the x86 specific MTRR code to
the architecture agnostic arch_phys_wc_add(). arch_phys_wc_add()
will avoid MTRR if write-combining is available, in order to
take advantage of that also ensure the ioremap'd area is requested
as write-combining.

There are a few motivations for this:

a) Take advantage of PAT when available

b) Help bury MTRR code away, MTRR is architecture specific and on
   x86 its replaced by PAT

c) Help with the goal of eventually using _PAGE_CACHE_UC over
   _PAGE_CACHE_UC_MINUS on x86 on ioremap_nocache() (see commit
   de33c442e titled "x86 PAT: fix performance drop for glx,
   use UC minus for ioremap(), ioremap_nocache() and
   pci_mmap_page_range()")

The conversion done is expressed by the following Coccinelle
SmPL patch, it additionally required manual intervention to
address all the #ifdery and removal of redundant things which
arch_phys_wc_add() already addresses such as verbose message
about when MTRR fails and doing nothing when we didn't get
an MTRR.

@ mtrr_found @
expression index, base, size;
@@

-index = mtrr_add(base, size, MTRR_TYPE_WRCOMB, 1);
+index = arch_phys_wc_add(base, size);

@ mtrr_rm depends on mtrr_found @
expression mtrr_found.index, mtrr_found.base, mtrr_found.size;
@@

-mtrr_del(index, base, size);
+arch_phys_wc_del(index);

@ mtrr_rm_zero_arg depends on mtrr_found @
expression mtrr_found.index;
@@

-mtrr_del(index, 0, 0);
+arch_phys_wc_del(index);

@ mtrr_rm_fb_info depends on mtrr_found @
struct fb_info *info;
expression mtrr_found.index;
@@

-mtrr_del(index, info->fix.smem_start, info->fix.smem_len);
+arch_phys_wc_del(index);

@ ioremap_replace_nocache depends on mtrr_found @
struct fb_info *info;
expression base, size;
@@

-info->screen_base = ioremap_nocache(base, size);
+info->screen_base = ioremap_wc(base, size);

@ ioremap_replace_default depends on mtrr_found @
struct fb_info *info;
expression base, size;
@@

-info->screen_base = ioremap(base, size);
+info->screen_base = ioremap_wc(base, size);

Generated-by: Coccinelle SmPL
Cc: Rob Clark <robdclark@gmail.com>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Jingoo Han <jg1.han@samsung.com>
Cc: "Lad, Prabhakar" <prabhakar.csengg@gmail.com>
Cc: Suresh Siddha <sbsiddha@gmail.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Juergen Gross <jgross@suse.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Dave Airlie <airlied@redhat.com>
Cc: Antonino Daplas <adaplas@gmail.com>
Cc: Jean-Christophe Plagniol-Villard <plagnioj@jcrosoft.com>
Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
Cc: linux-fbdev@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Acked-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>
---
 drivers/video/fbdev/vt8623fb.c | 31 ++++++-------------------------
 1 file changed, 6 insertions(+), 25 deletions(-)

diff --git a/drivers/video/fbdev/vt8623fb.c b/drivers/video/fbdev/vt8623fb.c
index ea7f056..60f24828 100644
--- a/drivers/video/fbdev/vt8623fb.c
+++ b/drivers/video/fbdev/vt8623fb.c
@@ -26,13 +26,9 @@
 #include <linux/console.h> /* Why should fb driver call console functions? because console_lock() */
 #include <video/vga.h>
 
-#ifdef CONFIG_MTRR
-#include <asm/mtrr.h>
-#endif
-
 struct vt8623fb_info {
 	char __iomem *mmio_base;
-	int mtrr_reg;
+	int wc_cookie;
 	struct vgastate state;
 	struct mutex open_lock;
 	unsigned int ref_count;
@@ -99,10 +95,7 @@ static struct svga_timing_regs vt8623_timing_regs     = {
 /* Module parameters */
 
 static char *mode_option = "640x480-8@60";
-
-#ifdef CONFIG_MTRR
 static int mtrr = 1;
-#endif
 
 MODULE_AUTHOR("(c) 2006 Ondrej Zajicek <santiago@crfreenet.org>");
 MODULE_LICENSE("GPL");
@@ -112,11 +105,8 @@ module_param(mode_option, charp, 0644);
 MODULE_PARM_DESC(mode_option, "Default video mode ('640x480-8@60', etc)");
 module_param_named(mode, mode_option, charp, 0);
 MODULE_PARM_DESC(mode, "Default video mode e.g. '648x480-8@60' (deprecated)");
-
-#ifdef CONFIG_MTRR
 module_param(mtrr, int, 0444);
 MODULE_PARM_DESC(mtrr, "Enable write-combining with MTRR (1=enable, 0=disable, default=1)");
-#endif
 
 
 /* ------------------------------------------------------------------------- */
@@ -710,7 +700,7 @@ static int vt8623_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
 	info->fix.mmio_len = pci_resource_len(dev, 1);
 
 	/* Map physical IO memory address into kernel space */
-	info->screen_base = pci_iomap(dev, 0, 0);
+	info->screen_base = pci_iomap_wc(dev, 0, 0);
 	if (! info->screen_base) {
 		rc = -ENOMEM;
 		dev_err(info->device, "iomap for framebuffer failed\n");
@@ -781,12 +771,9 @@ static int vt8623_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
 	/* Record a reference to the driver data */
 	pci_set_drvdata(dev, info);
 
-#ifdef CONFIG_MTRR
-	if (mtrr) {
-		par->mtrr_reg = -1;
-		par->mtrr_reg = mtrr_add(info->fix.smem_start, info->fix.smem_len, MTRR_TYPE_WRCOMB, 1);
-	}
-#endif
+	if (mtrr)
+		par->wc_cookie = arch_phys_wc_add(info->fix.smem_start,
+						  info->fix.smem_len);
 
 	return 0;
 
@@ -816,13 +803,7 @@ static void vt8623_pci_remove(struct pci_dev *dev)
 	if (info) {
 		struct vt8623fb_info *par = info->par;
 
-#ifdef CONFIG_MTRR
-		if (par->mtrr_reg >= 0) {
-			mtrr_del(par->mtrr_reg, 0, 0);
-			par->mtrr_reg = -1;
-		}
-#endif
-
+		arch_phys_wc_del(par->wc_cookie);
 		unregister_framebuffer(info);
 		fb_dealloc_cmap(&info->cmap);
 
-- 
2.3.2.209.gd67f9d5.dirty