From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga05.intel.com ([192.55.52.43]:48276 "EHLO mga05.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932884AbcKXJ5A (ORCPT ); Thu, 24 Nov 2016 04:57:00 -0500 Subject: Re: [RFT PATCH 1/1] xhci: free xhci virtual devices with leaf nodes first To: Felipe Balbi , linux@roeck-us.net References: <582DC88C.5040308@linux.intel.com> <1479903867-561-1-git-send-email-mathias.nyman@linux.intel.com> <8760ndqmhm.fsf@linux.intel.com> Cc: linux-usb@vger.kernel.org, stable@vger.kernel.org From: Mathias Nyman Message-ID: <5836B995.5010609@linux.intel.com> Date: Thu, 24 Nov 2016 11:57:41 +0200 MIME-Version: 1.0 In-Reply-To: <8760ndqmhm.fsf@linux.intel.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Sender: stable-owner@vger.kernel.org List-ID: On 24.11.2016 11:02, Felipe Balbi wrote: > > Hi, > > Mathias Nyman writes: >> the tt_info provided by a HS hub might be in use to by a child device >> Make sure we free the devices in the correct order. >> >> This is needed in special cases such as when xhci controller is >> reset when resuming from hibernate, and all virt_devices are freed. >> >> Also free the virt_devices starting from max slot_id as children >> more commonly have higher slot_id than parent. >> >> CC: >> Signed-off-by: Mathias Nyman >> >> --- >> >> Guenter Roeck, does this work for you? >> >> A rework of how tt_info is stored and used might be needed, >> but that will take some time and won't go to stable as easily. >> --- >> drivers/usb/host/xhci-mem.c | 38 ++++++++++++++++++++++++++++++++++++-- >> 1 file changed, 36 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c >> index 6afe323..b3a5cd8 100644 >> --- a/drivers/usb/host/xhci-mem.c >> +++ b/drivers/usb/host/xhci-mem.c >> @@ -979,6 +979,40 @@ void xhci_free_virt_device(struct xhci_hcd *xhci, int slot_id) >> xhci->devs[slot_id] = NULL; >> } >> >> +/* >> + * Free a virt_device structure. >> + * If the virt_device added a tt_info (a hub) and has children pointing to >> + * that tt_info, then free the child first. Recursive. >> + * We can't rely on udev at this point to find child-parent relationships. >> + */ >> +void xhci_free_virt_devices_depth_first(struct xhci_hcd *xhci, int slot_id) >> +{ >> + struct xhci_virt_device *vdev; >> + struct list_head *tt_list_head; >> + struct xhci_tt_bw_info *tt_info, *next; >> + int i; >> + >> + vdev = xhci->devs[slot_id]; >> + if (!vdev) >> + return; >> + >> + tt_list_head = &(xhci->rh_bw[vdev->real_port - 1].tts); >> + list_for_each_entry_safe(tt_info, next, tt_list_head, tt_list) { >> + /* is this a hub device that added a tt_info to the tts list */ >> + if (tt_info->slot_id == slot_id) { > > if (tt_info->slot_id != slot_id) > continue; > >> + /* are any devices using this tt_info? */ >> + for (i = 1; i < HCS_MAX_SLOTS(xhci->hcs_params1); i++) { > > off-by-one here ? Why is i starting from 1? > >> + vdev = xhci->devs[i]; slit_id 0 is reserved and xhci->devs[0] is not used, so ne need to check it. All other places that check xhci->devs[0] are avtually buggy -Mathias