[PATCH] alarmtimer: check RTC features instead of ops

[PATCH] alarmtimer: check RTC features instead of ops

[Alexandre Belloni]

Test RTC_FEATURE_ALARM instead of relying on ops->set_alarm to know whether
alarms are available.

Fixes: 7ae41220ef58 ("rtc: introduce features bitfield")
Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
---
Hello,

This doesn't seem much but this solve an issue where following a change in the
RTC driver, this part of the code will think the RTC is alarm capable while it
is not, then breaking the alarmtimer functionnality.

 kernel/time/alarmtimer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/time/alarmtimer.c b/kernel/time/alarmtimer.c
index 98d7a15e8cf6..4d7a6dffa1e5 100644
--- a/kernel/time/alarmtimer.c
+++ b/kernel/time/alarmtimer.c
@@ -92,7 +92,7 @@ static int alarmtimer_rtc_add_device(struct device *dev,
 	if (rtcdev)
 		return -EBUSY;
 
-	if (!rtc->ops->set_alarm)
+	if (!test_bit(RTC_FEATURE_ALARM, rtc->features))
 		return -1;
 	if (!device_may_wakeup(rtc->dev.parent))
 		return -1;
-- 
2.30.2

Re: [PATCH] alarmtimer: check RTC features instead of ops

[Thomas Gleixner]

On Thu, Apr 29 2021 at 23:49, Alexandre Belloni wrote:
> Test RTC_FEATURE_ALARM instead of relying on ops->set_alarm to know whether
> alarms are available.
>
> Fixes: 7ae41220ef58 ("rtc: introduce features bitfield")
> Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
> ---
> Hello,
>
> This doesn't seem much but this solve an issue where following a change in the
> RTC driver, this part of the code will think the RTC is alarm capable while it
> is not, then breaking the alarmtimer functionnality.

So a driver has the set_alarm() callback but does not advertise
RTC_FEATURE_ALARM for whatever reason and why ever this makes sense.

I don't mind the patch, but the changelog is a bit meager in explaining
the WHY.

Thanks,

        tglx

Re: [PATCH] alarmtimer: check RTC features instead of ops

[Alexandre Belloni]

On 30/04/2021 09:16:40+0200, Thomas Gleixner wrote:
> On Thu, Apr 29 2021 at 23:49, Alexandre Belloni wrote:
> > Test RTC_FEATURE_ALARM instead of relying on ops->set_alarm to know whether
> > alarms are available.
> >
> > Fixes: 7ae41220ef58 ("rtc: introduce features bitfield")
> > Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
> > ---
> > Hello,
> >
> > This doesn't seem much but this solve an issue where following a change in the
> > RTC driver, this part of the code will think the RTC is alarm capable while it
> > is not, then breaking the alarmtimer functionnality.
> 
> So a driver has the set_alarm() callback but does not advertise
> RTC_FEATURE_ALARM for whatever reason and why ever this makes sense.
> 

No, it would be the other way around. The issue happens when you have
two RTCs, rtc0 is not alarm capable and rtc1 has alarms.

The driver for rtc0 used to not have .set_alarm() to signal it didn't
support alarms, it then switched to RTC_FEATURE_ALARM, making the
alarmtimer code select that RTC instead of rtc1, breaking suspend/resume
on the platform.

> I don't mind the patch, but the changelog is a bit meager in explaining
> the WHY.
> 
> Thanks,
> 
>         tglx

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH] alarmtimer: check RTC features instead of ops

[Thomas Gleixner]

On Fri, Apr 30 2021 at 10:10, Alexandre Belloni wrote:
> On 30/04/2021 09:16:40+0200, Thomas Gleixner wrote:
>> On Thu, Apr 29 2021 at 23:49, Alexandre Belloni wrote:
>> > Test RTC_FEATURE_ALARM instead of relying on ops->set_alarm to know whether
>> > alarms are available.
>> >
>> > Fixes: 7ae41220ef58 ("rtc: introduce features bitfield")
>> > Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
>> > ---
>> > Hello,
>> >
>> > This doesn't seem much but this solve an issue where following a change in the
>> > RTC driver, this part of the code will think the RTC is alarm capable while it
>> > is not, then breaking the alarmtimer functionnality.
>> 
>> So a driver has the set_alarm() callback but does not advertise
>> RTC_FEATURE_ALARM for whatever reason and why ever this makes sense.
>> 
>
> No, it would be the other way around. The issue happens when you have
> two RTCs, rtc0 is not alarm capable and rtc1 has alarms.
>
> The driver for rtc0 used to not have .set_alarm() to signal it didn't
> support alarms, it then switched to RTC_FEATURE_ALARM, making the
> alarmtimer code select that RTC instead of rtc1, breaking suspend/resume
> on the platform.

I'm even more confused. So RTC0 does not have .set_alarm() but why does
it turn on RTC_FEATURE_ALARM? I'm obviously misinterpreting the above...

Thanks,

        tglx

Re: [PATCH] alarmtimer: check RTC features instead of ops

[Alexandre Belloni]

Hello,

On 30/04/2021 10:59:53+0200, Thomas Gleixner wrote:
> On Fri, Apr 30 2021 at 10:10, Alexandre Belloni wrote:
> > On 30/04/2021 09:16:40+0200, Thomas Gleixner wrote:
> >> On Thu, Apr 29 2021 at 23:49, Alexandre Belloni wrote:
> >> > Test RTC_FEATURE_ALARM instead of relying on ops->set_alarm to know whether
> >> > alarms are available.
> >> >
> >> > Fixes: 7ae41220ef58 ("rtc: introduce features bitfield")
> >> > Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
> >> > ---
> >> > Hello,
> >> >
> >> > This doesn't seem much but this solve an issue where following a change in the
> >> > RTC driver, this part of the code will think the RTC is alarm capable while it
> >> > is not, then breaking the alarmtimer functionnality.
> >> 
> >> So a driver has the set_alarm() callback but does not advertise
> >> RTC_FEATURE_ALARM for whatever reason and why ever this makes sense.
> >> 
> >
> > No, it would be the other way around. The issue happens when you have
> > two RTCs, rtc0 is not alarm capable and rtc1 has alarms.
> >
> > The driver for rtc0 used to not have .set_alarm() to signal it didn't
> > support alarms, it then switched to RTC_FEATURE_ALARM, making the
> > alarmtimer code select that RTC instead of rtc1, breaking suspend/resume
> > on the platform.
> 
> I'm even more confused. So RTC0 does not have .set_alarm() but why does
> it turn on RTC_FEATURE_ALARM? I'm obviously misinterpreting the above...
> 

I'm sorry for not being clear.

With RTC0 not having alarms and RTC1 having alarms:

The previous situation was:

The driver for RTC0 didn't have any .set_alarm() to signel it doesn't
support alarms.
On registration, alarmtimer_rtc_add_device finds out it doesn't have the
.set_alarm() callback and doesn't select that RTC.
On registration of RTC1, alarmtimer_rtc_add_device finds .set_alarm()
and RTC1 is now the alarmtimer rtcdev.

The new situation is:

The driver for RTC0 always have .set_alarm() but clears
RTC_FEATURE_ALARM to signal it doesn't support alarms.
On registration, alarmtimer_rtc_add_device finds .set_alarm() and RTC0
is now the alarmtimer rtcdev, leading to an error when rtc_timer_start()
is called.

I hope this is clearer.

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH] alarmtimer: check RTC features instead of ops

[Thomas Gleixner]

On Mon, May 03 2021 at 17:34, Alexandre Belloni wrote:
> On 30/04/2021 10:59:53+0200, Thomas Gleixner wrote:
>> I'm even more confused. So RTC0 does not have .set_alarm() but why does
>> it turn on RTC_FEATURE_ALARM? I'm obviously misinterpreting the above...
>> 
>
> I'm sorry for not being clear.
>
> With RTC0 not having alarms and RTC1 having alarms:
>
> The previous situation was:
>
> The driver for RTC0 didn't have any .set_alarm() to signel it doesn't
> support alarms.
> On registration, alarmtimer_rtc_add_device finds out it doesn't have the
> .set_alarm() callback and doesn't select that RTC.
> On registration of RTC1, alarmtimer_rtc_add_device finds .set_alarm()
> and RTC1 is now the alarmtimer rtcdev.
>
> The new situation is:
>
> The driver for RTC0 always have .set_alarm() but clears
> RTC_FEATURE_ALARM to signal it doesn't support alarms.
> On registration, alarmtimer_rtc_add_device finds .set_alarm() and RTC0
> is now the alarmtimer rtcdev, leading to an error when rtc_timer_start()
> is called.
>
> I hope this is clearer.

Yes, that makes sense now!

Re: [PATCH] alarmtimer: check RTC features instead of ops

[Thomas Gleixner]

On Mon, May 03 2021 at 20:00, Thomas Gleixner wrote:
> On Mon, May 03 2021 at 17:34, Alexandre Belloni wrote:
>> On 30/04/2021 10:59:53+0200, Thomas Gleixner wrote:
>>
>> I hope this is clearer.
>
> Yes, that makes sense now!

Am I expected to rewrite the changelog or is there going to be a V2 of
this?

Thanks,

        tglx