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 EE837C43334 for ; Fri, 22 Jul 2022 02:00:40 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 143F28300F; Fri, 22 Jul 2022 04:00:38 +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="VEzZabaH"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 301F783445; Fri, 22 Jul 2022 04:00:37 +0200 (CEST) Received: from mail-yb1-xb30.google.com (mail-yb1-xb30.google.com [IPv6:2607:f8b0:4864:20::b30]) (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 D07FA82105 for ; Fri, 22 Jul 2022 04:00:33 +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=masahisa.kojima@linaro.org Received: by mail-yb1-xb30.google.com with SMTP id l11so5777240ybu.13 for ; Thu, 21 Jul 2022 19:00:33 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=oUi7RzPbkwBHHuI6aPdb+wsT5KTRm8GFPlXWHT6e09U=; b=VEzZabaH2/elg8Rc3eN0R4n2ugDitPz+TOSKMqr0015oy09qvAo4oSZX9NdzawOfJ4 p1yZLNaaKA0Ri7zYaWT6DGs6VF9cxF5Bom8FGztBKxkpA9jamN9cl92gOp/pQmeYtzmt hVGJYbyFrfdYtc0mUZ70v59DnOHkeoTywE3RgaVGZm2L6svL1D3IOKBQMhewDK0DLosz eZBrHsiHanhZZtYADOX/LecgnYfJ6Qd+e0WVnurD0WMXOKX3DLsRhAV1aOdW8DCKvpls rQ1gYEtEVAmDd/w2xgw4Sme9efYqDf1IkuLAcdtxYG2NzZC/sdJu6PU0uA3Az1b2jJ0b OT6A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=oUi7RzPbkwBHHuI6aPdb+wsT5KTRm8GFPlXWHT6e09U=; b=zUrJ/j3g1coz3oCgg5Jepq0xsdO21VlvTCP5UJJqdI4XdWlqYeXm0iiBgMPN/sTqsm YYqPLvBAxI+ZrOENRxz2x9QcXYRj/6+l3DkW0f0x1SKZAPFEK5lapvphu5zZbO9KHFGV R81tzwyWEmrYq81ccicb7/Dk90SEMTr5z3+75Wi0U1Di5iY1g7npWMkyymcV/Cy/hYJT cuI+tDVZHTy+j/0vUtXe9jV7YikpCgS3kqy7Zk5cQuET0iZYAz40yRfSb6503BlYe5fa WOH37NqRJliIbteHQJOpJFuLZmNSlIVZKTpjoGmwJ/x01wbp/G/rrDbR/M21ekcOWLXp ycNg== X-Gm-Message-State: AJIora9g7nAD72hKg5rcG4+6NiqPXTtyqClILboVR5cZVkCZ6isjmzup OOAINtlgQGUWLQnv8k2kVrGhqhRFNboE+ahf/t8TlQ== X-Google-Smtp-Source: AGRyM1umFnrX27Narx9FV2E02J8KV0DaS4UAZbPUxsCo4FZCnY71TUwx1Ne85/MalWy7zu99OBcfzYNzH9JiM1rOwgY= X-Received: by 2002:a5b:3ce:0:b0:66f:4692:27a2 with SMTP id t14-20020a5b03ce000000b0066f469227a2mr1202716ybp.167.1658455232351; Thu, 21 Jul 2022 19:00:32 -0700 (PDT) MIME-Version: 1.0 References: <20220715144749.30564-1-masahisa.kojima@linaro.org> <20220715144749.30564-2-masahisa.kojima@linaro.org> <0a2d9892-7bc7-a4e4-6052-325edcc7ace6@gmx.de> <20220720052326.GD36287@laputa> In-Reply-To: From: Masahisa Kojima Date: Fri, 22 Jul 2022 11:00:20 +0900 Message-ID: Subject: Re: [RESEND v9 1/9] efi_loader: move udevice pointer into struct efi_object To: Heinrich Schuchardt Cc: Takahiro Akashi , Ilias Apalodimas , Simon Glass , Mark Kettenis , u-boot@lists.denx.de Content-Type: text/plain; charset="UTF-8" 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 Hi Heinrich, Akashi-san, On Wed, 20 Jul 2022 at 16:42, Heinrich Schuchardt wrote: > > On 7/20/22 07:23, Takahiro Akashi wrote: > > On Sun, Jul 17, 2022 at 01:23:41PM +0200, Heinrich Schuchardt wrote: > >> On 7/17/22 10:09, Heinrich Schuchardt wrote: > >>> On 7/15/22 16:47, Masahisa Kojima wrote: > >>>> This is a preparation patch to provide the unified method > >>>> to access udevice pointer associated with the block io device. > >>>> The EFI handles of both EFI block io driver implemented in > >>>> lib/efi_loader/efi_disk.c and EFI block io driver implemented > >>>> as EFI payload can posess the udevice pointer in the struct efi_object. > >>>> > >>>> We can use this udevice pointer to get the U-Boot friendly > >>>> block device name(e.g. mmc 0:1, nvme 0:1) through efi_handle_t. > >>>> > >>>> Signed-off-by: Masahisa Kojima > >>>> --- > >>>> Newly created in v9 > >>>> > >>>> include/efi_loader.h | 8 ++++++++ > >>>> lib/efi_loader/efi_disk.c | 20 +++++++++++++------- > >>>> 2 files changed, 21 insertions(+), 7 deletions(-) > >>>> > >>>> diff --git a/include/efi_loader.h b/include/efi_loader.h > >>>> index 3a63a1f75f..bba5ffd482 100644 > >>>> --- a/include/efi_loader.h > >>>> +++ b/include/efi_loader.h > >>>> @@ -226,6 +226,12 @@ const char *__efi_nesting_dec(void); > >>>> #define EFI_CACHELINE_SIZE 128 > >>>> #endif > >>>> > >>>> +/** > >>>> + * efi_handle_to_udev - accessor to the DM device associated to the > >>>> EFI handle > >>>> + * @handle: pointer to the EFI handle > >>>> + */ > >>>> +#define efi_handle_to_udev(handle) (((struct efi_object *)handle)->dev) > >>> > >>> This conversion will hide errors if handle is not of type efi_handle_t. > >>> We should avoid the conversion and see build time errors instead. > >>> Please, remove the macro. > >>> > >>> For every handle of type efi_handle_t you can access the field > >>> handle->dev directly. > >>> > >>> For struct efi_disk_obj we can use disk->header.dev. > >>> > >>>> + > >>>> /* Key identifying current memory map */ > >>>> extern efi_uintn_t efi_memory_map_key; > >>>> > >>>> @@ -375,6 +381,7 @@ enum efi_object_type { > >>>> * @protocols: linked list with the protocol interfaces installed > >>>> on this > >>>> * handle > >>>> * @type: image type if the handle relates to an image > >>>> + * @dev: pointer to the DM device which is associated with this > >>>> EFI handle > >>>> * > >>>> * UEFI offers a flexible and expandable object model. The objects > >>>> in the UEFI > >>>> * API are devices, drivers, and loaded images. struct efi_object is > >>>> our storage > >>>> @@ -392,6 +399,7 @@ struct efi_object { > >>>> /* The list of protocols */ > >>>> struct list_head protocols; > >>>> enum efi_object_type type; > >>>> + struct udevice *dev; > >>>> }; > >>>> > >>>> enum efi_image_auth_status { > >>>> diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c > >>>> index 1d700b2a6b..a8e8521e3e 100644 > >>>> --- a/lib/efi_loader/efi_disk.c > >>>> +++ b/lib/efi_loader/efi_disk.c > >>>> @@ -46,7 +46,6 @@ struct efi_disk_obj { > >>>> struct efi_device_path *dp; > >>>> unsigned int part; > >>>> struct efi_simple_file_system_protocol *volume; > >>>> - struct udevice *dev; /* TODO: move it to efi_object */ > >>> > >>> ok > >>> > >>>> }; > >>>> > >>>> /** > >>>> @@ -124,16 +123,16 @@ static efi_status_t efi_disk_rw_blocks(struct > >>>> efi_block_io *this, > >>>> return EFI_BAD_BUFFER_SIZE; > >>>> > >>>> if (CONFIG_IS_ENABLED(PARTITIONS) && > >>>> - device_get_uclass_id(diskobj->dev) == UCLASS_PARTITION) { > >>>> + device_get_uclass_id(efi_handle_to_udev(diskobj)) == > >>>> UCLASS_PARTITION) { > >>> > >>> device_get_uclass_id(diskobj->header.hdev)) == UCLASS_PARTITION) { > >>> > >>>> if (direction == EFI_DISK_READ) > >>>> - n = dev_read(diskobj->dev, lba, blocks, buffer); > >>>> + n = dev_read(efi_handle_to_udev(diskobj), lba, blocks, > >>>> buffer); > >>> > >>> dev_read(diskobj->header.hdev) > >>> > >>>> else > >>>> - n = dev_write(diskobj->dev, lba, blocks, buffer); > >>>> + n = dev_write(efi_handle_to_udev(diskobj), lba, blocks, > >>>> buffer); > >>> > >>> dev_write(diskobj->header.hdev) > >>> > >>>> } else { > >>>> /* dev is a block device (UCLASS_BLK) */ > >>>> struct blk_desc *desc; > >>>> > >>>> - desc = dev_get_uclass_plat(diskobj->dev); > >>>> + desc = dev_get_uclass_plat(efi_handle_to_udev(diskobj)); > >>> > >>> dev_get_uclass(diskobj->header.hdev) > >>> > >>> > >>>> if (direction == EFI_DISK_READ) > >>>> n = blk_dread(desc, lba, blocks, buffer); > >>>> else > >>>> @@ -552,7 +551,7 @@ static int efi_disk_create_raw(struct udevice *dev) > >>>> > >>>> return -1; > >>>> } > >>>> - disk->dev = dev; > >>>> + efi_handle_to_udev(disk) = dev; > >>>> if (dev_tag_set_ptr(dev, DM_TAG_EFI, &disk->header)) { > >>>> efi_free_pool(disk->dp); > >>>> efi_delete_handle(&disk->header); > >>>> @@ -609,7 +608,7 @@ static int efi_disk_create_part(struct udevice *dev) > >>>> log_err("Adding partition for %s failed\n", dev->name); > >>>> return -1; > >>>> } > >>>> - disk->dev = dev; > >>>> + efi_handle_to_udev(disk) = dev; > >>> > >>> disk->header.dev = dev; > >>> > >>>> if (dev_tag_set_ptr(dev, DM_TAG_EFI, &disk->header)) { > >>>> efi_free_pool(disk->dp); > >>>> efi_delete_handle(&disk->header); > >>>> @@ -656,6 +655,13 @@ static int efi_disk_probe(void *ctx, struct event > >>>> *event) > >>>> ret = efi_disk_create_raw(dev); > >>>> if (ret) > >>>> return -1; > >>>> + } else { > >>>> + efi_handle_t handle; > >>>> + > >>>> + if (dev_tag_get_ptr(dev, DM_TAG_EFI, (void **)&handle)) > >> > >> Setting handle->dev can be done more easily in efi_bl_bind(): > > > > I don't think so. > > "dev" field should be maintained in *one* place, i.e. efi_disk_probe(), > > which is uniquely called from probe event (even for efi_block_dev case). > > > > To make this clear, we'd better put the code in efi_disk_create_raw(): > > efi_disk_create_raw() > > if (desc->if_type == IF_TYPE_EFI_LOADER) { > > /* handle should exist now */ > > dev_tag_get_ptr(dev, DM_TAG_EFI, (void **)&handle)); > > efi_set_udev(handle, dev); > > return 0; > > } > > > > /* normal block devices */ > > ... > > efi_set_udev(&disk->header, dev); > > ... > > Linking devices and handles is not block device specific. > > If we would properly link all devices and handles, we would be able to > generate device paths by walking the driver model device tree. This is > the direction into which our future development should go. > > Can we have a wrapper around dev_tag_set_ptr(dev, DM_TAG_EFI, handle) > which takes care of setting handle->dev and the tag replacing all > current invocations: > > int efi_link_dev(efi_handle_t handle, udevice dev) { > handle->dev = dev; > return dev_tag_set_ptr(dev, DM_TAG_EFI, handle); > } Thank you. I create efi_link_dev(). > >> We can further remove the field handle from struct blk_create_device as > >> it is now available in handle->dev. Do you mean struct efi_blk_plat? struct efi_blk_plat { >.......efi_handle_t>...>.......handle; >.......struct efi_block_io>....*io; }; Regards, Masahisa Kojima > > Best regards > > Heinrich > > > > > > > -Takahiro Akashi > > > >> handle->dev = bdev; > >> > >> We can further remove the field handle from struct blk_create_device as > >> it is now available in handle->dev. > >> > >> Best regards > >> > >> Heinrich > >> > >>>> + return -1; > >>>> + > >>>> + efi_handle_to_udev(handle) = dev; > >>> > >>> handle->dev = dev; > >>> > >>> Best regards > >>> > >>> Heinrich > >>> > >>>> } > >>>> > >>>> device_foreach_child(child, dev) { > >>> > >> >