linux-kernel - Re: [Resend PATCH v2 1/1] rtc: rtc-pm8xxx: Retrigger RTC alarm if it's fired

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] [day] [month] [year] [list]

Message-ID: <20240124024023df15ef6e@mail.local>
Date: Wed, 24 Jan 2024 03:40:23 +0100
From: Alexandre Belloni <alexandre.belloni@...tlin.com>
To: jianbinz <quic_jianbinz@...cinc.com>
Cc: a.zummo@...ertech.it, linux-rtc@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [Resend PATCH v2 1/1] rtc: rtc-pm8xxx: Retrigger RTC alarm if
 it's fired

Hello,

On 24/01/2024 10:24:43+0800, jianbinz wrote:
> If the alarm is triggered before the driver gets probed, the alarm
> interrupt will be missed and it won't be detected, and the stale
> alarm settings will be still retained because of not being cleared.
> Check this condition during driver probe, retrigger the alarm and
> clear the settings manually if it's such case.
> 
> Changes in v2:
> *Adapt the V1 patch according to the newest rtc-pm8xxx
> 
> Changes in v1:
> *During driver probe: read ALARM_EN, read ALARM_DATA, read RTC_RDATA,
> if (ALARM_DATA < RTC_DATA), Trigger the alarm event and clear the alarm settins
> Link to v1:https://lore.kernel.org/linux-rtc/20220321090514.4523-1-quic_jianbinz@quicinc.com/
> 
> Signed-off-by: jianbinz <quic_jianbinz@...cinc.com>
> ---
>  drivers/rtc/rtc-pm8xxx.c | 57 +++++++++++++++++++++++++++++++++++++---
>  1 file changed, 53 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-pm8xxx.c b/drivers/rtc/rtc-pm8xxx.c
> index f6b779c12ca7..eac4e7f23aaa 100644
> --- a/drivers/rtc/rtc-pm8xxx.c
> +++ b/drivers/rtc/rtc-pm8xxx.c
> @@ -309,21 +309,33 @@ static int pm8xxx_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alarm)
>  	return 0;
>  }
>  
> +static int pm8xxx_rtc_read_alarm_raw(struct pm8xxx_rtc *rtc_dd, u32 *secs)
> +{
> +	const struct pm8xxx_rtc_regs *regs = rtc_dd->regs;
> +	u8 value[NUM_8_BIT_RTC_REGS];
> +	int rc;
> +
> +	rc = regmap_bulk_read(rtc_dd->regmap, regs->read, value, sizeof(value));
> +	if (rc)
> +		return rc;
> +
> +	*secs = get_unaligned_le32(value);
> +
> +	return 0;
> +}
> +
>  static int pm8xxx_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alarm)
>  {
>  	struct pm8xxx_rtc *rtc_dd = dev_get_drvdata(dev);
>  	const struct pm8xxx_rtc_regs *regs = rtc_dd->regs;
> -	u8 value[NUM_8_BIT_RTC_REGS];
>  	unsigned int ctrl_reg;
>  	u32 secs;
>  	int rc;
>  
> -	rc = regmap_bulk_read(rtc_dd->regmap, regs->alarm_rw, value,
> -			      sizeof(value));
> +	rc = pm8xxx_rtc_read_alarm_raw(rtc_dd, &secs);
>  	if (rc)
>  		return rc;
>  
> -	secs = get_unaligned_le32(value);
>  	secs += rtc_dd->offset;
>  	rtc_time64_to_tm(secs, &alarm->time);
>  
> @@ -398,6 +410,39 @@ static irqreturn_t pm8xxx_alarm_trigger(int irq, void *dev_id)
>  	return IRQ_HANDLED;
>  }
>  
> +/*
> + * Trigger the alarm event and clear the alarm settings
> + * if the alarm data has been behind the RTC data which
> + * means the alarm has been triggered before the driver
> + * is probed.
> + */
> +static int pm8xxx_rtc_init_alarm(struct pm8xxx_rtc *rtc_dd)
> +{
> +	int rc;
> +	u32 alarm_sec, rtc_sec;
> +	unsigned int ctrl_reg, alarm_en;
> +	const struct pm8xxx_rtc_regs *regs = rtc_dd->regs;
> +
> +	rc = pm8xxx_rtc_read_raw(rtc_dd, &rtc_sec);
> +	if (rc)
> +		return rc;
> +
> +	rc = pm8xxx_rtc_read_alarm_raw(rtc_dd, &alarm_sec);
> +	if (rc)
> +		return rc;
> +
> +	rc = regmap_read(rtc_dd->regmap, regs->alarm_ctrl, &ctrl_reg);
> +	if (rc)
> +		return rc;
> +
> +	alarm_en = !!(ctrl_reg & PM8xxx_RTC_ALARM_ENABLE);
> +
> +	if (alarm_en && rtc_sec >= alarm_sec)
> +		pm8xxx_alarm_trigger(0, rtc_dd);


I have not been replying because I had to go back to the original
description of the problem which you are not linking here.

Anyway, all of this doesn't do want you explain, you are looking at
whether the alarm is enabled but not whether it has actually
triggered...

> +
> +	return 0;
> +}
> +
>  static int pm8xxx_rtc_enable(struct pm8xxx_rtc *rtc_dd)
>  {
>  	const struct pm8xxx_rtc_regs *regs = rtc_dd->regs;
> @@ -527,6 +572,10 @@ static int pm8xxx_rtc_probe(struct platform_device *pdev)
>  	if (rc)
>  		return rc;
>  
> +	rc = pm8xxx_rtc_init_alarm(rtc_dd);

..But isn't it useless? I guess that what you can do and what I was
referring to in my first reply is clear the interrupt in your probe
unconditionally, here:

rc = regmap_update_bits(rtc_dd->regmap, regs->alarm_ctrl2, PM8xxx_RTC_ALARM_CLEAR, 0);

That should do it just fine. There is no point in calling
pm8xxx_alarm_trigger because there is no chance that userspace is ready
to handle the rtc update.

> +	if (rc)
> +		return rc;
> +
>  	return 0;
>  }
>  
> -- 
> 2.17.1
> 

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