* [PATCH v2 1/6] platform/chrome: cros_ec: fix error handling in cros_ec_register()
2022-02-09 4:50 [PATCH v2 0/6] platform/chrome: cros_ec: miscellaneous cleanups Tzung-Bi Shih
@ 2022-02-09 4:50 ` Tzung-Bi Shih
2022-02-09 5:38 ` Prashant Malani
2022-02-09 4:50 ` [PATCH v2 2/6] platform/chrome: cros_ec: remove unused variable `was_wake_device` Tzung-Bi Shih
` (4 subsequent siblings)
5 siblings, 1 reply; 19+ messages in thread
From: Tzung-Bi Shih @ 2022-02-09 4:50 UTC (permalink / raw)
To: bleung, groeck; +Cc: chrome-platform, tzungbi
Fix cros_ec_register() doesn't unregister platform devices if
blocking_notifier_chain_register() fails.
Also use the single exit path to handle the platform device
unregistration.
This fix depends on the fact that all the callers of cros_ec_register()
allocate zeroed memory.
Fixes: 42cd0ab476e2 ("platform/chrome: cros_ec: Query EC protocol version if EC transitions between RO/RW")
Signed-off-by: Tzung-Bi Shih <tzungbi@google.com>
---
drivers/platform/chrome/cros_ec.c | 16 ++++++++++------
1 file changed, 10 insertions(+), 6 deletions(-)
diff --git a/drivers/platform/chrome/cros_ec.c b/drivers/platform/chrome/cros_ec.c
index fc5aa1525d13..7ce667ff08e0 100644
--- a/drivers/platform/chrome/cros_ec.c
+++ b/drivers/platform/chrome/cros_ec.c
@@ -245,18 +245,16 @@ int cros_ec_register(struct cros_ec_device *ec_dev)
if (IS_ERR(ec_dev->pd)) {
dev_err(ec_dev->dev,
"Failed to create CrOS PD platform device\n");
- platform_device_unregister(ec_dev->ec);
- return PTR_ERR(ec_dev->pd);
+ err = PTR_ERR(ec_dev->pd);
+ goto exit;
}
}
if (IS_ENABLED(CONFIG_OF) && dev->of_node) {
err = devm_of_platform_populate(dev);
if (err) {
- platform_device_unregister(ec_dev->pd);
- platform_device_unregister(ec_dev->ec);
dev_err(dev, "Failed to register sub-devices\n");
- return err;
+ goto exit;
}
}
@@ -278,7 +276,7 @@ int cros_ec_register(struct cros_ec_device *ec_dev)
err = blocking_notifier_chain_register(&ec_dev->event_notifier,
&ec_dev->notifier_ready);
if (err)
- return err;
+ goto exit;
}
dev_info(dev, "Chrome EC device registered\n");
@@ -291,6 +289,12 @@ int cros_ec_register(struct cros_ec_device *ec_dev)
cros_ec_irq_thread(0, ec_dev);
return 0;
+exit:
+ if (!IS_ERR_OR_NULL(ec_dev->pd))
+ platform_device_unregister(ec_dev->pd);
+ if (!IS_ERR_OR_NULL(ec_dev->ec))
+ platform_device_unregister(ec_dev->ec);
+ return err;
}
EXPORT_SYMBOL(cros_ec_register);
--
2.35.0.263.gb82422642f-goog
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v2 1/6] platform/chrome: cros_ec: fix error handling in cros_ec_register()
2022-02-09 4:50 ` [PATCH v2 1/6] platform/chrome: cros_ec: fix error handling in cros_ec_register() Tzung-Bi Shih
@ 2022-02-09 5:38 ` Prashant Malani
2022-02-09 9:35 ` Tzung-Bi Shih
0 siblings, 1 reply; 19+ messages in thread
From: Prashant Malani @ 2022-02-09 5:38 UTC (permalink / raw)
To: Tzung-Bi Shih; +Cc: bleung, groeck, chrome-platform
Hi,
On Tue, Feb 8, 2022 at 8:50 PM Tzung-Bi Shih <tzungbi@google.com> wrote:
>
> Fix cros_ec_register() doesn't unregister platform devices if
> blocking_notifier_chain_register() fails.
This isn't grammatically correct. Instead:
Fix cros_ec_register() to unregister platform devices if blocking....
>
> Also use the single exit path to handle the platform device
> unregistration.
>
> This fix depends on the fact that all the callers of cros_ec_register()
> allocate zeroed memory.
Is that a fair assumption? What happens if a future driver calls
cros_ec_register()
without zeroed memory?
>
> Fixes: 42cd0ab476e2 ("platform/chrome: cros_ec: Query EC protocol version if EC transitions between RO/RW")
> Signed-off-by: Tzung-Bi Shih <tzungbi@google.com>
> ---
What has changed since v2? Please add a change log of the versions here.
> drivers/platform/chrome/cros_ec.c | 16 ++++++++++------
> 1 file changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/platform/chrome/cros_ec.c b/drivers/platform/chrome/cros_ec.c
> index fc5aa1525d13..7ce667ff08e0 100644
> --- a/drivers/platform/chrome/cros_ec.c
> +++ b/drivers/platform/chrome/cros_ec.c
> @@ -245,18 +245,16 @@ int cros_ec_register(struct cros_ec_device *ec_dev)
> if (IS_ERR(ec_dev->pd)) {
> dev_err(ec_dev->dev,
> "Failed to create CrOS PD platform device\n");
> - platform_device_unregister(ec_dev->ec);
> - return PTR_ERR(ec_dev->pd);
> + err = PTR_ERR(ec_dev->pd);
> + goto exit;
> }
> }
>
> if (IS_ENABLED(CONFIG_OF) && dev->of_node) {
> err = devm_of_platform_populate(dev);
> if (err) {
> - platform_device_unregister(ec_dev->pd);
> - platform_device_unregister(ec_dev->ec);
> dev_err(dev, "Failed to register sub-devices\n");
> - return err;
> + goto exit;
> }
> }
>
> @@ -278,7 +276,7 @@ int cros_ec_register(struct cros_ec_device *ec_dev)
> err = blocking_notifier_chain_register(&ec_dev->event_notifier,
> &ec_dev->notifier_ready);
> if (err)
> - return err;
> + goto exit;
> }
>
> dev_info(dev, "Chrome EC device registered\n");
> @@ -291,6 +289,12 @@ int cros_ec_register(struct cros_ec_device *ec_dev)
> cros_ec_irq_thread(0, ec_dev);
>
> return 0;
> +exit:
> + if (!IS_ERR_OR_NULL(ec_dev->pd))
> + platform_device_unregister(ec_dev->pd);
> + if (!IS_ERR_OR_NULL(ec_dev->ec))
> + platform_device_unregister(ec_dev->ec);
> + return err;
You don't need these "if" checks. They are already performed by
platform_device_unregister(), or rather, the functions that it calls [1][2].
[1] https://elixir.bootlin.com/linux/latest/source/drivers/base/platform.c#L756
[2] https://elixir.bootlin.com/linux/latest/source/drivers/base/platform.c#L553
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v2 1/6] platform/chrome: cros_ec: fix error handling in cros_ec_register()
2022-02-09 5:38 ` Prashant Malani
@ 2022-02-09 9:35 ` Tzung-Bi Shih
0 siblings, 0 replies; 19+ messages in thread
From: Tzung-Bi Shih @ 2022-02-09 9:35 UTC (permalink / raw)
To: Prashant Malani; +Cc: bleung, groeck, chrome-platform
On Tue, Feb 08, 2022 at 09:38:55PM -0800, Prashant Malani wrote:
> On Tue, Feb 8, 2022 at 8:50 PM Tzung-Bi Shih <tzungbi@google.com> wrote:
> >
> > Fix cros_ec_register() doesn't unregister platform devices if
> > blocking_notifier_chain_register() fails.
>
> This isn't grammatically correct. Instead:
> Fix cros_ec_register() to unregister platform devices if blocking....
Thanks, will fix in next version.
> > Also use the single exit path to handle the platform device
> > unregistration.
> >
> > This fix depends on the fact that all the callers of cros_ec_register()
> > allocate zeroed memory.
>
> Is that a fair assumption? What happens if a future driver calls
> cros_ec_register()
> without zeroed memory?
Suppose ec_dev->pd is garbage data, platform_device_unregister() will operate
on invalid memory address.
Let's use another guard condition in next version instead of relying on
zeroed memory.
> > ---
> What has changed since v2? Please add a change log of the versions here.
They are in the cover letter actually. Will copy them to each patch.
> > @@ -291,6 +289,12 @@ int cros_ec_register(struct cros_ec_device *ec_dev)
> > cros_ec_irq_thread(0, ec_dev);
> >
> > return 0;
> > +exit:
> > + if (!IS_ERR_OR_NULL(ec_dev->pd))
> > + platform_device_unregister(ec_dev->pd);
> > + if (!IS_ERR_OR_NULL(ec_dev->ec))
> > + platform_device_unregister(ec_dev->ec);
> > + return err;
> You don't need these "if" checks. They are already performed by
> platform_device_unregister(), or rather, the functions that it calls [1][2].
Ack, will remove the check in next version.
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v2 2/6] platform/chrome: cros_ec: remove unused variable `was_wake_device`
2022-02-09 4:50 [PATCH v2 0/6] platform/chrome: cros_ec: miscellaneous cleanups Tzung-Bi Shih
2022-02-09 4:50 ` [PATCH v2 1/6] platform/chrome: cros_ec: fix error handling in cros_ec_register() Tzung-Bi Shih
@ 2022-02-09 4:50 ` Tzung-Bi Shih
2022-02-09 5:58 ` Prashant Malani
2022-02-09 4:50 ` [PATCH v2 3/6] platform/chrome: cros_ec: determine `wake_enabled` in cros_ec_suspend() Tzung-Bi Shih
` (3 subsequent siblings)
5 siblings, 1 reply; 19+ messages in thread
From: Tzung-Bi Shih @ 2022-02-09 4:50 UTC (permalink / raw)
To: bleung, groeck; +Cc: chrome-platform, tzungbi
`was_wake_device` is unused. Remove it.
Signed-off-by: Tzung-Bi Shih <tzungbi@google.com>
---
drivers/platform/chrome/cros_ec.c | 1 -
include/linux/platform_data/cros_ec_proto.h | 3 ---
2 files changed, 4 deletions(-)
diff --git a/drivers/platform/chrome/cros_ec.c b/drivers/platform/chrome/cros_ec.c
index 7ce667ff08e0..303af630c03e 100644
--- a/drivers/platform/chrome/cros_ec.c
+++ b/drivers/platform/chrome/cros_ec.c
@@ -344,7 +344,6 @@ int cros_ec_suspend(struct cros_ec_device *ec_dev)
ec_dev->wake_enabled = !enable_irq_wake(ec_dev->irq);
disable_irq(ec_dev->irq);
- ec_dev->was_wake_device = ec_dev->wake_enabled;
ec_dev->suspended = true;
return 0;
diff --git a/include/linux/platform_data/cros_ec_proto.h b/include/linux/platform_data/cros_ec_proto.h
index df3c78c92ca2..c65971ec90ea 100644
--- a/include/linux/platform_data/cros_ec_proto.h
+++ b/include/linux/platform_data/cros_ec_proto.h
@@ -76,8 +76,6 @@ struct cros_ec_command {
* struct cros_ec_device - Information about a ChromeOS EC device.
* @phys_name: Name of physical comms layer (e.g. 'i2c-4').
* @dev: Device pointer for physical comms device
- * @was_wake_device: True if this device was set to wake the system from
- * sleep at the last suspend.
* @cros_class: The class structure for this device.
* @cmd_readmem: Direct read of the EC memory-mapped region, if supported.
* @offset: Is within EC_LPC_ADDR_MEMMAP region.
@@ -137,7 +135,6 @@ struct cros_ec_device {
/* These are used by other drivers that want to talk to the EC */
const char *phys_name;
struct device *dev;
- bool was_wake_device;
struct class *cros_class;
int (*cmd_readmem)(struct cros_ec_device *ec, unsigned int offset,
unsigned int bytes, void *dest);
--
2.35.0.263.gb82422642f-goog
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v2 2/6] platform/chrome: cros_ec: remove unused variable `was_wake_device`
2022-02-09 4:50 ` [PATCH v2 2/6] platform/chrome: cros_ec: remove unused variable `was_wake_device` Tzung-Bi Shih
@ 2022-02-09 5:58 ` Prashant Malani
2022-02-09 9:36 ` Tzung-Bi Shih
0 siblings, 1 reply; 19+ messages in thread
From: Prashant Malani @ 2022-02-09 5:58 UTC (permalink / raw)
To: Tzung-Bi Shih; +Cc: bleung, groeck, chrome-platform
Hi,
On Tue, Feb 8, 2022 at 8:50 PM Tzung-Bi Shih <tzungbi@google.com> wrote:
>
> `was_wake_device` is unused. Remove it.
nit: This commit message doesn't add anything to the patch (it is
repeating the subject), so it
should be fine to drop it.
>
> Signed-off-by: Tzung-Bi Shih <tzungbi@google.com>
Reviewed-by: Prashant Malani <pmalani@chromium.org>
> ---
In the future, please add a change log for versions here. Quoting the
patch submission guide [1]
"One good use for the additional comments after the --- marker
is...Other comments relevant
only to the moment or the maintainer, not suitable for the permanent
changelog, should also
go here. A good example of such comments might be patch changelogs
which describe what
has changed between the v1 and v2 version of the patch."
[1] https://www.kernel.org/doc/html/latest/process/submitting-patches.html#the-canonical-patch-format
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 2/6] platform/chrome: cros_ec: remove unused variable `was_wake_device`
2022-02-09 5:58 ` Prashant Malani
@ 2022-02-09 9:36 ` Tzung-Bi Shih
0 siblings, 0 replies; 19+ messages in thread
From: Tzung-Bi Shih @ 2022-02-09 9:36 UTC (permalink / raw)
To: Prashant Malani; +Cc: bleung, groeck, chrome-platform
On Tue, Feb 08, 2022 at 09:58:41PM -0800, Prashant Malani wrote:
> On Tue, Feb 8, 2022 at 8:50 PM Tzung-Bi Shih <tzungbi@google.com> wrote:
> >
> > `was_wake_device` is unused. Remove it.
> nit: This commit message doesn't add anything to the patch (it is
> repeating the subject), so it
> should be fine to drop it.
Ack.
> > ---
> In the future, please add a change log for versions here. Quoting the
> patch submission guide [1]
>
> "One good use for the additional comments after the --- marker
> is...Other comments relevant
> only to the moment or the maintainer, not suitable for the permanent
> changelog, should also
> go here. A good example of such comments might be patch changelogs
> which describe what
> has changed between the v1 and v2 version of the patch."
>
> [1] https://www.kernel.org/doc/html/latest/process/submitting-patches.html#the-canonical-patch-format
They are in the cover letter. Will copy them to each patch.
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v2 3/6] platform/chrome: cros_ec: determine `wake_enabled` in cros_ec_suspend()
2022-02-09 4:50 [PATCH v2 0/6] platform/chrome: cros_ec: miscellaneous cleanups Tzung-Bi Shih
2022-02-09 4:50 ` [PATCH v2 1/6] platform/chrome: cros_ec: fix error handling in cros_ec_register() Tzung-Bi Shih
2022-02-09 4:50 ` [PATCH v2 2/6] platform/chrome: cros_ec: remove unused variable `was_wake_device` Tzung-Bi Shih
@ 2022-02-09 4:50 ` Tzung-Bi Shih
2022-02-09 6:05 ` Prashant Malani
2022-02-09 4:50 ` [PATCH v2 4/6] platform/chrome: cros_ec: don't initialize `err` in cros_ec_register() Tzung-Bi Shih
` (2 subsequent siblings)
5 siblings, 1 reply; 19+ messages in thread
From: Tzung-Bi Shih @ 2022-02-09 4:50 UTC (permalink / raw)
To: bleung, groeck; +Cc: chrome-platform, tzungbi
`wake_enabled` indicates cros_ec_resume() needs to call
disable_irq_wake() to undo enable_irq_wake() in cros_ec_suspend().
Determine `wake_enabled` in cros_ec_suspend() instead of
reset-after-used in cros_ec_resume().
Signed-off-by: Tzung-Bi Shih <tzungbi@google.com>
---
drivers/platform/chrome/cros_ec.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/platform/chrome/cros_ec.c b/drivers/platform/chrome/cros_ec.c
index 303af630c03e..a97a47ae0472 100644
--- a/drivers/platform/chrome/cros_ec.c
+++ b/drivers/platform/chrome/cros_ec.c
@@ -342,6 +342,8 @@ int cros_ec_suspend(struct cros_ec_device *ec_dev)
if (device_may_wakeup(dev))
ec_dev->wake_enabled = !enable_irq_wake(ec_dev->irq);
+ else
+ ec_dev->wake_enabled = false;
disable_irq(ec_dev->irq);
ec_dev->suspended = true;
@@ -383,10 +385,9 @@ int cros_ec_resume(struct cros_ec_device *ec_dev)
dev_dbg(ec_dev->dev, "Error %d sending resume event to ec",
ret);
- if (ec_dev->wake_enabled) {
+ if (ec_dev->wake_enabled)
disable_irq_wake(ec_dev->irq);
- ec_dev->wake_enabled = 0;
- }
+
/*
* Let the mfd devices know about events that occur during
* suspend. This way the clients know what to do with them.
--
2.35.0.263.gb82422642f-goog
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v2 3/6] platform/chrome: cros_ec: determine `wake_enabled` in cros_ec_suspend()
2022-02-09 4:50 ` [PATCH v2 3/6] platform/chrome: cros_ec: determine `wake_enabled` in cros_ec_suspend() Tzung-Bi Shih
@ 2022-02-09 6:05 ` Prashant Malani
2022-02-09 9:36 ` Tzung-Bi Shih
0 siblings, 1 reply; 19+ messages in thread
From: Prashant Malani @ 2022-02-09 6:05 UTC (permalink / raw)
To: Tzung-Bi Shih; +Cc: bleung, groeck, chrome-platform
Hi,
On Tue, Feb 8, 2022 at 8:50 PM Tzung-Bi Shih <tzungbi@google.com> wrote:
>
> `wake_enabled` indicates cros_ec_resume() needs to call
> disable_irq_wake() to undo enable_irq_wake() in cros_ec_suspend().
>
> Determine `wake_enabled` in cros_ec_suspend() instead of
> reset-after-used in cros_ec_resume().
It sounds like we can accomplish the same thing as this patch by either:
- Initializing ec_dev->wake_enabled = false during cros_ec_register()
or
- Setting ec_dev->wake_enabled = false just before the
device_may_wakeup(dev) check.
Both of these options accomplish the same thing as this patch, with a
much smaller diff.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 3/6] platform/chrome: cros_ec: determine `wake_enabled` in cros_ec_suspend()
2022-02-09 6:05 ` Prashant Malani
@ 2022-02-09 9:36 ` Tzung-Bi Shih
2022-02-16 1:07 ` Prashant Malani
0 siblings, 1 reply; 19+ messages in thread
From: Tzung-Bi Shih @ 2022-02-09 9:36 UTC (permalink / raw)
To: Prashant Malani; +Cc: bleung, groeck, chrome-platform
On Tue, Feb 08, 2022 at 10:05:58PM -0800, Prashant Malani wrote:
> On Tue, Feb 8, 2022 at 8:50 PM Tzung-Bi Shih <tzungbi@google.com> wrote:
> >
> > `wake_enabled` indicates cros_ec_resume() needs to call
> > disable_irq_wake() to undo enable_irq_wake() in cros_ec_suspend().
> >
> > Determine `wake_enabled` in cros_ec_suspend() instead of
> > reset-after-used in cros_ec_resume().
> It sounds like we can accomplish the same thing as this patch by either:
> - Initializing ec_dev->wake_enabled = false during cros_ec_register()
This doesn't sound like a good idea. Value of device_may_wakeup(dev) changes
during runtime. It should check in cros_ec_suspend() instead of in
cros_ec_register().
> or
> - Setting ec_dev->wake_enabled = false just before the
> device_may_wakeup(dev) check.
Did you mean:
ec_dev->wake_enabled = false;
if (device_may_wakeup(dev))
ec_dev->wake_enabled = !enable_irq_wake(ec_dev->irq);
If so, this way is suboptimal to me. ec_dev->wake_enabled can be written
once; however, it is written twice if device_may_wakeup(dev) is true.
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v2 3/6] platform/chrome: cros_ec: determine `wake_enabled` in cros_ec_suspend()
2022-02-09 9:36 ` Tzung-Bi Shih
@ 2022-02-16 1:07 ` Prashant Malani
2022-02-16 4:13 ` Tzung-Bi Shih
0 siblings, 1 reply; 19+ messages in thread
From: Prashant Malani @ 2022-02-16 1:07 UTC (permalink / raw)
To: Tzung-Bi Shih; +Cc: bleung, groeck, chrome-platform
On Wed, Feb 9, 2022 at 1:36 AM Tzung-Bi Shih <tzungbi@google.com> wrote:
>
> On Tue, Feb 08, 2022 at 10:05:58PM -0800, Prashant Malani wrote:
> > On Tue, Feb 8, 2022 at 8:50 PM Tzung-Bi Shih <tzungbi@google.com> wrote:
> > >
> > > `wake_enabled` indicates cros_ec_resume() needs to call
> > > disable_irq_wake() to undo enable_irq_wake() in cros_ec_suspend().
> > >
> > > Determine `wake_enabled` in cros_ec_suspend() instead of
> > > reset-after-used in cros_ec_resume().
> > It sounds like we can accomplish the same thing as this patch by either:
> > - Initializing ec_dev->wake_enabled = false during cros_ec_register()
>
> This doesn't sound like a good idea. Value of device_may_wakeup(dev) changes
> during runtime. It should check in cros_ec_suspend() instead of in
> cros_ec_register().
Hmm, I'm pretty sure it shouldn't change for Cros EC. In any case, I'm
not suggesting
moving the check away from cros_ec_suspend(), just the initialization
of the flag.
>
> > or
> > - Setting ec_dev->wake_enabled = false just before the
> > device_may_wakeup(dev) check.
>
> Did you mean:
>
> ec_dev->wake_enabled = false;
> if (device_may_wakeup(dev))
> ec_dev->wake_enabled = !enable_irq_wake(ec_dev->irq);
>
> If so, this way is suboptimal to me. ec_dev->wake_enabled can be written
> once; however, it is written twice if device_may_wakeup(dev) is true.
I don't think it is a concern on any modern computer hardware, and
seems more readable to me, but
it's up to you.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 3/6] platform/chrome: cros_ec: determine `wake_enabled` in cros_ec_suspend()
2022-02-16 1:07 ` Prashant Malani
@ 2022-02-16 4:13 ` Tzung-Bi Shih
2022-02-16 5:55 ` Prashant Malani
0 siblings, 1 reply; 19+ messages in thread
From: Tzung-Bi Shih @ 2022-02-16 4:13 UTC (permalink / raw)
To: Prashant Malani; +Cc: bleung, groeck, chrome-platform
On Tue, Feb 15, 2022 at 05:07:43PM -0800, Prashant Malani wrote:
> On Wed, Feb 9, 2022 at 1:36 AM Tzung-Bi Shih <tzungbi@google.com> wrote:
> >
> > On Tue, Feb 08, 2022 at 10:05:58PM -0800, Prashant Malani wrote:
> > > On Tue, Feb 8, 2022 at 8:50 PM Tzung-Bi Shih <tzungbi@google.com> wrote:
> > > >
> > > > `wake_enabled` indicates cros_ec_resume() needs to call
> > > > disable_irq_wake() to undo enable_irq_wake() in cros_ec_suspend().
> > > >
> > > > Determine `wake_enabled` in cros_ec_suspend() instead of
> > > > reset-after-used in cros_ec_resume().
> > > It sounds like we can accomplish the same thing as this patch by either:
> > > - Initializing ec_dev->wake_enabled = false during cros_ec_register()
> >
> > This doesn't sound like a good idea. Value of device_may_wakeup(dev) changes
> > during runtime. It should check in cros_ec_suspend() instead of in
> > cros_ec_register().
>
> Hmm, I'm pretty sure it shouldn't change for Cros EC. In any case, I'm
> not suggesting
> moving the check away from cros_ec_suspend(), just the initialization
> of the flag.
Got it. I misunderstood. Let's go this direction. Will fix in the next
version.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 3/6] platform/chrome: cros_ec: determine `wake_enabled` in cros_ec_suspend()
2022-02-16 4:13 ` Tzung-Bi Shih
@ 2022-02-16 5:55 ` Prashant Malani
2022-02-16 7:33 ` Tzung-Bi Shih
0 siblings, 1 reply; 19+ messages in thread
From: Prashant Malani @ 2022-02-16 5:55 UTC (permalink / raw)
To: Tzung-Bi Shih; +Cc: bleung, groeck, chrome-platform
On Tue, Feb 15, 2022 at 8:13 PM Tzung-Bi Shih <tzungbi@google.com> wrote:
>
> On Tue, Feb 15, 2022 at 05:07:43PM -0800, Prashant Malani wrote:
> > On Wed, Feb 9, 2022 at 1:36 AM Tzung-Bi Shih <tzungbi@google.com> wrote:
> > >
> > > On Tue, Feb 08, 2022 at 10:05:58PM -0800, Prashant Malani wrote:
> > > > On Tue, Feb 8, 2022 at 8:50 PM Tzung-Bi Shih <tzungbi@google.com> wrote:
> > > > >
> > > > > `wake_enabled` indicates cros_ec_resume() needs to call
> > > > > disable_irq_wake() to undo enable_irq_wake() in cros_ec_suspend().
> > > > >
> > > > > Determine `wake_enabled` in cros_ec_suspend() instead of
> > > > > reset-after-used in cros_ec_resume().
> > > > It sounds like we can accomplish the same thing as this patch by either:
> > > > - Initializing ec_dev->wake_enabled = false during cros_ec_register()
> > >
> > > This doesn't sound like a good idea. Value of device_may_wakeup(dev) changes
> > > during runtime. It should check in cros_ec_suspend() instead of in
> > > cros_ec_register().
Actually, circling back to this (apologies), can you provide an
example of this changing during runtime?
I'm not aware of one, but I could be omitting some use cases that you
might be familiar with.
> >
> > Hmm, I'm pretty sure it shouldn't change for Cros EC. In any case, I'm
> > not suggesting
> > moving the check away from cros_ec_suspend(), just the initialization
> > of the flag.
>
> Got it. I misunderstood. Let's go this direction. Will fix in the next
> version.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 3/6] platform/chrome: cros_ec: determine `wake_enabled` in cros_ec_suspend()
2022-02-16 5:55 ` Prashant Malani
@ 2022-02-16 7:33 ` Tzung-Bi Shih
0 siblings, 0 replies; 19+ messages in thread
From: Tzung-Bi Shih @ 2022-02-16 7:33 UTC (permalink / raw)
To: Prashant Malani; +Cc: bleung, groeck, chrome-platform
On Tue, Feb 15, 2022 at 09:55:13PM -0800, Prashant Malani wrote:
> On Tue, Feb 15, 2022 at 8:13 PM Tzung-Bi Shih <tzungbi@google.com> wrote:
> >
> > On Tue, Feb 15, 2022 at 05:07:43PM -0800, Prashant Malani wrote:
> > > On Wed, Feb 9, 2022 at 1:36 AM Tzung-Bi Shih <tzungbi@google.com> wrote:
> > > >
> > > > On Tue, Feb 08, 2022 at 10:05:58PM -0800, Prashant Malani wrote:
> > > > > On Tue, Feb 8, 2022 at 8:50 PM Tzung-Bi Shih <tzungbi@google.com> wrote:
> > > > > >
> > > > > > `wake_enabled` indicates cros_ec_resume() needs to call
> > > > > > disable_irq_wake() to undo enable_irq_wake() in cros_ec_suspend().
> > > > > >
> > > > > > Determine `wake_enabled` in cros_ec_suspend() instead of
> > > > > > reset-after-used in cros_ec_resume().
> > > > > It sounds like we can accomplish the same thing as this patch by either:
> > > > > - Initializing ec_dev->wake_enabled = false during cros_ec_register()
> > > >
> > > > This doesn't sound like a good idea. Value of device_may_wakeup(dev) changes
> > > > during runtime. It should check in cros_ec_suspend() instead of in
> > > > cros_ec_register().
> Actually, circling back to this (apologies), can you provide an
> example of this changing during runtime?
> I'm not aware of one, but I could be omitting some use cases that you
> might be familiar with.
If someone `echo disabled > /sys/devices/.../spi0/spi0.0/power/wakeup`, the
value of device_may_wakeup(dev) would change.
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v2 4/6] platform/chrome: cros_ec: don't initialize `err` in cros_ec_register()
2022-02-09 4:50 [PATCH v2 0/6] platform/chrome: cros_ec: miscellaneous cleanups Tzung-Bi Shih
` (2 preceding siblings ...)
2022-02-09 4:50 ` [PATCH v2 3/6] platform/chrome: cros_ec: determine `wake_enabled` in cros_ec_suspend() Tzung-Bi Shih
@ 2022-02-09 4:50 ` Tzung-Bi Shih
2022-02-09 6:08 ` Prashant Malani
2022-02-09 4:50 ` [PATCH v2 5/6] platform/chrome: cros_ec: sort header inclusion alphabetically Tzung-Bi Shih
2022-02-09 4:50 ` [PATCH v2 6/6] platform/chrome: cros_ec: append newline to all logs Tzung-Bi Shih
5 siblings, 1 reply; 19+ messages in thread
From: Tzung-Bi Shih @ 2022-02-09 4:50 UTC (permalink / raw)
To: bleung, groeck; +Cc: chrome-platform, tzungbi
`err` in cros_ec_register() doesn't need to be initialized.
Signed-off-by: Tzung-Bi Shih <tzungbi@google.com>
---
drivers/platform/chrome/cros_ec.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/platform/chrome/cros_ec.c b/drivers/platform/chrome/cros_ec.c
index a97a47ae0472..c2105621b368 100644
--- a/drivers/platform/chrome/cros_ec.c
+++ b/drivers/platform/chrome/cros_ec.c
@@ -182,7 +182,7 @@ static int cros_ec_ready_event(struct notifier_block *nb,
int cros_ec_register(struct cros_ec_device *ec_dev)
{
struct device *dev = ec_dev->dev;
- int err = 0;
+ int err;
BLOCKING_INIT_NOTIFIER_HEAD(&ec_dev->event_notifier);
--
2.35.0.263.gb82422642f-goog
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH v2 5/6] platform/chrome: cros_ec: sort header inclusion alphabetically
2022-02-09 4:50 [PATCH v2 0/6] platform/chrome: cros_ec: miscellaneous cleanups Tzung-Bi Shih
` (3 preceding siblings ...)
2022-02-09 4:50 ` [PATCH v2 4/6] platform/chrome: cros_ec: don't initialize `err` in cros_ec_register() Tzung-Bi Shih
@ 2022-02-09 4:50 ` Tzung-Bi Shih
2022-02-09 4:50 ` [PATCH v2 6/6] platform/chrome: cros_ec: append newline to all logs Tzung-Bi Shih
5 siblings, 0 replies; 19+ messages in thread
From: Tzung-Bi Shih @ 2022-02-09 4:50 UTC (permalink / raw)
To: bleung, groeck; +Cc: chrome-platform, tzungbi
Sort header inclusion alphabetically.
Signed-off-by: Tzung-Bi Shih <tzungbi@google.com>
---
drivers/platform/chrome/cros_ec.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/platform/chrome/cros_ec.c b/drivers/platform/chrome/cros_ec.c
index c2105621b368..008dc2fc5066 100644
--- a/drivers/platform/chrome/cros_ec.c
+++ b/drivers/platform/chrome/cros_ec.c
@@ -9,12 +9,12 @@
* battery charging and regulator control, firmware update.
*/
-#include <linux/of_platform.h>
#include <linux/interrupt.h>
-#include <linux/slab.h>
#include <linux/module.h>
+#include <linux/of_platform.h>
#include <linux/platform_data/cros_ec_commands.h>
#include <linux/platform_data/cros_ec_proto.h>
+#include <linux/slab.h>
#include <linux/suspend.h>
#include "cros_ec.h"
--
2.35.0.263.gb82422642f-goog
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH v2 6/6] platform/chrome: cros_ec: append newline to all logs
2022-02-09 4:50 [PATCH v2 0/6] platform/chrome: cros_ec: miscellaneous cleanups Tzung-Bi Shih
` (4 preceding siblings ...)
2022-02-09 4:50 ` [PATCH v2 5/6] platform/chrome: cros_ec: sort header inclusion alphabetically Tzung-Bi Shih
@ 2022-02-09 4:50 ` Tzung-Bi Shih
5 siblings, 0 replies; 19+ messages in thread
From: Tzung-Bi Shih @ 2022-02-09 4:50 UTC (permalink / raw)
To: bleung, groeck; +Cc: chrome-platform, tzungbi
To be consistent, append newline ("\n") to all logs.
Signed-off-by: Tzung-Bi Shih <tzungbi@google.com>
---
drivers/platform/chrome/cros_ec.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/platform/chrome/cros_ec.c b/drivers/platform/chrome/cros_ec.c
index 008dc2fc5066..426314e4b7f6 100644
--- a/drivers/platform/chrome/cros_ec.c
+++ b/drivers/platform/chrome/cros_ec.c
@@ -213,7 +213,7 @@ int cros_ec_register(struct cros_ec_device *ec_dev)
IRQF_TRIGGER_LOW | IRQF_ONESHOT,
"chromeos-ec", ec_dev);
if (err) {
- dev_err(dev, "Failed to request IRQ %d: %d",
+ dev_err(dev, "Failed to request IRQ %d: %d\n",
ec_dev->irq, err);
return err;
}
@@ -264,7 +264,7 @@ int cros_ec_register(struct cros_ec_device *ec_dev)
*/
err = cros_ec_sleep_event(ec_dev, 0);
if (err < 0)
- dev_dbg(ec_dev->dev, "Error %d clearing sleep event to ec",
+ dev_dbg(ec_dev->dev, "Error %d clearing sleep event to ec\n",
err);
if (ec_dev->mkbp_event_supported) {
@@ -337,7 +337,7 @@ int cros_ec_suspend(struct cros_ec_device *ec_dev)
ret = cros_ec_sleep_event(ec_dev, sleep_event);
if (ret < 0)
- dev_dbg(ec_dev->dev, "Error %d sending suspend event to ec",
+ dev_dbg(ec_dev->dev, "Error %d sending suspend event to ec\n",
ret);
if (device_may_wakeup(dev))
@@ -382,7 +382,7 @@ int cros_ec_resume(struct cros_ec_device *ec_dev)
ret = cros_ec_sleep_event(ec_dev, sleep_event);
if (ret < 0)
- dev_dbg(ec_dev->dev, "Error %d sending resume event to ec",
+ dev_dbg(ec_dev->dev, "Error %d sending resume event to ec\n",
ret);
if (ec_dev->wake_enabled)
--
2.35.0.263.gb82422642f-goog
^ permalink raw reply related [flat|nested] 19+ messages in thread