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 mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id D8E27C433F5 for ; Tue, 12 Oct 2021 05:13:15 +0000 (UTC) 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 mail.kernel.org (Postfix) with ESMTPS id E979060F38 for ; Tue, 12 Oct 2021 05:13:14 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org E979060F38 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=linaro.org Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=lists.denx.de Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 6E65983310; Tue, 12 Oct 2021 07:13:12 +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="RY7dUa0x"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 4C5AB8341B; Tue, 12 Oct 2021 07:13:10 +0200 (CEST) Received: from mail-pl1-x635.google.com (mail-pl1-x635.google.com [IPv6:2607:f8b0:4864:20::635]) (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 7021F83305 for ; Tue, 12 Oct 2021 07:13: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-x635.google.com with SMTP id x8so12719421plv.8 for ; Mon, 11 Oct 2021 22:13:05 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:mail-followup-to:references :mime-version:content-disposition:in-reply-to; bh=stRCapaeJCxqPMUk55EtsyE5E8hpECOqKU6JcEbcKJI=; b=RY7dUa0xWnR1HgVUy7mhywXFZPzfT+eA1HTBNa9b25pTOoMv9qPLx7mH7MYlxJDzHC XAWe0anxCnvpLjjkekUyrRt402++lZVUUUXOrA9oGeJ/0nQX80GUJDyKNjgvFKNlWLgG luAqkJN+LJp+3VJuR43H4ehcycIbGbnvh262eOLTcSmHWiuAcCht5i/9AB8Our9aMSId JBzEkAq+B+VAYEJsBYg3el4y4dsuC2mA6WleVEFD1fX2lphQPt5s/OX3zGf+InGZjS/h OgMX6YR4eGUjiK9GNjcvVHYmoHpm6ARcucMVwuSxeRgPejP4NQ0/WDE98qwoIEmSELMP slhw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id :mail-followup-to:references:mime-version:content-disposition :in-reply-to; bh=stRCapaeJCxqPMUk55EtsyE5E8hpECOqKU6JcEbcKJI=; b=xlrjsQ3O6eBPFI7o0kd1s5SNzBqA8iHXAMlZmf70YcF7WyzIjqEmXoVlRkxNppCul5 SWbEI5REQWabYBesUgHq58WrZjtTBjy1r7+V2wre+52eBfSO0qr4694NhMHcIlhbWrHg aZKzZsmg3zSwPFvSZ90sVGrt3+/fw6Y4DvbsCLE1imart7+NapopcfOXbXuSk+u3GZ8R BZ7SblSuQEOi3Zi6t3KCHYtWpVxaWDY1FlH7+9jHDDLE3RApqdu8FU+MnZ+sXF41JvQF Z5kweJylQB4GrOP4RWQzvp2YRsSsvZHAtKw7Iob0AvvKlsq74HKOInUDv+hL7iUQDxKB rssA== X-Gm-Message-State: AOAM53320yCyEiZmmHXtBwo2p0Rl4fi2FtSDnNnDS4W5aCxYuNPvU+we kJ/SzigvDwr0XzeXnPaztcGkYQ== X-Google-Smtp-Source: ABdhPJwaBLHIun7dEy92z/o/4KdE/beV0+txebHK/qvktlDsBElAHRUfnZNNF2rbqwH7HG6tlCNHtw== X-Received: by 2002:a17:902:7043:b0:13e:1007:3d6d with SMTP id h3-20020a170902704300b0013e10073d6dmr28162082plt.79.1634015583516; Mon, 11 Oct 2021 22:13:03 -0700 (PDT) Received: from laputa ([2400:4050:c3e1:100:dbc:a6fe:8374:7fdb]) by smtp.gmail.com with ESMTPSA id f33sm1085959pjk.42.2021.10.11.22.13.01 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 11 Oct 2021 22:13:03 -0700 (PDT) Date: Tue, 12 Oct 2021 14:12:59 +0900 From: AKASHI Takahiro To: Simon Glass Cc: Heinrich Schuchardt , U-Boot Mailing List , Alex Graf , Ilias Apalodimas Subject: Re: [RFC 07/22] dm: blk: add UCLASS_PARTITION Message-ID: <20211012051259.GG38222@laputa> Mail-Followup-To: AKASHI Takahiro , Simon Glass , Heinrich Schuchardt , U-Boot Mailing List , Alex Graf , Ilias Apalodimas References: <20211004032759.GC39720@laputa> <20211008005156.GA34717@laputa> <3455705e-1b04-cf01-3cff-b89f673c5e05@gmx.de> <20211011022925.GE44356@laputa> <64774cd7-2119-347a-4d2b-3715fa2a8029@gmx.de> <9e772e73-c5a3-14dd-657a-a3684c5af492@gmx.de> 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.34 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.2 at phobos.denx.de X-Virus-Status: Clean Simon, Heinrich, On Mon, Oct 11, 2021 at 11:41:02AM -0600, Simon Glass wrote: > Hi Heinrich,, > > On Mon, 11 Oct 2021 at 10:53, Heinrich Schuchardt wrote: > > > > > > > > On 10/11/21 18:14, Simon Glass wrote: > > > Hi Heinrich, > > > > > > On Mon, 11 Oct 2021 at 09:02, Heinrich Schuchardt wrote: > > >> > > >> > > >> > > >> On 10/11/21 16:54, Simon Glass wrote: > > >>> Hi Takahiro, > > >>> > > >>> On Sun, 10 Oct 2021 at 20:29, AKASHI Takahiro > > >>> wrote: > > >>>> > > >>>> Heinrich, > > >>>> > > >>>> On Fri, Oct 08, 2021 at 10:23:52AM +0200, Heinrich Schuchardt wrote: > > >>>>> > > >>>>> > > >>>>> On 10/8/21 02:51, AKASHI Takahiro wrote: > > >>>>>> On Mon, Oct 04, 2021 at 12:27:59PM +0900, AKASHI Takahiro wrote: > > >>>>>>> On Fri, Oct 01, 2021 at 11:30:37AM +0200, Heinrich Schuchardt wrote: > > >>>>>>>> > > >>>>>>>> > > >>>>>>>> On 10/1/21 07:01, AKASHI Takahiro wrote: > > >>>>>>>>> UCLASS_PARTITION device will be created as a child node of > > >>>>>>>>> UCLASS_BLK device. > > >>>>>>>>> > > >>>>>>>>> Signed-off-by: AKASHI Takahiro > > >>>>>>>>> --- > > >>>>>>>>> drivers/block/blk-uclass.c | 111 +++++++++++++++++++++++++++++++++++++ > > >>>>>>>>> include/blk.h | 9 +++ > > >>>>>>>>> include/dm/uclass-id.h | 1 + > > >>>>>>>>> 3 files changed, 121 insertions(+) > > >>>>>>>>> > > >>>>>>>>> diff --git a/drivers/block/blk-uclass.c b/drivers/block/blk-uclass.c > > >>>>>>>>> index 83682dcc181a..dd7f3c0fe31e 100644 > > >>>>>>>>> --- a/drivers/block/blk-uclass.c > > >>>>>>>>> +++ b/drivers/block/blk-uclass.c > > >>>>>>>>> @@ -12,6 +12,7 @@ > > >>>>>>>>> #include > > >>>>>>>>> #include > > >>>>>>>>> #include > > >>>>>>>>> +#include > > >>>>>>>>> #include > > >>>>>>>>> #include > > >>>>>>>>> #include > > >>>>>>>>> @@ -695,6 +696,44 @@ int blk_unbind_all(int if_type) > > >>>>>>>>> return 0; > > >>>>>>>>> } > > >>>>>>>>> > > >>>>>>>>> +int blk_create_partitions(struct udevice *parent) > > >>>>>>>>> +{ > > >>>>>>>>> + int part, count; > > >>>>>>>>> + struct blk_desc *desc = dev_get_uclass_plat(parent); > > >>>>>>>>> + struct disk_partition info; > > >>>>>>>>> + struct disk_part *part_data; > > >>>>>>>>> + char devname[32]; > > >>>>>>>>> + struct udevice *dev; > > >>>>>>>>> + int ret; > > >>>>>>>>> + > > >>>>>>>>> + if (!CONFIG_IS_ENABLED(PARTITIONS) || > > >>>>>>>>> + !CONFIG_IS_ENABLED(HAVE_BLOCK_DEVICE)) > > >>>>>>>>> + return 0; > > >>>>>>>>> + > > >>>>>>>>> + /* Add devices for each partition */ > > >>>>>>>>> + for (count = 0, part = 1; part <= MAX_SEARCH_PARTITIONS; part++) { > > >>>>>>>>> + if (part_get_info(desc, part, &info)) > > >>>>>>>>> + continue; > > >>>>>>>>> + snprintf(devname, sizeof(devname), "%s:%d", parent->name, > > >>>>>>>>> + part); > > >>>>>>>>> + > > >>>>>>>>> + ret = device_bind_driver(parent, "blk_partition", > > >>>>>>>>> + strdup(devname), &dev); > > >>>>>>>>> + if (ret) > > >>>>>>>>> + return ret; > > >>>>>>>>> + > > >>>>>>>>> + part_data = dev_get_uclass_plat(dev); > > >>>>>>>>> + part_data->partnum = part; > > >>>>>>>>> + part_data->gpt_part_info = info; > > >>>>>>>>> + count++; > > >>>>>>>>> + > > >>>>>>>>> + device_probe(dev); > > >>>>>>>>> + } > > >>>>>>>>> + debug("%s: %d partitions found in %s\n", __func__, count, parent->name); > > >>>>>>>>> + > > >>>>>>>>> + return 0; > > >>>>>>>>> +} > > >>>>>>>>> + > > >>>>>>>>> static int blk_post_probe(struct udevice *dev) > > >>>>>>>>> { > > >>>>>>>>> if (IS_ENABLED(CONFIG_PARTITIONS) && > > >>>>>>>>> @@ -713,3 +752,75 @@ UCLASS_DRIVER(blk) = { > > >>>>>>>>> .post_probe = blk_post_probe, > > >>>>>>>>> .per_device_plat_auto = sizeof(struct blk_desc), > > >>>>>>>>> }; > > >>>>>>>>> + > > >>>>>>>>> +static ulong blk_part_read(struct udevice *dev, lbaint_t start, > > >>>>>>>>> + lbaint_t blkcnt, void *buffer) > > >>>>>>>>> +{ > > >>>>>>>>> + struct udevice *parent; > > >>>>>>>>> + struct disk_part *part; > > >>>>>>>>> + const struct blk_ops *ops; > > >>>>>>>>> + > > >>>>>>>>> + parent = dev_get_parent(dev); > > >>>>>>>> > > >>>>>>>> What device type will the parent have if it is a eMMC hardware partition? > > >>>>>>>> > > >>>>>>>>> + ops = blk_get_ops(parent); > > >>>>>>>>> + if (!ops->read) > > >>>>>>>>> + return -ENOSYS; > > >>>>>>>>> + > > >>>>>>>>> + part = dev_get_uclass_plat(dev); > > >>>>>>>> > > >>>>>>>> You should check that we do not access the block device past the > > >>>>>>>> partition end: > > >>>>>>> > > >>>>>>> Yes, I will fix all of checks. > > >>>>>>> > > >>>>>>>> struct blk_desc *desc = dev_get_uclass_plat(parent); > > >>>>>>>> if ((start + blkcnt) * desc->blksz < part->gpt_part_info.blksz) > > >>>>>>>> return -EFAULT. > > >>>>>>>> > > >>>>>>>>> + start += part->gpt_part_info.start; > > >>>>>> > > >>>>>> A better solution is: > > >>>>>> if (start >= part->gpt_part_info.size) > > >>>>>> return 0; > > >>>>>> > > >>>>>> if ((start + blkcnt) > part->gpt_part_info.size) > > >>>>>> blkcnt = part->gpt_part_info.size - start; > > >>>>>> start += part->gpt_part_info.start; > > >>>>>> instead of returning -EFAULT. > > >>>>>> (note that start and blkcnt are in "block".) > > >>>>> > > >>>>> What is your motivation to support an illegal access? > > >>>>> > > >>>>> We will implement the EFI_BLOCK_IO_PROTOCOL based on this function. The > > >>>>> ReadBlocks() and WriteBlocks() services must return > > >>>>> EFI_INVALID_PARAMETER if the read request contains LBAs that are not > > >>>>> valid. > > >>>> > > >>>> I interpreted that 'LBA' was the third parameter to ReadBlocks API, > > >>>> and that if the starting block is out of partition region, we should > > >>>> return an error (and if not, we still want to trim IO request to fit > > >>>> into partition size as other OS's API like linux does). > > >>>> Do you think it's incorrect? > > >>> > > >>> [..] > > >>> > > >>> Related to this patch I think that the partition type should be really > > >>> be a child of the media device: > > >>> > > >>> - MMC > > >>> |- BLK > > >>> |- PARTITION > > >>> |- BLK > > >>> |- PARTITION > > >>> |- BLK > > >>> |- PARTITION > > >>> |- BLK > > >>> > > >>> It seems more natural to me that putting the partitions under the > > >>> top-level BLK device, so that BLK remains a 'terminal' device. > > >>> > > >>> The partition uclass is different from BLK, of course. It could > > >>> contain information about the partition such as its partition number > > >>> and UUID. > > >> > > >> Do you mean hardware partition here? Otherwise I would not know what BLK > > >> should model. > > > > > > I mean that (I think) we should not use BLK to model partitions. A BLK > > > should just be a block device. > > > > That is fine. But this implies that a software partition is the child of > > a block partition and not the other way round. So the tree should like: > > > > MMC > > |- BLK (user hardware partition) > > ||- PARTITION 1 (software partition) > > ||- PARTITION 2 (software partition) > > |... > > ||- PARTITION n (software partition) > > |- BLK (rpmb hardware partition) > > |- BLK (boot0 hardware partition) > > |- BLK (boot1 hardware partition) > > I presume you meant to include a BLK device under each PARTITION? I do understand that PARTITION is not BLK, but thinking that there is always one-to-one link between a PARTITION device and a BLK device, what benefit will we see in distinguishing one from the other? I'm afraid that this makes things a bit complicated. > But anyway, I was more thinking of this: > > MMC > | HWPARTITION rpmb > || BLK whole rpmb > || PARTITION 1 > ||| BLK > || PARTITION 2 > ||| BLK > || PARTITION 3 > ||| BLK > | HWPARTITION boot0 > || BLK > (maybe have PARTITION in here too? > | HWPARTITION boot1 > (maybe have PARTITION in here too? > || BLK I simply wonder why not we see "HWPARTITION" as yet another block device [controller] (like scsi LUN's and/or NVME namespaces as discussed in a thread of "part: call part_init() in blk_get_device_by_str() only for MMC"?). > > > > > > > > I don't see any difference between a partition and a hardware > > > partition. We presumably end up with a hierarchy though. Do we need a > > > HWPARTITION uclass so we can handle the hardware partitions > > > differently? > > > > Software partitions are defined and discovered via partition tables. > > Hardware partitions are defined in a hardware specific way. > > > > All software partitions map to HD() device tree nodes in UEFI. > > An MMC device maps to an eMMC() node > > MMC hardware partitions are mapped to Ctrl() nodes by EDK II. We should > > do the same in U-Boot. > > An SD-card maps to an SD() node. > > An NVMe namespace maps to a NVMe() node. > > An SCSI LUN maps to a Scsi() node. > > SCSI channels of multiple channel controllers are mapped to Ctrl() nodes. > > I'm not quite sure about the terminology here. I'm not even talking > about UEFI, really, just how best to model this stuff in U-Boot. Anyhow, if we pursue the direction that you suggested here, we will end up with having one PARTITION *driver* per partition type, efi, dos or iso, under disk/? # Oops, extra work :) Thanks, -Takahiro Akashi > In U-Boot, UCLASS_SCSI should be a SCSI controller, not a device, > right? I'm a little worried it is not modelled correctly. After all, > what is the parent of a SCSI device? > > > > > The simple file protocol is only provided by HD() nodes and not by nodes > > representing hardware partitions. If the whole hardware partition is > > formatted as a file system you would still create a HD() node with > > partition number 0. > > Regards, > Simon