Re: [PATCH v1 05/47] pci: add pci_iomap_wc() variants

Hi Luis,

This seems OK to me, but I'm curious about a few things.

On Fri, Mar 20, 2015 at 6:17 PM, Luis R. Rodriguez
[off-list ref] wrote:

From: "Luis R. Rodriguez" <redacted>

This allows drivers to take advantage of write-combining
when possible. Ideally we'd have pci_read_bases() just
peg an IORESOURCE_WC flag for us

We do set IORESOURCE_PREFETCH.  Do you mean something different?

 but where exactly
video devices memory lie varies *largely* and at times things
are mixed with MMIO registers, sometimes we can address
the changes in drivers, other times the change requires
intrusive changes.

What does a video device address have to do with this?  I do see that
if a BAR maps only a frame buffer, the device might be able to mark it
prefetchable, while if the BAR mapped both a frame buffer and some
registers, it might not be able to make it prefetchable.  But that
doesn't seem like it depends on the *address*.

pci_iomap_range() already makes a cacheable mapping if
IORESOURCE_CACHEABLE; I'm guessing that you would like it to
automatically use WC if the BAR if IORESOURCE_PREFETCH, e.g.,

  if (flags & IORESOURCE_CACHEABLE)
    return ioremap(start, len);
  if (flags & IORESOURCE_PREFETCH)
    return ioremap_wc(start, len);
  return ioremap_nocache(start, len);

Is there a reason not to do that?

Although there is also arch_phys_wc_add() that makes use of
architecture specific write-combinging alternatives (MTRR on
x86 when a system does not have PAT) we void polluting
pci_iomap() space with it and force drivers and subsystems
that want to use it to be explicit.

There are a few motivations for this:

a) Take advantage of PAT when available

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

c) Help with the goal of eventually using _PAGE_CACHE_UC over
   _PAGE_CACHE_UC_MINUS on x86 on ioremap_nocache() (de33c442e)
...

+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 __pci_ioport_map(dev, start, len);
+       if (flags & IORESOURCE_MEM)

Should we log a note in dmesg if the BAR is *not* IORESOURCE_PREFETCH?
 I know the driver might know it's safe even if the device didn't mark
the BAR as prefetchable, but it does seem like an easy way for a
driver to shoot itself in the foot.

+               return ioremap_wc(start, len);
+       /* What? */
+       return NULL;
+}