[Very RFC 22/46] powernv/eeh: Allocate eeh_dev's when needed

[Alexey Kardashevskiy]

On 25/11/2019 15:26, Oliver O'Halloran wrote:
> On Mon, Nov 25, 2019 at 2:27 PM Alexey Kardashevskiy <aik at ozlabs.ru> wrote:
>>
>>
>>
>> On 20/11/2019 12:28, Oliver O'Halloran wrote:
>>> Have the PowerNV EEH backend allocate the eeh_dev if needed rather than using
>>> the one attached to the pci_dn.
>>
>> So that pci_dn attached one is leaked then?
> 
> Sorta, the eeh_dev attached to the pci_dn is supposed to have the same
> lifetime as the pci_dn it's attached to. Whatever frees the pci_dn
> should also be freeing the eeh_dev, but I'm pretty sure the only
> situation where that actually happens is when removing the pci_dn for
> VFs.


Oh, that's lovely. add_sriov_vf_pdns() calls eeh_dev_init() to allocate
@edev but remove_sriov_vf_pdns() does kfree(edev) by itself.


> It's bad.

No sh*t :)

> 
>>> This gets us most of the way towards decoupling
>>> pci_dn from the PowerNV EEH code.
>>>
>>> Signed-off-by: Oliver O'Halloran <oohall at gmail.com>
>>> ---
>>> We should probably be free()ing the eeh_dev somewhere. The pci_dev release
>>> function is the right place for it.
>>> ---
>>>  arch/powerpc/platforms/powernv/eeh-powernv.c | 22 ++++++++++++++++----
>>>  1 file changed, 18 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/arch/powerpc/platforms/powernv/eeh-powernv.c b/arch/powerpc/platforms/powernv/eeh-powernv.c
>>> index 1cd80b399995..7aba18e08996 100644
>>> --- a/arch/powerpc/platforms/powernv/eeh-powernv.c
>>> +++ b/arch/powerpc/platforms/powernv/eeh-powernv.c
>>> @@ -366,10 +366,9 @@ static int pnv_eeh_write_config(struct eeh_dev *edev,
>>>   */
>>>  static struct eeh_dev *pnv_eeh_probe_pdev(struct pci_dev *pdev)
>>>  {
>>> -     struct pci_dn *pdn = pci_get_pdn(pdev);
>>> -     struct pci_controller *hose = pdn->phb;
>>> -     struct pnv_phb *phb = hose->private_data;
>>> -     struct eeh_dev *edev = pdn_to_eeh_dev(pdn);
>>> +     struct pnv_phb *phb = pci_bus_to_pnvhb(pdev->bus);
>>> +     struct pci_controller *hose = phb->hose;
>>> +     struct eeh_dev *edev;
>>>       uint32_t pcie_flags;
>>>       int ret;
>>>       int config_addr = (pdev->bus->number << 8) | (pdev->devfn);
>>> @@ -415,12 +414,27 @@ static struct eeh_dev *pnv_eeh_probe_pdev(struct pci_dev *pdev)
>>>       if ((pdev->class >> 8) == PCI_CLASS_BRIDGE_ISA)
>>>               return NULL;
>>>
>>> +     /* otherwise allocate and initialise a new eeh_dev */
>>> +     edev = kzalloc(sizeof(*edev), GFP_KERNEL);
>>> +     if (!edev) {
>>> +             pr_err("%s: out of memory lol\n", __func__);
>>
>> "lol"?
> 
> yeah lol

"unprofessional" is the word for this ;)


> 
> I am pretty sure we do not have to print anything if alloc failed
>> as alloc prints an error anyway. Thanks,
> 
> It does? Neat.

Well, it is this:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst#n878

===
These generic allocation functions all emit a stack dump on failure when
used
without __GFP_NOWARN so there is no use in emitting an additional failure
message when NULL is returned.
===

More than a printk. A small detail though.


-- 
Alexey