Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Subject: Re: [PATCH v2 3/8] crash_core: change parse_crashkernel() to support
 crashkernel=,high|low parsing
To:     Baoquan He <bhe@redhat.com>
Cc:     linux-kernel@vger.kernel.org, akpm@linux-foundation.org,
        catalin.marinas@arm.com, thunder.leizhen@huawei.com,
        dyoung@redhat.com, prudo@redhat.com, samuel.holland@sifive.com,
        kexec@lists.infradead.org, linux-arm-kernel@lists.infradead.org,
        x86@kernel.org
References: <20230829121610.138107-1-bhe@redhat.com>
 <20230829121610.138107-4-bhe@redhat.com>
 <c4a1f96e-2c8d-a496-61f3-0299d77c5c84@huaweicloud.com>
 <ZPGzlIkXqVThTwjg@MiWiFi-R3L-srv>
From:   "Leizhen (ThunderTown)" <thunder.leizhen@huaweicloud.com>
Message-ID: <2b9af1b7-8d80-46b7-d582-156a97456a36@huaweicloud.com>
Date:   Mon, 4 Sep 2023 10:47:58 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:60.0) Gecko/20100101
 Thunderbird/60.7.0
MIME-Version: 1.0
In-Reply-To: <ZPGzlIkXqVThTwjg@MiWiFi-R3L-srv>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk


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

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

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

-- 
Regards,
  Zhen Lei