Re: [PATCH] hwmon: ibmpex: check ibmpex_send_message() return value

[Guenter Roeck]

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@xxxxxxxxx>

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
>