On Tue, Jan 29, 2019 at 11:33:31PM +0100, Heinrich Schuchardt wrote: > On 1/29/19 3:59 AM, AKASHI Takahiro wrote: > > efi_disk_create() will initialize efi_disk attributes for each device, > > either UCLASS_BLK or UCLASS_PARTITION. > > > > Currently (temporarily), efi_disk_obj structure is embedded into > > blk_desc to hold efi-specific attributes. > > > > Signed-off-by: AKASHI Takahiro <takahiro.aka...@linaro.org> > > --- > > drivers/block/blk-uclass.c | 9 ++ > > drivers/core/device.c | 3 + > > include/blk.h | 24 +++++ > > include/dm/device.h | 4 + > > lib/efi_loader/efi_disk.c | 192 ++++++++++++++++++++++++++----------- > > 5 files changed, 174 insertions(+), 58 deletions(-) > > > > diff --git a/drivers/block/blk-uclass.c b/drivers/block/blk-uclass.c > > index d4ca30f23fc1..28c45d724113 100644 > > --- a/drivers/block/blk-uclass.c > > +++ b/drivers/block/blk-uclass.c > > @@ -657,6 +657,9 @@ UCLASS_DRIVER(blk) = { > > .per_device_platdata_auto_alloc_size = sizeof(struct blk_desc), > > }; > > > > +/* FIXME */ > > +extern int efi_disk_create(struct udevice *dev); > > + > > U_BOOT_DRIVER(blk_partition) = { > > .name = "blk_partition", > > .id = UCLASS_PARTITION, > > @@ -695,6 +698,12 @@ int blk_create_partitions(struct udevice *parent) > > part_data->partnum = part; > > part_data->gpt_part_info = info; > > > > + ret = efi_disk_create(dev); > > + if (ret) { > > + device_unbind(dev); > > + return ret; > > + } > > + > > disks++; > > } > > > > diff --git a/drivers/core/device.c b/drivers/core/device.c > > index 0d15e5062b66..8625fccb0dcb 100644 > > --- a/drivers/core/device.c > > +++ b/drivers/core/device.c > > @@ -67,6 +67,9 @@ static int device_bind_common(struct udevice *parent, > > const struct driver *drv, > > dev->parent = parent; > > dev->driver = drv; > > dev->uclass = uc; > > +#ifdef CONFIG_EFI_LOADER > > + INIT_LIST_HEAD(&dev->efi_obj.protocols); > > This looks like an incomplete initialization of efi_obj. Why don't we > use efi_create_handle.
I think that it is more or less a matter of implementation. I will address this issue later if necessary. > Why should a device be aware of struct efi_obj? We only need a handle to > install protocols. > > > +#endif > > > > dev->seq = -1; > > dev->req_seq = -1; > > diff --git a/include/blk.h b/include/blk.h > > index d0c033aece0f..405f6f790d66 100644 > > --- a/include/blk.h > > +++ b/include/blk.h > > @@ -8,6 +8,7 @@ > > #define BLK_H > > > > #include <efi.h> > > +#include <efi_api.h> > > > > #ifdef CONFIG_SYS_64BIT_LBA > > typedef uint64_t lbaint_t; > > @@ -53,6 +54,26 @@ enum sig_type { > > SIG_TYPE_COUNT /* Number of signature types */ > > }; > > > > +/* FIXME */ > > Fix what? For simplicity, this data structure was copied from efi_disk.c in this initial release. As the implementation goes *sophisticated*, some members may go away or move somewhere, say in blk_desc itself. > > +/** > > + * struct efi_disk_obj - EFI disk object > > + * > > + * @ops: EFI disk I/O protocol interface > > + * @media: block I/O media information > > + * @dp: device path to the block device > > + * @part: partition > > + * @volume: simple file system protocol of the partition > > + * @offset: offset into disk for simple partition > > + */ > > +struct efi_disk_obj { > > + struct efi_block_io ops; > > + struct efi_block_io_media media; > > + struct efi_device_path *dp; > > + unsigned int part; > > + struct efi_simple_file_system_protocol *volume; > > + lbaint_t offset; > > +}; > > + > > /* > > * With driver model (CONFIG_BLK) this is uclass platform data, accessible > > * with dev_get_uclass_platdata(dev) > > @@ -92,6 +113,9 @@ struct blk_desc { > > * device. Once these functions are removed we can drop this field. > > */ > > struct udevice *bdev; > > +#ifdef CONFIG_EFI_LOADER > > + struct efi_disk_obj efi_disk; > > This must be a handle (i.e. a pointer). Otherwise when the last protocol > is removed we try to free memory that was never malloc'ed. Who actually frees? > > +#endif > > #else > > unsigned long (*block_read)(struct blk_desc *block_dev, > > lbaint_t start, > > diff --git a/include/dm/device.h b/include/dm/device.h > > index 27a6d7b9fdb0..121bfb46b1a0 100644 > > --- a/include/dm/device.h > > +++ b/include/dm/device.h > > @@ -12,6 +12,7 @@ > > > > #include <dm/ofnode.h> > > #include <dm/uclass-id.h> > > +#include <efi_loader.h> > > #include <fdtdec.h> > > #include <linker_lists.h> > > #include <linux/compat.h> > > @@ -146,6 +147,9 @@ struct udevice { > > #ifdef CONFIG_DEVRES > > struct list_head devres_head; > > #endif > > +#ifdef CONFIG_EFI_LOADER > > + struct efi_object efi_obj; > > +#endif > > }; > > > > /* Maximum sequence number supported */ > > diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c > > index c037526ad2d0..84fa0ddfba14 100644 > > --- a/lib/efi_loader/efi_disk.c > > +++ b/lib/efi_loader/efi_disk.c > > @@ -14,33 +14,6 @@ > > > > const efi_guid_t efi_block_io_guid = BLOCK_IO_GUID; > > > > -/** > > - * struct efi_disk_obj - EFI disk object > > - * > > - * @header: EFI object header > > - * @ops: EFI disk I/O protocol interface > > - * @ifname: interface name for block device > > - * @dev_index: device index of block device > > - * @media: block I/O media information > > - * @dp: device path to the block device > > - * @part: partition > > - * @volume: simple file system protocol of the partition > > - * @offset: offset into disk for simple partition > > - * @desc: internal block device descriptor > > - */ > > -struct efi_disk_obj { > > - struct efi_object header; > > - struct efi_block_io ops; > > - const char *ifname; > > - int dev_index; > > - struct efi_block_io_media media; > > - struct efi_device_path *dp; > > - unsigned int part; > > - struct efi_simple_file_system_protocol *volume; > > - lbaint_t offset; > > - struct blk_desc *desc; > > -}; > > - > > static efi_status_t EFIAPI efi_disk_reset(struct efi_block_io *this, > > char extended_verification) > > { > > @@ -64,7 +37,7 @@ static efi_status_t efi_disk_rw_blocks(struct > > efi_block_io *this, > > unsigned long n; > > > > diskobj = container_of(this, struct efi_disk_obj, ops); > > - desc = (struct blk_desc *) diskobj->desc; > > + desc = container_of(diskobj, struct blk_desc, efi_disk); > > blksz = desc->blksz; > > blocks = buffer_size / blksz; > > lba += diskobj->offset; > > @@ -217,6 +190,7 @@ efi_fs_from_path(struct efi_device_path *full_path) > > return handler->protocol_interface; > > } > > > > +#ifndef CONFIG_BLK > > /* > > * Create a handle for a partition or disk > > * > > @@ -343,6 +317,136 @@ int efi_disk_create_partitions(efi_handle_t parent, > > struct blk_desc *desc, > > > > return disks; > > } > > +#else > > +static int efi_disk_create_common(efi_handle_t handle, > > + struct efi_disk_obj *disk, > > + struct blk_desc *desc) > > +{ > > + efi_status_t ret; > > + > > + /* Hook up to the device list */ > > + efi_add_handle(handle); > > + > > + /* Fill in EFI IO Media info (for read/write callbacks) */ > > + disk->media.removable_media = desc->removable; > > + disk->media.media_present = 1; > > + disk->media.block_size = desc->blksz; > > + disk->media.io_align = desc->blksz; > > + disk->media.last_block = desc->lba - disk->offset; > > + disk->ops.media = &disk->media; > > + > > + /* add protocols */ > > + disk->ops = block_io_disk_template; > > + ret = efi_add_protocol(handle, &efi_block_io_guid, &disk->ops); > > + if (ret != EFI_SUCCESS) > > + goto err; > > + > > + ret = efi_add_protocol(handle, &efi_guid_device_path, disk->dp); > > + if (ret != EFI_SUCCESS) > > + goto err; > > + > > + return 0; > > + > > +err: > > + /* FIXME: undo adding protocols */ > > + return -1; > > +} > > + > > +/* > > + * Create a handle for a raw disk > > + * > > + * @dev uclass device > > + * @return 0 on success, -1 otherwise > > + */ > > +int efi_disk_create_raw(struct udevice *dev) > > +{ > > + struct blk_desc *desc = dev_get_uclass_platdata(dev); > > + struct efi_disk_obj *disk = &desc->efi_disk; > > + > > + /* Don't add empty devices */ > > + if (!desc->lba) > > + return -1; > > + > > + /* raw block device */ > > + disk->offset = 0; > > + disk->part = 0; > > + disk->dp = efi_dp_from_part(desc, 0); > > + > > + /* efi IO media */ > > + disk->media.logical_partition = 0; > > + > > + return efi_disk_create_common(&dev->efi_obj, disk, desc); > > +} > > + > > +/* > > + * Create a handle for a partition > > + * > > + * @dev uclass device > > + * @return 0 on success, -1 otherwise > > + */ > > +int efi_disk_create_part(struct udevice *dev) > > +{ > > + struct udevice *parent = dev->parent; > > + struct blk_desc *desc = dev_get_uclass_platdata(parent); > > + struct blk_desc *this; > > + struct disk_part *pdata = dev_get_uclass_platdata(dev); > > + struct efi_disk_obj *disk; > > + struct efi_device_path *node; > > + > > + int ret; > > + > > + /* dummy block device */ > > + this = malloc(sizeof(*this)); > > + if (!this) > > + return -1; > > + disk = &this->efi_disk; > > + > > + /* logical disk partition */ > > + disk->offset = pdata->gpt_part_info.start; > > + disk->part = pdata->partnum; > > + > > + node = efi_dp_part_node(desc, disk->part); > > + disk->dp = efi_dp_append_node(desc->efi_disk.dp, node); > > + efi_free_pool(node); > > + > > + /* efi IO media */ > > + disk->media.logical_partition = 1; > > + > > + ret = efi_disk_create_common(&dev->efi_obj, disk, desc); > > + if (ret) > > + goto err; > > + > > + /* partition may support file system access */ > > + disk->volume = efi_simple_file_system(desc, disk->part, disk->dp); > > + ret = efi_add_protocol(&dev->efi_obj, > > + &efi_simple_file_system_protocol_guid, > > + disk->volume); > > + if (ret != EFI_SUCCESS) > > + goto err; > > + > > + return 0; > > + > > +err: > > + free(this); > > + /* FIXME: undo create */ > > + > > + return -1; > > +} > > + > > +int efi_disk_create(struct udevice *dev) > > +{ > > + enum uclass_id id; > > + > > + id = device_get_uclass_id(dev); > > + > > + if (id == UCLASS_BLK) > > + return efi_disk_create_raw(dev); > > + else if (id == UCLASS_PARTITION) > > + return efi_disk_create_part(dev); > > + else > > + return -1; > > +} > > +#endif /* CONFIG_BLK */ > > > > /* > > * U-Boot doesn't have a list of all online disk devices. So when running > > our > > @@ -357,38 +461,10 @@ int efi_disk_create_partitions(efi_handle_t parent, > > struct blk_desc *desc, > > */ > > efi_status_t efi_disk_register(void) > > { > > +#ifndef CONFIG_BLK > > struct efi_disk_obj *disk; > > int disks = 0; > > efi_status_t ret; > > -#ifdef CONFIG_BLK > > - struct udevice *dev; > > - > > - for (uclass_first_device_check(UCLASS_BLK, &dev); dev; > > - uclass_next_device_check(&dev)) { > > - struct blk_desc *desc = dev_get_uclass_platdata(dev); > > - const char *if_typename = blk_get_if_type_name(desc->if_type); > > - > > - /* Add block device for the full device */ > > - printf("Scanning disk %s...\n", dev->name); > > Who cares for this output? If really needed make it debug(). Please note that this is a line to be deleted. > > - ret = efi_disk_add_dev(NULL, NULL, if_typename, > > - desc, desc->devnum, 0, 0, &disk); > > - if (ret == EFI_NOT_READY) { > > - printf("Disk %s not ready\n", dev->name); > > Who minds if it is a CD-ROM drive with no disk inserted? Debug() might > be adequate here. Ditto > > - continue; > > - } > > - if (ret) { > > - printf("ERROR: failure to add disk device %s, r = > > %lu\n", > > - dev->name, ret & ~EFI_ERROR_MASK); > > - return ret; > > - } > > - disks++; > > - > > - /* Partitions show up as block devices in EFI */ > > - disks += efi_disk_create_partitions( > > - &disk->header, desc, if_typename, > > - desc->devnum, dev->name); > > - } > > -#else > > int i, if_type; > > > > /* Search for all available disk devices */ > > @@ -435,8 +511,8 @@ efi_status_t efi_disk_register(void) > > if_typename, i, devname); > > } > > } > > -#endif > > printf("Found %d disks\n", disks); > > I would prefer this to be debug() or eliminated. I didn't change anything on this line and the function, efi_disk_register(), will eventually go away. Thanks, -Takahiro Akashi > Best regards > > Heinrich > > > +#endif > > > > return EFI_SUCCESS; > > } > > > _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot