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

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

[Hannes Reinecke]

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.

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

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

[Daniel Wagner]

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.

To give Guenter a bit more info, it is not just warnings, user space is
able to crash the system by reading the hwmon entries when the nvme
device has been removed.

and here is the initial report and discussion on this:

  https://lore.kernel.org/linux-nvme/20201209213228.5044-1-ematsumiya@suse.de/

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

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

[Guenter Roeck]

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

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

[Hannes Reinecke]

On 1/19/21 3:56 PM, Guenter Roeck wrote:
> 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 ?
> 
Problem is that it's not really a 'new' infrastructure, it's 
NVMe-over-fabrics, which behaves slightly different from the PCI-based 
NVMe devices.
In particular we can and will have connectivity failures, requiring the 
controller to be reset or recreated.
And as the hwmon sysfs attributes were never removed correctly in the 
first place we're seeing these errors.
It isn't easily recreated on PCI devices as the sysfs attributes will 
only ever be removed on shutdown (or module unload).
So I fear the issue was with us since day-1.

As for the parent change: for fabrics the '->dev' device points to a 
static nvmf_device, which is identical for every fabrics controller.
So you really need to use the actual device in '->device' to have a 
different parent device for different controllers.
So for PCI that means that the 'hwmon' device moved one element down 
(from the PCI device to the nvme controller).
But shouldn't make much of a difference; I'll check on if 'sensors' 
continues to work.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

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

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

[Enzo Matsumiya]

On 01/19, 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.

Works fine on my NVMe-oF/TCP setup now. Thanks.

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

Tested-by: Enzo Matsumiya <ematsumiya@suse.de>


Cheers,

Enzo

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

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

[Guenter Roeck]

On 1/19/21 11:07 AM, Hannes Reinecke wrote:
> On 1/19/21 3:56 PM, Guenter Roeck wrote:
>> 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 ?
>>
> Problem is that it's not really a 'new' infrastructure, it's NVMe-over-fabrics, which behaves slightly different from the PCI-based NVMe devices.
> In particular we can and will have connectivity failures, requiring the controller to be reset or recreated.
> And as the hwmon sysfs attributes were never removed correctly in the first place we're seeing these errors.
> It isn't easily recreated on PCI devices as the sysfs attributes will only ever be removed on shutdown (or module unload).
> So I fear the issue was with us since day-1.
> 
I understand; that is why I suggested to add a Fixes: tag.

> As for the parent change: for fabrics the '->dev' device points to a static nvmf_device, which is identical for every fabrics controller.
> So you really need to use the actual device in '->device' to have a different parent device for different controllers.
> So for PCI that means that the 'hwmon' device moved one element down (from the PCI device to the nvme controller).
> But shouldn't make much of a difference; I'll check on if 'sensors' continues to work.
> 

It needs lm-sensors version 3.5.0 or later. Older versions
don't support a device hierarchy.

Guenter

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

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

[Christoph Hellwig]

On Tue, Jan 19, 2021 at 09:54:30PM -0800, Guenter Roeck wrote:
> It needs lm-sensors version 3.5.0 or later. Older versions
> don't support a device hierarchy.

Hannes, did you get a chance to test this with lm-sensors?

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

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

[Daniel Wagner]

On Wed, Jan 27, 2021 at 05:35:55PM +0100, Christoph Hellwig wrote:
> On Tue, Jan 19, 2021 at 09:54:30PM -0800, Guenter Roeck wrote:
> > It needs lm-sensors version 3.5.0 or later. Older versions
> > don't support a device hierarchy.
>
> Hannes, did you get a chance to test this with lm-sensors?

I gave it a go. All looks good.

Tested-by: Daniel Wagner <dwagner@suse.de>


  dolin:~/:[0]# sensors --version
  sensors version 3.5.0 with libsensors version 3.5.0

Baseline:

  dolin:~/:[0]# sensors
  coretemp-isa-0000
  Adapter: ISA adapter
  Package id 0:  +36.0°C  (high = +67.0°C, crit = +77.0°C)
  Core 0:        +28.0°C  (high = +67.0°C, crit = +77.0°C)
  Core 1:        +29.0°C  (high = +67.0°C, crit = +77.0°C)
  Core 2:        +29.0°C  (high = +67.0°C, crit = +77.0°C)
  Core 3:        +28.0°C  (high = +67.0°C, crit = +77.0°C)
  Core 4:        +29.0°C  (high = +67.0°C, crit = +77.0°C)
  Core 8:        +27.0°C  (high = +67.0°C, crit = +77.0°C)
  Core 9:        +28.0°C  (high = +67.0°C, crit = +77.0°C)
  Core 10:       +28.0°C  (high = +67.0°C, crit = +77.0°C)
  Core 11:       +28.0°C  (high = +67.0°C, crit = +77.0°C)
  Core 12:       +28.0°C  (high = +67.0°C, crit = +77.0°C)

  nvme-virtual-0
  Adapter: Virtual device
  Composite:   -273.1°C

  nvme-virtual-0
  Adapter: Virtual device
  Composite:   -273.1°C
  [...]


With the patch:

  dolin:~/:[0]# sensors
  coretemp-isa-0000
  Adapter: ISA adapter
  Package id 0:  +38.0°C  (high = +67.0°C, crit = +77.0°C)
  Core 0:        +29.0°C  (high = +67.0°C, crit = +77.0°C)
  Core 1:        +31.0°C  (high = +67.0°C, crit = +77.0°C)
  Core 2:        +29.0°C  (high = +67.0°C, crit = +77.0°C)
  Core 3:        +30.0°C  (high = +67.0°C, crit = +77.0°C)
  Core 4:        +29.0°C  (high = +67.0°C, crit = +77.0°C)
  Core 8:        +30.0°C  (high = +67.0°C, crit = +77.0°C)
  Core 9:        +28.0°C  (high = +67.0°C, crit = +77.0°C)
  Core 10:       +30.0°C  (high = +67.0°C, crit = +77.0°C)
  Core 11:       +30.0°C  (high = +67.0°C, crit = +77.0°C)
  Core 12:       +28.0°C  (high = +67.0°C, crit = +77.0°C)

  nvme-virtual-0
  Adapter: Virtual device
  Composite:   -273.1°C

  nvme-virtual-0
  Adapter: Virtual device
  Composite:   -273.1°C
  [...]


  dolin:~/:[0]# find /sys -name hwmon*
  /sys/kernel/debug/tracing/events/hwmon
  /sys/kernel/debug/tracing/events/hwmon/hwmon_attr_show
  /sys/kernel/debug/tracing/events/hwmon/hwmon_attr_store
  /sys/kernel/debug/tracing/events/hwmon/hwmon_attr_show_string
  /sys/class/hwmon
  /sys/class/hwmon/hwmon16
  /sys/class/hwmon/hwmon14
  /sys/class/hwmon/hwmon8
  /sys/class/hwmon/hwmon12
  /sys/class/hwmon/hwmon6
  /sys/class/hwmon/hwmon20
  /sys/class/hwmon/hwmon10
  /sys/class/hwmon/hwmon4
  /sys/class/hwmon/hwmon19
  /sys/class/hwmon/hwmon2
  /sys/class/hwmon/hwmon17
  /sys/class/hwmon/hwmon0
  /sys/class/hwmon/hwmon15
  /sys/class/hwmon/hwmon9
  /sys/class/hwmon/hwmon13
  /sys/class/hwmon/hwmon7
  /sys/class/hwmon/hwmon11
  /sys/class/hwmon/hwmon5
  /sys/class/hwmon/hwmon3
  /sys/class/hwmon/hwmon18
  /sys/class/hwmon/hwmon1
  /sys/devices/platform/coretemp.3/hwmon
  /sys/devices/platform/coretemp.3/hwmon/hwmon19
  /sys/devices/platform/coretemp.1/hwmon
  /sys/devices/platform/coretemp.1/hwmon/hwmon17
  /sys/devices/platform/coretemp.2/hwmon
  /sys/devices/platform/coretemp.2/hwmon/hwmon18
  /sys/devices/platform/coretemp.0/hwmon
  /sys/devices/platform/coretemp.0/hwmon/hwmon16
  /sys/devices/pci0000:00/0000:00:02.2/0000:03:00.0/hwmon
  /sys/devices/pci0000:00/0000:00:02.2/0000:03:00.0/hwmon/hwmon20
  /sys/devices/virtual/nvme-fabrics/ctl/nvme23/hwmon11
  /sys/devices/virtual/nvme-fabrics/ctl/nvme7/hwmon1
  /sys/devices/virtual/nvme-fabrics/ctl/nvme21/hwmon9
  /sys/devices/virtual/nvme-fabrics/ctl/nvme18/hwmon6
  /sys/devices/virtual/nvme-fabrics/ctl/nvme26/hwmon14
  /sys/devices/virtual/nvme-fabrics/ctl/nvme16/hwmon4
  /sys/devices/virtual/nvme-fabrics/ctl/nvme24/hwmon12
  /sys/devices/virtual/nvme-fabrics/ctl/nvme14/hwmon2
  /sys/devices/virtual/nvme-fabrics/ctl/nvme22/hwmon10
  /sys/devices/virtual/nvme-fabrics/ctl/nvme6/hwmon0
  /sys/devices/virtual/nvme-fabrics/ctl/nvme20/hwmon8
  /sys/devices/virtual/nvme-fabrics/ctl/nvme19/hwmon7
  /sys/devices/virtual/nvme-fabrics/ctl/nvme27/hwmon15
  /sys/devices/virtual/nvme-fabrics/ctl/nvme17/hwmon5
  /sys/devices/virtual/nvme-fabrics/ctl/nvme25/hwmon13
  /sys/devices/virtual/nvme-fabrics/ctl/nvme15/hwmon3
  

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

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

[Christoph Hellwig]

Thanks,

applied to nvme-5.12.

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

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

[Minwoo Im]

On 21-01-19 07:43:18, 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.
> 
> Cc: Guenter Roeck <linux@roeck-us.net>
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> Tested-by: Daniel Wagner <dwagner@suse.de>
> ---
>  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) {}

Currently, nvme-5.12 has build errors:

rivers/nvme/target/../host/nvme.h: In function ‘nvme_hwmon_exit’:
drivers/nvme/target/../host/nvme.h:825:42: error: no return statement in function returning non-void [-Werror=return-type]
 static inline int nvme_hwmon_exit(struct nvme_ctrl *ctrl) {}
                                          ^~~~~~~~~

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

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

[Christoph Hellwig]

I've fixed the !CONFIG_NVME_HWMON path up, thanks.

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