From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752719AbdGFCZe (ORCPT ); Wed, 5 Jul 2017 22:25:34 -0400 Received: from mail-yw0-f175.google.com ([209.85.161.175]:34702 "EHLO mail-yw0-f175.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752086AbdGFCZc (ORCPT ); Wed, 5 Jul 2017 22:25:32 -0400 MIME-Version: 1.0 In-Reply-To: References: <20170705071215.17603-1-tfiga@chromium.org> <20170705071215.17603-5-tfiga@chromium.org> From: Tomasz Figa Date: Thu, 6 Jul 2017 11:25:09 +0900 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [RFC PATCH 4/5] iommu/dma: Export non-static functions to use in modules To: Robin Murphy Cc: "open list:IOMMU DRIVERS" , "linux-kernel@vger.kernel.org" , Christoph Hellwig , Marek Szyprowski , Greg Kroah-Hartman , Joerg Roedel , Will Deacon , Vineet Gupta , Hans-Christian Noren Egtvedt , Mitchel Humpherys , Krzysztof Kozlowski , Arnd Bergmann Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Jul 6, 2017 at 1:22 AM, Robin Murphy wrote: > On 05/07/17 08:12, Tomasz Figa wrote: >> There is nothing wrong in having a loadable module implementing DMA API, >> for example to be used for sub-devices registered by the module. However, >> most of the functions from dma-iommu do not have their symbols exported, >> making it impossible to use them from loadable modules. >> >> Export all the non-static functions in the file, so that loadable modules >> can benefit from them. Use EXPORT_SYMBOL() for consistency with other >> exports in the file. > > To echo what Christoph said, everything not already exported here > shouldn't in any way be considered a driver-facing API in the general > sense, it's horrible glue code to sit behind an arch-specific DMA > mapping implementation (and frankly I'd consider even the current > exports more of an unfortunate abstraction leakage). Well, if I remember correctly, we agreed that the IPU3 driver would benefit from using all the iommu_dma_*() helpers in its DMA ops, similarly to ARM64. This is IMHO much better than re-implementing them again internally just for this driver. However almost none of necessary helpers are currently exported... > >> Signed-off-by: Tomasz Figa >> --- > > [...] > >> @@ -829,17 +838,20 @@ dma_addr_t iommu_dma_map_resource(struct device *dev, phys_addr_t phys, >> return __iommu_dma_map(dev, phys, size, >> dma_info_to_prot(dir, false, attrs) | IOMMU_MMIO); >> } >> +EXPORT_SYMBOL(iommu_dma_map_resource); >> >> void iommu_dma_unmap_resource(struct device *dev, dma_addr_t handle, >> size_t size, enum dma_data_direction dir, unsigned long attrs) >> { >> __iommu_dma_unmap(iommu_get_domain_for_dev(dev), handle, size); >> } >> +EXPORT_SYMBOL(iommu_dma_unmap_resource); > > Do you need these two? Unless your custom DMA ops really have to support > slave DMA or other peer-to-peer traffic through their IOMMU, I'd be more > inclined to implement dma_map_resource as "return 0;" and ignore > dma_unmap_resource. I don't need them. Getting an idea what is desirable to export and what not is actually one of the goals of this RFC. > >> @@ -913,3 +925,4 @@ void iommu_dma_map_msi_msg(int irq, struct msi_msg *msg) >> msg->address_lo += lower_32_bits(msi_page->iova); >> } >> } >> +EXPORT_SYMBOL(iommu_dma_map_msi_msg); > > Given the nature of the kind of irqchip drivers this exists for, the > chances of one ever being modular seem vanishingly small. Agreed. The IPU3 driver does not need it either. Let me list the (not yet exported) helpers it requires: dma-iommu.c - iommu_dma_init, - dma_info_to_prot, - iommu_dma_free, - iommu_dma_alloc, - iommu_dma_mmap, - iommu_dma_map_page, - iommu_dma_unmap_page, - iommu_dma_map_sg, - iommu_dma_unmap_sg, - iommu_dma_mapping_error, (added by my patch) iommu_dma_cleanup, iommu.c - iommu_group_get_for_dev, base/dma-mapping.c - dma_common_pages_remap, - dma_common_free_remap, (added by my patch) dma_common_get_mapped_pages (OR find_vm_area), Best regards, Tomasz From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tomasz Figa Subject: Re: [RFC PATCH 4/5] iommu/dma: Export non-static functions to use in modules Date: Thu, 6 Jul 2017 11:25:09 +0900 Message-ID: References: <20170705071215.17603-1-tfiga@chromium.org> <20170705071215.17603-5-tfiga@chromium.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: iommu-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org Errors-To: iommu-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org To: Robin Murphy Cc: Arnd Bergmann , Greg Kroah-Hartman , Will Deacon , "linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" , Krzysztof Kozlowski , "open list:IOMMU DRIVERS" , Vineet Gupta , Hans-Christian Noren Egtvedt , Christoph Hellwig List-Id: iommu@lists.linux-foundation.org On Thu, Jul 6, 2017 at 1:22 AM, Robin Murphy wrote: > On 05/07/17 08:12, Tomasz Figa wrote: >> There is nothing wrong in having a loadable module implementing DMA API, >> for example to be used for sub-devices registered by the module. However, >> most of the functions from dma-iommu do not have their symbols exported, >> making it impossible to use them from loadable modules. >> >> Export all the non-static functions in the file, so that loadable modules >> can benefit from them. Use EXPORT_SYMBOL() for consistency with other >> exports in the file. > > To echo what Christoph said, everything not already exported here > shouldn't in any way be considered a driver-facing API in the general > sense, it's horrible glue code to sit behind an arch-specific DMA > mapping implementation (and frankly I'd consider even the current > exports more of an unfortunate abstraction leakage). Well, if I remember correctly, we agreed that the IPU3 driver would benefit from using all the iommu_dma_*() helpers in its DMA ops, similarly to ARM64. This is IMHO much better than re-implementing them again internally just for this driver. However almost none of necessary helpers are currently exported... > >> Signed-off-by: Tomasz Figa >> --- > > [...] > >> @@ -829,17 +838,20 @@ dma_addr_t iommu_dma_map_resource(struct device *dev, phys_addr_t phys, >> return __iommu_dma_map(dev, phys, size, >> dma_info_to_prot(dir, false, attrs) | IOMMU_MMIO); >> } >> +EXPORT_SYMBOL(iommu_dma_map_resource); >> >> void iommu_dma_unmap_resource(struct device *dev, dma_addr_t handle, >> size_t size, enum dma_data_direction dir, unsigned long attrs) >> { >> __iommu_dma_unmap(iommu_get_domain_for_dev(dev), handle, size); >> } >> +EXPORT_SYMBOL(iommu_dma_unmap_resource); > > Do you need these two? Unless your custom DMA ops really have to support > slave DMA or other peer-to-peer traffic through their IOMMU, I'd be more > inclined to implement dma_map_resource as "return 0;" and ignore > dma_unmap_resource. I don't need them. Getting an idea what is desirable to export and what not is actually one of the goals of this RFC. > >> @@ -913,3 +925,4 @@ void iommu_dma_map_msi_msg(int irq, struct msi_msg *msg) >> msg->address_lo += lower_32_bits(msi_page->iova); >> } >> } >> +EXPORT_SYMBOL(iommu_dma_map_msi_msg); > > Given the nature of the kind of irqchip drivers this exists for, the > chances of one ever being modular seem vanishingly small. Agreed. The IPU3 driver does not need it either. Let me list the (not yet exported) helpers it requires: dma-iommu.c - iommu_dma_init, - dma_info_to_prot, - iommu_dma_free, - iommu_dma_alloc, - iommu_dma_mmap, - iommu_dma_map_page, - iommu_dma_unmap_page, - iommu_dma_map_sg, - iommu_dma_unmap_sg, - iommu_dma_mapping_error, (added by my patch) iommu_dma_cleanup, iommu.c - iommu_group_get_for_dev, base/dma-mapping.c - dma_common_pages_remap, - dma_common_free_remap, (added by my patch) dma_common_get_mapped_pages (OR find_vm_area), Best regards, Tomasz