linux-kernel - Re: [PATCH V5 1/3] rtc: atcrtc100: Add ATCRTC100 RTC driver

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: <Z_jVLzgZjnF1thbq@swlinux02>
Date: Fri, 11 Apr 2025 16:39:11 +0800
From: CL Wang <cl634@...estech.com>
To: Alexandre Belloni <alexandre.belloni@...tlin.com>
CC: <robh@...nel.org>, <krzk+dt@...nel.org>, <conor+dt@...nel.org>,
        <devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <linux-rtc@...r.kernel.org>, <tim609@...estech.com>,
        <ycliang@...estech.com>, <cl634@...estect.com>
Subject: Re: [PATCH V5 1/3] rtc: atcrtc100: Add ATCRTC100 RTC driver

Hi Alexandre,

Thank you very much for your feedback on the patch, and sorry for the delayed response.
Below are my replies to your comments and questions. I will prepare and send the next
version of the patch as soon as possible.

> +#define RTC_MINUTE(x)        ((x >> MIN_OFF) & MIN_MSK)      /* RTC min */
> +#define RTC_HOUR(x)  ((x >> HOUR_OFF) & HOUR_MSK)    /* RTC hour */
> +#define RTC_DAYS(x)  ((x >> DAY_OFF) & DAY_MSK)      /* RTC day */

FIELD_PREP can probably replace those.
=> That's a good suggestion. I will update this to use bitfield-related macros instead.

> +struct atcrtc_dev {
> +     struct rtc_device       *rtc_dev;
> +     struct regmap           *regmap;
> +     struct delayed_work     rtc_work;
> +     struct mutex            lock;

This mutex is not necessary, simply use rtc_lock() in you interrupt handler, the rtc core is already locking before calling the rtc_ops.
=> You're absolutely right. I will remove the mutex and clean up this
   part accordingly.

> +             usleep_range(ATCRTC_TIMEOUT_USLEEP_MIN,
> +                          ATCRTC_TIMEOUT_USLEEP_MAX);
> +     }
> +     dev_err(&rtc->rtc_dev->dev, "Device is busy too long\n");

Is this error message useful, what would be the user action after seeing this?
==> This message indicates that the RTC hardware might be stuck in a busy state.
    If this occurs, it suggests a potential hardware issue. During development, it
    can serve as a hint to review the RTC module's design. In production, a system
    reset might be required to recover. Based on that, I would prefer to keep this
    error message for diagnostic purposes.

> +static time64_t atcrtc_read_rtc_time(struct atcrtc_dev *rtc)

Does this have to be in a separate function?
=> Not necessarily. It can be merged into atcrtc_read_time(). I will
   make this adjustment.

> +     rtc_time64_to_tm(time, tm);
> +     if (rtc_valid_tm(tm) < 0) {

This is not necessary, the core always checks whether the tm is valid.
=> Thanks for pointing that out. I’ll remove this check.

> +     rem -= hour * 3600;
> +     min = rem / 60;
> +     sec = rem - min * 60;

You already had the broken down hour, min and sec, it is not necessary to compute that again here, just fold this function in atcrtc_set_time
=> You're right, I will simplify this part by integrating it directly
   into atcrtc_set_time().

> +     ret = atcrtc_check_write_done(rtc);
> +     if (ret)
> +             return ret;
> +     regmap_update_bits(rtc->regmap, RTC_CR, RTC_EN, RTC_EN);

This is losing some important information, the RTC must only be enabled once the time has been correctly set, then you can check RTC_EN in
atcrtc_read_time() to know whether the time is actually valid or not.
=> I will move the RTC_EN setting to atcrtc_set_time() and add a check for
   this bit in atcrtc_read_time() to ensure the time from RTC is valid.

> +     if (IS_ERR(atcrtc_dev->rtc_dev)) {
> +             dev_err(&pdev->dev,
> +                     "Failed to allocate RTC device: %ld\n",
> +                     PTR_ERR(atcrtc_dev->rtc_dev));
> +             return PTR_ERR(atcrtc_dev->rtc_dev);
> +     }
> +
> +     ret = atcrtc_alarm_enable(&pdev->dev, true);

Can't atcrtc_alarm_enable be part of atcrtc_hw_init so you don't have to wait twice?
=> After reviewing your comment, I agree. I think atcrtc_alarm_enable()
   should instead be integrated into atcrtc_set_alarm() and removed from
   here.

Thanks again for your detailed feedback. I'll revise the patch accordingly
and send out the updated version soon.

Best regards,
CL