| Message ID | 20240229202300.3321cc11@jic23-huawei (mailing list archive) |
|---|---|
| State | Deferred |
| Headers | show |
| Series | [PULL] IIO: 3rd set for 6.9 - cleanup.h handling of fwnode_handle_put() related (not ordered wrt to PULL 2) | expand |
On Thu, Feb 29, 2024 at 08:23:00PM +0000, Jonathan Cameron wrote: > The following changes since commit d4551c189d6e6a3fcf7f625bd4b273e770fad35a: > > Merge tag 'iio-for-6.9a' of http://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio into char-misc-next (2024-02-25 14:11:41 +0100) > > are available in the Git repository at: > > https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git tags/iio-for-6.9c > > for you to fetch changes up to 64e19caa5564ecc43edaa7fb818d53de650d9b34: > > iio: adc: rzg2l_adc: Use device_for_each_child_node_scoped() (2024-02-28 19:15:43 +0000) > > ---------------------------------------------------------------- > IIO: 3rd set for 6.9 - cleanup.h related. > > I have separated this set out from the more normal patches as they can > go separately and that may simplify the merge window. Greg, up to you > how you wish to handle this in the char-misc tree. > > Introduces __free() based handling for fwnode_handle_put() to > allow scope based release of these handles on early exit from functions. This should be fine, right? No one complains about these. > Also introduced device_for_each_child_node_scoped() to provide a > a convenient way to process child nodes without the need to explicitly > handle the fwnode_handle_put() needed on early exits from the loop. > Typically these early exits are a result of error handling or completion > of a search and have proven very prone to being missed. This is trickier, there was a load of different versions floating around, do you have a link to the "last" version of this patch series that got applied here? > One instance of such a leaked resource was found during these conversions > though review of that patch was too late for this pull request. I don't understand, does this series fix that found problem? Or is it coming? > A number of drivers are also converted over to generic fwnode handling from > the device tree specific version. Deleting code is always good, but: > > ---------------------------------------------------------------- > Jonathan Cameron (16): > device property: Move fwnode_handle_put() into property.h > device property: Add cleanup.h based fwnode_handle_put() scope based cleanup. > device property: Introduce device_for_each_child_node_scoped() > iio: adc: max11410: Use device_for_each_child_node_scoped() > iio: addac: ad74413r: Use device_for_each_child_node_scoped() > iio: dac: ltc2688: Use device_for_each_child_node_scoped() > iio: adc: fsl-imx25-gcq: Switch from of specific handing to fwnode based. > iio: adc: fsl-imx25-gcq: Use devm_* and dev_err_probe() to simplify probe > iio: adc: ad7124: Switch from of specific to fwnode based property handling > iio: adc: ad7292: Switch from of specific to fwnode property handling > iio: adc: ad7192: Convert from of specific to fwnode property handling > iio: accel: mma8452: Switch from of specific to fwnode property handling. > iio: accel: fxls8962af: Switch from of specific to fwnode based properties. > iio: adc: hx711: Switch from of specific to fwnode property handling. > iio: temp: ltc2983: Use __free(fwnode_handle) and device_for_each_node_scoped() > iio: adc: rzg2l_adc: Use device_for_each_child_node_scoped() You are mixing the two different handlers in this series, right? How about 2 different ones, one for each? Or do they start to conflict? thanks, greg k-h
On Sat, 2 Mar 2024 20:06:03 +0100 Greg KH <gregkh@linuxfoundation.org> wrote: > On Thu, Feb 29, 2024 at 08:23:00PM +0000, Jonathan Cameron wrote: > > The following changes since commit d4551c189d6e6a3fcf7f625bd4b273e770fad35a: > > > > Merge tag 'iio-for-6.9a' of http://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio into char-misc-next (2024-02-25 14:11:41 +0100) > > > > are available in the Git repository at: > > > > https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git tags/iio-for-6.9c > > > > for you to fetch changes up to 64e19caa5564ecc43edaa7fb818d53de650d9b34: > > > > iio: adc: rzg2l_adc: Use device_for_each_child_node_scoped() (2024-02-28 19:15:43 +0000) > > > > ---------------------------------------------------------------- > > IIO: 3rd set for 6.9 - cleanup.h related. > > > > I have separated this set out from the more normal patches as they can > > go separately and that may simplify the merge window. Greg, up to you > > how you wish to handle this in the char-misc tree. > > > > Introduces __free() based handling for fwnode_handle_put() to > > allow scope based release of these handles on early exit from functions. > > This should be fine, right? No one complains about these. Agreed this type isn't novel. However, there is only use use so far outside of device_for_each_child_node_scoped() and that's in a patch that includes both a direct use of __free(fwnode_handle) and a couple of device_for_each_child_node_scoped() [PATCH v5 1/9] iio: temp: ltc2983: Use __free(fwnode_handle) and device_for_each_node_scoped() https://lore.kernel.org/all/20240224123215.161469-2-jic23@kernel.org/ > > > Also introduced device_for_each_child_node_scoped() to provide a > > a convenient way to process child nodes without the need to explicitly > > handle the fwnode_handle_put() needed on early exits from the loop. > > Typically these early exits are a result of error handling or completion > > of a search and have proven very prone to being missed. > > This is trickier, there was a load of different versions floating > around, do you have a link to the "last" version of this patch series > that got applied here? https://lore.kernel.org/all/20240217164249.921878-1-jic23@kernel.org/ For the infrastructure - some of the users were in follow up series (the DT to fwnode conversions Andy asked for and a v5 series to update a few things and pester people for reviews). > > > One instance of such a leaked resource was found during these conversions > > though review of that patch was too late for this pull request. > > I don't understand, does this series fix that found problem? Or is it > coming? That one is still to come - review came in too late for this pull request. https://lore.kernel.org/all/20240224123215.161469-6-jic23@kernel.org/ Though I remembered the issue a little wrong as it's a failure to set the return value to an error, so arguably worse than an error leak as we'll falsely carry on when parsing the fw failed. The resource leak cases were review of DT versions that came from Julia's coccinelle script. > > > A number of drivers are also converted over to generic fwnode handling from > > the device tree specific version. > > Deleting code is always good, but: > > > > > > ---------------------------------------------------------------- > > Jonathan Cameron (16): > > device property: Move fwnode_handle_put() into property.h > > device property: Add cleanup.h based fwnode_handle_put() scope based cleanup. > > device property: Introduce device_for_each_child_node_scoped() > > iio: adc: max11410: Use device_for_each_child_node_scoped() > > iio: addac: ad74413r: Use device_for_each_child_node_scoped() > > iio: dac: ltc2688: Use device_for_each_child_node_scoped() > > iio: adc: fsl-imx25-gcq: Switch from of specific handing to fwnode based. > > iio: adc: fsl-imx25-gcq: Use devm_* and dev_err_probe() to simplify probe > > iio: adc: ad7124: Switch from of specific to fwnode based property handling > > iio: adc: ad7292: Switch from of specific to fwnode property handling > > iio: adc: ad7192: Convert from of specific to fwnode property handling > > iio: accel: mma8452: Switch from of specific to fwnode property handling. > > iio: accel: fxls8962af: Switch from of specific to fwnode based properties. > > iio: adc: hx711: Switch from of specific to fwnode property handling. > > iio: temp: ltc2983: Use __free(fwnode_handle) and device_for_each_node_scoped() > > iio: adc: rzg2l_adc: Use device_for_each_child_node_scoped() > > You are mixing the two different handlers in this series, right? How > about 2 different ones, one for each? Or do they start to conflict? It's one handler in here but two usecases so they are dependent. First the basic __free(fwnode_handle) then the device_for_each_child_node_scoped() which uses it internally.. The patches that mention DT are conversions over to fwnode_handle that incorporate using device_for_each_child_node() As for the ltc2983, there are users of just the __free(fwnode_handle) but in IIO at least, far more users of it via device_for_each_child_node_scoped(). So not that useful to split the series :( The device tree equivalent is a separate series not included here. That just got the necessary DT maintainer tags RB from Rob Herring today. If we get a little more time (i.e. an rc8) this cycle, it would be good to get that in place as well - I'll queue it up for linux-next today. This is the DT series. https://lore.kernel.org/linux-iio/20240301223942.GA3179769-robh@kernel.org/T/#t Jonathan > > thanks, > > greg k-h
> > > > > > ---------------------------------------------------------------- > > > Jonathan Cameron (16): > > > device property: Move fwnode_handle_put() into property.h > > > device property: Add cleanup.h based fwnode_handle_put() scope based cleanup. > > > device property: Introduce device_for_each_child_node_scoped() > > > iio: adc: max11410: Use device_for_each_child_node_scoped() > > > iio: addac: ad74413r: Use device_for_each_child_node_scoped() > > > iio: dac: ltc2688: Use device_for_each_child_node_scoped() > > > iio: adc: fsl-imx25-gcq: Switch from of specific handing to fwnode based. > > > iio: adc: fsl-imx25-gcq: Use devm_* and dev_err_probe() to simplify probe > > > iio: adc: ad7124: Switch from of specific to fwnode based property handling > > > iio: adc: ad7292: Switch from of specific to fwnode property handling > > > iio: adc: ad7192: Convert from of specific to fwnode property handling > > > iio: accel: mma8452: Switch from of specific to fwnode property handling. > > > iio: accel: fxls8962af: Switch from of specific to fwnode based properties. > > > iio: adc: hx711: Switch from of specific to fwnode property handling. > > > iio: temp: ltc2983: Use __free(fwnode_handle) and device_for_each_node_scoped() > > > iio: adc: rzg2l_adc: Use device_for_each_child_node_scoped() > > > > You are mixing the two different handlers in this series, right? How > > about 2 different ones, one for each? Or do they start to conflict? > > It's one handler in here but two usecases so they are dependent. > First the basic __free(fwnode_handle) then the device_for_each_child_node_scoped() > which uses it internally.. The patches that mention DT are conversions over > to fwnode_handle that incorporate using device_for_each_child_node() > > As for the ltc2983, there are users of just the __free(fwnode_handle) but in > IIO at least, far more users of it via device_for_each_child_node_scoped(). > So not that useful to split the series :( > > The device tree equivalent is a separate series not included here. > That just got the necessary DT maintainer tags RB from Rob Herring today. > If we get a little more time (i.e. an rc8) this cycle, it would be > good to get that in place as well - I'll queue it up for linux-next today. > This is the DT series. > https://lore.kernel.org/linux-iio/20240301223942.GA3179769-robh@kernel.org/T/#t Given there were no issues raised with the DT equivalent patches that Rob took, I think I was being overly cautions. Ah well, I'm sure I'll veer the other way next time :) Anyhow, I'm going to put these at the top of my tree (based on v6.9-rc1) so that I can tag the first couple for anyone who wants them for other trees. (a lazy way to get an immutable branch given I'm rebasing the togreg tree anyway) Note that means I'm retiring the two separate feeder branches and going back to togreg alone (the one linux-next picks up). So please ignore this PULL request as it will be superseded by a fresh one for 6.10. For those that want it (who happen to be reading this thread) im-tag-device_for_each_child_node_scoped() and includes patches 1-3 of this pull request. Thanks, Jonathan > > Jonathan > > > > > > thanks, > > > > greg k-h > >
The following changes since commit d4551c189d6e6a3fcf7f625bd4b273e770fad35a: Merge tag 'iio-for-6.9a' of http://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio into char-misc-next (2024-02-25 14:11:41 +0100) are available in the Git repository at: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git tags/iio-for-6.9c for you to fetch changes up to 64e19caa5564ecc43edaa7fb818d53de650d9b34: iio: adc: rzg2l_adc: Use device_for_each_child_node_scoped() (2024-02-28 19:15:43 +0000) ---------------------------------------------------------------- IIO: 3rd set for 6.9 - cleanup.h related. I have separated this set out from the more normal patches as they can go separately and that may simplify the merge window. Greg, up to you how you wish to handle this in the char-misc tree. Introduces __free() based handling for fwnode_handle_put() to allow scope based release of these handles on early exit from functions. Also introduced device_for_each_child_node_scoped() to provide a a convenient way to process child nodes without the need to explicitly handle the fwnode_handle_put() needed on early exits from the loop. Typically these early exits are a result of error handling or completion of a search and have proven very prone to being missed. One instance of such a leaked resource was found during these conversions though review of that patch was too late for this pull request. A number of drivers are also converted over to generic fwnode handling from the device tree specific version. ---------------------------------------------------------------- Jonathan Cameron (16): device property: Move fwnode_handle_put() into property.h device property: Add cleanup.h based fwnode_handle_put() scope based cleanup. device property: Introduce device_for_each_child_node_scoped() iio: adc: max11410: Use device_for_each_child_node_scoped() iio: addac: ad74413r: Use device_for_each_child_node_scoped() iio: dac: ltc2688: Use device_for_each_child_node_scoped() iio: adc: fsl-imx25-gcq: Switch from of specific handing to fwnode based. iio: adc: fsl-imx25-gcq: Use devm_* and dev_err_probe() to simplify probe iio: adc: ad7124: Switch from of specific to fwnode based property handling iio: adc: ad7292: Switch from of specific to fwnode property handling iio: adc: ad7192: Convert from of specific to fwnode property handling iio: accel: mma8452: Switch from of specific to fwnode property handling. iio: accel: fxls8962af: Switch from of specific to fwnode based properties. iio: adc: hx711: Switch from of specific to fwnode property handling. iio: temp: ltc2983: Use __free(fwnode_handle) and device_for_each_node_scoped() iio: adc: rzg2l_adc: Use device_for_each_child_node_scoped() drivers/base/property.c | 14 ---- drivers/iio/accel/fxls8962af-core.c | 10 +-- drivers/iio/accel/mma8452.c | 6 +- drivers/iio/adc/ad7124.c | 55 ++++++-------- drivers/iio/adc/ad7192.c | 38 +++++----- drivers/iio/adc/ad7292.c | 13 ++-- drivers/iio/adc/fsl-imx25-gcq.c | 140 +++++++++++++++--------------------- drivers/iio/adc/hx711.c | 5 +- drivers/iio/adc/max11410.c | 27 ++----- drivers/iio/adc/rzg2l_adc.c | 11 +-- drivers/iio/addac/ad74413r.c | 10 +-- drivers/iio/dac/ltc2688.c | 28 +++----- drivers/iio/temperature/ltc2983.c | 137 +++++++++++++---------------------- include/linux/property.h | 22 +++++- 14 files changed, 206 insertions(+), 310 deletions(-)