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
In-Reply-To: <YFClVBShBuy9/VPY@piout.net> (Alexandre Belloni's message of
        "Tue, 16 Mar 2021 13:32:20 +0100")
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux)
Date:   Tue, 16 Mar 2021 19:04:14 +0100
Message-ID: <dleftjblbjc6w1.fsf%l.stelmach@samsung.com>
MIME-Version: 1.0
Content-Type: multipart/signed; boundary="=-=-="; micalg="pgp-sha256";
        protocol="application/pgp-signature"
CMS-TYPE: 201P
References: <YFClVBShBuy9/VPY@piout.net>
        <CGME20210316180430eucas1p1a3cb26f40772b0af75782ff84908d731@eucas1p1.samsung.com>
Precedence: bulk

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

It was <2021-03-16 wto 13:32>, when Alexandre Belloni wrote:
> On 16/03/2021 13:12:08+0100, Lukasz Stelmach wrote:
>> 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?
>>=20
>> 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
>>=20
>>     want_irq =3D 0
>>     ds1307_can_wakeup_device =3D false
>>=20
>> uie_unsupported remains[4] false. And this is the problem.
>>=20
>> hwclock(8) when setting system clock from rtc (--hctosys) calls
>> synchronize_to_clock_tick_rtc()[5]. There goes
>>=20
>>     ioctl(rtc_fd, RTC_UIE_ON, 0);
>>=20
>> which leads us to
>>=20
>>     rtc_update_irq_enable(rtc, 1);
>>=20
>> and finally here [6]
>>=20
>>     if (rtc->uie_unsupported) {
>>         err =3D -EINVAL;
>>         goto out;
>>     }
>>=20
>> and we keep going (uie_unsupported =3D 0). All the following operations
>> succeed because chip supports alarms.
>>=20
>
> But then, HAS_ALARM is not set and ds1337_set_alarm should fail which
> makes rtc_timer_enqueue return an error. I admit this whole part is a
> mess, I'm just trying to understand how you can hit that.

OK, you are right. The problem seems to be elsewhere.

How about this scnario? We call rtc_update_irq_enable(). We read rtc
with __rtc_read_time() and calculate the alarm time. We get through
rtc_timer_enqueue() and down to __rtc_set_alarm(). We loose the race
condition (I can do it, I've got really slow connection to DS3231) and
we return -ETIME from __rtc_set_alarm()

    if (scheduled <=3D now)
        return -ETIME;

and 0 from rtc_timer_enqueue() and the very same zero from
rtc_update_irq_enable(). The caller of ioctl() thinks they can expect
interrupts when, in fact, they won't receive any.

The really weird stuff happens in rtc_timer_do_work(). For the timer to
be dequeued __rtc_set_alarm() needs to return EINVAL three times in a
row. In my setup this doesn't happen and the code keeps running loops
around "reporogram" and "again" labels.

With my patch we never risk the above race condition between
__rtc_read_time() in rtc_update_irq_enable() and the one in
__rtc_set_alarm(), because we know rtc doesn't support alarms before we
start the race. In fact there is another race between __rtc_read_time()
and actually setting the alarm in the chip.

IMHO the solution is to introduce RTC_HAS_ALARM flag for struct
rtc_device and check it at the very beginning of __rtc_set_alarm() the
same way it is being done in ds1337_set_alarm(). What are your thoughts?

>> 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])
>>=20
>> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/t=
ree/drivers/rtc/rtc-ds1307.c?h=3Dv5.11#n1032
>> [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/t=
ree/drivers/rtc/rtc-ds1307.c?h=3Dv5.11#n1779
>> [3] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/t=
ree/drivers/rtc/rtc-ds1307.c?h=3Dv5.11#n1802
>> [4] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/t=
ree/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/t=
ree/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
>>=20
>> >> 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 *cli=
ent,
>> >>  	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 *cli=
ent,
>> >>  		} 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 82e2d=
43f6315
>> >
>> >
>>=20
>> Done.
>>=20
>> >> +		/* 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
>>=20
>> --=20
>> =C5=81ukasz Stelmach
>> Samsung R&D Institute Poland
>> Samsung Electronics

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

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

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

iQEzBAEBCAAdFiEEXpuyqjq9kGEVr9UQsK4enJilgBAFAmBQ8x4ACgkQsK4enJil
gBBFIAgArjbHvHF9KbZdxXnfsTvd4NmsJmsLJSNlRv6IXEsB7pONKcbjuRiy/EVj
sozBhBrDkrWGjijFrGN10L+EZ1GElza9bGYMVrKNVBip2qt5ye1g3eNyg+V6b8kZ
cw4NH2Mi5RhNkO4RvbLsbcHDp2XKcnPbDi5fUQnUol5kWfKttrQidjVe+7AKP/uo
aus6SodvFUNjOE82mGj507/y4pb1KEv14Z5YV3C8ck3mP11GpYwZN/ff4NNPtsBv
begwjmBeMEjmCUfMG7nigrsRHnAP2ix9Q4vpgv9E2qHjU6+WJX4BMU070rbxLKZ9
DKlO4Xtk/8jkj//joUTyGzJwl73MnA==
=x9vy
-----END PGP SIGNATURE-----
--=-=-=--