linux-kernel - Re: [RESEND PATCH v2 3/3] rtc: Add driver for Nuvoton ma35d1 rtc controller

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20230810073015d5545903@mail.local>
Date:   Thu, 10 Aug 2023 09:30:15 +0200
From:   Alexandre Belloni <alexandre.belloni@...tlin.com>
To:     Jacky Huang <ychuang570808@...il.com>
Cc:     a.zummo@...ertech.it, robh+dt@...nel.org,
        krzysztof.kozlowski+dt@...aro.org, conor+dt@...nel.org,
        linux-rtc@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        soc@...nel.org, mjchen@...oton.com, schung@...oton.com,
        Jacky Huang <ychuang3@...oton.com>
Subject: Re: [RESEND PATCH v2 3/3] rtc: Add driver for Nuvoton ma35d1 rtc
 controller

On 10/08/2023 15:21:47+0800, Jacky Huang wrote:
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > > +static int ma35d1_rtc_suspend(struct platform_device *pdev, pm_message_t state)
> > > > > +{
> > > > > +	struct ma35_rtc *rtc = platform_get_drvdata(pdev);
> > > > > +	u32 regval;
> > > > > +
> > > > > +	if (device_may_wakeup(&pdev->dev))
> > > > > +		enable_irq_wake(rtc->irq_num);
> > > > > +
> > > > > +	regval = rtc_reg_read(rtc, MA35_REG_RTC_INTEN);
> > > > > +	regval &= ~RTC_INTEN_TICKIEN;
> > > > > +	rtc_reg_write(rtc, MA35_REG_RTC_INTEN, regval);
> > > > This is not what the user is asking, don't do this. Also, how was this
> > > > tested?
> > > Sure, I will remove these three lines of code.
> > > 
> > > We test it with "echo mem > /sys/power/state".
> > > 
> > Yes, my point is that if UIE is enabled, then the user wants to be woken
> > up every second. If this is not what is wanted, then UIE has to be
> > disabled before going to suspend.
> > 
> > My question is why are you enabling RTC_INTEN_TICKIEN in probe? I don't
> > expect anyone to use an actual hardware tick interrupt, unless the alarm
> > is broken and can't be set every second. This is why I questioned the
> > RTC_UF path because I don't expect it to be taken at all.
> 
> Yes, we will remove TICKIEN from probe and modify ma35d1_alarm_irq_enable().
> TICKIEN will be enabled only if UIE is enabled.
> 
> static int ma35d1_alarm_irq_enable(struct device *dev, unsigned int enabled)
> {
>     struct ma35d1_rtc *rtc = dev_get_drvdata(dev);
> 
>     if (enabled) {
>         if (rtc->rtc->uie_rtctimer.enabled)
>             rtc_reg_write(rtc, NVT_RTC_INTEN,
>                       (rtc_reg_read(rtc,
> NVT_RTC_INTEN)|(RTC_INTEN_TICKIEN)));


Don't do that unless the regular alarm can't be set every second. Simply
always use ALMIEN, then check rtctest is passing properly.

>         if (rtc->rtc->aie_timer.enabled)
>             rtc_reg_write(rtc, NVT_RTC_INTEN,
>                       (rtc_reg_read(rtc,
> NVT_RTC_INTEN)|(RTC_INTEN_ALMIEN)));
>     } else {
>         if (rtc->rtc->uie_rtctimer.enabled)
>             rtc_reg_write(rtc, NVT_RTC_INTEN,
>                       (rtc_reg_read(rtc, NVT_RTC_INTEN) &
> (~RTC_INTEN_TICKIEN)));
>         if (rtc->rtc->aie_timer.enabled)
>             rtc_reg_write(rtc, NVT_RTC_INTEN,
>                       (rtc_reg_read(rtc, NVT_RTC_INTEN) &
> (~RTC_INTEN_ALMIEN)));
>     }
>     return 0;
> }
> 

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