Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
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,
        =?utf-8?Q?Bart=C5=82omiej_=C5=BBolnierkiewicz?= 
        <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
In-Reply-To: <YE/ZLtdK0ZlVFOhp@piout.net> (Alexandre Belloni's message of
        "Mon, 15 Mar 2021 23:01:18 +0100")
Message-ID: <dleftj1rcfe1rb.fsf%l.stelmach@samsung.com>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux)
MIME-Version: 1.0
Content-Type: multipart/signed; boundary="=-=-="; micalg="pgp-sha256";
        protocol="application/pgp-signature"
CMS-TYPE: 201P
References: <YE/ZLtdK0ZlVFOhp@piout.net>
        <20210305174411.9657-1-l.stelmach@samsung.com>
        <CGME20210316121218eucas1p1f74d6e6cec7fc897051902a7da478fd0@eucas1p1.samsung.com>
Precedence: bulk

--=-=-=
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable

It was <2021-03-15 pon 23:01>, when Alexandre Belloni wrote:
> Hello,
>
> On 05/03/2021 18:44:11+0100, =C5=81ukasz Stelmach wrote:
>> For an RTC without an IRQ assigned rtc_update_irq_enable() should
>> return -EINVAL.  It will, when uie_unsupported is set.
>>=20
>
> I'm surprised this is an issue because the current code seems to cover
> all cases:
>
>  - no irq and not wakeup-source =3D> set_alarm should fail
>  - no irq and wakeup-source =3D> uie_unsupported is set
>  - irq =3D> 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 =3D 0
    ds1307_can_wakeup_device =3D 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 =3D -EINVAL;
        goto out;
    }

and we keep going (uie_unsupported =3D 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=3Dv5.11#n1032
[2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree=
/drivers/rtc/rtc-ds1307.c?h=3Dv5.11#n1779
[3] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree=
/drivers/rtc/rtc-ds1307.c?h=3Dv5.11#n1802
[4] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree=
/drivers/rtc/rtc-ds1307.c?h=3Dv5.11#n1977
[5] https://git.kernel.org/pub/scm/utils/util-linux/util-linux.git/tree/sys=
-utils/hwclock-rtc.c?h=3Dv2.36.2#n252
[6] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree=
/drivers/rtc/interface.c?h=3Dv5.11#n564
[7] https://git.kernel.org/pub/scm/utils/util-linux/util-linux.git/tree/sys=
-utils/hwclock-rtc.c?h=3Dv2.36.2#n283
[8] https://git.kernel.org/pub/scm/utils/util-linux/util-linux.git/tree/sys=
-utils/hwclock-rtc.c?h=3Dv2.36.2#n297

>> Signed-off-by: =C5=81ukasz Stelmach <l.stelmach@samsung.com>
>> ---
>>  drivers/rtc/rtc-ds1307.c | 14 +++++++-------
>>  1 file changed, 7 insertions(+), 7 deletions(-)
>>=20
>> 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);
>>=20=20
>> -	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 =3D 1;
>> -	}
>> -
>>  	if (want_irq) {
>>  		err =3D 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 82e2d43f=
6315
>
>

Done.

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

=2D-=20
=C5=81ukasz Stelmach
Samsung R&D Institute Poland
Samsung Electronics

--=-=-=
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQEzBAEBCAAdFiEEXpuyqjq9kGEVr9UQsK4enJilgBAFAmBQoJkACgkQsK4enJil
gBDK3wf/RAOEvk8xZy90QMhxfRYU8XSE8UvPBaXXPOsJLFIcjbvUX5Q9s1rUrXT7
LPbKuG3lJDNFthL/wqDj9RNx+bmDK5bf2LWZ0YgfUMuJJc/Ruc2wvr2l6DTXiKp9
TmeTyC1e5j/VuiEsyJxQ7jss+l4Lp4QevUoVbLB+r6LAsxfbzfx4q77Ivispf+GX
B45XoUEPzr7FE3UinHY0Zs+L0sOg9ePYd8uHYYFWMiCUTfKzc4TQNrH7WohbNOwB
/8ocqA094cx0zTWJKnFjzwpIrFWtB/MYKAyvbMmrVlKic3tiHddB7alQWL7mOqn9
fCm79ml5dWTFwXrkK9GP7zpJHcYxZw==
=mr4q
-----END PGP SIGNATURE-----
--=-=-=--