linux-pm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] thermal: sysfs: fall back to vzalloc for cooling device's statistics
@ 2020-08-18  6:30 Yue Hu
  2020-08-19 11:05 ` Amit Kucheria
                   ` (2 more replies)
  0 siblings, 3 replies; 12+ messages in thread
From: Yue Hu @ 2020-08-18  6:30 UTC (permalink / raw)
  To: rui.zhang, daniel.lezcano, amit.kucheria, viresh.kumar
  Cc: linux-pm, linux-kernel, huyue2, zbestahu

From: Yue Hu <huyue2@yulong.com>

We observed warning about kzalloc() when register thermal cooling device
in backlight_device_register(). backlight display can be a cooling device
since reducing screen brightness will can help reduce temperature.

However, ->get_max_state of backlight will assign max brightness of 1024
to states. The memory size can be getting 1MB+ due to states * states.
That is so large to trigger kmalloc() warning.

So, let's remove it and try vzalloc() if kzalloc() fails.

Signed-off-by: Yue Hu <huyue2@yulong.com>
---
 drivers/thermal/thermal_sysfs.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c
index aa99edb..9bae0b6 100644
--- a/drivers/thermal/thermal_sysfs.c
+++ b/drivers/thermal/thermal_sysfs.c
@@ -16,6 +16,8 @@
 #include <linux/device.h>
 #include <linux/err.h>
 #include <linux/slab.h>
+#include <linux/vmalloc.h>
+#include <linux/mm.h>
 #include <linux/string.h>
 #include <linux/jiffies.h>
 
@@ -919,7 +921,9 @@ static void cooling_device_stats_setup(struct thermal_cooling_device *cdev)
 	var += sizeof(*stats->time_in_state) * states;
 	var += sizeof(*stats->trans_table) * states * states;
 
-	stats = kzalloc(var, GFP_KERNEL);
+	stats = kzalloc(var, GFP_KERNEL | __GFP_NOWARN);
+	if (!stats)
+		stats = vzalloc(var);
 	if (!stats)
 		return;
 
@@ -938,7 +942,7 @@ static void cooling_device_stats_setup(struct thermal_cooling_device *cdev)
 
 static void cooling_device_stats_destroy(struct thermal_cooling_device *cdev)
 {
-	kfree(cdev->stats);
+	kvfree(cdev->stats);
 	cdev->stats = NULL;
 }
 
-- 
1.9.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH] thermal: sysfs: fall back to vzalloc for cooling device's statistics
  2020-08-18  6:30 [PATCH] thermal: sysfs: fall back to vzalloc for cooling device's statistics Yue Hu
@ 2020-08-19 11:05 ` Amit Kucheria
  2020-08-19 11:17 ` Amit Kucheria
  2020-08-24 10:40 ` Daniel Lezcano
  2 siblings, 0 replies; 12+ messages in thread
From: Amit Kucheria @ 2020-08-19 11:05 UTC (permalink / raw)
  To: Yue Hu
  Cc: Zhang Rui, Daniel Lezcano, Viresh Kumar, Linux PM list, LKML,
	huyue2, zbestahu

On Tue, Aug 18, 2020 at 12:00 PM Yue Hu <zbestahu@gmail.com> wrote:
>
> From: Yue Hu <huyue2@yulong.com>
>
> We observed warning about kzalloc() when register thermal cooling device
> in backlight_device_register(). backlight display can be a cooling device
> since reducing screen brightness will can help reduce temperature.
>
> However, ->get_max_state of backlight will assign max brightness of 1024
> to states. The memory size can be getting 1MB+ due to states * states.
> That is so large to trigger kmalloc() warning.
>
> So, let's remove it and try vzalloc() if kzalloc() fails.

If we can do with vzalloc()'ed memory i.e. we don't need contiguous
physical memory, why even attempt kzalloc?

>
> Signed-off-by: Yue Hu <huyue2@yulong.com>
> ---
>  drivers/thermal/thermal_sysfs.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c
> index aa99edb..9bae0b6 100644
> --- a/drivers/thermal/thermal_sysfs.c
> +++ b/drivers/thermal/thermal_sysfs.c
> @@ -16,6 +16,8 @@
>  #include <linux/device.h>
>  #include <linux/err.h>
>  #include <linux/slab.h>
> +#include <linux/vmalloc.h>
> +#include <linux/mm.h>
>  #include <linux/string.h>
>  #include <linux/jiffies.h>
>
> @@ -919,7 +921,9 @@ static void cooling_device_stats_setup(struct thermal_cooling_device *cdev)
>         var += sizeof(*stats->time_in_state) * states;
>         var += sizeof(*stats->trans_table) * states * states;
>
> -       stats = kzalloc(var, GFP_KERNEL);
> +       stats = kzalloc(var, GFP_KERNEL | __GFP_NOWARN);
> +       if (!stats)
> +               stats = vzalloc(var);
>         if (!stats)
>                 return;
>
> @@ -938,7 +942,7 @@ static void cooling_device_stats_setup(struct thermal_cooling_device *cdev)
>
>  static void cooling_device_stats_destroy(struct thermal_cooling_device *cdev)
>  {
> -       kfree(cdev->stats);
> +       kvfree(cdev->stats);
>         cdev->stats = NULL;
>  }
>
> --
> 1.9.1
>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] thermal: sysfs: fall back to vzalloc for cooling device's statistics
  2020-08-18  6:30 [PATCH] thermal: sysfs: fall back to vzalloc for cooling device's statistics Yue Hu
  2020-08-19 11:05 ` Amit Kucheria
@ 2020-08-19 11:17 ` Amit Kucheria
  2020-08-20  2:57   ` Yue Hu
  2020-08-24 10:40 ` Daniel Lezcano
  2 siblings, 1 reply; 12+ messages in thread
From: Amit Kucheria @ 2020-08-19 11:17 UTC (permalink / raw)
  To: Yue Hu
  Cc: Zhang Rui, Daniel Lezcano, Viresh Kumar, Linux PM list, LKML,
	huyue2, zbestahu

On Tue, Aug 18, 2020 at 12:00 PM Yue Hu <zbestahu@gmail.com> wrote:
>
> From: Yue Hu <huyue2@yulong.com>
>
> We observed warning about kzalloc() when register thermal cooling device
> in backlight_device_register(). backlight display can be a cooling device
> since reducing screen brightness will can help reduce temperature.
>
> However, ->get_max_state of backlight will assign max brightness of 1024
> to states. The memory size can be getting 1MB+ due to states * states.
> That is so large to trigger kmalloc() warning.
>
> So, let's remove it and try vzalloc() if kzalloc() fails.
>
> Signed-off-by: Yue Hu <huyue2@yulong.com>
> ---
>  drivers/thermal/thermal_sysfs.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c
> index aa99edb..9bae0b6 100644
> --- a/drivers/thermal/thermal_sysfs.c
> +++ b/drivers/thermal/thermal_sysfs.c
> @@ -16,6 +16,8 @@
>  #include <linux/device.h>
>  #include <linux/err.h>
>  #include <linux/slab.h>
> +#include <linux/vmalloc.h>
> +#include <linux/mm.h>
>  #include <linux/string.h>
>  #include <linux/jiffies.h>
>
> @@ -919,7 +921,9 @@ static void cooling_device_stats_setup(struct thermal_cooling_device *cdev)
>         var += sizeof(*stats->time_in_state) * states;
>         var += sizeof(*stats->trans_table) * states * states;
>
> -       stats = kzalloc(var, GFP_KERNEL);
> +       stats = kzalloc(var, GFP_KERNEL | __GFP_NOWARN);
> +       if (!stats)
> +               stats = vzalloc(var);

Couldn't this be replaced by kvzalloc()?

>         if (!stats)
>                 return;
>
> @@ -938,7 +942,7 @@ static void cooling_device_stats_setup(struct thermal_cooling_device *cdev)
>
>  static void cooling_device_stats_destroy(struct thermal_cooling_device *cdev)
>  {
> -       kfree(cdev->stats);
> +       kvfree(cdev->stats);
>         cdev->stats = NULL;
>  }
>
> --
> 1.9.1
>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] thermal: sysfs: fall back to vzalloc for cooling device's statistics
  2020-08-19 11:17 ` Amit Kucheria
@ 2020-08-20  2:57   ` Yue Hu
  0 siblings, 0 replies; 12+ messages in thread
From: Yue Hu @ 2020-08-20  2:57 UTC (permalink / raw)
  To: Amit Kucheria
  Cc: Zhang Rui, Daniel Lezcano, Viresh Kumar, Linux PM list, LKML,
	huyue2, zbestahu

On Wed, 19 Aug 2020 16:47:01 +0530
Amit Kucheria <amitk@kernel.org> wrote:

> On Tue, Aug 18, 2020 at 12:00 PM Yue Hu <zbestahu@gmail.com> wrote:
> >
> > From: Yue Hu <huyue2@yulong.com>
> >
> > We observed warning about kzalloc() when register thermal cooling device
> > in backlight_device_register(). backlight display can be a cooling device
> > since reducing screen brightness will can help reduce temperature.
> >
> > However, ->get_max_state of backlight will assign max brightness of 1024
> > to states. The memory size can be getting 1MB+ due to states * states.
> > That is so large to trigger kmalloc() warning.
> >
> > So, let's remove it and try vzalloc() if kzalloc() fails.
> >
> > Signed-off-by: Yue Hu <huyue2@yulong.com>
> > ---
> >  drivers/thermal/thermal_sysfs.c | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c
> > index aa99edb..9bae0b6 100644
> > --- a/drivers/thermal/thermal_sysfs.c
> > +++ b/drivers/thermal/thermal_sysfs.c
> > @@ -16,6 +16,8 @@
> >  #include <linux/device.h>
> >  #include <linux/err.h>
> >  #include <linux/slab.h>
> > +#include <linux/vmalloc.h>
> > +#include <linux/mm.h>
> >  #include <linux/string.h>
> >  #include <linux/jiffies.h>
> >
> > @@ -919,7 +921,9 @@ static void cooling_device_stats_setup(struct thermal_cooling_device *cdev)
> >         var += sizeof(*stats->time_in_state) * states;
> >         var += sizeof(*stats->trans_table) * states * states;
> >
> > -       stats = kzalloc(var, GFP_KERNEL);
> > +       stats = kzalloc(var, GFP_KERNEL | __GFP_NOWARN);
> > +       if (!stats)
> > +               stats = vzalloc(var);  
> 
> Couldn't this be replaced by kvzalloc()?

Yes, it should be more better as kvzalloc has a vmalloc fallback.

Thx.

> 
> >         if (!stats)
> >                 return;
> >
> > @@ -938,7 +942,7 @@ static void cooling_device_stats_setup(struct thermal_cooling_device *cdev)
> >
> >  static void cooling_device_stats_destroy(struct thermal_cooling_device *cdev)
> >  {
> > -       kfree(cdev->stats);
> > +       kvfree(cdev->stats);
> >         cdev->stats = NULL;
> >  }
> >
> > --
> > 1.9.1
> >  


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] thermal: sysfs: fall back to vzalloc for cooling device's statistics
  2020-08-18  6:30 [PATCH] thermal: sysfs: fall back to vzalloc for cooling device's statistics Yue Hu
  2020-08-19 11:05 ` Amit Kucheria
  2020-08-19 11:17 ` Amit Kucheria
@ 2020-08-24 10:40 ` Daniel Lezcano
  2020-08-26  2:13   ` Yue Hu
  2 siblings, 1 reply; 12+ messages in thread
From: Daniel Lezcano @ 2020-08-24 10:40 UTC (permalink / raw)
  To: Yue Hu, rui.zhang, amit.kucheria, viresh.kumar
  Cc: linux-pm, linux-kernel, huyue2, zbestahu

On 18/08/2020 08:30, Yue Hu wrote:
> From: Yue Hu <huyue2@yulong.com>
> 
> We observed warning about kzalloc() when register thermal cooling device
> in backlight_device_register(). backlight display can be a cooling device
> since reducing screen brightness will can help reduce temperature.
> 
> However, ->get_max_state of backlight will assign max brightness of 1024
> to states. The memory size can be getting 1MB+ due to states * states.

What are the benefits of a 1024 states cooling device ? Is the
difference noticeable with a such small step ?


> That is so large to trigger kmalloc() warning.
> 
> So, let's remove it and try vzalloc() if kzalloc() fails.
> 
> Signed-off-by: Yue Hu <huyue2@yulong.com>
> ---
>  drivers/thermal/thermal_sysfs.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c
> index aa99edb..9bae0b6 100644
> --- a/drivers/thermal/thermal_sysfs.c
> +++ b/drivers/thermal/thermal_sysfs.c
> @@ -16,6 +16,8 @@
>  #include <linux/device.h>
>  #include <linux/err.h>
>  #include <linux/slab.h>
> +#include <linux/vmalloc.h>
> +#include <linux/mm.h>
>  #include <linux/string.h>
>  #include <linux/jiffies.h>
>  
> @@ -919,7 +921,9 @@ static void cooling_device_stats_setup(struct thermal_cooling_device *cdev)
>  	var += sizeof(*stats->time_in_state) * states;
>  	var += sizeof(*stats->trans_table) * states * states;
>  
> -	stats = kzalloc(var, GFP_KERNEL);
> +	stats = kzalloc(var, GFP_KERNEL | __GFP_NOWARN);
> +	if (!stats)
> +		stats = vzalloc(var);
>  	if (!stats)
>  		return;
>  
> @@ -938,7 +942,7 @@ static void cooling_device_stats_setup(struct thermal_cooling_device *cdev)
>  
>  static void cooling_device_stats_destroy(struct thermal_cooling_device *cdev)
>  {
> -	kfree(cdev->stats);
> +	kvfree(cdev->stats);
>  	cdev->stats = NULL;
>  }
>  
> 


-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] thermal: sysfs: fall back to vzalloc for cooling device's statistics
  2020-08-24 10:40 ` Daniel Lezcano
@ 2020-08-26  2:13   ` Yue Hu
  2020-08-26  9:19     ` Daniel Lezcano
  0 siblings, 1 reply; 12+ messages in thread
From: Yue Hu @ 2020-08-26  2:13 UTC (permalink / raw)
  To: Daniel Lezcano
  Cc: rui.zhang, amit.kucheria, viresh.kumar, linux-pm, linux-kernel,
	huyue2, zbestahu

On Mon, 24 Aug 2020 12:40:35 +0200
Daniel Lezcano <daniel.lezcano@linaro.org> wrote:

> On 18/08/2020 08:30, Yue Hu wrote:
> > From: Yue Hu <huyue2@yulong.com>
> > 
> > We observed warning about kzalloc() when register thermal cooling device
> > in backlight_device_register(). backlight display can be a cooling device
> > since reducing screen brightness will can help reduce temperature.
> > 
> > However, ->get_max_state of backlight will assign max brightness of 1024
> > to states. The memory size can be getting 1MB+ due to states * states.  
> 
> What are the benefits of a 1024 states cooling device ? Is the
> difference noticeable with a such small step ?

Okay, this issue is happened under MSM/Android platform. QCOM spmi wled driver
will define the max brightness. We needs to fix the issue to get thermal statistics.

Thx.

> 
> 
> > That is so large to trigger kmalloc() warning.
> > 
> > So, let's remove it and try vzalloc() if kzalloc() fails.
> > 
> > Signed-off-by: Yue Hu <huyue2@yulong.com>
> > ---
> >  drivers/thermal/thermal_sysfs.c | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c
> > index aa99edb..9bae0b6 100644
> > --- a/drivers/thermal/thermal_sysfs.c
> > +++ b/drivers/thermal/thermal_sysfs.c
> > @@ -16,6 +16,8 @@
> >  #include <linux/device.h>
> >  #include <linux/err.h>
> >  #include <linux/slab.h>
> > +#include <linux/vmalloc.h>
> > +#include <linux/mm.h>
> >  #include <linux/string.h>
> >  #include <linux/jiffies.h>
> >  
> > @@ -919,7 +921,9 @@ static void cooling_device_stats_setup(struct thermal_cooling_device *cdev)
> >  	var += sizeof(*stats->time_in_state) * states;
> >  	var += sizeof(*stats->trans_table) * states * states;
> >  
> > -	stats = kzalloc(var, GFP_KERNEL);
> > +	stats = kzalloc(var, GFP_KERNEL | __GFP_NOWARN);
> > +	if (!stats)
> > +		stats = vzalloc(var);
> >  	if (!stats)
> >  		return;
> >  
> > @@ -938,7 +942,7 @@ static void cooling_device_stats_setup(struct thermal_cooling_device *cdev)
> >  
> >  static void cooling_device_stats_destroy(struct thermal_cooling_device *cdev)
> >  {
> > -	kfree(cdev->stats);
> > +	kvfree(cdev->stats);
> >  	cdev->stats = NULL;
> >  }
> >  
> >   
> 
> 


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] thermal: sysfs: fall back to vzalloc for cooling device's statistics
  2020-08-26  2:13   ` Yue Hu
@ 2020-08-26  9:19     ` Daniel Lezcano
  2020-08-27  4:03       ` Yue Hu
  0 siblings, 1 reply; 12+ messages in thread
From: Daniel Lezcano @ 2020-08-26  9:19 UTC (permalink / raw)
  To: Yue Hu
  Cc: rui.zhang, amit.kucheria, viresh.kumar, linux-pm, linux-kernel,
	huyue2, zbestahu


Hi Yue,

On 26/08/2020 04:13, Yue Hu wrote:
> On Mon, 24 Aug 2020 12:40:35 +0200
> Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
> 
>> On 18/08/2020 08:30, Yue Hu wrote:
>>> From: Yue Hu <huyue2@yulong.com>
>>>
>>> We observed warning about kzalloc() when register thermal cooling device
>>> in backlight_device_register(). backlight display can be a cooling device
>>> since reducing screen brightness will can help reduce temperature.
>>>
>>> However, ->get_max_state of backlight will assign max brightness of 1024
>>> to states. The memory size can be getting 1MB+ due to states * states.  
>>
>> What are the benefits of a 1024 states cooling device ? Is the
>> difference noticeable with a such small step ?
> 
> Okay, this issue is happened under MSM/Android platform. QCOM spmi wled driver
> will define the max brightness. We needs to fix the issue to get thermal statistics.

Let me rephrase my questions:

Don't you think there is something wrong in creating a 1024 x 1024
matrix to show transitions ?

What is the benefit of such stats ?

What is the benefit of having a 1024 states cooling device ?




-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] thermal: sysfs: fall back to vzalloc for cooling device's statistics
  2020-08-26  9:19     ` Daniel Lezcano
@ 2020-08-27  4:03       ` Yue Hu
  2020-08-27  5:14         ` Viresh Kumar
  0 siblings, 1 reply; 12+ messages in thread
From: Yue Hu @ 2020-08-27  4:03 UTC (permalink / raw)
  To: Daniel Lezcano, viresh.kumar
  Cc: rui.zhang, amit.kucheria, linux-pm, linux-kernel, huyue2, zbestahu

On Wed, 26 Aug 2020 11:19:02 +0200
Daniel Lezcano <daniel.lezcano@linaro.org> wrote:

> Hi Yue,
> 
> On 26/08/2020 04:13, Yue Hu wrote:
> > On Mon, 24 Aug 2020 12:40:35 +0200
> > Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
> >   
> >> On 18/08/2020 08:30, Yue Hu wrote:  
> >>> From: Yue Hu <huyue2@yulong.com>
> >>>
> >>> We observed warning about kzalloc() when register thermal cooling device
> >>> in backlight_device_register(). backlight display can be a cooling device
> >>> since reducing screen brightness will can help reduce temperature.
> >>>
> >>> However, ->get_max_state of backlight will assign max brightness of 1024
> >>> to states. The memory size can be getting 1MB+ due to states * states.    
> >>
> >> What are the benefits of a 1024 states cooling device ? Is the
> >> difference noticeable with a such small step ?  
> > 
> > Okay, this issue is happened under MSM/Android platform. QCOM spmi wled driver
> > will define the max brightness. We needs to fix the issue to get thermal statistics.  
> 
> Let me rephrase my questions:
> 
> Don't you think there is something wrong in creating a 1024 x 1024
> matrix to show transitions ?
> 
> What is the benefit of such stats ?
> 
> What is the benefit of having a 1024 states cooling device ?

Hi Daniel,

Now, i'm just focus on removing the kernel warning based on current code logic.
Commit 8ea229511e06 (thermal: Add cooling device's statistics in sysfs) added
the thermal statistics by viresh and viresh gived the patch an acknowledgement
in anther mail thread. 

Hi viresh,

Could you review the patch again about the question above?

Thank you.

> 
> 
> 
> 


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] thermal: sysfs: fall back to vzalloc for cooling device's statistics
  2020-08-27  4:03       ` Yue Hu
@ 2020-08-27  5:14         ` Viresh Kumar
  2020-08-27  6:20           ` Yue Hu
  0 siblings, 1 reply; 12+ messages in thread
From: Viresh Kumar @ 2020-08-27  5:14 UTC (permalink / raw)
  To: Yue Hu, rafael.j.wysocki
  Cc: Daniel Lezcano, rui.zhang, amit.kucheria, linux-pm, linux-kernel,
	huyue2, zbestahu

On 27-08-20, 12:03, Yue Hu wrote:
> Hi Daniel,
> 
> Now, i'm just focus on removing the kernel warning based on current code logic.
> Commit 8ea229511e06 (thermal: Add cooling device's statistics in sysfs) added
> the thermal statistics by viresh and viresh gived the patch an acknowledgement
> in anther mail thread. 
> 
> Hi viresh,
> 
> Could you review the patch again about the question above?

Yeah, I Acked it but the questions raised by Daniel are very valid and must be
answered.

I understand that you only cared about fixing the warning, but maybe we need to
fix the driver and the warning will go away by itself. If you don't want to do
it, then someone who is responsible for the driver should do it.

Was it the acpi_video.c driver that you got the warning from ? I have added
Rafael to the email in case that driver needs getting fixed.

-- 
viresh

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] thermal: sysfs: fall back to vzalloc for cooling device's statistics
  2020-08-27  5:14         ` Viresh Kumar
@ 2020-08-27  6:20           ` Yue Hu
  2020-08-27  6:26             ` Viresh Kumar
  0 siblings, 1 reply; 12+ messages in thread
From: Yue Hu @ 2020-08-27  6:20 UTC (permalink / raw)
  To: Viresh Kumar
  Cc: rafael.j.wysocki, Daniel Lezcano, rui.zhang, amit.kucheria,
	linux-pm, linux-kernel, huyue2, zbestahu

On Thu, 27 Aug 2020 10:44:01 +0530
Viresh Kumar <viresh.kumar@linaro.org> wrote:

> On 27-08-20, 12:03, Yue Hu wrote:
> > Hi Daniel,
> > 
> > Now, i'm just focus on removing the kernel warning based on current code logic.
> > Commit 8ea229511e06 (thermal: Add cooling device's statistics in sysfs) added
> > the thermal statistics by viresh and viresh gived the patch an acknowledgement
> > in anther mail thread. 
> > 
> > Hi viresh,
> > 
> > Could you review the patch again about the question above?  
> 
> Yeah, I Acked it but the questions raised by Daniel are very valid and must be
> answered.

Yes, sure.

> 
> I understand that you only cared about fixing the warning, but maybe we need to
> fix the driver and the warning will go away by itself. If you don't want to do
> it, then someone who is responsible for the driver should do it.

Yes, maybe the patch is not totally correct. maybe the driver has issue. Let's
check the driver firstly.

> 
> Was it the acpi_video.c driver that you got the warning from ? I have added
> Rafael to the email in case that driver needs getting fixed.
> 

Currenly, drivers/video/backlight does not call thermal_of_cooling_device_register()
to register thermal cooling device. The issue happened in msm-4.19 kernel for
QCOM/Android platform. Backlight in msm-4.19 kernel will register thermal cooling
device as below:

+static int bd_cdev_get_max_brightness(struct thermal_cooling_device *cdev,
+                                 unsigned long *state)
+{
+ struct backlight_device *bd = (struct backlight_device *)cdev->devdata;
+
+ *state = bd->props.max_brightness;
+
+ return 0;
+}


+static struct thermal_cooling_device_ops bd_cdev_ops = {
+ .get_max_state = bd_cdev_get_max_brightness,

+static void backlight_cdev_register(struct device *parent,
+                             struct backlight_device *bd)
+{
+ if (of_find_property(parent->of_node, "#cooling-cells", NULL)) {
+         bd->cdev = thermal_of_cooling_device_register(parent->of_node,
+                         (char *)dev_name(&bd->dev), bd, &bd_cdev_ops);

And the bd->props.max_brightness is getting from video/backlight/qcom-wled.c. Maybe
the driver should not assign 1024 to states/max_brightness. I'm not sure about it.
So i consider to change memory allocation methord. That's the origin of the patch.

Thank you.


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] thermal: sysfs: fall back to vzalloc for cooling device's statistics
  2020-08-27  6:20           ` Yue Hu
@ 2020-08-27  6:26             ` Viresh Kumar
  2020-08-27  6:40               ` Yue Hu
  0 siblings, 1 reply; 12+ messages in thread
From: Viresh Kumar @ 2020-08-27  6:26 UTC (permalink / raw)
  To: Yue Hu
  Cc: rafael.j.wysocki, Daniel Lezcano, rui.zhang, amit.kucheria,
	linux-pm, linux-kernel, huyue2, zbestahu

On 27-08-20, 14:20, Yue Hu wrote:
> Currenly, drivers/video/backlight does not call thermal_of_cooling_device_register()
> to register thermal cooling device. The issue happened in msm-4.19 kernel for
> QCOM/Android platform. Backlight in msm-4.19 kernel will register thermal cooling
> device as below:
> 
> +static int bd_cdev_get_max_brightness(struct thermal_cooling_device *cdev,
> +                                 unsigned long *state)
> +{
> + struct backlight_device *bd = (struct backlight_device *)cdev->devdata;
> +
> + *state = bd->props.max_brightness;
> +
> + return 0;
> +}
> 
> 
> +static struct thermal_cooling_device_ops bd_cdev_ops = {
> + .get_max_state = bd_cdev_get_max_brightness,
> 
> +static void backlight_cdev_register(struct device *parent,
> +                             struct backlight_device *bd)
> +{
> + if (of_find_property(parent->of_node, "#cooling-cells", NULL)) {
> +         bd->cdev = thermal_of_cooling_device_register(parent->of_node,
> +                         (char *)dev_name(&bd->dev), bd, &bd_cdev_ops);
> 
> And the bd->props.max_brightness is getting from video/backlight/qcom-wled.c. Maybe
> the driver should not assign 1024 to states/max_brightness. I'm not sure about it.
> So i consider to change memory allocation methord. That's the origin of the patch.

Thanks for the details. So this is not about upstream tree, as a rule
we aren't going to make changes here for any downstream tree.

Now coming back to the downstream driver, I also don't see a point in
returning bd->props.max_brightness as the max number of states there.
Maybe have 10 states, each occupying bd->props.max_brightness/10
brightness and so you will end up with 10 states only. But yeah,
whatever downstream decides on that.

-- 
viresh

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] thermal: sysfs: fall back to vzalloc for cooling device's statistics
  2020-08-27  6:26             ` Viresh Kumar
@ 2020-08-27  6:40               ` Yue Hu
  0 siblings, 0 replies; 12+ messages in thread
From: Yue Hu @ 2020-08-27  6:40 UTC (permalink / raw)
  To: Viresh Kumar
  Cc: rafael.j.wysocki, Daniel Lezcano, rui.zhang, amit.kucheria,
	linux-pm, linux-kernel, huyue2, zbestahu

On Thu, 27 Aug 2020 11:56:46 +0530
Viresh Kumar <viresh.kumar@linaro.org> wrote:

> On 27-08-20, 14:20, Yue Hu wrote:
> > Currenly, drivers/video/backlight does not call thermal_of_cooling_device_register()
> > to register thermal cooling device. The issue happened in msm-4.19 kernel for
> > QCOM/Android platform. Backlight in msm-4.19 kernel will register thermal cooling
> > device as below:
> > 
> > +static int bd_cdev_get_max_brightness(struct thermal_cooling_device *cdev,
> > +                                 unsigned long *state)
> > +{
> > + struct backlight_device *bd = (struct backlight_device *)cdev->devdata;
> > +
> > + *state = bd->props.max_brightness;
> > +
> > + return 0;
> > +}
> > 
> > 
> > +static struct thermal_cooling_device_ops bd_cdev_ops = {
> > + .get_max_state = bd_cdev_get_max_brightness,
> > 
> > +static void backlight_cdev_register(struct device *parent,
> > +                             struct backlight_device *bd)
> > +{
> > + if (of_find_property(parent->of_node, "#cooling-cells", NULL)) {
> > +         bd->cdev = thermal_of_cooling_device_register(parent->of_node,
> > +                         (char *)dev_name(&bd->dev), bd, &bd_cdev_ops);
> > 
> > And the bd->props.max_brightness is getting from video/backlight/qcom-wled.c. Maybe
> > the driver should not assign 1024 to states/max_brightness. I'm not sure about it.
> > So i consider to change memory allocation methord. That's the origin of the patch.  
> 
> Thanks for the details. So this is not about upstream tree, as a rule
> we aren't going to make changes here for any downstream tree.

I at first thought it's a possible issue in upstream tree.

> 
> Now coming back to the downstream driver, I also don't see a point in
> returning bd->props.max_brightness as the max number of states there.
> Maybe have 10 states, each occupying bd->props.max_brightness/10
> brightness and so you will end up with 10 states only. But yeah,
> whatever downstream decides on that.
> 

Got it.

Thx.

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2020-08-27  6:40 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-18  6:30 [PATCH] thermal: sysfs: fall back to vzalloc for cooling device's statistics Yue Hu
2020-08-19 11:05 ` Amit Kucheria
2020-08-19 11:17 ` Amit Kucheria
2020-08-20  2:57   ` Yue Hu
2020-08-24 10:40 ` Daniel Lezcano
2020-08-26  2:13   ` Yue Hu
2020-08-26  9:19     ` Daniel Lezcano
2020-08-27  4:03       ` Yue Hu
2020-08-27  5:14         ` Viresh Kumar
2020-08-27  6:20           ` Yue Hu
2020-08-27  6:26             ` Viresh Kumar
2020-08-27  6:40               ` Yue Hu

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).