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

On 09/06/23 at 05:07pm, Leizhen (ThunderTown) wrote:

On 2023/9/5 16:29, Baoquan He wrote:

quoted

On 09/04/23 at 10:47am, Leizhen (ThunderTown) wrote:

quoted

On 2023/9/1 17:49, Baoquan He wrote:

quoted

quoted

quoted

+
+		*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.

quoted

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.

quoted

        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.

quoted

        } 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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel