| 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: <4a293c46-f112-e985-f9ad-19a41dd64f01@intel.com>
Date: Wed, 19 Apr 2023 17:23:45 +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 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?
> +
> + 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)
> +};
> +
> +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.
> +
> +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