Re: [PATCH v2 6/9] iio: adc: rzt2h: expose sampling frequency

[Jonathan Cameron]

> Expose the sampling frequency as a per-channel IIO_CHAN_INFO_SAMP_FREQ
> to let userspace control conversion time.
> 
> Each channel conversion takes a fixed 13 ADCLK cycles plus the sample
> time programmed in ADSSTRn, giving a rate of ADCLK / (13 + ADSSTRn).
> 
> Read the ADCLK rate from the "adclk" clock to derive the frequency.
> 
> Claim direct mode while writing so the rate cannot change during a
> capture.
> 
> Program the sample time into ADSSTRn for each enabled channel on single
> reads and on buffer enable.
> 
> Wait for the maximum amount of time a conversion can take + 1 jiffy for
> the completion event to come after triggering a single read.
> 
> Signed-off-by: Cosmin Tanislav <cosmin-gabriel.tanislav.xa@xxxxxxxxxxx>

There is a sashiko comment on this one (See below) but I think it's
in the category of things that we don't are about if a timeout
has occured.  Please sanity check but I'll not hold series for that
one.

>
> diff --git a/drivers/iio/adc/rzt2h_adc.c b/drivers/iio/adc/rzt2h_adc.c
> index 0460dffe189e..8ba7b0fde8a1 100644
> --- a/drivers/iio/adc/rzt2h_adc.c
> +++ b/drivers/iio/adc/rzt2h_adc.c
> @@ -2,6 +2,7 @@
>  
>  #include <linux/bitfield.h>
>  #include <linux/cleanup.h>
> +#include <linux/clk.h>
>  #include <linux/completion.h>
>  #include <linux/delay.h>
>  #include <linux/dma-mapping.h>

..

> -	ret = wait_for_completion_timeout(&adc->completion, usecs_to_jiffies(1));
> +	ret = wait_for_completion_timeout(&adc->completion, usecs_to_jiffies(5) + 1);
>  	if (!ret) {
>  		ret = -ETIMEDOUT;
>  		goto disable;

Sashiko:
[Severity: High]
This is a pre-existing issue, but does branching to the disable path on a 
timeout leave the driver vulnerable to a race with the interrupt handler?

If the timeout expires and the hardware interrupt fires immediately after,
the ISR can execute locklessly and call complete(&adc->completion).

Since the disable path calls rzt2h_adc_stop(), which only masks the interrupt
at the device level without waiting for executing handlers via 
synchronize_irq(), could the delayed interrupt complete the structure after
the next read operation calls reinit_completion() in rzt2h_adc_read_single()?

If this sequence occurs, the next read could return immediately with stale
data, and its own triggered conversion could eventually interrupt and corrupt
the subsequent read, leading to a persistent desynchronization.
-
With a timeout we never expect to see I don't think it makes sense
to care too much about stale data.

Jonathan

-- 
Jonathan Cameron <jonathan.cameron@xxxxxxxxxxxxxxxx>