Re: [PATCH] thermal: sysfs: fall back to vzalloc for cooling device's statistics

[Yue Hu]

On Thu, 27 Aug 2020 11:56:46 +0530
Viresh Kumar  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(>dev), 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.

Re: [PATCH] thermal: sysfs: fall back to vzalloc for cooling device's statistics

[Viresh Kumar]

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(>dev), 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

Re: [PATCH] thermal: sysfs: fall back to vzalloc for cooling device's statistics

[Yue Hu]

On Thu, 27 Aug 2020 10:44:01 +0530
Viresh Kumar  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(>dev), 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.

Re: [PATCH] thermal: sysfs: fall back to vzalloc for cooling device's statistics

[Viresh Kumar]

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

Re: [PATCH] thermal: sysfs: fall back to vzalloc for cooling device's statistics

[Yue Hu]

On Wed, 26 Aug 2020 11:19:02 +0200
Daniel Lezcano  wrote:

> Hi Yue,
> 
> On 26/08/2020 04:13, Yue Hu wrote:
> > On Mon, 24 Aug 2020 12:40:35 +0200
> > Daniel Lezcano  wrote:
> >   
> >> On 18/08/2020 08:30, Yue Hu wrote:  
> >>> From: Yue Hu 
> >>>
> >>> 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.

> 
> 
> 
> 

Re: [PATCH] thermal: sysfs: fall back to vzalloc for cooling device's statistics

[Daniel Lezcano]

Hi Yue,

On 26/08/2020 04:13, Yue Hu wrote:
> On Mon, 24 Aug 2020 12:40:35 +0200
> Daniel Lezcano  wrote:
> 
>> On 18/08/2020 08:30, Yue Hu wrote:
>>> From: Yue Hu 
>>>
>>> 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 ?




-- 
 Linaro.org │ Open source software for ARM SoCs

Follow Linaro:   Facebook |
 Twitter |
 Blog

Re: [PATCH] thermal: sysfs: fall back to vzalloc for cooling device's statistics

[Yue Hu]

On Mon, 24 Aug 2020 12:40:35 +0200
Daniel Lezcano  wrote:

> On 18/08/2020 08:30, Yue Hu wrote:
> > From: Yue Hu 
> > 
> > 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 
> > ---
> >  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 
> >  #include 
> >  #include 
> > +#include 
> > +#include 
> >  #include 
> >  #include 
> >  
> > @@ -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;
> >  }
> >  
> >   
> 
> 

Re: [PATCH] thermal: sysfs: fall back to vzalloc for cooling device's statistics

[Daniel Lezcano]

On 18/08/2020 08:30, Yue Hu wrote:
> From: Yue Hu 
> 
> 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 
> ---
>  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 
>  #include 
>  #include 
> +#include 
> +#include 
>  #include 
>  #include 
>  
> @@ -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;
>  }
>  
> 


-- 
 Linaro.org │ Open source software for ARM SoCs

Follow Linaro:   Facebook |
 Twitter |
 Blog

Re: [PATCH] thermal: sysfs: fall back to vzalloc for cooling device's statistics

[Yue Hu]

On Wed, 19 Aug 2020 16:47:01 +0530
Amit Kucheria  wrote:

> On Tue, Aug 18, 2020 at 12:00 PM Yue Hu  wrote:
> >
> > From: Yue Hu 
> >
> > 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 
> > ---
> >  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 
> >  #include 
> >  #include 
> > +#include 
> > +#include 
> >  #include 
> >  #include 
> >
> > @@ -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
> >  

Re: [PATCH] thermal: sysfs: fall back to vzalloc for cooling device's statistics

[Amit Kucheria]

On Tue, Aug 18, 2020 at 12:00 PM Yue Hu  wrote:
>
> From: Yue Hu 
>
> 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 
> ---
>  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 
>  #include 
>  #include 
> +#include 
> +#include 
>  #include 
>  #include 
>
> @@ -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
>

Re: [PATCH] thermal: sysfs: fall back to vzalloc for cooling device's statistics

[Amit Kucheria]

On Tue, Aug 18, 2020 at 12:00 PM Yue Hu  wrote:
>
> From: Yue Hu 
>
> 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 
> ---
>  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 
>  #include 
>  #include 
> +#include 
> +#include 
>  #include 
>  #include 
>
> @@ -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
>

[PATCH] thermal: sysfs: fall back to vzalloc for cooling device's statistics

[Yue Hu]

From: Yue Hu 

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 
---
 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 
 #include 
 #include 
+#include 
+#include 
 #include 
 #include 
 
@@ -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