Re: [PATCH v3] powerpc/pseries: use pci_host_bridge.release_fn() to kfree(phb)

[Mauricio Faria de Oliveira <mauricfo@linux.vnet.ibm.com>]

Hi Gavin,

tl;dr: thanks for the comments & suggestions; i'll submit v4.

On 08/11/2016 03:40 AM, Gavin Shan wrote: [added some line breaks]
> It seems the user has two options here:

> (1) Setup bridge's release_fn() and call
> pcibios_free_controller() explicitly;

I think the v3 design was non-intuitive in that point -- it does not
seem right for an user to use both options:

if release_fn() is set and is called before pcibios_free_controller()
(normal case w/ DLPAR/PCI hotplug/cxl, as buses/devices are supposed
to be removed before the controller is released) the latter will use
an invalid 'phb' pointer. (what Andrew reported)

In that scenario, it's not even possible for pcibios_free_controller()
to try to detect if release_fn() was already run or not, as the only
information it has is the 'phb' pointer, which may be invalid.

So, I believe the elegant way out of this is your suggestion to have
"immediate or deferred release" and make the user *choose* either one.

Obviously, let's make this explicit to the user -- w/ rename & comments.

 > (2) Call pcibios_free_controller() without
> a valid bridge's release_fn() initialized.

Ok, that looks legitimate for those using immediate release (default).

i.e., once an user decides to use deferred released, it's understood
that pcibios_free_controller() should not be called.

 > I think we can provide better interface
> to users: what we do in pcibios_free_controller() and pcibios_host_bridge_release()
> should be (almost) same. pcibios_host_bridge_release() can be a wrapper of
> pcibios_free_controller().

Right; I implemented only kfree() in pcibios_host_bridge_release()
because I was focused on when it runs *after* pcibios_free_controller();
but it turns out that if it runs *before*, phb becomes invalid pointer.

So, you're right -- both functions are expected to have the same effect
(slightly different code), that is all of what pcibios_free_controller()
does.  The only difference should be the timing. (good point on wrapper)

 > With this, the users have two options: (1) Rely on bridge's
> release_fn() to free the PCI controller; (2) Call pcibios_free_controller() as we're
> doing currently. Those two options corresponds to immediately or deferred releasing.

Looks very good.  I'll submit a v4 like this:
-rename pcibios_host_bridge_release()/pcibios_free_controller_deferred()
-add comments about using _either_ one or another
-pcibios_free_controller_deferred() calls pcibios_free_controller().

-- 
Mauricio Faria de Oliveira
IBM Linux Technology Center