diff mbox series

[RFC,09/27] iio: adc: ad4000: Stop using iio_device_claim_direct_scoped()

Message ID 20250105172613.1204781-10-jic23@kernel.org (mailing list archive)
State New
Headers show
Series iio: improve handling of direct mode claim and release | expand

Commit Message

Jonathan Cameron Jan. 5, 2025, 5:25 p.m. UTC 
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(-)

Comments

David Lechner Jan. 6, 2025, 11:19 p.m. UTC | #1 
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;
>  	}
Marcelo Schmitt Jan. 7, 2025, 11:29 a.m. UTC | #2 
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
Jonathan Cameron Jan. 7, 2025, 2:28 p.m. UTC | #3 
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
diff mbox series

Patch

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, &reg_val);
-			if (ret < 0)
-				return ret;
+	ret = ad4000_read_reg(st, &reg_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;
 	}