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

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

[Wayne Lin]

[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@linux.intel.com>
Cc: Jani Nikula <jani.nikula@intel.com>
Cc: Mario Limonciello <mario.limonciello@amd.com>
Cc: Harry Wentland <harry.wentland@amd.com>
Cc: stable@vger.kernel.org
Signed-off-by: Wayne Lin <Wayne.Lin@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*/
+	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,
-- 
2.43.0

Re: [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@linux.intel.com>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: Mario Limonciello <mario.limonciello@amd.com>
> Cc: Harry Wentland <harry.wentland@amd.com>
> Cc: stable@vger.kernel.org
> Signed-off-by: Wayne Lin <Wayne.Lin@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,

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

[Lin, Wayne]

[AMD Official Use Only - AMD Internal Distribution Only]

Thanks for the feedback! I'll adjust it and give v2 soon.

Thanks,
Wayne

> -----Original Message-----
> From: Limonciello, Mario <Mario.Limonciello@amd.com>
> Sent: Tuesday, May 6, 2025 4:22 AM
> To: Lin, Wayne <Wayne.Lin@amd.com>; dri-devel@lists.freedesktop.org
> Cc: ville.syrjala@linux.intel.com; jani.nikula@intel.com; Wentland, Harry
> <Harry.Wentland@amd.com>; stable@vger.kernel.org
> Subject: Re: [PATCH] drm/dp: Fix Write_Status_Update_Request AUX request
> format
>
> 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@linux.intel.com>
> > Cc: Jani Nikula <jani.nikula@intel.com>
> > Cc: Mario Limonciello <mario.limonciello@amd.com>
> > Cc: Harry Wentland <harry.wentland@amd.com>
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Wayne Lin <Wayne.Lin@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,

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

[Jani Nikula]

On Sun, 27 Apr 2025, Wayne Lin <Wayne.Lin@amd.com> wrote:
> +			/*
> +			 * 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.
> +			 */

My brain gives me syntax and parse error here. ;)

BR,
Jani.

-- 
Jani Nikula, Intel

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

[Lin, Wayne]

[Public]

> -----Original Message-----
> From: Jani Nikula <jani.nikula@intel.com>
> Sent: Thursday, May 8, 2025 4:19 PM
> To: Lin, Wayne <Wayne.Lin@amd.com>; dri-devel@lists.freedesktop.org
> Cc: ville.syrjala@linux.intel.com; Limonciello, Mario <Mario.Limonciello@amd.com>;
> Wentland, Harry <Harry.Wentland@amd.com>; Lin, Wayne
> <Wayne.Lin@amd.com>; stable@vger.kernel.org
> Subject: Re: [PATCH] drm/dp: Fix Write_Status_Update_Request AUX request
> format
>
> On Sun, 27 Apr 2025, Wayne Lin <Wayne.Lin@amd.com> wrote:
> > +                   /*
> > +                    * 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.
> > +                    */
>
> My brain gives me syntax and parse error here. ;)

Appreciate for the feedback, Jani.
Could you elaborate more on your concerns please?

Since Write_Status_Update_Request is address only request. Data length
is 0. When I2C write request completes, reply for
Write_Status_Update_Request from DPRx will be ACK only (i.e. data
length is 0).

Is your concern about returning 0 from aux->transfer?
My thoughts is drm_dp_i2c_do_msg() is designed to handle I2C-Over-Aux
reply data, and aux->transfer() is handling hw specific manipulation and
return transferred bytes. For Write_Status_Update_Request request itself,
nothing new to be transferred. I think drm_dp_i2c_do_msg() should be
responsible for determining the correct transferred data bytes under this
case. Or do you expect aux->transfer to memorize the data length of
write request?

Thanks!
>
> BR,
> Jani.
>
> --
> Jani Nikula, Intel
--
Wayne Lin

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

[Jani Nikula]

On Thu, 08 May 2025, "Lin, Wayne" <Wayne.Lin@amd.com> wrote:
> [Public]
>
>> -----Original Message-----
>> From: Jani Nikula <jani.nikula@intel.com>
>> Sent: Thursday, May 8, 2025 4:19 PM
>> To: Lin, Wayne <Wayne.Lin@amd.com>; dri-devel@lists.freedesktop.org
>> Cc: ville.syrjala@linux.intel.com; Limonciello, Mario <Mario.Limonciello@amd.com>;
>> Wentland, Harry <Harry.Wentland@amd.com>; Lin, Wayne
>> <Wayne.Lin@amd.com>; stable@vger.kernel.org
>> Subject: Re: [PATCH] drm/dp: Fix Write_Status_Update_Request AUX request
>> format
>>
>> On Sun, 27 Apr 2025, Wayne Lin <Wayne.Lin@amd.com> wrote:
>> > +                   /*
>> > +                    * 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.
>> > +                    */
>>
>> My brain gives me syntax and parse error here. ;)
>
> Appreciate for the feedback, Jani.
> Could you elaborate more on your concerns please?
>
> Since Write_Status_Update_Request is address only request. Data length
> is 0. When I2C write request completes, reply for
> Write_Status_Update_Request from DPRx will be ACK only (i.e. data
> length is 0).
>
> Is your concern about returning 0 from aux->transfer?
> My thoughts is drm_dp_i2c_do_msg() is designed to handle I2C-Over-Aux
> reply data, and aux->transfer() is handling hw specific manipulation and
> return transferred bytes. For Write_Status_Update_Request request itself,
> nothing new to be transferred. I think drm_dp_i2c_do_msg() should be
> responsible for determining the correct transferred data bytes under this
> case. Or do you expect aux->transfer to memorize the data length of
> write request?

My concern is that I don't understand what the comment is trying to say.

"when i2c write firstly get defer" - what does it mean?

"wirte_status_update" - typo

"we have to" - why?

"return all data bytes get transferred" - what does it mean?

>
> Thanks!
>>
>> BR,
>> Jani.
>>
>> --
>> Jani Nikula, Intel
> --
> Wayne Lin

-- 
Jani Nikula, Intel

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

[Lin, Wayne]

[Public]

> -----Original Message-----
> From: Jani Nikula <jani.nikula@intel.com>
> Sent: Thursday, May 8, 2025 8:16 PM
> To: Lin, Wayne <Wayne.Lin@amd.com>; dri-devel@lists.freedesktop.org
> Cc: ville.syrjala@linux.intel.com; Limonciello, Mario <Mario.Limonciello@amd.com>;
> Wentland, Harry <Harry.Wentland@amd.com>; stable@vger.kernel.org
> Subject: RE: [PATCH] drm/dp: Fix Write_Status_Update_Request AUX request
> format
>
> On Thu, 08 May 2025, "Lin, Wayne" <Wayne.Lin@amd.com> wrote:
> > [Public]
> >
> >> -----Original Message-----
> >> From: Jani Nikula <jani.nikula@intel.com>
> >> Sent: Thursday, May 8, 2025 4:19 PM
> >> To: Lin, Wayne <Wayne.Lin@amd.com>; dri-devel@lists.freedesktop.org
> >> Cc: ville.syrjala@linux.intel.com; Limonciello, Mario
> >> <Mario.Limonciello@amd.com>; Wentland, Harry
> >> <Harry.Wentland@amd.com>; Lin, Wayne <Wayne.Lin@amd.com>;
> >> stable@vger.kernel.org
> >> Subject: Re: [PATCH] drm/dp: Fix Write_Status_Update_Request AUX
> >> request format
> >>
> >> On Sun, 27 Apr 2025, Wayne Lin <Wayne.Lin@amd.com> wrote:
> >> > +                   /*
> >> > +                    * 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.
> >> > +                    */
> >>
> >> My brain gives me syntax and parse error here. ;)
> >
> > Appreciate for the feedback, Jani.
> > Could you elaborate more on your concerns please?
> >
> > Since Write_Status_Update_Request is address only request. Data length
> > is 0. When I2C write request completes, reply for
> > Write_Status_Update_Request from DPRx will be ACK only (i.e. data
> > length is 0).
> >
> > Is your concern about returning 0 from aux->transfer?
> > My thoughts is drm_dp_i2c_do_msg() is designed to handle I2C-Over-Aux
> > reply data, and aux->transfer() is handling hw specific manipulation
> > and return transferred bytes. For Write_Status_Update_Request request
> > itself, nothing new to be transferred. I think drm_dp_i2c_do_msg()
> > should be responsible for determining the correct transferred data
> > bytes under this case. Or do you expect aux->transfer to memorize the
> > data length of write request?
>
> My concern is that I don't understand what the comment is trying to say.
>
> "when i2c write firstly get defer" - what does it mean?
>
> "wirte_status_update" - typo
>
> "we have to" - why?
>
> "return all data bytes get transferred" - what does it mean?

I see. Thanks!
We can't reply 0 since drm_dp_i2c_drain_msg() take 0 returned from
drm_dp_i2c_do_msg() as error. And it actually completes transferring all
data bytes.

I'll refine the comment. Thanks again.

>
> >
> > Thanks!
> >>
> >> BR,
> >> Jani.
> >>
> >> --
> >> Jani Nikula, Intel
> > --
> > Wayne Lin
>
> --
> Jani Nikula, Intel

--
Wayne Lin