Several ibmpex request helpers ignore the return value of
ibmpex_send_message() and then unconditionally wait for
read_complete.

If transmit fails, no response will arrive and the code may wait
indefinitely. Check ibmpex_send_message() return values and
propagate errors to callers instead of continuing.

Also propagate reset command failures through
ibmpex_high_low_store().

Signed-off-by: Jaime Saguillo Revilla <jaime.saguillo@gmail.com>
---
 drivers/hwmon/ibmpex.c | 35 +++++++++++++++++++++++++++++------
 1 file changed, 29 insertions(+), 6 deletions(-)

diff --git a/drivers/hwmon/ibmpex.c b/drivers/hwmon/ibmpex.c
index dec730798d58..92cda205e57c 100644
--- a/drivers/hwmon/ibmpex.c
+++ b/drivers/hwmon/ibmpex.c
@@ -133,9 +133,13 @@ static int ibmpex_send_message(struct ibmpex_bmc_data *data)
 
 static int ibmpex_ver_check(struct ibmpex_bmc_data *data)
 {
+	int err;
+
 	data->tx_msg_data[0] = PEX_GET_VERSION;
 	data->tx_message.data_len = 1;
-	ibmpex_send_message(data);
+	err = ibmpex_send_message(data);
+	if (err)
+		return err;
 
 	wait_for_completion(&data->read_complete);
 
@@ -159,9 +163,13 @@ static int ibmpex_ver_check(struct ibmpex_bmc_data *data)
 
 static int ibmpex_query_sensor_count(struct ibmpex_bmc_data *data)
 {
+	int err;
+
 	data->tx_msg_data[0] = PEX_GET_SENSOR_COUNT;
 	data->tx_message.data_len = 1;
-	ibmpex_send_message(data);
+	err = ibmpex_send_message(data);
+	if (err)
+		return err;
 
 	wait_for_completion(&data->read_complete);
 
@@ -173,10 +181,14 @@ static int ibmpex_query_sensor_count(struct ibmpex_bmc_data *data)
 
 static int ibmpex_query_sensor_name(struct ibmpex_bmc_data *data, int sensor)
 {
+	int err;
+
 	data->tx_msg_data[0] = PEX_GET_SENSOR_NAME;
 	data->tx_msg_data[1] = sensor;
 	data->tx_message.data_len = 2;
-	ibmpex_send_message(data);
+	err = ibmpex_send_message(data);
+	if (err)
+		return err;
 
 	wait_for_completion(&data->read_complete);
 
@@ -188,10 +200,14 @@ static int ibmpex_query_sensor_name(struct ibmpex_bmc_data *data, int sensor)
 
 static int ibmpex_query_sensor_data(struct ibmpex_bmc_data *data, int sensor)
 {
+	int err;
+
 	data->tx_msg_data[0] = PEX_GET_SENSOR_DATA;
 	data->tx_msg_data[1] = sensor;
 	data->tx_message.data_len = 2;
-	ibmpex_send_message(data);
+	err = ibmpex_send_message(data);
+	if (err)
+		return err;
 
 	wait_for_completion(&data->read_complete);
 
@@ -206,9 +222,13 @@ static int ibmpex_query_sensor_data(struct ibmpex_bmc_data *data, int sensor)
 
 static int ibmpex_reset_high_low_data(struct ibmpex_bmc_data *data)
 {
+	int err;
+
 	data->tx_msg_data[0] = PEX_RESET_HIGH_LOW;
 	data->tx_message.data_len = 1;
-	ibmpex_send_message(data);
+	err = ibmpex_send_message(data);
+	if (err)
+		return err;
 
 	wait_for_completion(&data->read_complete);
 
@@ -276,8 +296,11 @@ static ssize_t ibmpex_high_low_store(struct device *dev,
 				     const char *buf, size_t count)
 {
 	struct ibmpex_bmc_data *data = dev_get_drvdata(dev);
+	int err;
 
-	ibmpex_reset_high_low_data(data);
+	err = ibmpex_reset_high_low_data(data);
+	if (err)
+		return err;
 
 	return count;
 }
-- 
2.34.1

On Tue, Mar 10, 2026 at 08:35:55PM +0000, Jaime Saguillo Revilla wrote:
> Several ibmpex request helpers ignore the return value of
> ibmpex_send_message() and then unconditionally wait for
> read_complete.
> 
> If transmit fails, no response will arrive and the code may wait
> indefinitely. Check ibmpex_send_message() return values and
> propagate errors to callers instead of continuing.
> 
> Also propagate reset command failures through
> ibmpex_high_low_store().
> 
> Signed-off-by: Jaime Saguillo Revilla <jaime.saguillo@gmail.com>

Is that an actual problem ? This driver has a variety of problems,
starting with its use of a long since deprecated ABI.
Checking those return values is a minor issus. Also see inline comments
(from AI review).

> ---
>  drivers/hwmon/ibmpex.c | 35 +++++++++++++++++++++++++++++------
>  1 file changed, 29 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/hwmon/ibmpex.c b/drivers/hwmon/ibmpex.c
> index dec730798d58..92cda205e57c 100644
> --- a/drivers/hwmon/ibmpex.c
> +++ b/drivers/hwmon/ibmpex.c
> @@ -133,9 +133,13 @@ static int ibmpex_send_message(struct ibmpex_bmc_data *data)
>  
>  static int ibmpex_ver_check(struct ibmpex_bmc_data *data)
>  {
> +	int err;
> +
>  	data->tx_msg_data[0] = PEX_GET_VERSION;
>  	data->tx_message.data_len = 1;
> -	ibmpex_send_message(data);
> +	err = ibmpex_send_message(data);
> +	if (err)
> +		return err;
>  
>  	wait_for_completion(&data->read_complete);
>  
> @@ -159,9 +163,13 @@ static int ibmpex_ver_check(struct ibmpex_bmc_data *data)
>  
>  static int ibmpex_query_sensor_count(struct ibmpex_bmc_data *data)
>  {
> +	int err;
> +
>  	data->tx_msg_data[0] = PEX_GET_SENSOR_COUNT;
>  	data->tx_message.data_len = 1;
> -	ibmpex_send_message(data);
> +	err = ibmpex_send_message(data);
> +	if (err)
> +		return err;
>  
>  	wait_for_completion(&data->read_complete);
>  
> @@ -173,10 +181,14 @@ static int ibmpex_query_sensor_count(struct ibmpex_bmc_data *data)
>  
>  static int ibmpex_query_sensor_name(struct ibmpex_bmc_data *data, int sensor)
>  {
> +	int err;
> +
>  	data->tx_msg_data[0] = PEX_GET_SENSOR_NAME;
>  	data->tx_msg_data[1] = sensor;
>  	data->tx_message.data_len = 2;
> -	ibmpex_send_message(data);
> +	err = ibmpex_send_message(data);
> +	if (err)
> +		return err;
>  
>  	wait_for_completion(&data->read_complete);
>  
> @@ -188,10 +200,14 @@ static int ibmpex_query_sensor_name(struct ibmpex_bmc_data *data, int sensor)
>  
>  static int ibmpex_query_sensor_data(struct ibmpex_bmc_data *data, int sensor)
>  {
> +	int err;
> +
>  	data->tx_msg_data[0] = PEX_GET_SENSOR_DATA;
>  	data->tx_msg_data[1] = sensor;
>  	data->tx_message.data_len = 2;
> -	ibmpex_send_message(data);
> +	err = ibmpex_send_message(data);
> +	if (err)
> +		return err;
>  
>  	wait_for_completion(&data->read_complete);
>  
> @@ -206,9 +222,13 @@ static int ibmpex_query_sensor_data(struct ibmpex_bmc_data *data, int sensor)
>  
>  static int ibmpex_reset_high_low_data(struct ibmpex_bmc_data *data)
>  {
> +	int err;
> +
>  	data->tx_msg_data[0] = PEX_RESET_HIGH_LOW;
>  	data->tx_message.data_len = 1;
> -	ibmpex_send_message(data);
> +	err = ibmpex_send_message(data);
> +	if (err)
> +		return err;
>  
>  	wait_for_completion(&data->read_complete);

Should we also check data->rx_result here?
The commit message mentions propagating reset command failures through
ibmpex_high_low_store. While the local transmission error is caught by
checking the return value of ibmpex_send_message, if the BMC rejects or
fails the reset command, data->rx_result will contain a non-zero error
code.
Since data->rx_result is ignored here, the function will still return
success to userspace, which seems to result in incomplete error propagation.

>  
> @@ -276,8 +296,11 @@ static ssize_t ibmpex_high_low_store(struct device *dev,
>  				     const char *buf, size_t count)
>  {
>  	struct ibmpex_bmc_data *data = dev_get_drvdata(dev);
> +	int err;
>  
> -	ibmpex_reset_high_low_data(data);
> +	err = ibmpex_reset_high_low_data(data);
> +	if (err)
> +		return err;

Does this sysfs store function need to acquire the data->lock mutex?
ibmpex_reset_high_low_data writes directly to the shared message buffers
data->tx_msg_data and data->tx_message.data_len, and ibmpex_send_message
increments data->tx_msgid.
If sysfs operations run concurrently, this could corrupt the transmitted
messages. Furthermore, if data->tx_msgid is incremented concurrently before
the BMC responds, the IPMI response handler might silently drop the
response due to a message ID mismatch. This would skip the completion call
and cause the thread to hang indefinitely.
Other functions like ibmpex_update_device appear to protect these shared
resources by holding mutex_lock(&data->lock).

>  
>  	return count;
>  }
> -- 
> 2.34.1
>