From: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> To: Daniel Lezcano <daniel.lezcano@linaro.org> Cc: Andrzej Pietrasiewicz <andrzej.p@collabora.com>, linux-pm@vger.kernel.org, Zhang Rui <rui.zhang@intel.com>, "Rafael J . Wysocki" <rjw@rjwysocki.net>, Len Brown <lenb@kernel.org>, Jiri Pirko <jiri@mellanox.com>, Ido Schimmel <idosch@mellanox.com>, "David S . Miller" <davem@davemloft.net>, Peter Kaestle <peter@piie.net>, Darren Hart <dvhart@infradead.org>, Andy Shevchenko <andy@infradead.org>, Support Opensource <support.opensource@diasemi.com>, Amit Kucheria <amit.kucheria@verdurent.com>, Shawn Guo <shawnguo@kernel.org>, Sascha Hauer <s.hauer@pengutronix.de>, Pengutronix Kernel Team <kernel@pengutronix.de>, Fabio Estevam <festevam@gmail.com>, NXP Linux Team <linux-imx@nxp.com>, Allison Randal <allison@lohutok.net>, Enrico Weigelt <info@metux.net>, Gayatri Kammela <gayatri.kammela@intel.com>, Thomas Gleixner <tglx@linutronix.de>, linux-acpi@vger.kernel.org, netdev@vger.kernel.org, platform-driver-x86@vger.kernel.org, linux-arm-kernel@lists.infradead.org, kernel@collabora.com Subject: Re: [RFC v3 1/2] thermal: core: Let thermal zone device's mode be stored in its struct Date: Wed, 27 May 2020 15:30:30 +0200 Message-ID: <4493c0e4-51aa-3907-810c-74949ff27ca4@samsung.com> (raw) In-Reply-To: <f39c5ca6-5efa-889c-21f5-632dfd24715e@linaro.org> Hi Daniel, On 5/23/20 11:24 PM, Daniel Lezcano wrote: > Hi Andrzej, > > On 17/04/2020 18:20, Andrzej Pietrasiewicz wrote: >> Thermal zone devices' mode is stored in individual drivers. This patch >> changes it so that mode is stored in struct thermal_zone_device instead. >> >> As a result all driver-specific variables storing the mode are not needed >> and are removed. Consequently, the get_mode() implementations have nothing >> to operate on and need to be removed, too. >> >> Some thermal framework specific functions are introduced: >> >> thermal_zone_device_get_mode() >> thermal_zone_device_set_mode() >> thermal_zone_device_enable() >> thermal_zone_device_disable() >> >> thermal_zone_device_get_mode() and its "set" counterpart take tzd's lock >> and the "set" calls driver's set_mode() if provided, so the latter must >> not take this lock again. At the end of the "set" >> thermal_zone_device_update() is called so drivers don't need to repeat this >> invocation in their specific set_mode() implementations. >> >> The scope of the above 4 functions is purposedly limited to the thermal >> framework and drivers are not supposed to call them. This encapsulation >> does not fully work at the moment for some drivers, though: >> >> - platform/x86/acerhdf.c >> - drivers/thermal/imx_thermal.c >> - drivers/thermal/intel/intel_quark_dts_thermal.c >> - drivers/thermal/of-thermal.c >> >> and they manipulate struct thermal_zone_device's members directly. >> >> struct thermal_zone_params gains a new member called initial_mode, which >> is used to set tzd's mode at registration time. >> >> The sysfs "mode" attribute is always exposed from now on, because all >> thermal zone devices now have their get_mode() implemented at the generic >> level and it is always available. Exposing "mode" doesn't hurt the drivers >> which don't provide their own set_mode(), because writing to "mode" will >> result in -EPERM, as expected. > > The result is great, that is a nice cleanup of the thermal framework. > > After review it appears there are still problems IMO, especially with > the suspend / resume path. The patch is big, it is a bit complex to > comment. I suggest to re-org the changes as following: There are still issues with the related existing thermal code but this patch seems to be a step in the right direction. For the latest version posted ("v3" one, your mail was replied to the older "RFC v3" one): https://lore.kernel.org/linux-pm/20200423165705.13585-2-andrzej.p@collabora.com/ I couldn't find the problems with the patch itself (no new issues being introduced, all changes seem to be improvements over the current situation). Also the patch is not small but it also not that big and it mostly removes the code: 17 files changed, 105 insertions(+), 244 deletions(-) I worry that since the original code is intertwined in the interesting ways the cost of work on splitting the patch on smaller changes may be higher than its benefits. > - patch 1 : Add the four functions: > > * thermal_zone_device_set_mode() > * thermal_zone_device_enable() > * thermal_zone_device_disable() > * thermal_zone_device_is_enabled() > > *but* do not export thermal_zone_device_set_mode(), it must stay private > to the thermal framework ATM. > > - patch 2 : Add the mode THERMAL_DEVICE_SUSPENDED > > In the thermal_pm_notify() in the: > > - PM_SUSPEND_PREPARE case, set the mode to THERMAL_DEVICE_SUSPENDED if > the mode is THERMAL_DEVICE_ENABLED > > - PM_POST_SUSPEND case, set the mode to THERMAL_DEVICE_ENABLED, if the > mode is THERMAL_DEVICE_SUSPENDED > > - patch 3 : Change the monitor function > > Change monitor_thermal_zone() function to set the polling to zero if the > mode is THERMAL_DEVICE_DISABLED > > - patch 4 : Do the changes to remove get_mode() ops > > Make sure there is no access to tz->mode from the drivers anymore but > use of the functions of patch 1. IMO, this is the tricky part because a > part of the drivers are not calling the update after setting the mode > while the function thermal_zone_device_enable()/disable() call update > via the thermal_zone_device_set_mode(), so we must be sure to not break > anything. > > - patch 5 : Do the changes to remove set_mode() ops users > > As the patch 3 sets the polling to zero, the routine in the driver > setting the polling to zero is no longer needed (eg. in the mellanox > driver). I expect int300 to be the last user of this ops, hopefully we > can find a way to get rid of the specific call done inside and then > remove the ops. > > The initial_mode approach looks hackish, I suggest to make the default > the thermal zone disabled after creating and then explicitly enable it. > Note that is what do a lot of drivers already. > > Hopefully, these changes are git-bisect safe. > > Does it make sense ? Besides the requirement to split the patch it seems that the above list contains a lot of problematic areas with the existing thermal code yet to be addressed.. Best regards, -- Bartlomiej Zolnierkiewicz Samsung R&D Institute Poland Samsung Electronics
next prev parent reply index Thread overview: 137+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-04-07 17:49 [RFC 0/8] Stop monitoring disabled devices Andrzej Pietrasiewicz 2020-04-07 17:49 ` [RFC 1/8] thermal: int3400_thermal: Statically initialize .get_mode()/.set_mode() ops Andrzej Pietrasiewicz [not found] ` <CGME20200415091204eucas1p1983548c2db52d8d0c2a5367034ec80dd@eucas1p1.samsung.com> 2020-04-15 9:12 ` Bartlomiej Zolnierkiewicz 2020-04-07 17:49 ` [RFC 2/8] thermal: Properly handle mode values in .set_mode() Andrzej Pietrasiewicz [not found] ` <CGME20200415100543eucas1p24e24293da39844ca8791db86af5365a7@eucas1p2.samsung.com> 2020-04-15 10:05 ` Bartlomiej Zolnierkiewicz 2020-04-07 17:49 ` [RFC 3/8] thermal: Store thermal mode in a dedicated enum Andrzej Pietrasiewicz [not found] ` <CGME20200415100944eucas1p1dcfeb784e790ca2fc3417fd2797e3f5d@eucas1p1.samsung.com> 2020-04-15 10:09 ` Bartlomiej Zolnierkiewicz 2020-04-07 17:49 ` [RFC 4/8] thermal: core: Introduce THERMAL_DEVICE_INITIAL Andrzej Pietrasiewicz 2020-04-07 17:49 ` [RFC 5/8] thermal: core: Monitor thermal zone after mode change Andrzej Pietrasiewicz 2020-04-07 17:49 ` [RFC 6/8] thermal: Set initial state to THERMAL_DEVICE_INITIAL Andrzej Pietrasiewicz 2020-04-07 17:49 ` [RFC 7/8] thermal: of: Monitor thermal zone after enabling it Andrzej Pietrasiewicz [not found] ` <CGME20200415101829eucas1p18e21324d3c39926fffc6c8b5dc52f206@eucas1p1.samsung.com> 2020-04-15 10:18 ` Bartlomiej Zolnierkiewicz 2020-04-07 17:49 ` [RFC 8/8] thermal: Stop polling DISABLED thermal devices Andrzej Pietrasiewicz 2020-04-09 10:29 ` [RFC 0/8] Stop monitoring disabled devices Daniel Lezcano 2020-04-09 11:10 ` Andrzej Pietrasiewicz [not found] ` <CGME20200415104010eucas1p101278e53e34a2e56dfc7c82b533a9122@eucas1p1.samsung.com> 2020-04-15 10:40 ` Bartlomiej Zolnierkiewicz 2020-04-17 16:23 ` Andrzej Pietrasiewicz 2020-04-19 11:42 ` Bartlomiej Zolnierkiewicz 2020-04-17 21:11 ` Peter Kästle 2020-04-14 18:00 ` [RFC v2 0/9] " Andrzej Pietrasiewicz 2020-04-14 18:00 ` [RFC v2 1/9] thermal: int3400_thermal: Statically initialize .get_mode()/.set_mode() ops Andrzej Pietrasiewicz 2020-04-15 9:23 ` Daniel Lezcano 2020-04-14 18:00 ` [RFC v2 2/9] thermal: Eliminate an always-false condition Andrzej Pietrasiewicz 2020-04-14 18:00 ` [RFC v2 3/9] thermal: Properly handle mode values in .set_mode() Andrzej Pietrasiewicz 2020-04-14 22:14 ` Daniel Lezcano 2020-04-14 18:01 ` [RFC v2 4/9] thermal: core: Let thermal zone device's mode be stored in its struct Andrzej Pietrasiewicz 2020-04-14 22:30 ` Daniel Lezcano 2020-04-14 18:01 ` [RFC v2 5/9] thermal: Store mode in thermal_zone_device Andrzej Pietrasiewicz 2020-04-15 10:55 ` Daniel Lezcano 2020-04-17 16:20 ` [RFC v3 0/2] Stop monitoring disabled devices Andrzej Pietrasiewicz 2020-04-17 16:20 ` [RFC v3 1/2] thermal: core: Let thermal zone device's mode be stored in its struct Andrzej Pietrasiewicz 2020-04-17 20:44 ` Andy Shevchenko 2020-04-19 11:38 ` Bartlomiej Zolnierkiewicz 2020-04-19 13:10 ` Bartlomiej Zolnierkiewicz 2020-04-20 18:17 ` [PATCH 0/2] Stop monitoring disabled devices Andrzej Pietrasiewicz 2020-04-20 18:17 ` [PATCH 1/2] thermal: core: Let thermal zone device's mode be stored in its struct Andrzej Pietrasiewicz 2020-04-22 7:49 ` kbuild test robot 2020-04-22 17:14 ` Andrzej Pietrasiewicz 2020-04-22 17:03 ` kbuild test robot 2020-04-22 17:15 ` Andrzej Pietrasiewicz 2020-04-20 18:17 ` [PATCH 2/2] thermal: core: Stop polling DISABLED thermal devices Andrzej Pietrasiewicz 2020-04-22 16:12 ` [PATCH RESEND 1/2] thermal: core: Let thermal zone device's mode be stored in its struct Andrzej Pietrasiewicz 2020-04-23 14:39 ` Bartlomiej Zolnierkiewicz 2020-04-23 16:57 ` [PATCH v3 0/2] Stop monitoring disabled devices Andrzej Pietrasiewicz 2020-04-23 16:57 ` [PATCH v3 1/2] thermal: core: Let thermal zone device's mode be stored in its struct Andrzej Pietrasiewicz 2020-05-04 7:00 ` Bartlomiej Zolnierkiewicz 2020-04-23 16:57 ` [PATCH v3 2/2] thermal: core: Stop polling DISABLED thermal devices Andrzej Pietrasiewicz 2020-04-24 9:03 ` Zhang, Rui 2020-04-27 14:20 ` Zhang, Rui 2020-04-27 18:34 ` Andrzej Pietrasiewicz 2020-04-28 13:55 ` Zhang, Rui 2020-04-20 11:03 ` [RFC v3 1/2] thermal: core: Let thermal zone device's mode be stored in its struct Andrzej Pietrasiewicz 2020-04-20 18:19 ` Andrzej Pietrasiewicz 2020-05-23 21:24 ` Daniel Lezcano 2020-05-25 19:35 ` Andrzej Pietrasiewicz 2020-05-25 22:08 ` Daniel Lezcano 2020-05-26 16:56 ` Andrzej Pietrasiewicz 2020-05-27 13:30 ` Bartlomiej Zolnierkiewicz [this message] 2020-04-17 16:20 ` [RFC v3 2/2] thermal: core: Stop polling DISABLED thermal devices Andrzej Pietrasiewicz 2020-04-19 13:50 ` [RFC v3 1/2] thermal: core: Let thermal zone device's mode be stored in its struct Peter Kästle 2020-04-14 18:01 ` [RFC v2 6/9] thermal: Remove get_mode() method Andrzej Pietrasiewicz 2020-04-14 18:01 ` [RFC v2 7/9] thermal: core: Monitor thermal zone after mode change Andrzej Pietrasiewicz 2020-04-14 18:01 ` [RFC v2 8/9] thermal: of: Monitor thermal zone after enabling it Andrzej Pietrasiewicz 2020-04-14 18:01 ` [RFC v2 9/9] thermal: core: Stop polling DISABLED thermal devices Andrzej Pietrasiewicz 2020-04-14 21:52 ` [RFC v2 0/9] Stop monitoring disabled devices Ezequiel Garcia [not found] <Message-ID: <4493c0e4-51aa-3907-810c-74949ff27ca4@samsung.com> 2020-05-28 19:20 ` [PATCH v4 00/11] " Andrzej Pietrasiewicz 2020-05-28 19:20 ` [PATCH v4 01/11] acpi: thermal: Fix error handling in the register function Andrzej Pietrasiewicz 2020-05-29 14:50 ` Guenter Roeck 2020-06-24 8:16 ` Bartlomiej Zolnierkiewicz 2020-05-28 19:20 ` [PATCH v4 02/11] thermal: Store thermal mode in a dedicated enum Andrzej Pietrasiewicz 2020-05-29 14:48 ` Guenter Roeck 2020-05-29 15:13 ` Andrzej Pietrasiewicz 2020-05-29 15:34 ` Guenter Roeck 2020-06-24 9:38 ` Bartlomiej Zolnierkiewicz 2020-05-28 19:20 ` [PATCH v4 03/11] thermal: Add current mode to thermal zone device Andrzej Pietrasiewicz 2020-05-29 14:51 ` Guenter Roeck 2020-06-24 9:39 ` Bartlomiej Zolnierkiewicz 2020-05-28 19:20 ` [PATCH v4 04/11] thermal: Store device mode in struct thermal_zone_device Andrzej Pietrasiewicz 2020-06-24 9:40 ` Bartlomiej Zolnierkiewicz 2020-05-28 19:20 ` [PATCH v4 05/11] thermal: remove get_mode() operation of drivers Andrzej Pietrasiewicz 2020-06-24 9:47 ` Bartlomiej Zolnierkiewicz 2020-05-28 19:20 ` [PATCH v4 06/11] thermal: Add mode helpers Andrzej Pietrasiewicz 2020-06-24 9:49 ` Bartlomiej Zolnierkiewicz 2020-05-28 19:20 ` [PATCH v4 07/11] thermal: Use mode helpers in drivers Andrzej Pietrasiewicz 2020-06-24 9:51 ` Bartlomiej Zolnierkiewicz 2020-06-26 16:08 ` Andrzej Pietrasiewicz 2020-05-28 19:20 ` [PATCH v4 08/11] thermal: Explicitly enable non-changing thermal zone devices Andrzej Pietrasiewicz 2020-06-24 10:00 ` Bartlomiej Zolnierkiewicz 2020-05-28 19:20 ` [PATCH v4 09/11] thermal: core: Stop polling DISABLED thermal devices Andrzej Pietrasiewicz 2020-06-24 10:02 ` Bartlomiej Zolnierkiewicz 2020-05-28 19:20 ` [PATCH v4 10/11] thermal: Simplify or eliminate unnecessary set_mode() methods Andrzej Pietrasiewicz 2020-06-24 10:03 ` Bartlomiej Zolnierkiewicz 2020-05-28 19:20 ` [PATCH v4 11/11] thermal: Rename set_mode() to change_mode() Andrzej Pietrasiewicz 2020-06-24 10:03 ` Bartlomiej Zolnierkiewicz 2020-06-01 11:36 ` [PATCH v4 02/11] thermal: Store thermal mode in a dedicated enum Peter Kästle 2020-06-01 11:36 ` [PATCH v4 04/11] thermal: Store device mode in struct thermal_zone_device Peter Kästle 2020-06-01 11:37 ` [PATCH v4 05/11] thermal: remove get_mode() operation of drivers Peter Kästle 2020-06-01 11:37 ` [PATCH v4 07/11] thermal: Use mode helpers in drivers Peter Kästle 2020-06-01 11:38 ` [PATCH v4 10/11] thermal: Simplify or eliminate unnecessary set_mode() methods Peter Kästle 2020-06-01 11:38 ` [PATCH v4 11/11] thermal: Rename set_mode() to change_mode() Peter Kästle 2020-06-23 14:37 ` [PATCH v4 00/11] Stop monitoring disabled devices Daniel Lezcano 2020-06-26 17:37 ` [PATCH v5 " Andrzej Pietrasiewicz 2020-06-26 17:37 ` [PATCH v5 01/11] acpi: thermal: Fix error handling in the register function Andrzej Pietrasiewicz 2020-06-29 7:19 ` Amit Kucheria 2020-06-26 17:37 ` [PATCH v5 02/11] thermal: Store thermal mode in a dedicated enum Andrzej Pietrasiewicz 2020-06-29 7:19 ` Amit Kucheria 2020-06-26 17:37 ` [PATCH v5 03/11] thermal: Add current mode to thermal zone device Andrzej Pietrasiewicz 2020-06-29 7:19 ` Amit Kucheria 2020-06-26 17:37 ` [PATCH v5 04/11] thermal: Store device mode in struct thermal_zone_device Andrzej Pietrasiewicz 2020-06-29 7:19 ` Amit Kucheria 2020-06-26 17:37 ` [PATCH v5 05/11] thermal: remove get_mode() operation of drivers Andrzej Pietrasiewicz 2020-06-29 7:20 ` Amit Kucheria 2020-06-26 17:37 ` [PATCH v5 06/11] thermal: Add mode helpers Andrzej Pietrasiewicz 2020-06-26 20:50 ` kernel test robot 2020-06-26 20:50 ` [RFC PATCH] thermal: thermal_zone_device_set_mode() can be static kernel test robot 2020-06-26 20:55 ` [PATCH v5 06/11] thermal: Add mode helpers kernel test robot 2020-06-29 7:04 ` Amit Kucheria 2020-06-29 11:16 ` [PATCH v6 00/11] Stop monitoring disabled devices Andrzej Pietrasiewicz 2020-06-29 11:16 ` [PATCH v6 01/11] acpi: thermal: Fix error handling in the register function Andrzej Pietrasiewicz 2020-06-29 11:16 ` [PATCH v6 02/11] thermal: Store thermal mode in a dedicated enum Andrzej Pietrasiewicz 2020-06-29 11:16 ` [PATCH v6 03/11] thermal: Add current mode to thermal zone device Andrzej Pietrasiewicz 2020-06-29 11:16 ` [PATCH v6 04/11] thermal: Store device mode in struct thermal_zone_device Andrzej Pietrasiewicz 2020-06-29 11:16 ` [PATCH v6 05/11] thermal: remove get_mode() operation of drivers Andrzej Pietrasiewicz 2020-06-29 11:16 ` [PATCH v6 06/11] thermal: Add mode helpers Andrzej Pietrasiewicz 2020-06-29 12:08 ` Daniel Lezcano 2020-06-29 11:16 ` [PATCH v6 07/11] thermal: Use mode helpers in drivers Andrzej Pietrasiewicz 2020-06-29 11:16 ` [PATCH v6 08/11] thermal: Explicitly enable non-changing thermal zone devices Andrzej Pietrasiewicz 2020-06-29 11:16 ` [PATCH v6 09/11] thermal: core: Stop polling DISABLED thermal devices Andrzej Pietrasiewicz 2020-06-29 11:16 ` [PATCH v6 10/11] thermal: Simplify or eliminate unnecessary set_mode() methods Andrzej Pietrasiewicz 2020-06-29 11:16 ` [PATCH v6 11/11] thermal: Rename set_mode() to change_mode() Andrzej Pietrasiewicz 2020-06-26 17:37 ` [PATCH v5 07/11] thermal: Use mode helpers in drivers Andrzej Pietrasiewicz 2020-06-26 17:37 ` [PATCH v5 08/11] thermal: Explicitly enable non-changing thermal zone devices Andrzej Pietrasiewicz 2020-06-26 17:37 ` [PATCH v5 09/11] thermal: core: Stop polling DISABLED thermal devices Andrzej Pietrasiewicz 2020-06-26 17:37 ` [PATCH v5 10/11] thermal: Simplify or eliminate unnecessary set_mode() methods Andrzej Pietrasiewicz 2020-06-26 17:37 ` [PATCH v5 11/11] thermal: Rename set_mode() to change_mode() Andrzej Pietrasiewicz 2020-06-01 10:02 ` [PATCH v4 00/11] Stop monitoring disabled devices Peter Kästle 2020-06-01 10:20 ` Andrzej Pietrasiewicz
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=4493c0e4-51aa-3907-810c-74949ff27ca4@samsung.com \ --to=b.zolnierkie@samsung.com \ --cc=allison@lohutok.net \ --cc=amit.kucheria@verdurent.com \ --cc=andrzej.p@collabora.com \ --cc=andy@infradead.org \ --cc=daniel.lezcano@linaro.org \ --cc=davem@davemloft.net \ --cc=dvhart@infradead.org \ --cc=festevam@gmail.com \ --cc=gayatri.kammela@intel.com \ --cc=idosch@mellanox.com \ --cc=info@metux.net \ --cc=jiri@mellanox.com \ --cc=kernel@collabora.com \ --cc=kernel@pengutronix.de \ --cc=lenb@kernel.org \ --cc=linux-acpi@vger.kernel.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-imx@nxp.com \ --cc=linux-pm@vger.kernel.org \ --cc=netdev@vger.kernel.org \ --cc=peter@piie.net \ --cc=platform-driver-x86@vger.kernel.org \ --cc=rjw@rjwysocki.net \ --cc=rui.zhang@intel.com \ --cc=s.hauer@pengutronix.de \ --cc=shawnguo@kernel.org \ --cc=support.opensource@diasemi.com \ --cc=tglx@linutronix.de \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
Linux-PM Archive on lore.kernel.org Archives are clonable: git clone --mirror https://lore.kernel.org/linux-pm/0 linux-pm/git/0.git # If you have public-inbox 1.1+ installed, you may # initialize and index your mirror using the following commands: public-inbox-init -V2 linux-pm linux-pm/ https://lore.kernel.org/linux-pm \ linux-pm@vger.kernel.org public-inbox-index linux-pm Example config snippet for mirrors Newsgroup available over NNTP: nntp://nntp.lore.kernel.org/org.kernel.vger.linux-pm AGPL code for this site: git clone https://public-inbox.org/public-inbox.git