* [PATCH] rtc: ds1307: set uie_unsupported if no interrupt is available [not found] <CGME20210305174419eucas1p1639ed3bbcbc37ab3e3619e9c5d1e1629@eucas1p1.samsung.com> @ 2021-03-05 17:44 ` Łukasz Stelmach 2021-03-15 22:01 ` Alexandre Belloni [not found] ` <CGME20210316121218eucas1p1f74d6e6cec7fc897051902a7da478fd0@eucas1p1.samsung.com> 0 siblings, 2 replies; 15+ messages in thread From: Łukasz Stelmach @ 2021-03-05 17:44 UTC (permalink / raw) To: Alessandro Zummo, Alexandre Belloni, linux-rtc Cc: Bartłomiej Żolnierkiewicz, Marek Szyprowski, Łukasz Stelmach For an RTC without an IRQ assigned rtc_update_irq_enable() should return -EINVAL. It will, when uie_unsupported is set. 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"); + + /* 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH] rtc: ds1307: set uie_unsupported if no interrupt is available 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> 1 sibling, 0 replies; 15+ messages in thread From: Alexandre Belloni @ 2021-03-15 22:01 UTC (permalink / raw) To: Łukasz Stelmach Cc: Alessandro Zummo, linux-rtc, Bartłomiej Żolnierkiewicz, Marek Szyprowski 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? > 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 > + /* 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 > -- Alexandre Belloni, co-owner and COO, Bootlin Embedded Linux and Kernel engineering https://bootlin.com ^ permalink raw reply [flat|nested] 15+ messages in thread
[parent not found: <CGME20210316121218eucas1p1f74d6e6cec7fc897051902a7da478fd0@eucas1p1.samsung.com>]
* Re: [PATCH] rtc: ds1307: set uie_unsupported if no interrupt is available [not found] ` <CGME20210316121218eucas1p1f74d6e6cec7fc897051902a7da478fd0@eucas1p1.samsung.com> @ 2021-03-16 12:12 ` Lukasz Stelmach 2021-03-16 12:32 ` Alexandre Belloni 0 siblings, 1 reply; 15+ messages in thread From: Lukasz Stelmach @ 2021-03-16 12:12 UTC (permalink / raw) To: Alexandre Belloni Cc: Alessandro Zummo, linux-rtc, Bartłomiej Żolnierkiewicz, Marek Szyprowski, linux-kernel [-- 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 --] ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] rtc: ds1307: set uie_unsupported if no interrupt is available 2021-03-16 12:12 ` Lukasz Stelmach @ 2021-03-16 12:32 ` Alexandre Belloni [not found] ` <CGME20210316180430eucas1p1a3cb26f40772b0af75782ff84908d731@eucas1p1.samsung.com> 0 siblings, 1 reply; 15+ messages in thread From: Alexandre Belloni @ 2021-03-16 12:32 UTC (permalink / raw) To: Lukasz Stelmach Cc: Alessandro Zummo, linux-rtc, Bartłomiej Żolnierkiewicz, Marek Szyprowski, linux-kernel 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
[parent not found: <CGME20210316180430eucas1p1a3cb26f40772b0af75782ff84908d731@eucas1p1.samsung.com>]
* Re: [PATCH] rtc: ds1307: set uie_unsupported if no interrupt is available [not found] ` <CGME20210316180430eucas1p1a3cb26f40772b0af75782ff84908d731@eucas1p1.samsung.com> @ 2021-03-16 18:04 ` Lukasz Stelmach [not found] ` <CGME20210317081944eucas1p123bd27f3203c937d2969a66fb06d6d9e@eucas1p1.samsung.com> 2021-03-30 0:02 ` [PATCH] rtc: ds1307: set uie_unsupported if no interrupt is available Alexandre Belloni 0 siblings, 2 replies; 15+ messages in thread From: Lukasz Stelmach @ 2021-03-16 18:04 UTC (permalink / raw) To: Alexandre Belloni Cc: Alessandro Zummo, linux-rtc, Bartłomiej Żolnierkiewicz, Marek Szyprowski, linux-kernel [-- Attachment #1: Type: text/plain, Size: 6337 bytes --] 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, Ł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. 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 <= 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]) >> >> [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 -- Łukasz Stelmach Samsung R&D Institute Poland Samsung Electronics [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 487 bytes --] ^ permalink raw reply [flat|nested] 15+ messages in thread
[parent not found: <CGME20210317081944eucas1p123bd27f3203c937d2969a66fb06d6d9e@eucas1p1.samsung.com>]
* [PATCH 1/2] WIP: Introduce has_alarm method for rtc devices [not found] ` <CGME20210317081944eucas1p123bd27f3203c937d2969a66fb06d6d9e@eucas1p1.samsung.com> @ 2021-03-17 8:19 ` Łukasz Stelmach [not found] ` <CGME20210317081947eucas1p15a31b346977fba94bf154716c3d89cb0@eucas1p1.samsung.com> 0 siblings, 1 reply; 15+ messages in thread From: Łukasz Stelmach @ 2021-03-17 8:19 UTC (permalink / raw) To: alexandre.belloni Cc: l.stelmach, a.zummo, b.zolnierkie, linux-kernel, linux-rtc, m.szyprowski The method enables determining whether a device supports setting alarms or not before checking if the alarm to be set is in the past; thus, provides clear indication of support for alarms in a given configuration. Signed-off-by: Łukasz Stelmach <l.stelmach@samsung.com> --- How about has_alarm() method. It can be checked at the beginning of __rtc_set_alarm() like RTC_HAS_ALARM flag I proposed above, but doesn't need to be introduced in all drivers at once. See the following message for the implementation in the ds1307 driver. The first uie_unsupported patch should be kept regardless of these two. drivers/rtc/interface.c | 6 ++++++ include/linux/rtc.h | 1 + 2 files changed, 7 insertions(+) diff --git a/drivers/rtc/interface.c b/drivers/rtc/interface.c index 794a4f036b99..1eb180370d9b 100644 --- a/drivers/rtc/interface.c +++ b/drivers/rtc/interface.c @@ -412,6 +412,12 @@ static int __rtc_set_alarm(struct rtc_device *rtc, struct rtc_wkalrm *alarm) time64_t now, scheduled; int err; + if (!rtc->ops) + err = -ENODEV; + else if (rtc->ops->has_alarm && + !rtc->ops->has_alarm(rtc->dev.parent)) + return -EINVAL; + err = rtc_valid_tm(&alarm->time); if (err) return err; diff --git a/include/linux/rtc.h b/include/linux/rtc.h index 22d1575e4991..ce9fc77ccd02 100644 --- a/include/linux/rtc.h +++ b/include/linux/rtc.h @@ -66,6 +66,7 @@ struct rtc_class_ops { int (*alarm_irq_enable)(struct device *, unsigned int enabled); int (*read_offset)(struct device *, long *offset); int (*set_offset)(struct device *, long offset); + int (*has_alarm)(struct device *); }; struct rtc_device; -- 2.26.2 ^ permalink raw reply related [flat|nested] 15+ messages in thread
[parent not found: <CGME20210317081947eucas1p15a31b346977fba94bf154716c3d89cb0@eucas1p1.samsung.com>]
* [PATCH 2/2] WIP: Provide has_alarm method [not found] ` <CGME20210317081947eucas1p15a31b346977fba94bf154716c3d89cb0@eucas1p1.samsung.com> @ 2021-03-17 8:19 ` Łukasz Stelmach 0 siblings, 0 replies; 15+ messages in thread From: Łukasz Stelmach @ 2021-03-17 8:19 UTC (permalink / raw) To: alexandre.belloni Cc: l.stelmach, a.zummo, b.zolnierkie, linux-kernel, linux-rtc, m.szyprowski Signed-off-by: Łukasz Stelmach <l.stelmach@samsung.com> --- drivers/rtc/rtc-ds1307.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c index b21e06583bd5..dee60f459a3e 100644 --- a/drivers/rtc/rtc-ds1307.c +++ b/drivers/rtc/rtc-ds1307.c @@ -387,6 +387,13 @@ static int ds1307_set_time(struct device *dev, struct rtc_time *t) return 0; } +static int ds1307_has_alarm(struct device *dev) +{ + struct ds1307 *ds1307 = dev_get_drvdata(dev); + + return test_bit(HAS_ALARM, &ds1307->flags); +} + static int ds1337_read_alarm(struct device *dev, struct rtc_wkalrm *t) { struct ds1307 *ds1307 = dev_get_drvdata(dev); @@ -1201,6 +1208,7 @@ static const struct rtc_class_ops ds13xx_rtc_ops = { .read_alarm = ds1337_read_alarm, .set_alarm = ds1337_set_alarm, .alarm_irq_enable = ds1307_alarm_irq_enable, + .has_alarm = ds1307_has_alarm, }; static ssize_t frequency_test_store(struct device *dev, -- 2.26.2 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH] rtc: ds1307: set uie_unsupported if no interrupt is available 2021-03-16 18:04 ` Lukasz Stelmach [not found] ` <CGME20210317081944eucas1p123bd27f3203c937d2969a66fb06d6d9e@eucas1p1.samsung.com> @ 2021-03-30 0:02 ` Alexandre Belloni 2021-03-30 0:03 ` [PATCH 1/3] rtc: ds1307: replace HAS_ALARM by RTC_FEATURE_ALARM Alexandre Belloni [not found] ` <CGME20210330065217eucas1p2c50a100bf0101e72dd753f37257f6a57@eucas1p2.samsung.com> 1 sibling, 2 replies; 15+ messages in thread From: Alexandre Belloni @ 2021-03-30 0:02 UTC (permalink / raw) To: Lukasz Stelmach Cc: Alessandro Zummo, linux-rtc, Bartłomiej Żolnierkiewicz, Marek Szyprowski, linux-kernel On 16/03/2021 19:04:14+0100, Lukasz Stelmach wrote: > 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 <= 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? > 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. -- Alexandre Belloni, co-owner and COO, Bootlin Embedded Linux and Kernel engineering https://bootlin.com ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 1/3] rtc: ds1307: replace HAS_ALARM by RTC_FEATURE_ALARM 2021-03-30 0:02 ` [PATCH] rtc: ds1307: set uie_unsupported if no interrupt is available Alexandre Belloni @ 2021-03-30 0:03 ` Alexandre Belloni 2021-03-30 0:03 ` [PATCH 2/3] rtc: ds1307: remove flags Alexandre Belloni ` (2 more replies) [not found] ` <CGME20210330065217eucas1p2c50a100bf0101e72dd753f37257f6a57@eucas1p2.samsung.com> 1 sibling, 3 replies; 15+ messages in thread From: Alexandre Belloni @ 2021-03-30 0:03 UTC (permalink / raw) To: Lukasz Stelmach Cc: linux-rtc, Bartłomiej Żolnierkiewicz, Marek Szyprowski, linux-kernel, Alexandre Belloni The core now has RTC_FEATURE_ALARM for the driver to indicate whether alarms are available. Use that instead of HAS_ALARM to ensure the alarm callbacks are not even called. Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com> --- drivers/rtc/rtc-ds1307.c | 42 +++++++--------------------------------- 1 file changed, 7 insertions(+), 35 deletions(-) diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c index cd8e438bc9c4..76d67c419f7d 100644 --- a/drivers/rtc/rtc-ds1307.c +++ b/drivers/rtc/rtc-ds1307.c @@ -171,7 +171,6 @@ struct ds1307 { enum ds_type type; unsigned long flags; #define HAS_NVRAM 0 /* bit 0 == sysfs file active */ -#define HAS_ALARM 1 /* bit 1 == irq claimed */ struct device *dev; struct regmap *regmap; const char *name; @@ -411,9 +410,6 @@ static int ds1337_read_alarm(struct device *dev, struct rtc_wkalrm *t) int ret; u8 regs[9]; - if (!test_bit(HAS_ALARM, &ds1307->flags)) - return -EINVAL; - /* read all ALARM1, ALARM2, and status registers at once */ ret = regmap_bulk_read(ds1307->regmap, DS1339_REG_ALARM1_SECS, regs, sizeof(regs)); @@ -454,9 +450,6 @@ static int ds1337_set_alarm(struct device *dev, struct rtc_wkalrm *t) u8 control, status; int ret; - if (!test_bit(HAS_ALARM, &ds1307->flags)) - return -EINVAL; - dev_dbg(dev, "%s secs=%d, mins=%d, " "hours=%d, mday=%d, enabled=%d, pending=%d\n", "alarm set", t->time.tm_sec, t->time.tm_min, @@ -512,9 +505,6 @@ static int ds1307_alarm_irq_enable(struct device *dev, unsigned int enabled) { struct ds1307 *ds1307 = dev_get_drvdata(dev); - if (!test_bit(HAS_ALARM, &ds1307->flags)) - return -ENOTTY; - return regmap_update_bits(ds1307->regmap, DS1337_REG_CONTROL, DS1337_BIT_A1IE, enabled ? DS1337_BIT_A1IE : 0); @@ -592,9 +582,6 @@ static int rx8130_read_alarm(struct device *dev, struct rtc_wkalrm *t) u8 ald[3], ctl[3]; int ret; - if (!test_bit(HAS_ALARM, &ds1307->flags)) - return -EINVAL; - /* Read alarm registers. */ ret = regmap_bulk_read(ds1307->regmap, RX8130_REG_ALARM_MIN, ald, sizeof(ald)); @@ -634,9 +621,6 @@ static int rx8130_set_alarm(struct device *dev, struct rtc_wkalrm *t) u8 ald[3], ctl[3]; int ret; - if (!test_bit(HAS_ALARM, &ds1307->flags)) - return -EINVAL; - dev_dbg(dev, "%s, sec=%d min=%d hour=%d wday=%d mday=%d mon=%d " "enabled=%d pending=%d\n", __func__, t->time.tm_sec, t->time.tm_min, t->time.tm_hour, @@ -681,9 +665,6 @@ static int rx8130_alarm_irq_enable(struct device *dev, unsigned int enabled) struct ds1307 *ds1307 = dev_get_drvdata(dev); int ret, reg; - if (!test_bit(HAS_ALARM, &ds1307->flags)) - return -EINVAL; - ret = regmap_read(ds1307->regmap, RX8130_REG_CONTROL0, ®); if (ret < 0) return ret; @@ -735,9 +716,6 @@ static int mcp794xx_read_alarm(struct device *dev, struct rtc_wkalrm *t) u8 regs[10]; int ret; - if (!test_bit(HAS_ALARM, &ds1307->flags)) - return -EINVAL; - /* Read control and alarm 0 registers. */ ret = regmap_bulk_read(ds1307->regmap, MCP794XX_REG_CONTROL, regs, sizeof(regs)); @@ -793,9 +771,6 @@ static int mcp794xx_set_alarm(struct device *dev, struct rtc_wkalrm *t) unsigned char regs[10]; int wday, ret; - if (!test_bit(HAS_ALARM, &ds1307->flags)) - return -EINVAL; - wday = mcp794xx_alm_weekday(dev, &t->time); if (wday < 0) return wday; @@ -842,9 +817,6 @@ static int mcp794xx_alarm_irq_enable(struct device *dev, unsigned int enabled) { struct ds1307 *ds1307 = dev_get_drvdata(dev); - if (!test_bit(HAS_ALARM, &ds1307->flags)) - return -EINVAL; - return regmap_update_bits(ds1307->regmap, MCP794XX_REG_CONTROL, MCP794XX_BIT_ALM0_EN, enabled ? MCP794XX_BIT_ALM0_EN : 0); @@ -1641,7 +1613,7 @@ static int ds3231_clks_register(struct ds1307 *ds1307) * Interrupt signal due to alarm conditions and square-wave * output share same pin, so don't initialize both. */ - if (i == DS3231_CLK_SQW && test_bit(HAS_ALARM, &ds1307->flags)) + if (i == DS3231_CLK_SQW && test_bit(RTC_FEATURE_ALARM, ds1307->rtc->features)) continue; init.name = ds3231_clks_names[i]; @@ -1964,15 +1936,15 @@ static int ds1307_probe(struct i2c_client *client, bin2bcd(tmp)); } - if (want_irq || ds1307_can_wakeup_device) { - device_set_wakeup_capable(ds1307->dev, true); - set_bit(HAS_ALARM, &ds1307->flags); - } - ds1307->rtc = devm_rtc_allocate_device(ds1307->dev); if (IS_ERR(ds1307->rtc)) return PTR_ERR(ds1307->rtc); + if (want_irq || ds1307_can_wakeup_device) + device_set_wakeup_capable(ds1307->dev, true); + else + clear_bit(RTC_FEATURE_ALARM, ds1307->rtc->features); + if (ds1307_can_wakeup_device && !want_irq) { dev_info(ds1307->dev, "'wakeup-source' is set, request for an IRQ is disabled!\n"); @@ -1988,7 +1960,7 @@ static int ds1307_probe(struct i2c_client *client, if (err) { client->irq = 0; device_set_wakeup_capable(ds1307->dev, false); - clear_bit(HAS_ALARM, &ds1307->flags); + clear_bit(RTC_FEATURE_ALARM, ds1307->rtc->features); dev_err(ds1307->dev, "unable to request IRQ!\n"); } else { dev_dbg(ds1307->dev, "got IRQ %d\n", client->irq); -- 2.30.2 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 2/3] rtc: ds1307: remove flags 2021-03-30 0:03 ` [PATCH 1/3] rtc: ds1307: replace HAS_ALARM by RTC_FEATURE_ALARM Alexandre Belloni @ 2021-03-30 0:03 ` Alexandre Belloni [not found] ` <CGME20210402102743eucas1p23690e0df642a10fa0326a84fac6c0ed3@eucas1p2.samsung.com> 2021-03-30 0:03 ` [PATCH 3/3] rtc: rtc_update_irq_enable: rework UIE emulation Alexandre Belloni [not found] ` <CGME20210402102655eucas1p24ee7e879816089921bf9752c3c483122@eucas1p2.samsung.com> 2 siblings, 1 reply; 15+ messages in thread From: Alexandre Belloni @ 2021-03-30 0:03 UTC (permalink / raw) To: Lukasz Stelmach Cc: linux-rtc, Bartłomiej Żolnierkiewicz, Marek Szyprowski, linux-kernel, Alexandre Belloni flags is now unused, drop it. Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com> --- drivers/rtc/rtc-ds1307.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c index 76d67c419f7d..089509d0a3a0 100644 --- a/drivers/rtc/rtc-ds1307.c +++ b/drivers/rtc/rtc-ds1307.c @@ -169,8 +169,6 @@ enum ds_type { struct ds1307 { enum ds_type type; - unsigned long flags; -#define HAS_NVRAM 0 /* bit 0 == sysfs file active */ struct device *dev; struct regmap *regmap; const char *name; -- 2.30.2 ^ permalink raw reply related [flat|nested] 15+ messages in thread
[parent not found: <CGME20210402102743eucas1p23690e0df642a10fa0326a84fac6c0ed3@eucas1p2.samsung.com>]
* Re: [PATCH 2/3] rtc: ds1307: remove flags [not found] ` <CGME20210402102743eucas1p23690e0df642a10fa0326a84fac6c0ed3@eucas1p2.samsung.com> @ 2021-04-02 10:27 ` Łukasz Stelmach 0 siblings, 0 replies; 15+ messages in thread From: Łukasz Stelmach @ 2021-04-02 10:27 UTC (permalink / raw) To: Alexandre Belloni Cc: linux-rtc, Bartłomiej Żolnierkiewicz, Marek Szyprowski, linux-kernel [-- Attachment #1: Type: text/plain, Size: 868 bytes --] It was <2021-03-30 wto 02:03>, when Alexandre Belloni wrote: > flags is now unused, drop it. > > Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com> > --- > drivers/rtc/rtc-ds1307.c | 2 -- > 1 file changed, 2 deletions(-) > Tested-by: Łukasz Stelmach <l.stelmach@samsung.com> Reviewed-by: Łukasz Stelmach <l.stelmach@samsung.com> > diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c > index 76d67c419f7d..089509d0a3a0 100644 > --- a/drivers/rtc/rtc-ds1307.c > +++ b/drivers/rtc/rtc-ds1307.c > @@ -169,8 +169,6 @@ enum ds_type { > > struct ds1307 { > enum ds_type type; > - unsigned long flags; > -#define HAS_NVRAM 0 /* bit 0 == sysfs file active */ > struct device *dev; > struct regmap *regmap; > const char *name; -- Łukasz Stelmach Samsung R&D Institute Poland Samsung Electronics [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 487 bytes --] ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 3/3] rtc: rtc_update_irq_enable: rework UIE emulation 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 @ 2021-03-30 0:03 ` Alexandre Belloni [not found] ` <CGME20210402102813eucas1p1bae7ec57559fa8df622118275bf6fae0@eucas1p1.samsung.com> [not found] ` <CGME20210402102655eucas1p24ee7e879816089921bf9752c3c483122@eucas1p2.samsung.com> 2 siblings, 1 reply; 15+ messages in thread From: Alexandre Belloni @ 2021-03-30 0:03 UTC (permalink / raw) To: Lukasz Stelmach Cc: linux-rtc, Bartłomiej Żolnierkiewicz, Marek Szyprowski, linux-kernel, Alexandre Belloni Now that the core is aware of whether alarms are available, it is possible to decide whether UIE emulation is required before actually trying to set the alarm. This greatly simplifies rtc_update_irq_enable because there is now only one error value to track and is not relying on the return value of __rtc_set_alarm anymore. Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com> --- drivers/rtc/interface.c | 28 +++++++--------------------- 1 file changed, 7 insertions(+), 21 deletions(-) diff --git a/drivers/rtc/interface.c b/drivers/rtc/interface.c index dcb34c73319e..b162964d2b39 100644 --- a/drivers/rtc/interface.c +++ b/drivers/rtc/interface.c @@ -561,8 +561,12 @@ int rtc_update_irq_enable(struct rtc_device *rtc, unsigned int enabled) if (rtc->uie_rtctimer.enabled == enabled) goto out; - if (rtc->uie_unsupported) { + if (rtc->uie_unsupported || !test_bit(RTC_FEATURE_ALARM, rtc->features)) { +#ifdef CONFIG_RTC_INTF_DEV_UIE_EMUL + err = rtc_dev_update_irq_enable_emul(rtc, enabled); +#else err = -EINVAL; +#endif goto out; } @@ -570,8 +574,8 @@ int rtc_update_irq_enable(struct rtc_device *rtc, unsigned int enabled) struct rtc_time tm; ktime_t now, onesec; - rc = __rtc_read_time(rtc, &tm); - if (rc) + err = __rtc_read_time(rtc, &tm); + if (err) goto out; onesec = ktime_set(1, 0); now = rtc_tm_to_ktime(tm); @@ -585,24 +589,6 @@ int rtc_update_irq_enable(struct rtc_device *rtc, unsigned int enabled) out: mutex_unlock(&rtc->ops_lock); - /* - * __rtc_read_time() failed, this probably means that the RTC time has - * never been set or less probably there is a transient error on the - * bus. In any case, avoid enabling emulation has this will fail when - * reading the time too. - */ - if (rc) - return rc; - -#ifdef CONFIG_RTC_INTF_DEV_UIE_EMUL - /* - * Enable emulation if the driver returned -EINVAL to signal that it has - * been configured without interrupts or they are not available at the - * moment. - */ - if (err == -EINVAL) - err = rtc_dev_update_irq_enable_emul(rtc, enabled); -#endif return err; } EXPORT_SYMBOL_GPL(rtc_update_irq_enable); -- 2.30.2 ^ permalink raw reply related [flat|nested] 15+ messages in thread
[parent not found: <CGME20210402102813eucas1p1bae7ec57559fa8df622118275bf6fae0@eucas1p1.samsung.com>]
* Re: [PATCH 3/3] rtc: rtc_update_irq_enable: rework UIE emulation [not found] ` <CGME20210402102813eucas1p1bae7ec57559fa8df622118275bf6fae0@eucas1p1.samsung.com> @ 2021-04-02 10:28 ` Łukasz Stelmach 0 siblings, 0 replies; 15+ messages in thread From: Łukasz Stelmach @ 2021-04-02 10:28 UTC (permalink / raw) To: Alexandre Belloni Cc: linux-rtc, Bartłomiej Żolnierkiewicz, Marek Szyprowski, linux-kernel [-- Attachment #1: Type: text/plain, Size: 2540 bytes --] It was <2021-03-30 wto 02:03>, when Alexandre Belloni wrote: > Now that the core is aware of whether alarms are available, it is possible > to decide whether UIE emulation is required before actually trying to set > the alarm. > > This greatly simplifies rtc_update_irq_enable because there is now only one > error value to track and is not relying on the return value of > __rtc_set_alarm anymore. > > Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com> > --- > drivers/rtc/interface.c | 28 +++++++--------------------- > 1 file changed, 7 insertions(+), 21 deletions(-) > Tested-by: Łukasz Stelmach <l.stelmach@samsung.com> > diff --git a/drivers/rtc/interface.c b/drivers/rtc/interface.c > index dcb34c73319e..b162964d2b39 100644 > --- a/drivers/rtc/interface.c > +++ b/drivers/rtc/interface.c > @@ -561,8 +561,12 @@ int rtc_update_irq_enable(struct rtc_device *rtc, unsigned int enabled) > if (rtc->uie_rtctimer.enabled == enabled) > goto out; > > - if (rtc->uie_unsupported) { > + if (rtc->uie_unsupported || !test_bit(RTC_FEATURE_ALARM, rtc->features)) { > +#ifdef CONFIG_RTC_INTF_DEV_UIE_EMUL > + err = rtc_dev_update_irq_enable_emul(rtc, enabled); > +#else > err = -EINVAL; > +#endif > goto out; > } > > @@ -570,8 +574,8 @@ int rtc_update_irq_enable(struct rtc_device *rtc, unsigned int enabled) > struct rtc_time tm; > ktime_t now, onesec; > > - rc = __rtc_read_time(rtc, &tm); > - if (rc) > + err = __rtc_read_time(rtc, &tm); > + if (err) > goto out; > onesec = ktime_set(1, 0); > now = rtc_tm_to_ktime(tm); > @@ -585,24 +589,6 @@ int rtc_update_irq_enable(struct rtc_device *rtc, unsigned int enabled) > out: > mutex_unlock(&rtc->ops_lock); > > - /* > - * __rtc_read_time() failed, this probably means that the RTC time has > - * never been set or less probably there is a transient error on the > - * bus. In any case, avoid enabling emulation has this will fail when > - * reading the time too. > - */ > - if (rc) > - return rc; > - > -#ifdef CONFIG_RTC_INTF_DEV_UIE_EMUL > - /* > - * Enable emulation if the driver returned -EINVAL to signal that it has > - * been configured without interrupts or they are not available at the > - * moment. > - */ > - if (err == -EINVAL) > - err = rtc_dev_update_irq_enable_emul(rtc, enabled); > -#endif > return err; > } > EXPORT_SYMBOL_GPL(rtc_update_irq_enable); -- Łukasz Stelmach Samsung R&D Institute Poland Samsung Electronics [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 487 bytes --] ^ permalink raw reply [flat|nested] 15+ messages in thread
[parent not found: <CGME20210402102655eucas1p24ee7e879816089921bf9752c3c483122@eucas1p2.samsung.com>]
* Re: [PATCH 1/3] rtc: ds1307: replace HAS_ALARM by RTC_FEATURE_ALARM [not found] ` <CGME20210402102655eucas1p24ee7e879816089921bf9752c3c483122@eucas1p2.samsung.com> @ 2021-04-02 10:26 ` Łukasz Stelmach 0 siblings, 0 replies; 15+ messages in thread From: Łukasz Stelmach @ 2021-04-02 10:26 UTC (permalink / raw) To: Alexandre Belloni Cc: linux-rtc, Bartłomiej Żolnierkiewicz, Marek Szyprowski, linux-kernel [-- Attachment #1: Type: text/plain, Size: 5894 bytes --] It was <2021-03-30 wto 02:03>, when Alexandre Belloni wrote: > The core now has RTC_FEATURE_ALARM for the driver to indicate whether > alarms are available. Use that instead of HAS_ALARM to ensure the alarm > callbacks are not even called. > > Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com> > --- > drivers/rtc/rtc-ds1307.c | 42 +++++++--------------------------------- > 1 file changed, 7 insertions(+), 35 deletions(-) > Tested-by: Łukasz Stelmach <l.stelmach@samsung.com> Reviewed-by: Łukasz Stelmach <l.stelmach@samsung.com> > diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c > index cd8e438bc9c4..76d67c419f7d 100644 > --- a/drivers/rtc/rtc-ds1307.c > +++ b/drivers/rtc/rtc-ds1307.c > @@ -171,7 +171,6 @@ struct ds1307 { > enum ds_type type; > unsigned long flags; > #define HAS_NVRAM 0 /* bit 0 == sysfs file active */ > -#define HAS_ALARM 1 /* bit 1 == irq claimed */ > struct device *dev; > struct regmap *regmap; > const char *name; > @@ -411,9 +410,6 @@ static int ds1337_read_alarm(struct device *dev, struct rtc_wkalrm *t) > int ret; > u8 regs[9]; > > - if (!test_bit(HAS_ALARM, &ds1307->flags)) > - return -EINVAL; > - > /* read all ALARM1, ALARM2, and status registers at once */ > ret = regmap_bulk_read(ds1307->regmap, DS1339_REG_ALARM1_SECS, > regs, sizeof(regs)); > @@ -454,9 +450,6 @@ static int ds1337_set_alarm(struct device *dev, struct rtc_wkalrm *t) > u8 control, status; > int ret; > > - if (!test_bit(HAS_ALARM, &ds1307->flags)) > - return -EINVAL; > - > dev_dbg(dev, "%s secs=%d, mins=%d, " > "hours=%d, mday=%d, enabled=%d, pending=%d\n", > "alarm set", t->time.tm_sec, t->time.tm_min, > @@ -512,9 +505,6 @@ static int ds1307_alarm_irq_enable(struct device *dev, unsigned int enabled) > { > struct ds1307 *ds1307 = dev_get_drvdata(dev); > > - if (!test_bit(HAS_ALARM, &ds1307->flags)) > - return -ENOTTY; > - > return regmap_update_bits(ds1307->regmap, DS1337_REG_CONTROL, > DS1337_BIT_A1IE, > enabled ? DS1337_BIT_A1IE : 0); > @@ -592,9 +582,6 @@ static int rx8130_read_alarm(struct device *dev, struct rtc_wkalrm *t) > u8 ald[3], ctl[3]; > int ret; > > - if (!test_bit(HAS_ALARM, &ds1307->flags)) > - return -EINVAL; > - > /* Read alarm registers. */ > ret = regmap_bulk_read(ds1307->regmap, RX8130_REG_ALARM_MIN, ald, > sizeof(ald)); > @@ -634,9 +621,6 @@ static int rx8130_set_alarm(struct device *dev, struct rtc_wkalrm *t) > u8 ald[3], ctl[3]; > int ret; > > - if (!test_bit(HAS_ALARM, &ds1307->flags)) > - return -EINVAL; > - > dev_dbg(dev, "%s, sec=%d min=%d hour=%d wday=%d mday=%d mon=%d " > "enabled=%d pending=%d\n", __func__, > t->time.tm_sec, t->time.tm_min, t->time.tm_hour, > @@ -681,9 +665,6 @@ static int rx8130_alarm_irq_enable(struct device *dev, unsigned int enabled) > struct ds1307 *ds1307 = dev_get_drvdata(dev); > int ret, reg; > > - if (!test_bit(HAS_ALARM, &ds1307->flags)) > - return -EINVAL; > - > ret = regmap_read(ds1307->regmap, RX8130_REG_CONTROL0, ®); > if (ret < 0) > return ret; > @@ -735,9 +716,6 @@ static int mcp794xx_read_alarm(struct device *dev, struct rtc_wkalrm *t) > u8 regs[10]; > int ret; > > - if (!test_bit(HAS_ALARM, &ds1307->flags)) > - return -EINVAL; > - > /* Read control and alarm 0 registers. */ > ret = regmap_bulk_read(ds1307->regmap, MCP794XX_REG_CONTROL, regs, > sizeof(regs)); > @@ -793,9 +771,6 @@ static int mcp794xx_set_alarm(struct device *dev, struct rtc_wkalrm *t) > unsigned char regs[10]; > int wday, ret; > > - if (!test_bit(HAS_ALARM, &ds1307->flags)) > - return -EINVAL; > - > wday = mcp794xx_alm_weekday(dev, &t->time); > if (wday < 0) > return wday; > @@ -842,9 +817,6 @@ static int mcp794xx_alarm_irq_enable(struct device *dev, unsigned int enabled) > { > struct ds1307 *ds1307 = dev_get_drvdata(dev); > > - if (!test_bit(HAS_ALARM, &ds1307->flags)) > - return -EINVAL; > - > return regmap_update_bits(ds1307->regmap, MCP794XX_REG_CONTROL, > MCP794XX_BIT_ALM0_EN, > enabled ? MCP794XX_BIT_ALM0_EN : 0); > @@ -1641,7 +1613,7 @@ static int ds3231_clks_register(struct ds1307 *ds1307) > * Interrupt signal due to alarm conditions and square-wave > * output share same pin, so don't initialize both. > */ > - if (i == DS3231_CLK_SQW && test_bit(HAS_ALARM, &ds1307->flags)) > + if (i == DS3231_CLK_SQW && test_bit(RTC_FEATURE_ALARM, ds1307->rtc->features)) > continue; > > init.name = ds3231_clks_names[i]; > @@ -1964,15 +1936,15 @@ static int ds1307_probe(struct i2c_client *client, > bin2bcd(tmp)); > } > > - if (want_irq || ds1307_can_wakeup_device) { > - device_set_wakeup_capable(ds1307->dev, true); > - set_bit(HAS_ALARM, &ds1307->flags); > - } > - > ds1307->rtc = devm_rtc_allocate_device(ds1307->dev); > if (IS_ERR(ds1307->rtc)) > return PTR_ERR(ds1307->rtc); > > + if (want_irq || ds1307_can_wakeup_device) > + device_set_wakeup_capable(ds1307->dev, true); > + else > + clear_bit(RTC_FEATURE_ALARM, ds1307->rtc->features); > + > if (ds1307_can_wakeup_device && !want_irq) { > dev_info(ds1307->dev, > "'wakeup-source' is set, request for an IRQ is disabled!\n"); > @@ -1988,7 +1960,7 @@ static int ds1307_probe(struct i2c_client *client, > if (err) { > client->irq = 0; > device_set_wakeup_capable(ds1307->dev, false); > - clear_bit(HAS_ALARM, &ds1307->flags); > + clear_bit(RTC_FEATURE_ALARM, ds1307->rtc->features); > dev_err(ds1307->dev, "unable to request IRQ!\n"); > } else { > dev_dbg(ds1307->dev, "got IRQ %d\n", client->irq); -- Łukasz Stelmach Samsung R&D Institute Poland Samsung Electronics [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 487 bytes --] ^ permalink raw reply [flat|nested] 15+ messages in thread
[parent not found: <CGME20210330065217eucas1p2c50a100bf0101e72dd753f37257f6a57@eucas1p2.samsung.com>]
* Re: [PATCH] rtc: ds1307: set uie_unsupported if no interrupt is available [not found] ` <CGME20210330065217eucas1p2c50a100bf0101e72dd753f37257f6a57@eucas1p2.samsung.com> @ 2021-03-30 6:52 ` Lukasz Stelmach 0 siblings, 0 replies; 15+ messages in thread From: Lukasz Stelmach @ 2021-03-30 6:52 UTC (permalink / raw) To: Alexandre Belloni Cc: Alessandro Zummo, linux-rtc, Bartłomiej Żolnierkiewicz, Marek Szyprowski, linux-kernel [-- Attachment #1: Type: text/plain, Size: 1985 bytes --] 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. >> >> 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 <= 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? >> > > 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. -- Łukasz Stelmach Samsung R&D Institute Poland Samsung Electronics [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 487 bytes --] ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2021-04-02 10:28 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [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 [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
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).