Re: [PATCH net-next v3 0/8] net: Remove switchdev_ops
On Wed, Feb 27, 2019 at 11:44:24AM -0800, Florian Fainelli wrote: > Hi all, > > This patch series completes the removal of the switchdev_ops by > converting switchdev_port_attr_set() to use either the blocking > (process) or non-blocking (atomic) notifier since we typically need to > deal with both depending on where in the bridge code we get called from. > > This was tested with the forwarding selftests and DSA hardware. I ran some basic tests and nothing exploded :) Thanks, Florian! ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH net-next v3 8/8] net: Remove switchdev_ops
On Wed, Feb 27, 2019 at 11:44:32AM -0800, Florian Fainelli wrote: > Now that we have converted all possible callers to using a switchdev > notifier for attributes we do not have a need for implementing > switchdev_ops anymore, and this can be removed from all drivers the > net_device structure. > > Signed-off-by: Florian Fainelli Reviewed-by: Ido Schimmel ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH net-next v3 7/8] net: switchdev: Replace port attr set SDO with a notification
On Wed, Feb 27, 2019 at 11:44:31AM -0800, Florian Fainelli wrote:
> Drop switchdev_ops.switchdev_port_attr_set. Drop the uses of this field
> from all clients, which were migrated to use switchdev notification in
> the previous patches.
>
> Add a new function switchdev_port_attr_notify() that sends the switchdev
> notifications SWITCHDEV_PORT_ATTR_SET and calls the blocking (process)
> notifier chain.
>
> We have one odd case within net/bridge/br_switchdev.c with the
> SWITCHDEV_ATTR_ID_PORT_PRE_BRIDGE_FLAGS attribute identifier that
> requires executing from atomic context, we deal with that one
> specifically.
>
> Drop __switchdev_port_attr_set() and update switchdev_port_attr_set()
> likewise.
>
> Signed-off-by: Florian Fainelli
Reviewed-by: Ido Schimmel
One small nit that you can address in a follow-up:
> @@ -67,12 +67,18 @@ int br_switchdev_set_port_flag(struct net_bridge_port *p,
> .id = SWITCHDEV_ATTR_ID_PORT_PRE_BRIDGE_FLAGS,
> .u.brport_flags = mask,
> };
> + struct switchdev_notifier_port_attr_info info = {
> + .attr = ,
> + };
> int err;
>
> if (mask & ~BR_PORT_FLAGS_HW_OFFLOAD)
> return 0;
>
> - err = switchdev_port_attr_set(p->dev, );
> + /* We run from atomic context here */
> + err = call_switchdev_notifiers(SWITCHDEV_PORT_ATTR_SET, p->dev,
> +, NULL);
> + err = notifier_to_errno(err);
> if (err == -EOPNOTSUPP)
> return 0;
This check can be removed. The code below checks `err` and fails the
operation in case of error.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH net-next v2 8/8] net: Remove switchdev_ops
On Tue, Feb 26, 2019 at 05:14:27PM -0800, Florian Fainelli wrote:
> diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
> b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
> index b00f6f74f91a..995426ea9a43 100644
> --- a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
> +++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
> @@ -3660,7 +3660,6 @@ static int mlxsw_sp_port_create(struct mlxsw_sp
> *mlxsw_sp, u8 local_port,
> }
> mlxsw_sp_port->default_vlan = mlxsw_sp_port_vlan;
>
> - mlxsw_sp_port_switchdev_init(mlxsw_sp_port);
> mlxsw_sp->ports[local_port] = mlxsw_sp_port;
> err = register_netdev(dev);
> if (err) {
> @@ -3677,7 +3676,6 @@ static int mlxsw_sp_port_create(struct mlxsw_sp
> *mlxsw_sp, u8 local_port,
>
> err_register_netdev:
> mlxsw_sp->ports[local_port] = NULL;
> - mlxsw_sp_port_switchdev_fini(mlxsw_sp_port);
> mlxsw_sp_port_vlan_destroy(mlxsw_sp_port_vlan);
> err_port_vlan_create:
> err_port_pvid_set:
> @@ -3720,7 +3718,6 @@ static void mlxsw_sp_port_remove(struct mlxsw_sp
> *mlxsw_sp, u8 local_port)
> mlxsw_core_port_clear(mlxsw_sp->core, local_port, mlxsw_sp);
> unregister_netdev(mlxsw_sp_port->dev); /* This calls ndo_stop */
> mlxsw_sp->ports[local_port] = NULL;
> - mlxsw_sp_port_switchdev_fini(mlxsw_sp_port);
> mlxsw_sp_port_vlan_flush(mlxsw_sp_port, true);
> mlxsw_sp_port_nve_fini(mlxsw_sp_port);
> mlxsw_sp_tc_qdisc_fini(mlxsw_sp_port);
> @@ -4441,12 +4438,6 @@ static int mlxsw_sp_init(struct mlxsw_core *mlxsw_core,
> goto err_span_init;
> }
>
> - err = mlxsw_sp_switchdev_init(mlxsw_sp);
I missed that and got a trace as soon as I tried to enslave a port. You
should only remove mlxsw_sp_port_switchdev_init() and not
mlxsw_sp_switchdev_init()
> - if (err) {
> - dev_err(mlxsw_sp->bus_info->dev, "Failed to initialize
> switchdev\n");
> - goto err_switchdev_init;
> - }
> -
> err = mlxsw_sp_counter_pool_init(mlxsw_sp);
> if (err) {
> dev_err(mlxsw_sp->bus_info->dev, "Failed to init counter
> pool\n");
> @@ -4517,8 +4508,6 @@ static int mlxsw_sp_init(struct mlxsw_core *mlxsw_core,
> err_afa_init:
> mlxsw_sp_counter_pool_fini(mlxsw_sp);
> err_counter_pool_init:
> - mlxsw_sp_switchdev_fini(mlxsw_sp);
> -err_switchdev_init:
> mlxsw_sp_span_fini(mlxsw_sp);
> err_span_init:
> mlxsw_sp_lag_fini(mlxsw_sp);
> @@ -4585,7 +4574,6 @@ static void mlxsw_sp_fini(struct mlxsw_core *mlxsw_core)
> mlxsw_sp_nve_fini(mlxsw_sp);
> mlxsw_sp_afa_fini(mlxsw_sp);
> mlxsw_sp_counter_pool_fini(mlxsw_sp);
> - mlxsw_sp_switchdev_fini(mlxsw_sp);
> mlxsw_sp_span_fini(mlxsw_sp);
> mlxsw_sp_lag_fini(mlxsw_sp);
> mlxsw_sp_buffers_fini(mlxsw_sp);
> diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum.h
> b/drivers/net/ethernet/mellanox/mlxsw/spectrum.h
> index a61c1130d9e3..da6278b0caa4 100644
> --- a/drivers/net/ethernet/mellanox/mlxsw/spectrum.h
> +++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum.h
> @@ -407,8 +407,6 @@ extern const struct mlxsw_sp_sb_vals mlxsw_sp2_sb_vals;
> /* spectrum_switchdev.c */
> int mlxsw_sp_switchdev_init(struct mlxsw_sp *mlxsw_sp);
> void mlxsw_sp_switchdev_fini(struct mlxsw_sp *mlxsw_sp);
> -void mlxsw_sp_port_switchdev_init(struct mlxsw_sp_port *mlxsw_sp_port);
> -void mlxsw_sp_port_switchdev_fini(struct mlxsw_sp_port *mlxsw_sp_port);
> int mlxsw_sp_rif_fdb_op(struct mlxsw_sp *mlxsw_sp, const char *mac, u16 fid,
> bool adding);
> void
> diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c
> b/drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c
> index c1aedfea3a31..f6ce386c3036 100644
> --- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c
> +++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c
> @@ -1938,10 +1938,6 @@ static struct mlxsw_sp_port
> *mlxsw_sp_lag_rep_port(struct mlxsw_sp *mlxsw_sp,
> return NULL;
> }
>
> -static const struct switchdev_ops mlxsw_sp_port_switchdev_ops = {
> - .switchdev_port_attr_set= mlxsw_sp_port_attr_set,
> -};
> -
> static int
> mlxsw_sp_bridge_8021q_port_join(struct mlxsw_sp_bridge_device *bridge_device,
> struct mlxsw_sp_bridge_port *bridge_port,
> @@ -3545,11 +3541,3 @@ void mlxsw_sp_switchdev_fini(struct mlxsw_sp *mlxsw_sp)
> kfree(mlxsw_sp->bridge);
> }
>
> -void mlxsw_sp_port_switchdev_init(struct mlxsw_sp_port *mlxsw_sp_port)
> -{
> - mlxsw_sp_port->dev->switchdev_ops = _sp_port_switchdev_ops;
> -}
> -
> -void mlxsw_sp_port_switchdev_fini(struct mlxsw_sp_port *mlxsw_sp_port)
> -{
> -}
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH net-next v2 1/8] switchdev: Add SWITCHDEV_PORT_ATTR_SET
On Tue, Feb 26, 2019 at 05:14:20PM -0800, Florian Fainelli wrote: > In preparation for allowing switchdev enabled drivers to veto specific > attribute settings from within the context of the caller, introduce a > new switchdev notifier type for port attributes. > > Suggested-by: Ido Schimmel > Signed-off-by: Florian Fainelli Reviewed-by: Ido Schimmel ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH net-next 7/8] net: switchdev: Replace port attr set SDO with a notification
On Mon, Feb 25, 2019 at 11:47:12AM -0800, Florian Fainelli wrote: > On 2/25/19 1:49 AM, Ido Schimmel wrote: > > On Sun, Feb 24, 2019 at 08:47:27AM -0800, Florian Fainelli wrote: > >> Le 2/23/19 à 2:32 AM, Ido Schimmel a écrit : > >>> On Fri, Feb 22, 2019 at 03:59:25PM -0800, Florian Fainelli wrote: > >>>> -if (attr->flags & SWITCHDEV_F_NO_RECURSE) > >>>> +if (attr & SWITCHDEV_F_DEFER) > >>>> +rc = call_switchdev_blocking_notifiers(nt, dev, > >>>> + _info.info, > >>>> NULL); > >>>> +else > >>>> +rc = call_switchdev_notifiers(nt, dev, _info.info, > >>>> NULL); > >>> > >>> I don't believe this is needed. You're calling this function from > >>> switchdev_port_attr_set_now() which is always called from process > >>> context. switchdev_port_attr_set() takes care of that. Similar to > >>> switchdev_port_obj_add(). > >> > >> Except for net/bridge/br_switchdev.c when we check the bridge port's > >> flags support with PRE_BRIDGE_FLAGS. In that case we are executing from > >> the caller (atomic) context and we can't defer otherwise that trumps the > >> whole idea of being able to do a quick check and return that to the > >> caller that we cannot support specific flags. How would you recommend > >> approaching that? > > > > In this case you can invoke call_switchdev_notifiers() directly from > > br_switchdev_set_port_flag(). Eventually switchdev_port_attr_set() will > > be gone and bridge code will invoke the notifiers directly. > > That can be done, but it still requires the target driver (mlxsw, > ocelot, dsa, etc.) to support attribute notification from blocking and > non-blocking context. Are you fine with that? Yes. Sorry for the latency. I was away yesterday. Reviewed your v2 and only found one problem. Will run some tests now. Thanks! ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH net-next v2 7/8] net: switchdev: Replace port attr set SDO with a notification
On Tue, Feb 26, 2019 at 05:14:26PM -0800, Florian Fainelli wrote:
> diff --git a/net/bridge/br_switchdev.c b/net/bridge/br_switchdev.c
> index af57c4a2b78a..b7988d49d708 100644
> --- a/net/bridge/br_switchdev.c
> +++ b/net/bridge/br_switchdev.c
> @@ -67,12 +67,17 @@ int br_switchdev_set_port_flag(struct net_bridge_port *p,
> .id = SWITCHDEV_ATTR_ID_PORT_PRE_BRIDGE_FLAGS,
> .u.brport_flags = mask,
> };
> + struct switchdev_notifier_port_attr_info info = {
> + .attr = ,
> + };
> int err;
>
> if (mask & ~BR_PORT_FLAGS_HW_OFFLOAD)
> return 0;
>
> - err = switchdev_port_attr_set(p->dev, );
> + /* We run from atomic context here */
> + err = call_switchdev_notifiers(SWITCHDEV_PORT_ATTR_SET, p->dev,
> +, NULL);
> if (err == -EOPNOTSUPP)
Florian, this needs to use notifier_to_errno() and check for any error.
With the ops, `-EOPNOTSUPP` was returned for devices that did not
implement `switchdev_ops`. Now they will simply not listen / reply to
the notification.
> return 0;
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH net-next v2 8/8] net: Remove switchdev_ops
On Tue, Feb 26, 2019 at 05:14:27PM -0800, Florian Fainelli wrote: > Now that we have converted all possible callers to using a switchdev > notifier for attributes we do not have a need for implementing > switchdev_ops anymore, and this can be removed from all drivers the > net_device structure. > > Signed-off-by: Florian Fainelli Reviewed-by: Ido Schimmel ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH net-next v2 4/8] mlxsw: spectrum_switchdev: Handle SWITCHDEV_PORT_ATTR_SET
On Tue, Feb 26, 2019 at 05:14:23PM -0800, Florian Fainelli wrote: > Following patches will change the way we communicate setting a port's > attribute and use a notifier to perform those tasks. > > Prepare mlxsw to support receiving notifier events targeting > SWITCHDEV_PORT_ATTR_SET and utilize the switchdev_handle_port_attr_set() > to handle stacking of devices. > > Signed-off-by: Florian Fainelli Reviewed-by: Ido Schimmel ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH net-next 7/8] net: switchdev: Replace port attr set SDO with a notification
On Sun, Feb 24, 2019 at 08:47:27AM -0800, Florian Fainelli wrote: > Le 2/23/19 à 2:32 AM, Ido Schimmel a écrit : > > On Fri, Feb 22, 2019 at 03:59:25PM -0800, Florian Fainelli wrote: > >> - if (attr->flags & SWITCHDEV_F_NO_RECURSE) > >> + if (attr & SWITCHDEV_F_DEFER) > >> + rc = call_switchdev_blocking_notifiers(nt, dev, > >> + _info.info, NULL); > >> + else > >> + rc = call_switchdev_notifiers(nt, dev, _info.info, NULL); > > > > I don't believe this is needed. You're calling this function from > > switchdev_port_attr_set_now() which is always called from process > > context. switchdev_port_attr_set() takes care of that. Similar to > > switchdev_port_obj_add(). > > Except for net/bridge/br_switchdev.c when we check the bridge port's > flags support with PRE_BRIDGE_FLAGS. In that case we are executing from > the caller (atomic) context and we can't defer otherwise that trumps the > whole idea of being able to do a quick check and return that to the > caller that we cannot support specific flags. How would you recommend > approaching that? In this case you can invoke call_switchdev_notifiers() directly from br_switchdev_set_port_flag(). Eventually switchdev_port_attr_set() will be gone and bridge code will invoke the notifiers directly. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH net-next 4/8] mlxsw: spectrum_switchdev: Handle SWITCHDEV_PORT_ATTR_SET
On Fri, Feb 22, 2019 at 03:59:22PM -0800, Florian Fainelli wrote:
> Following patches will change the way we communicate setting a port's
> attribute and use a notifier to perform those tasks.
>
> Prepare mlxsw to support receiving notifier events targeting
> SWITCHDEV_PORT_ATTR_SET and utilize the switchdev_handle_port_attr_set()
> to handle stacking of devices.
>
> Signed-off-by: Florian Fainelli
> ---
> .../net/ethernet/mellanox/mlxsw/spectrum_switchdev.c | 12
> 1 file changed, 12 insertions(+)
>
> diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c
> b/drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c
> index 766f5b5f1cf5..c1aedfea3a31 100644
> --- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c
> +++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c
> @@ -3123,6 +3123,13 @@ static int mlxsw_sp_switchdev_event(struct
> notifier_block *unused,
> struct net_device *br_dev;
> int err;
>
> + if (event == SWITCHDEV_PORT_ATTR_SET) {
> + err = switchdev_handle_port_attr_set(dev, ptr,
> + mlxsw_sp_port_dev_check,
> + mlxsw_sp_port_attr_set);
> + return notifier_from_errno(err);
I don't think this code is ever executed. And if it was executed, we
would have problems because switchdev_handle_port_attr_set() might
block.
> + }
> +
> /* Tunnel devices are not our uppers, so check their master instead */
> br_dev = netdev_master_upper_dev_get_rcu(dev);
> if (!br_dev)
> @@ -3446,6 +3453,11 @@ 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:
> + err = switchdev_handle_port_attr_set(dev, ptr,
> + mlxsw_sp_port_dev_check,
> + mlxsw_sp_port_attr_set);
> + return notifier_from_errno(err);
> }
>
> return NOTIFY_DONE;
> --
> 2.17.1
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH net-next 1/8] switchdev: Add SWITCHDEV_PORT_ATTR_SET
On Fri, Feb 22, 2019 at 03:59:19PM -0800, Florian Fainelli wrote:
> In preparation for allowing switchdev enabled drivers to veto specific
> attribute settings from within the context of the caller, introduce a
> new switchdev notifier type for port attributes.
>
> Suggested-by: Ido Schimmel
> Signed-off-by: Florian Fainelli
> ---
> include/net/switchdev.h | 27 +
> net/switchdev/switchdev.c | 51 +++
> 2 files changed, 78 insertions(+)
>
> diff --git a/include/net/switchdev.h b/include/net/switchdev.h
> index 45310ddf2d7e..ca56b7487540 100644
> --- a/include/net/switchdev.h
> +++ b/include/net/switchdev.h
> @@ -136,6 +136,7 @@ enum switchdev_notifier_type {
>
> SWITCHDEV_PORT_OBJ_ADD, /* Blocking. */
> SWITCHDEV_PORT_OBJ_DEL, /* Blocking. */
> + SWITCHDEV_PORT_ATTR_SET, /* May be blocking . */
See my comment on 7/8. IIUC, this is always blocking, so comment needs to
be changed.
>
> SWITCHDEV_VXLAN_FDB_ADD_TO_BRIDGE,
> SWITCHDEV_VXLAN_FDB_DEL_TO_BRIDGE,
> @@ -164,6 +165,13 @@ struct switchdev_notifier_port_obj_info {
> bool handled;
> };
>
> +struct switchdev_notifier_port_attr_info {
> + struct switchdev_notifier_info info; /* must be first */
> + const struct switchdev_attr *attr;
> + struct switchdev_trans *trans;
> + bool handled;
> +};
> +
> static inline struct net_device *
> switchdev_notifier_info_to_dev(const struct switchdev_notifier_info *info)
> {
> @@ -216,7 +224,15 @@ int switchdev_handle_port_obj_del(struct net_device *dev,
> int (*del_cb)(struct net_device *dev,
> const struct switchdev_obj *obj));
>
> +int switchdev_handle_port_attr_set(struct net_device *dev,
> + struct switchdev_notifier_port_attr_info
> *port_attr_info,
> + bool (*check_cb)(const struct net_device *dev),
> + int (*set_cb)(struct net_device *dev,
> + const struct switchdev_attr *attr,
> + struct switchdev_trans *trans));
> +
> #define SWITCHDEV_SET_OPS(netdev, ops) ((netdev)->switchdev_ops = (ops))
> +
> #else
>
> static inline void switchdev_deferred_process(void)
> @@ -303,6 +319,17 @@ switchdev_handle_port_obj_del(struct net_device *dev,
> return 0;
> }
>
> +static inline int
> +switchdev_handle_port_attr_set(struct net_device *dev,
> + struct switchdev_notifier_port_attr_info
> *port_attr_info,
> + bool (*check_cb)(const struct net_device *dev),
> + int (*set_cb)(struct net_device *dev,
> + const struct switchdev_attr *attr,
> + struct switchdev_trans *trans))
> +{
> + return 0;
> +}
> +
> #define SWITCHDEV_SET_OPS(netdev, ops) do {} while (0)
>
> #endif
> diff --git a/net/switchdev/switchdev.c b/net/switchdev/switchdev.c
> index 7e1357db33d7..94400f5b8e07 100644
> --- a/net/switchdev/switchdev.c
> +++ b/net/switchdev/switchdev.c
> @@ -697,3 +697,54 @@ int switchdev_handle_port_obj_del(struct net_device *dev,
> return err;
> }
> EXPORT_SYMBOL_GPL(switchdev_handle_port_obj_del);
> +
> +static int __switchdev_handle_port_attr_set(struct net_device *dev,
> + struct switchdev_notifier_port_attr_info
> *port_attr_info,
> + bool (*check_cb)(const struct net_device *dev),
> + int (*set_cb)(struct net_device *dev,
> + const struct switchdev_attr *attr,
> + struct switchdev_trans *trans))
> +{
> + struct net_device *lower_dev;
> + struct list_head *iter;
> + int err = -EOPNOTSUPP;
> +
> + if (check_cb(dev)) {
> + port_attr_info->handled = true;
> + return set_cb(dev, port_attr_info->attr,
> + port_attr_info->trans);
> + }
> +
> + /* Switch ports might be stacked under e.g. a LAG. Ignore the
> + * unsupported devices, another driver might be able to handle them. But
> + * propagate to the callers any hard errors.
> + *
> + * If the driver does its own bookkeeping of stacked ports, it's not
> + * necessary to go through this helper.
> + */
> + netdev_for_each_lower_dev(dev, lower_dev, iter) {
> + err = __switchdev_handle_port_attr_set(lower_dev,
> port_attr_info,
> +check_cb, set_cb);
> + if (err && err !=
Re: [PATCH net-next 7/8] net: switchdev: Replace port attr set SDO with a notification
On Fri, Feb 22, 2019 at 03:59:25PM -0800, Florian Fainelli wrote:
> Drop switchdev_ops.switchdev_port_attr_set. Drop the uses of this field
> from all clients, which were migrated to use switchdev notification in
> the previous patches.
>
> Add a new function switchdev_port_attr_notify() that sends the switchdev
> notifications SWITCHDEV_PORT_ATTR_SET and takes care, depending on
> SWITCHDEV_F_DEFER to call the blocking (process) or non-blocking
> (atomic) notifier chain accordingly.
>
> Drop __switchdev_port_attr_set() and update switchdev_port_attr_set()
> likewise.
>
> Signed-off-by: Florian Fainelli
> ---
> net/switchdev/switchdev.c | 96 +++
> 1 file changed, 26 insertions(+), 70 deletions(-)
>
> diff --git a/net/switchdev/switchdev.c b/net/switchdev/switchdev.c
> index 94400f5b8e07..a1f16836ef89 100644
> --- a/net/switchdev/switchdev.c
> +++ b/net/switchdev/switchdev.c
> @@ -174,81 +174,35 @@ static int switchdev_deferred_enqueue(struct net_device
> *dev,
> return 0;
> }
>
> -/**
> - * switchdev_port_attr_get - Get port attribute
Hmm, why do you remove it here? Can't you remove it in a separate patch?
I thought we already got rid of it :p
> - *
> - * @dev: port device
> - * @attr: attribute to get
> - */
> -int switchdev_port_attr_get(struct net_device *dev, struct switchdev_attr
> *attr)
> +static int switchdev_port_attr_notify(enum switchdev_notifier_type nt,
> + struct net_device *dev,
> + const struct switchdev_attr *attr,
> + struct switchdev_trans *trans)
> {
> - const struct switchdev_ops *ops = dev->switchdev_ops;
> - struct net_device *lower_dev;
> - struct list_head *iter;
> - struct switchdev_attr first = {
> - .id = SWITCHDEV_ATTR_ID_UNDEFINED
> - };
> - int err = -EOPNOTSUPP;
> + int err;
> + int rc;
>
> - if (ops && ops->switchdev_port_attr_get)
> - return ops->switchdev_port_attr_get(dev, attr);
> + struct switchdev_notifier_port_attr_info attr_info = {
> + .attr = attr,
> + .trans = trans,
> + .handled = false,
> + };
>
> - if (attr->flags & SWITCHDEV_F_NO_RECURSE)
> + if (attr & SWITCHDEV_F_DEFER)
> + rc = call_switchdev_blocking_notifiers(nt, dev,
> +_info.info, NULL);
> + else
> + rc = call_switchdev_notifiers(nt, dev, _info.info, NULL);
I don't believe this is needed. You're calling this function from
switchdev_port_attr_set_now() which is always called from process
context. switchdev_port_attr_set() takes care of that. Similar to
switchdev_port_obj_add().
The event `SWITCHDEV_PORT_ATTR_SET` is therefore always blocking and
drivers only need to take care of it from their blocking notifier.
> + err = notifier_to_errno(rc);
> + if (err) {
> + WARN_ON(!attr_info.handled);
> return err;
> -
> - /* Switch device port(s) may be stacked under
> - * bond/team/vlan dev, so recurse down to get attr on
> - * each port. Return -ENODATA if attr values don't
> - * compare across ports.
> - */
> -
> - netdev_for_each_lower_dev(dev, lower_dev, iter) {
> - err = switchdev_port_attr_get(lower_dev, attr);
> - if (err)
> - break;
> - if (first.id == SWITCHDEV_ATTR_ID_UNDEFINED)
> - first = *attr;
> - else if (memcmp(, attr, sizeof(*attr)))
> - return -ENODATA;
> }
>
> - return err;
> -}
> -EXPORT_SYMBOL_GPL(switchdev_port_attr_get);
> -
> -static int __switchdev_port_attr_set(struct net_device *dev,
> - const struct switchdev_attr *attr,
> - struct switchdev_trans *trans)
> -{
> - const struct switchdev_ops *ops = dev->switchdev_ops;
> - struct net_device *lower_dev;
> - struct list_head *iter;
> - int err = -EOPNOTSUPP;
> -
> - if (ops && ops->switchdev_port_attr_set) {
> - err = ops->switchdev_port_attr_set(dev, attr, trans);
> - goto done;
> - }
> -
> - if (attr->flags & SWITCHDEV_F_NO_RECURSE)
> - goto done;
> -
> - /* Switch device port(s) may be stacked under
> - * bond/team/vlan dev, so recurse down to set attr on
> - * each port.
> - */
> -
> - netdev_for_each_lower_dev(dev, lower_dev, iter) {
> - err = __switchdev_port_attr_set(lower_dev, attr, trans);
> - if (err)
> - break;
> - }
> -
> -done:
> - if (err == -EOPNOTSUPP && attr->flags & SWITCHDEV_F_SKIP_EOPNOTSUPP)
> - err = 0;
> + if (!attr_info.handled)
> + return -EOPNOTSUPP;
>
> - return err;
> + return 0;
> }
>
> static int
Re: [PATCH net-next v3 2/8] mlxsw: spectrum: Handle PORT_PRE_BRIDGE_FLAGS
On Wed, Feb 20, 2019 at 04:58:20PM -0800, Florian Fainelli wrote: > In preparation for getting rid of switchdev_port_attr_get(), have mlxsw > check for the bridge flags being set through switchdev_port_attr_set() > when the SWITCHDEV_ATTR_ID_PORT_PRE_BRIDGE_FLAGS attribute identifier is > used. > > Signed-off-by: Florian Fainelli Reviewed-by: Ido Schimmel ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH net-next v3 6/8] net: bridge: Stop calling switchdev_port_attr_get()
On Wed, Feb 20, 2019 at 04:58:24PM -0800, Florian Fainelli wrote: > Now that all switchdev drivers have been converted to check the > SWITCHDEV_ATTR_ID_PORT_PRE_BRIDGE_FLAGS flags and report flags that they > do not support accordingly, we can migrate the bridge code to try to set > that attribute first, check the results and then do the actual setting. > > Signed-off-by: Florian Fainelli Reviewed-by: Ido Schimmel ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH net-next v3 8/8] net: Get rid of switchdev_port_attr_get()
On Wed, Feb 20, 2019 at 04:58:26PM -0800, Florian Fainelli wrote: > With the bridge no longer calling switchdev_port_attr_get() to obtain > the supported bridge port flags from a driver but instead trying to set > the bridge port flags directly and relying on driver to reject > unsupported configurations, we can effectively get rid of > switchdev_port_attr_get() entirely since this was the only place where > it was called. > > Signed-off-by: Florian Fainelli Reviewed-by: Ido Schimmel ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH net-next v3 1/8] net: switchdev: Add PORT_PRE_BRIDGE_FLAGS
On Wed, Feb 20, 2019 at 04:58:19PM -0800, Florian Fainelli wrote: > In preparation for removing switchdev_port_attr_get(), introduce > PORT_PRE_BRIDGE_FLAGS which will be called through > switchdev_port_attr_set(), in the caller's context (possibly atomic) and > which must be checked by the switchdev driver in order to return whether > the operation is supported or not. > > This is entirely analoguous to how the BRIDGE_FLAGS_SUPPORT works, > except it goes through a set() instead of get(). > > Signed-off-by: Florian Fainelli Reviewed-by: Ido Schimmel ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH net-next 3/9] mlxsw: spectrum: Check bridge flags during prepare phase
On Wed, Feb 13, 2019 at 02:06:32PM -0800, Florian Fainelli wrote:
> In preparation for getting rid of switchdev_port_attr_get(), have mlxsw
> check for the bridge flags being set through switchdev_port_attr_set()
> when the SWITCHDEV_ATTR_ID_PORT_PRE_BRIDGE_FLAGS attribute identifier is
> used.
>
> Signed-off-by: Florian Fainelli
> ---
> .../ethernet/mellanox/mlxsw/spectrum_switchdev.c | 14 +++---
> 1 file changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c
> b/drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c
> index 1f492b7dbea8..7616eab50035 100644
> --- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c
> +++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c
> @@ -598,13 +598,17 @@ mlxsw_sp_bridge_port_learning_set(struct mlxsw_sp_port
> *mlxsw_sp_port,
> static int mlxsw_sp_port_attr_br_flags_set(struct mlxsw_sp_port
> *mlxsw_sp_port,
> struct switchdev_trans *trans,
> struct net_device *orig_dev,
> -unsigned long brport_flags)
> +unsigned long brport_flags,
> +bool pre_set)
> {
> struct mlxsw_sp_bridge_port *bridge_port;
> int err;
>
> - if (switchdev_trans_ph_prepare(trans))
> + if (switchdev_trans_ph_prepare(trans) && pre_set) {
> + if (brport_flags & ~(BR_LEARNING | BR_FLOOD | BR_MCAST_FLOOD))
> + return -EOPNOTSUPP;
> return 0;
> + }
When we get SWITCHDEV_ATTR_ID_PORT_PRE_BRIDGE_FLAGS we only want to
perform a check. With this code in case it's not prepare phase, then we
continue to set the flags. Better do:
if (pre_set) {
if (switchdev_trans_ph_commit(trans))
return 0;
// perform check here
}
>
> bridge_port = mlxsw_sp_bridge_port_find(mlxsw_sp_port->mlxsw_sp->bridge,
> orig_dev);
> @@ -833,6 +837,7 @@ static int mlxsw_sp_port_attr_set(struct net_device *dev,
> struct switchdev_trans *trans)
> {
> struct mlxsw_sp_port *mlxsw_sp_port = netdev_priv(dev);
> + bool pre_set = false;
> int err;
>
> switch (attr->id) {
> @@ -841,10 +846,13 @@ static int mlxsw_sp_port_attr_set(struct net_device
> *dev,
> attr->orig_dev,
> attr->u.stp_state);
> break;
> + case SWITCHDEV_ATTR_ID_PORT_PRE_BRIDGE_FLAGS:
> + pre_set = true; /* fall through */
> case SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS:
> err = mlxsw_sp_port_attr_br_flags_set(mlxsw_sp_port, trans,
> attr->orig_dev,
> - attr->u.brport_flags);
> + attr->u.brport_flags,
> + pre_set);
> break;
> case SWITCHDEV_ATTR_ID_BRIDGE_AGEING_TIME:
> err = mlxsw_sp_port_attr_br_ageing_set(mlxsw_sp_port, trans,
> --
> 2.17.1
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH net-next 7/9] net: bridge: Stop calling switchdev_port_attr_get()
On Thu, Feb 14, 2019 at 01:20:02PM +0200, Ido Schimmel wrote:
> On Wed, Feb 13, 2019 at 02:06:36PM -0800, Florian Fainelli wrote:
> > Now that all switchdev drivers have been converted to checking the
> > bridge port flags during the prepare phase of the
> > switchdev_port_attr_set() when the process
> > SWITCHDEV_ATTR_ID_PORT_PRE_BRIDGE_FLAGS, we can avoid calling
> > switchdev_port_attr_get() with
> > SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS_SUPPORT.
> >
> > Signed-off-by: Florian Fainelli
> > ---
> > net/bridge/br_switchdev.c | 16 +++-
> > 1 file changed, 7 insertions(+), 9 deletions(-)
> >
> > diff --git a/net/bridge/br_switchdev.c b/net/bridge/br_switchdev.c
> > index db9e8ab96d48..8f88f8a1a7fa 100644
> > --- a/net/bridge/br_switchdev.c
> > +++ b/net/bridge/br_switchdev.c
> > @@ -64,29 +64,27 @@ int br_switchdev_set_port_flag(struct net_bridge_port
> > *p,
> > {
> > struct switchdev_attr attr = {
> > .orig_dev = p->dev,
> > - .id = SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS_SUPPORT,
> > + .id = SWITCHDEV_ATTR_ID_PORT_PRE_BRIDGE_FLAGS,
> > + .u.brport_flags = flags,
> > };
> > int err;
> >
> > if (mask & ~BR_PORT_FLAGS_HW_OFFLOAD)
> > return 0;
> >
> > - err = switchdev_port_attr_get(p->dev, );
> > - if (err == -EOPNOTSUPP)
> > - return 0;
> > - if (err)
> > + err = switchdev_port_attr_set(p->dev, );
> > + if (err && err != -EOPNOTSUPP)
> > return err;
> >
> > - /* Check if specific bridge flag attribute offload is supported */
> > - if (!(attr.u.brport_flags_support & mask)) {
> > + if (err == -EOPNOTSUPP) {
> > br_warn(p->br, "bridge flag offload is not supported %u(%s)\n",
> > (unsigned int)p->port_no, p->dev->name);
> > - return -EOPNOTSUPP;
> > + return err;
> > }
>
> I see that you return -EOPNOTSUPP from drivers in case of unsupported
> flags. I believe this is problematic (I'll test soon). The same return
> code is used by:
>
> 1. Switch drivers to indicate unsupported flags
> 2. switchdev code to indicate unsupported netdev (no switchdev ops)
>
> I guess that with this patch any attempt to set bridge port flags on
> veth/dummy device will result in an error.
Yea, that's the case. You can test with
tools/testing/selftests/net/forwarding/bridge_vlan_aware.sh and other
bridge-related tests we have there.
Another problem is that during PORT_PRE_BRIDGE_FLAGS you pass 'flags'
and not 'mask'. This breaks mlxsw (and probably others as well) given
BR_BCAST_FLOOD is set by default.
>
> >
> > attr.id = SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS;
> > attr.flags = SWITCHDEV_F_DEFER;
> > - attr.u.brport_flags = flags;
> > +
> > err = switchdev_port_attr_set(p->dev, );
> > if (err) {
> > br_warn(p->br, "error setting offload flag on port %u(%s)\n",
> > --
> > 2.17.1
> >
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH net-next 9/9] net: Get rid of switchdev_port_attr_get()
On Wed, Feb 13, 2019 at 02:06:38PM -0800, Florian Fainelli wrote: > With the bridge no longer calling switchdev_port_attr_get() to obtain > the supported bridge port flags from a driver but instead trying to set > the bridge port flags directly and relying on driver to reject > unsupported configurations, we can effectively get rid of > switchdev_port_attr_get() entirely since this was the only place where > it was called. > > Signed-off-by: Florian Fainelli > --- > Documentation/networking/switchdev.txt | 5 ++--- > drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c | 7 --- > drivers/net/ethernet/rocker/rocker_main.c| 7 --- > drivers/staging/fsl-dpaa2/ethsw/ethsw.c | 7 --- > include/net/switchdev.h | 8 > net/dsa/slave.c | 7 --- > 6 files changed, 2 insertions(+), 39 deletions(-) > > diff --git a/Documentation/networking/switchdev.txt > b/Documentation/networking/switchdev.txt > index ea90243340a9..327afe754230 100644 > --- a/Documentation/networking/switchdev.txt > +++ b/Documentation/networking/switchdev.txt > @@ -233,9 +233,8 @@ the bridge's FDB. It's possible, but not optimal, to > enable learning on the > device port and on the bridge port, and disable learning_sync. > > To support learning and learning_sync port attributes, the driver implements > -switchdev op switchdev_port_attr_get/set for > -SWITCHDEV_ATTR_PORT_ID_BRIDGE_FLAGS. The driver should initialize the > attributes > -to the hardware defaults. > +switchdev op switchdev_port_attr_set for SWITCHDEV_ATTR_PORT_ID_BRIDGE_FLAGS. > +The driver should initialize the attributes to the hardware defaults. Last sentence is not relevant anymore. learning_sync can also be dropped ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH net-next 8/9] net: Remove SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS_SUPPORT
On Wed, Feb 13, 2019 at 02:06:37PM -0800, Florian Fainelli wrote: > Now that we have converted the bridge code and the drivers to check for > bridge port(s) flags at the time we try to set them, there is no need > for a get() -> set() sequence anymore and > SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS_SUPPORT therefore becomes unused. > > Signed-off-by: Florian Fainelli Reviewed-by: Ido Schimmel ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH net-next 7/9] net: bridge: Stop calling switchdev_port_attr_get()
On Wed, Feb 13, 2019 at 02:06:36PM -0800, Florian Fainelli wrote:
> Now that all switchdev drivers have been converted to checking the
> bridge port flags during the prepare phase of the
> switchdev_port_attr_set() when the process
> SWITCHDEV_ATTR_ID_PORT_PRE_BRIDGE_FLAGS, we can avoid calling
> switchdev_port_attr_get() with
> SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS_SUPPORT.
>
> Signed-off-by: Florian Fainelli
> ---
> net/bridge/br_switchdev.c | 16 +++-
> 1 file changed, 7 insertions(+), 9 deletions(-)
>
> diff --git a/net/bridge/br_switchdev.c b/net/bridge/br_switchdev.c
> index db9e8ab96d48..8f88f8a1a7fa 100644
> --- a/net/bridge/br_switchdev.c
> +++ b/net/bridge/br_switchdev.c
> @@ -64,29 +64,27 @@ int br_switchdev_set_port_flag(struct net_bridge_port *p,
> {
> struct switchdev_attr attr = {
> .orig_dev = p->dev,
> - .id = SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS_SUPPORT,
> + .id = SWITCHDEV_ATTR_ID_PORT_PRE_BRIDGE_FLAGS,
> + .u.brport_flags = flags,
> };
> int err;
>
> if (mask & ~BR_PORT_FLAGS_HW_OFFLOAD)
> return 0;
>
> - err = switchdev_port_attr_get(p->dev, );
> - if (err == -EOPNOTSUPP)
> - return 0;
> - if (err)
> + err = switchdev_port_attr_set(p->dev, );
> + if (err && err != -EOPNOTSUPP)
> return err;
>
> - /* Check if specific bridge flag attribute offload is supported */
> - if (!(attr.u.brport_flags_support & mask)) {
> + if (err == -EOPNOTSUPP) {
> br_warn(p->br, "bridge flag offload is not supported %u(%s)\n",
> (unsigned int)p->port_no, p->dev->name);
> - return -EOPNOTSUPP;
> + return err;
> }
I see that you return -EOPNOTSUPP from drivers in case of unsupported
flags. I believe this is problematic (I'll test soon). The same return
code is used by:
1. Switch drivers to indicate unsupported flags
2. switchdev code to indicate unsupported netdev (no switchdev ops)
I guess that with this patch any attempt to set bridge port flags on
veth/dummy device will result in an error.
>
> attr.id = SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS;
> attr.flags = SWITCHDEV_F_DEFER;
> - attr.u.brport_flags = flags;
> +
> err = switchdev_port_attr_set(p->dev, );
> if (err) {
> br_warn(p->br, "error setting offload flag on port %u(%s)\n",
> --
> 2.17.1
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH net-next 1/9] Documentation: networking: switchdev: Update port parent ID section
On Wed, Feb 13, 2019 at 02:06:30PM -0800, Florian Fainelli wrote:
> Update the section about switchdev drivers having to implement a
> switchdev_port_attr_get() function to return
> SWITCHDEV_ATTR_ID_PORT_PARENT_ID since that is no longer valid after
> commit bccb30254a4a ("net: Get rid of
> SWITCHDEV_ATTR_ID_PORT_PARENT_ID").
>
> Fixes: bccb30254a4a ("net: Get rid of SWITCHDEV_ATTR_ID_PORT_PARENT_ID")
> Acked-by: Jiri Pirko
> Signed-off-by: Florian Fainelli
Reviewed-by: Ido Schimmel
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH net-next v2 06/16] net: bridge: Stop calling switchdev_port_attr_get()
On Sun, Feb 10, 2019 at 11:34:14AM -0800, Florian Fainelli wrote:
> Le 2/10/19 à 11:05 AM, Ido Schimmel a écrit :
> > On Sun, Feb 10, 2019 at 09:50:55AM -0800, Florian Fainelli wrote:
> >> Now that all switchdev drivers have been converted to checking the
> >> bridge port flags during the prepare phase of the
> >> switchdev_port_attr_set(), we can move straight to trying to set the
> >> desired flag through SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS.
> >>
> >> Acked-by: Jiri Pirko
> >> Signed-off-by: Florian Fainelli
> >> ---
> >> net/bridge/br_switchdev.c | 20 +++-
> >> 1 file changed, 3 insertions(+), 17 deletions(-)
> >>
> >> diff --git a/net/bridge/br_switchdev.c b/net/bridge/br_switchdev.c
> >> index db9e8ab96d48..939f300522c5 100644
> >> --- a/net/bridge/br_switchdev.c
> >> +++ b/net/bridge/br_switchdev.c
> >> @@ -64,29 +64,15 @@ int br_switchdev_set_port_flag(struct net_bridge_port
> >> *p,
> >> {
> >>struct switchdev_attr attr = {
> >>.orig_dev = p->dev,
> >> - .id = SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS_SUPPORT,
> >> + .id = SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS,
> >> + .flags = SWITCHDEV_F_DEFER,
> >
> > How does this work? You defer the operation, so the driver cannot veto
> > it. This is why we have SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS_SUPPORT
> > which is not deferred.
>
> I missed that indeed, how would you feel about splitting the attribute
> setting into different phases:
>
> - checking that the attribute setting is supported (caller context, so
> possibly atomic)
> - allocating and committing resources (deferred context)
Yes, this is what I suggested in the other thread. Lets continue
discussion there.
We are doing that when processing route notifications (for example), but
we are missing a check in caller context that resources can actually be
allocated. That's part of a bigger task to track resources in mlxsw.
>
> For pretty much any DSA driver, we will have to be in sleepable context
> anyway because of MDIO, SPI, I2C, whatever transport layer.
>
> Not sure how to best approach this now...
> --
> Florian
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH net-next v4 9/9] net: Remove switchdev_ops
On Mon, Feb 11, 2019 at 11:10:01AM -0800, Florian Fainelli wrote: > Now that we have converted all possible callers to using a switchdev > notifier for attributes we do not have a need for implementing > switchdev_ops anymore, and this can be removed from all drivers the > net_device structure. > > Acked-by: Jiri Pirko > Signed-off-by: Florian Fainelli > --- > drivers/net/ethernet/mellanox/mlxsw/spectrum.c | 12 > drivers/net/ethernet/mellanox/mlxsw/spectrum.h | 2 -- > .../mellanox/mlxsw/spectrum_switchdev.c| 13 - > drivers/net/ethernet/mscc/ocelot.c | 5 - > drivers/net/ethernet/rocker/rocker_main.c | 6 -- > drivers/staging/fsl-dpaa2/ethsw/ethsw.c| 6 -- > include/linux/netdevice.h | 3 --- > include/net/switchdev.h| 18 -- > net/dsa/slave.c| 6 -- There's also this line that can be removed from the 8021q driver: net/8021q/vlan_dev.c:34:#include ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH net-next v4 4/9] mlxsw: spectrum_switchdev: Handle SWITCHDEV_PORT_ATTR_GET/SET
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
> Signed-off-by: Florian Fainelli
> ---
> .../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
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH net-next v4 2/9] switchdev: Add SWITCHDEV_PORT_ATTR_SET, SWITCHDEV_PORT_ATTR_GET
On Mon, Feb 11, 2019 at 11:09:54AM -0800, Florian Fainelli wrote:
> In preparation for allowing switchdev enabled drivers to veto specific
> attribute settings from within the context of the caller, introduce a
> new switchdev notifier type for port attributes.
>
> Suggested-by: Ido Schimmel
> Acked-by: Jiri Pirko
> Signed-off-by: Florian Fainelli
> ---
> include/net/switchdev.h | 10 ++
> 1 file changed, 10 insertions(+)
>
> diff --git a/include/net/switchdev.h b/include/net/switchdev.h
> index 5e87b54c5dc5..b8becabbef38 100644
> --- a/include/net/switchdev.h
> +++ b/include/net/switchdev.h
> @@ -143,6 +143,9 @@ enum switchdev_notifier_type {
> SWITCHDEV_VXLAN_FDB_ADD_TO_DEVICE,
> SWITCHDEV_VXLAN_FDB_DEL_TO_DEVICE,
> SWITCHDEV_VXLAN_FDB_OFFLOADED,
> +
> + SWITCHDEV_PORT_ATTR_SET, /* Blocking. */
> + SWITCHDEV_PORT_ATTR_GET, /* Blocking. */
As I wrote in the cover letter, I don't believe GET is needed.
> };
>
> struct switchdev_notifier_info {
> @@ -165,6 +168,13 @@ struct switchdev_notifier_port_obj_info {
> bool handled;
> };
>
> +struct switchdev_notifier_port_attr_info {
> + struct switchdev_notifier_info info; /* must be first */
> + struct switchdev_attr *attr;
> + struct switchdev_trans *trans;
> + bool handled;
> +};
> +
> static inline struct net_device *
> switchdev_notifier_info_to_dev(const struct switchdev_notifier_info *info)
> {
> --
> 2.17.1
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH net-next 1/3] mlxsw: spectrum_switchdev: Remove getting PORT_BRIDGE_FLAGS
On Mon, Feb 11, 2019 at 01:17:47PM -0800, Florian Fainelli wrote: > There is no code that will query the SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS > attribute remove support for that. > > Signed-off-by: Florian Fainelli Reviewed-by: Ido Schimmel ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH net-next v4 0/9] net: Remove switchdev_ops
On Mon, Feb 11, 2019 at 11:09:52AM -0800, Florian Fainelli wrote:
> Hi all,
>
> This patch series finishes by the removal of switchdev_ops. To get there
> we convert the existing switchdev_port_attr_{set,get} switchdev_ops to
> use a blocking notifier, thus making it consistent with how the objects
> are pushed to the switchdev enabled devices.
>
> Please review and let me know what you think!
>
> David, I would like to get Ido's feedback on this to make sure I did not
> miss something, thank you!
Hi Florian,
Why do you still keep switchdev_port_attr_get()? I believe we can remove
it and simplify things.
After your recent patchset to remove 'PORT_BRIDGE_FLAGS', the only
remaining user of get() is 'PORT_BRIDGE_FLAGS_SUPPORT'. It can be
converted to a blocking set() with 'PORT_PRE_BRIDGE_FLAGS' (or a similar
name).
I would like to make sure we're in sync with regards to future changes.
After this patchset to get rid of switchdev_ops we can continue to
completely removing switchdev (I believe Jiri approves). The
prepare-commit model is not really needed and the two switchdev
notification chains can be split into bridge and vxlan specific chains.
Notifications sent in an atomic context can be handled by drivers
directly in this context. Similar to how FDB/route/neighbour are
handled. It will really simplify things. No need for the defer flag
anymore and tricks like 'PORT_BRIDGE_FLAGS_SUPPORT' and
'PORT_PRE_BRIDGE_FLAGS'. In the atomic context the driver can veto the
requested bridge flags, but program the device from a blocking context
(using a workqueue).
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH net-next v4 1/9] Documentation: networking: switchdev: Update port parent ID section
On Mon, Feb 11, 2019 at 11:09:53AM -0800, Florian Fainelli wrote:
> Update the section about switchdev drivers having to implement a
> switchdev_port_attr_get() function to return
> SWITCHDEV_ATTR_ID_PORT_PARENT_ID since that is no longer valid after
> commit bccb30254a4a ("net: Get rid of
> SWITCHDEV_ATTR_ID_PORT_PARENT_ID").
>
> Fixes: bccb30254a4a ("net: Get rid of SWITCHDEV_ATTR_ID_PORT_PARENT_ID")
> Acked-by: Jiri Pirko
> Signed-off-by: Florian Fainelli
Reviewed-by: Ido Schimmel
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH net-next v4 0/9] net: Remove switchdev_ops
On Mon, Feb 11, 2019 at 12:16:57PM -0800, David Miller wrote: > From: Florian Fainelli > Date: Mon, 11 Feb 2019 11:09:52 -0800 > > > David, I would like to get Ido's feedback on this to make sure I did not > > miss something, thank you! > > Ok, Ido please look at this when you can. Will review tomorrow. I was occupied with other stuff last couple of days. Sorry ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH net-next v2 06/16] net: bridge: Stop calling switchdev_port_attr_get()
On Sun, Feb 10, 2019 at 09:50:55AM -0800, Florian Fainelli wrote:
> Now that all switchdev drivers have been converted to checking the
> bridge port flags during the prepare phase of the
> switchdev_port_attr_set(), we can move straight to trying to set the
> desired flag through SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS.
>
> Acked-by: Jiri Pirko
> Signed-off-by: Florian Fainelli
> ---
> net/bridge/br_switchdev.c | 20 +++-
> 1 file changed, 3 insertions(+), 17 deletions(-)
>
> diff --git a/net/bridge/br_switchdev.c b/net/bridge/br_switchdev.c
> index db9e8ab96d48..939f300522c5 100644
> --- a/net/bridge/br_switchdev.c
> +++ b/net/bridge/br_switchdev.c
> @@ -64,29 +64,15 @@ int br_switchdev_set_port_flag(struct net_bridge_port *p,
> {
> struct switchdev_attr attr = {
> .orig_dev = p->dev,
> - .id = SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS_SUPPORT,
> + .id = SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS,
> + .flags = SWITCHDEV_F_DEFER,
How does this work? You defer the operation, so the driver cannot veto
it. This is why we have SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS_SUPPORT
which is not deferred.
> + .u.brport_flags = flags,
> };
> int err;
>
> if (mask & ~BR_PORT_FLAGS_HW_OFFLOAD)
> return 0;
>
> - err = switchdev_port_attr_get(p->dev, );
> - if (err == -EOPNOTSUPP)
> - return 0;
> - if (err)
> - return err;
> -
> - /* Check if specific bridge flag attribute offload is supported */
> - if (!(attr.u.brport_flags_support & mask)) {
> - br_warn(p->br, "bridge flag offload is not supported %u(%s)\n",
> - (unsigned int)p->port_no, p->dev->name);
> - return -EOPNOTSUPP;
> - }
> -
> - attr.id = SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS;
> - attr.flags = SWITCHDEV_F_DEFER;
> - attr.u.brport_flags = flags;
> err = switchdev_port_attr_set(p->dev, );
> if (err) {
> br_warn(p->br, "error setting offload flag on port %u(%s)\n",
> --
> 2.19.1
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH net-next v4 12/12] net: Get rid of SWITCHDEV_ATTR_ID_PORT_PARENT_ID
On Wed, Feb 06, 2019 at 09:45:46AM -0800, Florian Fainelli wrote: > Now that we have a dedicated NDO for getting a port's parent ID, get rid > of SWITCHDEV_ATTR_ID_PORT_PARENT_ID and convert all callers to use the > NDO exclusively. This is a preliminary change to getting rid of > switchdev_ops eventually. > > Signed-off-by: Florian Fainelli Reviewed-by: Ido Schimmel ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH net-next v4 01/12] net: Introduce ndo_get_port_parent_id()
On Wed, Feb 06, 2019 at 09:45:35AM -0800, Florian Fainelli wrote: > In preparation for getting rid of switchdev_ops, create a dedicated NDO > operation for getting the port's parent identifier. There are > essentially two classes of drivers that need to implement getting the > port's parent ID which are VF/PF drivers with a built-in switch, and > pure switchdev drivers such as mlxsw, ocelot, dsa etc. > > We introduce a helper function: dev_get_port_parent_id() which supports > recursion into the lower devices to obtain the first port's parent ID. > > Convert the bridge, core and ipv4 multicast routing code to check for > such ndo_get_port_parent_id() and call the helper function when valid > before falling back to switchdev_port_attr_get(). This will allow us to > convert all relevant drivers in one go instead of having to implement > both switchdev_port_attr_get() and ndo_get_port_parent_id() operations, > then get rid of switchdev_port_attr_get(). > > Acked-by: Jiri Pirko > Signed-off-by: Florian Fainelli Reviewed-by: Ido Schimmel ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH net-next v4 05/12] mlxsw: Implement ndo_get_port_parent_id()
On Wed, Feb 06, 2019 at 09:45:39AM -0800, Florian Fainelli wrote: > mlxsw implements SWITCHDEV_ATTR_ID_PORT_PARENT_ID and we want to get rid > of switchdev_ops eventually, ease that migration by implementing a > ndo_get_port_parent_id() function which returns what > switchdev_port_attr_get() would do. > > Acked-by: Jiri Pirko > Signed-off-by: Florian Fainelli Reviewed-by: Ido Schimmel ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH net-next v3 00/12] net: Introduce ndo_get_port_parent_id()
On Wed, Feb 06, 2019 at 09:51:36AM +0200, Ido Schimmel wrote: > On Tue, Feb 05, 2019 at 03:53:14PM -0800, Florian Fainelli wrote: > > Hi all, > > > > Based on discussion with Ido and feedback from Jakub there are clearly > > two classes of users that implement SWITCHDEV_ATTR_ID_PORT_PARENT_ID: > > > > - PF/VF drivers which typically only implement return the port's parent > > ID, yet have to implement switchdev_port_attr_get() just for that > > > > - Ethernet switch drivers: mlxsw, ocelot, DSA, etc. which implement more > > attributes which we want to be able to eventually veto in the context > > of the caller, thus making them candidates for using a blocking notifier > > chain > > Florian, patches look good to me. I'm going to build a kernel with these > patches and run some tests. Will report later today. Ran most of our tests and nothing exploded. Thanks! ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH net-next v3 00/12] net: Introduce ndo_get_port_parent_id()
On Tue, Feb 05, 2019 at 03:53:14PM -0800, Florian Fainelli wrote: > Hi all, > > Based on discussion with Ido and feedback from Jakub there are clearly > two classes of users that implement SWITCHDEV_ATTR_ID_PORT_PARENT_ID: > > - PF/VF drivers which typically only implement return the port's parent > ID, yet have to implement switchdev_port_attr_get() just for that > > - Ethernet switch drivers: mlxsw, ocelot, DSA, etc. which implement more > attributes which we want to be able to eventually veto in the context > of the caller, thus making them candidates for using a blocking notifier > chain Florian, patches look good to me. I'm going to build a kernel with these patches and run some tests. Will report later today. Thanks ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 12/12] net: Get rid of SWITCHDEV_ATTR_ID_PORT_PARENT_ID
On Mon, Feb 04, 2019 at 03:36:33PM -0800, Florian Fainelli wrote:
> Now that we have a dedicated NDO for getting a port's parent ID, get rid
> of SWITCHDEV_ATTR_ID_PORT_PARENT_ID and convert all callers to use the
> NDO exclusively. This is a preliminary change to getting rid of
> switchdev_ops eventually.
>
> Signed-off-by: Florian Fainelli
> ---
> include/net/switchdev.h | 2 --
> net/bridge/br_switchdev.c | 11 +++
> net/core/net-sysfs.c | 14 +++---
> net/core/rtnetlink.c | 16
> net/ipv4/ipmr.c | 14 --
> net/switchdev/switchdev.c | 20 +---
> 6 files changed, 23 insertions(+), 54 deletions(-)
>
> diff --git a/include/net/switchdev.h b/include/net/switchdev.h
> index 63843ae5dc81..e1a5e8bc24b8 100644
> --- a/include/net/switchdev.h
> +++ b/include/net/switchdev.h
> @@ -43,7 +43,6 @@ static inline bool switchdev_trans_ph_commit(struct
> switchdev_trans *trans)
>
> enum switchdev_attr_id {
> SWITCHDEV_ATTR_ID_UNDEFINED,
> - SWITCHDEV_ATTR_ID_PORT_PARENT_ID,
> SWITCHDEV_ATTR_ID_PORT_STP_STATE,
> SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS,
> SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS_SUPPORT,
> @@ -61,7 +60,6 @@ struct switchdev_attr {
> void *complete_priv;
> void (*complete)(struct net_device *dev, int err, void *priv);
> union {
> - struct netdev_phys_item_id ppid;/* PORT_PARENT_ID */
> u8 stp_state; /* PORT_STP_STATE */
> unsigned long brport_flags; /* PORT_BRIDGE_FLAGS */
> unsigned long brport_flags_support; /*
> PORT_BRIDGE_FLAGS_SUPPORT */
> diff --git a/net/bridge/br_switchdev.c b/net/bridge/br_switchdev.c
> index 501a4221220a..620fd645f6f1 100644
> --- a/net/bridge/br_switchdev.c
> +++ b/net/bridge/br_switchdev.c
> @@ -24,18 +24,13 @@ static int br_switchdev_mark_get(struct net_bridge *br,
> struct net_device *dev)
> int nbp_switchdev_mark_set(struct net_bridge_port *p)
> {
> const struct net_device_ops *ops = p->dev->netdev_ops;
> - struct switchdev_attr attr = {
> - .orig_dev = p->dev,
> - .id = SWITCHDEV_ATTR_ID_PORT_PARENT_ID,
> - };
> - int err;
> + struct netdev_phys_item_id ppid = { };
> + int err = -EOPNOTSUPP;
>
> ASSERT_RTNL();
>
> if (ops->ndo_get_port_parent_id)
> - err = ops->ndo_get_port_parent_id(p->dev, );
> - else
> - err = switchdev_port_attr_get(p->dev, );
> + err = ops->ndo_get_port_parent_id(p->dev, );
> if (err) {
> if (err == -EOPNOTSUPP)
> return 0;
> diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
> index cc05e8b72657..1d2d76930afd 100644
> --- a/net/core/net-sysfs.c
> +++ b/net/core/net-sysfs.c
> @@ -12,7 +12,6 @@
> #include
> #include
> #include
> -#include
> #include
> #include
> #include
> @@ -502,19 +501,12 @@ static ssize_t phys_switch_id_show(struct device *dev,
> return restart_syscall();
>
> if (dev_isalive(netdev)) {
> - struct switchdev_attr attr = {
> - .orig_dev = netdev,
> - .id = SWITCHDEV_ATTR_ID_PORT_PARENT_ID,
> - .flags = SWITCHDEV_F_NO_RECURSE,
Florian, note this flag. We should not return the parent id in case this
is called for a stacked device. Maybe extend the ndo with a new
parameter called 'recurse' ? Or you can check here if device has lower
devices and bail.
> - };
> + struct netdev_phys_item_id ppid = { };
>
> if (ops->ndo_get_port_parent_id)
> - ret = ops->ndo_get_port_parent_id(netdev, );
> - else
> - ret = switchdev_port_attr_get(netdev, );
> + ret = ops->ndo_get_port_parent_id(netdev, );
> if (!ret)
> - ret = sprintf(buf, "%*phN\n", attr.u.ppid.id_len,
> - attr.u.ppid.id);
> + ret = sprintf(buf, "%*phN\n", ppid.id_len, ppid.id);
> }
> rtnl_unlock();
>
> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index ed8564eb97c8..27bccf68538e 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -46,7 +46,6 @@
>
> #include
> #include
> -#include
> #include
> #include
> #include
> @@ -1147,25 +1146,18 @@ static int rtnl_phys_port_name_fill(struct sk_buff
> *skb, struct net_device *dev)
> static int rtnl_phys_switch_id_fill(struct sk_buff *skb, struct net_device
> *dev)
> {
> const struct net_device_ops *ops = dev->netdev_ops;
> - int err;
> - struct switchdev_attr attr = {
> - .orig_dev = dev,
> - .id = SWITCHDEV_ATTR_ID_PORT_PARENT_ID,
> - .flags = SWITCHDEV_F_NO_RECURSE,
Same here.
> - };
> + struct netdev_phys_item_id ppid = { };
> + int err
Re: [PATCH 00/12] net: Introduce ndo_get_port_parent_id()
On Mon, Feb 04, 2019 at 03:36:21PM -0800, Florian Fainelli wrote: > Hi all, > > Based on discussion with Ido and feedback from Jakub there are clearly > two classes of users that implement SWITCHDEV_ATTR_ID_PORT_PARENT_ID: > > - PF/VF drivers which typically only implement return the port's parent > ID, yet have to implement switchdev_port_attr_get() just for that > > - Ethernet switch drivers: mlxsw, ocelot, DSA, etc. which implement more > attributes which we want to be able to eventually veto in the context > of the caller, thus making them candidates for using a blocking notifier > chain Florian, thanks for working on this. I think you're missing implementation over stacked devices. At least 8021q, bond and team need to implement this ndo as well. For bond and team we can return an error if not all the slaves have the same parent ID. It would be consistent with what switchdev_port_attr_get() is doing. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH net-next v3 0/7] net: bridge: Notify about bridge VLANs
On Tue, May 29, 2018 at 01:46:09PM +0300, Dan Carpenter wrote: > It occured to me that I should read the cover letter and here are the > answers I was looking for. But the cover letter isn't saved after the > commits are merged. DaveM adds the cover letter to the merge commit. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
