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, 30 Mar 2021 08:52:08 +0200
In-Reply-To: <YGJqfVFVb7Z1LBTu@piout.net> (Alexandre Belloni's message of
        "Tue, 30 Mar 2021 02:02:05 +0200")
Message-ID: <dleftj8s65gmlj.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: <YGJqfVFVb7Z1LBTu@piout.net>
        <CGME20210330065217eucas1p2c50a100bf0101e72dd753f37257f6a57@eucas1p2.samsung.com>
Precedence: bulk

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

It was <2021-03-30 wto 02:02>, when Alexandre Belloni wrote:
> On 16/03/2021 19:04:14+0100, Lukasz Stelmach wrote:
>> OK, you are right. The problem seems to be elsewhere.
>>=20
>> 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()
>>=20
>>     if (scheduled <=3D now)
>>         return -ETIME;
>>=20
>> 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.
>>=20
>> 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.
>>=20
>> 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.
>>=20
>> 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?
>>=20
>
> I did introduce RTC_FEATURE_ALARM for that in v5.12. I'm sending patches
> that are not well tested but should solve your issue.

Oh, I didn't see that one coming (-; I was working on a slightly larger
feature elsewhere in the tree and didn't rebase too often. I will test
the patches.

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

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

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

iQEzBAEBCAAdFiEEXpuyqjq9kGEVr9UQsK4enJilgBAFAmBiypgACgkQsK4enJil
gBDwvggAil1Ff0dCZuWdYkRAm05OuqXUM9ki+5t+wJOpnNeL+ukrHagoKTMQt7um
LiqYV+QLk+4YMkgdQ0NrIFTOoXBk4k0Vbj333/vTEk30RK1qVLTHAkN4mHi2v1LW
Te/5h64p3U+DZWDkmniLWNmhJqjoXpohytmbBo5BDQ75+ZttkaN2KZHqdIDSxs41
RpWMNDn3qnpv0xUggUoTkh0gwljToLPPpyFOAg9GZZALRBkCmMqipnzDUKNlk8ir
mulX0PFjptdgVBknElxtrbdvqrO0zSAv+zwEAOAz4G3Apn2RCpfDv/kaRO1g1hlA
OuA1u86B92uhNjhycumYhPiB3RPH6g==
=5P0R
-----END PGP SIGNATURE-----
--=-=-=--