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 5B80DC0015E for ; Thu, 20 Jul 2023 00:19:21 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id C7E9C867AB; Thu, 20 Jul 2023 02:19:18 +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="VqULSSt6"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 1EC4B867B2; Thu, 20 Jul 2023 02:19:12 +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 A31FF8679F for ; Thu, 20 Jul 2023 02:19:05 +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 d9443c01a7336-1b89b0c73d7so323555ad.1 for ; Wed, 19 Jul 2023 17:19:05 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1689812344; x=1690417144; 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=4u4zENJmZUwQBI2joBLinghCpa4yYjtC89NPAs9/pdA=; b=VqULSSt6u0w+zr9F31rtuBprGlyQoGj8375OUm7ZTWaAye2+dCqT0I0m0lg+3O1vlW xEIcURF+HJQOArIfPolfuz5hoCLCtk/bir9/vwuYqqKonYVQUCX4B6Y0AGjHbEhrXYK6 2JNcOFjHsqKYLDAkYHZtKaF1asOVDk6BDWUHe45ZJOoOA3dqoHlXBLOqmTkKaEENdkoB AttSPdOQoIvUGzNSJOjXtOQV4yzHN5IldXG2bYkIYlYGm4fLy3BjRM/1ljINNvB7qI0U rEqMImBoEc6yxpwBuHqvvXwPHZBoRPtomR1nEB4DAwdAYCa5fOUI96owWdTIiPrOqcgK BEOg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1689812344; x=1690417144; 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=4u4zENJmZUwQBI2joBLinghCpa4yYjtC89NPAs9/pdA=; b=RPZ2oJ+/dAea5x3v3Ag77CtRQKnrD6tqXrO6agS7YtjecIjzppfheZEwANfisHBlbn jttZxyB8Og6gYbJK32mV0b+926+43Y46GpshHZb7X+kuH+gjoGx9V4n0VJvwR+CG2+sQ a5xoDhgghXRAQrsvisVa/1rp8Zh1zraY5d++/Id6ATBXaRIeplPHVduW+4wBYFmQ6YLo WXMy9ECvetRGwnYmNMu+m2jY4UE4qRqM9um3OpVLF7ZF4QDS4JJ9Op8wPMaku424zqQT wGawfQOmOm0xyAJ/krcJHB5dvUaHteYWGcMMoVIyHrrA93/Zrh/7nBDynbSOZp5OEtb8 OhYw== X-Gm-Message-State: ABy/qLajtdSkKzPVSosMQI27vgg/FTFcmXwFCKAXonQlLGpU2GnC0QEq BQkGS9Synb8FeW8/sA8cZ0ISkV0aamIek5ueDH8= X-Google-Smtp-Source: APBJJlGkejEMcwAgnbzj8nHv0+Wig67u6oF2eEYpfURPH4Pt8Y/FERZGvJEg2J5/czlzN9rvn4Iozg== X-Received: by 2002:a17:902:e892:b0:1b8:a469:53d8 with SMTP id w18-20020a170902e89200b001b8a46953d8mr1592883plg.0.1689812343842; Wed, 19 Jul 2023 17:19:03 -0700 (PDT) Received: from laputa ([2400:4050:c3e1:100:b19f:5d18:5b09:a6df]) by smtp.gmail.com with ESMTPSA id l8-20020a170902f68800b001b892aac5c9sm4544453plg.298.2023.07.19.17.19.01 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 19 Jul 2023 17:19:03 -0700 (PDT) Date: Thu, 20 Jul 2023 09:19:00 +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 Wed, Jul 19, 2023 at 07:04:06AM -0600, Simon Glass wrote: > Hi, > > On Tue, 18 Jul 2023 at 19:54, AKASHI Takahiro > wrote: > > > > 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. > > I suggest an EFI_MEDIA device. > > > > > 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. > > Block devices must have a media device as their parent. This seems to > be a persistent area of confusion...probably when the uclass ID goes > away from blk_desc it will be more obvious. Please refer to my other comment to Heinrich in this thread. UCLASS_EFI_LOADER works exactly as UCLASS_EFI_MEDIA does as a "media" device which is set to invoke "bind" for associated devices (blocks in this case). -Takahiro Akashi > > > > > > > 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). > > OK, so we should really sort that out :-) > > > > > Please note again that efi_loader/efi_disk.c and efi_driver/efi_block_device.c > > are totally different things. > > OK > > Regards, > Simon