Message-ID: <ef00d1ec-9b0e-4204-b1dd-885245db35fa@linux.intel.com>
Date: Fri, 16 Jan 2026 08:57:37 +0800
From: "Mi, Dapeng" <dapeng1.mi@...ux.intel.com>
To: "Chen, Zide" <zide.chen@...el.com>, 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>,
 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>
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/16/2026 5:03 AM, Chen, Zide wrote:
>
> 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.

Agree.

IMO, we'd better keep this patch is simple and focused and then it can be
reviewed and merged into perf/core tree quickly. So we can avoid to cause
the subsequent backporting work. 

About the further optimization, we can have an independent patchset to do
it. :)

Thanks.


>
>>>> …
>>>>> -	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