Re: [PATCH 1/3] pmem: Initial version of persistent memory driver

[Ross Zwisler <ross.zwisler@linux.intel.com>]

On Wed, 2015-03-25 at 17:04 +0100, Christoph Hellwig wrote:
> From: Ross Zwisler <ross.zwisler@linux.intel.com>
> 
> PMEM is a new driver that presents a reserved range of memory as a
> block device.  This is useful for developing with NV-DIMMs, and
> can be used with volatile memory as a development platform.
> 
> Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
> [hch: convert to use a platform_device for discovery, fix partition
>  support]

Overall I really like this approach.  It makes things simpler, removes
unneeded code and most importantly removes the ability for the user to have a
configuration where the PMEM / memmap reservation via the command line don't
match the parameters given to pmem.

What needed to be fixed with the partition support?  I used to have real
numbers for first_minor and passed into alloc_disk(), but simplified it based
on code found in this commit in the nvme driver:

469071a37afc NVMe: Dynamically allocate partition numbers

This has worked fine for me - is there some test case in which it breaks?

> +static int pmem_probe(struct platform_device *pdev)
> +{
> +	struct pmem_device *pmem;
> +	struct gendisk *disk;
> +	struct resource *res;
> +	int idx, err;
> +
> +	if (WARN_ON(pdev->num_resources > 1))
> +		return -ENXIO;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res)
> +		return -ENXIO;
> +
> +	pmem = kzalloc(sizeof(*pmem), GFP_KERNEL);
> +	if (unlikely(!pmem))
> +		return -ENOMEM;
> +
> +	pmem->phys_addr = res->start;
> +	pmem->size = resource_size(res);
> +
> +	err = pmem_mapmem(pmem);
> +	if (unlikely(err))
> +		goto out_free_dev;
> +
> +	err = -ENOMEM;
> +	pmem->pmem_queue = blk_alloc_queue(GFP_KERNEL);
> +	if (unlikely(!pmem->pmem_queue))
> +		goto out_unmap;
> +
> +	blk_queue_make_request(pmem->pmem_queue, pmem_make_request);
> +	blk_queue_max_hw_sectors(pmem->pmem_queue, 1024);
> +	blk_queue_bounce_limit(pmem->pmem_queue, BLK_BOUNCE_ANY);
> +
> +	disk = alloc_disk(PMEM_MINORS);
> +	if (unlikely(!disk))
> +		goto out_free_queue;
> +
> +	idx = atomic_inc_return(&pmem_index) - 1;
> +
> +	disk->major		= pmem_major;
> +	disk->first_minor	= PMEM_MINORS * idx;
> +	disk->fops		= &pmem_fops;
> +	disk->private_data	= pmem;
> +	disk->queue		= pmem->pmem_queue;
> +	disk->flags		= GENHD_FL_EXT_DEVT;
> +	sprintf(disk->disk_name, "pmem%d", idx);
> +	disk->driverfs_dev = &pdev->dev;
> +	set_capacity(disk, pmem->size >> SECTOR_SHIFT);
> +	pmem->pmem_disk = disk;
> +
> +	add_disk(disk);
> +
> +	platform_set_drvdata(pdev, pmem);
> +	return 0;
> +
> +out_free_queue:
> +	blk_cleanup_queue(pmem->pmem_queue);
> +out_unmap:
> +	pmem_unmapmem(pmem);
> +out_free_dev:
> +	kfree(pmem);
> +out:

This label is no longer used, and can be removed.