| 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
| ||
|
Message-ID: <CAE5UKNp8KYgohe4D_Esrpzi4V-Koi8r1frBedGJk1AnKvQc+Uw@mail.gmail.com>
Date: Thu, 18 Jan 2024 18:28:49 +0100
From: Łukasz Majczak <lma@...omium.org>
To: Krzysztof Kozlowski <krzk@...nel.org>
Cc: Gwendal Grignou <gwendal@...omium.org>, Lee Jones <lee@...nel.org>,
Benson Leung <bleung@...omium.org>, Wim Van Sebroeck <wim@...ux-watchdog.org>,
Tzung-Bi Shih <tzungbi@...nel.org>, Radoslaw Biernacki <biernacki@...gle.com>, linux-kernel@...r.kernel.org,
chrome-platform@...ts.linux.dev, linux-watchdog@...r.kernel.org
Subject: Re: [PATCH] drivers: watchdog: Add ChromeOS EC watchdog
Hi Krzysztof,
Please find the answers below.
On Wed, Jan 17, 2024 at 4:27 PM Krzysztof Kozlowski <krzk@...nel.org> wrote:
>
> On 17/01/2024 11:24, Lukasz Majczak wrote:
> > This adds EC-based watchdog support for ChromeOS
> > based devices equipped with embedded controller (EC).
> >
> > Signed-off-by: Lukasz Majczak <lma@...omium.org>
> > ---
> > MAINTAINERS | 6 +
> > drivers/mfd/cros_ec_dev.c | 9 +
> > drivers/watchdog/Kconfig | 15 +
> > drivers/watchdog/Makefile | 3 +
> > drivers/watchdog/cros_ec_wdt.c | 303 ++++++++++++++++++
> > .../linux/platform_data/cros_ec_commands.h | 78 ++---
> > 6 files changed, 370 insertions(+), 44 deletions(-)
> > create mode 100644 drivers/watchdog/cros_ec_wdt.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 391bbb855cbe..55cd626a525f 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -4952,6 +4952,12 @@ R: Sami Kyöstilä <skyostil@...omium.org>
> > S: Maintained
> > F: drivers/platform/chrome/cros_hps_i2c.c
> >
> > +CHROMEOS EC WATCHDOG
> > +M: Lukasz Majczak <lma@...omium.org>
> > +L: chrome-platform@...ts.linux.dev
> > +S: Maintained
> > +F: drivers/watchdog/cros_ec_wdt.c
> > +
> > CHRONTEL CH7322 CEC DRIVER
> > M: Joe Tessler <jrt@...gle.com>
> > L: linux-media@...r.kernel.org
> > diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
> > index 79d393b602bf..60fe831cf30a 100644
> > --- a/drivers/mfd/cros_ec_dev.c
> > +++ b/drivers/mfd/cros_ec_dev.c
> > @@ -91,6 +91,10 @@ static const struct mfd_cell cros_usbpd_notify_cells[] = {
> > { .name = "cros-usbpd-notify", },
> > };
> >
> > +static const struct mfd_cell cros_ec_wdt_cells[] = {
> > + { .name = "cros-ec-wdt-drv", }
> > +};
> > +
> > static const struct cros_feature_to_cells cros_subdevices[] = {
> > {
> > .id = EC_FEATURE_CEC,
> > @@ -107,6 +111,11 @@ static const struct cros_feature_to_cells cros_subdevices[] = {
> > .mfd_cells = cros_usbpd_charger_cells,
> > .num_cells = ARRAY_SIZE(cros_usbpd_charger_cells),
> > },
> > + {
> > + .id = EC_FEATURE_HANG_DETECT,
> > + .mfd_cells = cros_ec_wdt_cells,
> > + .num_cells = ARRAY_SIZE(cros_ec_wdt_cells),
> > + },
> > };
> >
> > static const struct mfd_cell cros_ec_platform_cells[] = {
> > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> > index 7d22051b15a2..1da4be661be8 100644
> > --- a/drivers/watchdog/Kconfig
> > +++ b/drivers/watchdog/Kconfig
> > @@ -2251,4 +2251,19 @@ config KEEMBAY_WATCHDOG
> > To compile this driver as a module, choose M here: the
> > module will be called keembay_wdt.
> >
> > +#
> > +# ChromeOS EC-based Watchdog
> > +#
>
> Drop comment, useless, copies what's below.
ACK
>
> > +
> > +config CROS_EC_WATCHDOG
> > + tristate "ChromeOS EC-based watchdog driver"
> > + select WATCHDOG_CORE
> > + depends on CROS_EC
> > + help
> > + This is the watchdog driver for ChromeOS devices equipped with EC.
> > + The EC watchdog - when enabled - expects to be kicked within a given
> > + time (default set to 30 seconds) otherwise it will simple reboot
> > + the AP. Priori to that it can also send an event (configurable timeout)
> > + about upcoming reboot.
>
> Instead you could say what will be the name of the module.
>
ACK
> > +
> > endif # WATCHDOG
> > diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> > index 7cbc34514ec1..8295c209ddb0 100644
> > --- a/drivers/watchdog/Makefile
> > +++ b/drivers/watchdog/Makefile
> > @@ -234,3 +234,6 @@ obj-$(CONFIG_MENZ069_WATCHDOG) += menz69_wdt.o
> > obj-$(CONFIG_RAVE_SP_WATCHDOG) += rave-sp-wdt.o
> > obj-$(CONFIG_STPMIC1_WATCHDOG) += stpmic1_wdt.o
> > obj-$(CONFIG_SL28CPLD_WATCHDOG) += sl28cpld_wdt.o
> > +
> > +# Cros EC watchdog
>
> Drop comment.
>
> Also, are you sure you placed it in appropriate place, not just at the
> end of both files?
>
ACK, I will find a better place for above (both in Makefile and
Kconfig) and will try to keep alphabetic order.
> > +obj-$(CONFIG_CROS_EC_WATCHDOG) += cros_ec_wdt.o
> > diff --git a/drivers/watchdog/cros_ec_wdt.c b/drivers/watchdog/cros_ec_wdt.c
> > new file mode 100644
> > index 000000000000..b461c0613269
> > --- /dev/null
> > +++ b/drivers/watchdog/cros_ec_wdt.c
> > @@ -0,0 +1,303 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +
> > +#include <linux/slab.h>
> > +#include <linux/err.h>
> > +#include <linux/of_device.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/platform_data/cros_ec_commands.h>
> > +#include <linux/platform_data/cros_ec_proto.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/watchdog.h>
> > +#include <linux/uaccess.h>
> > +
> > +#define CROS_EC_WATCHDOG_DEFAULT_TIME 30 /* seconds */
> > +
> > +#define DEV_NAME "cros-ec-wdt-dev"
>
> Drop unused defines.
ACK
>
> > +#define DRV_NAME "cros-ec-wdt-drv"
> > +
> > +static int cros_ec_wdt_ping(struct watchdog_device *wdd);
> > +static int cros_ec_wdt_start(struct watchdog_device *wdd);
> > +static int cros_ec_wdt_stop(struct watchdog_device *wdd);
> > +static int cros_ec_wdt_set_timeout(struct watchdog_device *wdd, unsigned int t);
> > +
>
> ...
>
> > +
> > +static int cros_ec_wdt_probe(struct platform_device *pdev)
> > +{
> > + struct device *dev = &pdev->dev;
> > + /* We need to get a reference to cros_ec_devices
> > + * (provides communication layer) which is a parent of
> > + * the cros-ec-dev (our parent)
> > + */
> > + struct cros_ec_device *cros_ec = dev_get_drvdata(dev->parent->parent);
> > + int ret = 0;
> > + uint32_t bootstatus;
> > +
> > + if (!cros_ec) {
> > + ret = -ENODEV;
> > + dev_err_probe(dev, ret, "There is no coresponding EC device!");
>
> Syntax is return dev_err_probe
>
ACK
> > + return ret;
> > + }
> > +
> > + cros_ec_wdd.parent = &pdev->dev;
> > + wd_data.cros_ec = cros_ec;
> > + wd_data.wdd = &cros_ec_wdd;
> > + wd_data.start_on_resume = false;
> > + wd_data.keepalive_on = false;
> > + wd_data.wdd->timeout = CROS_EC_WATCHDOG_DEFAULT_TIME;
> > +
> > + watchdog_stop_on_reboot(&cros_ec_wdd);
> > + watchdog_stop_on_unregister(&cros_ec_wdd);
> > + watchdog_set_drvdata(&cros_ec_wdd, &wd_data);
> > + platform_set_drvdata(pdev, &wd_data);
> > +
> > + /* Get current AP boot status */
> > + ret = cros_ec_wdt_send_hang_detect(&wd_data, EC_HANG_DETECT_CMD_GET_STATUS, 0,
> > + &bootstatus);
> > + if (ret < 0) {
> > + dev_err_probe(dev, ret, "Couldn't get AP boot status from EC");
>
> Syntax is return dev_err_probe
ACK
>
> > + return ret;
> > + }
> > +
> > + /*
> > + * If bootstatus is not EC_HANG_DETECT_AP_BOOT_NORMAL
> > + * it mens EC has rebooted the AP due to watchdog timeout.
> > + * Lets translate it to watchdog core status code.
> > + */
> > + if (bootstatus != EC_HANG_DETECT_AP_BOOT_NORMAL)
> > + wd_data.wdd->bootstatus = WDIOF_CARDRESET;
> > +
> > + ret = watchdog_register_device(&cros_ec_wdd);
> > + if (ret < 0)
> > + dev_err_probe(dev, ret, "Couldn't get AP boot status from EC");
>
> Syntax is return dev_err_probe
>
ACK
> > +
> > + return ret;
>
> return 0
>
ACK
> > +}
> > +
> > +static int cros_ec_wdt_remove(struct platform_device *pdev)
> > +{
> > + struct cros_ec_wdt_data *wd_data = platform_get_drvdata(pdev);
> > +
> > + watchdog_unregister_device(wd_data->wdd);
> > +
> > + return 0;
> > +}
> > +
> > +static void cros_ec_wdt_shutdown(struct platform_device *pdev)
> > +{
> > + struct device *dev = &pdev->dev;
> > + struct cros_ec_wdt_data *wd_data = platform_get_drvdata(pdev);
> > + int ret;
> > +
> > + /*
> > + * Clean only bootstatus flag.
> > + * EC wdt is are already stopped by watchdog framework.
> > + */
> > + ret = cros_ec_wdt_send_hang_detect(wd_data,
> > + EC_HANG_DETECT_CMD_CLEAR_STATUS, 0, NULL);
> > + if (ret < 0)
> > + dev_err(dev, "%s failed (%d)", __func__, ret);
> > +
> > + watchdog_unregister_device(wd_data->wdd);
> > +}
> > +
> > +static int __maybe_unused cros_ec_wdt_suspend(struct platform_device *pdev, pm_message_t state)
> > +{
> > + struct device *dev = &pdev->dev;
> > + struct cros_ec_wdt_data *wd_data = platform_get_drvdata(pdev);
> > + int ret;
> > +
> > + if (watchdog_active(wd_data->wdd)) {
> > + ret = cros_ec_wdt_send_hang_detect(wd_data,
> > + EC_HANG_DETECT_CMD_CANCEL, 0, NULL);
> > + if (ret < 0)
> > + dev_err(dev, "%s failed (%d)", __func__, ret);
> > + wd_data->start_on_resume = true;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int __maybe_unused cros_ec_wdt_resume(struct platform_device *pdev)
> > +{
> > + struct device *dev = &pdev->dev;
> > + struct cros_ec_wdt_data *wd_data = platform_get_drvdata(pdev);
> > + int ret;
> > +
> > + /* start_on_resume is only set if watchdog was active
> > + * when device was going to sleep
> > + */
> > + if (wd_data->start_on_resume) {
> > + /* On resume we just need to setup a EC watchdog the same way as
>
> Use comment style from Linux coding style.
>
ACK
> > + * in cros_ec_wdt_start(). When userspace resumes from suspend
> > + * the watchdog app should just start petting the watchdog again.
> > + */
> > + ret = cros_ec_wdt_start(wd_data->wdd);
> > + if (ret < 0)
> > + dev_err(dev, "%s failed (%d)", __func__, ret);
>
> That's not really helpful. This applies everywhere. You have all over
> the place debugging prints with __func__. This is not useful pattern.
> Print something useful, not function name.
>
ACK, I will revisit the logs messages (both the meanings and log levels)
> > +
> > + wd_data->start_on_resume = false;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static struct platform_driver cros_ec_wdt_driver = {
> > + .probe = cros_ec_wdt_probe,
> > + .remove = cros_ec_wdt_remove,
> > + .shutdown = cros_ec_wdt_shutdown,
> > + .suspend = pm_ptr(cros_ec_wdt_suspend),
> > + .resume = pm_ptr(cros_ec_wdt_resume),
> > + .driver = {
> > + .name = DRV_NAME,
> > + },
> > +};
> > +
> > +module_platform_driver(cros_ec_wdt_driver);
> > +
> > +MODULE_ALIAS("platform:" DRV_NAME);
>
> You should not need MODULE_ALIAS() in normal cases. If you need it,
> usually it means your device ID table is wrong (e.g. misses either
> entries or MODULE_DEVICE_TABLE()). MODULE_ALIAS() is not a substitute
> for incomplete ID table.
>
>
ACK, I will use MODULE_DEVICE_TABLE().
> > +MODULE_DESCRIPTION("Cros EC Watchdog Device Driver");
> > +MODULE_LICENSE("GPL");
> > diff --git a/include/linux/platform_data/cros_ec_commands.h b/include/linux/platform_data/cros_ec_commands.h
> > index 7dae17b62a4d..35a7a2d32819 100644
> > --- a/include/linux/platform_data/cros_ec_commands.h
> > +++ b/include/linux/platform_data/cros_ec_commands.h
> > @@ -3961,60 +3961,50 @@ struct ec_response_i2c_passthru {
> > } __ec_align1;
> >
> > /*****************************************************************************/
> > -/* Power button hang detect */
> > -
> > +/* AP hang detect */
>
> I don't understand how this change is related to the new driver. This
> entire hunk is confusing.
>
Yes, that updates the API and definition for embedded controller (EC).
It should go in the separate patch.
>
> Best regards,
> Krzysztof
>
Best regards,
Lukasz
Powered by blists - more mailing lists