| Message ID | 1552602855-26086-11-git-send-email-info@metux.net (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | [v2,01/45] drivers: tty: serial: 8250_bcm2835aux: use devm_platform_ioremap_resource() | expand |
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? > > Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net> > --- > drivers/tty/serial/zs.c | 16 ++++++++++------ > 1 file changed, 10 insertions(+), 6 deletions(-) > > diff --git a/drivers/tty/serial/zs.c b/drivers/tty/serial/zs.c > index b03d3e4..0b1ec2f 100644 > --- a/drivers/tty/serial/zs.c > +++ b/drivers/tty/serial/zs.c > @@ -984,16 +984,17 @@ static const char *zs_type(struct uart_port *uport) > > static void zs_release_port(struct uart_port *uport) > { > - iounmap(uport->membase); > + devm_iounmap(uport->dev, uport->membase); > uport->membase = 0; > - release_mem_region(uport->mapbase, ZS_CHAN_IO_SIZE); > + devm_release_mem_region(uport->dev, uport->mapbase, ZS_CHAN_IO_SIZE); Isn't the whole goal of the devm* functions such that you are not required to call "release" on them? If so, are you sure this patchset is correct? 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? thanks, greg k-h
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
On Fri, Mar 15, 2019 at 10:06:16AM +0100, Enrico Weigelt, metux IT consult wrote: > 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 :) At times, but not when you do not use the api correctly :) > Several drivers didn't seem to clean up properly (maybe these're just > used compiled-in, so nobody noticed yet). 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. > In general, I think devm_* should be the standard case, unless there's > a really good reason to do otherwise. 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 :( > > 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) 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. > > 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). Move everything in what part of the kernel? The whole kernel or just one subsystem? > 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. > In the queue are also other minor cleanups like using dev_err() > instead of printk(), etc. Should I send these separately ? Of course. > 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. thanks, greg k-h
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: >> 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 :) > > 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. >> 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 ? >> 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. >> 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 >> 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 >> 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
On Fri, Mar 15, 2019 at 08:12:47PM +0100, Enrico Weigelt, metux IT consult wrote: > On 15.03.19 15:26, Greg KH wrote: > > 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, it's just that those systems do not allow those devices to be removed because they are probably not on a removable bus. > > 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. "Consistent" is good, and valid, but touching old drivers that have few users is always risky, and you need a solid reason to do so. > >> 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 ? If it actually makes the code "simpler" or "more obvious", sure, that's fine. But churn for churns sake is not ok. > >> 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 ? I put the review of new patch submissions on hold, yes. Almost all maintainers do that as we can not add new patches to our trees at that point in time. And I do have other things I do during that period so it's not like I'm just sitting around doing nothing :) thanks, greg k-h
On 16.03.19 04:26, Greg KH wrote: > No, it's just that those systems do not allow those devices to be > removed because they are probably not on a removable bus. Ok, devices (hw) might not be removable - that also the case for uarts builtin some SoCs, or the good old PC w/ 8250. But does that also mean that the driver should not be removable ? IMHO, even if that's the case, it's still inconsistent. The driver then shouldn't support a remove at all (or even builtin only), not just incomplete remove. >> 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. > > "Consistent" is good, and valid, but touching old drivers that have few > users is always risky, and you need a solid reason to do so. Understood. By the way: do we have some people who have those old hw and could test? Should we (try to) create some ? Perhaps some "tester" entry in MAINTAINERS file ? (I could ask around several people who might have lots of old / rare hardware.) >> Understood. Assuming I've found some of these cases, shall I use devm >> oder just add the missing release ? > > If it actually makes the code "simpler" or "more obvious", sure, that's > fine. But churn for churns sake is not ok. Ok. > I put the review of new patch submissions on hold, yes. Almost all > maintainers do that as we can not add new patches to our trees at that > point in time. hmm, looks like a pipeline stall ;-) why not collecting in a separate branch, which later gets rebased to mainline when rc is out ? > And I do have other things I do during that period so it's not like I'm > just sitting around doing nothing :) So it's also a fixed schedule for your other work. Understood. It seems that this workflow can confuse people. Few days ago, somebody became nervous about missing reactions on patches. Your autoresponder worked for me, but maybe not for everybody. --mtx
On Sat, Mar 16, 2019 at 10:17:11AM +0100, Enrico Weigelt, metux IT consult wrote: > On 16.03.19 04:26, Greg KH wrote: > > > No, it's just that those systems do not allow those devices to be > > removed because they are probably not on a removable bus. > > Ok, devices (hw) might not be removable - that also the case for uarts > builtin some SoCs, or the good old PC w/ 8250. But does that also mean > that the driver should not be removable ? No, but 'rmmod' is not a normal operation that anyone ever does in a working system. It is only for developer's ease-of-use. > IMHO, even if that's the case, it's still inconsistent. The driver then > shouldn't support a remove at all (or even builtin only), not just > incomplete remove. Cleaning up properly when the module is unloaded is a good idea, but so far the patches you submitted did not change anything from a logic point of view. They all just cleaned up memory the same way it was cleaned up before, so I really do not understand what you are trying to do here. > >> 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. > > > > "Consistent" is good, and valid, but touching old drivers that have few > > users is always risky, and you need a solid reason to do so. > > Understood. > > By the way: do we have some people who have those old hw and could test? > Should we (try to) create some ? Perhaps some "tester" entry in > MAINTAINERS file ? (I could ask around several people who might have > lots of old / rare hardware.) Let's not clutter up MAINTAINERS with anything else please. > >> Understood. Assuming I've found some of these cases, shall I use devm > >> oder just add the missing release ? > > > > If it actually makes the code "simpler" or "more obvious", sure, that's > > fine. But churn for churns sake is not ok. > > Ok. > > > I put the review of new patch submissions on hold, yes. Almost all > > maintainers do that as we can not add new patches to our trees at that > > point in time. > > hmm, looks like a pipeline stall ;-) > why not collecting in a separate branch, which later gets rebased to > mainline when rc is out ? I do do that for subsystems that actually have a high patch rate. The tty/serial subsystem is not such a thing, and it can handle 2 weeks of delay just fine. > > And I do have other things I do during that period so it's not like I'm > > just sitting around doing nothing :) > > So it's also a fixed schedule for your other work. Understood. > > It seems that this workflow can confuse people. Few days ago, somebody > became nervous about missing reactions on patches. Your autoresponder > worked for me, but maybe not for everybody. Why would it not work for everybody? Kernel development has been done in this manner for over a decade. Having a 2 week window like this is good for the maintainers, remember they are the most limited resource we have, not developers. thanks, greg k-h
On Sat, 16 Mar 2019, Enrico Weigelt, metux IT consult wrote: > > No, it's just that those systems do not allow those devices to be > > removed because they are probably not on a removable bus. > > Ok, devices (hw) might not be removable - that also the case for uarts > builtin some SoCs, or the good old PC w/ 8250. But does that also mean > that the driver should not be removable ? > > IMHO, even if that's the case, it's still inconsistent. The driver then > shouldn't support a remove at all (or even builtin only), not just > incomplete remove. This device (as well as `dz') is typically used for the serial console as well, so being built-in is the usual configuration. Nevertheless modular operation is supposed to be supported, however it may not have been verified for ages. A further complication is in the virtual console configuration one of the serial lines is dedicated for the keyboard, so again you want the driver built-in (although hooking up the virtual console keyboard this way has been broken with the conversion to the serial core in the 2.6 timeframe and I have never figured it out how it is supposed to be done correctly with the new serial infrastructure and SERIO_SERPORT; I believe some platforms do it with the use of horrible hacks rather than SERIO_SERPORT). Maciej
diff --git a/drivers/tty/serial/zs.c b/drivers/tty/serial/zs.c index b03d3e4..0b1ec2f 100644 --- a/drivers/tty/serial/zs.c +++ b/drivers/tty/serial/zs.c @@ -984,16 +984,17 @@ static const char *zs_type(struct uart_port *uport) static void zs_release_port(struct uart_port *uport) { - iounmap(uport->membase); + devm_iounmap(uport->dev, uport->membase); uport->membase = 0; - release_mem_region(uport->mapbase, ZS_CHAN_IO_SIZE); + devm_release_mem_region(uport->dev, uport->mapbase, ZS_CHAN_IO_SIZE); } static int zs_map_port(struct uart_port *uport) { if (!uport->membase) - uport->membase = ioremap_nocache(uport->mapbase, - ZS_CHAN_IO_SIZE); + uport->membase = devm_ioremap_nocache(uport->dev, + uport->mapbase, + ZS_CHAN_IO_SIZE); if (!uport->membase) { printk(KERN_ERR "zs: Cannot map MMIO\n"); return -ENOMEM; @@ -1005,13 +1006,16 @@ static int zs_request_port(struct uart_port *uport) { int ret; - if (!request_mem_region(uport->mapbase, ZS_CHAN_IO_SIZE, "scc")) { + if (!devm_request_mem_region(uport->mapbase, + ZS_CHAN_IO_SIZE, "scc")) { printk(KERN_ERR "zs: Unable to reserve MMIO resource\n"); return -EBUSY; } ret = zs_map_port(uport); if (ret) { - release_mem_region(uport->mapbase, ZS_CHAN_IO_SIZE); + devm_release_mem_region(uport-dev, + uport->mapbase, + ZS_CHAN_IO_SIZE); return ret; } return 0;
Use the safer devm versions of memory mapping functions. Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net> --- drivers/tty/serial/zs.c | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-)