Re: [PATCH 1/8] dax: add region-available-size attribute

[Johannes Thumshirn]

Hi Dan,

On Sat, Dec 10, 2016 at 10:28:30PM -0800, Dan Williams wrote:
> In preparation for a facility that enables dax regions to be
> sub-divided, introduce a 'dax/available_size' attribute.  This attribute
> appears under the parent device that registered the device-dax region,
> and it assumes that the device-dax-core owns the driver-data for that
> device.
> 
> 'dax/available_size' adjusts dynamically as dax-device instances are
> registered and unregistered.
> 
> As a side effect of using __request_region() to reserve capacity from
> the dax_region we now track pointers to those returned resources rather
> than duplicating the passed in resource array.
> 
> Signed-off-by: Dan Williams <dan.j.williams@xxxxxxxxx>
> ---

[...]

> +static const struct attribute_group *dax_region_attribute_groups[] = {
> +	&dax_region_attribute_group,
> +	NULL,
>  };
>  
>  static struct inode *dax_alloc_inode(struct super_block *sb)
> @@ -200,12 +251,27 @@ void dax_region_put(struct dax_region *dax_region)
>  }
>  EXPORT_SYMBOL_GPL(dax_region_put);
>  
> +

Stray extra newline?

[...]

>  struct dax_region *alloc_dax_region(struct device *parent, int region_id,
>  		struct resource *res, unsigned int align, void *addr,
>  		unsigned long pfn_flags)
>  {
>  	struct dax_region *dax_region;
>  
> +	if (dev_get_drvdata(parent)) {
> +		dev_WARN(parent, "dax core found drvdata already in use\n");
> +		return NULL;
> +	}
> +

My first thought was, it might be interesting to see who already claimed
the drvdata. Then I figured, how are multiple sub-regions of a dax-device
supposed to work? What am I missing here?

>  	if (!IS_ALIGNED(res->start, align)
>  			|| !IS_ALIGNED(resource_size(res), align))
>  		return NULL;
> @@ -213,16 +279,26 @@ struct dax_region *alloc_dax_region(struct device *parent, int region_id,
>  	dax_region = kzalloc(sizeof(*dax_region), GFP_KERNEL);
>  	if (!dax_region)
>  		return NULL;
> -
> -	memcpy(&dax_region->res, res, sizeof(*res));
> +	dev_set_drvdata(parent, dax_region);
> +	dax_region->res.name = dev_name(parent);
> +	dax_region->res.start = res->start;
> +	dax_region->res.end = res->end;
>  	dax_region->pfn_flags = pfn_flags;
> +	mutex_init(&dax_region->lock);
>  	kref_init(&dax_region->kref);
>  	dax_region->id = region_id;
>  	ida_init(&dax_region->ida);
>  	dax_region->align = align;
>  	dax_region->dev = parent;
>  	dax_region->base = addr;
> +	if (sysfs_create_groups(&parent->kobj, dax_region_attribute_groups)) {
> +		kfree(dax_region);
> +		return NULL;;
> +	}
>  
> +	kref_get(&dax_region->kref);
> +	if (devm_add_action_or_reset(parent, dax_region_unregister, dax_region))
> +		return NULL;
>  	return dax_region;
>  }
>  EXPORT_SYMBOL_GPL(alloc_dax_region);

[...]

> @@ -568,28 +654,42 @@ struct dax_dev *devm_create_dax_dev(struct dax_region *dax_region,
>  	struct cdev *cdev;
>  	dev_t dev_t;
>  
> -	dax_dev = kzalloc(sizeof(*dax_dev) + sizeof(*res) * count, GFP_KERNEL);
> +	dax_dev = kzalloc(sizeof(*dax_dev), GFP_KERNEL);
>  	if (!dax_dev)
>  		return ERR_PTR(-ENOMEM);
>  
> +	dax_dev->res = kzalloc(sizeof(res) * count, GFP_KERNEL);

	dax_dev->res = kcalloc(sizeof(res), count, GFP_KERNEL); ?

> +	if (!dax_dev->res)
> +		goto err_res;
> +

Byte,
	Johannes