Re: [PATCH v8 5/9] block: implement NVMEM provider

Christoph Hellwig <[email protected]> Thu, 23 Jul 2026 21:57:51 -0700
Newsgroups org.infradead.lists.ath10k,org.kernel.vger.linux-arm-msm,org.kernel.vger.linux-block,org.kernel.vger.linux-bluetooth,org.kernel.vger.linux-devicetree,org.kernel.vger.linux-kernel,org.kernel.vger.linux-mmc,org.kernel.vger.linux-wireless,org.kernel.vger.netdev
Message-ID <[email protected]>
On Fri, Jul 03, 2026 at 03:45:18PM +0200, Loic Poulain wrote:
> +	  Allow block devices (or partitions) to act as NVMEM providers,

Based on the code below I don't think you actually support partitions.

> +static int blk_nvmem_reg_read(void *priv, unsigned int from, void *val, size_t bytes)

Overly long line.

> +{
> +	dev_t devt = (dev_t)(uintptr_t)priv;
> +	size_t bytes_left = bytes;
> +	loff_t pos = from;
> +	int ret = 0;
> +
> +	struct file *bdev_file __free(fput) =
> +		bdev_file_open_by_dev(devt, BLK_OPEN_READ, NULL, NULL);

Please try to avoid all that magic autfree stuff that makes the code
impossible to read about.

> +	if (IS_ERR(bdev_file))
> +		return PTR_ERR(bdev_file);
> +
> +	while (bytes_left) {

.. and just use __kernel_read instead of badly reimplementing it.

> +	config.name = dev_name(dev);
> +	config.owner = THIS_MODULE;
> +	config.priv = (void *)(uintptr_t)dev->devt;

Why is this storing the dev_t and not the block device itself?

> +
> +		blk_nvmem_add(disk->part0);

Hmm, and you're always doing it for the whole device.  So the intefaces
really should be based on a gendisk, not a block_device.

Also once you register the whole devices as nvmem provider, we really
should keep it from being used a block device, shouldn't we?