[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20210318144421.ecnqtrlhfn6zntkx@skbuf>
Date: Thu, 18 Mar 2021 16:44:21 +0200
From: Vladimir Oltean <olteanv@...il.com>
To: Tobias Waldekranz <tobias@...dekranz.com>
Cc: davem@...emloft.net, kuba@...nel.org, andrew@...n.ch,
vivien.didelot@...il.com, f.fainelli@...il.com,
netdev@...r.kernel.org
Subject: Re: [PATCH v2 net-next 1/8] net: dsa: Add helper to resolve bridge
port from DSA port
On Thu, Mar 18, 2021 at 03:15:43PM +0100, Tobias Waldekranz wrote:
> In order for a driver to be able to query a bridge for information
> about itself, e.g. reading out port flags, it has to use a netdev that
> is known to the bridge. In the simple case, that is just the netdev
> representing the port, e.g. swp0 or swp1 in this example:
>
> br0
> / \
> swp0 swp1
>
> But in the case of an offloaded lag, this will be the bond or team
> interface, e.g. bond0 in this example:
>
> br0
> /
> bond0
> / \
> swp0 swp1
>
> Add a helper that hides some of this complexity from the
> drivers. Then, redefine dsa_port_offloads_bridge_port using the helper
> to avoid double accounting of the set of possible offloaded uppers.
>
> Signed-off-by: Tobias Waldekranz <tobias@...dekranz.com>
> ---
> include/net/dsa.h | 14 ++++++++++++++
> net/dsa/dsa_priv.h | 14 +-------------
> 2 files changed, 15 insertions(+), 13 deletions(-)
>
> diff --git a/include/net/dsa.h b/include/net/dsa.h
> index dac303edd33d..5c4340ecfeb2 100644
> --- a/include/net/dsa.h
> +++ b/include/net/dsa.h
> @@ -493,6 +493,20 @@ static inline bool dsa_port_is_vlan_filtering(const struct dsa_port *dp)
> return dp->vlan_filtering;
> }
>
> +static inline
> +struct net_device *dsa_port_to_bridge_port(const struct dsa_port *dp)
> +{
> + if (!dsa_is_user_port(dp->ds, dp->index))
> + return NULL;
> +
According to my comment from 8/8, you could have replaced this here with
if (!dp->bridge_dev)
return NULL;
I think it's more intuitive to not return a bridge port if there isn't
any bridge to speak of. Whether you prefer to do that or not is up to
you, here's my review tag anyway.
Reviewed-by: Vladimir Oltean <olteanv@...il.com>
> + if (dp->lag_dev)
> + return dp->lag_dev;
> + else if (dp->hsr_dev)
> + return dp->hsr_dev;
> +
> + return dp->slave;
> +}
> +
> typedef int dsa_fdb_dump_cb_t(const unsigned char *addr, u16 vid,
> bool is_static, void *data);
> struct dsa_switch_ops {
Powered by blists - more mailing lists