On Wed, Apr 26, 2017 at 12:11:33PM -0600, Logan Gunthorpe wrote:
Ok, well for starters I think you are mistaken about kmap being able to fail. I'm having a hard time finding many users of that function that bother to check for an error when calling it.
A quick audit of the arch code shows you're right - kmap can't fail anywhere anymore.
The main difficulty we have now is that neither of those functions are expected to fail and we need them to be able to in cases where the page doesn't map to system RAM. This patch series is trying to address it for users of scatterlist. I'm certainly open to other suggestions.
I think you'll need to follow the existing kmap semantics and never fail the iomem version either. Otherwise you'll have a special case that's almost never used that has a different error path.
There are a fair number of cases in the kernel that do something like:
if (something) x = kmap(page); else x = kmap_atomic(page); ... if (something) kunmap(page) else kunmap_atomic(x)
Which just seems cumbersome to me.
Passing a different flag based on something isn't really much better.
In any case, if you can accept an sg_kmap and sg_kmap_atomic api just say so and I'll make the change. But I'll still need a flags variable for SG_MAP_MUST_NOT_FAIL to support legacy cases that have no fail path and both of those functions will need to be pretty nearly replicas of each other.
Again, wrong way. Suddenly making things fail for your special case that normally don't fail is a receipe for bugs.