Re: [Xen-devel] [PATCH v6 1/4] pci: add pci_iomap_wc() variants
On Tue, Jun 16, 2015 at 3:20 PM, Bjorn Helgaas bhelg...@google.com wrote: On Tue, Jun 16, 2015 at 2:16 PM, Luis R. Rodriguez mcg...@do-not-panic.com wrote: On Thu, May 28, 2015 at 5:36 PM, Luis R. Rodriguez mcg...@do-not-panic.com wrote: On Wed, May 27, 2015 at 1:04 PM, Luis R. Rodriguez mcg...@suse.com wrote: On Tue, May 26, 2015 at 12:40:08PM -0500, Bjorn Helgaas wrote: On Fri, May 22, 2015 at 02:23:41AM +0200, Luis R. Rodriguez wrote: On Thu, May 21, 2015 at 05:33:21PM -0500, Bjorn Helgaas wrote: I tentatively put this (and the rest of the series) on a pci/resource branch. I'm hoping you'll propose some clarification about EXPORT_SYMBOL_GPL(). EXPORT_SYMBOL_GPL() also serves to ensure only GPL modules can only run that code. So for instance although we have Dual BSD/GPL tags for modules pure BSD tags do not exist for module tags and cannot run EXPORT_SYMBOL_GPL() code [0]. Also there is some folks who do believe tha at run time all kernel modules are GPL [1] [2]. And to be precise even though the FSF may claim a list of licenses are GPL-compatible we cannot rely on this list alone for our own goals and if folks want to use our EXPORT_SYMBOL_GPL()s they must discuss this on lkml [2]. By propose some clarification, I meant that I hoped you would propose a patch to Documentation/ that would give maintainers some guidance. I *really really* would hate to do so but only because you insist, I'll look into this... OK done. Bjorn, This is now on Jonathan Corbet's tree and visible on linux-next: https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/?id=582ed8d51e2b6cb8a168c94852bca482685c2509 Sorry, I'm just not comfortable with using EXPORT_SYMBOL_GPL() this way. I'm happy to use it when it has a technical justification, e.g., for internal interfaces where users of the interface are clearly derived works. I believe it is completely fair for some maintainers to take the position of what the documentation used to say prior to the new documentation patch on queue for v4.2, but note that that is an old position and I seriously caution against it. A few reasons I caution against it: 1) It used to be believed that EXPORT_SYMBOL_GPL() was pretty pointless by many maintainers and developers -- in particular for all those who have always written even EXPORT_SYMBOL() code with the same intent as EXPORT_SYMBOL_GPL(). Experience and comments with attorneys even on Linus' part reflects that there is a huge legal value to EXPORT_SYMBOL_GPL() [0]. The skinny: Intent matters a lot and circumventing a GPL-only export requires an explicit action, making it clear that the resulting copyright infringement was a deliberate act. Naturally lax positions on the matter over use of EXPORT_SYMBOL_GPL() have evolved over time then, not only about its value but also then as a consequence about where its used in practice today by maintainers and developers in different trees. [0] https://lwn.net/Articles/154602/ 2) The old position of use of EXPORT_SYMBOL_GPL() about the derivatives work punts onto the maintainer the onus over the question of derivatives work -- such question really should not be taken lightly and this responsibility should really not fall onto the developer, it requires attorney involvement and should not be taken lightly by any means. 3) Most drivers are upstream these days, we want to avoid bug reports from crappy proprietary drivers, specially as we add new features. We don't have to be begging vendors to work upstream these days, that's rather the norm. The landscape has changed dramatically. Even Linus has told Nvidia to go fuck themselves lately, bravo. But pci_iomap_wc() is not in that category, and I think it should be symmetric with similar interfaces like pci_iomap() and ioremap_wc(). Those are two old APIs, and quite the contrary, as I noted we have a series of new PAT APIs that old modules did not use that are now being added and spread into the kernel onto which upstream maintainers have been insisting on using EXPORT_SYMBOL_GPL() even though older similar APIs only used EXPORT_SYMBOL(). As a recent example the family of APIs set_pages_array_xx() are all EXPORT_SYMBOL() but Toshi's new set_pages_array_wt() was asked to be changed to EXPORT_SYMBOL_GPL() because as noted by Ingo: -- By default we make new APIs EXPORT_SYMBOL_GPL(): we don't want proprietary modules mucking around with new code PAT interfaces, we only want modules we can analyze and fix in detail. -- http://article.gmane.org/gmane.linux.kernel.mm/129104 I don't want to use EXPORT_SYMBOL_GPL() for a random collection of things depending on the whim of the author. Its not just me as I note above, and the new APIs I'm introducing are also to help with PAT usage. That makes for a messy environment to work in, and it's messy enough already. You're right about the mismatch over the kernel and even on a set of family of APIs,
Re: [Xen-devel] [PATCH v6 1/4] pci: add pci_iomap_wc() variants
On Fri, Jun 19, 2015 at 4:06 PM, Luis R. Rodriguez mcg...@do-not-panic.com wrote: I hope to have provided a bit of new information to help you reconsider this series to go through you but since you seem to be fine for this to go through another tree and since I failed to notice that I should also get Arnd's Ack I am in hopes this might be able to go through Arnd's tree if not through you. Please let me know, in case I have to resubmit to Arnd. If you want to have Arnd merge it, that's fine with me. Bjorn ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v6 1/4] pci: add pci_iomap_wc() variants
On Tue, Jun 16, 2015 at 2:16 PM, Luis R. Rodriguez mcg...@do-not-panic.com wrote: On Thu, May 28, 2015 at 5:36 PM, Luis R. Rodriguez mcg...@do-not-panic.com wrote: On Wed, May 27, 2015 at 1:04 PM, Luis R. Rodriguez mcg...@suse.com wrote: On Tue, May 26, 2015 at 12:40:08PM -0500, Bjorn Helgaas wrote: On Fri, May 22, 2015 at 02:23:41AM +0200, Luis R. Rodriguez wrote: On Thu, May 21, 2015 at 05:33:21PM -0500, Bjorn Helgaas wrote: I tentatively put this (and the rest of the series) on a pci/resource branch. I'm hoping you'll propose some clarification about EXPORT_SYMBOL_GPL(). EXPORT_SYMBOL_GPL() also serves to ensure only GPL modules can only run that code. So for instance although we have Dual BSD/GPL tags for modules pure BSD tags do not exist for module tags and cannot run EXPORT_SYMBOL_GPL() code [0]. Also there is some folks who do believe tha at run time all kernel modules are GPL [1] [2]. And to be precise even though the FSF may claim a list of licenses are GPL-compatible we cannot rely on this list alone for our own goals and if folks want to use our EXPORT_SYMBOL_GPL()s they must discuss this on lkml [2]. By propose some clarification, I meant that I hoped you would propose a patch to Documentation/ that would give maintainers some guidance. I *really really* would hate to do so but only because you insist, I'll look into this... OK done. Bjorn, This is now on Jonathan Corbet's tree and visible on linux-next: https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/?id=582ed8d51e2b6cb8a168c94852bca482685c2509 Sorry, I'm just not comfortable with using EXPORT_SYMBOL_GPL() this way. I'm happy to use it when it has a technical justification, e.g., for internal interfaces where users of the interface are clearly derived works. But pci_iomap_wc() is not in that category, and I think it should be symmetric with similar interfaces like pci_iomap() and ioremap_wc(). I don't want to use EXPORT_SYMBOL_GPL() for a random collection of things depending on the whim of the author. That makes for a messy environment to work in, and it's messy enough already. If we wanted to remove the EXPORT_SYMBOL/EXPORT_SYMBOL_GPL distinction completely, that'd be fine with me, too. But as long as we keep it, I think it should mean something more than the preference of the author. I know I did already ack this, and I even said I would merge it, but a month of thinking about this hasn't made me more comfortable with it, so I've changed my mind. I said before that I wouldn't try to stop you if you want to merge it some other way, but I don't want to ack it, and I don't want to merge it via my tree. Bjorn ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v6 1/4] pci: add pci_iomap_wc() variants
On Thu, May 28, 2015 at 5:36 PM, Luis R. Rodriguez mcg...@do-not-panic.com wrote: On Wed, May 27, 2015 at 1:04 PM, Luis R. Rodriguez mcg...@suse.com wrote: On Tue, May 26, 2015 at 12:40:08PM -0500, Bjorn Helgaas wrote: On Fri, May 22, 2015 at 02:23:41AM +0200, Luis R. Rodriguez wrote: On Thu, May 21, 2015 at 05:33:21PM -0500, Bjorn Helgaas wrote: I tentatively put this (and the rest of the series) on a pci/resource branch. I'm hoping you'll propose some clarification about EXPORT_SYMBOL_GPL(). EXPORT_SYMBOL_GPL() also serves to ensure only GPL modules can only run that code. So for instance although we have Dual BSD/GPL tags for modules pure BSD tags do not exist for module tags and cannot run EXPORT_SYMBOL_GPL() code [0]. Also there is some folks who do believe tha at run time all kernel modules are GPL [1] [2]. And to be precise even though the FSF may claim a list of licenses are GPL-compatible we cannot rely on this list alone for our own goals and if folks want to use our EXPORT_SYMBOL_GPL()s they must discuss this on lkml [2]. By propose some clarification, I meant that I hoped you would propose a patch to Documentation/ that would give maintainers some guidance. I *really really* would hate to do so but only because you insist, I'll look into this... OK done. Bjorn, This is now on Jonathan Corbet's tree and visible on linux-next: https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/?id=582ed8d51e2b6cb8a168c94852bca482685c2509 Please let me know if there is anything else I can do to help. Please let me know. Also as per review with Tomi, the framebuffer maintainer, he would prefer for only the required symbols to go through your tree. We'd then wait for the next merge window for them to perculate to Linus' tree and once there I'd send him a pull request for the framebuffer device driver changes alone. So this does mean we'll have no users of the symbols for a full release, but again, this is as per Tomi's preference. This strategy is also the preference then for the pci_iomap_wc() series as well. With that in mind, perhaps the lib patch can go in as we'd have no users but we do have a few future possible expected users. I repoked Tomi about this topic with a new context provided, my expressed hope was to just merge the fbvdev dependent changes for both series (now both Acked by Tomi) through your tree. Luis ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v6 1/4] pci: add pci_iomap_wc() variants
On Thu, May 28, 2015 at 10:57 PM, Tomi Valkeinen tomi.valkei...@ti.com wrote: On 29/05/15 03:36, Luis R. Rodriguez wrote: On Wed, May 27, 2015 at 1:04 PM, Luis R. Rodriguez mcg...@suse.com wrote: On Tue, May 26, 2015 at 12:40:08PM -0500, Bjorn Helgaas wrote: On Fri, May 22, 2015 at 02:23:41AM +0200, Luis R. Rodriguez wrote: On Thu, May 21, 2015 at 05:33:21PM -0500, Bjorn Helgaas wrote: I tentatively put this (and the rest of the series) on a pci/resource branch. I'm hoping you'll propose some clarification about EXPORT_SYMBOL_GPL(). EXPORT_SYMBOL_GPL() also serves to ensure only GPL modules can only run that code. So for instance although we have Dual BSD/GPL tags for modules pure BSD tags do not exist for module tags and cannot run EXPORT_SYMBOL_GPL() code [0]. Also there is some folks who do believe tha at run time all kernel modules are GPL [1] [2]. And to be precise even though the FSF may claim a list of licenses are GPL-compatible we cannot rely on this list alone for our own goals and if folks want to use our EXPORT_SYMBOL_GPL()s they must discuss this on lkml [2]. By propose some clarification, I meant that I hoped you would propose a patch to Documentation/ that would give maintainers some guidance. I *really really* would hate to do so but only because you insist, I'll look into this... OK done. Please let me know if there is anything else I can do to help. Also as per review with Tomi, the framebuffer maintainer, he would prefer for only the required symbols to go through your tree. We'd then wait for the next merge window for them to perculate to Linus' tree and once there I'd send him a pull request for the framebuffer device driver changes alone. So this does mean we'll have no users of the symbols for a full release, but again, this is as per Tomi's preference. This strategy is also the preference then for the pci_iomap_wc() series as well. With that in mind, perhaps the lib patch can go in as we'd have no users but we do have a few future possible expected users. I don't have any issue with fbdev changes going through other trees, but I'd rather do that only if there are good reasons to go that way. OK, either way I prefer to go with maintainer's preferences. Most changes are for framebuffer drivers as that's where MTRR is used most these days. These changes to fbdev drivers look like cleanups, so they are not critical, right? I'll let you make the call, I'll just provide information to you. I trust your judgement and what you prefer. This and other series which change use of MTRR to arch_phys enable use of PAT when available, we want to bury out MTRR from further usage so all these arch_phys changes will help with that. MTRR, although supported, should be seen as a first step temporary architectural evolution to what PAT became. There are known architectural issue with MTRR, let me list the issues for you to review and consider and evaluate: * MTRR acts on physical addresses and requires power-of-two alignment, on both the base used and size, this limits the flexibility of MTRR use * MTRR is known to be unreliable, it can at times not work even on modern systems * MTRRs are limited, if using a large number of devices MTRRs will run out fast, its why Andy ended up adding the arch_phys APIs * PAT has been available for quite a long time, since Pentium III (circa 1999) and newer, but having PAT enabled does not restrict use of MTRR and because of this some systems may end up then combining MTRR and PAT. I do not believe this wasn't an original highly expected wide use situation, it technically should work to combine both but there might be issues with interactions between both, exactly what issues can exist or have existed remains quite unclear as MTRR in and of itself has been known to be unreliable anyway. If possible its best to just be binary about this and only use MTRR if and only if necessary because of the other issues known with MTRR. With all these changes being merged the only use case for MTRR then would be through the arch_phys APIs, which would just enable use of MTRR when PAT is not available or a system / driver is known to require MTRR -- those systems are expected to low numbered, in the end after all these series Linux will will end up with only two device drivers which will require MTRR to be enabled always: * ipath: this device driver is old, powers the old HTX bus cards that only work in AMD systems, while the newer IB/qib device driver powers all PCI-e cards. The ipath device driver is obsolete, hardware hard to find. In fact the maintainers of this driver have recently even seriously discussed removing the driver from upstream altogether. * ivtv: the hardware is really rare these days, its expected only some lost souls in some third world country are expected to be using the feature which requires MTRR. The way this driver uses MTRR is also quite questionable, but still has been present in the driver for
Re: [Xen-devel] [PATCH v6 1/4] pci: add pci_iomap_wc() variants
On Wed, May 27, 2015 at 1:04 PM, Luis R. Rodriguez mcg...@suse.com wrote: On Tue, May 26, 2015 at 12:40:08PM -0500, Bjorn Helgaas wrote: On Fri, May 22, 2015 at 02:23:41AM +0200, Luis R. Rodriguez wrote: On Thu, May 21, 2015 at 05:33:21PM -0500, Bjorn Helgaas wrote: I tentatively put this (and the rest of the series) on a pci/resource branch. I'm hoping you'll propose some clarification about EXPORT_SYMBOL_GPL(). EXPORT_SYMBOL_GPL() also serves to ensure only GPL modules can only run that code. So for instance although we have Dual BSD/GPL tags for modules pure BSD tags do not exist for module tags and cannot run EXPORT_SYMBOL_GPL() code [0]. Also there is some folks who do believe tha at run time all kernel modules are GPL [1] [2]. And to be precise even though the FSF may claim a list of licenses are GPL-compatible we cannot rely on this list alone for our own goals and if folks want to use our EXPORT_SYMBOL_GPL()s they must discuss this on lkml [2]. By propose some clarification, I meant that I hoped you would propose a patch to Documentation/ that would give maintainers some guidance. I *really really* would hate to do so but only because you insist, I'll look into this... OK done. Please let me know if there is anything else I can do to help. Also as per review with Tomi, the framebuffer maintainer, he would prefer for only the required symbols to go through your tree. We'd then wait for the next merge window for them to perculate to Linus' tree and once there I'd send him a pull request for the framebuffer device driver changes alone. So this does mean we'll have no users of the symbols for a full release, but again, this is as per Tomi's preference. This strategy is also the preference then for the pci_iomap_wc() series as well. With that in mind, perhaps the lib patch can go in as we'd have no users but we do have a few future possible expected users. Luis ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v6 1/4] pci: add pci_iomap_wc() variants
On 29/05/15 03:36, Luis R. Rodriguez wrote: On Wed, May 27, 2015 at 1:04 PM, Luis R. Rodriguez mcg...@suse.com wrote: On Tue, May 26, 2015 at 12:40:08PM -0500, Bjorn Helgaas wrote: On Fri, May 22, 2015 at 02:23:41AM +0200, Luis R. Rodriguez wrote: On Thu, May 21, 2015 at 05:33:21PM -0500, Bjorn Helgaas wrote: I tentatively put this (and the rest of the series) on a pci/resource branch. I'm hoping you'll propose some clarification about EXPORT_SYMBOL_GPL(). EXPORT_SYMBOL_GPL() also serves to ensure only GPL modules can only run that code. So for instance although we have Dual BSD/GPL tags for modules pure BSD tags do not exist for module tags and cannot run EXPORT_SYMBOL_GPL() code [0]. Also there is some folks who do believe tha at run time all kernel modules are GPL [1] [2]. And to be precise even though the FSF may claim a list of licenses are GPL-compatible we cannot rely on this list alone for our own goals and if folks want to use our EXPORT_SYMBOL_GPL()s they must discuss this on lkml [2]. By propose some clarification, I meant that I hoped you would propose a patch to Documentation/ that would give maintainers some guidance. I *really really* would hate to do so but only because you insist, I'll look into this... OK done. Please let me know if there is anything else I can do to help. Also as per review with Tomi, the framebuffer maintainer, he would prefer for only the required symbols to go through your tree. We'd then wait for the next merge window for them to perculate to Linus' tree and once there I'd send him a pull request for the framebuffer device driver changes alone. So this does mean we'll have no users of the symbols for a full release, but again, this is as per Tomi's preference. This strategy is also the preference then for the pci_iomap_wc() series as well. With that in mind, perhaps the lib patch can go in as we'd have no users but we do have a few future possible expected users. I don't have any issue with fbdev changes going through other trees, but I'd rather do that only if there are good reasons to go that way. These changes to fbdev drivers look like cleanups, so they are not critical, right? Does delaying the fbdev changes until the dependencies are in prevent some other development? Tomi signature.asc Description: OpenPGP digital signature ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v6 1/4] pci: add pci_iomap_wc() variants
On Tue, May 26, 2015 at 12:40:08PM -0500, Bjorn Helgaas wrote: On Fri, May 22, 2015 at 02:23:41AM +0200, Luis R. Rodriguez wrote: On Thu, May 21, 2015 at 05:33:21PM -0500, Bjorn Helgaas wrote: I tentatively put this (and the rest of the series) on a pci/resource branch. I'm hoping you'll propose some clarification about EXPORT_SYMBOL_GPL(). EXPORT_SYMBOL_GPL() also serves to ensure only GPL modules can only run that code. So for instance although we have Dual BSD/GPL tags for modules pure BSD tags do not exist for module tags and cannot run EXPORT_SYMBOL_GPL() code [0]. Also there is some folks who do believe tha at run time all kernel modules are GPL [1] [2]. And to be precise even though the FSF may claim a list of licenses are GPL-compatible we cannot rely on this list alone for our own goals and if folks want to use our EXPORT_SYMBOL_GPL()s they must discuss this on lkml [2]. By propose some clarification, I meant that I hoped you would propose a patch to Documentation/ that would give maintainers some guidance. I *really really* would hate to do so but only because you insist, I'll look into this... ASDF Luis ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v6 1/4] pci: add pci_iomap_wc() variants
On Fri, May 22, 2015 at 02:23:41AM +0200, Luis R. Rodriguez wrote: On Thu, May 21, 2015 at 05:33:21PM -0500, Bjorn Helgaas wrote: I tentatively put this (and the rest of the series) on a pci/resource branch. I'm hoping you'll propose some clarification about EXPORT_SYMBOL_GPL(). EXPORT_SYMBOL_GPL() also serves to ensure only GPL modules can only run that code. So for instance although we have Dual BSD/GPL tags for modules pure BSD tags do not exist for module tags and cannot run EXPORT_SYMBOL_GPL() code [0]. Also there is some folks who do believe tha at run time all kernel modules are GPL [1] [2]. And to be precise even though the FSF may claim a list of licenses are GPL-compatible we cannot rely on this list alone for our own goals and if folks want to use our EXPORT_SYMBOL_GPL()s they must discuss this on lkml [2]. By propose some clarification, I meant that I hoped you would propose a patch to Documentation/ that would give maintainers some guidance. ... ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v6 1/4] pci: add pci_iomap_wc() variants
On Wed, May 20, 2015 at 04:08:10PM -0700, Luis R. Rodriguez wrote: ... --- 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); Huh. So you let me talk about marking the unused pcim_iomap_wc() EXPORT_SYMBOL_GPL(), but didn't remind me that you also proposed to mark the symbol you really care about, the one you already have a use for, as EXPORT_SYMBOL_GPL(). Sigh. In my opinion, if we're going to use EXPORT_SYMBOL_GPL() at all, we should use it consistently and based on technical considerations. I base this on statements like the following: - [EXPORT_SYMBOL_GPL()] implies that the function is considered an internal implementation issue, and not really an interface. [Rusty Russell, 1] - ... using the xxx_GPL() version to show that it's an internal interface ... [Linus Torvalds, 2] - Anything exported via EXPORT_SYMBOL_GPL() is considered by the author to be so fundamental to the kernel that using it would be impossible without creating a derivative work. [Matthew Garrett, 3] - Linus's initial point for [_GPL symbols] has been so diluted by random lobby groups asking for every symbol to be _GPL that they are becoming effectively pointless now. [Dave Airlie, 4] Existing interfaces like these are exported with EXPORT_SYMBOL(): ioremap() ioremap_wc() ioremap_prot() pci_iomap() pci_map_rom() I would argue that pci_iomap_wc() is similar in spirit and is no more an internal implementation issue than they are, and should be exported similarly. So my *advice* is to use EXPORT_SYMBOL() in this case, because that's a choice you can defend on technical grounds. I think it's hard to argue that pci_iomap_wc() is so fundamental or unique to Linux that a caller would automatically be a derivative work. Will I still merge it as EXPORT_SYMBOL_GPL()? Maybe. I don't feel *good* about it because the only explanation I can give is the author wanted it that way, and that's unsatisfying. But I did already ack it (before I noticed the _GPL() issue), and I won't try to retract that and prevent somebody else from merging it. And maybe your proposal to clarify the kernel-hacking.tmpl language will convince me. Bjorn [1]
Re: [Xen-devel] [PATCH v6 1/4] pci: add pci_iomap_wc() variants
On Wed, May 20, 2015 at 04:08:10PM -0700, Luis R. Rodriguez wrote: From: Luis R. Rodriguez mcg...@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 I tentatively put this (and the rest of the series) on a pci/resource branch. I'm hoping you'll propose some clarification about EXPORT_SYMBOL_GPL(). In the meantime, I tried to make the changelog a bit more concise, as below. Let me know if I omitted something essential. We still have URLs for the discussion for the fine points. commit 75387ae87b7aebc2a5b447f4d1b8b17a23c15b08 Author: Luis R. Rodriguez mcg...@suse.com Date: Wed May 20 16:08:10 2015 -0700 PCI: Add pci_iomap_wc() variants 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()) [bhelgaas: changelog] Link: http://lkml.kernel.org/r/1426893517-2511-6-git-send-email-mcg...@do-not-panic.com Original-posting: http://lkml.kernel.org/r/1432163293-20965-1-git-send-email-mcg...@do-not-panic.com Signed-off-by: Luis R. Rodriguez mcg...@suse.com Signed-off-by: Bjorn Helgaas bhelg...@google.com ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v6 1/4] pci: add pci_iomap_wc() variants
On Thu, May 21, 2015 at 05:33:21PM -0500, Bjorn Helgaas wrote: I tentatively put this (and the rest of the series) on a pci/resource branch. I'm hoping you'll propose some clarification about EXPORT_SYMBOL_GPL(). EXPORT_SYMBOL_GPL() also serves to ensure only GPL modules can only run that code. So for instance although we have Dual BSD/GPL tags for modules pure BSD tags do not exist for module tags and cannot run EXPORT_SYMBOL_GPL() code [0]. Also there is some folks who do believe tha at run time all kernel modules are GPL [1] [2]. And to be precise even though the FSF may claim a list of licenses are GPL-compatible we cannot rely on this list alone for our own goals and if folks want to use our EXPORT_SYMBOL_GPL()s they must discuss this on lkml [2]. In the past when I've tried to try to clarify EXPORT_SYMBOL_GPL() requirements, implications, its been said that its best to leave some things as-is [3] and let attorneys figure things out. In so far as to what exactly it is and can be used for requires legal attorney review, but the question of derivative work certainly comes up [4]. Now folks companies seem to want to obviously use and abuse our symbols despite of all the things above, for instance Red Hat once tried to change an EXPORT_SYMBOL_GPL() to EXPORT_SYMBOL() [5]. Obviously that didn't go so well, and some folks went off on a good rant about this [6]. What developers do and accept varies, I'm not going into pointing out specifics and I do not wnat to do homework for folks who wish to abuse things further, but by no means should a developer be nack'd entry to new code if their functionality is not replacing old one [9]. In this case this is new functionality. Also in terms of preference: nobody has said that symbols exported with plain EXPORT_SYMBOL() can be freely used by proprietary code; indeed, a number of developers claim that all (or nearly all) loadable modules are derived products of the kernel regardless of whether they use GPL-only symbols or not [7]. And spot on: In general, the kernel community has long worked to maintain a vague and scary ambiguity around the legal status of proprietary modules while being unwilling to attempt to ban such modules outright. [7]. Now, a few maintainers insist on tons of new symbols to be EXPORT_SYMBOL_GPL() though proactively [8] [9] and the reasons vary, I just happen to also write my code to be perfectly clear with my goals and intent and you are the first to ask me to reconsider this, even if you do make me use EXPORT_SYMBOL() my intent and goal does not change, as with others. No code I ever write should be used by proprietary shit, and I hope to convince others of the importance to do this as well. In the meantime, I tried to make the changelog a bit more concise, as below. Let me know if I omitted something essential. We still have URLs for the discussion for the fine points. Looks good. [0] https://lkml.org/lkml/2012/4/7/102 [1] https://lkml.org/lkml/2012/4/8/71 [2] https://lkml.org/lkml/2012/4/8/71 [3] https://lkml.org/lkml/2009/6/1/385 [4] https://lkml.org/lkml/2009/6/1/376 [5] https://lkml.org/lkml/2012/4/20/328 [6] https://plus.google.com/+AlanCoxLinux/posts/D2feRNc6R4d [7] https://lwn.net/Articles/603131/ [8] https://lwn.net/Articles/603139/ [9] https://lkml.org/lkml/2015/2/26/379 Luis ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel