Re: [PATCH v5 5/8] powerpc/pci/IOV: Add support for runtime enabling the VFs

[Sergey Miroshnichenko <s.miroshnichenko@yadro.com>]

On 4/30/19 9:00 AM, Oliver O'Halloran wrote:
> On Mon, 2019-03-11 at 14:52 +0300, Sergey Miroshnichenko wrote:
> 
>> When called within pcibios_sriov_enable(), the pci_sriov_get_totalvfs(pdev)
>> returns zero, because the device is yet preparing to enable the VFs.
> 
> I don't think this is correct. The earliest pcibios_sriov_enable() can
> be called is during a driver probe function. The totalvfs field is
> initialised by pci_iov_init() which is called before the device has
> been added to the bus. If it's returning zero then maybe the driver
> limited the number of VFs to zero?
> 
> That said, you need to reset numvfs to zero before changing the value. 
> So limiting the number of pci_dns that are created to the number
> actually required rather than totalvfs doesn't hurt.
> 
>> With this patch it becomes possible to enable VFs via sysfs "sriov_numvfs"
>> on PowerNV.
> 
> I tested on a few of our lab systems with random kernel versions
> spanning from 4.15 to 5.0 and sriov_numvfs seemed to work fine on all
> of them. Is there a specific configuration you're testing that needed
> this change?
> 

Thanks a lot for the review and testing!

I've just received back the hardware (Mellanox ConnectX-4 -
drivers/net/ethernet/mellanox/mlx5), and got surprised: the issue with the
pci_sriov_get_totalvfs(pdev) returning zero can't be reproduced anymore :/ I've rechecked
the code and don't know how could this even happen. I'm sorry about that; if it will
happen again, I have to investigate deeper.

The PCI subsystem doesn't let the number of VFs to be changed from non-zero value to
another non-zero value: it needs to sriov_disable() first. I guess we can rely on that and
don't reset the numvfs to zero explicitly.

I'll change the patch description and resend it in v6 with other fixes of this patchset.

Best regards,
Serge

>> Signed-off-by: Sergey Miroshnichenko <s.miroshnichenko@yadro.com>
>> ---
>>  arch/powerpc/include/asm/pci-bridge.h     |  4 +--
>>  arch/powerpc/kernel/pci_dn.c              | 32 ++++++++++++++---------
>>  arch/powerpc/platforms/powernv/pci-ioda.c |  4 +--
>>  arch/powerpc/platforms/pseries/pci.c      |  4 +--
>>  4 files changed, 25 insertions(+), 19 deletions(-)
>>
>> diff --git a/arch/powerpc/include/asm/pci-bridge.h b/arch/powerpc/include/asm/pci-bridge.h
>> index fc188e0e9179..6479bc96e0b6 100644
>> --- a/arch/powerpc/include/asm/pci-bridge.h
>> +++ b/arch/powerpc/include/asm/pci-bridge.h
>> @@ -225,8 +225,8 @@ struct pci_dn {
>>  extern struct pci_dn *pci_get_pdn_by_devfn(struct pci_bus *bus,
>>  					   int devfn);
>>  extern struct pci_dn *pci_get_pdn(struct pci_dev *pdev);
>> -extern struct pci_dn *add_dev_pci_data(struct pci_dev *pdev);
>> -extern void remove_dev_pci_data(struct pci_dev *pdev);
>> +extern struct pci_dn *pci_create_vf_pdns(struct pci_dev *pdev, int num_vfs);
>> +extern void pci_destroy_vf_pdns(struct pci_dev *pdev);
>>  extern struct pci_dn *pci_add_device_node_info(struct pci_controller *hose,
>>  					       struct device_node *dn);
>>  extern void pci_remove_device_node_info(struct device_node *dn);
>> diff --git a/arch/powerpc/kernel/pci_dn.c b/arch/powerpc/kernel/pci_dn.c
>> index 7f12882d8882..7fa362f8038d 100644
>> --- a/arch/powerpc/kernel/pci_dn.c
>> +++ b/arch/powerpc/kernel/pci_dn.c
>> @@ -222,18 +222,19 @@ static struct pci_dn *pci_create_pdn_from_dev(struct pci_dev *pdev,
>>  	return pdn;
>>  }
>>  
>> -struct pci_dn *add_dev_pci_data(struct pci_dev *pdev)
>> +struct pci_dn *pci_create_vf_pdns(struct pci_dev *pdev, int num_vfs)
>>  {
>> +	struct pci_dn *pdn = pci_get_pdn(pdev);
>> +
>>  #ifdef CONFIG_PCI_IOV
>> -	struct pci_dn *parent, *pdn;
>> +	struct pci_dn *parent;
>>  	int i;
>>  
>>  	/* Only support IOV for now */
>>  	if (!pdev->is_physfn)
>> -		return pci_get_pdn(pdev);
>> +		return pdn;
>>  
>>  	/* Check if VFs have been populated */
>> -	pdn = pci_get_pdn(pdev);
>>  	if (!pdn || (pdn->flags & PCI_DN_FLAG_IOV_VF))
>>  		return NULL;
>>  
>> @@ -242,33 +243,38 @@ struct pci_dn *add_dev_pci_data(struct pci_dev *pdev)
>>  	if (!parent)
>>  		return NULL;
>>  
>> -	for (i = 0; i < pci_sriov_get_totalvfs(pdev); i++) {
>> +	for (i = 0; i < num_vfs; i++) {
>>  		struct eeh_dev *edev __maybe_unused;
>> +		struct pci_dn *vpdn;
>>  
>> -		pdn = pci_alloc_pdn(parent,
>> -				    pci_iov_virtfn_bus(pdev, i),
>> -				    pci_iov_virtfn_devfn(pdev, i));
>> -		if (!pdn) {
>> +		vpdn = pci_alloc_pdn(parent,
>> +				     pci_iov_virtfn_bus(pdev, i),
>> +				     pci_iov_virtfn_devfn(pdev, i));
>> +		if (!vpdn) {
>>  			dev_warn(&pdev->dev, "%s: Cannot create firmware data for VF#%d\n",
>>  				 __func__, i);
>>  			return NULL;
>>  		}
>>  
>> -		pdn->vf_index = i;
>> +		vpdn->vf_index = i;
>> +		vpdn->vendor_id = pdn->vendor_id;
>> +		vpdn->device_id = pdn->device_id;
>> +		vpdn->class_code = pdn->class_code;
>> +		vpdn->pci_ext_config_space = 0;
>>  
>>  #ifdef CONFIG_EEH
>>  		/* Create the EEH device for the VF */
>> -		edev = eeh_dev_init(pdn);
>> +		edev = eeh_dev_init(vpdn);
>>  		BUG_ON(!edev);
>>  		edev->physfn = pdev;
>>  #endif /* CONFIG_EEH */
>>  	}
>>  #endif /* CONFIG_PCI_IOV */
>>  
>> -	return pci_get_pdn(pdev);
>> +	return pdn;
>>  }
>>  
>> -void remove_dev_pci_data(struct pci_dev *pdev)
>> +void pci_destroy_vf_pdns(struct pci_dev *pdev)
>>  {
>>  #ifdef CONFIG_PCI_IOV
>>  	struct pci_dn *parent;
>> diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c
>> index ed500f51d449..979c901535f2 100644
>> --- a/arch/powerpc/platforms/powernv/pci-ioda.c
>> +++ b/arch/powerpc/platforms/powernv/pci-ioda.c
>> @@ -1720,14 +1720,14 @@ int pnv_pcibios_sriov_disable(struct pci_dev *pdev)
>>  	pnv_pci_sriov_disable(pdev);
>>  
>>  	/* Release PCI data */
>> -	remove_dev_pci_data(pdev);
>> +	pci_destroy_vf_pdns(pdev);
>>  	return 0;
>>  }
>>  
>>  int pnv_pcibios_sriov_enable(struct pci_dev *pdev, u16 num_vfs)
>>  {
>>  	/* Allocate PCI data */
>> -	add_dev_pci_data(pdev);
>> +	pci_create_vf_pdns(pdev, num_vfs);
>>  
>>  	return pnv_pci_sriov_enable(pdev, num_vfs);
>>  }
>> diff --git a/arch/powerpc/platforms/pseries/pci.c b/arch/powerpc/platforms/pseries/pci.c
>> index 37a77e57893e..5e87596903a6 100644
>> --- a/arch/powerpc/platforms/pseries/pci.c
>> +++ b/arch/powerpc/platforms/pseries/pci.c
>> @@ -205,7 +205,7 @@ int pseries_pci_sriov_enable(struct pci_dev *pdev, u16 num_vfs)
>>  int pseries_pcibios_sriov_enable(struct pci_dev *pdev, u16 num_vfs)
>>  {
>>  	/* Allocate PCI data */
>> -	add_dev_pci_data(pdev);
>> +	pci_create_vf_pdns(pdev, num_vfs);
>>  	return pseries_pci_sriov_enable(pdev, num_vfs);
>>  }
>>  
>> @@ -217,7 +217,7 @@ int pseries_pcibios_sriov_disable(struct pci_dev *pdev)
>>  	/* Releasing pe_num_map */
>>  	kfree(pdn->pe_num_map);
>>  	/* Release PCI data */
>> -	remove_dev_pci_data(pdev);
>> +	pci_destroy_vf_pdns(pdev);
>>  	pci_vf_drivers_autoprobe(pdev, true);
>>  	return 0;
>>  }
>