Re: [PATCH] rtc: ds1307: set uie_unsupported if no interrupt is available

From: Lukasz Stelmach <l.stelmach@samsung.com>
To: Alexandre Belloni <alexandre.belloni@bootlin.com>
Cc: "Alessandro Zummo" <a.zummo@towertech.it>,
	linux-rtc@vger.kernel.org,
	"Bartłomiej Żolnierkiewicz" <b.zolnierkie@samsung.com>,
	"Marek Szyprowski" <m.szyprowski@samsung.com>,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH] rtc: ds1307: set uie_unsupported if no interrupt is available
Date: Tue, 16 Mar 2021 13:12:08 +0100	[thread overview]
Message-ID: <dleftj1rcfe1rb.fsf%l.stelmach@samsung.com> (raw)
In-Reply-To: <YE/ZLtdK0ZlVFOhp@piout.net> (Alexandre Belloni's message of "Mon, 15 Mar 2021 23:01:18 +0100")

[-- Attachment #1: Type: text/plain, Size: 4188 bytes --]

It was <2021-03-15 pon 23:01>, when Alexandre Belloni wrote:
> Hello,
>
> On 05/03/2021 18:44:11+0100, Łukasz Stelmach wrote:
>> For an RTC without an IRQ assigned rtc_update_irq_enable() should
>> return -EINVAL.  It will, when uie_unsupported is set.
>> 
>
> I'm surprised this is an issue because the current code seems to cover
> all cases:
>
>  - no irq and not wakeup-source => set_alarm should fail
>  - no irq and wakeup-source => uie_unsupported is set
>  - irq => UIE should work
>
> Can you elaborate on your failing use case?

I've got ds3231 which supports alarms[1] but is not connected to any
interrupt line. Hence, client->irq is 0 as well as want_irq[2]. There
is also no other indirect connection, so I don't set wakeup-source
property and ds1307_can_wakeup_device remains[3] false. Under these
conditions

    want_irq = 0
    ds1307_can_wakeup_device = false

uie_unsupported remains[4] false. And this is the problem.

hwclock(8) when setting system clock from rtc (--hctosys) calls
synchronize_to_clock_tick_rtc()[5]. There goes

    ioctl(rtc_fd, RTC_UIE_ON, 0);

which leads us to

    rtc_update_irq_enable(rtc, 1);

and finally here [6]

    if (rtc->uie_unsupported) {
        err = -EINVAL;
        goto out;
    }

and we keep going (uie_unsupported = 0). All the following operations
succeed because chip supports alarms.

We go back to hwclock(8) and we start waiting[7] for the update from
interrupt which never arrives instead of calling
busywiat_for_rtc_clock_tick()[8] (mind the invalid indentation) because
of EINVAL returned from ioctl() (conf. [6])

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/rtc/rtc-ds1307.c?h=v5.11#n1032
[2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/rtc/rtc-ds1307.c?h=v5.11#n1779
[3] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/rtc/rtc-ds1307.c?h=v5.11#n1802
[4] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/rtc/rtc-ds1307.c?h=v5.11#n1977
[5] https://git.kernel.org/pub/scm/utils/util-linux/util-linux.git/tree/sys-utils/hwclock-rtc.c?h=v2.36.2#n252
[6] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/rtc/interface.c?h=v5.11#n564
[7] https://git.kernel.org/pub/scm/utils/util-linux/util-linux.git/tree/sys-utils/hwclock-rtc.c?h=v2.36.2#n283
[8] https://git.kernel.org/pub/scm/utils/util-linux/util-linux.git/tree/sys-utils/hwclock-rtc.c?h=v2.36.2#n297

>> Signed-off-by: Łukasz Stelmach <l.stelmach@samsung.com>
>> ---
>>  drivers/rtc/rtc-ds1307.c | 14 +++++++-------
>>  1 file changed, 7 insertions(+), 7 deletions(-)
>> 
>> diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
>> index cd8e438bc9c4..b08a9736fa77 100644
>> --- a/drivers/rtc/rtc-ds1307.c
>> +++ b/drivers/rtc/rtc-ds1307.c
>> @@ -1973,13 +1973,6 @@ static int ds1307_probe(struct i2c_client *client,
>>  	if (IS_ERR(ds1307->rtc))
>>  		return PTR_ERR(ds1307->rtc);
>>  
>> -	if (ds1307_can_wakeup_device && !want_irq) {
>> -		dev_info(ds1307->dev,
>> -			 "'wakeup-source' is set, request for an IRQ is disabled!\n");
>> -		/* We cannot support UIE mode if we do not have an IRQ line */
>> -		ds1307->rtc->uie_unsupported = 1;
>> -	}
>> -
>>  	if (want_irq) {
>>  		err = devm_request_threaded_irq(ds1307->dev, client->irq, NULL,
>>  						chip->irq_handler ?: ds1307_irq,
>> @@ -1993,6 +1986,13 @@ static int ds1307_probe(struct i2c_client *client,
>>  		} else {
>>  			dev_dbg(ds1307->dev, "got IRQ %d\n", client->irq);
>>  		}
>> +	} else {
>> +		if (ds1307_can_wakeup_device)
>> +			dev_info(ds1307->dev,
>> +				 "'wakeup-source' is set, request for an IRQ is disabled!\n");
>> +
>
> Honestly, just drop this message, it should have been removed by 82e2d43f6315
>
>

Done.

>> +		/* We cannot support UIE mode if we do not have an IRQ line */
>> +		ds1307->rtc->uie_unsupported = 1;
>>  	}
>>  
>>  	ds1307->rtc->ops = chip->rtc_ops ?: &ds13xx_rtc_ops;
>> -- 
>> 2.26.2
>> 

-- 
Łukasz Stelmach
Samsung R&D Institute Poland
Samsung Electronics

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 487 bytes --]