Re: [PATCHv2] nvme/hwmon: rework to avoid devm allocation

[Guenter Roeck <linux@roeck-us.net>]

On Tue, Jan 19, 2021 at 07:43:18AM +0100, Hannes Reinecke wrote:
> The original design to use device-managed resource allocation
> doesn't really work as the NVMe controller has a vastly different
> lifetime than the hwmon sysfs attributes, causing warning about
> duplicate sysfs entries upon reconnection.
> This patch reworks the hwmon allocation to avoid device-managed
> resource allocation, and uses the NVMe controller as parent for
> the sysfs attributes.

I don't really know enough about the nvme infrastructure to understand
this part of the change. Couple of questions: Why is the parent change
needed, and does the "sensors" command still work after this change ?

Guenter

> 
> Cc: Guenter Roeck <linux@roeck-us.net>
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> Tested-by: Daniel Wagner <dwagner@suse.de>

Maybe add

Fixes: 400b6a7b13a3f ("nvme: Add hardware monitoring support")

> ---
>  drivers/nvme/host/core.c  |  1 +
>  drivers/nvme/host/hwmon.c | 31 +++++++++++++++++++++----------
>  drivers/nvme/host/nvme.h  |  6 ++++++
>  3 files changed, 28 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index fff49e544fdf..3c6c77e44bf7 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -4419,6 +4419,7 @@ EXPORT_SYMBOL_GPL(nvme_start_ctrl);
>  
>  void nvme_uninit_ctrl(struct nvme_ctrl *ctrl)
>  {
> +	nvme_hwmon_exit(ctrl);
>  	nvme_fault_inject_fini(&ctrl->fault_inject);
>  	dev_pm_qos_hide_latency_tolerance(ctrl->device);
>  	cdev_device_del(&ctrl->cdev, ctrl->device);
> diff --git a/drivers/nvme/host/hwmon.c b/drivers/nvme/host/hwmon.c
> index 552dbc04567b..8f9e96986780 100644
> --- a/drivers/nvme/host/hwmon.c
> +++ b/drivers/nvme/host/hwmon.c
> @@ -223,12 +223,12 @@ static const struct hwmon_chip_info nvme_hwmon_chip_info = {
>  
>  int nvme_hwmon_init(struct nvme_ctrl *ctrl)
>  {
> -	struct device *dev = ctrl->dev;
> +	struct device *dev = ctrl->device;
>  	struct nvme_hwmon_data *data;
>  	struct device *hwmon;
>  	int err;
>  
> -	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> +	data = kzalloc(sizeof(*data), GFP_KERNEL);
>  	if (!data)
>  		return 0;
>  
> @@ -237,19 +237,30 @@ int nvme_hwmon_init(struct nvme_ctrl *ctrl)
>  
>  	err = nvme_hwmon_get_smart_log(data);
>  	if (err) {
> -		dev_warn(ctrl->device,
> -			"Failed to read smart log (error %d)\n", err);
> -		devm_kfree(dev, data);
> +		dev_warn(dev, "Failed to read smart log (error %d)\n", err);
> +		kfree(data);
>  		return err;
>  	}
>  
> -	hwmon = devm_hwmon_device_register_with_info(dev, "nvme", data,
> -						     &nvme_hwmon_chip_info,
> -						     NULL);
> +	hwmon = hwmon_device_register_with_info(dev, "nvme",
> +						data, &nvme_hwmon_chip_info,
> +						NULL);
>  	if (IS_ERR(hwmon)) {
>  		dev_warn(dev, "Failed to instantiate hwmon device\n");
> -		devm_kfree(dev, data);
> +		kfree(data);
>  	}
> -
> +	ctrl->hwmon_device = hwmon;
>  	return 0;
>  }
> +
> +void nvme_hwmon_exit(struct nvme_ctrl *ctrl)
> +{
> +	if (ctrl->hwmon_device) {
> +		struct nvme_hwmon_data *data =
> +			dev_get_drvdata(ctrl->hwmon_device);
> +
> +		hwmon_device_unregister(ctrl->hwmon_device);
> +		ctrl->hwmon_device = NULL;
> +		kfree(data);
> +	}
> +}
> diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
> index 88a6b97247f5..f51b942bb4f5 100644
> --- a/drivers/nvme/host/nvme.h
> +++ b/drivers/nvme/host/nvme.h
> @@ -246,6 +246,9 @@ struct nvme_ctrl {
>  	struct rw_semaphore namespaces_rwsem;
>  	struct device ctrl_device;
>  	struct device *device;	/* char device */
> +#ifdef CONFIG_NVME_HWMON
> +	struct device *hwmon_device;
> +#endif
>  	struct cdev cdev;
>  	struct work_struct reset_work;
>  	struct work_struct delete_work;
> @@ -809,11 +812,14 @@ static inline struct nvme_ns *nvme_get_ns_from_dev(struct device *dev)
>  
>  #ifdef CONFIG_NVME_HWMON
>  int nvme_hwmon_init(struct nvme_ctrl *ctrl);
> +void nvme_hwmon_exit(struct nvme_ctrl *ctrl);
>  #else
>  static inline int nvme_hwmon_init(struct nvme_ctrl *ctrl)
>  {
>  	return 0;
>  }
> +
> +static inline int nvme_hwmon_exit(struct nvme_ctrl *ctrl) {}
>  #endif
>  
>  u32 nvme_command_effects(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
> -- 
> 2.29.2
> 

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme