> On 20 Mar 2019, at 16.38, Marcin Dziegielewski <marcin.dziegielewski@intel.com> wrote:
> 
> Currently if we issue reboot to the system pblk will close
> ungracefully and in consequence it will need recovery on load.
> 
> This patch propose utilize of reboot notifier feature to trigger
> gracefull pblk shutdown on reboot.
> 
> Signed-off-by: Marcin Dziegielewski <marcin.dziegielewski@intel.com>
> ---
> drivers/lightnvm/core.c | 64 +++++++++++++++++++++++++++++++++++++++----------
> 1 file changed, 51 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/lightnvm/core.c b/drivers/lightnvm/core.c
> index 5f82036..5488051 100644
> --- a/drivers/lightnvm/core.c
> +++ b/drivers/lightnvm/core.c
> @@ -27,6 +27,7 @@
> #include <linux/miscdevice.h>
> #include <linux/lightnvm.h>
> #include <linux/sched/sysctl.h>
> +#include <linux/reboot.h>
> 
> static LIST_HEAD(nvm_tgt_types);
> static DECLARE_RWSEM(nvm_tgtt_lock);
> @@ -1138,6 +1139,48 @@ struct nvm_dev *nvm_alloc_dev(int node)
> }
> EXPORT_SYMBOL(nvm_alloc_dev);
> 
> +static void _nvm_unregister(struct nvm_dev *dev, bool graceful)
> +{
> +	struct nvm_target *t, *tmp;
> +
> +	mutex_lock(&dev->mlock);
> +	list_for_each_entry_safe(t, tmp, &dev->targets, list) {
> +		if (t->dev->parent != dev)
> +			continue;
> +		__nvm_remove_target(t, graceful);
> +	}
> +	mutex_unlock(&dev->mlock);
> +
> +	list_del(&dev->devices);
> +
> +	nvm_free(dev);
> +}
> +
> +static int nvm_notify_reboot(struct notifier_block *this,
> +			    unsigned long code, void *x)
> +{
> +	struct nvm_dev *dev, *t;
> +
> +	down_write(&nvm_lock);
> +	if (list_empty(&nvm_devices)) {
> +		up_write(&nvm_lock);
> +		return NOTIFY_DONE;
> +	}
> +
> +	list_for_each_entry_safe(dev, t, &nvm_devices, devices)
> +		_nvm_unregister(dev, true);
> +
> +	up_write(&nvm_lock);
> +
> +	return NOTIFY_DONE;
> +}
> +
> +static struct notifier_block nvm_notifier = {
> +	.notifier_call	= nvm_notify_reboot,
> +	.next		= NULL,
> +	.priority	= INT_MAX, /* before any real devices */

Why this priority?

> +};
> +
> int nvm_register(struct nvm_dev *dev)
> {
> 	int ret, exp_pool_size;
> @@ -1161,8 +1204,11 @@ int nvm_register(struct nvm_dev *dev)
> 		return -ENOMEM;
> 	}
> 
> -	/* register device with a supported media manager */
> 	down_write(&nvm_lock);
> +	if (list_empty(&nvm_devices))
> +		register_reboot_notifier(&nvm_notifier);
> +
> +	/* register device with a supported media manager */
> 	list_add(&dev->devices, &nvm_devices);
> 	up_write(&nvm_lock);
> 
> @@ -1172,21 +1218,13 @@ int nvm_register(struct nvm_dev *dev)
> 
> void nvm_unregister(struct nvm_dev *dev)
> {
> -	struct nvm_target *t, *tmp;
> +	down_write(&nvm_lock);
> 
> -	mutex_lock(&dev->mlock);
> -	list_for_each_entry_safe(t, tmp, &dev->targets, list) {
> -		if (t->dev->parent != dev)
> -			continue;
> -		__nvm_remove_target(t, false);
> -	}
> -	mutex_unlock(&dev->mlock);
> +	_nvm_unregister(dev, false);

You are adding an extra lock dependency here. I cannot see any obvious
problem with it, but you probably want to test this with lock debugging
enabled.

> 
> -	down_write(&nvm_lock);
> -	list_del(&dev->devices);
> +	if (list_empty(&nvm_devices))
> +		unregister_reboot_notifier(&nvm_notifier);
> 	up_write(&nvm_lock);
> -
> -	nvm_free(dev);
> }
> EXPORT_SYMBOL(nvm_unregister);
> 
> --
> 1.8.3.1

Otherwise, I think adding this functionality is beneficial.