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 EE597EB64DD for ; Wed, 19 Jul 2023 01:54:37 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 97E9686205; Wed, 19 Jul 2023 03:54:35 +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="JvbSoZSz"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 2A5EB866F7; Wed, 19 Jul 2023 03:54:34 +0200 (CEST) Received: from mail-pf1-x433.google.com (mail-pf1-x433.google.com [IPv6:2607:f8b0:4864:20::433]) (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 B66E686020 for ; Wed, 19 Jul 2023 03:54:30 +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-pf1-x433.google.com with SMTP id d2e1a72fcca58-682eef7d752so1782276b3a.0 for ; Tue, 18 Jul 2023 18:54:30 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1689731669; x=1690336469; 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=ULz8KDXJP+FjT6aKa4kyBGvN7kgBFZuWotYmajwsiQg=; b=JvbSoZSzOP2YYucCNYRY0wwVHsHtzTdY+89gQEagdekYrZZ5pqOsiNEjf0OhCel0mS 2EW8yF2KGFvAGI46kFQyCNai2/Uoee9xXe4GK9RCxSEWfyf5MW+aHOIGIUofMbEOnMcP FOqsRL+G0Y1LTUNHR4UHm68ALaV2nYFUJqiuycLYskJHTXh8s5Ghou2b08DU3JpY5on3 Dx8rAEtqoNYk4NeAh5/NHg+I+dbFHMRG3Mae/tsBboqL6Zn3P7qXrv6Hr4Mkqd/PZ3RA 7HInonqmqtXgY36z3I6JLlT4P+qEhz8PamkqnnFHj8r0SIblcX2aAO5TzoD5FDrqC+0W nktQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1689731669; x=1690336469; 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=ULz8KDXJP+FjT6aKa4kyBGvN7kgBFZuWotYmajwsiQg=; b=SVwHp6am9xwT6b0AgNk2r+yO6ipk7P6Pxhw2wCAqOhT9cFEkH9AZQrVdNsA8kQYs+V lP8dwo64D+XyEA5U3j827G6HvZ6to2u93ZXHmcUxyO4noeJeIXWvzBh7nFs7qTP1bqyn FOLZtD8DEjduetlznSdDrXMlj+Av6bBHFmulChYvOpZG2vt/E3E9JlCIL1FfxEt3Vtz5 ZUuaYv9k6EtFE2xr3ShtnoYE9DLZUtWzkSWMuTWVp1v8yy8hDtDgIFGpp0f6YxkAtzOT WFRc5qlImYyjfd8o11zMJEa0P99SawgONvL/04Rcja16dknfyUCXKK3RB3CqBb8EAd7O Iuag== X-Gm-Message-State: ABy/qLaadWoCP1icTlnQHVQAdbPx+KDl9DRzkT2XTCstvqkfpUzUA6cT O0MCzDmHU/SsqYifgxwaTM1wqw== X-Google-Smtp-Source: APBJJlEQ5k6bJ13BSlV+cQTP6x/9SQ9lNV1Om+ErFJ7Hwta8GCW+93zOougp712qgmhSmJKNxQ6zVQ== X-Received: by 2002:a05:6a20:144c:b0:12d:77e:ba3 with SMTP id a12-20020a056a20144c00b0012d077e0ba3mr1723087pzi.0.1689731668834; Tue, 18 Jul 2023 18:54:28 -0700 (PDT) Received: from laputa ([2400:4050:c3e1:100:21dc:6a82:5204:1378]) by smtp.gmail.com with ESMTPSA id x48-20020a056a000bf000b00682b2044149sm2129847pfu.4.2023.07.18.18.54.27 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 18 Jul 2023 18:54:28 -0700 (PDT) Date: Wed, 19 Jul 2023 10:54:25 +0900 From: AKASHI Takahiro To: Simon Glass Cc: xypron.glpk@gmx.de, masahisa.kojima@linaro.org, u-boot@lists.denx.de Subject: Re: [RFC] efi_driver: fix a parent issue in efi-created block devices Message-ID: Mail-Followup-To: AKASHI Takahiro , Simon Glass , xypron.glpk@gmx.de, masahisa.kojima@linaro.org, u-boot@lists.denx.de References: <20230719002158.27004-1-takahiro.akashi@linaro.org> 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.8 at phobos.denx.de X-Virus-Status: Clean Hi Simon, On Tue, Jul 18, 2023 at 07:08:45PM -0600, Simon Glass wrote: > Hi AKASHI, > > On Tue, 18 Jul 2023 at 18:22, AKASHI Takahiro > wrote: > > > > An EFI application may create an EFI block device (EFI_BLOCK_IO_PROTOCOL) in > > EFI world, which in turn generates a corresponding U-Boot block device based on > > U-Boot's Driver Model. > > The latter device, however, doesn't work as U-Boot proper block device > > due to an issue in efi_driver's implementation. We saw discussions in the past, > > most recently in [1]. > > > > [1] https://lists.denx.de/pipermail/u-boot/2023-July/522565.html > > > > This RFC patch tries to address (part of) the issue. > > If it is considered acceptable, I will create a formal patch. > > > > Withtout this patch, > > ===8<=== > > => env set efi_selftest 'block device' > > => bootefi selftest > > ... > > > > Summary: 0 failures > > > > => dm tree > > Class Index Probed Driver Name > > ----------------------------------------------------------- > > root 0 [ + ] root_driver root_driver > > ... > > bootmeth 7 [ ] vbe_simple | `-- vbe_simple > > blk 0 [ + ] efi_blk `-- efiblk#0 > > partition 0 [ + ] blk_partition `-- efiblk#0:1 > > => ls efiloader 0:1 > > ** Bad device specification efiloader 0 ** > > Couldn't find partition efiloader 0:1 > > ===>8=== > > > > With this patch applied, efiblk#0(:1) now gets accessible. > > > > ===8<=== > > => env set efi_selftest 'block device' > > => bootefi selftest > > ... > > > > Summary: 0 failures > > > > => dm tree > > Class Index Probed Driver Name > > ----------------------------------------------------------- > > root 0 [ + ] root_driver root_driver > > ... > > bootmeth 7 [ ] vbe_simple | `-- vbe_simple > > efi 0 [ + ] EFI block driver `-- /VenHw(dbca4c98-6cb0-694d-0872-819c650cb7b8) > > blk 0 [ + ] efi_blk `-- efiblk#0 > > partition 0 [ + ] blk_partition `-- efiblk#0:1 > > => ls efiloader 0:1 > > 13 hello.txt > > 7 u-boot.txt > > > > 2 file(s), 0 dir(s) > > ===>8=== > > > > Signed-off-by: AKASHI Takahiro > > --- > > include/efi_driver.h | 2 +- > > lib/efi_driver/efi_block_device.c | 17 ++++++++++++----- > > lib/efi_driver/efi_uclass.c | 8 +++++++- > > lib/efi_selftest/efi_selftest_block_device.c | 2 ++ > > 4 files changed, 22 insertions(+), 7 deletions(-) > > > > diff --git a/include/efi_driver.h b/include/efi_driver.h > > index 63a95e4cf800..ed66796f3519 100644 > > --- a/include/efi_driver.h > > +++ b/include/efi_driver.h > > @@ -42,7 +42,7 @@ struct efi_driver_ops { > > const efi_guid_t *child_protocol; > > efi_status_t (*init)(struct efi_driver_binding_extended_protocol *this); > > efi_status_t (*bind)(struct efi_driver_binding_extended_protocol *this, > > - efi_handle_t handle, void *interface); > > + efi_handle_t handle, void *interface, char *name); > > }; > > > > #endif /* _EFI_DRIVER_H */ > > diff --git a/lib/efi_driver/efi_block_device.c b/lib/efi_driver/efi_block_device.c > > index add00eeebbea..43b7ed7c973c 100644 > > --- a/lib/efi_driver/efi_block_device.c > > +++ b/lib/efi_driver/efi_block_device.c > > @@ -115,9 +115,9 @@ static ulong efi_bl_write(struct udevice *dev, lbaint_t blknr, lbaint_t blkcnt, > > * Return: status code > > */ > > static efi_status_t > > -efi_bl_create_block_device(efi_handle_t handle, void *interface) > > +efi_bl_create_block_device(efi_handle_t handle, void *interface, struct udevice *parent) > > { > > - struct udevice *bdev = NULL, *parent = dm_root(); > > + struct udevice *bdev = NULL; > > efi_status_t ret; > > int devnum; > > char *name; > > @@ -181,7 +181,7 @@ err: > > */ > > static efi_status_t efi_bl_bind( > > struct efi_driver_binding_extended_protocol *this, > > - efi_handle_t handle, void *interface) > > + efi_handle_t handle, void *interface, char *name) > > { > > efi_status_t ret = EFI_SUCCESS; > > struct efi_object *obj = efi_search_obj(handle); > > @@ -191,8 +191,15 @@ static efi_status_t efi_bl_bind( > > if (!obj || !interface) > > return EFI_INVALID_PARAMETER; > > > > - if (!handle->dev) > > - ret = efi_bl_create_block_device(handle, interface); > > + if (!handle->dev) { > > + struct udevice *parent; > > + > > + ret = device_bind_driver(dm_root(), "EFI block driver", name, &parent); > > Can you use a non-root device as the parent? I have no idea what else can be the parent in this case. Please note: > > => dm tree > > Class Index Probed Driver Name > > ----------------------------------------------------------- > > root 0 [ + ] root_driver root_driver > > ... > > bootmeth 7 [ ] vbe_simple | `-- vbe_simple > > efi 0 [ + ] EFI block driver `-- /VenHw(dbca4c98-6cb0-694d-0872-819c650cb7b8) This "efi" object is created by an EFI application (i.e. efi_selftest_block_device.c) and don't have any practical parent. > > blk 0 [ + ] efi_blk `-- efiblk#0 > > partition 0 [ + ] blk_partition `-- efiblk#0:1 > > + if (!ret) > > + ret = efi_bl_create_block_device(handle, interface, parent); > > + else > > + ret = EFI_DEVICE_ERROR; > > + } > > > > return ret; > > } > > diff --git a/lib/efi_driver/efi_uclass.c b/lib/efi_driver/efi_uclass.c > > index 45f935198874..bf669742783e 100644 > > --- a/lib/efi_driver/efi_uclass.c > > +++ b/lib/efi_driver/efi_uclass.c > > @@ -145,7 +145,13 @@ static efi_status_t EFIAPI efi_uc_start( > > ret = check_node_type(controller_handle); > > if (ret != EFI_SUCCESS) > > goto err; > > - ret = bp->ops->bind(bp, controller_handle, interface); > > + > > + struct efi_handler *handler; > > + char tmpname[256] = "AppName"; > > + ret = efi_search_protocol(controller_handle, &efi_guid_device_path, > > + &handler); > > + snprintf(tmpname, 256, "%pD", handler->protocol_interface); > > + ret = bp->ops->bind(bp, controller_handle, interface, strdup(tmpname)); > > if (ret == EFI_SUCCESS) > > goto out; > > > > diff --git a/lib/efi_selftest/efi_selftest_block_device.c b/lib/efi_selftest/efi_selftest_block_device.c > > index a367e8b89d17..0ab8e4590dfe 100644 > > --- a/lib/efi_selftest/efi_selftest_block_device.c > > +++ b/lib/efi_selftest/efi_selftest_block_device.c > > @@ -248,6 +248,7 @@ static int teardown(void) > > { > > efi_status_t r = EFI_ST_SUCCESS; > > > > +#if 0 /* Temporarily out for confirmation */ > > if (disk_handle) { > > r = boottime->uninstall_protocol_interface(disk_handle, > > &guid_device_path, > > @@ -273,6 +274,7 @@ static int teardown(void) > > return EFI_ST_FAILURE; > > } > > } > > +#endif > > return r; > > } > > > > -- > > 2.41.0 > > > > Otherwise this looks good to me. We should have DM devices for all EFI > ones (in fact EFI ones should just be some extra data on top of DM > ones). Unfortunately, in this specific case (efi_block_device.c), UEFI object (handle) is set to be created first, then U-Boot device (efiblk#xxx). So "some extra data on top of DM ones" is not accurate (doesn't reflect the current implementation). Please note again that efi_loader/efi_disk.c and efi_driver/efi_block_device.c are totally different things. -Takahiro Akashi > > Regards, > Simon