| Message ID | 20210305174411.9657-1-l.stelmach@samsung.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | rtc: ds1307: set uie_unsupported if no interrupt is available | expand |
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 >
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 >>
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
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
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.
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.
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;
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(-)