On Tue, 13 Mar 2018 06:22:30 +0000, Minas Harutyunyan <Minas.Harutyunyan@xxxxxxxxxxxx> wrote: > Hi Paul, > > On 3/13/2018 6:26 AM, Paul Zimmerman wrote: > > Hi Minas, > > > > While looking at the QH queuing in the DWC2 driver, I think I've found > > some places where the QH struct may not be freed. Normally, the sequence is: > > > > dwc2_hcd_qh_unlink(); > > < other stuff > > > dwc2_hcd_qh_free(); > > > > or else: > > > > dwc2_hcd_qh_unlink(); > > < other stuff > > > < link the QH to some other list > > > > > For non-periodic EPs, dwc2_hcd_qh_unlink() does > > list_del_init(&qh->qh_list_entry), or for periodic EPs it calls > > dwc2_deschedule_periodic() which in turn does the list_del_init(). > > This means the QH is removed from whatever list it was on. > > > > So after the call to dwc2_hcd_qh_unlink(), the QH either needs to be freed > > by calling dwc2_hcd_qh_free(), or it needs to be re-linked to another list, > > otherwise the QH would be "lost" and could never be freed. > > > > The places where I think a problem can happen are in dwc2_hcd_qh_deactivate(), > > dwc_hcd_urb_dequeue(), and dwc_hcd_complete_xfer_ddma(). In most if not all > > of these places, interrupts are disabled, which means that dwc2_hcd_qh_free() > > cannot be called, since it can sleep. So maybe the freeing was omitted because > > it was hard to do in these places? > > > > What do you think, am I reading the code correctly and this could be a real > > problem, or am I crazy? :) > > > > Thank you for your input. I'm planing to review all host code in 1 or 2 > months. Yes, in host code there are lot of stuff should be > updated/corrected. Currently we want to complete all fixes/improvements > on gadget side. We still have lot of prepared patches in our queue which > need to be included in mainline Kernel. BTW, besides fixes/improvements > there are few new features like Service Interval support on device side > which currently under development and debugging. Looks like this was a false alarm. What I missed is that a pointer to the QH is saved in usb_host_endpoint->hcpriv, and when dwc2_hcd_endpoint_disable() is called, the QH is fetched from there and then freed. I also added some debugging code to count the number of QHs allocated, and did not see the number rising, except when a new device was connected, and in that case the count fell back to the previous number once the device was disconnected. So nevermind, and sorry for the noise. -- Paul -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
