Message ID | 20231130191050.3165862-2-hugo@hugovil.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:bcd1:0:b0:403:3b70:6f57 with SMTP id r17csp619290vqy; Thu, 30 Nov 2023 11:11:18 -0800 (PST) X-Google-Smtp-Source: AGHT+IGmvBjJZ/DNxwuXliJccps55kZCFJ5O/ty72z7vHuE1gMEeUYwFgel/+kbTTjeGXDTzUaBU X-Received: by 2002:a05:6808:4484:b0:3b7:6a5:3cc6 with SMTP id eq4-20020a056808448400b003b706a53cc6mr652823oib.44.1701371478098; Thu, 30 Nov 2023 11:11:18 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1701371478; cv=none; d=google.com; s=arc-20160816; b=arzRuPWIYXhwIkQzr2uG9szi6H0A/BpgvgsWgI7U3ppWYd+dHVr0NSJPjV0ZkFr89s vDtb0Vo/4DRavAqq/Rz+fidD7sOEnrQ5z3b3C2E5MzBxwsHD+0kv7dQ40iNepW21vyyy MDfNfuVwraKsS8nWdkfa47qcvZnGnYLt9lRBxHfSKfLW7LOIKourq9CdJoRGx93z7j0H 2ZY6lOStFc42gCysGi+QTwMke7cb/038v96JW4G40XpG+wfda1oky5nEBwr//5VF7Agm CaIGlvDz/F3QGN3F6bSKzCbWG1Iti9z8jBt8Y6/LZYz5qVNSSGR/jh+kNGTb9ZRldHE3 YQkQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:subject:content-transfer-encoding:mime-version :references:in-reply-to:message-id:date:cc:to:from:dkim-signature; bh=AEm+IaX9A4uvKk4v1WSGhlTmQPSBKm4TU4lKJNYntmE=; fh=xaQPD6qLSjFCQPlkdqf6j0Kq0gyZF2Liq76NGJH/bQ8=; b=WijndwT+WmvZQCaKpbZmx3Sl8fF9H7sMxRL1kui61qlBHpnUoJLiSInpPFq5AIzO+v /v2jmd5uZscGIDIZDSMj2dwgzPtTaAYoKekfX89qPlAGPb7GmDPgCMLTA2IiKZaQ4n4K U3DO636E72sL/oUWzHTiNEGwrav+sF56WjSc1X4PgpuNjDuk8kv9gCewCPVjIAH/JOil 2oqYG0L5Zol0OQCPrwJbdlgHA6XYABDQ3sTzdpK6Q36fUZx81aGJuVJeH25qJNM5QkkV mHMGrxIvBppV7SLNjzTDEIFLldKp4FlgaM2qmdJbkzDi5bkJv17FzOu9JnLZjM0E9jv2 ccCA== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@hugovil.com header.s=x header.b=Gm9kSMoJ; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:8 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: from fry.vger.email (fry.vger.email. [2620:137:e000::3:8]) by mx.google.com with ESMTPS id z27-20020a630a5b000000b005c1b2c911f0si1849754pgk.328.2023.11.30.11.11.08 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 30 Nov 2023 11:11:18 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:8 as permitted sender) client-ip=2620:137:e000::3:8; Authentication-Results: mx.google.com; dkim=pass header.i=@hugovil.com header.s=x header.b=Gm9kSMoJ; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:8 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: from out1.vger.email (depot.vger.email [IPv6:2620:137:e000::3:0]) by fry.vger.email (Postfix) with ESMTP id EF14A8021EF8; Thu, 30 Nov 2023 11:11:05 -0800 (PST) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.11 at fry.vger.email Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1346806AbjK3TKy (ORCPT <rfc822;ruipengqi7@gmail.com> + 99 others); Thu, 30 Nov 2023 14:10:54 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:57850 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232101AbjK3TKv (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Thu, 30 Nov 2023 14:10:51 -0500 Received: from mail.hugovil.com (mail.hugovil.com [162.243.120.170]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id B0E7910E4; Thu, 30 Nov 2023 11:10:57 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=hugovil.com ; s=x; h=Subject:Content-Transfer-Encoding:MIME-Version:Message-Id:Date:Cc:To :From:subject:date:message-id:reply-to; bh=AEm+IaX9A4uvKk4v1WSGhlTmQPSBKm4TU4lKJNYntmE=; b=Gm9kSMoJBgC1JfrodPB2e0/Yvp 8xiv7QWolS5WzDxSL3I06gC7hYgmBUIQVJpvlghkP1mO1D8Yt4NxZL3BXM5/fO9h7yryINSJ4MWYd jSi5d3SarkgfCVVtil2rAEX3fa9mreEqZQAsVbEovOK2mvP8qLxMCXxFe9UI+UAsMOZM=; Received: from modemcable168.174-80-70.mc.videotron.ca ([70.80.174.168]:48272 helo=pettiford.lan) by mail.hugovil.com with esmtpa (Exim 4.92) (envelope-from <hugo@hugovil.com>) id 1r8mR4-0003sb-No; Thu, 30 Nov 2023 14:10:55 -0500 From: Hugo Villeneuve <hugo@hugovil.com> To: gregkh@linuxfoundation.org, jirislaby@kernel.org, hvilleneuve@dimonoff.com Cc: linux-kernel@vger.kernel.org, linux-serial@vger.kernel.org, hugo@hugovil.com, stable@vger.kernel.org, Andy Shevchenko <andy.shevchenko@gmail.com> Date: Thu, 30 Nov 2023 14:10:43 -0500 Message-Id: <20231130191050.3165862-2-hugo@hugovil.com> X-Mailer: git-send-email 2.39.2 In-Reply-To: <20231130191050.3165862-1-hugo@hugovil.com> References: <20231130191050.3165862-1-hugo@hugovil.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-SA-Exim-Connect-IP: 70.80.174.168 X-SA-Exim-Mail-From: hugo@hugovil.com X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on fry.vger.email X-Spam-Level: X-Spam-Status: No, score=-0.8 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE,URIBL_CSS autolearn=unavailable autolearn_force=no version=3.4.6 Subject: [PATCH 1/7] serial: sc16is7xx: fix snprintf format specifier in sc16is7xx_regmap_name() X-SA-Exim-Version: 4.2.1 (built Wed, 08 May 2019 21:11:16 +0000) X-SA-Exim-Scanned: Yes (on mail.hugovil.com) 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 (fry.vger.email [0.0.0.0]); Thu, 30 Nov 2023 11:11:06 -0800 (PST) X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1784017299016469442 X-GMAIL-MSGID: 1784017299016469442 |
Series |
serial: sc16is7xx and max310x: regmap fixes and improvements
|
|
Commit Message
Hugo Villeneuve
Nov. 30, 2023, 7:10 p.m. UTC
From: Hugo Villeneuve <hvilleneuve@dimonoff.com> Change snprint format specifier from %d to %u since port_id is unsigned. Fixes: 3837a0379533 ("serial: sc16is7xx: improve regmap debugfs by using one regmap per port") Cc: stable@vger.kernel.org # 6.1.x: 3837a03 serial: sc16is7xx: improve regmap debugfs by using one regmap per port Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com> Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com> --- I did not originally add a "Cc: stable" tag for commit 3837a0379533 ("serial: sc16is7xx: improve regmap debugfs by using one regmap per port") as it was intended only to improve debugging using debugfs. But since then, I have been able to confirm that it also fixes a long standing bug in our system where the Tx interrupt are no longer enabled at some point when transmitting large RS-485 paquets (> 64 bytes, which is the size of the FIFO). I have been investigating why, but so far I haven't found the exact cause, altough I suspect it has something to do with regmap caching. Therefore, I have added it as a prerequisite for this patch so that it is automatically added to the stable kernels. --- drivers/tty/serial/sc16is7xx.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Comments
Hi Hugo, kernel test robot noticed the following build warnings: [auto build test WARNING on d804987153e7bedf503f8e4ba649afe52cfd7f6d] url: https://github.com/intel-lab-lkp/linux/commits/Hugo-Villeneuve/serial-sc16is7xx-fix-snprintf-format-specifier-in-sc16is7xx_regmap_name/20231201-031413 base: d804987153e7bedf503f8e4ba649afe52cfd7f6d patch link: https://lore.kernel.org/r/20231130191050.3165862-2-hugo%40hugovil.com patch subject: [PATCH 1/7] serial: sc16is7xx: fix snprintf format specifier in sc16is7xx_regmap_name() config: x86_64-buildonly-randconfig-001-20231201 (https://download.01.org/0day-ci/archive/20231206/202312061443.Cknef7Uq-lkp@intel.com/config) compiler: gcc-12 (Debian 12.2.0-14) 12.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231206/202312061443.Cknef7Uq-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202312061443.Cknef7Uq-lkp@intel.com/ All warnings (new ones prefixed by >>): drivers/tty/serial/sc16is7xx.c: In function 'sc16is7xx_i2c_probe': >> drivers/tty/serial/sc16is7xx.c:1703:41: warning: '%u' directive output may be truncated writing between 1 and 10 bytes into a region of size 2 [-Wformat-truncation=] 1703 | snprintf(buf, sizeof(buf), "port%u", port_id); | ^~ In function 'sc16is7xx_regmap_name', inlined from 'sc16is7xx_i2c_probe' at drivers/tty/serial/sc16is7xx.c:1805:17: drivers/tty/serial/sc16is7xx.c:1703:36: note: directive argument in the range [0, 4294967294] 1703 | snprintf(buf, sizeof(buf), "port%u", port_id); | ^~~~~~~~ drivers/tty/serial/sc16is7xx.c:1703:9: note: 'snprintf' output between 6 and 15 bytes into a destination of size 6 1703 | snprintf(buf, sizeof(buf), "port%u", port_id); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ vim +1703 drivers/tty/serial/sc16is7xx.c 1698 1699 static const char *sc16is7xx_regmap_name(unsigned int port_id) 1700 { 1701 static char buf[6]; 1702 > 1703 snprintf(buf, sizeof(buf), "port%u", port_id); 1704 1705 return buf; 1706 } 1707
On Thu, Nov 30, 2023 at 02:10:43PM -0500, Hugo Villeneuve wrote: > From: Hugo Villeneuve <hvilleneuve@dimonoff.com> > > Change snprint format specifier from %d to %u since port_id is unsigned. > > Fixes: 3837a0379533 ("serial: sc16is7xx: improve regmap debugfs by using one regmap per port") > Cc: stable@vger.kernel.org # 6.1.x: 3837a03 serial: sc16is7xx: improve regmap debugfs by using one regmap per port > Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com> > Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com> > --- > I did not originally add a "Cc: stable" tag for commit 3837a0379533 ("serial: sc16is7xx: improve regmap debugfs by using one regmap per port") > as it was intended only to improve debugging using debugfs. But > since then, I have been able to confirm that it also fixes a long standing > bug in our system where the Tx interrupt are no longer enabled at some > point when transmitting large RS-485 paquets (> 64 bytes, which is the size > of the FIFO). I have been investigating why, but so far I haven't found the > exact cause, altough I suspect it has something to do with regmap caching. > Therefore, I have added it as a prerequisite for this patch so that it is > automatically added to the stable kernels. Looks like the 0-day test bot found problems with this, so I'll hold off on taking this patch and the rest of the series until that's fixed up with a new version of this series. thanks, greg k-h
On Thu, Nov 30, 2023 at 02:10:43PM -0500, Hugo Villeneuve wrote: > From: Hugo Villeneuve <hvilleneuve@dimonoff.com> > > Change snprint format specifier from %d to %u since port_id is unsigned. > > Fixes: 3837a0379533 ("serial: sc16is7xx: improve regmap debugfs by using one regmap per port") > Cc: stable@vger.kernel.org # 6.1.x: 3837a03 serial: sc16is7xx: improve regmap debugfs by using one regmap per port > Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com> > Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com> > --- > I did not originally add a "Cc: stable" tag for commit 3837a0379533 ("serial: sc16is7xx: improve regmap debugfs by using one regmap per port") > as it was intended only to improve debugging using debugfs. But > since then, I have been able to confirm that it also fixes a long standing > bug in our system where the Tx interrupt are no longer enabled at some > point when transmitting large RS-485 paquets (> 64 bytes, which is the size > of the FIFO). I have been investigating why, but so far I haven't found the > exact cause, altough I suspect it has something to do with regmap caching. > Therefore, I have added it as a prerequisite for this patch so that it is > automatically added to the stable kernels. As you are splitting fixes from non-fixes in this series, please resend this as 2 different series, one that I can apply now to my tty-linus branch to get merged for 6.7-final, and one that can go into tty-next for 6.8-rc1. Mixing them up here just ensures that they all would get applied to tty-next. thanks, greg k-h
On Thu, 7 Dec 2023 10:44:33 +0900 Greg KH <gregkh@linuxfoundation.org> wrote: > On Thu, Nov 30, 2023 at 02:10:43PM -0500, Hugo Villeneuve wrote: > > From: Hugo Villeneuve <hvilleneuve@dimonoff.com> > > > > Change snprint format specifier from %d to %u since port_id is unsigned. > > > > Fixes: 3837a0379533 ("serial: sc16is7xx: improve regmap debugfs by using one regmap per port") > > Cc: stable@vger.kernel.org # 6.1.x: 3837a03 serial: sc16is7xx: improve regmap debugfs by using one regmap per port > > Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com> > > Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com> > > --- > > I did not originally add a "Cc: stable" tag for commit 3837a0379533 ("serial: sc16is7xx: improve regmap debugfs by using one regmap per port") > > as it was intended only to improve debugging using debugfs. But > > since then, I have been able to confirm that it also fixes a long standing > > bug in our system where the Tx interrupt are no longer enabled at some > > point when transmitting large RS-485 paquets (> 64 bytes, which is the size > > of the FIFO). I have been investigating why, but so far I haven't found the > > exact cause, altough I suspect it has something to do with regmap caching. > > Therefore, I have added it as a prerequisite for this patch so that it is > > automatically added to the stable kernels. > > Looks like the 0-day test bot found problems with this, so I'll hold off > on taking this patch and the rest of the series until that's fixed up > with a new version of this series. No problem, I am on it. Hugo
On Thu, 7 Dec 2023 10:45:48 +0900 Greg KH <gregkh@linuxfoundation.org> wrote: > On Thu, Nov 30, 2023 at 02:10:43PM -0500, Hugo Villeneuve wrote: > > From: Hugo Villeneuve <hvilleneuve@dimonoff.com> > > > > Change snprint format specifier from %d to %u since port_id is unsigned. > > > > Fixes: 3837a0379533 ("serial: sc16is7xx: improve regmap debugfs by using one regmap per port") > > Cc: stable@vger.kernel.org # 6.1.x: 3837a03 serial: sc16is7xx: improve regmap debugfs by using one regmap per port > > Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com> > > Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com> > > --- > > I did not originally add a "Cc: stable" tag for commit 3837a0379533 ("serial: sc16is7xx: improve regmap debugfs by using one regmap per port") > > as it was intended only to improve debugging using debugfs. But > > since then, I have been able to confirm that it also fixes a long standing > > bug in our system where the Tx interrupt are no longer enabled at some > > point when transmitting large RS-485 paquets (> 64 bytes, which is the size > > of the FIFO). I have been investigating why, but so far I haven't found the > > exact cause, altough I suspect it has something to do with regmap caching. > > Therefore, I have added it as a prerequisite for this patch so that it is > > automatically added to the stable kernels. > > As you are splitting fixes from non-fixes in this series, please resend > this as 2 different series, one that I can apply now to my tty-linus > branch to get merged for 6.7-final, and one that can go into tty-next > for 6.8-rc1. Mixing them up here just ensures that they all would get > applied to tty-next. Ok, makes sense. Will do after I fix the 0-day issue. Thank you, Hugo.
On Wed, 6 Dec 2023 14:29:39 +0800 kernel test robot <lkp@intel.com> wrote: > Hi Hugo, > > kernel test robot noticed the following build warnings: > > [auto build test WARNING on d804987153e7bedf503f8e4ba649afe52cfd7f6d] > > url: https://github.com/intel-lab-lkp/linux/commits/Hugo-Villeneuve/serial-sc16is7xx-fix-snprintf-format-specifier-in-sc16is7xx_regmap_name/20231201-031413 > base: d804987153e7bedf503f8e4ba649afe52cfd7f6d > patch link: https://lore.kernel.org/r/20231130191050.3165862-2-hugo%40hugovil.com > patch subject: [PATCH 1/7] serial: sc16is7xx: fix snprintf format specifier in sc16is7xx_regmap_name() > config: x86_64-buildonly-randconfig-001-20231201 (https://download.01.org/0day-ci/archive/20231206/202312061443.Cknef7Uq-lkp@intel.com/config) > compiler: gcc-12 (Debian 12.2.0-14) 12.2.0 > reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231206/202312061443.Cknef7Uq-lkp@intel.com/reproduce) > > If you fix the issue in a separate patch/commit (i.e. not just a new version of > the same patch/commit), kindly add following tags > | Reported-by: kernel test robot <lkp@intel.com> > | Closes: https://lore.kernel.org/oe-kbuild-all/202312061443.Cknef7Uq-lkp@intel.com/ > > All warnings (new ones prefixed by >>): > > drivers/tty/serial/sc16is7xx.c: In function 'sc16is7xx_i2c_probe': > >> drivers/tty/serial/sc16is7xx.c:1703:41: warning: '%u' directive output may be truncated writing between 1 and 10 bytes into a region of size 2 [-Wformat-truncation=] > 1703 | snprintf(buf, sizeof(buf), "port%u", port_id); > | ^~ > In function 'sc16is7xx_regmap_name', > inlined from 'sc16is7xx_i2c_probe' at drivers/tty/serial/sc16is7xx.c:1805:17: > drivers/tty/serial/sc16is7xx.c:1703:36: note: directive argument in the range [0, 4294967294] > 1703 | snprintf(buf, sizeof(buf), "port%u", port_id); > | ^~~~~~~~ > drivers/tty/serial/sc16is7xx.c:1703:9: note: 'snprintf' output between 6 and 15 bytes into a destination of size 6 > 1703 | snprintf(buf, sizeof(buf), "port%u", port_id); > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ Hi, the only solution I could find is to add this line just before snprintf: BUG_ON(port_id > MAX310X_MAX_PORTS); it allows us to have the smallest buffer size possible. One other solution would be to change port_id from "unsigned int" to "u8", and increase the buffer by an additional 2 bytes to silence the warning, but then wasting 2 bytes for each channel, like so: static const char *max310x_regmap_name(u8 port_id) { static char buf[ sizeof(MAX310X_PORT_NAME_SUFFIX __stringify(UCHAR_MAX))]; I prefer solution 1, unless there is another solution that I am unaware of. Hugo. > vim +1703 drivers/tty/serial/sc16is7xx.c > > 1698 > 1699 static const char *sc16is7xx_regmap_name(unsigned int port_id) > 1700 { > 1701 static char buf[6]; > 1702 > > 1703 snprintf(buf, sizeof(buf), "port%u", port_id); > 1704 > 1705 return buf; > 1706 } > 1707 > > -- > 0-DAY CI Kernel Test Service > https://github.com/intel/lkp-tests/wiki >
On Thu, Dec 7, 2023 at 7:52 PM Hugo Villeneuve <hugo@hugovil.com> wrote: > On Wed, 6 Dec 2023 14:29:39 +0800 > kernel test robot <lkp@intel.com> wrote: ... > > drivers/tty/serial/sc16is7xx.c: In function 'sc16is7xx_i2c_probe': > > >> drivers/tty/serial/sc16is7xx.c:1703:41: warning: '%u' directive output may be truncated writing between 1 and 10 bytes into a region of size 2 [-Wformat-truncation=] > > 1703 | snprintf(buf, sizeof(buf), "port%u", port_id); > > | ^~ > > In function 'sc16is7xx_regmap_name', > > inlined from 'sc16is7xx_i2c_probe' at drivers/tty/serial/sc16is7xx.c:1805:17: > > drivers/tty/serial/sc16is7xx.c:1703:36: note: directive argument in the range [0, 4294967294] > > 1703 | snprintf(buf, sizeof(buf), "port%u", port_id); > > | ^~~~~~~~ > > drivers/tty/serial/sc16is7xx.c:1703:9: note: 'snprintf' output between 6 and 15 bytes into a destination of size 6 > > 1703 | snprintf(buf, sizeof(buf), "port%u", port_id); > > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > Hi, > the only solution I could find is to add this line just before snprintf: > > BUG_ON(port_id > MAX310X_MAX_PORTS); > > it allows us to have the smallest buffer size possible. > > One other solution would be to change port_id from "unsigned int" > to "u8", and increase the buffer by an additional 2 bytes to silence > the warning, but then wasting 2 bytes for each channel, like so: I didn't get this. It's a buffer that is rewritten on each port (why is it even static?). Just make sure it's enough for any given number and drop the static. ... While at it, can you look at the following items to improve? - sc16is7xx_alloc_line() can be updated to use IDA framework - move return xxx; to the default cases in a few functions - if (div > 0xffff) { --> if (div >= BIT(16)) { as it better shows why the limit is that (we have only 16 bits for the divider) - do {} while (0) in the sc16is7xx_port_irq, WTH?! - while (1) { -- do { } while (keep_polling); in sc16is7xx_irq() - use in_range() in sc16is7xx_setup_mctrl_ports() ? (maybe not, dunno) - for (i--; i >= 0; i--) { --> while (i--) { - use spi_get_device_match_data() and i2c_get_match_data() - 15000000 --> 15 * HZ_PER_MHZ ? - dropping MODULE_ALIAS (and fix the ID tables, _if_ needed) - split the code to the core / main + SPI + I2C glue drivers * These just come on the first glance at the code, perhaps there is more room to improve. -- With Best Regards, Andy Shevchenko
On Thu, 7 Dec 2023 20:24:45 +0200 Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Thu, Dec 7, 2023 at 7:52 PM Hugo Villeneuve <hugo@hugovil.com> wrote: > > On Wed, 6 Dec 2023 14:29:39 +0800 > > kernel test robot <lkp@intel.com> wrote: > > ... > > > > drivers/tty/serial/sc16is7xx.c: In function 'sc16is7xx_i2c_probe': > > > >> drivers/tty/serial/sc16is7xx.c:1703:41: warning: '%u' directive output may be truncated writing between 1 and 10 bytes into a region of size 2 [-Wformat-truncation=] > > > 1703 | snprintf(buf, sizeof(buf), "port%u", port_id); > > > | ^~ > > > In function 'sc16is7xx_regmap_name', > > > inlined from 'sc16is7xx_i2c_probe' at drivers/tty/serial/sc16is7xx.c:1805:17: > > > drivers/tty/serial/sc16is7xx.c:1703:36: note: directive argument in the range [0, 4294967294] > > > 1703 | snprintf(buf, sizeof(buf), "port%u", port_id); > > > | ^~~~~~~~ > > > drivers/tty/serial/sc16is7xx.c:1703:9: note: 'snprintf' output between 6 and 15 bytes into a destination of size 6 > > > 1703 | snprintf(buf, sizeof(buf), "port%u", port_id); > > > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > > > Hi, > > the only solution I could find is to add this line just before snprintf: > > > > BUG_ON(port_id > MAX310X_MAX_PORTS); > > > > it allows us to have the smallest buffer size possible. > > > > One other solution would be to change port_id from "unsigned int" > > to "u8", and increase the buffer by an additional 2 bytes to silence > > the warning, but then wasting 2 bytes for each channel, like so: > > I didn't get this. It's a buffer that is rewritten on each port (why > is it even static?). Just make sure it's enough for any given number > and drop the static. Yes, using static is not appropriate, as regmap will copy each name into its internal buffer. I will drop the static and refactor the code accordingly. > While at it, can you look at the following items to improve? > - sc16is7xx_alloc_line() can be updated to use IDA framework > - move return xxx; to the default cases in a few functions > - if (div > 0xffff) { --> if (div >= BIT(16)) { as it better shows why > the limit is that (we have only 16 bits for the divider) > - do {} while (0) in the sc16is7xx_port_irq, WTH?! > - while (1) { -- do { } while (keep_polling); in sc16is7xx_irq() > - use in_range() in sc16is7xx_setup_mctrl_ports() ? (maybe not, dunno) > - for (i--; i >= 0; i--) { --> while (i--) { > - use spi_get_device_match_data() and i2c_get_match_data() > - 15000000 --> 15 * HZ_PER_MHZ ? > - dropping MODULE_ALIAS (and fix the ID tables, _if_ needed) > - split the code to the core / main + SPI + I2C glue drivers > > * These just come on the first glance at the code, perhaps there is > more room to improve. Ok, no problem, I will have a look at it. Thank you, Hugo Villeneuve
From: Hugo Villeneuve > Sent: 07 December 2023 17:53 ... > > kernel test robot noticed the following build warnings: > > > > [auto build test WARNING on d804987153e7bedf503f8e4ba649afe52cfd7f6d] > > > > url: https://github.com/intel-lab-lkp/linux/commits/Hugo-Villeneuve/serial-sc16is7xx-fix- > snprintf-format-specifier-in-sc16is7xx_regmap_name/20231201-031413 > > base: d804987153e7bedf503f8e4ba649afe52cfd7f6d > > patch link: https://lore.kernel.org/r/20231130191050.3165862-2-hugo%40hugovil.com > > patch subject: [PATCH 1/7] serial: sc16is7xx: fix snprintf format specifier in > sc16is7xx_regmap_name() > > config: x86_64-buildonly-randconfig-001-20231201 (https://download.01.org/0day- > ci/archive/20231206/202312061443.Cknef7Uq-lkp@intel.com/config) > > compiler: gcc-12 (Debian 12.2.0-14) 12.2.0 > > reproduce (this is a W=1 build): (https://download.01.org/0day- > ci/archive/20231206/202312061443.Cknef7Uq-lkp@intel.com/reproduce) > > > > If you fix the issue in a separate patch/commit (i.e. not just a new version of > > the same patch/commit), kindly add following tags > > | Reported-by: kernel test robot <lkp@intel.com> > > | Closes: https://lore.kernel.org/oe-kbuild-all/202312061443.Cknef7Uq-lkp@intel.com/ > > > > All warnings (new ones prefixed by >>): > > > > drivers/tty/serial/sc16is7xx.c: In function 'sc16is7xx_i2c_probe': > > >> drivers/tty/serial/sc16is7xx.c:1703:41: warning: '%u' directive output may be truncated writing > between 1 and 10 bytes into a region of size 2 [-Wformat-truncation=] > > 1703 | snprintf(buf, sizeof(buf), "port%u", port_id); > > | ^~ > > In function 'sc16is7xx_regmap_name', > > inlined from 'sc16is7xx_i2c_probe' at drivers/tty/serial/sc16is7xx.c:1805:17: > > drivers/tty/serial/sc16is7xx.c:1703:36: note: directive argument in the range [0, 4294967294] > > 1703 | snprintf(buf, sizeof(buf), "port%u", port_id); > > | ^~~~~~~~ > > drivers/tty/serial/sc16is7xx.c:1703:9: note: 'snprintf' output between 6 and 15 bytes into a > destination of size 6 > > 1703 | snprintf(buf, sizeof(buf), "port%u", port_id); > > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > Hi, > the only solution I could find is to add this line just before snprintf: > > BUG_ON(port_id > MAX310X_MAX_PORTS); > > it allows us to have the smallest buffer size possible. Or "port%c", '0' + port_id); Or maybe: size_t buflen = sizeof (buf); OPTIMIZER_HIDE_VAR(buflen); snprintf(buf, buflen, fmt, args); See https://godbolt.org/z/Wjz3xG5c4 Maybe there should be snprintf_may_truncate() (etc) in one of the headers. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Thu, 7 Dec 2023 20:24:45 +0200 Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Thu, Dec 7, 2023 at 7:52 PM Hugo Villeneuve <hugo@hugovil.com> wrote: > > On Wed, 6 Dec 2023 14:29:39 +0800 > > kernel test robot <lkp@intel.com> wrote: > > ... > > > > drivers/tty/serial/sc16is7xx.c: In function 'sc16is7xx_i2c_probe': > > > >> drivers/tty/serial/sc16is7xx.c:1703:41: warning: '%u' directive output may be truncated writing between 1 and 10 bytes into a region of size 2 [-Wformat-truncation=] > > > 1703 | snprintf(buf, sizeof(buf), "port%u", port_id); > > > | ^~ > > > In function 'sc16is7xx_regmap_name', > > > inlined from 'sc16is7xx_i2c_probe' at drivers/tty/serial/sc16is7xx.c:1805:17: > > > drivers/tty/serial/sc16is7xx.c:1703:36: note: directive argument in the range [0, 4294967294] > > > 1703 | snprintf(buf, sizeof(buf), "port%u", port_id); > > > | ^~~~~~~~ > > > drivers/tty/serial/sc16is7xx.c:1703:9: note: 'snprintf' output between 6 and 15 bytes into a destination of size 6 > > > 1703 | snprintf(buf, sizeof(buf), "port%u", port_id); > > > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > > > Hi, > > the only solution I could find is to add this line just before snprintf: > > > > BUG_ON(port_id > MAX310X_MAX_PORTS); > > > > it allows us to have the smallest buffer size possible. > > > > One other solution would be to change port_id from "unsigned int" > > to "u8", and increase the buffer by an additional 2 bytes to silence > > the warning, but then wasting 2 bytes for each channel, like so: > > I didn't get this. It's a buffer that is rewritten on each port (why > is it even static?). Just make sure it's enough for any given number > and drop the static. > > ... > > While at it, can you look at the following items to improve? > - sc16is7xx_alloc_line() can be updated to use IDA framework > - move return xxx; to the default cases in a few functions > - if (div > 0xffff) { --> if (div >= BIT(16)) { as it better shows why > the limit is that (we have only 16 bits for the divider) > - do {} while (0) in the sc16is7xx_port_irq, WTH?! > - while (1) { -- do { } while (keep_polling); in sc16is7xx_irq() > - use in_range() in sc16is7xx_setup_mctrl_ports() ? (maybe not, dunno) > - for (i--; i >= 0; i--) { --> while (i--) { > - use spi_get_device_match_data() and i2c_get_match_data() > - 15000000 --> 15 * HZ_PER_MHZ ? > - dropping MODULE_ALIAS (and fix the ID tables, _if_ needed) > - split the code to the core / main + SPI + I2C glue drivers > > * These just come on the first glance at the code, perhaps there is > more room to improve. Hi Andy, just to let you know that I have implemented almost all of the fixes / improvements. I will submit them once V2 of this current series lands in Greg's next tree. However, for sc16is7xx_alloc_line(), I looked at using the IDA framework but it doesn't seem possible because there is no IDA function to search if a bit is set, which is a needed functionality. Hugo Villeneuve
On Tue, Dec 12, 2023 at 10:03 PM Hugo Villeneuve <hugo@hugovil.com> wrote: > On Thu, 7 Dec 2023 20:24:45 +0200 > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Thu, Dec 7, 2023 at 7:52 PM Hugo Villeneuve <hugo@hugovil.com> wrote: ... > > While at it, can you look at the following items to improve? > > - sc16is7xx_alloc_line() can be updated to use IDA framework > > - move return xxx; to the default cases in a few functions > > - if (div > 0xffff) { --> if (div >= BIT(16)) { as it better shows why > > the limit is that (we have only 16 bits for the divider) > > - do {} while (0) in the sc16is7xx_port_irq, WTH?! > > - while (1) { -- do { } while (keep_polling); in sc16is7xx_irq() > > - use in_range() in sc16is7xx_setup_mctrl_ports() ? (maybe not, dunno) > > - for (i--; i >= 0; i--) { --> while (i--) { > > - use spi_get_device_match_data() and i2c_get_match_data() > > - 15000000 --> 15 * HZ_PER_MHZ ? > > - dropping MODULE_ALIAS (and fix the ID tables, _if_ needed) > > - split the code to the core / main + SPI + I2C glue drivers > > > > * These just come on the first glance at the code, perhaps there is > > more room to improve. > > Hi Andy, > just to let you know that I have implemented almost all of the fixes / > improvements. I will submit them once V2 of this current series > lands in Greg's next tree. Hooray! > However, for sc16is7xx_alloc_line(), I looked at using the IDA framework > but it doesn't seem possible because there is no IDA function > to search if a bit is set, which is a needed functionality. It can be done via trying to get it, but probably it's uglier than current behaviour. Okay, let's leave it as is for now.
diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c index 10e90a7774f0..8e5baf2f6ec6 100644 --- a/drivers/tty/serial/sc16is7xx.c +++ b/drivers/tty/serial/sc16is7xx.c @@ -1700,7 +1700,7 @@ static const char *sc16is7xx_regmap_name(unsigned int port_id) { static char buf[6]; - snprintf(buf, sizeof(buf), "port%d", port_id); + snprintf(buf, sizeof(buf), "port%u", port_id); return buf; }