lkml.org 
[lkml]   [2019]   [Aug]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectAW: Resend [PATCH] kernel/resource.c: invalidate parent when freed resource has childs
Date
> -----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

\
 
 \ /
  Last update: 2019-08-12 10:47    [W:1.069 / U:0.032 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site