On 15.03.19 15:26, Greg KH wrote:

On Fri, Mar 15, 2019 at 10:06:16AM +0100, Enrico Weigelt, metux IT consult wrote:

quoted

On 14.03.19 23:52, Greg KH wrote:

quoted

On Thu, Mar 14, 2019 at 11:33:40PM +0100, Enrico Weigelt, metux IT consult wrote:

quoted

Use the safer devm versions of memory mapping functions.

What is "safer" about them?

Garbage collection :)

At times, but not when you do not use the api correctly :)

Okay, my fault that I didn't read the code careful enough :o

But still, I think the name is a bit misleading as it *sounds* as just
a wrapper around devm_ioremap() that just picks the params from a
struct resource. I guess we can't change the name easily ?

Yes, there are lots of drivers for devices that are never unloaded or
removed from the system.  The fact that no one has reported any problems
with them means that they are never used in situations like this.

So, never touch a running system ?

No, you need to have a good reason why it needs to be changed, when you
can not verify that this actually resolves a problem.  As this patch
shows, you just replaced one api call with another, so nothing changed
at all, except you actually took up more memory and logic to do the same
thing :(

Okay, I was on a wrong track here - I had the silly idea that it would
make things easier if we'd do it the same way everywhere.

quoted

IMHO, we should have a closer look at those and check whether that's
really okay (just adding request/release blindly could cause trouble)

I agree, please feel free to audit these to verify they are all correct.
But that's not what you did here, so that's not a valid justification
for these patches to be accepted.

Understood. Assuming I've found some of these cases, shall I use devm
oder just add the missing release ?

quoted

In this consolidation process, I'm trying to move everything to
devm_*, to have it more generic (for now, I still need two versions
of the request/release/ioremap/iounmap helpers - one w/ and one
w/o devm).

Move everything in what part of the kernel?  The whole kernel or just
one subsystem?

Just talking about the serial drivers.

quoted

My idea was moving to devm first, so it can be reviewed/tested
independently, before moving forward. Smaller, easily digestable
pieces should minimize the risk of breaking anything. But if you
prefer having this things squashed together, just let me know.

Small pieces are required, that's fine, but those pieces need to have a
justification for why they should be accepted at all points along the
way.

Hmm, okay, in these cases, I agree there's no real justification if
we're not seeing it as an intermediate step to the upcoming stuff.
Having thought a bit more about this, my underlying dumbness was
setting everything on the devm horse when converting introducing the
helpers, and then splitted out the change to devm in even more patches
... Silly me, I better should have catched some sleep instead :o

quoted

In the queue are also other minor cleanups like using dev_err()
instead of printk(), etc. Should I send these separately ?

Of course.

Ok. I'll collect those things in a separate branch and send out the
queue from time to time:

https://github.com/metux/linux/tree/wip/serial/charlady

quoted

By the way: do you have some public branch where you're collecting
accepted patches, which I could base mine on ? (tty.git/tty-next ?)

Yes, that is the tree and branch, but remember that none of my trees can
open up until after -rc1 is out.

So, within a merge window, you put everything else on hold ?


--mtx

-- 
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287