| Message ID | 20240530074416.13697-1-johan+linaro@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | iio: inkern: fix channel read regression | expand |
On Thu, 30 May 2024 09:44:16 +0200 Johan Hovold <johan+linaro@kernel.org> wrote: > A recent "cleanup" broke IIO channel read outs and thereby thermal > mitigation on the Lenovo ThinkPad X13s by returning zero instead of the > expected IIO value type in iio_read_channel_processed_scale(): > > thermal thermal_zone12: failed to read out thermal zone (-22) > > Fixes: 3092bde731ca ("iio: inkern: move to the cleanup.h magic") > Cc: Nuno Sa <nuno.sa@analog.com> > Signed-off-by: Johan Hovold <johan+linaro@kernel.org> Hi Johan, The documentation for this function unfortunately says * Returns an error code or 0 so not surprising this went wrong. All those docs should be dragged alongside the implementations to make it easier to spot errors and seems we need to do some fixing up of those docs in general :( In meantime, Nuno please take another look at these and see if we have additional problem cases like this. Given the patch queue I have and a busy few days it will be a while before I get to it but I'll try and take a close look soon as well. Longer term, in my view the readability and chance of bugs is reduced, but churn always introduces the possibility of issues like this in the short term :( Jonathan > --- > drivers/iio/inkern.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > > Someone should re-review the offending commit so that there are no more > of these subtle regressions that are bound to happen when people use > cleanup.h to save a few lines of code at the cost of readability. > > Johan > > > > diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c > index 52d773261828..485e6fc44a04 100644 > --- a/drivers/iio/inkern.c > +++ b/drivers/iio/inkern.c > @@ -721,7 +721,7 @@ int iio_read_channel_processed_scale(struct iio_channel *chan, int *val, > return ret; > *val *= scale; > > - return 0; > + return ret; > } else { > ret = iio_channel_read(chan, val, NULL, IIO_CHAN_INFO_RAW); > if (ret < 0)
Hi Jonathan, On Sat, Jun 01, 2024 at 02:21:47PM +0100, Jonathan Cameron wrote: > On Thu, 30 May 2024 09:44:16 +0200 > Johan Hovold <johan+linaro@kernel.org> wrote: > > > A recent "cleanup" broke IIO channel read outs and thereby thermal > > mitigation on the Lenovo ThinkPad X13s by returning zero instead of the > > expected IIO value type in iio_read_channel_processed_scale(): > > > > thermal thermal_zone12: failed to read out thermal zone (-22) > > > > Fixes: 3092bde731ca ("iio: inkern: move to the cleanup.h magic") > > Cc: Nuno Sa <nuno.sa@analog.com> > > Signed-off-by: Johan Hovold <johan+linaro@kernel.org> > In meantime, Nuno please take another look at these and see if > we have additional problem cases like this. Given the patch > queue I have and a busy few days it will be a while before I > get to it but I'll try and take a close look soon as well. Please consider getting this one into mainline as soon as possible as it breaks thermal mitigation, which can cause literal skin burns, on all Qualcomm platforms (and probably other platforms too). > Longer term, in my view the readability and chance of bugs > is reduced, but churn always introduces the possibility of > issues like this in the short term :( I just worry about some of the cleanup.h conversion I've seen where inexperienced developers potentially break tested and reviewed code for something which is often not very readable and for very little gain. Johan
On Mon, 3 Jun 2024 09:24:38 +0200 Johan Hovold <johan@kernel.org> wrote: > Hi Jonathan, > > On Sat, Jun 01, 2024 at 02:21:47PM +0100, Jonathan Cameron wrote: > > On Thu, 30 May 2024 09:44:16 +0200 > > Johan Hovold <johan+linaro@kernel.org> wrote: > > > > > A recent "cleanup" broke IIO channel read outs and thereby thermal > > > mitigation on the Lenovo ThinkPad X13s by returning zero instead of the > > > expected IIO value type in iio_read_channel_processed_scale(): > > > > > > thermal thermal_zone12: failed to read out thermal zone (-22) > > > > > > Fixes: 3092bde731ca ("iio: inkern: move to the cleanup.h magic") > > > Cc: Nuno Sa <nuno.sa@analog.com> > > > Signed-off-by: Johan Hovold <johan+linaro@kernel.org> > > > In meantime, Nuno please take another look at these and see if > > we have additional problem cases like this. Given the patch > > queue I have and a busy few days it will be a while before I > > get to it but I'll try and take a close look soon as well. > > Please consider getting this one into mainline as soon as possible as it > breaks thermal mitigation, which can cause literal skin burns, on all > Qualcomm platforms (and probably other platforms too). I'll queue this fix and we can follow with any others that surface from reviews. Need it to sit in linux-next for a day or two before a pull request though as I've just pushed some other fixes out that needs some build coverage. > > > Longer term, in my view the readability and chance of bugs > > is reduced, but churn always introduces the possibility of > > issues like this in the short term :( > > I just worry about some of the cleanup.h conversion I've seen where > inexperienced developers potentially break tested and reviewed code for > something which is often not very readable and for very little gain. > Fair comment. There have been some 'interesting' patches. On the plus side this one highlighted that our docs are wrong. Applied to the fixes-togreg branch of iio.git Jonathan > Johan
diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c index 52d773261828..485e6fc44a04 100644 --- a/drivers/iio/inkern.c +++ b/drivers/iio/inkern.c @@ -721,7 +721,7 @@ int iio_read_channel_processed_scale(struct iio_channel *chan, int *val, return ret; *val *= scale; - return 0; + return ret; } else { ret = iio_channel_read(chan, val, NULL, IIO_CHAN_INFO_RAW); if (ret < 0)
A recent "cleanup" broke IIO channel read outs and thereby thermal mitigation on the Lenovo ThinkPad X13s by returning zero instead of the expected IIO value type in iio_read_channel_processed_scale(): thermal thermal_zone12: failed to read out thermal zone (-22) Fixes: 3092bde731ca ("iio: inkern: move to the cleanup.h magic") Cc: Nuno Sa <nuno.sa@analog.com> Signed-off-by: Johan Hovold <johan+linaro@kernel.org> --- drivers/iio/inkern.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) Someone should re-review the offending commit so that there are no more of these subtle regressions that are bound to happen when people use cleanup.h to save a few lines of code at the cost of readability. Johan