From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-2.2 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,UNPARSEABLE_RELAY,USER_AGENT_SANE_1 autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 0A246C54EEB for ; Mon, 27 Apr 2020 18:34:46 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id E4A2621707 for ; Mon, 27 Apr 2020 18:34:45 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726249AbgD0Sep (ORCPT ); Mon, 27 Apr 2020 14:34:45 -0400 Received: from bhuna.collabora.co.uk ([46.235.227.227]:40318 "EHLO bhuna.collabora.co.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726230AbgD0Seo (ORCPT ); Mon, 27 Apr 2020 14:34:44 -0400 Received: from [127.0.0.1] (localhost [127.0.0.1]) (Authenticated sender: andrzej.p) with ESMTPSA id BAEC52A0D29 Subject: Re: [PATCH v3 2/2] thermal: core: Stop polling DISABLED thermal devices To: "Zhang, Rui" , "'linux-pm@vger.kernel.org'" Cc: "'Rafael J . Wysocki'" , 'Len Brown' , 'Jiri Pirko' , 'Ido Schimmel' , "'David S . Miller'" , 'Peter Kaestle' , 'Darren Hart' , 'Andy Shevchenko' , 'Support Opensource' , 'Daniel Lezcano' , 'Amit Kucheria' , 'Shawn Guo' , 'Sascha Hauer' , 'Pengutronix Kernel Team' , 'Fabio Estevam' , 'NXP Linux Team' , 'Heiko Stuebner' , 'Orson Zhai' , 'Baolin Wang' , 'Chunyan Zhang' , "'linux-acpi@vger.kernel.org'" , "'netdev@vger.kernel.org'" , "'platform-driver-x86@vger.kernel.org'" , "'linux-arm-kernel@lists.infradead.org'" , "'kernel@collabora.com'" , 'Barlomiej Zolnierkiewicz' References: <20200423165705.13585-1-andrzej.p@collabora.com> <20200423165705.13585-3-andrzej.p@collabora.com> <744357E9AAD1214791ACBA4B0B90926377CF60E3@SHSMSX108.ccr.corp.intel.com> <744357E9AAD1214791ACBA4B0B90926377CF9A10@SHSMSX108.ccr.corp.intel.com> From: Andrzej Pietrasiewicz Message-ID: Date: Mon, 27 Apr 2020 20:34:35 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.7.0 MIME-Version: 1.0 In-Reply-To: <744357E9AAD1214791ACBA4B0B90926377CF9A10@SHSMSX108.ccr.corp.intel.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit Sender: linux-pm-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-pm@vger.kernel.org Hi, W dniu 27.04.2020 o 16:20, Zhang, Rui pisze: > > >> -----Original Message----- >> From: Zhang, Rui >> Sent: Friday, April 24, 2020 5:03 PM >> To: Andrzej Pietrasiewicz ; linux- >> pm@vger.kernel.org >> Cc: Rafael J . Wysocki ; Len Brown ; >> Jiri Pirko ; Ido Schimmel ; David >> S . Miller ; Peter Kaestle ; Darren >> Hart ; Andy Shevchenko ; >> Support Opensource ; Daniel Lezcano >> ; Amit Kucheria >> ; Shawn Guo ; >> Sascha Hauer ; Pengutronix Kernel Team >> ; Fabio Estevam ; NXP >> Linux Team ; Heiko Stuebner ; >> Orson Zhai ; Baolin Wang >> ; Chunyan Zhang ; linux- >> acpi@vger.kernel.org; netdev@vger.kernel.org; platform-driver- >> x86@vger.kernel.org; linux-arm-kernel@lists.infradead.org; >> kernel@collabora.com; Barlomiej Zolnierkiewicz >> Subject: RE: [PATCH v3 2/2] thermal: core: Stop polling DISABLED thermal >> devices >> >> Hi, Andrzej, >> >> Thanks for the patches. My Linux laptop was broken and won't get fixed till >> next week, so I may lost some of the discussions previously. >> >>> -----Original Message----- >>> From: Andrzej Pietrasiewicz >>> Sent: Friday, April 24, 2020 12:57 AM >>> To: linux-pm@vger.kernel.org >>> Cc: Zhang, Rui ; Rafael J . Wysocki >>> ; Len Brown ; Jiri Pirko >>> ; Ido Schimmel ; David S . >>> Miller ; Peter Kaestle ; Darren >>> Hart ; Andy Shevchenko ; >>> Support Opensource ; Daniel Lezcano >>> ; Amit Kucheria >>> ; Shawn Guo ; >> Sascha >>> Hauer ; Pengutronix Kernel Team >>> ; Fabio Estevam ; NXP >> Linux >>> Team ; Heiko Stuebner ; Orson >> Zhai >>> ; Baolin Wang ; >> Chunyan >>> Zhang ; linux- acpi@vger.kernel.org; >>> netdev@vger.kernel.org; platform-driver- x86@vger.kernel.org; >>> linux-arm-kernel@lists.infradead.org; >>> kernel@collabora.com; Andrzej Pietrasiewicz ; >>> Barlomiej Zolnierkiewicz >>> Subject: [PATCH v3 2/2] thermal: core: Stop polling DISABLED thermal >>> devices >>> Importance: High >>> >>> Polling DISABLED devices is not desired, as all such "disabled" >>> devices are meant to be handled by userspace. This patch introduces >>> and uses >>> should_stop_polling() to decide whether the device should be polled or >> not. >>> >> Thanks for the fix, and IMO, this reveal some more problems. >> Say, we need to define "DISABLED" thermal zone. >> Can we read the temperature? Can we trust the trip point value? >> >> IMO, a disabled thermal zone does not mean it is handled by userspace, >> because that is what the userspace governor designed for. >> Instead, if a thermal zone is disabled, in thermal_zone_device_update(), we >> should basically skip all the other operations as well. >> > I overlooked the last line of the patch. So thermal_zone_device_update() returns > immediately if the thermal zone is disabled, right? > > But how can we stop polling in this case? It does stop. However, I indeed observe an extra call to thermal_zone_device_update() before it fully stops. I think what happens is this: - storing "disabled" in mode ends up in thermal_zone_device_set_mode(), which calls driver's ->set_mode() and then calls thermal_zone_device_update(), which returns immediately and does not touch the tz->poll_queue delayed work - thermal_zone_device_update() is called from the delayed work when its time comes and this time it also returns immediately, not modifying the said delayed work, so polling effectively stops now. > There is no chance to call into monitor_thermal_zone() in thermal_zone_device_update(), > or do I miss something? Without the last "if" statement in this patch polling stops with the first call to thermal_zone_device_update() because it indeed disables the delayed work. So you are probably right - that last "if" should not be introduced. > >> I'll try your patches and probably make an incremental patch. > > I have finished a small patch set to improve this based on my understanding, and will post it > tomorrow after testing. > Is your small patchset based on top of this series or is it a completely rewritten version? Andrzej