| Message ID | 20241104154252.1463188-1-ukaszb@chromium.org |
|---|---|
| State | New |
| Headers | show |
| Series | [v1] usb: typec: ucsi: Fix completion notifications | expand |
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 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@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. 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... > > 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
On Thu, Nov 21, 2024 at 11:53 PM Dmitry Baryshkov <dmitry.baryshkov@linaro.org> 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@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. > > 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(). 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 ? Thanks, Lukasz > > > > 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 > > -- > With best wishes > Dmitry
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@linaro.org> 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@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. > > > > 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. > 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. > > > > > 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);
On Thu, Nov 28, 2024 at 3:16 PM Dmitry Baryshkov <dmitry.baryshkov@linaro.org> 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@linaro.org> 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@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. > > > > > > 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@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); > > -- > 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(-)