| Message ID | 20210816154632.381968-1-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 5571ea3117ca22849072adb58074fb5a2fd12c00 |
| Headers | show |
| Series | [regression,fix] usb: typec: tcpm: Fix VDMs sometimes not being forwarded to alt-mode drivers | expand |
On Mon, Aug 16, 2021 at 05:46:32PM +0200, Hans de Goede wrote: > Commit a20dcf53ea98 ("usb: typec: tcpm: Respond Not_Supported if no > snk_vdo"), stops tcpm_pd_data_request() calling tcpm_handle_vdm_request() > when port->nr_snk_vdo is not set. But the VDM might be intended for an > altmode-driver, in which case nr_snk_vdo does not matter. > > This change breaks the forwarding of connector hotplug (HPD) events > for displayport altmode on devices which don't set nr_snk_vdo. > > tcpm_pd_data_request() is the only caller of tcpm_handle_vdm_request(), > so we can move the nr_snk_vdo check to inside it, at which point we > have already looked up the altmode device so we can check for this too. > > Doing this check here also ensures that vdm_state gets set to > VDM_STATE_DONE if it was VDM_STATE_BUSY, even if we end up with > responding with PD_MSG_CTRL_NOT_SUPP later. > > Note that tcpm_handle_vdm_request() was already sending > PD_MSG_CTRL_NOT_SUPP in some circumstances, after moving the nr_snk_vdo > check the same error-path is now taken when that check fails. So that > we have only one error-path for this and not two. Replace the > tcpm_queue_message(PD_MSG_CTRL_NOT_SUPP) used by the existing error-path > with the more robust tcpm_pd_handle_msg() from the (now removed) second > error-path. > > Cc: Kyle Tso <kyletso@google.com> > Fixes: a20dcf53ea98 ("usb: typec: tcpm: Respond Not_Supported if no snk_vdo") > Signed-off-by: Hans de Goede <hdegoede@redhat.com> Acked-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> > --- > drivers/usb/typec/tcpm/tcpm.c | 13 +++++++------ > 1 file changed, 7 insertions(+), 6 deletions(-) > > diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c > index b9bb63d749ec..f4079b5cb26d 100644 > --- a/drivers/usb/typec/tcpm/tcpm.c > +++ b/drivers/usb/typec/tcpm/tcpm.c > @@ -1737,6 +1737,10 @@ static int tcpm_pd_svdm(struct tcpm_port *port, struct typec_altmode *adev, > return rlen; > } > > +static void tcpm_pd_handle_msg(struct tcpm_port *port, > + enum pd_msg_request message, > + enum tcpm_ams ams); > + > static void tcpm_handle_vdm_request(struct tcpm_port *port, > const __le32 *payload, int cnt) > { > @@ -1764,11 +1768,11 @@ static void tcpm_handle_vdm_request(struct tcpm_port *port, > port->vdm_state = VDM_STATE_DONE; > } > > - if (PD_VDO_SVDM(p[0])) { > + if (PD_VDO_SVDM(p[0]) && (adev || tcpm_vdm_ams(port) || port->nr_snk_vdo)) { > rlen = tcpm_pd_svdm(port, adev, p, cnt, response, &adev_action); > } else { > if (port->negotiated_rev >= PD_REV30) > - tcpm_queue_message(port, PD_MSG_CTRL_NOT_SUPP); > + tcpm_pd_handle_msg(port, PD_MSG_CTRL_NOT_SUPP, NONE_AMS); > } > > /* > @@ -2471,10 +2475,7 @@ static void tcpm_pd_data_request(struct tcpm_port *port, > NONE_AMS); > break; > case PD_DATA_VENDOR_DEF: > - if (tcpm_vdm_ams(port) || port->nr_snk_vdo) > - tcpm_handle_vdm_request(port, msg->payload, cnt); > - else if (port->negotiated_rev > PD_REV20) > - tcpm_pd_handle_msg(port, PD_MSG_CTRL_NOT_SUPP, NONE_AMS); > + tcpm_handle_vdm_request(port, msg->payload, cnt); > break; > case PD_DATA_BIST: > port->bist_request = le32_to_cpu(msg->payload[0]); > -- > 2.31.1
On Tue, Aug 17, 2021 at 6:07 PM Heikki Krogerus <heikki.krogerus@linux.intel.com> wrote: > > On Mon, Aug 16, 2021 at 05:46:32PM +0200, Hans de Goede wrote: > > Commit a20dcf53ea98 ("usb: typec: tcpm: Respond Not_Supported if no > > snk_vdo"), stops tcpm_pd_data_request() calling tcpm_handle_vdm_request() > > when port->nr_snk_vdo is not set. But the VDM might be intended for an > > altmode-driver, in which case nr_snk_vdo does not matter. > > > > This change breaks the forwarding of connector hotplug (HPD) events > > for displayport altmode on devices which don't set nr_snk_vdo. > > > > tcpm_pd_data_request() is the only caller of tcpm_handle_vdm_request(), > > so we can move the nr_snk_vdo check to inside it, at which point we > > have already looked up the altmode device so we can check for this too. > > > > Doing this check here also ensures that vdm_state gets set to > > VDM_STATE_DONE if it was VDM_STATE_BUSY, even if we end up with > > responding with PD_MSG_CTRL_NOT_SUPP later. > > > > Note that tcpm_handle_vdm_request() was already sending > > PD_MSG_CTRL_NOT_SUPP in some circumstances, after moving the nr_snk_vdo > > check the same error-path is now taken when that check fails. So that > > we have only one error-path for this and not two. Replace the > > tcpm_queue_message(PD_MSG_CTRL_NOT_SUPP) used by the existing error-path > > with the more robust tcpm_pd_handle_msg() from the (now removed) second > > error-path. Thanks for fixing this problem! > > > > Cc: Kyle Tso <kyletso@google.com> > > Fixes: a20dcf53ea98 ("usb: typec: tcpm: Respond Not_Supported if no snk_vdo") > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > > Acked-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> > Acked-by: Kyle Tso <kyletso@google.com> > > --- > > drivers/usb/typec/tcpm/tcpm.c | 13 +++++++------ > > 1 file changed, 7 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c > > index b9bb63d749ec..f4079b5cb26d 100644 > > --- a/drivers/usb/typec/tcpm/tcpm.c > > +++ b/drivers/usb/typec/tcpm/tcpm.c > > @@ -1737,6 +1737,10 @@ static int tcpm_pd_svdm(struct tcpm_port *port, struct typec_altmode *adev, > > return rlen; > > } > > > > +static void tcpm_pd_handle_msg(struct tcpm_port *port, > > + enum pd_msg_request message, > > + enum tcpm_ams ams); > > + > > static void tcpm_handle_vdm_request(struct tcpm_port *port, > > const __le32 *payload, int cnt) > > { > > @@ -1764,11 +1768,11 @@ static void tcpm_handle_vdm_request(struct tcpm_port *port, > > port->vdm_state = VDM_STATE_DONE; > > } > > > > - if (PD_VDO_SVDM(p[0])) { > > + if (PD_VDO_SVDM(p[0]) && (adev || tcpm_vdm_ams(port) || port->nr_snk_vdo)) { > > rlen = tcpm_pd_svdm(port, adev, p, cnt, response, &adev_action); > > } else { > > if (port->negotiated_rev >= PD_REV30) > > - tcpm_queue_message(port, PD_MSG_CTRL_NOT_SUPP); > > + tcpm_pd_handle_msg(port, PD_MSG_CTRL_NOT_SUPP, NONE_AMS); > > } > > > > /* > > @@ -2471,10 +2475,7 @@ static void tcpm_pd_data_request(struct tcpm_port *port, > > NONE_AMS); > > break; > > case PD_DATA_VENDOR_DEF: > > - if (tcpm_vdm_ams(port) || port->nr_snk_vdo) > > - tcpm_handle_vdm_request(port, msg->payload, cnt); > > - else if (port->negotiated_rev > PD_REV20) > > - tcpm_pd_handle_msg(port, PD_MSG_CTRL_NOT_SUPP, NONE_AMS); > > + tcpm_handle_vdm_request(port, msg->payload, cnt); > > break; > > case PD_DATA_BIST: > > port->bist_request = le32_to_cpu(msg->payload[0]); > > -- > > 2.31.1 > > -- > heikki
On 8/16/21 8:46 AM, Hans de Goede wrote: > Commit a20dcf53ea98 ("usb: typec: tcpm: Respond Not_Supported if no > snk_vdo"), stops tcpm_pd_data_request() calling tcpm_handle_vdm_request() > when port->nr_snk_vdo is not set. But the VDM might be intended for an > altmode-driver, in which case nr_snk_vdo does not matter. > > This change breaks the forwarding of connector hotplug (HPD) events > for displayport altmode on devices which don't set nr_snk_vdo. > > tcpm_pd_data_request() is the only caller of tcpm_handle_vdm_request(), > so we can move the nr_snk_vdo check to inside it, at which point we > have already looked up the altmode device so we can check for this too. > > Doing this check here also ensures that vdm_state gets set to > VDM_STATE_DONE if it was VDM_STATE_BUSY, even if we end up with > responding with PD_MSG_CTRL_NOT_SUPP later. > > Note that tcpm_handle_vdm_request() was already sending > PD_MSG_CTRL_NOT_SUPP in some circumstances, after moving the nr_snk_vdo > check the same error-path is now taken when that check fails. So that > we have only one error-path for this and not two. Replace the > tcpm_queue_message(PD_MSG_CTRL_NOT_SUPP) used by the existing error-path > with the more robust tcpm_pd_handle_msg() from the (now removed) second > error-path. > > Cc: Kyle Tso <kyletso@google.com> > Fixes: a20dcf53ea98 ("usb: typec: tcpm: Respond Not_Supported if no snk_vdo") > Signed-off-by: Hans de Goede <hdegoede@redhat.com> Reviewed-by: Guenter Roeck <linux@roeck-us.net> > --- > drivers/usb/typec/tcpm/tcpm.c | 13 +++++++------ > 1 file changed, 7 insertions(+), 6 deletions(-) > > diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c > index b9bb63d749ec..f4079b5cb26d 100644 > --- a/drivers/usb/typec/tcpm/tcpm.c > +++ b/drivers/usb/typec/tcpm/tcpm.c > @@ -1737,6 +1737,10 @@ static int tcpm_pd_svdm(struct tcpm_port *port, struct typec_altmode *adev, > return rlen; > } > > +static void tcpm_pd_handle_msg(struct tcpm_port *port, > + enum pd_msg_request message, > + enum tcpm_ams ams); > + > static void tcpm_handle_vdm_request(struct tcpm_port *port, > const __le32 *payload, int cnt) > { > @@ -1764,11 +1768,11 @@ static void tcpm_handle_vdm_request(struct tcpm_port *port, > port->vdm_state = VDM_STATE_DONE; > } > > - if (PD_VDO_SVDM(p[0])) { > + if (PD_VDO_SVDM(p[0]) && (adev || tcpm_vdm_ams(port) || port->nr_snk_vdo)) { > rlen = tcpm_pd_svdm(port, adev, p, cnt, response, &adev_action); > } else { > if (port->negotiated_rev >= PD_REV30) > - tcpm_queue_message(port, PD_MSG_CTRL_NOT_SUPP); > + tcpm_pd_handle_msg(port, PD_MSG_CTRL_NOT_SUPP, NONE_AMS); > } > > /* > @@ -2471,10 +2475,7 @@ static void tcpm_pd_data_request(struct tcpm_port *port, > NONE_AMS); > break; > case PD_DATA_VENDOR_DEF: > - if (tcpm_vdm_ams(port) || port->nr_snk_vdo) > - tcpm_handle_vdm_request(port, msg->payload, cnt); > - else if (port->negotiated_rev > PD_REV20) > - tcpm_pd_handle_msg(port, PD_MSG_CTRL_NOT_SUPP, NONE_AMS); > + tcpm_handle_vdm_request(port, msg->payload, cnt); > break; > case PD_DATA_BIST: > port->bist_request = le32_to_cpu(msg->payload[0]); >
diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c index b9bb63d749ec..f4079b5cb26d 100644 --- a/drivers/usb/typec/tcpm/tcpm.c +++ b/drivers/usb/typec/tcpm/tcpm.c @@ -1737,6 +1737,10 @@ static int tcpm_pd_svdm(struct tcpm_port *port, struct typec_altmode *adev, return rlen; } +static void tcpm_pd_handle_msg(struct tcpm_port *port, + enum pd_msg_request message, + enum tcpm_ams ams); + static void tcpm_handle_vdm_request(struct tcpm_port *port, const __le32 *payload, int cnt) { @@ -1764,11 +1768,11 @@ static void tcpm_handle_vdm_request(struct tcpm_port *port, port->vdm_state = VDM_STATE_DONE; } - if (PD_VDO_SVDM(p[0])) { + if (PD_VDO_SVDM(p[0]) && (adev || tcpm_vdm_ams(port) || port->nr_snk_vdo)) { rlen = tcpm_pd_svdm(port, adev, p, cnt, response, &adev_action); } else { if (port->negotiated_rev >= PD_REV30) - tcpm_queue_message(port, PD_MSG_CTRL_NOT_SUPP); + tcpm_pd_handle_msg(port, PD_MSG_CTRL_NOT_SUPP, NONE_AMS); } /* @@ -2471,10 +2475,7 @@ static void tcpm_pd_data_request(struct tcpm_port *port, NONE_AMS); break; case PD_DATA_VENDOR_DEF: - if (tcpm_vdm_ams(port) || port->nr_snk_vdo) - tcpm_handle_vdm_request(port, msg->payload, cnt); - else if (port->negotiated_rev > PD_REV20) - tcpm_pd_handle_msg(port, PD_MSG_CTRL_NOT_SUPP, NONE_AMS); + tcpm_handle_vdm_request(port, msg->payload, cnt); break; case PD_DATA_BIST: port->bist_request = le32_to_cpu(msg->payload[0]);
Commit a20dcf53ea98 ("usb: typec: tcpm: Respond Not_Supported if no snk_vdo"), stops tcpm_pd_data_request() calling tcpm_handle_vdm_request() when port->nr_snk_vdo is not set. But the VDM might be intended for an altmode-driver, in which case nr_snk_vdo does not matter. This change breaks the forwarding of connector hotplug (HPD) events for displayport altmode on devices which don't set nr_snk_vdo. tcpm_pd_data_request() is the only caller of tcpm_handle_vdm_request(), so we can move the nr_snk_vdo check to inside it, at which point we have already looked up the altmode device so we can check for this too. Doing this check here also ensures that vdm_state gets set to VDM_STATE_DONE if it was VDM_STATE_BUSY, even if we end up with responding with PD_MSG_CTRL_NOT_SUPP later. Note that tcpm_handle_vdm_request() was already sending PD_MSG_CTRL_NOT_SUPP in some circumstances, after moving the nr_snk_vdo check the same error-path is now taken when that check fails. So that we have only one error-path for this and not two. Replace the tcpm_queue_message(PD_MSG_CTRL_NOT_SUPP) used by the existing error-path with the more robust tcpm_pd_handle_msg() from the (now removed) second error-path. Cc: Kyle Tso <kyletso@google.com> Fixes: a20dcf53ea98 ("usb: typec: tcpm: Respond Not_Supported if no snk_vdo") Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/usb/typec/tcpm/tcpm.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)