| Message ID | 20230811084523.1689671-1-rui.zhang@intel.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | thermal: intel: intel_soc_dts_iosf: Fix thermal_zone removal | expand |
On Fri, Aug 11, 2023 at 10:45 AM Zhang Rui <rui.zhang@intel.com> wrote: > > All of the existing callers of remove_dts_thermal_zone() pass a valid > pointer as the argument, so checking for the NULL pointer is redundant. True. > Plus, when calling remove_dts_thermal_zone() from > intel_soc_dts_iosf_init(), it is possible that > 1. dts->tzone is an error pointer, when the sensor fails to be > registered as a valid thermal zone > 2. dts->tzone is unregistered in add_dts_thermal_zone(), when some > failure occurs after thermal zone registered > In both cases, there is no need to unregister dts->tzone in > remove_dts_thermal_zone(). > > Clear dst->tzone when add_dts_thermal_zone() fails. And do thermal zone > removal in remove_dts_thermal_zone() only when dts->tzone is set. Well, I'm not sure. thermal_zone_device_unregister() will do nothing if the thermal zone is not really registered AFAICS and it is prudent to restore SOC_DTS_OFFSET_ENABLE on failure IMO. > Signed-off-by: Zhang Rui <rui.zhang@intel.com> > --- > drivers/thermal/intel/intel_soc_dts_iosf.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/thermal/intel/intel_soc_dts_iosf.c b/drivers/thermal/intel/intel_soc_dts_iosf.c > index 7a66d0f077b0..c5203ba8f0b9 100644 > --- a/drivers/thermal/intel/intel_soc_dts_iosf.c > +++ b/drivers/thermal/intel/intel_soc_dts_iosf.c > @@ -212,7 +212,7 @@ static int soc_dts_enable(int id) > > static void remove_dts_thermal_zone(struct intel_soc_dts_sensor_entry *dts) > { > - if (dts) { > + if (dts->tzone) { > iosf_mbi_write(BT_MBI_UNIT_PMC, MBI_REG_WRITE, > SOC_DTS_OFFSET_ENABLE, dts->store_status); > thermal_zone_device_unregister(dts->tzone); > @@ -277,6 +277,7 @@ static int add_dts_thermal_zone(int id, struct intel_soc_dts_sensor_entry *dts, > err_enable: > thermal_zone_device_unregister(dts->tzone); > err_ret: > + dts->tzone = NULL; > return ret; > } > > -- > 2.34.1 >
On Fri, 2023-08-11 at 18:55 +0200, Rafael J. Wysocki wrote: > On Fri, Aug 11, 2023 at 10:45 AM Zhang Rui <rui.zhang@intel.com> > wrote: > > > > All of the existing callers of remove_dts_thermal_zone() pass a > > valid > > pointer as the argument, so checking for the NULL pointer is > > redundant. > > True. > > > Plus, when calling remove_dts_thermal_zone() from > > intel_soc_dts_iosf_init(), it is possible that > > 1. dts->tzone is an error pointer, when the sensor fails to be > > registered as a valid thermal zone > > 2. dts->tzone is unregistered in add_dts_thermal_zone(), when some > > failure occurs after thermal zone registered > > In both cases, there is no need to unregister dts->tzone in > > remove_dts_thermal_zone(). > > > > Clear dst->tzone when add_dts_thermal_zone() fails. And do thermal > > zone > > removal in remove_dts_thermal_zone() only when dts->tzone is set. > > Well, I'm not sure. > > thermal_zone_device_unregister() will do nothing if the thermal zone > is not really registered AFAICS yes. > and it is prudent to restore > SOC_DTS_OFFSET_ENABLE on failure IMO. agreed. Refreshed patch sent. As this becomes really trivial, feel free to apply it or drop it. thanks, rui
diff --git a/drivers/thermal/intel/intel_soc_dts_iosf.c b/drivers/thermal/intel/intel_soc_dts_iosf.c index 7a66d0f077b0..c5203ba8f0b9 100644 --- a/drivers/thermal/intel/intel_soc_dts_iosf.c +++ b/drivers/thermal/intel/intel_soc_dts_iosf.c @@ -212,7 +212,7 @@ static int soc_dts_enable(int id) static void remove_dts_thermal_zone(struct intel_soc_dts_sensor_entry *dts) { - if (dts) { + if (dts->tzone) { iosf_mbi_write(BT_MBI_UNIT_PMC, MBI_REG_WRITE, SOC_DTS_OFFSET_ENABLE, dts->store_status); thermal_zone_device_unregister(dts->tzone); @@ -277,6 +277,7 @@ static int add_dts_thermal_zone(int id, struct intel_soc_dts_sensor_entry *dts, err_enable: thermal_zone_device_unregister(dts->tzone); err_ret: + dts->tzone = NULL; return ret; }
All of the existing callers of remove_dts_thermal_zone() pass a valid pointer as the argument, so checking for the NULL pointer is redundant. Plus, when calling remove_dts_thermal_zone() from intel_soc_dts_iosf_init(), it is possible that 1. dts->tzone is an error pointer, when the sensor fails to be registered as a valid thermal zone 2. dts->tzone is unregistered in add_dts_thermal_zone(), when some failure occurs after thermal zone registered In both cases, there is no need to unregister dts->tzone in remove_dts_thermal_zone(). Clear dst->tzone when add_dts_thermal_zone() fails. And do thermal zone removal in remove_dts_thermal_zone() only when dts->tzone is set. Signed-off-by: Zhang Rui <rui.zhang@intel.com> --- drivers/thermal/intel/intel_soc_dts_iosf.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)