Re: [PATCH] hwmon: (occ) Prevent power cap command overwriting poll response

[Guenter Roeck]

On Tue, Jun 28, 2022 at 03:30:29PM -0500, Eddie James wrote:
> Currently, the response to the power cap command overwrites the
> first eight bytes of the poll response, since the commands use
> the same buffer. This means that user's get the wrong data between
> the time of sending the power cap and the next poll response update.
> Fix this by specifying a different buffer for the power cap command
> response.
> 
> Fixes: 5b5513b88002 ("hwmon: Add On-Chip Controller (OCC) hwmon driver")
> Signed-off-by: Eddie James <eajames@xxxxxxxxxxxxx>

Applied.

Thanks,
Guenter

> ---
>  drivers/hwmon/occ/common.c |  5 +++--
>  drivers/hwmon/occ/common.h |  3 ++-
>  drivers/hwmon/occ/p8_i2c.c | 13 +++++++------
>  drivers/hwmon/occ/p9_sbe.c |  7 +++----
>  4 files changed, 15 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/hwmon/occ/common.c b/drivers/hwmon/occ/common.c
> index ea070b91e5b9..157b73a3da29 100644
> --- a/drivers/hwmon/occ/common.c
> +++ b/drivers/hwmon/occ/common.c
> @@ -145,7 +145,7 @@ static int occ_poll(struct occ *occ)
>  	cmd[6] = 0;			/* checksum lsb */
>  
>  	/* mutex should already be locked if necessary */
> -	rc = occ->send_cmd(occ, cmd, sizeof(cmd));
> +	rc = occ->send_cmd(occ, cmd, sizeof(cmd), &occ->resp, sizeof(occ->resp));
>  	if (rc) {
>  		occ->last_error = rc;
>  		if (occ->error_count++ > OCC_ERROR_COUNT_THRESHOLD)
> @@ -182,6 +182,7 @@ static int occ_set_user_power_cap(struct occ *occ, u16 user_power_cap)
>  {
>  	int rc;
>  	u8 cmd[8];
> +	u8 resp[8];
>  	__be16 user_power_cap_be = cpu_to_be16(user_power_cap);
>  
>  	cmd[0] = 0;	/* sequence number */
> @@ -198,7 +199,7 @@ static int occ_set_user_power_cap(struct occ *occ, u16 user_power_cap)
>  	if (rc)
>  		return rc;
>  
> -	rc = occ->send_cmd(occ, cmd, sizeof(cmd));
> +	rc = occ->send_cmd(occ, cmd, sizeof(cmd), resp, sizeof(resp));
>  
>  	mutex_unlock(&occ->lock);
>  
> diff --git a/drivers/hwmon/occ/common.h b/drivers/hwmon/occ/common.h
> index 64d5ec7e169b..7ac4b2febce6 100644
> --- a/drivers/hwmon/occ/common.h
> +++ b/drivers/hwmon/occ/common.h
> @@ -96,7 +96,8 @@ struct occ {
>  
>  	int powr_sample_time_us;	/* average power sample time */
>  	u8 poll_cmd_data;		/* to perform OCC poll command */
> -	int (*send_cmd)(struct occ *occ, u8 *cmd, size_t len);
> +	int (*send_cmd)(struct occ *occ, u8 *cmd, size_t len, void *resp,
> +			size_t resp_len);
>  
>  	unsigned long next_update;
>  	struct mutex lock;		/* lock OCC access */
> diff --git a/drivers/hwmon/occ/p8_i2c.c b/drivers/hwmon/occ/p8_i2c.c
> index da39ea28df31..b221be1f35f3 100644
> --- a/drivers/hwmon/occ/p8_i2c.c
> +++ b/drivers/hwmon/occ/p8_i2c.c
> @@ -111,7 +111,8 @@ static int p8_i2c_occ_putscom_be(struct i2c_client *client, u32 address,
>  				      be32_to_cpu(data1));
>  }
>  
> -static int p8_i2c_occ_send_cmd(struct occ *occ, u8 *cmd, size_t len)
> +static int p8_i2c_occ_send_cmd(struct occ *occ, u8 *cmd, size_t len,
> +			       void *resp, size_t resp_len)
>  {
>  	int i, rc;
>  	unsigned long start;
> @@ -120,7 +121,7 @@ static int p8_i2c_occ_send_cmd(struct occ *occ, u8 *cmd, size_t len)
>  	const long wait_time = msecs_to_jiffies(OCC_CMD_IN_PRG_WAIT_MS);
>  	struct p8_i2c_occ *ctx = to_p8_i2c_occ(occ);
>  	struct i2c_client *client = ctx->client;
> -	struct occ_response *resp = &occ->resp;
> +	struct occ_response *or = (struct occ_response *)resp;
>  
>  	start = jiffies;
>  
> @@ -151,7 +152,7 @@ static int p8_i2c_occ_send_cmd(struct occ *occ, u8 *cmd, size_t len)
>  			return rc;
>  
>  		/* wait for OCC */
> -		if (resp->return_status == OCC_RESP_CMD_IN_PRG) {
> +		if (or->return_status == OCC_RESP_CMD_IN_PRG) {
>  			rc = -EALREADY;
>  
>  			if (time_after(jiffies, start + timeout))
> @@ -163,7 +164,7 @@ static int p8_i2c_occ_send_cmd(struct occ *occ, u8 *cmd, size_t len)
>  	} while (rc);
>  
>  	/* check the OCC response */
> -	switch (resp->return_status) {
> +	switch (or->return_status) {
>  	case OCC_RESP_CMD_IN_PRG:
>  		rc = -ETIMEDOUT;
>  		break;
> @@ -192,8 +193,8 @@ static int p8_i2c_occ_send_cmd(struct occ *occ, u8 *cmd, size_t len)
>  	if (rc < 0)
>  		return rc;
>  
> -	data_length = get_unaligned_be16(&resp->data_length);
> -	if (data_length > OCC_RESP_DATA_BYTES)
> +	data_length = get_unaligned_be16(&or->data_length);
> +	if ((data_length + 7) > resp_len)
>  		return -EMSGSIZE;
>  
>  	/* fetch the rest of the response data */
> diff --git a/drivers/hwmon/occ/p9_sbe.c b/drivers/hwmon/occ/p9_sbe.c
> index 01405ae2f9bd..c1e0a1d96cd4 100644
> --- a/drivers/hwmon/occ/p9_sbe.c
> +++ b/drivers/hwmon/occ/p9_sbe.c
> @@ -77,11 +77,10 @@ static bool p9_sbe_occ_save_ffdc(struct p9_sbe_occ *ctx, const void *resp,
>  	return notify;
>  }
>  
> -static int p9_sbe_occ_send_cmd(struct occ *occ, u8 *cmd, size_t len)
> +static int p9_sbe_occ_send_cmd(struct occ *occ, u8 *cmd, size_t len,
> +			       void *resp, size_t resp_len)
>  {
> -	struct occ_response *resp = &occ->resp;
>  	struct p9_sbe_occ *ctx = to_p9_sbe_occ(occ);
> -	size_t resp_len = sizeof(*resp);
>  	int rc;
>  
>  	rc = fsi_occ_submit(ctx->sbe, cmd, len, resp, &resp_len);
> @@ -95,7 +94,7 @@ static int p9_sbe_occ_send_cmd(struct occ *occ, u8 *cmd, size_t len)
>  		return rc;
>  	}
>  
> -	switch (resp->return_status) {
> +	switch (((struct occ_response *)resp)->return_status) {
>  	case OCC_RESP_CMD_IN_PRG:
>  		rc = -ETIMEDOUT;
>  		break;