diff mbox series

[v2] usb: typec: tcpm: fix warning when handle discover_identity message

Message ID 20230216031515.4151117-1-xu.yang_2@nxp.com (mailing list archive)
State Accepted
Commit abfc4fa28f0160df61c7149567da4f6494dfb488
Headers show
Series [v2] usb: typec: tcpm: fix warning when handle discover_identity message | expand

Commit Message

Xu Yang Feb. 16, 2023, 3:15 a.m. UTC 
Since both source and sink device can send discover_identity message in
PD3, kernel may dump below warning:

------------[ cut here ]------------
WARNING: CPU: 0 PID: 169 at drivers/usb/typec/tcpm/tcpm.c:1446 tcpm_queue_vdm+0xe0/0xf0
Modules linked in:
CPU: 0 PID: 169 Comm: 1-0050 Not tainted 6.1.1-00038-g6a3c36cf1da2-dirty #567
Hardware name: NXP i.MX8MPlus EVK board (DT)
pstate: 20000005 (nzCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
pc : tcpm_queue_vdm+0xe0/0xf0
lr : tcpm_queue_vdm+0x2c/0xf0
sp : ffff80000c19bcd0
x29: ffff80000c19bcd0 x28: 0000000000000001 x27: ffff0000d11c8ab8
x26: ffff0000d11cc000 x25: 0000000000000000 x24: 00000000ff008081
x23: 0000000000000001 x22: 00000000ff00a081 x21: ffff80000c19bdbc
x20: 0000000000000000 x19: ffff0000d11c8080 x18: ffffffffffffffff
x17: 0000000000000000 x16: 0000000000000000 x15: ffff0000d716f580
x14: 0000000000000001 x13: ffff0000d716f507 x12: 0000000000000001
x11: 0000000000000000 x10: 0000000000000020 x9 : 00000000000ee098
x8 : 00000000ffffffff x7 : 000000000000001c x6 : ffff0000d716f580
x5 : 0000000000000000 x4 : 0000000000000000 x3 : 0000000000000000
x2 : ffff80000c19bdbc x1 : 00000000ff00a081 x0 : 0000000000000004
Call trace:
tcpm_queue_vdm+0xe0/0xf0
tcpm_pd_rx_handler+0x340/0x1ab0
kthread_worker_fn+0xcc/0x18c
kthread+0x10c/0x110
ret_from_fork+0x10/0x20
---[ end trace 0000000000000000 ]---

Below sequences may trigger this warning:

tcpm_send_discover_work(work)
  tcpm_send_vdm(port, USB_SID_PD, CMD_DISCOVER_IDENT, NULL, 0);
   tcpm_queue_vdm(port, header, data, count);
    port->vdm_state = VDM_STATE_READY;

vdm_state_machine_work(work);
			<-- received discover_identity from partner
 vdm_run_state_machine(port);
  port->vdm_state = VDM_STATE_SEND_MESSAGE;
   mod_vdm_delayed_work(port, x);

tcpm_pd_rx_handler(work);
 tcpm_pd_data_request(port, msg);
  tcpm_handle_vdm_request(port, msg->payload, cnt);
   tcpm_queue_vdm(port, response[0], &response[1], rlen - 1);
--> WARN_ON(port->vdm_state > VDM_STATE_DONE);

For this case, the state machine could still send out discover
identity message later if we skip current discover_identity message.
So we should handle the received message firstly and override the pending
discover_identity message without warning in this case. Then, a delayed
send_discover work will send discover_identity message again.

Fixes: e00943e91678 ("usb: typec: tcpm: PD3.0 sinks can send Discover Identity even in device mode")
cc: <stable@vger.kernel.org>
Signed-off-by: Xu Yang <xu.yang_2@nxp.com>

---
Changelogs:
v2: modify some code format and commit message
---
 drivers/usb/typec/tcpm/tcpm.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

Comments

Guenter Roeck Feb. 16, 2023, 7:59 p.m. UTC | #1 
On 2/15/23 19:15, Xu Yang wrote:
> Since both source and sink device can send discover_identity message in
> PD3, kernel may dump below warning:
> 
> ------------[ cut here ]------------
> WARNING: CPU: 0 PID: 169 at drivers/usb/typec/tcpm/tcpm.c:1446 tcpm_queue_vdm+0xe0/0xf0
> Modules linked in:
> CPU: 0 PID: 169 Comm: 1-0050 Not tainted 6.1.1-00038-g6a3c36cf1da2-dirty #567
> Hardware name: NXP i.MX8MPlus EVK board (DT)
> pstate: 20000005 (nzCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> pc : tcpm_queue_vdm+0xe0/0xf0
> lr : tcpm_queue_vdm+0x2c/0xf0
> sp : ffff80000c19bcd0
> x29: ffff80000c19bcd0 x28: 0000000000000001 x27: ffff0000d11c8ab8
> x26: ffff0000d11cc000 x25: 0000000000000000 x24: 00000000ff008081
> x23: 0000000000000001 x22: 00000000ff00a081 x21: ffff80000c19bdbc
> x20: 0000000000000000 x19: ffff0000d11c8080 x18: ffffffffffffffff
> x17: 0000000000000000 x16: 0000000000000000 x15: ffff0000d716f580
> x14: 0000000000000001 x13: ffff0000d716f507 x12: 0000000000000001
> x11: 0000000000000000 x10: 0000000000000020 x9 : 00000000000ee098
> x8 : 00000000ffffffff x7 : 000000000000001c x6 : ffff0000d716f580
> x5 : 0000000000000000 x4 : 0000000000000000 x3 : 0000000000000000
> x2 : ffff80000c19bdbc x1 : 00000000ff00a081 x0 : 0000000000000004
> Call trace:
> tcpm_queue_vdm+0xe0/0xf0
> tcpm_pd_rx_handler+0x340/0x1ab0
> kthread_worker_fn+0xcc/0x18c
> kthread+0x10c/0x110
> ret_from_fork+0x10/0x20
> ---[ end trace 0000000000000000 ]---
> 
> Below sequences may trigger this warning:
> 
> tcpm_send_discover_work(work)
>    tcpm_send_vdm(port, USB_SID_PD, CMD_DISCOVER_IDENT, NULL, 0);
>     tcpm_queue_vdm(port, header, data, count);
>      port->vdm_state = VDM_STATE_READY;
> 
> vdm_state_machine_work(work);
> 			<-- received discover_identity from partner
>   vdm_run_state_machine(port);
>    port->vdm_state = VDM_STATE_SEND_MESSAGE;
>     mod_vdm_delayed_work(port, x);
> 
> tcpm_pd_rx_handler(work);
>   tcpm_pd_data_request(port, msg);
>    tcpm_handle_vdm_request(port, msg->payload, cnt);
>     tcpm_queue_vdm(port, response[0], &response[1], rlen - 1);
> --> WARN_ON(port->vdm_state > VDM_STATE_DONE);
> 
> For this case, the state machine could still send out discover
> identity message later if we skip current discover_identity message.
> So we should handle the received message firstly and override the pending
> discover_identity message without warning in this case. Then, a delayed
> send_discover work will send discover_identity message again.
> 
> Fixes: e00943e91678 ("usb: typec: tcpm: PD3.0 sinks can send Discover Identity even in device mode")
> cc: <stable@vger.kernel.org>
> Signed-off-by: Xu Yang <xu.yang_2@nxp.com>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> 
> ---
> Changelogs:
> v2: modify some code format and commit message
> ---
>   drivers/usb/typec/tcpm/tcpm.c | 19 +++++++++++++++----
>   1 file changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> index 7f39cb9b3429..1ee774c263f0 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -1445,10 +1445,18 @@ static int tcpm_ams_start(struct tcpm_port *port, enum tcpm_ams ams)
>   static void tcpm_queue_vdm(struct tcpm_port *port, const u32 header,
>   			   const u32 *data, int cnt)
>   {
> +	u32 vdo_hdr = port->vdo_data[0];
> +
>   	WARN_ON(!mutex_is_locked(&port->lock));
>   
> -	/* Make sure we are not still processing a previous VDM packet */
> -	WARN_ON(port->vdm_state > VDM_STATE_DONE);
> +	/* If is sending discover_identity, handle received message first */
> +	if (PD_VDO_SVDM(vdo_hdr) && PD_VDO_CMD(vdo_hdr) == CMD_DISCOVER_IDENT) {
> +		port->send_discover = true;
> +		mod_send_discover_delayed_work(port, SEND_DISCOVER_RETRY_MS);
> +	} else {
> +		/* Make sure we are not still processing a previous VDM packet */
> +		WARN_ON(port->vdm_state > VDM_STATE_DONE);
> +	}
>   
>   	port->vdo_count = cnt + 1;
>   	port->vdo_data[0] = header;
> @@ -1948,11 +1956,13 @@ static void vdm_run_state_machine(struct tcpm_port *port)
>   			switch (PD_VDO_CMD(vdo_hdr)) {
>   			case CMD_DISCOVER_IDENT:
>   				res = tcpm_ams_start(port, DISCOVER_IDENTITY);
> -				if (res == 0)
> +				if (res == 0) {
>   					port->send_discover = false;
> -				else if (res == -EAGAIN)
> +				} else if (res == -EAGAIN) {
> +					port->vdo_data[0] = 0;
>   					mod_send_discover_delayed_work(port,
>   								       SEND_DISCOVER_RETRY_MS);
> +				}
>   				break;
>   			case CMD_DISCOVER_SVID:
>   				res = tcpm_ams_start(port, DISCOVER_SVIDS);
> @@ -2035,6 +2045,7 @@ static void vdm_run_state_machine(struct tcpm_port *port)
>   			unsigned long timeout;
>   
>   			port->vdm_retries = 0;
> +			port->vdo_data[0] = 0;
>   			port->vdm_state = VDM_STATE_BUSY;
>   			timeout = vdm_ready_timeout(vdo_hdr);
>   			mod_vdm_delayed_work(port, timeout);
Heikki Krogerus Feb. 21, 2023, 1:36 p.m. UTC | #2 
On Thu, Feb 16, 2023 at 11:15:15AM +0800, Xu Yang wrote:
> Since both source and sink device can send discover_identity message in
> PD3, kernel may dump below warning:
> 
> ------------[ cut here ]------------
> WARNING: CPU: 0 PID: 169 at drivers/usb/typec/tcpm/tcpm.c:1446 tcpm_queue_vdm+0xe0/0xf0
> Modules linked in:
> CPU: 0 PID: 169 Comm: 1-0050 Not tainted 6.1.1-00038-g6a3c36cf1da2-dirty #567
> Hardware name: NXP i.MX8MPlus EVK board (DT)
> pstate: 20000005 (nzCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> pc : tcpm_queue_vdm+0xe0/0xf0
> lr : tcpm_queue_vdm+0x2c/0xf0
> sp : ffff80000c19bcd0
> x29: ffff80000c19bcd0 x28: 0000000000000001 x27: ffff0000d11c8ab8
> x26: ffff0000d11cc000 x25: 0000000000000000 x24: 00000000ff008081
> x23: 0000000000000001 x22: 00000000ff00a081 x21: ffff80000c19bdbc
> x20: 0000000000000000 x19: ffff0000d11c8080 x18: ffffffffffffffff
> x17: 0000000000000000 x16: 0000000000000000 x15: ffff0000d716f580
> x14: 0000000000000001 x13: ffff0000d716f507 x12: 0000000000000001
> x11: 0000000000000000 x10: 0000000000000020 x9 : 00000000000ee098
> x8 : 00000000ffffffff x7 : 000000000000001c x6 : ffff0000d716f580
> x5 : 0000000000000000 x4 : 0000000000000000 x3 : 0000000000000000
> x2 : ffff80000c19bdbc x1 : 00000000ff00a081 x0 : 0000000000000004
> Call trace:
> tcpm_queue_vdm+0xe0/0xf0
> tcpm_pd_rx_handler+0x340/0x1ab0
> kthread_worker_fn+0xcc/0x18c
> kthread+0x10c/0x110
> ret_from_fork+0x10/0x20
> ---[ end trace 0000000000000000 ]---
> 
> Below sequences may trigger this warning:
> 
> tcpm_send_discover_work(work)
>   tcpm_send_vdm(port, USB_SID_PD, CMD_DISCOVER_IDENT, NULL, 0);
>    tcpm_queue_vdm(port, header, data, count);
>     port->vdm_state = VDM_STATE_READY;
> 
> vdm_state_machine_work(work);
> 			<-- received discover_identity from partner
>  vdm_run_state_machine(port);
>   port->vdm_state = VDM_STATE_SEND_MESSAGE;
>    mod_vdm_delayed_work(port, x);
> 
> tcpm_pd_rx_handler(work);
>  tcpm_pd_data_request(port, msg);
>   tcpm_handle_vdm_request(port, msg->payload, cnt);
>    tcpm_queue_vdm(port, response[0], &response[1], rlen - 1);
> --> WARN_ON(port->vdm_state > VDM_STATE_DONE);
> 
> For this case, the state machine could still send out discover
> identity message later if we skip current discover_identity message.
> So we should handle the received message firstly and override the pending
> discover_identity message without warning in this case. Then, a delayed
> send_discover work will send discover_identity message again.
> 
> Fixes: e00943e91678 ("usb: typec: tcpm: PD3.0 sinks can send Discover Identity even in device mode")
> cc: <stable@vger.kernel.org>
> Signed-off-by: Xu Yang <xu.yang_2@nxp.com>

Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

> ---
> Changelogs:
> v2: modify some code format and commit message
> ---
>  drivers/usb/typec/tcpm/tcpm.c | 19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> index 7f39cb9b3429..1ee774c263f0 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -1445,10 +1445,18 @@ static int tcpm_ams_start(struct tcpm_port *port, enum tcpm_ams ams)
>  static void tcpm_queue_vdm(struct tcpm_port *port, const u32 header,
>  			   const u32 *data, int cnt)
>  {
> +	u32 vdo_hdr = port->vdo_data[0];
> +
>  	WARN_ON(!mutex_is_locked(&port->lock));
>  
> -	/* Make sure we are not still processing a previous VDM packet */
> -	WARN_ON(port->vdm_state > VDM_STATE_DONE);
> +	/* If is sending discover_identity, handle received message first */
> +	if (PD_VDO_SVDM(vdo_hdr) && PD_VDO_CMD(vdo_hdr) == CMD_DISCOVER_IDENT) {
> +		port->send_discover = true;
> +		mod_send_discover_delayed_work(port, SEND_DISCOVER_RETRY_MS);
> +	} else {
> +		/* Make sure we are not still processing a previous VDM packet */
> +		WARN_ON(port->vdm_state > VDM_STATE_DONE);
> +	}
>  
>  	port->vdo_count = cnt + 1;
>  	port->vdo_data[0] = header;
> @@ -1948,11 +1956,13 @@ static void vdm_run_state_machine(struct tcpm_port *port)
>  			switch (PD_VDO_CMD(vdo_hdr)) {
>  			case CMD_DISCOVER_IDENT:
>  				res = tcpm_ams_start(port, DISCOVER_IDENTITY);
> -				if (res == 0)
> +				if (res == 0) {
>  					port->send_discover = false;
> -				else if (res == -EAGAIN)
> +				} else if (res == -EAGAIN) {
> +					port->vdo_data[0] = 0;
>  					mod_send_discover_delayed_work(port,
>  								       SEND_DISCOVER_RETRY_MS);
> +				}
>  				break;
>  			case CMD_DISCOVER_SVID:
>  				res = tcpm_ams_start(port, DISCOVER_SVIDS);
> @@ -2035,6 +2045,7 @@ static void vdm_run_state_machine(struct tcpm_port *port)
>  			unsigned long timeout;
>  
>  			port->vdm_retries = 0;
> +			port->vdo_data[0] = 0;
>  			port->vdm_state = VDM_STATE_BUSY;
>  			timeout = vdm_ready_timeout(vdo_hdr);
>  			mod_vdm_delayed_work(port, timeout);
> -- 
> 2.34.1
diff mbox series

Patch

diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
index 7f39cb9b3429..1ee774c263f0 100644
--- a/drivers/usb/typec/tcpm/tcpm.c
+++ b/drivers/usb/typec/tcpm/tcpm.c
@@ -1445,10 +1445,18 @@  static int tcpm_ams_start(struct tcpm_port *port, enum tcpm_ams ams)
 static void tcpm_queue_vdm(struct tcpm_port *port, const u32 header,
 			   const u32 *data, int cnt)
 {
+	u32 vdo_hdr = port->vdo_data[0];
+
 	WARN_ON(!mutex_is_locked(&port->lock));
 
-	/* Make sure we are not still processing a previous VDM packet */
-	WARN_ON(port->vdm_state > VDM_STATE_DONE);
+	/* If is sending discover_identity, handle received message first */
+	if (PD_VDO_SVDM(vdo_hdr) && PD_VDO_CMD(vdo_hdr) == CMD_DISCOVER_IDENT) {
+		port->send_discover = true;
+		mod_send_discover_delayed_work(port, SEND_DISCOVER_RETRY_MS);
+	} else {
+		/* Make sure we are not still processing a previous VDM packet */
+		WARN_ON(port->vdm_state > VDM_STATE_DONE);
+	}
 
 	port->vdo_count = cnt + 1;
 	port->vdo_data[0] = header;
@@ -1948,11 +1956,13 @@  static void vdm_run_state_machine(struct tcpm_port *port)
 			switch (PD_VDO_CMD(vdo_hdr)) {
 			case CMD_DISCOVER_IDENT:
 				res = tcpm_ams_start(port, DISCOVER_IDENTITY);
-				if (res == 0)
+				if (res == 0) {
 					port->send_discover = false;
-				else if (res == -EAGAIN)
+				} else if (res == -EAGAIN) {
+					port->vdo_data[0] = 0;
 					mod_send_discover_delayed_work(port,
 								       SEND_DISCOVER_RETRY_MS);
+				}
 				break;
 			case CMD_DISCOVER_SVID:
 				res = tcpm_ams_start(port, DISCOVER_SVIDS);
@@ -2035,6 +2045,7 @@  static void vdm_run_state_machine(struct tcpm_port *port)
 			unsigned long timeout;
 
 			port->vdm_retries = 0;
+			port->vdo_data[0] = 0;
 			port->vdm_state = VDM_STATE_BUSY;
 			timeout = vdm_ready_timeout(vdo_hdr);
 			mod_vdm_delayed_work(port, timeout);