diff mbox

[v4,2/5] lib: devres: add pcim_iomap_wc() variants

Message ID 1430343372-687-3-git-send-email-mcgrof@do-not-panic.com (mailing list archive)
State New, archived
Headers show

Commit Message

Luis R. Rodriguez April 29, 2015, 9:36 p.m. UTC 
From: "Luis R. Rodriguez" <mcgrof@suse.com>

Now that we have pci_iomap_wc() add the respective devres helpers.

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
Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>
---
 include/linux/pci.h |  2 ++
 lib/devres.c        | 78 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 80 insertions(+)

Comments

Bjorn Helgaas April 30, 2015, 4:26 p.m. UTC | #1 
[+cc linux-pci]

Hi Luis,

On Wed, Apr 29, 2015 at 02:36:09PM -0700, Luis R. Rodriguez wrote:
> From: "Luis R. Rodriguez" <mcgrof@suse.com>
> 
> Now that we have pci_iomap_wc() add the respective devres helpers.

I guess I'm still confused about the relationship between pci_iomap_wc()
and arch_phys_wc_add().

Do you expect every caller of pcim_iomap_wc() to also call
arch_phys_wc_add()?

If so, I'm not sure how pcim_iomap_wc() fits into the picture.  A driver
can call both pcim_iomap_wc() and arch_phys_wc_add(), but the driver
doesn't explicitly do the unmap, so where would the arch_phys_wc_del()
happen?

If not, how does a driver know whether it should call arch_phys_wc_add()?

> ...
>  /**
> + * pcim_iomap_wc_regions - Request and iomap PCI BARs with write-combining
> + * @pdev: PCI device to map IO resources for
> + * @mask: Mask of BARs to request and iomap
> + * @name: Name used when requesting regions
> + *
> + * Request and iomap regions specified by @mask with a preference for
> + * write-combining.
> + */
> +int pcim_iomap_wc_regions(struct pci_dev *pdev, int mask, const char *name)
> +{
> +	void __iomem * const *iomap;
> +	int i, rc;
> +
> +	iomap = pcim_iomap_table(pdev);
> +	if (!iomap)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {
> +		unsigned long len;
> +
> +		if (!(mask & (1 << i)))
> +			continue;
> +
> +		rc = -EINVAL;
> +		len = pci_resource_len(pdev, i);
> +		if (!len)
> +			goto err_inval;
> +
> +		rc = pci_request_region(pdev, i, name);
> +		if (rc)
> +			goto err_inval;
> +
> +		rc = -ENOMEM;
> +		if (!pcim_iomap_wc(pdev, i, 0))
> +			goto err_region;

Is there a user for this?  Are there really devices where *all* the BARs
can be mapped with WC?  Are there enough of them to make it worth adding
this?

I don't see users of either pcim_iomap_wc() or pcim_iomap_wc_regions() so
far.  Did I miss them, or do you just expect them in the near future?

Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Luis Chamberlain April 30, 2015, 5:27 p.m. UTC | #2 
On Thu, Apr 30, 2015 at 11:26:47AM -0500, Bjorn Helgaas wrote:
> [+cc linux-pci]
> 
> Hi Luis,
> 
> On Wed, Apr 29, 2015 at 02:36:09PM -0700, Luis R. Rodriguez wrote:
> > From: "Luis R. Rodriguez" <mcgrof@suse.com>
> > 
> > Now that we have pci_iomap_wc() add the respective devres helpers.
> 
> I guess I'm still confused about the relationship between pci_iomap_wc()
> and arch_phys_wc_add().
> 
> Do you expect every caller of pcim_iomap_wc() to also call
> arch_phys_wc_add()?

Yeap.

> If so, I'm not sure how pcim_iomap_wc() fits into the picture.  A driver
> can call both pcim_iomap_wc() and arch_phys_wc_add(), but the driver
> doesn't explicitly do the unmap, so where would the arch_phys_wc_del()
> happen?

As with other current drivers not using devres, upon exit or where they
would otherwise typically iounmap().

> If not, how does a driver know whether it should call arch_phys_wc_add()?

Sadly they'd have to figure it out, as Andy notes arch_phys_wc_add() is
a hack so I think we need to leave it as such and hope to see arch_phys_wc_add()
use phased as it won't be needed anymore really. arch_phys_wc_add() really should
only be used by device drivers that know that are working with non-PAT systems.
The code already takes care of this but since its an x86 write-combining hack
we should not consider meshing it with devres.

> > ...
> >  /**
> > + * pcim_iomap_wc_regions - Request and iomap PCI BARs with write-combining
> > + * @pdev: PCI device to map IO resources for
> > + * @mask: Mask of BARs to request and iomap
> > + * @name: Name used when requesting regions
> > + *
> > + * Request and iomap regions specified by @mask with a preference for
> > + * write-combining.
> > + */
> > +int pcim_iomap_wc_regions(struct pci_dev *pdev, int mask, const char *name)
> > +{
> > +	void __iomem * const *iomap;
> > +	int i, rc;
> > +
> > +	iomap = pcim_iomap_table(pdev);
> > +	if (!iomap)
> > +		return -ENOMEM;
> > +
> > +	for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {
> > +		unsigned long len;
> > +
> > +		if (!(mask & (1 << i)))
> > +			continue;
> > +
> > +		rc = -EINVAL;
> > +		len = pci_resource_len(pdev, i);
> > +		if (!len)
> > +			goto err_inval;
> > +
> > +		rc = pci_request_region(pdev, i, name);
> > +		if (rc)
> > +			goto err_inval;
> > +
> > +		rc = -ENOMEM;
> > +		if (!pcim_iomap_wc(pdev, i, 0))
> > +			goto err_region;
> 
> Is there a user for this?  Are there really devices where *all* the BARs
> can be mapped with WC?  Are there enough of them to make it worth adding
> this?

Not right now, I did this more to help with a friend who is testing one
driver for a feature. The driver is upstream but a way to make the feature
take effect only under certain conditions still would need to be done.

> I don't see users of either pcim_iomap_wc() or pcim_iomap_wc_regions() so
> far.  Did I miss them, or do you just expect them in the near future?

The later, and also I hate seeing folks later add code under EXPORT_SYMBOL()
rather than EXPORT_SYMBOL_GPL() so I figure I'd rather do it first. It happened
recently in my v1 series, someone beat me to a write-combining export symbol
and changed it to EXPORT_SYMBOL(). Feel free to drop this though but I hope
no one out there then tries to just add an EXPORT_SYMBOL() later for this...

 Luis
--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bjorn Helgaas April 30, 2015, 9:46 p.m. UTC | #3 
On Thu, Apr 30, 2015 at 07:27:23PM +0200, Luis R. Rodriguez wrote:
> On Thu, Apr 30, 2015 at 11:26:47AM -0500, Bjorn Helgaas wrote:

> > I don't see users of either pcim_iomap_wc() or pcim_iomap_wc_regions() so
> > far.  Did I miss them, or do you just expect them in the near future?
> 
> The later, and also I hate seeing folks later add code under EXPORT_SYMBOL()
> rather than EXPORT_SYMBOL_GPL() so I figure I'd rather do it first. It happened
> recently in my v1 series, someone beat me to a write-combining export symbol
> and changed it to EXPORT_SYMBOL(). Feel free to drop this though but I hope
> no one out there then tries to just add an EXPORT_SYMBOL() later for this...

Why do you want them to be EXPORT_SYMBOL_GPL?  I would expect them to be
exported the same way pcim_iomap(), pcim_iomap_regions(), and ioremap_wc()
are exported, i.e., with EXPORT_SYMBOL.

Per Documentation/DocBook/kernel-hacking.tmpl, EXPORT_SYMBOL_GPL "implies
that the function is considered an internal implementation issue, and not
really an interface."  I don't think these are internal implementation
issues.

Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Luis Chamberlain May 1, 2015, 12:20 a.m. UTC | #4 
On Thu, Apr 30, 2015 at 04:46:38PM -0500, Bjorn Helgaas wrote:
> On Thu, Apr 30, 2015 at 07:27:23PM +0200, Luis R. Rodriguez wrote:
> > On Thu, Apr 30, 2015 at 11:26:47AM -0500, Bjorn Helgaas wrote:
> 
> > > I don't see users of either pcim_iomap_wc() or pcim_iomap_wc_regions() so
> > > far.  Did I miss them, or do you just expect them in the near future?
> > 
> > The later, and also I hate seeing folks later add code under EXPORT_SYMBOL()
> > rather than EXPORT_SYMBOL_GPL() so I figure I'd rather do it first. It happened
> > recently in my v1 series, someone beat me to a write-combining export symbol
> > and changed it to EXPORT_SYMBOL(). Feel free to drop this though but I hope
> > no one out there then tries to just add an EXPORT_SYMBOL() later for this...
> 
> Why do you want them to be EXPORT_SYMBOL_GPL?  I would expect them to be
> exported the same way pcim_iomap(), pcim_iomap_regions(), and ioremap_wc()
> are exported, i.e., with EXPORT_SYMBOL.
>> 
> Per Documentation/DocBook/kernel-hacking.tmpl, EXPORT_SYMBOL_GPL "implies
> that the function is considered an internal implementation issue, and not
> really an interface."  I don't think these are internal implementation
> issues.

What Documentation/DocBook/kernel-hacking.tmpl states over EXPORT_SYMBOL_GPL()
is old and in no way reflects current trends and reality. For instance, some
folks believe that if some code has EXPORT_SYMBOL() declared that they can use
it on proprietary modules. This is terribly incorrect, quite a few developers
do not in any way stand by this as a "needed" clarification on their code [0].
I'm one of them, but to be even more clear on this I simply *always* use
EXPORT_SYMBOL_GPL() to remove any possible doubt over this on any symbols
that I export. Heck, even tons of driver library code uses EXPORT_SYMBOL_GPL().

[0] https://lkml.org/lkml/2012/4/20/402

  Luis
--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/include/linux/pci.h b/include/linux/pci.h
index 490ca41..cb317c4 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1619,9 +1619,11 @@  static inline void pci_dev_specific_enable_acs(struct pci_dev *dev) { }
 #endif
 
 void __iomem *pcim_iomap(struct pci_dev *pdev, int bar, unsigned long maxlen);
+void __iomem *pcim_iomap_wc(struct pci_dev *pdev, int bar, unsigned long maxlen);
 void pcim_iounmap(struct pci_dev *pdev, void __iomem *addr);
 void __iomem * const *pcim_iomap_table(struct pci_dev *pdev);
 int pcim_iomap_regions(struct pci_dev *pdev, int mask, const char *name);
+int pcim_iomap_wc_regions(struct pci_dev *pdev, int mask, const char *name);
 int pcim_iomap_regions_request_all(struct pci_dev *pdev, int mask,
 				   const char *name);
 void pcim_iounmap_regions(struct pci_dev *pdev, int mask);
diff --git a/lib/devres.c b/lib/devres.c
index fbe2aac..d59a2b9 100644
--- a/lib/devres.c
+++ b/lib/devres.c
@@ -304,6 +304,30 @@  void __iomem *pcim_iomap(struct pci_dev *pdev, int bar, unsigned long maxlen)
 EXPORT_SYMBOL(pcim_iomap);
 
 /**
+ * pcim_iomap_wc - Managed pcim_iomap_wc()
+ * @pdev: PCI device to iomap for
+ * @bar: BAR to iomap
+ * @maxlen: Maximum length of iomap
+ *
+ * Managed pci_iomap_wc().  Map is automatically unmapped on driver
+ * detach.
+ */
+void __iomem *pcim_iomap_wc(struct pci_dev *pdev, int bar, unsigned long maxlen)
+{
+	void __iomem **tbl;
+
+	BUG_ON(bar >= PCIM_IOMAP_MAX);
+
+	tbl = (void __iomem **)pcim_iomap_table(pdev);
+	if (!tbl || tbl[bar])	/* duplicate mappings not allowed */
+		return NULL;
+
+	tbl[bar] = pci_iomap_wc(pdev, bar, maxlen);
+	return tbl[bar];
+}
+EXPORT_SYMBOL_GPL(pcim_iomap_wc);
+
+/**
  * pcim_iounmap - Managed pci_iounmap()
  * @pdev: PCI device to iounmap for
  * @addr: Address to unmap
@@ -383,6 +407,60 @@  int pcim_iomap_regions(struct pci_dev *pdev, int mask, const char *name)
 EXPORT_SYMBOL(pcim_iomap_regions);
 
 /**
+ * pcim_iomap_wc_regions - Request and iomap PCI BARs with write-combining
+ * @pdev: PCI device to map IO resources for
+ * @mask: Mask of BARs to request and iomap
+ * @name: Name used when requesting regions
+ *
+ * Request and iomap regions specified by @mask with a preference for
+ * write-combining.
+ */
+int pcim_iomap_wc_regions(struct pci_dev *pdev, int mask, const char *name)
+{
+	void __iomem * const *iomap;
+	int i, rc;
+
+	iomap = pcim_iomap_table(pdev);
+	if (!iomap)
+		return -ENOMEM;
+
+	for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {
+		unsigned long len;
+
+		if (!(mask & (1 << i)))
+			continue;
+
+		rc = -EINVAL;
+		len = pci_resource_len(pdev, i);
+		if (!len)
+			goto err_inval;
+
+		rc = pci_request_region(pdev, i, name);
+		if (rc)
+			goto err_inval;
+
+		rc = -ENOMEM;
+		if (!pcim_iomap_wc(pdev, i, 0))
+			goto err_region;
+	}
+
+	return 0;
+
+ err_region:
+	pci_release_region(pdev, i);
+ err_inval:
+	while (--i >= 0) {
+		if (!(mask & (1 << i)))
+			continue;
+		pcim_iounmap(pdev, iomap[i]);
+		pci_release_region(pdev, i);
+	}
+
+	return rc;
+}
+EXPORT_SYMBOL_GPL(pcim_iomap_wc_regions);
+
+/**
  * pcim_iomap_regions_request_all - Request all BARs and iomap specified ones
  * @pdev: PCI device to map IO resources for
  * @mask: Mask of BARs to iomap