From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 7076AC433F5 for ; Wed, 5 Oct 2022 06:43:32 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id ABC1384E37; Wed, 5 Oct 2022 08:43:29 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (2048-bit key; unprotected) header.d=linaro.org header.i=@linaro.org header.b="IZbEIaHw"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 803D084967; Wed, 5 Oct 2022 08:43:27 +0200 (CEST) Received: from mail-pl1-x62d.google.com (mail-pl1-x62d.google.com [IPv6:2607:f8b0:4864:20::62d]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id 8DDFC84967 for ; Wed, 5 Oct 2022 08:43:23 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=takahiro.akashi@linaro.org Received: by mail-pl1-x62d.google.com with SMTP id c24so14641595plo.3 for ; Tue, 04 Oct 2022 23:43:23 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=in-reply-to:content-disposition:mime-version:references :mail-followup-to:message-id:subject:cc:to:from:date:from:to:cc :subject:date:message-id:reply-to; bh=wMjtxaaIwyqTPoX6F1Ct2HEfd1HdQ98yodG27mSCnMY=; b=IZbEIaHwtOrJdiMoT9PxZQSjTFlyhORWAMtlQYAPYQfL63YBGESnqcVpIUzf4c4McL EC/OfyaMpfB7K/0P21+Ijxc8aO5wnXNpqUFlTNg2HMv+VyWTYTh3JKNF/0C3lzeyLDY7 p8iSLL3A9y6dHjhq0w7HnK6cYsFrgPjsZklD8tMnpM++o5+9EJlt9K8nVRHt3D58mzeS PR1eUXZbgC+vVSBluSPwrxOv8V9GgXdUSuzHoC5hrqWuoUgKmHnCOJxqJdf9FXzohnfU ny2lZwi6BUtxzVrdpO7O3fnkBlLnKDS2fxPH/uOjTWS2kRuej6KkfF08MASwgMorY7gM 1wgQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=in-reply-to:content-disposition:mime-version:references :mail-followup-to:message-id:subject:cc:to:from:date :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=wMjtxaaIwyqTPoX6F1Ct2HEfd1HdQ98yodG27mSCnMY=; b=dwMUt3vpirJ0LSNhoMt035vmkTU/vky+567GlbzKcbgjtSfMho1FLsdnnHlkVSRhl0 3rqylcKx8UgxxxY6mqU7Zm13+fiGuIxQq9CoVy+tkOvM9Q9GmDh5yd1WYFH74cBdGxgg 2dbXexY3JumjM+aECurilrpcdZExh3LoT/4tj/gwm/3krCbXpKBslyfM39QO9Yp7nI79 xtXvNkjqzkZVomQyfrwBhugTgPXoPLGjUVgUd4MXSXtg6kkCyFIFSZ4HpZRP5ZD94pKr fN93zXwIKauEShLMl0NGM+bhwV1OY7zdaiTzfDySHXDYqYhsCAeCfC3msyimHnWIaRhY 2d3w== X-Gm-Message-State: ACrzQf15+x3WRMkv+DQ0zSj8UBXze6Jl2F0C85Lq7dSVz/K/SQToufF8 QCNAtbWxyzToKrlIH1tizhEdxw== X-Google-Smtp-Source: AMsMyM6ypUwSjL1w4kv4JpcY9GVokYZCjuhuxK+ydXfOlJwP29Ikcc/T/zXvJcFjk8t3e3od7hm/Fw== X-Received: by 2002:a17:902:6a86:b0:176:a6bc:54c0 with SMTP id n6-20020a1709026a8600b00176a6bc54c0mr31140244plk.87.1664952201638; Tue, 04 Oct 2022 23:43:21 -0700 (PDT) Received: from laputa ([2400:4050:c3e1:100:39e7:db31:fd97:9c8d]) by smtp.gmail.com with ESMTPSA id k14-20020aa7972e000000b0056109e15638sm6133214pfg.54.2022.10.04.23.43.19 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 04 Oct 2022 23:43:20 -0700 (PDT) Date: Wed, 5 Oct 2022 15:43:17 +0900 From: AKASHI Takahiro To: Heinrich Schuchardt Cc: Ilias Apalodimas , u-boot@lists.denx.de Subject: Re: [PATCH 1/1] cmd: simplify command efidebug Message-ID: <20221005064317.GA51793@laputa> Mail-Followup-To: AKASHI Takahiro , Heinrich Schuchardt , Ilias Apalodimas , u-boot@lists.denx.de References: <20221004134032.128142-1-heinrich.schuchardt@canonical.com> <20221005002330.GA34350@laputa> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.39 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.6 at phobos.denx.de X-Virus-Status: Clean On Wed, Oct 05, 2022 at 08:27:37AM +0200, Heinrich Schuchardt wrote: > On 10/5/22 02:23, AKASHI Takahiro wrote: > > Heinrich, > > > > On Tue, Oct 04, 2022 at 03:40:32PM +0200, Heinrich Schuchardt wrote: > > > Currently we have subcommands 'efidebug dh' which shows protocols per > > > handle and 'efidebug devices' which shows the device path. None shows which > > > U-Boot device matches the handle. > > > > If you know how a device path display format is constructed, > > you can identify it. For instance, > > /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b) > > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -> efi_root > > /VenHw(bbe4e671-5773-4ea1-9aab-3a7dbf40c482,00) > > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > -> host 0 > > /HD(1,GPT,7e5c17c5-3f5f-49d0-ae96-511b21d7f273,0x800,0x3f7df) > > ^ -> partition 1 > > > > > Change 'efidebug dh' to show the device path and the U-Boot device if any > > > is associated with the handle. > > > > There is a good reason for two separate subcommands. While "dh" shows > > all the handles which currently exist on the system, "devices" shows > > only ones with device path protocol. > > The number of handles can possibly be huge (try "dh" on EDK2). > > We only have to care about the low number of EFI handles in U-Boot which is > block devices + 5. Who knows the future. > > For those who are interested in what devices are connected at some point, > > "devices" subcommand is still useful. > > Having to look up information in two separate commands instead of one place > is inconvenient. I don't think so. I sometimes felt comfortable only with the output from "devices" in my past development. > The output of efidebug devices does not contain any information which will > not be shown by modified efidebug dh command. So the devices command becomes > superfluous. That is why I suggested the same change be applied to "devices" because such information is only meaningful for device paths. -Takahiro Akashi > > > > The problem is not "devices" subcommand itself, but the display format > > of device path. UEFI spec says, in "10.6 EFI Device Path Display Format Overview", > > "This section describes the recommended conversion between an EFI Device > > ^^^^^^^^^^^ > > Path Protocol and text." > > > > So if there is more readable/understandable format, we may want to use it > > for U-Boot UEFI (at least, as an alternate option of format). > > Say, > > /VenHw(root)/VenHw(host, 0)/HD(1,GPT,....) > > I believe that it is simple and unambiguous. > > This might be implemented subject to the DisplayOnly argument of > EFI_DEVICE_PATH_TO_TEXT_PROTOCOL.ConvertDevicePathToText() in a separate > patch. > > It would require adding the GUIDs to list_guid[] in lib/uuid.c and switching > between '%pUl' and '%pUs' in efi_convert_device_node_to_text. > > > > > > Remove 'efidebug devices'. > > > > So please don't remove "devices". > > Instead, I recommend that the same change be applied to this subcommand, > > displaying both formats at "devices". > > There is no benefit in having two sub-commands showing the same information. > Instead we should reduce the code size. > > Best regards > > Heinrich > > > > > -Takahiro Akashi > > > > > > > Old output of 'efidebug dh': > > > > > > Handle Protocols > > > ================ ==================== > > > 000000001b22e690 Device Path, Block IO > > > 000000001b22e800 Device Path, Block IO, system, Simple File System > > > > > > New output of 'efidebug dh': > > > > > > 000000001b22e690 (host0) > > > /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/VenHw(bbe4e671-5773-4ea1-9aab-3a7dbf40c482,00) > > > Block IO > > > > > > 000000001b22e800 (host0:1) > > > /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/VenHw(bbe4e671-5773-4ea1-9aab-3a7dbf40c482,00)/HD(1,GPT,7e5c17c5-3f5f-49d0-ae96-511b21d7f273,0x800,0x3f7df) > > > Block IO > > > system > > > Simple File System > > > > > > Signed-off-by: Heinrich Schuchardt > > > --- > > > cmd/efidebug.c | 102 ++++++++----------------------------------------- > > > 1 file changed, 15 insertions(+), 87 deletions(-) > > > > > > diff --git a/cmd/efidebug.c b/cmd/efidebug.c > > > index 84e6ff5565..76a6b4d8eb 100644 > > > --- a/cmd/efidebug.c > > > +++ b/cmd/efidebug.c > > > @@ -8,6 +8,7 @@ > > > #include > > > #include > > > #include > > > +#include > > > #include > > > #include > > > #include > > > @@ -344,79 +345,11 @@ static int do_efi_capsule(struct cmd_tbl *cmdtp, int flag, > > > } > > > #endif /* CONFIG_EFI_HAVE_CAPSULE_SUPPORT */ > > > -/** > > > - * efi_get_device_path_text() - get device path text > > > - * > > > - * Return the text representation of the device path of a handle. > > > - * > > > - * @handle: handle of UEFI device > > > - * Return: > > > - * Pointer to the device path text or NULL. > > > - * The caller is responsible for calling FreePool(). > > > - */ > > > -static u16 *efi_get_device_path_text(efi_handle_t handle) > > > -{ > > > - struct efi_handler *handler; > > > - efi_status_t ret; > > > - > > > - ret = efi_search_protocol(handle, &efi_guid_device_path, &handler); > > > - if (ret == EFI_SUCCESS && handler->protocol_interface) { > > > - struct efi_device_path *dp = handler->protocol_interface; > > > - > > > - return efi_dp_str(dp); > > > - } else { > > > - return NULL; > > > - } > > > -} > > > - > > > #define EFI_HANDLE_WIDTH ((int)sizeof(efi_handle_t) * 2) > > > static const char spc[] = " "; > > > static const char sep[] = "================"; > > > -/** > > > - * do_efi_show_devices() - show UEFI devices > > > - * > > > - * @cmdtp: Command table > > > - * @flag: Command flag > > > - * @argc: Number of arguments > > > - * @argv: Argument array > > > - * Return: CMD_RET_SUCCESS on success, CMD_RET_RET_FAILURE on failure > > > - * > > > - * Implement efidebug "devices" sub-command. > > > - * Show all UEFI devices and their information. > > > - */ > > > -static int do_efi_show_devices(struct cmd_tbl *cmdtp, int flag, > > > - int argc, char *const argv[]) > > > -{ > > > - efi_handle_t *handles; > > > - efi_uintn_t num, i; > > > - u16 *dev_path_text; > > > - efi_status_t ret; > > > - > > > - ret = EFI_CALL(efi_locate_handle_buffer(ALL_HANDLES, NULL, NULL, > > > - &num, &handles)); > > > - if (ret != EFI_SUCCESS) > > > - return CMD_RET_FAILURE; > > > - > > > - if (!num) > > > - return CMD_RET_SUCCESS; > > > - > > > - printf("Device%.*s Device Path\n", EFI_HANDLE_WIDTH - 6, spc); > > > - printf("%.*s ====================\n", EFI_HANDLE_WIDTH, sep); > > > - for (i = 0; i < num; i++) { > > > - dev_path_text = efi_get_device_path_text(handles[i]); > > > - if (dev_path_text) { > > > - printf("%p %ls\n", handles[i], dev_path_text); > > > - efi_free_pool(dev_path_text); > > > - } > > > - } > > > - > > > - efi_free_pool(handles); > > > - > > > - return CMD_RET_SUCCESS; > > > -} > > > - > > > /** > > > * efi_get_driver_handle_info() - get information of UEFI driver > > > * > > > @@ -535,26 +468,25 @@ static int do_efi_show_handles(struct cmd_tbl *cmdtp, int flag, > > > if (!num) > > > return CMD_RET_SUCCESS; > > > - printf("Handle%.*s Protocols\n", EFI_HANDLE_WIDTH - 6, spc); > > > - printf("%.*s ====================\n", EFI_HANDLE_WIDTH, sep); > > > for (i = 0; i < num; i++) { > > > - printf("%p", handles[i]); > > > + struct efi_handler *handler; > > > + > > > + printf("\n%p", handles[i]); > > > + if (handles[i]->dev) > > > + printf(" (%s)", handles[i]->dev->name); > > > + printf("\n"); > > > + /* Print device path */ > > > + ret = efi_search_protocol(handles[i], &efi_guid_device_path, > > > + &handler); > > > + if (ret == EFI_SUCCESS) > > > + printf(" %pD\n", handler->protocol_interface); > > > ret = EFI_CALL(BS->protocols_per_handle(handles[i], &guid, > > > &count)); > > > - if (ret || !count) { > > > - putc('\n'); > > > - continue; > > > - } > > > - > > > + /* Print other protocols */ > > > for (j = 0; j < count; j++) { > > > - if (j) > > > - printf(", "); > > > - else > > > - putc(' '); > > > - > > > - printf("%pUs", guid[j]); > > > + if (guidcmp(guid[j], &efi_guid_device_path)) > > > + printf(" %pUs\n", guid[j]); > > > } > > > - putc('\n'); > > > } > > > efi_free_pool(handles); > > > @@ -1539,8 +1471,6 @@ static struct cmd_tbl cmd_efidebug_sub[] = { > > > U_BOOT_CMD_MKENT(capsule, CONFIG_SYS_MAXARGS, 1, do_efi_capsule, > > > "", ""), > > > #endif > > > - U_BOOT_CMD_MKENT(devices, CONFIG_SYS_MAXARGS, 1, do_efi_show_devices, > > > - "", ""), > > > U_BOOT_CMD_MKENT(drivers, CONFIG_SYS_MAXARGS, 1, do_efi_show_drivers, > > > "", ""), > > > U_BOOT_CMD_MKENT(dh, CONFIG_SYS_MAXARGS, 1, do_efi_show_handles, > > > @@ -1630,8 +1560,6 @@ static char efidebug_help_text[] = > > > #endif > > > "\n" > > > #endif > > > - "efidebug devices\n" > > > - " - show UEFI devices\n" > > > "efidebug drivers\n" > > > " - show UEFI drivers\n" > > > "efidebug dh\n" > > > -- > > > 2.37.2 > > >