Message ID | 20250105172613.1204781-10-jic23@kernel.org (mailing list archive) |
---|---|
State | Changes Requested |
Headers | show |
Series | iio: improve handling of direct mode claim and release | expand |
On 1/5/25 11:25 AM, Jonathan Cameron wrote: > From: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > This complex cleanup.h use case of conditional guards has proved > to be more trouble that it is worth in terms of false positive compiler > warnings and hard to read code. > > Move directly to the new claim/release_direct() that allow sparse > to check for unbalanced context. > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > --- > drivers/iio/adc/ad4000.c | 61 +++++++++++++++++++++++++--------------- > 1 file changed, 38 insertions(+), 23 deletions(-) > > diff --git a/drivers/iio/adc/ad4000.c b/drivers/iio/adc/ad4000.c > index 1d556a842a68..ef0acaafbcdb 100644 ... > +static int ad4000_write_raw(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, > + int val, int val2, long mask) > +{ > + int ret; > + > + switch (mask) { > + case IIO_CHAN_INFO_SCALE: > + Spurious blank line added. > + if (!iio_device_claim_direct(indio_dev)) > + return -EBUSY; > + ret = __ad4000_write_raw(indio_dev, chan, val2); > + iio_device_release_direct(indio_dev); > + return ret; > default: > return -EINVAL; > }
On 01/05, Jonathan Cameron wrote: > From: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > This complex cleanup.h use case of conditional guards has proved > to be more trouble that it is worth in terms of false positive compiler > warnings and hard to read code. > > Move directly to the new claim/release_direct() that allow sparse > to check for unbalanced context. > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > --- > drivers/iio/adc/ad4000.c | 61 +++++++++++++++++++++++++--------------- > 1 file changed, 38 insertions(+), 23 deletions(-) > Hi Jonathan, aside from the spurious blank line noted by David, the changes for ad4000 look good to me. Acked-by: <marcelo.schmitt1@gmail.com> I also tried running Sparse on IIO subsystem but didn't see any warns for the drivers being changed (nor prior nor after applying the patches). make CHECK="path_to_local_sparse_v0.6.4-66-g0196afe1" C=2 drivers/iio/ Did see warns after adding incorrect type in assignments in the driver. Mind sharing how you are running Sparse? Thanks, Marcelo
On Tue, 7 Jan 2025 08:29:36 -0300 Marcelo Schmitt <marcelo.schmitt1@gmail.com> wrote: > On 01/05, Jonathan Cameron wrote: > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > > > This complex cleanup.h use case of conditional guards has proved > > to be more trouble that it is worth in terms of false positive compiler > > warnings and hard to read code. > > > > Move directly to the new claim/release_direct() that allow sparse > > to check for unbalanced context. > > > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > --- > > drivers/iio/adc/ad4000.c | 61 +++++++++++++++++++++++++--------------- > > 1 file changed, 38 insertions(+), 23 deletions(-) > > > Hi Jonathan, aside from the spurious blank line noted by David, the changes for > ad4000 look good to me. > > Acked-by: <marcelo.schmitt1@gmail.com> > > I also tried running Sparse on IIO subsystem but didn't see any warns for the > drivers being changed (nor prior nor after applying the patches). > > make CHECK="path_to_local_sparse_v0.6.4-66-g0196afe1" C=2 drivers/iio/ > > Did see warns after adding incorrect type in assignments in the driver. > > Mind sharing how you are running Sparse? I just used C=1 but that doesn't really matter for this. With this series there should be no false positive warnings (or before it where we didn't have any markings so sparse didn't know to do anything). Testing wise, I sprinkled in some early returns, breaks etc to add some broken paths and those triggered context imbalance warnings. This isn't fixing warnings, it is just about moving to code where we will get them if we do something silly in the future. Jonathan > > Thanks, > Marcelo
On Tue, 7 Jan 2025 14:28:54 +0000 Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote: > On Tue, 7 Jan 2025 08:29:36 -0300 > Marcelo Schmitt <marcelo.schmitt1@gmail.com> wrote: > > > On 01/05, Jonathan Cameron wrote: > > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > > > > > This complex cleanup.h use case of conditional guards has proved > > > to be more trouble that it is worth in terms of false positive compiler > > > warnings and hard to read code. > > > > > > Move directly to the new claim/release_direct() that allow sparse > > > to check for unbalanced context. > > > > > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > > --- > > > drivers/iio/adc/ad4000.c | 61 +++++++++++++++++++++++++--------------- > > > 1 file changed, 38 insertions(+), 23 deletions(-) > > > > > Hi Jonathan, aside from the spurious blank line noted by David, the changes for > > ad4000 look good to me. > > > > Acked-by: <marcelo.schmitt1@gmail.com> > > > > I also tried running Sparse on IIO subsystem but didn't see any warns for the > > drivers being changed (nor prior nor after applying the patches). > > > > make CHECK="path_to_local_sparse_v0.6.4-66-g0196afe1" C=2 drivers/iio/ > > > > Did see warns after adding incorrect type in assignments in the driver. > > > > Mind sharing how you are running Sparse? > > I just used C=1 but that doesn't really matter for this. > With this series there should be no false positive warnings (or before > it where we didn't have any markings so sparse didn't know to do anything). > > Testing wise, I sprinkled in some early returns, breaks etc to add > some broken paths and those triggered context imbalance warnings. > > This isn't fixing warnings, it is just about moving to code where we > will get them if we do something silly in the future. Seems David is also not seeing warnings when he deliberately breaks the code. See discussion on patch 1. Hopefully we'll soon get to the bottom of why! Jonathan > > Jonathan > > > > > Thanks, > > Marcelo >
Hi Jonathan, Thanks for clarifying the intent of the series and for the hints on how to test the patches. I think we will need a v2. Didn't look through all drivers being updated but most of the ones I looked at had a bugy check of iio_device_claim_direct() return. Please see my comments to the cover letter. Thanks, Marcelo On 01/11, Jonathan Cameron wrote: > On Tue, 7 Jan 2025 14:28:54 +0000 > Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote: > > > On Tue, 7 Jan 2025 08:29:36 -0300 > > Marcelo Schmitt <marcelo.schmitt1@gmail.com> wrote: > > > > > On 01/05, Jonathan Cameron wrote: > > > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > > > > > > > This complex cleanup.h use case of conditional guards has proved > > > > to be more trouble that it is worth in terms of false positive compiler > > > > warnings and hard to read code. > > > > > > > > Move directly to the new claim/release_direct() that allow sparse > > > > to check for unbalanced context. > > > > > > > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > > > --- > > > > drivers/iio/adc/ad4000.c | 61 +++++++++++++++++++++++++--------------- > > > > 1 file changed, 38 insertions(+), 23 deletions(-) > > > > > > > Hi Jonathan, aside from the spurious blank line noted by David, the changes for > > > ad4000 look good to me. > > > > > > Acked-by: <marcelo.schmitt1@gmail.com> > > > > > > I also tried running Sparse on IIO subsystem but didn't see any warns for the > > > drivers being changed (nor prior nor after applying the patches). > > > > > > make CHECK="path_to_local_sparse_v0.6.4-66-g0196afe1" C=2 drivers/iio/ > > > > > > Did see warns after adding incorrect type in assignments in the driver. > > > > > > Mind sharing how you are running Sparse? > > > > I just used C=1 but that doesn't really matter for this. > > With this series there should be no false positive warnings (or before > > it where we didn't have any markings so sparse didn't know to do anything). > > > > Testing wise, I sprinkled in some early returns, breaks etc to add > > some broken paths and those triggered context imbalance warnings. > > > > This isn't fixing warnings, it is just about moving to code where we > > will get them if we do something silly in the future. > > Seems David is also not seeing warnings when he deliberately breaks > the code. See discussion on patch 1. Hopefully we'll soon get to the > bottom of why! > > Jonathan > > > > > Jonathan > > > > > > > > Thanks, > > > Marcelo > > >
Hi Jonathan, If you end up not changing this patch much, please consider taking my tags. On 01/05, Jonathan Cameron wrote: > From: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > This complex cleanup.h use case of conditional guards has proved > to be more trouble that it is worth in terms of false positive compiler > warnings and hard to read code. > > Move directly to the new claim/release_direct() that allow sparse > to check for unbalanced context. > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > --- Reviewed-by: Marcelo Schmitt <marcelo.schmitt@analog.com> Tested-by: Marcelo Schmitt <marcelo.schmitt@analog.com>
diff --git a/drivers/iio/adc/ad4000.c b/drivers/iio/adc/ad4000.c index 1d556a842a68..ef0acaafbcdb 100644 --- a/drivers/iio/adc/ad4000.c +++ b/drivers/iio/adc/ad4000.c @@ -535,12 +535,16 @@ static int ad4000_read_raw(struct iio_dev *indio_dev, int *val2, long info) { struct ad4000_state *st = iio_priv(indio_dev); + int ret; switch (info) { case IIO_CHAN_INFO_RAW: - iio_device_claim_direct_scoped(return -EBUSY, indio_dev) - return ad4000_single_conversion(indio_dev, chan, val); - unreachable(); + if (!iio_device_claim_direct(indio_dev)) + return -EBUSY; + + ret = ad4000_single_conversion(indio_dev, chan, val); + iio_device_release_direct(indio_dev); + return ret; case IIO_CHAN_INFO_SCALE: *val = st->scale_tbl[st->span_comp][0]; *val2 = st->scale_tbl[st->span_comp][1]; @@ -585,36 +589,47 @@ static int ad4000_write_raw_get_fmt(struct iio_dev *indio_dev, } } -static int ad4000_write_raw(struct iio_dev *indio_dev, - struct iio_chan_spec const *chan, int val, int val2, - long mask) +static int __ad4000_write_raw(struct iio_dev *indio_dev, + struct iio_chan_spec const *chan, + int val2) { struct ad4000_state *st = iio_priv(indio_dev); unsigned int reg_val; bool span_comp_en; int ret; - switch (mask) { - case IIO_CHAN_INFO_SCALE: - iio_device_claim_direct_scoped(return -EBUSY, indio_dev) { - guard(mutex)(&st->lock); + guard(mutex)(&st->lock); - ret = ad4000_read_reg(st, ®_val); - if (ret < 0) - return ret; + ret = ad4000_read_reg(st, ®_val); + if (ret < 0) + return ret; - span_comp_en = val2 == st->scale_tbl[1][1]; - reg_val &= ~AD4000_CFG_SPAN_COMP; - reg_val |= FIELD_PREP(AD4000_CFG_SPAN_COMP, span_comp_en); + span_comp_en = val2 == st->scale_tbl[1][1]; + reg_val &= ~AD4000_CFG_SPAN_COMP; + reg_val |= FIELD_PREP(AD4000_CFG_SPAN_COMP, span_comp_en); - ret = ad4000_write_reg(st, reg_val); - if (ret < 0) - return ret; + ret = ad4000_write_reg(st, reg_val); + if (ret < 0) + return ret; - st->span_comp = span_comp_en; - return 0; - } - unreachable(); + st->span_comp = span_comp_en; + return 0; +} + +static int ad4000_write_raw(struct iio_dev *indio_dev, + struct iio_chan_spec const *chan, + int val, int val2, long mask) +{ + int ret; + + switch (mask) { + case IIO_CHAN_INFO_SCALE: + + if (!iio_device_claim_direct(indio_dev)) + return -EBUSY; + ret = __ad4000_write_raw(indio_dev, chan, val2); + iio_device_release_direct(indio_dev); + return ret; default: return -EINVAL; }