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

[Baoquan He <bhe@redhat.com>]

On 09/06/23 at 05:07pm, Leizhen (ThunderTown) wrote:
> 
> 
> 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.

I resisted this in the first place, after rethinking, it makes sense.
parse_crashkernel() returning 0 indicates a meaningful crashkernel vlaue
parsed, otherwise non-zero. I will go with this.

> 
> > 
> >         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().

Hmm, I will consider how to comment these better, will update and post
v3.

Thanks, Lei.

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