On Thu, Nov 28, 2024 at 3:16 PM Dmitry Baryshkov
<dmitry.baryshkov@xxxxxxxxxx> wrote:
>
> On Thu, Nov 28, 2024 at 11:08:46AM +0100, Łukasz Bartosik wrote:
> > On Thu, Nov 21, 2024 at 11:53 PM Dmitry Baryshkov
> > <dmitry.baryshkov@xxxxxxxxxx> wrote:
> > >
> > > On Wed, Nov 20, 2024 at 03:56:41PM +0100, Łukasz Bartosik wrote:
> > > > On Mon, Nov 18, 2024 at 6:58 PM Dmitry Baryshkov
> > > > <dmitry.baryshkov@xxxxxxxxxx> wrote:
> > > > >
> > > > > On Mon, Nov 04, 2024 at 03:42:52PM +0000, Łukasz Bartosik wrote:
> > > > > > OPM PPM LPM
> > > > > > | 1.send cmd | |
> > > > > > |-------------------------->| |
> > > > > > | |-- |
> > > > > > | | | 2.set busy bit in CCI |
> > > > > > | |<- |
> > > > > > | 3.notify the OPM | |
> > > > > > |<--------------------------| |
> > > > > > | | 4.send cmd to be executed |
> > > > > > | |-------------------------->|
> > > > > > | | |
> > > > > > | | 5.cmd completed |
> > > > > > | |<--------------------------|
> > > > > > | | |
> > > > > > | |-- |
> > > > > > | | | 6.set cmd completed |
> > > > > > | |<- bit in CCI |
> > > > > > | | |
> > > > > > | 7.handle notification | |
> > > > > > | from point 3, read CCI | |
> > > > > > |<--------------------------| |
> > > > > > | | |
> > > > > > | 8.notify the OPM | |
> > > > > > |<--------------------------| |
> > > > > > | | |
> > > > > >
> > > > > > When the PPM receives command from the OPM (p.1) it sets the busy bit
> > > > > > in the CCI (p.2), sends notification to the OPM (p.3) and forwards the
> > > > > > command to be executed by the LPM (p.4). When the PPM receives command
> > > > > > completion from the LPM (p.5) it sets command completion bit in the CCI
> > > > > > (p.6) and sends notification to the OPM (p.8). If command execution by
> > > > > > the LPM is fast enough then when the OPM starts handling the notification
> > > > > > from p.3 in p.7 and reads the CCI value it will see command completion bit
> > > > > > and will call complete(). Then complete() might be called again when the
> > > > > > OPM handles notification from p.8.
> > > > >
> > > > > I think the change is fine, but I'd like to understand, what code path
> > > > > causes the first read from the OPM side before the notification from
> > > > > the PPM?
> > > > >
> > > >
> > > > The read from the OPM in p.7 is a result of notification in p.3 but I agree
> > > > it is misleading since you pointed it out. I will reorder p.7 and p.8.
> > >
> > > Ack, thanks for the explanation. Do you think that it also might be
> > > beneficial to call reinit_completion() when sending the command? I think
> > > we discussed this change few months ago on the ML, but I failed to send
> > > the patch...
> > >
> >
> > Dmitry sorry for delayed response.
> >
> > IMHO it makes sense to clear completion in ucsi_sync_control_common()
> > with reinit_completion().
>
> yes. before sending the command.
>
Ack
> > But I wonder whether with this change moving from test_bit ->
> > test_and_clear_bit do you still see a potential
> > scenario for a race ?
>
> Two notifications coming close enough so that the second one starts being
> processed after receiving the first one but before completing it? On the
> other hand, test_and_clear_bit() will handle that already.
>
I agree test_and_clear_bit() should handle such a scenario.
Nonetheless I will add a call to reinit_completion() in
ucsi_sync_control_common().
Thanks,
Lukasz
> > > > > > This fix replaces test_bit() with test_and_clear_bit()
> > > > > > in ucsi_notify_common() in order to call complete() only
> > > > > > once per request.
> > > > > >
> > > > > > Fixes: 584e8df58942 ("usb: typec: ucsi: extract common code for command handling")
> > > > > > Cc: stable@xxxxxxxxxxxxxxx
> > > > > > Signed-off-by: Łukasz Bartosik <ukaszb@xxxxxxxxxxxx>
> > > > > > ---
> > > > > > drivers/usb/typec/ucsi/ucsi.c | 4 ++--
> > > > > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
> > > > > > index e0f3925e401b..7a9b987ea80c 100644
> > > > > > --- a/drivers/usb/typec/ucsi/ucsi.c
> > > > > > +++ b/drivers/usb/typec/ucsi/ucsi.c
> > > > > > @@ -46,11 +46,11 @@ void ucsi_notify_common(struct ucsi *ucsi, u32 cci)
> > > > > > ucsi_connector_change(ucsi, UCSI_CCI_CONNECTOR(cci));
> > > > > >
> > > > > > if (cci & UCSI_CCI_ACK_COMPLETE &&
> > > > > > - test_bit(ACK_PENDING, &ucsi->flags))
> > > > > > + test_and_clear_bit(ACK_PENDING, &ucsi->flags))
> > > > > > complete(&ucsi->complete);
> > > > > >
> > > > > > if (cci & UCSI_CCI_COMMAND_COMPLETE &&
> > > > > > - test_bit(COMMAND_PENDING, &ucsi->flags))
> > > > > > + test_and_clear_bit(COMMAND_PENDING, &ucsi->flags))
> > > > > > complete(&ucsi->complete);
> > > > > > }
> > > > > > EXPORT_SYMBOL_GPL(ucsi_notify_common);
>
> --
> With best wishes
> Dmitry
