On Fri, Jan 22, 2021 at 4:48 PM Naresh Kamboju
[off-list ref] wrote:

On Fri, 22 Jan 2021 at 20:39, Rafael J. Wysocki [off-list ref] wrote:

quoted

On Thu, Jan 21, 2021 at 8:01 PM Rafael J. Wysocki [off-list ref] wrote:

quoted

On Thu, Jan 21, 2021 at 6:23 PM Nicolas Pitre [off-list ref] wrote:

quoted

The clock API splits its interface into sleepable ant atomic contexts:

- clk_prepare/clk_unprepare for stuff that might sleep

- clk_enable_clk_disable for anything that may be done in atomic context

The code handling runtime PM for clocks only calls clk_disable() on
suspend requests, and clk_enable on resume requests. This means that
runtime PM with clock providers that only have the prepare/unprepare
methods implemented is basically useless.

Many clock implementations can't accommodate atomic contexts. This is
often the case when communication with the clock happens through another
subsystem like I2C or SCMI.

Let's make the clock PM code useful with such clocks by safely invoking
clk_prepare/clk_unprepare upon resume/suspend requests. Of course, when
such clocks are registered with the PM layer then pm_runtime_irq_safe()
can't be used, and neither pm_runtime_suspend() nor pm_runtime_resume()
may be invoked in atomic context.

For clocks that do implement the enable and disable methods then
everything just works as before.

Signed-off-by: Nicolas Pitre <redacted>

---

On Thu, 21 Jan 2021, Rafael J. Wysocki wrote:

quoted

So I'm going to drop this patch from linux-next until the issue is
resolved, thanks!

Here's the fixed version.

Applied instead of the v1, thanks!

quoted

Changes from v1:

- Moved clk_is_enabled_when_prepared() declaration under
  CONFIG_HAVE_CLK_PREPARE and provided a dummy definition when that
  config option is unset.

diff --git a/drivers/base/power/clock_ops.c b/drivers/base/power/clock_ops.c
index ced6863a16..a62fb0f9b1 100644
--- a/drivers/base/power/clock_ops.c
+++ b/drivers/base/power/clock_ops.c

@@ -23,6 +23,7 @@
 enum pce_status {
        PCE_STATUS_NONE = 0,
        PCE_STATUS_ACQUIRED,
+       PCE_STATUS_PREPARED,
        PCE_STATUS_ENABLED,
        PCE_STATUS_ERROR,
 };

@@ -32,8 +33,102 @@ struct pm_clock_entry {
        char *con_id;
        struct clk *clk;
        enum pce_status status;
+       bool enabled_when_prepared;
 };

+/**
+ * pm_clk_list_lock - ensure exclusive access for modifying the PM clock
+ *                   entry list.
+ * @psd: pm_subsys_data instance corresponding to the PM clock entry list
+ *      and clk_op_might_sleep count to be modified.
+ *
+ * Get exclusive access before modifying the PM clock entry list and the
+ * clock_op_might_sleep count to guard against concurrent modifications.
+ * This also protects against a concurrent clock_op_might_sleep and PM clock
+ * entry list usage in pm_clk_suspend()/pm_clk_resume() that may or may not
+ * happen in atomic context, hence both the mutex and the spinlock must be
+ * taken here.
+ */
+static void pm_clk_list_lock(struct pm_subsys_data *psd)
+{
+       mutex_lock(&psd->clock_mutex);
+       spin_lock_irq(&psd->lock);
+}
+
+/**
+ * pm_clk_list_unlock - counterpart to pm_clk_list_lock().
+ * @psd: the same pm_subsys_data instance previously passed to
+ *      pm_clk_list_lock().
+ */
+static void pm_clk_list_unlock(struct pm_subsys_data *psd)

Locking annotations for sparse were missing here and above, so I've
added them by hand.

Please double check the result in my linux-next branch (just pushed).

May i request to add Reported-by: Naresh Kamboju [off-list ref]

If this had been a patch fixing a problem reported by you, there would
have been a reason to add a Reported-by,

In this case, it is just a new version of a patch taking your testing
feedback into account.

I can add a Tested-by for you to it if desired, though.