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 69CD8C05027 for ; Mon, 6 Feb 2023 08:30:30 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 1B48385A27; Mon, 6 Feb 2023 09:30:28 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=none (p=none dis=none) header.from=waldekranz.com 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=waldekranz-com.20210112.gappssmtp.com header.i=@waldekranz-com.20210112.gappssmtp.com header.b="OgaCgOiP"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 5946685A0C; Mon, 6 Feb 2023 09:30:26 +0100 (CET) Received: from mail-oi1-x236.google.com (mail-oi1-x236.google.com [IPv6:2607:f8b0:4864:20::236]) (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 0941F859D7 for ; Mon, 6 Feb 2023 09:30:22 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=none (p=none dis=none) header.from=waldekranz.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=tobias@waldekranz.com Received: by mail-oi1-x236.google.com with SMTP id t5so1584649oiw.1 for ; Mon, 06 Feb 2023 00:30:22 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=waldekranz-com.20210112.gappssmtp.com; s=20210112; h=mime-version:message-id:date:references:in-reply-to:subject:cc:to :from:from:to:cc:subject:date:message-id:reply-to; bh=7lH7IpMsOaOyKBYVRWojMzSaCgEuS2q6C+Q7K0+0NLg=; b=OgaCgOiPgycJq0+Be0VPRbYcT1iAHDZa/N9jykLMK7Ky1AHSsb1ZknOtK8Z88J6rTa mTSf7y5Hecxwx4B3UcEp1B9cRBcIoHtBtAPObsIMzyOzd6+l2dWgPZyUDQpJWTt/Jd5c nSeyP+Q8qaFExCqok9iVJuC2BdK7iGnz/JxavvyxaWsFh0vGti2oPQco8b2ennXEV2g7 6zeYJ84yRvZQ8tLhkz/cHIdzflpOacbwrz3kbVfMXUenA4PbVjea+n0/vrPVXnDa2WY0 boPZLkwXQyJ/1/wp1O9TDCsosfwBlde2HXnhNm/3CGN4PMOnrvmTAXbAWLqwitPf66m4 7nAg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=mime-version:message-id:date:references:in-reply-to:subject:cc:to :from:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=7lH7IpMsOaOyKBYVRWojMzSaCgEuS2q6C+Q7K0+0NLg=; b=6aApzd0d8Kqrt8XzaSsE6S2BIqTCSkNYmVCYZBkN+5dHxocu4Bhs3xm94fdFKwT/cE 7EdmK87jjfAVfMnDIt3302ImDVIo9+JDObPailTsI13A+Hfzue5n5Lqd2A5o8NgUmAU9 bgK3aw0NbPRKB/YBLFylq+0n46/lEptGYea+FwjVFUC9erbKf2F9r7yfeyjSCfuoN1YV ygW07DHIduyIkbC2dc4TTKWvl3OT8pJ9znooJXdk6iJmtIvAAL85UARv0LEPrgftHYLT uaFggBEgh8vpIKkNZd4LLTQnRV7tox7LmjuIVNYAwQUwZ4KCnExJd/hfMYe1xnugUxil ms/w== X-Gm-Message-State: AO0yUKXbwGVLTuT6vaMmuDJwtA8Hqrg1M6M5qbDSru6M7HSJ1TKp/A5s aYcn5PjMca6q3xf/hSiYwAZYvdSKMk9irb1d X-Google-Smtp-Source: AK7set8+U7csCeUuDI+52AK6C76GC/YtXYQjRvyec0SaebSW7sq6CNS/xFNYzWtIspHmCIRW+w/i2A== X-Received: by 2002:a05:6808:8e2:b0:36a:7d3c:b423 with SMTP id d2-20020a05680808e200b0036a7d3cb423mr10008576oic.21.1675672221125; Mon, 06 Feb 2023 00:30:21 -0800 (PST) Received: from wkz-x13 (a124.broadband3.quicknet.se. [46.17.184.124]) by smtp.gmail.com with ESMTPSA id bm49-20020a0568081ab100b0035c073aa0d8sm3834316oib.18.2023.02.06.00.30.19 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 06 Feb 2023 00:30:20 -0800 (PST) From: Tobias Waldekranz To: Simon Glass Cc: xypron.glpk@gmx.de, ilias.apalodimas@linaro.org, u-boot@lists.denx.de Subject: Re: [PATCH 3/8] blk: blkmap: Add basic infrastructure In-Reply-To: References: <20230201181016.4145834-1-tobias@waldekranz.com> <20230201181016.4145834-4-tobias@waldekranz.com> <875ycjm6li.fsf@waldekranz.com> Date: Mon, 06 Feb 2023 09:30:17 +0100 Message-ID: <87357jmc12.fsf@waldekranz.com> MIME-Version: 1.0 Content-Type: text/plain 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 fre, feb 03, 2023 at 17:20, Simon Glass wrote: > Hi Tobias, > > On Fri, 3 Feb 2023 at 02:38, Tobias Waldekranz wrote: >> >> On ons, feb 01, 2023 at 13:20, Simon Glass wrote: >> > Hi Tobias, >> >> Hi Simon, >> >> Thanks for the review! >> >> > On Wed, 1 Feb 2023 at 11:10, Tobias Waldekranz wrote: >> >> >> >> blkmaps are loosely modeled on Linux's device mapper subsystem. The >> >> basic idea is that you can create virtual block devices whose blocks >> >> can be backed by a plethora of sources that are user configurable. >> >> >> >> This change just adds the basic infrastructure for creating and >> >> removing blkmap devices. Subsequent changes will extend this to add >> >> support for actual mappings. >> >> >> >> Signed-off-by: Tobias Waldekranz >> >> --- >> >> MAINTAINERS | 6 + >> >> disk/part.c | 1 + >> >> drivers/block/Kconfig | 18 ++ >> >> drivers/block/Makefile | 1 + >> >> drivers/block/blk-uclass.c | 1 + >> >> drivers/block/blkmap.c | 275 +++++++++++++++++++++++++++++++ >> >> include/blkmap.h | 15 ++ >> >> include/dm/uclass-id.h | 1 + >> >> include/efi_loader.h | 4 + >> >> lib/efi_loader/efi_device_path.c | 30 ++++ >> >> 10 files changed, 352 insertions(+) >> >> create mode 100644 drivers/block/blkmap.c >> >> create mode 100644 include/blkmap.h >> >> > > [..] > >> > This needs to be created as part of DM. See how host_create_device() >> > works. It attaches something to the uclass and then creates child >> > devices from there. It also operations (struct host_ops) but you don't >> > need to do that. >> > >> > Note that the host commands support either an label or a devnum, which >> > I think is useful, so you might copy that? >> > >> >> I took a look at the hostfs implementation. I agree that labels are much >> nicer than bare integers. However, for block maps the idea is to fit in >> to the existing filesystem infrastructure. Addressing block devices >> using the " [:]" pattern seems very well >> established... > > You can still do that, so long as the labels are "0" and "1", etc. But > it lets us move to a more flexible system in future. > >> >> >> +{ >> >> + static struct udevice *dev; >> >> + int err; >> >> + >> >> + if (dev) >> >> + return dev; >> >> + >> >> + err = device_bind_driver(dm_root(), "blkmap_root", "blkmap", &dev); >> >> + if (err) >> >> + return NULL; >> >> + >> >> + err = device_probe(dev); >> >> + if (err) { >> >> + device_unbind(dev); >> >> + return NULL; >> >> + } >> > >> > Should not be needed as probing children will cause this to be probed. >> > >> > So this function just becomes >> > >> > uclass_first_device(UCLASS_BLKDEV, & >> > >> >> + >> >> + return dev; >> >> +} >> >> + >> >> +int blkmap_create(int devnum) >> > >> > Again, please drop the use of devnum and use devices. Here you could >> > use a label, perhaps? >> >> ...which is why I don't think a label is going to fly here. Let's say I >> create a new ramdisk with a label instead, e.g.: >> >> blkmap create rd >> blkmap map rd 0 0x100 mem ${loadaddr} >> >> How do I know which to supply to, e.g.: >> >> ls blkmap /boot >> >> It seems like labels are a hostfs-specific feature, or am I missing >> something? > > We have the same problem with hostfs, since we have not implemented > labels in block devices. For now you must use integer labels. But we > will get there. But there is no connection to the devnum that is allocated internally by U-Boot. Here's an experiment I just ran: I created two squashfs images containing a single directory: zero.squashfs: i_am_zero one.squashfs: i_am_one Then I added a binding to them: => host bind 1 one.squashfs => host bind 0 zero.squashfs When accessing them, we see that the existing filesystem utilities work on the internally generated devnums, ignoring the labels: => ls host 1 i_am_zero/ 0 file(s), 1 dir(s) => ls host 0 i_am_one/ 0 file(s), 1 dir(s) => Doesn't it therefore make more sense to stick with the established abstraction? >> >> >> +{ >> >> + struct udevice *root; >> > >> > Please don't use that name , as we use it for the DM root device. How >> > about bm_parent? It isn't actually a tree of devices, so root doesn't >> > sound right to me anyway. >> >> Alright, I'll change it. >> >> >> + struct blk_desc *bd; >> >> + struct blkmap *bm; >> >> + int err; >> >> + >> >> + if (devnum >= 0 && blkmap_from_devnum(devnum)) >> >> + return -EBUSY; >> >> + >> >> + root = blkmap_root(); >> >> + if (!root) >> >> + return -ENODEV; >> >> + >> >> + bm = calloc(1, sizeof(*bm)); >> > >> > Can this be attached to the device as private data, so avoiding the malloc()? >> >> Maybe, I'm not familiar enough with the U-Boot internals. >> >> As it is now, all blkmaps are children of a single "blkmap_root" >> device. I chose that approach so that devnums would be allocated from a >> single pool. > > Well, driver model handles this for you (see dev_seq()). You have a > single uclass so can attach your 'overall' blkmap data to that. Then > each device can have its own private data attached. > > The only requirement is that BLK devices have a parent (so we know the > media type). I had understood that you had one blkmap for each block > map. If that is true, then you don't need to have a parent one as > well. You can use the BLKMAP uclass to hold any overall data. > >> >> AFAIK, that would mean having to store it in the "blkmap_blk" device, >> but I thought that its private data was owned by the block subsystem? > > [..] > > Regards, > Simon