Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.38 as permitted sender) client-ip=23.128.96.38;
Message-ID: <fbc90ef0-1418-4c7b-8f3e-7dd1a0d7499e@amd.com>
Date:   Fri, 13 Oct 2023 13:14:04 +0530
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] perf/x86/amd/uncore: fix error codes in amd_uncore_init()
Content-Language: en-US
To:     Dan Carpenter <dan.carpenter@linaro.org>
Cc:     Peter Zijlstra <peterz@infradead.org>,
        Ingo Molnar <mingo@redhat.com>,
        Arnaldo Carvalho de Melo <acme@kernel.org>,
        Mark Rutland <mark.rutland@arm.com>,
        Alexander Shishkin <alexander.shishkin@linux.intel.com>,
        Jiri Olsa <jolsa@kernel.org>,
        Namhyung Kim <namhyung@kernel.org>,
        Ian Rogers <irogers@google.com>,
        Adrian Hunter <adrian.hunter@intel.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Borislav Petkov <bp@alien8.de>,
        Dave Hansen <dave.hansen@linux.intel.com>, x86@kernel.org,
        "H. Peter Anvin" <hpa@zytor.com>, linux-perf-users@vger.kernel.org,
        linux-kernel@vger.kernel.org, kernel-janitors@vger.kernel.org
References: <cec62eba-c4b8-4cb7-9671-58894dd4b974@moroto.mountain>
From:   Sandipan Das <sandipan.das@amd.com>
In-Reply-To: <cec62eba-c4b8-4cb7-9671-58894dd4b974@moroto.mountain>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?NmZySXJPeHVCU0s1anhQYmxPeXRmY1dMU0pzeGJEY3Z5TnlqalVlOHhYLzRB?=
 =?utf-8?B?MUNscERTMXpkSk1GNEVhRjBRWk9hd09sMUo4Y2JQUzVlUnVyRmZsY1ZONmM4?=
 =?utf-8?B?L3lkZkZjU29qcXNRcE1JbTh1dWhPU1dNNUUwWUpxVXJnMnJxV3FMTGRKNk1q?=
 =?utf-8?B?Y0N1QklnMkUrWmFIZjd2Skl0R21obnlmUGdTLzRFRnVFOEEvejZjOUlJNHlt?=
 =?utf-8?B?Mzlsc1lHdjJvVnZuUTNqRklpM1hDWUNLbnpjTis5L2dnZUxuRkwra3NVN2xy?=
 =?utf-8?B?eUU0bytaTlRDMWt4YUVzcVZxeVpUMG9Ob3RIWjlrYWJOZU9iMlJZZlBjamNr?=
 =?utf-8?B?eEdaWlVRcTVaUUZobzJ6bjJQcUFTdFN5R29saURwKzE2UUFVdDJ6SU43eFhR?=
 =?utf-8?B?L3F0TmRzaEZJUUkyNTlIOE9KQTFnYWZvM09mQjNQK1VtZXNZOFAyNDF3MXZZ?=
 =?utf-8?B?b3k2T2d3SjZCTFdibU55TjFiL2RXZTVtREYvVkx4Tm1wMzRSbm5PZWY3VEpG?=
 =?utf-8?B?c0NkSC9rVk9wei8xZ1NVaW5WSVZtTFZReW1QSXlnWnI4aTVic3EzUVI0ZDBJ?=
 =?utf-8?B?UlNZTTNLcHd1OWtZMjAzem03d0FDY2lUZG9CK245aExqVjhCLzRUTTcvZUYy?=
 =?utf-8?B?dW9PR2x5ejlSZGhvdHovSmU5MkRReFFPeDdlU1hrSExjdGtDMmJnK1ZCNnc4?=
 =?utf-8?B?YXJIYmNuR09tYk1mdzBBdHZNNGVXNWpHaU95aGoxOTlzelFyT1FCeGp5dnRX?=
 =?utf-8?B?OG9hSndNbit4TmRkcWMra3ZNL2Y1U1NkNi9HU3VRb1NkTWh3YXdubyt1ZEVo?=
 =?utf-8?B?Mm9RdE5iajZURzdOZUJGblVya01ZekJSZnVJakJvcG5lek1WQkRvT0FQWU1K?=
 =?utf-8?B?bi85QjJ1WjdOM3dsY21EbkpmUlRyb0hhR0tVNVV3OEh3ZkhlRk5zaHJPRWxx?=
 =?utf-8?B?K05sWnpUZFBzaVpCVHc3VUJ6NFZ5UXd0UVRzZy9neXp1K3BkYVF1ZDc5MkdX?=
 =?utf-8?B?aE9LNDlkKzZNMWpBcGxRNzhyc2dhUkd3OG5KRGdmWWhXQ3RFdUZYSGloVmZH?=
 =?utf-8?B?dmhaWDRJK2FkdlhuSE5tdXhxMEFYZEZzT0gwdWs0SjN2TEpBeWNvNzFPWFov?=
 =?utf-8?B?M2IrNlQyckNJVUpnVi95NU8rSldsMWpLczJvSUwyWTVCUndtTWIxdmVONG1L?=
 =?utf-8?B?WnRnZ0xVNDNKS3lCZ056WWJCb3pJQkNGTVlqbnVWYkF2UWwvUTZxTXpLWFcy?=
 =?utf-8?B?UTROZFIrSHFKMEFyd0xGUDZPb0o3c3FUVUNOZGVUVUdQZE9VTW9qQzBIa1R2?=
 =?utf-8?B?eWczeUlEVko3TGpDZWx0WnlESjd5UUt0bDJndnNLZ1hjMncyZTNRWEQ1NjBq?=
 =?utf-8?B?RjI4cHBzZlNJVG4wb3VoRjVWSVJFbkNJTGZnSk03ZUFmS3RwSWFNaXBJUFY4?=
 =?utf-8?B?UlAyTEhuZ2hoWUtBWjd5d0FveFdRVml0bDFOdnhwSlJrb1BteVo0MUxvK045?=
 =?utf-8?B?eW5QcWp6cE5GN3dkcExjMlI2VndQRFN5RnQvK0w2V29teFlZMVJqTStIVlh2?=
 =?utf-8?B?NU1Mc0hCeDl0R2VUZ3B2VXNTZlBKWnFBRUQ3WStpRkF3U1FvNEZPcTl5eURZ?=
 =?utf-8?B?TEh1Qzd0UWpYTWNBT0VJeEVFZ25lZ0hTbDRjUHZYRW9NT0p6SUlZbE1ROHlK?=
 =?utf-8?B?QkRyWTFhQ0FuSmdQRkVaSUxXTElJKzBSZjM5RFNlYWUrdmZGczRCUUFCWDhZ?=
 =?utf-8?B?N0NJbEJUaXFOZnI5Y2VCQk5uLzdCajgvdWN3cnZ5VDdkdFpCZnlKTDlET2ha?=
 =?utf-8?B?QkJHN2hLTk9HTlZJVkdHN0hCMmhUYXlVaVFPMjRXVk9zQ2U0VUIyMzZhY1Ew?=
 =?utf-8?B?eEZTSVpBLzB5TjkzZFVVOXIwSTladnQ2SWJMRXhlb01LVGNhL2pudnN4eWMw?=
 =?utf-8?B?YWpLaXcvOXoxbEpSWVpHRGF1NjRzdkxReEtLRm82SkxKK3BWL0tWZnIwUEkr?=
 =?utf-8?B?anVUWTNUb0VxZUJJeERXaVJOTDB2N3VXOUJpbFEyWGdXaXJEUE5wTFArTW4v?=
 =?utf-8?B?YXZmMldQb09pL2FWNXJlSGowWjRVaWhlTENhK1RrOUZFM09KTFJwTzh5VzQ5?=
 =?utf-8?Q?b1cfG9SNEtZLrECAeldEPjyGp?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 961196c2-1c15-4242-8581-08dbcbc02933
X-MS-Exchange-CrossTenant-AuthSource: PH7PR12MB5712.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 13 Oct 2023 07:43:59.2085
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 8LUug/4goWeW4VdaW8jHJbisvxvcosUWNwmWRp4LbiXO8e6Wmm9yvLMeKuD54No5AwUjfvPJXwb2O3e60h5tIg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW3PR12MB4443
X-Spam-Status: No, score=-0.9 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED,
	DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=unavailable
	autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on fry.vger.email
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]); Fri, 13 Oct 2023 00:44:14 -0700 (PDT)

On 10/13/2023 12:48 PM, Dan Carpenter wrote:
> Some of the error paths in this function return don't initialize the
> error code.  Return -ENODEV.
> 
> Fixes: d6389d3ccc13 ("perf/x86/amd/uncore: Refactor uncore management")
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> ---
>  arch/x86/events/amd/uncore.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/events/amd/uncore.c b/arch/x86/events/amd/uncore.c
> index 9b444ce24108..a389828f378c 100644
> --- a/arch/x86/events/amd/uncore.c
> +++ b/arch/x86/events/amd/uncore.c
> @@ -1009,7 +1009,8 @@ static struct amd_uncore uncores[UNCORE_TYPE_MAX] = {
>  static int __init amd_uncore_init(void)
>  {
>  	struct amd_uncore *uncore;
> -	int ret, i;
> +	int ret = -ENODEV;
> +	int i;
>  
>  	if (boot_cpu_data.x86_vendor != X86_VENDOR_AMD &&
>  	    boot_cpu_data.x86_vendor != X86_VENDOR_HYGON)


Thanks for catching this. I see that 'ret' remains uninitialized for cases
where the hotplug callback registration fails and was thinking if the
following is a better fix for this as the reason might not be ENODEV.

diff --git a/arch/x86/events/amd/uncore.c b/arch/x86/events/amd/uncore.c
index 91f01d6c8f7d..7d768dd01522 100644
--- a/arch/x86/events/amd/uncore.c
+++ b/arch/x86/events/amd/uncore.c
@@ -1039,20 +1039,25 @@ static int __init amd_uncore_init(void)
        /*
         * Install callbacks. Core will call them for each online cpu.
         */
-       if (cpuhp_setup_state(CPUHP_PERF_X86_AMD_UNCORE_PREP,
-                             "perf/x86/amd/uncore:prepare",
-                             NULL, amd_uncore_cpu_dead))
+       ret = cpuhp_setup_state(CPUHP_PERF_X86_AMD_UNCORE_PREP,
+                               "perf/x86/amd/uncore:prepare",
+                               NULL, amd_uncore_cpu_dead);
+       if (ret)
                goto fail;

-       if (cpuhp_setup_state(CPUHP_AP_PERF_X86_AMD_UNCORE_STARTING,
-                             "perf/x86/amd/uncore:starting",
-                             amd_uncore_cpu_starting, NULL))
+       ret = cpuhp_setup_state(CPUHP_AP_PERF_X86_AMD_UNCORE_STARTING,
+                               "perf/x86/amd/uncore:starting",
+                               amd_uncore_cpu_starting, NULL);
+       if (ret)
                goto fail_prep;
-       if (cpuhp_setup_state(CPUHP_AP_PERF_X86_AMD_UNCORE_ONLINE,
-                             "perf/x86/amd/uncore:online",
-                             amd_uncore_cpu_online,
-                             amd_uncore_cpu_down_prepare))
+
+       ret = cpuhp_setup_state(CPUHP_AP_PERF_X86_AMD_UNCORE_ONLINE,
+                               "perf/x86/amd/uncore:online",
+                               amd_uncore_cpu_online,
+                               amd_uncore_cpu_down_prepare);
+       if (ret)
                goto fail_start;
+
        return 0;

 fail_start: