* [PATCH v3] usb: cdnsp: Fix issue with CV Bad Descriptor test [not found] <20250620074306.2278838-1-pawell@cadence.com> @ 2025-06-20 8:23 ` Pawel Laszczak 2025-06-21 0:36 ` Peter Chen (CIX) 2025-06-23 11:02 ` Peter Chen (CIX) 0 siblings, 2 replies; 6+ messages in thread From: Pawel Laszczak @ 2025-06-20 8:23 UTC (permalink / raw) To: peter.chen@kernel.org Cc: gregkh@linuxfoundation.org, linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org, stable@vger.kernel.org, Pawel Laszczak The SSP2 controller has extra endpoint state preserve bit (ESP) which setting causes that endpoint state will be preserved during Halt Endpoint command. It is used only for EP0. Without this bit the Command Verifier "TD 9.10 Bad Descriptor Test" failed. Setting this bit doesn't have any impact for SSP controller. Fixes: 3d82904559f4 ("usb: cdnsp: cdns3 Add main part of Cadence USBSSP DRD Driver") cc: stable@vger.kernel.org Signed-off-by: Pawel Laszczak <pawell@cadence.com> --- Changelog: v3: - removed else {} v2: - removed some typos - added pep variable initialization - updated TRB_ESP description drivers/usb/cdns3/cdnsp-debug.h | 5 +++-- drivers/usb/cdns3/cdnsp-ep0.c | 18 +++++++++++++++--- drivers/usb/cdns3/cdnsp-gadget.h | 6 ++++++ drivers/usb/cdns3/cdnsp-ring.c | 3 ++- 4 files changed, 26 insertions(+), 6 deletions(-) diff --git a/drivers/usb/cdns3/cdnsp-debug.h b/drivers/usb/cdns3/cdnsp-debug.h index cd138acdcce1..86860686d836 100644 --- a/drivers/usb/cdns3/cdnsp-debug.h +++ b/drivers/usb/cdns3/cdnsp-debug.h @@ -327,12 +327,13 @@ static inline const char *cdnsp_decode_trb(char *str, size_t size, u32 field0, case TRB_RESET_EP: case TRB_HALT_ENDPOINT: ret = scnprintf(str, size, - "%s: ep%d%s(%d) ctx %08x%08x slot %ld flags %c", + "%s: ep%d%s(%d) ctx %08x%08x slot %ld flags %c %c", cdnsp_trb_type_string(type), ep_num, ep_id % 2 ? "out" : "in", TRB_TO_EP_INDEX(field3), field1, field0, TRB_TO_SLOT_ID(field3), - field3 & TRB_CYCLE ? 'C' : 'c'); + field3 & TRB_CYCLE ? 'C' : 'c', + field3 & TRB_ESP ? 'P' : 'p'); break; case TRB_STOP_RING: ret = scnprintf(str, size, diff --git a/drivers/usb/cdns3/cdnsp-ep0.c b/drivers/usb/cdns3/cdnsp-ep0.c index f317d3c84781..5cd9b898ce97 100644 --- a/drivers/usb/cdns3/cdnsp-ep0.c +++ b/drivers/usb/cdns3/cdnsp-ep0.c @@ -414,6 +414,7 @@ static int cdnsp_ep0_std_request(struct cdnsp_device *pdev, void cdnsp_setup_analyze(struct cdnsp_device *pdev) { struct usb_ctrlrequest *ctrl = &pdev->setup; + struct cdnsp_ep *pep; int ret = -EINVAL; u16 len; @@ -427,10 +428,21 @@ void cdnsp_setup_analyze(struct cdnsp_device *pdev) goto out; } + pep = &pdev->eps[0]; + /* Restore the ep0 to Stopped/Running state. */ - if (pdev->eps[0].ep_state & EP_HALTED) { - trace_cdnsp_ep0_halted("Restore to normal state"); - cdnsp_halt_endpoint(pdev, &pdev->eps[0], 0); + if (pep->ep_state & EP_HALTED) { + if (GET_EP_CTX_STATE(pep->out_ctx) == EP_STATE_HALTED) + cdnsp_halt_endpoint(pdev, pep, 0); + + /* + * Halt Endpoint Command for SSP2 for ep0 preserve current + * endpoint state and driver has to synchronize the + * software endpoint state with endpoint output context + * state. + */ + pep->ep_state &= ~EP_HALTED; + pep->ep_state |= EP_STOPPED; } /* diff --git a/drivers/usb/cdns3/cdnsp-gadget.h b/drivers/usb/cdns3/cdnsp-gadget.h index 2afa3e558f85..a91cca509db0 100644 --- a/drivers/usb/cdns3/cdnsp-gadget.h +++ b/drivers/usb/cdns3/cdnsp-gadget.h @@ -987,6 +987,12 @@ enum cdnsp_setup_dev { #define STREAM_ID_FOR_TRB(p) ((((p)) << 16) & GENMASK(31, 16)) #define SCT_FOR_TRB(p) (((p) << 1) & 0x7) +/* + * Halt Endpoint Command TRB field. + * The ESP bit only exists in the SSP2 controller. + */ +#define TRB_ESP BIT(9) + /* Link TRB specific fields. */ #define TRB_TC BIT(1) diff --git a/drivers/usb/cdns3/cdnsp-ring.c b/drivers/usb/cdns3/cdnsp-ring.c index fd06cb85c4ea..d397d28efc6e 100644 --- a/drivers/usb/cdns3/cdnsp-ring.c +++ b/drivers/usb/cdns3/cdnsp-ring.c @@ -2483,7 +2483,8 @@ void cdnsp_queue_halt_endpoint(struct cdnsp_device *pdev, unsigned int ep_index) { cdnsp_queue_command(pdev, 0, 0, 0, TRB_TYPE(TRB_HALT_ENDPOINT) | SLOT_ID_FOR_TRB(pdev->slot_id) | - EP_ID_FOR_TRB(ep_index)); + EP_ID_FOR_TRB(ep_index) | + (!ep_index ? TRB_ESP : 0)); } void cdnsp_force_header_wakeup(struct cdnsp_device *pdev, int intf_num) -- 2.43.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v3] usb: cdnsp: Fix issue with CV Bad Descriptor test 2025-06-20 8:23 ` [PATCH v3] usb: cdnsp: Fix issue with CV Bad Descriptor test Pawel Laszczak @ 2025-06-21 0:36 ` Peter Chen (CIX) 2025-06-23 5:51 ` Pawel Laszczak 2025-06-23 11:02 ` Peter Chen (CIX) 1 sibling, 1 reply; 6+ messages in thread From: Peter Chen (CIX) @ 2025-06-21 0:36 UTC (permalink / raw) To: Pawel Laszczak Cc: gregkh@linuxfoundation.org, linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org, stable@vger.kernel.org On 25-06-20 08:23:12, Pawel Laszczak wrote: > The SSP2 controller has extra endpoint state preserve bit (ESP) which > setting causes that endpoint state will be preserved during > Halt Endpoint command. It is used only for EP0. > Without this bit the Command Verifier "TD 9.10 Bad Descriptor Test" > failed. > Setting this bit doesn't have any impact for SSP controller. > > Fixes: 3d82904559f4 ("usb: cdnsp: cdns3 Add main part of Cadence USBSSP DRD Driver") > cc: stable@vger.kernel.org > Signed-off-by: Pawel Laszczak <pawell@cadence.com> > --- > Changelog: > v3: > - removed else {} > > v2: > - removed some typos > - added pep variable initialization > - updated TRB_ESP description > > drivers/usb/cdns3/cdnsp-debug.h | 5 +++-- > drivers/usb/cdns3/cdnsp-ep0.c | 18 +++++++++++++++--- > drivers/usb/cdns3/cdnsp-gadget.h | 6 ++++++ > drivers/usb/cdns3/cdnsp-ring.c | 3 ++- > 4 files changed, 26 insertions(+), 6 deletions(-) > > diff --git a/drivers/usb/cdns3/cdnsp-debug.h b/drivers/usb/cdns3/cdnsp-debug.h > index cd138acdcce1..86860686d836 100644 > --- a/drivers/usb/cdns3/cdnsp-debug.h > +++ b/drivers/usb/cdns3/cdnsp-debug.h > @@ -327,12 +327,13 @@ static inline const char *cdnsp_decode_trb(char *str, size_t size, u32 field0, > case TRB_RESET_EP: > case TRB_HALT_ENDPOINT: > ret = scnprintf(str, size, > - "%s: ep%d%s(%d) ctx %08x%08x slot %ld flags %c", > + "%s: ep%d%s(%d) ctx %08x%08x slot %ld flags %c %c", > cdnsp_trb_type_string(type), > ep_num, ep_id % 2 ? "out" : "in", > TRB_TO_EP_INDEX(field3), field1, field0, > TRB_TO_SLOT_ID(field3), > - field3 & TRB_CYCLE ? 'C' : 'c'); > + field3 & TRB_CYCLE ? 'C' : 'c', > + field3 & TRB_ESP ? 'P' : 'p'); > break; > case TRB_STOP_RING: > ret = scnprintf(str, size, > diff --git a/drivers/usb/cdns3/cdnsp-ep0.c b/drivers/usb/cdns3/cdnsp-ep0.c > index f317d3c84781..5cd9b898ce97 100644 > --- a/drivers/usb/cdns3/cdnsp-ep0.c > +++ b/drivers/usb/cdns3/cdnsp-ep0.c > @@ -414,6 +414,7 @@ static int cdnsp_ep0_std_request(struct cdnsp_device *pdev, > void cdnsp_setup_analyze(struct cdnsp_device *pdev) > { > struct usb_ctrlrequest *ctrl = &pdev->setup; > + struct cdnsp_ep *pep; > int ret = -EINVAL; > u16 len; > > @@ -427,10 +428,21 @@ void cdnsp_setup_analyze(struct cdnsp_device *pdev) > goto out; > } > > + pep = &pdev->eps[0]; > + > /* Restore the ep0 to Stopped/Running state. */ > - if (pdev->eps[0].ep_state & EP_HALTED) { > - trace_cdnsp_ep0_halted("Restore to normal state"); > - cdnsp_halt_endpoint(pdev, &pdev->eps[0], 0); > + if (pep->ep_state & EP_HALTED) { > + if (GET_EP_CTX_STATE(pep->out_ctx) == EP_STATE_HALTED) > + cdnsp_halt_endpoint(pdev, pep, 0); > + > + /* > + * Halt Endpoint Command for SSP2 for ep0 preserve current > + * endpoint state and driver has to synchronize the > + * software endpoint state with endpoint output context > + * state. > + */ > + pep->ep_state &= ~EP_HALTED; > + pep->ep_state |= EP_STOPPED; You do not reset endpoint by calling clear_halt, could we change ep_state directly? Peter > } > > /* > diff --git a/drivers/usb/cdns3/cdnsp-gadget.h b/drivers/usb/cdns3/cdnsp-gadget.h > index 2afa3e558f85..a91cca509db0 100644 > --- a/drivers/usb/cdns3/cdnsp-gadget.h > +++ b/drivers/usb/cdns3/cdnsp-gadget.h > @@ -987,6 +987,12 @@ enum cdnsp_setup_dev { > #define STREAM_ID_FOR_TRB(p) ((((p)) << 16) & GENMASK(31, 16)) > #define SCT_FOR_TRB(p) (((p) << 1) & 0x7) > > +/* > + * Halt Endpoint Command TRB field. > + * The ESP bit only exists in the SSP2 controller. > + */ > +#define TRB_ESP BIT(9) > + > /* Link TRB specific fields. */ > #define TRB_TC BIT(1) > > diff --git a/drivers/usb/cdns3/cdnsp-ring.c b/drivers/usb/cdns3/cdnsp-ring.c > index fd06cb85c4ea..d397d28efc6e 100644 > --- a/drivers/usb/cdns3/cdnsp-ring.c > +++ b/drivers/usb/cdns3/cdnsp-ring.c > @@ -2483,7 +2483,8 @@ void cdnsp_queue_halt_endpoint(struct cdnsp_device *pdev, unsigned int ep_index) > { > cdnsp_queue_command(pdev, 0, 0, 0, TRB_TYPE(TRB_HALT_ENDPOINT) | > SLOT_ID_FOR_TRB(pdev->slot_id) | > - EP_ID_FOR_TRB(ep_index)); > + EP_ID_FOR_TRB(ep_index) | > + (!ep_index ? TRB_ESP : 0)); > } > > void cdnsp_force_header_wakeup(struct cdnsp_device *pdev, int intf_num) > -- > 2.43.0 > -- Best regards, Peter ^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH v3] usb: cdnsp: Fix issue with CV Bad Descriptor test 2025-06-21 0:36 ` Peter Chen (CIX) @ 2025-06-23 5:51 ` Pawel Laszczak 2025-06-23 8:22 ` Peter Chen (CIX) 0 siblings, 1 reply; 6+ messages in thread From: Pawel Laszczak @ 2025-06-23 5:51 UTC (permalink / raw) To: Peter Chen (CIX) Cc: gregkh@linuxfoundation.org, linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org, stable@vger.kernel.org >On 25-06-20 08:23:12, Pawel Laszczak wrote: >> The SSP2 controller has extra endpoint state preserve bit (ESP) which >> setting causes that endpoint state will be preserved during Halt >> Endpoint command. It is used only for EP0. >> Without this bit the Command Verifier "TD 9.10 Bad Descriptor Test" >> failed. >> Setting this bit doesn't have any impact for SSP controller. >> >> Fixes: 3d82904559f4 ("usb: cdnsp: cdns3 Add main part of Cadence >> USBSSP DRD Driver") >> cc: stable@vger.kernel.org >> Signed-off-by: Pawel Laszczak <pawell@cadence.com> >> --- >> Changelog: >> v3: >> - removed else {} >> >> v2: >> - removed some typos >> - added pep variable initialization >> - updated TRB_ESP description >> >> drivers/usb/cdns3/cdnsp-debug.h | 5 +++-- >> drivers/usb/cdns3/cdnsp-ep0.c | 18 +++++++++++++++--- >> drivers/usb/cdns3/cdnsp-gadget.h | 6 ++++++ >> drivers/usb/cdns3/cdnsp-ring.c | 3 ++- >> 4 files changed, 26 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/usb/cdns3/cdnsp-debug.h >> b/drivers/usb/cdns3/cdnsp-debug.h index cd138acdcce1..86860686d836 >> 100644 >> --- a/drivers/usb/cdns3/cdnsp-debug.h >> +++ b/drivers/usb/cdns3/cdnsp-debug.h >> @@ -327,12 +327,13 @@ static inline const char *cdnsp_decode_trb(char >*str, size_t size, u32 field0, >> case TRB_RESET_EP: >> case TRB_HALT_ENDPOINT: >> ret = scnprintf(str, size, >> - "%s: ep%d%s(%d) ctx %08x%08x slot %ld flags >%c", >> + "%s: ep%d%s(%d) ctx %08x%08x slot %ld flags >%c %c", >> cdnsp_trb_type_string(type), >> ep_num, ep_id % 2 ? "out" : "in", >> TRB_TO_EP_INDEX(field3), field1, field0, >> TRB_TO_SLOT_ID(field3), >> - field3 & TRB_CYCLE ? 'C' : 'c'); >> + field3 & TRB_CYCLE ? 'C' : 'c', >> + field3 & TRB_ESP ? 'P' : 'p'); >> break; >> case TRB_STOP_RING: >> ret = scnprintf(str, size, >> diff --git a/drivers/usb/cdns3/cdnsp-ep0.c >> b/drivers/usb/cdns3/cdnsp-ep0.c index f317d3c84781..5cd9b898ce97 >> 100644 >> --- a/drivers/usb/cdns3/cdnsp-ep0.c >> +++ b/drivers/usb/cdns3/cdnsp-ep0.c >> @@ -414,6 +414,7 @@ static int cdnsp_ep0_std_request(struct >> cdnsp_device *pdev, void cdnsp_setup_analyze(struct cdnsp_device >> *pdev) { >> struct usb_ctrlrequest *ctrl = &pdev->setup; >> + struct cdnsp_ep *pep; >> int ret = -EINVAL; >> u16 len; >> >> @@ -427,10 +428,21 @@ void cdnsp_setup_analyze(struct cdnsp_device >*pdev) >> goto out; >> } >> >> + pep = &pdev->eps[0]; >> + >> /* Restore the ep0 to Stopped/Running state. */ >> - if (pdev->eps[0].ep_state & EP_HALTED) { >> - trace_cdnsp_ep0_halted("Restore to normal state"); >> - cdnsp_halt_endpoint(pdev, &pdev->eps[0], 0); >> + if (pep->ep_state & EP_HALTED) { >> + if (GET_EP_CTX_STATE(pep->out_ctx) == EP_STATE_HALTED) >> + cdnsp_halt_endpoint(pdev, pep, 0); >> + >> + /* >> + * Halt Endpoint Command for SSP2 for ep0 preserve current >> + * endpoint state and driver has to synchronize the >> + * software endpoint state with endpoint output context >> + * state. >> + */ >> + pep->ep_state &= ~EP_HALTED; >> + pep->ep_state |= EP_STOPPED; > >You do not reset endpoint by calling clear_halt, could we change ep_state >directly? It's only "software" endpoint state and this code is related only with ep0. For SSP2 the state in pep->out_ctx - "hardware" endpoint state in this place will be in EP_STATE_STOPPED but "software" pep->ep_state will be EP_HALTED. Driver only synchronizes pep->ep_state with this included in pep->out_ctx. For SSP the state in pep->out_ctx - "hardware" endpoint state in this please will be in EP_STATE_HALTED, and "software" pep->ep_state will be EP_HALTED. For SSP driver will call cdnsp_halt_endpoint in which it changes the "hardware" and "software" endpoint state to EP_STOPPED/EP_STATE_STOPPED. So for SSP the extra code: pep->ep_state &= ~EP_HALTED; pep->ep_state |= EP_STOPPED; will not change anything Pawel > >Peter >> } >> >> /* >> diff --git a/drivers/usb/cdns3/cdnsp-gadget.h >> b/drivers/usb/cdns3/cdnsp-gadget.h >> index 2afa3e558f85..a91cca509db0 100644 >> --- a/drivers/usb/cdns3/cdnsp-gadget.h >> +++ b/drivers/usb/cdns3/cdnsp-gadget.h >> @@ -987,6 +987,12 @@ enum cdnsp_setup_dev { >> #define STREAM_ID_FOR_TRB(p) ((((p)) << 16) & GENMASK(31, >16)) >> #define SCT_FOR_TRB(p) (((p) << 1) & 0x7) >> >> +/* >> + * Halt Endpoint Command TRB field. >> + * The ESP bit only exists in the SSP2 controller. >> + */ >> +#define TRB_ESP BIT(9) >> + >> /* Link TRB specific fields. */ >> #define TRB_TC BIT(1) >> >> diff --git a/drivers/usb/cdns3/cdnsp-ring.c >> b/drivers/usb/cdns3/cdnsp-ring.c index fd06cb85c4ea..d397d28efc6e >> 100644 >> --- a/drivers/usb/cdns3/cdnsp-ring.c >> +++ b/drivers/usb/cdns3/cdnsp-ring.c >> @@ -2483,7 +2483,8 @@ void cdnsp_queue_halt_endpoint(struct >> cdnsp_device *pdev, unsigned int ep_index) { >> cdnsp_queue_command(pdev, 0, 0, 0, TRB_TYPE(TRB_HALT_ENDPOINT) >| >> SLOT_ID_FOR_TRB(pdev->slot_id) | >> - EP_ID_FOR_TRB(ep_index)); >> + EP_ID_FOR_TRB(ep_index) | >> + (!ep_index ? TRB_ESP : 0)); >> } >> >> void cdnsp_force_header_wakeup(struct cdnsp_device *pdev, int >> intf_num) >> -- >> 2.43.0 >> > >-- > >Best regards, >Peter ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3] usb: cdnsp: Fix issue with CV Bad Descriptor test 2025-06-23 5:51 ` Pawel Laszczak @ 2025-06-23 8:22 ` Peter Chen (CIX) 2025-06-23 10:54 ` Pawel Laszczak 0 siblings, 1 reply; 6+ messages in thread From: Peter Chen (CIX) @ 2025-06-23 8:22 UTC (permalink / raw) To: Pawel Laszczak Cc: gregkh@linuxfoundation.org, linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org, stable@vger.kernel.org On 25-06-23 05:51:08, Pawel Laszczak wrote: > >On 25-06-20 08:23:12, Pawel Laszczak wrote: > >> The SSP2 controller has extra endpoint state preserve bit (ESP) which > >> setting causes that endpoint state will be preserved during Halt > >> Endpoint command. It is used only for EP0. > >> Without this bit the Command Verifier "TD 9.10 Bad Descriptor Test" > >> failed. > >> Setting this bit doesn't have any impact for SSP controller. > >> > >> Fixes: 3d82904559f4 ("usb: cdnsp: cdns3 Add main part of Cadence > >> USBSSP DRD Driver") > >> cc: stable@vger.kernel.org > >> Signed-off-by: Pawel Laszczak <pawell@cadence.com> > >> --- > >> Changelog: > >> v3: > >> - removed else {} > >> > >> v2: > >> - removed some typos > >> - added pep variable initialization > >> - updated TRB_ESP description > >> > >> drivers/usb/cdns3/cdnsp-debug.h | 5 +++-- > >> drivers/usb/cdns3/cdnsp-ep0.c | 18 +++++++++++++++--- > >> drivers/usb/cdns3/cdnsp-gadget.h | 6 ++++++ > >> drivers/usb/cdns3/cdnsp-ring.c | 3 ++- > >> 4 files changed, 26 insertions(+), 6 deletions(-) > >> > >> diff --git a/drivers/usb/cdns3/cdnsp-debug.h > >> b/drivers/usb/cdns3/cdnsp-debug.h index cd138acdcce1..86860686d836 > >> 100644 > >> --- a/drivers/usb/cdns3/cdnsp-debug.h > >> +++ b/drivers/usb/cdns3/cdnsp-debug.h > >> @@ -327,12 +327,13 @@ static inline const char *cdnsp_decode_trb(char > >*str, size_t size, u32 field0, > >> case TRB_RESET_EP: > >> case TRB_HALT_ENDPOINT: > >> ret = scnprintf(str, size, > >> - "%s: ep%d%s(%d) ctx %08x%08x slot %ld flags > >%c", > >> + "%s: ep%d%s(%d) ctx %08x%08x slot %ld flags > >%c %c", > >> cdnsp_trb_type_string(type), > >> ep_num, ep_id % 2 ? "out" : "in", > >> TRB_TO_EP_INDEX(field3), field1, field0, > >> TRB_TO_SLOT_ID(field3), > >> - field3 & TRB_CYCLE ? 'C' : 'c'); > >> + field3 & TRB_CYCLE ? 'C' : 'c', > >> + field3 & TRB_ESP ? 'P' : 'p'); > >> break; > >> case TRB_STOP_RING: > >> ret = scnprintf(str, size, > >> diff --git a/drivers/usb/cdns3/cdnsp-ep0.c > >> b/drivers/usb/cdns3/cdnsp-ep0.c index f317d3c84781..5cd9b898ce97 > >> 100644 > >> --- a/drivers/usb/cdns3/cdnsp-ep0.c > >> +++ b/drivers/usb/cdns3/cdnsp-ep0.c > >> @@ -414,6 +414,7 @@ static int cdnsp_ep0_std_request(struct > >> cdnsp_device *pdev, void cdnsp_setup_analyze(struct cdnsp_device > >> *pdev) { > >> struct usb_ctrlrequest *ctrl = &pdev->setup; > >> + struct cdnsp_ep *pep; > >> int ret = -EINVAL; > >> u16 len; > >> > >> @@ -427,10 +428,21 @@ void cdnsp_setup_analyze(struct cdnsp_device > >*pdev) > >> goto out; > >> } > >> > >> + pep = &pdev->eps[0]; > >> + > >> /* Restore the ep0 to Stopped/Running state. */ > >> - if (pdev->eps[0].ep_state & EP_HALTED) { > >> - trace_cdnsp_ep0_halted("Restore to normal state"); > >> - cdnsp_halt_endpoint(pdev, &pdev->eps[0], 0); > >> + if (pep->ep_state & EP_HALTED) { > >> + if (GET_EP_CTX_STATE(pep->out_ctx) == EP_STATE_HALTED) > >> + cdnsp_halt_endpoint(pdev, pep, 0); You mean above is only called for SSP? And below two lines needs to be executed no matter cdnsp_halt_endpoint(pdev, pep, 0) is called? pep->ep_state &= ~EP_HALTED; pep->ep_state |= EP_STOPPED; If it is the case, I am okay with this patch. Peter > >> + > >> + /* > >> + * Halt Endpoint Command for SSP2 for ep0 preserve current > >> + * endpoint state and driver has to synchronize the > >> + * software endpoint state with endpoint output context > >> + * state. > >> + */ > >> + pep->ep_state &= ~EP_HALTED; > >> + pep->ep_state |= EP_STOPPED; > > > >You do not reset endpoint by calling clear_halt, could we change ep_state > >directly? > > It's only "software" endpoint state and this code is related only with ep0. > For SSP2 the state in pep->out_ctx - "hardware" endpoint state in this > place will be in EP_STATE_STOPPED but "software" pep->ep_state > will be EP_HALTED. > Driver only synchronizes pep->ep_state with this included in pep->out_ctx. > > For SSP the state in pep->out_ctx - "hardware" endpoint state in this please > will be in EP_STATE_HALTED, and "software" pep->ep_state will be > EP_HALTED. For SSP driver will call cdnsp_halt_endpoint in which > it changes the "hardware" and "software" endpoint state > to EP_STOPPED/EP_STATE_STOPPED. > > So for SSP the extra code: > pep->ep_state &= ~EP_HALTED; > pep->ep_state |= EP_STOPPED; > will not change anything > > Pawel > > > > >Peter > >> } > >> > >> /* > >> diff --git a/drivers/usb/cdns3/cdnsp-gadget.h > >> b/drivers/usb/cdns3/cdnsp-gadget.h > >> index 2afa3e558f85..a91cca509db0 100644 > >> --- a/drivers/usb/cdns3/cdnsp-gadget.h > >> +++ b/drivers/usb/cdns3/cdnsp-gadget.h > >> @@ -987,6 +987,12 @@ enum cdnsp_setup_dev { > >> #define STREAM_ID_FOR_TRB(p) ((((p)) << 16) & GENMASK(31, > >16)) > >> #define SCT_FOR_TRB(p) (((p) << 1) & 0x7) > >> > >> +/* > >> + * Halt Endpoint Command TRB field. > >> + * The ESP bit only exists in the SSP2 controller. > >> + */ > >> +#define TRB_ESP BIT(9) > >> + > >> /* Link TRB specific fields. */ > >> #define TRB_TC BIT(1) > >> > >> diff --git a/drivers/usb/cdns3/cdnsp-ring.c > >> b/drivers/usb/cdns3/cdnsp-ring.c index fd06cb85c4ea..d397d28efc6e > >> 100644 > >> --- a/drivers/usb/cdns3/cdnsp-ring.c > >> +++ b/drivers/usb/cdns3/cdnsp-ring.c > >> @@ -2483,7 +2483,8 @@ void cdnsp_queue_halt_endpoint(struct > >> cdnsp_device *pdev, unsigned int ep_index) { > >> cdnsp_queue_command(pdev, 0, 0, 0, TRB_TYPE(TRB_HALT_ENDPOINT) > >| > >> SLOT_ID_FOR_TRB(pdev->slot_id) | > >> - EP_ID_FOR_TRB(ep_index)); > >> + EP_ID_FOR_TRB(ep_index) | > >> + (!ep_index ? TRB_ESP : 0)); > >> } > >> > >> void cdnsp_force_header_wakeup(struct cdnsp_device *pdev, int > >> intf_num) > >> -- > >> 2.43.0 > >> > > > >-- > > > >Best regards, > >Peter -- Best regards, Peter ^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH v3] usb: cdnsp: Fix issue with CV Bad Descriptor test 2025-06-23 8:22 ` Peter Chen (CIX) @ 2025-06-23 10:54 ` Pawel Laszczak 0 siblings, 0 replies; 6+ messages in thread From: Pawel Laszczak @ 2025-06-23 10:54 UTC (permalink / raw) To: Peter Chen (CIX) Cc: gregkh@linuxfoundation.org, linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org, stable@vger.kernel.org > > >On 25-06-23 05:51:08, Pawel Laszczak wrote: >> >On 25-06-20 08:23:12, Pawel Laszczak wrote: >> >> The SSP2 controller has extra endpoint state preserve bit (ESP) >> >> which setting causes that endpoint state will be preserved during >> >> Halt Endpoint command. It is used only for EP0. >> >> Without this bit the Command Verifier "TD 9.10 Bad Descriptor Test" >> >> failed. >> >> Setting this bit doesn't have any impact for SSP controller. >> >> >> >> Fixes: 3d82904559f4 ("usb: cdnsp: cdns3 Add main part of Cadence >> >> USBSSP DRD Driver") >> >> cc: stable@vger.kernel.org >> >> Signed-off-by: Pawel Laszczak <pawell@cadence.com> >> >> --- >> >> Changelog: >> >> v3: >> >> - removed else {} >> >> >> >> v2: >> >> - removed some typos >> >> - added pep variable initialization >> >> - updated TRB_ESP description >> >> >> >> drivers/usb/cdns3/cdnsp-debug.h | 5 +++-- >> >> drivers/usb/cdns3/cdnsp-ep0.c | 18 +++++++++++++++--- >> >> drivers/usb/cdns3/cdnsp-gadget.h | 6 ++++++ >> >> drivers/usb/cdns3/cdnsp-ring.c | 3 ++- >> >> 4 files changed, 26 insertions(+), 6 deletions(-) >> >> >> >> diff --git a/drivers/usb/cdns3/cdnsp-debug.h >> >> b/drivers/usb/cdns3/cdnsp-debug.h index cd138acdcce1..86860686d836 >> >> 100644 >> >> --- a/drivers/usb/cdns3/cdnsp-debug.h >> >> +++ b/drivers/usb/cdns3/cdnsp-debug.h >> >> @@ -327,12 +327,13 @@ static inline const char >> >> *cdnsp_decode_trb(char >> >*str, size_t size, u32 field0, >> >> case TRB_RESET_EP: >> >> case TRB_HALT_ENDPOINT: >> >> ret = scnprintf(str, size, >> >> - "%s: ep%d%s(%d) ctx %08x%08x slot %ld flags >> >%c", >> >> + "%s: ep%d%s(%d) ctx %08x%08x slot %ld flags >> >%c %c", >> >> cdnsp_trb_type_string(type), >> >> ep_num, ep_id % 2 ? "out" : "in", >> >> TRB_TO_EP_INDEX(field3), field1, field0, >> >> TRB_TO_SLOT_ID(field3), >> >> - field3 & TRB_CYCLE ? 'C' : 'c'); >> >> + field3 & TRB_CYCLE ? 'C' : 'c', >> >> + field3 & TRB_ESP ? 'P' : 'p'); >> >> break; >> >> case TRB_STOP_RING: >> >> ret = scnprintf(str, size, >> >> diff --git a/drivers/usb/cdns3/cdnsp-ep0.c >> >> b/drivers/usb/cdns3/cdnsp-ep0.c index f317d3c84781..5cd9b898ce97 >> >> 100644 >> >> --- a/drivers/usb/cdns3/cdnsp-ep0.c >> >> +++ b/drivers/usb/cdns3/cdnsp-ep0.c >> >> @@ -414,6 +414,7 @@ static int cdnsp_ep0_std_request(struct >> >> cdnsp_device *pdev, void cdnsp_setup_analyze(struct cdnsp_device >> >> *pdev) { >> >> struct usb_ctrlrequest *ctrl = &pdev->setup; >> >> + struct cdnsp_ep *pep; >> >> int ret = -EINVAL; >> >> u16 len; >> >> >> >> @@ -427,10 +428,21 @@ void cdnsp_setup_analyze(struct cdnsp_device >> >*pdev) >> >> goto out; >> >> } >> >> >> >> + pep = &pdev->eps[0]; >> >> + >> >> /* Restore the ep0 to Stopped/Running state. */ >> >> - if (pdev->eps[0].ep_state & EP_HALTED) { >> >> - trace_cdnsp_ep0_halted("Restore to normal state"); >> >> - cdnsp_halt_endpoint(pdev, &pdev->eps[0], 0); >> >> + if (pep->ep_state & EP_HALTED) { >> >> + if (GET_EP_CTX_STATE(pep->out_ctx) == EP_STATE_HALTED) >> >> + cdnsp_halt_endpoint(pdev, pep, 0); > >You mean above is only called for SSP? And below two lines needs to be executed >no matter cdnsp_halt_endpoint(pdev, pep, 0) is called? The answer is "yes" for both your questions. > >pep->ep_state &= ~EP_HALTED; >pep->ep_state |= EP_STOPPED; > >If it is the case, I am okay with this patch. > >Peter > >> >> + >> >> + /* >> >> + * Halt Endpoint Command for SSP2 for ep0 preserve current >> >> + * endpoint state and driver has to synchronize the >> >> + * software endpoint state with endpoint output context >> >> + * state. >> >> + */ >> >> + pep->ep_state &= ~EP_HALTED; >> >> + pep->ep_state |= EP_STOPPED; >> > >> >You do not reset endpoint by calling clear_halt, could we change >> >ep_state directly? >> >> It's only "software" endpoint state and this code is related only with ep0. >> For SSP2 the state in pep->out_ctx - "hardware" endpoint state in this >> place will be in EP_STATE_STOPPED but "software" pep->ep_state will be >> EP_HALTED. >> Driver only synchronizes pep->ep_state with this included in pep->out_ctx. >> >> For SSP the state in pep->out_ctx - "hardware" endpoint state in this >> please will be in EP_STATE_HALTED, and "software" pep->ep_state will >> be EP_HALTED. For SSP driver will call cdnsp_halt_endpoint in which it >> changes the "hardware" and "software" endpoint state to >> EP_STOPPED/EP_STATE_STOPPED. >> >> So for SSP the extra code: >> pep->ep_state &= ~EP_HALTED; >> pep->ep_state |= EP_STOPPED; >> will not change anything >> >> Pawel >> >> > >> >Peter >> >> } >> >> >> >> /* >> >> diff --git a/drivers/usb/cdns3/cdnsp-gadget.h >> >> b/drivers/usb/cdns3/cdnsp-gadget.h >> >> index 2afa3e558f85..a91cca509db0 100644 >> >> --- a/drivers/usb/cdns3/cdnsp-gadget.h >> >> +++ b/drivers/usb/cdns3/cdnsp-gadget.h >> >> @@ -987,6 +987,12 @@ enum cdnsp_setup_dev { >> >> #define STREAM_ID_FOR_TRB(p) ((((p)) << 16) & GENMASK(31, >> >16)) >> >> #define SCT_FOR_TRB(p) (((p) << 1) & 0x7) >> >> >> >> +/* >> >> + * Halt Endpoint Command TRB field. >> >> + * The ESP bit only exists in the SSP2 controller. >> >> + */ >> >> +#define TRB_ESP BIT(9) >> >> + >> >> /* Link TRB specific fields. */ >> >> #define TRB_TC BIT(1) >> >> >> >> diff --git a/drivers/usb/cdns3/cdnsp-ring.c >> >> b/drivers/usb/cdns3/cdnsp-ring.c index fd06cb85c4ea..d397d28efc6e >> >> 100644 >> >> --- a/drivers/usb/cdns3/cdnsp-ring.c >> >> +++ b/drivers/usb/cdns3/cdnsp-ring.c >> >> @@ -2483,7 +2483,8 @@ void cdnsp_queue_halt_endpoint(struct >> >> cdnsp_device *pdev, unsigned int ep_index) { >> >> cdnsp_queue_command(pdev, 0, 0, 0, TRB_TYPE(TRB_HALT_ENDPOINT) >> >| >> >> SLOT_ID_FOR_TRB(pdev->slot_id) | >> >> - EP_ID_FOR_TRB(ep_index)); >> >> + EP_ID_FOR_TRB(ep_index) | >> >> + (!ep_index ? TRB_ESP : 0)); >> >> } >> >> >> >> void cdnsp_force_header_wakeup(struct cdnsp_device *pdev, int >> >> intf_num) >> >> -- >> >> 2.43.0 >> >> >> > >> >-- >> > >> >Best regards, >> >Peter > >-- > >Best regards, >Peter ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3] usb: cdnsp: Fix issue with CV Bad Descriptor test 2025-06-20 8:23 ` [PATCH v3] usb: cdnsp: Fix issue with CV Bad Descriptor test Pawel Laszczak 2025-06-21 0:36 ` Peter Chen (CIX) @ 2025-06-23 11:02 ` Peter Chen (CIX) 1 sibling, 0 replies; 6+ messages in thread From: Peter Chen (CIX) @ 2025-06-23 11:02 UTC (permalink / raw) To: Pawel Laszczak Cc: gregkh@linuxfoundation.org, linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org, stable@vger.kernel.org On 25-06-20 08:23:12, Pawel Laszczak wrote: > The SSP2 controller has extra endpoint state preserve bit (ESP) which > setting causes that endpoint state will be preserved during > Halt Endpoint command. It is used only for EP0. > Without this bit the Command Verifier "TD 9.10 Bad Descriptor Test" > failed. > Setting this bit doesn't have any impact for SSP controller. > > Fixes: 3d82904559f4 ("usb: cdnsp: cdns3 Add main part of Cadence USBSSP DRD Driver") > cc: stable@vger.kernel.org > Signed-off-by: Pawel Laszczak <pawell@cadence.com> Acked-by: Peter Chen <peter.chen@kernel.org> Peter > --- > Changelog: > v3: > - removed else {} > > v2: > - removed some typos > - added pep variable initialization > - updated TRB_ESP description > > drivers/usb/cdns3/cdnsp-debug.h | 5 +++-- > drivers/usb/cdns3/cdnsp-ep0.c | 18 +++++++++++++++--- > drivers/usb/cdns3/cdnsp-gadget.h | 6 ++++++ > drivers/usb/cdns3/cdnsp-ring.c | 3 ++- > 4 files changed, 26 insertions(+), 6 deletions(-) > > diff --git a/drivers/usb/cdns3/cdnsp-debug.h b/drivers/usb/cdns3/cdnsp-debug.h > index cd138acdcce1..86860686d836 100644 > --- a/drivers/usb/cdns3/cdnsp-debug.h > +++ b/drivers/usb/cdns3/cdnsp-debug.h > @@ -327,12 +327,13 @@ static inline const char *cdnsp_decode_trb(char *str, size_t size, u32 field0, > case TRB_RESET_EP: > case TRB_HALT_ENDPOINT: > ret = scnprintf(str, size, > - "%s: ep%d%s(%d) ctx %08x%08x slot %ld flags %c", > + "%s: ep%d%s(%d) ctx %08x%08x slot %ld flags %c %c", > cdnsp_trb_type_string(type), > ep_num, ep_id % 2 ? "out" : "in", > TRB_TO_EP_INDEX(field3), field1, field0, > TRB_TO_SLOT_ID(field3), > - field3 & TRB_CYCLE ? 'C' : 'c'); > + field3 & TRB_CYCLE ? 'C' : 'c', > + field3 & TRB_ESP ? 'P' : 'p'); > break; > case TRB_STOP_RING: > ret = scnprintf(str, size, > diff --git a/drivers/usb/cdns3/cdnsp-ep0.c b/drivers/usb/cdns3/cdnsp-ep0.c > index f317d3c84781..5cd9b898ce97 100644 > --- a/drivers/usb/cdns3/cdnsp-ep0.c > +++ b/drivers/usb/cdns3/cdnsp-ep0.c > @@ -414,6 +414,7 @@ static int cdnsp_ep0_std_request(struct cdnsp_device *pdev, > void cdnsp_setup_analyze(struct cdnsp_device *pdev) > { > struct usb_ctrlrequest *ctrl = &pdev->setup; > + struct cdnsp_ep *pep; > int ret = -EINVAL; > u16 len; > > @@ -427,10 +428,21 @@ void cdnsp_setup_analyze(struct cdnsp_device *pdev) > goto out; > } > > + pep = &pdev->eps[0]; > + > /* Restore the ep0 to Stopped/Running state. */ > - if (pdev->eps[0].ep_state & EP_HALTED) { > - trace_cdnsp_ep0_halted("Restore to normal state"); > - cdnsp_halt_endpoint(pdev, &pdev->eps[0], 0); > + if (pep->ep_state & EP_HALTED) { > + if (GET_EP_CTX_STATE(pep->out_ctx) == EP_STATE_HALTED) > + cdnsp_halt_endpoint(pdev, pep, 0); > + > + /* > + * Halt Endpoint Command for SSP2 for ep0 preserve current > + * endpoint state and driver has to synchronize the > + * software endpoint state with endpoint output context > + * state. > + */ > + pep->ep_state &= ~EP_HALTED; > + pep->ep_state |= EP_STOPPED; > } > > /* > diff --git a/drivers/usb/cdns3/cdnsp-gadget.h b/drivers/usb/cdns3/cdnsp-gadget.h > index 2afa3e558f85..a91cca509db0 100644 > --- a/drivers/usb/cdns3/cdnsp-gadget.h > +++ b/drivers/usb/cdns3/cdnsp-gadget.h > @@ -987,6 +987,12 @@ enum cdnsp_setup_dev { > #define STREAM_ID_FOR_TRB(p) ((((p)) << 16) & GENMASK(31, 16)) > #define SCT_FOR_TRB(p) (((p) << 1) & 0x7) > > +/* > + * Halt Endpoint Command TRB field. > + * The ESP bit only exists in the SSP2 controller. > + */ > +#define TRB_ESP BIT(9) > + > /* Link TRB specific fields. */ > #define TRB_TC BIT(1) > > diff --git a/drivers/usb/cdns3/cdnsp-ring.c b/drivers/usb/cdns3/cdnsp-ring.c > index fd06cb85c4ea..d397d28efc6e 100644 > --- a/drivers/usb/cdns3/cdnsp-ring.c > +++ b/drivers/usb/cdns3/cdnsp-ring.c > @@ -2483,7 +2483,8 @@ void cdnsp_queue_halt_endpoint(struct cdnsp_device *pdev, unsigned int ep_index) > { > cdnsp_queue_command(pdev, 0, 0, 0, TRB_TYPE(TRB_HALT_ENDPOINT) | > SLOT_ID_FOR_TRB(pdev->slot_id) | > - EP_ID_FOR_TRB(ep_index)); > + EP_ID_FOR_TRB(ep_index) | > + (!ep_index ? TRB_ESP : 0)); > } > > void cdnsp_force_header_wakeup(struct cdnsp_device *pdev, int intf_num) > -- > 2.43.0 > -- Best regards, Peter ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2025-06-23 11:02 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20250620074306.2278838-1-pawell@cadence.com>
2025-06-20 8:23 ` [PATCH v3] usb: cdnsp: Fix issue with CV Bad Descriptor test Pawel Laszczak
2025-06-21 0:36 ` Peter Chen (CIX)
2025-06-23 5:51 ` Pawel Laszczak
2025-06-23 8:22 ` Peter Chen (CIX)
2025-06-23 10:54 ` Pawel Laszczak
2025-06-23 11:02 ` Peter Chen (CIX)
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox