arch/x86/events/intel/uncore_discovery.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-)
Kernel test robot reported:
Unverified Error/Warning (likely false positive, kindly check if
interested):
arch/x86/events/intel/uncore_discovery.c:293:2-8:
ERROR: missing iounmap; ioremap on line 288 and execution via
conditional on line 292
If domain->global_init() fails in __parse_discovery_table(), the
ioremap'ed MMIO region is not released before returning, resulting
in an MMIO mapping leak.
Reported-by: kernel test robot <lkp@intel.com>
Fixes: b575fc0e3357 ("perf/x86/intel/uncore: Add domain global init callback")
Signed-off-by: Zide Chen <zide.chen@intel.com>
---
arch/x86/events/intel/uncore_discovery.c | 15 ++++++++++-----
1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/arch/x86/events/intel/uncore_discovery.c b/arch/x86/events/intel/uncore_discovery.c
index 0e414cecb6f2..efd1fc99a908 100644
--- a/arch/x86/events/intel/uncore_discovery.c
+++ 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;
size = UNCORE_DISCOVERY_GLOBAL_MAP_SIZE;
@@ -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)) {
pr_info("Invalid Global Discovery State: 0x%llx 0x%llx 0x%llx\n",
global.table1, global.ctl, global.table3);
- iounmap(io_addr);
return -EINVAL;
}
- iounmap(io_addr);
size = (1 + global.max_units) * global.stride * 8;
io_addr = ioremap(addr, size);
if (!io_addr)
return -ENOMEM;
- if (domain->global_init && domain->global_init(global.ctl))
- return -ENODEV;
+ if (domain->global_init && domain->global_init(global.ctl)) {
+ ret = -ENODEV;
+ goto out;
+ }
/* Parsing Unit Discovery State */
for (i = 0; i < global.max_units; i++) {
@@ -307,8 +310,10 @@ static int __parse_discovery_table(struct uncore_discovery_domain *domain,
}
*parsed = true;
+
+out:
iounmap(io_addr);
- return 0;
+ return ret;
}
static int parse_discovery_table(struct uncore_discovery_domain *domain,
--
2.52.0
On 1/15/2026 3:38 AM, Zide Chen wrote:
> Kernel test robot reported:
>
> Unverified Error/Warning (likely false positive, kindly check if
> interested):
> arch/x86/events/intel/uncore_discovery.c:293:2-8:
> ERROR: missing iounmap; ioremap on line 288 and execution via
> conditional on line 292
>
> If domain->global_init() fails in __parse_discovery_table(), the
> ioremap'ed MMIO region is not released before returning, resulting
> in an MMIO mapping leak.
>
> Reported-by: kernel test robot <lkp@intel.com>
> Fixes: b575fc0e3357 ("perf/x86/intel/uncore: Add domain global init callback")
> Signed-off-by: Zide Chen <zide.chen@intel.com>
> ---
> arch/x86/events/intel/uncore_discovery.c | 15 ++++++++++-----
> 1 file changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/arch/x86/events/intel/uncore_discovery.c b/arch/x86/events/intel/uncore_discovery.c
> index 0e414cecb6f2..efd1fc99a908 100644
> --- a/arch/x86/events/intel/uncore_discovery.c
> +++ 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;
>
> size = UNCORE_DISCOVERY_GLOBAL_MAP_SIZE;
> @@ -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)) {
> pr_info("Invalid Global Discovery State: 0x%llx 0x%llx 0x%llx\n",
> global.table1, global.ctl, global.table3);
> - iounmap(io_addr);
> return -EINVAL;
> }
> - iounmap(io_addr);
>
> size = (1 + global.max_units) * global.stride * 8;
> io_addr = ioremap(addr, size);
> if (!io_addr)
> return -ENOMEM;
>
> - if (domain->global_init && domain->global_init(global.ctl))
> - return -ENODEV;
> + if (domain->global_init && domain->global_init(global.ctl)) {
> + ret = -ENODEV;
> + goto out;
> + }
>
> /* Parsing Unit Discovery State */
> for (i = 0; i < global.max_units; i++) {
> @@ -307,8 +310,10 @@ static int __parse_discovery_table(struct uncore_discovery_domain *domain,
> }
>
> *parsed = true;
> +
> +out:
> iounmap(io_addr);
> - return 0;
> + return ret;
> }
>
> static int parse_discovery_table(struct uncore_discovery_domain *domain,
LGTM.
Reviewed-by: Dapeng Mi <dapeng1.mi@linux.intel.com>
> Kernel test robot reported:
Is this duplicate information according to a known tag?
> Unverified Error/Warning (likely false positive, kindly check if
> interested):
> arch/x86/events/intel/uncore_discovery.c:293:2-8:
> ERROR: missing iounmap; ioremap on line 288 and execution via
> conditional on line 292
See also:
https://elixir.bootlin.com/linux/v6.19-rc5/source/scripts/coccinelle/free/iounmap.cocci#L2-L8
> If domain->global_init() fails in __parse_discovery_table(), the
> ioremap'ed MMIO region is not released before returning, resulting
> in an MMIO mapping leak.
>
> Reported-by: kernel test robot <lkp@intel.com>
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?
…
> ---
> 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
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.19-rc5#n310
Is there a need to perform desirable changes by a small patch series?
* Specific fix
* Related refinements
> +++ 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?
> @@ -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?
…
> - 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?
unmap_io:
> iounmap(io_addr);
> - return 0;
> + return ret;
> }
…
Regards,
Markus
On 1/14/2026 12:57 PM, Markus Elfring wrote:
>> Kernel test robot reported:
>
> Is this duplicate information according to a known tag?
I was trying to follow some examples of how test robot reports are shown
in commit messages, for example:
https://lore.kernel.org/all/20240626151131.550535-2-torvalds@linux-foundation.org/
https://lore.kernel.org/all/173058261037.3137.8690137124888546964.tip-bot2@tip-bot2/
>> Unverified Error/Warning (likely false positive, kindly check if
>> interested):
>> arch/x86/events/intel/uncore_discovery.c:293:2-8:
>> ERROR: missing iounmap; ioremap on line 288 and execution via
>> conditional on line 292
>
> See also:
> https://elixir.bootlin.com/linux/v6.19-rc5/source/scripts/coccinelle/free/iounmap.cocci#L2-L8
>
>
>> If domain->global_init() fails in __parse_discovery_table(), the
>> ioremap'ed MMIO region is not released before returning, resulting
>> in an MMIO mapping leak.
>>
>> Reported-by: kernel test robot <lkp@intel.com>
>
> 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?
> …
>> ---
>> 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
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.19-rc5#n310
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.
- Add the original warning from the kernel test robot to the commit message.
- 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.
>
>> +++ 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.
https://lore.kernel.org/all/e7d74d9d-cb45-4f5f-8e44-502dd7c4bcff@intel.com/T/#t
>> @@ -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. Since the patch is already
refactoring the iounmap() handling, my thinking was that it would be
reasonable to include it in the same patch.
> …
>> - 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;
>> }
> …
>
>
> Regards,
> Markus
>> 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”.
>> …
>>> ---
>>> 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.
> - 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
>>> +++ 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?
> 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?
>>> @@ -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.
>> …
>>> - 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?
Regards,
Markus
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
>>>> 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. 1. The mentioned source code analysis report is obviously helpful. 2. You added a custom explanation accordingly. 3. Which text part of your change description does contain “orders to the codebase to change its behaviour”? >>>> 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. I find this adjustment approach also “logical” (in principle). >>>>> +++ 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? Does anything hinder you to take patch series better into account? > My understanding > is that it isn’t directly related to the scope of this patch, so I would > prefer not to include it here. Can we imagine further patch variations? > Please let me know if you see it differently. This is the case according to another evolving patch review. >>> 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. 1. You announced planned changes for an initial patch. https://lore.kernel.org/linux-perf-users/fc565620-a3ef-4a28-bfb8-87bb5ce135f5@intel.com/ 2. I offered review comments accordingly. 3. You published the second patch version. https://lore.kernel.org/linux-perf-users/20260114193825.17973-1-zide.chen@intel.com/ 4. You replied to the mentioned review approach three minutes later, didn't you? https://lore.kernel.org/linux-perf-users/e7d74d9d-cb45-4f5f-8e44-502dd7c4bcff@intel.com/ >>> 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. Will backporting be usually desirable for Linux software components? https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/backporting.rst?h=v6.19-rc5#n14 It happened again that further change possibilities were noticed. Thus companion patches can be considered on demand. The corresponding change recombination can occasionally become more interesting for selected development ideas. >> 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. This software design technique influences also if we would still like to fiddle with goto chains (or not). https://elixir.bootlin.com/linux/v6.19-rc5/source/include/linux/cleanup.h#L12-L17 Do you see opportunities fur further collateral evolution? Regards, Markus
On 1/15/2026 11:15 PM, 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.
>
> 1. The mentioned source code analysis report is obviously helpful.
>
> 2. You added a custom explanation accordingly.
>
> 3. Which text part of your change description does contain “orders to
> the codebase to change its behaviour”?
OK, I see your point. Yes, it's good to add one sentence to describe
what the patches does. But I guess this patch is simple enough.
>>>>> 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.
>
> I find this adjustment approach also “logical” (in principle).
>
>
>>>>>> +++ 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?
>
> Does anything hinder you to take patch series better into account?
This “optimization” is trivial and does not appear to be related to the
issue this patch is trying to address.
I don’t think this change alone justifies a separate patch, as it would
add review overhead without providing much practical benefit.
- int i;
- for (i = 0; i < global.max_units; i++) {
+ for (int i = 0; i < global.max_units; i++) {
Overall, the Intel perf driver largely follows a pre-C99 coding style.
Updating a single instance in isolation does not seem very helpful; if
there is interest, this could be handled as part of a broader,
driver-wide cleanup.
>
>> My understanding
>> is that it isn’t directly related to the scope of this patch, so I would
>> prefer not to include it here.
>
> Can we imagine further patch variations?
Ditto.
>
>> Please let me know if you see it differently.
>
> This is the case according to another evolving patch review.
>
>
>>>> 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.
>
> 1. You announced planned changes for an initial patch.
> https://lore.kernel.org/linux-perf-users/fc565620-a3ef-4a28-bfb8-87bb5ce135f5@intel.com/
>
> 2. I offered review comments accordingly.
>
> 3. You published the second patch version.
> https://lore.kernel.org/linux-perf-users/20260114193825.17973-1-zide.chen@intel.com/
>
> 4. You replied to the mentioned review approach three minutes later, didn't you?
> https://lore.kernel.org/linux-perf-users/e7d74d9d-cb45-4f5f-8e44-502dd7c4bcff@intel.com/
>
Really appreciated for your review!
>>>> 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.
>
> Will backporting be usually desirable for Linux software components?
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/backporting.rst?h=v6.19-rc5#n14
>
> It happened again that further change possibilities were noticed.
> Thus companion patches can be considered on demand.
> The corresponding change recombination can occasionally become more interesting
> for selected development ideas.
Are you suggesting putting this into a separate patch?
My impression is that the change is simple and closely related, though I
may be missing something. I understand others may see it differently.
@@ -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)) {
pr_info("Invalid Global Discovery State: 0x%llx 0x%llx
0x%llx\n",
global.table1, global.ctl, global.table3);
- iounmap(io_addr);
return -EINVAL;
}
- iounmap(io_addr);
>>> 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.
>
> This software design technique influences also if we would still like to fiddle
> with goto chains (or not).
> https://elixir.bootlin.com/linux/v6.19-rc5/source/include/linux/cleanup.h#L12-L17
>
> Do you see opportunities fur further collateral evolution?
Yes, there are some code cleanup/improvement ideas in mind.
> Regards,
> Markus
…
>> 3. Which text part of your change description does contain “orders to
>> the codebase to change its behaviour”?
>
> OK, I see your point. Yes, it's good to add one sentence to describe
> what the patches does.
I became curious somehow if further contributors would be willing to care
a bit more also for this wording requirement.
> But I guess this patch is simple enough.
But it seems that several technical details triggered special communication challenges.
> I don’t think this change alone justifies a separate patch, as it would
> add review overhead without providing much practical benefit.
Will any other contributors dare to add related insights?
>> The corresponding change recombination can occasionally become more interesting
>> for selected development ideas.
>
>
> Are you suggesting putting this into a separate patch?
Yes.
I propose a stricter distinction between a “quick” fix and subsequent refinements
at the discussed source code places.
> My impression is that the change is simple and closely related, though I
> may be missing something. I understand others may see it differently.
It seems that we are struggling according to recurring factors of change resistance.
> @@ -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)) {
> pr_info("Invalid Global Discovery State: 0x%llx 0x%llx
> 0x%llx\n",
> global.table1, global.ctl, global.table3);
> - iounmap(io_addr);
> return -EINVAL;
> }
> - iounmap(io_addr);
Regards,
Markus
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
© 2016 - 2026 Red Hat, Inc.