[U-Boot,RFC,3/3] drivers: align block device drivers with DM-efi integration

Efi_disk_create() should be hook up at every creation of block device
at each driver. Associated blk_desc must be properly set up before
calling this function.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 common/usb_storage.c              | 27 +++++++++++++++++++++++++--
 drivers/scsi/scsi.c               | 22 ++++++++++++++++++++++
 lib/efi_driver/efi_block_device.c | 30 +++++++++---------------------
 3 files changed, 56 insertions(+), 23 deletions(-)

On 01/29/2019 03:59 AM, AKASHI Takahiro wrote:
> Efi_disk_create() should be hook up at every creation of block device
> at each driver. Associated blk_desc must be properly set up before
> calling this function.
>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>   common/usb_storage.c              | 27 +++++++++++++++++++++++++--
>   drivers/scsi/scsi.c               | 22 ++++++++++++++++++++++
>   lib/efi_driver/efi_block_device.c | 30 +++++++++---------------------
>   3 files changed, 56 insertions(+), 23 deletions(-)
>
> diff --git a/common/usb_storage.c b/common/usb_storage.c
> index 8c889bb1a648..ff895c0e4557 100644
> --- a/common/usb_storage.c
> +++ b/common/usb_storage.c
> @@ -46,6 +46,10 @@
>   #include <part.h>
>   #include <usb.h>
>   
> +/* FIXME */
> +extern int efi_disk_create(struct udevice *dev);
> +extern int blk_create_partitions(struct udevice *parent);
> +
>   #undef BBB_COMDAT_TRACE
>   #undef BBB_XPORT_TRACE
>   
> @@ -227,8 +231,27 @@ static int usb_stor_probe_device(struct usb_device *udev)
>   
>   		ret = usb_stor_get_info(udev, data, blkdev);
>   		if (ret == 1) {
> -			usb_max_devs++;
> -			debug("%s: Found device %p\n", __func__, udev);
> +			ret = efi_disk_create(dev);
> +			if (ret) {
> +				debug("Cannot create efi_disk device\n");
> +				ret = device_unbind(dev);
> +				if (ret)
> +					return ret;
> +			} else {
> +				usb_max_devs++;
> +				ret = blk_create_partitions(dev);
> +				if (ret < 0) {
> +					debug("Cannot create disk partition device\n");
> +					/* TODO: undo create */
> +
> +					ret = device_unbind(dev);
> +					if (ret)
> +						return ret;
> +				}
> +				usb_max_devs += ret;
> +				debug("%s: Found device %p, partitions:%d\n",
> +				      __func__, udev, ret);
> +			}

Why do we need to do this in device specific code?

Alex


>   		} else {
>   			debug("usb_stor_get_info: Invalid device\n");
>   			ret = device_unbind(dev);
> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
> index df47e2fc78bd..f0f8cbc0bf26 100644
> --- a/drivers/scsi/scsi.c
> +++ b/drivers/scsi/scsi.c
> @@ -11,6 +11,10 @@
>   #include <dm/device-internal.h>
>   #include <dm/uclass-internal.h>
>   
> +/* FIXME */
> +int efi_disk_create(struct udevice *dev);
> +int blk_create_partitions(struct udevice *parent);
> +
>   #if !defined(CONFIG_DM_SCSI)
>   # ifdef CONFIG_SCSI_DEV_LIST
>   #  define SCSI_DEV_LIST CONFIG_SCSI_DEV_LIST
> @@ -593,9 +597,27 @@ static int do_scsi_scan_one(struct udevice *dev, int id, int lun, bool verbose)
>   	memcpy(&bdesc->product, &bd.product, sizeof(bd.product));
>   	memcpy(&bdesc->revision, &bd.revision,	sizeof(bd.revision));
>   
> +	ret = efi_disk_create(bdev);
> +	if (ret) {
> +		debug("Can't create efi_disk device\n");
> +		ret = device_unbind(bdev);
> +
> +		return ret;
> +	}
> +	ret = blk_create_partitions(bdev);
> +	if (ret < 0) {
> +		debug("Can't create efi_disk partitions\n");
> +		/* TODO: undo create */
> +
> +		ret = device_unbind(bdev);
> +
> +		return ret;
> +	}
> +
>   	if (verbose) {
>   		printf("  Device %d: ", 0);
>   		dev_print(bdesc);
> +		debug("Found %d partitions\n", ret);
>   	}
>   	return 0;
>   }
> diff --git a/lib/efi_driver/efi_block_device.c b/lib/efi_driver/efi_block_device.c
> index 3f147cf60879..4ab3402d6768 100644
> --- a/lib/efi_driver/efi_block_device.c
> +++ b/lib/efi_driver/efi_block_device.c
> @@ -32,6 +32,10 @@
>   #include <dm/device-internal.h>
>   #include <dm/root.h>
>   
> +/* FIXME */
> +extern int efi_disk_create(struct udevice *dev);
> +extern int blk_create_partitions(struct udevice *parent);
> +
>   /*
>    * EFI attributes of the udevice handled by this driver.
>    *
> @@ -102,24 +106,6 @@ static ulong efi_bl_write(struct udevice *dev, lbaint_t blknr, lbaint_t blkcnt,
>   	return blkcnt;
>   }
>   
> -/*
> - * Create partions for the block device.
> - *
> - * @handle	EFI handle of the block device
> - * @dev		udevice of the block device
> - */
> -static int efi_bl_bind_partitions(efi_handle_t handle, struct udevice *dev)
> -{
> -	struct blk_desc *desc;
> -	const char *if_typename;
> -
> -	desc = dev_get_uclass_platdata(dev);
> -	if_typename = blk_get_if_type_name(desc->if_type);
> -
> -	return efi_disk_create_partitions(handle, desc, if_typename,
> -					  desc->devnum, dev->name);
> -}
> -
>   /*
>    * Create a block device for a handle
>    *
> @@ -168,15 +154,17 @@ static int efi_bl_bind(efi_handle_t handle, void *interface)
>   	platdata->handle = handle;
>   	platdata->io = interface;
>   
> +	ret = efi_disk_create(bdev);
> +	if (ret)
> +		return ret;
> +
>   	ret = device_probe(bdev);
>   	if (ret)
>   		return ret;
>   	EFI_PRINT("%s: block device '%s' created\n", __func__, bdev->name);
> -
>   	/* Create handles for the partions of the block device */
> -	disks = efi_bl_bind_partitions(handle, bdev);
> +	disks = blk_create_partitions(bdev);
>   	EFI_PRINT("Found %d partitions\n", disks);
> -
>   	return 0;
>   }
>

Alex,

On Tue, Jan 29, 2019 at 05:19:38PM +0100, Alexander Graf wrote:
> On 01/29/2019 03:59 AM, AKASHI Takahiro wrote:
> >Efi_disk_create() should be hook up at every creation of block device
> >at each driver. Associated blk_desc must be properly set up before
> >calling this function.
> >
> >Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> >---
> >  common/usb_storage.c              | 27 +++++++++++++++++++++++++--
> >  drivers/scsi/scsi.c               | 22 ++++++++++++++++++++++
> >  lib/efi_driver/efi_block_device.c | 30 +++++++++---------------------
> >  3 files changed, 56 insertions(+), 23 deletions(-)
> >
> >diff --git a/common/usb_storage.c b/common/usb_storage.c
> >index 8c889bb1a648..ff895c0e4557 100644
> >--- a/common/usb_storage.c
> >+++ b/common/usb_storage.c
> >@@ -46,6 +46,10 @@
> >  #include <part.h>
> >  #include <usb.h>
> >+/* FIXME */
> >+extern int efi_disk_create(struct udevice *dev);
> >+extern int blk_create_partitions(struct udevice *parent);
> >+
> >  #undef BBB_COMDAT_TRACE
> >  #undef BBB_XPORT_TRACE
> >@@ -227,8 +231,27 @@ static int usb_stor_probe_device(struct usb_device *udev)
> >  		ret = usb_stor_get_info(udev, data, blkdev);
> >  		if (ret == 1) {
> >-			usb_max_devs++;
> >-			debug("%s: Found device %p\n", __func__, udev);
> >+			ret = efi_disk_create(dev);
> >+			if (ret) {
> >+				debug("Cannot create efi_disk device\n");
> >+				ret = device_unbind(dev);
> >+				if (ret)
> >+					return ret;
> >+			} else {
> >+				usb_max_devs++;
> >+				ret = blk_create_partitions(dev);
> >+				if (ret < 0) {
> >+					debug("Cannot create disk partition device\n");
> >+					/* TODO: undo create */
> >+
> >+					ret = device_unbind(dev);
> >+					if (ret)
> >+						return ret;
> >+				}
> >+				usb_max_devs += ret;
> >+				debug("%s: Found device %p, partitions:%d\n",
> >+				      __func__, udev, ret);
> >+			}
> 
> Why do we need to do this in device specific code?

Good point.

* efi_disk_create()
As I said in the past, efi_disk_create() will expect that blk_desc has
been initialized before it is called.
(There may be some possibility of removing this assumption.)

Blk_desc is often initialized after blk_create_device() in a driver.
So it won't be able to be placed in blk_create_device().

If, however, we have a "delayed" execution mechanism (like timer event as
you suggested before), we may put this function in a single point.

* blk_create_partions()
I initially thought of putting this function in part_init() which is
to be called at "probe," but was concerned that there might be a chance
that efi API be called before probing.
If this is not the case, we may also place this function in a single point.
(Unfortunately, "scsi rescan" won't kick off a probe hook though.)

Thanks,
-Takahiro Akashi

> Alex
> 
> 
> >  		} else {
> >  			debug("usb_stor_get_info: Invalid device\n");
> >  			ret = device_unbind(dev);
> >diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
> >index df47e2fc78bd..f0f8cbc0bf26 100644
> >--- a/drivers/scsi/scsi.c
> >+++ b/drivers/scsi/scsi.c
> >@@ -11,6 +11,10 @@
> >  #include <dm/device-internal.h>
> >  #include <dm/uclass-internal.h>
> >+/* FIXME */
> >+int efi_disk_create(struct udevice *dev);
> >+int blk_create_partitions(struct udevice *parent);
> >+
> >  #if !defined(CONFIG_DM_SCSI)
> >  # ifdef CONFIG_SCSI_DEV_LIST
> >  #  define SCSI_DEV_LIST CONFIG_SCSI_DEV_LIST
> >@@ -593,9 +597,27 @@ static int do_scsi_scan_one(struct udevice *dev, int id, int lun, bool verbose)
> >  	memcpy(&bdesc->product, &bd.product, sizeof(bd.product));
> >  	memcpy(&bdesc->revision, &bd.revision,	sizeof(bd.revision));
> >+	ret = efi_disk_create(bdev);
> >+	if (ret) {
> >+		debug("Can't create efi_disk device\n");
> >+		ret = device_unbind(bdev);
> >+
> >+		return ret;
> >+	}
> >+	ret = blk_create_partitions(bdev);
> >+	if (ret < 0) {
> >+		debug("Can't create efi_disk partitions\n");
> >+		/* TODO: undo create */
> >+
> >+		ret = device_unbind(bdev);
> >+
> >+		return ret;
> >+	}
> >+
> >  	if (verbose) {
> >  		printf("  Device %d: ", 0);
> >  		dev_print(bdesc);
> >+		debug("Found %d partitions\n", ret);
> >  	}
> >  	return 0;
> >  }
> >diff --git a/lib/efi_driver/efi_block_device.c b/lib/efi_driver/efi_block_device.c
> >index 3f147cf60879..4ab3402d6768 100644
> >--- a/lib/efi_driver/efi_block_device.c
> >+++ b/lib/efi_driver/efi_block_device.c
> >@@ -32,6 +32,10 @@
> >  #include <dm/device-internal.h>
> >  #include <dm/root.h>
> >+/* FIXME */
> >+extern int efi_disk_create(struct udevice *dev);
> >+extern int blk_create_partitions(struct udevice *parent);
> >+
> >  /*
> >   * EFI attributes of the udevice handled by this driver.
> >   *
> >@@ -102,24 +106,6 @@ static ulong efi_bl_write(struct udevice *dev, lbaint_t blknr, lbaint_t blkcnt,
> >  	return blkcnt;
> >  }
> >-/*
> >- * Create partions for the block device.
> >- *
> >- * @handle	EFI handle of the block device
> >- * @dev		udevice of the block device
> >- */
> >-static int efi_bl_bind_partitions(efi_handle_t handle, struct udevice *dev)
> >-{
> >-	struct blk_desc *desc;
> >-	const char *if_typename;
> >-
> >-	desc = dev_get_uclass_platdata(dev);
> >-	if_typename = blk_get_if_type_name(desc->if_type);
> >-
> >-	return efi_disk_create_partitions(handle, desc, if_typename,
> >-					  desc->devnum, dev->name);
> >-}
> >-
> >  /*
> >   * Create a block device for a handle
> >   *
> >@@ -168,15 +154,17 @@ static int efi_bl_bind(efi_handle_t handle, void *interface)
> >  	platdata->handle = handle;
> >  	platdata->io = interface;
> >+	ret = efi_disk_create(bdev);
> >+	if (ret)
> >+		return ret;
> >+
> >  	ret = device_probe(bdev);
> >  	if (ret)
> >  		return ret;
> >  	EFI_PRINT("%s: block device '%s' created\n", __func__, bdev->name);
> >-
> >  	/* Create handles for the partions of the block device */
> >-	disks = efi_bl_bind_partitions(handle, bdev);
> >+	disks = blk_create_partitions(bdev);
> >  	EFI_PRINT("Found %d partitions\n", disks);
> >-
> >  	return 0;
> >  }
> 
>