| Message ID | 1675158735-2757-1-git-send-email-quic_linyyuan@quicinc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] usb: typec: ucsi: change role command to async write | expand |
On Tue, Jan 31, 2023 at 05:52:15PM +0800, Linyu Yuan wrote: > In ucsi_dr_swap() and ucsi_pr_swap(), it will wait complete. > it is better change role switch command to async mode which will avoid > reset ppm in condition that data/power switch can't operate. I think I need a little bit more information. Reseting the whole PPM is a heavy operation, I do admit that, but you are not really explaining what happens in your case when the driver does it - why is it a problem? The proposal of using async_write here is in any case not acceptable. You would now end up generationg a spurious command completion event that can in worst case will screws up some other command. If the PPM reset is the problem here, then perhaps the proper thing to do would be to remove that instead? thanks, > Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com> > --- > drivers/usb/typec/ucsi/ucsi.c | 12 ++++++++---- > 1 file changed, 8 insertions(+), 4 deletions(-) > > diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c > index 00fc867..466ae93 100644 > --- a/drivers/usb/typec/ucsi/ucsi.c > +++ b/drivers/usb/typec/ucsi/ucsi.c > @@ -997,17 +997,21 @@ static int ucsi_reset_ppm(struct ucsi *ucsi) > > static int ucsi_role_cmd(struct ucsi_connector *con, u64 command) > { > + struct ucsi *ucsi = con->ucsi; > int ret; > > - ret = ucsi_send_command(con->ucsi, command, NULL, 0); > + mutex_lock(&ucsi->ppm_lock); > + ret = ucsi->ops->async_write(ucsi, UCSI_CONTROL, &command, sizeof(command)); > + mutex_unlock(&ucsi->ppm_lock); > + > if (ret == -ETIMEDOUT) { > u64 c; > > /* PPM most likely stopped responding. Resetting everything. */ > - ucsi_reset_ppm(con->ucsi); > + ucsi_reset_ppm(ucsi); > > - c = UCSI_SET_NOTIFICATION_ENABLE | con->ucsi->ntfy; > - ucsi_send_command(con->ucsi, c, NULL, 0); > + c = UCSI_SET_NOTIFICATION_ENABLE | ucsi->ntfy; > + ucsi_send_command(ucsi, c, NULL, 0); > > ucsi_reset_connector(con, true); > } > -- > 2.7.4
On 2/1/2023 4:50 PM, Heikki Krogerus wrote: > On Tue, Jan 31, 2023 at 05:52:15PM +0800, Linyu Yuan wrote: >> In ucsi_dr_swap() and ucsi_pr_swap(), it will wait complete. >> it is better change role switch command to async mode which will avoid >> reset ppm in condition that data/power switch can't operate. > I think I need a little bit more information. Reseting the whole PPM > is a heavy operation, I do admit that, but you are not really > explaining what happens in your case when the driver does it - why is > it a problem? the case i can think is connect a mobile device to PC through USB A-C cable, and run data role switch command on mobile device, it definitely will fail, right ? the problem is if ppm can't response reset timely, the data role switch will exit after 10 seconds, it is very long time. > > The proposal of using async_write here is in any case not acceptable. > You would now end up generationg a spurious command completion event > that can in worst case will screws up some other command. do you mean other command can run before role switch command operation ? > > If the PPM reset is the problem here, then perhaps the proper thing to > do would be to remove that instead? > > thanks, > >> Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com> >> --- >> drivers/usb/typec/ucsi/ucsi.c | 12 ++++++++---- >> 1 file changed, 8 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c >> index 00fc867..466ae93 100644 >> --- a/drivers/usb/typec/ucsi/ucsi.c >> +++ b/drivers/usb/typec/ucsi/ucsi.c >> @@ -997,17 +997,21 @@ static int ucsi_reset_ppm(struct ucsi *ucsi) >> >> static int ucsi_role_cmd(struct ucsi_connector *con, u64 command) >> { >> + struct ucsi *ucsi = con->ucsi; >> int ret; >> >> - ret = ucsi_send_command(con->ucsi, command, NULL, 0); >> + mutex_lock(&ucsi->ppm_lock); >> + ret = ucsi->ops->async_write(ucsi, UCSI_CONTROL, &command, sizeof(command)); >> + mutex_unlock(&ucsi->ppm_lock); >> + >> if (ret == -ETIMEDOUT) { >> u64 c; >> >> /* PPM most likely stopped responding. Resetting everything. */ >> - ucsi_reset_ppm(con->ucsi); >> + ucsi_reset_ppm(ucsi); >> >> - c = UCSI_SET_NOTIFICATION_ENABLE | con->ucsi->ntfy; >> - ucsi_send_command(con->ucsi, c, NULL, 0); >> + c = UCSI_SET_NOTIFICATION_ENABLE | ucsi->ntfy; >> + ucsi_send_command(ucsi, c, NULL, 0); >> >> ucsi_reset_connector(con, true); >> } >> -- >> 2.7.4
On Thu, Feb 02, 2023 at 05:57:58PM +0800, Linyu Yuan wrote: > > On 2/1/2023 4:50 PM, Heikki Krogerus wrote: > > On Tue, Jan 31, 2023 at 05:52:15PM +0800, Linyu Yuan wrote: > > > In ucsi_dr_swap() and ucsi_pr_swap(), it will wait complete. > > > it is better change role switch command to async mode which will avoid > > > reset ppm in condition that data/power switch can't operate. > > I think I need a little bit more information. Reseting the whole PPM > > is a heavy operation, I do admit that, but you are not really > > explaining what happens in your case when the driver does it - why is > > it a problem? > > > the case i can think is connect a mobile device to PC through USB A-C cable, > > and run data role switch command on mobile device, it definitely will fail, > right ? > > the problem is if ppm can't response reset timely, the data role switch will > exit after 10 seconds, > > it is very long time. So the problem is that it takes too long? If that is really a problem, then just consider removing the resets. Don't try to use tricks like this. > > The proposal of using async_write here is in any case not acceptable. > > You would now end up generationg a spurious command completion event > > that can in worst case will screws up some other command. > > do you mean other command can run before role switch command operation ? Both role swap operations release the connector lock after sending the role swap command, and then start waiting for the completion. That wait would now almoust always timeout because the underlying driver does not know that there is a command pending. The only case where it would not timeout is if there is an other command that is queued after the role swap. In that case the role swap would hog the completion of that other command. Do not call the IO callbacks directly! Always use ucsi_send_command() instead. That will guarantee that the CCI is always checked and that errors are handled if there are any. > > If the PPM reset is the problem here, then perhaps the proper thing to > > do would be to remove that instead? > > > > thanks, > > > > > Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com> > > > --- > > > drivers/usb/typec/ucsi/ucsi.c | 12 ++++++++---- > > > 1 file changed, 8 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c > > > index 00fc867..466ae93 100644 > > > --- a/drivers/usb/typec/ucsi/ucsi.c > > > +++ b/drivers/usb/typec/ucsi/ucsi.c > > > @@ -997,17 +997,21 @@ static int ucsi_reset_ppm(struct ucsi *ucsi) > > > static int ucsi_role_cmd(struct ucsi_connector *con, u64 command) > > > { > > > + struct ucsi *ucsi = con->ucsi; > > > int ret; > > > - ret = ucsi_send_command(con->ucsi, command, NULL, 0); > > > + mutex_lock(&ucsi->ppm_lock); > > > + ret = ucsi->ops->async_write(ucsi, UCSI_CONTROL, &command, sizeof(command)); > > > + mutex_unlock(&ucsi->ppm_lock); > > > + > > > if (ret == -ETIMEDOUT) { > > > u64 c; > > > /* PPM most likely stopped responding. Resetting everything. */ > > > - ucsi_reset_ppm(con->ucsi); > > > + ucsi_reset_ppm(ucsi); > > > - c = UCSI_SET_NOTIFICATION_ENABLE | con->ucsi->ntfy; > > > - ucsi_send_command(con->ucsi, c, NULL, 0); > > > + c = UCSI_SET_NOTIFICATION_ENABLE | ucsi->ntfy; > > > + ucsi_send_command(ucsi, c, NULL, 0); > > > ucsi_reset_connector(con, true); > > > } > > > -- > > > 2.7.4
On 2/2/2023 8:47 PM, Heikki Krogerus wrote: > On Thu, Feb 02, 2023 at 05:57:58PM +0800, Linyu Yuan wrote: >> On 2/1/2023 4:50 PM, Heikki Krogerus wrote: >>> On Tue, Jan 31, 2023 at 05:52:15PM +0800, Linyu Yuan wrote: >>>> In ucsi_dr_swap() and ucsi_pr_swap(), it will wait complete. >>>> it is better change role switch command to async mode which will avoid >>>> reset ppm in condition that data/power switch can't operate. >>> I think I need a little bit more information. Reseting the whole PPM >>> is a heavy operation, I do admit that, but you are not really >>> explaining what happens in your case when the driver does it - why is >>> it a problem? >> >> the case i can think is connect a mobile device to PC through USB A-C cable, >> >> and run data role switch command on mobile device, it definitely will fail, >> right ? >> >> the problem is if ppm can't response reset timely, the data role switch will >> exit after 10 seconds, >> >> it is very long time. > So the problem is that it takes too long? If that is really a problem, > then just consider removing the resets. Don't try to use tricks like > this. is there any PPM product from intel support data/power role switch ? seem PPM firmware didn't work very well if there is no reset. static int ucsi_role_cmd(struct ucsi_connector *con, u64 command) { - int ret; - - ret = ucsi_send_command(con->ucsi, command, NULL, 0); - if (ret == -ETIMEDOUT) { - u64 c; - - /* PPM most likely stopped responding. Resetting everything. */ - ucsi_reset_ppm(con->ucsi); - - c = UCSI_SET_NOTIFICATION_ENABLE | con->ucsi->ntfy; - ucsi_send_command(con->ucsi, c, NULL, 0); - - ucsi_reset_connector(con, true); - } - - return ret; + return ucsi_send_command(con->ucsi, command, NULL, 0); } >>> The proposal of using async_write here is in any case not acceptable. >>> You would now end up generationg a spurious command completion event >>> that can in worst case will screws up some other command. >> do you mean other command can run before role switch command operation ? > Both role swap operations release the connector lock after sending the > role swap command, and then start waiting for the completion. That > wait would now almoust always timeout because the underlying driver > does not know that there is a command pending. The only case where it > would not timeout is if there is an other command that is queued after > the role swap. In that case the role swap would hog the completion of > that other command. > > Do not call the IO callbacks directly! Always use ucsi_send_command() > instead. That will guarantee that the CCI is always checked and that > errors are handled if there are any. thanks for explanation. > >>> If the PPM reset is the problem here, then perhaps the proper thing to >>> do would be to remove that instead? >>> >>> thanks, >>> >>>> Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com> >>>> --- >>>> drivers/usb/typec/ucsi/ucsi.c | 12 ++++++++---- >>>> 1 file changed, 8 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c >>>> index 00fc867..466ae93 100644 >>>> --- a/drivers/usb/typec/ucsi/ucsi.c >>>> +++ b/drivers/usb/typec/ucsi/ucsi.c >>>> @@ -997,17 +997,21 @@ static int ucsi_reset_ppm(struct ucsi *ucsi) >>>> static int ucsi_role_cmd(struct ucsi_connector *con, u64 command) >>>> { >>>> + struct ucsi *ucsi = con->ucsi; >>>> int ret; >>>> - ret = ucsi_send_command(con->ucsi, command, NULL, 0); >>>> + mutex_lock(&ucsi->ppm_lock); >>>> + ret = ucsi->ops->async_write(ucsi, UCSI_CONTROL, &command, sizeof(command)); >>>> + mutex_unlock(&ucsi->ppm_lock); >>>> + >>>> if (ret == -ETIMEDOUT) { >>>> u64 c; >>>> /* PPM most likely stopped responding. Resetting everything. */ >>>> - ucsi_reset_ppm(con->ucsi); >>>> + ucsi_reset_ppm(ucsi); >>>> - c = UCSI_SET_NOTIFICATION_ENABLE | con->ucsi->ntfy; >>>> - ucsi_send_command(con->ucsi, c, NULL, 0); >>>> + c = UCSI_SET_NOTIFICATION_ENABLE | ucsi->ntfy; >>>> + ucsi_send_command(ucsi, c, NULL, 0); >>>> ucsi_reset_connector(con, true); >>>> } >>>> -- >>>> 2.7.4
diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c index 00fc867..466ae93 100644 --- a/drivers/usb/typec/ucsi/ucsi.c +++ b/drivers/usb/typec/ucsi/ucsi.c @@ -997,17 +997,21 @@ static int ucsi_reset_ppm(struct ucsi *ucsi) static int ucsi_role_cmd(struct ucsi_connector *con, u64 command) { + struct ucsi *ucsi = con->ucsi; int ret; - ret = ucsi_send_command(con->ucsi, command, NULL, 0); + mutex_lock(&ucsi->ppm_lock); + ret = ucsi->ops->async_write(ucsi, UCSI_CONTROL, &command, sizeof(command)); + mutex_unlock(&ucsi->ppm_lock); + if (ret == -ETIMEDOUT) { u64 c; /* PPM most likely stopped responding. Resetting everything. */ - ucsi_reset_ppm(con->ucsi); + ucsi_reset_ppm(ucsi); - c = UCSI_SET_NOTIFICATION_ENABLE | con->ucsi->ntfy; - ucsi_send_command(con->ucsi, c, NULL, 0); + c = UCSI_SET_NOTIFICATION_ENABLE | ucsi->ntfy; + ucsi_send_command(ucsi, c, NULL, 0); ucsi_reset_connector(con, true); }
In ucsi_dr_swap() and ucsi_pr_swap(), it will wait complete. it is better change role switch command to async mode which will avoid reset ppm in condition that data/power switch can't operate. Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com> --- drivers/usb/typec/ucsi/ucsi.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)