Re: [PATCH v3 4/5] iio: light: vcnl4000: Add IRQ disable callback on cleanup

[Jonathan Cameron]

> During driver unbind, devm cleans up resources in LIFO order. The IRQ
> handler is freed before the device is powered down. This can lead to
> unhandled interrupts.

Add more on which interrupts and whether they are race conditions
like maybe happen for the buffer tear down, or this is actually
disabling events (as the core can't do that).


> 
> Register a devm action to disable interrupt explicitly after
> devm_request_threaded_irq(), so that the interrupt is disabled before
> the IRQ handler is freed. This prevents any unhandled interrupts.
> 
> Add a disable_irq callback to chip info structure. This allows each chip
> type to implement its own disable sequence.
> 
> Signed-off-by: Tsz Shan Chan <tchan@xxxxxxxxxxxxxx>

There are various bits of this that look rather incorrect. Please
take another close look at this one.

Given we've had a lot of races around event disabling and device tear
down recently it may be me need to look at adding a suitable hook
in the core to turn them off.  Anyhow, that is a job for another day.

>
> diff --git a/drivers/iio/light/vcnl4000.c b/drivers/iio/light/vcnl4000.c
> index 9f5a9dc6f87e..82ce940d3bef 100644
> --- a/drivers/iio/light/vcnl4000.c
> +++ b/drivers/iio/light/vcnl4000.c
> @@ -218,6 +218,7 @@ struct vcnl4000_chip_spec {
>  	int (*measure_light)(struct vcnl4000_data *data, int *val);
>  	int (*measure_proximity)(struct vcnl4000_data *data, int *val);
>  	int (*set_power_state)(struct vcnl4000_data *data, bool on);
> +	int (*disable_irq)(struct vcnl4000_data *data);
>  	irqreturn_t (*irq_thread)(int irq, void *priv);
>  	irqreturn_t (*trig_buffer_func)(int irq, void *priv);
>  
> @@ -1467,6 +1468,48 @@ static int vcnl4040_write_event_config(struct iio_dev *indio_dev,
>  	}
>  }
>  
> +static int vcnl4010_disable_irq(struct vcnl4000_data *data)
> +{
> +	int ret;
> +
> +	guard(mutex)(&data->vcnl4000_lock);
> +
> +	ret = vcnl4010_stop(data);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = i2c_smbus_read_byte_data(data->client, VCNL4010_ISR);

Does this regsiter have anything to do with disabling the interrupt?
I'd expect that to be somewhere in INT_CTRL.

> +	if (ret < 0)
> +		return ret;
> +
> +	ret &= VCNL4010_INT_THR | VCNL4010_INT_DRDY;

This bothers me. Data ready should be turned off by the buffer disable
that happens when the device unregiser occurs. Is that not happening
for some reason.  The threshold is more interesting here but I'd
generally prefer this code to be turning the event off (side effect
being to disable that interrupt).  So
vcln4010_config_threshold_disable() should be doing the job - possibly
via vcl4010_config_threshold() (I haven't checked locking etc).

> +	if (!ret)
> +		return 0;
> +
> +	return i2c_smbus_write_byte_data(data->client, VCNL4010_ISR, ret);
> +}
> +
> +static int vcnl4040_disable_irq(struct vcnl4000_data *data)
> +{
> +	int ret;
> +
> +	guard(mutex)(&data->vcnl4000_lock);
> +
> +	ret = vcnl4040_update_ps_int(data, VCNL4040_PS_CONF2_PS_INT, false);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = vcnl4040_update_als_int(data, VCNL4040_ALS_CONF_INT_EN, false);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = i2c_smbus_read_word_data(data->client, data->chip_spec->int_reg);

What is this doing? Just to check, is this stuff LLM assisted? That
read is random enough I'd not really expect to see it human generated
code.

-- 
Jonathan Cameron <jonathan.cameron@xxxxxxxxxxxxxxxx>