| Message ID | 20230215113136.2838773-1-xu.yang_2@nxp.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: typec: tcpm: fix warning when handle discover_identity message | expand |
Hi, On Wed, Feb 15, 2023 at 07:31:36PM +0800, Xu Yang wrote: > diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c > index 7f39cb9b3429..914bbaf4e25e 100644 > --- a/drivers/usb/typec/tcpm/tcpm.c > +++ b/drivers/usb/typec/tcpm/tcpm.c > @@ -1445,10 +1445,19 @@ 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 firstly */ > + if (PD_VDO_SVDM(vdo_hdr) && > + PD_VDO_CMD(vdo_hdr) == CMD_DISCOVER_IDENT) { One line is enough. > + port->send_discover = true; > + mod_send_discover_delayed_work(port, > + SEND_DISCOVER_RETRY_MS); Ditto. > + } else > + /* Make sure we are not still processing a previous VDM packet */ > + WARN_ON(port->vdm_state > VDM_STATE_DONE); You need braces around the else statement in this case. > > port->vdo_count = cnt + 1; > port->vdo_data[0] = header; > @@ -1950,9 +1959,11 @@ static void vdm_run_state_machine(struct tcpm_port *port) > res = tcpm_ams_start(port, DISCOVER_IDENTITY); > 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 +2046,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); thanks,
On 2/15/23 03:31, 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, since the state machine could still send out discover > identity message later if we skip current discover_identity message. the second part of "since" is missing. since ... what ? If this is supposed to be "since ... we should handle", just drop the "since". > 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> > --- > drivers/usb/typec/tcpm/tcpm.c | 18 +++++++++++++++--- > 1 file changed, 15 insertions(+), 3 deletions(-) > > diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c > index 7f39cb9b3429..914bbaf4e25e 100644 > --- a/drivers/usb/typec/tcpm/tcpm.c > +++ b/drivers/usb/typec/tcpm/tcpm.c > @@ -1445,10 +1445,19 @@ 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 firstly */ firstly -> 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; > @@ -1950,9 +1959,11 @@ static void vdm_run_state_machine(struct tcpm_port *port) > res = tcpm_ams_start(port, DISCOVER_IDENTITY); > 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); > + } Both sides of if/else should have braces. > break; > case CMD_DISCOVER_SVID: > res = tcpm_ams_start(port, DISCOVER_SVIDS); > @@ -2035,6 +2046,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);
Hi Heikki, > -----Original Message----- > From: Heikki Krogerus <heikki.krogerus@linux.intel.com> > Sent: Wednesday, February 15, 2023 9:42 PM > To: Xu Yang <xu.yang_2@nxp.com> > Cc: linux@roeck-us.net; gregkh@linuxfoundation.org; linux-usb@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>; Jun > Li <jun.li@nxp.com> > Subject: [EXT] Re: [PATCH] usb: typec: tcpm: fix warning when handle discover_identity message > > Caution: EXT Email > > Hi, > > On Wed, Feb 15, 2023 at 07:31:36PM +0800, Xu Yang wrote: > > diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c > > index 7f39cb9b3429..914bbaf4e25e 100644 > > --- a/drivers/usb/typec/tcpm/tcpm.c > > +++ b/drivers/usb/typec/tcpm/tcpm.c > > @@ -1445,10 +1445,19 @@ 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 firstly */ > > + if (PD_VDO_SVDM(vdo_hdr) && > > + PD_VDO_CMD(vdo_hdr) == CMD_DISCOVER_IDENT) { > > One line is enough. Okay. > > > + port->send_discover = true; > > + mod_send_discover_delayed_work(port, > > + SEND_DISCOVER_RETRY_MS); > > Ditto. Okay. > > > + } else > > + /* Make sure we are not still processing a previous VDM packet */ > > + WARN_ON(port->vdm_state > VDM_STATE_DONE); > > You need braces around the else statement in this case. Okay. > > > > > port->vdo_count = cnt + 1; > > port->vdo_data[0] = header; > > @@ -1950,9 +1959,11 @@ static void vdm_run_state_machine(struct tcpm_port *port) > > res = tcpm_ams_start(port, DISCOVER_IDENTITY); > > 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 +2046,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); > Thanks, Xu Yang > thanks, > > -- > heikki
Hi Guenter, > -----Original Message----- > From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck > Sent: Wednesday, February 15, 2023 11:27 PM > To: Xu Yang <xu.yang_2@nxp.com>; heikki.krogerus@linux.intel.com > Cc: gregkh@linuxfoundation.org; linux-usb@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>; Jun Li <jun.li@nxp.com> > Subject: [EXT] Re: [PATCH] usb: typec: tcpm: fix warning when handle discover_identity message > > Caution: EXT Email > > On 2/15/23 03:31, 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, since the state machine could still send out discover > > identity message later if we skip current discover_identity message. > > the second part of "since" is missing. since ... what ? > > If this is supposed to be "since ... we should handle", just drop the > "since". Okay. > > > 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> > > --- > > drivers/usb/typec/tcpm/tcpm.c | 18 +++++++++++++++--- > > 1 file changed, 15 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c > > index 7f39cb9b3429..914bbaf4e25e 100644 > > --- a/drivers/usb/typec/tcpm/tcpm.c > > +++ b/drivers/usb/typec/tcpm/tcpm.c > > @@ -1445,10 +1445,19 @@ 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 firstly */ > > firstly -> first Okay. > > > + 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; > > @@ -1950,9 +1959,11 @@ static void vdm_run_state_machine(struct tcpm_port *port) > > res = tcpm_ams_start(port, DISCOVER_IDENTITY); > > 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); > > + } > > > Both sides of if/else should have braces. Will change. > > > break; > > case CMD_DISCOVER_SVID: > > res = tcpm_ams_start(port, DISCOVER_SVIDS); > > @@ -2035,6 +2046,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); Thanks, Xu Yang
diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c index 7f39cb9b3429..914bbaf4e25e 100644 --- a/drivers/usb/typec/tcpm/tcpm.c +++ b/drivers/usb/typec/tcpm/tcpm.c @@ -1445,10 +1445,19 @@ 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 firstly */ + 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; @@ -1950,9 +1959,11 @@ static void vdm_run_state_machine(struct tcpm_port *port) res = tcpm_ams_start(port, DISCOVER_IDENTITY); 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 +2046,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);
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, since 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> --- drivers/usb/typec/tcpm/tcpm.c | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-)