Re: [PATCH v2 2/4] input: keyboard: tegra: use devm_* for resource allocation

[Wolfram Sang <w.sang@pengutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 3757 bytes --]

Hi,

> > > > > I am sorry, but I do not consider a function that was added a little
> > > > > over a year ago as a canon. If you look at the uses of EADDRNOTAVAIL it
> > > > > is used predominantly in networking code to indicate that attempted
> > > > > _network_ address is not available.
> > > > 
> > > > EBUSY might be misleading, though. devm_request_and_ioremap() can fail
> > > > in both the request_mem_region() and ioremap() calls. Furthermore it'd
> > > > be good to settle on a consistent error-code instead of doing it
> > > > differently depending on subsystem and/or driver. Currently the various
> > > > error codes used are:
> > > > 
> > > > 	EBUSY, EADDRNOTAVAIL, ENXIO, ENOMEM, ENODEV, ENOENT, EINVAL,
> > > > 	EIO, EFAULT, EADDRINUSE
> > > > 
> > > > Also if we can settle on one error code we should follow up with a patch
> > > > to make it consistent across the tree and also update that kerneldoc
> > > > comment. I volunteer to do that if nobody else steps up. I'm also Cc'ing
> > > > Wolfram (the original author), maybe he has some thoughts on this.

Handling the error case was the biggest discussion back then. I
initially did not want to use ERR_PTR, because I see already enough
patches adding a forgotten ERR_PTR to drivers. My initial idea was to
return a simple errno and have the pointer a function argument. I was
convinced [1], however, that the dev_err printout is enough to make
visible what actually went wrong and return a NULL pointer instead. So
much for why the function does NOT return a PTR_ERR, and I still prefer
that.

Then, I added the example code in the documentation using EADDRNOTAVAIL.
Yes, I was brave with this one. Yet, EINVAL, EBUSY, ENOENT, did not
really cut it and are so heavily used in drivers that they turned into a
generic "something is wrong" error. I tried here to use a not overloaded
error code in order to be specific again. Since the patches were
accepted, I assumed it wasn't seen as a namespace violation. (Then
again, it probably would have been if that error code would go out to
userspace) Naturally, I didn't have the resources to check all patches
for a consistent error code.

> > > If you going to change all drivers make devm_request_and_ioremap()
> > > return ERR_PTR()-encoded errors and then we can differentiate what
> > > part of it failed.
> > 
> > Yeah, that thought also crossed my mind. I'll give other people some
> > time to comment before hurling myself into preparing patches.

As said above, that was argued away when committing the patches.

But there is more to that:

When working with this function, there was also the idea to abstract
getting the resource away. Which then gave Sascha Hauer and me the
question, if drivers really have to do this or if this couldn't be done
by the kernel somehow, i.e. giving the drivers already the resources
they need, completely prepared.

Of course, then we would need a similar function for interrupt
resources. Which has much bigger problem with return codes, since we
then step into the area of the "0 is no interrupt" topic (while
platform_get_irq returns an error code).

As a result, I got the impression that the whole topic needs ONE
concentrated, major rehaul or at least a master plan. Adding an idea
here and there doesn't seem to cut it, at least not in the way
devm_request_and_ioremap() was done. I would be interested in doing that
but my resources don't allow me to even think about it at the moment,
sadly.

Regards,

   Wolfram

[1] http://lkml.org/lkml/2011/10/24/278

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]