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 Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id B9F06ECAAA1 for ; Fri, 28 Oct 2022 17:37:21 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=DTrhzq06D/4QQI4liTpOJKVSHn2Dhif5mVSZ63BwL8k=; b=e6MMl5TCyfgIEE JYAJ4jBknQPUEkulrsfbjmvE+AXDRhbvSnQMSk7whKIOIE4UVrPZrbK7UNxkd0LLnZ/Zvpys2Dtvv SgLpkej1to6ZK3K5j63la7fA6Hdqc8P8/Ewgj7invZJlnxv8AQqqlDP5hpUc7eaztNYC29UVrJSa5 UGVuCk7betF5ERIq92Qlg2BN4mVR7D0v/7NiKJBbOG0sA5jZeQ0PbCsp0cFDaHeeeS9kEK4aECdkb 8SD4Kk1Ya2d2UPxjLUswtnlSsvE0mmMqigFmEKeyWriTtfJemYeNCL5cQdF1iGrxwH2NfMybxwzgT Y8S/iBSFS7xVxCyS+rJw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1ooTH6-001N5l-77; Fri, 28 Oct 2022 17:36:08 +0000 Received: from foss.arm.com ([217.140.110.172]) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1ooSu3-001BIH-P3 for linux-arm-kernel@lists.infradead.org; Fri, 28 Oct 2022 17:12:21 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id D8A121FB; Fri, 28 Oct 2022 10:12:24 -0700 (PDT) Received: from e120937-lin (unknown [172.31.20.19]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id A29C93F703; Fri, 28 Oct 2022 10:12:17 -0700 (PDT) Date: Fri, 28 Oct 2022 18:12:11 +0100 From: Cristian Marussi To: Guenter Roeck Cc: linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, sudeep.holla@arm.com, Daniel Lezcano , linux-hwmon@vger.kernel.org Subject: Re: [PATCH 7/8] hwmon: (scmi) Register explicitly with Thermal Framework Message-ID: References: <20221028140833.280091-1-cristian.marussi@arm.com> <20221028140833.280091-7-cristian.marussi@arm.com> <7acc7a49-debb-abdb-f01c-f8adef4c1f0e@roeck-us.net> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <7acc7a49-debb-abdb-f01c-f8adef4c1f0e@roeck-us.net> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20221028_101219_948472_3CDECC99 X-CRM114-Status: GOOD ( 38.03 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Fri, Oct 28, 2022 at 09:34:05AM -0700, Guenter Roeck wrote: > On 10/28/22 09:15, Cristian Marussi wrote: > > On Fri, Oct 28, 2022 at 08:58:58AM -0700, Guenter Roeck wrote: > > > On 10/28/22 08:35, Cristian Marussi wrote: > > > [ ... ] > > > > > > + /* > > > > > > + * Try to register a temperature sensor with the Thermal Framework: > > > > > > + * skip sensors not defined as part of any thermal zone (-ENODEV) but > > > > > > + * report any other errors related to misconfigured zones/sensors. > > > > > > + */ > > > > > > + tzd = devm_thermal_of_zone_register(dev, th_sensor->info->id, th_sensor, > > > > > > + &scmi_hwmon_thermal_ops); > > > > > > + if (IS_ERR(tzd)) { > > > > > > + devm_kfree(dev, th_sensor); > > > > > > + > > > > > > + if (PTR_ERR(tzd) != -ENODEV) > > > > > > + return PTR_ERR(tzd); > > > > > > + > > > > > > + dev_info(dev, "Sensor '%s' not attached to any thermal zone.\n", > > > > > > + sensor->name); > > > > > > > > > > There were complaints about this message as it is noisy. If you send > > > > > another version, please drop it unless attaching each sensor to a thermal > > > > > zone is strongly expected. If you don't send another version, I'll drop it > > > > > while applying. > > > > > > > > > > > > > Ok fine for me. I am waiting to have some feedback from Sudeep too, but > > > > I do not have plan for another version as of now. > > > > > > > > As a side note, though, I understand the 'noisiness' argument, but, > > > > sincerely this same message in the original HWMON code was the only > > > > reason why I spotted that something was wrong with the SCMI/HWMON > > > > interactions and discovered the indexes/ids mismatch...if not for > > > > that it would have gone un-noticed that a perfectly configured > > > > ThermalZone/Sensor was not working properly... > > > > (un-noticed at least until something would have been burnt to fire > > > > in my house .. joking :P) > > > > > > > > > > Good point. > > > > > > Did you ever check the returned error code ? Maybe we could use it to > > > distinguish "it is not attached to a thermal zone because it is not > > > associated with one" from "attaching to a thermal zone failed because > > > its configuration is bad/incomplete". > > > > > > > Yes, it is what I do already indeed, in this regards I mimicked what > > the hwmon-thermal bridge was doing. > > > > In scmi_thermal_sensor_register() this message is printed out only > > if Thermal registration returned -ENODEV and no err is reported > > (which means teh specified sensor was not found attached to any TZ), > > while in the caller of scmi_thermal_sensor_register() for any error > > returned but -ENOMEM I print: > > > > "Thermal zone misconfigured for %s. err=%d\n", > > > > since any error reported by Thermal other than ENODEV and ENOMEM > > means the DT parsing unveiled some configuration anomaly. > > > > Ok, then let's hope that this finds misconfigurations and drop the > info message. The mismatch of indexes at hand won't be catched being reported by Thermal as misconfig but just as not found ENODEV. Anyway it is fine for me to drop the message. > > I just noticed another problem in your code: > > + if (ret == -ENOMEM) > + return ret; > + else if (ret) > + dev_warn(dev, > + "Thermal zone misconfigured for %s. err=%d\n", > + sensor->name, ret); > > Static analyzers will rightfully notice that else after return is unnecessary. > Please rewrite and drop the else. I think something like > Ah yes...my bad. > if (ret) { > if (ret == -ENOMEM) > return ret; > dev_warn(dev, > "Thermal zone misconfigured for %s. err=%d\n", > sensor->name, ret); > } > > would be better since ret would only be evaluated once in the no-error case. > I'll resend this one with the fix and the dropped message. Thanks, Cristian _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel