Re: [PATCH RESEND v3 6/7] rtc-cmos: avoid UIP when reading alarm time

From: Alexandre Belloni
Date: Wed Nov 24 2021 - 17:44:20 EST


On 19/11/2021 21:42:20+0100, Mateusz Jończyk wrote:
> Some Intel chipsets disconnect the time and date RTC registers when the
> clock update is in progress: during this time reads may return bogus
> values and writes fail silently. This includes the RTC alarm registers.
> [1]
>
> cmos_read_alarm() did not take account for that, which caused alarm time
> reads to sometimes return bogus values. This can be shown with a test
> patch that I am attaching to this patch series.
>
> [1] 7th Generation Intel ® Processor Family I/O for U/Y Platforms [...]
> Datasheet, Volume 1 of 2 (Intel's Document Number: 334658-006)
> Page 208
> https://www.intel.com/content/dam/www/public/us/en/documents/datasheets/7th-and-8th-gen-core-family-mobile-u-y-processor-lines-i-o-datasheet-vol-1.pdf
> "If a RAM read from the ten time and date bytes is attempted
> during an update cycle, the value read do not necessarily
> represent the true contents of those locations. Any RAM writes
> under the same conditions are ignored."
>
> Signed-off-by: Mateusz Jończyk <mat.jonczyk@xxxxx>
> Cc: Alessandro Zummo <a.zummo@xxxxxxxxxxxx>
> Cc: Alexandre Belloni <alexandre.belloni@xxxxxxxxxxx>
>
> ---
> drivers/rtc/rtc-cmos.c | 72 ++++++++++++++++++++++++++++--------------
> 1 file changed, 49 insertions(+), 23 deletions(-)
>

You have a few checkpatch --strict issues in this patch and the next one
that I would be grateful if you could correct them.

> diff --git a/drivers/rtc/rtc-cmos.c b/drivers/rtc/rtc-cmos.c
> index 9404f58ee01d..2def331e88b6 100644
> --- a/drivers/rtc/rtc-cmos.c
> +++ b/drivers/rtc/rtc-cmos.c
> @@ -242,10 +242,46 @@ static int cmos_set_time(struct device *dev, struct rtc_time *t)
> return mc146818_set_time(t);
> }
>
> +struct cmos_read_alarm_callback_param {
> + struct cmos_rtc *cmos;
> + struct rtc_time *time;
> + unsigned char rtc_control;
> +};
> +
> +static void cmos_read_alarm_callback(unsigned char __always_unused seconds,
> + void *param_in)
> +{
> + struct cmos_read_alarm_callback_param *p =
> + (struct cmos_read_alarm_callback_param *) param_in;
> + struct rtc_time *time = p->time;
> +
> + time->tm_sec = CMOS_READ(RTC_SECONDS_ALARM);
> + time->tm_min = CMOS_READ(RTC_MINUTES_ALARM);
> + time->tm_hour = CMOS_READ(RTC_HOURS_ALARM);
> +
> + if (p->cmos->day_alrm) {
> + /* ignore upper bits on readback per ACPI spec */
> + time->tm_mday = CMOS_READ(p->cmos->day_alrm) & 0x3f;
> + if (!time->tm_mday)
> + time->tm_mday = -1;
> +
> + if (p->cmos->mon_alrm) {
> + time->tm_mon = CMOS_READ(p->cmos->mon_alrm);
> + if (!time->tm_mon)
> + time->tm_mon = -1;
> + }
> + }
> +
> + p->rtc_control = CMOS_READ(RTC_CONTROL);
> +}
> +
> static int cmos_read_alarm(struct device *dev, struct rtc_wkalrm *t)
> {
> struct cmos_rtc *cmos = dev_get_drvdata(dev);
> - unsigned char rtc_control;
> + struct cmos_read_alarm_callback_param p = {
> + .cmos = cmos,
> + .time = &(t->time)
> + };
>
> /* This not only a rtc_op, but also called directly */
> if (!is_valid_irq(cmos->irq))
> @@ -256,28 +292,18 @@ static int cmos_read_alarm(struct device *dev, struct rtc_wkalrm *t)
> * the future.
> */
>
> - spin_lock_irq(&rtc_lock);
> - t->time.tm_sec = CMOS_READ(RTC_SECONDS_ALARM);
> - t->time.tm_min = CMOS_READ(RTC_MINUTES_ALARM);
> - t->time.tm_hour = CMOS_READ(RTC_HOURS_ALARM);
> -
> - if (cmos->day_alrm) {
> - /* ignore upper bits on readback per ACPI spec */
> - t->time.tm_mday = CMOS_READ(cmos->day_alrm) & 0x3f;
> - if (!t->time.tm_mday)
> - t->time.tm_mday = -1;
> -
> - if (cmos->mon_alrm) {
> - t->time.tm_mon = CMOS_READ(cmos->mon_alrm);
> - if (!t->time.tm_mon)
> - t->time.tm_mon = -1;
> - }
> - }
> -
> - rtc_control = CMOS_READ(RTC_CONTROL);
> - spin_unlock_irq(&rtc_lock);
> + /* Some Intel chipsets disconnect the alarm registers when the clock
> + * update is in progress - during this time reads return bogus values
> + * and writes may fail silently. See for example "7th Generation Intel®
> + * Processor Family I/O for U/Y Platforms [...] Datasheet", section
> + * 27.7.1
> + *
> + * Use the mc146818_do_avoiding_UIP() function to avoid this.
> + */
> + if (!mc146818_do_avoiding_UIP(cmos_read_alarm_callback, &p))
> + return -EIO;
>
> - if (!(rtc_control & RTC_DM_BINARY) || RTC_ALWAYS_BCD) {
> + if (!(p.rtc_control & RTC_DM_BINARY) || RTC_ALWAYS_BCD) {
> if (((unsigned)t->time.tm_sec) < 0x60)
> t->time.tm_sec = bcd2bin(t->time.tm_sec);
> else
> @@ -306,7 +332,7 @@ static int cmos_read_alarm(struct device *dev, struct rtc_wkalrm *t)
> }
> }
>
> - t->enabled = !!(rtc_control & RTC_AIE);
> + t->enabled = !!(p.rtc_control & RTC_AIE);
> t->pending = 0;
>
> return 0;
> --
> 2.25.1
>

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