Received: by 2002:a05:6a10:9848:0:0:0:0 with SMTP id x8csp4243155pxf; Tue, 16 Mar 2021 08:48:53 -0700 (PDT) X-Google-Smtp-Source: ABdhPJyQAVHd3K5S1Mmdl27c3UB5OeNHGI2rotHSczIgXeZtO4QypuerJl0+f1jNIawJMCqnLfmY X-Received: by 2002:a17:906:3395:: with SMTP id v21mr30327830eja.322.1615909733708; Tue, 16 Mar 2021 08:48:53 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1615909733; cv=none; d=google.com; s=arc-20160816; b=d3XcQ7Z8bj68QTX8t5lrEO4WZmx5F1oZLLbmZFmC+5eOe6mUjvsoxmZBNXook0sqnK cO5gN86Lu+7JHjmGi+aP4jRzvhycbGXk9Vx+j1wTKPorXjGvBR1UCrv+lRI7vRL1xBH0 3tUiOIxDIgaFWR1BHyOIiFBCcUcFmJLzooAf1Hg5GVtkhcNhNTE68+z0r0p2o6csxZrq Vxj3TvaBz3VET086lLEb4L0ZnAMMUr4ru5AvXW8qJ3iNFAtuHc5n4OP7axIamT/q+X0j Nd0/ovm12whxwzlmGOjTv85hgTa90CbbFoW5buyFZPcjoBPmbu5Vxc4kkcHUgFunIj8w QYbg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:in-reply-to:content-transfer-encoding :content-disposition:mime-version:references:message-id:subject:cc :to:from:date; bh=UnulM5SWwE+G2Zdj3kn1cm27xS2LHoJ0W2dkKg2eV34=; b=oAfv3uhxjpyt08ag/UvS1qIWCH99rRqAKUi5Y4QkEy5QZ3fIn0ajPYZmwPTYUEkC8F +/94KN3aw8cp4yt68og/DmVNdXbMEItUXjsqu0z28S96tSqk0U2O4ESq67Do8XLzRI4l c/2mp/6zJM+8bAbYbsgMwN0JAEHKEaWBAwcAfKRBHwn+ApKQ3zNEWvoCHxk3t5PLZuKF l++WMozNqO06Fwh71vzl3LFzGEARg/C1J3nJy2jm+m6ewJRheBNTkWpcw8P57nCLUeGh 3a/FNFkOB13GxfHqzbCV9Ov66JdTw2Rkf9GFW0afqZUAc+E9Zzj/z4CMYuT3LhcMRTh9 xTlA== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [23.128.96.18]) by mx.google.com with ESMTP id k13si13993994ejc.452.2021.03.16.08.48.30; Tue, 16 Mar 2021 08:48:53 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231800AbhCPMcv (ORCPT + 99 others); Tue, 16 Mar 2021 08:32:51 -0400 Received: from relay9-d.mail.gandi.net ([217.70.183.199]:57769 "EHLO relay9-d.mail.gandi.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231802AbhCPMc2 (ORCPT ); Tue, 16 Mar 2021 08:32:28 -0400 X-Originating-IP: 90.65.108.55 Received: from localhost (lfbn-lyo-1-1676-55.w90-65.abo.wanadoo.fr [90.65.108.55]) (Authenticated sender: alexandre.belloni@bootlin.com) by relay9-d.mail.gandi.net (Postfix) with ESMTPSA id B317EFF80A; Tue, 16 Mar 2021 12:32:20 +0000 (UTC) Date: Tue, 16 Mar 2021 13:32:20 +0100 From: Alexandre Belloni To: Lukasz Stelmach Cc: Alessandro Zummo , linux-rtc@vger.kernel.org, =?utf-8?Q?Bart=C5=82omiej_=C5=BBolnierkiewicz?= , Marek Szyprowski , linux-kernel@vger.kernel.org Subject: Re: [PATCH] rtc: ds1307: set uie_unsupported if no interrupt is available Message-ID: References: <20210305174411.9657-1-l.stelmach@samsung.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > >> --- > >> 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