[PATCH v2 0/1] nvme: make SMART / Health Status AENs reach hwmon alarms

[PATCH v2 0/1] nvme: make SMART / Health Status AENs reach hwmon alarms

[Guixin Liu]

The NVMe hwmon support exposes the temperature alarm as a plain sysfs
attribute, so monitoring tools have to poll temp1_alarm to learn about
a critical temperature condition. The device does send a SMART / Health
Status asynchronous event when a critical warning is raised, but the
host never enables it in the Asynchronous Event Configuration, and the
completion path silently drops the event.

The temperature warning is subscribed unconditionally when hwmon is
present (it has no OAES capability bit), the deferred work re-reads
the SMART log to clear the latched event before the Asynchronous Event
Request is re-armed, and the alarm is announced with
hwmon_notify_event(), which also feeds the registered thermal zone.

Testing: verified end to end against vanilla QEMU's nvme emulation:
injecting smart_critical_warning over QMP delivers the AEN, poll(2) on
temp1_alarm wakes immediately with EPOLLERR|EPOLLPRI and reads alarm=1;
clearing the warning produces no event and no spurious wake;
re-injecting the bit wakes again.

Changes since v1:

 - drop the uapi patch defining an OAES bit for the SMART event: OAES
   bits 7:0 are reserved; the warning is configured through the
   SMART / Health Critical Warnings field of the Asynchronous Event
   Configuration feature (sashiko)
 - subscribe to the temperature warning unconditionally when hwmon is
   present, instead of masking against ctrl->oaes (sashiko)
 - re-read the SMART log in the deferred work to clear the latched
   event before the Asynchronous Event Request is re-armed, closing an
   AEN completion tight loop (sashiko)
 - call hwmon_notify_event() instead of sysfs_notify(), so the thermal
   zone registered with HWMON_C_REGISTER_TZ is fed as well (sashiko)
 - correct the cover letter: temp1_alarm reflects the temperature
   warning only, not the spare-block condition

Link: https://lore.kernel.org/all/20261109120207.1352165-1-kanie@linux.alibaba.com/

Guixin Liu (1):
  nvme-hwmon: notify hwmon alarms on the SMART / Health Status AEN

 drivers/nvme/host/core.c  | 17 ++++++++++++++++-
 drivers/nvme/host/hwmon.c | 40 +++++++++++++++++++++++++++++++++++++++
 drivers/nvme/host/nvme.h  | 15 +++++++++++++++
 3 files changed, 71 insertions(+), 1 deletion(-)

Signed-off-by: Guixin Liu <kanie@linux.alibaba.com>

-- 
2.43.7

[PATCH v2 1/1] nvme-hwmon: notify hwmon alarms on the SMART / Health Status AEN

[Guixin Liu]

The NVMe hwmon support exposes the temperature alarm as a plain sysfs
attribute, so monitoring tools have to poll temp1_alarm to learn about
a critical temperature condition. The device does send a SMART / Health
Status asynchronous event when a critical warning is raised, but the
host never enables it in the Asynchronous Event Configuration, and the
completion path silently drops the event.

The temperature warning is bit 1 of the SMART / Health Critical
Warnings field of the Asynchronous Event Configuration (feature 0x0b,
command dword 11 bits 7:0), which mirrors the critical_warning byte of
the SMART log. Unlike the notice events, it has no OAES capability bit,
so the subscription does not go through ctrl->oaes.

The event stays latched until the SMART log is read with RAE cleared,
so the deferred work re-reads it before the completion path re-arms
the Asynchronous Event Request, and then calls hwmon_notify_event()
on temp1_alarm. hwmon_notify_event() also feeds the thermal zone
registered with HWMON_C_REGISTER_TZ and emits the standard hwmon
uevent, which a plain sysfs_notify() would skip.

The nvme-pci transport completes asynchronous events from interrupt
context, so the completion path only queues the work.

Signed-off-by: Guixin Liu <kanie@linux.alibaba.com>
---
 drivers/nvme/host/core.c  | 17 ++++++++++++++++-
 drivers/nvme/host/hwmon.c | 36 ++++++++++++++++++++++++++++++++++++
 drivers/nvme/host/nvme.h  | 15 +++++++++++++++
 3 files changed, 67 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index cd0df9b038063..e932e1a7e479c 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1792,6 +1792,17 @@ static void nvme_enable_aen(struct nvme_ctrl *ctrl)
 	u32 result, supported_aens = ctrl->oaes & NVME_AEN_SUPPORTED;
 	int status;
 
+	/*
+	 * Unlike the notice events above, SMART / Health Status events have
+	 * no OAES capability bit: AEC dw11 bits 7:0 are the SMART / Health
+	 * Critical Warnings (SHCW) mask, which mirrors the critical_warning
+	 * byte of the SMART log.  Subscribe to the temperature warning
+	 * whenever the hwmon support is present, as that is the only
+	 * consumer.
+	 */
+	if (nvme_hwmon_present(ctrl))
+		supported_aens |= NVME_SMART_CRIT_TEMPERATURE;
+
 	if (!supported_aens)
 		return;
 
@@ -5009,8 +5020,12 @@ void nvme_complete_async_event(struct nvme_ctrl *ctrl, __le16 status,
 			nvme_handle_aer_persistent_error(ctrl);
 			return;
 		}
-		fallthrough;
+		ctrl->aen_result = result;
+		break;
 	case NVME_AER_SMART:
+		nvme_hwmon_smart_event(ctrl);
+		ctrl->aen_result = result;
+		break;
 	case NVME_AER_CSS:
 	case NVME_AER_VS:
 		ctrl->aen_result = result;
diff --git a/drivers/nvme/host/hwmon.c b/drivers/nvme/host/hwmon.c
index 406203d8b9adb..172ef63f1180c 100644
--- a/drivers/nvme/host/hwmon.c
+++ b/drivers/nvme/host/hwmon.c
@@ -14,6 +14,7 @@ struct nvme_hwmon_data {
 	struct nvme_ctrl *ctrl;
 	struct nvme_smart_log *log;
 	struct mutex read_lock;
+	struct work_struct smart_work;
 };
 
 static int nvme_get_temp_thresh(struct nvme_ctrl *ctrl, int sensor, bool under,
@@ -223,6 +224,39 @@ static const struct hwmon_chip_info nvme_hwmon_chip_info = {
 	.info	= nvme_hwmon_info,
 };
 
+static void nvme_hwmon_smart_notify(struct work_struct *work)
+{
+	struct nvme_hwmon_data *data =
+		container_of(work, struct nvme_hwmon_data, smart_work);
+	struct nvme_ctrl *ctrl = data->ctrl;
+	int ret;
+
+	/*
+	 * The event stays latched on the controller until the SMART log
+	 * is read with RAE cleared, so re-read it here before the
+	 * completion path re-arms the Asynchronous Event Request.
+	 */
+	mutex_lock(&data->read_lock);
+	ret = nvme_hwmon_get_smart_log(data);
+	mutex_unlock(&data->read_lock);
+	if (ret)
+		return;
+
+	hwmon_notify_event(ctrl->hwmon_device, hwmon_temp,
+			   hwmon_temp_alarm, 0);
+}
+
+void nvme_hwmon_smart_event(struct nvme_ctrl *ctrl)
+{
+	struct nvme_hwmon_data *data;
+
+	if (!ctrl->hwmon_device)
+		return;
+
+	data = dev_get_drvdata(ctrl->hwmon_device);
+	queue_work(nvme_wq, &data->smart_work);
+}
+
 int nvme_hwmon_init(struct nvme_ctrl *ctrl)
 {
 	struct device *dev = ctrl->device;
@@ -242,6 +276,7 @@ int nvme_hwmon_init(struct nvme_ctrl *ctrl)
 
 	data->ctrl = ctrl;
 	mutex_init(&data->read_lock);
+	INIT_WORK(&data->smart_work, nvme_hwmon_smart_notify);
 
 	err = nvme_hwmon_get_smart_log(data);
 	if (err) {
@@ -273,6 +308,7 @@ void nvme_hwmon_exit(struct nvme_ctrl *ctrl)
 		struct nvme_hwmon_data *data =
 			dev_get_drvdata(ctrl->hwmon_device);
 
+		cancel_work_sync(&data->smart_work);
 		hwmon_device_unregister(ctrl->hwmon_device);
 		ctrl->hwmon_device = NULL;
 		kfree(data->log);
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 2543e52d6358a..fe43ab17c3967 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -1276,6 +1276,12 @@ 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);
+void nvme_hwmon_smart_event(struct nvme_ctrl *ctrl);
+
+static inline bool nvme_hwmon_present(struct nvme_ctrl *ctrl)
+{
+	return ctrl->hwmon_device != NULL;
+}
 #else
 static inline int nvme_hwmon_init(struct nvme_ctrl *ctrl)
 {
@@ -1285,6 +1291,15 @@ static inline int nvme_hwmon_init(struct nvme_ctrl *ctrl)
 static inline void nvme_hwmon_exit(struct nvme_ctrl *ctrl)
 {
 }
+
+static inline void nvme_hwmon_smart_event(struct nvme_ctrl *ctrl)
+{
+}
+
+static inline bool nvme_hwmon_present(struct nvme_ctrl *ctrl)
+{
+	return false;
+}
 #endif
 
 static inline void nvme_start_request(struct request *rq)
-- 
2.43.7

Re: [PATCH v2 1/1] nvme-hwmon: notify hwmon alarms on the SMART / Health Status AEN

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [High] nvme-hwmon: race between smart_work and async_event_work
  re-arms AER early
- [High] nvme: NULL submit_async_event() dereference via
  nvme_enable_aen()
- [Low] nvme-hwmon: unfiltered NVME_AER_SMART subtype in
  nvme_complete_async_event()
--

commit 0ce1f50fb06d20be5bc969ec67713426ed866b3c
Author: Guixin Liu <kanie@linux.alibaba.com>

nvme-hwmon: notify hwmon alarms on the SMART / Health Status AEN

This commit enables the temperature critical warning in the Asynchronous
Event Configuration when hwmon is present and schedules work on SMART /
Health Status AEN completions to re-read the SMART log and notify hwmon
on temp1_alarm.

> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index f4aa02869b790..9e355fb0e3992 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -1792,6 +1792,17 @@ static void nvme_enable_aen(struct nvme_ctrl *ctrl)
>  	u32 result, supported_aens = ctrl->oaes & NVME_AEN_SUPPORTED;
>  	int status;
>  
> +	/*
> +	 * Unlike the notice events above, SMART / Health Status events have
> +	 * no OAES capability bit: AEC dw11 bits 7:0 are the SMART / Health
> +	 * Critical Warnings (SHCW) mask, which mirrors the critical_warning
> +	 * byte of the SMART log.  Subscribe to the temperature warning
> +	 * whenever the hwmon support is present, as that is the only
> +	 * consumer.
> +	 */
> +	if (nvme_hwmon_present(ctrl))
> +		supported_aens |= NVME_SMART_CRIT_TEMPERATURE;
> +
>  	if (!supported_aens)
>  		return;

[Severity: High]
Can this cause a NULL pointer dereference in nvme_async_event_work() on
controllers whose nvme_ctrl_ops do not implement submit_async_event()?

The apple-nvme driver does not support asynchronous events, reports
ctrl->oaes == 0, and leaves submit_async_event NULL in its nvme_ctrl_ops,
but still initializes hwmon via nvme_init_ctrl_finish().  Previously,
supported_aens remained 0 so nvme_enable_aen() returned early.  With
NVME_SMART_CRIT_TEMPERATURE set whenever ctrl->hwmon_device is present,
nvme_enable_aen() now queues ctrl->async_event_work, and
nvme_async_event_work() unconditionally calls
ctrl->ops->submit_async_event(ctrl).

> @@ -5006,8 +5017,12 @@ void nvme_complete_async_event(struct nvme_ctrl *ctrl, __le16 status,
>  			nvme_handle_aer_persistent_error(ctrl);
>  			return;
>  		}
> -		fallthrough;
> +		ctrl->aen_result = result;
> +		break;
>  	case NVME_AER_SMART:
> +		nvme_hwmon_smart_event(ctrl);
> +		ctrl->aen_result = result;
> +		break;

[Severity: Low]
Should this check aer_subtype (or log->critical_warning in
nvme_hwmon_smart_notify()) before notifying temp1_alarm?

If any non-temperature SMART / Health Status warning is enabled in
NVME_FEAT_ASYNC_EVENT (for example via admin passthrough),
nvme_hwmon_smart_notify() will read the SMART log with RAE cleared and
emit a false temp1_alarm notification and thermal zone update.

> diff --git a/drivers/nvme/host/hwmon.c b/drivers/nvme/host/hwmon.c
> index 406203d8b9adb..172ef63f1180c 100644
> --- a/drivers/nvme/host/hwmon.c
> +++ b/drivers/nvme/host/hwmon.c
[ ... ]
> @@ -223,6 +224,39 @@ static const struct hwmon_chip_info nvme_hwmon_chip_info = {
>  	.info	= nvme_hwmon_info,
>  };
>  
> +static void nvme_hwmon_smart_notify(struct work_struct *work)
> +{
> +	struct nvme_hwmon_data *data =
> +		container_of(work, struct nvme_hwmon_data, smart_work);
> +	struct nvme_ctrl *ctrl = data->ctrl;
> +	int ret;
> +
> +	/*
> +	 * The event stays latched on the controller until the SMART log
> +	 * is read with RAE cleared, so re-read it here before the
> +	 * completion path re-arms the Asynchronous Event Request.
> +	 */
> +	mutex_lock(&data->read_lock);
> +	ret = nvme_hwmon_get_smart_log(data);
> +	mutex_unlock(&data->read_lock);
> +	if (ret)
> +		return;
> +
> +	hwmon_notify_event(ctrl->hwmon_device, hwmon_temp,
> +			   hwmon_temp_alarm, 0);
> +}

[Severity: High]
Does queueing both data->smart_work and ctrl->async_event_work on nvme_wq
actually guarantee that the SMART log is read before the Asynchronous
Event Request is re-armed?

nvme_wq is allocated with WQ_UNBOUND and default max_active, so both work
items run concurrently:

CPU0                                  CPU1
nvme_complete_async_event()
  nvme_hwmon_smart_event(ctrl)
    queue_work(nvme_wq, &smart_work)
  queue_work(nvme_wq, &async_event_work)

nvme_hwmon_smart_notify()             nvme_async_event_work()
  nvme_hwmon_get_smart_log()            ctrl->ops->submit_async_event(ctrl)
    ... (sleeps on admin I/O)             <-- re-arms AER before RAE clear

Because submit_async_event() runs before the synchronous Get Log Page
command completes and clears the latched event, the controller can
immediately complete the new AER for the same latched SMART warning,
causing repeated AER completions and duplicate uevent and hwmon
notifications.  By comparison, NVME_AER_NOTICE_FW_ACT_STARTING sets
requeue = false in nvme_handle_aen_notice() and queues
ctrl->async_event_work from nvme_fw_act_work() only after reading the log
page.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110110737.3965373-1-kanie@linux.alibaba.com?part=1