[Question] about patch: don't use [delayed_]work_pending()

[Question] about patch: don't use [delayed_]work_pending()

[qiaozhou]

Hi Tejun,

I have a question related with below patch, and need your suggestion.

In our system, we do cpu clock init in of_clk_init path, and use pm qos 
to maintain cpu/cci clock. Firstly we init a CCI_CLK_QOS and set a 
default value, then update CCI_CLK_QOS to limit CCI min frequency 
according to current cpu frequency. Before calling 
pm_qos_update_request, irq is disabled, but after the calling, irq is 
enabled in cancel_delayed_work_sync, which causes some inconvenience 
before Before this patch is applied, it checks pending work and won't do 
cancel_delayed_work_sync in this boot up phase.

The simple calling sequence is like this:

start_kernel -> of_clk_init -> cpu_clk_init -> pm_qos_add_request(xx, 
default_value),

then pm_qos_update_request.

I don't know whether it's meaningful to still check pending work here, 
or it's not suggested to use pm_qos_update_request in this early boot up 
phase. Could you help to share some opinions? (I can fix this issue by 
adding the current qos value directly instead of default value, though.)

Thanks a lot.

commit ed1ac6e91a3ff7c561008ba57747cd6cbc49385e
Author: Tejun Heo <tj@kernel.org>
Date:   Fri Jan 11 13:37:33 2013 +0100

     PM: don't use [delayed_]work_pending()

     There's no need to test whether a (delayed) work item is pending
     before queueing, flushing or cancelling it, so remove work_pending()
     tests used in those cases.

     Signed-off-by: Tejun Heo <tj@kernel.org>
     Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

...

@@ -359,8 +359,7 @@ void pm_qos_update_request(struct pm_qos_request *req,
                 return;
         }

-       if (delayed_work_pending(&req->work))
-               cancel_delayed_work_sync(&req->work);
+       cancel_delayed_work_sync(&req->work);
...

Best Regards

Qiao

Re: [Question] about patch: don't use [delayed_]work_pending()

[Tejun Heo]

Hello,

On Thu, Sep 01, 2016 at 05:09:36PM +0800, qiaozhou wrote:
> In our system, we do cpu clock init in of_clk_init path, and use pm qos to
> maintain cpu/cci clock. Firstly we init a CCI_CLK_QOS and set a default
> value, then update CCI_CLK_QOS to limit CCI min frequency according to
> current cpu frequency. Before calling pm_qos_update_request, irq is
> disabled, but after the calling, irq is enabled in cancel_delayed_work_sync,
> which causes some inconvenience before Before this patch is applied, it
> checks pending work and won't do cancel_delayed_work_sync in this boot up
> phase.

So, cancel_delayed_work_sync() usually shouldn't be called with irq
disabled as it's a possibly blocking call.

> The simple calling sequence is like this:
> 
> start_kernel -> of_clk_init -> cpu_clk_init -> pm_qos_add_request(xx,
> default_value),
> 
> then pm_qos_update_request.
> 
> I don't know whether it's meaningful to still check pending work here, or
> it's not suggested to use pm_qos_update_request in this early boot up phase.
> Could you help to share some opinions? (I can fix this issue by adding the
> current qos value directly instead of default value, though.)

Hmmm... but I suppose this is super-early in the boot.  Would it make
sense to have a static variable (e.g. bool clk_fully_initailized) to
gate the cancel_delayed_sync() call?

Thanks.

-- 
tejun

Re: [Question] about patch: don't use [delayed_]work_pending()

[qiaozhou]

On 2016年09月02日 02:45, Tejun Heo wrote:
> Hello,
>
> On Thu, Sep 01, 2016 at 05:09:36PM +0800, qiaozhou wrote:
>> In our system, we do cpu clock init in of_clk_init path, and use pm qos to
>> maintain cpu/cci clock. Firstly we init a CCI_CLK_QOS and set a default
>> value, then update CCI_CLK_QOS to limit CCI min frequency according to
>> current cpu frequency. Before calling pm_qos_update_request, irq is
>> disabled, but after the calling, irq is enabled in cancel_delayed_work_sync,
>> which causes some inconvenience before Before this patch is applied, it
>> checks pending work and won't do cancel_delayed_work_sync in this boot up
>> phase.
> So, cancel_delayed_work_sync() usually shouldn't be called with irq
> disabled as it's a possibly blocking call.
Agree.
>
>> The simple calling sequence is like this:
>>
>> start_kernel -> of_clk_init -> cpu_clk_init -> pm_qos_add_request(xx,
>> default_value),
>>
>> then pm_qos_update_request.
>>
>> I don't know whether it's meaningful to still check pending work here, or
>> it's not suggested to use pm_qos_update_request in this early boot up phase.
>> Could you help to share some opinions? (I can fix this issue by adding the
>> current qos value directly instead of default value, though.)
> Hmmm... but I suppose this is super-early in the boot.  Would it make
> sense to have a static variable (e.g. bool clk_fully_initailized) to
> gate the cancel_delayed_sync() call?
You're right that it's indeed super-early stage. But currently we can't 
control the gate of can_delayed_work_sync, since it's inside 
pm_qos_update_request. Out of our control. We can choose to not call 
pm_qos_update_request to avoid this issue, and use pm_qos_add_request 
alternatively. Good to have it.
Thanks a lot.

Re: [Question] about patch: don't use [delayed_]work_pending()

[Tejun Heo]

Hello,

On Fri, Sep 02, 2016 at 09:17:04AM +0800, qiaozhou wrote:
> > > I don't know whether it's meaningful to still check pending work here, or
> > > it's not suggested to use pm_qos_update_request in this early boot up phase.
> > > Could you help to share some opinions? (I can fix this issue by adding the
> > > current qos value directly instead of default value, though.)
> > Hmmm... but I suppose this is super-early in the boot.  Would it make
> > sense to have a static variable (e.g. bool clk_fully_initailized) to
> > gate the cancel_delayed_sync() call?
>
> You're right that it's indeed super-early stage. But currently we can't
> control the gate of can_delayed_work_sync, since it's inside
> pm_qos_update_request. Out of our control. We can choose to not call
> pm_qos_update_request to avoid this issue, and use pm_qos_add_request
> alternatively. Good to have it.

Ah sorry, didn't understand that the offending cancel_sync call is in
the generic part.  Hmm... but yeah, we should still be able to take
the same approach.  I'll see what's the right thing to gate the
operation there.

Thanks.

-- 
tejun

Re: [Question] about patch: don't use [delayed_]work_pending()

[Tejun Heo]

On Fri, Sep 02, 2016 at 09:50:07AM -0400, Tejun Heo wrote:
> Hello,
> 
> On Fri, Sep 02, 2016 at 09:17:04AM +0800, qiaozhou wrote:
> > > > I don't know whether it's meaningful to still check pending work here, or
> > > > it's not suggested to use pm_qos_update_request in this early boot up phase.
> > > > Could you help to share some opinions? (I can fix this issue by adding the
> > > > current qos value directly instead of default value, though.)
> > > Hmmm... but I suppose this is super-early in the boot.  Would it make
> > > sense to have a static variable (e.g. bool clk_fully_initailized) to
> > > gate the cancel_delayed_sync() call?
> >
> > You're right that it's indeed super-early stage. But currently we can't
> > control the gate of can_delayed_work_sync, since it's inside
> > pm_qos_update_request. Out of our control. We can choose to not call
> > pm_qos_update_request to avoid this issue, and use pm_qos_add_request
> > alternatively. Good to have it.
> 
> Ah sorry, didn't understand that the offending cancel_sync call is in
> the generic part.  Hmm... but yeah, we should still be able to take
> the same approach.  I'll see what's the right thing to gate the
> operation there.

Does the following patch work?

Subject: power: avoid calling cancel_delayed_work_sync() during early boot

of_clk_init() ends up calling into pm_qos_update_request() very early
during boot where irq is expected to stay disabled.
pm_qos_update_request() uses cancel_delayed_work_sync() which
correctly assumes that irq is enabled on invocation and
unconditionally disables and re-enables it.

Gate cancel_delayed_work_sync() invocation with kevented_up() to avoid
enabling irq unexpectedly during early boot.

Signed-off-by: Tejun Heo <tj@kernel.org>
Reported-by: Qiao Zhou <qiaozhou@asrmicro.com>
Link: http://lkml.kernel.org/r/d2501c4c-8e7b-bea3-1b01-000b36b5dfe9@asrmicro.com
---
 kernel/power/qos.c |   11 ++++++++++-
  1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/kernel/power/qos.c b/kernel/power/qos.c
index 97b0df7..168ff44 100644
--- a/kernel/power/qos.c
+++ b/kernel/power/qos.c
@@ -482,7 +482,16 @@ void pm_qos_update_request(struct pm_qos_request *req,
 		return;
 	}
 
-	cancel_delayed_work_sync(&req->work);
+	/*
+	 * This function may be called very early during boot, for example,
+	 * from of_clk_init(), where irq needs to stay disabled.
+	 * cancel_delayed_work_sync() assumes that irq is enabled on
+	 * invocation and re-enables it on return.  Avoid calling it until
+	 * workqueue is initialized.
+	 */
+	if (keventd_up())
+		cancel_delayed_work_sync(&req->work);
+
 	__pm_qos_update_request(req, new_value);
 }
 EXPORT_SYMBOL_GPL(pm_qos_update_request);

Re: [Question] about patch: don't use [delayed_]work_pending()

[qiaozhou]

On 2016年09月02日 22:21, Tejun Heo wrote:
> On Fri, Sep 02, 2016 at 09:50:07AM -0400, Tejun Heo wrote:
>> Hello,
>>
>> On Fri, Sep 02, 2016 at 09:17:04AM +0800, qiaozhou wrote:
>>>>> I don't know whether it's meaningful to still check pending work here, or
>>>>> it's not suggested to use pm_qos_update_request in this early boot up phase.
>>>>> Could you help to share some opinions? (I can fix this issue by adding the
>>>>> current qos value directly instead of default value, though.)
>>>> Hmmm... but I suppose this is super-early in the boot.  Would it make
>>>> sense to have a static variable (e.g. bool clk_fully_initailized) to
>>>> gate the cancel_delayed_sync() call?
>>> You're right that it's indeed super-early stage. But currently we can't
>>> control the gate of can_delayed_work_sync, since it's inside
>>> pm_qos_update_request. Out of our control. We can choose to not call
>>> pm_qos_update_request to avoid this issue, and use pm_qos_add_request
>>> alternatively. Good to have it.
>> Ah sorry, didn't understand that the offending cancel_sync call is in
>> the generic part.  Hmm... but yeah, we should still be able to take
>> the same approach.  I'll see what's the right thing to gate the
>> operation there.
> Does the following patch work?
I'll have a try next Monday and let you know ASAP. Thanks for the patch.
>
> Subject: power: avoid calling cancel_delayed_work_sync() during early boot
>
> of_clk_init() ends up calling into pm_qos_update_request() very early
> during boot where irq is expected to stay disabled.
> pm_qos_update_request() uses cancel_delayed_work_sync() which
> correctly assumes that irq is enabled on invocation and
> unconditionally disables and re-enables it.
>
> Gate cancel_delayed_work_sync() invocation with kevented_up() to avoid
> enabling irq unexpectedly during early boot.
>
> Signed-off-by: Tejun Heo <tj@kernel.org>
> Reported-by: Qiao Zhou <qiaozhou@asrmicro.com>
> Link: http://lkml.kernel.org/r/d2501c4c-8e7b-bea3-1b01-000b36b5dfe9@asrmicro.com
> ---
>   kernel/power/qos.c |   11 ++++++++++-
>    1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/power/qos.c b/kernel/power/qos.c
> index 97b0df7..168ff44 100644
> --- a/kernel/power/qos.c
> +++ b/kernel/power/qos.c
> @@ -482,7 +482,16 @@ void pm_qos_update_request(struct pm_qos_request *req,
>   		return;
>   	}
>   
> -	cancel_delayed_work_sync(&req->work);
> +	/*
> +	 * This function may be called very early during boot, for example,
> +	 * from of_clk_init(), where irq needs to stay disabled.
> +	 * cancel_delayed_work_sync() assumes that irq is enabled on
> +	 * invocation and re-enables it on return.  Avoid calling it until
> +	 * workqueue is initialized.
> +	 */
> +	if (keventd_up())
> +		cancel_delayed_work_sync(&req->work);
> +
>   	__pm_qos_update_request(req, new_value);
>   }
>   EXPORT_SYMBOL_GPL(pm_qos_update_request);

Re: [Question] about patch: don't use [delayed_]work_pending()

[qiaozhou]

On 2016年09月02日 22:21, Tejun Heo wrote:
> On Fri, Sep 02, 2016 at 09:50:07AM -0400, Tejun Heo wrote:
>> Hello,
>>
>> On Fri, Sep 02, 2016 at 09:17:04AM +0800, qiaozhou wrote:
>>>>> I don't know whether it's meaningful to still check pending work here, or
>>>>> it's not suggested to use pm_qos_update_request in this early boot up phase.
>>>>> Could you help to share some opinions? (I can fix this issue by adding the
>>>>> current qos value directly instead of default value, though.)
>>>> Hmmm... but I suppose this is super-early in the boot.  Would it make
>>>> sense to have a static variable (e.g. bool clk_fully_initailized) to
>>>> gate the cancel_delayed_sync() call?
>>> You're right that it's indeed super-early stage. But currently we can't
>>> control the gate of can_delayed_work_sync, since it's inside
>>> pm_qos_update_request. Out of our control. We can choose to not call
>>> pm_qos_update_request to avoid this issue, and use pm_qos_add_request
>>> alternatively. Good to have it.
>> Ah sorry, didn't understand that the offending cancel_sync call is in
>> the generic part.  Hmm... but yeah, we should still be able to take
>> the same approach.  I'll see what's the right thing to gate the
>> operation there.
> Does the following patch work?
The patch can fix this issue. Thanks a lot.
>
> Subject: power: avoid calling cancel_delayed_work_sync() during early boot
>
> of_clk_init() ends up calling into pm_qos_update_request() very early
> during boot where irq is expected to stay disabled.
> pm_qos_update_request() uses cancel_delayed_work_sync() which
> correctly assumes that irq is enabled on invocation and
> unconditionally disables and re-enables it.
>
> Gate cancel_delayed_work_sync() invocation with kevented_up() to avoid
> enabling irq unexpectedly during early boot.
>
> Signed-off-by: Tejun Heo <tj@kernel.org>
> Reported-by: Qiao Zhou <qiaozhou@asrmicro.com>
> Link: http://lkml.kernel.org/r/d2501c4c-8e7b-bea3-1b01-000b36b5dfe9@asrmicro.com
> ---
>   kernel/power/qos.c |   11 ++++++++++-
>    1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/power/qos.c b/kernel/power/qos.c
> index 97b0df7..168ff44 100644
> --- a/kernel/power/qos.c
> +++ b/kernel/power/qos.c
> @@ -482,7 +482,16 @@ void pm_qos_update_request(struct pm_qos_request *req,
>   		return;
>   	}
>   
> -	cancel_delayed_work_sync(&req->work);
> +	/*
> +	 * This function may be called very early during boot, for example,
> +	 * from of_clk_init(), where irq needs to stay disabled.
> +	 * cancel_delayed_work_sync() assumes that irq is enabled on
> +	 * invocation and re-enables it on return.  Avoid calling it until
> +	 * workqueue is initialized.
> +	 */
> +	if (keventd_up())
> +		cancel_delayed_work_sync(&req->work);
> +
>   	__pm_qos_update_request(req, new_value);
>   }
>   EXPORT_SYMBOL_GPL(pm_qos_update_request);

[PATCH] power: avoid calling cancel_delayed_work_sync() during early boot

[Tejun Heo]

of_clk_init() ends up calling into pm_qos_update_request() very early
during boot where irq is expected to stay disabled.
pm_qos_update_request() uses cancel_delayed_work_sync() which
correctly assumes that irq is enabled on invocation and
unconditionally disables and re-enables it.

Gate cancel_delayed_work_sync() invocation with kevented_up() to avoid
enabling irq unexpectedly during early boot.

Signed-off-by: Tejun Heo <tj@kernel.org>
Reported-and-tested-by: Qiao Zhou <qiaozhou@asrmicro.com>
Link: http://lkml.kernel.org/r/d2501c4c-8e7b-bea3-1b01-000b36b5dfe9@asrmicro.com
---

Rafael, can you please route this patch?

Thanks.

 kernel/power/qos.c |   11 ++++++++++-
  1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/kernel/power/qos.c b/kernel/power/qos.c
index 97b0df7..168ff44 100644
--- a/kernel/power/qos.c
+++ b/kernel/power/qos.c
@@ -482,7 +482,16 @@ void pm_qos_update_request(struct pm_qos_request *req,
 		return;
 	}
 
-	cancel_delayed_work_sync(&req->work);
+	/*
+	 * This function may be called very early during boot, for example,
+	 * from of_clk_init(), where irq needs to stay disabled.
+	 * cancel_delayed_work_sync() assumes that irq is enabled on
+	 * invocation and re-enables it on return.  Avoid calling it until
+	 * workqueue is initialized.
+	 */
+	if (keventd_up())
+		cancel_delayed_work_sync(&req->work);
+
 	__pm_qos_update_request(req, new_value);
 }
 EXPORT_SYMBOL_GPL(pm_qos_update_request);

Re: [PATCH] power: avoid calling cancel_delayed_work_sync() during early boot

[Rafael J. Wysocki]

On Mon, Sep 5, 2016 at 2:38 PM, Tejun Heo <tj@kernel.org> wrote:
> of_clk_init() ends up calling into pm_qos_update_request() very early
> during boot where irq is expected to stay disabled.
> pm_qos_update_request() uses cancel_delayed_work_sync() which
> correctly assumes that irq is enabled on invocation and
> unconditionally disables and re-enables it.
>
> Gate cancel_delayed_work_sync() invocation with kevented_up() to avoid
> enabling irq unexpectedly during early boot.
>
> Signed-off-by: Tejun Heo <tj@kernel.org>
> Reported-and-tested-by: Qiao Zhou <qiaozhou@asrmicro.com>
> Link: http://lkml.kernel.org/r/d2501c4c-8e7b-bea3-1b01-000b36b5dfe9@asrmicro.com
> ---
>
> Rafael, can you please route this patch?

I will.

Thanks,
Rafael