[PATCH iwl-next] ice: store VF's pci_dev ptr in ice_vf

[PATCH iwl-next] ice: store VF's pci_dev ptr in ice_vf

[Przemek Kitszel]

Extend struct ice_vf by vfdev.
Calculation of vfdev falls more nicely into ice_create_vf_entries().

Caching of vfdev enables simplification of ice_restore_all_vfs_msi_state().

Reviewed-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>
Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
---
add/remove: 0/0 grow/shrink: 2/1 up/down: 157/-130 (27)
Function                                     old     new   delta
ice_sriov_configure                         1712    1866    +154
ice_pci_err_resume                           168     171      +3
ice_restore_all_vfs_msi_state                200      70    -130
---
 drivers/net/ethernet/intel/ice/ice_main.c   |  2 +-
 drivers/net/ethernet/intel/ice/ice_sriov.c  | 40 +++++++++------------
 drivers/net/ethernet/intel/ice/ice_sriov.h  |  4 +--
 drivers/net/ethernet/intel/ice/ice_vf_lib.h |  2 +-
 4 files changed, 21 insertions(+), 27 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
index a6dd336d2500..d04498c2fd6d 100644
--- a/drivers/net/ethernet/intel/ice/ice_main.c
+++ b/drivers/net/ethernet/intel/ice/ice_main.c
@@ -5561,7 +5561,7 @@ static void ice_pci_err_resume(struct pci_dev *pdev)
 		return;
 	}
 
-	ice_restore_all_vfs_msi_state(pdev);
+	ice_restore_all_vfs_msi_state(pf);
 
 	ice_do_reset(pf, ICE_RESET_PFR);
 	ice_service_task_restart(pf);
diff --git a/drivers/net/ethernet/intel/ice/ice_sriov.c b/drivers/net/ethernet/intel/ice/ice_sriov.c
index 31314e7540f8..48bc8ea55265 100644
--- a/drivers/net/ethernet/intel/ice/ice_sriov.c
+++ b/drivers/net/ethernet/intel/ice/ice_sriov.c
@@ -789,14 +789,19 @@ static const struct ice_vf_ops ice_sriov_vf_ops = {
  */
 static int ice_create_vf_entries(struct ice_pf *pf, u16 num_vfs)
 {
+	struct pci_dev *pdev = pf->pdev;
 	struct ice_vfs *vfs = &pf->vfs;
+	struct pci_dev *vfdev = NULL;
 	struct ice_vf *vf;
-	u16 vf_id;
-	int err;
+	u16 vf_pdev_id;
+	int err, pos;
 
 	lockdep_assert_held(&vfs->table_lock);
 
-	for (vf_id = 0; vf_id < num_vfs; vf_id++) {
+	pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_SRIOV);
+	pci_read_config_word(pdev, pos + PCI_SRIOV_VF_DID, &vf_pdev_id);
+
+	for (u16 vf_id = 0; vf_id < num_vfs; vf_id++) {
 		vf = kzalloc(sizeof(*vf), GFP_KERNEL);
 		if (!vf) {
 			err = -ENOMEM;
@@ -812,6 +817,10 @@ static int ice_create_vf_entries(struct ice_pf *pf, u16 num_vfs)
 
 		ice_initialize_vf_entry(vf);
 
+		do {
+			vfdev = pci_get_device(pdev->vendor, vf_pdev_id, vfdev);
+		} while (vfdev && vfdev->physfn != pdev);
+		vf->vfdev = vfdev;
 		vf->vf_sw_id = pf->first_sw;
 
 		hash_add_rcu(vfs->table, &vf->entry, vf_id);
@@ -1714,26 +1723,11 @@ void ice_print_vfs_mdd_events(struct ice_pf *pf)
  * Called when recovering from a PF FLR to restore interrupt capability to
  * the VFs.
  */
-void ice_restore_all_vfs_msi_state(struct pci_dev *pdev)
+void ice_restore_all_vfs_msi_state(struct ice_pf *pf)
 {
-	u16 vf_id;
-	int pos;
-
-	if (!pci_num_vf(pdev))
-		return;
+	struct ice_vf *vf;
+	u32 bkt;
 
-	pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_SRIOV);
-	if (pos) {
-		struct pci_dev *vfdev;
-
-		pci_read_config_word(pdev, pos + PCI_SRIOV_VF_DID,
-				     &vf_id);
-		vfdev = pci_get_device(pdev->vendor, vf_id, NULL);
-		while (vfdev) {
-			if (vfdev->is_virtfn && vfdev->physfn == pdev)
-				pci_restore_msi_state(vfdev);
-			vfdev = pci_get_device(pdev->vendor, vf_id,
-					       vfdev);
-		}
-	}
+	ice_for_each_vf(pf, bkt, vf)
+		pci_restore_msi_state(vf->vfdev);
 }
diff --git a/drivers/net/ethernet/intel/ice/ice_sriov.h b/drivers/net/ethernet/intel/ice/ice_sriov.h
index 346cb2666f3a..06829443d540 100644
--- a/drivers/net/ethernet/intel/ice/ice_sriov.h
+++ b/drivers/net/ethernet/intel/ice/ice_sriov.h
@@ -33,7 +33,7 @@ int
 ice_get_vf_cfg(struct net_device *netdev, int vf_id, struct ifla_vf_info *ivi);
 
 void ice_free_vfs(struct ice_pf *pf);
-void ice_restore_all_vfs_msi_state(struct pci_dev *pdev);
+void ice_restore_all_vfs_msi_state(struct ice_pf *pf);
 
 int
 ice_set_vf_port_vlan(struct net_device *netdev, int vf_id, u16 vlan_id, u8 qos,
@@ -67,7 +67,7 @@ static inline
 void ice_vf_lan_overflow_event(struct ice_pf *pf, struct ice_rq_event_info *event) { }
 static inline void ice_print_vfs_mdd_events(struct ice_pf *pf) { }
 static inline void ice_print_vf_rx_mdd_event(struct ice_vf *vf) { }
-static inline void ice_restore_all_vfs_msi_state(struct pci_dev *pdev) { }
+static inline void ice_restore_all_vfs_msi_state(struct ice_pf *pf) { }
 
 static inline int
 ice_sriov_configure(struct pci_dev __always_unused *pdev,
diff --git a/drivers/net/ethernet/intel/ice/ice_vf_lib.h b/drivers/net/ethernet/intel/ice/ice_vf_lib.h
index 48fea6fa0362..57c36e4ccf91 100644
--- a/drivers/net/ethernet/intel/ice/ice_vf_lib.h
+++ b/drivers/net/ethernet/intel/ice/ice_vf_lib.h
@@ -82,7 +82,7 @@ struct ice_vf {
 	struct rcu_head rcu;
 	struct kref refcnt;
 	struct ice_pf *pf;
-
+	struct pci_dev *vfdev;
 	/* Used during virtchnl message handling and NDO ops against the VF
 	 * that will trigger a VFR
 	 */

base-commit: 0ad204c4acb8ba1ed99564b001609e62547bc79d
-- 
2.40.1

Re: [PATCH iwl-next] ice: store VF's pci_dev ptr in ice_vf

[Leon Romanovsky]

On Wed, Aug 16, 2023 at 04:54:54AM -0400, Przemek Kitszel wrote:
> Extend struct ice_vf by vfdev.
> Calculation of vfdev falls more nicely into ice_create_vf_entries().
> 
> Caching of vfdev enables simplification of ice_restore_all_vfs_msi_state().

I see that old code had access to pci_dev * of VF without any locking
from concurrent PCI core access. How is it protected? How do you make
sure that vfdev is valid?

Generally speaking, it is rarely good idea to cache VF pci_dev pointers
inside driver.

Thanks

> 
> Reviewed-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
> Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>
> Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> ---
> add/remove: 0/0 grow/shrink: 2/1 up/down: 157/-130 (27)
> Function                                     old     new   delta
> ice_sriov_configure                         1712    1866    +154
> ice_pci_err_resume                           168     171      +3
> ice_restore_all_vfs_msi_state                200      70    -130
> ---
>  drivers/net/ethernet/intel/ice/ice_main.c   |  2 +-
>  drivers/net/ethernet/intel/ice/ice_sriov.c  | 40 +++++++++------------
>  drivers/net/ethernet/intel/ice/ice_sriov.h  |  4 +--
>  drivers/net/ethernet/intel/ice/ice_vf_lib.h |  2 +-
>  4 files changed, 21 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
> index a6dd336d2500..d04498c2fd6d 100644
> --- a/drivers/net/ethernet/intel/ice/ice_main.c
> +++ b/drivers/net/ethernet/intel/ice/ice_main.c
> @@ -5561,7 +5561,7 @@ static void ice_pci_err_resume(struct pci_dev *pdev)
>  		return;
>  	}
>  
> -	ice_restore_all_vfs_msi_state(pdev);
> +	ice_restore_all_vfs_msi_state(pf);
>  
>  	ice_do_reset(pf, ICE_RESET_PFR);
>  	ice_service_task_restart(pf);
> diff --git a/drivers/net/ethernet/intel/ice/ice_sriov.c b/drivers/net/ethernet/intel/ice/ice_sriov.c
> index 31314e7540f8..48bc8ea55265 100644
> --- a/drivers/net/ethernet/intel/ice/ice_sriov.c
> +++ b/drivers/net/ethernet/intel/ice/ice_sriov.c
> @@ -789,14 +789,19 @@ static const struct ice_vf_ops ice_sriov_vf_ops = {
>   */
>  static int ice_create_vf_entries(struct ice_pf *pf, u16 num_vfs)
>  {
> +	struct pci_dev *pdev = pf->pdev;
>  	struct ice_vfs *vfs = &pf->vfs;
> +	struct pci_dev *vfdev = NULL;
>  	struct ice_vf *vf;
> -	u16 vf_id;
> -	int err;
> +	u16 vf_pdev_id;
> +	int err, pos;
>  
>  	lockdep_assert_held(&vfs->table_lock);
>  
> -	for (vf_id = 0; vf_id < num_vfs; vf_id++) {
> +	pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_SRIOV);
> +	pci_read_config_word(pdev, pos + PCI_SRIOV_VF_DID, &vf_pdev_id);
> +
> +	for (u16 vf_id = 0; vf_id < num_vfs; vf_id++) {
>  		vf = kzalloc(sizeof(*vf), GFP_KERNEL);
>  		if (!vf) {
>  			err = -ENOMEM;
> @@ -812,6 +817,10 @@ static int ice_create_vf_entries(struct ice_pf *pf, u16 num_vfs)
>  
>  		ice_initialize_vf_entry(vf);
>  
> +		do {
> +			vfdev = pci_get_device(pdev->vendor, vf_pdev_id, vfdev);
> +		} while (vfdev && vfdev->physfn != pdev);
> +		vf->vfdev = vfdev;
>  		vf->vf_sw_id = pf->first_sw;
>  
>  		hash_add_rcu(vfs->table, &vf->entry, vf_id);
> @@ -1714,26 +1723,11 @@ void ice_print_vfs_mdd_events(struct ice_pf *pf)
>   * Called when recovering from a PF FLR to restore interrupt capability to
>   * the VFs.
>   */
> -void ice_restore_all_vfs_msi_state(struct pci_dev *pdev)
> +void ice_restore_all_vfs_msi_state(struct ice_pf *pf)
>  {
> -	u16 vf_id;
> -	int pos;
> -
> -	if (!pci_num_vf(pdev))
> -		return;
> +	struct ice_vf *vf;
> +	u32 bkt;
>  
> -	pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_SRIOV);
> -	if (pos) {
> -		struct pci_dev *vfdev;
> -
> -		pci_read_config_word(pdev, pos + PCI_SRIOV_VF_DID,
> -				     &vf_id);
> -		vfdev = pci_get_device(pdev->vendor, vf_id, NULL);
> -		while (vfdev) {
> -			if (vfdev->is_virtfn && vfdev->physfn == pdev)
> -				pci_restore_msi_state(vfdev);
> -			vfdev = pci_get_device(pdev->vendor, vf_id,
> -					       vfdev);
> -		}
> -	}
> +	ice_for_each_vf(pf, bkt, vf)
> +		pci_restore_msi_state(vf->vfdev);
>  }
> diff --git a/drivers/net/ethernet/intel/ice/ice_sriov.h b/drivers/net/ethernet/intel/ice/ice_sriov.h
> index 346cb2666f3a..06829443d540 100644
> --- a/drivers/net/ethernet/intel/ice/ice_sriov.h
> +++ b/drivers/net/ethernet/intel/ice/ice_sriov.h
> @@ -33,7 +33,7 @@ int
>  ice_get_vf_cfg(struct net_device *netdev, int vf_id, struct ifla_vf_info *ivi);
>  
>  void ice_free_vfs(struct ice_pf *pf);
> -void ice_restore_all_vfs_msi_state(struct pci_dev *pdev);
> +void ice_restore_all_vfs_msi_state(struct ice_pf *pf);
>  
>  int
>  ice_set_vf_port_vlan(struct net_device *netdev, int vf_id, u16 vlan_id, u8 qos,
> @@ -67,7 +67,7 @@ static inline
>  void ice_vf_lan_overflow_event(struct ice_pf *pf, struct ice_rq_event_info *event) { }
>  static inline void ice_print_vfs_mdd_events(struct ice_pf *pf) { }
>  static inline void ice_print_vf_rx_mdd_event(struct ice_vf *vf) { }
> -static inline void ice_restore_all_vfs_msi_state(struct pci_dev *pdev) { }
> +static inline void ice_restore_all_vfs_msi_state(struct ice_pf *pf) { }
>  
>  static inline int
>  ice_sriov_configure(struct pci_dev __always_unused *pdev,
> diff --git a/drivers/net/ethernet/intel/ice/ice_vf_lib.h b/drivers/net/ethernet/intel/ice/ice_vf_lib.h
> index 48fea6fa0362..57c36e4ccf91 100644
> --- a/drivers/net/ethernet/intel/ice/ice_vf_lib.h
> +++ b/drivers/net/ethernet/intel/ice/ice_vf_lib.h
> @@ -82,7 +82,7 @@ struct ice_vf {
>  	struct rcu_head rcu;
>  	struct kref refcnt;
>  	struct ice_pf *pf;
> -
> +	struct pci_dev *vfdev;
>  	/* Used during virtchnl message handling and NDO ops against the VF
>  	 * that will trigger a VFR
>  	 */
> 
> base-commit: 0ad204c4acb8ba1ed99564b001609e62547bc79d
> -- 
> 2.40.1
> 
> 

Re: [PATCH iwl-next] ice: store VF's pci_dev ptr in ice_vf

[kernel test robot]

Hi Przemek,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 0ad204c4acb8ba1ed99564b001609e62547bc79d]

url:    https://github.com/intel-lab-lkp/linux/commits/Przemek-Kitszel/ice-store-VF-s-pci_dev-ptr-in-ice_vf/20230816-165953
base:   0ad204c4acb8ba1ed99564b001609e62547bc79d
patch link:    https://lore.kernel.org/r/20230816085454.235440-1-przemyslaw.kitszel%40intel.com
patch subject: [PATCH iwl-next] ice: store VF's pci_dev ptr in ice_vf
config: arm-allmodconfig (https://download.01.org/0day-ci/archive/20230817/202308171518.cvGLIHQ3-lkp@intel.com/config)
compiler: arm-linux-gnueabi-gcc (GCC) 12.3.0
reproduce: (https://download.01.org/0day-ci/archive/20230817/202308171518.cvGLIHQ3-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202308171518.cvGLIHQ3-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/net/ethernet/intel/ice/ice_sriov.c:1727: warning: Function parameter or member 'pf' not described in 'ice_restore_all_vfs_msi_state'
>> drivers/net/ethernet/intel/ice/ice_sriov.c:1727: warning: Excess function parameter 'pdev' description in 'ice_restore_all_vfs_msi_state'


vim +1727 drivers/net/ethernet/intel/ice/ice_sriov.c

a54a0b24f4f5a3 drivers/net/ethernet/intel/ice/ice_virtchnl_pf.c Nick Nunley     2020-07-13  1718  
a54a0b24f4f5a3 drivers/net/ethernet/intel/ice/ice_virtchnl_pf.c Nick Nunley     2020-07-13  1719  /**
a54a0b24f4f5a3 drivers/net/ethernet/intel/ice/ice_virtchnl_pf.c Nick Nunley     2020-07-13  1720   * ice_restore_all_vfs_msi_state - restore VF MSI state after PF FLR
a54a0b24f4f5a3 drivers/net/ethernet/intel/ice/ice_virtchnl_pf.c Nick Nunley     2020-07-13  1721   * @pdev: pointer to a pci_dev structure
a54a0b24f4f5a3 drivers/net/ethernet/intel/ice/ice_virtchnl_pf.c Nick Nunley     2020-07-13  1722   *
a54a0b24f4f5a3 drivers/net/ethernet/intel/ice/ice_virtchnl_pf.c Nick Nunley     2020-07-13  1723   * Called when recovering from a PF FLR to restore interrupt capability to
a54a0b24f4f5a3 drivers/net/ethernet/intel/ice/ice_virtchnl_pf.c Nick Nunley     2020-07-13  1724   * the VFs.
a54a0b24f4f5a3 drivers/net/ethernet/intel/ice/ice_virtchnl_pf.c Nick Nunley     2020-07-13  1725   */
ab8f0331b8a892 drivers/net/ethernet/intel/ice/ice_sriov.c       Przemek Kitszel 2023-08-16  1726  void ice_restore_all_vfs_msi_state(struct ice_pf *pf)
a54a0b24f4f5a3 drivers/net/ethernet/intel/ice/ice_virtchnl_pf.c Nick Nunley     2020-07-13 @1727  {

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH iwl-next] ice: store VF's pci_dev ptr in ice_vf

[Przemek Kitszel]

On 8/16/23 16:31, Leon Romanovsky wrote:
> On Wed, Aug 16, 2023 at 04:54:54AM -0400, Przemek Kitszel wrote:
>> Extend struct ice_vf by vfdev.
>> Calculation of vfdev falls more nicely into ice_create_vf_entries().
>>
>> Caching of vfdev enables simplification of ice_restore_all_vfs_msi_state().
> 
> I see that old code had access to pci_dev * of VF without any locking
> from concurrent PCI core access. How is it protected? How do you make
> sure that vfdev is valid?
> 
> Generally speaking, it is rarely good idea to cache VF pci_dev pointers
> inside driver.
> 
> Thanks

Overall, I do agree that ice driver, as a whole, has room for 
improvement in terms of synchronization, objects lifetime, and similar.

In this particular case, I don't see any reason of PCI reconfiguration 
during VF lifetime, but likely I'm missing something?

Other thing is that I've forgot to call pci_dev_put(vfdev),
thus this pointer will remain valid forever :~/
Assuming proper refcount balancing in v2, what could go wrong here?

> 
>>
>> Reviewed-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
>> Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>
>> Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
>> ---
>> add/remove: 0/0 grow/shrink: 2/1 up/down: 157/-130 (27)
>> Function                                     old     new   delta
>> ice_sriov_configure                         1712    1866    +154
>> ice_pci_err_resume                           168     171      +3
>> ice_restore_all_vfs_msi_state                200      70    -130
>> ---
>>   drivers/net/ethernet/intel/ice/ice_main.c   |  2 +-
>>   drivers/net/ethernet/intel/ice/ice_sriov.c  | 40 +++++++++------------
>>   drivers/net/ethernet/intel/ice/ice_sriov.h  |  4 +--
>>   drivers/net/ethernet/intel/ice/ice_vf_lib.h |  2 +-
>>   4 files changed, 21 insertions(+), 27 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
>> index a6dd336d2500..d04498c2fd6d 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_main.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_main.c
>> @@ -5561,7 +5561,7 @@ static void ice_pci_err_resume(struct pci_dev *pdev)
>>   		return;
>>   	}
>>   
>> -	ice_restore_all_vfs_msi_state(pdev);
>> +	ice_restore_all_vfs_msi_state(pf);
>>   
>>   	ice_do_reset(pf, ICE_RESET_PFR);
>>   	ice_service_task_restart(pf);
>> diff --git a/drivers/net/ethernet/intel/ice/ice_sriov.c b/drivers/net/ethernet/intel/ice/ice_sriov.c
>> index 31314e7540f8..48bc8ea55265 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_sriov.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_sriov.c
>> @@ -789,14 +789,19 @@ static const struct ice_vf_ops ice_sriov_vf_ops = {
>>    */
>>   static int ice_create_vf_entries(struct ice_pf *pf, u16 num_vfs)
>>   {
>> +	struct pci_dev *pdev = pf->pdev;
>>   	struct ice_vfs *vfs = &pf->vfs;
>> +	struct pci_dev *vfdev = NULL;
>>   	struct ice_vf *vf;
>> -	u16 vf_id;
>> -	int err;
>> +	u16 vf_pdev_id;
>> +	int err, pos;
>>   
>>   	lockdep_assert_held(&vfs->table_lock);
>>   
>> -	for (vf_id = 0; vf_id < num_vfs; vf_id++) {
>> +	pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_SRIOV);
>> +	pci_read_config_word(pdev, pos + PCI_SRIOV_VF_DID, &vf_pdev_id);
>> +
>> +	for (u16 vf_id = 0; vf_id < num_vfs; vf_id++) {
>>   		vf = kzalloc(sizeof(*vf), GFP_KERNEL);
>>   		if (!vf) {
>>   			err = -ENOMEM;
>> @@ -812,6 +817,10 @@ static int ice_create_vf_entries(struct ice_pf *pf, u16 num_vfs)
>>   
>>   		ice_initialize_vf_entry(vf);
>>   
>> +		do {
>> +			vfdev = pci_get_device(pdev->vendor, vf_pdev_id, vfdev);
>> +		} while (vfdev && vfdev->physfn != pdev);
>> +		vf->vfdev = vfdev;
>>   		vf->vf_sw_id = pf->first_sw;
>>   
>>   		hash_add_rcu(vfs->table, &vf->entry, vf_id);
>> @@ -1714,26 +1723,11 @@ void ice_print_vfs_mdd_events(struct ice_pf *pf)
>>    * Called when recovering from a PF FLR to restore interrupt capability to
>>    * the VFs.
>>    */
>> -void ice_restore_all_vfs_msi_state(struct pci_dev *pdev)
>> +void ice_restore_all_vfs_msi_state(struct ice_pf *pf)
>>   {
>> -	u16 vf_id;
>> -	int pos;
>> -
>> -	if (!pci_num_vf(pdev))
>> -		return;
>> +	struct ice_vf *vf;
>> +	u32 bkt;
>>   
>> -	pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_SRIOV);
>> -	if (pos) {
>> -		struct pci_dev *vfdev;
>> -
>> -		pci_read_config_word(pdev, pos + PCI_SRIOV_VF_DID,
>> -				     &vf_id);
>> -		vfdev = pci_get_device(pdev->vendor, vf_id, NULL);
>> -		while (vfdev) {
>> -			if (vfdev->is_virtfn && vfdev->physfn == pdev)
>> -				pci_restore_msi_state(vfdev);
>> -			vfdev = pci_get_device(pdev->vendor, vf_id,
>> -					       vfdev);
>> -		}
>> -	}
>> +	ice_for_each_vf(pf, bkt, vf)
>> +		pci_restore_msi_state(vf->vfdev);
>>   }
>> diff --git a/drivers/net/ethernet/intel/ice/ice_sriov.h b/drivers/net/ethernet/intel/ice/ice_sriov.h
>> index 346cb2666f3a..06829443d540 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_sriov.h
>> +++ b/drivers/net/ethernet/intel/ice/ice_sriov.h
>> @@ -33,7 +33,7 @@ int
>>   ice_get_vf_cfg(struct net_device *netdev, int vf_id, struct ifla_vf_info *ivi);
>>   
>>   void ice_free_vfs(struct ice_pf *pf);
>> -void ice_restore_all_vfs_msi_state(struct pci_dev *pdev);
>> +void ice_restore_all_vfs_msi_state(struct ice_pf *pf);
>>   
>>   int
>>   ice_set_vf_port_vlan(struct net_device *netdev, int vf_id, u16 vlan_id, u8 qos,
>> @@ -67,7 +67,7 @@ static inline
>>   void ice_vf_lan_overflow_event(struct ice_pf *pf, struct ice_rq_event_info *event) { }
>>   static inline void ice_print_vfs_mdd_events(struct ice_pf *pf) { }
>>   static inline void ice_print_vf_rx_mdd_event(struct ice_vf *vf) { }
>> -static inline void ice_restore_all_vfs_msi_state(struct pci_dev *pdev) { }
>> +static inline void ice_restore_all_vfs_msi_state(struct ice_pf *pf) { }
>>   
>>   static inline int
>>   ice_sriov_configure(struct pci_dev __always_unused *pdev,
>> diff --git a/drivers/net/ethernet/intel/ice/ice_vf_lib.h b/drivers/net/ethernet/intel/ice/ice_vf_lib.h
>> index 48fea6fa0362..57c36e4ccf91 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_vf_lib.h
>> +++ b/drivers/net/ethernet/intel/ice/ice_vf_lib.h
>> @@ -82,7 +82,7 @@ struct ice_vf {
>>   	struct rcu_head rcu;
>>   	struct kref refcnt;
>>   	struct ice_pf *pf;
>> -
>> +	struct pci_dev *vfdev;
>>   	/* Used during virtchnl message handling and NDO ops against the VF
>>   	 * that will trigger a VFR
>>   	 */
>>
>> base-commit: 0ad204c4acb8ba1ed99564b001609e62547bc79d
>> -- 
>> 2.40.1
>>
>>
> 

Re: [PATCH iwl-next] ice: store VF's pci_dev ptr in ice_vf

[Leon Romanovsky]

On Fri, Aug 18, 2023 at 02:20:51PM +0200, Przemek Kitszel wrote:
> On 8/16/23 16:31, Leon Romanovsky wrote:
> > On Wed, Aug 16, 2023 at 04:54:54AM -0400, Przemek Kitszel wrote:
> > > Extend struct ice_vf by vfdev.
> > > Calculation of vfdev falls more nicely into ice_create_vf_entries().
> > > 
> > > Caching of vfdev enables simplification of ice_restore_all_vfs_msi_state().
> > 
> > I see that old code had access to pci_dev * of VF without any locking
> > from concurrent PCI core access. How is it protected? How do you make
> > sure that vfdev is valid?
> > 
> > Generally speaking, it is rarely good idea to cache VF pci_dev pointers
> > inside driver.
> > 
> > Thanks
> 
> Overall, I do agree that ice driver, as a whole, has room for improvement in
> terms of synchronization, objects lifetime, and similar.
> 
> In this particular case, I don't see any reason of PCI reconfiguration
> during VF lifetime, but likely I'm missing something?

You are caching VF pointer in PF, and you are subjected to PF lifetime
and not VF lifetime.

Thanks

Re: [PATCH iwl-next] ice: store VF's pci_dev ptr in ice_vf

[Przemek Kitszel]

On 8/18/23 20:20, Leon Romanovsky wrote:
> On Fri, Aug 18, 2023 at 02:20:51PM +0200, Przemek Kitszel wrote:
>> On 8/16/23 16:31, Leon Romanovsky wrote:
>>> On Wed, Aug 16, 2023 at 04:54:54AM -0400, Przemek Kitszel wrote:
>>>> Extend struct ice_vf by vfdev.
>>>> Calculation of vfdev falls more nicely into ice_create_vf_entries().
>>>>
>>>> Caching of vfdev enables simplification of ice_restore_all_vfs_msi_state().
>>>
>>> I see that old code had access to pci_dev * of VF without any locking
>>> from concurrent PCI core access. How is it protected? How do you make
>>> sure that vfdev is valid?
>>>
>>> Generally speaking, it is rarely good idea to cache VF pci_dev pointers
>>> inside driver.
>>>
>>> Thanks
>>
>> Overall, I do agree that ice driver, as a whole, has room for improvement in
>> terms of synchronization, objects lifetime, and similar.
>>
>> In this particular case, I don't see any reason of PCI reconfiguration
>> during VF lifetime, but likely I'm missing something?
> 
> You are caching VF pointer in PF, 

that's correct that the driver is PF/ice

> and you are subjected to PF lifetime
> and not VF lifetime.

this belongs to struct ice_vf, which should have VF lifetime,
otherwise it's already at risk

> 
> Thanks

Thank you!

Re: [PATCH iwl-next] ice: store VF's pci_dev ptr in ice_vf

[Leon Romanovsky]

On Mon, Aug 21, 2023 at 12:48:40PM +0200, Przemek Kitszel wrote:
> On 8/18/23 20:20, Leon Romanovsky wrote:
> > On Fri, Aug 18, 2023 at 02:20:51PM +0200, Przemek Kitszel wrote:
> > > On 8/16/23 16:31, Leon Romanovsky wrote:
> > > > On Wed, Aug 16, 2023 at 04:54:54AM -0400, Przemek Kitszel wrote:
> > > > > Extend struct ice_vf by vfdev.
> > > > > Calculation of vfdev falls more nicely into ice_create_vf_entries().
> > > > > 
> > > > > Caching of vfdev enables simplification of ice_restore_all_vfs_msi_state().
> > > > 
> > > > I see that old code had access to pci_dev * of VF without any locking
> > > > from concurrent PCI core access. How is it protected? How do you make
> > > > sure that vfdev is valid?
> > > > 
> > > > Generally speaking, it is rarely good idea to cache VF pci_dev pointers
> > > > inside driver.
> > > > 
> > > > Thanks
> > > 
> > > Overall, I do agree that ice driver, as a whole, has room for improvement in
> > > terms of synchronization, objects lifetime, and similar.
> > > 
> > > In this particular case, I don't see any reason of PCI reconfiguration
> > > during VF lifetime, but likely I'm missing something?
> > 
> > You are caching VF pointer in PF,
> 
> that's correct that the driver is PF/ice
> 
> > and you are subjected to PF lifetime
> > and not VF lifetime.
> 
> this belongs to struct ice_vf, which should have VF lifetime,
> otherwise it's already at risk

I'm not so sure about it. ICE used to use devm_* API and not explicit
kalloc/kfree calls, it is not clear anymore the lifetime scope of VF
structure.

Thanks

> 
> > 
> > Thanks
> 
> Thank you!

RE: [PATCH iwl-next] ice: store VF's pci_dev ptr in ice_vf

[Keller, Jacob E]

> -----Original Message-----
> From: Leon Romanovsky <leon@kernel.org>
> Sent: Monday, August 21, 2023 4:02 AM
> To: Kitszel, Przemyslaw <przemyslaw.kitszel@intel.com>
> Cc: Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Brandeburg, Jesse
> <jesse.brandeburg@intel.com>; intel-wired-lan@lists.osuosl.org;
> netdev@vger.kernel.org; Polchlopek, Mateusz <mateusz.polchlopek@intel.com>;
> Keller, Jacob E <jacob.e.keller@intel.com>
> Subject: Re: [PATCH iwl-next] ice: store VF's pci_dev ptr in ice_vf
> 
> On Mon, Aug 21, 2023 at 12:48:40PM +0200, Przemek Kitszel wrote:
> > On 8/18/23 20:20, Leon Romanovsky wrote:
> > > On Fri, Aug 18, 2023 at 02:20:51PM +0200, Przemek Kitszel wrote:
> > > > On 8/16/23 16:31, Leon Romanovsky wrote:
> > > > > On Wed, Aug 16, 2023 at 04:54:54AM -0400, Przemek Kitszel wrote:
> > > > > > Extend struct ice_vf by vfdev.
> > > > > > Calculation of vfdev falls more nicely into ice_create_vf_entries().
> > > > > >
> > > > > > Caching of vfdev enables simplification of
> ice_restore_all_vfs_msi_state().
> > > > >
> > > > > I see that old code had access to pci_dev * of VF without any locking
> > > > > from concurrent PCI core access. How is it protected? How do you make
> > > > > sure that vfdev is valid?
> > > > >
> > > > > Generally speaking, it is rarely good idea to cache VF pci_dev pointers
> > > > > inside driver.
> > > > >
> > > > > Thanks
> > > >
> > > > Overall, I do agree that ice driver, as a whole, has room for improvement in
> > > > terms of synchronization, objects lifetime, and similar.
> > > >
> > > > In this particular case, I don't see any reason of PCI reconfiguration
> > > > during VF lifetime, but likely I'm missing something?
> > >
> > > You are caching VF pointer in PF,
> >
> > that's correct that the driver is PF/ice
> >
> > > and you are subjected to PF lifetime
> > > and not VF lifetime.
> >
> > this belongs to struct ice_vf, which should have VF lifetime,
> > otherwise it's already at risk
> 
> I'm not so sure about it. ICE used to use devm_* API and not explicit
> kalloc/kfree calls, it is not clear anymore the lifetime scope of VF
> structure.
> 
> Thanks
> 

The ice_vf structure is now reference counted with a kref, and is created when VFs are added, and removed when the VF is removed.

Thanks,
Jake

Re: [PATCH iwl-next] ice: store VF's pci_dev ptr in ice_vf

[Leon Romanovsky]

On Tue, Aug 22, 2023 at 08:46:46PM +0000, Keller, Jacob E wrote:
> 
> 
> > -----Original Message-----
> > From: Leon Romanovsky <leon@kernel.org>
> > Sent: Monday, August 21, 2023 4:02 AM
> > To: Kitszel, Przemyslaw <przemyslaw.kitszel@intel.com>
> > Cc: Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Brandeburg, Jesse
> > <jesse.brandeburg@intel.com>; intel-wired-lan@lists.osuosl.org;
> > netdev@vger.kernel.org; Polchlopek, Mateusz <mateusz.polchlopek@intel.com>;
> > Keller, Jacob E <jacob.e.keller@intel.com>
> > Subject: Re: [PATCH iwl-next] ice: store VF's pci_dev ptr in ice_vf
> > 
> > On Mon, Aug 21, 2023 at 12:48:40PM +0200, Przemek Kitszel wrote:
> > > On 8/18/23 20:20, Leon Romanovsky wrote:
> > > > On Fri, Aug 18, 2023 at 02:20:51PM +0200, Przemek Kitszel wrote:
> > > > > On 8/16/23 16:31, Leon Romanovsky wrote:
> > > > > > On Wed, Aug 16, 2023 at 04:54:54AM -0400, Przemek Kitszel wrote:
> > > > > > > Extend struct ice_vf by vfdev.
> > > > > > > Calculation of vfdev falls more nicely into ice_create_vf_entries().
> > > > > > >
> > > > > > > Caching of vfdev enables simplification of
> > ice_restore_all_vfs_msi_state().
> > > > > >
> > > > > > I see that old code had access to pci_dev * of VF without any locking
> > > > > > from concurrent PCI core access. How is it protected? How do you make
> > > > > > sure that vfdev is valid?
> > > > > >
> > > > > > Generally speaking, it is rarely good idea to cache VF pci_dev pointers
> > > > > > inside driver.
> > > > > >
> > > > > > Thanks
> > > > >
> > > > > Overall, I do agree that ice driver, as a whole, has room for improvement in
> > > > > terms of synchronization, objects lifetime, and similar.
> > > > >
> > > > > In this particular case, I don't see any reason of PCI reconfiguration
> > > > > during VF lifetime, but likely I'm missing something?
> > > >
> > > > You are caching VF pointer in PF,
> > >
> > > that's correct that the driver is PF/ice
> > >
> > > > and you are subjected to PF lifetime
> > > > and not VF lifetime.
> > >
> > > this belongs to struct ice_vf, which should have VF lifetime,
> > > otherwise it's already at risk
> > 
> > I'm not so sure about it. ICE used to use devm_* API and not explicit
> > kalloc/kfree calls, it is not clear anymore the lifetime scope of VF
> > structure.
> > 
> > Thanks
> > 
> 
> The ice_vf structure is now reference counted with a kref, and is created when VFs are added, and removed when the VF is removed.

Ohh, great, thanks

> 
> Thanks,
> Jake
> 
>