| Message ID | 20241104154252.1463188-1-ukaszb@chromium.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v1] usb: typec: ucsi: Fix completion notifications | expand |
On Mon, Nov 4, 2024 at 4:43 PM Łukasz Bartosik <ukaszb@chromium.org> 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. > > 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@vger.kernel.org > Signed-off-by: Łukasz Bartosik <ukaszb@chromium.org> > --- > 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); Guys, Any thoughts on this change ? Thank you, Lukasz > -- > 2.47.0.199.ga7371fff76-goog >
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? > > 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@vger.kernel.org > Signed-off-by: Łukasz Bartosik <ukaszb@chromium.org> > --- > 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); > -- > 2.47.0.199.ga7371fff76-goog >
On Mon, Nov 18, 2024 at 6:58 PM Dmitry Baryshkov <dmitry.baryshkov@linaro.org> 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. 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@vger.kernel.org > > Signed-off-by: Łukasz Bartosik <ukaszb@chromium.org> > > --- > > 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); > > -- > > 2.47.0.199.ga7371fff76-goog > > > > -- > With best wishes > Dmitry
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);
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. 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@vger.kernel.org Signed-off-by: Łukasz Bartosik <ukaszb@chromium.org> --- drivers/usb/typec/ucsi/ucsi.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)