Re: [PATCH v2 3/8] crash_core: change parse_crashkernel() to support crashkernel=,high|low parsing

[Leizhen (ThunderTown)]

On 2023/9/5 16:29, Baoquan He wrote:
> On 09/04/23 at 10:47am, Leizhen (ThunderTown) wrote:
>>
>>
>> On 2023/9/1 17:49, Baoquan He wrote:
>>>>> +
>>>>> +		*high = true;
>>>>> +	} else if (ret || !*crash_size) {
>>>> This check can be moved outside of #ifdef. Because even '!high', it's completely
>>>> applicable. The overall adjustment is as follows:
>>> Hmm, the current logic is much easier to understand. However, I may not
>>> 100% get your suggestion. Can you paste the complete code in your
>>> suggested way? Do not need 100% correct code, just the skeleton of code logic
>>> so that I can better understand it and add inline comment.
>>
>> int __init parse_crashkernel(...)
>> {
>> 	int ret;
>>
>> 	/* crashkernel=X[@offset] */
>> 	ret = __parse_crashkernel(cmdline, system_ram, crash_size,
>> 				crash_base, NULL);
>>
>> #ifdef CONFIG_ARCH_HAS_GENERIC_CRASHKERNEL_RESERVATION
>> 	if (high && ret == -ENOENT) {
>> 		... ...		//The code for your original branch "else if (ret == -ENOENT) {"
>> 		ret = 0;	//Added based on the next discussion
>> 	}
>> +#endif
>>
>>  	if (!*crash_size)
>> 		ret = -EINVAL;
>>
>> 	return ret;
>> }
>>
> Thanks, Zhen Lei.
> 
> I paste the whole parse_crashkernel() as you suggested at bottom. Please
> check if it's what you want. 

Yes.

> To me, both is fine to me. I have two minor
> concerns to your suggested way.
> 
> 1)
> I took the "if (!high) return" way because except of x86/arm64, all
> other architectures will call parse_crashkerne() and check
> if *crash_size ==0. Please try 'git grep "parse_crashkernel(" arch'
> and check those call sites. With that, we will have duplicated checking.

Add some patches to remove the duplicated checking of other ARCHs? After this
patch series upstreamed.

> 
>         ret = __parse_crashkernel(cmdline, system_ram, crash_size,
>                                 crash_base, NULL);
>         if (!high)
>                 return ret;
> 2)
> I actually like below branch and the code comment. It can give people
> hint about what's going on in that case. Discarding it is a little pity.

Except that "!*crash_size" and "(high && ret == -ENOENT)" needs special comments,
other common errors do not need to be described, I think. Even if some is required,
it should be placed in function __parse_crashkernel().

> 
>         } else if (ret || !*crash_size) {
>                 /* The specified value is invalid */
>                 return -1;
>         }
> 
> int __init parse_crashkernel(...)
> {
> 	int ret;
> 
> 	/* crashkernel=X[@offset] */
> 	ret = __parse_crashkernel(cmdline, system_ram, crash_size,
> 				crash_base, NULL);
> #ifdef CONFIG_ARCH_HAS_GENERIC_CRASHKERNEL_RESERVATION
> 	if (high && ret == -ENOENT) {
> 		ret = __parse_crashkernel(cmdline, 0, crash_size,
> 				crash_base, suffix_tbl[SUFFIX_HIGH]);
> 		if (ret || !*crash_size)
> 			return -EINVAL;
> 
> 		/*
> 		 * crashkernel=Y,low can be specified or not, but invalid value
> 		 * is not allowed.
> 		 */
> 		ret = __parse_crashkernel(cmdline, 0, low_size,
> 				crash_base, suffix_tbl[SUFFIX_LOW]);
> 		if (ret == -ENOENT) {
> 			*low_size = DEFAULT_CRASH_KERNEL_LOW_SIZE;
> 			ret = 0;
> 		} else if (ret) {
> 			return ret;
> 		}
> 
> 		*high = true;
> 	}
> #endif
> 
> 	if (!*crash_size)
> 		ret = -EINVAL;
> 
> 	return ret;
> }
> 
> .
> 

-- 
Regards,
  Zhen Lei