Re: [PATCH] usb: typec: ucsi: Clear pending after acking connector change

[Benjamin Berg <bberg@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3120 bytes --]

On Mon, 2021-05-17 at 13:03 +0300, Heikki Krogerus wrote:
> On Sat, May 15, 2021 at 09:09:53PM -0700, Bjorn Andersson wrote:
> > It's possible that the interrupt handler for the UCSI driver
> > signals a
> > connector changes after the handler clears the PENDING bit, but
> > before
> > it has sent the acknowledge request. The result is that the handler
> > is
> > invoked yet again, to ack the same connector change.
> > 
> > At least some versions of the Qualcomm UCSI firmware will not
> > handle the
> > second - "spurious" - acknowledgment gracefully. So make sure to
> > not
> > clear the pending flag untuntil the changeil the change is
> > acknowledged.
> > 
> > Any connector changes coming in after the acknowledgment, that
> > would
> > have the pending flag incorrectly cleared, would afaict be covered
> > by
> > the subsequent connector status check.
> > 
> > Fixes: 217504a05532 ("usb: typec: ucsi: Work around PPM losing
> > change information")
> > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> 
> I'm OK with this if Bejamin does not see any problems with it. I'll
> wait for his comments before giving my reviewed-by tag.

Interesting problem. I wonder if we can detect this situation and
restart the worker instead of sending the ACK.

As for the patch, I do believe it is safe. I agree with the assessment
in the commit message, that the subsequent connector check will detect
any race condition and recovers from it.

Acked-By: Benjamin Berg <bberg@redhat.com>

Benjamin

> That workaround (commit 217504a05532) is unfortunately too fragile.
> I'm going to now separate the processing of the connector state from
> the event handler (interrupt handler). That way we should be fairly
> sure we don't loose any of the connector states even if an event is
> generated while we are still in the middle of processing the previous
> one(s), and at the same time be sure that we also don't confuse the
> firmware.
> So the event handler shall after that only read the connector status,
> schedule the unique job where it's processed and ACK the event.
> Nothing else.
> 
> > ---
> >  drivers/usb/typec/ucsi/ucsi.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/usb/typec/ucsi/ucsi.c
> > b/drivers/usb/typec/ucsi/ucsi.c
> > index 282c3c825c13..f451ce0132a9 100644
> > --- a/drivers/usb/typec/ucsi/ucsi.c
> > +++ b/drivers/usb/typec/ucsi/ucsi.c
> > @@ -694,8 +694,8 @@ static void ucsi_handle_connector_change(struct
> > work_struct *work)
> >         ucsi_send_command(con->ucsi, command, NULL, 0);
> >  
> >         /* 3. ACK connector change */
> > -       clear_bit(EVENT_PENDING, &ucsi->flags);
> >         ret = ucsi_acknowledge_connector_change(ucsi);
> > +       clear_bit(EVENT_PENDING, &ucsi->flags);
> >         if (ret) {
> >                 dev_err(ucsi->dev, "%s: ACK failed (%d)", __func__,
> > ret);
> >                 goto out_unlock;
> > -- 
> > 2.29.2
> 
> thanks,
> 


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]