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 40E4BC433F5 for ; Mon, 11 Oct 2021 02:29:44 +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 25D2160F0F for ; Mon, 11 Oct 2021 02:29:42 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org 25D2160F0F 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 91D8083270; Mon, 11 Oct 2021 04:29:40 +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="cqLFu5uZ"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id DA62083475; Mon, 11 Oct 2021 04:29:38 +0200 (CEST) Received: from mail-pl1-x636.google.com (mail-pl1-x636.google.com [IPv6:2607:f8b0:4864:20::636]) (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 A610C831BA for ; Mon, 11 Oct 2021 04:29:34 +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-x636.google.com with SMTP id y1so10221961plk.10 for ; Sun, 10 Oct 2021 19:29:34 -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=G9wWh3JlvELaTy15+8TbKwaUJ1BdtdruvVYY8WTzvk8=; b=cqLFu5uZhwOQcfy6k399aecE1mrJa2hh0rE0P7w+pIMm74njfdyRcEjv9lv/bX5Y45 LBz3J4mZiguKjAiSaHyn9fOG6Z1Ycv07NrnHDrfNGV0NsXoUmK8WM/j/mJP2ifUcPdU9 wJ2ADKNoYY43E0keiM/II26s7t4CFyYPqzSHHpmZwkIfQrkp5V+hf+VUPKs1wGF9o/7z 8W3WmpuDAikMJY1lKNj/031sBJn5AO3Oa/5WgplnM1oEX6cSrjOIkrTqxYRTA79kD/3K z+O/itu5DyhNxVEnc16ZZa1RDd1pJjn2yXy1rvmasQ/yzHEvFJGSlmY56UzTZn0WyZbR xzDg== 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=G9wWh3JlvELaTy15+8TbKwaUJ1BdtdruvVYY8WTzvk8=; b=oI++gEL93YHljXSMd6vU0N43dFSFTVVA9KdPExAkbHiWLEw8PCS2lkz0CoIc2AfMMk UysvzkwRuh5w9FLK1lUBJvDIxpeoGOtYBhqGa7rDLQY/7njvNA6GprJT9PjbFTyAAYOO rYr5breaH/s5PEe6JJ8e3VU3qbV61Qz0Xztl50BzWKxZ1LzgNINGLkxS1XRv0hYWE5xM 4NpTc5SXEvMShhkScxbc5KrunL6FnF2pmhGAD6oGuBUxtmq+TrVQLqqeKKvIfrJ1wcGZ lyujD6ORTNeRB3fPR0AWpSS3xmcwuyh6cO2oDtmFiRrjwYUssA4FXYjppRdvd6zXPrpp Bukw== X-Gm-Message-State: AOAM531pvq2PRCpEL6ypY4+LR0EBLeB9B39MaLp9HdmhXmqrO7pd6neW 0tQNkiV6Jor8xl/NVhoJDAUiwA== X-Google-Smtp-Source: ABdhPJxCcnwxfxMkQnA6oZG4zF+qYgRpuksR77aEdJi7gfScy/kjEQg0n5xblXQhxnXhCVzM/At1Ng== X-Received: by 2002:a17:90a:17cd:: with SMTP id q71mr27532651pja.129.1633919372606; Sun, 10 Oct 2021 19:29:32 -0700 (PDT) Received: from laputa (122-100-26-39m5.mineo.jp. [122.100.26.39]) by smtp.gmail.com with ESMTPSA id c192sm5803563pfb.110.2021.10.10.19.29.29 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sun, 10 Oct 2021 19:29:32 -0700 (PDT) Date: Mon, 11 Oct 2021 11:29:25 +0900 From: AKASHI Takahiro To: Heinrich Schuchardt Cc: u-boot@lists.denx.de, agraf@csgraf.de, sjg@chromium.org, ilias.apalodimas@linaro.org Subject: Re: [RFC 07/22] dm: blk: add UCLASS_PARTITION Message-ID: <20211011022925.GE44356@laputa> Mail-Followup-To: AKASHI Takahiro , Heinrich Schuchardt , u-boot@lists.denx.de, agraf@csgraf.de, sjg@chromium.org, ilias.apalodimas@linaro.org References: <20211001050228.55183-1-takahiro.akashi@linaro.org> <20211001050228.55183-15-takahiro.akashi@linaro.org> <20211004032759.GC39720@laputa> <20211008005156.GA34717@laputa> <3455705e-1b04-cf01-3cff-b89f673c5e05@gmx.de> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <3455705e-1b04-cf01-3cff-b89f673c5e05@gmx.de> 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 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? -Takahiro Akashi > So we should return an error code here that can be translated > into EFI_INVALID_PARAMETER. > > Best regards > > Heinrich > > > > > -Takahiro Akashi > > > > > > > + > > > > > + return ops->read(parent, start, blkcnt, buffer); > > > > > +} > > > > > + > > > > > +static ulong blk_part_write(struct udevice *dev, lbaint_t start, > > > > > + lbaint_t blkcnt, const void *buffer) > > > > > +{ > > > > > + struct udevice *parent; > > > > > + struct disk_part *part; > > > > > + const struct blk_ops *ops; > > > > > + > > > > > + parent = dev_get_parent(dev); > > > > > + ops = blk_get_ops(parent); > > > > > + if (!ops->write) > > > > > + return -ENOSYS; > > > > > + > > > > > + part = dev_get_uclass_plat(dev); > > > > > + start += part->gpt_part_info.start; > > > > > > > > here too > > > > > > > > > + > > > > > + return ops->write(parent, start, blkcnt, buffer); > > > > > +} > > > > > + > > > > > +static ulong blk_part_erase(struct udevice *dev, lbaint_t start, > > > > > + lbaint_t blkcnt) > > > > > +{ > > > > > + struct udevice *parent; > > > > > + struct disk_part *part; > > > > > + const struct blk_ops *ops; > > > > > + > > > > > + parent = dev_get_parent(dev); > > > > > + ops = blk_get_ops(parent); > > > > > + if (!ops->erase) > > > > > + return -ENOSYS; > > > > > + > > > > > + part = dev_get_uclass_plat(dev); > > > > > + start += part->gpt_part_info.start; > > > > > > > > here too > > > > > > > > Best regards > > > > > > > > Heinrich > > > > > > > > > + > > > > > + return ops->erase(parent, start, blkcnt); > > > > > +} > > > > > + > > > > > +static const struct blk_ops blk_part_ops = { > > > > > + .read = blk_part_read, > > > > > + .write = blk_part_write, > > > > > + .erase = blk_part_erase, > > > > > +}; > > > > > + > > > > > +U_BOOT_DRIVER(blk_partition) = { > > > > > + .name = "blk_partition", > > > > > + .id = UCLASS_PARTITION, > > > > > + .ops = &blk_part_ops, > > > > > +}; > > > > > + > > > > > +UCLASS_DRIVER(partition) = { > > > > > + .id = UCLASS_PARTITION, > > > > > + .per_device_plat_auto = sizeof(struct disk_part), > > > > > + .name = "partition", > > > > > +}; > > > > > diff --git a/include/blk.h b/include/blk.h > > > > > index 19bab081c2cd..3d883eb1db64 100644 > > > > > --- a/include/blk.h > > > > > +++ b/include/blk.h > > > > > @@ -366,6 +366,15 @@ int blk_create_devicef(struct udevice *parent, const char *drv_name, > > > > > const char *name, int if_type, int devnum, int blksz, > > > > > lbaint_t lba, struct udevice **devp); > > > > > > > > > > +/** > > > > > + * blk_create_partitions - Create block devices for disk partitions > > > > > + * > > > > > + * Create UCLASS_PARTITION udevices for each of disk partitions in @parent > > > > > + * > > > > > + * @parent: Whole disk device > > > > > + */ > > > > > +int blk_create_partitions(struct udevice *parent); > > > > > + > > > > > /** > > > > > * blk_unbind_all() - Unbind all device of the given interface type > > > > > * > > > > > diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h > > > > > index e7edd409f307..30892d01ce13 100644 > > > > > --- a/include/dm/uclass-id.h > > > > > +++ b/include/dm/uclass-id.h > > > > > @@ -80,6 +80,7 @@ enum uclass_id { > > > > > UCLASS_P2SB, /* (x86) Primary-to-Sideband Bus */ > > > > > UCLASS_PANEL, /* Display panel, such as an LCD */ > > > > > UCLASS_PANEL_BACKLIGHT, /* Backlight controller for panel */ > > > > > + UCLASS_PARTITION, /* Logical disk partition device */ > > > > > UCLASS_PCH, /* x86 platform controller hub */ > > > > > UCLASS_PCI, /* PCI bus */ > > > > > UCLASS_PCI_EP, /* PCI endpoint device */ > > > > >