[PATCH] usb: cdns3: Do not access memory after free

[PATCH] usb: cdns3: Do not access memory after free

[Andrew Goodbody]

The call to cdns3_gadget_ep_free_request will free priv_req so do the
call to list_del_init which accesses the memory pointed to by priv_req
before the free.

This issue was found by Smatch.

Signed-off-by: Andrew Goodbody <andrew.goodbody@linaro.org>
---
 drivers/usb/cdns3/gadget.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/cdns3/gadget.c b/drivers/usb/cdns3/gadget.c
index a30c40ef80e..9eaf7e40ab6 100644
--- a/drivers/usb/cdns3/gadget.c
+++ b/drivers/usb/cdns3/gadget.c
@@ -557,10 +557,10 @@ static void cdns3_wa2_remove_old_request(struct cdns3_endpoint *priv_ep)
 
 		trace_cdns3_wa2(priv_ep, "removes eldest request");
 
+		list_del_init(&priv_req->list);
 		kfree(priv_req->request.buf);
 		cdns3_gadget_ep_free_request(&priv_ep->endpoint,
 					     &priv_req->request);
-		list_del_init(&priv_req->list);
 		--priv_ep->wa2_counter;
 
 		if (!chain)
@@ -1959,10 +1959,10 @@ static int cdns3_gadget_ep_disable(struct usb_ep *ep)
 	while (!list_empty(&priv_ep->wa2_descmiss_req_list)) {
 		priv_req = cdns3_next_priv_request(&priv_ep->wa2_descmiss_req_list);
 
+		list_del_init(&priv_req->list);
 		kfree(priv_req->request.buf);
 		cdns3_gadget_ep_free_request(&priv_ep->endpoint,
 					     &priv_req->request);
-		list_del_init(&priv_req->list);
 		--priv_ep->wa2_counter;
 	}
 

---
base-commit: 7807ed921314cd7af83fd88162d0b8c6fb20a9ca
change-id: 20250813-usb_cdns3-67b975488fe8

Best regards,
-- 
Andrew Goodbody <andrew.goodbody@linaro.org>

Re: [PATCH] usb: cdns3: Do not access memory after free

[Marek Vasut]

On 8/13/25 6:30 PM, Andrew Goodbody wrote:
> The call to cdns3_gadget_ep_free_request will free priv_req so do the
> call to list_del_init which accesses the memory pointed to by priv_req
> before the free.
> 
> This issue was found by Smatch.
> 
> Signed-off-by: Andrew Goodbody <andrew.goodbody@linaro.org>
> ---
>   drivers/usb/cdns3/gadget.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/cdns3/gadget.c b/drivers/usb/cdns3/gadget.c
> index a30c40ef80e..9eaf7e40ab6 100644
> --- a/drivers/usb/cdns3/gadget.c
> +++ b/drivers/usb/cdns3/gadget.c
> @@ -557,10 +557,10 @@ static void cdns3_wa2_remove_old_request(struct cdns3_endpoint *priv_ep)
>   
>   		trace_cdns3_wa2(priv_ep, "removes eldest request");
>   
> +		list_del_init(&priv_req->list);
>   		kfree(priv_req->request.buf);
>   		cdns3_gadget_ep_free_request(&priv_ep->endpoint,
>   					     &priv_req->request);
> -		list_del_init(&priv_req->list);

Shouldn't the kfree() be moved here instead ? 
cdns3_gadget_ep_free_request() also accesses priv_req->request .

>   		--priv_ep->wa2_counter;
>   
>   		if (!chain)
> @@ -1959,10 +1959,10 @@ static int cdns3_gadget_ep_disable(struct usb_ep *ep)
>   	while (!list_empty(&priv_ep->wa2_descmiss_req_list)) {
>   		priv_req = cdns3_next_priv_request(&priv_ep->wa2_descmiss_req_list);
>   
> +		list_del_init(&priv_req->list);
>   		kfree(priv_req->request.buf);
>   		cdns3_gadget_ep_free_request(&priv_ep->endpoint,
>   					     &priv_req->request);
> -		list_del_init(&priv_req->list);

DTTO ?

Re: [PATCH] usb: cdns3: Do not access memory after free

[Andrew Goodbody]

On 14/08/2025 04:21, Marek Vasut wrote:
> On 8/13/25 6:30 PM, Andrew Goodbody wrote:
>> The call to cdns3_gadget_ep_free_request will free priv_req so do the
>> call to list_del_init which accesses the memory pointed to by priv_req
>> before the free.
>>
>> This issue was found by Smatch.
>>
>> Signed-off-by: Andrew Goodbody <andrew.goodbody@linaro.org>
>> ---
>>   drivers/usb/cdns3/gadget.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/usb/cdns3/gadget.c b/drivers/usb/cdns3/gadget.c
>> index a30c40ef80e..9eaf7e40ab6 100644
>> --- a/drivers/usb/cdns3/gadget.c
>> +++ b/drivers/usb/cdns3/gadget.c
>> @@ -557,10 +557,10 @@ static void cdns3_wa2_remove_old_request(struct 
>> cdns3_endpoint *priv_ep)
>>           trace_cdns3_wa2(priv_ep, "removes eldest request");
>> +        list_del_init(&priv_req->list);
>>           kfree(priv_req->request.buf);
>>           cdns3_gadget_ep_free_request(&priv_ep->endpoint,
>>                            &priv_req->request);
>> -        list_del_init(&priv_req->list);
> 
> Shouldn't the kfree() be moved here instead ? 
> cdns3_gadget_ep_free_request() also accesses priv_req->request .

No, I do not think so. The kfree frees priv_req->request.buf not 
priv_req->request so must happen before the call to 
cdns3_gadget_ep_free_request. Nothing should touch priv_req after the 
call to to cdns3_gadget_ep_free_request. Moving the kfree as you suggest 
would make the problem worse.

Andrew

>>           --priv_ep->wa2_counter;
>>           if (!chain)
>> @@ -1959,10 +1959,10 @@ static int cdns3_gadget_ep_disable(struct 
>> usb_ep *ep)
>>       while (!list_empty(&priv_ep->wa2_descmiss_req_list)) {
>>           priv_req = cdns3_next_priv_request(&priv_ep- 
>> >wa2_descmiss_req_list);
>> +        list_del_init(&priv_req->list);
>>           kfree(priv_req->request.buf);
>>           cdns3_gadget_ep_free_request(&priv_ep->endpoint,
>>                            &priv_req->request);
>> -        list_del_init(&priv_req->list);
> 
> DTTO ?

Re: [PATCH] usb: cdns3: Do not access memory after free

[Marek Vasut]

On 8/14/25 12:45 PM, Andrew Goodbody wrote:
> On 14/08/2025 04:21, Marek Vasut wrote:
>> On 8/13/25 6:30 PM, Andrew Goodbody wrote:
>>> The call to cdns3_gadget_ep_free_request will free priv_req so do the
>>> call to list_del_init which accesses the memory pointed to by priv_req
>>> before the free.
>>>
>>> This issue was found by Smatch.
>>>
>>> Signed-off-by: Andrew Goodbody <andrew.goodbody@linaro.org>
>>> ---
>>>   drivers/usb/cdns3/gadget.c | 4 ++--
>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/usb/cdns3/gadget.c b/drivers/usb/cdns3/gadget.c
>>> index a30c40ef80e..9eaf7e40ab6 100644
>>> --- a/drivers/usb/cdns3/gadget.c
>>> +++ b/drivers/usb/cdns3/gadget.c
>>> @@ -557,10 +557,10 @@ static void cdns3_wa2_remove_old_request(struct 
>>> cdns3_endpoint *priv_ep)
>>>           trace_cdns3_wa2(priv_ep, "removes eldest request");
>>> +        list_del_init(&priv_req->list);
>>>           kfree(priv_req->request.buf);
>>>           cdns3_gadget_ep_free_request(&priv_ep->endpoint,
>>>                            &priv_req->request);
>>> -        list_del_init(&priv_req->list);
>>
>> Shouldn't the kfree() be moved here instead ? 
>> cdns3_gadget_ep_free_request() also accesses priv_req->request .
> 
> No, I do not think so. The kfree frees priv_req->request.buf not 
> priv_req->request so must happen before the call to 
> cdns3_gadget_ep_free_request.
Thank you for clarifying. Please add this into the commit message, 
ideally wait for TI to test this and provide RB, then send V2 so this 
can go in.

Re: [PATCH] usb: cdns3: Do not access memory after free

[Siddharth Vadapalli]

On Tue, Aug 19, 2025 at 04:51:25PM +0200, Marek Vasut wrote:

Hello Marek,

> On 8/14/25 12:45 PM, Andrew Goodbody wrote:
> > On 14/08/2025 04:21, Marek Vasut wrote:
> > > On 8/13/25 6:30 PM, Andrew Goodbody wrote:
> > > > The call to cdns3_gadget_ep_free_request will free priv_req so do the
> > > > call to list_del_init which accesses the memory pointed to by priv_req
> > > > before the free.
> > > > 
> > > > This issue was found by Smatch.
> > > > 
> > > > Signed-off-by: Andrew Goodbody <andrew.goodbody@linaro.org>
> > > > ---
> > > >   drivers/usb/cdns3/gadget.c | 4 ++--
> > > >   1 file changed, 2 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/usb/cdns3/gadget.c b/drivers/usb/cdns3/gadget.c
> > > > index a30c40ef80e..9eaf7e40ab6 100644
> > > > --- a/drivers/usb/cdns3/gadget.c
> > > > +++ b/drivers/usb/cdns3/gadget.c
> > > > @@ -557,10 +557,10 @@ static void
> > > > cdns3_wa2_remove_old_request(struct cdns3_endpoint *priv_ep)
> > > >           trace_cdns3_wa2(priv_ep, "removes eldest request");
> > > > +        list_del_init(&priv_req->list);
> > > >           kfree(priv_req->request.buf);
> > > >           cdns3_gadget_ep_free_request(&priv_ep->endpoint,
> > > >                            &priv_req->request);
> > > > -        list_del_init(&priv_req->list);
> > > 
> > > Shouldn't the kfree() be moved here instead ?
> > > cdns3_gadget_ep_free_request() also accesses priv_req->request .
> > 
> > No, I do not think so. The kfree frees priv_req->request.buf not
> > priv_req->request so must happen before the call to
> > cdns3_gadget_ep_free_request.
> Thank you for clarifying. Please add this into the commit message, ideally
> wait for TI to test this and provide RB, then send V2 so this can go in.

I was planning to test this patch but the change being made is only
applicable to Controller Versions:
	#define DEV_VER_NXP_V1		0x00024502
	#define DEV_VER_TI_V1		0x00024509
and not to:
	#define DEV_VER_V2		0x0002450C
	#define DEV_VER_V3		0x0002450d

Since I don't have an SoC and a Board with DEV_VER_TI_V1, I cannot test
it. However, the change looks correct to me.

Reviewed-by: Siddharth Vadapalli <s-vadapalli@ti.com>

Regards,
Siddharth.

Re: [PATCH] usb: cdns3: Do not access memory after free

[Marek Vasut]

On 8/21/25 8:16 AM, Siddharth Vadapalli wrote:

Hi,

>>>>> diff --git a/drivers/usb/cdns3/gadget.c b/drivers/usb/cdns3/gadget.c
>>>>> index a30c40ef80e..9eaf7e40ab6 100644
>>>>> --- a/drivers/usb/cdns3/gadget.c
>>>>> +++ b/drivers/usb/cdns3/gadget.c
>>>>> @@ -557,10 +557,10 @@ static void
>>>>> cdns3_wa2_remove_old_request(struct cdns3_endpoint *priv_ep)
>>>>>            trace_cdns3_wa2(priv_ep, "removes eldest request");
>>>>> +        list_del_init(&priv_req->list);
>>>>>            kfree(priv_req->request.buf);
>>>>>            cdns3_gadget_ep_free_request(&priv_ep->endpoint,
>>>>>                             &priv_req->request);
>>>>> -        list_del_init(&priv_req->list);
>>>>
>>>> Shouldn't the kfree() be moved here instead ?
>>>> cdns3_gadget_ep_free_request() also accesses priv_req->request .
>>>
>>> No, I do not think so. The kfree frees priv_req->request.buf not
>>> priv_req->request so must happen before the call to
>>> cdns3_gadget_ep_free_request.
>> Thank you for clarifying. Please add this into the commit message, ideally
>> wait for TI to test this and provide RB, then send V2 so this can go in.
> 
> I was planning to test this patch but the change being made is only
> applicable to Controller Versions:
> 	#define DEV_VER_NXP_V1		0x00024502
> 	#define DEV_VER_TI_V1		0x00024509
> and not to:
> 	#define DEV_VER_V2		0x0002450C
> 	#define DEV_VER_V3		0x0002450d
> 
> Since I don't have an SoC and a Board with DEV_VER_TI_V1, I cannot test
> it. However, the change looks correct to me.
> 
> Reviewed-by: Siddharth Vadapalli <s-vadapalli@ti.com>
The change does indeed look correct.

Do you know who might still have that board and could test ? (and which 
board/soc is that) ?

Re: [PATCH] usb: cdns3: Do not access memory after free

[Siddharth Vadapalli]

On Thu, Aug 21, 2025 at 10:22:43PM +0200, Marek Vasut wrote:
> On 8/21/25 8:16 AM, Siddharth Vadapalli wrote:
> 
> Hi,
> 
> > > > > > diff --git a/drivers/usb/cdns3/gadget.c b/drivers/usb/cdns3/gadget.c
> > > > > > index a30c40ef80e..9eaf7e40ab6 100644
> > > > > > --- a/drivers/usb/cdns3/gadget.c
> > > > > > +++ b/drivers/usb/cdns3/gadget.c
> > > > > > @@ -557,10 +557,10 @@ static void
> > > > > > cdns3_wa2_remove_old_request(struct cdns3_endpoint *priv_ep)
> > > > > >            trace_cdns3_wa2(priv_ep, "removes eldest request");
> > > > > > +        list_del_init(&priv_req->list);
> > > > > >            kfree(priv_req->request.buf);
> > > > > >            cdns3_gadget_ep_free_request(&priv_ep->endpoint,
> > > > > >                             &priv_req->request);
> > > > > > -        list_del_init(&priv_req->list);
> > > > > 
> > > > > Shouldn't the kfree() be moved here instead ?
> > > > > cdns3_gadget_ep_free_request() also accesses priv_req->request .
> > > > 
> > > > No, I do not think so. The kfree frees priv_req->request.buf not
> > > > priv_req->request so must happen before the call to
> > > > cdns3_gadget_ep_free_request.
> > > Thank you for clarifying. Please add this into the commit message, ideally
> > > wait for TI to test this and provide RB, then send V2 so this can go in.
> > 
> > I was planning to test this patch but the change being made is only
> > applicable to Controller Versions:
> > 	#define DEV_VER_NXP_V1		0x00024502
> > 	#define DEV_VER_TI_V1		0x00024509
> > and not to:
> > 	#define DEV_VER_V2		0x0002450C
> > 	#define DEV_VER_V3		0x0002450d
> > 
> > Since I don't have an SoC and a Board with DEV_VER_TI_V1, I cannot test
> > it. However, the change looks correct to me.
> > 
> > Reviewed-by: Siddharth Vadapalli <s-vadapalli@ti.com>
> The change does indeed look correct.
> 
> Do you know who might still have that board and could test ? (and which
> board/soc is that) ?

None of the boards that I have worked with have a DEV_VER_TI_V1 version
of the controller. I also tried to use the Linux device-tree to check if
I could identify the SoC/board but I was unable to do so.

Regards,
Siddharth.

Re: [PATCH] usb: cdns3: Do not access memory after free

[Marek Vasut]

On 8/22/25 4:14 PM, Siddharth Vadapalli wrote:
> On Thu, Aug 21, 2025 at 10:22:43PM +0200, Marek Vasut wrote:
>> On 8/21/25 8:16 AM, Siddharth Vadapalli wrote:
>>
>> Hi,
>>
>>>>>>> diff --git a/drivers/usb/cdns3/gadget.c b/drivers/usb/cdns3/gadget.c
>>>>>>> index a30c40ef80e..9eaf7e40ab6 100644
>>>>>>> --- a/drivers/usb/cdns3/gadget.c
>>>>>>> +++ b/drivers/usb/cdns3/gadget.c
>>>>>>> @@ -557,10 +557,10 @@ static void
>>>>>>> cdns3_wa2_remove_old_request(struct cdns3_endpoint *priv_ep)
>>>>>>>             trace_cdns3_wa2(priv_ep, "removes eldest request");
>>>>>>> +        list_del_init(&priv_req->list);
>>>>>>>             kfree(priv_req->request.buf);
>>>>>>>             cdns3_gadget_ep_free_request(&priv_ep->endpoint,
>>>>>>>                              &priv_req->request);
>>>>>>> -        list_del_init(&priv_req->list);
>>>>>>
>>>>>> Shouldn't the kfree() be moved here instead ?
>>>>>> cdns3_gadget_ep_free_request() also accesses priv_req->request .
>>>>>
>>>>> No, I do not think so. The kfree frees priv_req->request.buf not
>>>>> priv_req->request so must happen before the call to
>>>>> cdns3_gadget_ep_free_request.
>>>> Thank you for clarifying. Please add this into the commit message, ideally
>>>> wait for TI to test this and provide RB, then send V2 so this can go in.
>>>
>>> I was planning to test this patch but the change being made is only
>>> applicable to Controller Versions:
>>> 	#define DEV_VER_NXP_V1		0x00024502
>>> 	#define DEV_VER_TI_V1		0x00024509
>>> and not to:
>>> 	#define DEV_VER_V2		0x0002450C
>>> 	#define DEV_VER_V3		0x0002450d
>>>
>>> Since I don't have an SoC and a Board with DEV_VER_TI_V1, I cannot test
>>> it. However, the change looks correct to me.
>>>
>>> Reviewed-by: Siddharth Vadapalli <s-vadapalli@ti.com>
>> The change does indeed look correct.
>>
>> Do you know who might still have that board and could test ? (and which
>> board/soc is that) ?
> 
> None of the boards that I have worked with have a DEV_VER_TI_V1 version
> of the controller. I also tried to use the Linux device-tree to check if
> I could identify the SoC/board but I was unable to do so.
Do you know which SoC is V2 and V3 ?

Also +CC Nishanth .

Re: [PATCH] usb: cdns3: Do not access memory after free

[Siddharth Vadapalli]

On Fri, Aug 22, 2025 at 04:55:49PM +0200, Marek Vasut wrote:
> On 8/22/25 4:14 PM, Siddharth Vadapalli wrote:
> > On Thu, Aug 21, 2025 at 10:22:43PM +0200, Marek Vasut wrote:
> > > On 8/21/25 8:16 AM, Siddharth Vadapalli wrote:
> > > 
> > > Hi,
> > > 
> > > > > > > > diff --git a/drivers/usb/cdns3/gadget.c b/drivers/usb/cdns3/gadget.c
> > > > > > > > index a30c40ef80e..9eaf7e40ab6 100644
> > > > > > > > --- a/drivers/usb/cdns3/gadget.c
> > > > > > > > +++ b/drivers/usb/cdns3/gadget.c
> > > > > > > > @@ -557,10 +557,10 @@ static void
> > > > > > > > cdns3_wa2_remove_old_request(struct cdns3_endpoint *priv_ep)
> > > > > > > >             trace_cdns3_wa2(priv_ep, "removes eldest request");
> > > > > > > > +        list_del_init(&priv_req->list);
> > > > > > > >             kfree(priv_req->request.buf);
> > > > > > > >             cdns3_gadget_ep_free_request(&priv_ep->endpoint,
> > > > > > > >                              &priv_req->request);
> > > > > > > > -        list_del_init(&priv_req->list);
> > > > > > > 
> > > > > > > Shouldn't the kfree() be moved here instead ?
> > > > > > > cdns3_gadget_ep_free_request() also accesses priv_req->request .
> > > > > > 
> > > > > > No, I do not think so. The kfree frees priv_req->request.buf not
> > > > > > priv_req->request so must happen before the call to
> > > > > > cdns3_gadget_ep_free_request.
> > > > > Thank you for clarifying. Please add this into the commit message, ideally
> > > > > wait for TI to test this and provide RB, then send V2 so this can go in.
> > > > 
> > > > I was planning to test this patch but the change being made is only
> > > > applicable to Controller Versions:
> > > > 	#define DEV_VER_NXP_V1		0x00024502
> > > > 	#define DEV_VER_TI_V1		0x00024509
> > > > and not to:
> > > > 	#define DEV_VER_V2		0x0002450C
> > > > 	#define DEV_VER_V3		0x0002450d
> > > > 
> > > > Since I don't have an SoC and a Board with DEV_VER_TI_V1, I cannot test
> > > > it. However, the change looks correct to me.
> > > > 
> > > > Reviewed-by: Siddharth Vadapalli <s-vadapalli@ti.com>
> > > The change does indeed look correct.
> > > 
> > > Do you know who might still have that board and could test ? (and which
> > > board/soc is that) ?
> > 
> > None of the boards that I have worked with have a DEV_VER_TI_V1 version
> > of the controller. I also tried to use the Linux device-tree to check if
> > I could identify the SoC/board but I was unable to do so.
> Do you know which SoC is V2 and V3 ?

I spent more time on this and found out that J721E SR 1.0 has the
controller with DEV_VER_TI_V1 version but other revisions of J721E as
well as all of the following SoCs have DEV_VER_V3 version of the
controller:
AM64, AM68, AM69, J7200, J721S2, J722S, J742S2 and J784S4.

I will try to find an SR 1.0 J721E SoC and test the patch on it and
share the results here.

Regards,
Siddharth.

Re: [PATCH] usb: cdns3: Do not access memory after free

[Marek Vasut]

On 8/23/25 4:07 AM, Siddharth Vadapalli wrote:

Hi,

>>>>> I was planning to test this patch but the change being made is only
>>>>> applicable to Controller Versions:
>>>>> 	#define DEV_VER_NXP_V1		0x00024502
>>>>> 	#define DEV_VER_TI_V1		0x00024509
>>>>> and not to:
>>>>> 	#define DEV_VER_V2		0x0002450C
>>>>> 	#define DEV_VER_V3		0x0002450d
>>>>>
>>>>> Since I don't have an SoC and a Board with DEV_VER_TI_V1, I cannot test
>>>>> it. However, the change looks correct to me.
>>>>>
>>>>> Reviewed-by: Siddharth Vadapalli <s-vadapalli@ti.com>
>>>> The change does indeed look correct.
>>>>
>>>> Do you know who might still have that board and could test ? (and which
>>>> board/soc is that) ?
>>>
>>> None of the boards that I have worked with have a DEV_VER_TI_V1 version
>>> of the controller. I also tried to use the Linux device-tree to check if
>>> I could identify the SoC/board but I was unable to do so.
>> Do you know which SoC is V2 and V3 ?
> 
> I spent more time on this and found out that J721E SR 1.0 has the
> controller with DEV_VER_TI_V1 version but other revisions of J721E as
> well as all of the following SoCs have DEV_VER_V3 version of the
> controller:
> AM64, AM68, AM69, J7200, J721S2, J722S, J742S2 and J784S4.
> 
> I will try to find an SR 1.0 J721E SoC and test the patch on it and
> share the results here.
This is awesome, thank you !

Re: [PATCH] usb: cdns3: Do not access memory after free

[Siddharth Vadapalli]

On Sat, Aug 23, 2025 at 02:21:18PM +0200, Marek Vasut wrote:
> On 8/23/25 4:07 AM, Siddharth Vadapalli wrote:
> 
> Hi,
> 
> > > > > > I was planning to test this patch but the change being made is only
> > > > > > applicable to Controller Versions:
> > > > > > 	#define DEV_VER_NXP_V1		0x00024502
> > > > > > 	#define DEV_VER_TI_V1		0x00024509
> > > > > > and not to:
> > > > > > 	#define DEV_VER_V2		0x0002450C
> > > > > > 	#define DEV_VER_V3		0x0002450d
> > > > > > 
> > > > > > Since I don't have an SoC and a Board with DEV_VER_TI_V1, I cannot test
> > > > > > it. However, the change looks correct to me.
> > > > > > 
> > > > > > Reviewed-by: Siddharth Vadapalli <s-vadapalli@ti.com>
> > > > > The change does indeed look correct.
> > > > > 
> > > > > Do you know who might still have that board and could test ? (and which
> > > > > board/soc is that) ?
> > > > 
> > > > None of the boards that I have worked with have a DEV_VER_TI_V1 version
> > > > of the controller. I also tried to use the Linux device-tree to check if
> > > > I could identify the SoC/board but I was unable to do so.
> > > Do you know which SoC is V2 and V3 ?
> > 
> > I spent more time on this and found out that J721E SR 1.0 has the
> > controller with DEV_VER_TI_V1 version but other revisions of J721E as
> > well as all of the following SoCs have DEV_VER_V3 version of the
> > controller:
> > AM64, AM68, AM69, J7200, J721S2, J722S, J742S2 and J784S4.
> > 
> > I will try to find an SR 1.0 J721E SoC and test the patch on it and
> > share the results here.
> This is awesome, thank you !

I was able to get an SR 1.0 J721E SoC and also the J721E
Common-Processor-Board for testing the patch.

Enabling debug info in the cdns3/gadget.c driver, I see:
	cdns-usb3-peripheral usb@6000000: Device Controller version: 00024509
	cdns-usb3-peripheral usb@6000000: USB Capabilities:: 09203324
	cdns-usb3-peripheral usb@6000000: On-Chip memory cnfiguration: 00000c34
which confirms the Controller Version.

However, the code changed by the current patch is only affecting the
execution of code associated with Workaround 2 which is described in
detail in the driver. I am summarizing it here for your reference:

Issue:
	Controller for OUT endpoints has shared on-chip buffers for all
	incoming packets. The buffer acts as a FIFO, due to which,
	missing a DMA descriptor for one packet will block subsequent
	transfers/packets meant for other Endpoints that were queued.

Workaround:
	If the Endpoint Status register indicates a Descriptor Miss,
	rearm the DMA transfer to complete the missed transfer.

In order to test the patch, I used USBACM for STDIO/STDOUT to check if I
could trigger the descriptor miss workaround. The command I ran was:
setenv stdio usbacm; setenv stdout usbacm;
/dev/ttyACM0 showed up on my PC and I was also able to access the U-Boot
prompt via ACM0. While it is functional, I didn't see the code
associated with the workaround being triggered. Reviewing the driver
again, I identified that the workaround is being disabled very early on
within "cdns3_wa2_gadget_ep_queue()" with the comment stating:

	 * If transfer was queued before DESCMISS appear than we
	 * can disable handling of DESCMISS interrupt. Driver assumes that it
	 * can disable special treatment for this endpoint.

Given the above, it doesn't seem easy to recreate the issue since I
would have to trigger a descriptor miss event prior to the very first
USB transfer for the Endpoint. Please let me know if you have any
suggestions for speeding up testing.

Regards,
Siddharth.

Re: [PATCH] usb: cdns3: Do not access memory after free

[Marek Vasut]

On 8/24/25 10:02 AM, Siddharth Vadapalli wrote:
> On Sat, Aug 23, 2025 at 02:21:18PM +0200, Marek Vasut wrote:
>> On 8/23/25 4:07 AM, Siddharth Vadapalli wrote:
>>
>> Hi,
>>
>>>>>>> I was planning to test this patch but the change being made is only
>>>>>>> applicable to Controller Versions:
>>>>>>> 	#define DEV_VER_NXP_V1		0x00024502
>>>>>>> 	#define DEV_VER_TI_V1		0x00024509
>>>>>>> and not to:
>>>>>>> 	#define DEV_VER_V2		0x0002450C
>>>>>>> 	#define DEV_VER_V3		0x0002450d
>>>>>>>
>>>>>>> Since I don't have an SoC and a Board with DEV_VER_TI_V1, I cannot test
>>>>>>> it. However, the change looks correct to me.
>>>>>>>
>>>>>>> Reviewed-by: Siddharth Vadapalli <s-vadapalli@ti.com>
>>>>>> The change does indeed look correct.
>>>>>>
>>>>>> Do you know who might still have that board and could test ? (and which
>>>>>> board/soc is that) ?
>>>>>
>>>>> None of the boards that I have worked with have a DEV_VER_TI_V1 version
>>>>> of the controller. I also tried to use the Linux device-tree to check if
>>>>> I could identify the SoC/board but I was unable to do so.
>>>> Do you know which SoC is V2 and V3 ?
>>>
>>> I spent more time on this and found out that J721E SR 1.0 has the
>>> controller with DEV_VER_TI_V1 version but other revisions of J721E as
>>> well as all of the following SoCs have DEV_VER_V3 version of the
>>> controller:
>>> AM64, AM68, AM69, J7200, J721S2, J722S, J742S2 and J784S4.
>>>
>>> I will try to find an SR 1.0 J721E SoC and test the patch on it and
>>> share the results here.
>> This is awesome, thank you !
> 
> I was able to get an SR 1.0 J721E SoC and also the J721E
> Common-Processor-Board for testing the patch.
> 
> Enabling debug info in the cdns3/gadget.c driver, I see:
> 	cdns-usb3-peripheral usb@6000000: Device Controller version: 00024509
> 	cdns-usb3-peripheral usb@6000000: USB Capabilities:: 09203324
> 	cdns-usb3-peripheral usb@6000000: On-Chip memory cnfiguration: 00000c34
> which confirms the Controller Version.
> 
> However, the code changed by the current patch is only affecting the
> execution of code associated with Workaround 2 which is described in
> detail in the driver. I am summarizing it here for your reference:
> 
> Issue:
> 	Controller for OUT endpoints has shared on-chip buffers for all
> 	incoming packets. The buffer acts as a FIFO, due to which,
> 	missing a DMA descriptor for one packet will block subsequent
> 	transfers/packets meant for other Endpoints that were queued.
> 
> Workaround:
> 	If the Endpoint Status register indicates a Descriptor Miss,
> 	rearm the DMA transfer to complete the missed transfer.
> 
> In order to test the patch, I used USBACM for STDIO/STDOUT to check if I
> could trigger the descriptor miss workaround. The command I ran was:
> setenv stdio usbacm; setenv stdout usbacm;
> /dev/ttyACM0 showed up on my PC and I was also able to access the U-Boot
> prompt via ACM0. While it is functional, I didn't see the code
> associated with the workaround being triggered. Reviewing the driver
> again, I identified that the workaround is being disabled very early on
> within "cdns3_wa2_gadget_ep_queue()" with the comment stating:
> 
> 	 * If transfer was queued before DESCMISS appear than we
> 	 * can disable handling of DESCMISS interrupt. Driver assumes that it
> 	 * can disable special treatment for this endpoint.
> 
> Given the above, it doesn't seem easy to recreate the issue since I
> would have to trigger a descriptor miss event prior to the very first
> USB transfer for the Endpoint. Please let me know if you have any
> suggestions for speeding up testing.
Can you maybe simply force-enable the workaround code and see if that 
itself works, without crashing ? If yes, then I would say let's apply this.

Re: [PATCH] usb: cdns3: Do not access memory after free

[Siddharth Vadapalli]

On Thu, Aug 28, 2025 at 05:46:56PM +0200, Marek Vasut wrote:
> On 8/24/25 10:02 AM, Siddharth Vadapalli wrote:
> > On Sat, Aug 23, 2025 at 02:21:18PM +0200, Marek Vasut wrote:
> > > On 8/23/25 4:07 AM, Siddharth Vadapalli wrote:
> > > 
> > > Hi,
> > > 
> > > > > > > > I was planning to test this patch but the change being made is only
> > > > > > > > applicable to Controller Versions:
> > > > > > > > 	#define DEV_VER_NXP_V1		0x00024502
> > > > > > > > 	#define DEV_VER_TI_V1		0x00024509
> > > > > > > > and not to:
> > > > > > > > 	#define DEV_VER_V2		0x0002450C
> > > > > > > > 	#define DEV_VER_V3		0x0002450d
> > > > > > > > 
> > > > > > > > Since I don't have an SoC and a Board with DEV_VER_TI_V1, I cannot test
> > > > > > > > it. However, the change looks correct to me.
> > > > > > > > 
> > > > > > > > Reviewed-by: Siddharth Vadapalli <s-vadapalli@ti.com>
> > > > > > > The change does indeed look correct.
> > > > > > > 
> > > > > > > Do you know who might still have that board and could test ? (and which
> > > > > > > board/soc is that) ?
> > > > > > 
> > > > > > None of the boards that I have worked with have a DEV_VER_TI_V1 version
> > > > > > of the controller. I also tried to use the Linux device-tree to check if
> > > > > > I could identify the SoC/board but I was unable to do so.
> > > > > Do you know which SoC is V2 and V3 ?
> > > > 
> > > > I spent more time on this and found out that J721E SR 1.0 has the
> > > > controller with DEV_VER_TI_V1 version but other revisions of J721E as
> > > > well as all of the following SoCs have DEV_VER_V3 version of the
> > > > controller:
> > > > AM64, AM68, AM69, J7200, J721S2, J722S, J742S2 and J784S4.
> > > > 
> > > > I will try to find an SR 1.0 J721E SoC and test the patch on it and
> > > > share the results here.
> > > This is awesome, thank you !
> > 
> > I was able to get an SR 1.0 J721E SoC and also the J721E
> > Common-Processor-Board for testing the patch.
> > 
> > Enabling debug info in the cdns3/gadget.c driver, I see:
> > 	cdns-usb3-peripheral usb@6000000: Device Controller version: 00024509
> > 	cdns-usb3-peripheral usb@6000000: USB Capabilities:: 09203324
> > 	cdns-usb3-peripheral usb@6000000: On-Chip memory cnfiguration: 00000c34
> > which confirms the Controller Version.
> > 
> > However, the code changed by the current patch is only affecting the
> > execution of code associated with Workaround 2 which is described in
> > detail in the driver. I am summarizing it here for your reference:
> > 
> > Issue:
> > 	Controller for OUT endpoints has shared on-chip buffers for all
> > 	incoming packets. The buffer acts as a FIFO, due to which,
> > 	missing a DMA descriptor for one packet will block subsequent
> > 	transfers/packets meant for other Endpoints that were queued.
> > 
> > Workaround:
> > 	If the Endpoint Status register indicates a Descriptor Miss,
> > 	rearm the DMA transfer to complete the missed transfer.
> > 
> > In order to test the patch, I used USBACM for STDIO/STDOUT to check if I
> > could trigger the descriptor miss workaround. The command I ran was:
> > setenv stdio usbacm; setenv stdout usbacm;
> > /dev/ttyACM0 showed up on my PC and I was also able to access the U-Boot
> > prompt via ACM0. While it is functional, I didn't see the code
> > associated with the workaround being triggered. Reviewing the driver
> > again, I identified that the workaround is being disabled very early on
> > within "cdns3_wa2_gadget_ep_queue()" with the comment stating:
> > 
> > 	 * If transfer was queued before DESCMISS appear than we
> > 	 * can disable handling of DESCMISS interrupt. Driver assumes that it
> > 	 * can disable special treatment for this endpoint.
> > 
> > Given the above, it doesn't seem easy to recreate the issue since I
> > would have to trigger a descriptor miss event prior to the very first
> > USB transfer for the Endpoint. Please let me know if you have any
> > suggestions for speeding up testing.
> Can you maybe simply force-enable the workaround code and see if that itself
> works, without crashing ? If yes, then I would say let's apply this.

I did try it out when I wasn't able to trigger the workaround and I
didn't see a crash. It seemed to work without issues but I didn't
mention it since I felt that this wasn't the right way to test it.
Nevertheless, I can confirm that I did not see a crash when forcing the
workaround code to execute.

Regards,
Siddharth.

Re: [PATCH] usb: cdns3: Do not access memory after free

[Marek Vasut]

On 8/29/25 6:22 AM, Siddharth Vadapalli wrote:
> On Thu, Aug 28, 2025 at 05:46:56PM +0200, Marek Vasut wrote:
>> On 8/24/25 10:02 AM, Siddharth Vadapalli wrote:
>>> On Sat, Aug 23, 2025 at 02:21:18PM +0200, Marek Vasut wrote:
>>>> On 8/23/25 4:07 AM, Siddharth Vadapalli wrote:
>>>>
>>>> Hi,
>>>>
>>>>>>>>> I was planning to test this patch but the change being made is only
>>>>>>>>> applicable to Controller Versions:
>>>>>>>>> 	#define DEV_VER_NXP_V1		0x00024502
>>>>>>>>> 	#define DEV_VER_TI_V1		0x00024509
>>>>>>>>> and not to:
>>>>>>>>> 	#define DEV_VER_V2		0x0002450C
>>>>>>>>> 	#define DEV_VER_V3		0x0002450d
>>>>>>>>>
>>>>>>>>> Since I don't have an SoC and a Board with DEV_VER_TI_V1, I cannot test
>>>>>>>>> it. However, the change looks correct to me.
>>>>>>>>>
>>>>>>>>> Reviewed-by: Siddharth Vadapalli <s-vadapalli@ti.com>
>>>>>>>> The change does indeed look correct.
>>>>>>>>
>>>>>>>> Do you know who might still have that board and could test ? (and which
>>>>>>>> board/soc is that) ?
>>>>>>>
>>>>>>> None of the boards that I have worked with have a DEV_VER_TI_V1 version
>>>>>>> of the controller. I also tried to use the Linux device-tree to check if
>>>>>>> I could identify the SoC/board but I was unable to do so.
>>>>>> Do you know which SoC is V2 and V3 ?
>>>>>
>>>>> I spent more time on this and found out that J721E SR 1.0 has the
>>>>> controller with DEV_VER_TI_V1 version but other revisions of J721E as
>>>>> well as all of the following SoCs have DEV_VER_V3 version of the
>>>>> controller:
>>>>> AM64, AM68, AM69, J7200, J721S2, J722S, J742S2 and J784S4.
>>>>>
>>>>> I will try to find an SR 1.0 J721E SoC and test the patch on it and
>>>>> share the results here.
>>>> This is awesome, thank you !
>>>
>>> I was able to get an SR 1.0 J721E SoC and also the J721E
>>> Common-Processor-Board for testing the patch.
>>>
>>> Enabling debug info in the cdns3/gadget.c driver, I see:
>>> 	cdns-usb3-peripheral usb@6000000: Device Controller version: 00024509
>>> 	cdns-usb3-peripheral usb@6000000: USB Capabilities:: 09203324
>>> 	cdns-usb3-peripheral usb@6000000: On-Chip memory cnfiguration: 00000c34
>>> which confirms the Controller Version.
>>>
>>> However, the code changed by the current patch is only affecting the
>>> execution of code associated with Workaround 2 which is described in
>>> detail in the driver. I am summarizing it here for your reference:
>>>
>>> Issue:
>>> 	Controller for OUT endpoints has shared on-chip buffers for all
>>> 	incoming packets. The buffer acts as a FIFO, due to which,
>>> 	missing a DMA descriptor for one packet will block subsequent
>>> 	transfers/packets meant for other Endpoints that were queued.
>>>
>>> Workaround:
>>> 	If the Endpoint Status register indicates a Descriptor Miss,
>>> 	rearm the DMA transfer to complete the missed transfer.
>>>
>>> In order to test the patch, I used USBACM for STDIO/STDOUT to check if I
>>> could trigger the descriptor miss workaround. The command I ran was:
>>> setenv stdio usbacm; setenv stdout usbacm;
>>> /dev/ttyACM0 showed up on my PC and I was also able to access the U-Boot
>>> prompt via ACM0. While it is functional, I didn't see the code
>>> associated with the workaround being triggered. Reviewing the driver
>>> again, I identified that the workaround is being disabled very early on
>>> within "cdns3_wa2_gadget_ep_queue()" with the comment stating:
>>>
>>> 	 * If transfer was queued before DESCMISS appear than we
>>> 	 * can disable handling of DESCMISS interrupt. Driver assumes that it
>>> 	 * can disable special treatment for this endpoint.
>>>
>>> Given the above, it doesn't seem easy to recreate the issue since I
>>> would have to trigger a descriptor miss event prior to the very first
>>> USB transfer for the Endpoint. Please let me know if you have any
>>> suggestions for speeding up testing.
>> Can you maybe simply force-enable the workaround code and see if that itself
>> works, without crashing ? If yes, then I would say let's apply this.
> 
> I did try it out when I wasn't able to trigger the workaround and I
> didn't see a crash. It seemed to work without issues but I didn't
> mention it since I felt that this wasn't the right way to test it.
> Nevertheless, I can confirm that I did not see a crash when forcing the
> workaround code to execute.
All right, thanks for checking, I'll pick this for 2025.10-rc4 .