* [PATCH] usb: cdnsp: Fix issue with Clear Feature Halt Endpoint
@ 2022-10-26 9:37 Pawel Laszczak
2022-11-06 9:08 ` Peter Chen
2022-11-09 14:43 ` Peter Chen
0 siblings, 2 replies; 5+ messages in thread
From: Pawel Laszczak @ 2022-10-26 9:37 UTC (permalink / raw)
To: peter.chen; +Cc: gregkh, linux-usb, linux-kernel, Pawel Laszczak, stable
During handling Clear Halt Endpoint Feature request driver invokes
Reset Endpoint command. Because this command has some issue with
transition endpoint from Running to Idle state the driver must
stop the endpoint by using Stop Endpoint command.
cc: <stable@vger.kernel.org>
Fixes: 3d82904559f4 ("usb: cdnsp: cdns3 Add main part of Cadence USBSSP DRD Driver")
Signed-off-by: Pawel Laszczak <pawell@cadence.com>
---
drivers/usb/cdns3/cdnsp-gadget.c | 12 ++++--------
drivers/usb/cdns3/cdnsp-ring.c | 3 ++-
2 files changed, 6 insertions(+), 9 deletions(-)
diff --git a/drivers/usb/cdns3/cdnsp-gadget.c b/drivers/usb/cdns3/cdnsp-gadget.c
index e2e7d16f43f4..0576f9b0e4aa 100644
--- a/drivers/usb/cdns3/cdnsp-gadget.c
+++ b/drivers/usb/cdns3/cdnsp-gadget.c
@@ -600,11 +600,11 @@ int cdnsp_halt_endpoint(struct cdnsp_device *pdev,
trace_cdnsp_ep_halt(value ? "Set" : "Clear");
- if (value) {
- ret = cdnsp_cmd_stop_ep(pdev, pep);
- if (ret)
- return ret;
+ ret = cdnsp_cmd_stop_ep(pdev, pep);
+ if (ret)
+ return ret;
+ if (value) {
if (GET_EP_CTX_STATE(pep->out_ctx) == EP_STATE_STOPPED) {
cdnsp_queue_halt_endpoint(pdev, pep->idx);
cdnsp_ring_cmd_db(pdev);
@@ -613,10 +613,6 @@ int cdnsp_halt_endpoint(struct cdnsp_device *pdev,
pep->ep_state |= EP_HALTED;
} else {
- /*
- * In device mode driver can call reset endpoint command
- * from any endpoint state.
- */
cdnsp_queue_reset_ep(pdev, pep->idx);
cdnsp_ring_cmd_db(pdev);
ret = cdnsp_wait_for_cmd_compl(pdev);
diff --git a/drivers/usb/cdns3/cdnsp-ring.c b/drivers/usb/cdns3/cdnsp-ring.c
index 25e5e51cf5a2..aa79bce89d8a 100644
--- a/drivers/usb/cdns3/cdnsp-ring.c
+++ b/drivers/usb/cdns3/cdnsp-ring.c
@@ -2081,7 +2081,8 @@ int cdnsp_cmd_stop_ep(struct cdnsp_device *pdev, struct cdnsp_ep *pep)
u32 ep_state = GET_EP_CTX_STATE(pep->out_ctx);
int ret = 0;
- if (ep_state == EP_STATE_STOPPED || ep_state == EP_STATE_DISABLED) {
+ if (ep_state == EP_STATE_STOPPED || ep_state == EP_STATE_DISABLED ||
+ ep_state == EP_STATE_HALTED) {
trace_cdnsp_ep_stopped_or_disabled(pep->out_ctx);
goto ep_stopped;
}
--
2.25.1
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] usb: cdnsp: Fix issue with Clear Feature Halt Endpoint
2022-10-26 9:37 [PATCH] usb: cdnsp: Fix issue with Clear Feature Halt Endpoint Pawel Laszczak
@ 2022-11-06 9:08 ` Peter Chen
2022-11-07 6:01 ` Pawel Laszczak
2022-11-09 14:43 ` Peter Chen
1 sibling, 1 reply; 5+ messages in thread
From: Peter Chen @ 2022-11-06 9:08 UTC (permalink / raw)
To: Pawel Laszczak; +Cc: gregkh, linux-usb, linux-kernel, stable
On 22-10-26 05:37:10, Pawel Laszczak wrote:
> During handling Clear Halt Endpoint Feature request driver invokes
> Reset Endpoint command. Because this command has some issue with
What are issues? Would you please explain more?
> transition endpoint from Running to Idle state the driver must
> stop the endpoint by using Stop Endpoint command.
>
> cc: <stable@vger.kernel.org>
> Fixes: 3d82904559f4 ("usb: cdnsp: cdns3 Add main part of Cadence USBSSP DRD Driver")
> Signed-off-by: Pawel Laszczak <pawell@cadence.com>
> ---
> drivers/usb/cdns3/cdnsp-gadget.c | 12 ++++--------
> drivers/usb/cdns3/cdnsp-ring.c | 3 ++-
> 2 files changed, 6 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/usb/cdns3/cdnsp-gadget.c b/drivers/usb/cdns3/cdnsp-gadget.c
> index e2e7d16f43f4..0576f9b0e4aa 100644
> --- a/drivers/usb/cdns3/cdnsp-gadget.c
> +++ b/drivers/usb/cdns3/cdnsp-gadget.c
> @@ -600,11 +600,11 @@ int cdnsp_halt_endpoint(struct cdnsp_device *pdev,
>
> trace_cdnsp_ep_halt(value ? "Set" : "Clear");
>
> - if (value) {
> - ret = cdnsp_cmd_stop_ep(pdev, pep);
> - if (ret)
> - return ret;
> + ret = cdnsp_cmd_stop_ep(pdev, pep);
> + if (ret)
> + return ret;
>
In your change ,it call cdnsp_cmd_stop_ep unconditionally, no matter
set or clear halt? Is it your expectation? If it is, why?
> + if (value) {
> if (GET_EP_CTX_STATE(pep->out_ctx) == EP_STATE_STOPPED) {
> cdnsp_queue_halt_endpoint(pdev, pep->idx);
> cdnsp_ring_cmd_db(pdev);
> @@ -613,10 +613,6 @@ int cdnsp_halt_endpoint(struct cdnsp_device *pdev,
>
> pep->ep_state |= EP_HALTED;
> } else {
> - /*
> - * In device mode driver can call reset endpoint command
> - * from any endpoint state.
> - */
> cdnsp_queue_reset_ep(pdev, pep->idx);
> cdnsp_ring_cmd_db(pdev);
> ret = cdnsp_wait_for_cmd_compl(pdev);
> diff --git a/drivers/usb/cdns3/cdnsp-ring.c b/drivers/usb/cdns3/cdnsp-ring.c
> index 25e5e51cf5a2..aa79bce89d8a 100644
> --- a/drivers/usb/cdns3/cdnsp-ring.c
> +++ b/drivers/usb/cdns3/cdnsp-ring.c
> @@ -2081,7 +2081,8 @@ int cdnsp_cmd_stop_ep(struct cdnsp_device *pdev, struct cdnsp_ep *pep)
> u32 ep_state = GET_EP_CTX_STATE(pep->out_ctx);
> int ret = 0;
>
> - if (ep_state == EP_STATE_STOPPED || ep_state == EP_STATE_DISABLED) {
> + if (ep_state == EP_STATE_STOPPED || ep_state == EP_STATE_DISABLED ||
> + ep_state == EP_STATE_HALTED) {
> trace_cdnsp_ep_stopped_or_disabled(pep->out_ctx);
> goto ep_stopped;
> }
> --
> 2.25.1
>
--
Thanks,
Peter Chen
^ permalink raw reply [flat|nested] 5+ messages in thread* RE: [PATCH] usb: cdnsp: Fix issue with Clear Feature Halt Endpoint
2022-11-06 9:08 ` Peter Chen
@ 2022-11-07 6:01 ` Pawel Laszczak
0 siblings, 0 replies; 5+ messages in thread
From: Pawel Laszczak @ 2022-11-07 6:01 UTC (permalink / raw)
To: Peter Chen
Cc: gregkh@linuxfoundation.org, linux-usb@vger.kernel.org,
linux-kernel@vger.kernel.org, stable@vger.kernel.org
>
>On 22-10-26 05:37:10, Pawel Laszczak wrote:
>> During handling Clear Halt Endpoint Feature request driver invokes
>> Reset Endpoint command. Because this command has some issue with
>
>What are issues? Would you please explain more?
I don't know the internal behavior of controller. I know that there is the issue with resetting endpoint
in running state, resulting in the controller after rearming start handling incorrect TRB.
>
>> transition endpoint from Running to Idle state the driver must stop
>> the endpoint by using Stop Endpoint command.
>>
>> cc: <stable@vger.kernel.org>
>> Fixes: 3d82904559f4 ("usb: cdnsp: cdns3 Add main part of Cadence
>> USBSSP DRD Driver")
>> Signed-off-by: Pawel Laszczak <pawell@cadence.com>
>> ---
>> drivers/usb/cdns3/cdnsp-gadget.c | 12 ++++--------
>> drivers/usb/cdns3/cdnsp-ring.c | 3 ++-
>> 2 files changed, 6 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/usb/cdns3/cdnsp-gadget.c
>> b/drivers/usb/cdns3/cdnsp-gadget.c
>> index e2e7d16f43f4..0576f9b0e4aa 100644
>> --- a/drivers/usb/cdns3/cdnsp-gadget.c
>> +++ b/drivers/usb/cdns3/cdnsp-gadget.c
>> @@ -600,11 +600,11 @@ int cdnsp_halt_endpoint(struct cdnsp_device
>> *pdev,
>>
>> trace_cdnsp_ep_halt(value ? "Set" : "Clear");
>>
>> - if (value) {
>> - ret = cdnsp_cmd_stop_ep(pdev, pep);
>> - if (ret)
>> - return ret;
>> + ret = cdnsp_cmd_stop_ep(pdev, pep);
>> + if (ret)
>> + return ret;
>>
>
>In your change ,it call cdnsp_cmd_stop_ep unconditionally, no matter set or
>clear halt? Is it your expectation? If it is, why?
No exactly unconditionally.
The below condition:
if (ep_state == EP_STATE_STOPPED || ep_state == EP_STATE_DISABLED
|| ep_state == EP_STATE_HALTED) {
goto ep_stopped;
Will decide whether command will be called or not.
It will be called only when endpoint is in RUNNING state.
Pawel
>
>> + if (value) {
>> if (GET_EP_CTX_STATE(pep->out_ctx) ==
>EP_STATE_STOPPED) {
>> cdnsp_queue_halt_endpoint(pdev, pep->idx);
>> cdnsp_ring_cmd_db(pdev);
>> @@ -613,10 +613,6 @@ int cdnsp_halt_endpoint(struct cdnsp_device
>> *pdev,
>>
>> pep->ep_state |= EP_HALTED;
>> } else {
>> - /*
>> - * In device mode driver can call reset endpoint command
>> - * from any endpoint state.
>> - */
>> cdnsp_queue_reset_ep(pdev, pep->idx);
>> cdnsp_ring_cmd_db(pdev);
>> ret = cdnsp_wait_for_cmd_compl(pdev); diff --git
>> a/drivers/usb/cdns3/cdnsp-ring.c b/drivers/usb/cdns3/cdnsp-ring.c
>> index 25e5e51cf5a2..aa79bce89d8a 100644
>> --- a/drivers/usb/cdns3/cdnsp-ring.c
>> +++ b/drivers/usb/cdns3/cdnsp-ring.c
>> @@ -2081,7 +2081,8 @@ int cdnsp_cmd_stop_ep(struct cdnsp_device
>*pdev, struct cdnsp_ep *pep)
>> u32 ep_state = GET_EP_CTX_STATE(pep->out_ctx);
>> int ret = 0;
>>
>> - if (ep_state == EP_STATE_STOPPED || ep_state ==
>EP_STATE_DISABLED) {
>> + if (ep_state == EP_STATE_STOPPED || ep_state ==
>EP_STATE_DISABLED ||
>> + ep_state == EP_STATE_HALTED) {
>> trace_cdnsp_ep_stopped_or_disabled(pep->out_ctx);
>> goto ep_stopped;
>> }
>> --
>> 2.25.1
>>
>
>--
>
>Thanks,
>Peter Chen
Regards,
Pawel Laszczak
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] usb: cdnsp: Fix issue with Clear Feature Halt Endpoint
2022-10-26 9:37 [PATCH] usb: cdnsp: Fix issue with Clear Feature Halt Endpoint Pawel Laszczak
2022-11-06 9:08 ` Peter Chen
@ 2022-11-09 14:43 ` Peter Chen
1 sibling, 0 replies; 5+ messages in thread
From: Peter Chen @ 2022-11-09 14:43 UTC (permalink / raw)
To: Pawel Laszczak; +Cc: peter.chen, gregkh, linux-usb, linux-kernel, stable
On Wed, Oct 26, 2022 at 5:37 PM Pawel Laszczak <pawell@cadence.com> wrote:
>
> During handling Clear Halt Endpoint Feature request driver invokes
Add "," between request and driver. Otherwise, it is okay for me.
Reviewed-by: Peter Chen <peter.chen@kernel.org>
Peter
> Reset Endpoint command. Because this command has some issue with
> transition endpoint from Running to Idle state the driver must
> stop the endpoint by using Stop Endpoint command.
>
> cc: <stable@vger.kernel.org>
> Fixes: 3d82904559f4 ("usb: cdnsp: cdns3 Add main part of Cadence USBSSP DRD Driver")
> Signed-off-by: Pawel Laszczak <pawell@cadence.com>
> ---
> drivers/usb/cdns3/cdnsp-gadget.c | 12 ++++--------
> drivers/usb/cdns3/cdnsp-ring.c | 3 ++-
> 2 files changed, 6 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/usb/cdns3/cdnsp-gadget.c b/drivers/usb/cdns3/cdnsp-gadget.c
> index e2e7d16f43f4..0576f9b0e4aa 100644
> --- a/drivers/usb/cdns3/cdnsp-gadget.c
> +++ b/drivers/usb/cdns3/cdnsp-gadget.c
> @@ -600,11 +600,11 @@ int cdnsp_halt_endpoint(struct cdnsp_device *pdev,
>
> trace_cdnsp_ep_halt(value ? "Set" : "Clear");
>
> - if (value) {
> - ret = cdnsp_cmd_stop_ep(pdev, pep);
> - if (ret)
> - return ret;
> + ret = cdnsp_cmd_stop_ep(pdev, pep);
> + if (ret)
> + return ret;
>
> + if (value) {
> if (GET_EP_CTX_STATE(pep->out_ctx) == EP_STATE_STOPPED) {
> cdnsp_queue_halt_endpoint(pdev, pep->idx);
> cdnsp_ring_cmd_db(pdev);
> @@ -613,10 +613,6 @@ int cdnsp_halt_endpoint(struct cdnsp_device *pdev,
>
> pep->ep_state |= EP_HALTED;
> } else {
> - /*
> - * In device mode driver can call reset endpoint command
> - * from any endpoint state.
> - */
> cdnsp_queue_reset_ep(pdev, pep->idx);
> cdnsp_ring_cmd_db(pdev);
> ret = cdnsp_wait_for_cmd_compl(pdev);
> diff --git a/drivers/usb/cdns3/cdnsp-ring.c b/drivers/usb/cdns3/cdnsp-ring.c
> index 25e5e51cf5a2..aa79bce89d8a 100644
> --- a/drivers/usb/cdns3/cdnsp-ring.c
> +++ b/drivers/usb/cdns3/cdnsp-ring.c
> @@ -2081,7 +2081,8 @@ int cdnsp_cmd_stop_ep(struct cdnsp_device *pdev, struct cdnsp_ep *pep)
> u32 ep_state = GET_EP_CTX_STATE(pep->out_ctx);
> int ret = 0;
>
> - if (ep_state == EP_STATE_STOPPED || ep_state == EP_STATE_DISABLED) {
> + if (ep_state == EP_STATE_STOPPED || ep_state == EP_STATE_DISABLED ||
> + ep_state == EP_STATE_HALTED) {
> trace_cdnsp_ep_stopped_or_disabled(pep->out_ctx);
> goto ep_stopped;
> }
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH] usb: cdnsp: Fix issue with Clear Feature Halt Endpoint
@ 2022-11-10 6:30 Pawel Laszczak
0 siblings, 0 replies; 5+ messages in thread
From: Pawel Laszczak @ 2022-11-10 6:30 UTC (permalink / raw)
To: peter.chen; +Cc: gregkh, linux-usb, linux-kernel, Pawel Laszczak, stable
During handling Clear Halt Endpoint Feature request, driver invokes
Reset Endpoint command. Because this command has some issue with
transition endpoint from Running to Idle state the driver must
stop the endpoint by using Stop Endpoint command.
cc: <stable@vger.kernel.org>
Fixes: 3d82904559f4 ("usb: cdnsp: cdns3 Add main part of Cadence USBSSP DRD Driver")
Reviewed-by: Peter Chen <peter.chen@kernel.org>
Signed-off-by: Pawel Laszczak <pawell@cadence.com>
---
Changelog:
v2:
- added comma in patch description
drivers/usb/cdns3/cdnsp-gadget.c | 12 ++++--------
drivers/usb/cdns3/cdnsp-ring.c | 3 ++-
2 files changed, 6 insertions(+), 9 deletions(-)
diff --git a/drivers/usb/cdns3/cdnsp-gadget.c b/drivers/usb/cdns3/cdnsp-gadget.c
index e2e7d16f43f4..0576f9b0e4aa 100644
--- a/drivers/usb/cdns3/cdnsp-gadget.c
+++ b/drivers/usb/cdns3/cdnsp-gadget.c
@@ -600,11 +600,11 @@ int cdnsp_halt_endpoint(struct cdnsp_device *pdev,
trace_cdnsp_ep_halt(value ? "Set" : "Clear");
- if (value) {
- ret = cdnsp_cmd_stop_ep(pdev, pep);
- if (ret)
- return ret;
+ ret = cdnsp_cmd_stop_ep(pdev, pep);
+ if (ret)
+ return ret;
+ if (value) {
if (GET_EP_CTX_STATE(pep->out_ctx) == EP_STATE_STOPPED) {
cdnsp_queue_halt_endpoint(pdev, pep->idx);
cdnsp_ring_cmd_db(pdev);
@@ -613,10 +613,6 @@ int cdnsp_halt_endpoint(struct cdnsp_device *pdev,
pep->ep_state |= EP_HALTED;
} else {
- /*
- * In device mode driver can call reset endpoint command
- * from any endpoint state.
- */
cdnsp_queue_reset_ep(pdev, pep->idx);
cdnsp_ring_cmd_db(pdev);
ret = cdnsp_wait_for_cmd_compl(pdev);
diff --git a/drivers/usb/cdns3/cdnsp-ring.c b/drivers/usb/cdns3/cdnsp-ring.c
index 25e5e51cf5a2..aa79bce89d8a 100644
--- a/drivers/usb/cdns3/cdnsp-ring.c
+++ b/drivers/usb/cdns3/cdnsp-ring.c
@@ -2081,7 +2081,8 @@ int cdnsp_cmd_stop_ep(struct cdnsp_device *pdev, struct cdnsp_ep *pep)
u32 ep_state = GET_EP_CTX_STATE(pep->out_ctx);
int ret = 0;
- if (ep_state == EP_STATE_STOPPED || ep_state == EP_STATE_DISABLED) {
+ if (ep_state == EP_STATE_STOPPED || ep_state == EP_STATE_DISABLED ||
+ ep_state == EP_STATE_HALTED) {
trace_cdnsp_ep_stopped_or_disabled(pep->out_ctx);
goto ep_stopped;
}
--
2.25.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-11-10 6:30 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-10-26 9:37 [PATCH] usb: cdnsp: Fix issue with Clear Feature Halt Endpoint Pawel Laszczak
2022-11-06 9:08 ` Peter Chen
2022-11-07 6:01 ` Pawel Laszczak
2022-11-09 14:43 ` Peter Chen
-- strict thread matches above, loose matches on Subject: below --
2022-11-10 6:30 Pawel Laszczak
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox