NACK: [G] [PATCH 5/7] thermal/core: Remove notify ops

[Stefan Bader]

On 26.01.21 13:15, Kai-Heng Feng wrote:
> On Tue, Jan 26, 2021 at 4:36 PM Stefan Bader <stefan.bader at canonical.com> wrote:
>>
>> On 21.01.21 09:49, Kai-Heng Feng wrote:
>>> From: Daniel Lezcano <daniel.lezcano at linaro.org>
>>>
>>> BugLink: https://bugs.launchpad.net/bugs/1906168
>>>
>>> With the removal of the notifys user in a previous patches, the ops is no
>>> longer needed, remove it.
>>>
>>> Signed-off-by: Daniel Lezcano <daniel.lezcano at linaro.org>
>>> Reviewed-by: Lukasz Luba <lukasz.luba at arm.com>
>>> Link: https://lore.kernel.org/r/20201210121514.25760-5-daniel.lezcano@linaro.org
>>> (cherry picked from commit 04f111130e9afa41c10d7bcec14e00e3be8b6977 linux-next)
>>> Signed-off-by: Kai-Heng Feng <kai.heng.feng at canonical.com>
>>> ---
>>
>> Removing interfaces like this in a backport sounds rather dangerous. Upstream
>> usually does this after fixing all drivers which make use of it and do not care
>> about anything else. For stable releases we have to care about 3rd party drivers
>> as well as potentially still having usage in the kernel. I suppose the latter
>> has to some degree be tested by test compiling it but there might be drivers not
>> enabled. Not that I saw any mention of a test compile.
> 
> I didn't think of the case of 3rd party module, thanks for pointing that out.
> 
>> Instead of removing the call completely, I would use something like this:
>>
>>         if ((!tz-ops->hot && !tz-ops->critical) && tz->ops->notify)
>>                 ts->ops->notify(tz, trip, trip_type);
> 
> Actually, it makes more sense to keep the code as is.
> So we can drop this one and merge the other patches.

Hm, if we drop this one but keep the others, won't those process critical events
twice? Oh probably not as they no longer set a notify hook. So yeah that should
be ok. Could you send a v2 set for G only (since the other places picked it up).
I think that would be simpler to follow.

Thanks.
> 
> Kai-Heng
> 
>>
>>>  drivers/thermal/thermal_core.c | 3 ---
>>>  include/linux/thermal.h        | 2 --
>>>  2 files changed, 5 deletions(-)
>>>
>>> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
>>> index aba84e1a4396..b53305b8d643 100644
>>> --- a/drivers/thermal/thermal_core.c
>>> +++ b/drivers/thermal/thermal_core.c
>>> @@ -401,9 +401,6 @@ static void handle_critical_trips(struct thermal_zone_device *tz,
>>>
>>>       trace_thermal_zone_trip(tz, trip, trip_type);
>>>
>>> -     if (tz->ops->notify)
>>> -             tz->ops->notify(tz, trip, trip_type);
>>> -
>>>       if (trip_type == THERMAL_TRIP_HOT && tz->ops->hot)
>>>               tz->ops->hot(tz);
>>>       else if (trip_type == THERMAL_TRIP_CRITICAL)
>>> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
>>> index e7989cec090a..5777a9e4ea54 100644
>>> --- a/include/linux/thermal.h
>>> +++ b/include/linux/thermal.h
>>> @@ -90,8 +90,6 @@ struct thermal_zone_device_ops {
>>>       int (*set_emul_temp) (struct thermal_zone_device *, int);
>>>       int (*get_trend) (struct thermal_zone_device *, int,
>>>                         enum thermal_trend *);
>>> -     int (*notify) (struct thermal_zone_device *, int,
>>> -                    enum thermal_trip_type);
>>>       void (*hot)(struct thermal_zone_device *);
>>>       void (*critical)(struct thermal_zone_device *);
>>>  };
>>>
>>
>>


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.ubuntu.com/archives/kernel-team/attachments/20210126/cfa0ad28/attachment.sig>