Re: [PATCH net-next 06/12] ice: Implement basic eswitch bridge setup

[Vlad Buslov <vladbu@nvidia.com>]

On Mon 26 Jun 2023 at 16:26, Michal Swiatkowski <michal.swiatkowski@linux.intel.com> wrote:
> On Tue, Jun 20, 2023 at 10:44:17AM -0700, Tony Nguyen wrote:
>> From: Wojciech Drewek <wojciech.drewek@intel.com>
>> 
>> With this patch, ice driver is able to track if the port
>> representors or uplink port were added to the linux bridge in
>> switchdev mode. Listen for NETDEV_CHANGEUPPER events in order to
>> detect this. ice_esw_br data structure reflects the linux bridge
>> and stores all the ports of the bridge (ice_esw_br_port) in
>> xarray, it's created when the first port is added to the bridge and
>> freed once the last port is removed. Note that only one bridge is
>> supported per eswitch.
>> 
>> Bridge port (ice_esw_br_port) can be either a VF port representor
>> port or uplink port (ice_esw_br_port_type). In both cases bridge port
>> holds a reference to the VSI, VF's VSI in case of the PR and uplink
>> VSI in case of the uplink. VSI's index is used as an index to the
>> xarray in which ports are stored.
>> 
>> Add a check which prevents configuring switchdev mode if uplink is
>> already added to any bridge. This is needed because we need to listen
>> for NETDEV_CHANGEUPPER events to record if the uplink was added to
>> the bridge. Netdevice notifier is registered after eswitch mode
>> is changed to switchdev.
>> 
>> Reviewed-by: Simon Horman <simon.horman@corigine.com>
>> Signed-off-by: Wojciech Drewek <wojciech.drewek@intel.com>
>> Tested-by: Sujai Buvaneswaran <sujai.buvaneswaran@intel.com>
>> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
>> ---
>>  drivers/net/ethernet/intel/ice/Makefile       |   2 +-
>>  drivers/net/ethernet/intel/ice/ice.h          |   4 +-
>>  drivers/net/ethernet/intel/ice/ice_eswitch.c  |  26 +-
>>  .../net/ethernet/intel/ice/ice_eswitch_br.c   | 384 ++++++++++++++++++
>>  .../net/ethernet/intel/ice/ice_eswitch_br.h   |  42 ++
>>  drivers/net/ethernet/intel/ice/ice_main.c     |   2 +-
>>  drivers/net/ethernet/intel/ice/ice_repr.c     |   2 +-
>>  drivers/net/ethernet/intel/ice/ice_repr.h     |   3 +-
>>  8 files changed, 456 insertions(+), 9 deletions(-)
>>  create mode 100644 drivers/net/ethernet/intel/ice/ice_eswitch_br.c
>>  create mode 100644 drivers/net/ethernet/intel/ice/ice_eswitch_br.h
>> +
>> +static int
>> +ice_eswitch_br_port_changeupper(struct notifier_block *nb, void *ptr)
>> +{
>> +	struct net_device *dev = netdev_notifier_info_to_dev(ptr);
>> +	struct netdev_notifier_changeupper_info *info = ptr;
>> +	struct ice_esw_br_offloads *br_offloads;
>> +	struct netlink_ext_ack *extack;
>> +	struct net_device *upper;
>> +
>> +	br_offloads = ice_nb_to_br_offloads(nb, netdev_nb);
>> +
>> +	if (!ice_eswitch_br_is_dev_valid(dev))
>> +		return 0;
>> +
>> +	upper = info->upper_dev;
>> +	if (!netif_is_bridge_master(upper))
>> +		return 0;
>> +
>> +	extack = netdev_notifier_info_to_extack(&info->info);
>> +
>> +	if (info->linking)
>> +		return ice_eswitch_br_port_link(br_offloads, dev,
>> +						upper->ifindex, extack);
>> +	else
>> +		return ice_eswitch_br_port_unlink(br_offloads, dev,
>> +						  upper->ifindex, extack);
>> +}
>> +
>> +static int
>> +ice_eswitch_br_port_event(struct notifier_block *nb,
>> +			  unsigned long event, void *ptr)
>> +{
>> +	int err = 0;
>> +
>> +	switch (event) {
>> +	case NETDEV_CHANGEUPPER:
>> +		err = ice_eswitch_br_port_changeupper(nb, ptr);
>> +		break;
>> +	}
>> +
>> +	return notifier_from_errno(err);
>> +}
> Hi Vlad,
>
> We found out that adding VF and corresponding port representor to the
> bridge cause loop in the bridge. Packets are looping through the bridge.
> I know that it isn't valid configuration, howevere, it can happen and
> after that the server is quite unstable.
>
> Does mellanox validate the port for this scenario? Or we should assume
> that user will add port wisely? I was looking at your code, but didn't
> find that. You are using NETDEV_PRECHANGEUPPER, do you think we should
> validate if user is trying to add VF when his PR is currently added?

Hmm, no, it is not something we validate. Also, I assume it will be
quite tricky to properly test for it, since user could try to add some
other netdevice connected to the VF (VLAN, tunneling dev, bonding, etc.)
which will probably lead to same result.