Subject: Re: [RFC PATCH] lightnvm: expose configuration through sysfs
To: Greg KH <gregkh@linuxfoundation.org>
References: <1461777537-8145-1-git-send-email-slund@cnexlabs.com>
 <1461777537-8145-2-git-send-email-slund@cnexlabs.com>
 <20160427174137.GA10513@kroah.com> <572102F1.3060007@lightnvm.io>
 <20160427190023.GA25619@kroah.com>
Cc: "Simon A. F. Lund" <slund@cnexlabs.com>, linux-block@vger.kernel.org,
        linux-kernel@vger.kernel.org
From: =?UTF-8?Q?Matias_Bj=c3=b8rling?= <mb@lightnvm.io>
Message-ID: <5721126F.4090108@lightnvm.io>
Date: Wed, 27 Apr 2016 21:26:39 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101
 Thunderbird/38.6.0
MIME-Version: 1.0
In-Reply-To: <20160427190023.GA25619@kroah.com>
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3288
Lines: 107

On 04/27/2016 09:00 PM, Greg KH wrote:
> On Wed, Apr 27, 2016 at 08:20:33PM +0200, Matias Bj?rling wrote:
>>
>>
>> On 04/27/2016 07:41 PM, Greg KH wrote:
>>> On Wed, Apr 27, 2016 at 10:18:57AM -0700, Simon A. F. Lund wrote:
>>>> --- a/include/linux/lightnvm.h
>>>> +++ b/include/linux/lightnvm.h
>>>> @@ -174,6 +174,7 @@ struct nvm_id_group {
>>>>    	u16	cpar;
>>>>
>>>>    	struct nvm_id_lp_tbl lptbl;
>>>> +	struct kobject kobj;
>>>>    };
>>>>
>>>>    struct nvm_addr_format {
>>>> @@ -205,6 +206,7 @@ struct nvm_target {
>>>>    	struct list_head list;
>>>>    	struct nvm_tgt_type *type;
>>>>    	struct gendisk *disk;
>>>> +	struct kobject kobj;
>>>>    };
>>>>
>>>>    struct nvm_tgt_instance {
>>>> @@ -360,6 +362,8 @@ struct nvm_dev {
>>>>
>>>>    	struct mutex mlock;
>>>>    	spinlock_t lock;
>>>> +
>>>> +	struct kobject kobj;
>>>>    };
>>>>
>>>>    static inline struct ppa_addr generic_to_dev_addr(struct nvm_dev *dev,
>>>
>>> Never use "raw" kobjects in a driver for a device.  You just guaranteed
>>> that userspace tools will not see these devices or attributes, which
>>> implies you didn't really test this using libudev :(
>>>
>>> Please use real devices, attached to the real devices your disks already
>>> have in the tree.
>>>
>>> And are you sure you didn't just mess up your reference counting by
>>> now having the lifecycle of these structures be dictated by the kobject?
>>>
>>> thanks,
>>>
>>> greg k-h
>>>
>>
>> Hi Greg,
>>
>> Thanks for the feedback.
>>
>> lightnvm doesn't have anything to hook up with in the /dev/block/* until a
>> device is exposed through a target. A device goes into a staging area, and
>> then later is configured to expose a block device.
>>
>> In the case of NVMe device driver, the driver brings up a device, identifies
>> it as a lightnvm device, then calls nvm_register and registers the device.
>> It skips the registration as a block device.
>
> But you could register it with sysfs at this point in time, giving you
> a place in the device tree.  Which would be good.

As an example, when the device is identified by the nvme device driver, 
the nvm_register() registers the device (e.g. nvme0n1) in sysfs and 
places it here until further configuration:

   /sys/devices/virtual/misc/lightnvm/devices/nvme0n1

It would expose a representation of the lightnvm configuration

Then when targets are added, we would put the target (e.g. tgt0) in

/sys/devices/virtual/misc/lightnvm/targets/tgt0

and that one could reference the device by

   /sys/devices/virtual/misc/lightnvm/targets/tgt0/devices/nvme0n1

     pointing to

   /sys/devices/virtual/misc/lightnvm/devices/nvme0n1

A target can span multiple devices (that's why the targets/*/devices 
link is there)

Does that make sense? and in this case, the raw kobjects make sense to 
use, as we don't have anything to bind them up to, other than the misc 
device we registered.

>
>> At the nvm_register point, the user can list the available devices through
>> an ioctl, and then choose a target to put on top. The target will then
>> expose it as a block device.
>
> Then move the device at this point in time.
>
>> This might not be the ideal way. I like your input on what would be the
>> proper way to expose such a device.
>
> See above.
>
> thanks,
>
> greg k-h
>