Message ID | 20231116022330.2696-3-xingtong_wu@163.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:b909:0:b0:403:3b70:6f57 with SMTP id t9csp2948680vqg; Wed, 15 Nov 2023 18:39:46 -0800 (PST) X-Google-Smtp-Source: AGHT+IGR2wJ161zSQc+j+68cG9HNKwBQWMggO5jhZfNZ0Uov3PBgEyyg95ZcACqDlrSii1sJY1nz X-Received: by 2002:a05:6a20:7fa2:b0:186:cbf2:d99e with SMTP id d34-20020a056a207fa200b00186cbf2d99emr11023195pzj.18.1700102386383; Wed, 15 Nov 2023 18:39:46 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1700102386; cv=none; d=google.com; s=arc-20160816; b=g9QrCHskxc/ArH+lIgl9lJZpFLJtyRkYkTOjOPc8zoDFH67bqUZlE7UWCBdqUn+6TA bEp1Bny8/maCWxh+E64DsY4RfMmaDqJ5NHVe702yxBdeAxV1uIoqaf1IL45IO/xlllAA hzFRi3myPpZZj8zHALOD603WEPgf52yk3/3cru+zmRCGy+W/5Eh3J+4LFG7pT260ULKm vTYe0SEpnio7sPq5oxi14HN0vNTC0E/vJI+Ymrmm6icLh71IR9nwy69jj4qLgE+vHn2s 8KSKrWEQ2sY3+DklS2trVbz/1r6iDnBD5vyZVgIrdFGgZ+OdRgYuu66os7bDAYi66jf8 PYCA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :references:in-reply-to:message-id:date:subject:cc:to:from :dkim-signature; bh=cHgkt6wFw4yulka3yAnNtGtv/ycgWvgfCPluWVrqKrA=; fh=brTD9uWwHSBEuDMsNSIyrWZVEeJXt4Ae/wkRCvfJiZk=; b=qnLfIt5w6++6Rn5KL2pzKM+CvMeBnZiP9Wf9Gk5hj3BJbRcx0I9z3Fcw/Ev8I7ZKK0 7bcMNPpEHjYOIed6KJdQQ3clYTYTfSQdVhnevjIH2UpD9KYrhDEFPwpcz4NzVjEKvEhP UnbFXitCE7y/DZLqsrNhfaiZPnxb4F/l8gMTXGuzNU/W6WBeqeCVrD7AzHu39XgciNVO iDkybUpVjWM6yH8w/8+4B9Hle+Cq+P6wxwhzA0sB8Yz2urRFFTp0AlBZPkxjy/2vm7n0 kRjeug1qJzWmPxGv37014NMAGKi/Cr3JzBtcCIuhWJ8NATBaY+BKWfsVsTYtcIGmKC1U ZsnA== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@163.com header.s=s110527 header.b=kzXMORIJ; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=163.com Received: from snail.vger.email (snail.vger.email. [2620:137:e000::3:7]) by mx.google.com with ESMTPS id e4-20020a170902d38400b001ca96a6eefesi11243233pld.577.2023.11.15.18.39.46 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 15 Nov 2023 18:39:46 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) client-ip=2620:137:e000::3:7; Authentication-Results: mx.google.com; dkim=pass header.i=@163.com header.s=s110527 header.b=kzXMORIJ; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=163.com Received: from out1.vger.email (depot.vger.email [IPv6:2620:137:e000::3:0]) by snail.vger.email (Postfix) with ESMTP id 6E3398127F5E; Wed, 15 Nov 2023 18:39:45 -0800 (PST) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.11 at snail.vger.email Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1344462AbjKPCjo (ORCPT <rfc822;lhua1029@gmail.com> + 28 others); Wed, 15 Nov 2023 21:39:44 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:33596 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S235571AbjKPCjm (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Wed, 15 Nov 2023 21:39:42 -0500 Received: from m12.mail.163.com (m12.mail.163.com [220.181.12.216]) by lindbergh.monkeyblade.net (Postfix) with ESMTP id 02F6C19F; Wed, 15 Nov 2023 18:39:38 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=163.com; s=s110527; h=From:Subject:Date:Message-Id:MIME-Version; bh=cHgkt 6wFw4yulka3yAnNtGtv/ycgWvgfCPluWVrqKrA=; b=kzXMORIJgWIWLtVcRBc6+ CHeJZmWclLrJHzUWHTD+48VXG07e45AXH1BXU+NO/pGWL7xLsEDaknuskHNX+f/W KOgLW4dXiv7qJfKdb+CRmIIygvCNbOEOhVNFbHJzS9ZrAIwaxMUXUIHczM5GU0Me CJcOEsJLJOxemzngAD7zUc= Received: from localhost.localdomain (unknown [39.144.137.125]) by zwqz-smtp-mta-g4-4 (Coremail) with SMTP id _____wD3H2U+fVVl5bnXDA--.19779S4; Thu, 16 Nov 2023 10:24:09 +0800 (CST) From: Xing Tong Wu <xingtong_wu@163.com> To: Guenter Roeck <linux@roeck-us.net>, Jean Delvare <jdelvare@suse.com>, linux-hwmon@vger.kernel.org, linux-kernel@vger.kernel.org Cc: xingtong.wu@siemens.com, tobias.schaffner@siemens.com, gerd.haeussler.ext@siemens.com Subject: [PATCH 2/3] hwmon: (nct6775) Fix logic error for PWM enable Date: Thu, 16 Nov 2023 10:23:29 +0800 Message-Id: <20231116022330.2696-3-xingtong_wu@163.com> X-Mailer: git-send-email 2.25.1 In-Reply-To: <20231116022330.2696-1-xingtong_wu@163.com> References: <20231116022330.2696-1-xingtong_wu@163.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-CM-TRANSID: _____wD3H2U+fVVl5bnXDA--.19779S4 X-Coremail-Antispam: 1Uf129KBjvdXoW7Wry8Kr4xXw17trWfGF4Uurg_yoWkKFX_Ww 4rGrZ7Zw1Y9r13CF4jgF4rtFW2ka1UWr17Jw1xKa98J347AFn5Cr1kXrZxZrnru3yDZF93 Xa1DAr4Iy342vjkaLaAFLSUrUUUUjb8apTn2vfkv8UJUUUU8Yxn0WfASr-VFAUDa7-sFnT 9fnUUvcSsGvfC2KfnxnUUI43ZEXa7IUeo7K5UUUUU== X-Originating-IP: [39.144.137.125] X-CM-SenderInfo: p0lqw35rqjs4rx6rljoofrz/1tbiTA4q0GI0cSnRZAAAsD X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM, RCVD_IN_DNSWL_BLOCKED,RCVD_IN_MSPIKE_BL,RCVD_IN_MSPIKE_L4, SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (snail.vger.email [0.0.0.0]); Wed, 15 Nov 2023 18:39:45 -0800 (PST) X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1782686559855889467 X-GMAIL-MSGID: 1782686559855889467 |
Series |
*** hwmon: (nct6775) Fix pwm bugs for NCT chips ***
|
|
Commit Message
Xing Tong Wu
Nov. 16, 2023, 2:23 a.m. UTC
From: Xing Tong Wu <xingtong.wu@siemens.com> The determination of the "pwm_enable" should be based solely on the mode, regardless of the pwm value. According to the specification, the default values for pwm and pwm_enable are 255 and 0 respectively. However, there is a bug in the code where the fan control is actually enabled, but the file "pwm_enable" incorrectly displays "off", indicating that fan control is disabled. This contradiction needs to be addressed and resolved. Solution: Update the logic so that "pwm_enable" is determined by mode + 1, remove the "off" value for "pwm_enable" since it is not specified in the documentation. Signed-off-by: Xing Tong Wu <xingtong.wu@siemens.com> --- drivers/hwmon/nct6775-core.c | 2 -- 1 file changed, 2 deletions(-)
Comments
On Thu, Nov 16, 2023 at 10:23:29AM +0800, Xing Tong Wu wrote: > From: Xing Tong Wu <xingtong.wu@siemens.com> > > The determination of the "pwm_enable" should be based solely on the mode, > regardless of the pwm value. > According to the specification, the default values for pwm and pwm_enable > are 255 and 0 respectively. However, there is a bug in the code where the > fan control is actually enabled, but the file "pwm_enable" incorrectly > displays "off", indicating that fan control is disabled. This contradiction > needs to be addressed and resolved. > Solution: Update the logic so that "pwm_enable" is determined by mode + 1, > remove the "off" value for "pwm_enable" since it is not specified in the > documentation. The chip specification is irrelevant. What is relevant is the hwmon ABI, which says What: /sys/class/hwmon/hwmonX/pwmY_enable Description: Fan speed control method: - 0: no fan speed control (i.e. fan at full speed) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ - 1: manual fan speed control enabled (using `pwmY`) - 2+: automatic fan speed control enabled which is what the code currently implements or at least tries to implement. Guenter > > Signed-off-by: Xing Tong Wu <xingtong.wu@siemens.com> > --- > drivers/hwmon/nct6775-core.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/drivers/hwmon/nct6775-core.c b/drivers/hwmon/nct6775-core.c > index 2111f0cd9787..575db6cb96e9 100644 > --- a/drivers/hwmon/nct6775-core.c > +++ b/drivers/hwmon/nct6775-core.c > @@ -900,8 +900,6 @@ static const u16 NCT6116_REG_TSI_TEMP[] = { 0x59, 0x5b }; > > static enum pwm_enable reg_to_pwm_enable(int pwm, int mode) > { > - if (mode == 0 && pwm == 255) > - return off; > return mode + 1; > } > > -- > 2.25.1 >
On 2023/11/16 16:07, Guenter Roeck wrote: > On Thu, Nov 16, 2023 at 10:23:29AM +0800, Xing Tong Wu wrote: >> From: Xing Tong Wu <xingtong.wu@siemens.com> >> >> The determination of the "pwm_enable" should be based solely on the mode, >> regardless of the pwm value. >> According to the specification, the default values for pwm and pwm_enable >> are 255 and 0 respectively. However, there is a bug in the code where the >> fan control is actually enabled, but the file "pwm_enable" incorrectly >> displays "off", indicating that fan control is disabled. This contradiction >> needs to be addressed and resolved. >> Solution: Update the logic so that "pwm_enable" is determined by mode + 1, >> remove the "off" value for "pwm_enable" since it is not specified in the >> documentation. > > The chip specification is irrelevant. What is relevant is the hwmon ABI, > which says > > What: /sys/class/hwmon/hwmonX/pwmY_enable > Description: > Fan speed control method: > > - 0: no fan speed control (i.e. fan at full speed) > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ I think this description may lead to misunderstanding. There are certain fans that cannot be controlled and operate at full speed while system is running. However, there are also fans whose speed can be controlled, even if they are currently set to full speed. In this particular situation, it would be better to inform the user that the fan can still be controlled despite being at full speed. How do you think that? > - 1: manual fan speed control enabled (using `pwmY`) > - 2+: automatic fan speed control enabled > > which is what the code currently implements or at least tries to > implement. > > Guenter > >> >> Signed-off-by: Xing Tong Wu <xingtong.wu@siemens.com> >> --- >> drivers/hwmon/nct6775-core.c | 2 -- >> 1 file changed, 2 deletions(-) >> >> diff --git a/drivers/hwmon/nct6775-core.c b/drivers/hwmon/nct6775-core.c >> index 2111f0cd9787..575db6cb96e9 100644 >> --- a/drivers/hwmon/nct6775-core.c >> +++ b/drivers/hwmon/nct6775-core.c >> @@ -900,8 +900,6 @@ static const u16 NCT6116_REG_TSI_TEMP[] = { 0x59, 0x5b }; >> >> static enum pwm_enable reg_to_pwm_enable(int pwm, int mode) >> { >> - if (mode == 0 && pwm == 255) >> - return off; >> return mode + 1; >> } >> >> -- >> 2.25.1 >>
On Thu, Nov 16, 2023 at 12:07:06AM -0800, Guenter Roeck wrote: > On Thu, Nov 16, 2023 at 10:23:29AM +0800, Xing Tong Wu wrote: > > From: Xing Tong Wu <xingtong.wu@siemens.com> > > > > The determination of the "pwm_enable" should be based solely on the mode, > > regardless of the pwm value. > > According to the specification, the default values for pwm and pwm_enable > > are 255 and 0 respectively. However, there is a bug in the code where the > > fan control is actually enabled, but the file "pwm_enable" incorrectly > > displays "off", indicating that fan control is disabled. This contradiction > > needs to be addressed and resolved. > > Solution: Update the logic so that "pwm_enable" is determined by mode + 1, > > remove the "off" value for "pwm_enable" since it is not specified in the > > documentation. > > The chip specification is irrelevant. What is relevant is the hwmon ABI, > which says > > What: /sys/class/hwmon/hwmonX/pwmY_enable > Description: > Fan speed control method: > > - 0: no fan speed control (i.e. fan at full speed) > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > - 1: manual fan speed control enabled (using `pwmY`) > - 2+: automatic fan speed control enabled > > which is what the code currently implements or at least tries to > implement. > As a follow-up, the existing code also handles setting _enable to 0 explicitly by selecting manual mode and setting the pwm value to the maximum. This does not match the chip specification, but is the best we can do to match ABI expectations. That also means that we can not reject setting pwm values if pwm control is disabled (off) since pwm==255 in manual mode is equivalent to disabling pwm. Yes, that means that setting pwm to 254 while pwm_enable==0 automatically enables manual mode, but that can not be helped. We _could_ possibly combine setting pwm_enable to manual mode with setting the pwm value to something other than 255 if it is currently set to 25, but that would be an optimization, not a bug fix. Guenter > Guenter > > > > > Signed-off-by: Xing Tong Wu <xingtong.wu@siemens.com> > > --- > > drivers/hwmon/nct6775-core.c | 2 -- > > 1 file changed, 2 deletions(-) > > > > diff --git a/drivers/hwmon/nct6775-core.c b/drivers/hwmon/nct6775-core.c > > index 2111f0cd9787..575db6cb96e9 100644 > > --- a/drivers/hwmon/nct6775-core.c > > +++ b/drivers/hwmon/nct6775-core.c > > @@ -900,8 +900,6 @@ static const u16 NCT6116_REG_TSI_TEMP[] = { 0x59, 0x5b }; > > > > static enum pwm_enable reg_to_pwm_enable(int pwm, int mode) > > { > > - if (mode == 0 && pwm == 255) > > - return off; > > return mode + 1; > > } > > > > -- > > 2.25.1 > > >
On Thu, Nov 16, 2023 at 04:36:39PM +0800, xingtong.wu wrote: > > On 2023/11/16 16:07, Guenter Roeck wrote: > > On Thu, Nov 16, 2023 at 10:23:29AM +0800, Xing Tong Wu wrote: > >> From: Xing Tong Wu <xingtong.wu@siemens.com> > >> > >> The determination of the "pwm_enable" should be based solely on the mode, > >> regardless of the pwm value. > >> According to the specification, the default values for pwm and pwm_enable > >> are 255 and 0 respectively. However, there is a bug in the code where the > >> fan control is actually enabled, but the file "pwm_enable" incorrectly > >> displays "off", indicating that fan control is disabled. This contradiction > >> needs to be addressed and resolved. > >> Solution: Update the logic so that "pwm_enable" is determined by mode + 1, > >> remove the "off" value for "pwm_enable" since it is not specified in the > >> documentation. > > > > The chip specification is irrelevant. What is relevant is the hwmon ABI, > > which says > > > > What: /sys/class/hwmon/hwmonX/pwmY_enable > > Description: > > Fan speed control method: > > > > - 0: no fan speed control (i.e. fan at full speed) > > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > I think this description may lead to misunderstanding. There are certain > fans that cannot be controlled and operate at full speed while system is > running. However, there are also fans whose speed can be controlled, even > if they are currently set to full speed. In this particular situation, it > would be better to inform the user that the fan can still be controlled > despite being at full speed. > How do you think that? We need to be consistent. Yes, it might be arguable that we should not return 0 if fan control can not be disabled by the chip, but that would effectively be a behavioral change. We don't know if there is some userspace program which expects to be able to disable fan control completely (and, when doing so, setting fan speed to its maximum). Given that, I don't think it is feasible to change the behavior. Guenter
diff --git a/drivers/hwmon/nct6775-core.c b/drivers/hwmon/nct6775-core.c index 2111f0cd9787..575db6cb96e9 100644 --- a/drivers/hwmon/nct6775-core.c +++ b/drivers/hwmon/nct6775-core.c @@ -900,8 +900,6 @@ static const u16 NCT6116_REG_TSI_TEMP[] = { 0x59, 0x5b }; static enum pwm_enable reg_to_pwm_enable(int pwm, int mode) { - if (mode == 0 && pwm == 255) - return off; return mode + 1; }