Hi Svyatoslav,
On mar., nov. 14, 2023 at 12:30, Svyatoslav Ryhel <clamo...@gmail.com> wrote:
> 14 листопада 2023 р. 12:24:52 GMT+02:00, Mattijs Korpershoek
> <mkorpersh...@baylibre.com> написав(-ла):
>>Hi Svyatoslav,
>>
>>Thank you for your patch.
>>
>>On mar., nov. 07, 2023 at 14:42, Svyatoslav Ryhel <clamo...@gmail.com> wrote:
>>
>>> From: Ion Agorria <i...@agorria.com>
>>>
>>> "oem console" serves to read console record buffer.
>>>
>>> Signed-off-by: Ion Agorria <i...@agorria.com>
>>> Signed-off-by: Svyatoslav Ryhel <clamo...@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
