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@xxxxxxxxxxx>
> >> ---
> >>  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