linux-kernel - Re: [PATCH v2 5/9] platform/chrome: rtc: Add RTC driver for Wilco EC

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: <20190114222551.GC2589@piout.net>
Date:   Mon, 14 Jan 2019 23:25:51 +0100
From:   Alexandre Belloni <alexandre.belloni@...tlin.com>
To:     Nick Crews <ncrews@...gle.com>
Cc:     linux-kernel@...r.kernel.org, groeck@...omium.org,
        sjg@...omium.org, dlaurie@...omium.org,
        Duncan Laurie <dlaurie@...gle.com>, linux-rtc@...r.kernel.org,
        Enric Balletbo i Serra <enric.balletbo@...labora.com>,
        Alessandro Zummo <a.zummo@...ertech.it>,
        Benson Leung <bleung@...omium.org>
Subject: Re: [PATCH v2 5/9] platform/chrome: rtc: Add RTC driver for Wilco EC

Hello,

On 14/01/2019 15:03:52-0700, Nick Crews wrote:
> diff --git a/drivers/rtc/rtc-wilco-ec.c b/drivers/rtc/rtc-wilco-ec.c
> new file mode 100644
> index 000000000000..d8ed791da82d
> --- /dev/null
> +++ b/drivers/rtc/rtc-wilco-ec.c
> @@ -0,0 +1,178 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * RTC interface for Wilco Embedded Controller with R/W abilities
> + *
> + * Copyright 2018 Google LLC
> + *
> + * The corresponding platform device is typically registered in
> + * drivers/platform/chrome/wilco_ec/core.c
> + */
> +
> +#include <linux/bcd.h>
> +#include <linux/err.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/platform_data/wilco-ec.h>
> +#include <linux/rtc.h>
> +#include <linux/timekeeping.h>
> +
> +#define DRV_NAME "rtc-wilco-ec"

Please get rid of that define.

> +int wilco_ec_rtc_read(struct device *dev, struct rtc_time *tm)
> +{
> +	struct wilco_ec_device *ec = dev_get_drvdata(dev->parent);
> +	u8 param = EC_CMOS_TOD_READ;
> +	struct ec_rtc_read rtc;
> +	struct wilco_ec_message msg = {
> +		.type = WILCO_EC_MSG_LEGACY,
> +		.flags = WILCO_EC_FLAG_RAW_RESPONSE,
> +		.command = EC_COMMAND_CMOS,
> +		.request_data = &param,
> +		.request_size = sizeof(param),
> +		.response_data = &rtc,
> +		.response_size = sizeof(rtc),
> +	};
> +	struct rtc_time calc_tm;
> +	unsigned long time;
> +	int ret;
> +
> +	ret = wilco_ec_mailbox(ec, &msg);
> +	if (ret < 0)
> +		return ret;
> +
> +	tm->tm_sec	= rtc.second;
> +	tm->tm_min	= rtc.minute;
> +	tm->tm_hour	= rtc.hour;
> +	tm->tm_mday	= rtc.day;
> +	/*
> +	 * The RTC stores the month value as 1-12 but the kernel expects 0-11,
> +	 * so ensure invalid/zero month value from RTC is not converted to -1.

I'm pretty sure this is not necessary and will certainly make the core
think the date is valid when it has obviously been corrupted.

> +	 */
> +	tm->tm_mon	= rtc.month ? rtc.month - 1 : 0;
> +	tm->tm_year	= rtc.year + (rtc.century * 100) - 1900;
> +	tm->tm_yday	= rtc_year_days(tm->tm_mday, tm->tm_mon, tm->tm_year);
> +
> +	/* Compute day of week */
> +	rtc_tm_to_time(tm, &time);
> +	rtc_time_to_tm(time, &calc_tm);
> +	tm->tm_wday = calc_tm.tm_wday;

Do you really need an accurate wday? There are no userspace applications
that I know of that care and converting back and forth is quite
expensive.

> +
> +	return 0;
> +}
> +
> +int wilco_ec_rtc_write(struct device *dev, struct rtc_time *tm)
> +{
> +	struct wilco_ec_device *ec = dev_get_drvdata(dev->parent);
> +	struct ec_rtc_write rtc;
> +	struct wilco_ec_message msg = {
> +		.type = WILCO_EC_MSG_LEGACY,
> +		.flags = WILCO_EC_FLAG_RAW_RESPONSE,
> +		.command = EC_COMMAND_CMOS,
> +		.request_data = &rtc,
> +		.request_size = sizeof(rtc),
> +	};
> +	int year = tm->tm_year + 1900;
> +	/* Convert from 0=Sunday to 0=Saturday for the EC */
> +	int wday = tm->tm_wday == 6 ? 0 : tm->tm_wday + 1;

I'm not sure how useful it is to set an accurate wday when the RTC is
not able to give it back.

> +	int ret;
> +
> +	rtc.param	= EC_CMOS_TOD_WRITE;
> +	rtc.century	= bin2bcd(year / 100);
> +	rtc.year	= bin2bcd(year % 100);
> +	rtc.month	= bin2bcd(tm->tm_mon + 1);
> +	rtc.day		= bin2bcd(tm->tm_mday);
> +	rtc.hour	= bin2bcd(tm->tm_hour);
> +	rtc.minute	= bin2bcd(tm->tm_min);
> +	rtc.second	= bin2bcd(tm->tm_sec);
> +	rtc.weekday	= bin2bcd(wday);
> +
> +	ret = wilco_ec_mailbox(ec, &msg);
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static const struct rtc_class_ops wilco_ec_rtc_ops = {
> +	.read_time = wilco_ec_rtc_read,
> +	.set_time = wilco_ec_rtc_write,
> +};
> +
> +static int wilco_ec_rtc_probe(struct platform_device *pdev)
> +{
> +	struct rtc_device *rtc = devm_rtc_device_register(&pdev->dev,
> +							  DRV_NAME,
> +							  &wilco_ec_rtc_ops,
> +							  THIS_MODULE);

Please use devm_rtc_allocate_device() and set rtc->range_min and
rtc->range_max before registering with rtc_register_device().

> +	if (IS_ERR(rtc))
> +		return PTR_ERR(rtc);
> +
> +	return 0;
> +}
> +
> +static struct platform_driver wilco_ec_rtc_driver = {
> +	.driver = {
> +		.name = DRV_NAME,
> +	},
> +	.probe = wilco_ec_rtc_probe,
> +};
> +
> +module_platform_driver(wilco_ec_rtc_driver);
> +
> +MODULE_ALIAS("platform:" DRV_NAME);
> +MODULE_AUTHOR("Nick Crews <ncrews@...gle.com>");
> +MODULE_LICENSE("GPL v2");
> +MODULE_DESCRIPTION("Wilco EC RTC driver");
> -- 
> 2.20.1.97.g81188d93c3-goog
> 

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