| Message ID | 20210121084902.672855-7-kai.heng.feng@canonical.com |
|---|---|
| State | New |
| Headers | show |
| Series | Prevent thermal shutdown during boot process | expand |
On 21.01.21 09:49, Kai-Heng Feng wrote: > From: Daniel Lezcano <daniel.lezcano@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@linaro.org> > Reviewed-by: Lukasz Luba <lukasz.luba@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@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. 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); > 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 *); > }; >
On Tue, Jan 26, 2021 at 4:36 PM Stefan Bader <stefan.bader@canonical.com> wrote: > > On 21.01.21 09:49, Kai-Heng Feng wrote: > > From: Daniel Lezcano <daniel.lezcano@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@linaro.org> > > Reviewed-by: Lukasz Luba <lukasz.luba@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@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. 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 *); > > }; > > > >
On 26.01.21 13:15, Kai-Heng Feng wrote: > On Tue, Jan 26, 2021 at 4:36 PM Stefan Bader <stefan.bader@canonical.com> wrote: >> >> On 21.01.21 09:49, Kai-Heng Feng wrote: >>> From: Daniel Lezcano <daniel.lezcano@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@linaro.org> >>> Reviewed-by: Lukasz Luba <lukasz.luba@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@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 *); >>> }; >>> >> >>
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 *); };