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


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