Date:   Thu, 20 Apr 2023 11:27:11 +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 05/12] ice: Switchdev FDB events support



> -----Original Message-----
> From: Lobakin, Aleksander <aleksander.lobakin@...el.com>
> Sent: środa, 19 kwietnia 2023 17:39
> To: Drewek, Wojciech <wojciech.drewek@...el.com>
> Cc: intel-wired-lan@...ts.osuosl.org; netdev@...r.kernel.org; Lobakin, Aleksander <aleksander.lobakin@...el.com>; 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 05/12] ice: Switchdev FDB events support
> 
> From: Wojciech Drewek <wojciech.drewek@...el.com>
> Date: Mon, 17 Apr 2023 11:34:05 +0200
> 
> > Listen for SWITCHDEV_FDB_{ADD|DEL}_TO_DEVICE events while in switchdev
> > mode. Accept these events on both uplink and VF PR ports. Add HW
> > rules in newly created workqueue. FDB entries are stored in rhashtable
> > for lookup when removing the entry and in the list for cleanup
> > purpose. Direction of the HW rule depends on the type of the ports
> > on which the FDB event was received:
> 
> [...]
> 
> > +static int
> > +ice_eswitch_br_rule_delete(struct ice_hw *hw, struct ice_rule_query_data *rule)
> > +{
> > +	int err;
> > +
> > +	if (!rule)
> > +		return -EINVAL;
> > +
> > +	err = ice_rem_adv_rule_by_id(hw, rule);
> > +	kfree(rule);
> > +
> > +	return err;
> > +}
> > +
> > +static struct ice_rule_query_data *
> > +ice_eswitch_br_fwd_rule_create(struct ice_hw *hw, u16 vsi_idx, int port_type,
> 
> (no types shorter than u32 on the stack reminder)
> 
> > +			       const unsigned char *mac)
> > +{
> > +	struct ice_adv_rule_info rule_info = { 0 };
> > +	struct ice_rule_query_data *rule;
> > +	struct ice_adv_lkup_elem *list;
> > +	u16 lkups_cnt = 1;
> 
> Why have it as variable if it doesn't change? Just embed it into the
> ice_add_adv_rule() call and replace kcalloc() with kzalloc().

It will be useful later, with vlans support lkups_cnt will be equal to 1 or 2.
Can we keep it as it is?

> 
> > +	int err;
> > +
> > +	rule = kzalloc(sizeof(*rule), GFP_KERNEL);
> > +	if (!rule)
> > +		return ERR_PTR(-ENOMEM);
> > +
> > +	list = kcalloc(lkups_cnt, sizeof(*list), GFP_ATOMIC);
> 
> [...]
> 
> > +	fwd_rule = ice_eswitch_br_fwd_rule_create(hw, vsi_idx, port_type, mac);
> > +	if (IS_ERR(fwd_rule)) {
> > +		err = PTR_ERR(fwd_rule);
> > +		dev_err(dev, "Failed to create eswitch bridge %sgress forward rule, err: %d\n",
> > +			port_type == ICE_ESWITCH_BR_UPLINK_PORT ? "e" : "in",
> > +			err);
> > +		goto err_fwd_rule;
> 
> A bit suboptimal. To print errno pointer, you have %pe modifier, so you
> can just print err as:
> 
> 		... forward rule, err: %pe\n", ... : "in", fwd_rule);
> 
> Then you don't need @err at all and then below...

This is really cool, but I think it won't work here. I need to keep err in order to
return it in the err flow. I can't use fwd_rule for this purpose because
return type is ice_esw_br_flow not ice_rule_query_data.

> 
> > +	}
> > +
> > +	flow->fwd_rule = fwd_rule;
> > +
> > +	return flow;
> > +
> > +err_fwd_rule:
> > +	kfree(flow);
> > +
> > +	return ERR_PTR(err);
> 
> ...you can return @fwd_rule directly.
> 

I can't return @fwd_rule here because return type is different
This function is meant to return @flow.


[...]

> > +static int
> > +ice_eswitch_br_switchdev_event(struct notifier_block *nb,
> > +			       unsigned long event, void *ptr)
> > +{
> > +	struct net_device *dev = switchdev_notifier_info_to_dev(ptr);
> > +	struct ice_esw_br_offloads *br_offloads =
> > +		ice_nb_to_br_offloads(nb, switchdev_nb);
> > +	struct netlink_ext_ack *extack =
> > +		switchdev_notifier_info_to_extack(ptr);
> 
> (initialize-later-to-avoid-line-breaks?)
> 
> > +	struct switchdev_notifier_fdb_info *fdb_info;
> > +	struct switchdev_notifier_info *info = ptr;
> > +	struct ice_esw_br_fdb_work *work;
> > +	struct net_device *upper;
> > +	struct ice_esw_br_port *br_port;
> 
> RCT :s
> 
> > +
> > +	upper = netdev_master_upper_dev_get_rcu(dev);
> > +	if (!upper)
> > +		return NOTIFY_DONE;
> > +
> > +	if (!netif_is_bridge_master(upper))
> > +		return NOTIFY_DONE;
> > +
> > +	if (!ice_eswitch_br_is_dev_valid(dev))
> > +		return NOTIFY_DONE;
> > +
> > +	br_port = ice_eswitch_br_netdev_to_port(dev);
> > +	if (!br_port)
> > +		return NOTIFY_DONE;
> > +
> > +	switch (event) {
> > +	case SWITCHDEV_FDB_ADD_TO_DEVICE:
> > +	case SWITCHDEV_FDB_DEL_TO_DEVICE:
> > +		fdb_info = container_of(info,
> > +					struct switchdev_notifier_fdb_info,
> 
> Nit: `typeof(*fdb_info)` is shorter and would probably fit into the prev
> line.

I can make it in to one line now, thanks.

> 
> > +					info);
> > +
> > +		work = ice_eswitch_br_fdb_work_alloc(fdb_info, br_port, dev,
> > +						     event);
> 
> [...]
> 
> > +enum {
> > +	ICE_ESWITCH_BR_FDB_ADDED_BY_USER = BIT(0),
> > +};
> > +
> > +struct ice_esw_br_fdb_entry {
> > +	struct ice_esw_br_fdb_data data;
> > +	struct rhash_head ht_node;
> > +	struct list_head list;
> > +
> > +	int flags;
> 
> They can't be negative I believe? u32 then? Also I think here's a 4-byte
> hole :s But since all of the members here except this one are 8-byte
> aligned, you can't avoid it (can be filled anytime later with some other
> <= 4-byte field)
> 
> > +
> > +	struct net_device *dev;
> > +	struct ice_esw_br_port *br_port;
> > +	struct ice_esw_br_flow *flow;
> > +};
> [...]
> 
> Thanks,
> Olek

Powered by blists - more mailing lists