Change the PCI bus driver to free resources leaked by drivers

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

Change the PCI bus driver to free resources leaked by drivers

John Baldwin
Currently our driver model trusts drivers to DTRT and properly release any
resources they allocated during probe() and attach().  I've added a new
resource_list() helper method to release active resources on a resource
list and used this to write a pci_child_detached() which cleans up any
active resources when a device fails to probe or a driver finishes
detach.  It also fixes an issue where we did not power down devices when
the driver was detached (e.g. via kldunload).  I've tested the resource
bits by writing a dummy driver that intentionally attached to an unattached
device and leaked a memory BAR and verified that the bus warned about the
leak and cleaned it up.

http://www.FreeBSD.org/~jhb/patches/pci_clean_detach.patch

--
John Baldwin
_______________________________________________
[hidden email] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-new-bus
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: Change the PCI bus driver to free resources leaked by drivers

Brooks Davis-2
On Mon, Jun 24, 2013 at 04:59:14PM -0400, John Baldwin wrote:

> Currently our driver model trusts drivers to DTRT and properly release any
> resources they allocated during probe() and attach().  I've added a new
> resource_list() helper method to release active resources on a resource
> list and used this to write a pci_child_detached() which cleans up any
> active resources when a device fails to probe or a driver finishes
> detach.  It also fixes an issue where we did not power down devices when
> the driver was detached (e.g. via kldunload).  I've tested the resource
> bits by writing a dummy driver that intentionally attached to an unattached
> device and leaked a memory BAR and verified that the bus warned about the
> leak and cleaned it up.
Looks good in a quick review.

-- Brooks

attachment0 (195 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Change the PCI bus driver to free resources leaked by drivers

Warner Losh
In reply to this post by John Baldwin

On Jun 24, 2013, at 2:59 PM, John Baldwin wrote:

> Currently our driver model trusts drivers to DTRT and properly release any
> resources they allocated during probe() and attach().  I've added a new
> resource_list() helper method to release active resources on a resource
> list and used this to write a pci_child_detached() which cleans up any
> active resources when a device fails to probe or a driver finishes
> detach.  It also fixes an issue where we did not power down devices when
> the driver was detached (e.g. via kldunload).  I've tested the resource
> bits by writing a dummy driver that intentionally attached to an unattached
> device and leaked a memory BAR and verified that the bus warned about the
> leak and cleaned it up.
>
> http://www.FreeBSD.org/~jhb/patches/pci_clean_detach.patch

I think most of pci_child_detached() could be a generic thing (except for the weird interaction with the msi-wart).  This is likely fixable.

We don't tear down any interrupt handlers that the device established. This is fixable, but the PCI bus would need to start tracking interrupts that are established...

We don't tear down any timers the device may have setup. This likely isn't fixable.

We likely don't want to tear down interrupts in bus_deactivate_resource, since I think it is theoretically legal to deactivate an interrupt resource without tearing down the interrupt. But maybe it shouldn't be...

Warner


_______________________________________________
[hidden email] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-new-bus
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: Change the PCI bus driver to free resources leaked by drivers

John Baldwin
On Tuesday, June 25, 2013 12:43:35 am Warner Losh wrote:

>
> On Jun 24, 2013, at 2:59 PM, John Baldwin wrote:
>
> > Currently our driver model trusts drivers to DTRT and properly release any
> > resources they allocated during probe() and attach().  I've added a new
> > resource_list() helper method to release active resources on a resource
> > list and used this to write a pci_child_detached() which cleans up any
> > active resources when a device fails to probe or a driver finishes
> > detach.  It also fixes an issue where we did not power down devices when
> > the driver was detached (e.g. via kldunload).  I've tested the resource
> > bits by writing a dummy driver that intentionally attached to an unattached
> > device and leaked a memory BAR and verified that the bus warned about the
> > leak and cleaned it up.
> >
> > http://www.FreeBSD.org/~jhb/patches/pci_clean_detach.patch
>
> I think most of pci_child_detached() could be a generic thing (except for the
> weird interaction with the msi-wart).  This is likely fixable.

The existing design we've gone with for this sort of thing is to provide
resource_list helpers, but let each bus driver decide which types of
resources it manages (see bus_print_child).  Also, I think the order
matters (interrupts before memory & I/O).  One thing the patch doesn't do
currently is explicitly list the resource ranges being freed.

> We don't tear down any interrupt handlers that the device established. This
> is fixable, but the PCI bus would need to start tracking interrupts that are
> established...

Eh, that is the part I don't like.  That would be a lot of non-PCI specific
crap in the PCI bus driver.

> We don't tear down any timers the device may have setup. This likely isn't
> fixable.

Nor taskqueues, dma tags, static DMA allocations, etc.  There are all sorts
of things that new-bus is not aware of.

> We likely don't want to tear down interrupts in bus_deactivate_resource,
> since I think it is theoretically legal to deactivate an interrupt resource
> without tearing down the interrupt. But maybe it shouldn't be...

I have long thought that bus_setup_intr/bus_teardown_intr was a gross hack
in terms of the API.  It's a necessary hack (interrupts have more metadata
when they are "active"), but still gross.

If we want to solve the interrupt problem I do think it belongs in the nexus
layer (at least on x86.. whoever provides IRQ resources should be the
maintain the state).  You could easily have something that associated a
device_t with each interrupt handler and then add a new method along the
lines of:

bus_revoke_intr(device_t child, struct resource *r)

Which did a teardown of all handlers on resource 'r' associated with device
'child'.

However, I would probably prefer to do that sort of thing as a next step.

--
John Baldwin
_______________________________________________
[hidden email] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-new-bus
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: Change the PCI bus driver to free resources leaked by drivers

Warner Losh

On Jun 25, 2013, at 6:37 AM, John Baldwin wrote:

> On Tuesday, June 25, 2013 12:43:35 am Warner Losh wrote:
>>
>> On Jun 24, 2013, at 2:59 PM, John Baldwin wrote:
>>
>>> Currently our driver model trusts drivers to DTRT and properly release any
>>> resources they allocated during probe() and attach().  I've added a new
>>> resource_list() helper method to release active resources on a resource
>>> list and used this to write a pci_child_detached() which cleans up any
>>> active resources when a device fails to probe or a driver finishes
>>> detach.  It also fixes an issue where we did not power down devices when
>>> the driver was detached (e.g. via kldunload).  I've tested the resource
>>> bits by writing a dummy driver that intentionally attached to an unattached
>>> device and leaked a memory BAR and verified that the bus warned about the
>>> leak and cleaned it up.
>>>
>>> http://www.FreeBSD.org/~jhb/patches/pci_clean_detach.patch
>>
>> I think most of pci_child_detached() could be a generic thing (except for the
>> weird interaction with the msi-wart).  This is likely fixable.
>
> The existing design we've gone with for this sort of thing is to provide
> resource_list helpers, but let each bus driver decide which types of
> resources it manages (see bus_print_child).  Also, I think the order
> matters (interrupts before memory & I/O).  One thing the patch doesn't do
> currently is explicitly list the resource ranges being freed.

Yea, if ordering didn't matter, freeing them all would be a snap...

>> We don't tear down any interrupt handlers that the device established. This
>> is fixable, but the PCI bus would need to start tracking interrupts that are
>> established...
>
> Eh, that is the part I don't like.  That would be a lot of non-PCI specific
> crap in the PCI bus driver.

I'm not sure I understand this objection. We do (or did at one time) this sort of thing for the cardbus code and it found a few bugs in a couple of drivers...

>> We don't tear down any timers the device may have setup. This likely isn't
>> fixable.
>
> Nor taskqueues, dma tags, static DMA allocations, etc.  There are all sorts
> of things that new-bus is not aware of.

True.

>> We likely don't want to tear down interrupts in bus_deactivate_resource,
>> since I think it is theoretically legal to deactivate an interrupt resource
>> without tearing down the interrupt. But maybe it shouldn't be...
>
> I have long thought that bus_setup_intr/bus_teardown_intr was a gross hack
> in terms of the API.  It's a necessary hack (interrupts have more metadata
> when they are "active"), but still gross.

Yes. that's true. However, until we have something better...

> If we want to solve the interrupt problem I do think it belongs in the nexus
> layer (at least on x86.. whoever provides IRQ resources should be the
> maintain the state).  You could easily have something that associated a
> device_t with each interrupt handler and then add a new method along the
> lines of:
>
> bus_revoke_intr(device_t child, struct resource *r)
>
> Which did a teardown of all handlers on resource 'r' associated with device
> 'child'.
>
> However, I would probably prefer to do that sort of thing as a next step.

I'm OK doing this as a separate step, I guess...

Warner
_______________________________________________
[hidden email] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-new-bus
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: Change the PCI bus driver to free resources leaked by drivers

John Baldwin
On Tuesday, June 25, 2013 10:15:00 am Warner Losh wrote:

>
> On Jun 25, 2013, at 6:37 AM, John Baldwin wrote:
>
> > On Tuesday, June 25, 2013 12:43:35 am Warner Losh wrote:
> >>
> >> On Jun 24, 2013, at 2:59 PM, John Baldwin wrote:
> >>
> >>> Currently our driver model trusts drivers to DTRT and properly release any
> >>> resources they allocated during probe() and attach().  I've added a new
> >>> resource_list() helper method to release active resources on a resource
> >>> list and used this to write a pci_child_detached() which cleans up any
> >>> active resources when a device fails to probe or a driver finishes
> >>> detach.  It also fixes an issue where we did not power down devices when
> >>> the driver was detached (e.g. via kldunload).  I've tested the resource
> >>> bits by writing a dummy driver that intentionally attached to an unattached
> >>> device and leaked a memory BAR and verified that the bus warned about the
> >>> leak and cleaned it up.
> >>>
> >>> http://www.FreeBSD.org/~jhb/patches/pci_clean_detach.patch
> >>
> >> I think most of pci_child_detached() could be a generic thing (except for the
> >> weird interaction with the msi-wart).  This is likely fixable.
> >
> > The existing design we've gone with for this sort of thing is to provide
> > resource_list helpers, but let each bus driver decide which types of
> > resources it manages (see bus_print_child).  Also, I think the order
> > matters (interrupts before memory & I/O).  One thing the patch doesn't do
> > currently is explicitly list the resource ranges being freed.
>
> Yea, if ordering didn't matter, freeing them all would be a snap...
>
> >> We don't tear down any interrupt handlers that the device established. This
> >> is fixable, but the PCI bus would need to start tracking interrupts that are
> >> established...
> >
> > Eh, that is the part I don't like.  That would be a lot of non-PCI specific
> > crap in the PCI bus driver.
>
> I'm not sure I understand this objection. We do (or did at one time) this sort
> of thing for the cardbus code and it found a few bugs in a couple of drivers...

I would rather solve this problem more generically so that if we want to add
a child_detached method for other buses like ACPI then they can just use a
library call (e.g. a bus_revoke_intr()) instead of having to do all the same
tracking themselves.  The "right" place for this seems to be in whatever is
providing the IRQ resources and handles the actual bus_setup_intr calls to
create cookies, etc.  On x86 this is the nexus.  On other platforms it may
be in a nexus-like driver.  I can work at prototyping something for review as
a next step.

--
John Baldwin
_______________________________________________
[hidden email] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-new-bus
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: Change the PCI bus driver to free resources leaked by drivers

Warner Losh

On Jun 25, 2013, at 9:51 AM, John Baldwin wrote:

> On Tuesday, June 25, 2013 10:15:00 am Warner Losh wrote:
>>
>> On Jun 25, 2013, at 6:37 AM, John Baldwin wrote:
>>
>>> On Tuesday, June 25, 2013 12:43:35 am Warner Losh wrote:
>>>>
>>>> On Jun 24, 2013, at 2:59 PM, John Baldwin wrote:
>>>>
>>>>> Currently our driver model trusts drivers to DTRT and properly release any
>>>>> resources they allocated during probe() and attach().  I've added a new
>>>>> resource_list() helper method to release active resources on a resource
>>>>> list and used this to write a pci_child_detached() which cleans up any
>>>>> active resources when a device fails to probe or a driver finishes
>>>>> detach.  It also fixes an issue where we did not power down devices when
>>>>> the driver was detached (e.g. via kldunload).  I've tested the resource
>>>>> bits by writing a dummy driver that intentionally attached to an unattached
>>>>> device and leaked a memory BAR and verified that the bus warned about the
>>>>> leak and cleaned it up.
>>>>>
>>>>> http://www.FreeBSD.org/~jhb/patches/pci_clean_detach.patch
>>>>
>>>> I think most of pci_child_detached() could be a generic thing (except for the
>>>> weird interaction with the msi-wart).  This is likely fixable.
>>>
>>> The existing design we've gone with for this sort of thing is to provide
>>> resource_list helpers, but let each bus driver decide which types of
>>> resources it manages (see bus_print_child).  Also, I think the order
>>> matters (interrupts before memory & I/O).  One thing the patch doesn't do
>>> currently is explicitly list the resource ranges being freed.
>>
>> Yea, if ordering didn't matter, freeing them all would be a snap...
>>
>>>> We don't tear down any interrupt handlers that the device established. This
>>>> is fixable, but the PCI bus would need to start tracking interrupts that are
>>>> established...
>>>
>>> Eh, that is the part I don't like.  That would be a lot of non-PCI specific
>>> crap in the PCI bus driver.
>>
>> I'm not sure I understand this objection. We do (or did at one time) this sort
>> of thing for the cardbus code and it found a few bugs in a couple of drivers...
>
> I would rather solve this problem more generically so that if we want to add
> a child_detached method for other buses like ACPI then they can just use a
> library call (e.g. a bus_revoke_intr()) instead of having to do all the same
> tracking themselves.  The "right" place for this seems to be in whatever is
> providing the IRQ resources and handles the actual bus_setup_intr calls to
> create cookies, etc.  On x86 this is the nexus.  On other platforms it may
> be in a nexus-like driver.  I can work at prototyping something for review as
> a next step.

I'd rather have that done at the interrupt controller level, which sadly we don't model and put most, but not quite all, of the functionality in the nexus driver. My experience is a bit colored from the PC Card and CardBus stuff, since there we intercepted the setup_intr calls and did the interrupt pass through directly for the child to cope with the sudden removal cases where the bridge driver know the card was gone, so the interrupt was dispatched to it, and only further dispatched to the child if the bridge thought it was still there. I'd assumed we'd need to that for proper support of hot-plug PCIe (including surprise removal support), but since we don't have that, I guess the PCI code just passes everything to the nexus.

I think that's a long way of saying that this is a good path, and we may have code on x86 that could benefit from it now that's doing ad-hoc things... I'd love to see this next step, and the current patch you have is sufficient for the  more constrained problem it is trying to solve.

Warner
_______________________________________________
[hidden email] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-new-bus
To unsubscribe, send any mail to "[hidden email]"