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

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

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

quoted

-  	if (!high)
-  		return ret;

#ifdef CONFIG_ARCH_HAS_GENERIC_CRASHKERNEL_RESERVATION
	if (high && ret == -ENOENT) {
		... ...
		if (ret || !*crash_size)	//parse HIGH
		... ...
	}

	//At this point, *crash_size is not 0 and ret is 0.
	//We can also delete if (!*crash_size) above because it will be checked later.
#endif

	if (!*crash_size)
		ret = -EINVAL;

	return ret;

When crashkernel=,high is specified while crashkernel=,low is omitted,
the ret==-ENOENT, but we can't return ret directly. That is still an
acceptable way.

Oh, yes. Sorry, I didn't notice branch "ret==-ENOENT" didn't return. So "ret = 0;"
needs to be added.

	if (high && ret == -ENOENT) {
		... ...
		*high = true;
+		ret = 0;
	}

quoted

-  	return 0;

quoted

+		/* The specified value is invalid */
+		return -1;
+	}
+#endif
 	return 0;
 }
 

-- 
Regards,
  Zhen Lei


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