acerhdf thermal question

acerhdf thermal question

[Daniel Lezcano]

Hi,

the polling interval is specified and modified via a kernel module 
parameter [1]

The value is used to change the polling interval of the thermal zone, 
implying that is accessing the thermal zone device structure internals 
directly [2]

In real use case, is the interval changed at runtime? Or just when the 
module is loaded? If the latter, the interval can be passed to the 
thermal zone at init time without doing a polling change rate after the 
thermal zone started. In this case, we can remove the 
polling_delay_jiffies change in the code and fix the structure leakage 
in this driver.

Otherwise, I can add accessors to this field but I would like to prevent 
adding them if they are not really necessary.

Thanks
   -- D.

[1] 
https://git.kernel.org/pub/scm/linux/kernel/git/thermal/linux.git/tree/drivers/platform/x86/acerhdf.c?h=thermal/bleeding-edge#n792

[2] 
https://git.kernel.org/pub/scm/linux/kernel/git/thermal/linux.git/tree/drivers/platform/x86/acerhdf.c?h=thermal/bleeding-edge#n359

-- 
<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

Re: acerhdf thermal question

[Hans de Goede]

Hi Daniel,

On 2/16/23 09:57, Daniel Lezcano wrote:
> 
> Hi,
> 
> the polling interval is specified and modified via a kernel module parameter [1]
> 
> The value is used to change the polling interval of the thermal zone, implying that is accessing the thermal zone device structure internals directly [2]
> 
> In real use case, is the interval changed at runtime? Or just when the module is loaded? If the latter, the interval can be passed to the thermal zone at init time without doing a polling change rate after the thermal zone started. In this case, we can remove the polling_delay_jiffies change in the code and fix the structure leakage in this driver.

I believe this very likely only is used at module load-time.
So the changes you suggest are fine with me.

I have added Paul Gortmaker to the Cc, Paul is the last person
to have done any real (*) work on acerhfd AFAICT.

Paul any objections against making the acerhdf.interval parameter
something which only works when set at boot / module load time
and removing the ability to change it at runtime ?

Regards,

Hans


*) Real as in not related to keeping it aligned with the thermal
framework/core

Re: acerhdf thermal question

[Hans de Goede]

Hi,

On 2/16/23 10:08, Hans de Goede wrote:
> Hi Daniel,
> 
> On 2/16/23 09:57, Daniel Lezcano wrote:
>>
>> Hi,
>>
>> the polling interval is specified and modified via a kernel module parameter [1]
>>
>> The value is used to change the polling interval of the thermal zone, implying that is accessing the thermal zone device structure internals directly [2]
>>
>> In real use case, is the interval changed at runtime? Or just when the module is loaded? If the latter, the interval can be passed to the thermal zone at init time without doing a polling change rate after the thermal zone started. In this case, we can remove the polling_delay_jiffies change in the code and fix the structure leakage in this driver.
> 
> I believe this very likely only is used at module load-time.
> So the changes you suggest are fine with me.
> 
> I have added Paul Gortmaker to the Cc, Paul is the last person
> to have done any real (*) work on acerhfd AFAICT.
> 
> Paul any objections against making the acerhdf.interval parameter
> something which only works when set at boot / module load time
> and removing the ability to change it at runtime ?

p.s.

I see now that Peter Kaestle is also still active in acking / reviewing
acerhfd changes. So lets wait for Peter's input on this too.

Regards,

Hans


> *) Real as in not related to keeping it aligned with the thermal
> framework/core

Re: acerhdf thermal question

[Paul Gortmaker]

[Re: acerhdf thermal question] On 16/02/2023 (Thu 10:08) Hans de Goede wrote:

> Hi Daniel,
> 
> On 2/16/23 09:57, Daniel Lezcano wrote:
> > 
> > Hi,
> > 
> > the polling interval is specified and modified via a kernel module parameter [1]
> > 
> > The value is used to change the polling interval of the thermal zone, implying that is accessing the thermal zone device structure internals directly [2]
> > 
> > In real use case, is the interval changed at runtime? Or just when the module is loaded? If the latter, the interval can be passed to the thermal zone at init time without doing a polling change rate after the thermal zone started. In this case, we can remove the polling_delay_jiffies change in the code and fix the structure leakage in this driver.
> 
> I believe this very likely only is used at module load-time.
> So the changes you suggest are fine with me.
> 
> I have added Paul Gortmaker to the Cc, Paul is the last person
> to have done any real (*) work on acerhfd AFAICT.
> 
> Paul any objections against making the acerhdf.interval parameter
> something which only works when set at boot / module load time
> and removing the ability to change it at runtime ?

Not that I have any real authority, beyond "touched it last", but
that aside, I'd say that if it is blocking other subsystem cleanups,
then by all means make it load-time only.

It was already obsolete hardware when I was tinkering with it, and
to my surprise -- that was already five years ago!  There can't be a
large user base out there - and even less tinkering with poll delay.

Paul.
--

> 
> Regards,
> 
> Hans
> 
> 
> *) Real as in not related to keeping it aligned with the thermal
> framework/core
> 
> 

Re: acerhdf thermal question

[Peter Kästle]

Hello,

16. Februar 2023 12:46, "Paul Gortmaker" <paul.gortmaker@windriver.com> schrieb:

> [Re: acerhdf thermal question] On 16/02/2023 (Thu 10:08) Hans de Goede wrote:
> 
>> Hi Daniel,
>> 
>> On 2/16/23 09:57, Daniel Lezcano wrote:
>> 
>> Hi,
>> 
>> the polling interval is specified and modified via a kernel module parameter [1]
>> 
>> The value is used to change the polling interval of the thermal zone, implying that is accessing
>> the thermal zone device structure internals directly [2]
>> 
>> In real use case, is the interval changed at runtime? Or just when the module is loaded? If the
>> latter, the interval can be passed to the thermal zone at init time without doing a polling change
>> rate after the thermal zone started. In this case, we can remove the polling_delay_jiffies change
>> in the code and fix the structure leakage in this driver.
>> 
>> I believe this very likely only is used at module load-time.
>> So the changes you suggest are fine with me.
>> 
>> I have added Paul Gortmaker to the Cc, Paul is the last person
>> to have done any real (*) work on acerhfd AFAICT.
>> 
>> Paul any objections against making the acerhdf.interval parameter
>> something which only works when set at boot / module load time
>> and removing the ability to change it at runtime ?
> 
> Not that I have any real authority, beyond "touched it last", but
> that aside, I'd say that if it is blocking other subsystem cleanups,
> then by all means make it load-time only.
> 
> It was already obsolete hardware when I was tinkering with it, and
> to my surprise -- that was already five years ago! There can't be a
> large user base out there - and even less tinkering with poll delay.

At least I'm still using it :)  Setting the interval at module load-time is good enough.

-- 
Thanks,
--peter;