From: "Luis R. Rodriguez" <[email protected]>
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 <[email protected]>
Cc: Andy Lutomirski <[email protected]>
Cc: Suresh Siddha <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Juergen Gross <[email protected]>
Cc: Daniel Vetter <[email protected]>
Cc: Dave Airlie <[email protected]>
Cc: Bjorn Helgaas <[email protected]>
Cc: Antonino Daplas <[email protected]>
Cc: Jean-Christophe Plagniol-Villard <[email protected]>
Cc: Tomi Valkeinen <[email protected]>
Cc: Dave Hansen <[email protected]>
Cc: Arnd Bergmann <[email protected]>
Cc: Michael S. Tsirkin <[email protected]>
Cc: [email protected]
Cc: Stefan Bader <[email protected]>
Cc: Ville Syrjälä <[email protected]>
Cc: Mel Gorman <[email protected]>
Cc: Vlastimil Babka <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: Davidlohr Bueso <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: Roger Pau Monné <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Luis R. Rodriguez <[email protected]>
---
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
On Thu, Apr 30, 2015 at 10:36:04AM -0700, Luis R. Rodriguez wrote:
> From: "Luis R. Rodriguez" <[email protected]>
<-- 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
On Thu, Apr 30, 2015 at 10:36:04AM -0700, Luis R. Rodriguez wrote:
> From: "Luis R. Rodriguez" <[email protected]>
>
> 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 <[email protected]>
> Cc: Andy Lutomirski <[email protected]>
> Cc: Suresh Siddha <[email protected]>
> Cc: Ingo Molnar <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Cc: Juergen Gross <[email protected]>
> Cc: Daniel Vetter <[email protected]>
> Cc: Dave Airlie <[email protected]>
> Cc: Bjorn Helgaas <[email protected]>
> Cc: Antonino Daplas <[email protected]>
> Cc: Jean-Christophe Plagniol-Villard <[email protected]>
> Cc: Tomi Valkeinen <[email protected]>
> Cc: Dave Hansen <[email protected]>
> Cc: Arnd Bergmann <[email protected]>
> Cc: Michael S. Tsirkin <[email protected]>
> Cc: [email protected]
> Cc: Stefan Bader <[email protected]>
> Cc: Ville Syrj?l? <[email protected]>
> Cc: Mel Gorman <[email protected]>
> Cc: Vlastimil Babka <[email protected]>
> Cc: Borislav Petkov <[email protected]>
> Cc: Davidlohr Bueso <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: Roger Pau Monn? <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Signed-off-by: Luis R. Rodriguez <[email protected]>
Acked-by: Bjorn Helgaas <[email protected]>
> ---
>
> 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
>
On Tue, May 19, 2015 at 3:44 PM, Bjorn Helgaas <[email protected]> wrote:
> Acked-by: Bjorn Helgaas <[email protected]>
Thanks! Who's tree should this go through?
Luis
[-cc Venkatesh (bouncing)
On Tue, May 19, 2015 at 5:46 PM, Luis R. Rodriguez
<[email protected]> wrote:
> On Tue, May 19, 2015 at 3:44 PM, Bjorn Helgaas <[email protected]> wrote:
>> Acked-by: Bjorn Helgaas <[email protected]>
>
> 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
On Tue, May 19, 2015 at 4:02 PM, Bjorn Helgaas <[email protected]> wrote:
> [-cc Venkatesh (bouncing)
>
> On Tue, May 19, 2015 at 5:46 PM, Luis R. Rodriguez
> <[email protected]> wrote:
>> On Tue, May 19, 2015 at 3:44 PM, Bjorn Helgaas <[email protected]> wrote:
>>> Acked-by: Bjorn Helgaas <[email protected]>
>>
>> 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
> On Tue, May 19, 2015 at 4:02 PM, Bjorn Helgaas <[email protected]> wrote:
> > [-cc Venkatesh (bouncing)
> >
> > On Tue, May 19, 2015 at 5:46 PM, Luis R. Rodriguez
> > <[email protected]> wrote:
> >> On Tue, May 19, 2015 at 3:44 PM, Bjorn Helgaas <[email protected]>
> >> wrote:
> >>> Acked-by: Bjorn Helgaas <[email protected]>
> >>
> >> 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.
On Tue, May 19, 2015 at 4:29 PM, David Airlie <[email protected]> wrote:
>
>> On Tue, May 19, 2015 at 4:02 PM, Bjorn Helgaas <[email protected]> wrote:
>> > [-cc Venkatesh (bouncing)
>> >
>> > On Tue, May 19, 2015 at 5:46 PM, Luis R. Rodriguez
>> > <[email protected]> wrote:
>> >> On Tue, May 19, 2015 at 3:44 PM, Bjorn Helgaas <[email protected]>
>> >> wrote:
>> >>> Acked-by: Bjorn Helgaas <[email protected]>
>> >>
>> >> 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
On 20/05/15 02:45, Luis R. Rodriguez wrote:
> On Tue, May 19, 2015 at 4:29 PM, David Airlie <[email protected]> wrote:
>>
>>> On Tue, May 19, 2015 at 4:02 PM, Bjorn Helgaas <[email protected]> wrote:
>>>> [-cc Venkatesh (bouncing)
>>>>
>>>> On Tue, May 19, 2015 at 5:46 PM, Luis R. Rodriguez
>>>> <[email protected]> wrote:
>>>>> On Tue, May 19, 2015 at 3:44 PM, Bjorn Helgaas <[email protected]>
>>>>> wrote:
>>>>>> Acked-by: Bjorn Helgaas <[email protected]>
>>>>>
>>>>> 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 <[email protected]>
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
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 <[email protected]> wrote:
> >
> >> On Tue, May 19, 2015 at 4:02 PM, Bjorn Helgaas <[email protected]> wrote:
> >> > [-cc Venkatesh (bouncing)
> >> >
> >> > On Tue, May 19, 2015 at 5:46 PM, Luis R. Rodriguez
> >> > <[email protected]> wrote:
> >> >> On Tue, May 19, 2015 at 3:44 PM, Bjorn Helgaas <[email protected]>
> >> >> wrote:
> >> >>> Acked-by: Bjorn Helgaas <[email protected]>
> >> >>
> >> >> 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
On Wed, May 20, 2015 at 1:39 PM, Bjorn Helgaas <[email protected]> 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 <[email protected]> wrote:
>> >
>> >> On Tue, May 19, 2015 at 4:02 PM, Bjorn Helgaas <[email protected]> wrote:
>> >> > [-cc Venkatesh (bouncing)
>> >> >
>> >> > On Tue, May 19, 2015 at 5:46 PM, Luis R. Rodriguez
>> >> > <[email protected]> wrote:
>> >> >> On Tue, May 19, 2015 at 3:44 PM, Bjorn Helgaas <[email protected]>
>> >> >> wrote:
>> >> >>> Acked-by: Bjorn Helgaas <[email protected]>
>> >> >>
>> >> >> 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