netdev - Re: [PATCH net-next 05/12] ice: Switchdev FDB events support

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <92c09efa-7e47-f580-4f60-19f9bb0f045d@intel.com>
Date:   Thu, 20 Apr 2023 18:59:43 +0200
From:   Alexander Lobakin <aleksander.lobakin@...el.com>
To:     "Drewek, Wojciech" <wojciech.drewek@...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

From: Wojciech Drewek <wojciech.drewek@...el.com>
Date: Thu, 20 Apr 2023 13:27:11 +0200

> 
> 
>> -----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

[...]

>> (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?

Ah, okay, then it's surely better to keep as-is. Maybe I'd only mention
then in the commit message that this variable will be expanded to have
several values later. So that other reviewers won't trigger on the same
stuff.

> 
>>
>>> +	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.

My bad, forgot to mention. If you want to return error pointer of a type
different from the return value's one, there's ERR_CAST(). It casts
error pointer to `void *`, so that there'll be no warnings then.
Here's nice example: [0]

> 
>>
>>> +	}
>>> +
>>> +	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.

[...]

>>> +	struct net_device *dev;
>>> +	struct ice_esw_br_port *br_port;
>>> +	struct ice_esw_br_flow *flow;
>>> +};
>> [...]
>>
>> Thanks,
>> Olek

[0]
https://elixir.bootlin.com/linux/latest/source/drivers/clk/clk-fractional-divider.c#L293

Thanks,
Olek