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.