[Intel-wired-lan] [PATCH net-next 0/6] minor cleanups for ice virtualization

[Intel-wired-lan] [PATCH net-next 0/6] minor cleanups for ice virtualization

[Jacob Keller]

This series contains a few minor fix ups for virtualization files. Mainly
this includes fixes for function header comments and minor wording changes.

The only larger change is a patch which adds NULL checks to ice_get_vf_vsi.
This was done in order to prevent static analysis tools from complaining
that a NULL value could be dereferenced. Flows where an error is not
feasible have a WARN_ON check added. This change enhances the robustness of
the driver by avoiding a panic in the event that a VF does not have a valid
VSI.

Jacob Keller (6):
  ice: add newline to dev_dbg in ice_vf_fdir_dump_info
  ice: always check VF VSI pointer values
  ice: remove return value comment for ice_reset_all_vfs
  ice: fix wording in comment for ice_reset_vf
  ice: add a function comment for ice_cfg_mac_antispoof
  ice: remove period on argument description in ice_for_each_vf

 drivers/net/ethernet/intel/ice/ice_devlink.c  |  5 ++-
 drivers/net/ethernet/intel/ice/ice_repr.c     |  7 ++-
 drivers/net/ethernet/intel/ice/ice_sriov.c    | 32 ++++++++++++--
 drivers/net/ethernet/intel/ice/ice_vf_lib.c   | 43 ++++++++++++++++---
 drivers/net/ethernet/intel/ice/ice_vf_lib.h   |  4 +-
 drivers/net/ethernet/intel/ice/ice_virtchnl.c |  5 +++
 .../ethernet/intel/ice/ice_virtchnl_fdir.c    |  9 +++-
 7 files changed, 91 insertions(+), 14 deletions(-)


base-commit: c3ac33fdeac6d1a23f2d28aafaee5520632c159f
-- 
2.35.1.456.ga9c7032d4631

[Intel-wired-lan] [PATCH net-next 1/6] ice: add newline to dev_dbg in ice_vf_fdir_dump_info

[Jacob Keller]

The debug print in ice_vf_fdir_dump_info don't end in newlines. This can
look confusing when reading the kernel log, as the next print will
immediately continue on the same line.

Fix this.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_virtchnl_fdir.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_virtchnl_fdir.c b/drivers/net/ethernet/intel/ice/ice_virtchnl_fdir.c
index 8e38ee2faf58..dbc1965c0609 100644
--- a/drivers/net/ethernet/intel/ice/ice_virtchnl_fdir.c
+++ b/drivers/net/ethernet/intel/ice/ice_virtchnl_fdir.c
@@ -1349,7 +1349,7 @@ static void ice_vf_fdir_dump_info(struct ice_vf *vf)
 
 	fd_size = rd32(hw, VSIQF_FD_SIZE(vsi_num));
 	fd_cnt = rd32(hw, VSIQF_FD_CNT(vsi_num));
-	dev_dbg(dev, "VF %d: space allocated: guar:0x%x, be:0x%x, space consumed: guar:0x%x, be:0x%x",
+	dev_dbg(dev, "VF %d: space allocated: guar:0x%x, be:0x%x, space consumed: guar:0x%x, be:0x%x\n",
 		vf->vf_id,
 		(fd_size & VSIQF_FD_CNT_FD_GCNT_M) >> VSIQF_FD_CNT_FD_GCNT_S,
 		(fd_size & VSIQF_FD_CNT_FD_BCNT_M) >> VSIQF_FD_CNT_FD_BCNT_S,
-- 
2.35.1.456.ga9c7032d4631

[Intel-wired-lan] [PATCH net-next 1/6] ice: add newline to dev_dbg in ice_vf_fdir_dump_info

[Paul Menzel]

Dear Jacob,


Am 12.04.22 um 01:29 schrieb Jacob Keller:
> The debug print in ice_vf_fdir_dump_info don't end in newlines. This can

Singular? does not

> look confusing when reading the kernel log, as the next print will
> immediately continue on the same line.
> 
> Fix this.
> 

Add a Fixes tag?

Maybe also add *forgotten* to the commit message summary:

Add forgotten newline ?

> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> ---
>   drivers/net/ethernet/intel/ice/ice_virtchnl_fdir.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_virtchnl_fdir.c b/drivers/net/ethernet/intel/ice/ice_virtchnl_fdir.c
> index 8e38ee2faf58..dbc1965c0609 100644
> --- a/drivers/net/ethernet/intel/ice/ice_virtchnl_fdir.c
> +++ b/drivers/net/ethernet/intel/ice/ice_virtchnl_fdir.c
> @@ -1349,7 +1349,7 @@ static void ice_vf_fdir_dump_info(struct ice_vf *vf)
>   
>   	fd_size = rd32(hw, VSIQF_FD_SIZE(vsi_num));
>   	fd_cnt = rd32(hw, VSIQF_FD_CNT(vsi_num));
> -	dev_dbg(dev, "VF %d: space allocated: guar:0x%x, be:0x%x, space consumed: guar:0x%x, be:0x%x",
> +	dev_dbg(dev, "VF %d: space allocated: guar:0x%x, be:0x%x, space consumed: guar:0x%x, be:0x%x\n",
>   		vf->vf_id,
>   		(fd_size & VSIQF_FD_CNT_FD_GCNT_M) >> VSIQF_FD_CNT_FD_GCNT_S,
>   		(fd_size & VSIQF_FD_CNT_FD_BCNT_M) >> VSIQF_FD_CNT_FD_BCNT_S,

Reviewed-by: Paul Menzel <pmenzel@molgen.mpg.de>


Kind regards,

Paul

[Intel-wired-lan] [PATCH net-next 1/6] ice: add newline to dev_dbg in ice_vf_fdir_dump_info

[Keller, Jacob E]

> -----Original Message-----
> From: Paul Menzel <pmenzel@molgen.mpg.de>
> Sent: Monday, April 11, 2022 10:11 PM
> To: Keller, Jacob E <jacob.e.keller@intel.com>
> Cc: intel-wired-lan at lists.osuosl.org
> Subject: Re: [Intel-wired-lan] [PATCH net-next 1/6] ice: add newline to dev_dbg in
> ice_vf_fdir_dump_info
> 
> Dear Jacob,
> 
> 
> Am 12.04.22 um 01:29 schrieb Jacob Keller:
> > The debug print in ice_vf_fdir_dump_info don't end in newlines. This can
> 
> Singular? does not
> 

Ah, yep.

> > look confusing when reading the kernel log, as the next print will
> > immediately continue on the same line.
> >
> > Fix this.
> >
> 
> Add a Fixes tag?
> 
> Maybe also add *forgotten* to the commit message summary:
> 
> Add forgotten newline ?
> 

Makes sense.

> > Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> > ---
> >   drivers/net/ethernet/intel/ice/ice_virtchnl_fdir.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/ethernet/intel/ice/ice_virtchnl_fdir.c
> b/drivers/net/ethernet/intel/ice/ice_virtchnl_fdir.c
> > index 8e38ee2faf58..dbc1965c0609 100644
> > --- a/drivers/net/ethernet/intel/ice/ice_virtchnl_fdir.c
> > +++ b/drivers/net/ethernet/intel/ice/ice_virtchnl_fdir.c
> > @@ -1349,7 +1349,7 @@ static void ice_vf_fdir_dump_info(struct ice_vf *vf)
> >
> >   	fd_size = rd32(hw, VSIQF_FD_SIZE(vsi_num));
> >   	fd_cnt = rd32(hw, VSIQF_FD_CNT(vsi_num));
> > -	dev_dbg(dev, "VF %d: space allocated: guar:0x%x, be:0x%x, space
> consumed: guar:0x%x, be:0x%x",
> > +	dev_dbg(dev, "VF %d: space allocated: guar:0x%x, be:0x%x, space
> consumed: guar:0x%x, be:0x%x\n",
> >   		vf->vf_id,
> >   		(fd_size & VSIQF_FD_CNT_FD_GCNT_M) >>
> VSIQF_FD_CNT_FD_GCNT_S,
> >   		(fd_size & VSIQF_FD_CNT_FD_BCNT_M) >>
> VSIQF_FD_CNT_FD_BCNT_S,
> 
> Reviewed-by: Paul Menzel <pmenzel@molgen.mpg.de>
> 
> 
> Kind regards,
> 
> Paul

[Intel-wired-lan] [PATCH net-next 2/6] ice: always check VF VSI pointer values

[Jacob Keller]

The ice_get_vf_vsi function can return NULL in some cases, such as if
handling messages during a reset where the VSI is being removed and
recreated.

Several places throughout the driver do not bother to check whether this
VSI pointer is valid. Static analysis tools maybe report issues because
they detect paths where a potentially NULL pointer could be
dereferenced.

Fix this by checking the return value of ice_get_vf_vsi everywhere.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_devlink.c  |  5 ++-
 drivers/net/ethernet/intel/ice/ice_repr.c     |  7 +++-
 drivers/net/ethernet/intel/ice/ice_sriov.c    | 32 +++++++++++++++++--
 drivers/net/ethernet/intel/ice/ice_vf_lib.c   | 28 +++++++++++++++-
 drivers/net/ethernet/intel/ice/ice_virtchnl.c |  5 +++
 .../ethernet/intel/ice/ice_virtchnl_fdir.c    |  7 +++-
 6 files changed, 77 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_devlink.c b/drivers/net/ethernet/intel/ice/ice_devlink.c
index a230edb38466..4a9de59121d8 100644
--- a/drivers/net/ethernet/intel/ice/ice_devlink.c
+++ b/drivers/net/ethernet/intel/ice/ice_devlink.c
@@ -753,9 +753,12 @@ int ice_devlink_create_vf_port(struct ice_vf *vf)
 
 	pf = vf->pf;
 	dev = ice_pf_to_dev(pf);
-	vsi = ice_get_vf_vsi(vf);
 	devlink_port = &vf->devlink_port;
 
+	vsi = ice_get_vf_vsi(vf);
+	if (!vsi)
+		return -EINVAL;
+
 	attrs.flavour = DEVLINK_PORT_FLAVOUR_PCI_VF;
 	attrs.pci_vf.pf = pf->hw.bus.func;
 	attrs.pci_vf.vf = vf->vf_id;
diff --git a/drivers/net/ethernet/intel/ice/ice_repr.c b/drivers/net/ethernet/intel/ice/ice_repr.c
index 848f2adea563..a91b81c3088b 100644
--- a/drivers/net/ethernet/intel/ice/ice_repr.c
+++ b/drivers/net/ethernet/intel/ice/ice_repr.c
@@ -293,8 +293,13 @@ static int ice_repr_add(struct ice_vf *vf)
 	struct ice_q_vector *q_vector;
 	struct ice_netdev_priv *np;
 	struct ice_repr *repr;
+	struct ice_vsi *vsi;
 	int err;
 
+	vsi = ice_get_vf_vsi(vf);
+	if (!vsi)
+		return -EINVAL;
+
 	repr = kzalloc(sizeof(*repr), GFP_KERNEL);
 	if (!repr)
 		return -ENOMEM;
@@ -313,7 +318,7 @@ static int ice_repr_add(struct ice_vf *vf)
 		goto err_alloc;
 	}
 
-	repr->src_vsi = ice_get_vf_vsi(vf);
+	repr->src_vsi = vsi;
 	repr->vf = vf;
 	vf->repr = repr;
 	np = netdev_priv(repr->netdev);
diff --git a/drivers/net/ethernet/intel/ice/ice_sriov.c b/drivers/net/ethernet/intel/ice/ice_sriov.c
index 8915a9d39e36..29278e23691f 100644
--- a/drivers/net/ethernet/intel/ice/ice_sriov.c
+++ b/drivers/net/ethernet/intel/ice/ice_sriov.c
@@ -46,7 +46,12 @@ static void ice_free_vf_entries(struct ice_pf *pf)
  */
 static void ice_vf_vsi_release(struct ice_vf *vf)
 {
-	ice_vsi_release(ice_get_vf_vsi(vf));
+	struct ice_vsi *vsi = ice_get_vf_vsi(vf);
+
+	if (WARN_ON(!vsi))
+		return;
+
+	ice_vsi_release(vsi);
 	ice_vf_invalidate_vsi(vf);
 }
 
@@ -104,6 +109,8 @@ static void ice_dis_vf_mappings(struct ice_vf *vf)
 
 	hw = &pf->hw;
 	vsi = ice_get_vf_vsi(vf);
+	if (WARN_ON(!vsi))
+		return;
 
 	dev = ice_pf_to_dev(pf);
 	wr32(hw, VPINT_ALLOC(vf->vf_id), 0);
@@ -341,6 +348,9 @@ static void ice_ena_vf_q_mappings(struct ice_vf *vf, u16 max_txq, u16 max_rxq)
 	struct ice_hw *hw = &vf->pf->hw;
 	u32 reg;
 
+	if (WARN_ON(!vsi))
+		return;
+
 	/* set regardless of mapping mode */
 	wr32(hw, VPLAN_TXQ_MAPENA(vf->vf_id), VPLAN_TXQ_MAPENA_TX_ENA_M);
 
@@ -386,6 +396,9 @@ static void ice_ena_vf_mappings(struct ice_vf *vf)
 {
 	struct ice_vsi *vsi = ice_get_vf_vsi(vf);
 
+	if (WARN_ON(!vsi))
+		return;
+
 	ice_ena_vf_msix_mappings(vf);
 	ice_ena_vf_q_mappings(vf, vsi->alloc_txq, vsi->alloc_rxq);
 }
@@ -1128,6 +1141,8 @@ static struct ice_vf *ice_get_vf_from_pfq(struct ice_pf *pf, u16 pfq)
 		u16 rxq_idx;
 
 		vsi = ice_get_vf_vsi(vf);
+		if (!vsi)
+			continue;
 
 		ice_for_each_rxq(vsi, rxq_idx)
 			if (vsi->rxq_map[rxq_idx] == pfq) {
@@ -1521,8 +1536,15 @@ static int ice_calc_all_vfs_min_tx_rate(struct ice_pf *pf)
 static bool
 ice_min_tx_rate_oversubscribed(struct ice_vf *vf, int min_tx_rate)
 {
-	int link_speed_mbps = ice_get_link_speed_mbps(ice_get_vf_vsi(vf));
-	int all_vfs_min_tx_rate = ice_calc_all_vfs_min_tx_rate(vf->pf);
+	struct ice_vsi *vsi = ice_get_vf_vsi(vf);
+	int all_vfs_min_tx_rate;
+	int link_speed_mbps;
+
+	if (WARN_ON(!vsi))
+		return false;
+
+	link_speed_mbps = ice_get_link_speed_mbps(vsi);
+	all_vfs_min_tx_rate = ice_calc_all_vfs_min_tx_rate(vf->pf);
 
 	/* this VF's previous rate is being overwritten */
 	all_vfs_min_tx_rate -= vf->min_tx_rate;
@@ -1566,6 +1588,10 @@ ice_set_vf_bw(struct net_device *netdev, int vf_id, int min_tx_rate,
 		goto out_put_vf;
 
 	vsi = ice_get_vf_vsi(vf);
+	if (!vsi) {
+		ret = -EINVAL;
+		goto out_put_vf;
+	}
 
 	/* when max_tx_rate is zero that means no max Tx rate limiting, so only
 	 * check if max_tx_rate is non-zero
diff --git a/drivers/net/ethernet/intel/ice/ice_vf_lib.c b/drivers/net/ethernet/intel/ice/ice_vf_lib.c
index 6578059d9479..aefd66a4db80 100644
--- a/drivers/net/ethernet/intel/ice/ice_vf_lib.c
+++ b/drivers/net/ethernet/intel/ice/ice_vf_lib.c
@@ -220,8 +220,10 @@ static void ice_vf_clear_counters(struct ice_vf *vf)
 {
 	struct ice_vsi *vsi = ice_get_vf_vsi(vf);
 
+	if (vsi)
+		vsi->num_vlan = 0;
+
 	vf->num_mac = 0;
-	vsi->num_vlan = 0;
 	memset(&vf->mdd_tx_events, 0, sizeof(vf->mdd_tx_events));
 	memset(&vf->mdd_rx_events, 0, sizeof(vf->mdd_rx_events));
 }
@@ -251,6 +253,9 @@ static int ice_vf_rebuild_vsi(struct ice_vf *vf)
 	struct ice_vsi *vsi = ice_get_vf_vsi(vf);
 	struct ice_pf *pf = vf->pf;
 
+	if (WARN_ON(!vsi))
+		return -EINVAL;
+
 	if (ice_vsi_rebuild(vsi, true)) {
 		dev_err(ice_pf_to_dev(pf), "failed to rebuild VF %d VSI\n",
 			vf->vf_id);
@@ -514,6 +519,10 @@ int ice_reset_vf(struct ice_vf *vf, u32 flags)
 	ice_trigger_vf_reset(vf, flags & ICE_VF_RESET_VFLR, false);
 
 	vsi = ice_get_vf_vsi(vf);
+	if (WARN_ON(!vsi)) {
+		err = -EIO;
+		goto out_unlock;
+	}
 
 	ice_dis_vf_qs(vf);
 
@@ -572,6 +581,11 @@ int ice_reset_vf(struct ice_vf *vf, u32 flags)
 
 	vf->vf_ops->post_vsi_rebuild(vf);
 	vsi = ice_get_vf_vsi(vf);
+	if (WARN_ON(!vsi)) {
+		err = -EINVAL;
+		goto out_unlock;
+	}
+
 	ice_eswitch_update_repr(vsi);
 	ice_eswitch_replay_vf_mac_rule(vf);
 
@@ -610,6 +624,9 @@ void ice_dis_vf_qs(struct ice_vf *vf)
 {
 	struct ice_vsi *vsi = ice_get_vf_vsi(vf);
 
+	if (WARN_ON(!vsi))
+		return;
+
 	ice_vsi_stop_lan_tx_rings(vsi, ICE_NO_RESET, vf->vf_id);
 	ice_vsi_stop_all_rx_rings(vsi);
 	ice_set_vf_state_qs_dis(vf);
@@ -790,6 +807,9 @@ static int ice_vf_rebuild_host_mac_cfg(struct ice_vf *vf)
 	u8 broadcast[ETH_ALEN];
 	int status;
 
+	if (WARN_ON(!vsi))
+		return -EINVAL;
+
 	if (ice_is_eswitch_mode_switchdev(vf->pf))
 		return 0;
 
@@ -875,6 +895,9 @@ static int ice_vf_rebuild_host_tx_rate_cfg(struct ice_vf *vf)
 	struct ice_vsi *vsi = ice_get_vf_vsi(vf);
 	int err;
 
+	if (WARN_ON(!vsi))
+		return -EINVAL;
+
 	if (vf->min_tx_rate) {
 		err = ice_set_min_bw_limit(vsi, (u64)vf->min_tx_rate * 1000);
 		if (err) {
@@ -938,6 +961,9 @@ void ice_vf_rebuild_host_cfg(struct ice_vf *vf)
 	struct device *dev = ice_pf_to_dev(vf->pf);
 	struct ice_vsi *vsi = ice_get_vf_vsi(vf);
 
+	if (WARN_ON(!vsi))
+		return;
+
 	ice_vf_set_host_trust_cfg(vf);
 
 	if (ice_vf_rebuild_host_mac_cfg(vf))
diff --git a/drivers/net/ethernet/intel/ice/ice_virtchnl.c b/drivers/net/ethernet/intel/ice/ice_virtchnl.c
index 8ddb462e1af2..ca13995cc5ab 100644
--- a/drivers/net/ethernet/intel/ice/ice_virtchnl.c
+++ b/drivers/net/ethernet/intel/ice/ice_virtchnl.c
@@ -2342,6 +2342,11 @@ static int ice_vc_ena_vlan_stripping(struct ice_vf *vf)
 	}
 
 	vsi = ice_get_vf_vsi(vf);
+	if (!vsi) {
+		v_ret = VIRTCHNL_STATUS_ERR_PARAM;
+		goto error_param;
+	}
+
 	if (vsi->inner_vlan_ops.ena_stripping(vsi, ETH_P_8021Q))
 		v_ret = VIRTCHNL_STATUS_ERR_PARAM;
 
diff --git a/drivers/net/ethernet/intel/ice/ice_virtchnl_fdir.c b/drivers/net/ethernet/intel/ice/ice_virtchnl_fdir.c
index dbc1965c0609..c6a58343d81d 100644
--- a/drivers/net/ethernet/intel/ice/ice_virtchnl_fdir.c
+++ b/drivers/net/ethernet/intel/ice/ice_virtchnl_fdir.c
@@ -1344,7 +1344,12 @@ static void ice_vf_fdir_dump_info(struct ice_vf *vf)
 	pf = vf->pf;
 	hw = &pf->hw;
 	dev = ice_pf_to_dev(pf);
-	vf_vsi = pf->vsi[vf->lan_vsi_idx];
+	vf_vsi = ice_get_vf_vsi(vf);
+	if (!vf_vsi) {
+		dev_dbg(dev, "VF %d: invalid VSI pointer\n", vf->vf_id);
+		return;
+	}
+
 	vsi_num = ice_get_hw_vsi_num(hw, vf_vsi->idx);
 
 	fd_size = rd32(hw, VSIQF_FD_SIZE(vsi_num));
-- 
2.35.1.456.ga9c7032d4631

[Intel-wired-lan] [PATCH net-next 2/6] ice: always check VF VSI pointer values

[Paul Menzel]

Dear Jacob,


Thank you for the patch.

Am 12.04.22 um 01:29 schrieb Jacob Keller:
> The ice_get_vf_vsi function can return NULL in some cases, such as if
> handling messages during a reset where the VSI is being removed and
> recreated.
> 
> Several places throughout the driver do not bother to check whether this
> VSI pointer is valid. Static analysis tools maybe report issues because
> they detect paths where a potentially NULL pointer could be
> dereferenced.

(side note: scripts/checkpatch.pl checks for 75 characters per line, and 
you seem to use 72 characters per line.)

> Fix this by checking the return value of ice_get_vf_vsi everywhere.

I didn?t understand, when WARN_ON() is necessary, and when not, but 
looks fine.

> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> ---
>   drivers/net/ethernet/intel/ice/ice_devlink.c  |  5 ++-
>   drivers/net/ethernet/intel/ice/ice_repr.c     |  7 +++-
>   drivers/net/ethernet/intel/ice/ice_sriov.c    | 32 +++++++++++++++++--
>   drivers/net/ethernet/intel/ice/ice_vf_lib.c   | 28 +++++++++++++++-
>   drivers/net/ethernet/intel/ice/ice_virtchnl.c |  5 +++
>   .../ethernet/intel/ice/ice_virtchnl_fdir.c    |  7 +++-
>   6 files changed, 77 insertions(+), 7 deletions(-)

[?]

Reviewed-by: Paul Menzel <pmenzel@molgen.mpg.de>


Kind regards,

Paul

[Intel-wired-lan] [PATCH net-next 2/6] ice: always check VF VSI pointer values

[Jacob Keller]

On 4/11/2022 10:38 PM, Paul Menzel wrote:
> Dear Jacob,
> 
> 
> Thank you for the patch.
> 
> Am 12.04.22 um 01:29 schrieb Jacob Keller:
>> The ice_get_vf_vsi function can return NULL in some cases, such as if
>> handling messages during a reset where the VSI is being removed and
>> recreated.
>>
>> Several places throughout the driver do not bother to check whether this
>> VSI pointer is valid. Static analysis tools maybe report issues because
>> they detect paths where a potentially NULL pointer could be
>> dereferenced.
> 
> (side note: scripts/checkpatch.pl checks for 75 characters per line, and 
> you seem to use 72 characters per line.)
> 

For commit message wrapping? I use some default from a vim plugin which
I guess decided that 72 was a good choice. Technically thats 8
characters less than 80 which is one full tabstop if you render tabs as
8 spaces, which is sometimes used as a method of indenting commits from
git tools..

>> Fix this by checking the return value of ice_get_vf_vsi everywhere.
> 
> I didn?t understand, when WARN_ON() is necessary, and when not, but 
> looks fine.
> 

I tried my best to use WARN_ON in places which have no suitable way to
report an error back out, but for places where we can simply fail the
operation I avoided it.

This was similar to how we handled invalid VF pointers in certain places.

The WARN_ONs aren't strictly needed, but generally speaking those flows
shouldn't be hitting an invalid pointer. (Indeed, prior to this patch
they would have been BUGs instead!)

Hope that explains why not every place has the WARN_ON.

>> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
>> ---
>>   drivers/net/ethernet/intel/ice/ice_devlink.c  |  5 ++-
>>   drivers/net/ethernet/intel/ice/ice_repr.c     |  7 +++-
>>   drivers/net/ethernet/intel/ice/ice_sriov.c    | 32 +++++++++++++++++--
>>   drivers/net/ethernet/intel/ice/ice_vf_lib.c   | 28 +++++++++++++++-
>>   drivers/net/ethernet/intel/ice/ice_virtchnl.c |  5 +++
>>   .../ethernet/intel/ice/ice_virtchnl_fdir.c    |  7 +++-
>>   6 files changed, 77 insertions(+), 7 deletions(-)
> 
> [?]
> 
> Reviewed-by: Paul Menzel <pmenzel@molgen.mpg.de>
> 
> 
> Kind regards,
> 
> Paul

[Intel-wired-lan] [PATCH net-next 2/6] ice: always check VF VSI pointer values

[Paul Menzel]

Dear Jacob,


Am 12.04.22 um 19:35 schrieb Jacob Keller:

> On 4/11/2022 10:38 PM, Paul Menzel wrote:

>> Am 12.04.22 um 01:29 schrieb Jacob Keller:
>>> The ice_get_vf_vsi function can return NULL in some cases, such as if
>>> handling messages during a reset where the VSI is being removed and
>>> recreated.
>>>
>>> Several places throughout the driver do not bother to check whether this
>>> VSI pointer is valid. Static analysis tools maybe report issues because
>>> they detect paths where a potentially NULL pointer could be
>>> dereferenced.
>>
>> (side note: scripts/checkpatch.pl checks for 75 characters per line, and
>> you seem to use 72 characters per line.)
> 
> For commit message wrapping? I use some default from a vim plugin which
> I guess decided that 72 was a good choice. Technically thats 8
> characters less than 80 which is one full tabstop if you render tabs as
> 8 spaces, which is sometimes used as a method of indenting commits from
> git tools..

Yes, but the git tools indent with four spaces. See commit 2a076f40d8c9 
(checkpatch, SubmittingPatches: suggest line wrapping commit messages at 
75 columns) [1]:

> Commit messages lines are sometimes overly long.
> 
> Suggest line wrapping at 75 columns so the default git commit log
> indentation of 4 plus the commit message text still fits on an 80 column
> screen.
> 
> Add a checkpatch test for long commit messages lines too.


>>> Fix this by checking the return value of ice_get_vf_vsi everywhere.
>>
>> I didn?t understand, when WARN_ON() is necessary, and when not, but
>> looks fine.
>>
> 
> I tried my best to use WARN_ON in places which have no suitable way to
> report an error back out, but for places where we can simply fail the
> operation I avoided it.
> 
> This was similar to how we handled invalid VF pointers in certain places.
> 
> The WARN_ONs aren't strictly needed, but generally speaking those flows
> shouldn't be hitting an invalid pointer. (Indeed, prior to this patch
> they would have been BUGs instead!)
> 
> Hope that explains why not every place has the WARN_ON.

Yes, thank you for clarifying this.

>>> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
>>> ---
>>>    drivers/net/ethernet/intel/ice/ice_devlink.c  |  5 ++-
>>>    drivers/net/ethernet/intel/ice/ice_repr.c     |  7 +++-
>>>    drivers/net/ethernet/intel/ice/ice_sriov.c    | 32 +++++++++++++++++--
>>>    drivers/net/ethernet/intel/ice/ice_vf_lib.c   | 28 +++++++++++++++-
>>>    drivers/net/ethernet/intel/ice/ice_virtchnl.c |  5 +++
>>>    .../ethernet/intel/ice/ice_virtchnl_fdir.c    |  7 +++-
>>>    6 files changed, 77 insertions(+), 7 deletions(-)
>>
>> [?]
>>
>> Reviewed-by: Paul Menzel <pmenzel@molgen.mpg.de>


Kind regards,

Paul


[1]: 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=2a076f40d8c9be95bee7bcf18436655e1140447f

[Intel-wired-lan] [PATCH net-next 2/6] ice: always check VF VSI pointer values

[Jacob Keller]

On 4/12/2022 10:50 AM, Paul Menzel wrote:
> Dear Jacob,
> 
> 
> Am 12.04.22 um 19:35 schrieb Jacob Keller:
> 
>> On 4/11/2022 10:38 PM, Paul Menzel wrote:
> 
>>> Am 12.04.22 um 01:29 schrieb Jacob Keller:
>>>> The ice_get_vf_vsi function can return NULL in some cases, such as if
>>>> handling messages during a reset where the VSI is being removed and
>>>> recreated.
>>>>
>>>> Several places throughout the driver do not bother to check whether this
>>>> VSI pointer is valid. Static analysis tools maybe report issues because
>>>> they detect paths where a potentially NULL pointer could be
>>>> dereferenced.
>>>
>>> (side note: scripts/checkpatch.pl checks for 75 characters per line, and
>>> you seem to use 72 characters per line.)
>>
>> For commit message wrapping? I use some default from a vim plugin which
>> I guess decided that 72 was a good choice. Technically thats 8
>> characters less than 80 which is one full tabstop if you render tabs as
>> 8 spaces, which is sometimes used as a method of indenting commits from
>> git tools..
> 
> Yes, but the git tools indent with four spaces. See commit 2a076f40d8c9 
> (checkpatch, SubmittingPatches: suggest line wrapping commit messages at 
> 75 columns) [1]:
> 

I fixed my local editor config to use 75 characters going forward. I
also requested Tony fix the commits as he applied them to his tree, so
this should be corrected with what is applied to his next-queue now.

Thanks,
Jake

[Intel-wired-lan] [PATCH net-next 2/6] ice: always check VF VSI pointer values

[Jankowski, Konrad0]

> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
> Jacob Keller
> Sent: Tuesday, April 12, 2022 1:29 AM
> To: Intel Wired LAN <intel-wired-lan@lists.osuosl.org>
> Subject: [Intel-wired-lan] [PATCH net-next 2/6] ice: always check VF VSI
> pointer values
> 
> The ice_get_vf_vsi function can return NULL in some cases, such as if
> handling messages during a reset where the VSI is being removed and
> recreated.
> 
> Several places throughout the driver do not bother to check whether this VSI
> pointer is valid. Static analysis tools maybe report issues because they detect
> paths where a potentially NULL pointer could be dereferenced.
> 
> Fix this by checking the return value of ice_get_vf_vsi everywhere.
> 
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> ---
>  drivers/net/ethernet/intel/ice/ice_devlink.c  |  5 ++-
>  drivers/net/ethernet/intel/ice/ice_repr.c     |  7 +++-
>  drivers/net/ethernet/intel/ice/ice_sriov.c    | 32 +++++++++++++++++--
>  drivers/net/ethernet/intel/ice/ice_vf_lib.c   | 28 +++++++++++++++-
>  drivers/net/ethernet/intel/ice/ice_virtchnl.c |  5 +++
>  .../ethernet/intel/ice/ice_virtchnl_fdir.c    |  7 +++-
>  6 files changed, 77 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_devlink.c
> b/drivers/net/ethernet/intel/ice/ice_devlink.c
> index a230edb38466..4a9de59121d8 100644
> --- a/drivers/net/ethernet/intel/ice/ice_devlink.c
> +++ b/drivers/net/ethernet/intel/ice/ice_devlink.c
> @@ -753,9 +753,12 @@ int ice_devlink_create_vf_port(struct ice_vf *vf)
> 
>  	pf = vf->pf;
>  	dev = ice_pf_to_dev(pf);
> -	vsi = ice_get_vf_vsi(vf);
>  	devlink_port = &vf->devlink_port;
> 
> +	vsi = ice_get_vf_vsi(vf);
> +	if (!vsi)
> +		return -EINVAL;
> +
>  	attrs.flavour = DEVLINK_PORT_FLAVOUR_PCI_VF;
>  	attrs.pci_vf.pf = pf->hw.bus.func;
>  	attrs.pci_vf.vf = vf->vf_id;
> diff --git a/drivers/net/ethernet/intel/ice/ice_repr.c
> b/drivers/net/ethernet/intel/ice/ice_repr.c
> index 848f2adea563..a91b81c3088b 100644
> --- a/drivers/net/ethernet/intel/ice/ice_repr.c
> +++ b/drivers/net/ethernet/intel/ice/ice_repr.c
> @@ -293,8 +293,13 @@ static int ice_repr_add(struct ice_vf *vf)

Tested-by: Konrad Jankowski <konrad0.jankowski@intel.com>

[Intel-wired-lan] [PATCH net-next 3/6] ice: remove return value comment for ice_reset_all_vfs

[Jacob Keller]

Since commit fe99d1c06c16 ("ice: make ice_reset_all_vfs void"), the
ice_reset_all_vfs function has not returned anything. The function
comment still indicated it did. Fix this.

While here, also add a line to clarify the function resets all VFs at
once in response to hardware resets such as a PF reset.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_vf_lib.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_vf_lib.c b/drivers/net/ethernet/intel/ice/ice_vf_lib.c
index aefd66a4db80..24cf6a5b49fe 100644
--- a/drivers/net/ethernet/intel/ice/ice_vf_lib.c
+++ b/drivers/net/ethernet/intel/ice/ice_vf_lib.c
@@ -359,12 +359,12 @@ ice_vf_clear_vsi_promisc(struct ice_vf *vf, struct ice_vsi *vsi, u8 promisc_m)
  * ice_reset_all_vfs - reset all allocated VFs in one go
  * @pf: pointer to the PF structure
  *
+ * Reset all VFs at once, in response to a PF or other device reset.
+ *
  * First, tell the hardware to reset each VF, then do all the waiting in one
  * chunk, and finally finish restoring each VF after the wait. This is useful
  * during PF routines which need to reset all VFs, as otherwise it must perform
  * these resets in a serialized fashion.
- *
- * Returns true if any VFs were reset, and false otherwise.
  */
 void ice_reset_all_vfs(struct ice_pf *pf)
 {
-- 
2.35.1.456.ga9c7032d4631

[Intel-wired-lan] [PATCH net-next 4/6] ice: fix wording in comment for ice_reset_vf

[Jacob Keller]

The comment explaining ice_reset_vf has an extraneous "the" with the "if
the resets are disabled". Remove it.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_vf_lib.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_vf_lib.c b/drivers/net/ethernet/intel/ice/ice_vf_lib.c
index 24cf6a5b49fe..8f875a17755f 100644
--- a/drivers/net/ethernet/intel/ice/ice_vf_lib.c
+++ b/drivers/net/ethernet/intel/ice/ice_vf_lib.c
@@ -477,8 +477,8 @@ static void ice_notify_vf_reset(struct ice_vf *vf)
  *   ICE_VF_RESET_NOTIFY - Send VF a notification prior to reset
  *   ICE_VF_RESET_LOCK - Acquire VF cfg_lock before resetting
  *
- * Returns 0 if the VF is currently in reset, if the resets are disabled, or
- * if the VF resets successfully. Returns an error code if the VF fails to
+ * Returns 0 if the VF is currently in reset, if resets are disabled, or if
+ * the VF resets successfully. Returns an error code if the VF fails to
  * rebuild.
  */
 int ice_reset_vf(struct ice_vf *vf, u32 flags)
-- 
2.35.1.456.ga9c7032d4631

[Intel-wired-lan] [PATCH net-next 5/6] ice: add a function comment for ice_cfg_mac_antispoof

[Jacob Keller]

This function definition was missing a comment describing its
implementation. Add one.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_vf_lib.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/net/ethernet/intel/ice/ice_vf_lib.c b/drivers/net/ethernet/intel/ice/ice_vf_lib.c
index 8f875a17755f..cd8e6b50968c 100644
--- a/drivers/net/ethernet/intel/ice/ice_vf_lib.c
+++ b/drivers/net/ethernet/intel/ice/ice_vf_lib.c
@@ -657,6 +657,13 @@ struct ice_port_info *ice_vf_get_port_info(struct ice_vf *vf)
 	return vf->pf->hw.port_info;
 }
 
+/**
+ * ice_cfg_mac_antispoof - Configure MAC antispoof checking behavior
+ * @vsi: the VSI to configure
+ * @enable: whether to enable or disable the spoof checking
+ *
+ * Configure a VSI to enable (or disable) spoof checking behavior.
+ */
 static int ice_cfg_mac_antispoof(struct ice_vsi *vsi, bool enable)
 {
 	struct ice_vsi_ctx *ctx;
-- 
2.35.1.456.ga9c7032d4631

[Intel-wired-lan] [PATCH net-next 6/6] ice: remove period on argument description in ice_for_each_vf

[Jacob Keller]

The ice_for_each_vf macros have comments describing the implementation.
One of the arguments has a period on the end, which is not our typical
style. Remove the unnecessary period.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_vf_lib.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_vf_lib.h b/drivers/net/ethernet/intel/ice/ice_vf_lib.h
index 831b667dc5b2..1b4380d6d949 100644
--- a/drivers/net/ethernet/intel/ice/ice_vf_lib.h
+++ b/drivers/net/ethernet/intel/ice/ice_vf_lib.h
@@ -176,7 +176,7 @@ static inline u16 ice_vf_get_port_vlan_tpid(struct ice_vf *vf)
  * ice_for_each_vf - Iterate over each VF entry
  * @pf: pointer to the PF private structure
  * @bkt: bucket index used for iteration
- * @vf: pointer to the VF entry currently being processed in the loop.
+ * @vf: pointer to the VF entry currently being processed in the loop
  *
  * The bkt variable is an unsigned integer iterator used to traverse the VF
  * entries. It is *not* guaranteed to be the VF's vf_id. Do not assume it is.
@@ -192,7 +192,7 @@ static inline u16 ice_vf_get_port_vlan_tpid(struct ice_vf *vf)
  * ice_for_each_vf_rcu - Iterate over each VF entry protected by RCU
  * @pf: pointer to the PF private structure
  * @bkt: bucket index used for iteration
- * @vf: pointer to the VF entry currently being processed in the loop.
+ * @vf: pointer to the VF entry currently being processed in the loop
  *
  * The bkt variable is an unsigned integer iterator used to traverse the VF
  * entries. It is *not* guaranteed to be the VF's vf_id. Do not assume it is.
-- 
2.35.1.456.ga9c7032d4631

[Intel-wired-lan] [PATCH net-next 0/6] minor cleanups for ice virtualization

[Paul Menzel]

Dear Jacob,


Am 12.04.22 um 01:29 schrieb Jacob Keller:
> This series contains a few minor fix ups for virtualization files. Mainly
> this includes fixes for function header comments and minor wording changes.
> 
> The only larger change is a patch which adds NULL checks to ice_get_vf_vsi.
> This was done in order to prevent static analysis tools from complaining
> that a NULL value could be dereferenced. Flows where an error is not
> feasible have a WARN_ON check added. This change enhances the robustness of
> the driver by avoiding a panic in the event that a VF does not have a valid
> VSI.
> 
> Jacob Keller (6):
>    ice: add newline to dev_dbg in ice_vf_fdir_dump_info
>    ice: always check VF VSI pointer values
>    ice: remove return value comment for ice_reset_all_vfs
>    ice: fix wording in comment for ice_reset_vf
>    ice: add a function comment for ice_cfg_mac_antispoof
>    ice: remove period on argument description in ice_for_each_vf
> 
>   drivers/net/ethernet/intel/ice/ice_devlink.c  |  5 ++-
>   drivers/net/ethernet/intel/ice/ice_repr.c     |  7 ++-
>   drivers/net/ethernet/intel/ice/ice_sriov.c    | 32 ++++++++++++--
>   drivers/net/ethernet/intel/ice/ice_vf_lib.c   | 43 ++++++++++++++++---
>   drivers/net/ethernet/intel/ice/ice_vf_lib.h   |  4 +-
>   drivers/net/ethernet/intel/ice/ice_virtchnl.c |  5 +++
>   .../ethernet/intel/ice/ice_virtchnl_fdir.c    |  9 +++-
>   7 files changed, 91 insertions(+), 14 deletions(-)

Besides the nitpicks, the whole series looks good. Thank you very much 
for doing such cleanup work.

Reviewed-by: Paul Menzel <pmenzel@molgen.mpg.de>


Kind regards,

Paul