Re: 答复: [PATCH v3] usb: gadget: u_serial: Disable ep before setting port to null to fix the crash caused by port being null

[Jon Hunter <jonathanh@nvidia.com>]

Hi Lianqin,

On 17/01/2025 05:04, 胡连勤 wrote:
> Hi  Jon：
> 
>>>>> Considering that in some extreme cases, when performing the
>>>>> unbinding operation, gserial_disconnect has cleared gser->ioport,
>>>>> which triggers gadget reconfiguration, and then calls
>>>>> gs_read_complete, resulting in access to a null pointer. Therefore,
>>>>> ep is disabled before gserial_disconnect sets port to null to prevent this
>> from happening.
>>>>>
>>>>> Call trace:
>>>>>     gs_read_complete+0x58/0x240
>>>>>     usb_gadget_giveback_request+0x40/0x160
>>>>>     dwc3_remove_requests+0x170/0x484
>>>>>     dwc3_ep0_out_start+0xb0/0x1d4
>>>>>     __dwc3_gadget_start+0x25c/0x720
>>>>>     kretprobe_trampoline.cfi_jt+0x0/0x8
>>>>>     kretprobe_trampoline.cfi_jt+0x0/0x8
>>>>>     udc_bind_to_driver+0x1d8/0x300
>>>>>     usb_gadget_probe_driver+0xa8/0x1dc
>>>>>     gadget_dev_desc_UDC_store+0x13c/0x188
>>>>>     configfs_write_iter+0x160/0x1f4
>>>>>     vfs_write+0x2d0/0x40c
>>>>>     ksys_write+0x7c/0xf0
>>>>>     __arm64_sys_write+0x20/0x30
>>>>>     invoke_syscall+0x60/0x150
>>>>>     el0_svc_common+0x8c/0xf8
>>>>>     do_el0_svc+0x28/0xa0
>>>>>     el0_svc+0x24/0x84
>>>>>
>>>>> Fixes: c1dca562be8a ("usb gadget: split out serial core")
>>>>> Cc: stable@vger.kernel.org
>>>>> Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>>>>> Signed-off-by: Lianqin Hu <hulianqin@vivo.com>
>>>>> ---
>>>>>
>>>>> Changes in v3:
>>>>>     - Add --- line above the version tag information
>>>>>     - Remove extra blank lines in commit messages
>>>>>     - Version tag information from v2 to changes in v2
>>>>>     - Link to v2:
>>>>>
>> https://lo/
>>>>>
>> re.kernel.org%2Fall%2FTYUPR06MB6217DAA095A9863D4B58D57CD23B2%40T
>> YUPR
>>>>>
>> 06MB6217.apcprd06.prod.outlook.com%2F&data=05%7C02%7Chulianqin%40
>> viv
>>>>>
>> o.com%7Cc88bd260faf9470d244f08dd363eb8df%7C923e42dc48d54cbeb5821
>> a797
>>>>>
>> a6412ed%7C0%7C0%7C638726365197191059%7CUnknown%7CTWFpbGZsb3d
>> 8eyJFbXB
>>>>>
>> 0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFp
>> bCI
>>>>>
>> sIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=G4l0lGF093J44BuTeGpEYpYvC
>> MGiK3d
>>>>> TnR%2Fd2Zn8Q1U%3D&reserved=0
>>>>>
>>>>> Changes in v2:
>>>>>     - Remove some address information from patch descriptions
>>>>>     - Link to v1:
>> https://lore.k/
>> ernel.org%2Fall%2FTYUPR06MB621763AB815989161F4033AFD2762%40TYUPR
>> 06MB6217.apcprd06.prod.outlook.com%2F&data=05%7C02%7Chulianqin%40
>> vivo.com%7Cc88bd260faf9470d244f08dd363eb8df%7C923e42dc48d54cbeb58
>> 21a797a6412ed%7C0%7C0%7C638726365197206594%7CUnknown%7CTWFpb
>> GZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4z
>> MiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=6rGddQJ
>> L9K%2Bqyr98IXYkf6zM8AzxN6%2FaJZztGybx%2FUw%3D&reserved=0
>>>>>     - Link to suggestions:
>>>>>
>> https://lo/
>>>>>
>> re.kernel.org%2Fall%2FTYUPR06MB6217DE28012FFEC5E808DD64D2962%40TY
>> UPR
>>>>>
>> 06MB6217.apcprd06.prod.outlook.com%2F&data=05%7C02%7Chulianqin%40
>> viv
>>>>>
>> o.com%7Cc88bd260faf9470d244f08dd363eb8df%7C923e42dc48d54cbeb5821
>> a797
>>>>>
>> a6412ed%7C0%7C0%7C638726365197216620%7CUnknown%7CTWFpbGZsb3d
>> 8eyJFbXB
>>>>>
>> 0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFp
>> bCI
>>>>>
>> sIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=7EvOpENpZd8s6S8PR1D%2B2
>> HnQmBLPa
>>>>> UNotpV5UGSDiDE%3D&reserved=0
>>>>>
>>>>>     drivers/usb/gadget/function/u_serial.c | 8 ++++----
>>>>>     1 file changed, 4 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/drivers/usb/gadget/function/u_serial.c
>>>>> b/drivers/usb/gadget/function/u_serial.c
>>>>> index 53d9fc41acc5..bc143a86c2dd 100644
>>>>> --- a/drivers/usb/gadget/function/u_serial.c
>>>>> +++ b/drivers/usb/gadget/function/u_serial.c
>>>>> @@ -1420,6 +1420,10 @@ void gserial_disconnect(struct gserial *gser)
>>>>>      /* REVISIT as above: how best to track this? */
>>>>>      port->port_line_coding = gser->port_line_coding;
>>>>> +   /* disable endpoints, aborting down any active I/O */
>>>>> +   usb_ep_disable(gser->out);
>>>>> +   usb_ep_disable(gser->in);
>>>>> +
>>>>>      port->port_usb = NULL;
>>>>>      gser->ioport = NULL;
>>>>>      if (port->port.count > 0) {
>>>>> @@ -1431,10 +1435,6 @@ void gserial_disconnect(struct gserial *gser)
>>>>>      spin_unlock(&port->port_lock);
>>>>>      spin_unlock_irqrestore(&serial_port_lock, flags);
>>>>> -   /* disable endpoints, aborting down any active I/O */
>>>>> -   usb_ep_disable(gser->out);
>>>>> -   usb_ep_disable(gser->in);
>>>>> -
>>>>>      /* finally, free any unused/unusable I/O buffers */
>>>>>      spin_lock_irqsave(&port->port_lock, flags);
>>>>>      if (port->port.count == 0)
>>>>
>>>>
>>>> We have observed a reboot regression on Tegra234 (I have not tried
>>>> other
>>>> boards) and bisect is pointing to this commit. Reverting this on top
>>>> of mainline is fixing the problem.
>>>>
>>>> With this change, when the board reboots we see ...
>>>>
>>>> [   59.918177] tegra-xudc 3550000.usb: ep 3 disabled
>>>> [   59.923097] tegra-xudc 3550000.usb: ep 2 disabled
>>>> [   59.927955] tegra-xudc 3550000.usb: ep 5 disabled
>>>> [   80.911432] rcu: INFO: rcu_preempt self-detected stall on CPU
>>>> [   80.917354] rcu:     6-....: (5248 ticks this GP)
>> idle=ec24/1/0x4000000000000000 softirq=1213/1213 fqs=2623
>>>> [   80.927146] rcu:     (t=5253 jiffies g=3781 q=1490 ncpus=12)
>>>> [   80.932704] Sending NMI from CPU 6 to CPUs 2:
>>>> [   90.981555] CPU: 6 UID: 0 PID: 18 Comm: rcu_exp_gp_kthr Not tainted
>> 6.13.0-rc7-00043-g619f0b6fad52 #1
>>>> [   90.981558] Hardware name: NVIDIA NVIDIA Jetson AGX Orin Developer
>> Kit/Jetson, BIOS 00.0.0-dev-main_92e5ae_88fd1_296de 12/16/2024
>>>> [   90.981559] pstate: 80400009 (Nzcv daif +PAN -UAO -TCO -DIT -SSBS
>> BTYPE=--)
>>>> [   90.981562] pc : smp_call_function_single+0xdc/0x1a0
>>>> [   90.981574] lr : __sync_rcu_exp_select_node_cpus+0x228/0x3c0
>>>> [   90.981578] sp : ffff800082eb3cd0
>>>> [   90.981579] x29: ffff800082eb3cd0 x28: 0000000000000010 x27:
>> ffff0000802933c0
>>>> [   90.981582] x26: ffff0007a8a1d700 x25: ffff800082895500 x24:
>> ffff800080132018
>>>> [   90.981584] x23: 0000000000000014 x22: ffff800081fb7700 x21:
>> ffff80008280d970
>>>> [   90.981586] x20: 0000000000000feb x19: ffff800082eb3d00 x18:
>> 0000000000000000
>>>> [   90.981588] x17: 0000000000000000 x16: 0000000000000000 x15:
>> 0000000000000000
>>>> [   90.981590] x14: ffff000080293440 x13: 0000000000000001 x12:
>> 0000000000000000
>>>> [   90.981591] x11: ffff800081fb2388 x10: ffff0000802933c0 x9 :
>> 0000000000000001
>>>> [   90.981593] x8 : 0000000000000040 x7 : 0000000000017068 x6 :
>> ffff800080132018
>>>> [   90.981595] x5 : 0000000000000000 x4 : ffff0007a8a4f9c8 x3 :
>> 0000000000000001
>>>> [   90.981597] x2 : 0000000000000000 x1 : ffff0007a8a4f9c0 x0 :
>> 0000000000000004
>>>> [   90.981599] Call trace:
>>>> [   90.981601]  smp_call_function_single+0xdc/0x1a0 (P)
>>>> [   90.981605]  __sync_rcu_exp_select_node_cpus+0x228/0x3c0
>>>> [   90.981607]  sync_rcu_exp_select_cpus+0x13c/0x2a0
>>>> [   90.981609]  wait_rcu_exp_gp+0x18/0x30
>>>> [   90.981611]  kthread_worker_fn+0xd0/0x188
>>>> [   90.981614]  kthread+0x118/0x11c
>>>> [   90.981619]  ret_from_fork+0x10/0x20
>>>> [  101.416347] sched: DL replenish lagged too much
>>>>
>>>
>>> Odd, you have a usb-serial gadget device in this system that is
>>> disconnecting somehow?  That oops doesn't point to anything in the usb
>>> gadget codebase, "all" we have done is move the call to shutdown the
>>> endpoints to earlier in the disconnect function.
>>
>> Yes the board starts usb-serial and usb-ethernet gadget and on reboot when
>> tearing it down I am seeing the above. As soon as it disables the tegra-xudc
>> endpoints (as seen above) the board appears to stall.
>>
>>> I'm glad to revert this, but it feels really odd that this is causing
>>> you an rcu stall issue.
>>
>> Thanks. I can't say I understand it either, but I am certain it is caused by this
>> change.
>>
>> Happy to run any tests to narrow this down a bit.
> 
> I'm really sorry. If we are sure that the problem is caused by this patch, we will roll it back first. In addition,
> what are the differences between the serial use of Tegra234 board and mobile phones?
> When we consider other solutions, we need to take this usage scenario of Tegra234 board into consideration.

Nothing specific that I am aware of. We just expose a serial and 
ethernet interface over USB for interfacing with the platform. I will 
see if I can narrow down the scenario a bit more.

Thanks
Jon
-- 
nvpublic