[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ee3d3162-bba6-d65b-92d4-e44930b9110b@intel.com>
Date: Fri, 21 Apr 2023 16:40:29 +0200
From: Alexander Lobakin <aleksander.lobakin@...el.com>
To: Wojciech Drewek <wojciech.drewek@...el.com>
CC: <intel-wired-lan@...ts.osuosl.org>, <netdev@...r.kernel.org>,
<david.m.ertman@...el.com>, <michal.swiatkowski@...ux.intel.com>,
<marcin.szycik@...ux.intel.com>, <pawel.chmielewski@...el.com>,
<sridhar.samudrala@...el.com>
Subject: Re: [PATCH net-next 07/12] ice: Accept LAG netdevs in bridge offloads
From: Wojciech Drewek <wojciech.drewek@...el.com>
Date: Mon, 17 Apr 2023 11:34:07 +0200
> Allow LAG interfaces to be used in bridge offload using
> netif_is_lag_master. In this case, search for ice netdev in
> the list of LAG's lower devices.
>
> Signed-off-by: Wojciech Drewek <wojciech.drewek@...el.com>
> ---
> .../net/ethernet/intel/ice/ice_eswitch_br.c | 40 ++++++++++++++++---
> 1 file changed, 35 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/ice/ice_eswitch_br.c b/drivers/net/ethernet/intel/ice/ice_eswitch_br.c
> index 82b5eb2020cd..49381e4bf62a 100644
> --- a/drivers/net/ethernet/intel/ice/ice_eswitch_br.c
> +++ b/drivers/net/ethernet/intel/ice/ice_eswitch_br.c
> @@ -15,8 +15,21 @@ static const struct rhashtable_params ice_fdb_ht_params = {
>
> static bool ice_eswitch_br_is_dev_valid(const struct net_device *dev)
> {
> - /* Accept only PF netdev and PRs */
> - return ice_is_port_repr_netdev(dev) || netif_is_ice(dev);
> + /* Accept only PF netdev, PRs and LAG */
> + return ice_is_port_repr_netdev(dev) || netif_is_ice(dev) ||
> + netif_is_lag_master(dev);
Nit: usually we align to `return` (7 spaces), not with one tab:
return ice_is_port_repr_netdev(dev) || netif_is_ice(dev) ||
netif_is_lag_master(dev);
> +}
> +
> +static struct net_device *
> +ice_eswitch_br_get_uplnik_from_lag(struct net_device *lag_dev)
> +{
> + struct net_device *lower;
> + struct list_head *iter;
> +
> + netdev_for_each_lower_dev(lag_dev, lower, iter)
> + if (netif_is_ice(lower))
> + return lower;
Here I think the kernel guidelines would require to have a set of braces
(each multi-line code block must be enclosed, even if it works without).
I mean, I wasn't doing it myself using the rule "as minimum braces as
needed to work", but then my colleague showed me the doc :D
for_each_lover(...) {
if (is_ice(lover))
return lover;
}
In contrary, this:
for_each_something()
/* Some useful comment */
do_something();
is not mentioned in the rules as requiring braces :s
> + return NULL;
> }
>
> static struct ice_esw_br_port *
> @@ -26,8 +39,16 @@ ice_eswitch_br_netdev_to_port(struct net_device *dev)
> struct ice_repr *repr = ice_netdev_to_repr(dev);
>
> return repr->br_port;
> - } else if (netif_is_ice(dev)) {
> - struct ice_pf *pf = ice_netdev_to_pf(dev);
> + } else if (netif_is_ice(dev) || netif_is_lag_master(dev)) {
> + struct net_device *ice_dev = dev;
> + struct ice_pf *pf;
> +
> + if (netif_is_lag_master(dev))
> + ice_dev = ice_eswitch_br_get_uplnik_from_lag(dev);
Maybe just reuse @dev instead of one more var?
Or do it this way:
struct net_device *ice_dev;
...
if (netif_is_lag_master(dev))
ice_dev = ice_eswitch ...
else
ice_dev = dev;
if (!ice_dev)
return NULL;
Otherwise it's a bit confusing to have `if` in one place and `else`
(implicit) in another one, at least it took some time for me ._.
> + if (!ice_dev)
> + return NULL;
> +
> + pf = ice_netdev_to_pf(ice_dev);
>
> return pf->br_port;
> }
> @@ -719,7 +740,16 @@ ice_eswitch_br_port_link(struct ice_esw_br_offloads *br_offloads,
>
> err = ice_eswitch_br_vf_repr_port_init(bridge, repr);
> } else {
> - struct ice_pf *pf = ice_netdev_to_pf(dev);
> + struct net_device *ice_dev = dev;
> + struct ice_pf *pf;
> +
> + if (netif_is_lag_master(dev))
> + ice_dev = ice_eswitch_br_get_uplnik_from_lag(dev);
(same)
> +
> + if (!ice_dev)
> + return 0;
> +
> + pf = ice_netdev_to_pf(ice_dev);
>
> err = ice_eswitch_br_uplink_port_init(bridge, pf);
> }
Thanks,
Olek
Powered by blists - more mailing lists