Messages in this thread | | | From | "Schmid, Carsten" <> | Subject | AW: Resend [PATCH] kernel/resource.c: invalidate parent when freed resource has childs | Date | Mon, 12 Aug 2019 08:39:10 +0000 |
| |
> -----Ursprüngliche Nachricht----- > Von: Wei Yang [mailto:richard.weiyang@gmail.com] > Gesendet: Samstag, 10. August 2019 02:45 > An: Linus Torvalds <torvalds@linux-foundation.org> > Cc: Wei Yang <richard.weiyang@gmail.com>; Schmid, Carsten > <Carsten_Schmid@mentor.com>; bp@suse.de; dan.j.williams@intel.com; > mingo@kernel.org; dave.hansen@linux.intel.com; linux- > kernel@vger.kernel.org; bhelgaas@google.com; osalvador@suse.de; > rdunlap@infradead.org; richardw.yang@linux.intel.com; > gregkh@linuxfoundation.org > Betreff: Re: Resend [PATCH] kernel/resource.c: invalidate parent when > freed resource has childs > > On Fri, Aug 09, 2019 at 03:45:59PM -0700, Linus Torvalds wrote: > >On Fri, Aug 9, 2019 at 3:38 PM Wei Yang <richard.weiyang@gmail.com> > wrote: > >> > >> In theory, child may have siblings. Would it be possible to have several > >> devices under xhci-hcd? > > > >I'm less interested in the xhci-hcd case - which I certainly *hope* is > >fixed already? - than in "if this happens somewhere else". > > > > Agree, this is what I want to say. > Unfortunately this xhci-hcd case is not fixed yet. I'm working on a driver fix proposal, discussing with Hans de Goede who wrote the intel_xhci_usb_sw platform driver.
For me there is nothing invalid in the intel_xhci_usb_sw driver. It is initialized from xhci-hcd/xhci-pci in xhci_pci_probe --> xhci_ext_cap_init --> xhci_create_intel_xhci_sw_pdev which then does devm_add_action_or_reset(dev, xhci_intel_unregister_pdev, pdev)
So far, so good. Doesn't look bad.
When unbinding the xhci-hcd driver, i can see that xhci_pci_remove executes, and after this the xhci_intel_unregister_pdev is called. This is what fails because xhci_pci_remove removes the parent of the resource created in the xhci_create_intel_xhci_sw_pdev. So the "devm framework" which is used here fails in this specific case. Very unintentional. The proposal will call xhci_intel_unregister_pdev from within the xhci-pci in a similar way like the driver is created. So we will have the child killed before the parent :)
> >So if we do want to remove the parent (which may be a good idea with a > >warning), and want to make sure that the children are really removed > >from the resource hierarchy, we should do somethiing like > > > > static bool detach_children(struct resource *res) > > { > > res = res->child; > > if (!res) > > return false; > > do { > > res->parent = NULL; > > res = res->sibling; > > } while (res); > > return true; > > } > > > >and then we could write the __release_region() warning as > > > > /* You should not release a resource that has children */ > > WARN_ON_ONCE(detach_children(res)); > > Fine for me, this extended sanity check. This didn't came up to my mind. Because i have a reproducer, i can test it and send it as V2. If you have any additional ideas, let me know.
> > I am thinking about why this could happen. See above explanation.
> > To guard the core kernel code, it looks reasonable. > Exactly my motivation :)
> >or something? > > > >NOTE! The above is entirely untested, and written purely in my mail > >reader. It might be seriously buggy, including not compiling, or doing > >odd things. See it more as a "maybe something like this" code snippet > >example than any kind of final form. > > > > Linus > I'll implement and check it, of course. Development as usual. Thanks!
Carsten > -- > Wei Yang > Help you, Help me
| |