netdev - Re: [PATCH RFC net-next 3/4] net/mlx5: Separate between eswitch and MPFS l2 table logic

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]

Date:   Mon, 12 Jun 2017 13:52:54 -0400
From:   Jes Sorensen <jsorensen@...com>
To:     Saeed Mahameed <saeedm@...lanox.com>, <netdev@...r.kernel.org>
CC:     <kernel-team@...com>, Or Gerlitz <ogerlitz@...lanox.com>,
        Tzahi Oved <tzahio@...lanox.com>
Subject: Re: [PATCH RFC net-next 3/4] net/mlx5: Separate between eswitch and
 MPFS l2 table logic

On 06/07/2017 07:42 PM, Saeed Mahameed wrote:
> Multi-Physical Function Switch (MPFs) is required for when multi-PF
> configuration is enabled to allow passing user configured unicast MAC
> addresses to the requesting PF.
> 
> Before this patch eswitch.c used to manage the HW MPFS l2 table,
> eswitch always enabled vport(0) (NIC PF) vport's contexts update on unicast
> mac address list changes, to populate the PF's MPFS L2 table accordingly,
> even if SRIOV was not enabled.
> 
> In downstream patch we would like to allow compiling the driver without
> eswitch functionalities, for that we move MPFS l2 table logic out
> of eswitch.c into its own file, and provide Kconfig flag (MLX5_MPFS) to
> allow compiling out MPFS for those who don't want Multi-PF support
> 
> NIC PF netdevice will now directly update MPFS l2 table via the new MPFS
> API. VF netdevice has no access to MPFS L2 table, so e-Switch will remain
> responsible of updating its PF MPFS l2 table on behalf of its VFs.
> 
> Due to this change we also don't require enabling vport(0) (PF vport)
> unicast mac changes events anymore, for when SRIOV is not enabled.
> Which means eswitch is now activated only on SRIOV activation, and not
> required otherwise.


On overall it looks good - one nit.

> +static int alloc_l2table_index(struct mlx5_mpfs *l2table, u32 *ix)
> +{
> +	int err = 0;
> +
> +	*ix = find_first_zero_bit(l2table->bitmap, l2table->size);
> +	if (*ix >= l2table->size)
> +		err = -ENOSPC;
> +	else
> +		__set_bit(*ix, l2table->bitmap);
> +
> +	return err;
> +}

You pass in a pointer for ix but you don't modify it, why not just pass 
in the value?.

> +static void free_l2table_index(struct mlx5_mpfs *l2table, u32 ix)
> +{
> +	__clear_bit(ix, l2table->bitmap);
> +}

Here you stick to the u32.

Jes