| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <MW4PR11MB5776C7DDDB91DD98A960AD20FD639@MW4PR11MB5776.namprd11.prod.outlook.com>
Date: Thu, 20 Apr 2023 09:54:15 +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 04/12] ice: Implement basic eswitch bridge setup
Thanks for review Olek!
Most of the comments sound reasonable to me (and I will include them) with some exceptions.
> -----Original Message-----
> From: Lobakin, Aleksander <aleksander.lobakin@...el.com>
> Sent: środa, 19 kwietnia 2023 17:24
> 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 04/12] ice: Implement basic eswitch bridge setup
>
> From: Wojciech Drewek <wojciech.drewek@...el.com>
> Date: Mon, 17 Apr 2023 11:34:04 +0200
>
> > With this patch, ice driver is able to track if the port
> > representors or uplink port were added to the linux bridge in
> > switchdev mode. Listen for NETDEV_CHANGEUPPER events in order to
> > detect this. ice_esw_br data structure reflects the linux bridge
> > and stores all the ports of the bridge (ice_esw_br_port) in
> > xarray, it's created when the first port is added to the bridge and
> > freed once the last port is removed. Note that only one bridge is
> > supported per eswitch.
>
> [...]
>
> > diff --git a/drivers/net/ethernet/intel/ice/ice.h b/drivers/net/ethernet/intel/ice/ice.h
> > index ac2971073fdd..5b2ade5908e8 100644
> > --- a/drivers/net/ethernet/intel/ice/ice.h
> > +++ b/drivers/net/ethernet/intel/ice/ice.h
> > @@ -511,6 +511,7 @@ struct ice_switchdev_info {
> > struct ice_vsi *control_vsi;
> > struct ice_vsi *uplink_vsi;
> > bool is_running;
> > + struct ice_esw_br_offloads *br_offloads;
>
> 7-byte hole here unfortunately =\ After ::is_running. You can place
> ::br_offloads *before* ::is_running to avoid this (well, you'll still
> have it, but as padding at the end of the structure).
> ...or change ::is_running to "unsigned long flags" to not waste 1 byte
> for 1 bit and have 63 free flags more :D
>
> > };
> >
> > struct ice_agg_node {
>
> [...]
>
> > +static struct ice_esw_br_port *
> > +ice_eswitch_br_netdev_to_port(struct net_device *dev)
>
> Also const?
>
> > +{
> > + if (ice_is_port_repr_netdev(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);
>
> Both @repr and @pf can also be const :p
>
> > +
> > + return pf->br_port;
> > + }
> > +
> > + return NULL;
> > +}
>
> [...]
>
> > +static struct ice_esw_br_port *
> > +ice_eswitch_br_port_init(struct ice_esw_br *bridge)
> > +{
> > + struct ice_esw_br_port *br_port;
> > +
> > + br_port = kzalloc(sizeof(*br_port), GFP_KERNEL);
> > + if (!br_port)
> > + return ERR_PTR(-ENOMEM);
> > +
> > + br_port->bridge = bridge;
>
> Since you always pass @bridge from the call site either way, does it
> make sense to do that or you could just assign -> bridge on the call
> sites after a successful allocation?
I could do that but I prefer to keep it this way.
We have two types of ports and this function is generic, It setups
things common for both types, including bridge ref.
Are you ok with it?
>
> > +
> > + return br_port;
> > +}
>
> [...]
>
> > +static int
> > +ice_eswitch_br_port_changeupper(struct notifier_block *nb, void *ptr)
> > +{
> > + struct net_device *dev = netdev_notifier_info_to_dev(ptr);
> > + struct netdev_notifier_changeupper_info *info = ptr;
> > + struct ice_esw_br_offloads *br_offloads =
> > + ice_nb_to_br_offloads(nb, netdev_nb);
>
> Maybe assign it outside the declaration block to avoid line wrap?
>
> > + struct netlink_ext_ack *extack;
> > + struct net_device *upper;
> > +
> > + if (!ice_eswitch_br_is_dev_valid(dev))
> > + return 0;
> > +
> > + upper = info->upper_dev;
> > + if (!netif_is_bridge_master(upper))
> > + return 0;
> > +
> > + extack = netdev_notifier_info_to_extack(&info->info);
> > +
> > + return info->linking ?
> > + ice_eswitch_br_port_link(br_offloads, dev, upper->ifindex,
> > + extack) :
> > + ice_eswitch_br_port_unlink(br_offloads, dev, upper->ifindex,
> > + extack);
>
> And here do that via `if return else return` to avoid multi-line ternary?
>
> > +}
> > +
> > +static int
> > +ice_eswitch_br_port_event(struct notifier_block *nb,
> > + unsigned long event, void *ptr)
>
> [...]
>
> > diff --git a/drivers/net/ethernet/intel/ice/ice_eswitch_br.h b/drivers/net/ethernet/intel/ice/ice_eswitch_br.h
> > new file mode 100644
> > index 000000000000..53ea29569c36
> > --- /dev/null
> > +++ b/drivers/net/ethernet/intel/ice/ice_eswitch_br.h
> > @@ -0,0 +1,42 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/* Copyright (C) 2023, Intel Corporation. */
> > +
> > +#ifndef _ICE_ESWITCH_BR_H_
> > +#define _ICE_ESWITCH_BR_H_
> > +
> > +enum ice_esw_br_port_type {
> > + ICE_ESWITCH_BR_UPLINK_PORT = 0,
> > + ICE_ESWITCH_BR_VF_REPR_PORT = 1,
> > +};
> > +
> > +struct ice_esw_br_port {
> > + struct ice_esw_br *bridge;
> > + enum ice_esw_br_port_type type;
>
> Also hole :s I'd move it one line below.
>
> > + struct ice_vsi *vsi;
> > + u16 vsi_idx;
> > +};
> > +
> > +struct ice_esw_br {
> > + struct ice_esw_br_offloads *br_offloads;
> > + int ifindex;
> > +
> > + struct xarray ports;
>
> (not sure about this one, but potentially there can be a hole between
> those two)
Move ifindex at the end?
>
> > +};
> > +
> > +struct ice_esw_br_offloads {
> > + struct ice_pf *pf;
> > + struct ice_esw_br *bridge;
> > + struct notifier_block netdev_nb;
> > +};
> > +
> > +#define ice_nb_to_br_offloads(nb, nb_name) \
> > + container_of(nb, \
> > + struct ice_esw_br_offloads, \
> > + nb_name)
>
> Hmm, you use it only once and only with `netdev_nb` field. Do you plan
> to add more call sites of this macro? Otherwise you could embed the
> second argument into the macro itself (mentioned `netdev_nb`) or even
> just open-code the whole macro in the sole call site.
I the next patch it is used with different nb_name (switchdev_nb)
>
> > +
> > +void
> > +ice_eswitch_br_offloads_deinit(struct ice_pf *pf);
> > +int
> > +ice_eswitch_br_offloads_init(struct ice_pf *pf);
> > +
> > +#endif /* _ICE_ESWITCH_BR_H_ */
> [...]
>
> Thanks,
> Olek
Powered by blists - more mailing lists