Date: Mon, 22 May 2023 12:05:43 +0200
From: Paul Menzel <pmenzel@...gen.mpg.de>
To: Wojciech Drewek <wojciech.drewek@...el.com>
Cc: intel-wired-lan@...ts.osuosl.org, netdev@...r.kernel.org
Subject: Re: [Intel-wired-lan] [PATCH iwl-next v3 01/10] ice: Minor switchdev
 fixes

Dear Wojciech,


Thank you for your patches.

Am 22.05.23 um 11:05 schrieb Wojciech Drewek:
> Introduce a few fixes that are needed for bridge offload
> to work properly.
> 
> - Skip adv rule removal in ice_eswitch_disable_switchdev.
>    Advanced rules for ctrl VSI will be removed anyway when the
>    VSI will cleaned up, no need to do it explicitly.
> 
> - Don't allow to change promisc mode in switchdev mode.
>    When switchdev is configured, PF netdev is set to be a
>    default VSI. This is needed for the slow-path to work correctly.
>    All the unmatched packets will be directed to PF netdev.
> 
>    It is possible that this setting might be overwritten by
>    ndo_set_rx_mode. Prevent this by checking if switchdev is
>    enabled in ice_set_rx_mode.
> 
> - Disable vlan pruning for uplink VSI. In switchdev mode, uplink VSI
>    is configured to be default VSI which means it will receive all
>    unmatched packets. In order to receive vlan packets we need to
>    disable vlan pruning as well. This is done by dis_rx_filtering
>    vlan op.
> 
> - There is possibility that ice_eswitch_port_start_xmit might be
>    called while some resources are still not allocated which might
>    cause NULL pointer dereference. Fix this by checking if switchdev
>    configuration was finished.

If you enumerate/list changes in a commit message, it’s a good indicator 
to make one patch/commit for each item. ;-) Doing this also makes it 
easier to use a statement as the commit message summary, that means 
using a verb (in imperative mood) – Fix minor switchdev things – and 
making `git log --oneline`) more useful. Smaller commits are also easier 
to revert or to backport.

> Signed-off-by: Wojciech Drewek <wojciech.drewek@...el.com>
> ---
> v2: enclose bitops into separate set of braces, move
>      ice_is_switchdev_running check to ice_set_rx_mode
>      from ice_vsi_sync_fltr
> ---
>   drivers/net/ethernet/intel/ice/ice_eswitch.c | 14 +++++++++++++-
>   drivers/net/ethernet/intel/ice/ice_main.c    |  4 ++--
>   2 files changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_eswitch.c b/drivers/net/ethernet/intel/ice/ice_eswitch.c
> index ad0a007b7398..bfd003135fc8 100644
> --- a/drivers/net/ethernet/intel/ice/ice_eswitch.c
> +++ b/drivers/net/ethernet/intel/ice/ice_eswitch.c
> @@ -103,6 +103,10 @@ static int ice_eswitch_setup_env(struct ice_pf *pf)
>   		rule_added = true;
>   	}
>   
> +	vlan_ops = ice_get_compat_vsi_vlan_ops(uplink_vsi);
> +	if (vlan_ops->dis_rx_filtering(uplink_vsi))
> +		goto err_dis_rx;
> +
>   	if (ice_vsi_update_security(uplink_vsi, ice_vsi_ctx_set_allow_override))
>   		goto err_override_uplink;
>   
> @@ -114,6 +118,8 @@ static int ice_eswitch_setup_env(struct ice_pf *pf)
>   err_override_control:
>   	ice_vsi_update_security(uplink_vsi, ice_vsi_ctx_clear_allow_override);
>   err_override_uplink:
> +	vlan_ops->ena_rx_filtering(uplink_vsi);
> +err_dis_rx:
>   	if (rule_added)
>   		ice_clear_dflt_vsi(uplink_vsi);
>   err_def_rx:
> @@ -331,6 +337,9 @@ ice_eswitch_port_start_xmit(struct sk_buff *skb, struct net_device *netdev)
>   	np = netdev_priv(netdev);
>   	vsi = np->vsi;
>   
> +	if (!vsi || !ice_is_switchdev_running(vsi->back))
> +		return NETDEV_TX_BUSY;
> +
>   	if (ice_is_reset_in_progress(vsi->back->state) ||
>   	    test_bit(ICE_VF_DIS, vsi->back->state))
>   		return NETDEV_TX_BUSY;
> @@ -378,9 +387,13 @@ static void ice_eswitch_release_env(struct ice_pf *pf)
>   {
>   	struct ice_vsi *uplink_vsi = pf->switchdev.uplink_vsi;
>   	struct ice_vsi *ctrl_vsi = pf->switchdev.control_vsi;
> +	struct ice_vsi_vlan_ops *vlan_ops;
> +
> +	vlan_ops = ice_get_compat_vsi_vlan_ops(uplink_vsi);
>   
>   	ice_vsi_update_security(ctrl_vsi, ice_vsi_ctx_clear_allow_override);
>   	ice_vsi_update_security(uplink_vsi, ice_vsi_ctx_clear_allow_override);
> +	vlan_ops->ena_rx_filtering(uplink_vsi);
>   	ice_clear_dflt_vsi(uplink_vsi);
>   	ice_fltr_add_mac_and_broadcast(uplink_vsi,
>   				       uplink_vsi->port_info->mac.perm_addr,
> @@ -503,7 +516,6 @@ static void ice_eswitch_disable_switchdev(struct ice_pf *pf)
>   
>   	ice_eswitch_napi_disable(pf);
>   	ice_eswitch_release_env(pf);
> -	ice_rem_adv_rule_for_vsi(&pf->hw, ctrl_vsi->idx);
>   	ice_eswitch_release_reprs(pf, ctrl_vsi);
>   	ice_vsi_release(ctrl_vsi);
>   	ice_repr_rem_from_all_vfs(pf);
> diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
> index b0d1e6116eb9..80b2b4d39278 100644
> --- a/drivers/net/ethernet/intel/ice/ice_main.c
> +++ b/drivers/net/ethernet/intel/ice/ice_main.c
> @@ -385,7 +385,7 @@ static int ice_vsi_sync_fltr(struct ice_vsi *vsi)
>   	}
>   	err = 0;
>   	/* check for changes in promiscuous modes */
> -	if (changed_flags & IFF_ALLMULTI) {
> +	if ((changed_flags & IFF_ALLMULTI)) {
>   		if (vsi->current_netdev_flags & IFF_ALLMULTI) {
>   			err = ice_set_promisc(vsi, ICE_MCAST_PROMISC_BITS);
>   			if (err) {
> @@ -5767,7 +5767,7 @@ static void ice_set_rx_mode(struct net_device *netdev)
>   	struct ice_netdev_priv *np = netdev_priv(netdev);
>   	struct ice_vsi *vsi = np->vsi;
>   
> -	if (!vsi)
> +	if (!vsi || ice_is_switchdev_running(vsi->back))
>   		return;
>   
>   	/* Set the flags to synchronize filters

The diff itself looks good.

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


Kind regards,

Paul

Powered by blists - more mailing lists