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@xxxxxxxxxxx>
> > > ---
> > > 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
--
heikki
