| Message ID | e8221bff-3e2a-7607-c5c8-abcf9cebb1b5@free.fr (mailing list archive) |
|---|---|
| Headers | show |
| Series | Small devm helper for devm implementations | expand |
Looks like this series has fallen through the cracks :( Greg, you would be taking the drivers/base/devres.c changes? As mentioned in patch 2, "This patch needs testing on a platform with many clocks." (I've only tested using a trivial kernel module) On 10/03/2020 11:11, Marc Gonzalez wrote: > Differences from v4 to v5 > x Fix the grammar in devm_add comments [Geert] > x Undo an unrelated change in devm_clk_put [Geert] > > Differences from v3 to v4 > x Add a bunch of kerneldoc above devm_add() [Greg KH] > x Split patch in two [Greg KH] > > Differences from v2 to v3 > x Make devm_add() return an error-code rather than the raw data pointer > (in case devres_alloc ever returns an ERR_PTR) as suggested by Geert > x Provide a variadic version devm_vadd() to work with structs as suggested > by Geert > x Don't use nested ifs in clk_devm* implementations (hopefully simpler > code logic to follow) as suggested by Geert > > Marc Gonzalez (2): > devres: Provide new helper for devm functions > clk: Use devm_add in managed functions > > drivers/base/devres.c | 28 ++++++++++++ > drivers/clk/clk-devres.c | 97 +++++++++++++++------------------------- > include/linux/device.h | 3 ++ > 3 files changed, 67 insertions(+), 61 deletions(-)
On Thu, Jun 04, 2020 at 06:13:21PM +0200, Marc Gonzalez wrote: > Looks like this series has fallen through the cracks :( > Greg, you would be taking the drivers/base/devres.c changes? > As mentioned in patch 2, "This patch needs testing on a platform with many clocks." > (I've only tested using a trivial kernel module) I can't do anything during the merge window, sorry. Please feel free to resend it after 5.8-rc1 is out and I will be glad to review it then. thanks, greg k-h
Hello everyone, In my opinion, the small and simple devm_add() helper (and its cousin, devm_vadd) can help make devm code slightly easier to write and maintain. Would anyone care to agree or disagree? :-) Regards. On 10/03/2020 11:11, Marc Gonzalez wrote: > Differences from v4 to v5 > x Fix the grammar in devm_add comments [Geert] > x Undo an unrelated change in devm_clk_put [Geert] > > Differences from v3 to v4 > x Add a bunch of kerneldoc above devm_add() [Greg KH] > x Split patch in two [Greg KH] > > Differences from v2 to v3 > x Make devm_add() return an error-code rather than the raw data pointer > (in case devres_alloc ever returns an ERR_PTR) as suggested by Geert > x Provide a variadic version devm_vadd() to work with structs as suggested > by Geert > x Don't use nested ifs in clk_devm* implementations (hopefully simpler > code logic to follow) as suggested by Geert > > Marc Gonzalez (2): > devres: Provide new helper for devm functions > clk: Use devm_add in managed functions > > drivers/base/devres.c | 28 ++++++++++++ > drivers/clk/clk-devres.c | 97 +++++++++++++++------------------------- > include/linux/device.h | 3 ++ > 3 files changed, 67 insertions(+), 61 deletions(-)
Hello Greg, Would you agree to take this series? Regards. On 18/06/2020 13:38, Marc Gonzalez wrote: > Hello everyone, > > In my opinion, the small and simple devm_add() helper > (and its cousin, devm_vadd) can help make devm code > slightly easier to write and maintain. > > Would anyone care to agree or disagree? :-) > > Regards. > > On 10/03/2020 11:11, Marc Gonzalez wrote: > >> Differences from v4 to v5 >> x Fix the grammar in devm_add comments [Geert] >> x Undo an unrelated change in devm_clk_put [Geert] >> >> Differences from v3 to v4 >> x Add a bunch of kerneldoc above devm_add() [Greg KH] >> x Split patch in two [Greg KH] >> >> Differences from v2 to v3 >> x Make devm_add() return an error-code rather than the raw data pointer >> (in case devres_alloc ever returns an ERR_PTR) as suggested by Geert >> x Provide a variadic version devm_vadd() to work with structs as suggested >> by Geert >> x Don't use nested ifs in clk_devm* implementations (hopefully simpler >> code logic to follow) as suggested by Geert >> >> Marc Gonzalez (2): >> devres: Provide new helper for devm functions >> clk: Use devm_add in managed functions >> >> drivers/base/devres.c | 28 ++++++++++++ >> drivers/clk/clk-devres.c | 97 +++++++++++++++------------------------- >> include/linux/device.h | 3 ++ >> 3 files changed, 67 insertions(+), 61 deletions(-)
On Mon, Jul 06, 2020 at 05:56:04PM +0200, Marc Gonzalez wrote: > Hello Greg, > > Would you agree to take this series? Given the lack of testing of the patch, it doesn't seem wise to add this, right? Please get some testing, and some more users, and I'll be glad to consider it. thanks, greg k-h
On 06/07/2020 21:57, Greg Kroah-Hartman wrote: > Given the lack of testing of the patch, it doesn't seem wise to add > this, right? You're probably not wrong :) > Please get some testing, and some more users, and I'll be glad to > consider it. "Users" == files modified to use the new helper? How many files would you suggest? 3? 5? 10? The idea being to have the helper gain some kind of "critical mass"? Regards.
On Tue, Jul 21, 2020 at 09:10:23AM +0200, Marc Gonzalez wrote: > On 06/07/2020 21:57, Greg Kroah-Hartman wrote: > > > Given the lack of testing of the patch, it doesn't seem wise to add > > this, right? > > You're probably not wrong :) > > > Please get some testing, and some more users, and I'll be glad to > > consider it. > > "Users" == files modified to use the new helper? Yes. > How many files would you suggest? 3? 5? 10? How many do you see that can use it? I would suggest "all" :) thanks, greg k-h
On 23/07/2020 17:00, Greg Kroah-Hartman wrote: > On Tue, Jul 21, 2020 at 09:10:23AM +0200, Marc Gonzalez wrote: > >> On 06/07/2020 21:57, Greg Kroah-Hartman wrote: >> >>> Given the lack of testing of the patch, it doesn't seem wise to add >>> this, right? >> >> You're probably not wrong :) >> >>> Please get some testing, and some more users, and I'll be glad to >>> consider it. >> >> "Users" == files modified to use the new helper? > > Yes. > >> How many files would you suggest? 3? 5? 10? > > How many do you see that can use it? I would suggest "all" :) Hello Greg (and everyone on the CC list), I'm getting the itch to work on this patch-set again. To recap: I wrote a tiny devm helper. It's a trivial wrapper around devres_alloc() + devres_add() which releases the resource when devres_alloc() fails. That's all there is to it. Despite its triviality, the helper allows writing simpler code in drivers using devm, and generates smaller object code, so I think it's quite useful. With all that being said, I'm a bit concerned by Greg's "all" answer. $ git grep '= devres_alloc' | wc -l 173 $ git grep -c '= devres_alloc' | wc -l 103 There are 173 calls to devres_alloc across 103 files. It looks (to me) too risky to change everything in a single patch-set. Perhaps we could define a few frameworks that would get the improvement as a first step? $ git grep '= devres_alloc' v5.12 | grep -o 'v5.12:[[:alnum:]]*/[[:alnum:]]*' | uniq -c 1 v5.12:Documentation/driver 3 v5.12:drivers/ata 11 v5.12:drivers/base 1 v5.12:drivers/bus 1 v5.12:drivers/char 13 v5.12:drivers/clk 1 v5.12:drivers/counter 4 v5.12:drivers/devfreq 2 v5.12:drivers/dma 4 v5.12:drivers/extcon 2 v5.12:drivers/firmware 4 v5.12:drivers/fpga 8 v5.12:drivers/gpio 1 v5.12:drivers/gpu 2 v5.12:drivers/hid 2 v5.12:drivers/hwmon 3 v5.12:drivers/hwspinlock 1 v5.12:drivers/i2c 12 v5.12:drivers/iio 2 v5.12:drivers/input 1 v5.12:drivers/interconnect 5 v5.12:drivers/leds 1 v5.12:drivers/macintosh 1 v5.12:drivers/mailbox 2 v5.12:drivers/media 2 v5.12:drivers/mfd 1 v5.12:drivers/mtd 3 v5.12:drivers/mux 6 v5.12:drivers/net 1 v5.12:drivers/ntb 3 v5.12:drivers/nvmem 1 v5.12:drivers/of 4 v5.12:drivers/pci 5 v5.12:drivers/phy 3 v5.12:drivers/pinctrl 2 v5.12:drivers/platform 4 v5.12:drivers/power 3 v5.12:drivers/pwm 6 v5.12:drivers/regulator 1 v5.12:drivers/remoteproc 3 v5.12:drivers/reset 3 v5.12:drivers/spi 2 v5.12:drivers/staging 3 v5.12:drivers/thermal 1 v5.12:drivers/tty 1 v5.12:drivers/uio 2 v5.12:drivers/usb 2 v5.12:drivers/video 1 v5.12:drivers/watchdog 1 v5.12:kernel/dma 1 v5.12:kernel/iomem 5 v5.12:kernel/irq 1 v5.12:kernel/reboot 2 v5.12:kernel/resource 3 v5.12:lib/devres 1 v5.12:lib/genalloc 1 v5.12:mm/dmapool 2 v5.12:net/devres 1 v5.12:sound/hda 6 v5.12:sound/soc 1 v5.12:tools/testing Perhaps I could take a look at all the subsystems/frameworks that you maintain, Greg? What do you think? Regards.
On Mon, May 03, 2021 at 02:08:33PM +0200, Marc Gonzalez wrote: > On 23/07/2020 17:00, Greg Kroah-Hartman wrote: > > > On Tue, Jul 21, 2020 at 09:10:23AM +0200, Marc Gonzalez wrote: > > > >> On 06/07/2020 21:57, Greg Kroah-Hartman wrote: > >> > >>> Given the lack of testing of the patch, it doesn't seem wise to add > >>> this, right? > >> > >> You're probably not wrong :) > >> > >>> Please get some testing, and some more users, and I'll be glad to > >>> consider it. > >> > >> "Users" == files modified to use the new helper? > > > > Yes. > > > >> How many files would you suggest? 3? 5? 10? > > > > How many do you see that can use it? I would suggest "all" :) > > Hello Greg (and everyone on the CC list), > > I'm getting the itch to work on this patch-set again. > > To recap: I wrote a tiny devm helper. > It's a trivial wrapper around devres_alloc() + devres_add() > which releases the resource when devres_alloc() fails. > That's all there is to it. > > Despite its triviality, the helper allows writing simpler code > in drivers using devm, and generates smaller object code, > so I think it's quite useful. > > > With all that being said, I'm a bit concerned by Greg's "all" answer. > > $ git grep '= devres_alloc' | wc -l > 173 > > $ git grep -c '= devres_alloc' | wc -l > 103 > > There are 173 calls to devres_alloc across 103 files. > > It looks (to me) too risky to change everything in a single patch-set. > > Perhaps we could define a few frameworks that would get the improvement > as a first step? > > $ git grep '= devres_alloc' v5.12 | grep -o 'v5.12:[[:alnum:]]*/[[:alnum:]]*' | uniq -c > 1 v5.12:Documentation/driver > 3 v5.12:drivers/ata > 11 v5.12:drivers/base > 1 v5.12:drivers/bus > 1 v5.12:drivers/char > 13 v5.12:drivers/clk > 1 v5.12:drivers/counter > 4 v5.12:drivers/devfreq > 2 v5.12:drivers/dma > 4 v5.12:drivers/extcon > 2 v5.12:drivers/firmware > 4 v5.12:drivers/fpga > 8 v5.12:drivers/gpio > 1 v5.12:drivers/gpu > 2 v5.12:drivers/hid > 2 v5.12:drivers/hwmon > 3 v5.12:drivers/hwspinlock > 1 v5.12:drivers/i2c > 12 v5.12:drivers/iio > 2 v5.12:drivers/input > 1 v5.12:drivers/interconnect > 5 v5.12:drivers/leds > 1 v5.12:drivers/macintosh > 1 v5.12:drivers/mailbox > 2 v5.12:drivers/media > 2 v5.12:drivers/mfd > 1 v5.12:drivers/mtd > 3 v5.12:drivers/mux > 6 v5.12:drivers/net > 1 v5.12:drivers/ntb > 3 v5.12:drivers/nvmem > 1 v5.12:drivers/of > 4 v5.12:drivers/pci > 5 v5.12:drivers/phy > 3 v5.12:drivers/pinctrl > 2 v5.12:drivers/platform > 4 v5.12:drivers/power > 3 v5.12:drivers/pwm > 6 v5.12:drivers/regulator > 1 v5.12:drivers/remoteproc > 3 v5.12:drivers/reset > 3 v5.12:drivers/spi > 2 v5.12:drivers/staging > 3 v5.12:drivers/thermal > 1 v5.12:drivers/tty > 1 v5.12:drivers/uio > 2 v5.12:drivers/usb > 2 v5.12:drivers/video > 1 v5.12:drivers/watchdog > 1 v5.12:kernel/dma > 1 v5.12:kernel/iomem > 5 v5.12:kernel/irq > 1 v5.12:kernel/reboot > 2 v5.12:kernel/resource > 3 v5.12:lib/devres > 1 v5.12:lib/genalloc > 1 v5.12:mm/dmapool > 2 v5.12:net/devres > 1 v5.12:sound/hda > 6 v5.12:sound/soc > 1 v5.12:tools/testing > > > Perhaps I could take a look at all the subsystems/frameworks that you maintain, Greg? > What do you think? Just do a few, not all to start with by any means. Just examples of how this will be used to show if it really does make things better or not. And sure, do USB and TTY to start with if that makes it easier. thanks, greg k-h