Re: [PATCH RESEND v3 4/7] rtc-mc146818-lib: refactor mc146818_get_time

[Alexandre Belloni]

On 25/11/2021 06:48:38+0100, Mateusz Jończyk wrote:
> W dniu 24.11.2021 o 23:41, Alexandre Belloni pisze:
> > On 19/11/2021 21:42:18+0100, Mateusz Jończyk wrote:
> >> Refactor mc146818_get_time() to make use of mc146818_do_avoiding_UIP().
> >>
> >> Signed-off-by: Mateusz Jończyk <mat.jonczyk@xxxxx>
> >> Cc: Alessandro Zummo <a.zummo@xxxxxxxxxxxx>
> >> Cc: Alexandre Belloni <alexandre.belloni@xxxxxxxxxxx>
> >>
> >> ---
> >>
> >>  drivers/rtc/rtc-mc146818-lib.c | 111 +++++++++++++--------------------
> >>  1 file changed, 43 insertions(+), 68 deletions(-)
> >>
> >> I'm sorry that the diff is quite difficult to read, but I was unable to
> >> fix this easily.
> >>
> >> diff --git a/drivers/rtc/rtc-mc146818-lib.c b/drivers/rtc/rtc-mc146818-lib.c
> >> index 946ad43a512c..f3178244db37 100644
> >> --- a/drivers/rtc/rtc-mc146818-lib.c
> >> +++ b/drivers/rtc/rtc-mc146818-lib.c
> >> @@ -102,50 +102,20 @@ bool mc146818_does_rtc_work(void)
> >>  }
> >>  EXPORT_SYMBOL_GPL(mc146818_does_rtc_work);
> >>  
> >> -unsigned int mc146818_get_time(struct rtc_time *time)
> >> -{
> >> +struct mc146818_get_time_callback_param {
> >> +	struct rtc_time *time;
> >>  	unsigned char ctrl;
> >> -	unsigned long flags;
> >> -	unsigned int iter_count = 0;
> >> -	unsigned char century = 0;
> >> -	bool retry;
> >> -
> >> +#ifdef CONFIG_ACPI
> >> +	unsigned char century;
> >> +#endif
> >>  #ifdef CONFIG_MACH_DECSTATION
> >>  	unsigned int real_year;
> >>  #endif
> >> +};
> >>  
> >> -again:
> >> -	if (iter_count > 10) {
> >> -		pr_err_ratelimited("Unable to read current time from RTC\n");
> >> -		memset(time, 0xff, sizeof(*time));
> >> -		return 0;
> >> -	}
> >> -	iter_count++;
> >> -
> >> -	spin_lock_irqsave(&rtc_lock, flags);
> >> -
> >> -	/*
> >> -	 * Check whether there is an update in progress during which the
> >> -	 * readout is unspecified. The maximum update time is ~2ms. Poll
> >> -	 * every msec for completion.
> >> -	 *
> >> -	 * Store the second value before checking UIP so a long lasting NMI
> >> -	 * which happens to hit after the UIP check cannot make an update
> >> -	 * cycle invisible.
> >> -	 */
> >> -	time->tm_sec = CMOS_READ(RTC_SECONDS);
> >> -
> >> -	if (CMOS_READ(RTC_FREQ_SELECT) & RTC_UIP) {
> >> -		spin_unlock_irqrestore(&rtc_lock, flags);
> >> -		mdelay(1);
> >> -		goto again;
> >> -	}
> >> -
> >> -	/* Revalidate the above readout */
> >> -	if (time->tm_sec != CMOS_READ(RTC_SECONDS)) {
> >> -		spin_unlock_irqrestore(&rtc_lock, flags);
> >> -		goto again;
> >> -	}
> >> +static void mc146818_get_time_callback(unsigned char seconds, void *param_in)
> >> +{
> >> +	struct mc146818_get_time_callback_param *p = param_in;
> >>  
> >>  	/*
> >>  	 * Only the values that we read from the RTC are set. We leave
> >> @@ -153,39 +123,40 @@ unsigned int mc146818_get_time(struct rtc_time *time)
> >>  	 * RTC has RTC_DAY_OF_WEEK, we ignore it, as it is only updated
> >>  	 * by the RTC when initially set to a non-zero value.
> >>  	 */
> >> -	time->tm_min = CMOS_READ(RTC_MINUTES);
> >> -	time->tm_hour = CMOS_READ(RTC_HOURS);
> >> -	time->tm_mday = CMOS_READ(RTC_DAY_OF_MONTH);
> >> -	time->tm_mon = CMOS_READ(RTC_MONTH);
> >> -	time->tm_year = CMOS_READ(RTC_YEAR);
> >> +	p->time->tm_sec = seconds;
> >> +	p->time->tm_min = CMOS_READ(RTC_MINUTES);
> >> +	p->time->tm_hour = CMOS_READ(RTC_HOURS);
> >> +	p->time->tm_mday = CMOS_READ(RTC_DAY_OF_MONTH);
> >> +	p->time->tm_mon = CMOS_READ(RTC_MONTH);
> >> +	p->time->tm_year = CMOS_READ(RTC_YEAR);
> >>  #ifdef CONFIG_MACH_DECSTATION
> >> -	real_year = CMOS_READ(RTC_DEC_YEAR);
> >> +	p->real_year = CMOS_READ(RTC_DEC_YEAR);
> >>  #endif
> >>  #ifdef CONFIG_ACPI
> >>  	if (acpi_gbl_FADT.header.revision >= FADT2_REVISION_ID &&
> >> -	    acpi_gbl_FADT.century)
> >> -		century = CMOS_READ(acpi_gbl_FADT.century);
> >> +	    acpi_gbl_FADT.century) {
> >> +		p->century = CMOS_READ(acpi_gbl_FADT.century);
> >> +	} else {
> >> +		p->century = 0;
> >> +	}
> >>  #endif
> >> -	ctrl = CMOS_READ(RTC_CONTROL);
> >> -	/*
> >> -	 * Check for the UIP bit again. If it is set now then
> >> -	 * the above values may contain garbage.
> >> -	 */
> >> -	retry = CMOS_READ(RTC_FREQ_SELECT) & RTC_UIP;
> >> -	/*
> >> -	 * A NMI might have interrupted the above sequence so check whether
> >> -	 * the seconds value has changed which indicates that the NMI took
> >> -	 * longer than the UIP bit was set. Unlikely, but possible and
> >> -	 * there is also virt...
> >> -	 */
> >> -	retry |= time->tm_sec != CMOS_READ(RTC_SECONDS);
> >>  
> >> -	spin_unlock_irqrestore(&rtc_lock, flags);
> >> +	p->ctrl = CMOS_READ(RTC_CONTROL);
> >> +}
> >>  
> >> -	if (retry)
> >> -		goto again;
> >> +unsigned int mc146818_get_time(struct rtc_time *time)
> >> +{
> >> +	struct mc146818_get_time_callback_param p = {
> >> +		.time = time
> >> +	};
> >>  
> >> -	if (!(ctrl & RTC_DM_BINARY) || RTC_ALWAYS_BCD)
> >> +	if (!mc146818_do_avoiding_UIP(mc146818_get_time_callback, &p)) {
> >> +		pr_err_ratelimited("Unable to read current time from RTC\n");
> >> +		memset(time, 0xff, sizeof(*time));
> >> +		return 0;
> >> +	}
> >> +
> >> +	if (!(p.ctrl & RTC_DM_BINARY) || RTC_ALWAYS_BCD)
> >>  	{
> >>  		time->tm_sec = bcd2bin(time->tm_sec);
> >>  		time->tm_min = bcd2bin(time->tm_min);
> >> @@ -193,15 +164,19 @@ unsigned int mc146818_get_time(struct rtc_time *time)
> >>  		time->tm_mday = bcd2bin(time->tm_mday);
> >>  		time->tm_mon = bcd2bin(time->tm_mon);
> >>  		time->tm_year = bcd2bin(time->tm_year);
> >> -		century = bcd2bin(century);
> >> +#ifdef CONFIG_ACPI
> >> +		p.century = bcd2bin(p.century);
> >> +#endif
> >>  	}
> >>  
> >>  #ifdef CONFIG_MACH_DECSTATION
> >> -	time->tm_year += real_year - 72;
> >> +	time->tm_year += p.real_year - 72;
> >>  #endif
> >>  
> >> -	if (century > 20)
> >> -		time->tm_year += (century - 19) * 100;
> >> +#ifdef CONFIG_ACPI
> > This is an unrelated change
> 
> Well, now, when CONFIG_ACPI is not defined, p.century does not exist, so this #ifdef
> is required. I could unconditionally define "century" in mc146818_get_time_callback_param and set
> it manually to 0 when CONFIG_ACPI is not defined. I thought that this approach is more difficult
> to analyse, so did not use it. Should I do this?
> 

No, that's fine, thanks!

> Previously, when CONFIG_ACPI was not defined, century was always 0, so
> this doesn't change behaviour.
> 
> Greetings,
> 
> Mateusz
> 
> >> +	if (p.century > 20)
> >> +		time->tm_year += (p.century - 19) * 100;
> >> +#endif
> >>  
> >>  	/*
> >>  	 * Account for differences between how the RTC uses the values
> >> -- 
> >> 2.25.1
> >>
> 

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