Message ID | 84128a3c83654493f637b8349153af10d69e2752.1706118776.git.babu.moger@amd.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel+bounces-37472-ouuuleilei=gmail.com@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a05:7300:2553:b0:103:945f:af90 with SMTP id p19csp1154295dyi; Wed, 24 Jan 2024 09:55:30 -0800 (PST) X-Google-Smtp-Source: AGHT+IGJroquKhr+gTlgoGYWXukQc4G2v36vzbR4QowzFvaX3yKFgivo8hp8ctc9CKuNOaGhlyjo X-Received: by 2002:ac2:592c:0:b0:510:11b6:bddc with SMTP id v12-20020ac2592c000000b0051011b6bddcmr655332lfi.79.1706118930374; Wed, 24 Jan 2024 09:55:30 -0800 (PST) Received: from am.mirrors.kernel.org (am.mirrors.kernel.org. [2604:1380:4601:e00::3]) by mx.google.com with ESMTPS id o3-20020a170906774300b00a28e758c286si109542ejn.61.2024.01.24.09.55.30 for <ouuuleilei@gmail.com> (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 24 Jan 2024 09:55:30 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel+bounces-37472-ouuuleilei=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3; Authentication-Results: mx.google.com; dkim=pass header.i=@amd.com header.s=selector1 header.b=T4WiChGj; arc=fail (signature failed); spf=pass (google.com: domain of linux-kernel+bounces-37472-ouuuleilei=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) smtp.mailfrom="linux-kernel+bounces-37472-ouuuleilei=gmail.com@vger.kernel.org"; dmarc=pass (p=QUARANTINE sp=QUARANTINE dis=NONE) header.from=amd.com Received: from smtp.subspace.kernel.org (wormhole.subspace.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by am.mirrors.kernel.org (Postfix) with ESMTPS id CA4821F23417 for <ouuuleilei@gmail.com>; Wed, 24 Jan 2024 17:55:29 +0000 (UTC) Received: from localhost.localdomain (localhost.localdomain [127.0.0.1]) by smtp.subspace.kernel.org (Postfix) with ESMTP id DFE9C12AAF4; Wed, 24 Jan 2024 17:53:13 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=amd.com header.i=@amd.com header.b="T4WiChGj" Received: from NAM04-MW2-obe.outbound.protection.outlook.com (mail-mw2nam04on2072.outbound.protection.outlook.com [40.107.101.72]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id A5F257C08B; Wed, 24 Jan 2024 17:53:08 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=fail smtp.client-ip=40.107.101.72 ARC-Seal: i=2; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1706118790; cv=fail; b=e3IEu2N/9tlOYI0Kd6wlcKgSVKAMH4hanDyLbqGR5GlyfAOSJRcojWbZdaHYvdPfcwjQqSe3R445G3/3/yu7BUecsK2c5EyuKI6i/BBW01aTcGXaAdIOVxFLjIHCwT5p2O6JzEumKc9gKq7WIU1sxKeVsv9pdcz3cUNH6P/6KDY= ARC-Message-Signature: i=2; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1706118790; c=relaxed/simple; bh=pLXEeu5AUTY0lXQVFbSlPTGM8hifEOLN8y55qnXpz7E=; h=From:To:CC:Subject:Date:Message-ID:In-Reply-To:References: MIME-Version:Content-Type; b=mej/RkUgypwYpcHV9cUtSYgZSnC1EkLbRcl84eb1vlupwcvsK1r8PCZ8Wjkg/OT9t2s7EoTEGB8qQnCLRsNByv12DUj3cOdlkyIhp2ySpGk1FSwT2VZ/aRcnw0vbct1H5B+oKm/TcDU7Y8KtMPnkUqD3l3Mhyb3F4t5o6NagO6s= ARC-Authentication-Results: i=2; smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=amd.com; spf=fail smtp.mailfrom=amd.com; dkim=pass (1024-bit key) header.d=amd.com header.i=@amd.com header.b=T4WiChGj; arc=fail smtp.client-ip=40.107.101.72 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=amd.com Authentication-Results: smtp.subspace.kernel.org; spf=fail smtp.mailfrom=amd.com ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=lNmTZAIPrWttnj++GATfYvb+lT43tFVIvGhjAmgCkjGtMu1DQr0aD5pSOCilGqc9PR8lcTAhLk8u3y6TMhRiWYoC24C40WZTHrxQtj7hkNUCKEcoUPWW8SYv5o+Sy83nORFB6+chCNcCDuZOEhuf59Pko4IpvgfvRUoT7mDCdf3uYIgOfvL0WJrKNEVBf/Uk0Qr7BvPk6DOcjDkTNkQ1wq9oNzEUUjXHITgVLXIdqHTgtKqlbGyx3naAtIVk0ozT28vpuNKta+p8wq8fvnqyNVg2ky5j9d99PRQVJP1aF7AyLvuHcY2oRSsTwGwpQFJTEOWYgCLiiWl0Q3kgjD/pYw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=CfB/j9lhCHvu8Pijbw6rDDaNOzMD/oxR8qbbNXqK2Jg=; b=U3Yv2Z+wvHcuzFpjeqxBd5IO6DvEoIDFI9OKMRwZyFClIE6JkeKwk+N6L9bUgMyq1bOCJI3H+xYvb2A2RDIXugGd/8064cAnbeCEbBLi0A52OpfpPJWaRqINsWH9iAMNidp2/JO+Y2EZHPEzpRpnNy62nbtDylDggKmYy7ozqHuPIr1aPUgpDKwovYOeMeblE7ltWpAswKFhn1me7bomdp11qQ0LQZKWFBH24+udcBAW8PJTGeG9hzJ5JJjPOYJS+m+yEr7mplL75h9FvNgHsNuWCioij+MpjMqguBrxXChOHB1AiwR/CsWAfNUb1Arx58H/QEdYco29Q+M2sJV7FA== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass (sender ip is 165.204.84.17) smtp.rcpttodomain=lwn.net smtp.mailfrom=amd.com; dmarc=pass (p=quarantine sp=quarantine pct=100) action=none header.from=amd.com; dkim=none (message not signed); arc=none (0) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=amd.com; s=selector1; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=CfB/j9lhCHvu8Pijbw6rDDaNOzMD/oxR8qbbNXqK2Jg=; b=T4WiChGjZsX4D59qzAK9Uk6ZwIbebXaduN7T2RcAnigsjWHGHOFu89OzTLwBJZ/Qeh3U/FszbN2RyWTTy/50/4BhntfEbh91kgrU5tZQJhoFOzdavTTTXc1d5Epf1kPFYFV7cJwwQfWgGlHveaxTatWhVwChhvU6PoJkPJLwlFw= Received: from BYAPR02CA0051.namprd02.prod.outlook.com (2603:10b6:a03:54::28) by SJ0PR12MB5456.namprd12.prod.outlook.com (2603:10b6:a03:3ae::12) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.7228.22; Wed, 24 Jan 2024 17:53:06 +0000 Received: from SJ5PEPF000001D4.namprd05.prod.outlook.com (2603:10b6:a03:54:cafe::fa) by BYAPR02CA0051.outlook.office365.com (2603:10b6:a03:54::28) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.7228.22 via Frontend Transport; Wed, 24 Jan 2024 17:53:06 +0000 X-MS-Exchange-Authentication-Results: spf=pass (sender IP is 165.204.84.17) smtp.mailfrom=amd.com; dkim=none (message not signed) header.d=none;dmarc=pass action=none header.from=amd.com; Received-SPF: Pass (protection.outlook.com: domain of amd.com designates 165.204.84.17 as permitted sender) receiver=protection.outlook.com; client-ip=165.204.84.17; helo=SATLEXMB04.amd.com; pr=C Received: from SATLEXMB04.amd.com (165.204.84.17) by SJ5PEPF000001D4.mail.protection.outlook.com (10.167.242.56) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.20.7202.16 via Frontend Transport; Wed, 24 Jan 2024 17:53:05 +0000 Received: from bmoger-ubuntu.amd.com (10.180.168.240) by SATLEXMB04.amd.com (10.181.40.145) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.34; Wed, 24 Jan 2024 11:53:01 -0600 From: Babu Moger <babu.moger@amd.com> To: <corbet@lwn.net>, <fenghua.yu@intel.com>, <reinette.chatre@intel.com>, <tglx@linutronix.de>, <mingo@redhat.com>, <bp@alien8.de>, <dave.hansen@linux.intel.com> CC: <x86@kernel.org>, <hpa@zytor.com>, <paulmck@kernel.org>, <rdunlap@infradead.org>, <tj@kernel.org>, <peterz@infradead.org>, <yanjiewtw@gmail.com>, <babu.moger@amd.com>, <kim.phillips@amd.com>, <lukas.bulwahn@gmail.com>, <seanjc@google.com>, <jmattson@google.com>, <leitao@debian.org>, <jpoimboe@kernel.org>, <rick.p.edgecombe@intel.com>, <kirill.shutemov@linux.intel.com>, <jithu.joseph@intel.com>, <kai.huang@intel.com>, <kan.liang@linux.intel.com>, <daniel.sneddon@linux.intel.com>, <pbonzini@redhat.com>, <sandipan.das@amd.com>, <ilpo.jarvinen@linux.intel.com>, <peternewman@google.com>, <maciej.wieczor-retman@intel.com>, <linux-doc@vger.kernel.org>, <linux-kernel@vger.kernel.org>, <eranian@google.com> Subject: [PATCH] x86/resctrl: Fix unneeded variable warning reported by kernel test robot Date: Wed, 24 Jan 2024 11:52:56 -0600 Message-ID: <84128a3c83654493f637b8349153af10d69e2752.1706118776.git.babu.moger@amd.com> X-Mailer: git-send-email 2.34.1 In-Reply-To: <202401241810.jbd8Ipa1-lkp@intel.com> References: <202401241810.jbd8Ipa1-lkp@intel.com> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: <linux-kernel.vger.kernel.org> List-Subscribe: <mailto:linux-kernel+subscribe@vger.kernel.org> List-Unsubscribe: <mailto:linux-kernel+unsubscribe@vger.kernel.org> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Content-Type: text/plain X-ClientProxiedBy: SATLEXMB03.amd.com (10.181.40.144) To SATLEXMB04.amd.com (10.181.40.145) X-EOPAttributedMessage: 0 X-MS-PublicTrafficType: Email X-MS-TrafficTypeDiagnostic: SJ5PEPF000001D4:EE_|SJ0PR12MB5456:EE_ X-MS-Office365-Filtering-Correlation-Id: d1891647-e394-45d1-87eb-08dc1d055150 X-MS-Exchange-SenderADCheck: 1 X-MS-Exchange-AntiSpam-Relay: 0 X-Microsoft-Antispam: BCL:0; X-Microsoft-Antispam-Message-Info: yIGm3rOYQ+JH1iC8EWmfQNLkj7cic4VZRlYlq9trMxp+LGOEV82biXgoqYyqiqhqtNlTZIxqXaB9+vOz7pvMbM8iX4I8OuVclub4Cgu9w0xfRY86ybhGarqSIOpNxMOgK2Hotx7qS6CmFuDbmSusbe0c5KYnehWbdmVSii3fixaxZG+7c3G32UuGkhIqyJhB4PimEhrAuWujHVCNBdK+ENON2hMwG0qh44El45b4lKCk8pxtyXrruEZYcLw/8na1gLpKs9OHeximvDOnUIfiKe+hxluCf1xXii1/Poft9phHSJmXE5Tk94oL5JpiBTajAvjpTVTd9ABBrEQvhwNO5RZbNT+yDkXQWp2wHxOKlUdQOK+0QqdcAdXCAWZZ77dvqMOug++XRc88KBsWlKlotW/QnQEfaGWuLp9U3kS3h8Jz8aNsBCDJSG7dbjlcJB0wZ0bozDmquYiizsIiRD8OcB/tbivmGSMuGvrIVZk6bHtQW2x3h82hBr72X82aPG5SDVcf9kh+e6MVh2HBmpdJMDnB8bgzdh1symkaoMMwhsNuCu3Yhb6cLkY4yo+PV8B0jqPssVvip3WXbFcE8bbdMiAjRTwgd2oA8CMX9+XTU6YntG17drAKxnbbCv5zKanVZ5/I1vvx/c58yhwH2WncQ57ica1x7zYcojn8CfiR047aLlJ6o2w/DbEgoxfJJlVfvzQGbxTzY9ZBx0b9EscBjI++hDJoSOEoG6RfIekhYIIo81fyGH0RqSG9JrDJVbra25drehW14oLdth7Aq88pewnWYhXQ5HR5DEQ3EOUmZV5CGM0THPTCz/gyIzZnLC2A X-Forefront-Antispam-Report: CIP:165.204.84.17;CTRY:US;LANG:en;SCL:1;SRV:;IPV:CAL;SFV:NSPM;H:SATLEXMB04.amd.com;PTR:InfoDomainNonexistent;CAT:NONE;SFS:(13230031)(4636009)(346002)(396003)(39860400002)(136003)(376002)(230173577357003)(230273577357003)(230922051799003)(82310400011)(1800799012)(64100799003)(186009)(451199024)(46966006)(36840700001)(40470700004)(110136005)(70206006)(316002)(70586007)(26005)(54906003)(40460700003)(40480700001)(2616005)(82740400003)(356005)(478600001)(966005)(81166007)(16526019)(7696005)(6666004)(8936002)(4326008)(8676002)(5660300002)(7406005)(7416002)(47076005)(83380400001)(44832011)(41300700001)(336012)(426003)(36860700001)(86362001)(2906002)(36756003)(36900700001);DIR:OUT;SFP:1101; X-OriginatorOrg: amd.com X-MS-Exchange-CrossTenant-OriginalArrivalTime: 24 Jan 2024 17:53:05.4635 (UTC) X-MS-Exchange-CrossTenant-Network-Message-Id: d1891647-e394-45d1-87eb-08dc1d055150 X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=3dd8961f-e488-4e60-8e11-a82d994e183d;Ip=[165.204.84.17];Helo=[SATLEXMB04.amd.com] X-MS-Exchange-CrossTenant-AuthSource: SJ5PEPF000001D4.namprd05.prod.outlook.com X-MS-Exchange-CrossTenant-AuthAs: Anonymous X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR12MB5456 X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1788995363118836702 X-GMAIL-MSGID: 1788995363118836702 |
Series |
x86/resctrl: Fix unneeded variable warning reported by kernel test robot
|
|
Commit Message
Moger, Babu
Jan. 24, 2024, 5:52 p.m. UTC
kernel test robot reported the following warning after the commit 54e35eb8611c ("x86/resctrl: Read supported bandwidth sources from CPUID"). cocci warnings: (new ones prefixed by >>) >> arch/x86/kernel/cpu/resctrl/rdtgroup.c:1621:5-8: Unneeded variable: "ret". Return " 0" on line 1655 Fix the warning by removing the variable "ret" and returning 0 directly. Reported-by: kernel test robot <lkp@intel.com> Closes: https://lore.kernel.org/oe-kbuild-all/202401241810.jbd8Ipa1-lkp@intel.com/ Signed-off-by: Babu Moger <babu.moger@amd.com> --- arch/x86/kernel/cpu/resctrl/rdtgroup.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
Comments
Hi Babu, Thank you for fixing this so promptly. I think the subject can just be: "x86/resctrl: Remove unneeded variable" On 1/24/2024 9:52 AM, Babu Moger wrote: > kernel test robot reported the following warning after the commit > 54e35eb8611c ("x86/resctrl: Read supported bandwidth sources from CPUID"). This can be confusing since it implies that the patch you mention introduces the issue but instead the variable has been unneeded since the original: 92bd5a139033 ("x86/resctrl: Add interface to write mbm_total_bytes_config") To help clarify you can mention this order of events and also add an appropriate "Fixes:" tag. > cocci warnings: (new ones prefixed by >>) >>> arch/x86/kernel/cpu/resctrl/rdtgroup.c:1621:5-8: Unneeded variable: "ret". Return " 0" on line 1655 > > Fix the warning by removing the variable "ret" and returning 0 directly. cocci warning was spot on*. This fix is not just a change to "make a warning go away" but instead fixing an actual problem. It can just be "Remove the unneeded variable and return 0 directly". > > Reported-by: kernel test robot <lkp@intel.com> > Closes: https://lore.kernel.org/oe-kbuild-all/202401241810.jbd8Ipa1-lkp@intel.com/ > Signed-off-by: Babu Moger <babu.moger@amd.com> Reinette * I'll add a private setup with the goal to catch these earlier.
On Wed, Jan 24, 2024 at 10:25:17AM -0800, Reinette Chatre wrote: > This can be confusing since it implies that the patch you mention > introduces the issue but instead the variable has been unneeded since > the original: > 92bd5a139033 ("x86/resctrl: Add interface to write mbm_total_bytes_config") What I said. :) > To help clarify you can mention this order of events and also add an > appropriate "Fixes:" tag. > > > cocci warnings: (new ones prefixed by >>) > >>> arch/x86/kernel/cpu/resctrl/rdtgroup.c:1621:5-8: Unneeded variable: "ret". Return " 0" on line 1655 > > > > Fix the warning by removing the variable "ret" and returning 0 directly. > > cocci warning was spot on*. This fix is not just a change to "make a > warning go away" but instead fixing an actual problem. > It can just be "Remove the unneeded variable and return 0 directly". I'll fix all up before applying. > * I'll add a private setup with the goal to catch these earlier. Except that it doesn't fire with the patch that added the code. It looks like the cocci script needs adjustment... Thx.
Hi Boris, On 1/24/2024 10:31 AM, Borislav Petkov wrote: > On Wed, Jan 24, 2024 at 10:25:17AM -0800, Reinette Chatre wrote: >> This can be confusing since it implies that the patch you mention >> introduces the issue but instead the variable has been unneeded since >> the original: >> 92bd5a139033 ("x86/resctrl: Add interface to write mbm_total_bytes_config") > > What I said. :) Right from the start, yes. > >> To help clarify you can mention this order of events and also add an >> appropriate "Fixes:" tag. >> >>> cocci warnings: (new ones prefixed by >>) >>>>> arch/x86/kernel/cpu/resctrl/rdtgroup.c:1621:5-8: Unneeded variable: "ret". Return " 0" on line 1655 >>> >>> Fix the warning by removing the variable "ret" and returning 0 directly. >> >> cocci warning was spot on*. This fix is not just a change to "make a >> warning go away" but instead fixing an actual problem. >> It can just be "Remove the unneeded variable and return 0 directly". > > I'll fix all up before applying. Thank you very much. For what it is worth, I do agree with the actual fix and you can add: Acked-by: Reinette Chatre <reinette.chatre@intel.com> > >> * I'll add a private setup with the goal to catch these earlier. > > Except that it doesn't fire with the patch that added the code. It looks > like the cocci script needs adjustment... Reinette
On Wed, Jan 24, 2024 at 10:51:49AM -0800, Reinette Chatre wrote: > Thank you very much. For what it is worth, I do agree with the actual fix > and you can add: > Acked-by: Reinette Chatre <reinette.chatre@intel.com> Ok, have a look at the below, pls, and lemme know if that's ok too. mbm_config_write_domain() only returns 0 so it can be void. So the callsite doesn't need to check retval either. Thx. --- From: Babu Moger <babu.moger@amd.com> Date: Wed, 24 Jan 2024 11:52:56 -0600 Subject: [PATCH] x86/resctrl: Remove redundant variable in mbm_config_write_domain() The kernel test robot reported the following warning after 54e35eb8611c ("x86/resctrl: Read supported bandwidth sources from CPUID"). even though the issue is present even in the original patch which added this function 92bd5a139033 ("x86/resctrl: Add interface to write mbm_total_bytes_config") $ make C=1 CHECK=scripts/coccicheck arch/x86/kernel/cpu/resctrl/rdtgroup.o ... arch/x86/kernel/cpu/resctrl/rdtgroup.c:1621:5-8: Unneeded variable: "ret". Return "0" on line 1655 Remove the local variable 'ret'. [ bp: Massage commit message, make mbm_config_write_domain() void. ] Fixes: 92bd5a139033 ("x86/resctrl: Add interface to write mbm_total_bytes_config") Closes: https://lore.kernel.org/oe-kbuild-all/202401241810.jbd8Ipa1-lkp@intel.com/ Reported-by: kernel test robot <lkp@intel.com> Signed-off-by: Babu Moger <babu.moger@amd.com> Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de> Acked-by: Reinette Chatre <reinette.chatre@intel.com> Link: https://lore.kernel.org/r/202401241810.jbd8Ipa1-lkp@intel.com --- arch/x86/kernel/cpu/resctrl/rdtgroup.c | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-) diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c index 2b69e560b05f..c33eb77b6d70 100644 --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c @@ -1614,11 +1614,10 @@ static void mon_event_config_write(void *info) wrmsr(MSR_IA32_EVT_CFG_BASE + index, mon_info->mon_config, 0); } -static int mbm_config_write_domain(struct rdt_resource *r, +static void mbm_config_write_domain(struct rdt_resource *r, struct rdt_domain *d, u32 evtid, u32 val) { struct mon_config_info mon_info = {0}; - int ret = 0; /* * Read the current config value first. If both are the same then @@ -1627,7 +1626,7 @@ static int mbm_config_write_domain(struct rdt_resource *r, mon_info.evtid = evtid; mondata_config_read(d, &mon_info); if (mon_info.mon_config == val) - goto out; + return; mon_info.mon_config = val; @@ -1650,9 +1649,6 @@ static int mbm_config_write_domain(struct rdt_resource *r, * mbm_local and mbm_total counts for all the RMIDs. */ resctrl_arch_reset_rmid_all(r, d); - -out: - return ret; } static int mon_config_write(struct rdt_resource *r, char *tok, u32 evtid) @@ -1661,7 +1657,6 @@ static int mon_config_write(struct rdt_resource *r, char *tok, u32 evtid) char *dom_str = NULL, *id_str; unsigned long dom_id, val; struct rdt_domain *d; - int ret = 0; next: if (!tok || tok[0] == '\0') @@ -1690,9 +1685,7 @@ static int mon_config_write(struct rdt_resource *r, char *tok, u32 evtid) list_for_each_entry(d, &r->domains, list) { if (d->id == dom_id) { - ret = mbm_config_write_domain(r, d, evtid, val); - if (ret) - return -EINVAL; + mbm_config_write_domain(r, d, evtid, val); goto next; } }
Hi Boris, On 1/24/24 13:14, Borislav Petkov wrote: > On Wed, Jan 24, 2024 at 10:51:49AM -0800, Reinette Chatre wrote: >> Thank you very much. For what it is worth, I do agree with the actual fix >> and you can add: >> Acked-by: Reinette Chatre <reinette.chatre@intel.com> > > Ok, have a look at the below, pls, and lemme know if that's ok too. > > mbm_config_write_domain() only returns 0 so it can be void. So the > callsite doesn't need to check retval either. Yes. Looks good. Compile tested also. Thanks > > Thx. > > --- > From: Babu Moger <babu.moger@amd.com> > Date: Wed, 24 Jan 2024 11:52:56 -0600 > Subject: [PATCH] x86/resctrl: Remove redundant variable in > mbm_config_write_domain() > > The kernel test robot reported the following warning after > > 54e35eb8611c ("x86/resctrl: Read supported bandwidth sources from CPUID"). > > even though the issue is present even in the original patch which added > this function > > 92bd5a139033 ("x86/resctrl: Add interface to write mbm_total_bytes_config") > > $ make C=1 CHECK=scripts/coccicheck arch/x86/kernel/cpu/resctrl/rdtgroup.o > ... > arch/x86/kernel/cpu/resctrl/rdtgroup.c:1621:5-8: Unneeded variable: "ret". Return "0" on line 1655 > > Remove the local variable 'ret'. > > [ bp: Massage commit message, make mbm_config_write_domain() void. ] > > Fixes: 92bd5a139033 ("x86/resctrl: Add interface to write mbm_total_bytes_config") > Closes: https://lore.kernel.org/oe-kbuild-all/202401241810.jbd8Ipa1-lkp@intel.com/ > Reported-by: kernel test robot <lkp@intel.com> > Signed-off-by: Babu Moger <babu.moger@amd.com> > Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de> > Acked-by: Reinette Chatre <reinette.chatre@intel.com> > Link: https://lore.kernel.org/r/202401241810.jbd8Ipa1-lkp@intel.com > --- > arch/x86/kernel/cpu/resctrl/rdtgroup.c | 13 +++---------- > 1 file changed, 3 insertions(+), 10 deletions(-) > > diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c > index 2b69e560b05f..c33eb77b6d70 100644 > --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c > +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c > @@ -1614,11 +1614,10 @@ static void mon_event_config_write(void *info) > wrmsr(MSR_IA32_EVT_CFG_BASE + index, mon_info->mon_config, 0); > } > > -static int mbm_config_write_domain(struct rdt_resource *r, > +static void mbm_config_write_domain(struct rdt_resource *r, > struct rdt_domain *d, u32 evtid, u32 val) > { > struct mon_config_info mon_info = {0}; > - int ret = 0; > > /* > * Read the current config value first. If both are the same then > @@ -1627,7 +1626,7 @@ static int mbm_config_write_domain(struct rdt_resource *r, > mon_info.evtid = evtid; > mondata_config_read(d, &mon_info); > if (mon_info.mon_config == val) > - goto out; > + return; > > mon_info.mon_config = val; > > @@ -1650,9 +1649,6 @@ static int mbm_config_write_domain(struct rdt_resource *r, > * mbm_local and mbm_total counts for all the RMIDs. > */ > resctrl_arch_reset_rmid_all(r, d); > - > -out: > - return ret; > } > > static int mon_config_write(struct rdt_resource *r, char *tok, u32 evtid) > @@ -1661,7 +1657,6 @@ static int mon_config_write(struct rdt_resource *r, char *tok, u32 evtid) > char *dom_str = NULL, *id_str; > unsigned long dom_id, val; > struct rdt_domain *d; > - int ret = 0; > > next: > if (!tok || tok[0] == '\0') > @@ -1690,9 +1685,7 @@ static int mon_config_write(struct rdt_resource *r, char *tok, u32 evtid) > > list_for_each_entry(d, &r->domains, list) { > if (d->id == dom_id) { > - ret = mbm_config_write_domain(r, d, evtid, val); > - if (ret) > - return -EINVAL; > + mbm_config_write_domain(r, d, evtid, val); > goto next; > } > }
Hi Boris, On 1/24/2024 11:14 AM, Borislav Petkov wrote: > On Wed, Jan 24, 2024 at 10:51:49AM -0800, Reinette Chatre wrote: >> Thank you very much. For what it is worth, I do agree with the actual fix >> and you can add: >> Acked-by: Reinette Chatre <reinette.chatre@intel.com> > > Ok, have a look at the below, pls, and lemme know if that's ok too. > > mbm_config_write_domain() only returns 0 so it can be void. So the > callsite doesn't need to check retval either. Thank you very much for looking further and catching this. You may already have taken care of this after sending this version, but I just have a couple of nitpick comments intended to help this patch in case it requires a clean slate from some checks. > > Thx. > > --- > From: Babu Moger <babu.moger@amd.com> > Date: Wed, 24 Jan 2024 11:52:56 -0600 > Subject: [PATCH] x86/resctrl: Remove redundant variable in > mbm_config_write_domain() > > The kernel test robot reported the following warning after > > 54e35eb8611c ("x86/resctrl: Read supported bandwidth sources from CPUID"). I think a "commit" prefix is required here and below. > > even though the issue is present even in the original patch which added > this function > > 92bd5a139033 ("x86/resctrl: Add interface to write mbm_total_bytes_config") > > $ make C=1 CHECK=scripts/coccicheck arch/x86/kernel/cpu/resctrl/rdtgroup.o > ... > arch/x86/kernel/cpu/resctrl/rdtgroup.c:1621:5-8: Unneeded variable: "ret". Return "0" on line 1655 > > Remove the local variable 'ret'. > > [ bp: Massage commit message, make mbm_config_write_domain() void. ] > > Fixes: 92bd5a139033 ("x86/resctrl: Add interface to write mbm_total_bytes_config") > Closes: https://lore.kernel.org/oe-kbuild-all/202401241810.jbd8Ipa1-lkp@intel.com/ > Reported-by: kernel test robot <lkp@intel.com> I do not know if this is something tip prefers but the general patch checkers prefer that "Reported-by:" is followed by "Closes:". > Signed-off-by: Babu Moger <babu.moger@amd.com> > Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de> > Acked-by: Reinette Chatre <reinette.chatre@intel.com> > Link: https://lore.kernel.org/r/202401241810.jbd8Ipa1-lkp@intel.com > --- > arch/x86/kernel/cpu/resctrl/rdtgroup.c | 13 +++---------- > 1 file changed, 3 insertions(+), 10 deletions(-) > > diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c > index 2b69e560b05f..c33eb77b6d70 100644 > --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c > +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c > @@ -1614,11 +1614,10 @@ static void mon_event_config_write(void *info) > wrmsr(MSR_IA32_EVT_CFG_BASE + index, mon_info->mon_config, 0); > } > > -static int mbm_config_write_domain(struct rdt_resource *r, > +static void mbm_config_write_domain(struct rdt_resource *r, > struct rdt_domain *d, u32 evtid, u32 val) This shifts the alignment slightly to no longer match the open parenthesis. > { > struct mon_config_info mon_info = {0}; > - int ret = 0; > > /* > * Read the current config value first. If both are the same then > @@ -1627,7 +1626,7 @@ static int mbm_config_write_domain(struct rdt_resource *r, > mon_info.evtid = evtid; > mondata_config_read(d, &mon_info); > if (mon_info.mon_config == val) > - goto out; > + return; > > mon_info.mon_config = val; > > @@ -1650,9 +1649,6 @@ static int mbm_config_write_domain(struct rdt_resource *r, > * mbm_local and mbm_total counts for all the RMIDs. > */ > resctrl_arch_reset_rmid_all(r, d); > - > -out: > - return ret; > } > > static int mon_config_write(struct rdt_resource *r, char *tok, u32 evtid) > @@ -1661,7 +1657,6 @@ static int mon_config_write(struct rdt_resource *r, char *tok, u32 evtid) > char *dom_str = NULL, *id_str; > unsigned long dom_id, val; > struct rdt_domain *d; > - int ret = 0; > > next: > if (!tok || tok[0] == '\0') > @@ -1690,9 +1685,7 @@ static int mon_config_write(struct rdt_resource *r, char *tok, u32 evtid) > > list_for_each_entry(d, &r->domains, list) { > if (d->id == dom_id) { > - ret = mbm_config_write_domain(r, d, evtid, val); > - if (ret) > - return -EINVAL; > + mbm_config_write_domain(r, d, evtid, val); > goto next; > } > } The core changes look good to me. Thank you very much for creating this fix. Reinette
Hi, On Wed, Jan 24, 2024 at 12:04:31PM -0800, Reinette Chatre wrote: > > 54e35eb8611c ("x86/resctrl: Read supported bandwidth sources from CPUID"). > > I think a "commit" prefix is required here and below. Yeah, but if you see a 12-char sha1 followed by a title in (" "), that is a commit and nothing else, right? If I say "commit" too it is kinda redundant. > > Fixes: 92bd5a139033 ("x86/resctrl: Add interface to write mbm_total_bytes_config") > > Closes: https://lore.kernel.org/oe-kbuild-all/202401241810.jbd8Ipa1-lkp@intel.com/ > > Reported-by: kernel test robot <lkp@intel.com> > > I do not know if this is something tip prefers but the general patch checkers prefer > that "Reported-by:" is followed by "Closes:". Good question. Documentation/process/maintainer-tip.rst doesn't clarify that, lemme send a patch for it and see what my brethren in arms think. :-) > This shifts the alignment slightly to no longer match the open parenthesis. Fixed. Thx.
On Wed, Jan 24, 2024 at 01:39:01PM -0600, Moger, Babu wrote:
> Yes. Looks good. Compile tested also. Thanks
Thanks.
Hi Boris, On 1/24/2024 12:45 PM, Borislav Petkov wrote: > Hi, > > On Wed, Jan 24, 2024 at 12:04:31PM -0800, Reinette Chatre wrote: >>> 54e35eb8611c ("x86/resctrl: Read supported bandwidth sources from CPUID"). >> >> I think a "commit" prefix is required here and below. > > Yeah, but if you see a 12-char sha1 followed by a title in (" "), that > is a commit and nothing else, right? > > If I say "commit" too it is kinda redundant. I do not know the motivation for that requirement. From what I can tell the change [1] that added that check went in as first version without discussion. [1] starts by saying that the format is "preferred" so I assume there is some history that I am not familiar with. Reinette [1] https://lore.kernel.org/all/976c6cdd680db4b55ae31b5fc2d1779da5c0dc66.camel@perches.com/
On Wed, Jan 24, 2024 at 01:31:01PM -0800, Reinette Chatre wrote: > I do not know the motivation for that requirement. From what I can tell the > change [1] that added that check went in as first version without discussion. > [1] starts by saying that the format is "preferred" so I assume there is > some history that I am not familiar with. My main goal with commit messages, code comments and every other *text* you have in the code is to be as succinct and understandable as possible for time considerations, clarity, etc. If I see a 12-char sha1 followed by a title, to me that is a commit. No need to say "commit" too. But as already said, if you prefer to have "commit" there, I'll add it - no biggie. Thx.
Hi Boris, On 1/24/2024 2:46 PM, Borislav Petkov wrote: > On Wed, Jan 24, 2024 at 01:31:01PM -0800, Reinette Chatre wrote: >> I do not know the motivation for that requirement. From what I can tell the >> change [1] that added that check went in as first version without discussion. >> [1] starts by saying that the format is "preferred" so I assume there is >> some history that I am not familiar with. > > My main goal with commit messages, code comments and every other *text* > you have in the code is to be as succinct and understandable as possible > for time considerations, clarity, etc. > > If I see a 12-char sha1 followed by a title, to me that is a commit. No > need to say "commit" too. But as already said, if you prefer to have > "commit" there, I'll add it - no biggie. I totally understand your sentiment. I am just the messenger here. Without the "commit" this patch triggers a loud: ERROR: Please use git commit description style 'commit <12+ chars of sha1> ("<title line>")' - ie: 'commit 92bd5a139033 ("x86/resctrl: Add interface to write mbm_total_bytes_config")' #15: 92bd5a139033 ("x86/resctrl: Add interface to write mbm_total_bytes_config") I usually (unless, for example, following checkpatch.pl advice causes a change to fall out of place with surrounding code) try to format my patches to get a clean slate from checkpatch.pl with the goal to eliminate obstacles to the patch getting included. Since you are the one that decides the rules for inclusion you can make this check to be one where checkpatch.pl can be ignored. No objection from me if you choose to do so (and I will note the precedent for future patches). Reinette
On Wed, Jan 24, 2024 at 03:03:15PM -0800, Reinette Chatre wrote: > Since you are the one that decides the rules for inclusion you can make this > check to be one where checkpatch.pl can be ignored. No objection from me if > you choose to do so (and I will note the precedent for future patches). Nah, that's not nearly as important for you to change your workflow. What I'd suggest, though, is to sanity-check what checkpatch suggests and ask yourself whether it always makes sense. Thx.
On Thu, Jan 25, 2024 at 12:34:24AM +0100, Borislav Petkov wrote: > On Wed, Jan 24, 2024 at 03:03:15PM -0800, Reinette Chatre wrote: > > Since you are the one that decides the rules for inclusion you can make this > > check to be one where checkpatch.pl can be ignored. No objection from me if > > you choose to do so (and I will note the precedent for future patches). > > Nah, that's not nearly as important for you to change your workflow. > > What I'd suggest, though, is to sanity-check what checkpatch suggests and > ask yourself whether it always makes sense. Dammit, there's a reason I don't use this abomination: $ cat /tmp/0001-x86-resctrl-Remove-redundant-variable-in-mbm_config_.patch | ./scripts/checkpatch.pl WARNING: Prefer a maximum 75 chars per line (possible unwrapped commit description?) #9: commit 54e35eb8611c ("x86/resctrl: Read supported bandwidth sources from CPUID"). WTF?! I have a line underneath which is even one char longer: " commit 92bd5a139033 ("x86/resctrl: Add interface to write mbm_total_bytes_config")" but it doesn't complain about it. What a bunch of crap. I'm writing it as a maximally readable commit message and that's it. Human-readable beats any script, any day of the week. Thx.
Hi Boris, On 1/24/2024 3:34 PM, Borislav Petkov wrote: > On Wed, Jan 24, 2024 at 03:03:15PM -0800, Reinette Chatre wrote: >> Since you are the one that decides the rules for inclusion you can make this >> check to be one where checkpatch.pl can be ignored. No objection from me if >> you choose to do so (and I will note the precedent for future patches). > > Nah, that's not nearly as important for you to change your workflow. > > What I'd suggest, though, is to sanity-check what checkpatch suggests and > ask yourself whether it always makes sense. Will do. Thank you for considering and discussing this detail. The final patch you just merged looks good to me. Thank you very much for fixing this. Reinette
diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c index 2b69e560b05f..6057f96df73f 100644 --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c @@ -1618,7 +1618,6 @@ static int mbm_config_write_domain(struct rdt_resource *r, struct rdt_domain *d, u32 evtid, u32 val) { struct mon_config_info mon_info = {0}; - int ret = 0; /* * Read the current config value first. If both are the same then @@ -1652,7 +1651,7 @@ static int mbm_config_write_domain(struct rdt_resource *r, resctrl_arch_reset_rmid_all(r, d); out: - return ret; + return 0; } static int mon_config_write(struct rdt_resource *r, char *tok, u32 evtid)