[PATCH] drm/dp: Fix Write_Status_Update_Request AUX request format

[Mario Limonciello]

On 4/27/2025 4:50 AM, Wayne Lin wrote:
> [Why]
> Notice AUX request format of I2C-over-AUX with
> Write_Status_Update_Request flag set is incorrect. It should
> be address only request without length and data like:
> "SYNC->COM3:0 (= 0110)|0000-> 0000|0000->
> 0|7-bit I2C address (the same as the last)-> STOP->".
> 
> [How]
> Refer to DP v2.1 Table 2-178, correct the
> Write_Status_Update_Request to be address only request.
> 
> Note that we might receive 0 returned by aux->transfer() when
> receive reply I2C_ACK|AUX_ACK of Write_Status_Update_Request
> transaction. Which indicating all data bytes get written.
> We should avoid to return 0 bytes get transferred under this
> case.
> 
> Fixes: 68ec2a2a2481 ("drm/dp: Use I2C_WRITE_STATUS_UPDATE to drain partial I2C_WRITE requests")
> Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> Cc: Jani Nikula <jani.nikula at intel.com>
> Cc: Mario Limonciello <mario.limonciello at amd.com>
> Cc: Harry Wentland <harry.wentland at amd.com>
> Cc: stable at vger.kernel.org
> Signed-off-by: Wayne Lin <Wayne.Lin at amd.com>
> ---
>   drivers/gpu/drm/display/drm_dp_helper.c | 45 +++++++++++++++++++++----
>   1 file changed, 38 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/display/drm_dp_helper.c b/drivers/gpu/drm/display/drm_dp_helper.c
> index 57828f2b7b5a..0c8cba7ed875 100644
> --- a/drivers/gpu/drm/display/drm_dp_helper.c
> +++ b/drivers/gpu/drm/display/drm_dp_helper.c
> @@ -1857,6 +1857,12 @@ static u32 drm_dp_i2c_functionality(struct i2c_adapter *adapter)
>   	       I2C_FUNC_10BIT_ADDR;
>   }
>   
> +static inline bool
> +drm_dp_i2c_msg_is_write_status_update(struct drm_dp_aux_msg *msg)
> +{
> +	return ((msg->request & ~DP_AUX_I2C_MOT) == DP_AUX_I2C_WRITE_STATUS_UPDATE);
> +}
> +
>   static void drm_dp_i2c_msg_write_status_update(struct drm_dp_aux_msg *msg)
>   {
>   	/*
> @@ -1965,6 +1971,7 @@ MODULE_PARM_DESC(dp_aux_i2c_speed_khz,
>   static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
>   {
>   	unsigned int retry, defer_i2c;
> +	struct drm_dp_aux_msg orig_msg = *msg;
>   	int ret;
>   	/*
>   	 * DP1.2 sections 2.7.7.1.5.6.1 and 2.7.7.1.6.6.1: A DP Source device
> @@ -1976,6 +1983,12 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
>   	int max_retries = max(7, drm_dp_i2c_retry_count(msg, dp_aux_i2c_speed_khz));
>   
>   	for (retry = 0, defer_i2c = 0; retry < (max_retries + defer_i2c); retry++) {
> +		if (drm_dp_i2c_msg_is_write_status_update(msg)) {
> +			/* Address only transaction */
> +			msg->buffer = NULL;
> +			msg->size = 0;
> +		}
> +
>   		ret = aux->transfer(aux, msg);
>   		if (ret < 0) {
>   			if (ret == -EBUSY)
> @@ -1993,7 +2006,7 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
>   			else
>   				drm_dbg_kms(aux->drm_dev, "%s: transaction failed: %d\n",
>   					    aux->name, ret);
> -			return ret;
> +			goto out;
>   		}
>   
>   
> @@ -2008,7 +2021,8 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
>   		case DP_AUX_NATIVE_REPLY_NACK:
>   			drm_dbg_kms(aux->drm_dev, "%s: native nack (result=%d, size=%zu)\n",
>   				    aux->name, ret, msg->size);
> -			return -EREMOTEIO;
> +			ret = -EREMOTEIO;
> +			goto out;
>   
>   		case DP_AUX_NATIVE_REPLY_DEFER:
>   			drm_dbg_kms(aux->drm_dev, "%s: native defer\n", aux->name);
> @@ -2027,24 +2041,35 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
>   		default:
>   			drm_err(aux->drm_dev, "%s: invalid native reply %#04x\n",
>   				aux->name, msg->reply);
> -			return -EREMOTEIO;
> +			ret = -EREMOTEIO;
> +			goto out;
>   		}
>   
>   		switch (msg->reply & DP_AUX_I2C_REPLY_MASK) {
>   		case DP_AUX_I2C_REPLY_ACK:
> +			/*
> +			 * When I2C write firstly get defer and get ack after
> +			 * retries by wirte_status_update, we have to return
> +			 * all data bytes get transferred instead of 0.
> +			 */
> +			if (drm_dp_i2c_msg_is_write_status_update(msg) && ret == 0)
> +				ret = orig_msg.size;
> +
>   			/*
>   			 * Both native ACK and I2C ACK replies received. We
>   			 * can assume the transfer was successful.
>   			 */
>   			if (ret != msg->size)
>   				drm_dp_i2c_msg_write_status_update(msg);
> -			return ret;
> +
> +			goto out;
>   
>   		case DP_AUX_I2C_REPLY_NACK:
>   			drm_dbg_kms(aux->drm_dev, "%s: I2C nack (result=%d, size=%zu)\n",
>   				    aux->name, ret, msg->size);
>   			aux->i2c_nack_count++;
> -			return -EREMOTEIO;
> +			ret = -EREMOTEIO;
> +			goto out;
>   
>   		case DP_AUX_I2C_REPLY_DEFER:
>   			drm_dbg_kms(aux->drm_dev, "%s: I2C defer\n", aux->name);
> @@ -2063,12 +2088,18 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
>   		default:
>   			drm_err(aux->drm_dev, "%s: invalid I2C reply %#04x\n",
>   				aux->name, msg->reply);
> -			return -EREMOTEIO;
> +			ret = -EREMOTEIO;
> +			goto out;
>   		}
>   	}
>   
>   	drm_dbg_kms(aux->drm_dev, "%s: Too many retries, giving up\n", aux->name);
> -	return -EREMOTEIO;
> +	ret = -EREMOTEIO;
> +out:
> +	/* In case we change original msg by Write_Status_Update*/

As there are multiple cases that jump to the "out" label, would it be 
clearer to use:

if (drm_dp_i2c_msg_is_write_status_update(msg)) {
   	msg->buffer = orig_msg.buffer;
   	msg->size = orig_msg.size;
}

return ret;

> +	msg->buffer = orig_msg.buffer;
> +	msg->size = orig_msg.size;
> +	return ret;
>   }
>   
>   static void drm_dp_i2c_msg_set_request(struct drm_dp_aux_msg *msg,