Re: [V2] perf/x86/intel/uncore: Fix iounmap() leak on global_init failure

[Markus Elfring <Markus.Elfring@web.de>]

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