[PATCH] rtc: ds1307: set uie_unsupported if no interrupt is available

[PATCH] rtc: ds1307: set uie_unsupported if no interrupt is available

[Ł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

[PATCH] rtc: ds1307: set uie_unsupported if no interrupt is available

[Ł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

Re: [PATCH] rtc: ds1307: set uie_unsupported if no interrupt is available

[Alexandre Belloni]

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

Re: [PATCH] rtc: ds1307: set uie_unsupported if no interrupt is available

[Lukasz Stelmach]

[-- 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 --]

Re: [PATCH] rtc: ds1307: set uie_unsupported if no interrupt is available

[Alexandre Belloni]

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

Re: [PATCH] rtc: ds1307: set uie_unsupported if no interrupt is available

[Lukasz Stelmach]

[-- 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 --]

[PATCH 1/2] WIP: Introduce has_alarm method for rtc devices

[Łukasz Stelmach]

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

[PATCH 2/2] WIP: Provide has_alarm method

[Łukasz Stelmach]

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

Re: [PATCH] rtc: ds1307: set uie_unsupported if no interrupt is available

[Alexandre Belloni]

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

[PATCH 1/3] rtc: ds1307: replace HAS_ALARM by RTC_FEATURE_ALARM

[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, &reg);
 	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

[PATCH 2/3] rtc: ds1307: remove flags

[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

Re: [PATCH 2/3] rtc: ds1307: remove flags

[Łukasz Stelmach]

[-- 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 --]

[PATCH 3/3] rtc: rtc_update_irq_enable: rework UIE emulation

[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

Re: [PATCH 3/3] rtc: rtc_update_irq_enable: rework UIE emulation

[Łukasz Stelmach]

[-- 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 --]

Re: [PATCH 1/3] rtc: ds1307: replace HAS_ALARM by RTC_FEATURE_ALARM

[Łukasz Stelmach]

[-- 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, &reg);
>  	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 --]

Re: [PATCH] rtc: ds1307: set uie_unsupported if no interrupt is available

[Lukasz Stelmach]

[-- 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 --]