[PATCH v5] platform: cros_ec: Query EC protocol version if EC transitions between RO/RW

[PATCH v5] platform: cros_ec: Query EC protocol version if EC transitions between RO/RW

[Yicheng Li]

RO and RW of EC may have different EC protocol version. If EC transitions
between RO and RW, but AP does not reboot (this is true for fingerprint
microcontroller / cros_fp, but not true for main ec / cros_ec), the AP
still uses the protocol version queried before transition, which can
cause problems. In the case of fingerprint microcontroller, this causes
AP to send the wrong version of EC_CMD_GET_NEXT_EVENT to RO in the
interrupt handler, which in turn prevents RO to clear the interrupt
line to AP, in an infinite loop.

Once an EC_HOST_EVENT_INTERFACE_READY is received, we know that there
might have been a transition between RO and RW, so re-query the protocol.

Change-Id: I49a51cc10d22a4ab9e75204a4c0c8819d5b3d282
Signed-off-by: Yicheng Li <yichengli@chromium.org>
---
 drivers/platform/chrome/cros_ec.c           | 24 +++++++++++++++++++++
 include/linux/platform_data/cros_ec_proto.h |  3 +++
 2 files changed, 27 insertions(+)

diff --git a/drivers/platform/chrome/cros_ec.c b/drivers/platform/chrome/cros_ec.c
index 9b2d07422e17..38ec1fb409a5 100644
--- a/drivers/platform/chrome/cros_ec.c
+++ b/drivers/platform/chrome/cros_ec.c
@@ -104,6 +104,23 @@ static int cros_ec_sleep_event(struct cros_ec_device *ec_dev, u8 sleep_event)
 	return ret;
 }
 
+static int cros_ec_ready_event(struct notifier_block *nb,
+	unsigned long queued_during_suspend, void *_notify)
+{
+	struct cros_ec_device *ec_dev = container_of(nb, struct cros_ec_device,
+						     notifier_ready);
+	u32 host_event = cros_ec_get_host_event(ec_dev);
+
+	if (host_event & EC_HOST_EVENT_MASK(EC_HOST_EVENT_INTERFACE_READY)) {
+		mutex_lock(&ec_dev->lock);
+		cros_ec_query_all(ec_dev);
+		mutex_unlock(&ec_dev->lock);
+		return NOTIFY_OK;
+	}
+
+	return NOTIFY_DONE;
+}
+
 /**
  * cros_ec_register() - Register a new ChromeOS EC, using the provided info.
  * @ec_dev: Device to register.
@@ -201,6 +218,13 @@ int cros_ec_register(struct cros_ec_device *ec_dev)
 		dev_dbg(ec_dev->dev, "Error %d clearing sleep event to ec",
 			err);
 
+	/* Register the notifier for EC_HOST_EVENT_INTERFACE_READY event. */
+	ec_dev->notifier_ready.notifier_call = cros_ec_ready_event;
+	err = blocking_notifier_chain_register(&ec_dev->event_notifier,
+					       &ec_dev->notifier_ready);
+	if (err)
+		return err;
+
 	dev_info(dev, "Chrome EC device registered\n");
 
 	return 0;
diff --git a/include/linux/platform_data/cros_ec_proto.h b/include/linux/platform_data/cros_ec_proto.h
index 0d4e4aaed37a..a1c545c464e7 100644
--- a/include/linux/platform_data/cros_ec_proto.h
+++ b/include/linux/platform_data/cros_ec_proto.h
@@ -121,6 +121,8 @@ struct cros_ec_command {
  * @event_data: Raw payload transferred with the MKBP event.
  * @event_size: Size in bytes of the event data.
  * @host_event_wake_mask: Mask of host events that cause wake from suspend.
+ * @notifier_ready: The notifier_block to let the kernel re-query EC
+ *      communication protocol when the EC sends EC_HOST_EVENT_INTERFACE_READY.
  * @ec: The platform_device used by the mfd driver to interface with the
  *      main EC.
  * @pd: The platform_device used by the mfd driver to interface with the
@@ -161,6 +163,7 @@ struct cros_ec_device {
 	int event_size;
 	u32 host_event_wake_mask;
 	u32 last_resume_result;
+	struct notifier_block notifier_ready;
 
 	/* The platform devices used by the mfd driver */
 	struct platform_device *ec;
-- 
2.21.0

Re: [PATCH v5] platform: cros_ec: Query EC protocol version if EC transitions between RO/RW

[Enric Balletbo Serra]

Hi Yicheng,

Missatge de Yicheng Li <yichengli@chromium.org> del dia dt., 7 de gen.
2020 a les 23:10:
>
> RO and RW of EC may have different EC protocol version. If EC transitions
> between RO and RW, but AP does not reboot (this is true for fingerprint
> microcontroller / cros_fp, but not true for main ec / cros_ec), the AP
> still uses the protocol version queried before transition, which can
> cause problems. In the case of fingerprint microcontroller, this causes
> AP to send the wrong version of EC_CMD_GET_NEXT_EVENT to RO in the
> interrupt handler, which in turn prevents RO to clear the interrupt
> line to AP, in an infinite loop.
>
> Once an EC_HOST_EVENT_INTERFACE_READY is received, we know that there
> might have been a transition between RO and RW, so re-query the protocol.
>
> Change-Id: I49a51cc10d22a4ab9e75204a4c0c8819d5b3d282

I changed the subject to 'platform/chrome' and removed the Change-id line

> Signed-off-by: Yicheng Li <yichengli@chromium.org>

And added the Fabien's Tested-by tag

> ---
>  drivers/platform/chrome/cros_ec.c           | 24 +++++++++++++++++++++
>  include/linux/platform_data/cros_ec_proto.h |  3 +++

Fixed a trivial conflict with current chrome-platform tree and applied for-next.

Please remember to cc all the maintainers and maintain a changelog
between version next time.

Thanks,
 Enric


>  2 files changed, 27 insertions(+)
>
> diff --git a/drivers/platform/chrome/cros_ec.c b/drivers/platform/chrome/cros_ec.c
> index 9b2d07422e17..38ec1fb409a5 100644
> --- a/drivers/platform/chrome/cros_ec.c
> +++ b/drivers/platform/chrome/cros_ec.c
> @@ -104,6 +104,23 @@ static int cros_ec_sleep_event(struct cros_ec_device *ec_dev, u8 sleep_event)
>         return ret;
>  }
>
> +static int cros_ec_ready_event(struct notifier_block *nb,
> +       unsigned long queued_during_suspend, void *_notify)
> +{
> +       struct cros_ec_device *ec_dev = container_of(nb, struct cros_ec_device,
> +                                                    notifier_ready);
> +       u32 host_event = cros_ec_get_host_event(ec_dev);
> +
> +       if (host_event & EC_HOST_EVENT_MASK(EC_HOST_EVENT_INTERFACE_READY)) {
> +               mutex_lock(&ec_dev->lock);
> +               cros_ec_query_all(ec_dev);
> +               mutex_unlock(&ec_dev->lock);
> +               return NOTIFY_OK;
> +       }
> +
> +       return NOTIFY_DONE;
> +}
> +
>  /**
>   * cros_ec_register() - Register a new ChromeOS EC, using the provided info.
>   * @ec_dev: Device to register.
> @@ -201,6 +218,13 @@ int cros_ec_register(struct cros_ec_device *ec_dev)
>                 dev_dbg(ec_dev->dev, "Error %d clearing sleep event to ec",
>                         err);
>
> +       /* Register the notifier for EC_HOST_EVENT_INTERFACE_READY event. */
> +       ec_dev->notifier_ready.notifier_call = cros_ec_ready_event;
> +       err = blocking_notifier_chain_register(&ec_dev->event_notifier,
> +                                              &ec_dev->notifier_ready);
> +       if (err)
> +               return err;
> +
>         dev_info(dev, "Chrome EC device registered\n");
>
>         return 0;
> diff --git a/include/linux/platform_data/cros_ec_proto.h b/include/linux/platform_data/cros_ec_proto.h
> index 0d4e4aaed37a..a1c545c464e7 100644
> --- a/include/linux/platform_data/cros_ec_proto.h
> +++ b/include/linux/platform_data/cros_ec_proto.h
> @@ -121,6 +121,8 @@ struct cros_ec_command {
>   * @event_data: Raw payload transferred with the MKBP event.
>   * @event_size: Size in bytes of the event data.
>   * @host_event_wake_mask: Mask of host events that cause wake from suspend.
> + * @notifier_ready: The notifier_block to let the kernel re-query EC
> + *      communication protocol when the EC sends EC_HOST_EVENT_INTERFACE_READY.
>   * @ec: The platform_device used by the mfd driver to interface with the
>   *      main EC.
>   * @pd: The platform_device used by the mfd driver to interface with the
> @@ -161,6 +163,7 @@ struct cros_ec_device {
>         int event_size;
>         u32 host_event_wake_mask;
>         u32 last_resume_result;
> +       struct notifier_block notifier_ready;
>
>         /* The platform devices used by the mfd driver */
>         struct platform_device *ec;
> --
> 2.21.0
>

Re: [PATCH v5] platform: cros_ec: Query EC protocol version if EC transitions between RO/RW

[Marek Szyprowski]

Hi

On 07.01.2020 23:06, Yicheng Li wrote:
> RO and RW of EC may have different EC protocol version. If EC transitions
> between RO and RW, but AP does not reboot (this is true for fingerprint
> microcontroller / cros_fp, but not true for main ec / cros_ec), the AP
> still uses the protocol version queried before transition, which can
> cause problems. In the case of fingerprint microcontroller, this causes
> AP to send the wrong version of EC_CMD_GET_NEXT_EVENT to RO in the
> interrupt handler, which in turn prevents RO to clear the interrupt
> line to AP, in an infinite loop.
>
> Once an EC_HOST_EVENT_INTERFACE_READY is received, we know that there
> might have been a transition between RO and RW, so re-query the protocol.
>
> Change-Id: I49a51cc10d22a4ab9e75204a4c0c8819d5b3d282
> Signed-off-by: Yicheng Li <yichengli@chromium.org>

This patch landed recently in linux-next as commit 
241a69ae8ea8e2defec751fe55dac1287aa044b8. Sadly, it causes following 
kernel oops on any key press on Samsung Exynos-based Chromebooks (Snow, 
Peach-Pit and Peach-Pi):

------------[ cut here ]------------
kernel BUG at drivers/platform/chrome/cros_ec_proto.c:727!
Internal error: Oops - BUG: 0 [#1] PREEMPT SMP ARM
Modules linked in:
CPU: 1 PID: 97 Comm: irq/158-chromeo Not tainted 
5.5.0-rc1-00013-g241a69ae8ea8 #228
Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
PC is at cros_ec_get_host_event+0x34/0x54
LR is at cros_ec_ready_event+0x14/0x44
pc : [<c07a2d5c>]    lr : [<c07a12b4>]    psr: 60000013
sp : ed7c5ec8  ip : eda65f40  fp : c019bc54
r10: eda80380  r9 : eda65c00  r8 : 00000000
r7 : 00000000  r6 : 00000000  r5 : eda65e40  r4 : eda65f40
r3 : 00000000  r2 : eda65e40  r1 : 00000000  r0 : eda65e40
Flags: nZCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none
Control: 10c5387d  Table: 6c44006a  DAC: 00000051
Process irq/158-chromeo (pid: 97, stack limit = 0xe6616d77)
Stack: (0xed7c5ec8 to 0xed7c6000)
5ec0:                   fffffffe eda65e40 00000000 c0151080 eda65ec8 
00000000
5ee0: eda65e40 ffffffff c019c24c c0151354 00000000 0000000d a0000013 
c1007648
5f00: eda65e40 0000000d 00000000 c0151384 00000000 eda65e40 0000000d 
c07a185c
5f20: 00014000 6c161f27 eda65e40 eda65c00 00000001 c07a18a8 eda80380 
c019bc70
5f40: ed7c4000 eda803a4 00000001 c019c2b8 c10bbbae c1007648 00000040 
00000000
5f60: c019bdd4 6c161f27 ed7c4000 eda82280 eda80440 00000000 eda80380 
c019c174
5f80: ee8d7c50 eda822b8 00000000 c014f7d4 eda80440 c014f6a4 00000000 
00000000
5fa0: 00000000 00000000 00000000 c01010b4 00000000 00000000 00000000 
00000000
5fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 
00000000
5fe0: 00000000 00000000 00000000 00000000 00000013 00000000 00000000 
00000000
[<c07a2d5c>] (cros_ec_get_host_event) from [<eda65e40>] (0xeda65e40)
Code: e3530004 1a000002 e59000d5 e12fff1e (e7f001f2)
---[ end trace 9dd28f4b1a9d14be ]---
BUG: sleeping function called from invalid context at 
./include/linux/percpu-rwsem.h:38
in_atomic(): 0, irqs_disabled(): 128, non_block: 0, pid: 97, name: 
irq/158-chromeo
INFO: lockdep is turned off.
irq event stamp: 156
hardirqs last  enabled at (155): [<c0af608c>] 
_raw_spin_unlock_irqrestore+0x68/0x70
hardirqs last disabled at (156): [<c0101b40>] __und_svc+0x60/0x74
softirqs last  enabled at (0): [<c0123530>] copy_process+0x378/0x1994
softirqs last disabled at (0): [<00000000>] 0x0
CPU: 1 PID: 97 Comm: irq/158-chromeo Tainted: G      D 
5.5.0-rc1-00013-g241a69ae8ea8 #228
Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
[<c0111618>] (unwind_backtrace) from [<c010d39c>] (show_stack+0x10/0x14)
[<c010d39c>] (show_stack) from [<c0ad21bc>] (dump_stack+0xb4/0xe0)
[<c0ad21bc>] (dump_stack) from [<c01570c8>] (___might_sleep+0x28c/0x2e0)
[<c01570c8>] (___might_sleep) from [<c013b550>] (exit_signals+0x38/0x3e4)
[<c013b550>] (exit_signals) from [<c012bcd8>] (do_exit+0xcc/0xc20)
[<c012bcd8>] (do_exit) from [<c010d5cc>] (die+0x22c/0x2f0)
[<c010d5cc>] (die) from [<c010d890>] (do_undefinstr+0xbc/0x270)
[<c010d890>] (do_undefinstr) from [<c0101b5c>] (__und_svc_finish+0x0/0x44)
Exception stack(0xed7c5e78 to 0xed7c5ec0)
5e60: eda65e40 00000000
5e80: eda65e40 00000000 eda65f40 eda65e40 00000000 00000000 00000000 
eda65c00
5ea0: eda80380 c019bc54 eda65f40 ed7c5ec8 c07a12b4 c07a2d5c 60000013 
ffffffff
[<c0101b5c>] (__und_svc_finish) from [<c07a2d5c>] 
(cros_ec_get_host_event+0x34/0x54)
[<c07a2d5c>] (cros_ec_get_host_event) from [<eda65e40>] (0xeda65e40)
genirq: exiting task "irq/158-chromeo" (97) is an active IRQ thread (irq 
158)
irq 158: nobody cared (try booting with the "irqpoll" option)
CPU: 0 PID: 0 Comm: swapper/0 Tainted: G      D W 
5.5.0-rc1-00013-g241a69ae8ea8 #228
Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
[<c0111618>] (unwind_backtrace) from [<c010d39c>] (show_stack+0x10/0x14)
[<c010d39c>] (show_stack) from [<c0ad21bc>] (dump_stack+0xb4/0xe0)
[<c0ad21bc>] (dump_stack) from [<c019e6c8>] (__report_bad_irq+0x28/0xc0)
[<c019e6c8>] (__report_bad_irq) from [<c019eb90>] 
(note_interrupt+0x264/0x2b4)
[<c019eb90>] (note_interrupt) from [<c019b33c>] 
(handle_irq_event_percpu+0x5c/0x7c)
[<c019b33c>] (handle_irq_event_percpu) from [<c019b394>] 
(handle_irq_event+0x38/0x5c)
[<c019b394>] (handle_irq_event) from [<c019f770>] 
(handle_level_irq+0xcc/0x150)
[<c019f770>] (handle_level_irq) from [<c0199ff8>] 
(generic_handle_irq+0x24/0x34)
[<c0199ff8>] (generic_handle_irq) from [<c04ccab8>] 
(exynos_irq_eint0_15+0x44/0x98)
[<c04ccab8>] (exynos_irq_eint0_15) from [<c0199ff8>] 
(generic_handle_irq+0x24/0x34)
[<c0199ff8>] (generic_handle_irq) from [<c04c1a40>] 
(combiner_handle_cascade_irq+0x8c/0xdc)
[<c04c1a40>] (combiner_handle_cascade_irq) from [<c0199ff8>] 
(generic_handle_irq+0x24/0x34)
[<c0199ff8>] (generic_handle_irq) from [<c019a618>] 
(__handle_domain_irq+0x7c/0xec)
[<c019a618>] (__handle_domain_irq) from [<c04c1ecc>] 
(gic_handle_irq+0x58/0x9c)
[<c04c1ecc>] (gic_handle_irq) from [<c0101a70>] (__irq_svc+0x70/0xb0)
Exception stack(0xc1001ed0 to 0xc1001f18)
1ec0:                                     c075ce14 00000000 2e00e000 
00000000
1ee0: 00000000 00000000 00000001 c10bca20 00000001 00000000 c1086398 
eefb1e70
1f00: 05355555 c1001f20 c075ce14 c075ce18 60000013 ffffffff
[<c0101a70>] (__irq_svc) from [<c075ce18>] (cpuidle_enter_state+0x318/0x5ac)
[<c075ce18>] (cpuidle_enter_state) from [<c075d0fc>] 
(cpuidle_enter+0x3c/0x54)
[<c075d0fc>] (cpuidle_enter) from [<c0161950>] (do_idle+0x228/0x2b8)
[<c0161950>] (do_idle) from [<c0161d94>] (cpu_startup_entry+0x18/0x1c)
[<c0161d94>] (cpu_startup_entry) from [<c0f00ee0>] 
(start_kernel+0x4a8/0x4d8)
handlers:
[<99982b69>] ec_irq_handler threaded [<8fcfc375>] ec_irq_thread
Disabling IRQ #158

Reverting it make built-in keyboard to operate properly again.

> ---
>   drivers/platform/chrome/cros_ec.c           | 24 +++++++++++++++++++++
>   include/linux/platform_data/cros_ec_proto.h |  3 +++
>   2 files changed, 27 insertions(+)
>
> diff --git a/drivers/platform/chrome/cros_ec.c b/drivers/platform/chrome/cros_ec.c
> index 9b2d07422e17..38ec1fb409a5 100644
> --- a/drivers/platform/chrome/cros_ec.c
> +++ b/drivers/platform/chrome/cros_ec.c
> @@ -104,6 +104,23 @@ static int cros_ec_sleep_event(struct cros_ec_device *ec_dev, u8 sleep_event)
>   	return ret;
>   }
>   
> +static int cros_ec_ready_event(struct notifier_block *nb,
> +	unsigned long queued_during_suspend, void *_notify)
> +{
> +	struct cros_ec_device *ec_dev = container_of(nb, struct cros_ec_device,
> +						     notifier_ready);
> +	u32 host_event = cros_ec_get_host_event(ec_dev);
> +
> +	if (host_event & EC_HOST_EVENT_MASK(EC_HOST_EVENT_INTERFACE_READY)) {
> +		mutex_lock(&ec_dev->lock);
> +		cros_ec_query_all(ec_dev);
> +		mutex_unlock(&ec_dev->lock);
> +		return NOTIFY_OK;
> +	}
> +
> +	return NOTIFY_DONE;
> +}
> +
>   /**
>    * cros_ec_register() - Register a new ChromeOS EC, using the provided info.
>    * @ec_dev: Device to register.
> @@ -201,6 +218,13 @@ int cros_ec_register(struct cros_ec_device *ec_dev)
>   		dev_dbg(ec_dev->dev, "Error %d clearing sleep event to ec",
>   			err);
>   
> +	/* Register the notifier for EC_HOST_EVENT_INTERFACE_READY event. */
> +	ec_dev->notifier_ready.notifier_call = cros_ec_ready_event;
> +	err = blocking_notifier_chain_register(&ec_dev->event_notifier,
> +					       &ec_dev->notifier_ready);
> +	if (err)
> +		return err;
> +
>   	dev_info(dev, "Chrome EC device registered\n");
>   
>   	return 0;
> diff --git a/include/linux/platform_data/cros_ec_proto.h b/include/linux/platform_data/cros_ec_proto.h
> index 0d4e4aaed37a..a1c545c464e7 100644
> --- a/include/linux/platform_data/cros_ec_proto.h
> +++ b/include/linux/platform_data/cros_ec_proto.h
> @@ -121,6 +121,8 @@ struct cros_ec_command {
>    * @event_data: Raw payload transferred with the MKBP event.
>    * @event_size: Size in bytes of the event data.
>    * @host_event_wake_mask: Mask of host events that cause wake from suspend.
> + * @notifier_ready: The notifier_block to let the kernel re-query EC
> + *      communication protocol when the EC sends EC_HOST_EVENT_INTERFACE_READY.
>    * @ec: The platform_device used by the mfd driver to interface with the
>    *      main EC.
>    * @pd: The platform_device used by the mfd driver to interface with the
> @@ -161,6 +163,7 @@ struct cros_ec_device {
>   	int event_size;
>   	u32 host_event_wake_mask;
>   	u32 last_resume_result;
> +	struct notifier_block notifier_ready;
>   
>   	/* The platform devices used by the mfd driver */
>   	struct platform_device *ec;

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH v5] platform: cros_ec: Query EC protocol version if EC transitions between RO/RW

[Enric Balletbo Serra]

Hi Marek,

Missatge de Marek Szyprowski <m.szyprowski@samsung.com> del dia dv.,
31 de gen. 2020 a les 8:41:
>
> Hi
>
> On 07.01.2020 23:06, Yicheng Li wrote:
> > RO and RW of EC may have different EC protocol version. If EC transitions
> > between RO and RW, but AP does not reboot (this is true for fingerprint
> > microcontroller / cros_fp, but not true for main ec / cros_ec), the AP
> > still uses the protocol version queried before transition, which can
> > cause problems. In the case of fingerprint microcontroller, this causes
> > AP to send the wrong version of EC_CMD_GET_NEXT_EVENT to RO in the
> > interrupt handler, which in turn prevents RO to clear the interrupt
> > line to AP, in an infinite loop.
> >
> > Once an EC_HOST_EVENT_INTERFACE_READY is received, we know that there
> > might have been a transition between RO and RW, so re-query the protocol.
> >
> > Change-Id: I49a51cc10d22a4ab9e75204a4c0c8819d5b3d282
> > Signed-off-by: Yicheng Li <yichengli@chromium.org>
>
> This patch landed recently in linux-next as commit
> 241a69ae8ea8e2defec751fe55dac1287aa044b8. Sadly, it causes following
> kernel oops on any key press on Samsung Exynos-based Chromebooks (Snow,
> Peach-Pit and Peach-Pi):
>

Many thanks for report the issue, we will take a look ASAP and revert
this commit meanwhile.

Thanks,
 Enric

> ------------[ cut here ]------------
> kernel BUG at drivers/platform/chrome/cros_ec_proto.c:727!
> Internal error: Oops - BUG: 0 [#1] PREEMPT SMP ARM
> Modules linked in:
> CPU: 1 PID: 97 Comm: irq/158-chromeo Not tainted
> 5.5.0-rc1-00013-g241a69ae8ea8 #228
> Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
> PC is at cros_ec_get_host_event+0x34/0x54
> LR is at cros_ec_ready_event+0x14/0x44
> pc : [<c07a2d5c>]    lr : [<c07a12b4>]    psr: 60000013
> sp : ed7c5ec8  ip : eda65f40  fp : c019bc54
> r10: eda80380  r9 : eda65c00  r8 : 00000000
> r7 : 00000000  r6 : 00000000  r5 : eda65e40  r4 : eda65f40
> r3 : 00000000  r2 : eda65e40  r1 : 00000000  r0 : eda65e40
> Flags: nZCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none
> Control: 10c5387d  Table: 6c44006a  DAC: 00000051
> Process irq/158-chromeo (pid: 97, stack limit = 0xe6616d77)
> Stack: (0xed7c5ec8 to 0xed7c6000)
> 5ec0:                   fffffffe eda65e40 00000000 c0151080 eda65ec8
> 00000000
> 5ee0: eda65e40 ffffffff c019c24c c0151354 00000000 0000000d a0000013
> c1007648
> 5f00: eda65e40 0000000d 00000000 c0151384 00000000 eda65e40 0000000d
> c07a185c
> 5f20: 00014000 6c161f27 eda65e40 eda65c00 00000001 c07a18a8 eda80380
> c019bc70
> 5f40: ed7c4000 eda803a4 00000001 c019c2b8 c10bbbae c1007648 00000040
> 00000000
> 5f60: c019bdd4 6c161f27 ed7c4000 eda82280 eda80440 00000000 eda80380
> c019c174
> 5f80: ee8d7c50 eda822b8 00000000 c014f7d4 eda80440 c014f6a4 00000000
> 00000000
> 5fa0: 00000000 00000000 00000000 c01010b4 00000000 00000000 00000000
> 00000000
> 5fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000
> 00000000
> 5fe0: 00000000 00000000 00000000 00000000 00000013 00000000 00000000
> 00000000
> [<c07a2d5c>] (cros_ec_get_host_event) from [<eda65e40>] (0xeda65e40)
> Code: e3530004 1a000002 e59000d5 e12fff1e (e7f001f2)
> ---[ end trace 9dd28f4b1a9d14be ]---
> BUG: sleeping function called from invalid context at
> ./include/linux/percpu-rwsem.h:38
> in_atomic(): 0, irqs_disabled(): 128, non_block: 0, pid: 97, name:
> irq/158-chromeo
> INFO: lockdep is turned off.
> irq event stamp: 156
> hardirqs last  enabled at (155): [<c0af608c>]
> _raw_spin_unlock_irqrestore+0x68/0x70
> hardirqs last disabled at (156): [<c0101b40>] __und_svc+0x60/0x74
> softirqs last  enabled at (0): [<c0123530>] copy_process+0x378/0x1994
> softirqs last disabled at (0): [<00000000>] 0x0
> CPU: 1 PID: 97 Comm: irq/158-chromeo Tainted: G      D
> 5.5.0-rc1-00013-g241a69ae8ea8 #228
> Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
> [<c0111618>] (unwind_backtrace) from [<c010d39c>] (show_stack+0x10/0x14)
> [<c010d39c>] (show_stack) from [<c0ad21bc>] (dump_stack+0xb4/0xe0)
> [<c0ad21bc>] (dump_stack) from [<c01570c8>] (___might_sleep+0x28c/0x2e0)
> [<c01570c8>] (___might_sleep) from [<c013b550>] (exit_signals+0x38/0x3e4)
> [<c013b550>] (exit_signals) from [<c012bcd8>] (do_exit+0xcc/0xc20)
> [<c012bcd8>] (do_exit) from [<c010d5cc>] (die+0x22c/0x2f0)
> [<c010d5cc>] (die) from [<c010d890>] (do_undefinstr+0xbc/0x270)
> [<c010d890>] (do_undefinstr) from [<c0101b5c>] (__und_svc_finish+0x0/0x44)
> Exception stack(0xed7c5e78 to 0xed7c5ec0)
> 5e60: eda65e40 00000000
> 5e80: eda65e40 00000000 eda65f40 eda65e40 00000000 00000000 00000000
> eda65c00
> 5ea0: eda80380 c019bc54 eda65f40 ed7c5ec8 c07a12b4 c07a2d5c 60000013
> ffffffff
> [<c0101b5c>] (__und_svc_finish) from [<c07a2d5c>]
> (cros_ec_get_host_event+0x34/0x54)
> [<c07a2d5c>] (cros_ec_get_host_event) from [<eda65e40>] (0xeda65e40)
> genirq: exiting task "irq/158-chromeo" (97) is an active IRQ thread (irq
> 158)
> irq 158: nobody cared (try booting with the "irqpoll" option)
> CPU: 0 PID: 0 Comm: swapper/0 Tainted: G      D W
> 5.5.0-rc1-00013-g241a69ae8ea8 #228
> Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
> [<c0111618>] (unwind_backtrace) from [<c010d39c>] (show_stack+0x10/0x14)
> [<c010d39c>] (show_stack) from [<c0ad21bc>] (dump_stack+0xb4/0xe0)
> [<c0ad21bc>] (dump_stack) from [<c019e6c8>] (__report_bad_irq+0x28/0xc0)
> [<c019e6c8>] (__report_bad_irq) from [<c019eb90>]
> (note_interrupt+0x264/0x2b4)
> [<c019eb90>] (note_interrupt) from [<c019b33c>]
> (handle_irq_event_percpu+0x5c/0x7c)
> [<c019b33c>] (handle_irq_event_percpu) from [<c019b394>]
> (handle_irq_event+0x38/0x5c)
> [<c019b394>] (handle_irq_event) from [<c019f770>]
> (handle_level_irq+0xcc/0x150)
> [<c019f770>] (handle_level_irq) from [<c0199ff8>]
> (generic_handle_irq+0x24/0x34)
> [<c0199ff8>] (generic_handle_irq) from [<c04ccab8>]
> (exynos_irq_eint0_15+0x44/0x98)
> [<c04ccab8>] (exynos_irq_eint0_15) from [<c0199ff8>]
> (generic_handle_irq+0x24/0x34)
> [<c0199ff8>] (generic_handle_irq) from [<c04c1a40>]
> (combiner_handle_cascade_irq+0x8c/0xdc)
> [<c04c1a40>] (combiner_handle_cascade_irq) from [<c0199ff8>]
> (generic_handle_irq+0x24/0x34)
> [<c0199ff8>] (generic_handle_irq) from [<c019a618>]
> (__handle_domain_irq+0x7c/0xec)
> [<c019a618>] (__handle_domain_irq) from [<c04c1ecc>]
> (gic_handle_irq+0x58/0x9c)
> [<c04c1ecc>] (gic_handle_irq) from [<c0101a70>] (__irq_svc+0x70/0xb0)
> Exception stack(0xc1001ed0 to 0xc1001f18)
> 1ec0:                                     c075ce14 00000000 2e00e000
> 00000000
> 1ee0: 00000000 00000000 00000001 c10bca20 00000001 00000000 c1086398
> eefb1e70
> 1f00: 05355555 c1001f20 c075ce14 c075ce18 60000013 ffffffff
> [<c0101a70>] (__irq_svc) from [<c075ce18>] (cpuidle_enter_state+0x318/0x5ac)
> [<c075ce18>] (cpuidle_enter_state) from [<c075d0fc>]
> (cpuidle_enter+0x3c/0x54)
> [<c075d0fc>] (cpuidle_enter) from [<c0161950>] (do_idle+0x228/0x2b8)
> [<c0161950>] (do_idle) from [<c0161d94>] (cpu_startup_entry+0x18/0x1c)
> [<c0161d94>] (cpu_startup_entry) from [<c0f00ee0>]
> (start_kernel+0x4a8/0x4d8)
> handlers:
> [<99982b69>] ec_irq_handler threaded [<8fcfc375>] ec_irq_thread
> Disabling IRQ #158
>
> Reverting it make built-in keyboard to operate properly again.
>
> > ---
> >   drivers/platform/chrome/cros_ec.c           | 24 +++++++++++++++++++++
> >   include/linux/platform_data/cros_ec_proto.h |  3 +++
> >   2 files changed, 27 insertions(+)
> >
> > diff --git a/drivers/platform/chrome/cros_ec.c b/drivers/platform/chrome/cros_ec.c
> > index 9b2d07422e17..38ec1fb409a5 100644
> > --- a/drivers/platform/chrome/cros_ec.c
> > +++ b/drivers/platform/chrome/cros_ec.c
> > @@ -104,6 +104,23 @@ static int cros_ec_sleep_event(struct cros_ec_device *ec_dev, u8 sleep_event)
> >       return ret;
> >   }
> >
> > +static int cros_ec_ready_event(struct notifier_block *nb,
> > +     unsigned long queued_during_suspend, void *_notify)
> > +{
> > +     struct cros_ec_device *ec_dev = container_of(nb, struct cros_ec_device,
> > +                                                  notifier_ready);
> > +     u32 host_event = cros_ec_get_host_event(ec_dev);
> > +
> > +     if (host_event & EC_HOST_EVENT_MASK(EC_HOST_EVENT_INTERFACE_READY)) {
> > +             mutex_lock(&ec_dev->lock);
> > +             cros_ec_query_all(ec_dev);
> > +             mutex_unlock(&ec_dev->lock);
> > +             return NOTIFY_OK;
> > +     }
> > +
> > +     return NOTIFY_DONE;
> > +}
> > +
> >   /**
> >    * cros_ec_register() - Register a new ChromeOS EC, using the provided info.
> >    * @ec_dev: Device to register.
> > @@ -201,6 +218,13 @@ int cros_ec_register(struct cros_ec_device *ec_dev)
> >               dev_dbg(ec_dev->dev, "Error %d clearing sleep event to ec",
> >                       err);
> >
> > +     /* Register the notifier for EC_HOST_EVENT_INTERFACE_READY event. */
> > +     ec_dev->notifier_ready.notifier_call = cros_ec_ready_event;
> > +     err = blocking_notifier_chain_register(&ec_dev->event_notifier,
> > +                                            &ec_dev->notifier_ready);
> > +     if (err)
> > +             return err;
> > +
> >       dev_info(dev, "Chrome EC device registered\n");
> >
> >       return 0;
> > diff --git a/include/linux/platform_data/cros_ec_proto.h b/include/linux/platform_data/cros_ec_proto.h
> > index 0d4e4aaed37a..a1c545c464e7 100644
> > --- a/include/linux/platform_data/cros_ec_proto.h
> > +++ b/include/linux/platform_data/cros_ec_proto.h
> > @@ -121,6 +121,8 @@ struct cros_ec_command {
> >    * @event_data: Raw payload transferred with the MKBP event.
> >    * @event_size: Size in bytes of the event data.
> >    * @host_event_wake_mask: Mask of host events that cause wake from suspend.
> > + * @notifier_ready: The notifier_block to let the kernel re-query EC
> > + *      communication protocol when the EC sends EC_HOST_EVENT_INTERFACE_READY.
> >    * @ec: The platform_device used by the mfd driver to interface with the
> >    *      main EC.
> >    * @pd: The platform_device used by the mfd driver to interface with the
> > @@ -161,6 +163,7 @@ struct cros_ec_device {
> >       int event_size;
> >       u32 host_event_wake_mask;
> >       u32 last_resume_result;
> > +     struct notifier_block notifier_ready;
> >
> >       /* The platform devices used by the mfd driver */
> >       struct platform_device *ec;
>
> Best regards
> --
> Marek Szyprowski, PhD
> Samsung R&D Institute Poland
>

Re: [PATCH v5] platform: cros_ec: Query EC protocol version if EC transitions between RO/RW

[Marek Szyprowski]

Hi Enric

On 03.02.2020 10:33, Enric Balletbo Serra wrote:
> Missatge de Marek Szyprowski <m.szyprowski@samsung.com> del dia dv.,
> 31 de gen. 2020 a les 8:41:
>> On 07.01.2020 23:06, Yicheng Li wrote:
>>> RO and RW of EC may have different EC protocol version. If EC transitions
>>> between RO and RW, but AP does not reboot (this is true for fingerprint
>>> microcontroller / cros_fp, but not true for main ec / cros_ec), the AP
>>> still uses the protocol version queried before transition, which can
>>> cause problems. In the case of fingerprint microcontroller, this causes
>>> AP to send the wrong version of EC_CMD_GET_NEXT_EVENT to RO in the
>>> interrupt handler, which in turn prevents RO to clear the interrupt
>>> line to AP, in an infinite loop.
>>>
>>> Once an EC_HOST_EVENT_INTERFACE_READY is received, we know that there
>>> might have been a transition between RO and RW, so re-query the protocol.
>>>
>>> Change-Id: I49a51cc10d22a4ab9e75204a4c0c8819d5b3d282
>>> Signed-off-by: Yicheng Li <yichengli@chromium.org>
>> This patch landed recently in linux-next as commit
>> 241a69ae8ea8e2defec751fe55dac1287aa044b8. Sadly, it causes following
>> kernel oops on any key press on Samsung Exynos-based Chromebooks (Snow,
>> Peach-Pit and Peach-Pi):
> Many thanks for report the issue, we will take a look ASAP and revert
> this commit meanwhile.

Simply removing the BUG_ON() from cros_ec_get_host_event() function 
fixes the issue, but I don't know the protocol details to judge if this 
is the correct way of fixing it.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH v5] platform: cros_ec: Query EC protocol version if EC transitions between RO/RW

[Marek Szyprowski]

Hi

On 04.02.2020 00:00, Yicheng Li wrote:
> Hi Enric and Marek,
>
> Those Samsung Chromebooks (Snow, Peach-Pit and Peach-Pi) do not 
> support mkbp events, thus we should not do this "ready event handling" 
> for them.
> Below is my fix, I also sent the entire thing as a v6 in a separate 
> email. Thanks!
>
> diff --git a/drivers/platform/chrome/cros_ec.c 
> b/drivers/platform/chrome/cros_ec.c
> index 38ec1fb409a5..f16804db805b 100644
> --- a/drivers/platform/chrome/cros_ec.c
> +++ b/drivers/platform/chrome/cros_ec.c
> @@ -218,12 +218,17 @@ int cros_ec_register(struct cros_ec_device *ec_dev)
>                 dev_dbg(ec_dev->dev, "Error %d clearing sleep event to 
> ec",
>                         err);
>
> -       /* Register the notifier for EC_HOST_EVENT_INTERFACE_READY 
> event. */
> -       ec_dev->notifier_ready.notifier_call = cros_ec_ready_event;
> -       err = blocking_notifier_chain_register(&ec_dev->event_notifier,
> -  &ec_dev->notifier_ready);
> -       if (err)
> -               return err;
> +       if (ec_dev->mkbp_event_supported) {
> +               /*
> +                * Register the notifier for EC_HOST_EVENT_INTERFACE_READY
> +                * event.
> +                */
> +               ec_dev->notifier_ready.notifier_call = 
> cros_ec_ready_event;
> +               err = blocking_notifier_chain_register(
> +                       &ec_dev->event_notifier, &ec_dev->notifier_ready);
> +               if (err)
> +                       return err;
> +       }
>
>         dev_info(dev, "Chrome EC device registered\n");
>
I confirm that this patch fixes the issue on Samsung Exynos-based 
Chromebooks (Snow, Peach-Pit and Peach-Pi).

 > ...

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland