| Message ID | 20230417093412.12161-8-wojciech.drewek@intel.com (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | ice: switchdev bridge offload | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/apply | fail | Patch does not apply to net-next |
From: Wojciech Drewek <wojciech.drewek@intel.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@intel.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
> -----Original Message----- > From: Lobakin, Aleksander <aleksander.lobakin@intel.com> > Sent: piątek, 21 kwietnia 2023 16:40 > To: Drewek, Wojciech <wojciech.drewek@intel.com> > Cc: intel-wired-lan@lists.osuosl.org; netdev@vger.kernel.org; Ertman, David M <david.m.ertman@intel.com>; > michal.swiatkowski@linux.intel.com; marcin.szycik@linux.intel.com; Chmielewski, Pawel <pawel.chmielewski@intel.com>; > Samudrala, Sridhar <sridhar.samudrala@intel.com> > Subject: Re: [PATCH net-next 07/12] ice: Accept LAG netdevs in bridge offloads > > From: Wojciech Drewek <wojciech.drewek@intel.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@intel.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); I've seen examples of both so either way is ok I think > > > +} > > + > > +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 Will be fixed > > > + 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 ._. Using else makes sense to 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
From: Wojciech Drewek <wojciech.drewek@intel.com> Date: Wed, 26 Apr 2023 13:31:17 +0200 > > >> -----Original Message----- >> From: Lobakin, Aleksander <aleksander.lobakin@intel.com> >> Sent: piątek, 21 kwietnia 2023 16:40 >> To: Drewek, Wojciech <wojciech.drewek@intel.com> >> Cc: intel-wired-lan@lists.osuosl.org; netdev@vger.kernel.org; Ertman, David M <david.m.ertman@intel.com>; >> michal.swiatkowski@linux.intel.com; marcin.szycik@linux.intel.com; Chmielewski, Pawel <pawel.chmielewski@intel.com>; >> Samudrala, Sridhar <sridhar.samudrala@intel.com> >> Subject: Re: [PATCH net-next 07/12] ice: Accept LAG netdevs in bridge offloads >> >> From: Wojciech Drewek <wojciech.drewek@intel.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@intel.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); > > I've seen examples of both so either way is ok I think Correct, that's more of my personal :D Or maybe I've seen a couple times that either checkpatch or something else complained on the second line being not aligned to the first one with `return`. [...] Thanks, Olek
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); +} + +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; + 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); + 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); + + if (!ice_dev) + return 0; + + pf = ice_netdev_to_pf(ice_dev); err = ice_eswitch_br_uplink_port_init(bridge, pf); }
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@intel.com> --- .../net/ethernet/intel/ice/ice_eswitch_br.c | 40 ++++++++++++++++--- 1 file changed, 35 insertions(+), 5 deletions(-)