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);
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))if (cci & UCSI_CCI_COMMAND_COMPLETE &&
test_bit(COMMAND_PENDING, &ucsi->flags))
test_and_clear_bit(COMMAND_PENDING, &ucsi->flags))} 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
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
linux-stable-mirror@lists.linaro.org
-
Dmitry Baryshkov -
Łukasz Bartosik