Hi Michael,
Thanks for the review.
On 09/21/2016 10:51 AM, Michael Ellerman wrote:
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
