Re: [PATCH v6 8/8] arm: dma-mapping: plumb our iommu mapping ops into arch_setup_dma_ops

[Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org>]

On Sun, Jan 18, 2015 at 11:18:51AM +0000, Laurent Pinchart wrote:
> On Sunday 18 January 2015 15:54:34 Alexandre Courbot wrote:
> > On 01/16/2015 08:18 AM, Laurent Pinchart wrote:
> > > On Thursday 15 January 2015 11:12:17 Will Deacon wrote:
> > >> On Thu, Jan 15, 2015 at 08:28:44AM +0000, Thierry Reding wrote:
> > >>> On Wed, Jan 14, 2015 at 10:46:10AM +0000, Will Deacon wrote:
> > >>>> On Wed, Jan 14, 2015 at 09:00:24AM +0000, Alexandre Courbot wrote:
> > >>> [...]
> > >>> 
> > >>>>> 2) Say you want to use the IOMMU API in your driver, and have an iommu
> > >>>>> property in your device's DT node. If by chance your IOMMU is
> > >>>>> registered early, you will already have a mapping automatically
> > >>>>> created even before your probe function is called. Can this be
> > >>>>> avoided? Is it even safe?
> > >>>> 
> > >>>> Currently, I think you have to either teardown the ops manually or
> > >>>> return an error from of_xlate. Thierry was also looking at this sort of
> > >>>> thing, so it might be worth talking to him.
> > >>> 
> > >>> I already explained in earlier threads why I think this is a bad idea.
> > >>> It's completely unnatural for any driver to manually tear down something
> > >>> that it didn't want set up in the first place. It also means that you
> > >>> have to carefully audit any users of these IOMMU APIs to make sure that
> > >>> they do tear down. That doesn't sound like a good incremental approach,
> > >>> as evidenced by the breakage that Alex and Heiko have encountered.
> > >> 
> > >> Well, perhaps we hide that behind a get_iommu API or something. We *do*
> > >> need this manual teardown step to support things like VFIO, so it makes
> > >> sense to reuse it for other users too imo.
> > >> 
> > >>> The solution for me has been to completely side-step the issue and not
> > >>> register the IOMMU with the new mechanism at all. That is, there's no
> > >>> .of_xlate() implementation, which means that the ARM DMA API glue won't
> > >>> try to be smart and use the IOMMU in ways it's not meant to be used.
> > > 
> > > That will break when someone will want to use the same IOMMU type for
> > > devices that use the DMA mapping API to hide the IOMMU. That might not be
> > > the case for your IOMMU today, but it's pretty fragile, we need to fix
> > > it.
> > > 
> > >>> This has several advantages, such as that I can also use the regular
> > >>> driver model for suspend/resume of the IOMMU, and I get to enjoy the
> > >>> benefits of devres in the IOMMU driver. Probe ordering is still a tiny
> > >>> issue, but we can easily solve that using explicit initcall ordering
> > >>> (which really isn't any worse than IOMMU_OF_DECLARE()).
> > >> 
> > >> That's a pity. I'd much rather extend what we currently have to satisfy
> > >> your use-case. Ho-hum.
> > > 
> > > Assuming we want the IOMMU to be handled transparently for the majority of
> > > devices I only see two ways to fix this,
> > > 
> > > The first way is to create a default DMA mapping unconditionally and let
> > > drivers that can't live with it tear it down. That's what is implemented
> > > today.
> > 
> > I strongly support Thierry's point that drivers should not have to tear
> > down things they don't need. The issue we are facing today is a very
> > good illustration of why one should not have to do this.
> > 
> > Everybody hates to receive unsollicited email with a link that says "to
> > unsubscribe, click here". Let's not import that unpleasant culture into
> > the kernel.
> > 
> > I am arriving late in this discussion, but what is wrong with asking
> > drivers to explicitly state that they want the DMA API to be backed by
> > the IOMMU instead of forcibly making it work that way?
> 
> The vast majority of the drivers are not IOMMU-aware. We would thus need to 
> add a call at the beginning of the probe function of nearly every driver that 
> can perform DMA to state that the driver doesn't need to handle any IOMMU that 
> might be present in the system itself. I don't think that's a better solution.
> 
> Explicitly tearing down mappings in drivers that want to manage IOMMUs isn't a 
> solution I like either. A possibly better solution would be to call a function 
> to state that the DMA mapping API shouldn't not handle IOMMUs. Something like
> 
> 	dma_mapping_ignore_iommu(dev);
> 
> at the beginning of the probe function of such drivers could do. The function 
> would perform behind the scene all operations needed to tear down everything 
> that shouldn't have been set up.

An alternative would be to add a flag to platform_driver, like we have for
"prevent_deferred_probe" which is something like "prevent_dma_configure".

For the moment, that would actually teardown the DMA configuration in
platform_drv_probe, but if things are reordering in future then we can avoid
setting up the ops altogether without an API change.

Will