Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH 1/2] iio: adc: at91: fix acking DRDY irq on simple
 conversions
To:     Jonathan Cameron <jic23@kernel.org>
CC:     <ludovic.desroches@microchip.com>,
        <linux-arm-kernel@lists.infradead.org>,
        <linux-kernel@vger.kernel.org>, <linux-iio@vger.kernel.org>,
        <nicolas.ferre@microchip.com>,
        Maxime Ripard <maxime.ripard@bootlin.com>,
        <stable@vger.kernel.org>
References: <1537447238-18674-1-git-send-email-eugen.hristev@microchip.com>
 <20180922113123.2c8da044@archlinux>
From:   Eugen Hristev <eugen.hristev@microchip.com>
Message-ID: <9846e106-8baf-b491-29ca-8306be9527ee@microchip.com>
Date:   Mon, 24 Sep 2018 09:19:43 +0300
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.9.1
MIME-Version: 1.0
In-Reply-To: <20180922113123.2c8da044@archlinux>
Content-Type: text/plain; charset="utf-8"; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


On 22.09.2018 13:31, Jonathan Cameron wrote:
> On Thu, 20 Sep 2018 15:40:37 +0300
> Eugen Hristev <eugen.hristev@microchip.com> wrote:
> 
>> When doing simple conversions, the driver did not acknowledge the DRDY irq.
>> If this irq is not acked, it will be left pending, and as soon as a trigger
>> is enabled, the irq handler will be called, it doesn't know why this irq
>> has occurred because no channel is pending, and then we will have irq loop
>> and board will hang.
>>
>> Fixes 0e589d5fb ("ARM: AT91: IIO: Add AT91 ADC driver.")
>> Cc: Maxime Ripard <maxime.ripard@bootlin.com>
>> Cc: <stable@vger.kernel.org>
>> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
>> ---
>>   drivers/iio/adc/at91_adc.c | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/iio/adc/at91_adc.c b/drivers/iio/adc/at91_adc.c
>> index 44b5168..e85f859 100644
>> --- a/drivers/iio/adc/at91_adc.c
>> +++ b/drivers/iio/adc/at91_adc.c
>> @@ -712,6 +712,11 @@ static int at91_adc_read_raw(struct iio_dev *idev,
>>   		at91_adc_writel(st, AT91_ADC_CHDR,
>>   				AT91_ADC_CH(chan->channel));
>>   		at91_adc_writel(st, AT91_ADC_IDR, BIT(chan->channel));
>> +		/*
>> +		 * we need to ack the DRDY irq, otherwise it will be
>> +		 * left pending and irq handler will be confused
>> +		 */
>> +		at91_adc_readl(st, AT91_ADC_LCDR);
> 
> I'm curious as to how things were working before.  Does this only occur
> if we do a raw_read whilst the buffer is enabled?

No. The situation is that the read raw does not properly cleans itself 
up after it's done.
The DRDY bit is cleared only when LCDR (last converted data ) is being read.
Even if we read the per channel conversion data, the LCDR still needs to 
be read to clear this irq status.
The driver does not use the DRDY irq but this irq status is still being 
set in the status register.

> 
> I would have assumed when it's not enabled, the irq would be masked and
> never generated in the first place...
> 
> It may be that what we actually need to do is to prevent read_raw accesses
> when the buffer is enabled (like lots of other drivers do precisely to
> avoid this sort of condition). The problem there comes if we have
> existing applications doing this combination as we are then breaking
> userspace. If that's the case we'll need to be a bit more clever.
> 
> Hammering down an irq state in a non irq handling path isn't a good
> solution.

Ok, I will move the clearing of the DRDY (LCDR read) in the irq path 
then, and send a v2.


> 
> Jonathan
> 
>>   
>>   		st->last_value = 0;
>>   		st->done = false;
>