Re: [PATCH 2/5] dm: Add initial device mapper infrastructure

[Ahmad Fatoum <a.fatoum@pengutronix.de>]

Hi,

On 8/28/25 5:05 PM, Tobias Waldekranz wrote:
> +struct dm_device *dm_create(const char *name, const char *table)
> +{
> +	struct dm_target *ti;
> +	struct dm_device *dm;
> +	int err;
> +
> +	dm = xzalloc(sizeof(*dm));
> +
> +	dev_set_name(&dm->dev, "%s", name);
> +	dm->dev.id = DEVICE_ID_SINGLE;
> +	err = register_device(&dm->dev);
> +	if (err)
> +		goto err_free;
> +
> +	INIT_LIST_HEAD(&dm->targets);
> +	err = dm_parse_table(dm, table);
> +	if (err)
> +		goto err_unregister;
> +
> +	dm->blk = (struct block_device) {
> +		.dev = &dm->dev,
> +		.cdev = {
> +			.name = xstrdup(name),
> +		},
> +
> +		.type = BLK_TYPE_VIRTUAL,
> +		.ops = &dm_blk_ops,
> +
> +		.num_blocks = dm_size(dm),
> +		.blockbits = SECTOR_SHIFT,
> +	};
> +
> +	err = blockdevice_register(&dm->blk);
> +	if (err)
> +		goto err_destroy;

Mhm, don't we have two block caching layers this way? Is this really
something we want? For drivers/block/ramdisk.c this was deemed
unnecessary, so it opencodes the blockdevice_register.

If this makes sense here as well, we could move that code into
common/block.c and make it reusable.

For now, I think a FIXME alerting to the fact that we have an
unnecessary caching step here is enough.

Cheers,
Ahmad

> +
> +	list_add_tail(&dm->list, &dm_device_list);
> +	return dm;
> +
> +err_destroy:
> +	list_for_each_entry_reverse(ti, &dm->targets, list) {
> +		ti->ops->destroy(ti);
> +	}
> +
> +err_unregister:
> +	unregister_device(&dm->dev);
> +
> +err_free:
> +	free(dm);
> +	return ERR_PTR(err);
> +}
> +EXPORT_SYMBOL(dm_create);
> diff --git a/drivers/block/dm/dm-target.h b/drivers/block/dm/dm-target.h
> new file mode 100644
> index 0000000000..506e808b79
> --- /dev/null
> +++ b/drivers/block/dm/dm-target.h
> @@ -0,0 +1,39 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/* SPDX-FileCopyrightText: © 2025 Tobias Waldekranz <tobias@waldekranz.com>, Wires */
> +
> +#ifndef __DM_TARGET_H
> +#define __DM_TARGET_H
> +
> +struct dm_device;
> +struct dm_target_ops;
> +
> +struct dm_target {
> +	struct dm_device *dm;
> +	struct list_head list;
> +
> +	sector_t base;
> +	blkcnt_t size;
> +
> +	const struct dm_target_ops *ops;
> +	void *private;
> +};
> +
> +void dm_target_err(struct dm_target *ti, const char *fmt, ...);
> +
> +struct dm_target_ops {
> +	struct list_head list;
> +	const char *name;
> +
> +	char *(*asprint)(struct dm_target *ti);
> +	int (*create)(struct dm_target *ti, unsigned int argc, char **argv);
> +	int (*destroy)(struct dm_target *ti);
> +	int (*read)(struct dm_target *ti, void *buf,
> +		    sector_t block, blkcnt_t num_blocks);
> +	int (*write)(struct dm_target *ti, const void *buf,
> +		     sector_t block, blkcnt_t num_blocks);
> +};
> +
> +int dm_target_register(struct dm_target_ops *ops);
> +void dm_target_unregister(struct dm_target_ops *ops);
> +
> +#endif	/* __DM_TARGET_H */
> diff --git a/include/dm.h b/include/dm.h
> new file mode 100644
> index 0000000000..255796ca2f
> --- /dev/null
> +++ b/include/dm.h
> @@ -0,0 +1,16 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +
> +#ifndef __DM_H
> +#define __DM_H
> +
> +struct dm_device;
> +
> +struct dm_device *dm_find_by_name(const char *name);
> +int dm_foreach(int (*cb)(struct dm_device *dm, void *ctx), void *ctx);
> +
> +char *dm_asprint(struct dm_device *dm);
> +
> +void dm_destroy(struct dm_device *dm);
> +struct dm_device *dm_create(const char *name, const char *ctable);
> +
> +#endif /* __DM_H */

-- 
Pengutronix e.K.                  |                             |
Steuerwalder Str. 21              | http://www.pengutronix.de/  |
31137 Hildesheim, Germany         | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686  | Fax:   +49-5121-206917-5555 |