Hi Michael,

Thanks for the review.

On 09/21/2016 10:51 AM, Michael Ellerman wrote:

quoted

That is important/requirement for the distribution kernels - where
the DMA_ATTR_NO_WARN changes to 'enum dma_attr' are not acceptable
because it breaks the kernel ABI.

[snip]
 > Removing an entry from an enum would break the API (Note _P_). But I 
don't see
 > how adding one does.

Agree.. I should have worded 'already-built out-of-tree modules'.

If I understand it correctly, this chunk will change the value of
DMA_ATTR_MAX, and thus break the ABI for out-of-tree modules that
depend on it. (Not that I know of any that use it, but that's the
reason that causes distro kernel builds to fail w/ this applied.)

@@ -19,6 +19,7 @@ enum dma_attr {
  	DMA_ATTR_SKIP_CPU_SYNC,
  	DMA_ATTR_FORCE_CONTIGUOUS,
  	DMA_ATTR_ALLOC_SINGLE_PAGES,
+	DMA_ATTR_NO_WARN,
  	DMA_ATTR_MAX,
  };

Also kernel command line parameters are really poor in terms of usability. Folks
really don't want to have to change their kernel command line.

If we really need a hack for distros then I'd suggest we add a global struct
driver * in the powerpc iommu code, and then when nvme loads it sets that to
point at itself, and then the check becomes:

Agree w/ cmdline args are poor for usability, and that this new hack is
certainly smarter -- however, it does not provide any option/choice for
the user of whether enable/disable it (ie driver automatically sets it).

Although that isn't a problem for older downstream nvme drivers, which
have a single OK-to-fail dma mapping callsite (so the message doesn't
matter at all), the newer downstream drivers also have a Not-OK-to-fail
callsite, which is still worth to get the 'iommu_alloc failed' message
(per Ben's point of 'message useful for debugging').

For the latter, I think an upstream patch like this suggestion is not
a generally acceptable approach.

So, the intent is to have a single/common hack that upstream is OK w/,
then apply that downstream in the multiple distros.  Of course, if you
are not OK w/ this patch (which is obviously fair) we'll have to try it
downstream only, and there we can diverge in the implementations.

Please let me know your thoughts on it.

Thanks!

-- 
Mauricio Faria de Oliveira
IBM Linux Technology Center