Re: [RFC PATCH] lightnvm: expose configuration through sysfs

["Matias Bjørling" <mb@lightnvm.io>]

On 05/06/2016 06:37 PM, Greg KH wrote:
> On Wed, Apr 27, 2016 at 09:26:39PM +0200, Matias Bjørling wrote:
>> 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.
>
> Maybe, but really, why not use a struct device?  You just prevented all
> userspace tools from finding your devices (i.e. libudev), so you are
> going to have a hard time with any tools that want to see the tree of
> devices in the system.
>
> And you also just lost the power management chain as well, the driver
> model doesn't know to drop into a kobject, as it can't, so you might not
> be working properly at all with that here for suspend/resume and the
> like.
>
> good luck,
>
> greg k-h
>

Thanks greg. We are reworking the struct device into it. Requires a 
little bit more work from the nvme device driver, else it should be 
good. Thanks for your feedback.