Re: [PATCH v2 1/1] iio: adc: tsc2046: rework the trigger state machine

From: Jonathan Cameron
Date: Sat Jan 15 2022 - 13:30:04 EST

Next message: Jonathan Cameron: "Re: [PATCH v2] iio: addac: Do not reference negative array offsets"
Previous message: Jonathan Cameron: "Re: [PATCH v2 1/1] iio: adc: tsc2046: add .read_raw support"
In reply to: Oleksij Rempel: "[PATCH v2 1/1] iio: adc: tsc2046: rework the trigger state machine"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Tue, 11 Jan 2022 14:04:02 +0100
Oleksij Rempel <o.rempel@xxxxxxxxxxxxxx> wrote:

> Initially this was designed to:
> | Fix sleeping in atomic context warning and a deadlock after iio_trigger_poll()
> | call
> |
> | If iio_trigger_poll() is called after IRQ was disabled, we will call
> | reenable_trigger() directly from hard IRQ or hrtimer context instead of
> | IRQ thread. In this case we will run in to multiple issue as sleeping in atomic
> | context and a deadlock.
> |
> | To avoid this issue, rework the trigger to use state machine. All state
> | changes are done over the hrtimer, so it allows us to drop fsleep() and
> | avoid the deadlock.
>
> Since this issue was fixed by: 9020ef659885 ("iio: trigger: Fix a scheduling
> whilst atomic issue seen on tsc2046"). This patch is a cleanup to make
> state machine easier to follow.
>
> Signed-off-by: Oleksij Rempel <o.rempel@xxxxxxxxxxxxxx>
One trivial thing inline that I've tidied up whilst applying.

Applied to the togreg branch of iio.git and pushed out as testing to let 0-day
poke at it.

Note I'll be rebaseing when rc1 is available and won't push out for linux-next
until after that.

Thanks,

Jonathan

> ---
> drivers/iio/adc/ti-tsc2046.c | 144 ++++++++++++++++++++++++-----------
> 1 file changed, 98 insertions(+), 46 deletions(-)
>
> diff --git a/drivers/iio/adc/ti-tsc2046.c b/drivers/iio/adc/ti-tsc2046.c
> index e8fc4d01f30b..c373da11bff1 100644
> --- a/drivers/iio/adc/ti-tsc2046.c
> +++ b/drivers/iio/adc/ti-tsc2046.c
> @@ -82,6 +82,10 @@
> #define TI_TSC2046_DATA_12BIT GENMASK(14, 3)
>
> #define TI_TSC2046_MAX_CHAN 8
> +#define TI_TSC2046_MIN_POLL_CNT 3
> +#define TI_TSC2046_EXT_POLL_CNT 3
> +#define TI_TSC2046_POLL_CNT \
> + (TI_TSC2046_MIN_POLL_CNT + TI_TSC2046_EXT_POLL_CNT)
>
> /* Represents a HW sample */
> struct tsc2046_adc_atom {
> @@ -123,14 +127,23 @@ struct tsc2046_adc_ch_cfg {
> unsigned int oversampling_ratio;
> };
>
> +enum tsc2046_state {
> + TSC2046_STATE_SHUTDOWN,
> + TSC2046_STATE_STANDBY,
> + TSC2046_STATE_POLL,
> + TSC2046_STATE_POLL_IRQ_DISABLE,
> + TSC2046_STATE_ENABLE_IRQ,
> +};
> +
> struct tsc2046_adc_priv {
> struct spi_device *spi;
> const struct tsc2046_adc_dcfg *dcfg;
>
> struct iio_trigger *trig;
> struct hrtimer trig_timer;
> - spinlock_t trig_lock;
> - unsigned int trig_more_count;
> + enum tsc2046_state state;
> + int poll_cnt;
> + spinlock_t state_lock;
>
> struct spi_transfer xfer;
> struct spi_message msg;
> @@ -411,21 +424,62 @@ static const struct iio_info tsc2046_adc_info = {
> .update_scan_mode = tsc2046_adc_update_scan_mode,
> };
>
> -static enum hrtimer_restart tsc2046_adc_trig_more(struct hrtimer *hrtimer)
> +static enum hrtimer_restart tsc2046_adc_timer(struct hrtimer *hrtimer)
> {
> struct tsc2046_adc_priv *priv = container_of(hrtimer,
> struct tsc2046_adc_priv,
> trig_timer);
> unsigned long flags;
>
> - spin_lock_irqsave(&priv->trig_lock, flags);
> -
> - disable_irq_nosync(priv->spi->irq);
> -
> - priv->trig_more_count++;
> - iio_trigger_poll(priv->trig);
> + /* This state machine should address following challenges :

Nitpick that I can fix whilst applying if there is nothing else needing a v3.

/*
* This state

> + * - the interrupt source is based on level shifter attached to the X
> + * channel of ADC. It will change the state every time we switch
> + * between channels. So, we need to disable IRQ if we do
> + * iio_trigger_poll().
> + * - we should do iio_trigger_poll() at some reduced sample rate
> + * - we should still trigger for some amount of time after last
> + * interrupt with enabled IRQ was processed.
> + */
>
> - spin_unlock_irqrestore(&priv->trig_lock, flags);
> + spin_lock_irqsave(&priv->state_lock, flags);
> + switch (priv->state) {
> + case TSC2046_STATE_ENABLE_IRQ:
> + if (priv->poll_cnt < TI_TSC2046_POLL_CNT) {
> + priv->poll_cnt++;
> + hrtimer_start(&priv->trig_timer,
> + ns_to_ktime(priv->scan_interval_us *
> + NSEC_PER_USEC),
> + HRTIMER_MODE_REL_SOFT);
> +
> + if (priv->poll_cnt >= TI_TSC2046_MIN_POLL_CNT) {
> + priv->state = TSC2046_STATE_POLL_IRQ_DISABLE;
> + enable_irq(priv->spi->irq);
> + } else {
> + priv->state = TSC2046_STATE_POLL;
> + }
> + } else {
> + priv->state = TSC2046_STATE_STANDBY;
> + enable_irq(priv->spi->irq);
> + }
> + break;
> + case TSC2046_STATE_POLL_IRQ_DISABLE:
> + disable_irq_nosync(priv->spi->irq);
> + fallthrough;
> + case TSC2046_STATE_POLL:
> + priv->state = TSC2046_STATE_ENABLE_IRQ;
> + /* iio_trigger_poll() starts hrtimer */
> + iio_trigger_poll(priv->trig);
> + break;
> + case TSC2046_STATE_SHUTDOWN:
> + break;
> + case TSC2046_STATE_STANDBY:
> + fallthrough;
> + default:
> + dev_warn(&priv->spi->dev, "Got unexpected state: %i\n",
> + priv->state);
> + break;
> + }
> + spin_unlock_irqrestore(&priv->state_lock, flags);
>
> return HRTIMER_NORESTART;
> }
> @@ -434,16 +488,20 @@ static irqreturn_t tsc2046_adc_irq(int irq, void *dev_id)
> {
> struct iio_dev *indio_dev = dev_id;
> struct tsc2046_adc_priv *priv = iio_priv(indio_dev);
> -
> - spin_lock(&priv->trig_lock);
> + unsigned long flags;
>
> hrtimer_try_to_cancel(&priv->trig_timer);
>
> - priv->trig_more_count = 0;
> - disable_irq_nosync(priv->spi->irq);
> - iio_trigger_poll(priv->trig);
> + spin_lock_irqsave(&priv->state_lock, flags);
> + if (priv->state != TSC2046_STATE_SHUTDOWN) {
> + priv->state = TSC2046_STATE_ENABLE_IRQ;
> + priv->poll_cnt = 0;
>
> - spin_unlock(&priv->trig_lock);
> + /* iio_trigger_poll() starts hrtimer */
> + disable_irq_nosync(priv->spi->irq);
> + iio_trigger_poll(priv->trig);
> + }
> + spin_unlock_irqrestore(&priv->state_lock, flags);
>
> return IRQ_HANDLED;
> }
> @@ -452,49 +510,42 @@ static void tsc2046_adc_reenable_trigger(struct iio_trigger *trig)
> {
> struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
> struct tsc2046_adc_priv *priv = iio_priv(indio_dev);
> - unsigned long flags;
> - int delta;
> + ktime_t tim;
>
> /*
> * We can sample it as fast as we can, but usually we do not need so
> * many samples. Reduce the sample rate for default (touchscreen) use
> * case.
> - * Currently we do not need a highly precise sample rate. It is enough
> - * to have calculated numbers.
> - */
> - delta = priv->scan_interval_us - priv->time_per_scan_us;
> - if (delta > 0)
> - fsleep(delta);
> -
> - spin_lock_irqsave(&priv->trig_lock, flags);
> -
> - /*
> - * We need to trigger at least one extra sample to detect state
> - * difference on ADC side.
> */
> - if (!priv->trig_more_count) {
> - int timeout_ms = DIV_ROUND_UP(priv->scan_interval_us,
> - USEC_PER_MSEC);
> -
> - hrtimer_start(&priv->trig_timer, ms_to_ktime(timeout_ms),
> - HRTIMER_MODE_REL_SOFT);
> - }
> -
> - enable_irq(priv->spi->irq);
> -
> - spin_unlock_irqrestore(&priv->trig_lock, flags);
> + tim = ns_to_ktime((priv->scan_interval_us - priv->time_per_scan_us) *
> + NSEC_PER_USEC);
> + hrtimer_start(&priv->trig_timer, tim, HRTIMER_MODE_REL_SOFT);
> }
>
> static int tsc2046_adc_set_trigger_state(struct iio_trigger *trig, bool enable)
> {
> struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
> struct tsc2046_adc_priv *priv = iio_priv(indio_dev);
> + unsigned long flags;
>
> if (enable) {
> - enable_irq(priv->spi->irq);
> + spin_lock_irqsave(&priv->state_lock, flags);
> + if (priv->state == TSC2046_STATE_SHUTDOWN) {
> + priv->state = TSC2046_STATE_STANDBY;
> + enable_irq(priv->spi->irq);
> + }
> + spin_unlock_irqrestore(&priv->state_lock, flags);
> } else {
> - disable_irq(priv->spi->irq);
> - hrtimer_try_to_cancel(&priv->trig_timer);
> + spin_lock_irqsave(&priv->state_lock, flags);
> +
> + if (priv->state == TSC2046_STATE_STANDBY ||
> + priv->state == TSC2046_STATE_POLL_IRQ_DISABLE)
> + disable_irq_nosync(priv->spi->irq);
> +
> + priv->state = TSC2046_STATE_SHUTDOWN;
> + spin_unlock_irqrestore(&priv->state_lock, flags);
> +
> + hrtimer_cancel(&priv->trig_timer);
> }
>
> return 0;
> @@ -668,10 +719,11 @@ static int tsc2046_adc_probe(struct spi_device *spi)
> iio_trigger_set_drvdata(trig, indio_dev);
> trig->ops = &tsc2046_adc_trigger_ops;
>
> - spin_lock_init(&priv->trig_lock);
> + spin_lock_init(&priv->state_lock);
> + priv->state = TSC2046_STATE_SHUTDOWN;
> hrtimer_init(&priv->trig_timer, CLOCK_MONOTONIC,
> HRTIMER_MODE_REL_SOFT);
> - priv->trig_timer.function = tsc2046_adc_trig_more;
> + priv->trig_timer.function = tsc2046_adc_timer;
>
> ret = devm_iio_trigger_register(dev, trig);
> if (ret) {

Next message: Jonathan Cameron: "Re: [PATCH v2] iio: addac: Do not reference negative array offsets"
Previous message: Jonathan Cameron: "Re: [PATCH v2 1/1] iio: adc: tsc2046: add .read_raw support"
In reply to: Oleksij Rempel: "[PATCH v2 1/1] iio: adc: tsc2046: rework the trigger state machine"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]