Date:   Fri, 15 Mar 2019 10:06:16 +0100
From:   "Enrico Weigelt, metux IT consult" <lkml@...ux.net>
To:     Greg KH <gregkh@...uxfoundation.org>,
        "Enrico Weigelt, metux IT consult" <info@...ux.net>
Cc:     linux-kernel@...r.kernel.org, eric@...olt.net,
        stefan.wahren@...e.com, f.fainelli@...il.com, rjui@...adcom.com,
        sbranden@...adcom.com, bcm-kernel-feedback-list@...adcom.com,
        andriy.shevchenko@...ux.intel.com, vz@...ia.com,
        matthias.bgg@...il.com, yamada.masahiro@...ionext.com,
        tklauser@...tanz.ch, richard.genoud@...il.com,
        macro@...ux-mips.org, u.kleine-koenig@...gutronix.de,
        kernel@...gutronix.de, slemieux.tyco@...il.com,
        andy.gross@...aro.org, david.brown@...aro.org, shawnguo@...nel.org,
        s.hauer@...gutronix.de, festevam@...il.com, linux-imx@....com,
        baohua@...nel.org, jacmet@...site.dk, linux-serial@...r.kernel.org,
        linux-arm-msm@...r.kernel.org, linuxppc-dev@...ts.ozlabs.org
Subject: Re: [PATCH v2 10/45] drivers: tty: serial: zs: use devm_* functions

On 14.03.19 23:52, Greg KH wrote:
> On Thu, Mar 14, 2019 at 11:33:40PM +0100, Enrico Weigelt, metux IT consult wrote:
>> Use the safer devm versions of memory mapping functions.
> 
> What is "safer" about them?

Garbage collection :)

Several drivers didn't seem to clean up properly (maybe these're just
used compiled-in, so nobody noticed yet).

In general, I think devm_* should be the standard case, unless there's
a really good reason to do otherwise.

<snip>

> Isn't the whole goal of the devm* functions such that you are not
> required to call "release" on them?

Looks that one slipped through, when I was doing that big bulk change
in the middle of the night and not enough coffe ;-)

One problem here is that many drivers do this stuff in request/release
port, instead of probe/remove. I'm not sure yet, whether we should
rewrite that. There're also cases which do request/release, but no
ioremap (doing hardcoded register accesses), others do ioremap w/o
request/release.

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

> And also, why make the change, you aren't changing any functionality for
> these old drivers at all from what I can tell (for the devm calls).
> What am I missing here?

Okay, there's a bigger story behind, you can't know yet. Finally, I'd
like to move everything to using struct resource and corresponding
helpers consistently, so most of the drivers would be pretty simple
at that point. (there're of course special cases, like devices w/
multiple register spaces, etc)

Here's my wip branch:

https://github.com/metux/linux/commits/wip/serial-res

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).

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.

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

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 ?)


--mtx

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

Powered by blists - more mailing lists