"cpufreq: fix serialization issues with freq change notifiers" breaks cpufreq too

"cpufreq: fix serialization issues with freq change notifiers" breaks cpufreq too

[Guennadi Liakhovetski]

Sorry guys, I'm trying my best to stop this patch from propagating to 
stable and to get it fixed asap, so, the CC list might be a bit excessive. 
Also trying to fix the originally spare cc list, which makes it impossible 
for me to reply to the original thread, instead have to start a new one.

Commit

commit dceff5ce18801dddc220d6238628619c93bc3cb6
Author: Viresh Kumar <viresh.kumar@linaro.org>
Date:   Sun Sep 1 22:19:37 2013 +0530

    cpufreq: fix serialization issues with freq change notifiers

breaks .transition_ongoing counting. This leads to cpufreq-cpu0 not 
working any more. In particular switching the governor from performance to 
powersave directly after boot doesn't result in a frequency switch any 
more. Reverting this patch fixes the problem again. Tested with today's 
-next.

Please, refrain from including into "stable" until clarified!

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

"cpufreq: fix serialization issues with freq change notifiers" breaks cpufreq too

[Guennadi Liakhovetski]

Sorry guys, I'm trying my best to stop this patch from propagating to 
stable and to get it fixed asap, so, the CC list might be a bit excessive. 
Also trying to fix the originally spare cc list, which makes it impossible 
for me to reply to the original thread, instead have to start a new one.

Commit

commit dceff5ce18801dddc220d6238628619c93bc3cb6
Author: Viresh Kumar <viresh.kumar@linaro.org>
Date:   Sun Sep 1 22:19:37 2013 +0530

    cpufreq: fix serialization issues with freq change notifiers

breaks .transition_ongoing counting. This leads to cpufreq-cpu0 not 
working any more. In particular switching the governor from performance to 
powersave directly after boot doesn't result in a frequency switch any 
more. Reverting this patch fixes the problem again. Tested with today's 
-next.

Please, refrain from including into "stable" until clarified!

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

"cpufreq: fix serialization issues with freq change notifiers" breaks cpufreq too

[Guennadi Liakhovetski]

Sorry guys, I'm trying my best to stop this patch from propagating to 
stable and to get it fixed asap, so, the CC list might be a bit excessive. 
Also trying to fix the originally spare cc list, which makes it impossible 
for me to reply to the original thread, instead have to start a new one.

Commit

commit dceff5ce18801dddc220d6238628619c93bc3cb6
Author: Viresh Kumar <viresh.kumar@linaro.org>
Date:   Sun Sep 1 22:19:37 2013 +0530

    cpufreq: fix serialization issues with freq change notifiers

breaks .transition_ongoing counting. This leads to cpufreq-cpu0 not 
working any more. In particular switching the governor from performance to 
powersave directly after boot doesn't result in a frequency switch any 
more. Reverting this patch fixes the problem again. Tested with today's 
-next.

Please, refrain from including into "stable" until clarified!

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

"cpufreq: fix serialization issues with freq change notifiers" breaks cpufreq too

[Guennadi Liakhovetski]

Sorry guys, I'm trying my best to stop this patch from propagating to 
stable and to get it fixed asap, so, the CC list might be a bit excessive. 
Also trying to fix the originally spare cc list, which makes it impossible 
for me to reply to the original thread, instead have to start a new one.

Commit

commit dceff5ce18801dddc220d6238628619c93bc3cb6
Author: Viresh Kumar <viresh.kumar@linaro.org>
Date:   Sun Sep 1 22:19:37 2013 +0530

    cpufreq: fix serialization issues with freq change notifiers

breaks .transition_ongoing counting. This leads to cpufreq-cpu0 not 
working any more. In particular switching the governor from performance to 
powersave directly after boot doesn't result in a frequency switch any 
more. Reverting this patch fixes the problem again. Tested with today's 
-next.

Please, refrain from including into "stable" until clarified!

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: "cpufreq: fix serialization issues with freq change notifiers" breaks cpufreq too

[Rafael J. Wysocki]

Hi,

On Monday, September 09, 2013 05:11:10 PM Guennadi Liakhovetski wrote:
> Sorry guys, I'm trying my best to stop this patch from propagating to 
> stable and to get it fixed asap, so, the CC list might be a bit excessive. 
> Also trying to fix the originally spare cc list, which makes it impossible 
> for me to reply to the original thread, instead have to start a new one.

I'm not sure what you're talking about.  What exactly was wrong with the
original CC list in particular?

> Commit
> 
> commit dceff5ce18801dddc220d6238628619c93bc3cb6
> Author: Viresh Kumar <viresh.kumar@linaro.org>
> Date:   Sun Sep 1 22:19:37 2013 +0530
> 
>     cpufreq: fix serialization issues with freq change notifiers
> 
> breaks .transition_ongoing counting.

Do you know how exactly it breaks that?  If so, care to share that knowledge?

> This leads to cpufreq-cpu0 not working any more. In particular switching the
> governor from performance to powersave directly after boot doesn't result in
> a frequency switch any more. Reverting this patch fixes the problem again.

However, this is a regression fix, so I'd prefer to fix the problem on top of
it instead of reverting this commit entirely.

> Tested with today's 
> -next.
> 
> Please, refrain from including into "stable" until clarified!

Well, dropping the commit altogether and dropping the "CC stable" tag are
equally disruptive at this point, so I think I'll just defer all of the
cpufreq fixes I wanted to push for 3.12 before the ending of the merge
window.

Thanks,
Rafael

Re: "cpufreq: fix serialization issues with freq change notifiers" breaks cpufreq too

[Rafael J. Wysocki]

Hi,

On Monday, September 09, 2013 05:11:10 PM Guennadi Liakhovetski wrote:
> Sorry guys, I'm trying my best to stop this patch from propagating to 
> stable and to get it fixed asap, so, the CC list might be a bit excessive. 
> Also trying to fix the originally spare cc list, which makes it impossible 
> for me to reply to the original thread, instead have to start a new one.

I'm not sure what you're talking about.  What exactly was wrong with the
original CC list in particular?

> Commit
> 
> commit dceff5ce18801dddc220d6238628619c93bc3cb6
> Author: Viresh Kumar <viresh.kumar@linaro.org>
> Date:   Sun Sep 1 22:19:37 2013 +0530
> 
>     cpufreq: fix serialization issues with freq change notifiers
> 
> breaks .transition_ongoing counting.

Do you know how exactly it breaks that?  If so, care to share that knowledge?

> This leads to cpufreq-cpu0 not working any more. In particular switching the
> governor from performance to powersave directly after boot doesn't result in
> a frequency switch any more. Reverting this patch fixes the problem again.

However, this is a regression fix, so I'd prefer to fix the problem on top of
it instead of reverting this commit entirely.

> Tested with today's 
> -next.
> 
> Please, refrain from including into "stable" until clarified!

Well, dropping the commit altogether and dropping the "CC stable" tag are
equally disruptive at this point, so I think I'll just defer all of the
cpufreq fixes I wanted to push for 3.12 before the ending of the merge
window.

Thanks,
Rafael

"cpufreq: fix serialization issues with freq change notifiers" breaks cpufreq too

[Rafael J. Wysocki]

Hi,

On Monday, September 09, 2013 05:11:10 PM Guennadi Liakhovetski wrote:
> Sorry guys, I'm trying my best to stop this patch from propagating to 
> stable and to get it fixed asap, so, the CC list might be a bit excessive. 
> Also trying to fix the originally spare cc list, which makes it impossible 
> for me to reply to the original thread, instead have to start a new one.

I'm not sure what you're talking about.  What exactly was wrong with the
original CC list in particular?

> Commit
> 
> commit dceff5ce18801dddc220d6238628619c93bc3cb6
> Author: Viresh Kumar <viresh.kumar@linaro.org>
> Date:   Sun Sep 1 22:19:37 2013 +0530
> 
>     cpufreq: fix serialization issues with freq change notifiers
> 
> breaks .transition_ongoing counting.

Do you know how exactly it breaks that?  If so, care to share that knowledge?

> This leads to cpufreq-cpu0 not working any more. In particular switching the
> governor from performance to powersave directly after boot doesn't result in
> a frequency switch any more. Reverting this patch fixes the problem again.

However, this is a regression fix, so I'd prefer to fix the problem on top of
it instead of reverting this commit entirely.

> Tested with today's 
> -next.
> 
> Please, refrain from including into "stable" until clarified!

Well, dropping the commit altogether and dropping the "CC stable" tag are
equally disruptive at this point, so I think I'll just defer all of the
cpufreq fixes I wanted to push for 3.12 before the ending of the merge
window.

Thanks,
Rafael

Re: "cpufreq: fix serialization issues with freq change notifiers" breaks cpufreq too

[Guennadi Liakhovetski]

Hi Rafael

On Mon, 9 Sep 2013, Rafael J. Wysocki wrote:

> Hi,
> 
> On Monday, September 09, 2013 05:11:10 PM Guennadi Liakhovetski wrote:
> > Sorry guys, I'm trying my best to stop this patch from propagating to 
> > stable and to get it fixed asap, so, the CC list might be a bit excessive. 
> > Also trying to fix the originally spare cc list, which makes it impossible 
> > for me to reply to the original thread, instead have to start a new one.
> 
> I'm not sure what you're talking about.  What exactly was wrong with the
> original CC list in particular?

I think you advised once to cc cpufreq related mails to linux-pm too at 
least. I haven't found this patch in my pm archive, have I missed it 
there?

> > Commit
> > 
> > commit dceff5ce18801dddc220d6238628619c93bc3cb6
> > Author: Viresh Kumar <viresh.kumar@linaro.org>
> > Date:   Sun Sep 1 22:19:37 2013 +0530
> > 
> >     cpufreq: fix serialization issues with freq change notifiers
> > 
> > breaks .transition_ongoing counting.
> 
> Do you know how exactly it breaks that?  If so, care to share that knowledge?

No, I don't. I only know that in __cpufreq_driver_target() the check for

	if (policy->transition_ongoing) {
		write_unlock_irqrestore(&cpufreq_driver_lock, flags);
		return -EBUSY;
	}

is failing with this patch and cpufreq-cpu0.

> > This leads to cpufreq-cpu0 not working any more. In particular switching the
> > governor from performance to powersave directly after boot doesn't result in
> > a frequency switch any more. Reverting this patch fixes the problem again.
> 
> However, this is a regression fix, so I'd prefer to fix the problem on top of
> it instead of reverting this commit entirely.

If I understood correctly, this patch fixed some warnings, that, however, 
didn't disrupt functionality, is this right? Whereas the patch really 
seems to break working set ups.

Anyway, we know about the problem now and I believe it'll get fixed one 
way or another.

Thanks
Guennadi

> > Tested with today's 
> > -next.
> > 
> > Please, refrain from including into "stable" until clarified!
> 
> Well, dropping the commit altogether and dropping the "CC stable" tag are
> equally disruptive at this point, so I think I'll just defer all of the
> cpufreq fixes I wanted to push for 3.12 before the ending of the merge
> window.
> 
> Thanks,
> Rafael
> 

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: "cpufreq: fix serialization issues with freq change notifiers" breaks cpufreq too

[Guennadi Liakhovetski]

Hi Rafael

On Mon, 9 Sep 2013, Rafael J. Wysocki wrote:

> Hi,
> 
> On Monday, September 09, 2013 05:11:10 PM Guennadi Liakhovetski wrote:
> > Sorry guys, I'm trying my best to stop this patch from propagating to 
> > stable and to get it fixed asap, so, the CC list might be a bit excessive. 
> > Also trying to fix the originally spare cc list, which makes it impossible 
> > for me to reply to the original thread, instead have to start a new one.
> 
> I'm not sure what you're talking about.  What exactly was wrong with the
> original CC list in particular?

I think you advised once to cc cpufreq related mails to linux-pm too at 
least. I haven't found this patch in my pm archive, have I missed it 
there?

> > Commit
> > 
> > commit dceff5ce18801dddc220d6238628619c93bc3cb6
> > Author: Viresh Kumar <viresh.kumar@linaro.org>
> > Date:   Sun Sep 1 22:19:37 2013 +0530
> > 
> >     cpufreq: fix serialization issues with freq change notifiers
> > 
> > breaks .transition_ongoing counting.
> 
> Do you know how exactly it breaks that?  If so, care to share that knowledge?

No, I don't. I only know that in __cpufreq_driver_target() the check for

	if (policy->transition_ongoing) {
		write_unlock_irqrestore(&cpufreq_driver_lock, flags);
		return -EBUSY;
	}

is failing with this patch and cpufreq-cpu0.

> > This leads to cpufreq-cpu0 not working any more. In particular switching the
> > governor from performance to powersave directly after boot doesn't result in
> > a frequency switch any more. Reverting this patch fixes the problem again.
> 
> However, this is a regression fix, so I'd prefer to fix the problem on top of
> it instead of reverting this commit entirely.

If I understood correctly, this patch fixed some warnings, that, however, 
didn't disrupt functionality, is this right? Whereas the patch really 
seems to break working set ups.

Anyway, we know about the problem now and I believe it'll get fixed one 
way or another.

Thanks
Guennadi

> > Tested with today's 
> > -next.
> > 
> > Please, refrain from including into "stable" until clarified!
> 
> Well, dropping the commit altogether and dropping the "CC stable" tag are
> equally disruptive at this point, so I think I'll just defer all of the
> cpufreq fixes I wanted to push for 3.12 before the ending of the merge
> window.
> 
> Thanks,
> Rafael
> 

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

"cpufreq: fix serialization issues with freq change notifiers" breaks cpufreq too

[Guennadi Liakhovetski]

Hi Rafael

On Mon, 9 Sep 2013, Rafael J. Wysocki wrote:

> Hi,
> 
> On Monday, September 09, 2013 05:11:10 PM Guennadi Liakhovetski wrote:
> > Sorry guys, I'm trying my best to stop this patch from propagating to 
> > stable and to get it fixed asap, so, the CC list might be a bit excessive. 
> > Also trying to fix the originally spare cc list, which makes it impossible 
> > for me to reply to the original thread, instead have to start a new one.
> 
> I'm not sure what you're talking about.  What exactly was wrong with the
> original CC list in particular?

I think you advised once to cc cpufreq related mails to linux-pm too at 
least. I haven't found this patch in my pm archive, have I missed it 
there?

> > Commit
> > 
> > commit dceff5ce18801dddc220d6238628619c93bc3cb6
> > Author: Viresh Kumar <viresh.kumar@linaro.org>
> > Date:   Sun Sep 1 22:19:37 2013 +0530
> > 
> >     cpufreq: fix serialization issues with freq change notifiers
> > 
> > breaks .transition_ongoing counting.
> 
> Do you know how exactly it breaks that?  If so, care to share that knowledge?

No, I don't. I only know that in __cpufreq_driver_target() the check for

	if (policy->transition_ongoing) {
		write_unlock_irqrestore(&cpufreq_driver_lock, flags);
		return -EBUSY;
	}

is failing with this patch and cpufreq-cpu0.

> > This leads to cpufreq-cpu0 not working any more. In particular switching the
> > governor from performance to powersave directly after boot doesn't result in
> > a frequency switch any more. Reverting this patch fixes the problem again.
> 
> However, this is a regression fix, so I'd prefer to fix the problem on top of
> it instead of reverting this commit entirely.

If I understood correctly, this patch fixed some warnings, that, however, 
didn't disrupt functionality, is this right? Whereas the patch really 
seems to break working set ups.

Anyway, we know about the problem now and I believe it'll get fixed one 
way or another.

Thanks
Guennadi

> > Tested with today's 
> > -next.
> > 
> > Please, refrain from including into "stable" until clarified!
> 
> Well, dropping the commit altogether and dropping the "CC stable" tag are
> equally disruptive at this point, so I think I'll just defer all of the
> cpufreq fixes I wanted to push for 3.12 before the ending of the merge
> window.
> 
> Thanks,
> Rafael
> 

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: "cpufreq: fix serialization issues with freq change notifiers" breaks cpufreq too

[Rafael J. Wysocki]

On Monday, September 09, 2013 11:42:41 PM Guennadi Liakhovetski wrote:
> Hi Rafael
> 
> On Mon, 9 Sep 2013, Rafael J. Wysocki wrote:
> 
> > Hi,
> > 
> > On Monday, September 09, 2013 05:11:10 PM Guennadi Liakhovetski wrote:
> > > Sorry guys, I'm trying my best to stop this patch from propagating to 
> > > stable and to get it fixed asap, so, the CC list might be a bit excessive. 
> > > Also trying to fix the originally spare cc list, which makes it impossible 
> > > for me to reply to the original thread, instead have to start a new one.
> > 
> > I'm not sure what you're talking about.  What exactly was wrong with the
> > original CC list in particular?
> 
> I think you advised once to cc cpufreq related mails to linux-pm too at 
> least.

Yes, I did.

> I haven't found this patch in my pm archive, have I missed it there?

Quite frankly, I don't remember if it was there.  ISTR having it it patchwork,
which would mean that it was there, but well.

> > > Commit
> > > 
> > > commit dceff5ce18801dddc220d6238628619c93bc3cb6
> > > Author: Viresh Kumar <viresh.kumar@linaro.org>
> > > Date:   Sun Sep 1 22:19:37 2013 +0530
> > > 
> > >     cpufreq: fix serialization issues with freq change notifiers
> > > 
> > > breaks .transition_ongoing counting.
> > 
> > Do you know how exactly it breaks that?  If so, care to share that knowledge?
> 
> No, I don't. I only know that in __cpufreq_driver_target() the check for
> 
> 	if (policy->transition_ongoing) {
> 		write_unlock_irqrestore(&cpufreq_driver_lock, flags);
> 		return -EBUSY;
> 	}
> 
> is failing with this patch and cpufreq-cpu0.

OK, we need to figure out that, then.

> > > This leads to cpufreq-cpu0 not working any more. In particular switching the
> > > governor from performance to powersave directly after boot doesn't result in
> > > a frequency switch any more. Reverting this patch fixes the problem again.
> > 
> > However, this is a regression fix, so I'd prefer to fix the problem on top of
> > it instead of reverting this commit entirely.
> 
> If I understood correctly, this patch fixed some warnings, that, however, 
> didn't disrupt functionality, is this right? Whereas the patch really 
> seems to break working set ups.

It fixed warnings that indicated problems and those problems should rather be
avoided.

Thanks,
Rafael

Re: "cpufreq: fix serialization issues with freq change notifiers" breaks cpufreq too

[Rafael J. Wysocki]

On Monday, September 09, 2013 11:42:41 PM Guennadi Liakhovetski wrote:
> Hi Rafael
> 
> On Mon, 9 Sep 2013, Rafael J. Wysocki wrote:
> 
> > Hi,
> > 
> > On Monday, September 09, 2013 05:11:10 PM Guennadi Liakhovetski wrote:
> > > Sorry guys, I'm trying my best to stop this patch from propagating to 
> > > stable and to get it fixed asap, so, the CC list might be a bit excessive. 
> > > Also trying to fix the originally spare cc list, which makes it impossible 
> > > for me to reply to the original thread, instead have to start a new one.
> > 
> > I'm not sure what you're talking about.  What exactly was wrong with the
> > original CC list in particular?
> 
> I think you advised once to cc cpufreq related mails to linux-pm too at 
> least.

Yes, I did.

> I haven't found this patch in my pm archive, have I missed it there?

Quite frankly, I don't remember if it was there.  ISTR having it it patchwork,
which would mean that it was there, but well.

> > > Commit
> > > 
> > > commit dceff5ce18801dddc220d6238628619c93bc3cb6
> > > Author: Viresh Kumar <viresh.kumar@linaro.org>
> > > Date:   Sun Sep 1 22:19:37 2013 +0530
> > > 
> > >     cpufreq: fix serialization issues with freq change notifiers
> > > 
> > > breaks .transition_ongoing counting.
> > 
> > Do you know how exactly it breaks that?  If so, care to share that knowledge?
> 
> No, I don't. I only know that in __cpufreq_driver_target() the check for
> 
> 	if (policy->transition_ongoing) {
> 		write_unlock_irqrestore(&cpufreq_driver_lock, flags);
> 		return -EBUSY;
> 	}
> 
> is failing with this patch and cpufreq-cpu0.

OK, we need to figure out that, then.

> > > This leads to cpufreq-cpu0 not working any more. In particular switching the
> > > governor from performance to powersave directly after boot doesn't result in
> > > a frequency switch any more. Reverting this patch fixes the problem again.
> > 
> > However, this is a regression fix, so I'd prefer to fix the problem on top of
> > it instead of reverting this commit entirely.
> 
> If I understood correctly, this patch fixed some warnings, that, however, 
> didn't disrupt functionality, is this right? Whereas the patch really 
> seems to break working set ups.

It fixed warnings that indicated problems and those problems should rather be
avoided.

Thanks,
Rafael

"cpufreq: fix serialization issues with freq change notifiers" breaks cpufreq too

[Rafael J. Wysocki]

On Monday, September 09, 2013 11:42:41 PM Guennadi Liakhovetski wrote:
> Hi Rafael
> 
> On Mon, 9 Sep 2013, Rafael J. Wysocki wrote:
> 
> > Hi,
> > 
> > On Monday, September 09, 2013 05:11:10 PM Guennadi Liakhovetski wrote:
> > > Sorry guys, I'm trying my best to stop this patch from propagating to 
> > > stable and to get it fixed asap, so, the CC list might be a bit excessive. 
> > > Also trying to fix the originally spare cc list, which makes it impossible 
> > > for me to reply to the original thread, instead have to start a new one.
> > 
> > I'm not sure what you're talking about.  What exactly was wrong with the
> > original CC list in particular?
> 
> I think you advised once to cc cpufreq related mails to linux-pm too at 
> least.

Yes, I did.

> I haven't found this patch in my pm archive, have I missed it there?

Quite frankly, I don't remember if it was there.  ISTR having it it patchwork,
which would mean that it was there, but well.

> > > Commit
> > > 
> > > commit dceff5ce18801dddc220d6238628619c93bc3cb6
> > > Author: Viresh Kumar <viresh.kumar@linaro.org>
> > > Date:   Sun Sep 1 22:19:37 2013 +0530
> > > 
> > >     cpufreq: fix serialization issues with freq change notifiers
> > > 
> > > breaks .transition_ongoing counting.
> > 
> > Do you know how exactly it breaks that?  If so, care to share that knowledge?
> 
> No, I don't. I only know that in __cpufreq_driver_target() the check for
> 
> 	if (policy->transition_ongoing) {
> 		write_unlock_irqrestore(&cpufreq_driver_lock, flags);
> 		return -EBUSY;
> 	}
> 
> is failing with this patch and cpufreq-cpu0.

OK, we need to figure out that, then.

> > > This leads to cpufreq-cpu0 not working any more. In particular switching the
> > > governor from performance to powersave directly after boot doesn't result in
> > > a frequency switch any more. Reverting this patch fixes the problem again.
> > 
> > However, this is a regression fix, so I'd prefer to fix the problem on top of
> > it instead of reverting this commit entirely.
> 
> If I understood correctly, this patch fixed some warnings, that, however, 
> didn't disrupt functionality, is this right? Whereas the patch really 
> seems to break working set ups.

It fixed warnings that indicated problems and those problems should rather be
avoided.

Thanks,
Rafael

Re: "cpufreq: fix serialization issues with freq change notifiers" breaks cpufreq too

[Rafael J. Wysocki]

On Tuesday, September 10, 2013 01:12:49 AM Rafael J. Wysocki wrote:
> On Monday, September 09, 2013 11:42:41 PM Guennadi Liakhovetski wrote:
> > Hi Rafael
> > 
> > On Mon, 9 Sep 2013, Rafael J. Wysocki wrote:
> > 
> > > Hi,
> > > 
> > > On Monday, September 09, 2013 05:11:10 PM Guennadi Liakhovetski wrote:
> > > > Sorry guys, I'm trying my best to stop this patch from propagating to 
> > > > stable and to get it fixed asap, so, the CC list might be a bit excessive. 
> > > > Also trying to fix the originally spare cc list, which makes it impossible 
> > > > for me to reply to the original thread, instead have to start a new one.
> > > 
> > > I'm not sure what you're talking about.  What exactly was wrong with the
> > > original CC list in particular?
> > 
> > I think you advised once to cc cpufreq related mails to linux-pm too at 
> > least.
> 
> Yes, I did.
> 
> > I haven't found this patch in my pm archive, have I missed it there?
> 
> Quite frankly, I don't remember if it was there.  ISTR having it it patchwork,
> which would mean that it was there, but well.
> 
> > > > Commit
> > > > 
> > > > commit dceff5ce18801dddc220d6238628619c93bc3cb6
> > > > Author: Viresh Kumar <viresh.kumar@linaro.org>
> > > > Date:   Sun Sep 1 22:19:37 2013 +0530
> > > > 
> > > >     cpufreq: fix serialization issues with freq change notifiers
> > > > 
> > > > breaks .transition_ongoing counting.
> > > 
> > > Do you know how exactly it breaks that?  If so, care to share that knowledge?
> > 
> > No, I don't. I only know that in __cpufreq_driver_target() the check for
> > 
> > 	if (policy->transition_ongoing) {
> > 		write_unlock_irqrestore(&cpufreq_driver_lock, flags);
> > 		return -EBUSY;
> > 	}
> > 
> > is failing with this patch and cpufreq-cpu0.
> 
> OK, we need to figure out that, then.

But given the timing I think I'll just start to revert things and we can add
them back later after we've sorted out all problems.

So I'm going to drop commit dceff5c from the linux-next branch and I'm going to
revert commit 7c30ed5 along with commit 266c13d that tried to fix it and we'll
revisit the transition serialization issue when we really know how to fix it.

Thanks,
Rafael

Re: "cpufreq: fix serialization issues with freq change notifiers" breaks cpufreq too

[Rafael J. Wysocki]

On Tuesday, September 10, 2013 01:12:49 AM Rafael J. Wysocki wrote:
> On Monday, September 09, 2013 11:42:41 PM Guennadi Liakhovetski wrote:
> > Hi Rafael
> > 
> > On Mon, 9 Sep 2013, Rafael J. Wysocki wrote:
> > 
> > > Hi,
> > > 
> > > On Monday, September 09, 2013 05:11:10 PM Guennadi Liakhovetski wrote:
> > > > Sorry guys, I'm trying my best to stop this patch from propagating to 
> > > > stable and to get it fixed asap, so, the CC list might be a bit excessive. 
> > > > Also trying to fix the originally spare cc list, which makes it impossible 
> > > > for me to reply to the original thread, instead have to start a new one.
> > > 
> > > I'm not sure what you're talking about.  What exactly was wrong with the
> > > original CC list in particular?
> > 
> > I think you advised once to cc cpufreq related mails to linux-pm too at 
> > least.
> 
> Yes, I did.
> 
> > I haven't found this patch in my pm archive, have I missed it there?
> 
> Quite frankly, I don't remember if it was there.  ISTR having it it patchwork,
> which would mean that it was there, but well.
> 
> > > > Commit
> > > > 
> > > > commit dceff5ce18801dddc220d6238628619c93bc3cb6
> > > > Author: Viresh Kumar <viresh.kumar@linaro.org>
> > > > Date:   Sun Sep 1 22:19:37 2013 +0530
> > > > 
> > > >     cpufreq: fix serialization issues with freq change notifiers
> > > > 
> > > > breaks .transition_ongoing counting.
> > > 
> > > Do you know how exactly it breaks that?  If so, care to share that knowledge?
> > 
> > No, I don't. I only know that in __cpufreq_driver_target() the check for
> > 
> > 	if (policy->transition_ongoing) {
> > 		write_unlock_irqrestore(&cpufreq_driver_lock, flags);
> > 		return -EBUSY;
> > 	}
> > 
> > is failing with this patch and cpufreq-cpu0.
> 
> OK, we need to figure out that, then.

But given the timing I think I'll just start to revert things and we can add
them back later after we've sorted out all problems.

So I'm going to drop commit dceff5c from the linux-next branch and I'm going to
revert commit 7c30ed5 along with commit 266c13d that tried to fix it and we'll
revisit the transition serialization issue when we really know how to fix it.

Thanks,
Rafael

Re: "cpufreq: fix serialization issues with freq change notifiers" breaks cpufreq too

[Viresh Kumar]

On 9 September 2013 20:41, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> Sorry guys, I'm trying my best to stop this patch from propagating to
> stable and to get it fixed asap, so, the CC list might be a bit excessive.
> Also trying to fix the originally spare cc list, which makes it impossible
> for me to reply to the original thread, instead have to start a new one.
>
> Commit
>
> commit dceff5ce18801dddc220d6238628619c93bc3cb6
> Author: Viresh Kumar <viresh.kumar@linaro.org>
> Date:   Sun Sep 1 22:19:37 2013 +0530
>
>     cpufreq: fix serialization issues with freq change notifiers
>
> breaks .transition_ongoing counting. This leads to cpufreq-cpu0 not
> working any more. In particular switching the governor from performance to
> powersave directly after boot doesn't result in a frequency switch any
> more. Reverting this patch fixes the problem again. Tested with today's
> -next.

I have tested it again on my exynos and intel machines and couldn't see
a single problem with this patch..

I am afraid you need to give us some more information on how it broke
your stuff.. :)

And I am also not sure cpufreq-cpu0 is different then any other driver..

--
viresh

Re: "cpufreq: fix serialization issues with freq change notifiers" breaks cpufreq too

[Viresh Kumar]

On 9 September 2013 20:41, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> Sorry guys, I'm trying my best to stop this patch from propagating to
> stable and to get it fixed asap, so, the CC list might be a bit excessive.
> Also trying to fix the originally spare cc list, which makes it impossible
> for me to reply to the original thread, instead have to start a new one.
>
> Commit
>
> commit dceff5ce18801dddc220d6238628619c93bc3cb6
> Author: Viresh Kumar <viresh.kumar@linaro.org>
> Date:   Sun Sep 1 22:19:37 2013 +0530
>
>     cpufreq: fix serialization issues with freq change notifiers
>
> breaks .transition_ongoing counting. This leads to cpufreq-cpu0 not
> working any more. In particular switching the governor from performance to
> powersave directly after boot doesn't result in a frequency switch any
> more. Reverting this patch fixes the problem again. Tested with today's
> -next.

I have tested it again on my exynos and intel machines and couldn't see
a single problem with this patch..

I am afraid you need to give us some more information on how it broke
your stuff.. :)

And I am also not sure cpufreq-cpu0 is different then any other driver..

--
viresh

"cpufreq: fix serialization issues with freq change notifiers" breaks cpufreq too

[Viresh Kumar]

On 9 September 2013 20:41, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> Sorry guys, I'm trying my best to stop this patch from propagating to
> stable and to get it fixed asap, so, the CC list might be a bit excessive.
> Also trying to fix the originally spare cc list, which makes it impossible
> for me to reply to the original thread, instead have to start a new one.
>
> Commit
>
> commit dceff5ce18801dddc220d6238628619c93bc3cb6
> Author: Viresh Kumar <viresh.kumar@linaro.org>
> Date:   Sun Sep 1 22:19:37 2013 +0530
>
>     cpufreq: fix serialization issues with freq change notifiers
>
> breaks .transition_ongoing counting. This leads to cpufreq-cpu0 not
> working any more. In particular switching the governor from performance to
> powersave directly after boot doesn't result in a frequency switch any
> more. Reverting this patch fixes the problem again. Tested with today's
> -next.

I have tested it again on my exynos and intel machines and couldn't see
a single problem with this patch..

I am afraid you need to give us some more information on how it broke
your stuff.. :)

And I am also not sure cpufreq-cpu0 is different then any other driver..

--
viresh

Re: "cpufreq: fix serialization issues with freq change notifiers" breaks cpufreq too

[Viresh Kumar]

On 9 September 2013 20:41, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> Sorry guys, I'm trying my best to stop this patch from propagating to
> stable and to get it fixed asap, so, the CC list might be a bit excessive.
> Also trying to fix the originally spare cc list, which makes it impossible
> for me to reply to the original thread, instead have to start a new one.
>
> Commit
>
> commit dceff5ce18801dddc220d6238628619c93bc3cb6
> Author: Viresh Kumar <viresh.kumar@linaro.org>
> Date:   Sun Sep 1 22:19:37 2013 +0530
>
>     cpufreq: fix serialization issues with freq change notifiers
>
> breaks .transition_ongoing counting. This leads to cpufreq-cpu0 not
> working any more. In particular switching the governor from performance to
> powersave directly after boot doesn't result in a frequency switch any
> more. Reverting this patch fixes the problem again. Tested with today's
> -next.

I have tested it again on my exynos and intel machines and couldn't see
a single problem with this patch..

I am afraid you need to give us some more information on how it broke
your stuff.. :)

And I am also not sure cpufreq-cpu0 is different then any other driver..

--
viresh

Re: "cpufreq: fix serialization issues with freq change notifiers" breaks cpufreq too

[Rafael J. Wysocki]

On Tuesday, September 10, 2013 04:59:29 PM Viresh Kumar wrote:
> On 9 September 2013 20:41, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> > Sorry guys, I'm trying my best to stop this patch from propagating to
> > stable and to get it fixed asap, so, the CC list might be a bit excessive.
> > Also trying to fix the originally spare cc list, which makes it impossible
> > for me to reply to the original thread, instead have to start a new one.
> >
> > Commit
> >
> > commit dceff5ce18801dddc220d6238628619c93bc3cb6
> > Author: Viresh Kumar <viresh.kumar@linaro.org>
> > Date:   Sun Sep 1 22:19:37 2013 +0530
> >
> >     cpufreq: fix serialization issues with freq change notifiers
> >
> > breaks .transition_ongoing counting. This leads to cpufreq-cpu0 not
> > working any more. In particular switching the governor from performance to
> > powersave directly after boot doesn't result in a frequency switch any
> > more. Reverting this patch fixes the problem again. Tested with today's
> > -next.
> 
> I have tested it again on my exynos and intel machines and couldn't see
> a single problem with this patch..
> 
> I am afraid you need to give us some more information on how it broke
> your stuff.. :)
> 
> And I am also not sure cpufreq-cpu0 is different then any other driver..

That said I'm actually unsure what problems *exactly* are fixed by commit
7c30ed5.  The commit log only says that PRECHANGE or POSTCHAGE shouldn't be
called twice in a row, but it doesn't say why.  As the fallout indicates,
that actually happened before commit 7c30ed5 and nothing visibly broke, so
the benefit from having that commit is questionable to me.  On the other hand,
the commit itself is evidently broken, so what exactly is the reason for
keeping it?

Rafael

Re: "cpufreq: fix serialization issues with freq change notifiers" breaks cpufreq too

[Rafael J. Wysocki]

On Tuesday, September 10, 2013 04:59:29 PM Viresh Kumar wrote:
> On 9 September 2013 20:41, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> > Sorry guys, I'm trying my best to stop this patch from propagating to
> > stable and to get it fixed asap, so, the CC list might be a bit excessive.
> > Also trying to fix the originally spare cc list, which makes it impossible
> > for me to reply to the original thread, instead have to start a new one.
> >
> > Commit
> >
> > commit dceff5ce18801dddc220d6238628619c93bc3cb6
> > Author: Viresh Kumar <viresh.kumar@linaro.org>
> > Date:   Sun Sep 1 22:19:37 2013 +0530
> >
> >     cpufreq: fix serialization issues with freq change notifiers
> >
> > breaks .transition_ongoing counting. This leads to cpufreq-cpu0 not
> > working any more. In particular switching the governor from performance to
> > powersave directly after boot doesn't result in a frequency switch any
> > more. Reverting this patch fixes the problem again. Tested with today's
> > -next.
> 
> I have tested it again on my exynos and intel machines and couldn't see
> a single problem with this patch..
> 
> I am afraid you need to give us some more information on how it broke
> your stuff.. :)
> 
> And I am also not sure cpufreq-cpu0 is different then any other driver..

That said I'm actually unsure what problems *exactly* are fixed by commit
7c30ed5.  The commit log only says that PRECHANGE or POSTCHAGE shouldn't be
called twice in a row, but it doesn't say why.  As the fallout indicates,
that actually happened before commit 7c30ed5 and nothing visibly broke, so
the benefit from having that commit is questionable to me.  On the other hand,
the commit itself is evidently broken, so what exactly is the reason for
keeping it?

Rafael

Re: "cpufreq: fix serialization issues with freq change notifiers" breaks cpufreq too

[Rafael J. Wysocki]

On Tuesday, September 10, 2013 04:59:29 PM Viresh Kumar wrote:
> On 9 September 2013 20:41, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> > Sorry guys, I'm trying my best to stop this patch from propagating to
> > stable and to get it fixed asap, so, the CC list might be a bit excessive.
> > Also trying to fix the originally spare cc list, which makes it impossible
> > for me to reply to the original thread, instead have to start a new one.
> >
> > Commit
> >
> > commit dceff5ce18801dddc220d6238628619c93bc3cb6
> > Author: Viresh Kumar <viresh.kumar@linaro.org>
> > Date:   Sun Sep 1 22:19:37 2013 +0530
> >
> >     cpufreq: fix serialization issues with freq change notifiers
> >
> > breaks .transition_ongoing counting. This leads to cpufreq-cpu0 not
> > working any more. In particular switching the governor from performance to
> > powersave directly after boot doesn't result in a frequency switch any
> > more. Reverting this patch fixes the problem again. Tested with today's
> > -next.
> 
> I have tested it again on my exynos and intel machines and couldn't see
> a single problem with this patch..
> 
> I am afraid you need to give us some more information on how it broke
> your stuff.. :)
> 
> And I am also not sure cpufreq-cpu0 is different then any other driver..

That said I'm actually unsure what problems *exactly* are fixed by commit
7c30ed5.  The commit log only says that PRECHANGE or POSTCHAGE shouldn't be
called twice in a row, but it doesn't say why.  As the fallout indicates,
that actually happened before commit 7c30ed5 and nothing visibly broke, so
the benefit from having that commit is questionable to me.  On the other hand,
the commit itself is evidently broken, so what exactly is the reason for
keeping it?

Rafael

"cpufreq: fix serialization issues with freq change notifiers" breaks cpufreq too

[Rafael J. Wysocki]

On Tuesday, September 10, 2013 04:59:29 PM Viresh Kumar wrote:
> On 9 September 2013 20:41, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> > Sorry guys, I'm trying my best to stop this patch from propagating to
> > stable and to get it fixed asap, so, the CC list might be a bit excessive.
> > Also trying to fix the originally spare cc list, which makes it impossible
> > for me to reply to the original thread, instead have to start a new one.
> >
> > Commit
> >
> > commit dceff5ce18801dddc220d6238628619c93bc3cb6
> > Author: Viresh Kumar <viresh.kumar@linaro.org>
> > Date:   Sun Sep 1 22:19:37 2013 +0530
> >
> >     cpufreq: fix serialization issues with freq change notifiers
> >
> > breaks .transition_ongoing counting. This leads to cpufreq-cpu0 not
> > working any more. In particular switching the governor from performance to
> > powersave directly after boot doesn't result in a frequency switch any
> > more. Reverting this patch fixes the problem again. Tested with today's
> > -next.
> 
> I have tested it again on my exynos and intel machines and couldn't see
> a single problem with this patch..
> 
> I am afraid you need to give us some more information on how it broke
> your stuff.. :)
> 
> And I am also not sure cpufreq-cpu0 is different then any other driver..

That said I'm actually unsure what problems *exactly* are fixed by commit
7c30ed5.  The commit log only says that PRECHANGE or POSTCHAGE shouldn't be
called twice in a row, but it doesn't say why.  As the fallout indicates,
that actually happened before commit 7c30ed5 and nothing visibly broke, so
the benefit from having that commit is questionable to me.  On the other hand,
the commit itself is evidently broken, so what exactly is the reason for
keeping it?

Rafael

Re: "cpufreq: fix serialization issues with freq change notifiers" breaks cpufreq too

[Guennadi Liakhovetski]

On Tue, 10 Sep 2013, Viresh Kumar wrote:

> On 9 September 2013 20:41, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> > Sorry guys, I'm trying my best to stop this patch from propagating to
> > stable and to get it fixed asap, so, the CC list might be a bit excessive.
> > Also trying to fix the originally spare cc list, which makes it impossible
> > for me to reply to the original thread, instead have to start a new one.
> >
> > Commit
> >
> > commit dceff5ce18801dddc220d6238628619c93bc3cb6
> > Author: Viresh Kumar <viresh.kumar@linaro.org>
> > Date:   Sun Sep 1 22:19:37 2013 +0530
> >
> >     cpufreq: fix serialization issues with freq change notifiers
> >
> > breaks .transition_ongoing counting. This leads to cpufreq-cpu0 not
> > working any more. In particular switching the governor from performance to
> > powersave directly after boot doesn't result in a frequency switch any
> > more. Reverting this patch fixes the problem again. Tested with today's
> > -next.
> 
> I have tested it again on my exynos and intel machines and couldn't see
> a single problem with this patch..

Ok, here's what I've just done.

1. checkout -next tag next-20130909

commit 98926a8004b453089368fda456b8c869240e9953
Author: Stephen Rothwell <sfr@canb.auug.org.au>
Date:   Mon Sep 9 16:05:53 2013 +1000

    Add linux-next specific files for 20130909

2. built and booted a kernel for kzm9g (.config available on request)

3. cpufreq failed to initialise:

cpufreq_cpu0: failed to find cpu0 node
cpufreq-cpu0: probe of cpufreq-cpu0 failed with error -2

due to commit f837a9b5ab05c52a07108c6f09ca66f2e0aee757 - as reported in 
another thread.

4. reverted that commit, resolving a trivial conflict. Added a debug 
output in __cpufreq_driver_target() of

 
 	if (cpufreq_disabled())
 		return -ENODEV;
+	pr_info("%s() %d\n", __func__, policy->transition_ongoing);
 	if (policy->transition_ongoing)
 		return -EBUSY;
 

Built and booted, got

cpufreq: __cpufreq_driver_target(): 1

printed out 4 times from the beginning.

5. tried

echo powersave > /sys/devices/system/cpu/cpu0/cpufreq/scaling_governor

the above output appeared 2 more times - no frequency change resulted.

6. reverted commit dceff5ce18801dddc220d6238628619c93bc3cb6, built booted 
- cpufreq works again.

> I am afraid you need to give us some more information on how it broke
> your stuff.. :)

Hope the above is enough.

Thanks
Guennadi

> And I am also not sure cpufreq-cpu0 is different then any other driver..
> 
> --
> viresh

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: "cpufreq: fix serialization issues with freq change notifiers" breaks cpufreq too

[Guennadi Liakhovetski]

On Tue, 10 Sep 2013, Viresh Kumar wrote:

> On 9 September 2013 20:41, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> > Sorry guys, I'm trying my best to stop this patch from propagating to
> > stable and to get it fixed asap, so, the CC list might be a bit excessive.
> > Also trying to fix the originally spare cc list, which makes it impossible
> > for me to reply to the original thread, instead have to start a new one.
> >
> > Commit
> >
> > commit dceff5ce18801dddc220d6238628619c93bc3cb6
> > Author: Viresh Kumar <viresh.kumar@linaro.org>
> > Date:   Sun Sep 1 22:19:37 2013 +0530
> >
> >     cpufreq: fix serialization issues with freq change notifiers
> >
> > breaks .transition_ongoing counting. This leads to cpufreq-cpu0 not
> > working any more. In particular switching the governor from performance to
> > powersave directly after boot doesn't result in a frequency switch any
> > more. Reverting this patch fixes the problem again. Tested with today's
> > -next.
> 
> I have tested it again on my exynos and intel machines and couldn't see
> a single problem with this patch..

Ok, here's what I've just done.

1. checkout -next tag next-20130909

commit 98926a8004b453089368fda456b8c869240e9953
Author: Stephen Rothwell <sfr@canb.auug.org.au>
Date:   Mon Sep 9 16:05:53 2013 +1000

    Add linux-next specific files for 20130909

2. built and booted a kernel for kzm9g (.config available on request)

3. cpufreq failed to initialise:

cpufreq_cpu0: failed to find cpu0 node
cpufreq-cpu0: probe of cpufreq-cpu0 failed with error -2

due to commit f837a9b5ab05c52a07108c6f09ca66f2e0aee757 - as reported in 
another thread.

4. reverted that commit, resolving a trivial conflict. Added a debug 
output in __cpufreq_driver_target() of

 
 	if (cpufreq_disabled())
 		return -ENODEV;
+	pr_info("%s() %d\n", __func__, policy->transition_ongoing);
 	if (policy->transition_ongoing)
 		return -EBUSY;
 

Built and booted, got

cpufreq: __cpufreq_driver_target(): 1

printed out 4 times from the beginning.

5. tried

echo powersave > /sys/devices/system/cpu/cpu0/cpufreq/scaling_governor

the above output appeared 2 more times - no frequency change resulted.

6. reverted commit dceff5ce18801dddc220d6238628619c93bc3cb6, built booted 
- cpufreq works again.

> I am afraid you need to give us some more information on how it broke
> your stuff.. :)

Hope the above is enough.

Thanks
Guennadi

> And I am also not sure cpufreq-cpu0 is different then any other driver..
> 
> --
> viresh

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: "cpufreq: fix serialization issues with freq change notifiers" breaks cpufreq too

[Guennadi Liakhovetski]

On Tue, 10 Sep 2013, Viresh Kumar wrote:

> On 9 September 2013 20:41, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> > Sorry guys, I'm trying my best to stop this patch from propagating to
> > stable and to get it fixed asap, so, the CC list might be a bit excessive.
> > Also trying to fix the originally spare cc list, which makes it impossible
> > for me to reply to the original thread, instead have to start a new one.
> >
> > Commit
> >
> > commit dceff5ce18801dddc220d6238628619c93bc3cb6
> > Author: Viresh Kumar <viresh.kumar@linaro.org>
> > Date:   Sun Sep 1 22:19:37 2013 +0530
> >
> >     cpufreq: fix serialization issues with freq change notifiers
> >
> > breaks .transition_ongoing counting. This leads to cpufreq-cpu0 not
> > working any more. In particular switching the governor from performance to
> > powersave directly after boot doesn't result in a frequency switch any
> > more. Reverting this patch fixes the problem again. Tested with today's
> > -next.
> 
> I have tested it again on my exynos and intel machines and couldn't see
> a single problem with this patch..

Ok, here's what I've just done.

1. checkout -next tag next-20130909

commit 98926a8004b453089368fda456b8c869240e9953
Author: Stephen Rothwell <sfr@canb.auug.org.au>
Date:   Mon Sep 9 16:05:53 2013 +1000

    Add linux-next specific files for 20130909

2. built and booted a kernel for kzm9g (.config available on request)

3. cpufreq failed to initialise:

cpufreq_cpu0: failed to find cpu0 node
cpufreq-cpu0: probe of cpufreq-cpu0 failed with error -2

due to commit f837a9b5ab05c52a07108c6f09ca66f2e0aee757 - as reported in 
another thread.

4. reverted that commit, resolving a trivial conflict. Added a debug 
output in __cpufreq_driver_target() of

 
 	if (cpufreq_disabled())
 		return -ENODEV;
+	pr_info("%s() %d\n", __func__, policy->transition_ongoing);
 	if (policy->transition_ongoing)
 		return -EBUSY;
 

Built and booted, got

cpufreq: __cpufreq_driver_target(): 1

printed out 4 times from the beginning.

5. tried

echo powersave > /sys/devices/system/cpu/cpu0/cpufreq/scaling_governor

the above output appeared 2 more times - no frequency change resulted.

6. reverted commit dceff5ce18801dddc220d6238628619c93bc3cb6, built booted 
- cpufreq works again.

> I am afraid you need to give us some more information on how it broke
> your stuff.. :)

Hope the above is enough.

Thanks
Guennadi

> And I am also not sure cpufreq-cpu0 is different then any other driver..
> 
> --
> viresh

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

"cpufreq: fix serialization issues with freq change notifiers" breaks cpufreq too

[Guennadi Liakhovetski]

On Tue, 10 Sep 2013, Viresh Kumar wrote:

> On 9 September 2013 20:41, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> > Sorry guys, I'm trying my best to stop this patch from propagating to
> > stable and to get it fixed asap, so, the CC list might be a bit excessive.
> > Also trying to fix the originally spare cc list, which makes it impossible
> > for me to reply to the original thread, instead have to start a new one.
> >
> > Commit
> >
> > commit dceff5ce18801dddc220d6238628619c93bc3cb6
> > Author: Viresh Kumar <viresh.kumar@linaro.org>
> > Date:   Sun Sep 1 22:19:37 2013 +0530
> >
> >     cpufreq: fix serialization issues with freq change notifiers
> >
> > breaks .transition_ongoing counting. This leads to cpufreq-cpu0 not
> > working any more. In particular switching the governor from performance to
> > powersave directly after boot doesn't result in a frequency switch any
> > more. Reverting this patch fixes the problem again. Tested with today's
> > -next.
> 
> I have tested it again on my exynos and intel machines and couldn't see
> a single problem with this patch..

Ok, here's what I've just done.

1. checkout -next tag next-20130909

commit 98926a8004b453089368fda456b8c869240e9953
Author: Stephen Rothwell <sfr@canb.auug.org.au>
Date:   Mon Sep 9 16:05:53 2013 +1000

    Add linux-next specific files for 20130909

2. built and booted a kernel for kzm9g (.config available on request)

3. cpufreq failed to initialise:

cpufreq_cpu0: failed to find cpu0 node
cpufreq-cpu0: probe of cpufreq-cpu0 failed with error -2

due to commit f837a9b5ab05c52a07108c6f09ca66f2e0aee757 - as reported in 
another thread.

4. reverted that commit, resolving a trivial conflict. Added a debug 
output in __cpufreq_driver_target() of

 
 	if (cpufreq_disabled())
 		return -ENODEV;
+	pr_info("%s() %d\n", __func__, policy->transition_ongoing);
 	if (policy->transition_ongoing)
 		return -EBUSY;
 

Built and booted, got

cpufreq: __cpufreq_driver_target(): 1

printed out 4 times from the beginning.

5. tried

echo powersave > /sys/devices/system/cpu/cpu0/cpufreq/scaling_governor

the above output appeared 2 more times - no frequency change resulted.

6. reverted commit dceff5ce18801dddc220d6238628619c93bc3cb6, built booted 
- cpufreq works again.

> I am afraid you need to give us some more information on how it broke
> your stuff.. :)

Hope the above is enough.

Thanks
Guennadi

> And I am also not sure cpufreq-cpu0 is different then any other driver..
> 
> --
> viresh

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: "cpufreq: fix serialization issues with freq change notifiers" breaks cpufreq too

[Viresh Kumar]

On 10 September 2013 17:19, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> That said I'm actually unsure what problems *exactly* are fixed by commit
> 7c30ed5.  The commit log only says that PRECHANGE or POSTCHAGE shouldn't be
> called twice in a row, but it doesn't say why.  As the fallout indicates,
> that actually happened before commit 7c30ed5 and nothing visibly broke, so
> the benefit from having that commit is questionable to me.  On the other hand,
> the commit itself is evidently broken, so what exactly is the reason for
> keeping it?

Okay, so the first question is can we have multiple PRECHANGE notfication
done without any POSTCHANGE inbetween?
- Scenario: One reading value of cpuinfo_cur_freq, which will call
__cpufreq_cpu_get()->cpufreq_out_of_sync()->cpufreq_notify_transition()..

And ondemand governor trying to change freq of cpu and so doing notification..

There can be more similar scenarios possible..


Now Second question, is this fine to have multiple PRECHANGE notfications
before any POSTCHANGE notification?

Logically it looks obvious to me that these must be serialized..
Otherwise this is what we are broadcasting:
- We are changing for freq X, please prepare and let us know if you are
okay with it..
- Oh.. So sorry, we are changing to freq Y instead, please adjust accordingly.

- Yes we have changed to freq Y...
- Yes we have changed to freq X...

This looks stupid, isn't it? I don't know how the drivers would behave when
they see such notifications and so got to this patch initially..

Re: "cpufreq: fix serialization issues with freq change notifiers" breaks cpufreq too

[Viresh Kumar]

On 10 September 2013 17:19, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> That said I'm actually unsure what problems *exactly* are fixed by commit
> 7c30ed5.  The commit log only says that PRECHANGE or POSTCHAGE shouldn't be
> called twice in a row, but it doesn't say why.  As the fallout indicates,
> that actually happened before commit 7c30ed5 and nothing visibly broke, so
> the benefit from having that commit is questionable to me.  On the other hand,
> the commit itself is evidently broken, so what exactly is the reason for
> keeping it?

Okay, so the first question is can we have multiple PRECHANGE notfication
done without any POSTCHANGE inbetween?
- Scenario: One reading value of cpuinfo_cur_freq, which will call
__cpufreq_cpu_get()->cpufreq_out_of_sync()->cpufreq_notify_transition()..

And ondemand governor trying to change freq of cpu and so doing notification..

There can be more similar scenarios possible..


Now Second question, is this fine to have multiple PRECHANGE notfications
before any POSTCHANGE notification?

Logically it looks obvious to me that these must be serialized..
Otherwise this is what we are broadcasting:
- We are changing for freq X, please prepare and let us know if you are
okay with it..
- Oh.. So sorry, we are changing to freq Y instead, please adjust accordingly.

- Yes we have changed to freq Y...
- Yes we have changed to freq X...

This looks stupid, isn't it? I don't know how the drivers would behave when
they see such notifications and so got to this patch initially..

"cpufreq: fix serialization issues with freq change notifiers" breaks cpufreq too

[Viresh Kumar]

On 10 September 2013 17:19, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> That said I'm actually unsure what problems *exactly* are fixed by commit
> 7c30ed5.  The commit log only says that PRECHANGE or POSTCHAGE shouldn't be
> called twice in a row, but it doesn't say why.  As the fallout indicates,
> that actually happened before commit 7c30ed5 and nothing visibly broke, so
> the benefit from having that commit is questionable to me.  On the other hand,
> the commit itself is evidently broken, so what exactly is the reason for
> keeping it?

Okay, so the first question is can we have multiple PRECHANGE notfication
done without any POSTCHANGE inbetween?
- Scenario: One reading value of cpuinfo_cur_freq, which will call
__cpufreq_cpu_get()->cpufreq_out_of_sync()->cpufreq_notify_transition()..

And ondemand governor trying to change freq of cpu and so doing notification..

There can be more similar scenarios possible..


Now Second question, is this fine to have multiple PRECHANGE notfications
before any POSTCHANGE notification?

Logically it looks obvious to me that these must be serialized..
Otherwise this is what we are broadcasting:
- We are changing for freq X, please prepare and let us know if you are
okay with it..
- Oh.. So sorry, we are changing to freq Y instead, please adjust accordingly.

- Yes we have changed to freq Y...
- Yes we have changed to freq X...

This looks stupid, isn't it? I don't know how the drivers would behave when
they see such notifications and so got to this patch initially..

Re: "cpufreq: fix serialization issues with freq change notifiers" breaks cpufreq too

[Viresh Kumar]

On 10 September 2013 17:19, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> That said I'm actually unsure what problems *exactly* are fixed by commit
> 7c30ed5.  The commit log only says that PRECHANGE or POSTCHAGE shouldn't be
> called twice in a row, but it doesn't say why.  As the fallout indicates,
> that actually happened before commit 7c30ed5 and nothing visibly broke, so
> the benefit from having that commit is questionable to me.  On the other hand,
> the commit itself is evidently broken, so what exactly is the reason for
> keeping it?

Okay, so the first question is can we have multiple PRECHANGE notfication
done without any POSTCHANGE inbetween?
- Scenario: One reading value of cpuinfo_cur_freq, which will call
__cpufreq_cpu_get()->cpufreq_out_of_sync()->cpufreq_notify_transition()..

And ondemand governor trying to change freq of cpu and so doing notification..

There can be more similar scenarios possible..


Now Second question, is this fine to have multiple PRECHANGE notfications
before any POSTCHANGE notification?

Logically it looks obvious to me that these must be serialized..
Otherwise this is what we are broadcasting:
- We are changing for freq X, please prepare and let us know if you are
okay with it..
- Oh.. So sorry, we are changing to freq Y instead, please adjust accordingly.

- Yes we have changed to freq Y...
- Yes we have changed to freq X...

This looks stupid, isn't it? I don't know how the drivers would behave when
they see such notifications and so got to this patch initially..

Re: "cpufreq: fix serialization issues with freq change notifiers" breaks cpufreq too

[Viresh Kumar]

On 10 September 2013 20:42, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> 4. reverted that commit, resolving a trivial conflict. Added a debug
> output in __cpufreq_driver_target() of
>
>
>         if (cpufreq_disabled())
>                 return -ENODEV;
> +       pr_info("%s() %d\n", __func__, policy->transition_ongoing);
>         if (policy->transition_ongoing)
>                 return -EBUSY;

Are you sure this diff is on linux-next and not after the revert that
you mentioned later in the mail? There is some locking introduced
by my patch which I can't see in you diff..

> Built and booted, got
>
> cpufreq: __cpufreq_driver_target(): 1
>
> printed out 4 times from the beginning.
>
> 5. tried
>
> echo powersave > /sys/devices/system/cpu/cpu0/cpufreq/scaling_governor
>
> the above output appeared 2 more times - no frequency change resulted.
>
> 6. reverted commit dceff5ce18801dddc220d6238628619c93bc3cb6, built booted
> - cpufreq works again.
>
>> I am afraid you need to give us some more information on how it broke
>> your stuff.. :)
>
> Hope the above is enough.

A bit more would be helpful.. Can you add these debug prints to all the places
where transition_ongoing is getting modified? with %s, __func__ to distinguish
them better? That will make it a bit easy for me...

Also, what's the configuration of your system? How many CPUs? And are all
of them sharing clock? (I believe yes, as you are using cpufreq-cpu0)..

Re: "cpufreq: fix serialization issues with freq change notifiers" breaks cpufreq too

[Viresh Kumar]

On 10 September 2013 20:42, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> 4. reverted that commit, resolving a trivial conflict. Added a debug
> output in __cpufreq_driver_target() of
>
>
>         if (cpufreq_disabled())
>                 return -ENODEV;
> +       pr_info("%s() %d\n", __func__, policy->transition_ongoing);
>         if (policy->transition_ongoing)
>                 return -EBUSY;

Are you sure this diff is on linux-next and not after the revert that
you mentioned later in the mail? There is some locking introduced
by my patch which I can't see in you diff..

> Built and booted, got
>
> cpufreq: __cpufreq_driver_target(): 1
>
> printed out 4 times from the beginning.
>
> 5. tried
>
> echo powersave > /sys/devices/system/cpu/cpu0/cpufreq/scaling_governor
>
> the above output appeared 2 more times - no frequency change resulted.
>
> 6. reverted commit dceff5ce18801dddc220d6238628619c93bc3cb6, built booted
> - cpufreq works again.
>
>> I am afraid you need to give us some more information on how it broke
>> your stuff.. :)
>
> Hope the above is enough.

A bit more would be helpful.. Can you add these debug prints to all the places
where transition_ongoing is getting modified? with %s, __func__ to distinguish
them better? That will make it a bit easy for me...

Also, what's the configuration of your system? How many CPUs? And are all
of them sharing clock? (I believe yes, as you are using cpufreq-cpu0)..

"cpufreq: fix serialization issues with freq change notifiers" breaks cpufreq too

[Viresh Kumar]

On 10 September 2013 20:42, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> 4. reverted that commit, resolving a trivial conflict. Added a debug
> output in __cpufreq_driver_target() of
>
>
>         if (cpufreq_disabled())
>                 return -ENODEV;
> +       pr_info("%s() %d\n", __func__, policy->transition_ongoing);
>         if (policy->transition_ongoing)
>                 return -EBUSY;

Are you sure this diff is on linux-next and not after the revert that
you mentioned later in the mail? There is some locking introduced
by my patch which I can't see in you diff..

> Built and booted, got
>
> cpufreq: __cpufreq_driver_target(): 1
>
> printed out 4 times from the beginning.
>
> 5. tried
>
> echo powersave > /sys/devices/system/cpu/cpu0/cpufreq/scaling_governor
>
> the above output appeared 2 more times - no frequency change resulted.
>
> 6. reverted commit dceff5ce18801dddc220d6238628619c93bc3cb6, built booted
> - cpufreq works again.
>
>> I am afraid you need to give us some more information on how it broke
>> your stuff.. :)
>
> Hope the above is enough.

A bit more would be helpful.. Can you add these debug prints to all the places
where transition_ongoing is getting modified? with %s, __func__ to distinguish
them better? That will make it a bit easy for me...

Also, what's the configuration of your system? How many CPUs? And are all
of them sharing clock? (I believe yes, as you are using cpufreq-cpu0)..

Re: "cpufreq: fix serialization issues with freq change notifiers" breaks cpufreq too

[Viresh Kumar]

On 10 September 2013 20:42, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> 4. reverted that commit, resolving a trivial conflict. Added a debug
> output in __cpufreq_driver_target() of
>
>
>         if (cpufreq_disabled())
>                 return -ENODEV;
> +       pr_info("%s() %d\n", __func__, policy->transition_ongoing);
>         if (policy->transition_ongoing)
>                 return -EBUSY;

Are you sure this diff is on linux-next and not after the revert that
you mentioned later in the mail? There is some locking introduced
by my patch which I can't see in you diff..

> Built and booted, got
>
> cpufreq: __cpufreq_driver_target(): 1
>
> printed out 4 times from the beginning.
>
> 5. tried
>
> echo powersave > /sys/devices/system/cpu/cpu0/cpufreq/scaling_governor
>
> the above output appeared 2 more times - no frequency change resulted.
>
> 6. reverted commit dceff5ce18801dddc220d6238628619c93bc3cb6, built booted
> - cpufreq works again.
>
>> I am afraid you need to give us some more information on how it broke
>> your stuff.. :)
>
> Hope the above is enough.

A bit more would be helpful.. Can you add these debug prints to all the places
where transition_ongoing is getting modified? with %s, __func__ to distinguish
them better? That will make it a bit easy for me...

Also, what's the configuration of your system? How many CPUs? And are all
of them sharing clock? (I believe yes, as you are using cpufreq-cpu0)..

Re: "cpufreq: fix serialization issues with freq change notifiers" breaks cpufreq too

[Guennadi Liakhovetski]

On Tue, 10 Sep 2013, Viresh Kumar wrote:

> On 10 September 2013 20:42, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> > 4. reverted that commit, resolving a trivial conflict. Added a debug
> > output in __cpufreq_driver_target() of
> >
> >
> >         if (cpufreq_disabled())
> >                 return -ENODEV;
> > +       pr_info("%s() %d\n", __func__, policy->transition_ongoing);
> >         if (policy->transition_ongoing)
> >                 return -EBUSY;
> 
> Are you sure this diff is on linux-next and not after the revert that
> you mentioned later in the mail? There is some locking introduced
> by my patch which I can't see in you diff..

Of course, isn't that what I've written above? reverted a commit and added 
debug - in that order.

> > Built and booted, got
> >
> > cpufreq: __cpufreq_driver_target(): 1
> >
> > printed out 4 times from the beginning.
> >
> > 5. tried
> >
> > echo powersave > /sys/devices/system/cpu/cpu0/cpufreq/scaling_governor
> >
> > the above output appeared 2 more times - no frequency change resulted.
> >
> > 6. reverted commit dceff5ce18801dddc220d6238628619c93bc3cb6, built booted
> > - cpufreq works again.
> >
> >> I am afraid you need to give us some more information on how it broke
> >> your stuff.. :)
> >
> > Hope the above is enough.
> 
> A bit more would be helpful.. Can you add these debug prints to all the places
> where transition_ongoing is getting modified? with %s, __func__ to distinguish
> them better? That will make it a bit easy for me...

Sure, I can... So, with the performance governor I get

[    1.290000] cpufreq-cpu0 cpufreq-cpu0: Looking up cpu0-supply from device tree
[    1.290000] cpufreq: trying to register driver generic_cpu0
[    1.290000] cpufreq: adding CPU 0
[    1.290000] cpufreq: Adding link for CPU: 1
[    1.290000] cpufreq: setting new policy for CPU 0: 398667 - 1196000 kHz
[    1.290000] cpufreq: new min and max freqs are 398667 - 1196000 kHz
[    1.290000] cpufreq: governor switch
[    1.290000] cpufreq: __cpufreq_governor for CPU 0, event 4
[    1.290000] cpufreq: __cpufreq_governor for CPU 0, event 1
[    1.290000] cpufreq_performance: setting to 1196000 kHz because of event 1
[    1.290000] cpufreq: __cpufreq_driver_target().1665 1

This is my debug - .transition_ongoing is incremented ^^^^^^^^

[    1.300000] cpufreq: target for CPU 0: 1196000 kHz, relation 1, requested 1196000 kHz
[    1.300000] cpufreq: governor: change or update limits
[    1.300000] cpufreq: __cpufreq_governor for CPU 0, event 3
[    1.300000] cpufreq_performance: setting to 1196000 kHz because of event 3
[    1.300000] cpufreq: initialization complete
[    1.300000] cpufreq: adding CPU 1
[    1.300000] cpufreq: driver generic_cpu0 up and running

That's it. It never gets decremented again.

> Also, what's the configuration of your system? How many CPUs?

2 CPU cores.

> And are all of them sharing clock? (I believe yes, as you are using cpufreq-cpu0)..

Correct. Debug diff is below.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index ecc55d1..374e030 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -15,6 +15,8 @@
  * published by the Free Software Foundation.
  */
 
+#define DEBUG
+
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
 #include <linux/cpu.h>
@@ -292,6 +294,7 @@ static void __cpufreq_notify_transition(struct cpufreq_policy *policy,
 
 		policy->transition_ongoing++;
 		write_unlock_irqrestore(&cpufreq_driver_lock, flags);
+		pr_info("%s().%d %d\n", __func__, __LINE__, policy->transition_ongoing);
 
 		/* detect if the driver reported a value as "old frequency"
 		 * which is not equal to what the cpufreq core thinks is
@@ -321,6 +324,7 @@ static void __cpufreq_notify_transition(struct cpufreq_policy *policy,
 
 		policy->transition_ongoing--;
 		write_unlock_irqrestore(&cpufreq_driver_lock, flags);
+		pr_info("%s().%d %d\n", __func__, __LINE__, policy->transition_ongoing);
 
 		adjust_jiffies(CPUFREQ_POSTCHANGE, freqs);
 		pr_debug("FREQ: %lu - CPU: %lu", (unsigned long)freqs->new,
@@ -359,6 +363,7 @@ void cpufreq_notify_transition(struct cpufreq_policy *policy,
 		write_lock_irqsave(&cpufreq_driver_lock, flags);
 		policy->transition_ongoing--;
 		write_unlock_irqrestore(&cpufreq_driver_lock, flags);
+		pr_info("%s().%d %d\n", __func__, __LINE__, policy->transition_ongoing);
 	}
 }
 EXPORT_SYMBOL_GPL(cpufreq_notify_transition);
@@ -1356,6 +1361,7 @@ static void cpufreq_out_of_sync(unsigned int cpu, unsigned int old_freq,
 	}
 	policy->transition_ongoing++;
 	write_unlock_irqrestore(&cpufreq_driver_lock, flags);
+	pr_info("%s().%d %d\n", __func__, __LINE__, policy->transition_ongoing);
 
 	cpufreq_notify_transition(policy, &freqs, CPUFREQ_PRECHANGE);
 	cpufreq_notify_transition(policy, &freqs, CPUFREQ_POSTCHANGE);
@@ -1656,6 +1662,7 @@ int __cpufreq_driver_target(struct cpufreq_policy *policy,
 	}
 	policy->transition_ongoing++;
 	write_unlock_irqrestore(&cpufreq_driver_lock, flags);
+	pr_info("%s().%d %d\n", __func__, __LINE__, policy->transition_ongoing);
 
 	/* Make sure that target_freq is within supported range */
 	if (target_freq > policy->max)
diff --git a/drivers/cpufreq/cpufreq_performance.c b/drivers/cpufreq/cpufreq_performance.c
index cf117de..5575b08 100644
--- a/drivers/cpufreq/cpufreq_performance.c
+++ b/drivers/cpufreq/cpufreq_performance.c
@@ -10,6 +10,8 @@
  *
  */
 
+#define DEBUG
+
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
 #include <linux/cpufreq.h>

Re: "cpufreq: fix serialization issues with freq change notifiers" breaks cpufreq too

[Guennadi Liakhovetski]

On Tue, 10 Sep 2013, Viresh Kumar wrote:

> On 10 September 2013 20:42, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> > 4. reverted that commit, resolving a trivial conflict. Added a debug
> > output in __cpufreq_driver_target() of
> >
> >
> >         if (cpufreq_disabled())
> >                 return -ENODEV;
> > +       pr_info("%s() %d\n", __func__, policy->transition_ongoing);
> >         if (policy->transition_ongoing)
> >                 return -EBUSY;
> 
> Are you sure this diff is on linux-next and not after the revert that
> you mentioned later in the mail? There is some locking introduced
> by my patch which I can't see in you diff..

Of course, isn't that what I've written above? reverted a commit and added 
debug - in that order.

> > Built and booted, got
> >
> > cpufreq: __cpufreq_driver_target(): 1
> >
> > printed out 4 times from the beginning.
> >
> > 5. tried
> >
> > echo powersave > /sys/devices/system/cpu/cpu0/cpufreq/scaling_governor
> >
> > the above output appeared 2 more times - no frequency change resulted.
> >
> > 6. reverted commit dceff5ce18801dddc220d6238628619c93bc3cb6, built booted
> > - cpufreq works again.
> >
> >> I am afraid you need to give us some more information on how it broke
> >> your stuff.. :)
> >
> > Hope the above is enough.
> 
> A bit more would be helpful.. Can you add these debug prints to all the places
> where transition_ongoing is getting modified? with %s, __func__ to distinguish
> them better? That will make it a bit easy for me...

Sure, I can... So, with the performance governor I get

[    1.290000] cpufreq-cpu0 cpufreq-cpu0: Looking up cpu0-supply from device tree
[    1.290000] cpufreq: trying to register driver generic_cpu0
[    1.290000] cpufreq: adding CPU 0
[    1.290000] cpufreq: Adding link for CPU: 1
[    1.290000] cpufreq: setting new policy for CPU 0: 398667 - 1196000 kHz
[    1.290000] cpufreq: new min and max freqs are 398667 - 1196000 kHz
[    1.290000] cpufreq: governor switch
[    1.290000] cpufreq: __cpufreq_governor for CPU 0, event 4
[    1.290000] cpufreq: __cpufreq_governor for CPU 0, event 1
[    1.290000] cpufreq_performance: setting to 1196000 kHz because of event 1
[    1.290000] cpufreq: __cpufreq_driver_target().1665 1

This is my debug - .transition_ongoing is incremented ^^^^^^^^

[    1.300000] cpufreq: target for CPU 0: 1196000 kHz, relation 1, requested 1196000 kHz
[    1.300000] cpufreq: governor: change or update limits
[    1.300000] cpufreq: __cpufreq_governor for CPU 0, event 3
[    1.300000] cpufreq_performance: setting to 1196000 kHz because of event 3
[    1.300000] cpufreq: initialization complete
[    1.300000] cpufreq: adding CPU 1
[    1.300000] cpufreq: driver generic_cpu0 up and running

That's it. It never gets decremented again.

> Also, what's the configuration of your system? How many CPUs?

2 CPU cores.

> And are all of them sharing clock? (I believe yes, as you are using cpufreq-cpu0)..

Correct. Debug diff is below.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index ecc55d1..374e030 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -15,6 +15,8 @@
  * published by the Free Software Foundation.
  */
 
+#define DEBUG
+
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
 #include <linux/cpu.h>
@@ -292,6 +294,7 @@ static void __cpufreq_notify_transition(struct cpufreq_policy *policy,
 
 		policy->transition_ongoing++;
 		write_unlock_irqrestore(&cpufreq_driver_lock, flags);
+		pr_info("%s().%d %d\n", __func__, __LINE__, policy->transition_ongoing);
 
 		/* detect if the driver reported a value as "old frequency"
 		 * which is not equal to what the cpufreq core thinks is
@@ -321,6 +324,7 @@ static void __cpufreq_notify_transition(struct cpufreq_policy *policy,
 
 		policy->transition_ongoing--;
 		write_unlock_irqrestore(&cpufreq_driver_lock, flags);
+		pr_info("%s().%d %d\n", __func__, __LINE__, policy->transition_ongoing);
 
 		adjust_jiffies(CPUFREQ_POSTCHANGE, freqs);
 		pr_debug("FREQ: %lu - CPU: %lu", (unsigned long)freqs->new,
@@ -359,6 +363,7 @@ void cpufreq_notify_transition(struct cpufreq_policy *policy,
 		write_lock_irqsave(&cpufreq_driver_lock, flags);
 		policy->transition_ongoing--;
 		write_unlock_irqrestore(&cpufreq_driver_lock, flags);
+		pr_info("%s().%d %d\n", __func__, __LINE__, policy->transition_ongoing);
 	}
 }
 EXPORT_SYMBOL_GPL(cpufreq_notify_transition);
@@ -1356,6 +1361,7 @@ static void cpufreq_out_of_sync(unsigned int cpu, unsigned int old_freq,
 	}
 	policy->transition_ongoing++;
 	write_unlock_irqrestore(&cpufreq_driver_lock, flags);
+	pr_info("%s().%d %d\n", __func__, __LINE__, policy->transition_ongoing);
 
 	cpufreq_notify_transition(policy, &freqs, CPUFREQ_PRECHANGE);
 	cpufreq_notify_transition(policy, &freqs, CPUFREQ_POSTCHANGE);
@@ -1656,6 +1662,7 @@ int __cpufreq_driver_target(struct cpufreq_policy *policy,
 	}
 	policy->transition_ongoing++;
 	write_unlock_irqrestore(&cpufreq_driver_lock, flags);
+	pr_info("%s().%d %d\n", __func__, __LINE__, policy->transition_ongoing);
 
 	/* Make sure that target_freq is within supported range */
 	if (target_freq > policy->max)
diff --git a/drivers/cpufreq/cpufreq_performance.c b/drivers/cpufreq/cpufreq_performance.c
index cf117de..5575b08 100644
--- a/drivers/cpufreq/cpufreq_performance.c
+++ b/drivers/cpufreq/cpufreq_performance.c
@@ -10,6 +10,8 @@
  *
  */
 
+#define DEBUG
+
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
 #include <linux/cpufreq.h>

Re: "cpufreq: fix serialization issues with freq change notifiers" breaks cpufreq too

[Guennadi Liakhovetski]

On Tue, 10 Sep 2013, Viresh Kumar wrote:

> On 10 September 2013 20:42, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> > 4. reverted that commit, resolving a trivial conflict. Added a debug
> > output in __cpufreq_driver_target() of
> >
> >
> >         if (cpufreq_disabled())
> >                 return -ENODEV;
> > +       pr_info("%s() %d\n", __func__, policy->transition_ongoing);
> >         if (policy->transition_ongoing)
> >                 return -EBUSY;
> 
> Are you sure this diff is on linux-next and not after the revert that
> you mentioned later in the mail? There is some locking introduced
> by my patch which I can't see in you diff..

Of course, isn't that what I've written above? reverted a commit and added 
debug - in that order.

> > Built and booted, got
> >
> > cpufreq: __cpufreq_driver_target(): 1
> >
> > printed out 4 times from the beginning.
> >
> > 5. tried
> >
> > echo powersave > /sys/devices/system/cpu/cpu0/cpufreq/scaling_governor
> >
> > the above output appeared 2 more times - no frequency change resulted.
> >
> > 6. reverted commit dceff5ce18801dddc220d6238628619c93bc3cb6, built booted
> > - cpufreq works again.
> >
> >> I am afraid you need to give us some more information on how it broke
> >> your stuff.. :)
> >
> > Hope the above is enough.
> 
> A bit more would be helpful.. Can you add these debug prints to all the places
> where transition_ongoing is getting modified? with %s, __func__ to distinguish
> them better? That will make it a bit easy for me...

Sure, I can... So, with the performance governor I get

[    1.290000] cpufreq-cpu0 cpufreq-cpu0: Looking up cpu0-supply from device tree
[    1.290000] cpufreq: trying to register driver generic_cpu0
[    1.290000] cpufreq: adding CPU 0
[    1.290000] cpufreq: Adding link for CPU: 1
[    1.290000] cpufreq: setting new policy for CPU 0: 398667 - 1196000 kHz
[    1.290000] cpufreq: new min and max freqs are 398667 - 1196000 kHz
[    1.290000] cpufreq: governor switch
[    1.290000] cpufreq: __cpufreq_governor for CPU 0, event 4
[    1.290000] cpufreq: __cpufreq_governor for CPU 0, event 1
[    1.290000] cpufreq_performance: setting to 1196000 kHz because of event 1
[    1.290000] cpufreq: __cpufreq_driver_target().1665 1

This is my debug - .transition_ongoing is incremented ^^^^^^^^

[    1.300000] cpufreq: target for CPU 0: 1196000 kHz, relation 1, requested 1196000 kHz
[    1.300000] cpufreq: governor: change or update limits
[    1.300000] cpufreq: __cpufreq_governor for CPU 0, event 3
[    1.300000] cpufreq_performance: setting to 1196000 kHz because of event 3
[    1.300000] cpufreq: initialization complete
[    1.300000] cpufreq: adding CPU 1
[    1.300000] cpufreq: driver generic_cpu0 up and running

That's it. It never gets decremented again.

> Also, what's the configuration of your system? How many CPUs?

2 CPU cores.

> And are all of them sharing clock? (I believe yes, as you are using cpufreq-cpu0)..

Correct. Debug diff is below.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index ecc55d1..374e030 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -15,6 +15,8 @@
  * published by the Free Software Foundation.
  */
 
+#define DEBUG
+
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
 #include <linux/cpu.h>
@@ -292,6 +294,7 @@ static void __cpufreq_notify_transition(struct cpufreq_policy *policy,
 
 		policy->transition_ongoing++;
 		write_unlock_irqrestore(&cpufreq_driver_lock, flags);
+		pr_info("%s().%d %d\n", __func__, __LINE__, policy->transition_ongoing);
 
 		/* detect if the driver reported a value as "old frequency"
 		 * which is not equal to what the cpufreq core thinks is
@@ -321,6 +324,7 @@ static void __cpufreq_notify_transition(struct cpufreq_policy *policy,
 
 		policy->transition_ongoing--;
 		write_unlock_irqrestore(&cpufreq_driver_lock, flags);
+		pr_info("%s().%d %d\n", __func__, __LINE__, policy->transition_ongoing);
 
 		adjust_jiffies(CPUFREQ_POSTCHANGE, freqs);
 		pr_debug("FREQ: %lu - CPU: %lu", (unsigned long)freqs->new,
@@ -359,6 +363,7 @@ void cpufreq_notify_transition(struct cpufreq_policy *policy,
 		write_lock_irqsave(&cpufreq_driver_lock, flags);
 		policy->transition_ongoing--;
 		write_unlock_irqrestore(&cpufreq_driver_lock, flags);
+		pr_info("%s().%d %d\n", __func__, __LINE__, policy->transition_ongoing);
 	}
 }
 EXPORT_SYMBOL_GPL(cpufreq_notify_transition);
@@ -1356,6 +1361,7 @@ static void cpufreq_out_of_sync(unsigned int cpu, unsigned int old_freq,
 	}
 	policy->transition_ongoing++;
 	write_unlock_irqrestore(&cpufreq_driver_lock, flags);
+	pr_info("%s().%d %d\n", __func__, __LINE__, policy->transition_ongoing);
 
 	cpufreq_notify_transition(policy, &freqs, CPUFREQ_PRECHANGE);
 	cpufreq_notify_transition(policy, &freqs, CPUFREQ_POSTCHANGE);
@@ -1656,6 +1662,7 @@ int __cpufreq_driver_target(struct cpufreq_policy *policy,
 	}
 	policy->transition_ongoing++;
 	write_unlock_irqrestore(&cpufreq_driver_lock, flags);
+	pr_info("%s().%d %d\n", __func__, __LINE__, policy->transition_ongoing);
 
 	/* Make sure that target_freq is within supported range */
 	if (target_freq > policy->max)
diff --git a/drivers/cpufreq/cpufreq_performance.c b/drivers/cpufreq/cpufreq_performance.c
index cf117de..5575b08 100644
--- a/drivers/cpufreq/cpufreq_performance.c
+++ b/drivers/cpufreq/cpufreq_performance.c
@@ -10,6 +10,8 @@
  *
  */
 
+#define DEBUG
+
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
 #include <linux/cpufreq.h>

"cpufreq: fix serialization issues with freq change notifiers" breaks cpufreq too

[Guennadi Liakhovetski]

On Tue, 10 Sep 2013, Viresh Kumar wrote:

> On 10 September 2013 20:42, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> > 4. reverted that commit, resolving a trivial conflict. Added a debug
> > output in __cpufreq_driver_target() of
> >
> >
> >         if (cpufreq_disabled())
> >                 return -ENODEV;
> > +       pr_info("%s() %d\n", __func__, policy->transition_ongoing);
> >         if (policy->transition_ongoing)
> >                 return -EBUSY;
> 
> Are you sure this diff is on linux-next and not after the revert that
> you mentioned later in the mail? There is some locking introduced
> by my patch which I can't see in you diff..

Of course, isn't that what I've written above? reverted a commit and added 
debug - in that order.

> > Built and booted, got
> >
> > cpufreq: __cpufreq_driver_target(): 1
> >
> > printed out 4 times from the beginning.
> >
> > 5. tried
> >
> > echo powersave > /sys/devices/system/cpu/cpu0/cpufreq/scaling_governor
> >
> > the above output appeared 2 more times - no frequency change resulted.
> >
> > 6. reverted commit dceff5ce18801dddc220d6238628619c93bc3cb6, built booted
> > - cpufreq works again.
> >
> >> I am afraid you need to give us some more information on how it broke
> >> your stuff.. :)
> >
> > Hope the above is enough.
> 
> A bit more would be helpful.. Can you add these debug prints to all the places
> where transition_ongoing is getting modified? with %s, __func__ to distinguish
> them better? That will make it a bit easy for me...

Sure, I can... So, with the performance governor I get

[    1.290000] cpufreq-cpu0 cpufreq-cpu0: Looking up cpu0-supply from device tree
[    1.290000] cpufreq: trying to register driver generic_cpu0
[    1.290000] cpufreq: adding CPU 0
[    1.290000] cpufreq: Adding link for CPU: 1
[    1.290000] cpufreq: setting new policy for CPU 0: 398667 - 1196000 kHz
[    1.290000] cpufreq: new min and max freqs are 398667 - 1196000 kHz
[    1.290000] cpufreq: governor switch
[    1.290000] cpufreq: __cpufreq_governor for CPU 0, event 4
[    1.290000] cpufreq: __cpufreq_governor for CPU 0, event 1
[    1.290000] cpufreq_performance: setting to 1196000 kHz because of event 1
[    1.290000] cpufreq: __cpufreq_driver_target().1665 1

This is my debug - .transition_ongoing is incremented ^^^^^^^^

[    1.300000] cpufreq: target for CPU 0: 1196000 kHz, relation 1, requested 1196000 kHz
[    1.300000] cpufreq: governor: change or update limits
[    1.300000] cpufreq: __cpufreq_governor for CPU 0, event 3
[    1.300000] cpufreq_performance: setting to 1196000 kHz because of event 3
[    1.300000] cpufreq: initialization complete
[    1.300000] cpufreq: adding CPU 1
[    1.300000] cpufreq: driver generic_cpu0 up and running

That's it. It never gets decremented again.

> Also, what's the configuration of your system? How many CPUs?

2 CPU cores.

> And are all of them sharing clock? (I believe yes, as you are using cpufreq-cpu0)..

Correct. Debug diff is below.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index ecc55d1..374e030 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -15,6 +15,8 @@
  * published by the Free Software Foundation.
  */
 
+#define DEBUG
+
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
 #include <linux/cpu.h>
@@ -292,6 +294,7 @@ static void __cpufreq_notify_transition(struct cpufreq_policy *policy,
 
 		policy->transition_ongoing++;
 		write_unlock_irqrestore(&cpufreq_driver_lock, flags);
+		pr_info("%s().%d %d\n", __func__, __LINE__, policy->transition_ongoing);
 
 		/* detect if the driver reported a value as "old frequency"
 		 * which is not equal to what the cpufreq core thinks is
@@ -321,6 +324,7 @@ static void __cpufreq_notify_transition(struct cpufreq_policy *policy,
 
 		policy->transition_ongoing--;
 		write_unlock_irqrestore(&cpufreq_driver_lock, flags);
+		pr_info("%s().%d %d\n", __func__, __LINE__, policy->transition_ongoing);
 
 		adjust_jiffies(CPUFREQ_POSTCHANGE, freqs);
 		pr_debug("FREQ: %lu - CPU: %lu", (unsigned long)freqs->new,
@@ -359,6 +363,7 @@ void cpufreq_notify_transition(struct cpufreq_policy *policy,
 		write_lock_irqsave(&cpufreq_driver_lock, flags);
 		policy->transition_ongoing--;
 		write_unlock_irqrestore(&cpufreq_driver_lock, flags);
+		pr_info("%s().%d %d\n", __func__, __LINE__, policy->transition_ongoing);
 	}
 }
 EXPORT_SYMBOL_GPL(cpufreq_notify_transition);
@@ -1356,6 +1361,7 @@ static void cpufreq_out_of_sync(unsigned int cpu, unsigned int old_freq,
 	}
 	policy->transition_ongoing++;
 	write_unlock_irqrestore(&cpufreq_driver_lock, flags);
+	pr_info("%s().%d %d\n", __func__, __LINE__, policy->transition_ongoing);
 
 	cpufreq_notify_transition(policy, &freqs, CPUFREQ_PRECHANGE);
 	cpufreq_notify_transition(policy, &freqs, CPUFREQ_POSTCHANGE);
@@ -1656,6 +1662,7 @@ int __cpufreq_driver_target(struct cpufreq_policy *policy,
 	}
 	policy->transition_ongoing++;
 	write_unlock_irqrestore(&cpufreq_driver_lock, flags);
+	pr_info("%s().%d %d\n", __func__, __LINE__, policy->transition_ongoing);
 
 	/* Make sure that target_freq is within supported range */
 	if (target_freq > policy->max)
diff --git a/drivers/cpufreq/cpufreq_performance.c b/drivers/cpufreq/cpufreq_performance.c
index cf117de..5575b08 100644
--- a/drivers/cpufreq/cpufreq_performance.c
+++ b/drivers/cpufreq/cpufreq_performance.c
@@ -10,6 +10,8 @@
  *
  */
 
+#define DEBUG
+
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
 #include <linux/cpufreq.h>

Re: "cpufreq: fix serialization issues with freq change notifiers" breaks cpufreq too

[Viresh Kumar]

[-- Attachment #1: Type: text/plain, Size: 1314 bytes --]

On 10 September 2013 21:52, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> Of course, isn't that what I've written above? reverted a commit and added
> debug - in that order.

Ok, I misread it then :(

> Sure, I can... So, with the performance governor I get
>
> [    1.290000] cpufreq-cpu0 cpufreq-cpu0: Looking up cpu0-supply from device tree
> [    1.290000] cpufreq: trying to register driver generic_cpu0
> [    1.290000] cpufreq: adding CPU 0
> [    1.290000] cpufreq: Adding link for CPU: 1
> [    1.290000] cpufreq: setting new policy for CPU 0: 398667 - 1196000 kHz
> [    1.290000] cpufreq: new min and max freqs are 398667 - 1196000 kHz
> [    1.290000] cpufreq: governor switch
> [    1.290000] cpufreq: __cpufreq_governor for CPU 0, event 4
> [    1.290000] cpufreq: __cpufreq_governor for CPU 0, event 1
> [    1.290000] cpufreq_performance: setting to 1196000 kHz because of event 1
> [    1.290000] cpufreq: __cpufreq_driver_target().1665 1
>
> This is my debug - .transition_ongoing is incremented ^^^^^^^^
>
> [    1.300000] cpufreq: target for CPU 0: 1196000 kHz, relation 1, requested 1196000 kHz

Quite straight forward actually.. Please try attached patch and see if it fixes
your problem.. Which it should if I am not wrong.. I will send it
separately then..

Thanks for your time..

[-- Attachment #2: 0001-fix-target.patch --]
[-- Type: application/octet-stream, Size: 1710 bytes --]

From 4f5379d2c10f2c6354721d9ea5915083dfa29ca7 Mon Sep 17 00:00:00 2001
Message-Id: <4f5379d2c10f2c6354721d9ea5915083dfa29ca7.1378830770.git.viresh.kumar@linaro.org>
From: Viresh Kumar <viresh.kumar@linaro.org>
Date: Tue, 10 Sep 2013 22:02:38 +0530
Subject: [PATCH] fix target()

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 09bbdb0..3d4938b 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1655,15 +1655,6 @@ int __cpufreq_driver_target(struct cpufreq_policy *policy,
 	if (cpufreq_disabled())
 		return -ENODEV;
 
-	write_lock_irqsave(&cpufreq_driver_lock, flags);
-	if (policy->transition_ongoing) {
-		pr_err("%s: Target failed\n", __func__);
-		write_unlock_irqrestore(&cpufreq_driver_lock, flags);
-		return -EBUSY;
-	}
-	policy->transition_ongoing++;
-	write_unlock_irqrestore(&cpufreq_driver_lock, flags);
-
 	/* Make sure that target_freq is within supported range */
 	if (target_freq > policy->max)
 		target_freq = policy->max;
@@ -1676,6 +1667,15 @@ int __cpufreq_driver_target(struct cpufreq_policy *policy,
 	if (target_freq == policy->cur)
 		return 0;
 
+	write_lock_irqsave(&cpufreq_driver_lock, flags);
+	if (policy->transition_ongoing) {
+		pr_err("%s: Target failed\n", __func__);
+		write_unlock_irqrestore(&cpufreq_driver_lock, flags);
+		return -EBUSY;
+	}
+	policy->transition_ongoing++;
+	write_unlock_irqrestore(&cpufreq_driver_lock, flags);
+
 	if (cpufreq_driver->target)
 		retval = cpufreq_driver->target(policy, target_freq, relation);
 
-- 
1.7.12.rc2.18.g61b472e

Re: "cpufreq: fix serialization issues with freq change notifiers" breaks cpufreq too

[Viresh Kumar]

[-- Attachment #1: Type: text/plain, Size: 1314 bytes --]

On 10 September 2013 21:52, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> Of course, isn't that what I've written above? reverted a commit and added
> debug - in that order.

Ok, I misread it then :(

> Sure, I can... So, with the performance governor I get
>
> [    1.290000] cpufreq-cpu0 cpufreq-cpu0: Looking up cpu0-supply from device tree
> [    1.290000] cpufreq: trying to register driver generic_cpu0
> [    1.290000] cpufreq: adding CPU 0
> [    1.290000] cpufreq: Adding link for CPU: 1
> [    1.290000] cpufreq: setting new policy for CPU 0: 398667 - 1196000 kHz
> [    1.290000] cpufreq: new min and max freqs are 398667 - 1196000 kHz
> [    1.290000] cpufreq: governor switch
> [    1.290000] cpufreq: __cpufreq_governor for CPU 0, event 4
> [    1.290000] cpufreq: __cpufreq_governor for CPU 0, event 1
> [    1.290000] cpufreq_performance: setting to 1196000 kHz because of event 1
> [    1.290000] cpufreq: __cpufreq_driver_target().1665 1
>
> This is my debug - .transition_ongoing is incremented ^^^^^^^^
>
> [    1.300000] cpufreq: target for CPU 0: 1196000 kHz, relation 1, requested 1196000 kHz

Quite straight forward actually.. Please try attached patch and see if it fixes
your problem.. Which it should if I am not wrong.. I will send it
separately then..

Thanks for your time..

[-- Attachment #2: 0001-fix-target.patch --]
[-- Type: application/octet-stream, Size: 1710 bytes --]

From 4f5379d2c10f2c6354721d9ea5915083dfa29ca7 Mon Sep 17 00:00:00 2001
Message-Id: <4f5379d2c10f2c6354721d9ea5915083dfa29ca7.1378830770.git.viresh.kumar@linaro.org>
From: Viresh Kumar <viresh.kumar@linaro.org>
Date: Tue, 10 Sep 2013 22:02:38 +0530
Subject: [PATCH] fix target()

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 09bbdb0..3d4938b 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1655,15 +1655,6 @@ int __cpufreq_driver_target(struct cpufreq_policy *policy,
 	if (cpufreq_disabled())
 		return -ENODEV;
 
-	write_lock_irqsave(&cpufreq_driver_lock, flags);
-	if (policy->transition_ongoing) {
-		pr_err("%s: Target failed\n", __func__);
-		write_unlock_irqrestore(&cpufreq_driver_lock, flags);
-		return -EBUSY;
-	}
-	policy->transition_ongoing++;
-	write_unlock_irqrestore(&cpufreq_driver_lock, flags);
-
 	/* Make sure that target_freq is within supported range */
 	if (target_freq > policy->max)
 		target_freq = policy->max;
@@ -1676,6 +1667,15 @@ int __cpufreq_driver_target(struct cpufreq_policy *policy,
 	if (target_freq == policy->cur)
 		return 0;
 
+	write_lock_irqsave(&cpufreq_driver_lock, flags);
+	if (policy->transition_ongoing) {
+		pr_err("%s: Target failed\n", __func__);
+		write_unlock_irqrestore(&cpufreq_driver_lock, flags);
+		return -EBUSY;
+	}
+	policy->transition_ongoing++;
+	write_unlock_irqrestore(&cpufreq_driver_lock, flags);
+
 	if (cpufreq_driver->target)
 		retval = cpufreq_driver->target(policy, target_freq, relation);
 
-- 
1.7.12.rc2.18.g61b472e

"cpufreq: fix serialization issues with freq change notifiers" breaks cpufreq too

[Viresh Kumar]

On 10 September 2013 21:52, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> Of course, isn't that what I've written above? reverted a commit and added
> debug - in that order.

Ok, I misread it then :(

> Sure, I can... So, with the performance governor I get
>
> [    1.290000] cpufreq-cpu0 cpufreq-cpu0: Looking up cpu0-supply from device tree
> [    1.290000] cpufreq: trying to register driver generic_cpu0
> [    1.290000] cpufreq: adding CPU 0
> [    1.290000] cpufreq: Adding link for CPU: 1
> [    1.290000] cpufreq: setting new policy for CPU 0: 398667 - 1196000 kHz
> [    1.290000] cpufreq: new min and max freqs are 398667 - 1196000 kHz
> [    1.290000] cpufreq: governor switch
> [    1.290000] cpufreq: __cpufreq_governor for CPU 0, event 4
> [    1.290000] cpufreq: __cpufreq_governor for CPU 0, event 1
> [    1.290000] cpufreq_performance: setting to 1196000 kHz because of event 1
> [    1.290000] cpufreq: __cpufreq_driver_target().1665 1
>
> This is my debug - .transition_ongoing is incremented ^^^^^^^^
>
> [    1.300000] cpufreq: target for CPU 0: 1196000 kHz, relation 1, requested 1196000 kHz

Quite straight forward actually.. Please try attached patch and see if it fixes
your problem.. Which it should if I am not wrong.. I will send it
separately then..

Thanks for your time..
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-fix-target.patch
Type: application/octet-stream
Size: 1710 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130910/a9e28bba/attachment-0001.obj>

Re: "cpufreq: fix serialization issues with freq change notifiers" breaks cpufreq too

[Viresh Kumar]

[-- Attachment #1: Type: text/plain, Size: 1314 bytes --]

On 10 September 2013 21:52, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> Of course, isn't that what I've written above? reverted a commit and added
> debug - in that order.

Ok, I misread it then :(

> Sure, I can... So, with the performance governor I get
>
> [    1.290000] cpufreq-cpu0 cpufreq-cpu0: Looking up cpu0-supply from device tree
> [    1.290000] cpufreq: trying to register driver generic_cpu0
> [    1.290000] cpufreq: adding CPU 0
> [    1.290000] cpufreq: Adding link for CPU: 1
> [    1.290000] cpufreq: setting new policy for CPU 0: 398667 - 1196000 kHz
> [    1.290000] cpufreq: new min and max freqs are 398667 - 1196000 kHz
> [    1.290000] cpufreq: governor switch
> [    1.290000] cpufreq: __cpufreq_governor for CPU 0, event 4
> [    1.290000] cpufreq: __cpufreq_governor for CPU 0, event 1
> [    1.290000] cpufreq_performance: setting to 1196000 kHz because of event 1
> [    1.290000] cpufreq: __cpufreq_driver_target().1665 1
>
> This is my debug - .transition_ongoing is incremented ^^^^^^^^
>
> [    1.300000] cpufreq: target for CPU 0: 1196000 kHz, relation 1, requested 1196000 kHz

Quite straight forward actually.. Please try attached patch and see if it fixes
your problem.. Which it should if I am not wrong.. I will send it
separately then..

Thanks for your time..

[-- Attachment #2: 0001-fix-target.patch --]
[-- Type: application/octet-stream, Size: 1710 bytes --]

From 4f5379d2c10f2c6354721d9ea5915083dfa29ca7 Mon Sep 17 00:00:00 2001
Message-Id: <4f5379d2c10f2c6354721d9ea5915083dfa29ca7.1378830770.git.viresh.kumar@linaro.org>
From: Viresh Kumar <viresh.kumar@linaro.org>
Date: Tue, 10 Sep 2013 22:02:38 +0530
Subject: [PATCH] fix target()

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 09bbdb0..3d4938b 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1655,15 +1655,6 @@ int __cpufreq_driver_target(struct cpufreq_policy *policy,
 	if (cpufreq_disabled())
 		return -ENODEV;
 
-	write_lock_irqsave(&cpufreq_driver_lock, flags);
-	if (policy->transition_ongoing) {
-		pr_err("%s: Target failed\n", __func__);
-		write_unlock_irqrestore(&cpufreq_driver_lock, flags);
-		return -EBUSY;
-	}
-	policy->transition_ongoing++;
-	write_unlock_irqrestore(&cpufreq_driver_lock, flags);
-
 	/* Make sure that target_freq is within supported range */
 	if (target_freq > policy->max)
 		target_freq = policy->max;
@@ -1676,6 +1667,15 @@ int __cpufreq_driver_target(struct cpufreq_policy *policy,
 	if (target_freq == policy->cur)
 		return 0;
 
+	write_lock_irqsave(&cpufreq_driver_lock, flags);
+	if (policy->transition_ongoing) {
+		pr_err("%s: Target failed\n", __func__);
+		write_unlock_irqrestore(&cpufreq_driver_lock, flags);
+		return -EBUSY;
+	}
+	policy->transition_ongoing++;
+	write_unlock_irqrestore(&cpufreq_driver_lock, flags);
+
 	if (cpufreq_driver->target)
 		retval = cpufreq_driver->target(policy, target_freq, relation);
 
-- 
1.7.12.rc2.18.g61b472e

Re: "cpufreq: fix serialization issues with freq change notifiers" breaks cpufreq too

[Guennadi Liakhovetski]

On Tue, 10 Sep 2013, Viresh Kumar wrote:

> On 10 September 2013 21:52, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> > Of course, isn't that what I've written above? reverted a commit and added
> > debug - in that order.
> 
> Ok, I misread it then :(
> 
> > Sure, I can... So, with the performance governor I get
> >
> > [    1.290000] cpufreq-cpu0 cpufreq-cpu0: Looking up cpu0-supply from device tree
> > [    1.290000] cpufreq: trying to register driver generic_cpu0
> > [    1.290000] cpufreq: adding CPU 0
> > [    1.290000] cpufreq: Adding link for CPU: 1
> > [    1.290000] cpufreq: setting new policy for CPU 0: 398667 - 1196000 kHz
> > [    1.290000] cpufreq: new min and max freqs are 398667 - 1196000 kHz
> > [    1.290000] cpufreq: governor switch
> > [    1.290000] cpufreq: __cpufreq_governor for CPU 0, event 4
> > [    1.290000] cpufreq: __cpufreq_governor for CPU 0, event 1
> > [    1.290000] cpufreq_performance: setting to 1196000 kHz because of event 1
> > [    1.290000] cpufreq: __cpufreq_driver_target().1665 1
> >
> > This is my debug - .transition_ongoing is incremented ^^^^^^^^
> >
> > [    1.300000] cpufreq: target for CPU 0: 1196000 kHz, relation 1, requested 1196000 kHz
> 
> Quite straight forward actually..

Apparently, not quite.

> Please try attached patch and see if it fixes
> your problem.. Which it should if I am not wrong.. I will send it
> separately then..

It helps only once. The first switching works, the second one doesn't. 
Below debug

[   12.010000] cpufreq: setting new policy for CPU 0: 398667 - 1196000 kHz
[   12.010000] cpufreq: new min and max freqs are 398667 - 1196000 kHz
[   12.010000] cpufreq: governor switch
[   12.010000] cpufreq: __cpufreq_governor for CPU 0, event 2
[   12.010000] cpufreq: __cpufreq_governor for CPU 0, event 5
[   12.010000] ondemand governor failed, too long transition latency of HW, fallback to performance governor
[   12.020000] cpufreq: __cpufreq_governor for CPU 0, event 4
[   12.020000] cpufreq: __cpufreq_governor for CPU 0, event 1
[   12.020000] cpufreq_performance: setting to 1196000 kHz because of event 1
[   12.020000] cpufreq: target for CPU 0: 1196000 kHz, relation 1, requested 1196000 kHz
[   12.020000] cpufreq: governor: change or update limits
[   12.020000] cpufreq: __cpufreq_governor for CPU 0, event 3
[   12.020000] cpufreq_performance: setting to 1196000 kHz because of event 3
[   12.020000] cpufreq: target for CPU 0: 1196000 kHz, relation 1, requested 1196000 kHz
[   12.030000] cpufreq: setting new policy for CPU 0: 398667 - 1196000 kHz
[   12.030000] cpufreq: new min and max freqs are 398667 - 1196000 kHz
[   12.030000] cpufreq: governor switch
[   12.030000] cpufreq: __cpufreq_governor for CPU 0, event 2
[   12.030000] cpufreq: __cpufreq_governor for CPU 0, event 5
[   12.030000] ondemand governor failed, too long transition latency of HW, fallback to performance governor
[   12.040000] cpufreq: __cpufreq_governor for CPU 0, event 4
[   12.040000] cpufreq: __cpufreq_governor for CPU 0, event 1
[   12.040000] cpufreq_performance: setting to 1196000 kHz because of event 1
[   12.040000] cpufreq: target for CPU 0: 1196000 kHz, relation 1, requested 1196000 kHz
[   12.040000] cpufreq: governor: change or update limits
[   12.040000] cpufreq: __cpufreq_governor for CPU 0, event 3
[   12.040000] cpufreq_performance: setting to 1196000 kHz because of event 3
[   12.040000] cpufreq: target for CPU 0: 1196000 kHz, relation 1, requested 1196000 kHz

echo powersave > /sys/devices/system/cpu/cpu0/cpufreq/scaling_governor

[   66.490000] cpufreq: setting new policy for CPU 0: 398667 - 1196000 kHz
[   66.490000] cpufreq: new min and max freqs are 398667 - 1196000 kHz
[   66.490000] cpufreq: governor switch
[   66.490000] cpufreq: __cpufreq_governor for CPU 0, event 2
[   66.490000] cpufreq: __cpufreq_governor for CPU 0, event 5
[   66.490000] cpufreq: __cpufreq_governor for CPU 0, event 4
[   66.490000] cpufreq: __cpufreq_governor for CPU 0, event 1
[   66.490000] cpufreq: target for CPU 0: 398667 kHz, relation 0, requested 398667 kHz
[   66.490000] cpufreq: __cpufreq_driver_target().1677 1
[   66.500000] cpufreq: notification 0 of frequency transition to 398666 kHz
[   66.500000] cpufreq: __cpufreq_notify_transition().297 2
[   66.500000] cpufreq: notification 0 of frequency transition to 398666 kHz
[   66.500000] cpufreq: __cpufreq_notify_transition().297 3
[   66.510000] cpufreq: notification 1 of frequency transition to 398666 kHz
[   66.510000] cpufreq: __cpufreq_notify_transition().327 2
[   66.520000] cpufreq: FREQ: 398666 - CPU: 0
[   66.520000] cpufreq: notification 1 of frequency transition to 398666 kHz
[   66.520000] cpufreq: __cpufreq_notify_transition().327 1
[   66.520000] cpufreq: FREQ: 398666 - CPU: 1
[   66.520000] cpufreq: cpufreq_notify_transition().366 0
[   66.530000] cpufreq: governor: change or update limits
[   66.530000] cpufreq: __cpufreq_governor for CPU 0, event 3
[   66.530000] cpufreq: target for CPU 0: 398667 kHz, relation 0, requested 398667 kHz
[   66.530000] cpufreq: __cpufreq_driver_target().1677 1

echo performance > /sys/devices/system/cpu/cpu0/cpufreq/scaling_governor

[   72.470000] cpufreq: setting new policy for CPU 0: 398667 - 1196000 kHz
[   72.470000] cpufreq: new min and max freqs are 398667 - 1196000 kHz
[   72.470000] cpufreq: governor switch
[   72.470000] cpufreq: __cpufreq_governor for CPU 0, event 2
[   72.470000] cpufreq: __cpufreq_governor for CPU 0, event 5
[   72.470000] cpufreq: __cpufreq_governor for CPU 0, event 4
[   72.470000] cpufreq: __cpufreq_governor for CPU 0, event 1
[   72.470000] cpufreq_performance: setting to 1196000 kHz because of event 1
[   72.470000] cpufreq: target for CPU 0: 1196000 kHz, relation 1, requested 1196000 kHz
[   72.470000] cpufreq: governor: change or update limits
[   72.470000] cpufreq: __cpufreq_governor for CPU 0, event 3
[   72.470000] cpufreq_performance: setting to 1196000 kHz because of event 3
[   72.470000] cpufreq: target for CPU 0: 1196000 kHz, relation 1, requested 1196000 kHz

> Thanks for your time..

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: "cpufreq: fix serialization issues with freq change notifiers" breaks cpufreq too

[Guennadi Liakhovetski]

On Tue, 10 Sep 2013, Viresh Kumar wrote:

> On 10 September 2013 21:52, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> > Of course, isn't that what I've written above? reverted a commit and added
> > debug - in that order.
> 
> Ok, I misread it then :(
> 
> > Sure, I can... So, with the performance governor I get
> >
> > [    1.290000] cpufreq-cpu0 cpufreq-cpu0: Looking up cpu0-supply from device tree
> > [    1.290000] cpufreq: trying to register driver generic_cpu0
> > [    1.290000] cpufreq: adding CPU 0
> > [    1.290000] cpufreq: Adding link for CPU: 1
> > [    1.290000] cpufreq: setting new policy for CPU 0: 398667 - 1196000 kHz
> > [    1.290000] cpufreq: new min and max freqs are 398667 - 1196000 kHz
> > [    1.290000] cpufreq: governor switch
> > [    1.290000] cpufreq: __cpufreq_governor for CPU 0, event 4
> > [    1.290000] cpufreq: __cpufreq_governor for CPU 0, event 1
> > [    1.290000] cpufreq_performance: setting to 1196000 kHz because of event 1
> > [    1.290000] cpufreq: __cpufreq_driver_target().1665 1
> >
> > This is my debug - .transition_ongoing is incremented ^^^^^^^^
> >
> > [    1.300000] cpufreq: target for CPU 0: 1196000 kHz, relation 1, requested 1196000 kHz
> 
> Quite straight forward actually..

Apparently, not quite.

> Please try attached patch and see if it fixes
> your problem.. Which it should if I am not wrong.. I will send it
> separately then..

It helps only once. The first switching works, the second one doesn't. 
Below debug

[   12.010000] cpufreq: setting new policy for CPU 0: 398667 - 1196000 kHz
[   12.010000] cpufreq: new min and max freqs are 398667 - 1196000 kHz
[   12.010000] cpufreq: governor switch
[   12.010000] cpufreq: __cpufreq_governor for CPU 0, event 2
[   12.010000] cpufreq: __cpufreq_governor for CPU 0, event 5
[   12.010000] ondemand governor failed, too long transition latency of HW, fallback to performance governor
[   12.020000] cpufreq: __cpufreq_governor for CPU 0, event 4
[   12.020000] cpufreq: __cpufreq_governor for CPU 0, event 1
[   12.020000] cpufreq_performance: setting to 1196000 kHz because of event 1
[   12.020000] cpufreq: target for CPU 0: 1196000 kHz, relation 1, requested 1196000 kHz
[   12.020000] cpufreq: governor: change or update limits
[   12.020000] cpufreq: __cpufreq_governor for CPU 0, event 3
[   12.020000] cpufreq_performance: setting to 1196000 kHz because of event 3
[   12.020000] cpufreq: target for CPU 0: 1196000 kHz, relation 1, requested 1196000 kHz
[   12.030000] cpufreq: setting new policy for CPU 0: 398667 - 1196000 kHz
[   12.030000] cpufreq: new min and max freqs are 398667 - 1196000 kHz
[   12.030000] cpufreq: governor switch
[   12.030000] cpufreq: __cpufreq_governor for CPU 0, event 2
[   12.030000] cpufreq: __cpufreq_governor for CPU 0, event 5
[   12.030000] ondemand governor failed, too long transition latency of HW, fallback to performance governor
[   12.040000] cpufreq: __cpufreq_governor for CPU 0, event 4
[   12.040000] cpufreq: __cpufreq_governor for CPU 0, event 1
[   12.040000] cpufreq_performance: setting to 1196000 kHz because of event 1
[   12.040000] cpufreq: target for CPU 0: 1196000 kHz, relation 1, requested 1196000 kHz
[   12.040000] cpufreq: governor: change or update limits
[   12.040000] cpufreq: __cpufreq_governor for CPU 0, event 3
[   12.040000] cpufreq_performance: setting to 1196000 kHz because of event 3
[   12.040000] cpufreq: target for CPU 0: 1196000 kHz, relation 1, requested 1196000 kHz

echo powersave > /sys/devices/system/cpu/cpu0/cpufreq/scaling_governor

[   66.490000] cpufreq: setting new policy for CPU 0: 398667 - 1196000 kHz
[   66.490000] cpufreq: new min and max freqs are 398667 - 1196000 kHz
[   66.490000] cpufreq: governor switch
[   66.490000] cpufreq: __cpufreq_governor for CPU 0, event 2
[   66.490000] cpufreq: __cpufreq_governor for CPU 0, event 5
[   66.490000] cpufreq: __cpufreq_governor for CPU 0, event 4
[   66.490000] cpufreq: __cpufreq_governor for CPU 0, event 1
[   66.490000] cpufreq: target for CPU 0: 398667 kHz, relation 0, requested 398667 kHz
[   66.490000] cpufreq: __cpufreq_driver_target().1677 1
[   66.500000] cpufreq: notification 0 of frequency transition to 398666 kHz
[   66.500000] cpufreq: __cpufreq_notify_transition().297 2
[   66.500000] cpufreq: notification 0 of frequency transition to 398666 kHz
[   66.500000] cpufreq: __cpufreq_notify_transition().297 3
[   66.510000] cpufreq: notification 1 of frequency transition to 398666 kHz
[   66.510000] cpufreq: __cpufreq_notify_transition().327 2
[   66.520000] cpufreq: FREQ: 398666 - CPU: 0
[   66.520000] cpufreq: notification 1 of frequency transition to 398666 kHz
[   66.520000] cpufreq: __cpufreq_notify_transition().327 1
[   66.520000] cpufreq: FREQ: 398666 - CPU: 1
[   66.520000] cpufreq: cpufreq_notify_transition().366 0
[   66.530000] cpufreq: governor: change or update limits
[   66.530000] cpufreq: __cpufreq_governor for CPU 0, event 3
[   66.530000] cpufreq: target for CPU 0: 398667 kHz, relation 0, requested 398667 kHz
[   66.530000] cpufreq: __cpufreq_driver_target().1677 1

echo performance > /sys/devices/system/cpu/cpu0/cpufreq/scaling_governor

[   72.470000] cpufreq: setting new policy for CPU 0: 398667 - 1196000 kHz
[   72.470000] cpufreq: new min and max freqs are 398667 - 1196000 kHz
[   72.470000] cpufreq: governor switch
[   72.470000] cpufreq: __cpufreq_governor for CPU 0, event 2
[   72.470000] cpufreq: __cpufreq_governor for CPU 0, event 5
[   72.470000] cpufreq: __cpufreq_governor for CPU 0, event 4
[   72.470000] cpufreq: __cpufreq_governor for CPU 0, event 1
[   72.470000] cpufreq_performance: setting to 1196000 kHz because of event 1
[   72.470000] cpufreq: target for CPU 0: 1196000 kHz, relation 1, requested 1196000 kHz
[   72.470000] cpufreq: governor: change or update limits
[   72.470000] cpufreq: __cpufreq_governor for CPU 0, event 3
[   72.470000] cpufreq_performance: setting to 1196000 kHz because of event 3
[   72.470000] cpufreq: target for CPU 0: 1196000 kHz, relation 1, requested 1196000 kHz

> Thanks for your time..

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: "cpufreq: fix serialization issues with freq change notifiers" breaks cpufreq too

[Guennadi Liakhovetski]

On Tue, 10 Sep 2013, Viresh Kumar wrote:

> On 10 September 2013 21:52, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> > Of course, isn't that what I've written above? reverted a commit and added
> > debug - in that order.
> 
> Ok, I misread it then :(
> 
> > Sure, I can... So, with the performance governor I get
> >
> > [    1.290000] cpufreq-cpu0 cpufreq-cpu0: Looking up cpu0-supply from device tree
> > [    1.290000] cpufreq: trying to register driver generic_cpu0
> > [    1.290000] cpufreq: adding CPU 0
> > [    1.290000] cpufreq: Adding link for CPU: 1
> > [    1.290000] cpufreq: setting new policy for CPU 0: 398667 - 1196000 kHz
> > [    1.290000] cpufreq: new min and max freqs are 398667 - 1196000 kHz
> > [    1.290000] cpufreq: governor switch
> > [    1.290000] cpufreq: __cpufreq_governor for CPU 0, event 4
> > [    1.290000] cpufreq: __cpufreq_governor for CPU 0, event 1
> > [    1.290000] cpufreq_performance: setting to 1196000 kHz because of event 1
> > [    1.290000] cpufreq: __cpufreq_driver_target().1665 1
> >
> > This is my debug - .transition_ongoing is incremented ^^^^^^^^
> >
> > [    1.300000] cpufreq: target for CPU 0: 1196000 kHz, relation 1, requested 1196000 kHz
> 
> Quite straight forward actually..

Apparently, not quite.

> Please try attached patch and see if it fixes
> your problem.. Which it should if I am not wrong.. I will send it
> separately then..

It helps only once. The first switching works, the second one doesn't. 
Below debug

[   12.010000] cpufreq: setting new policy for CPU 0: 398667 - 1196000 kHz
[   12.010000] cpufreq: new min and max freqs are 398667 - 1196000 kHz
[   12.010000] cpufreq: governor switch
[   12.010000] cpufreq: __cpufreq_governor for CPU 0, event 2
[   12.010000] cpufreq: __cpufreq_governor for CPU 0, event 5
[   12.010000] ondemand governor failed, too long transition latency of HW, fallback to performance governor
[   12.020000] cpufreq: __cpufreq_governor for CPU 0, event 4
[   12.020000] cpufreq: __cpufreq_governor for CPU 0, event 1
[   12.020000] cpufreq_performance: setting to 1196000 kHz because of event 1
[   12.020000] cpufreq: target for CPU 0: 1196000 kHz, relation 1, requested 1196000 kHz
[   12.020000] cpufreq: governor: change or update limits
[   12.020000] cpufreq: __cpufreq_governor for CPU 0, event 3
[   12.020000] cpufreq_performance: setting to 1196000 kHz because of event 3
[   12.020000] cpufreq: target for CPU 0: 1196000 kHz, relation 1, requested 1196000 kHz
[   12.030000] cpufreq: setting new policy for CPU 0: 398667 - 1196000 kHz
[   12.030000] cpufreq: new min and max freqs are 398667 - 1196000 kHz
[   12.030000] cpufreq: governor switch
[   12.030000] cpufreq: __cpufreq_governor for CPU 0, event 2
[   12.030000] cpufreq: __cpufreq_governor for CPU 0, event 5
[   12.030000] ondemand governor failed, too long transition latency of HW, fallback to performance governor
[   12.040000] cpufreq: __cpufreq_governor for CPU 0, event 4
[   12.040000] cpufreq: __cpufreq_governor for CPU 0, event 1
[   12.040000] cpufreq_performance: setting to 1196000 kHz because of event 1
[   12.040000] cpufreq: target for CPU 0: 1196000 kHz, relation 1, requested 1196000 kHz
[   12.040000] cpufreq: governor: change or update limits
[   12.040000] cpufreq: __cpufreq_governor for CPU 0, event 3
[   12.040000] cpufreq_performance: setting to 1196000 kHz because of event 3
[   12.040000] cpufreq: target for CPU 0: 1196000 kHz, relation 1, requested 1196000 kHz

echo powersave > /sys/devices/system/cpu/cpu0/cpufreq/scaling_governor

[   66.490000] cpufreq: setting new policy for CPU 0: 398667 - 1196000 kHz
[   66.490000] cpufreq: new min and max freqs are 398667 - 1196000 kHz
[   66.490000] cpufreq: governor switch
[   66.490000] cpufreq: __cpufreq_governor for CPU 0, event 2
[   66.490000] cpufreq: __cpufreq_governor for CPU 0, event 5
[   66.490000] cpufreq: __cpufreq_governor for CPU 0, event 4
[   66.490000] cpufreq: __cpufreq_governor for CPU 0, event 1
[   66.490000] cpufreq: target for CPU 0: 398667 kHz, relation 0, requested 398667 kHz
[   66.490000] cpufreq: __cpufreq_driver_target().1677 1
[   66.500000] cpufreq: notification 0 of frequency transition to 398666 kHz
[   66.500000] cpufreq: __cpufreq_notify_transition().297 2
[   66.500000] cpufreq: notification 0 of frequency transition to 398666 kHz
[   66.500000] cpufreq: __cpufreq_notify_transition().297 3
[   66.510000] cpufreq: notification 1 of frequency transition to 398666 kHz
[   66.510000] cpufreq: __cpufreq_notify_transition().327 2
[   66.520000] cpufreq: FREQ: 398666 - CPU: 0
[   66.520000] cpufreq: notification 1 of frequency transition to 398666 kHz
[   66.520000] cpufreq: __cpufreq_notify_transition().327 1
[   66.520000] cpufreq: FREQ: 398666 - CPU: 1
[   66.520000] cpufreq: cpufreq_notify_transition().366 0
[   66.530000] cpufreq: governor: change or update limits
[   66.530000] cpufreq: __cpufreq_governor for CPU 0, event 3
[   66.530000] cpufreq: target for CPU 0: 398667 kHz, relation 0, requested 398667 kHz
[   66.530000] cpufreq: __cpufreq_driver_target().1677 1

echo performance > /sys/devices/system/cpu/cpu0/cpufreq/scaling_governor

[   72.470000] cpufreq: setting new policy for CPU 0: 398667 - 1196000 kHz
[   72.470000] cpufreq: new min and max freqs are 398667 - 1196000 kHz
[   72.470000] cpufreq: governor switch
[   72.470000] cpufreq: __cpufreq_governor for CPU 0, event 2
[   72.470000] cpufreq: __cpufreq_governor for CPU 0, event 5
[   72.470000] cpufreq: __cpufreq_governor for CPU 0, event 4
[   72.470000] cpufreq: __cpufreq_governor for CPU 0, event 1
[   72.470000] cpufreq_performance: setting to 1196000 kHz because of event 1
[   72.470000] cpufreq: target for CPU 0: 1196000 kHz, relation 1, requested 1196000 kHz
[   72.470000] cpufreq: governor: change or update limits
[   72.470000] cpufreq: __cpufreq_governor for CPU 0, event 3
[   72.470000] cpufreq_performance: setting to 1196000 kHz because of event 3
[   72.470000] cpufreq: target for CPU 0: 1196000 kHz, relation 1, requested 1196000 kHz

> Thanks for your time..

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

"cpufreq: fix serialization issues with freq change notifiers" breaks cpufreq too

[Guennadi Liakhovetski]

On Tue, 10 Sep 2013, Viresh Kumar wrote:

> On 10 September 2013 21:52, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> > Of course, isn't that what I've written above? reverted a commit and added
> > debug - in that order.
> 
> Ok, I misread it then :(
> 
> > Sure, I can... So, with the performance governor I get
> >
> > [    1.290000] cpufreq-cpu0 cpufreq-cpu0: Looking up cpu0-supply from device tree
> > [    1.290000] cpufreq: trying to register driver generic_cpu0
> > [    1.290000] cpufreq: adding CPU 0
> > [    1.290000] cpufreq: Adding link for CPU: 1
> > [    1.290000] cpufreq: setting new policy for CPU 0: 398667 - 1196000 kHz
> > [    1.290000] cpufreq: new min and max freqs are 398667 - 1196000 kHz
> > [    1.290000] cpufreq: governor switch
> > [    1.290000] cpufreq: __cpufreq_governor for CPU 0, event 4
> > [    1.290000] cpufreq: __cpufreq_governor for CPU 0, event 1
> > [    1.290000] cpufreq_performance: setting to 1196000 kHz because of event 1
> > [    1.290000] cpufreq: __cpufreq_driver_target().1665 1
> >
> > This is my debug - .transition_ongoing is incremented ^^^^^^^^
> >
> > [    1.300000] cpufreq: target for CPU 0: 1196000 kHz, relation 1, requested 1196000 kHz
> 
> Quite straight forward actually..

Apparently, not quite.

> Please try attached patch and see if it fixes
> your problem.. Which it should if I am not wrong.. I will send it
> separately then..

It helps only once. The first switching works, the second one doesn't. 
Below debug

[   12.010000] cpufreq: setting new policy for CPU 0: 398667 - 1196000 kHz
[   12.010000] cpufreq: new min and max freqs are 398667 - 1196000 kHz
[   12.010000] cpufreq: governor switch
[   12.010000] cpufreq: __cpufreq_governor for CPU 0, event 2
[   12.010000] cpufreq: __cpufreq_governor for CPU 0, event 5
[   12.010000] ondemand governor failed, too long transition latency of HW, fallback to performance governor
[   12.020000] cpufreq: __cpufreq_governor for CPU 0, event 4
[   12.020000] cpufreq: __cpufreq_governor for CPU 0, event 1
[   12.020000] cpufreq_performance: setting to 1196000 kHz because of event 1
[   12.020000] cpufreq: target for CPU 0: 1196000 kHz, relation 1, requested 1196000 kHz
[   12.020000] cpufreq: governor: change or update limits
[   12.020000] cpufreq: __cpufreq_governor for CPU 0, event 3
[   12.020000] cpufreq_performance: setting to 1196000 kHz because of event 3
[   12.020000] cpufreq: target for CPU 0: 1196000 kHz, relation 1, requested 1196000 kHz
[   12.030000] cpufreq: setting new policy for CPU 0: 398667 - 1196000 kHz
[   12.030000] cpufreq: new min and max freqs are 398667 - 1196000 kHz
[   12.030000] cpufreq: governor switch
[   12.030000] cpufreq: __cpufreq_governor for CPU 0, event 2
[   12.030000] cpufreq: __cpufreq_governor for CPU 0, event 5
[   12.030000] ondemand governor failed, too long transition latency of HW, fallback to performance governor
[   12.040000] cpufreq: __cpufreq_governor for CPU 0, event 4
[   12.040000] cpufreq: __cpufreq_governor for CPU 0, event 1
[   12.040000] cpufreq_performance: setting to 1196000 kHz because of event 1
[   12.040000] cpufreq: target for CPU 0: 1196000 kHz, relation 1, requested 1196000 kHz
[   12.040000] cpufreq: governor: change or update limits
[   12.040000] cpufreq: __cpufreq_governor for CPU 0, event 3
[   12.040000] cpufreq_performance: setting to 1196000 kHz because of event 3
[   12.040000] cpufreq: target for CPU 0: 1196000 kHz, relation 1, requested 1196000 kHz

echo powersave > /sys/devices/system/cpu/cpu0/cpufreq/scaling_governor

[   66.490000] cpufreq: setting new policy for CPU 0: 398667 - 1196000 kHz
[   66.490000] cpufreq: new min and max freqs are 398667 - 1196000 kHz
[   66.490000] cpufreq: governor switch
[   66.490000] cpufreq: __cpufreq_governor for CPU 0, event 2
[   66.490000] cpufreq: __cpufreq_governor for CPU 0, event 5
[   66.490000] cpufreq: __cpufreq_governor for CPU 0, event 4
[   66.490000] cpufreq: __cpufreq_governor for CPU 0, event 1
[   66.490000] cpufreq: target for CPU 0: 398667 kHz, relation 0, requested 398667 kHz
[   66.490000] cpufreq: __cpufreq_driver_target().1677 1
[   66.500000] cpufreq: notification 0 of frequency transition to 398666 kHz
[   66.500000] cpufreq: __cpufreq_notify_transition().297 2
[   66.500000] cpufreq: notification 0 of frequency transition to 398666 kHz
[   66.500000] cpufreq: __cpufreq_notify_transition().297 3
[   66.510000] cpufreq: notification 1 of frequency transition to 398666 kHz
[   66.510000] cpufreq: __cpufreq_notify_transition().327 2
[   66.520000] cpufreq: FREQ: 398666 - CPU: 0
[   66.520000] cpufreq: notification 1 of frequency transition to 398666 kHz
[   66.520000] cpufreq: __cpufreq_notify_transition().327 1
[   66.520000] cpufreq: FREQ: 398666 - CPU: 1
[   66.520000] cpufreq: cpufreq_notify_transition().366 0
[   66.530000] cpufreq: governor: change or update limits
[   66.530000] cpufreq: __cpufreq_governor for CPU 0, event 3
[   66.530000] cpufreq: target for CPU 0: 398667 kHz, relation 0, requested 398667 kHz
[   66.530000] cpufreq: __cpufreq_driver_target().1677 1

echo performance > /sys/devices/system/cpu/cpu0/cpufreq/scaling_governor

[   72.470000] cpufreq: setting new policy for CPU 0: 398667 - 1196000 kHz
[   72.470000] cpufreq: new min and max freqs are 398667 - 1196000 kHz
[   72.470000] cpufreq: governor switch
[   72.470000] cpufreq: __cpufreq_governor for CPU 0, event 2
[   72.470000] cpufreq: __cpufreq_governor for CPU 0, event 5
[   72.470000] cpufreq: __cpufreq_governor for CPU 0, event 4
[   72.470000] cpufreq: __cpufreq_governor for CPU 0, event 1
[   72.470000] cpufreq_performance: setting to 1196000 kHz because of event 1
[   72.470000] cpufreq: target for CPU 0: 1196000 kHz, relation 1, requested 1196000 kHz
[   72.470000] cpufreq: governor: change or update limits
[   72.470000] cpufreq: __cpufreq_governor for CPU 0, event 3
[   72.470000] cpufreq_performance: setting to 1196000 kHz because of event 3
[   72.470000] cpufreq: target for CPU 0: 1196000 kHz, relation 1, requested 1196000 kHz

> Thanks for your time..

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: "cpufreq: fix serialization issues with freq change notifiers" breaks cpufreq too

[Rafael J. Wysocki]

On Tuesday, September 10, 2013 08:44:18 PM Viresh Kumar wrote:
> On 10 September 2013 17:19, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > That said I'm actually unsure what problems *exactly* are fixed by commit
> > 7c30ed5.  The commit log only says that PRECHANGE or POSTCHAGE shouldn't be
> > called twice in a row, but it doesn't say why.  As the fallout indicates,
> > that actually happened before commit 7c30ed5 and nothing visibly broke, so
> > the benefit from having that commit is questionable to me.  On the other hand,
> > the commit itself is evidently broken, so what exactly is the reason for
> > keeping it?
> 
> Okay, so the first question is can we have multiple PRECHANGE notfication
> done without any POSTCHANGE inbetween?
> - Scenario: One reading value of cpuinfo_cur_freq, which will call
> __cpufreq_cpu_get()->cpufreq_out_of_sync()->cpufreq_notify_transition()..
> 
> And ondemand governor trying to change freq of cpu and so doing notification..
> 
> There can be more similar scenarios possible..

OK, and what exactly does break here?  What's the exact race you're concerned
about?

What field is corrupted in which object, for example, or what incorrect
behavior of the hardware results from this?

> Now Second question, is this fine to have multiple PRECHANGE notfications
> before any POSTCHANGE notification?
> 
> Logically it looks obvious to me that these must be serialized..
> Otherwise this is what we are broadcasting:
> - We are changing for freq X, please prepare and let us know if you are
> okay with it..
> - Oh.. So sorry, we are changing to freq Y instead, please adjust accordingly.
> 
> - Yes we have changed to freq Y...
> - Yes we have changed to freq X...
> 
> This looks stupid, isn't it? I don't know how the drivers would behave when
> they see such notifications and so got to this patch initially..

Well, precisely, you don't know.

Can you please look for specific problems and address those instead of trying
to address potential issues that may or may not happen and may or may not really
be issues even if they actually happen?

And broken patches that try to fix such things definitely don't help.

For now I'm reverting the commit in question and everything on top of it.

Thanks,
Rafael

Re: "cpufreq: fix serialization issues with freq change notifiers" breaks cpufreq too

[Rafael J. Wysocki]

On Tuesday, September 10, 2013 08:44:18 PM Viresh Kumar wrote:
> On 10 September 2013 17:19, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > That said I'm actually unsure what problems *exactly* are fixed by commit
> > 7c30ed5.  The commit log only says that PRECHANGE or POSTCHAGE shouldn't be
> > called twice in a row, but it doesn't say why.  As the fallout indicates,
> > that actually happened before commit 7c30ed5 and nothing visibly broke, so
> > the benefit from having that commit is questionable to me.  On the other hand,
> > the commit itself is evidently broken, so what exactly is the reason for
> > keeping it?
> 
> Okay, so the first question is can we have multiple PRECHANGE notfication
> done without any POSTCHANGE inbetween?
> - Scenario: One reading value of cpuinfo_cur_freq, which will call
> __cpufreq_cpu_get()->cpufreq_out_of_sync()->cpufreq_notify_transition()..
> 
> And ondemand governor trying to change freq of cpu and so doing notification..
> 
> There can be more similar scenarios possible..

OK, and what exactly does break here?  What's the exact race you're concerned
about?

What field is corrupted in which object, for example, or what incorrect
behavior of the hardware results from this?

> Now Second question, is this fine to have multiple PRECHANGE notfications
> before any POSTCHANGE notification?
> 
> Logically it looks obvious to me that these must be serialized..
> Otherwise this is what we are broadcasting:
> - We are changing for freq X, please prepare and let us know if you are
> okay with it..
> - Oh.. So sorry, we are changing to freq Y instead, please adjust accordingly.
> 
> - Yes we have changed to freq Y...
> - Yes we have changed to freq X...
> 
> This looks stupid, isn't it? I don't know how the drivers would behave when
> they see such notifications and so got to this patch initially..

Well, precisely, you don't know.

Can you please look for specific problems and address those instead of trying
to address potential issues that may or may not happen and may or may not really
be issues even if they actually happen?

And broken patches that try to fix such things definitely don't help.

For now I'm reverting the commit in question and everything on top of it.

Thanks,
Rafael

Re: "cpufreq: fix serialization issues with freq change notifiers" breaks cpufreq too

[Rafael J. Wysocki]

On Tuesday, September 10, 2013 08:44:18 PM Viresh Kumar wrote:
> On 10 September 2013 17:19, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > That said I'm actually unsure what problems *exactly* are fixed by commit
> > 7c30ed5.  The commit log only says that PRECHANGE or POSTCHAGE shouldn't be
> > called twice in a row, but it doesn't say why.  As the fallout indicates,
> > that actually happened before commit 7c30ed5 and nothing visibly broke, so
> > the benefit from having that commit is questionable to me.  On the other hand,
> > the commit itself is evidently broken, so what exactly is the reason for
> > keeping it?
> 
> Okay, so the first question is can we have multiple PRECHANGE notfication
> done without any POSTCHANGE inbetween?
> - Scenario: One reading value of cpuinfo_cur_freq, which will call
> __cpufreq_cpu_get()->cpufreq_out_of_sync()->cpufreq_notify_transition()..
> 
> And ondemand governor trying to change freq of cpu and so doing notification..
> 
> There can be more similar scenarios possible..

OK, and what exactly does break here?  What's the exact race you're concerned
about?

What field is corrupted in which object, for example, or what incorrect
behavior of the hardware results from this?

> Now Second question, is this fine to have multiple PRECHANGE notfications
> before any POSTCHANGE notification?
> 
> Logically it looks obvious to me that these must be serialized..
> Otherwise this is what we are broadcasting:
> - We are changing for freq X, please prepare and let us know if you are
> okay with it..
> - Oh.. So sorry, we are changing to freq Y instead, please adjust accordingly.
> 
> - Yes we have changed to freq Y...
> - Yes we have changed to freq X...
> 
> This looks stupid, isn't it? I don't know how the drivers would behave when
> they see such notifications and so got to this patch initially..

Well, precisely, you don't know.

Can you please look for specific problems and address those instead of trying
to address potential issues that may or may not happen and may or may not really
be issues even if they actually happen?

And broken patches that try to fix such things definitely don't help.

For now I'm reverting the commit in question and everything on top of it.

Thanks,
Rafael

"cpufreq: fix serialization issues with freq change notifiers" breaks cpufreq too

[Rafael J. Wysocki]

On Tuesday, September 10, 2013 08:44:18 PM Viresh Kumar wrote:
> On 10 September 2013 17:19, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > That said I'm actually unsure what problems *exactly* are fixed by commit
> > 7c30ed5.  The commit log only says that PRECHANGE or POSTCHAGE shouldn't be
> > called twice in a row, but it doesn't say why.  As the fallout indicates,
> > that actually happened before commit 7c30ed5 and nothing visibly broke, so
> > the benefit from having that commit is questionable to me.  On the other hand,
> > the commit itself is evidently broken, so what exactly is the reason for
> > keeping it?
> 
> Okay, so the first question is can we have multiple PRECHANGE notfication
> done without any POSTCHANGE inbetween?
> - Scenario: One reading value of cpuinfo_cur_freq, which will call
> __cpufreq_cpu_get()->cpufreq_out_of_sync()->cpufreq_notify_transition()..
> 
> And ondemand governor trying to change freq of cpu and so doing notification..
> 
> There can be more similar scenarios possible..

OK, and what exactly does break here?  What's the exact race you're concerned
about?

What field is corrupted in which object, for example, or what incorrect
behavior of the hardware results from this?

> Now Second question, is this fine to have multiple PRECHANGE notfications
> before any POSTCHANGE notification?
> 
> Logically it looks obvious to me that these must be serialized..
> Otherwise this is what we are broadcasting:
> - We are changing for freq X, please prepare and let us know if you are
> okay with it..
> - Oh.. So sorry, we are changing to freq Y instead, please adjust accordingly.
> 
> - Yes we have changed to freq Y...
> - Yes we have changed to freq X...
> 
> This looks stupid, isn't it? I don't know how the drivers would behave when
> they see such notifications and so got to this patch initially..

Well, precisely, you don't know.

Can you please look for specific problems and address those instead of trying
to address potential issues that may or may not happen and may or may not really
be issues even if they actually happen?

And broken patches that try to fix such things definitely don't help.

For now I'm reverting the commit in question and everything on top of it.

Thanks,
Rafael

Re: "cpufreq: fix serialization issues with freq change notifiers" breaks cpufreq too

[Viresh Kumar]

[-- Attachment #1: Type: text/plain, Size: 406 bytes --]

On 10 September 2013 22:37, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> On Tue, 10 Sep 2013, Viresh Kumar wrote:
>> Quite straight forward actually..
>
> Apparently, not quite.

I overlooked the situation where we return early from ->target() routines.. :(

Please try attached patches, I will repost them later (once I am able to
convince Rafael that these are really important :) )

--
viresh

[-- Attachment #2: 0001-cpufreq-distinguish-drivers-that-do-asynchronous-not.patch --]
[-- Type: application/octet-stream, Size: 2313 bytes --]

From a153752719774763f9edd8f46291f1bab722f7d6 Mon Sep 17 00:00:00 2001
Message-Id: <a153752719774763f9edd8f46291f1bab722f7d6.1378885668.git.viresh.kumar@linaro.org>
From: Viresh Kumar <viresh.kumar@linaro.org>
Date: Wed, 14 Aug 2013 19:38:24 +0530
Subject: [PATCH 1/2] cpufreq: distinguish drivers that do asynchronous
 notifications

There are few special cases like exynos5440 which doesn't send POSTCHANGE
notification from their ->target() routine and call some kind of bottom halves
for doing this work, work/tasklet/etc.. From which they finally send POSTCHANGE
notification.

Its better if we distinguish them from other cpufreq drivers in some way so that
core can handle them specially. So this patch introduces another flag:
CPUFREQ_ASYNC_NOTIFICATION, which will be set by such drivers.

This also changes exynos5440-cpufreq.c in order to set this flag.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/exynos5440-cpufreq.c | 2 +-
 include/linux/cpufreq.h              | 6 ++++++
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/cpufreq/exynos5440-cpufreq.c b/drivers/cpufreq/exynos5440-cpufreq.c
index d514c15..f44664a 100644
--- a/drivers/cpufreq/exynos5440-cpufreq.c
+++ b/drivers/cpufreq/exynos5440-cpufreq.c
@@ -342,7 +342,7 @@ static int exynos_cpufreq_cpu_init(struct cpufreq_policy *policy)
 }
 
 static struct cpufreq_driver exynos_driver = {
-	.flags		= CPUFREQ_STICKY,
+	.flags		= CPUFREQ_STICKY | CPUFREQ_ASYNC_NOTIFICATION,
 	.verify		= exynos_verify_speed,
 	.target		= exynos_target,
 	.get		= exynos_getspeed,
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index d568f39..c770bc0 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -219,6 +219,12 @@ struct cpufreq_driver {
 					 * frequency transitions */
 #define CPUFREQ_PM_NO_WARN	0x04	/* don't warn on suspend/resume speed
 					 * mismatches */
+/*
+ * Driver will do POSTCHANGE notifications from outside of their ->target()
+ * routine and so must set cpufreq_driver->flags with this flag, so that core
+ * can handle them specially.
+ */
+#define CPUFREQ_ASYNC_NOTIFICATION 0x08
 
 int cpufreq_register_driver(struct cpufreq_driver *driver_data);
 int cpufreq_unregister_driver(struct cpufreq_driver *driver_data);
-- 
1.7.12.rc2.18.g61b472e


[-- Attachment #3: 0002-cpufreq-fix-notification-serialization-issues.patch --]
[-- Type: application/octet-stream, Size: 4388 bytes --]

From 42851e3c2fc4a8e704683c309c03d5af42a8f398 Mon Sep 17 00:00:00 2001
Message-Id: <42851e3c2fc4a8e704683c309c03d5af42a8f398.1378885668.git.viresh.kumar@linaro.org>
In-Reply-To: <a153752719774763f9edd8f46291f1bab722f7d6.1378885668.git.viresh.kumar@linaro.org>
References: <a153752719774763f9edd8f46291f1bab722f7d6.1378885668.git.viresh.kumar@linaro.org>
From: Viresh Kumar <viresh.kumar@linaro.org>
Date: Wed, 11 Sep 2013 11:42:18 +0530
Subject: [PATCH 2/2] cpufreq: fix notification serialization issues

Sometimes ->target() returns earlier than expected, even without doing any
PRECHANGE notifications, in such cases we aren't decrementing our
transition_ongoing counter and that makes it go crazy for those cases.

This patch fixes this issue by taking care of transition_ongoing count at
several places. Now we simply decrement refcount at the end of
cpufreq_out_of_sync() & __cpufreq_driver_target() for drivers that don't do
ASYNC notifications, for others we do that from POSTCHANGE notifier.

These drivers also return -EINPROGRESS from their target() routines to mark that
transtion will be completed later.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq.c            | 42 ++++++++++++++++++++++++++++--------
 drivers/cpufreq/exynos5440-cpufreq.c |  3 +++
 2 files changed, 36 insertions(+), 9 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 9cc5609..9644150 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -348,7 +348,8 @@ void cpufreq_notify_transition(struct cpufreq_policy *policy,
 	for_each_cpu(freqs->cpu, policy->cpus)
 		__cpufreq_notify_transition(policy, freqs, state);
 
-	if (state == CPUFREQ_POSTCHANGE) {
+	if ((cpufreq_driver->flags & CPUFREQ_ASYNC_NOTIFICATION)
+			&& (state == CPUFREQ_POSTCHANGE)) {
 		unsigned long flags;
 
 		/*
@@ -1406,6 +1407,17 @@ static void cpufreq_out_of_sync(unsigned int cpu, unsigned int old_freq,
 
 	cpufreq_notify_transition(policy, &freqs, CPUFREQ_PRECHANGE);
 	cpufreq_notify_transition(policy, &freqs, CPUFREQ_POSTCHANGE);
+
+	/*
+	 * For drivers with CPUFREQ_ASYNC_NOTIFICATION flag set, we decrement
+	 * transition_ongoing from POSTCHANGE notifiers.
+	 */
+	if (cpufreq_driver->flags & CPUFREQ_ASYNC_NOTIFICATION)
+		return;
+
+	write_lock_irqsave(&cpufreq_driver_lock, flags);
+	policy->transition_ongoing--;
+	write_unlock_irqrestore(&cpufreq_driver_lock, flags);
 }
 
 /**
@@ -1696,14 +1708,6 @@ int __cpufreq_driver_target(struct cpufreq_policy *policy,
 	if (cpufreq_disabled())
 		return -ENODEV;
 
-	write_lock_irqsave(&cpufreq_driver_lock, flags);
-	if (policy->transition_ongoing) {
-		write_unlock_irqrestore(&cpufreq_driver_lock, flags);
-		return -EBUSY;
-	}
-	policy->transition_ongoing++;
-	write_unlock_irqrestore(&cpufreq_driver_lock, flags);
-
 	/* Make sure that target_freq is within supported range */
 	if (target_freq > policy->max)
 		target_freq = policy->max;
@@ -1716,9 +1720,29 @@ int __cpufreq_driver_target(struct cpufreq_policy *policy,
 	if (target_freq == policy->cur)
 		return 0;
 
+	write_lock_irqsave(&cpufreq_driver_lock, flags);
+	if (policy->transition_ongoing) {
+		write_unlock_irqrestore(&cpufreq_driver_lock, flags);
+		return -EBUSY;
+	}
+	policy->transition_ongoing++;
+	write_unlock_irqrestore(&cpufreq_driver_lock, flags);
+
 	if (cpufreq_driver->target)
 		retval = cpufreq_driver->target(policy, target_freq, relation);
 
+	/*
+	 * For drivers with CPUFREQ_ASYNC_NOTIFICATION flag set, we decrement
+	 * transition_ongoing from POSTCHANGE notifiers.
+	 */
+	if ((cpufreq_driver->flags & CPUFREQ_ASYNC_NOTIFICATION)
+			&& (retval == -EINPROGRESS))
+		return retval;
+
+	write_lock_irqsave(&cpufreq_driver_lock, flags);
+	policy->transition_ongoing--;
+	write_unlock_irqrestore(&cpufreq_driver_lock, flags);
+
 	return retval;
 }
 EXPORT_SYMBOL_GPL(__cpufreq_driver_target);
diff --git a/drivers/cpufreq/exynos5440-cpufreq.c b/drivers/cpufreq/exynos5440-cpufreq.c
index f44664a..1e391ac 100644
--- a/drivers/cpufreq/exynos5440-cpufreq.c
+++ b/drivers/cpufreq/exynos5440-cpufreq.c
@@ -251,6 +251,9 @@ static int exynos_target(struct cpufreq_policy *policy,
 
 		__raw_writel(tmp, dvfs_info->base + XMU_C0_3_PSTATE + i * 4);
 	}
+
+	/* Mark transition as In-progress */
+	ret = -EINPROGRESS;
 out:
 	mutex_unlock(&cpufreq_lock);
 	return ret;
-- 
1.7.12.rc2.18.g61b472e

Re: "cpufreq: fix serialization issues with freq change notifiers" breaks cpufreq too

[Viresh Kumar]

[-- Attachment #1: Type: text/plain, Size: 406 bytes --]

On 10 September 2013 22:37, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> On Tue, 10 Sep 2013, Viresh Kumar wrote:
>> Quite straight forward actually..
>
> Apparently, not quite.

I overlooked the situation where we return early from ->target() routines.. :(

Please try attached patches, I will repost them later (once I am able to
convince Rafael that these are really important :) )

--
viresh

[-- Attachment #2: 0001-cpufreq-distinguish-drivers-that-do-asynchronous-not.patch --]
[-- Type: application/octet-stream, Size: 2313 bytes --]

From a153752719774763f9edd8f46291f1bab722f7d6 Mon Sep 17 00:00:00 2001
Message-Id: <a153752719774763f9edd8f46291f1bab722f7d6.1378885668.git.viresh.kumar@linaro.org>
From: Viresh Kumar <viresh.kumar@linaro.org>
Date: Wed, 14 Aug 2013 19:38:24 +0530
Subject: [PATCH 1/2] cpufreq: distinguish drivers that do asynchronous
 notifications

There are few special cases like exynos5440 which doesn't send POSTCHANGE
notification from their ->target() routine and call some kind of bottom halves
for doing this work, work/tasklet/etc.. From which they finally send POSTCHANGE
notification.

Its better if we distinguish them from other cpufreq drivers in some way so that
core can handle them specially. So this patch introduces another flag:
CPUFREQ_ASYNC_NOTIFICATION, which will be set by such drivers.

This also changes exynos5440-cpufreq.c in order to set this flag.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/exynos5440-cpufreq.c | 2 +-
 include/linux/cpufreq.h              | 6 ++++++
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/cpufreq/exynos5440-cpufreq.c b/drivers/cpufreq/exynos5440-cpufreq.c
index d514c15..f44664a 100644
--- a/drivers/cpufreq/exynos5440-cpufreq.c
+++ b/drivers/cpufreq/exynos5440-cpufreq.c
@@ -342,7 +342,7 @@ static int exynos_cpufreq_cpu_init(struct cpufreq_policy *policy)
 }
 
 static struct cpufreq_driver exynos_driver = {
-	.flags		= CPUFREQ_STICKY,
+	.flags		= CPUFREQ_STICKY | CPUFREQ_ASYNC_NOTIFICATION,
 	.verify		= exynos_verify_speed,
 	.target		= exynos_target,
 	.get		= exynos_getspeed,
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index d568f39..c770bc0 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -219,6 +219,12 @@ struct cpufreq_driver {
 					 * frequency transitions */
 #define CPUFREQ_PM_NO_WARN	0x04	/* don't warn on suspend/resume speed
 					 * mismatches */
+/*
+ * Driver will do POSTCHANGE notifications from outside of their ->target()
+ * routine and so must set cpufreq_driver->flags with this flag, so that core
+ * can handle them specially.
+ */
+#define CPUFREQ_ASYNC_NOTIFICATION 0x08
 
 int cpufreq_register_driver(struct cpufreq_driver *driver_data);
 int cpufreq_unregister_driver(struct cpufreq_driver *driver_data);
-- 
1.7.12.rc2.18.g61b472e


[-- Attachment #3: 0002-cpufreq-fix-notification-serialization-issues.patch --]
[-- Type: application/octet-stream, Size: 4388 bytes --]

From 42851e3c2fc4a8e704683c309c03d5af42a8f398 Mon Sep 17 00:00:00 2001
Message-Id: <42851e3c2fc4a8e704683c309c03d5af42a8f398.1378885668.git.viresh.kumar@linaro.org>
In-Reply-To: <a153752719774763f9edd8f46291f1bab722f7d6.1378885668.git.viresh.kumar@linaro.org>
References: <a153752719774763f9edd8f46291f1bab722f7d6.1378885668.git.viresh.kumar@linaro.org>
From: Viresh Kumar <viresh.kumar@linaro.org>
Date: Wed, 11 Sep 2013 11:42:18 +0530
Subject: [PATCH 2/2] cpufreq: fix notification serialization issues

Sometimes ->target() returns earlier than expected, even without doing any
PRECHANGE notifications, in such cases we aren't decrementing our
transition_ongoing counter and that makes it go crazy for those cases.

This patch fixes this issue by taking care of transition_ongoing count at
several places. Now we simply decrement refcount at the end of
cpufreq_out_of_sync() & __cpufreq_driver_target() for drivers that don't do
ASYNC notifications, for others we do that from POSTCHANGE notifier.

These drivers also return -EINPROGRESS from their target() routines to mark that
transtion will be completed later.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq.c            | 42 ++++++++++++++++++++++++++++--------
 drivers/cpufreq/exynos5440-cpufreq.c |  3 +++
 2 files changed, 36 insertions(+), 9 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 9cc5609..9644150 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -348,7 +348,8 @@ void cpufreq_notify_transition(struct cpufreq_policy *policy,
 	for_each_cpu(freqs->cpu, policy->cpus)
 		__cpufreq_notify_transition(policy, freqs, state);
 
-	if (state == CPUFREQ_POSTCHANGE) {
+	if ((cpufreq_driver->flags & CPUFREQ_ASYNC_NOTIFICATION)
+			&& (state == CPUFREQ_POSTCHANGE)) {
 		unsigned long flags;
 
 		/*
@@ -1406,6 +1407,17 @@ static void cpufreq_out_of_sync(unsigned int cpu, unsigned int old_freq,
 
 	cpufreq_notify_transition(policy, &freqs, CPUFREQ_PRECHANGE);
 	cpufreq_notify_transition(policy, &freqs, CPUFREQ_POSTCHANGE);
+
+	/*
+	 * For drivers with CPUFREQ_ASYNC_NOTIFICATION flag set, we decrement
+	 * transition_ongoing from POSTCHANGE notifiers.
+	 */
+	if (cpufreq_driver->flags & CPUFREQ_ASYNC_NOTIFICATION)
+		return;
+
+	write_lock_irqsave(&cpufreq_driver_lock, flags);
+	policy->transition_ongoing--;
+	write_unlock_irqrestore(&cpufreq_driver_lock, flags);
 }
 
 /**
@@ -1696,14 +1708,6 @@ int __cpufreq_driver_target(struct cpufreq_policy *policy,
 	if (cpufreq_disabled())
 		return -ENODEV;
 
-	write_lock_irqsave(&cpufreq_driver_lock, flags);
-	if (policy->transition_ongoing) {
-		write_unlock_irqrestore(&cpufreq_driver_lock, flags);
-		return -EBUSY;
-	}
-	policy->transition_ongoing++;
-	write_unlock_irqrestore(&cpufreq_driver_lock, flags);
-
 	/* Make sure that target_freq is within supported range */
 	if (target_freq > policy->max)
 		target_freq = policy->max;
@@ -1716,9 +1720,29 @@ int __cpufreq_driver_target(struct cpufreq_policy *policy,
 	if (target_freq == policy->cur)
 		return 0;
 
+	write_lock_irqsave(&cpufreq_driver_lock, flags);
+	if (policy->transition_ongoing) {
+		write_unlock_irqrestore(&cpufreq_driver_lock, flags);
+		return -EBUSY;
+	}
+	policy->transition_ongoing++;
+	write_unlock_irqrestore(&cpufreq_driver_lock, flags);
+
 	if (cpufreq_driver->target)
 		retval = cpufreq_driver->target(policy, target_freq, relation);
 
+	/*
+	 * For drivers with CPUFREQ_ASYNC_NOTIFICATION flag set, we decrement
+	 * transition_ongoing from POSTCHANGE notifiers.
+	 */
+	if ((cpufreq_driver->flags & CPUFREQ_ASYNC_NOTIFICATION)
+			&& (retval == -EINPROGRESS))
+		return retval;
+
+	write_lock_irqsave(&cpufreq_driver_lock, flags);
+	policy->transition_ongoing--;
+	write_unlock_irqrestore(&cpufreq_driver_lock, flags);
+
 	return retval;
 }
 EXPORT_SYMBOL_GPL(__cpufreq_driver_target);
diff --git a/drivers/cpufreq/exynos5440-cpufreq.c b/drivers/cpufreq/exynos5440-cpufreq.c
index f44664a..1e391ac 100644
--- a/drivers/cpufreq/exynos5440-cpufreq.c
+++ b/drivers/cpufreq/exynos5440-cpufreq.c
@@ -251,6 +251,9 @@ static int exynos_target(struct cpufreq_policy *policy,
 
 		__raw_writel(tmp, dvfs_info->base + XMU_C0_3_PSTATE + i * 4);
 	}
+
+	/* Mark transition as In-progress */
+	ret = -EINPROGRESS;
 out:
 	mutex_unlock(&cpufreq_lock);
 	return ret;
-- 
1.7.12.rc2.18.g61b472e

"cpufreq: fix serialization issues with freq change notifiers" breaks cpufreq too

[Viresh Kumar]

On 10 September 2013 22:37, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> On Tue, 10 Sep 2013, Viresh Kumar wrote:
>> Quite straight forward actually..
>
> Apparently, not quite.

I overlooked the situation where we return early from ->target() routines.. :(

Please try attached patches, I will repost them later (once I am able to
convince Rafael that these are really important :) )

--
viresh
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-cpufreq-distinguish-drivers-that-do-asynchronous-not.patch
Type: application/octet-stream
Size: 2313 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130911/89e6231f/attachment.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-cpufreq-fix-notification-serialization-issues.patch
Type: application/octet-stream
Size: 4388 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130911/89e6231f/attachment-0001.obj>

Re: "cpufreq: fix serialization issues with freq change notifiers" breaks cpufreq too

[Guennadi Liakhovetski]

On Wed, 11 Sep 2013, Viresh Kumar wrote:

> On 10 September 2013 22:37, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> > On Tue, 10 Sep 2013, Viresh Kumar wrote:
> >> Quite straight forward actually..
> >
> > Apparently, not quite.
> 
> I overlooked the situation where we return early from ->target() routines.. :(
> 
> Please try attached patches, I will repost them later (once I am able to
> convince Rafael that these are really important :) )

I'd rather wait until Rafael is convinced, then we'll see.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: "cpufreq: fix serialization issues with freq change notifiers" breaks cpufreq too

[Guennadi Liakhovetski]

On Wed, 11 Sep 2013, Viresh Kumar wrote:

> On 10 September 2013 22:37, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> > On Tue, 10 Sep 2013, Viresh Kumar wrote:
> >> Quite straight forward actually..
> >
> > Apparently, not quite.
> 
> I overlooked the situation where we return early from ->target() routines.. :(
> 
> Please try attached patches, I will repost them later (once I am able to
> convince Rafael that these are really important :) )

I'd rather wait until Rafael is convinced, then we'll see.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: "cpufreq: fix serialization issues with freq change notifiers" breaks cpufreq too

[Guennadi Liakhovetski]

On Wed, 11 Sep 2013, Viresh Kumar wrote:

> On 10 September 2013 22:37, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> > On Tue, 10 Sep 2013, Viresh Kumar wrote:
> >> Quite straight forward actually..
> >
> > Apparently, not quite.
> 
> I overlooked the situation where we return early from ->target() routines.. :(
> 
> Please try attached patches, I will repost them later (once I am able to
> convince Rafael that these are really important :) )

I'd rather wait until Rafael is convinced, then we'll see.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

"cpufreq: fix serialization issues with freq change notifiers" breaks cpufreq too

[Guennadi Liakhovetski]

On Wed, 11 Sep 2013, Viresh Kumar wrote:

> On 10 September 2013 22:37, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> > On Tue, 10 Sep 2013, Viresh Kumar wrote:
> >> Quite straight forward actually..
> >
> > Apparently, not quite.
> 
> I overlooked the situation where we return early from ->target() routines.. :(
> 
> Please try attached patches, I will repost them later (once I am able to
> convince Rafael that these are really important :) )

I'd rather wait until Rafael is convinced, then we'll see.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: "cpufreq: fix serialization issues with freq change notifiers" breaks cpufreq too

[Viresh Kumar]

[-- Attachment #1: Type: text/plain, Size: 406 bytes --]

On 10 September 2013 22:37, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> On Tue, 10 Sep 2013, Viresh Kumar wrote:
>> Quite straight forward actually..
>
> Apparently, not quite.

I overlooked the situation where we return early from ->target() routines.. :(

Please try attached patches, I will repost them later (once I am able to
convince Rafael that these are really important :) )

--
viresh

[-- Attachment #2: 0001-cpufreq-distinguish-drivers-that-do-asynchronous-not.patch --]
[-- Type: application/octet-stream, Size: 2313 bytes --]

From a153752719774763f9edd8f46291f1bab722f7d6 Mon Sep 17 00:00:00 2001
Message-Id: <a153752719774763f9edd8f46291f1bab722f7d6.1378885668.git.viresh.kumar@linaro.org>
From: Viresh Kumar <viresh.kumar@linaro.org>
Date: Wed, 14 Aug 2013 19:38:24 +0530
Subject: [PATCH 1/2] cpufreq: distinguish drivers that do asynchronous
 notifications

There are few special cases like exynos5440 which doesn't send POSTCHANGE
notification from their ->target() routine and call some kind of bottom halves
for doing this work, work/tasklet/etc.. From which they finally send POSTCHANGE
notification.

Its better if we distinguish them from other cpufreq drivers in some way so that
core can handle them specially. So this patch introduces another flag:
CPUFREQ_ASYNC_NOTIFICATION, which will be set by such drivers.

This also changes exynos5440-cpufreq.c in order to set this flag.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/exynos5440-cpufreq.c | 2 +-
 include/linux/cpufreq.h              | 6 ++++++
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/cpufreq/exynos5440-cpufreq.c b/drivers/cpufreq/exynos5440-cpufreq.c
index d514c15..f44664a 100644
--- a/drivers/cpufreq/exynos5440-cpufreq.c
+++ b/drivers/cpufreq/exynos5440-cpufreq.c
@@ -342,7 +342,7 @@ static int exynos_cpufreq_cpu_init(struct cpufreq_policy *policy)
 }
 
 static struct cpufreq_driver exynos_driver = {
-	.flags		= CPUFREQ_STICKY,
+	.flags		= CPUFREQ_STICKY | CPUFREQ_ASYNC_NOTIFICATION,
 	.verify		= exynos_verify_speed,
 	.target		= exynos_target,
 	.get		= exynos_getspeed,
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index d568f39..c770bc0 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -219,6 +219,12 @@ struct cpufreq_driver {
 					 * frequency transitions */
 #define CPUFREQ_PM_NO_WARN	0x04	/* don't warn on suspend/resume speed
 					 * mismatches */
+/*
+ * Driver will do POSTCHANGE notifications from outside of their ->target()
+ * routine and so must set cpufreq_driver->flags with this flag, so that core
+ * can handle them specially.
+ */
+#define CPUFREQ_ASYNC_NOTIFICATION 0x08
 
 int cpufreq_register_driver(struct cpufreq_driver *driver_data);
 int cpufreq_unregister_driver(struct cpufreq_driver *driver_data);
-- 
1.7.12.rc2.18.g61b472e


[-- Attachment #3: 0002-cpufreq-fix-notification-serialization-issues.patch --]
[-- Type: application/octet-stream, Size: 4388 bytes --]

From 42851e3c2fc4a8e704683c309c03d5af42a8f398 Mon Sep 17 00:00:00 2001
Message-Id: <42851e3c2fc4a8e704683c309c03d5af42a8f398.1378885668.git.viresh.kumar@linaro.org>
In-Reply-To: <a153752719774763f9edd8f46291f1bab722f7d6.1378885668.git.viresh.kumar@linaro.org>
References: <a153752719774763f9edd8f46291f1bab722f7d6.1378885668.git.viresh.kumar@linaro.org>
From: Viresh Kumar <viresh.kumar@linaro.org>
Date: Wed, 11 Sep 2013 11:42:18 +0530
Subject: [PATCH 2/2] cpufreq: fix notification serialization issues

Sometimes ->target() returns earlier than expected, even without doing any
PRECHANGE notifications, in such cases we aren't decrementing our
transition_ongoing counter and that makes it go crazy for those cases.

This patch fixes this issue by taking care of transition_ongoing count at
several places. Now we simply decrement refcount at the end of
cpufreq_out_of_sync() & __cpufreq_driver_target() for drivers that don't do
ASYNC notifications, for others we do that from POSTCHANGE notifier.

These drivers also return -EINPROGRESS from their target() routines to mark that
transtion will be completed later.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq.c            | 42 ++++++++++++++++++++++++++++--------
 drivers/cpufreq/exynos5440-cpufreq.c |  3 +++
 2 files changed, 36 insertions(+), 9 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 9cc5609..9644150 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -348,7 +348,8 @@ void cpufreq_notify_transition(struct cpufreq_policy *policy,
 	for_each_cpu(freqs->cpu, policy->cpus)
 		__cpufreq_notify_transition(policy, freqs, state);
 
-	if (state == CPUFREQ_POSTCHANGE) {
+	if ((cpufreq_driver->flags & CPUFREQ_ASYNC_NOTIFICATION)
+			&& (state == CPUFREQ_POSTCHANGE)) {
 		unsigned long flags;
 
 		/*
@@ -1406,6 +1407,17 @@ static void cpufreq_out_of_sync(unsigned int cpu, unsigned int old_freq,
 
 	cpufreq_notify_transition(policy, &freqs, CPUFREQ_PRECHANGE);
 	cpufreq_notify_transition(policy, &freqs, CPUFREQ_POSTCHANGE);
+
+	/*
+	 * For drivers with CPUFREQ_ASYNC_NOTIFICATION flag set, we decrement
+	 * transition_ongoing from POSTCHANGE notifiers.
+	 */
+	if (cpufreq_driver->flags & CPUFREQ_ASYNC_NOTIFICATION)
+		return;
+
+	write_lock_irqsave(&cpufreq_driver_lock, flags);
+	policy->transition_ongoing--;
+	write_unlock_irqrestore(&cpufreq_driver_lock, flags);
 }
 
 /**
@@ -1696,14 +1708,6 @@ int __cpufreq_driver_target(struct cpufreq_policy *policy,
 	if (cpufreq_disabled())
 		return -ENODEV;
 
-	write_lock_irqsave(&cpufreq_driver_lock, flags);
-	if (policy->transition_ongoing) {
-		write_unlock_irqrestore(&cpufreq_driver_lock, flags);
-		return -EBUSY;
-	}
-	policy->transition_ongoing++;
-	write_unlock_irqrestore(&cpufreq_driver_lock, flags);
-
 	/* Make sure that target_freq is within supported range */
 	if (target_freq > policy->max)
 		target_freq = policy->max;
@@ -1716,9 +1720,29 @@ int __cpufreq_driver_target(struct cpufreq_policy *policy,
 	if (target_freq == policy->cur)
 		return 0;
 
+	write_lock_irqsave(&cpufreq_driver_lock, flags);
+	if (policy->transition_ongoing) {
+		write_unlock_irqrestore(&cpufreq_driver_lock, flags);
+		return -EBUSY;
+	}
+	policy->transition_ongoing++;
+	write_unlock_irqrestore(&cpufreq_driver_lock, flags);
+
 	if (cpufreq_driver->target)
 		retval = cpufreq_driver->target(policy, target_freq, relation);
 
+	/*
+	 * For drivers with CPUFREQ_ASYNC_NOTIFICATION flag set, we decrement
+	 * transition_ongoing from POSTCHANGE notifiers.
+	 */
+	if ((cpufreq_driver->flags & CPUFREQ_ASYNC_NOTIFICATION)
+			&& (retval == -EINPROGRESS))
+		return retval;
+
+	write_lock_irqsave(&cpufreq_driver_lock, flags);
+	policy->transition_ongoing--;
+	write_unlock_irqrestore(&cpufreq_driver_lock, flags);
+
 	return retval;
 }
 EXPORT_SYMBOL_GPL(__cpufreq_driver_target);
diff --git a/drivers/cpufreq/exynos5440-cpufreq.c b/drivers/cpufreq/exynos5440-cpufreq.c
index f44664a..1e391ac 100644
--- a/drivers/cpufreq/exynos5440-cpufreq.c
+++ b/drivers/cpufreq/exynos5440-cpufreq.c
@@ -251,6 +251,9 @@ static int exynos_target(struct cpufreq_policy *policy,
 
 		__raw_writel(tmp, dvfs_info->base + XMU_C0_3_PSTATE + i * 4);
 	}
+
+	/* Mark transition as In-progress */
+	ret = -EINPROGRESS;
 out:
 	mutex_unlock(&cpufreq_lock);
 	return ret;
-- 
1.7.12.rc2.18.g61b472e

Re: "cpufreq: fix serialization issues with freq change notifiers" breaks cpufreq too

[Viresh Kumar]

On 11 September 2013 01:16, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Tuesday, September 10, 2013 08:44:18 PM Viresh Kumar wrote:

>> Now Second question, is this fine to have multiple PRECHANGE notfications
>> before any POSTCHANGE notification?
>>
>> Logically it looks obvious to me that these must be serialized..
>> Otherwise this is what we are broadcasting:
>> - We are changing for freq X, please prepare and let us know if you are
>> okay with it..
>> - Oh.. So sorry, we are changing to freq Y instead, please adjust accordingly.
>>
>> - Yes we have changed to freq Y...
>> - Yes we have changed to freq X...
>>
>> This looks stupid, isn't it? I don't know how the drivers would behave when
>> they see such notifications and so got to this patch initially..
>
> Well, precisely, you don't know.
>
> Can you please look for specific problems and address those instead of trying
> to address potential issues that may or may not happen and may or may not really
> be issues even if they actually happen?

That looked like a straight forward issue/bug to me and so I haven't
gotten deep into it..

Scenario 1:
--------------
Notifiers are not serialized and suppose this is what happened
- PRECHANGE Notification for freq A (from cpuinfo_cur_freq)
- PRECHANGE Notification for freq B (from target())
- Freq changed by target() to B
- POSTCHANGE Notification for freq B
- POSTCHANGE Notification for freq A

Now the last POSTCHANGE Notification happened for freq A and
hardware is currently running at freq B :)

Where would we break then?: adjust_jiffies() in cpufreq.c,
cpufreq_callback() in arch/arm/kernel/smp.c (which is also adjusting
jiffies)..

Now, all loops_per_jiffy based stuff is broken.. Have I missed
something?

Similarly there are numerous places where we may break.. As we
have broken expectations of receivers of these notifications..

Scenario 2:
--------------
Governor is changing freq and has called __cpufreq_driver_target().
At the same time we are changing scaling_{min|max}_freq from
sysfs, which would eventually end up calling governors:
CPUFREQ_GOV_LIMITS notification, that will also call:
__cpufreq_driver_target()..

So, we eventually have two concurrent calls to ->target() and we
don't really know how hardware will behave in this case.. Most of
the implementations of ->target() routines just go and change
freq/voltage without checking if we are already in progress of doing
that (i.e. based on expectation that this call is not re entrant)..

Now anything can happen at hardware level, which I don't have
all insight of :(

> And broken patches that try to fix such things definitely don't help.

Yeah, that's my fault but I honestly try to produce bug-less patches..
But being a normal human being, there are always some chances
that my patches are eventually broken :)

> For now I'm reverting the commit in question and everything on top of it.

Its okay..

Re: "cpufreq: fix serialization issues with freq change notifiers" breaks cpufreq too

[Viresh Kumar]

On 11 September 2013 01:16, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Tuesday, September 10, 2013 08:44:18 PM Viresh Kumar wrote:

>> Now Second question, is this fine to have multiple PRECHANGE notfications
>> before any POSTCHANGE notification?
>>
>> Logically it looks obvious to me that these must be serialized..
>> Otherwise this is what we are broadcasting:
>> - We are changing for freq X, please prepare and let us know if you are
>> okay with it..
>> - Oh.. So sorry, we are changing to freq Y instead, please adjust accordingly.
>>
>> - Yes we have changed to freq Y...
>> - Yes we have changed to freq X...
>>
>> This looks stupid, isn't it? I don't know how the drivers would behave when
>> they see such notifications and so got to this patch initially..
>
> Well, precisely, you don't know.
>
> Can you please look for specific problems and address those instead of trying
> to address potential issues that may or may not happen and may or may not really
> be issues even if they actually happen?

That looked like a straight forward issue/bug to me and so I haven't
gotten deep into it..

Scenario 1:
--------------
Notifiers are not serialized and suppose this is what happened
- PRECHANGE Notification for freq A (from cpuinfo_cur_freq)
- PRECHANGE Notification for freq B (from target())
- Freq changed by target() to B
- POSTCHANGE Notification for freq B
- POSTCHANGE Notification for freq A

Now the last POSTCHANGE Notification happened for freq A and
hardware is currently running at freq B :)

Where would we break then?: adjust_jiffies() in cpufreq.c,
cpufreq_callback() in arch/arm/kernel/smp.c (which is also adjusting
jiffies)..

Now, all loops_per_jiffy based stuff is broken.. Have I missed
something?

Similarly there are numerous places where we may break.. As we
have broken expectations of receivers of these notifications..

Scenario 2:
--------------
Governor is changing freq and has called __cpufreq_driver_target().
At the same time we are changing scaling_{min|max}_freq from
sysfs, which would eventually end up calling governors:
CPUFREQ_GOV_LIMITS notification, that will also call:
__cpufreq_driver_target()..

So, we eventually have two concurrent calls to ->target() and we
don't really know how hardware will behave in this case.. Most of
the implementations of ->target() routines just go and change
freq/voltage without checking if we are already in progress of doing
that (i.e. based on expectation that this call is not re entrant)..

Now anything can happen at hardware level, which I don't have
all insight of :(

> And broken patches that try to fix such things definitely don't help.

Yeah, that's my fault but I honestly try to produce bug-less patches..
But being a normal human being, there are always some chances
that my patches are eventually broken :)

> For now I'm reverting the commit in question and everything on top of it.

Its okay..

"cpufreq: fix serialization issues with freq change notifiers" breaks cpufreq too

[Viresh Kumar]

On 11 September 2013 01:16, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Tuesday, September 10, 2013 08:44:18 PM Viresh Kumar wrote:

>> Now Second question, is this fine to have multiple PRECHANGE notfications
>> before any POSTCHANGE notification?
>>
>> Logically it looks obvious to me that these must be serialized..
>> Otherwise this is what we are broadcasting:
>> - We are changing for freq X, please prepare and let us know if you are
>> okay with it..
>> - Oh.. So sorry, we are changing to freq Y instead, please adjust accordingly.
>>
>> - Yes we have changed to freq Y...
>> - Yes we have changed to freq X...
>>
>> This looks stupid, isn't it? I don't know how the drivers would behave when
>> they see such notifications and so got to this patch initially..
>
> Well, precisely, you don't know.
>
> Can you please look for specific problems and address those instead of trying
> to address potential issues that may or may not happen and may or may not really
> be issues even if they actually happen?

That looked like a straight forward issue/bug to me and so I haven't
gotten deep into it..

Scenario 1:
--------------
Notifiers are not serialized and suppose this is what happened
- PRECHANGE Notification for freq A (from cpuinfo_cur_freq)
- PRECHANGE Notification for freq B (from target())
- Freq changed by target() to B
- POSTCHANGE Notification for freq B
- POSTCHANGE Notification for freq A

Now the last POSTCHANGE Notification happened for freq A and
hardware is currently running at freq B :)

Where would we break then?: adjust_jiffies() in cpufreq.c,
cpufreq_callback() in arch/arm/kernel/smp.c (which is also adjusting
jiffies)..

Now, all loops_per_jiffy based stuff is broken.. Have I missed
something?

Similarly there are numerous places where we may break.. As we
have broken expectations of receivers of these notifications..

Scenario 2:
--------------
Governor is changing freq and has called __cpufreq_driver_target().
At the same time we are changing scaling_{min|max}_freq from
sysfs, which would eventually end up calling governors:
CPUFREQ_GOV_LIMITS notification, that will also call:
__cpufreq_driver_target()..

So, we eventually have two concurrent calls to ->target() and we
don't really know how hardware will behave in this case.. Most of
the implementations of ->target() routines just go and change
freq/voltage without checking if we are already in progress of doing
that (i.e. based on expectation that this call is not re entrant)..

Now anything can happen at hardware level, which I don't have
all insight of :(

> And broken patches that try to fix such things definitely don't help.

Yeah, that's my fault but I honestly try to produce bug-less patches..
But being a normal human being, there are always some chances
that my patches are eventually broken :)

> For now I'm reverting the commit in question and everything on top of it.

Its okay..

Re: "cpufreq: fix serialization issues with freq change notifiers" breaks cpufreq too

[Viresh Kumar]

On 11 September 2013 13:45, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> I'd rather wait until Rafael is convinced, then we'll see.

Okay.. I have just sent a mail to Rafael about that, see if you
are convinced with what I wrote :)

--
viresh

Re: "cpufreq: fix serialization issues with freq change notifiers" breaks cpufreq too

[Viresh Kumar]

On 11 September 2013 13:45, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> I'd rather wait until Rafael is convinced, then we'll see.

Okay.. I have just sent a mail to Rafael about that, see if you
are convinced with what I wrote :)

--
viresh

"cpufreq: fix serialization issues with freq change notifiers" breaks cpufreq too

[Viresh Kumar]

On 11 September 2013 13:45, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> I'd rather wait until Rafael is convinced, then we'll see.

Okay.. I have just sent a mail to Rafael about that, see if you
are convinced with what I wrote :)

--
viresh

Re: "cpufreq: fix serialization issues with freq change notifiers" breaks cpufreq too

[Viresh Kumar]

On 11 September 2013 01:16, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Tuesday, September 10, 2013 08:44:18 PM Viresh Kumar wrote:

>> Now Second question, is this fine to have multiple PRECHANGE notfications
>> before any POSTCHANGE notification?
>>
>> Logically it looks obvious to me that these must be serialized..
>> Otherwise this is what we are broadcasting:
>> - We are changing for freq X, please prepare and let us know if you are
>> okay with it..
>> - Oh.. So sorry, we are changing to freq Y instead, please adjust accordingly.
>>
>> - Yes we have changed to freq Y...
>> - Yes we have changed to freq X...
>>
>> This looks stupid, isn't it? I don't know how the drivers would behave when
>> they see such notifications and so got to this patch initially..
>
> Well, precisely, you don't know.
>
> Can you please look for specific problems and address those instead of trying
> to address potential issues that may or may not happen and may or may not really
> be issues even if they actually happen?

That looked like a straight forward issue/bug to me and so I haven't
gotten deep into it..

Scenario 1:
--------------
Notifiers are not serialized and suppose this is what happened
- PRECHANGE Notification for freq A (from cpuinfo_cur_freq)
- PRECHANGE Notification for freq B (from target())
- Freq changed by target() to B
- POSTCHANGE Notification for freq B
- POSTCHANGE Notification for freq A

Now the last POSTCHANGE Notification happened for freq A and
hardware is currently running at freq B :)

Where would we break then?: adjust_jiffies() in cpufreq.c,
cpufreq_callback() in arch/arm/kernel/smp.c (which is also adjusting
jiffies)..

Now, all loops_per_jiffy based stuff is broken.. Have I missed
something?

Similarly there are numerous places where we may break.. As we
have broken expectations of receivers of these notifications..

Scenario 2:
--------------
Governor is changing freq and has called __cpufreq_driver_target().
At the same time we are changing scaling_{min|max}_freq from
sysfs, which would eventually end up calling governors:
CPUFREQ_GOV_LIMITS notification, that will also call:
__cpufreq_driver_target()..

So, we eventually have two concurrent calls to ->target() and we
don't really know how hardware will behave in this case.. Most of
the implementations of ->target() routines just go and change
freq/voltage without checking if we are already in progress of doing
that (i.e. based on expectation that this call is not re entrant)..

Now anything can happen at hardware level, which I don't have
all insight of :(

> And broken patches that try to fix such things definitely don't help.

Yeah, that's my fault but I honestly try to produce bug-less patches..
But being a normal human being, there are always some chances
that my patches are eventually broken :)

> For now I'm reverting the commit in question and everything on top of it.

Its okay..

Re: "cpufreq: fix serialization issues with freq change notifiers" breaks cpufreq too

[Viresh Kumar]

On 11 September 2013 13:45, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> I'd rather wait until Rafael is convinced, then we'll see.

Okay.. I have just sent a mail to Rafael about that, see if you
are convinced with what I wrote :)

--
viresh

Re: "cpufreq: fix serialization issues with freq change notifiers" breaks cpufreq too

[Rafael J. Wysocki]

On Wednesday, September 11, 2013 02:08:44 PM Viresh Kumar wrote:
> On 11 September 2013 01:16, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > On Tuesday, September 10, 2013 08:44:18 PM Viresh Kumar wrote:
> 
> >> Now Second question, is this fine to have multiple PRECHANGE notfications
> >> before any POSTCHANGE notification?
> >>
> >> Logically it looks obvious to me that these must be serialized..
> >> Otherwise this is what we are broadcasting:
> >> - We are changing for freq X, please prepare and let us know if you are
> >> okay with it..
> >> - Oh.. So sorry, we are changing to freq Y instead, please adjust accordingly.
> >>
> >> - Yes we have changed to freq Y...
> >> - Yes we have changed to freq X...
> >>
> >> This looks stupid, isn't it? I don't know how the drivers would behave when
> >> they see such notifications and so got to this patch initially..
> >
> > Well, precisely, you don't know.
> >
> > Can you please look for specific problems and address those instead of trying
> > to address potential issues that may or may not happen and may or may not really
> > be issues even if they actually happen?
> 
> That looked like a straight forward issue/bug to me and so I haven't
> gotten deep into it..

Which you should always do when you're going to deal with concurrency issues.
Even if they appear to be obvious, they often are far from that, like in this
case.

> Scenario 1:
> --------------
> Notifiers are not serialized and suppose this is what happened
> - PRECHANGE Notification for freq A (from cpuinfo_cur_freq)
> - PRECHANGE Notification for freq B (from target())
> - Freq changed by target() to B
> - POSTCHANGE Notification for freq B
> - POSTCHANGE Notification for freq A
> 
> Now the last POSTCHANGE Notification happened for freq A and
> hardware is currently running at freq B :)
> 
> Where would we break then?: adjust_jiffies() in cpufreq.c,
> cpufreq_callback() in arch/arm/kernel/smp.c (which is also adjusting
> jiffies)..
> 
> Now, all loops_per_jiffy based stuff is broken.. Have I missed
> something?
>
> Similarly there are numerous places where we may break.. As we
> have broken expectations of receivers of these notifications..

Now it is clear what the problem is and that should go in the changelog of the
fix patch.
 
> Scenario 2:
> --------------
> Governor is changing freq and has called __cpufreq_driver_target().
> At the same time we are changing scaling_{min|max}_freq from
> sysfs, which would eventually end up calling governors:
> CPUFREQ_GOV_LIMITS notification, that will also call:
> __cpufreq_driver_target()..
> 
> So, we eventually have two concurrent calls to ->target() and we
> don't really know how hardware will behave in this case.. Most of
> the implementations of ->target() routines just go and change
> freq/voltage without checking if we are already in progress of doing
> that (i.e. based on expectation that this call is not re entrant)..
> 
> Now anything can happen at hardware level, which I don't have
> all insight of :(

That is more theoretical, however.

I think that the "Scenario 1" is a good enough reason for making this change,
but it needs to be done with care.

> > And broken patches that try to fix such things definitely don't help.
> 
> Yeah, that's my fault but I honestly try to produce bug-less patches..
> But being a normal human being, there are always some chances
> that my patches are eventually broken :)

Well, that happens for everyone, but in this particular case we had three
different issues because of a single patch ...

And actually I also should have spend more time on this before applying the
patch, especially given the laconic changelog, so it's my fault too.

> > For now I'm reverting the commit in question and everything on top of it.
> 
> Its okay..

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: "cpufreq: fix serialization issues with freq change notifiers" breaks cpufreq too

[Rafael J. Wysocki]

On Wednesday, September 11, 2013 02:08:44 PM Viresh Kumar wrote:
> On 11 September 2013 01:16, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > On Tuesday, September 10, 2013 08:44:18 PM Viresh Kumar wrote:
> 
> >> Now Second question, is this fine to have multiple PRECHANGE notfications
> >> before any POSTCHANGE notification?
> >>
> >> Logically it looks obvious to me that these must be serialized..
> >> Otherwise this is what we are broadcasting:
> >> - We are changing for freq X, please prepare and let us know if you are
> >> okay with it..
> >> - Oh.. So sorry, we are changing to freq Y instead, please adjust accordingly.
> >>
> >> - Yes we have changed to freq Y...
> >> - Yes we have changed to freq X...
> >>
> >> This looks stupid, isn't it? I don't know how the drivers would behave when
> >> they see such notifications and so got to this patch initially..
> >
> > Well, precisely, you don't know.
> >
> > Can you please look for specific problems and address those instead of trying
> > to address potential issues that may or may not happen and may or may not really
> > be issues even if they actually happen?
> 
> That looked like a straight forward issue/bug to me and so I haven't
> gotten deep into it..

Which you should always do when you're going to deal with concurrency issues.
Even if they appear to be obvious, they often are far from that, like in this
case.

> Scenario 1:
> --------------
> Notifiers are not serialized and suppose this is what happened
> - PRECHANGE Notification for freq A (from cpuinfo_cur_freq)
> - PRECHANGE Notification for freq B (from target())
> - Freq changed by target() to B
> - POSTCHANGE Notification for freq B
> - POSTCHANGE Notification for freq A
> 
> Now the last POSTCHANGE Notification happened for freq A and
> hardware is currently running at freq B :)
> 
> Where would we break then?: adjust_jiffies() in cpufreq.c,
> cpufreq_callback() in arch/arm/kernel/smp.c (which is also adjusting
> jiffies)..
> 
> Now, all loops_per_jiffy based stuff is broken.. Have I missed
> something?
>
> Similarly there are numerous places where we may break.. As we
> have broken expectations of receivers of these notifications..

Now it is clear what the problem is and that should go in the changelog of the
fix patch.
 
> Scenario 2:
> --------------
> Governor is changing freq and has called __cpufreq_driver_target().
> At the same time we are changing scaling_{min|max}_freq from
> sysfs, which would eventually end up calling governors:
> CPUFREQ_GOV_LIMITS notification, that will also call:
> __cpufreq_driver_target()..
> 
> So, we eventually have two concurrent calls to ->target() and we
> don't really know how hardware will behave in this case.. Most of
> the implementations of ->target() routines just go and change
> freq/voltage without checking if we are already in progress of doing
> that (i.e. based on expectation that this call is not re entrant)..
> 
> Now anything can happen at hardware level, which I don't have
> all insight of :(

That is more theoretical, however.

I think that the "Scenario 1" is a good enough reason for making this change,
but it needs to be done with care.

> > And broken patches that try to fix such things definitely don't help.
> 
> Yeah, that's my fault but I honestly try to produce bug-less patches..
> But being a normal human being, there are always some chances
> that my patches are eventually broken :)

Well, that happens for everyone, but in this particular case we had three
different issues because of a single patch ...

And actually I also should have spend more time on this before applying the
patch, especially given the laconic changelog, so it's my fault too.

> > For now I'm reverting the commit in question and everything on top of it.
> 
> Its okay..

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: "cpufreq: fix serialization issues with freq change notifiers" breaks cpufreq too

[Rafael J. Wysocki]

On Wednesday, September 11, 2013 02:08:44 PM Viresh Kumar wrote:
> On 11 September 2013 01:16, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > On Tuesday, September 10, 2013 08:44:18 PM Viresh Kumar wrote:
> 
> >> Now Second question, is this fine to have multiple PRECHANGE notfications
> >> before any POSTCHANGE notification?
> >>
> >> Logically it looks obvious to me that these must be serialized..
> >> Otherwise this is what we are broadcasting:
> >> - We are changing for freq X, please prepare and let us know if you are
> >> okay with it..
> >> - Oh.. So sorry, we are changing to freq Y instead, please adjust accordingly.
> >>
> >> - Yes we have changed to freq Y...
> >> - Yes we have changed to freq X...
> >>
> >> This looks stupid, isn't it? I don't know how the drivers would behave when
> >> they see such notifications and so got to this patch initially..
> >
> > Well, precisely, you don't know.
> >
> > Can you please look for specific problems and address those instead of trying
> > to address potential issues that may or may not happen and may or may not really
> > be issues even if they actually happen?
> 
> That looked like a straight forward issue/bug to me and so I haven't
> gotten deep into it..

Which you should always do when you're going to deal with concurrency issues.
Even if they appear to be obvious, they often are far from that, like in this
case.

> Scenario 1:
> --------------
> Notifiers are not serialized and suppose this is what happened
> - PRECHANGE Notification for freq A (from cpuinfo_cur_freq)
> - PRECHANGE Notification for freq B (from target())
> - Freq changed by target() to B
> - POSTCHANGE Notification for freq B
> - POSTCHANGE Notification for freq A
> 
> Now the last POSTCHANGE Notification happened for freq A and
> hardware is currently running at freq B :)
> 
> Where would we break then?: adjust_jiffies() in cpufreq.c,
> cpufreq_callback() in arch/arm/kernel/smp.c (which is also adjusting
> jiffies)..
> 
> Now, all loops_per_jiffy based stuff is broken.. Have I missed
> something?
>
> Similarly there are numerous places where we may break.. As we
> have broken expectations of receivers of these notifications..

Now it is clear what the problem is and that should go in the changelog of the
fix patch.
 
> Scenario 2:
> --------------
> Governor is changing freq and has called __cpufreq_driver_target().
> At the same time we are changing scaling_{min|max}_freq from
> sysfs, which would eventually end up calling governors:
> CPUFREQ_GOV_LIMITS notification, that will also call:
> __cpufreq_driver_target()..
> 
> So, we eventually have two concurrent calls to ->target() and we
> don't really know how hardware will behave in this case.. Most of
> the implementations of ->target() routines just go and change
> freq/voltage without checking if we are already in progress of doing
> that (i.e. based on expectation that this call is not re entrant)..
> 
> Now anything can happen at hardware level, which I don't have
> all insight of :(

That is more theoretical, however.

I think that the "Scenario 1" is a good enough reason for making this change,
but it needs to be done with care.

> > And broken patches that try to fix such things definitely don't help.
> 
> Yeah, that's my fault but I honestly try to produce bug-less patches..
> But being a normal human being, there are always some chances
> that my patches are eventually broken :)

Well, that happens for everyone, but in this particular case we had three
different issues because of a single patch ...

And actually I also should have spend more time on this before applying the
patch, especially given the laconic changelog, so it's my fault too.

> > For now I'm reverting the commit in question and everything on top of it.
> 
> Its okay..

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

"cpufreq: fix serialization issues with freq change notifiers" breaks cpufreq too

[Rafael J. Wysocki]

On Wednesday, September 11, 2013 02:08:44 PM Viresh Kumar wrote:
> On 11 September 2013 01:16, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > On Tuesday, September 10, 2013 08:44:18 PM Viresh Kumar wrote:
> 
> >> Now Second question, is this fine to have multiple PRECHANGE notfications
> >> before any POSTCHANGE notification?
> >>
> >> Logically it looks obvious to me that these must be serialized..
> >> Otherwise this is what we are broadcasting:
> >> - We are changing for freq X, please prepare and let us know if you are
> >> okay with it..
> >> - Oh.. So sorry, we are changing to freq Y instead, please adjust accordingly.
> >>
> >> - Yes we have changed to freq Y...
> >> - Yes we have changed to freq X...
> >>
> >> This looks stupid, isn't it? I don't know how the drivers would behave when
> >> they see such notifications and so got to this patch initially..
> >
> > Well, precisely, you don't know.
> >
> > Can you please look for specific problems and address those instead of trying
> > to address potential issues that may or may not happen and may or may not really
> > be issues even if they actually happen?
> 
> That looked like a straight forward issue/bug to me and so I haven't
> gotten deep into it..

Which you should always do when you're going to deal with concurrency issues.
Even if they appear to be obvious, they often are far from that, like in this
case.

> Scenario 1:
> --------------
> Notifiers are not serialized and suppose this is what happened
> - PRECHANGE Notification for freq A (from cpuinfo_cur_freq)
> - PRECHANGE Notification for freq B (from target())
> - Freq changed by target() to B
> - POSTCHANGE Notification for freq B
> - POSTCHANGE Notification for freq A
> 
> Now the last POSTCHANGE Notification happened for freq A and
> hardware is currently running at freq B :)
> 
> Where would we break then?: adjust_jiffies() in cpufreq.c,
> cpufreq_callback() in arch/arm/kernel/smp.c (which is also adjusting
> jiffies)..
> 
> Now, all loops_per_jiffy based stuff is broken.. Have I missed
> something?
>
> Similarly there are numerous places where we may break.. As we
> have broken expectations of receivers of these notifications..

Now it is clear what the problem is and that should go in the changelog of the
fix patch.
 
> Scenario 2:
> --------------
> Governor is changing freq and has called __cpufreq_driver_target().
> At the same time we are changing scaling_{min|max}_freq from
> sysfs, which would eventually end up calling governors:
> CPUFREQ_GOV_LIMITS notification, that will also call:
> __cpufreq_driver_target()..
> 
> So, we eventually have two concurrent calls to ->target() and we
> don't really know how hardware will behave in this case.. Most of
> the implementations of ->target() routines just go and change
> freq/voltage without checking if we are already in progress of doing
> that (i.e. based on expectation that this call is not re entrant)..
> 
> Now anything can happen at hardware level, which I don't have
> all insight of :(

That is more theoretical, however.

I think that the "Scenario 1" is a good enough reason for making this change,
but it needs to be done with care.

> > And broken patches that try to fix such things definitely don't help.
> 
> Yeah, that's my fault but I honestly try to produce bug-less patches..
> But being a normal human being, there are always some chances
> that my patches are eventually broken :)

Well, that happens for everyone, but in this particular case we had three
different issues because of a single patch ...

And actually I also should have spend more time on this before applying the
patch, especially given the laconic changelog, so it's my fault too.

> > For now I'm reverting the commit in question and everything on top of it.
> 
> Its okay..

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: "cpufreq: fix serialization issues with freq change notifiers" breaks cpufreq too

[Rafael J. Wysocki]

On Wednesday, September 11, 2013 10:15:53 AM Guennadi Liakhovetski wrote:
> On Wed, 11 Sep 2013, Viresh Kumar wrote:
> 
> > On 10 September 2013 22:37, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> > > On Tue, 10 Sep 2013, Viresh Kumar wrote:
> > >> Quite straight forward actually..
> > >
> > > Apparently, not quite.
> > 
> > I overlooked the situation where we return early from ->target() routines.. :(
> > 
> > Please try attached patches, I will repost them later (once I am able to
> > convince Rafael that these are really important :) )
> 
> I'd rather wait until Rafael is convinced, then we'll see.

I'm going to revert the commit that was the source of these issues, but that
doesn't mean that everything is now rosy.  We need to fix the concurrency
problems that Viresh was trying to fix in that commit, so it would be nice if
you could verify whether or not he is on the right track.

Thanks,
Rafael

Re: "cpufreq: fix serialization issues with freq change notifiers" breaks cpufreq too

[Rafael J. Wysocki]

On Wednesday, September 11, 2013 10:15:53 AM Guennadi Liakhovetski wrote:
> On Wed, 11 Sep 2013, Viresh Kumar wrote:
> 
> > On 10 September 2013 22:37, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> > > On Tue, 10 Sep 2013, Viresh Kumar wrote:
> > >> Quite straight forward actually..
> > >
> > > Apparently, not quite.
> > 
> > I overlooked the situation where we return early from ->target() routines.. :(
> > 
> > Please try attached patches, I will repost them later (once I am able to
> > convince Rafael that these are really important :) )
> 
> I'd rather wait until Rafael is convinced, then we'll see.

I'm going to revert the commit that was the source of these issues, but that
doesn't mean that everything is now rosy.  We need to fix the concurrency
problems that Viresh was trying to fix in that commit, so it would be nice if
you could verify whether or not he is on the right track.

Thanks,
Rafael

Re: "cpufreq: fix serialization issues with freq change notifiers" breaks cpufreq too

[Rafael J. Wysocki]

On Wednesday, September 11, 2013 10:15:53 AM Guennadi Liakhovetski wrote:
> On Wed, 11 Sep 2013, Viresh Kumar wrote:
> 
> > On 10 September 2013 22:37, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> > > On Tue, 10 Sep 2013, Viresh Kumar wrote:
> > >> Quite straight forward actually..
> > >
> > > Apparently, not quite.
> > 
> > I overlooked the situation where we return early from ->target() routines.. :(
> > 
> > Please try attached patches, I will repost them later (once I am able to
> > convince Rafael that these are really important :) )
> 
> I'd rather wait until Rafael is convinced, then we'll see.

I'm going to revert the commit that was the source of these issues, but that
doesn't mean that everything is now rosy.  We need to fix the concurrency
problems that Viresh was trying to fix in that commit, so it would be nice if
you could verify whether or not he is on the right track.

Thanks,
Rafael

"cpufreq: fix serialization issues with freq change notifiers" breaks cpufreq too

[Rafael J. Wysocki]

On Wednesday, September 11, 2013 10:15:53 AM Guennadi Liakhovetski wrote:
> On Wed, 11 Sep 2013, Viresh Kumar wrote:
> 
> > On 10 September 2013 22:37, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> > > On Tue, 10 Sep 2013, Viresh Kumar wrote:
> > >> Quite straight forward actually..
> > >
> > > Apparently, not quite.
> > 
> > I overlooked the situation where we return early from ->target() routines.. :(
> > 
> > Please try attached patches, I will repost them later (once I am able to
> > convince Rafael that these are really important :) )
> 
> I'd rather wait until Rafael is convinced, then we'll see.

I'm going to revert the commit that was the source of these issues, but that
doesn't mean that everything is now rosy.  We need to fix the concurrency
problems that Viresh was trying to fix in that commit, so it would be nice if
you could verify whether or not he is on the right track.

Thanks,
Rafael

Re: "cpufreq: fix serialization issues with freq change notifiers" breaks cpufreq too

[Viresh Kumar]

On 11 September 2013 18:48, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Wednesday, September 11, 2013 02:08:44 PM Viresh Kumar wrote:

>> That looked like a straight forward issue/bug to me and so I haven't
>> gotten deep into it..
>
> Which you should always do when you're going to deal with concurrency issues.
> Even if they appear to be obvious, they often are far from that, like in this
> case.

/me Nods

>> Scenario 2:
>> --------------
>> Governor is changing freq and has called __cpufreq_driver_target().
>> At the same time we are changing scaling_{min|max}_freq from
>> sysfs, which would eventually end up calling governors:
>> CPUFREQ_GOV_LIMITS notification, that will also call:
>> __cpufreq_driver_target()..
>>
>> So, we eventually have two concurrent calls to ->target() and we
>> don't really know how hardware will behave in this case.. Most of
>> the implementations of ->target() routines just go and change
>> freq/voltage without checking if we are already in progress of doing
>> that (i.e. based on expectation that this call is not re entrant)..
>>
>> Now anything can happen at hardware level, which I don't have
>> all insight of :(
>
> That is more theoretical, however.

Maybe we can get more deeper into it then :)
Platform have something like this in their target()

A. If new freq is more than old: Increase voltage
B. Change freq
C. If new freq is less than old: decrease voltage

Now, two concurrent calls to target are X and Y, where X is trying to increase
freq and Y is trying to decrease it..

And this is the sequence that followed due to races..

X.A: voltage increased for larger freq
Y.A: nothing happened here
Y.B: freq decreased
Y.C: voltage decreased
X.B: freq increased
X.C: nothing happened..

We ended up setting a freq which is not supported by the voltage we have
set.. That will probably make clock to CPU unstable and system wouldn't
be workable anymore...

And so I think even this case must also get some space in the changelog :)

Re: "cpufreq: fix serialization issues with freq change notifiers" breaks cpufreq too

[Viresh Kumar]

On 11 September 2013 18:48, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Wednesday, September 11, 2013 02:08:44 PM Viresh Kumar wrote:

>> That looked like a straight forward issue/bug to me and so I haven't
>> gotten deep into it..
>
> Which you should always do when you're going to deal with concurrency issues.
> Even if they appear to be obvious, they often are far from that, like in this
> case.

/me Nods

>> Scenario 2:
>> --------------
>> Governor is changing freq and has called __cpufreq_driver_target().
>> At the same time we are changing scaling_{min|max}_freq from
>> sysfs, which would eventually end up calling governors:
>> CPUFREQ_GOV_LIMITS notification, that will also call:
>> __cpufreq_driver_target()..
>>
>> So, we eventually have two concurrent calls to ->target() and we
>> don't really know how hardware will behave in this case.. Most of
>> the implementations of ->target() routines just go and change
>> freq/voltage without checking if we are already in progress of doing
>> that (i.e. based on expectation that this call is not re entrant)..
>>
>> Now anything can happen at hardware level, which I don't have
>> all insight of :(
>
> That is more theoretical, however.

Maybe we can get more deeper into it then :)
Platform have something like this in their target()

A. If new freq is more than old: Increase voltage
B. Change freq
C. If new freq is less than old: decrease voltage

Now, two concurrent calls to target are X and Y, where X is trying to increase
freq and Y is trying to decrease it..

And this is the sequence that followed due to races..

X.A: voltage increased for larger freq
Y.A: nothing happened here
Y.B: freq decreased
Y.C: voltage decreased
X.B: freq increased
X.C: nothing happened..

We ended up setting a freq which is not supported by the voltage we have
set.. That will probably make clock to CPU unstable and system wouldn't
be workable anymore...

And so I think even this case must also get some space in the changelog :)

"cpufreq: fix serialization issues with freq change notifiers" breaks cpufreq too

[Viresh Kumar]

On 11 September 2013 18:48, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Wednesday, September 11, 2013 02:08:44 PM Viresh Kumar wrote:

>> That looked like a straight forward issue/bug to me and so I haven't
>> gotten deep into it..
>
> Which you should always do when you're going to deal with concurrency issues.
> Even if they appear to be obvious, they often are far from that, like in this
> case.

/me Nods

>> Scenario 2:
>> --------------
>> Governor is changing freq and has called __cpufreq_driver_target().
>> At the same time we are changing scaling_{min|max}_freq from
>> sysfs, which would eventually end up calling governors:
>> CPUFREQ_GOV_LIMITS notification, that will also call:
>> __cpufreq_driver_target()..
>>
>> So, we eventually have two concurrent calls to ->target() and we
>> don't really know how hardware will behave in this case.. Most of
>> the implementations of ->target() routines just go and change
>> freq/voltage without checking if we are already in progress of doing
>> that (i.e. based on expectation that this call is not re entrant)..
>>
>> Now anything can happen at hardware level, which I don't have
>> all insight of :(
>
> That is more theoretical, however.

Maybe we can get more deeper into it then :)
Platform have something like this in their target()

A. If new freq is more than old: Increase voltage
B. Change freq
C. If new freq is less than old: decrease voltage

Now, two concurrent calls to target are X and Y, where X is trying to increase
freq and Y is trying to decrease it..

And this is the sequence that followed due to races..

X.A: voltage increased for larger freq
Y.A: nothing happened here
Y.B: freq decreased
Y.C: voltage decreased
X.B: freq increased
X.C: nothing happened..

We ended up setting a freq which is not supported by the voltage we have
set.. That will probably make clock to CPU unstable and system wouldn't
be workable anymore...

And so I think even this case must also get some space in the changelog :)

Re: "cpufreq: fix serialization issues with freq change notifiers" breaks cpufreq too

[Rafael J. Wysocki]

On 9/12/2013 2:39 AM, Viresh Kumar wrote:
> On 11 September 2013 18:48, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>> On Wednesday, September 11, 2013 02:08:44 PM Viresh Kumar wrote:
>>> That looked like a straight forward issue/bug to me and so I haven't
>>> gotten deep into it..
>> Which you should always do when you're going to deal with concurrency issues.
>> Even if they appear to be obvious, they often are far from that, like in this
>> case.
> /me Nods
>
>>> Scenario 2:
>>> --------------
>>> Governor is changing freq and has called __cpufreq_driver_target().
>>> At the same time we are changing scaling_{min|max}_freq from
>>> sysfs, which would eventually end up calling governors:
>>> CPUFREQ_GOV_LIMITS notification, that will also call:
>>> __cpufreq_driver_target()..
>>>
>>> So, we eventually have two concurrent calls to ->target() and we
>>> don't really know how hardware will behave in this case.. Most of
>>> the implementations of ->target() routines just go and change
>>> freq/voltage without checking if we are already in progress of doing
>>> that (i.e. based on expectation that this call is not re entrant)..
>>>
>>> Now anything can happen at hardware level, which I don't have
>>> all insight of :(
>> That is more theoretical, however.
> Maybe we can get more deeper into it then :)
> Platform have something like this in their target()

Which platform?

> A. If new freq is more than old: Increase voltage
> B. Change freq
> C. If new freq is less than old: decrease voltage
>
> Now, two concurrent calls to target are X and Y, where X is trying to increase
> freq and Y is trying to decrease it..
>
> And this is the sequence that followed due to races..
>
> X.A: voltage increased for larger freq
> Y.A: nothing happened here
> Y.B: freq decreased
> Y.C: voltage decreased
> X.B: freq increased
> X.C: nothing happened..
>
> We ended up setting a freq which is not supported by the voltage we have
> set.. That will probably make clock to CPU unstable and system wouldn't
> be workable anymore...
>
> And so I think even this case must also get some space in the changelog :)

Yes, if you can point to a specific driver having this problem.

Thanks,
Rafael

--------------------------------------------------------------------

Intel Technology Poland sp. z o.o.
ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wydzial Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-316 | Kapital zakladowy 200.000 PLN.

Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata i moze zawierac informacje poufne. W razie przypadkowego otrzymania tej wiadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; jakiekolwiek przegladanie lub rozpowszechnianie jest zabronione.
This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by others is strictly prohibited.

Re: "cpufreq: fix serialization issues with freq change notifiers" breaks cpufreq too

[Rafael J. Wysocki]

On 9/12/2013 2:39 AM, Viresh Kumar wrote:
> On 11 September 2013 18:48, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>> On Wednesday, September 11, 2013 02:08:44 PM Viresh Kumar wrote:
>>> That looked like a straight forward issue/bug to me and so I haven't
>>> gotten deep into it..
>> Which you should always do when you're going to deal with concurrency issues.
>> Even if they appear to be obvious, they often are far from that, like in this
>> case.
> /me Nods
>
>>> Scenario 2:
>>> --------------
>>> Governor is changing freq and has called __cpufreq_driver_target().
>>> At the same time we are changing scaling_{min|max}_freq from
>>> sysfs, which would eventually end up calling governors:
>>> CPUFREQ_GOV_LIMITS notification, that will also call:
>>> __cpufreq_driver_target()..
>>>
>>> So, we eventually have two concurrent calls to ->target() and we
>>> don't really know how hardware will behave in this case.. Most of
>>> the implementations of ->target() routines just go and change
>>> freq/voltage without checking if we are already in progress of doing
>>> that (i.e. based on expectation that this call is not re entrant)..
>>>
>>> Now anything can happen at hardware level, which I don't have
>>> all insight of :(
>> That is more theoretical, however.
> Maybe we can get more deeper into it then :)
> Platform have something like this in their target()

Which platform?

> A. If new freq is more than old: Increase voltage
> B. Change freq
> C. If new freq is less than old: decrease voltage
>
> Now, two concurrent calls to target are X and Y, where X is trying to increase
> freq and Y is trying to decrease it..
>
> And this is the sequence that followed due to races..
>
> X.A: voltage increased for larger freq
> Y.A: nothing happened here
> Y.B: freq decreased
> Y.C: voltage decreased
> X.B: freq increased
> X.C: nothing happened..
>
> We ended up setting a freq which is not supported by the voltage we have
> set.. That will probably make clock to CPU unstable and system wouldn't
> be workable anymore...
>
> And so I think even this case must also get some space in the changelog :)

Yes, if you can point to a specific driver having this problem.

Thanks,
Rafael

--------------------------------------------------------------------

Intel Technology Poland sp. z o.o.
ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wydzial Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-316 | Kapital zakladowy 200.000 PLN.

Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata i moze zawierac informacje poufne. W razie przypadkowego otrzymania tej wiadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; jakiekolwiek przegladanie lub rozpowszechnianie jest zabronione.
This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by others is strictly prohibited.

Re: "cpufreq: fix serialization issues with freq change notifiers" breaks cpufreq too

[Rafael J. Wysocki]

On 9/12/2013 2:39 AM, Viresh Kumar wrote:
> On 11 September 2013 18:48, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>> On Wednesday, September 11, 2013 02:08:44 PM Viresh Kumar wrote:
>>> That looked like a straight forward issue/bug to me and so I haven't
>>> gotten deep into it..
>> Which you should always do when you're going to deal with concurrency issues.
>> Even if they appear to be obvious, they often are far from that, like in this
>> case.
> /me Nods
>
>>> Scenario 2:
>>> --------------
>>> Governor is changing freq and has called __cpufreq_driver_target().
>>> At the same time we are changing scaling_{min|max}_freq from
>>> sysfs, which would eventually end up calling governors:
>>> CPUFREQ_GOV_LIMITS notification, that will also call:
>>> __cpufreq_driver_target()..
>>>
>>> So, we eventually have two concurrent calls to ->target() and we
>>> don't really know how hardware will behave in this case.. Most of
>>> the implementations of ->target() routines just go and change
>>> freq/voltage without checking if we are already in progress of doing
>>> that (i.e. based on expectation that this call is not re entrant)..
>>>
>>> Now anything can happen at hardware level, which I don't have
>>> all insight of :(
>> That is more theoretical, however.
> Maybe we can get more deeper into it then :)
> Platform have something like this in their target()

Which platform?

> A. If new freq is more than old: Increase voltage
> B. Change freq
> C. If new freq is less than old: decrease voltage
>
> Now, two concurrent calls to target are X and Y, where X is trying to increase
> freq and Y is trying to decrease it..
>
> And this is the sequence that followed due to races..
>
> X.A: voltage increased for larger freq
> Y.A: nothing happened here
> Y.B: freq decreased
> Y.C: voltage decreased
> X.B: freq increased
> X.C: nothing happened..
>
> We ended up setting a freq which is not supported by the voltage we have
> set.. That will probably make clock to CPU unstable and system wouldn't
> be workable anymore...
>
> And so I think even this case must also get some space in the changelog :)

Yes, if you can point to a specific driver having this problem.

Thanks,
Rafael

--------------------------------------------------------------------

Intel Technology Poland sp. z o.o.
ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wydzial Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-316 | Kapital zakladowy 200.000 PLN.

Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata i moze zawierac informacje poufne. W razie przypadkowego otrzymania tej wiadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; jakiekolwiek przegladanie lub rozpowszechnianie jest zabronione.
This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by others is strictly prohibited.

"cpufreq: fix serialization issues with freq change notifiers" breaks cpufreq too

[Rafael J. Wysocki]

On 9/12/2013 2:39 AM, Viresh Kumar wrote:
> On 11 September 2013 18:48, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>> On Wednesday, September 11, 2013 02:08:44 PM Viresh Kumar wrote:
>>> That looked like a straight forward issue/bug to me and so I haven't
>>> gotten deep into it..
>> Which you should always do when you're going to deal with concurrency issues.
>> Even if they appear to be obvious, they often are far from that, like in this
>> case.
> /me Nods
>
>>> Scenario 2:
>>> --------------
>>> Governor is changing freq and has called __cpufreq_driver_target().
>>> At the same time we are changing scaling_{min|max}_freq from
>>> sysfs, which would eventually end up calling governors:
>>> CPUFREQ_GOV_LIMITS notification, that will also call:
>>> __cpufreq_driver_target()..
>>>
>>> So, we eventually have two concurrent calls to ->target() and we
>>> don't really know how hardware will behave in this case.. Most of
>>> the implementations of ->target() routines just go and change
>>> freq/voltage without checking if we are already in progress of doing
>>> that (i.e. based on expectation that this call is not re entrant)..
>>>
>>> Now anything can happen at hardware level, which I don't have
>>> all insight of :(
>> That is more theoretical, however.
> Maybe we can get more deeper into it then :)
> Platform have something like this in their target()

Which platform?

> A. If new freq is more than old: Increase voltage
> B. Change freq
> C. If new freq is less than old: decrease voltage
>
> Now, two concurrent calls to target are X and Y, where X is trying to increase
> freq and Y is trying to decrease it..
>
> And this is the sequence that followed due to races..
>
> X.A: voltage increased for larger freq
> Y.A: nothing happened here
> Y.B: freq decreased
> Y.C: voltage decreased
> X.B: freq increased
> X.C: nothing happened..
>
> We ended up setting a freq which is not supported by the voltage we have
> set.. That will probably make clock to CPU unstable and system wouldn't
> be workable anymore...
>
> And so I think even this case must also get some space in the changelog :)

Yes, if you can point to a specific driver having this problem.

Thanks,
Rafael

--------------------------------------------------------------------

Intel Technology Poland sp. z o.o.
ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wydzial Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-316 | Kapital zakladowy 200.000 PLN.

Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata i moze zawierac informacje poufne. W razie przypadkowego otrzymania tej wiadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; jakiekolwiek przegladanie lub rozpowszechnianie jest zabronione.
This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by others is strictly prohibited.

Re: "cpufreq: fix serialization issues with freq change notifiers" breaks cpufreq too

[Viresh Kumar]

On 11 September 2013 18:48, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Wednesday, September 11, 2013 02:08:44 PM Viresh Kumar wrote:

>> That looked like a straight forward issue/bug to me and so I haven't
>> gotten deep into it..
>
> Which you should always do when you're going to deal with concurrency issues.
> Even if they appear to be obvious, they often are far from that, like in this
> case.

/me Nods

>> Scenario 2:
>> --------------
>> Governor is changing freq and has called __cpufreq_driver_target().
>> At the same time we are changing scaling_{min|max}_freq from
>> sysfs, which would eventually end up calling governors:
>> CPUFREQ_GOV_LIMITS notification, that will also call:
>> __cpufreq_driver_target()..
>>
>> So, we eventually have two concurrent calls to ->target() and we
>> don't really know how hardware will behave in this case.. Most of
>> the implementations of ->target() routines just go and change
>> freq/voltage without checking if we are already in progress of doing
>> that (i.e. based on expectation that this call is not re entrant)..
>>
>> Now anything can happen at hardware level, which I don't have
>> all insight of :(
>
> That is more theoretical, however.

Maybe we can get more deeper into it then :)
Platform have something like this in their target()

A. If new freq is more than old: Increase voltage
B. Change freq
C. If new freq is less than old: decrease voltage

Now, two concurrent calls to target are X and Y, where X is trying to increase
freq and Y is trying to decrease it..

And this is the sequence that followed due to races..

X.A: voltage increased for larger freq
Y.A: nothing happened here
Y.B: freq decreased
Y.C: voltage decreased
X.B: freq increased
X.C: nothing happened..

We ended up setting a freq which is not supported by the voltage we have
set.. That will probably make clock to CPU unstable and system wouldn't
be workable anymore...

And so I think even this case must also get some space in the changelog :)

Re: "cpufreq: fix serialization issues with freq change notifiers" breaks cpufreq too

[Viresh Kumar]

On 12 September 2013 06:13, Rafael J. Wysocki
<rafael.j.wysocki@intel.com> wrote:
> Yes, if you can point to a specific driver having this problem.

There are so many of those (I know it because I went through almost all drivers
recently with my cleanup series): cpufreq-cpu0, omap-cpufreq,
exynos-cpufreq, etc..

They all do this:

A. If new freq is more than old: Increase voltage
B. Change freq
C. If new freq is less than old: decrease voltage

Re: "cpufreq: fix serialization issues with freq change notifiers" breaks cpufreq too

[Viresh Kumar]

On 12 September 2013 06:13, Rafael J. Wysocki
<rafael.j.wysocki@intel.com> wrote:
> Yes, if you can point to a specific driver having this problem.

There are so many of those (I know it because I went through almost all drivers
recently with my cleanup series): cpufreq-cpu0, omap-cpufreq,
exynos-cpufreq, etc..

They all do this:

A. If new freq is more than old: Increase voltage
B. Change freq
C. If new freq is less than old: decrease voltage

"cpufreq: fix serialization issues with freq change notifiers" breaks cpufreq too

[Viresh Kumar]

On 12 September 2013 06:13, Rafael J. Wysocki
<rafael.j.wysocki@intel.com> wrote:
> Yes, if you can point to a specific driver having this problem.

There are so many of those (I know it because I went through almost all drivers
recently with my cleanup series): cpufreq-cpu0, omap-cpufreq,
exynos-cpufreq, etc..

They all do this:

A. If new freq is more than old: Increase voltage
B. Change freq
C. If new freq is less than old: decrease voltage

Re: "cpufreq: fix serialization issues with freq change notifiers" breaks cpufreq too

[Viresh Kumar]

On 12 September 2013 06:13, Rafael J. Wysocki
<rafael.j.wysocki@intel.com> wrote:
> Yes, if you can point to a specific driver having this problem.

There are so many of those (I know it because I went through almost all drivers
recently with my cleanup series): cpufreq-cpu0, omap-cpufreq,
exynos-cpufreq, etc..

They all do this:

A. If new freq is more than old: Increase voltage
B. Change freq
C. If new freq is less than old: decrease voltage

Re: "cpufreq: fix serialization issues with freq change notifiers" breaks cpufreq too

[Guennadi Liakhovetski]

Hi Viresh

On Wed, 11 Sep 2013, Viresh Kumar wrote:

> On 10 September 2013 22:37, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> > On Tue, 10 Sep 2013, Viresh Kumar wrote:
> >> Quite straight forward actually..
> >
> > Apparently, not quite.
> 
> I overlooked the situation where we return early from ->target() routines.. :(
> 
> Please try attached patches, I will repost them later (once I am able to
> convince Rafael that these are really important :) )

Yes, they seem to fix my issues. You probably aren't going to submit them 
in this form, instead, merge them with the original serialisation fix 
patch, right? So, you don't need my tested-by here. But feel free to cc me 
when you submit a fixed version.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: "cpufreq: fix serialization issues with freq change notifiers" breaks cpufreq too

[Guennadi Liakhovetski]

Hi Viresh

On Wed, 11 Sep 2013, Viresh Kumar wrote:

> On 10 September 2013 22:37, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> > On Tue, 10 Sep 2013, Viresh Kumar wrote:
> >> Quite straight forward actually..
> >
> > Apparently, not quite.
> 
> I overlooked the situation where we return early from ->target() routines.. :(
> 
> Please try attached patches, I will repost them later (once I am able to
> convince Rafael that these are really important :) )

Yes, they seem to fix my issues. You probably aren't going to submit them 
in this form, instead, merge them with the original serialisation fix 
patch, right? So, you don't need my tested-by here. But feel free to cc me 
when you submit a fixed version.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: "cpufreq: fix serialization issues with freq change notifiers" breaks cpufreq too

[Guennadi Liakhovetski]

Hi Viresh

On Wed, 11 Sep 2013, Viresh Kumar wrote:

> On 10 September 2013 22:37, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> > On Tue, 10 Sep 2013, Viresh Kumar wrote:
> >> Quite straight forward actually..
> >
> > Apparently, not quite.
> 
> I overlooked the situation where we return early from ->target() routines.. :(
> 
> Please try attached patches, I will repost them later (once I am able to
> convince Rafael that these are really important :) )

Yes, they seem to fix my issues. You probably aren't going to submit them 
in this form, instead, merge them with the original serialisation fix 
patch, right? So, you don't need my tested-by here. But feel free to cc me 
when you submit a fixed version.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

"cpufreq: fix serialization issues with freq change notifiers" breaks cpufreq too

[Guennadi Liakhovetski]

Hi Viresh

On Wed, 11 Sep 2013, Viresh Kumar wrote:

> On 10 September 2013 22:37, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> > On Tue, 10 Sep 2013, Viresh Kumar wrote:
> >> Quite straight forward actually..
> >
> > Apparently, not quite.
> 
> I overlooked the situation where we return early from ->target() routines.. :(
> 
> Please try attached patches, I will repost them later (once I am able to
> convince Rafael that these are really important :) )

Yes, they seem to fix my issues. You probably aren't going to submit them 
in this form, instead, merge them with the original serialisation fix 
patch, right? So, you don't need my tested-by here. But feel free to cc me 
when you submit a fixed version.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: "cpufreq: fix serialization issues with freq change notifiers" breaks cpufreq too

[Viresh Kumar]

On 12 September 2013 13:17, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> Yes, they seem to fix my issues.

Great!!

> You probably aren't going to submit them
> in this form, instead, merge them with the original serialisation fix
> patch, right? So, you don't need my tested-by here. But feel free to cc me
> when you submit a fixed version.

I will add your tested-by as you have eventually test all three patch that I
would merge :)

Thanks for testing this..

Re: "cpufreq: fix serialization issues with freq change notifiers" breaks cpufreq too

[Viresh Kumar]

On 12 September 2013 13:17, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> Yes, they seem to fix my issues.

Great!!

> You probably aren't going to submit them
> in this form, instead, merge them with the original serialisation fix
> patch, right? So, you don't need my tested-by here. But feel free to cc me
> when you submit a fixed version.

I will add your tested-by as you have eventually test all three patch that I
would merge :)

Thanks for testing this..

"cpufreq: fix serialization issues with freq change notifiers" breaks cpufreq too

[Viresh Kumar]

On 12 September 2013 13:17, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> Yes, they seem to fix my issues.

Great!!

> You probably aren't going to submit them
> in this form, instead, merge them with the original serialisation fix
> patch, right? So, you don't need my tested-by here. But feel free to cc me
> when you submit a fixed version.

I will add your tested-by as you have eventually test all three patch that I
would merge :)

Thanks for testing this..

Re: "cpufreq: fix serialization issues with freq change notifiers" breaks cpufreq too

[Viresh Kumar]

On 12 September 2013 13:17, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> Yes, they seem to fix my issues.

Great!!

> You probably aren't going to submit them
> in this form, instead, merge them with the original serialisation fix
> patch, right? So, you don't need my tested-by here. But feel free to cc me
> when you submit a fixed version.

I will add your tested-by as you have eventually test all three patch that I
would merge :)

Thanks for testing this..

Re: "cpufreq: fix serialization issues with freq change notifiers" breaks cpufreq too

[Rafael J. Wysocki]

On Thursday, September 12, 2013 11:06:14 AM Viresh Kumar wrote:
> On 12 September 2013 06:13, Rafael J. Wysocki
> <rafael.j.wysocki@intel.com> wrote:
> > Yes, if you can point to a specific driver having this problem.
> 
> There are so many of those (I know it because I went through almost all drivers
> recently with my cleanup series): cpufreq-cpu0, omap-cpufreq,
> exynos-cpufreq, etc..
> 
> They all do this:
> 
> A. If new freq is more than old: Increase voltage
> B. Change freq
> C. If new freq is less than old: decrease voltage

Well, if there's more than one it's even simpler.  You can just pick one. :-)

Like, "for example, <driver name> does the following ... which may cause the
hardware to misbehave, because ...".

Thanks,
Rafael

Re: "cpufreq: fix serialization issues with freq change notifiers" breaks cpufreq too

[Viresh Kumar]

On 12 September 2013 16:31, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> Well, if there's more than one it's even simpler.  You can just pick one. :-)
>
> Like, "for example, <driver name> does the following ... which may cause the
> hardware to misbehave, because ...".

:)

I have written one of my longest changelog ever on the resent of this patch..
Hope that doesn't have any unanswered stuff left :)

Re: "cpufreq: fix serialization issues with freq change notifiers" breaks cpufreq too

[Viresh Kumar]

On 12 September 2013 16:31, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> Well, if there's more than one it's even simpler.  You can just pick one. :-)
>
> Like, "for example, <driver name> does the following ... which may cause the
> hardware to misbehave, because ...".

:)

I have written one of my longest changelog ever on the resent of this patch..
Hope that doesn't have any unanswered stuff left :)

"cpufreq: fix serialization issues with freq change notifiers" breaks cpufreq too

[Viresh Kumar]

On 12 September 2013 16:31, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> Well, if there's more than one it's even simpler.  You can just pick one. :-)
>
> Like, "for example, <driver name> does the following ... which may cause the
> hardware to misbehave, because ...".

:)

I have written one of my longest changelog ever on the resent of this patch..
Hope that doesn't have any unanswered stuff left :)

Re: "cpufreq: fix serialization issues with freq change notifiers" breaks cpufreq too

[Viresh Kumar]

On 12 September 2013 16:31, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> Well, if there's more than one it's even simpler.  You can just pick one. :-)
>
> Like, "for example, <driver name> does the following ... which may cause the
> hardware to misbehave, because ...".

:)

I have written one of my longest changelog ever on the resent of this patch..
Hope that doesn't have any unanswered stuff left :)

Re: "cpufreq: fix serialization issues with freq change notifiers" breaks cpufreq too

[Rafael J. Wysocki]

On Thursday, September 12, 2013 11:06:14 AM Viresh Kumar wrote:
> On 12 September 2013 06:13, Rafael J. Wysocki
> <rafael.j.wysocki@intel.com> wrote:
> > Yes, if you can point to a specific driver having this problem.
> 
> There are so many of those (I know it because I went through almost all drivers
> recently with my cleanup series): cpufreq-cpu0, omap-cpufreq,
> exynos-cpufreq, etc..
> 
> They all do this:
> 
> A. If new freq is more than old: Increase voltage
> B. Change freq
> C. If new freq is less than old: decrease voltage

Well, if there's more than one it's even simpler.  You can just pick one. :-)

Like, "for example, <driver name> does the following ... which may cause the
hardware to misbehave, because ...".

Thanks,
Rafael

Re: "cpufreq: fix serialization issues with freq change notifiers" breaks cpufreq too

[Rafael J. Wysocki]

On Thursday, September 12, 2013 11:06:14 AM Viresh Kumar wrote:
> On 12 September 2013 06:13, Rafael J. Wysocki
> <rafael.j.wysocki@intel.com> wrote:
> > Yes, if you can point to a specific driver having this problem.
> 
> There are so many of those (I know it because I went through almost all drivers
> recently with my cleanup series): cpufreq-cpu0, omap-cpufreq,
> exynos-cpufreq, etc..
> 
> They all do this:
> 
> A. If new freq is more than old: Increase voltage
> B. Change freq
> C. If new freq is less than old: decrease voltage

Well, if there's more than one it's even simpler.  You can just pick one. :-)

Like, "for example, <driver name> does the following ... which may cause the
hardware to misbehave, because ...".

Thanks,
Rafael

"cpufreq: fix serialization issues with freq change notifiers" breaks cpufreq too

[Rafael J. Wysocki]

On Thursday, September 12, 2013 11:06:14 AM Viresh Kumar wrote:
> On 12 September 2013 06:13, Rafael J. Wysocki
> <rafael.j.wysocki@intel.com> wrote:
> > Yes, if you can point to a specific driver having this problem.
> 
> There are so many of those (I know it because I went through almost all drivers
> recently with my cleanup series): cpufreq-cpu0, omap-cpufreq,
> exynos-cpufreq, etc..
> 
> They all do this:
> 
> A. If new freq is more than old: Increase voltage
> B. Change freq
> C. If new freq is less than old: decrease voltage

Well, if there's more than one it's even simpler.  You can just pick one. :-)

Like, "for example, <driver name> does the following ... which may cause the
hardware to misbehave, because ...".

Thanks,
Rafael