From: Alexandre Belloni <alexandre.belloni@bootlin.com>
To: Lukasz Stelmach <l.stelmach@samsung.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:32:20 +0100 [thread overview]
Message-ID: <YFClVBShBuy9/VPY@piout.net> (raw)
In-Reply-To: <dleftj1rcfe1rb.fsf%l.stelmach@samsung.com>
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, Ł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.
>
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.
> 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
--
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
next prev parent reply other threads:[~2021-03-16 12:33 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <CGME20210305174419eucas1p1639ed3bbcbc37ab3e3619e9c5d1e1629@eucas1p1.samsung.com>
2021-03-05 17:44 ` [PATCH] rtc: ds1307: set uie_unsupported if no interrupt is available Łukasz Stelmach
2021-03-15 22:01 ` Alexandre Belloni
[not found] ` <CGME20210316121218eucas1p1f74d6e6cec7fc897051902a7da478fd0@eucas1p1.samsung.com>
2021-03-16 12:12 ` Lukasz Stelmach
2021-03-16 12:32 ` Alexandre Belloni [this message]
[not found] ` <CGME20210316180430eucas1p1a3cb26f40772b0af75782ff84908d731@eucas1p1.samsung.com>
2021-03-16 18:04 ` Lukasz Stelmach
[not found] ` <CGME20210317081944eucas1p123bd27f3203c937d2969a66fb06d6d9e@eucas1p1.samsung.com>
2021-03-17 8:19 ` [PATCH 1/2] WIP: Introduce has_alarm method for rtc devices Łukasz Stelmach
[not found] ` <CGME20210317081947eucas1p15a31b346977fba94bf154716c3d89cb0@eucas1p1.samsung.com>
2021-03-17 8:19 ` [PATCH 2/2] WIP: Provide has_alarm method Łukasz Stelmach
2021-03-30 0:02 ` [PATCH] rtc: ds1307: set uie_unsupported if no interrupt is available Alexandre Belloni
2021-03-30 0:03 ` [PATCH 1/3] rtc: ds1307: replace HAS_ALARM by RTC_FEATURE_ALARM Alexandre Belloni
2021-03-30 0:03 ` [PATCH 2/3] rtc: ds1307: remove flags Alexandre Belloni
[not found] ` <CGME20210402102743eucas1p23690e0df642a10fa0326a84fac6c0ed3@eucas1p2.samsung.com>
2021-04-02 10:27 ` Łukasz Stelmach
2021-03-30 0:03 ` [PATCH 3/3] rtc: rtc_update_irq_enable: rework UIE emulation Alexandre Belloni
[not found] ` <CGME20210402102813eucas1p1bae7ec57559fa8df622118275bf6fae0@eucas1p1.samsung.com>
2021-04-02 10:28 ` Łukasz Stelmach
[not found] ` <CGME20210402102655eucas1p24ee7e879816089921bf9752c3c483122@eucas1p2.samsung.com>
2021-04-02 10:26 ` [PATCH 1/3] rtc: ds1307: replace HAS_ALARM by RTC_FEATURE_ALARM Łukasz Stelmach
[not found] ` <CGME20210330065217eucas1p2c50a100bf0101e72dd753f37257f6a57@eucas1p2.samsung.com>
2021-03-30 6:52 ` [PATCH] rtc: ds1307: set uie_unsupported if no interrupt is available Lukasz Stelmach
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YFClVBShBuy9/VPY@piout.net \
--to=alexandre.belloni@bootlin.com \
--cc=a.zummo@towertech.it \
--cc=b.zolnierkie@samsung.com \
--cc=l.stelmach@samsung.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-rtc@vger.kernel.org \
--cc=m.szyprowski@samsung.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).