On Wed, 27 Nov 2019 10:10:13 +0100 Frederic Barrat <fbar...@linux.ibm.com> wrote:
> > > Le 27/11/2019 à 09:24, Greg Kurz a écrit : > > On Wed, 27 Nov 2019 18:09:40 +1100 > > Alexey Kardashevskiy <a...@ozlabs.ru> wrote: > > > >> > >> > >> On 20/11/2019 12:28, Oliver O'Halloran wrote: > >>> The comment here implies that we don't need to take a ref to the pci_dev > >>> because the ioda_pe will always have one. This implies that the current > >>> expection is that the pci_dev for an NPU device will *never* be torn > >>> down since the ioda_pe having a ref to the device will prevent the > >>> release function from being called. > >>> > >>> In other words, the desired behaviour here appears to be leaking a ref. > >>> > >>> Nice! > >> > >> > >> There is a history: https://patchwork.ozlabs.org/patch/1088078/ > >> > >> We did not fix anything in particular then, we do not seem to be fixing > >> anything now (in other words - we cannot test it in a normal natural > >> way). I'd drop this one. > >> > > > > Yeah, I didn't fix anything at the time. Just reverted to the ref > > count behavior we had before: > > > > https://patchwork.ozlabs.org/patch/829172/ > > > > Frederic recently posted his take on the same topic from the OpenCAPI > > point of view: > > > > http://patchwork.ozlabs.org/patch/1198947/ > > > > He seems to indicate the NPU devices as the real culprit because > > nobody ever cared for them to be removable. Fixing that seems be > > a chore nobody really wants to address obviously... :-\ > > > I had taken a stab at not leaking a ref for the nvlink devices and do > the proper thing regarding ref counting (i.e. fixing all the callers of > get_pci_dev() to drop the reference when they were done). With that, I > could see that the ref count of the nvlink devices could drop to 0 > (calling remove for the device in /sys) and that the devices could go away. > > But then, I realized it's not necessarily desirable at this point. There > are several comments in the code saying the npu devices (for nvlink) > don't go away, there's no device release callback defined when it seems > there should be, at least to handle releasing PEs.... All in all, it > seems that some work would be needed. And if it hasn't been required by > now... > If everyone is ok with leaking a reference in the NPU case, I guess this isn't a problem. But if we move forward with Oliver's patch, a pci_dev_put() would be needed for OpenCAPI, correct ? > Fred > > > >> > >> > >>> > >>> Signed-off-by: Oliver O'Halloran <ooh...@gmail.com> > >>> --- > >>> arch/powerpc/platforms/powernv/npu-dma.c | 11 +++-------- > >>> 1 file changed, 3 insertions(+), 8 deletions(-) > >>> > >>> diff --git a/arch/powerpc/platforms/powernv/npu-dma.c > >>> b/arch/powerpc/platforms/powernv/npu-dma.c > >>> index 72d3749da02c..2eb6e6d45a98 100644 > >>> --- a/arch/powerpc/platforms/powernv/npu-dma.c > >>> +++ b/arch/powerpc/platforms/powernv/npu-dma.c > >>> @@ -28,15 +28,10 @@ static struct pci_dev *get_pci_dev(struct device_node > >>> *dn) > >>> break; > >>> > >>> /* > >>> - * pci_get_domain_bus_and_slot() increased the reference count of > >>> - * the PCI device, but callers don't need that actually as the PE > >>> - * already holds a reference to the device. Since callers aren't > >>> - * aware of the reference count change, call pci_dev_put() now to > >>> - * avoid leaks. > >>> + * NB: for_each_pci_dev() elevates the pci_dev refcount. > >>> + * Caller is responsible for dropping the ref when it's > >>> + * finished with it. > >>> */ > >>> - if (pdev) > >>> - pci_dev_put(pdev); > >>> - > >>> return pdev; > >>> } > >>> > >>> > >> > > >