[PATCH 1/8] disk: Drop always true conditional check

[PATCH 1/8] disk: Drop always true conditional check

[Marek Vasut]

if (device_get_uclass_id(dev) == UCLASS_PARTITION) is always
true, because this disk_blk_read() function calls dev_get_blk()
above and checks its return value for non-NULL. The dev_get_blk()
performs the same device_get_uclass_id(dev) check and returns NULL
if not UCLASS_PARTITION. Drop the duplicate check.

Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
---
Cc: AKASHI Takahiro <takahiro.akashi@linaro.org>
Cc: Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>
Cc: Bin Meng <bmeng.cn@gmail.com>
Cc: Heinrich Schuchardt <xypron.glplk@gmx.de>
Cc: Joshua Watt <jpewhacker@gmail.com>
Cc: Michal Suchanek <msuchanek@suse.de>
Cc: Simon Glass <sjg@chromium.org>
Cc: Tobias Waldekranz <tobias@waldekranz.com>
---
 disk/disk-uclass.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/disk/disk-uclass.c b/disk/disk-uclass.c
index d32747e2242..5974dd8c2ec 100644
--- a/disk/disk-uclass.c
+++ b/disk/disk-uclass.c
@@ -186,10 +186,8 @@ unsigned long disk_blk_read(struct udevice *dev, lbaint_t start,
 		return -ENOSYS;
 
 	start_in_disk = start;
-	if (device_get_uclass_id(dev) == UCLASS_PARTITION) {
-		part = dev_get_uclass_plat(dev);
-		start_in_disk += part->gpt_part_info.start;
-	}
+	part = dev_get_uclass_plat(dev);
+	start_in_disk += part->gpt_part_info.start;
 
 	if (blkcache_read(desc->uclass_id, desc->devnum, start_in_disk, blkcnt,
 			  desc->blksz, buffer))
-- 
2.40.1

[PATCH 2/8] disk: Simplify disk_blk_read() using blk_read()

[Marek Vasut]

The disk_blk_read() can be simplified using blk_read(), the only
things which needs to be handled are the read offset based on the
partition properties, and the block device ops which are coming
from the parent udevice, not the partition udevice.

The later is currently not implemented correctly as far as I can
tell, since the current code extracts block device descriptor from
the parent udevice which is OK, but extracts block device operations
from the partition udevice, which does not seem OK.

Switching to the blk_read() fixes that too.

The dev_get_blk() usage is simplified using UCLASS_PARTITION check.

Add non-confusing documentation what this really does.

Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
---
Cc: AKASHI Takahiro <takahiro.akashi@linaro.org>
Cc: Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>
Cc: Bin Meng <bmeng.cn@gmail.com>
Cc: Heinrich Schuchardt <xypron.glplk@gmx.de>
Cc: Joshua Watt <jpewhacker@gmail.com>
Cc: Michal Suchanek <msuchanek@suse.de>
Cc: Simon Glass <sjg@chromium.org>
Cc: Tobias Waldekranz <tobias@waldekranz.com>
---
 disk/disk-uclass.c | 38 ++++++++++++++------------------------
 1 file changed, 14 insertions(+), 24 deletions(-)

diff --git a/disk/disk-uclass.c b/disk/disk-uclass.c
index 5974dd8c2ec..6daece1288f 100644
--- a/disk/disk-uclass.c
+++ b/disk/disk-uclass.c
@@ -168,36 +168,26 @@ static struct blk_desc *dev_get_blk(struct udevice *dev)
 	return desc;
 }
 
+/**
+ * disk_blk_read() - Read from a block device partition
+ *
+ * @dev: Device to read from (partition udevice)
+ * @start: Start block for the read (from start of partition)
+ * @blkcnt: Number of blocks to read (within the partition)
+ * @buffer: Place to put the data
+ * @return number of blocks read (which may be less than @blkcnt),
+ * or -ve on error. This never returns 0 unless @blkcnt is 0
+ */
 unsigned long disk_blk_read(struct udevice *dev, lbaint_t start,
 			    lbaint_t blkcnt, void *buffer)
 {
-	struct blk_desc *desc;
-	const struct blk_ops *ops;
-	struct disk_part *part;
-	lbaint_t start_in_disk;
-	ulong blks_read;
-
-	desc = dev_get_blk(dev);
-	if (!desc)
-		return -ENOSYS;
+	struct disk_part *part = dev_get_uclass_plat(dev);
 
-	ops = blk_get_ops(dev);
-	if (!ops->read)
+	if (device_get_uclass_id(dev) != UCLASS_PARTITION)
 		return -ENOSYS;
 
-	start_in_disk = start;
-	part = dev_get_uclass_plat(dev);
-	start_in_disk += part->gpt_part_info.start;
-
-	if (blkcache_read(desc->uclass_id, desc->devnum, start_in_disk, blkcnt,
-			  desc->blksz, buffer))
-		return blkcnt;
-	blks_read = ops->read(dev, start, blkcnt, buffer);
-	if (blks_read == blkcnt)
-		blkcache_fill(desc->uclass_id, desc->devnum, start_in_disk,
-			      blkcnt, desc->blksz, buffer);
-
-	return blks_read;
+	return blk_read(dev_get_parent(dev), start + part->gpt_part_info.start,
+			blkcnt, buffer);
 }
 
 unsigned long disk_blk_write(struct udevice *dev, lbaint_t start,
-- 
2.40.1

Re: [PATCH 2/8] disk: Simplify disk_blk_read() using blk_read()

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 895 bytes --]

On Mon, Aug 14, 2023 at 01:46:42AM +0200, Marek Vasut wrote:

> The disk_blk_read() can be simplified using blk_read(), the only
> things which needs to be handled are the read offset based on the
> partition properties, and the block device ops which are coming
> from the parent udevice, not the partition udevice.
> 
> The later is currently not implemented correctly as far as I can
> tell, since the current code extracts block device descriptor from
> the parent udevice which is OK, but extracts block device operations
> from the partition udevice, which does not seem OK.
> 
> Switching to the blk_read() fixes that too.
> 
> The dev_get_blk() usage is simplified using UCLASS_PARTITION check.
> 
> Add non-confusing documentation what this really does.
> 
> Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>

Applied to u-boot/next, thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

[PATCH 3/8] disk: Simplify disk_blk_{write, erase}() using blk_{write, erase}()

[Marek Vasut]

These two functions are basically identical, just call the blk_*()
functions from disk_blk_*() functions. The only difference is that
the disk_blk_*() functions have to use parent block device as the
udevice implementing block device operations.

Add documentation on what those functions really do. The documentation
is not wrong even though it likely does look that way. The write/erase
functions really do not take into account the partition offset. This
will be fixed in the next patch.

Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
---
Cc: AKASHI Takahiro <takahiro.akashi@linaro.org>
Cc: Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>
Cc: Bin Meng <bmeng.cn@gmail.com>
Cc: Heinrich Schuchardt <xypron.glplk@gmx.de>
Cc: Joshua Watt <jpewhacker@gmail.com>
Cc: Michal Suchanek <msuchanek@suse.de>
Cc: Simon Glass <sjg@chromium.org>
Cc: Tobias Waldekranz <tobias@waldekranz.com>
---
 disk/disk-uclass.c | 66 ++++++++++++++++------------------------------
 1 file changed, 23 insertions(+), 43 deletions(-)

diff --git a/disk/disk-uclass.c b/disk/disk-uclass.c
index 6daece1288f..5cb1594e015 100644
--- a/disk/disk-uclass.c
+++ b/disk/disk-uclass.c
@@ -149,25 +149,6 @@ U_BOOT_DRIVER(blk_partition) = {
 /*
  * BLOCK IO APIs
  */
-static struct blk_desc *dev_get_blk(struct udevice *dev)
-{
-	struct blk_desc *desc;
-
-	switch (device_get_uclass_id(dev)) {
-	/*
-	 * We won't support UCLASS_BLK with dev_* interfaces.
-	 */
-	case UCLASS_PARTITION:
-		desc = dev_get_uclass_plat(dev_get_parent(dev));
-		break;
-	default:
-		desc = NULL;
-		break;
-	}
-
-	return desc;
-}
-
 /**
  * disk_blk_read() - Read from a block device partition
  *
@@ -190,42 +171,41 @@ unsigned long disk_blk_read(struct udevice *dev, lbaint_t start,
 			blkcnt, buffer);
 }
 
+/**
+ * disk_blk_write() - Write to a block device
+ *
+ * @dev: Device to write to
+ * @start: Start block for the write
+ * @blkcnt: Number of blocks to write
+ * @buffer: Data to write
+ * @return number of blocks written (which may be less than @blkcnt),
+ * or -ve on error. This never returns 0 unless @blkcnt is 0
+ */
 unsigned long disk_blk_write(struct udevice *dev, lbaint_t start,
 			     lbaint_t blkcnt, const void *buffer)
 {
-	struct blk_desc *desc;
-	const struct blk_ops *ops;
-
-	desc = dev_get_blk(dev);
-	if (!desc)
-		return -ENOSYS;
-
-	ops = blk_get_ops(dev);
-	if (!ops->write)
+	if (device_get_uclass_id(dev) != UCLASS_PARTITION)
 		return -ENOSYS;
 
-	blkcache_invalidate(desc->uclass_id, desc->devnum);
-
-	return ops->write(dev, start, blkcnt, buffer);
+	return blk_write(dev_get_parent(dev), start, blkcnt, buffer);
 }
 
+/**
+ * disk_blk_erase() - Erase part of a block device
+ *
+ * @dev: Device to erase
+ * @start: Start block for the erase
+ * @blkcnt: Number of blocks to erase
+ * @return number of blocks erased (which may be less than @blkcnt),
+ * or -ve on error. This never returns 0 unless @blkcnt is 0
+ */
 unsigned long disk_blk_erase(struct udevice *dev, lbaint_t start,
 			     lbaint_t blkcnt)
 {
-	struct blk_desc *desc;
-	const struct blk_ops *ops;
-
-	desc = dev_get_blk(dev);
-	if (!desc)
-		return -ENOSYS;
-
-	ops = blk_get_ops(dev);
-	if (!ops->erase)
+	if (device_get_uclass_id(dev) != UCLASS_PARTITION)
 		return -ENOSYS;
 
-	blkcache_invalidate(desc->uclass_id, desc->devnum);
-
-	return ops->erase(dev, start, blkcnt);
+	return blk_erase(dev_get_parent(dev), start, blkcnt);
 }
 
 UCLASS_DRIVER(partition) = {
-- 
2.40.1

Re: [PATCH 3/8] disk: Simplify disk_blk_{write, erase}() using blk_{write, erase}()

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 697 bytes --]

On Mon, Aug 14, 2023 at 01:46:43AM +0200, Marek Vasut wrote:

> These two functions are basically identical, just call the blk_*()
> functions from disk_blk_*() functions. The only difference is that
> the disk_blk_*() functions have to use parent block device as the
> udevice implementing block device operations.
> 
> Add documentation on what those functions really do. The documentation
> is not wrong even though it likely does look that way. The write/erase
> functions really do not take into account the partition offset. This
> will be fixed in the next patch.
> 
> Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>

Applied to u-boot/next, thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

[PATCH 4/8] disk: Handle partition to block device offset conversion

[Marek Vasut]

Convert the read/write/erase offset from one within a partition
to one within a block device, to correctly access the data on
the block device for both write and erase operations.

Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
---
Cc: AKASHI Takahiro <takahiro.akashi@linaro.org>
Cc: Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>
Cc: Bin Meng <bmeng.cn@gmail.com>
Cc: Heinrich Schuchardt <xypron.glplk@gmx.de>
Cc: Joshua Watt <jpewhacker@gmail.com>
Cc: Michal Suchanek <msuchanek@suse.de>
Cc: Simon Glass <sjg@chromium.org>
Cc: Tobias Waldekranz <tobias@waldekranz.com>
---
 disk/disk-uclass.c | 68 +++++++++++++++++++++++++++++++++++-----------
 1 file changed, 52 insertions(+), 16 deletions(-)

diff --git a/disk/disk-uclass.c b/disk/disk-uclass.c
index 5cb1594e015..32722cf9176 100644
--- a/disk/disk-uclass.c
+++ b/disk/disk-uclass.c
@@ -17,6 +17,36 @@
 #include <dm/device-internal.h>
 #include <dm/lists.h>
 
+/**
+ * disk_blk_part_validate() - Check whether access to partition is within limits
+ *
+ * @dev: Device (partition udevice)
+ * @start: Start block for the access(from start of partition)
+ * @blkcnt: Number of blocks to access (within the partition)
+ * @return 0 on valid block range, or -ve on error.
+ */
+static int disk_blk_part_validate(struct udevice *dev, lbaint_t start, lbaint_t blkcnt)
+{
+	if (device_get_uclass_id(dev) != UCLASS_PARTITION)
+		return -ENOSYS;
+
+	return 0;
+}
+
+/**
+ * disk_blk_part_offset() - Compute offset from start of block device
+ *
+ * @dev: Device (partition udevice)
+ * @start: Start block for the access (from start of partition)
+ * @return Start block for the access (from start of block device)
+ */
+static lbaint_t disk_blk_part_offset(struct udevice *dev, lbaint_t start)
+{
+	struct disk_part *part = dev_get_uclass_plat(dev);
+
+	return start + part->gpt_part_info.start;
+}
+
 int part_create_block_devices(struct udevice *blk_dev)
 {
 	int part, count;
@@ -162,21 +192,21 @@ U_BOOT_DRIVER(blk_partition) = {
 unsigned long disk_blk_read(struct udevice *dev, lbaint_t start,
 			    lbaint_t blkcnt, void *buffer)
 {
-	struct disk_part *part = dev_get_uclass_plat(dev);
+	int ret = disk_blk_part_validate(dev, start, blkcnt);
 
-	if (device_get_uclass_id(dev) != UCLASS_PARTITION)
-		return -ENOSYS;
+	if (ret)
+		return ret;
 
-	return blk_read(dev_get_parent(dev), start + part->gpt_part_info.start,
+	return blk_read(dev_get_parent(dev), disk_blk_part_offset(dev, start),
 			blkcnt, buffer);
 }
 
 /**
  * disk_blk_write() - Write to a block device
  *
- * @dev: Device to write to
- * @start: Start block for the write
- * @blkcnt: Number of blocks to write
+ * @dev: Device to write to (partition udevice)
+ * @start: Start block for the write (from start of partition)
+ * @blkcnt: Number of blocks to write (within the partition)
  * @buffer: Data to write
  * @return number of blocks written (which may be less than @blkcnt),
  * or -ve on error. This never returns 0 unless @blkcnt is 0
@@ -184,28 +214,34 @@ unsigned long disk_blk_read(struct udevice *dev, lbaint_t start,
 unsigned long disk_blk_write(struct udevice *dev, lbaint_t start,
 			     lbaint_t blkcnt, const void *buffer)
 {
-	if (device_get_uclass_id(dev) != UCLASS_PARTITION)
-		return -ENOSYS;
+	int ret = disk_blk_part_validate(dev, start, blkcnt);
+
+	if (ret)
+		return ret;
 
-	return blk_write(dev_get_parent(dev), start, blkcnt, buffer);
+	return blk_write(dev_get_parent(dev), disk_blk_part_offset(dev, start),
+			 blkcnt, buffer);
 }
 
 /**
  * disk_blk_erase() - Erase part of a block device
  *
- * @dev: Device to erase
- * @start: Start block for the erase
- * @blkcnt: Number of blocks to erase
+ * @dev: Device to erase (partition udevice)
+ * @start: Start block for the erase (from start of partition)
+ * @blkcnt: Number of blocks to erase (within the partition)
  * @return number of blocks erased (which may be less than @blkcnt),
  * or -ve on error. This never returns 0 unless @blkcnt is 0
  */
 unsigned long disk_blk_erase(struct udevice *dev, lbaint_t start,
 			     lbaint_t blkcnt)
 {
-	if (device_get_uclass_id(dev) != UCLASS_PARTITION)
-		return -ENOSYS;
+	int ret = disk_blk_part_validate(dev, start, blkcnt);
+
+	if (ret)
+		return ret;
 
-	return blk_erase(dev_get_parent(dev), start, blkcnt);
+	return blk_erase(dev_get_parent(dev), disk_blk_part_offset(dev, start),
+			 blkcnt);
 }
 
 UCLASS_DRIVER(partition) = {
-- 
2.40.1

Re: [PATCH 4/8] disk: Handle partition to block device offset conversion

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 368 bytes --]

On Mon, Aug 14, 2023 at 01:46:44AM +0200, Marek Vasut wrote:

> Convert the read/write/erase offset from one within a partition
> to one within a block device, to correctly access the data on
> the block device for both write and erase operations.
> 
> Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>

Applied to u-boot/next, thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

[PATCH 5/8] disk: Extend disk_blk_part_validate() with range checking

[Marek Vasut]

Check whether access is out of bounds of the partition and
return an error. This way there is no danger of esp. write
or erase outside of the confines of partition.

Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
---
Cc: AKASHI Takahiro <takahiro.akashi@linaro.org>
Cc: Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>
Cc: Bin Meng <bmeng.cn@gmail.com>
Cc: Heinrich Schuchardt <xypron.glplk@gmx.de>
Cc: Joshua Watt <jpewhacker@gmail.com>
Cc: Michal Suchanek <msuchanek@suse.de>
Cc: Simon Glass <sjg@chromium.org>
Cc: Tobias Waldekranz <tobias@waldekranz.com>
---
 disk/disk-uclass.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/disk/disk-uclass.c b/disk/disk-uclass.c
index 32722cf9176..f262105375b 100644
--- a/disk/disk-uclass.c
+++ b/disk/disk-uclass.c
@@ -27,9 +27,17 @@
  */
 static int disk_blk_part_validate(struct udevice *dev, lbaint_t start, lbaint_t blkcnt)
 {
+	struct disk_part *part = dev_get_uclass_plat(dev);
+
 	if (device_get_uclass_id(dev) != UCLASS_PARTITION)
 		return -ENOSYS;
 
+	if (start >= part->gpt_part_info.size)
+		return -E2BIG;
+
+	if ((start + blkcnt) > part->gpt_part_info.size)
+		return -ERANGE;
+
 	return 0;
 }
 
-- 
2.40.1

Re: [PATCH 5/8] disk: Extend disk_blk_part_validate() with range checking

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 353 bytes --]

On Mon, Aug 14, 2023 at 01:46:45AM +0200, Marek Vasut wrote:

> Check whether access is out of bounds of the partition and
> return an error. This way there is no danger of esp. write
> or erase outside of the confines of partition.
> 
> Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>

Applied to u-boot/next, thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

[PATCH 6/8] disk: Switch part_blk_*() functions to disk_blk_*()

[Marek Vasut]

The behavior of the part_blk_*() functions is now identical
to disk_blk_*() functions, switch the former to the later.

Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
---
Cc: AKASHI Takahiro <takahiro.akashi@linaro.org>
Cc: Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>
Cc: Bin Meng <bmeng.cn@gmail.com>
Cc: Heinrich Schuchardt <xypron.glplk@gmx.de>
Cc: Joshua Watt <jpewhacker@gmail.com>
Cc: Michal Suchanek <msuchanek@suse.de>
Cc: Simon Glass <sjg@chromium.org>
Cc: Tobias Waldekranz <tobias@waldekranz.com>
---
 disk/disk-uclass.c | 93 ++++++----------------------------------------
 1 file changed, 12 insertions(+), 81 deletions(-)

diff --git a/disk/disk-uclass.c b/disk/disk-uclass.c
index f262105375b..90a7c6f0f8a 100644
--- a/disk/disk-uclass.c
+++ b/disk/disk-uclass.c
@@ -103,87 +103,6 @@ int part_create_block_devices(struct udevice *blk_dev)
 	return 0;
 }
 
-static ulong part_blk_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);
-	ops = blk_get_ops(parent);
-	if (!ops->read)
-		return -ENOSYS;
-
-	part = dev_get_uclass_plat(dev);
-	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;
-
-	return ops->read(parent, start, blkcnt, buffer);
-}
-
-static ulong part_blk_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);
-	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;
-
-	return ops->write(parent, start, blkcnt, buffer);
-}
-
-static ulong part_blk_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);
-	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;
-
-	return ops->erase(parent, start, blkcnt);
-}
-
-static const struct blk_ops blk_part_ops = {
-	.read	= part_blk_read,
-	.write	= part_blk_write,
-	.erase	= part_blk_erase,
-};
-
-U_BOOT_DRIVER(blk_partition) = {
-	.name		= "blk_partition",
-	.id		= UCLASS_PARTITION,
-	.ops		= &blk_part_ops,
-};
-
 /*
  * BLOCK IO APIs
  */
@@ -257,3 +176,15 @@ UCLASS_DRIVER(partition) = {
 	.per_device_plat_auto	= sizeof(struct disk_part),
 	.name		= "partition",
 };
+
+static const struct blk_ops blk_part_ops = {
+	.read	= disk_blk_read,
+	.write	= disk_blk_write,
+	.erase	= disk_blk_erase,
+};
+
+U_BOOT_DRIVER(blk_partition) = {
+	.name		= "blk_partition",
+	.id		= UCLASS_PARTITION,
+	.ops		= &blk_part_ops,
+};
-- 
2.40.1

Re: [PATCH 6/8] disk: Switch part_blk_*() functions to disk_blk_*()

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 304 bytes --]

On Mon, Aug 14, 2023 at 01:46:46AM +0200, Marek Vasut wrote:

> The behavior of the part_blk_*() functions is now identical
> to disk_blk_*() functions, switch the former to the later.
> 
> Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>

Applied to u-boot/next, thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

[PATCH 7/8] disk: Move part_create_block_devices() to blk uclass

[Marek Vasut]

Move part_create_block_devices() to blk uclass and unexpose
the function. This can now be internal to the block uclass.

Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
---
Cc: AKASHI Takahiro <takahiro.akashi@linaro.org>
Cc: Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>
Cc: Bin Meng <bmeng.cn@gmail.com>
Cc: Heinrich Schuchardt <xypron.glplk@gmx.de>
Cc: Joshua Watt <jpewhacker@gmail.com>
Cc: Michal Suchanek <msuchanek@suse.de>
Cc: Simon Glass <sjg@chromium.org>
Cc: Tobias Waldekranz <tobias@waldekranz.com>
---
 disk/disk-uclass.c         | 48 --------------------------------------
 drivers/block/blk-uclass.c | 48 ++++++++++++++++++++++++++++++++++++++
 include/part.h             |  9 -------
 3 files changed, 48 insertions(+), 57 deletions(-)

diff --git a/disk/disk-uclass.c b/disk/disk-uclass.c
index 90a7c6f0f8a..efe4bf1f949 100644
--- a/disk/disk-uclass.c
+++ b/disk/disk-uclass.c
@@ -55,54 +55,6 @@ static lbaint_t disk_blk_part_offset(struct udevice *dev, lbaint_t start)
 	return start + part->gpt_part_info.start;
 }
 
-int part_create_block_devices(struct udevice *blk_dev)
-{
-	int part, count;
-	struct blk_desc *desc = dev_get_uclass_plat(blk_dev);
-	struct disk_partition info;
-	struct disk_part *part_data;
-	char devname[32];
-	struct udevice *dev;
-	int ret;
-
-	if (!CONFIG_IS_ENABLED(PARTITIONS) || !blk_enabled())
-		return 0;
-
-	if (device_get_uclass_id(blk_dev) != UCLASS_BLK)
-		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", blk_dev->name,
-			 part);
-
-		ret = device_bind_driver(blk_dev, "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++;
-
-		ret = device_probe(dev);
-		if (ret) {
-			debug("Can't probe\n");
-			count--;
-			device_unbind(dev);
-
-			continue;
-		}
-	}
-	debug("%s: %d partitions found in %s\n", __func__, count,
-	      blk_dev->name);
-
-	return 0;
-}
-
 /*
  * BLOCK IO APIs
  */
diff --git a/drivers/block/blk-uclass.c b/drivers/block/blk-uclass.c
index 614b975e25c..9521b3eb878 100644
--- a/drivers/block/blk-uclass.c
+++ b/drivers/block/blk-uclass.c
@@ -766,6 +766,54 @@ int blk_unbind_all(int uclass_id)
 	return 0;
 }
 
+static int part_create_block_devices(struct udevice *blk_dev)
+{
+	int part, count;
+	struct blk_desc *desc = dev_get_uclass_plat(blk_dev);
+	struct disk_partition info;
+	struct disk_part *part_data;
+	char devname[32];
+	struct udevice *dev;
+	int ret;
+
+	if (!CONFIG_IS_ENABLED(PARTITIONS) || !blk_enabled())
+		return 0;
+
+	if (device_get_uclass_id(blk_dev) != UCLASS_BLK)
+		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", blk_dev->name,
+			 part);
+
+		ret = device_bind_driver(blk_dev, "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++;
+
+		ret = device_probe(dev);
+		if (ret) {
+			debug("Can't probe\n");
+			count--;
+			device_unbind(dev);
+
+			continue;
+		}
+	}
+	debug("%s: %d partitions found in %s\n", __func__, count,
+	      blk_dev->name);
+
+	return 0;
+}
+
 static int blk_post_probe(struct udevice *dev)
 {
 	if (CONFIG_IS_ENABLED(PARTITIONS) && blk_enabled()) {
diff --git a/include/part.h b/include/part.h
index edc46f8dcbe..74e4d42263e 100644
--- a/include/part.h
+++ b/include/part.h
@@ -306,15 +306,6 @@ part_get_info_by_dev_and_name_or_num(const char *dev_iface,
 int part_get_bootable(struct blk_desc *desc);
 
 struct udevice;
-/**
- * part_create_block_devices - Create block devices for disk partitions
- *
- * Create UCLASS_PARTITION udevices for each of disk partitions in @parent
- *
- * @blk_dev:	Whole disk device
- */
-int part_create_block_devices(struct udevice *blk_dev);
-
 /**
  * disk_blk_read() - read blocks from a disk partition
  *
-- 
2.40.1

Re: [PATCH 7/8] disk: Move part_create_block_devices() to blk uclass

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 305 bytes --]

On Mon, Aug 14, 2023 at 01:46:47AM +0200, Marek Vasut wrote:

> Move part_create_block_devices() to blk uclass and unexpose
> the function. This can now be internal to the block uclass.
> 
> Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>

Applied to u-boot/next, thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

[PATCH 8/8] disk: Make blk_get_ops() internal to blk uclass

[Marek Vasut]

Move the macro into blk-uclass.c , since it is only used there.

Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
---
Cc: AKASHI Takahiro <takahiro.akashi@linaro.org>
Cc: Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>
Cc: Bin Meng <bmeng.cn@gmail.com>
Cc: Heinrich Schuchardt <xypron.glplk@gmx.de>
Cc: Joshua Watt <jpewhacker@gmail.com>
Cc: Michal Suchanek <msuchanek@suse.de>
Cc: Simon Glass <sjg@chromium.org>
Cc: Tobias Waldekranz <tobias@waldekranz.com>
---
 drivers/block/blk-uclass.c | 2 ++
 include/blk.h              | 2 --
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/block/blk-uclass.c b/drivers/block/blk-uclass.c
index 9521b3eb878..6aac92d9962 100644
--- a/drivers/block/blk-uclass.c
+++ b/drivers/block/blk-uclass.c
@@ -17,6 +17,8 @@
 #include <dm/uclass-internal.h>
 #include <linux/err.h>
 
+#define blk_get_ops(dev)	((struct blk_ops *)(dev)->driver->ops)
+
 static struct {
 	enum uclass_id id;
 	const char *name;
diff --git a/include/blk.h b/include/blk.h
index 2c9c7985a88..8986e953e5a 100644
--- a/include/blk.h
+++ b/include/blk.h
@@ -262,8 +262,6 @@ struct blk_ops {
 	int (*select_hwpart)(struct udevice *dev, int hwpart);
 };
 
-#define blk_get_ops(dev)	((struct blk_ops *)(dev)->driver->ops)
-
 /*
  * These functions should take struct udevice instead of struct blk_desc,
  * but this is convenient for migration to driver model. Add a 'd' prefix
-- 
2.40.1

Re: [PATCH 8/8] disk: Make blk_get_ops() internal to blk uclass

[Simon Glass]

On Sun, 13 Aug 2023 at 17:47, Marek Vasut
<marek.vasut+renesas@mailbox.org> wrote:
>
> Move the macro into blk-uclass.c , since it is only used there.
>
> Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
> ---
> Cc: AKASHI Takahiro <takahiro.akashi@linaro.org>
> Cc: Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>
> Cc: Bin Meng <bmeng.cn@gmail.com>
> Cc: Heinrich Schuchardt <xypron.glplk@gmx.de>
> Cc: Joshua Watt <jpewhacker@gmail.com>
> Cc: Michal Suchanek <msuchanek@suse.de>
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Tobias Waldekranz <tobias@waldekranz.com>
> ---
>  drivers/block/blk-uclass.c | 2 ++
>  include/blk.h              | 2 --
>  2 files changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

Unfortunately this does not stop people using the ops member directly.

For this series, I tried a patch myself [1] but I think I stuffed it
up. So I will let Takahiro-San figure it out. I would very much like
to see this clean-up go in.

Regards,
Simon

[1] https://patchwork.ozlabs.org/project/uboot/patch/20230319192957.1084530-1-sjg@chromium.org/

Re: [PATCH 8/8] disk: Make blk_get_ops() internal to blk uclass

[AKASHI Takahiro]

On Mon, Aug 14, 2023 at 04:42:57PM -0600, Simon Glass wrote:
> On Sun, 13 Aug 2023 at 17:47, Marek Vasut
> <marek.vasut+renesas@mailbox.org> wrote:
> >
> > Move the macro into blk-uclass.c , since it is only used there.
> >
> > Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
> > ---
> > Cc: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > Cc: Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>
> > Cc: Bin Meng <bmeng.cn@gmail.com>
> > Cc: Heinrich Schuchardt <xypron.glplk@gmx.de>
> > Cc: Joshua Watt <jpewhacker@gmail.com>
> > Cc: Michal Suchanek <msuchanek@suse.de>
> > Cc: Simon Glass <sjg@chromium.org>
> > Cc: Tobias Waldekranz <tobias@waldekranz.com>
> > ---
> >  drivers/block/blk-uclass.c | 2 ++
> >  include/blk.h              | 2 --
> >  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> Reviewed-by: Simon Glass <sjg@chromium.org>
> 
> Unfortunately this does not stop people using the ops member directly.
> 
> For this series, I tried a patch myself [1] but I think I stuffed it
> up. So I will let Takahiro-San figure it out. I would very much like
> to see this clean-up go in.

At that time I thought that the necessary change was small and trivial:)

As for Marek's patch, let me first explain why I implement that way,
i.e. separating disk_blk_*() from part_disk_*():
- Initially I tried to implement disk_blk_*() work for both UCLASS_BLOCK
  and UCLASS_PARTITION, while this idea was rejected by Simon.
- Then, I implemented part_disk_*() with direct access to the devices,
  and part_disk_*(), as helper functions, with block caching.
  I thought that this approach was aligned with the implementation of
  block devices (blk_[read|wirte]).

If you don't think the second point makes sense, I can agree to Marek's
approach.

Thanks,
-Takahiro Akashi


> Regards,
> Simon
> 
> [1] https://patchwork.ozlabs.org/project/uboot/patch/20230319192957.1084530-1-sjg@chromium.org/

Re: [PATCH 8/8] disk: Make blk_get_ops() internal to blk uclass

[Marek Vasut]

On 8/15/23 02:31, AKASHI Takahiro wrote:
> On Mon, Aug 14, 2023 at 04:42:57PM -0600, Simon Glass wrote:
>> On Sun, 13 Aug 2023 at 17:47, Marek Vasut
>> <marek.vasut+renesas@mailbox.org> wrote:
>>>
>>> Move the macro into blk-uclass.c , since it is only used there.
>>>
>>> Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
>>> ---
>>> Cc: AKASHI Takahiro <takahiro.akashi@linaro.org>
>>> Cc: Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>
>>> Cc: Bin Meng <bmeng.cn@gmail.com>
>>> Cc: Heinrich Schuchardt <xypron.glplk@gmx.de>
>>> Cc: Joshua Watt <jpewhacker@gmail.com>
>>> Cc: Michal Suchanek <msuchanek@suse.de>
>>> Cc: Simon Glass <sjg@chromium.org>
>>> Cc: Tobias Waldekranz <tobias@waldekranz.com>
>>> ---
>>>   drivers/block/blk-uclass.c | 2 ++
>>>   include/blk.h              | 2 --
>>>   2 files changed, 2 insertions(+), 2 deletions(-)
>>
>> Reviewed-by: Simon Glass <sjg@chromium.org>
>>
>> Unfortunately this does not stop people using the ops member directly.
>>
>> For this series, I tried a patch myself [1] but I think I stuffed it
>> up. So I will let Takahiro-San figure it out. I would very much like
>> to see this clean-up go in.
> 
> At that time I thought that the necessary change was small and trivial:)
> 
> As for Marek's patch, let me first explain why I implement that way,
> i.e. separating disk_blk_*() from part_disk_*():
> - Initially I tried to implement disk_blk_*() work for both UCLASS_BLOCK
>    and UCLASS_PARTITION, while this idea was rejected by Simon.
> - Then, I implemented part_disk_*() with direct access to the devices,
>    and part_disk_*(), as helper functions, with block caching.

I think the later should not be part_disk_*(), but rather disk_blk_*() ?

>    I thought that this approach was aligned with the implementation of
>    block devices (blk_[read|wirte]).
> 
> If you don't think the second point makes sense, I can agree to Marek's
> approach.

Does it even make sense to have accessors which bypass the block cache ?

-- 
Best regards,
Marek Vasut

Re: [PATCH 8/8] disk: Make blk_get_ops() internal to blk uclass

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 293 bytes --]

On Mon, Aug 14, 2023 at 01:46:48AM +0200, Marek Vasut wrote:

> Move the macro into blk-uclass.c , since it is only used there.
> 
> Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
> Reviewed-by: Simon Glass <sjg@chromium.org>

Applied to u-boot/next, thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 1/8] disk: Drop always true conditional check

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 503 bytes --]

On Mon, Aug 14, 2023 at 01:46:41AM +0200, Marek Vasut wrote:

> if (device_get_uclass_id(dev) == UCLASS_PARTITION) is always
> true, because this disk_blk_read() function calls dev_get_blk()
> above and checks its return value for non-NULL. The dev_get_blk()
> performs the same device_get_uclass_id(dev) check and returns NULL
> if not UCLASS_PARTITION. Drop the duplicate check.
> 
> Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>

Applied to u-boot/next, thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]