From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755010AbcLOGsa (ORCPT ); Thu, 15 Dec 2016 01:48:30 -0500 Received: from mail-oi0-f50.google.com ([209.85.218.50]:35152 "EHLO mail-oi0-f50.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753581AbcLOGs3 (ORCPT ); Thu, 15 Dec 2016 01:48:29 -0500 MIME-Version: 1.0 In-Reply-To: References: <148143770485.10950.13227732273892953675.stgit@dwillia2-desk3.amr.corp.intel.com> <148143771052.10950.7622110904173815243.stgit@dwillia2-desk3.amr.corp.intel.com> <20161214143802.GD6279@linux-x5ow.site> From: Dan Williams Date: Wed, 14 Dec 2016 22:47:00 -0800 Message-ID: Subject: Re: [PATCH 1/8] dax: add region-available-size attribute To: Johannes Thumshirn Cc: "linux-nvdimm@lists.01.org" , "linux-kernel@vger.kernel.org" Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Dec 14, 2016 at 7:53 AM, Dan Williams wrote: > On Wed, Dec 14, 2016 at 6:38 AM, Johannes Thumshirn 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 >>> --- >> >> [...] >> >>> +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. No, I went to go delete this and remembered the real reason this was added. A device driver that calls alloc_dax_region() commits to letting the dax core own dev->driver_data. Since this wasn't even clear to me, I'll go fix up the comment.