Re: [PATCH net-next v4 4/9] mlxsw: spectrum_switchdev: Handle SWITCHDEV_PORT_ATTR_GET/SET

[Ido Schimmel <idosch@mellanox.com>]

On Mon, Feb 11, 2019 at 11:09:56AM -0800, Florian Fainelli wrote:
> Following patches will change the way we communicate getting or setting
> a port's attribute and use a blocking notifier to perform those tasks.
> 
> Prepare mlxsw to support receiving notifier events targeting
> SWITCHDEV_PORT_ATTR_GET/SET and simply translate that into the existing
> mlxsw_sp_port_attr_{set,get} calls.
> 
> Acked-by: Jiri Pirko <jiri@mellanox.com>
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
>  .../mellanox/mlxsw/spectrum_switchdev.c       | 23 +++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c
> index 95e37de3e48f..88d4994309a7 100644
> --- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c
> +++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c
> @@ -3443,6 +3443,26 @@ mlxsw_sp_switchdev_handle_vxlan_obj_del(struct net_device *vxlan_dev,
>  	}
>  }
>  
> +static int
> +mlxsw_sp_switchdev_port_attr_event(unsigned long event, struct net_device *dev,
> +		struct switchdev_notifier_port_attr_info *port_attr_info)
> +{
> +	int err = -EOPNOTSUPP;
> +
> +	switch (event) {
> +	case SWITCHDEV_PORT_ATTR_SET:
> +		err = mlxsw_sp_port_attr_set(dev, port_attr_info->attr,
> +					     port_attr_info->trans);

It is not that simple. These functions expect 'dev' to be an mlxsw
netdev since the operation is propagated using the device chain. This is
not the case with notification chains. 'dev' can be any netdev in the
system and then this line in mlxsw_sp_port_attr_set() is not correct:

struct mlxsw_sp_port *mlxsw_sp_port = netdev_priv(dev);

You can check commit f30f0601eb93 ("switchdev: Add helpers to aid
traversal through lower devices") for reference.

> +		break;
> +	case SWITCHDEV_PORT_ATTR_GET:
> +		err = mlxsw_sp_port_attr_get(dev, port_attr_info->attr);
> +		break;
> +	}
> +
> +	port_attr_info->handled = true;

I believe this should only be set in case the driver actually handled
the notification. That's how it works for objects.

I suggest looking at the series merged in commit 06d212900ea9 ("Merge
branch 'switchdev-blocking-notifiers'").

> +	return notifier_from_errno(err);
> +}
> +
>  static int mlxsw_sp_switchdev_blocking_event(struct notifier_block *unused,
>  					     unsigned long event, void *ptr)
>  {
> @@ -3466,6 +3486,9 @@ static int mlxsw_sp_switchdev_blocking_event(struct notifier_block *unused,
>  							mlxsw_sp_port_dev_check,
>  							mlxsw_sp_port_obj_del);
>  		return notifier_from_errno(err);
> +	case SWITCHDEV_PORT_ATTR_SET: /* fall through */
> +	case SWITCHDEV_PORT_ATTR_GET:
> +		return mlxsw_sp_switchdev_port_attr_event(event, dev, ptr);
>  	}
>  
>  	return NOTIFY_DONE;
> -- 
> 2.17.1
>