diff mbox series

rtc: ds1307: set uie_unsupported if no interrupt is available

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

Commit Message

Lukasz Stelmach March 5, 2021, 5:44 p.m. UTC
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(-)

Comments

Alexandre Belloni March 15, 2021, 10:01 p.m. UTC | #1
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
>
Lukasz Stelmach March 16, 2021, 12:12 p.m. UTC | #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
>>
Alexandre Belloni March 16, 2021, 12:32 p.m. UTC | #3
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
Lukasz Stelmach March 16, 2021, 6:04 p.m. UTC | #4
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
Alexandre Belloni March 30, 2021, 12:02 a.m. UTC | #5
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.
Lukasz Stelmach March 30, 2021, 6:52 a.m. UTC | #6
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 mbox series

Patch

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;