[PATCH v5 1/5] pci: add pci_iomap_wc() variants

[PATCH v5 1/5] 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
Cc: linux-pci@vger.kernel.org
Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>
---

This v5 makes the code return NULL for IORESOURCE_IO and fixes the commit
log to clarify the conclusions reached for MTRR and our review of
IORESOURCE_PREFETCH.

 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 v5 1/5] pci: add pci_iomap_wc() variants

[Luis R. Rodriguez]

On Thu, Apr 30, 2015 at 10:36:04AM -0700, Luis R. Rodriguez wrote:
> From: "Luis R. Rodriguez" <mcgrof@suse.com>

<-- snip -->
> ---
> 
> This v5 makes the code return NULL for IORESOURCE_IO and fixes the commit
> log to clarify the conclusions reached for MTRR and our review of
> IORESOURCE_PREFETCH.
> 
>  include/asm-generic/pci_iomap.h | 14 ++++++++++
>  lib/pci_iomap.c                 | 61 +++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 75 insertions(+)

Hey Bjorn, any feedack on this series? Thanks.

  Luis

Re: [PATCH v5 1/5] pci: add pci_iomap_wc() variants

[Bjorn Helgaas]

On Thu, Apr 30, 2015 at 10:36:04AM -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
> 
> 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
> Cc: linux-pci@vger.kernel.org
> Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>

Acked-by: Bjorn Helgaas <bhelgaas@google.com>

> ---
> 
> This v5 makes the code return NULL for IORESOURCE_IO and fixes the commit
> log to clarify the conclusions reached for MTRR and our review of
> IORESOURCE_PREFETCH.
> 
>  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 v5 1/5] pci: add pci_iomap_wc() variants

[Luis R. Rodriguez]

On Tue, May 19, 2015 at 3:44 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> Acked-by: Bjorn Helgaas <bhelgaas@google.com>

Thanks! Who's tree should this go through?

  Luis

Re: [PATCH v5 1/5] pci: add pci_iomap_wc() variants

[Bjorn Helgaas]

[-cc Venkatesh (bouncing)

On Tue, May 19, 2015 at 5:46 PM, Luis R. Rodriguez
<mcgrof@do-not-panic.com> wrote:
> On Tue, May 19, 2015 at 3:44 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>> Acked-by: Bjorn Helgaas <bhelgaas@google.com>
>
> Thanks! Who's tree should this go through?

I don't know.  This is the only patch that went to linux-pci, so I
haven't seen the rest.

Bjorn

Re: [PATCH v5 1/5] pci: add pci_iomap_wc() variants

[Luis R. Rodriguez]

On Tue, May 19, 2015 at 4:02 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> [-cc Venkatesh (bouncing)
>
> On Tue, May 19, 2015 at 5:46 PM, Luis R. Rodriguez
> <mcgrof@do-not-panic.com> wrote:
>> On Tue, May 19, 2015 at 3:44 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>>> Acked-by: Bjorn Helgaas <bhelgaas@google.com>
>>
>> Thanks! Who's tree should this go through?
>
> I don't know.  This is the only patch that went to linux-pci, so I
> haven't seen the rest.

Oh I only rev'd a v5 for 1/5 as that's the only one that had feedback
asking for changes.

Patch v4 2/5 was for "lib: devres: add pcim_iomap_wc() variants", you
had questions about EXPORT_SYMBOL_GPL() and the fact that this is not
yet used. I replied. This patch can then be ignored but again, I'd
hate for folks to go in and try to add a non EXPORT_SYMBOL_GPL()
symbol of this.

Patches v4 3-5 remain intact, I had addressed it to you, but failed to
Cc linux-pci, I'll go ahead and bounce those now.

Just today Dave Arlie provided a Reviewed-by to some simple
framebuffer device driver changes. I wonder if these changes should go
through the framebuffer tree provided you already gave the Acked-by
for the PCI parts, or if the PCI parts should go in first and only
later (I guess we'd have to wait) then intake the driver changes that
use the symbol.

What we decide should likely also apply to the series that adds
pci_ioremap_wc_bar() and makes use of it on drivers.

Dave, Tomi, any preference?

  Luis

Re: [PATCH v5 1/5] pci: add pci_iomap_wc() variants

[David Airlie]

> On Tue, May 19, 2015 at 4:02 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> > [-cc Venkatesh (bouncing)
> >
> > On Tue, May 19, 2015 at 5:46 PM, Luis R. Rodriguez
> > <mcgrof@do-not-panic.com> wrote:
> >> On Tue, May 19, 2015 at 3:44 PM, Bjorn Helgaas <bhelgaas@google.com>
> >> wrote:
> >>> Acked-by: Bjorn Helgaas <bhelgaas@google.com>
> >>
> >> Thanks! Who's tree should this go through?
> >
> > I don't know.  This is the only patch that went to linux-pci, so I
> > haven't seen the rest.
> 
> Oh I only rev'd a v5 for 1/5 as that's the only one that had feedback
> asking for changes.
> 
> Patch v4 2/5 was for "lib: devres: add pcim_iomap_wc() variants", you
> had questions about EXPORT_SYMBOL_GPL() and the fact that this is not
> yet used. I replied. This patch can then be ignored but again, I'd
> hate for folks to go in and try to add a non EXPORT_SYMBOL_GPL()
> symbol of this.
> 
> Patches v4 3-5 remain intact, I had addressed it to you, but failed to
> Cc linux-pci, I'll go ahead and bounce those now.
> 
> Just today Dave Arlie provided a Reviewed-by to some simple
> framebuffer device driver changes. I wonder if these changes should go
> through the framebuffer tree provided you already gave the Acked-by
> for the PCI parts, or if the PCI parts should go in first and only
> later (I guess we'd have to wait) then intake the driver changes that
> use the symbol.
> 
> What we decide should likely also apply to the series that adds
> pci_ioremap_wc_bar() and makes use of it on drivers.
> 
> Dave, Tomi, any preference?
> 

Maybe send Bjorn a pull request with a tree with the pci changes, and the fb changes reviewed-by me and acked by Tomi.

Seems like it could be the simplest path forward.

Dave.

Re: [PATCH v5 1/5] pci: add pci_iomap_wc() variants

[Luis R. Rodriguez]

On Tue, May 19, 2015 at 4:29 PM, David Airlie <airlied@redhat.com> wrote:
>
>> On Tue, May 19, 2015 at 4:02 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>> > [-cc Venkatesh (bouncing)
>> >
>> > On Tue, May 19, 2015 at 5:46 PM, Luis R. Rodriguez
>> > <mcgrof@do-not-panic.com> wrote:
>> >> On Tue, May 19, 2015 at 3:44 PM, Bjorn Helgaas <bhelgaas@google.com>
>> >> wrote:
>> >>> Acked-by: Bjorn Helgaas <bhelgaas@google.com>
>> >>
>> >> Thanks! Who's tree should this go through?
>> >
>> > I don't know.  This is the only patch that went to linux-pci, so I
>> > haven't seen the rest.
>>
>> Oh I only rev'd a v5 for 1/5 as that's the only one that had feedback
>> asking for changes.
>>
>> Patch v4 2/5 was for "lib: devres: add pcim_iomap_wc() variants", you
>> had questions about EXPORT_SYMBOL_GPL() and the fact that this is not
>> yet used. I replied. This patch can then be ignored but again, I'd
>> hate for folks to go in and try to add a non EXPORT_SYMBOL_GPL()
>> symbol of this.
>>
>> Patches v4 3-5 remain intact, I had addressed it to you, but failed to
>> Cc linux-pci, I'll go ahead and bounce those now.
>>
>> Just today Dave Arlie provided a Reviewed-by to some simple
>> framebuffer device driver changes. I wonder if these changes should go
>> through the framebuffer tree provided you already gave the Acked-by
>> for the PCI parts, or if the PCI parts should go in first and only
>> later (I guess we'd have to wait) then intake the driver changes that
>> use the symbol.
>>
>> What we decide should likely also apply to the series that adds
>> pci_ioremap_wc_bar() and makes use of it on drivers.
>>
>> Dave, Tomi, any preference?
>>
>
> Maybe send Bjorn a pull request with a tree with the pci changes, and the fb changes reviewed-by me and acked by Tomi.
>
> Seems like it could be the simplest path forward.

Works with me, Bjorn, are you OK with that?

Dave, Tomi, I still need your Reviewed-by and Acked-by's Tomi on these
two PCI series. Please let me know if I should resend those first, but
I think I had Cc'd you folks already on them.

 Luis

Re: [PATCH v5 1/5] pci: add pci_iomap_wc() variants

[Tomi Valkeinen]

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



On 20/05/15 02:45, Luis R. Rodriguez wrote:
> On Tue, May 19, 2015 at 4:29 PM, David Airlie <airlied@redhat.com> wrote:
>>
>>> On Tue, May 19, 2015 at 4:02 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>>>> [-cc Venkatesh (bouncing)
>>>>
>>>> On Tue, May 19, 2015 at 5:46 PM, Luis R. Rodriguez
>>>> <mcgrof@do-not-panic.com> wrote:
>>>>> On Tue, May 19, 2015 at 3:44 PM, Bjorn Helgaas <bhelgaas@google.com>
>>>>> wrote:
>>>>>> Acked-by: Bjorn Helgaas <bhelgaas@google.com>
>>>>>
>>>>> Thanks! Who's tree should this go through?
>>>>
>>>> I don't know.  This is the only patch that went to linux-pci, so I
>>>> haven't seen the rest.
>>>
>>> Oh I only rev'd a v5 for 1/5 as that's the only one that had feedback
>>> asking for changes.
>>>
>>> Patch v4 2/5 was for "lib: devres: add pcim_iomap_wc() variants", you
>>> had questions about EXPORT_SYMBOL_GPL() and the fact that this is not
>>> yet used. I replied. This patch can then be ignored but again, I'd
>>> hate for folks to go in and try to add a non EXPORT_SYMBOL_GPL()
>>> symbol of this.
>>>
>>> Patches v4 3-5 remain intact, I had addressed it to you, but failed to
>>> Cc linux-pci, I'll go ahead and bounce those now.
>>>
>>> Just today Dave Arlie provided a Reviewed-by to some simple
>>> framebuffer device driver changes. I wonder if these changes should go
>>> through the framebuffer tree provided you already gave the Acked-by
>>> for the PCI parts, or if the PCI parts should go in first and only
>>> later (I guess we'd have to wait) then intake the driver changes that
>>> use the symbol.
>>>
>>> What we decide should likely also apply to the series that adds
>>> pci_ioremap_wc_bar() and makes use of it on drivers.
>>>
>>> Dave, Tomi, any preference?
>>>
>>
>> Maybe send Bjorn a pull request with a tree with the pci changes, and the fb changes reviewed-by me and acked by Tomi.
>>
>> Seems like it could be the simplest path forward.
> 
> Works with me, Bjorn, are you OK with that?
> 
> Dave, Tomi, I still need your Reviewed-by and Acked-by's Tomi on these
> two PCI series. Please let me know if I should resend those first, but
> I think I had Cc'd you folks already on them.

For the fb parts:

Acked-by: Tomi Valkeinen <tomi.valkeinen@ti.com>

I don't have anything in my fbdev-next branch touching the fbdev drivers
changed in this series, so merging via some other tree might go fine
without conflicts.

 Tomi


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

Re: [PATCH v5 1/5] pci: add pci_iomap_wc() variants

[Bjorn Helgaas]

On Tue, May 19, 2015 at 04:45:30PM -0700, Luis R. Rodriguez wrote:
> On Tue, May 19, 2015 at 4:29 PM, David Airlie <airlied@redhat.com> wrote:
> >
> >> On Tue, May 19, 2015 at 4:02 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> >> > [-cc Venkatesh (bouncing)
> >> >
> >> > On Tue, May 19, 2015 at 5:46 PM, Luis R. Rodriguez
> >> > <mcgrof@do-not-panic.com> wrote:
> >> >> On Tue, May 19, 2015 at 3:44 PM, Bjorn Helgaas <bhelgaas@google.com>
> >> >> wrote:
> >> >>> Acked-by: Bjorn Helgaas <bhelgaas@google.com>
> >> >>
> >> >> Thanks! Who's tree should this go through?
> >> >
> >> > I don't know.  This is the only patch that went to linux-pci, so I
> >> > haven't seen the rest.
> >>
> >> Oh I only rev'd a v5 for 1/5 as that's the only one that had feedback
> >> asking for changes.
> >>
> >> Patch v4 2/5 was for "lib: devres: add pcim_iomap_wc() variants", you
> >> had questions about EXPORT_SYMBOL_GPL() and the fact that this is not
> >> yet used. I replied. This patch can then be ignored but again, I'd
> >> hate for folks to go in and try to add a non EXPORT_SYMBOL_GPL()
> >> symbol of this.

I'm not really a fan of adding code before it's needed.

But even when we have a user and that objection is resolved, I'd
like to have a little more guidance on the difference between
EXPORT_SYMBOL() and EXPORT_SYMBOL_GPL(), e.g., a patch to clarify
what's in Documentation/DocBook/kernel-hacking.tmpl.  I can't
evaluate that issue based on "current trends and reality"; I need
something a little more formal.

If we want to allow non-GPL modules and we want to give them a
consistent kernel interface, even though it might be lacking some
implementation-specific things, I can follow that guideline.

That's how I read the current kernel-hacking.tmpl text
("[EXPORT_SYMBOL_GPL()] implies that the function is considered
an internal implementation issue, and not really an interface"),
and that would lead me to suggest EXPORT_SYMBOL() in this case.

If we don't care about consistency, and EXPORT_SYMBOL_GPL()
should be used at the developer's preference, I can follow that
guideline instead, but it would be easier if it were made more
explicit.

> >> Patches v4 3-5 remain intact, I had addressed it to you, but failed to
> >> Cc linux-pci, I'll go ahead and bounce those now.
> >>
> >> Just today Dave Arlie provided a Reviewed-by to some simple
> >> framebuffer device driver changes. I wonder if these changes should go
> >> through the framebuffer tree provided you already gave the Acked-by
> >> for the PCI parts, or if the PCI parts should go in first and only
> >> later (I guess we'd have to wait) then intake the driver changes that
> >> use the symbol.
> >>
> >> What we decide should likely also apply to the series that adds
> >> pci_ioremap_wc_bar() and makes use of it on drivers.
> >>
> >> Dave, Tomi, any preference?
> >>
> >
> > Maybe send Bjorn a pull request with a tree with the pci changes, and the fb changes reviewed-by me and acked by Tomi.
> >
> > Seems like it could be the simplest path forward.
> 
> Works with me, Bjorn, are you OK with that?

Can you incorporate the acks and just post a complete v6?  I don't like
trying to assemble patches from different versions of a series; it's too
much administrative hassle and too easy for me to screw up.

Bjorn

Re: [PATCH v5 1/5] pci: add pci_iomap_wc() variants

[Luis R. Rodriguez]

On Wed, May 20, 2015 at 1:39 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Tue, May 19, 2015 at 04:45:30PM -0700, Luis R. Rodriguez wrote:
>> On Tue, May 19, 2015 at 4:29 PM, David Airlie <airlied@redhat.com> wrote:
>> >
>> >> On Tue, May 19, 2015 at 4:02 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>> >> > [-cc Venkatesh (bouncing)
>> >> >
>> >> > On Tue, May 19, 2015 at 5:46 PM, Luis R. Rodriguez
>> >> > <mcgrof@do-not-panic.com> wrote:
>> >> >> On Tue, May 19, 2015 at 3:44 PM, Bjorn Helgaas <bhelgaas@google.com>
>> >> >> wrote:
>> >> >>> Acked-by: Bjorn Helgaas <bhelgaas@google.com>
>> >> >>
>> >> >> Thanks! Who's tree should this go through?
>> >> >
>> >> > I don't know.  This is the only patch that went to linux-pci, so I
>> >> > haven't seen the rest.
>> >>
>> >> Oh I only rev'd a v5 for 1/5 as that's the only one that had feedback
>> >> asking for changes.
>> >>
>> >> Patch v4 2/5 was for "lib: devres: add pcim_iomap_wc() variants", you
>> >> had questions about EXPORT_SYMBOL_GPL() and the fact that this is not
>> >> yet used. I replied. This patch can then be ignored but again, I'd
>> >> hate for folks to go in and try to add a non EXPORT_SYMBOL_GPL()
>> >> symbol of this.
>
> I'm not really a fan of adding code before it's needed.

OK I'll skip this patch.

> But even when we have a user and that objection is resolved, I'd
> like to have a little more guidance on the difference between
> EXPORT_SYMBOL() and EXPORT_SYMBOL_GPL(), e.g., a patch to clarify
> what's in Documentation/DocBook/kernel-hacking.tmpl.  I can't
> evaluate that issue based on "current trends and reality"; I need
> something a little more formal.
>
> If we want to allow non-GPL modules and we want to give them a
> consistent kernel interface, even though it might be lacking some
> implementation-specific things, I can follow that guideline.
>
> That's how I read the current kernel-hacking.tmpl text
> ("[EXPORT_SYMBOL_GPL()] implies that the function is considered
> an internal implementation issue, and not really an interface"),
> and that would lead me to suggest EXPORT_SYMBOL() in this case.
>
> If we don't care about consistency, and EXPORT_SYMBOL_GPL()
> should be used at the developer's preference, I can follow that
> guideline instead, but it would be easier if it were made more
> explicit.

I think that's fair for a maintainer to ask, provided that some
maintainers may not be aware of some history, I'll send a patch to
update the documentation to reflect reality. Note that this is not
just about allowing or not proprietary drivers, some folks also take
code from the kernel and use it or sell it on proprietary systems. The
rabbit hole is pretty big.

>> >> Patches v4 3-5 remain intact, I had addressed it to you, but failed to
>> >> Cc linux-pci, I'll go ahead and bounce those now.
>> >>
>> >> Just today Dave Arlie provided a Reviewed-by to some simple
>> >> framebuffer device driver changes. I wonder if these changes should go
>> >> through the framebuffer tree provided you already gave the Acked-by
>> >> for the PCI parts, or if the PCI parts should go in first and only
>> >> later (I guess we'd have to wait) then intake the driver changes that
>> >> use the symbol.
>> >>
>> >> What we decide should likely also apply to the series that adds
>> >> pci_ioremap_wc_bar() and makes use of it on drivers.
>> >>
>> >> Dave, Tomi, any preference?
>> >>
>> >
>> > Maybe send Bjorn a pull request with a tree with the pci changes, and the fb changes reviewed-by me and acked by Tomi.
>> >
>> > Seems like it could be the simplest path forward.
>>
>> Works with me, Bjorn, are you OK with that?
>
> Can you incorporate the acks and just post a complete v6?  I don't like
> trying to assemble patches from different versions of a series; it's too
> much administrative hassle and too easy for me to screw up.

Yeah sure, will do. While at it, can you also review the patch that
adds pci_ioremap_wc_bar() as well? I'll bundle that series up too.

 Luis