On Fri, Nov 13, 2020 at 08:03:43AM +0100, Uwe Kleine-König wrote:
> Hello,
> 
> [Added lkml and the people involved in commit 7945f929f1a7
> ("drivers: provide devm_platform_ioremap_resource()") to Cc:. For the
> new readers: This is about patches making use of
> devm_platform_ioremap_resource() instead of open coding it. Full context
> at https://lore.kernel.org/r/20201112190649.GA908613@ulmo]
> 
> On Thu, Nov 12, 2020 at 10:14:29PM +0100, Uwe Kleine-König wrote:
> > On Thu, Nov 12, 2020 at 08:06:49PM +0100, Thierry Reding wrote:
> > > I also think that it's overly narrow is scope, so you can't actually
> > > "blindly" use this helper and I've seen quite a few cases where this was
> > > unknowingly used for cases where it shouldn't have been used and then
> > > broke things (because some drivers must not do the request_mem_region()
> > > for example).
> > 
> > You have a link to such an accident?
> 
> I got a hint in private here: https://lore.kernel.org/r/1555670144-24220-1-git-send-email-aisheng.dong@nxp.com
> 
> devm_platform_ioremap_resource() is platform_get_resource() +
> devm_ioremap_resource() and here it was used to replace
> platform_get_resource() + devm_ioremap().
> 
> IMHO the unlucky thing in this situation is that devm_ioremap_resource()
> and devm_ioremap() are different by more than just how they get the area
> to remap. (i.e. devm_ioremap_resource() also does
> devm_request_mem_region().)
> 
> So the problem is not the added wrapper, but unclear semantics in the
> functions it uses.

The semantics aren't unclear. It's just that the symbol name doesn't
spell out every detail that the function implements, which, frankly, no
function name ever does, at least not for anything beyond simple
instructional examples. That's what we have documentation for and why
people should read the documentation before they use a function and make
(potentially wrong) assumption about what it does.

>                    In my eyes devm_ioremap() and
> devm_platform_ioremap_resource() should better be named
> devm_request_ioremap() and devm_platform_request_ioremap_resource()
> respectively. Is it worth to rename these for clearity?

I think function names are always a compromise between giving you the
gist of what the implementation does and being short enough so it
doesn't become difficult to read or use.

One of the reasons why I dislike the addition of helpers for every
common special case (like devm_platform_ioremap_resource()) is because
it doesn't (always) actually make things easier for developers and/or
maintainers. Replacing three lines of code with one is a minor
improvement, even though there may be many callsites and therefore in
the sum this being a fairly sizeable reduction. The flip side is that
now we've got an extra symbol with an unwieldy name that people need
to become familiar with, and then, like the link above shows, it doesn't
work in all cases, so you either need to fall back to the open-coded
version or you keep adding helpers until you've covered all cases. And
then we end up with a bunch of helpers that you actually have to go and
read the documentation for in order to find out which one exactly fits
your use-case.

Without the helpers it's pretty simple to write, even if a little
repetitive:

  1) get the resource you want to map
  2) request the resource
  3) map the resource

2) & 3) are very commonly done together, so it makes sense to have a
generic helper for them. If you look at the implementation, the
devm_ioremap_request() implementation does quite a bit of things in
addition to just requesting and remapping, and that's the reason why
that helper makes sense.

For me personally, devm_platform_ioremap_resource() is just not adding
enough value to justify its existence. And then we get all these other
variants that operate on the resource name (_byname) and those which
remap write-combined (_wc). But don't we also need a _byname_wc()
variant for the combination? Where does it stop?

Thierry