Re: [PATCH v1 5/5] fastboot: add oem console command support

[Mattijs Korpershoek <mkorpershoek@baylibre.com>]

Hi Svyatoslav,

On mar., nov. 14, 2023 at 12:30, Svyatoslav Ryhel <clamor95@gmail.com> wrote:

> 14 листопада 2023 р. 12:24:52 GMT+02:00, Mattijs Korpershoek <mkorpershoek@baylibre.com> написав(-ла):
>>Hi Svyatoslav,
>>
>>Thank you for your patch.
>>
>>On mar., nov. 07, 2023 at 14:42, Svyatoslav Ryhel <clamor95@gmail.com> wrote:
>>
>>> From: Ion Agorria <ion@agorria.com>
>>>
>>> "oem console" serves to read console record buffer.
>>>
>>> Signed-off-by: Ion Agorria <ion@agorria.com>
>>> Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
>>> ---
>>>  doc/android/fastboot.rst      |  1 +
>>>  drivers/fastboot/Kconfig      |  7 +++++++
>>>  drivers/fastboot/fb_command.c | 39 +++++++++++++++++++++++++++++++++++
>>>  include/fastboot.h            |  1 +
>>>  4 files changed, 48 insertions(+)
>>>
>>> diff --git a/doc/android/fastboot.rst b/doc/android/fastboot.rst
>>> index 1ad8a897c8..05d8f77759 100644
>>> --- a/doc/android/fastboot.rst
>>> +++ b/doc/android/fastboot.rst
>>> @@ -29,6 +29,7 @@ The following OEM commands are supported (if enabled):
>>>    with <arg> = boot_ack boot_partition
>>>  - ``oem bootbus``  - this executes ``mmc bootbus %x %s`` to configure eMMC
>>>  - ``oem run`` - this executes an arbitrary U-Boot command
>>> +- ``oem console`` - this dumps U-Boot console record buffer
>>>  
>>>  Support for both eMMC and NAND devices is included.
>>>  
>>> diff --git a/drivers/fastboot/Kconfig b/drivers/fastboot/Kconfig
>>> index 837c6f1180..58b08120a4 100644
>>> --- a/drivers/fastboot/Kconfig
>>> +++ b/drivers/fastboot/Kconfig
>>> @@ -241,6 +241,13 @@ config FASTBOOT_OEM_RUN
>>>  	  this feature if you are using verified boot, as it will allow an
>>>  	  attacker to bypass any restrictions you have in place.
>>>  
>>> +config FASTBOOT_CMD_OEM_CONSOLE
>>> +	bool "Enable the 'oem console' command"
>>> +	depends on CONSOLE_RECORD
>>> +	help
>>> +	  Add support for the "oem console" command to input and read console
>>> +	  record buffer.
>>> +
>>>  endif # FASTBOOT
>>>  
>>>  endmenu
>>> diff --git a/drivers/fastboot/fb_command.c b/drivers/fastboot/fb_command.c
>>> index 6f621df074..acf5971108 100644
>>> --- a/drivers/fastboot/fb_command.c
>>> +++ b/drivers/fastboot/fb_command.c
>>> @@ -41,6 +41,7 @@ static void reboot_recovery(char *, char *);
>>>  static void oem_format(char *, char *);
>>>  static void oem_partconf(char *, char *);
>>>  static void oem_bootbus(char *, char *);
>>> +static void oem_console(char *, char *);
>>>  static void run_ucmd(char *, char *);
>>>  static void run_acmd(char *, char *);
>>>  
>>> @@ -108,6 +109,10 @@ static const struct {
>>>  		.command = "oem run",
>>>  		.dispatch = CONFIG_IS_ENABLED(FASTBOOT_OEM_RUN, (run_ucmd), (NULL))
>>>  	},
>>> +	[FASTBOOT_COMMAND_OEM_CONSOLE] = {
>>> +		.command = "oem console",
>>> +		.dispatch = CONFIG_IS_ENABLED(FASTBOOT_CMD_OEM_CONSOLE, (oem_console), (NULL))
>>> +	},
>>>  	[FASTBOOT_COMMAND_UCMD] = {
>>>  		.command = "UCmd",
>>>  		.dispatch = CONFIG_IS_ENABLED(FASTBOOT_UUU_SUPPORT, (run_ucmd), (NULL))
>>> @@ -159,6 +164,23 @@ void fastboot_multiresponse(int cmd, char *response)
>>>  	case FASTBOOT_COMMAND_GETVAR:
>>>  		fastboot_getvar_all(response);
>>>  		break;
>>> +#if CONFIG_IS_ENABLED(FASTBOOT_CMD_OEM_CONSOLE)
>>
>>Checkpatch also complains about this.
>>
>>Can we rewrite this using if (IS_ENABLED(CONFIG...)) please ?
>>
>
> Please, do not relay on checkpatch that much. In this case #ifdef is better since in this case all under ifdef will be cut off while using if(...) requires all code under the if to be able to be run even if config is not enabled. Thanks.

I understand that sometimes, checkpatch generates false positives or bad
suggestions.
I also understand the differences between #ifdef and if (IS_ENABLED()).

I did not measure whether the binary size is bigger when switching from
#ifdef to "if IS_ENABLED()" but I suspect that the compiler can
optimize this out as it's known at compile-time.

This file (and the fastboot code in general) mostly uses
"if (IS_ENABLED())" and to keep the code consistent I recommend using it.

Therefore, can we please use if (IS_ENABLED()) here ?

Thank you

Mattijs

>
>>> +	case FASTBOOT_COMMAND_OEM_CONSOLE:
>>> +		char buf[FASTBOOT_RESPONSE_LEN] = { 0 };
>>> +
>>> +		if (console_record_isempty()) {
>>> +			console_record_reset();
>>> +			fastboot_okay(NULL, response);
>>> +		} else {
>>> +			int ret = console_record_readline(buf, sizeof(buf) - 5);
>>> +
>>> +			if (ret < 0)
>>> +				fastboot_fail("Error reading console", response);
>>> +			else
>>> +				fastboot_response("INFO", response, "%s", buf);
>>> +		}
>>> +		break;
>>> +#endif
>>>  	default:
>>>  		fastboot_fail("Unknown multiresponse command", response);
>>>  		break;
>>> @@ -503,3 +525,20 @@ static void __maybe_unused oem_bootbus(char *cmd_parameter, char *response)
>>>  	else
>>>  		fastboot_okay(NULL, response);
>>>  }
>>> +
>>> +/**
>>> + * oem_console() - Execute the OEM console command
>>> + *
>>> + * @cmd_parameter: Pointer to command parameter
>>> + * @response: Pointer to fastboot response buffer
>>> + */
>>> +static void __maybe_unused oem_console(char *cmd_parameter, char *response)
>>> +{
>>> +	if (cmd_parameter)
>>> +		console_in_puts(cmd_parameter);
>>> +
>>> +	if (console_record_isempty())
>>> +		fastboot_fail("Empty console", response);
>>> +	else
>>> +		fastboot_response("MORE", response, NULL);
>>
>>MORE -> TEXT
>>
>>> +}
>>> diff --git a/include/fastboot.h b/include/fastboot.h
>>> index d1a2b74b2f..23d26fb4be 100644
>>> --- a/include/fastboot.h
>>> +++ b/include/fastboot.h
>>> @@ -37,6 +37,7 @@ enum {
>>>  	FASTBOOT_COMMAND_OEM_PARTCONF,
>>>  	FASTBOOT_COMMAND_OEM_BOOTBUS,
>>>  	FASTBOOT_COMMAND_OEM_RUN,
>>> +	FASTBOOT_COMMAND_OEM_CONSOLE,
>>>  	FASTBOOT_COMMAND_ACMD,
>>>  	FASTBOOT_COMMAND_UCMD,
>>>  	FASTBOOT_COMMAND_COUNT
>>> -- 
>>> 2.40.1