[Very RFC 40/46] powernv/npu: Don't drop refcount when looking up GPU pci_devs

[Frederic Barrat]

Le 27/11/2019 à 10:33, Greg Kurz a écrit :
> On Wed, 27 Nov 2019 10:10:13 +0100
> Frederic Barrat <fbarrat at 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 <aik at 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 ?


No, these code paths are nvlink-only.

   Fred



>>     Fred
>>
>>
>>>>
>>>>
>>>>>
>>>>> Signed-off-by: Oliver O'Halloran <oohall at 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;
>>>>>    }
>>>>>    
>>>>>
>>>>
>>>
>>
>