Message-ID: <b7c46240-d0b3-472d-87dc-88cdbd8e0b92@intel.com>
Date: Thu, 15 Jan 2026 13:03:02 -0800
From: "Chen, Zide" <zide.chen@...el.com>
To: Markus Elfring <Markus.Elfring@....de>, linux-perf-users@...r.kernel.org,
 Adrian Hunter <adrian.hunter@...el.com>,
 Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
 Andi Kleen <ak@...ux.intel.com>, Arnaldo Carvalho de Melo <acme@...nel.org>,
 Dapeng Mi <dapeng1.mi@...ux.intel.com>, Ian Rogers <irogers@...gle.com>,
 Ingo Molnar <mingo@...hat.com>, Namhyung Kim <namhyung@...nel.org>,
 Peter Zijlstra <peterz@...radead.org>, Stephane Eranian
 <eranian@...gle.com>, "Chen, Zide" <zide.chen@...el.com>
Cc: lkp@...el.com, LKML <linux-kernel@...r.kernel.org>,
 kernel-janitors@...r.kernel.org, Thomas Falcon <thomas.falcon@...el.com>,
 Xudong Hao <xudong.hao@...el.com>
Subject: Re: [V2] perf/x86/intel/uncore: Fix iounmap() leak on global_init
 failure



On 1/15/2026 1:01 AM, Markus Elfring wrote:
>>> See also once more:
>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.19-rc5#n94
>>>
>>> Will another imperative wording approach become helpful for an improved change description?
>>
>> My apologies — I did try to address the feedback, but I may still be
>> missing something.  Could you please point out what specifically could
>> be improved?
> 
> I hope that the understanding will improve somehow also for a development
> communication requirement like “imperative mood”.

For the commit message itself, I’ve tried to improve it as much as I can
based on the feedback so far. If there are still specific phrases or
wording that should be adjusted, I’d really appreciate it if you could
point them out.
>>> …
>>>> ---
>>>>  arch/x86/events/intel/uncore_discovery.c | 15 ++++++++++-----
>>> …
>>>
>>> Some contributors would appreciate patch version descriptions.
>>> https://lore.kernel.org/all/?q=%22This+looks+like+a+new+version+of+a+previously+submitted+patch%22
> …
>> Yes, that was the intention for v2.
>>
>> V2:
>> - As suggested by Markus, add an `out` label and use goto-based error
>>   handling to reduce duplicated iounmap() code.
> 
> We are still trying to discuss the corresponding identifier selection,
> aren't we?
> 
> 
>> - Add the original warning from the kernel test robot to the commit message.
> 
> It is possible then to identify presented information in the way
> that it is probably coming from coccicheck.
It was indeed from the kernel test robot report. I’m not familiar with
the Intel kernel test robot internals, and I’m not sure whether it
invokes coccicheck.

>> - Trivial rewording of the commit message.
>>
>>>
>>> Is there a need to perform desirable changes by a small patch series?
>>>
>>> * Specific fix
>>> * Related refinements
>>
>> It seems to me that the changes in this patch are small and closely
>> related, so I wondered whether splitting it might be unnecessary.
> 
> I propose to apply a more detailed change granularity.
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.19-rc5#n81

Thanks for the reference. I considered this a single logical fix, which
is why I kept the changes together.


>>>> +++ b/arch/x86/events/intel/uncore_discovery.c
>>>> @@ -264,6 +264,7 @@ static int __parse_discovery_table(struct uncore_discovery_domain *domain,
>>>>  	struct uncore_unit_discovery unit;
>>>>  	void __iomem *io_addr;
>>>>  	unsigned long size;
>>>> +	int ret = 0;
>>>>  	int i;
>>>
>>> Would scope adjustments become helpful for any of these local variables?
>>
>> As mentioned in my earlier reply, my suggestion was to avoid doing other
>> unrelated optimizations in this patch.
> 
> Will development interests grow for related update steps?

Are you suggesting including this change in this patch? My understanding
is that it isn’t directly related to the scope of this patch, so I would
prefer not to include it here. Please let me know if you see it differently.

diff --git a/arch/x86/events/intel/uncore_discovery.c
b/arch/x86/events/intel/uncore_discovery.c
index efd1fc99a908..8ab8f778285a 100644
--- a/arch/x86/events/intel/uncore_discovery.c
+++ b/arch/x86/events/intel/uncore_discovery.c
@@ -265,7 +265,6 @@ static int __parse_discovery_table(struct
uncore_discovery_domain *domain,
        void __iomem *io_addr;
        unsigned long size;
        int ret = 0;
-       int i;

        size = UNCORE_DISCOVERY_GLOBAL_MAP_SIZE;
        io_addr = ioremap(addr, size);
@@ -293,7 +292,7 @@ static int __parse_discovery_table(struct
uncore_discovery_domain *domain,
        }

        /* Parsing Unit Discovery State */
-       for (i = 0; i < global.max_units; i++) {
+       for (int i = 0; i < global.max_units; i++) {
                memcpy_fromio(&unit, io_addr + (i + 1) * (global.stride
* 8),
                              sizeof(struct uncore_unit_discovery));



>> https://lore.kernel.org/all/e7d74d9d-cb45-4f5f-8e44-502dd7c4bcff@intel.com/T/#t
> 
> Re: [PATCH] perf/x86/intel/uncore: Fix iounmap() leak on global_init failure
> 
> Can the timing trigger special considerations?

Sorry if I’m missing your point, but it seems to me that there are no
special considerations involved here.


>>>> @@ -273,21 +274,23 @@ static int __parse_discovery_table(struct uncore_discovery_domain *domain,
>>>>  
>>>>  	/* Read Global Discovery State */
>>>>  	memcpy_fromio(&global, io_addr, sizeof(struct uncore_global_discovery));
>>>> +	iounmap(io_addr);
>>>> +
>>>>  	if (uncore_discovery_invalid_unit(global)) {
>>> …
>>>>  	}
>>>> -	iounmap(io_addr);
>>>
>>> Can this modification part be interpreted as an optimisation?
>>
>> Yes, this is technically an optimization.
> 
> Thanks that we can come to the same conclusion on this aspect.
> 
> 
>>                                           Since the patch is already
>> refactoring the iounmap() handling, my thinking was that it would be
>> reasonable to include it in the same patch.
> 
> I dare to point another opinion out.
> 
> I assume that backporting concerns can influence this detail also a bit more.

Thanks for pointing that out. This patch is intended as a quick fix for
a change that is still staging in perf/core, so I assume that
backporting is unlikely to be needed.

>>> …
>>>> -	if (domain->global_init && domain->global_init(global.ctl))
>>>> -		return -ENODEV;
>>>> +	if (domain->global_init && domain->global_init(global.ctl)) {
>>>> +		ret = -ENODEV;
>>>> +		goto out;
>>>> +	}
>>> …
>>>>  	*parsed = true;
>>>> +
>>>> +out:
>>>
>>> Would an other label be a bit clearer here?
>>
>> I’d suggest keeping the label name out for style consistency, as
>> mentioned in my earlier reply.
>>
>> https://lore.kernel.org/all/e7d74d9d-cb45-4f5f-8e44-502dd7c4bcff@intel.com/T/#t
>>
>>>
>>> unmap_io:
>>>
>>>>  	iounmap(io_addr);
>>>> -	return 0;
>>>> +	return ret;
>>>>  }
>>> …
> 
> By the way:
> How do you think about to increase the application of scope-based resource management?

That’s an interesting topic, but for this patch I’d like to keep the
change minimal and focused.

> Regards,
> Markus

Powered by blists - more mailing lists