From: Luis R. Rodriguez <hidden> Date: 2015-07-09 01:54:40
From: "Luis R. Rodriguez" <redacted>
Ingo,
Boris is on vacation, he picked up these patches on his bp#tip-mm tree [0]
and they have baked there for a while now. That tree receives 0-day
bot testing, but other than that its not clear what other tests were
run on these patches. Boris modified the commit logs a bit, and made one
optimizaiton to bail early on an PCI ioremap call when it should. These
patches have no modifications from what is on Boris' tree and tip-mm branch.
The 0 day build bot did find issues on Boris' tree but those are related
to ioremap_uc() (already upstream) and its first use on atyfb (not
upstream) -- I will be addressing a fix for that ioremap_uc() issue through
another patch series prior to posting the final set for atyfb which makes
use of ioremap_uc().
No issues have been found with this series. Benh did note some possible issues
with expectations with what is done for write-combining for PowerPC [1] but
the issue is a rather general long standing issue with semantics of ioremap --
in the case for ioremap_wc() on PowerPC benh notes that writel() will never
write-combine as it uses too heavy barriers. Benh notes that although
writel_relaxed() today is identical to writel() this can be changed. There are
other general semantics issues with ioremap() variant calls -- we seem to have
all gotten together to discuss all these issues on a thread where Dan Williams
is proposing to "unify ioremap prototypes and macro aliases" [1], folks
intersted on these issues or semantic concerns can drop in and chime there.
Let me know if these are OK or if there are any questions.
[0] http://lkml.kernel.org/r/20150625204703.GC4898@pd.tnic
[1] http://lkml.kernel.org/r/20150707095012.GQ7021@wotan.suse.de
Luis R. Rodriguez (8):
PCI: Add pci_ioremap_wc_bar()
drivers/video/fbdev/i740fb: Use arch_phys_wc_add() and
pci_ioremap_wc_bar()
drivers/video/fbdev/kyrofb: Use arch_phys_wc_add() and
pci_ioremap_wc_bar()
drivers/video/fbdev/gxt4500: Use pci_ioremap_wc_bar() to map
framebuffer
PCI: Add pci_iomap_wc() variants
drivers/video/fbdev/arkfb.c: Use arch_phys_wc_add() and pci_iomap_wc()
drivers/video/fbdev/s3fb: Use arch_phys_wc_add() and pci_iomap_wc()
drivers/video/fbdev/vt8623fb: Use arch_phys_wc_add() and
pci_iomap_wc()
drivers/pci/pci.c | 14 +++++++++
drivers/video/fbdev/arkfb.c | 36 +++-------------------
drivers/video/fbdev/gxt4500.c | 2 +-
drivers/video/fbdev/i740fb.c | 35 ++++-----------------
drivers/video/fbdev/kyro/fbdev.c | 33 +++++++-------------
drivers/video/fbdev/s3fb.c | 35 ++++-----------------
drivers/video/fbdev/vt8623fb.c | 31 ++++---------------
include/asm-generic/pci_iomap.h | 14 +++++++++
include/linux/pci.h | 1 +
include/video/kyro.h | 4 +--
lib/pci_iomap.c | 66 ++++++++++++++++++++++++++++++++++++++++
11 files changed, 131 insertions(+), 140 deletions(-)
--
2.3.2.209.gd67f9d5.dirty
From: Luis R. Rodriguez <hidden> Date: 2015-07-09 01:54:57
From: "Luis R. Rodriguez" <redacted>
This lets drivers take advantage of PAT when available. It should help
with the transition of converting video drivers over to ioremap_wc()
to help with the goal of eventually using _PAGE_CACHE_UC over
_PAGE_CACHE_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()")
Signed-off-by: Luis R. Rodriguez <redacted>
Acked-by: Arnd Bergmann <arnd@arndb.de>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Antonino Daplas <adaplas@gmail.com>
Cc: benh@kernel.crashing.org
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Daniel Vetter <redacted>
Cc: Dave Airlie <airlied@redhat.com>
Cc: Davidlohr Bueso <redacted>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Jean-Christophe Plagniol-Villard <redacted>
Cc: Juergen Gross <jgross@suse.com>
Cc: linux-fbdev@vger.kernel.org
Cc: linux-pci@vger.kernel.org
Cc: Mel Gorman <mgorman@suse.de>
Cc: mst@redhat.com
Cc: Suresh Siddha <redacted>
Cc: Thomas Gleixner <redacted>
Cc: Tomi Valkeinen <redacted>
Cc: Toshi Kani <redacted>
Cc: Ville Syrjälä <syrjala@sci.fi>
Cc: Vlastimil Babka <redacted>
Link: http://lkml.kernel.org/r/1435195342-26879-2-git-send-email-mcgrof@do-not-panic.com
Signed-off-by: Borislav Petkov <redacted>
---
drivers/pci/pci.c | 14 ++++++++++++++
include/linux/pci.h | 1 +
2 files changed, 15 insertions(+)
From: Luis R. Rodriguez <hidden> Date: 2015-07-09 01:55:07
From: "Luis R. Rodriguez" <redacted>
Convert the driver from using the x86-specific MTRR code to the
architecture-agnostic arch_phys_wc_add(). It will avoid MTRR if
write-combining is available, in order to take advantage of that also
ensure the ioremapped 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 it is being 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
ifdeffery 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);
Signed-off-by: Luis R. Rodriguez <redacted>
Acked-by: Tomi Valkeinen <redacted>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Antonino Daplas <adaplas@gmail.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: benh@kernel.crashing.org
Cc: Benoit Taine <redacted>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Daniel Vetter <redacted>
Cc: Dave Airlie <airlied@redhat.com>
Cc: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Jean-Christophe Plagniol-Villard <redacted>
Cc: Jingoo Han <redacted>
Cc: Juergen Gross <jgross@suse.com>
Cc: linux-fbdev@vger.kernel.org
Cc: linux-pci@vger.kernel.org
Cc: mst@redhat.com
Cc: Rob Clark <redacted>
Cc: Suresh Siddha <redacted>
Cc: Thomas Gleixner <redacted>
Cc: toshi.kani@hp.com
Link: http://lkml.kernel.org/r/1435195342-26879-3-git-send-email-mcgrof@do-not-panic.com
Signed-off-by: Borislav Petkov <redacted>
---
drivers/video/fbdev/i740fb.c | 35 ++++++-----------------------------
1 file changed, 6 insertions(+), 29 deletions(-)
From: Luis R. Rodriguez <hidden> Date: 2015-07-09 01:55:17
From: "Luis R. Rodriguez" <redacted>
Convert the driver from using the x86-specific MTRR code to the
architecture-agnostic arch_phys_wc_add(). It will avoid MTRR if
write-combining is available, in order to take advantage of that
also ensure the ioremapped 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 it is being 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
ifdeffery 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);
Signed-off-by: Luis R. Rodriguez <redacted>
Acked-by: Tomi Valkeinen <redacted>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Antonino Daplas <adaplas@gmail.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: benh@kernel.crashing.org
Cc: bhelgaas@google.com
Cc: Daniel Vetter <redacted>
Cc: Dave Airlie <airlied@redhat.com>
Cc: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Jean-Christophe Plagniol-Villard <redacted>
Cc: Jingoo Han <redacted>
Cc: Juergen Gross <jgross@suse.com>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: linux-fbdev@vger.kernel.org
Cc: linux-pci@vger.kernel.org
Cc: mst@redhat.com
Cc: Suresh Siddha <redacted>
Cc: Thomas Gleixner <redacted>
Cc: toshi.kani@hp.com
Link: http://lkml.kernel.org/r/1435195342-26879-4-git-send-email-mcgrof@do-not-panic.com
Signed-off-by: Borislav Petkov <redacted>
---
drivers/video/fbdev/kyro/fbdev.c | 33 +++++++++++----------------------
include/video/kyro.h | 4 +---
2 files changed, 12 insertions(+), 25 deletions(-)
@@ -35,9 +35,7 @@ struct kyrofb_info {/* Useful to hold depth here for Linux */u8PIXDEPTH;-#ifdef CONFIG_MTRR-intmtrr_handle;-#endif+intwc_cookie;};externintkyro_dev_init(void);
From: Luis R. Rodriguez <hidden> Date: 2015-07-09 01:55:23
From: "Luis R. Rodriguez" <redacted>
The driver doesn't use mtrr_add() or arch_phys_wc_add() but since we
know the framebuffer is isolated already on an ioremap() we can take
advantage of write combining for performance where possible.
In this case there are a few motivations for this:
a) Take advantage of PAT when available.
b) 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()").
Signed-off-by: Luis R. Rodriguez <redacted>
Acked-by: Tomi Valkeinen <redacted>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Antonino Daplas <adaplas@gmail.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: benh@kernel.crashing.org
Cc: bhelgaas@google.com
Cc: Daniel Vetter <redacted>
Cc: Dave Airlie <airlied@redhat.com>
Cc: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Jean-Christophe Plagniol-Villard <redacted>
Cc: Juergen Gross <jgross@suse.com>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: linux-fbdev@vger.kernel.org
Cc: linux-pci@vger.kernel.org
Cc: mst@redhat.com
Cc: Rob Clark <redacted>
Cc: Suresh Siddha <redacted>
Cc: Thomas Gleixner <redacted>
Cc: toshi.kani@hp.com
Link: http://lkml.kernel.org/r/1435195342-26879-5-git-send-email-mcgrof@do-not-panic.com
Signed-off-by: Borislav Petkov <redacted>
---
drivers/video/fbdev/gxt4500.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: Luis R. Rodriguez <hidden> Date: 2015-07-09 01:55:56
From: "Luis R. Rodriguez" <redacted>
Convert the driver from using the x86-specific MTRR code to the
architecture-agnostic arch_phys_wc_add(). It will avoid MTRRs if
write-combining is available. In order to take advantage of that also
ensure the ioremapped 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 it is being 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 ifdeffery 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);
Signed-off-by: Luis R. Rodriguez <redacted>
Acked-by: Tomi Valkeinen <redacted>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Antonino Daplas <adaplas@gmail.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: benh@kernel.crashing.org
Cc: bhelgaas@google.com
Cc: Daniel Vetter <redacted>
Cc: Dave Airlie <airlied@redhat.com>
Cc: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Jean-Christophe Plagniol-Villard <redacted>
Cc: Juergen Gross <jgross@suse.com>
Cc: "Lad, Prabhakar" <prabhakar.csengg@gmail.com>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: linux-fbdev@vger.kernel.org
Cc: linux-pci@vger.kernel.org
Cc: mst@redhat.com
Cc: Suresh Siddha <redacted>
Cc: Thomas Gleixner <redacted>
Cc: toshi.kani@hp.com
Link: http://lkml.kernel.org/r/1435195342-26879-8-git-send-email-mcgrof@do-not-panic.com
Signed-off-by: Borislav Petkov <redacted>
---
drivers/video/fbdev/arkfb.c | 36 +++++-------------------------------
1 file changed, 5 insertions(+), 31 deletions(-)
@@ -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-structarkfb_info{intmclk_freq;-intmtrr_reg;+intwc_cookie;structdac_info*dac;structvgastatestate;
@@ -102,10 +98,6 @@ static const struct svga_timing_regs ark_timing_regs = {staticchar*mode_option="640x480-8@60";-#ifdef CONFIG_MTRR-staticintmtrr=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-staticintthreshold=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);return0;/* Error handling */
From: Luis R. Rodriguez <hidden> Date: 2015-07-09 01:56:08
From: "Luis R. Rodriguez" <redacted>
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(). It will avoid MTRRs if
write-combining is available. In order to take advantage of that
also ensure the ioremapped 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 it is being 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 ifdeffery 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);
Signed-off-by: Luis R. Rodriguez <redacted>
Acked-by: Tomi Valkeinen <redacted>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Antonino Daplas <adaplas@gmail.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: benh@kernel.crashing.org
Cc: bhelgaas@google.com
Cc: Daniel Vetter <redacted>
Cc: Dave Airlie <airlied@redhat.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Jean-Christophe Plagniol-Villard <redacted>
Cc: Jingoo Han <redacted>
Cc: Juergen Gross <jgross@suse.com>
Cc: "Lad, Prabhakar" <prabhakar.csengg@gmail.com>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: linux-fbdev@vger.kernel.org
Cc: linux-pci@vger.kernel.org
Cc: mst@redhat.com
Cc: Rob Clark <redacted>
Cc: Suresh Siddha <redacted>
Cc: Thomas Gleixner <redacted>
Cc: toshi.kani@hp.com
Link: http://lkml.kernel.org/r/1435195342-26879-10-git-send-email-mcgrof@do-not-panic.com
Signed-off-by: Borislav Petkov <redacted>
---
drivers/video/fbdev/vt8623fb.c | 31 ++++++-------------------------
1 file changed, 6 insertions(+), 25 deletions(-)
@@ -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_MTRRmodule_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);return0;
From: Luis R. Rodriguez <hidden> Date: 2015-07-09 01:56:16
From: "Luis R. Rodriguez" <redacted>
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(). It will avoid MTRRs if
write-combining is available. In order to take advantage of that
also ensure the ioremapped 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 it is being 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 ifdeffery 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);
Signed-off-by: Luis R. Rodriguez <redacted>
Acked-by: Tomi Valkeinen <redacted>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Antonino Daplas <adaplas@gmail.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: benh@kernel.crashing.org
Cc: bhelgaas@google.com
Cc: Daniel Vetter <redacted>
Cc: Dave Airlie <airlied@redhat.com>
Cc: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Jean-Christophe Plagniol-Villard <redacted>
Cc: Jingoo Han <redacted>
Cc: Juergen Gross <jgross@suse.com>
Cc: "Lad, Prabhakar" <prabhakar.csengg@gmail.com>
Cc: linux-fbdev@vger.kernel.org
Cc: linux-pci@vger.kernel.org
Cc: mst@redhat.com
Cc: Rickard Strandqvist <redacted>
Cc: Suresh Siddha <redacted>
Cc: Thomas Gleixner <redacted>
Cc: toshi.kani@hp.com
Link: http://lkml.kernel.org/r/1435195342-26879-9-git-send-email-mcgrof@do-not-panic.com
Signed-off-by: Borislav Petkov <redacted>
---
drivers/video/fbdev/s3fb.c | 35 ++++++-----------------------------
1 file changed, 6 insertions(+), 29 deletions(-)
@@ -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_MTRRmodule_param(mtrr,int,0444);MODULE_PARM_DESC(mtrr,"Enable write-combining with MTRR (1=enable, 0=disable, default=1)");-#endifmodule_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);return0;
From: Luis R. Rodriguez <hidden> Date: 2015-07-09 01:56:31
From: "Luis R. Rodriguez" <redacted>
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()").
Signed-off-by: Luis R. Rodriguez <redacted>
Acked-by: Arnd Bergmann <arnd@arndb.de>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Antonino Daplas <adaplas@gmail.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: benh@kernel.crashing.org
Cc: bhelgaas@google.com
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Daniel Vetter <redacted>
Cc: Dave Airlie <airlied@redhat.com>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Davidlohr Bueso <redacted>
Cc: david.vrabel@citrix.com
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: jbeulich@suse.com
Cc: Jean-Christophe Plagniol-Villard <redacted>
Cc: Juergen Gross <jgross@suse.com>
Cc: konrad.wilk@oracle.com
Cc: linux-arch@vger.kernel.org
Cc: linux-fbdev@vger.kernel.org
Cc: linux-pci@vger.kernel.org
Cc: Mel Gorman <mgorman@suse.de>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Roger Pau Monné <redacted>
Cc: Rusty Russell <redacted>
Cc: Stefan Bader <redacted>
Cc: Suresh Siddha <redacted>
Cc: Thomas Gleixner <redacted>
Cc: Tomi Valkeinen <redacted>
Cc: Toshi Kani <redacted>
Cc: venkatesh.pallipadi@intel.com
Cc: Ville Syrjälä <syrjala@sci.fi>
Cc: Vlastimil Babka <redacted>
Link: http://lkml.kernel.org/r/1426893517-2511-6-git-send-email-mcgrof@do-not-panic.com
Link: http://lkml.kernel.org/r/1435195342-26879-6-git-send-email-mcgrof@do-not-panic.com
[ Move IORESOURCE_IO check up, space out statements for better readability. ]
Signed-off-by: Borislav Petkov <redacted>
---
include/asm-generic/pci_iomap.h | 14 +++++++++
lib/pci_iomap.c | 66 +++++++++++++++++++++++++++++++++++++++++
2 files changed, 80 insertions(+)
@@ -15,9 +15,13 @@ struct pci_dev;#ifdef CONFIG_PCI/* Create a virtual mapping cookie for a PCI BAR (memory or IO) */externvoid__iomem*pci_iomap(structpci_dev*dev,intbar,unsignedlongmax);+externvoid__iomem*pci_iomap_wc(structpci_dev*dev,intbar,unsignedlongmax);externvoid__iomem*pci_iomap_range(structpci_dev*dev,intbar,unsignedlongoffset,unsignedlongmaxlen);+externvoid__iomem*pci_iomap_wc_range(structpci_dev*dev,intbar,+unsignedlongoffset,+unsignedlongmaxlen);/* Create a virtual mapping cookie for a port on a given PCI device.*Donotcallthisdirectly,itexiststomakeiteasierforarchitectures*tooverride*/
@@ -34,12 +38,22 @@ static inline void __iomem *pci_iomap(struct pci_dev *dev, int bar, unsigned lonreturnNULL;}+staticinlinevoid__iomem*pci_iomap_wc(structpci_dev*dev,intbar,unsignedlongmax)+{+returnNULL;+}staticinlinevoid__iomem*pci_iomap_range(structpci_dev*dev,intbar,unsignedlongoffset,unsignedlongmaxlen){returnNULL;}+staticinlinevoid__iomem*pci_iomap_wc_range(structpci_dev*dev,intbar,+unsignedlongoffset,+unsignedlongmaxlen)+{+returnNULL;+}#endif#endif /* __ASM_GENERIC_IO_H */
From: Luis R. Rodriguez <hidden> Date: 2015-07-17 20:29:37
On Wed, Jul 08, 2015 at 06:54:11PM -0700, Luis R. Rodriguez wrote:
From: "Luis R. Rodriguez" <redacted>
Ingo,
Boris is on vacation, he picked up these patches on his bp#tip-mm tree [0]
and they have baked there for a while now. That tree receives 0-day
bot testing, but other than that its not clear what other tests were
run on these patches. Boris modified the commit logs a bit, and made one
optimizaiton to bail early on an PCI ioremap call when it should. These
patches have no modifications from what is on Boris' tree and tip-mm branch.
The 0 day build bot did find issues on Boris' tree but those are related
to ioremap_uc() (already upstream) and its first use on atyfb (not
upstream) -- I will be addressing a fix for that ioremap_uc() issue through
another patch series prior to posting the final set for atyfb which makes
use of ioremap_uc().
No issues have been found with this series. Benh did note some possible issues
with expectations with what is done for write-combining for PowerPC [1] but
the issue is a rather general long standing issue with semantics of ioremap --
in the case for ioremap_wc() on PowerPC benh notes that writel() will never
write-combine as it uses too heavy barriers. Benh notes that although
writel_relaxed() today is identical to writel() this can be changed. There are
other general semantics issues with ioremap() variant calls -- we seem to have
all gotten together to discuss all these issues on a thread where Dan Williams
is proposing to "unify ioremap prototypes and macro aliases" [1], folks
intersted on these issues or semantic concerns can drop in and chime there.
Let me know if these are OK or if there are any questions.
[0] http://lkml.kernel.org/r/20150625204703.GC4898@pd.tnic
[1] http://lkml.kernel.org/r/20150707095012.GQ7021@wotan.suse.de
Ingo,
Just a friendly reminder. Let me know if there are any issues or questions.
Luis
On Wed, Jul 08, 2015 at 06:54:11PM -0700, Luis R. Rodriguez wrote:
quoted
From: "Luis R. Rodriguez" <redacted>
Ingo,
Boris is on vacation, he picked up these patches on his bp#tip-mm tree [0]
and they have baked there for a while now. That tree receives 0-day
bot testing, but other than that its not clear what other tests were
run on these patches. Boris modified the commit logs a bit, and made one
optimizaiton to bail early on an PCI ioremap call when it should. These
patches have no modifications from what is on Boris' tree and tip-mm branch.
The 0 day build bot did find issues on Boris' tree but those are related
to ioremap_uc() (already upstream) and its first use on atyfb (not
upstream) -- I will be addressing a fix for that ioremap_uc() issue through
another patch series prior to posting the final set for atyfb which makes
use of ioremap_uc().
No issues have been found with this series. Benh did note some possible issues
with expectations with what is done for write-combining for PowerPC [1] but
the issue is a rather general long standing issue with semantics of ioremap --
in the case for ioremap_wc() on PowerPC benh notes that writel() will never
write-combine as it uses too heavy barriers. Benh notes that although
writel_relaxed() today is identical to writel() this can be changed. There are
other general semantics issues with ioremap() variant calls -- we seem to have
all gotten together to discuss all these issues on a thread where Dan Williams
is proposing to "unify ioremap prototypes and macro aliases" [1], folks
intersted on these issues or semantic concerns can drop in and chime there.
Let me know if these are OK or if there are any questions.
[0] http://lkml.kernel.org/r/20150625204703.GC4898@pd.tnic
[1] http://lkml.kernel.org/r/20150707095012.GQ7021@wotan.suse.de
Ingo,
Just a friendly reminder. Let me know if there are any issues or questions.
It would be nice to get an Acked-by from Bjorn for the PCI API bits.
Thanks,
Ingo
On Tue, Jul 21, 2015 at 10:52:52AM +0200, Ingo Molnar wrote:
* Luis R. Rodriguez [off-list ref] wrote:
quoted
On Wed, Jul 08, 2015 at 06:54:11PM -0700, Luis R. Rodriguez wrote:
quoted
From: "Luis R. Rodriguez" <redacted>
Ingo,
Boris is on vacation, he picked up these patches on his bp#tip-mm tree [0]
and they have baked there for a while now. That tree receives 0-day
bot testing, but other than that its not clear what other tests were
run on these patches. Boris modified the commit logs a bit, and made one
optimizaiton to bail early on an PCI ioremap call when it should. These
patches have no modifications from what is on Boris' tree and tip-mm branch.
The 0 day build bot did find issues on Boris' tree but those are related
to ioremap_uc() (already upstream) and its first use on atyfb (not
upstream) -- I will be addressing a fix for that ioremap_uc() issue through
another patch series prior to posting the final set for atyfb which makes
use of ioremap_uc().
No issues have been found with this series. Benh did note some possible issues
with expectations with what is done for write-combining for PowerPC [1] but
the issue is a rather general long standing issue with semantics of ioremap --
in the case for ioremap_wc() on PowerPC benh notes that writel() will never
write-combine as it uses too heavy barriers. Benh notes that although
writel_relaxed() today is identical to writel() this can be changed. There are
other general semantics issues with ioremap() variant calls -- we seem to have
all gotten together to discuss all these issues on a thread where Dan Williams
is proposing to "unify ioremap prototypes and macro aliases" [1], folks
intersted on these issues or semantic concerns can drop in and chime there.
Let me know if these are OK or if there are any questions.
[0] http://lkml.kernel.org/r/20150625204703.GC4898@pd.tnic
[1] http://lkml.kernel.org/r/20150707095012.GQ7021@wotan.suse.de
Ingo,
Just a friendly reminder. Let me know if there are any issues or questions.
It would be nice to get an Acked-by from Bjorn for the PCI API bits.
I think the actual code of pci_ioremap_wc() and pci_ioremap_wc_bar() is
fine (although I might have named it pci_ioremap_bar_wc() for consistency).
I declined to merge or ack them myself because they're obvious extensions
of pci_ioremap() and pci_ioremap_bar(), and I would prefer that they be
exported the same way, i.e., with EXPORT_SYMBOL(), not EXPORT_SYMBOL_GPL().
I think using EXPORT_SYMBOL_GPL to express individual political aims rather
than as a hint about what might be derived work makes us look like zealots,
and that's not my style.
Bjorn
Ingo,
Just a friendly reminder. Let me know if there are any issues or questions.
It would be nice to get an Acked-by from Bjorn for the PCI API bits.
I think the actual code of pci_ioremap_wc() and pci_ioremap_wc_bar() is fine
(although I might have named it pci_ioremap_bar_wc() for consistency).
I declined to merge or ack them myself because they're obvious extensions of
pci_ioremap() and pci_ioremap_bar(), and I would prefer that they be exported
the same way, i.e., with EXPORT_SYMBOL(), not EXPORT_SYMBOL_GPL().
Huh? AFAICS pci_ioremap_bar() has been a _GPL export for a long time:
1684f5ddd4c0c (Andrew Morton 2008-12-01 14:30:30 -0800 126)#ifdef CONFIG_HAS_IOMEM
1684f5ddd4c0c (Andrew Morton 2008-12-01 14:30:30 -0800 127)void __iomem *pci_ioremap_bar(struct pci_dev *pdev, int bar)
1684f5ddd4c0c (Andrew Morton 2008-12-01 14:30:30 -0800 128){
1f7bf3bfb5d60 (Bjorn Helgaas 2015-03-12 12:30:11 -0500 129) struct resource *res = &pdev->resource[bar];
1f7bf3bfb5d60 (Bjorn Helgaas 2015-03-12 12:30:11 -0500 130)
1684f5ddd4c0c (Andrew Morton 2008-12-01 14:30:30 -0800 131) /*
1684f5ddd4c0c (Andrew Morton 2008-12-01 14:30:30 -0800 132) * Make sure the BAR is actually a memory resource, not an IO resource
1684f5ddd4c0c (Andrew Morton 2008-12-01 14:30:30 -0800 133) */
646c0282df042 (Bjorn Helgaas 2015-03-12 12:30:15 -0500 134) if (res->flags & IORESOURCE_UNSET || !(res->flags & IORESOURCE_MEM)) {
1f7bf3bfb5d60 (Bjorn Helgaas 2015-03-12 12:30:11 -0500 135) dev_warn(&pdev->dev, "can't ioremap BAR %d: %pR\n", bar, res);
1684f5ddd4c0c (Andrew Morton 2008-12-01 14:30:30 -0800 136) return NULL;
1684f5ddd4c0c (Andrew Morton 2008-12-01 14:30:30 -0800 137) }
1f7bf3bfb5d60 (Bjorn Helgaas 2015-03-12 12:30:11 -0500 138) return ioremap_nocache(res->start, resource_size(res));
1684f5ddd4c0c (Andrew Morton 2008-12-01 14:30:30 -0800 139)}
1684f5ddd4c0c (Andrew Morton 2008-12-01 14:30:30 -0800 140)EXPORT_SYMBOL_GPL(pci_ioremap_bar);
1684f5ddd4c0c (Andrew Morton 2008-12-01 14:30:30 -0800 141)#endif
commit 1684f5ddd4c0c happened in 2008, well before you became PCI maintainer in
2012.
and I'd prefer keeping the same EXPORT_SYMBOL_GPL() pattern for the new APIs.
(ioremap_wc() is EXPORT_SYMBOL() mostly by accident, it's the odd one out.)
Also, FWIIW: I personally got essentially zero feedback and help from proprietary
binary kernel module vendors in the past couple of years as x86 maintainer,
despite a fair chunk of kernel crashes reported on distro kernels occuring in
them...
Based on that very negative experience, when we introduce something as complex and
as critical as new caching APIs, the last thing I want is to have obscure bugs in
binary modules I cannot fix in any reasonable fashion. So even if the parent APIs
of new APIs weren't already _GPL exports (as in this case), I'd export them as
_GPL in this case.
I think using EXPORT_SYMBOL_GPL to express individual political aims rather than
as a hint about what might be derived work makes us look like zealots, and
that's not my style.
As far as I'm concerned it's a pure technological choice: I don't want to export
certain types of hard to fix and critical functionality to drivers that I cannot
then fix.
But I also applied EXPORT_SYMBOL() patches in the past, so I'm not one-sided about
it.
Thanks,
Ingo
Ingo,
Just a friendly reminder. Let me know if there are any issues or questions.
It would be nice to get an Acked-by from Bjorn for the PCI API bits.
I think the actual code of pci_ioremap_wc() and pci_ioremap_wc_bar() is fine
(although I might have named it pci_ioremap_bar_wc() for consistency).
I declined to merge or ack them myself because they're obvious extensions of
pci_ioremap() and pci_ioremap_bar(), and I would prefer that they be exported
the same way, i.e., with EXPORT_SYMBOL(), not EXPORT_SYMBOL_GPL().
Huh? AFAICS pci_ioremap_bar() has been a _GPL export for a long time:
...
(ioremap_wc() is EXPORT_SYMBOL() mostly by accident, it's the odd one out.)
You're right, I was mistaken about pci_ioremap_bar(). But I'm not
convinced yet that ioremap_wc() is the odd one out. All the interfaces I
found, with the exception of ioremap_uc() on x86 and pci_ioremap_bar(), are
EXPORT_SYMBOL(), even the _wc and _wt flavors.
Also, FWIIW: I personally got essentially zero feedback and help from proprietary
binary kernel module vendors in the past couple of years as x86 maintainer,
despite a fair chunk of kernel crashes reported on distro kernels occuring in
them...
Based on that very negative experience, when we introduce something as complex and
as critical as new caching APIs, the last thing I want is to have obscure bugs in
binary modules I cannot fix in any reasonable fashion. So even if the parent APIs
of new APIs weren't already _GPL exports (as in this case), I'd export them as
_GPL in this case.
quoted
I think using EXPORT_SYMBOL_GPL to express individual political aims rather than
as a hint about what might be derived work makes us look like zealots, and
that's not my style.
As far as I'm concerned it's a pure technological choice: I don't want to export
certain types of hard to fix and critical functionality to drivers that I cannot
then fix.
That's a good argument that I hadn't heard before (or possibly it was there
and I missed it). It would be stronger still if we could change the parent
APIs similarly. If a proprietary driver can't use pci_ioremap_wc() because
it's exported _GPL, it's trivial to use ioremap_wc() directly.
Bjorn
Ingo,
Just a friendly reminder. Let me know if there are any issues or questions.
It would be nice to get an Acked-by from Bjorn for the PCI API bits.
I think the actual code of pci_ioremap_wc() and pci_ioremap_wc_bar() is fine
(although I might have named it pci_ioremap_bar_wc() for consistency).
I declined to merge or ack them myself because they're obvious extensions of
pci_ioremap() and pci_ioremap_bar(), and I would prefer that they be exported
the same way, i.e., with EXPORT_SYMBOL(), not EXPORT_SYMBOL_GPL().
Huh? AFAICS pci_ioremap_bar() has been a _GPL export for a long time:
...
(ioremap_wc() is EXPORT_SYMBOL() mostly by accident, it's the odd one out.)
You're right, I was mistaken about pci_ioremap_bar(). But I'm not
convinced yet that ioremap_wc() is the odd one out. All the interfaces I
found, with the exception of ioremap_uc() on x86 and pci_ioremap_bar(), are
EXPORT_SYMBOL(), even the _wc and _wt flavors.
The documentation update I provided on EXPORT_SYMBOL_GPL() which is now
upstream, at *your request* specifically for this series, *acknowledges* these
differences in opinions about new features, but it also clarifies that
positions on EXPORT_SYMBOL_GPL() can be up to developers and maintainers then,
so long as its for *new features*.
So this can be a position to take, and the guidence is there now. You asked for
this.
I acknowledge a subtle topic we seem to have stumbled upon here is what happens
if there is old "related technology" APIs with EXPORT_SYMBPOL(). Let me
elaborate on that, in my effort to provide guidence to reflect some historical
baggage while being apolagetic for those who hold a position of exclusive use
for *any* new functionality with EXPORT_SYMBOL_GPL(), like myself.
You seem to be taking a position of not allowing patches in that express the
freedom to use EXPORT_SYMBOL() because "related technology APIs had used
EXPORT_SYMBOL()". This is rather unfair for a few reasons:
0) This assumes that folks who wrote some old ioremap() calls had a position to
green light proprietary drivers to use them and embrace the idea that
EXPORT_SYMBOL_GPL() is the whitelist. As discussed in *many places* this
cannot be a reasonably general assumption as it does a *huge disservice* to many
people who always have held the position over their code always to not be
usable by proprietary drivers, and in even some cases that EXPORT_SYMBOL_GPL()
is pointless and does more harm than good, to the point they want to throw
tables at you for trying to add EXPORT_SYMBOL_GPL() code. Please consider
this *seriously*, not doing so is unfair to them.
1) Regardless of 0) its unfair to developers who do not want to deal with bug
reports for new features *at all*. Now this is important: one reason to take a
position to use EXPORT_SYMBOL_GPL() for new features even on *related
technology* APIs can be that we *cannot* change older APIs, *even* if consensus
is gathered that we don't want bug reports for certain functionality or
features. That is, even if you think 0) is dodgy there can be a general
consensus and position by maintainers to not want bug reports on certain area
in the kernel. Also using your argument one could always negate this freedom
since EXPORT_SYMBOL() is historically spread out thorugh the entire kernel, so
one could make the argument that "related technology" APIs exist all over the
kernel, thereby denying any use of EXPORT_SYMBOL_GPL() everywhere. This is
really unfair for new developers who start contributing who *do not want*
proprietary drivers to make use of their own new features.
2) Even if you consider 0 and 1) dodgy... we have positions expressed from developers
and maintainers on PAT interfaces with consensus that we don't want to deal with bug
reports for new PAT interfaces for proprietary drivers.
quoted
Also, FWIIW: I personally got essentially zero feedback and help from proprietary
binary kernel module vendors in the past couple of years as x86 maintainer,
despite a fair chunk of kernel crashes reported on distro kernels occuring in
them...
Based on that very negative experience, when we introduce something as complex and
as critical as new caching APIs, the last thing I want is to have obscure bugs in
binary modules I cannot fix in any reasonable fashion. So even if the parent APIs
of new APIs weren't already _GPL exports (as in this case), I'd export them as
_GPL in this case.
quoted
I think using EXPORT_SYMBOL_GPL to express individual political aims rather than
as a hint about what might be derived work makes us look like zealots, and
that's not my style.
As far as I'm concerned it's a pure technological choice: I don't want to export
certain types of hard to fix and critical functionality to drivers that I cannot
then fix.
That's a good argument that I hadn't heard before (or possibly it was there
and I missed it).
Actually, that's likely the most common reason for these positions... so yes,
you missed it, but I don't blame you. Another strong reason is the strong
legal value over EXPORT_SYMBOL_GPL(). So folks in old camp 0) above may feel
now the need to be explicit due to the legal value of EXPORT_SYMBOL_GPL().
So let me re-iterate: camp 0) folks may have taken a slightly different
position these days. Also some folks who were maybe on the fence over
"related technology" positions may simply be fed up with proprietary drivers
in general, and they have the freedom to do so and if EXPORT_SYMBOL_GPL()
is a good technical stop-gap that's their choice.
It would be stronger still if we could change the parent APIs similarly.
Sorry, that cannot happen. It is widely accepted that this was something we
would not do, in fact Linus has held a *strong* position that this would be
highly frowned upon. So think about this -- if you acknowledge that its
sensible for developers or maintainers to not want to deal with bug reports for
hard to fix functionality and we cannot change old APIs to EXPORT_SYMBOL_GPL()
then that in and of itself is a reason then for why some developers and
maintainers have taken the position to accept *new features* to go in with
EXPORT_SYMBOL_GPL(), as a compromise.
Those who do not want to deal with the implications of proprietary drivers only
have the option to make new features EXPORTS_SYMBOL_GPL(). Denying them this
means allowing a slew of crap reports for new features for all of us. It also
is denying anyone the sentiment or change of heart that perhaps enabling
proprietary drivers was a bad idea. Letting them use EXPORTS_SYMBOL_GPL()
enables them to express this sentiment.
If a proprietary driver can't use pci_ioremap_wc() because
it's exported _GPL, it's trivial to use ioremap_wc() directly.
That's under the assumption again that people who wrote ioremap_wc()
meant to enable such use. And again, the documentation today does
let folks add new *features* under EXPORT_SYMBOL_GPL() so if it makes
proprietary folks just do a bit more work, why not.
Luis