Messages in this thread | | | From | Prashant Malani <> | Date | Wed, 21 Dec 2022 11:23:14 -0800 | Subject | Re: [PATCH] drivers/cros_ec: Handle CrOS EC panics |
| |
Hi Rob,
I'd suggest using the commit title tag "platform/chrome: cros_ec: ..." for commits which are "ChromeOS EC" wide. That's in line with other recent commits in this directory.
On Wed, Dec 21, 2022 at 10:56 AM Rob Barnes <robbarnes@google.com> wrote: > > Add handler for CrOS EC panic events. When a panic is reported, > poll EC log then force an orderly shutdown. > > This will preserve the EC log leading up to the crash. > > Signed-off-by: Rob Barnes <robbarnes@google.com> > --- > drivers/platform/chrome/cros_ec_debugfs.c | 24 +++++++++++++++++++++ > drivers/platform/chrome/cros_ec_lpc.c | 10 +++++++++ > include/linux/platform_data/cros_ec_proto.h | 9 ++++++++ > 3 files changed, 43 insertions(+) > > diff --git a/drivers/platform/chrome/cros_ec_debugfs.c b/drivers/platform/chrome/cros_ec_debugfs.c > index 21d973fc6be2..31637a4e4cf9 100644 > --- a/drivers/platform/chrome/cros_ec_debugfs.c > +++ b/drivers/platform/chrome/cros_ec_debugfs.c > @@ -49,6 +49,7 @@ struct cros_ec_debugfs { > struct delayed_work log_poll_work; > /* EC panicinfo */ > struct debugfs_blob_wrapper panicinfo_blob; > + struct notifier_block notifier_panic; > }; > > /* > @@ -437,6 +438,23 @@ static int cros_ec_create_panicinfo(struct cros_ec_debugfs *debug_info) > return ret; > } > > +static int cros_ec_debugfs_panic_event(struct notifier_block *nb, > + unsigned long queued_during_suspend, > + void *_notify) > +{ > + struct cros_ec_debugfs *debug_info = > + container_of(nb, struct cros_ec_debugfs, notifier_panic); > + > + if (debug_info->log_buffer.buf) { > + /* Force log poll work to run immediately */ > + mod_delayed_work(debug_info->log_poll_work.wq, &debug_info->log_poll_work, 0); > + /* Block until log poll work finishes */ > + flush_delayed_work(&debug_info->log_poll_work); > + } > + > + return NOTIFY_DONE; > +} > + > static int cros_ec_debugfs_probe(struct platform_device *pd) > { > struct cros_ec_dev *ec = dev_get_drvdata(pd->dev.parent); > @@ -473,6 +491,12 @@ static int cros_ec_debugfs_probe(struct platform_device *pd) > debugfs_create_u16("suspend_timeout_ms", 0664, debug_info->dir, > &ec->ec_dev->suspend_timeout_ms); > > + debug_info->notifier_panic.notifier_call = cros_ec_debugfs_panic_event; > + ret = blocking_notifier_chain_register(&ec->ec_dev->panic_notifier, > + &debug_info->notifier_panic); > + if (ret) > + goto remove_debugfs; > + > ec->debug_info = debug_info; > > dev_set_drvdata(&pd->dev, ec); > diff --git a/drivers/platform/chrome/cros_ec_lpc.c b/drivers/platform/chrome/cros_ec_lpc.c > index 7fc8f82280ac..21958c3b0c28 100644 > --- a/drivers/platform/chrome/cros_ec_lpc.c > +++ b/drivers/platform/chrome/cros_ec_lpc.c > @@ -21,6 +21,7 @@ > #include <linux/platform_data/cros_ec_proto.h> > #include <linux/platform_device.h> > #include <linux/printk.h> > +#include <linux/reboot.h> > #include <linux/suspend.h> > > #include "cros_ec.h" > @@ -332,6 +333,15 @@ static void cros_ec_lpc_acpi_notify(acpi_handle device, u32 value, void *data) > > if (value == ACPI_NOTIFY_DEVICE_WAKE) > pm_system_wakeup(); > + > + if (value == ACPI_NOTIFY_CROS_EC_PANIC) { > + dev_err(ec_dev->dev, > + "CrOS EC Panic Reported. Shutdown is imminent!"); > + blocking_notifier_call_chain(&ec_dev->panic_notifier, 0, > + ec_dev); > + /* Begin orderly shutdown. Force shutdown after 1 second. */ > + hw_protection_shutdown("CrOS EC Panic", 1000);
I feel like this patch is doing 2 things: pulling the logs, and then starting a shutdown. This should be split into 2 patches.
> + } > }
Line limits are now 100 chars[1], so most of these lines can fit on 1 line.
> > static int cros_ec_lpc_probe(struct platform_device *pdev) > diff --git a/include/linux/platform_data/cros_ec_proto.h b/include/linux/platform_data/cros_ec_proto.h > index e43107e0bee1..1c4487271836 100644 > --- a/include/linux/platform_data/cros_ec_proto.h > +++ b/include/linux/platform_data/cros_ec_proto.h > @@ -41,6 +41,13 @@ > #define EC_MAX_REQUEST_OVERHEAD 1 > #define EC_MAX_RESPONSE_OVERHEAD 32 > > +/* > + * EC panic is not covered by the standard (0-F) ACPI notify values. > + * Arbitrarily choosing B0 to notify ec panic, which is in the 84-BF > + * device specific ACPI notify range. > + */ > +#define ACPI_NOTIFY_CROS_EC_PANIC 0xB0
Can you provide a link (either in the commit, or here in the comment) to the coreboot/BIOS code which uses this value? I feel this should be documented in some form that correlates the caller and the callee.
> + > /* > * Command interface between EC and AP, for LPC, I2C and SPI interfaces. > */ > @@ -176,6 +183,8 @@ struct cros_ec_device { > /* The platform devices used by the mfd driver */ > struct platform_device *ec; > struct platform_device *pd; > + > + struct blocking_notifier_head panic_notifier;
Any reason we cannot use the existing event_notifier (with value argument)? It's a system panic, so I doubt that computational overhead for other notifier block listeners is a concern.
BR,
-Prashant
[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=bdc48fa11e46f867ea4d75fa59ee87a7f48be144
| |