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

[Dan Williams]

On Wed, Dec 14, 2016 at 6:38 AM, Johannes Thumshirn <jthumshirn@xxxxxxx> wrote:
> 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?

This is a check similar to the -EBUSY return you would get from
request_mem_region(). In fact if all dax drivers are correctly calling
request_mem_region() before alloc_dax_region() then it would be
impossible for this check to ever fire. It's already impossible
because there's only one dax driver upstream (dax_pmem). It's not
really benefiting the kernel at all until we have multiple dax
drivers, I'll remove it.