linux-kernel - Re: [PATCH v8 3/4] iio: pressure: bmp280: Add data ready trigger support

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: <20241012170823.3c6d3df9@jic23-huawei>
Date: Sat, 12 Oct 2024 17:08:23 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Vasileios Amoiridis <vassilisamir@...il.com>
Cc: lars@...afoo.de, robh@...nel.org, krzk+dt@...nel.org,
 conor+dt@...nel.org, andriy.shevchenko@...ux.intel.com,
 ang.iglesiasg@...il.com, linus.walleij@...aro.org,
 biju.das.jz@...renesas.com, javier.carrasco.cruz@...il.com,
 semen.protsenko@...aro.org, 579lpy@...il.com, ak@...klinger.de,
 linux-iio@...r.kernel.org, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org, christophe.jaillet@...adoo.fr
Subject: Re: [PATCH v8 3/4] iio: pressure: bmp280: Add data ready trigger
 support

On Mon,  7 Oct 2024 21:49:44 +0200
Vasileios Amoiridis <vassilisamir@...il.com> wrote:

> The BMP3xx and BMP5xx sensors have an interrupt pin which can be used as
> a trigger for when there are data ready in the sensor for pick up.
> 
> This use case is used along with NORMAL_MODE in the sensor, which allows
> the sensor to do consecutive measurements depending on the ODR rate value.
> 
> The trigger pin can be configured to be open-drain or push-pull and either
> rising or falling edge.
> 
> No support is added yet for interrupts for FIFO, WATERMARK and out of range
> values.
> 
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
> Signed-off-by: Vasileios Amoiridis <vassilisamir@...il.com>

Hi Vasileios,

One questing about locking below.  What you have is probably correct
but might be tighter than it needs to be, or need a comment to say why
for future readers.

I hate register reads with side effects btw.  It's an 'optimization'
hardware designers thing is nice, but makes for really ugly software
interfaces.

> @@ -2429,6 +2564,88 @@ static int bmp580_chip_config(struct bmp280_data *data)
>  	return 0;
>  }
>  
> +static void bmp580_trigger_reenable(struct iio_trigger *trig)
> +{
> +	struct bmp280_data *data = iio_trigger_get_drvdata(trig);
> +	unsigned int tmp;
> +	int ret;
> +
> +	ret = regmap_read(data->regmap, BMP580_REG_INT_STATUS, &tmp);
As below. Seems this read has side effects (horrible!)
I'm not sure if this is related to the locking though.
> +	if (ret)
> +		dev_err(data->dev, "Failed to reset interrupt.\n");
> +}

> +static int bmp580_int_pin_config(struct bmp280_data *data)
> +{
> +	int pin_drive_cfg = FIELD_PREP(BMP580_INT_CONFIG_OPEN_DRAIN,
> +				       data->trig_open_drain);
> +	int pin_level_cfg = FIELD_PREP(BMP580_INT_CONFIG_LEVEL,
> +				       data->trig_active_high);
> +	int ret, int_pin_cfg = pin_drive_cfg | pin_level_cfg;
	int int_pin_cfg = pin...
	int ret;

Is easier to follow.

> +
> +	ret = regmap_update_bits(data->regmap, BMP580_REG_INT_CONFIG,
> +				 BMP580_INT_CONFIG_MASK, int_pin_cfg);
> +	if (ret) {
> +		dev_err(data->dev, "Could not set interrupt settings.\n");
> +		return ret;
> +	}
> +
> +	ret = regmap_set_bits(data->regmap, BMP580_REG_INT_SOURCE,
> +			      BMP580_INT_SOURCE_DRDY);
> +	if (ret)
> +		dev_err(data->dev, "Could not set interrupt source.\n");
> +
> +	return ret;
> +}
> +
> +static irqreturn_t bmp580_irq_thread_handler(int irq, void *p)
> +{
> +	struct iio_dev *indio_dev = p;
> +	struct bmp280_data *data = iio_priv(indio_dev);
> +	unsigned int int_ctrl;
> +	int ret;
> +
> +	scoped_guard(mutex, &data->lock) {
> +		ret = regmap_read(data->regmap, BMP580_REG_INT_STATUS, &int_ctrl);
What are you locking against here?  Seems this read may have side effects?
If not the regmap internal locking should be enough for a register read.
> +		if (ret)
> +			return IRQ_NONE;
> +	}