Hi Tobias, On Fri, 3 Feb 2023 at 02:38, Tobias Waldekranz <tob...@waldekranz.com> wrote: > > On ons, feb 01, 2023 at 13:20, Simon Glass <s...@chromium.org> wrote: > > Hi Tobias, > > Hi Simon, > > Thanks for the review! > > > On Wed, 1 Feb 2023 at 11:10, Tobias Waldekranz <tob...@waldekranz.com> > > wrote: > >> > >> blkmaps are loosely modeled on Linux's device mapper subsystem. The > >> basic idea is that you can create virtual block devices whose blocks > >> can be backed by a plethora of sources that are user configurable. > >> > >> This change just adds the basic infrastructure for creating and > >> removing blkmap devices. Subsequent changes will extend this to add > >> support for actual mappings. > >> > >> Signed-off-by: Tobias Waldekranz <tob...@waldekranz.com> > >> --- > >> MAINTAINERS | 6 + > >> disk/part.c | 1 + > >> drivers/block/Kconfig | 18 ++ > >> drivers/block/Makefile | 1 + > >> drivers/block/blk-uclass.c | 1 + > >> drivers/block/blkmap.c | 275 +++++++++++++++++++++++++++++++ > >> include/blkmap.h | 15 ++ > >> include/dm/uclass-id.h | 1 + > >> include/efi_loader.h | 4 + > >> lib/efi_loader/efi_device_path.c | 30 ++++ > >> 10 files changed, 352 insertions(+) > >> create mode 100644 drivers/block/blkmap.c > >> create mode 100644 include/blkmap.h > >>
[..] > > This needs to be created as part of DM. See how host_create_device() > > works. It attaches something to the uclass and then creates child > > devices from there. It also operations (struct host_ops) but you don't > > need to do that. > > > > Note that the host commands support either an label or a devnum, which > > I think is useful, so you might copy that? > > > > I took a look at the hostfs implementation. I agree that labels are much > nicer than bare integers. However, for block maps the idea is to fit in > to the existing filesystem infrastructure. Addressing block devices > using the "<interface> <dev>[:<part>]" pattern seems very well > established... You can still do that, so long as the labels are "0" and "1", etc. But it lets us move to a more flexible system in future. > > >> +{ > >> + static struct udevice *dev; > >> + int err; > >> + > >> + if (dev) > >> + return dev; > >> + > >> + err = device_bind_driver(dm_root(), "blkmap_root", "blkmap", &dev); > >> + if (err) > >> + return NULL; > >> + > >> + err = device_probe(dev); > >> + if (err) { > >> + device_unbind(dev); > >> + return NULL; > >> + } > > > > Should not be needed as probing children will cause this to be probed. > > > > So this function just becomes > > > > uclass_first_device(UCLASS_BLKDEV, & > > > >> + > >> + return dev; > >> +} > >> + > >> +int blkmap_create(int devnum) > > > > Again, please drop the use of devnum and use devices. Here you could > > use a label, perhaps? > > ...which is why I don't think a label is going to fly here. Let's say I > create a new ramdisk with a label instead, e.g.: > > blkmap create rd > blkmap map rd 0 0x100 mem ${loadaddr} > > How do I know which <dev> to supply to, e.g.: > > ls blkmap <dev> /boot > > It seems like labels are a hostfs-specific feature, or am I missing > something? We have the same problem with hostfs, since we have not implemented labels in block devices. For now you must use integer labels. But we will get there. > > >> +{ > >> + struct udevice *root; > > > > Please don't use that name , as we use it for the DM root device. How > > about bm_parent? It isn't actually a tree of devices, so root doesn't > > sound right to me anyway. > > Alright, I'll change it. > > >> + struct blk_desc *bd; > >> + struct blkmap *bm; > >> + int err; > >> + > >> + if (devnum >= 0 && blkmap_from_devnum(devnum)) > >> + return -EBUSY; > >> + > >> + root = blkmap_root(); > >> + if (!root) > >> + return -ENODEV; > >> + > >> + bm = calloc(1, sizeof(*bm)); > > > > Can this be attached to the device as private data, so avoiding the > > malloc()? > > Maybe, I'm not familiar enough with the U-Boot internals. > > As it is now, all blkmaps are children of a single "blkmap_root" > device. I chose that approach so that devnums would be allocated from a > single pool. Well, driver model handles this for you (see dev_seq()). You have a single uclass so can attach your 'overall' blkmap data to that. Then each device can have its own private data attached. The only requirement is that BLK devices have a parent (so we know the media type). I had understood that you had one blkmap for each block map. If that is true, then you don't need to have a parent one as well. You can use the BLKMAP uclass to hold any overall data. > > AFAIK, that would mean having to store it in the "blkmap_blk" device, > but I thought that its private data was owned by the block subsystem? [..] Regards, Simon