[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <MW4PR11MB5776258BAC908BD75A739DF2FD659@MW4PR11MB5776.namprd11.prod.outlook.com>
Date: Wed, 26 Apr 2023 11:31:17 +0000
From: "Drewek, Wojciech" <wojciech.drewek@...el.com>
To: "Lobakin, Aleksander" <aleksander.lobakin@...el.com>
CC: "intel-wired-lan@...ts.osuosl.org" <intel-wired-lan@...ts.osuosl.org>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"Ertman, David M" <david.m.ertman@...el.com>,
"michal.swiatkowski@...ux.intel.com"
<michal.swiatkowski@...ux.intel.com>,
"marcin.szycik@...ux.intel.com" <marcin.szycik@...ux.intel.com>,
"Chmielewski, Pawel" <pawel.chmielewski@...el.com>,
"Samudrala, Sridhar" <sridhar.samudrala@...el.com>
Subject: RE: [PATCH net-next 07/12] ice: Accept LAG netdevs in bridge offloads
> -----Original Message-----
> From: Lobakin, Aleksander <aleksander.lobakin@...el.com>
> Sent: piÄ…tek, 21 kwietnia 2023 16:40
> To: Drewek, Wojciech <wojciech.drewek@...el.com>
> Cc: intel-wired-lan@...ts.osuosl.org; netdev@...r.kernel.org; Ertman, David M <david.m.ertman@...el.com>;
> michal.swiatkowski@...ux.intel.com; marcin.szycik@...ux.intel.com; Chmielewski, Pawel <pawel.chmielewski@...el.com>;
> Samudrala, Sridhar <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);
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
Powered by blists - more mailing lists