linux-kernel - Re: [PATCH next 4/4] iio: sca3000: stop interrupts via devm_add_action_or

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20260131163909.7c90c9d7@jic23-huawei>
Date: Sat, 31 Jan 2026 16:39:09 +0000
From: Jonathan Cameron <jic23@...nel.org>
To: Harshit Mogalapalli <harshit.m.mogalapalli@...cle.com>
Cc: David Lechner <dlechner@...libre.com>, Nuno Sá
 <nuno.sa@...log.com>, Andy Shevchenko <andy@...nel.org>, Gustavo Bastos
 <gustavobastos@....br>, Andrew Ijano <andrew.ijano@...il.com>,
 linux-iio@...r.kernel.org (open list:IIO SUBSYSTEM AND DRIVERS),
 linux-kernel@...r.kernel.org (open list), kernel-janitors@...r.kernel.org,
 Andy Shevchenko <andriy.shevchenko@...el.com>
Subject: Re: [PATCH next 4/4] iio: sca3000: stop interrupts via
 devm_add_action_or_reset()

On Fri, 30 Jan 2026 13:43:11 -0800
Harshit Mogalapalli <harshit.m.mogalapalli@...cle.com> wrote:

> sca3000_stop_all_interrupts() is moved above the probe routine so the
> new function sca3000_disable_interrupts() used in probe can directly
> call it without additional declaration.
> 
> Used devm_add_action_or_reset() for shutting doing the interrupts. After
> this there is no need to have a remove call back, so got rid of the
> remove callback.
> 
> No functional change intended.
> 
> Suggested-by: Andy Shevchenko <andriy.shevchenko@...el.com>
> Signed-off-by: Harshit Mogalapalli <harshit.m.mogalapalli@...cle.com>

Will need an update if you indeed didn't intend to change order in previous
patch.  devm_iio_device_register() is last in vast majority of probe
functions because it opens us up to userspace interaction. We almost
always want to cut that off on remove before doing anything else.

> ---
>  drivers/iio/accel/sca3000.c | 58 ++++++++++++++++++++-----------------
>  1 file changed, 32 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/iio/accel/sca3000.c b/drivers/iio/accel/sca3000.c
> index bf1957c7bc77..7f7d29688a81 100644
> --- a/drivers/iio/accel/sca3000.c
> +++ b/drivers/iio/accel/sca3000.c
> @@ -1437,6 +1437,34 @@ static const struct iio_info sca3000_info = {
>  	.write_event_config = &sca3000_write_event_config,
>  };
>  
> +static int sca3000_stop_all_interrupts(struct sca3000_state *st)
> +{
> +	int ret;
> +
> +	mutex_lock(&st->lock);

Use guard(mutex)(&st->lock); to simplify this. Remember to include cleanup.h
A future patch could then make use of guard() more widely in this driver.

> +	ret = sca3000_read_data_short(st, SCA3000_REG_INT_MASK_ADDR, 1);
> +	if (ret)
> +		goto error_ret;

Blank line here.  Keeps each group of action / error check clearly delineated
if we have a blank line between them. Note this is a case of do as I say
not as I did nearly 17 years back when I wrote this (younger me did many
things wrong ;)

With guard() above, you can just do
	if (ret)
		return ret;

here.
> +	ret = sca3000_write_reg(st, SCA3000_REG_INT_MASK_ADDR,
> +				(st->rx[0] &
> +				 ~(SCA3000_REG_INT_MASK_RING_THREE_QUARTER |
> +				   SCA3000_REG_INT_MASK_RING_HALF |
> +				   SCA3000_REG_INT_MASK_ALL_INTS)));

With guard() above, this becomes
	return sca3000_write_reg();


> +error_ret:
> +	mutex_unlock(&st->lock);
> +	return ret;
> +}
> +
> +static void sca3000_disable_interrupts(void *data)
> +{
> +	struct iio_dev *indio_dev = data;
> +	struct sca3000_state *st = iio_priv(indio_dev);
> +
> +	/* Must ensure no interrupts can be generated after this! */
> +	sca3000_stop_all_interrupts(st);
> +}
> +
> +
>  static int sca3000_probe(struct spi_device *spi)
>  {
>  	const struct spi_device_id *id = spi_get_device_id(spi);
> @@ -1481,6 +1509,7 @@ static int sca3000_probe(struct spi_device *spi)
>  		if (ret)
>  			return ret;
>  	}
> +

Unrelated change.  Make sure to check for these in patches before
you send them out.  It adds noise and typically means you'll end
up doing another version even if everything else is good.

>  	ret = sca3000_clean_setup(st);
>  	if (ret)
>  		return ret;
> @@ -1489,34 +1518,12 @@ static int sca3000_probe(struct spi_device *spi)
>  	if (ret)
>  		return ret;
>  
> -	return devm_iio_device_register(dev, indio_dev);
> -}
> -
> -static int sca3000_stop_all_interrupts(struct sca3000_state *st)
> -{
> -	int ret;
> -
> -	mutex_lock(&st->lock);
> -	ret = sca3000_read_data_short(st, SCA3000_REG_INT_MASK_ADDR, 1);
> +	ret = devm_iio_device_register(dev, indio_dev);
>  	if (ret)
> -		goto error_ret;
> -	ret = sca3000_write_reg(st, SCA3000_REG_INT_MASK_ADDR,
> -				(st->rx[0] &
> -				 ~(SCA3000_REG_INT_MASK_RING_THREE_QUARTER |
> -				   SCA3000_REG_INT_MASK_RING_HALF |
> -				   SCA3000_REG_INT_MASK_ALL_INTS)));
> -error_ret:
> -	mutex_unlock(&st->lock);
> -	return ret;
> -}
> +		return ret;
>  
> -static void sca3000_remove(struct spi_device *spi)
> -{
> -	struct iio_dev *indio_dev = spi_get_drvdata(spi);
> -	struct sca3000_state *st = iio_priv(indio_dev);
> +	return devm_add_action_or_reset(dev, sca3000_disable_interrupts, indio_dev);
As mentioned above. This looks unlikely to be a good idea as a reorder of code.

I'd expect interfaces to go away and then interrupts to be stopped. In general
that should always be safe unless we have some racey bug somewhere in the IIO
core or the driver is doing something unusual.

Thanks,

Jonathan

>  
> -	/* Must ensure no interrupts can be generated after this! */
> -	sca3000_stop_all_interrupts(st);
>  }
>  
>  static const struct spi_device_id sca3000_id[] = {
> @@ -1533,7 +1540,6 @@ static struct spi_driver sca3000_driver = {
>  		.name = "sca3000",
>  	},
>  	.probe = sca3000_probe,
> -	.remove = sca3000_remove,
>  	.id_table = sca3000_id,
>  };
>  module_spi_driver(sca3000_driver);