netdev - Re: [PATCH net-next 7/7] net: atlantic: unify get_mac

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: <20200507122957.5dd4b84b@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Thu, 7 May 2020 12:29:57 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Igor Russkikh <irusskikh@...vell.com>
Cc:     <netdev@...r.kernel.org>, "David S . Miller" <davem@...emloft.net>,
        Mark Starovoytov <mstarovoitov@...vell.com>
Subject: Re: [PATCH net-next 7/7] net: atlantic: unify get_mac_permanent

On Thu, 7 May 2020 11:15:10 +0300 Igor Russkikh wrote:
> From: Mark Starovoytov <mstarovoitov@...vell.com>
> 
> MAC generation in case if MAC is not populated is the same for both A1 and
> A2.
> This patch moves MAC generation into a separate function, which is called
> from both places to reduce the amount of copy/paste.

Right, but why do you have your own mac generation rather than using
eth_hw_addr_random(). You need to set NET_ADDR_RANDOM for example,
just use standard helpers, please.

> Signed-off-by: Mark Starovoytov <mstarovoitov@...vell.com>
> Signed-off-by: Igor Russkikh <irusskikh@...vell.com>
> ---
>  .../ethernet/aquantia/atlantic/aq_hw_utils.c  | 41 +++++++++++++++++--
>  .../ethernet/aquantia/atlantic/aq_hw_utils.h  |  9 ++--
>  .../atlantic/hw_atl/hw_atl_utils_fw2x.c       | 23 +----------
>  .../atlantic/hw_atl2/hw_atl2_utils_fw.c       | 24 ++---------
>  4 files changed, 49 insertions(+), 48 deletions(-)
> 
> diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_hw_utils.c b/drivers/net/ethernet/aquantia/atlantic/aq_hw_utils.c
> index 7dbf49adcea6..0bc01772ead2 100644
> --- a/drivers/net/ethernet/aquantia/atlantic/aq_hw_utils.c
> +++ b/drivers/net/ethernet/aquantia/atlantic/aq_hw_utils.c
> @@ -1,7 +1,8 @@
>  // SPDX-License-Identifier: GPL-2.0-only
> -/*
> - * aQuantia Corporation Network Driver
> - * Copyright (C) 2014-2017 aQuantia Corporation. All rights reserved
> +/* Atlantic Network Driver
> + *
> + * Copyright (C) 2014-2019 aQuantia Corporation
> + * Copyright (C) 2019-2020 Marvell International Ltd.
>   */
>  
>  /* File aq_hw_utils.c: Definitions of helper functions used across
> @@ -79,3 +80,37 @@ int aq_hw_err_from_flags(struct aq_hw_s *hw)
>  err_exit:
>  	return err;
>  }
> +
> +static inline bool aq_hw_is_zero_ether_addr(const u8 *addr)

No static inlines in C files, please. Let compiler decide inlineing &
generate a warning when function becomes unused.

> +{
> +	return (*(const u16 *)(addr) | addr[2]) == 0;

It's probably fine in practice but the potentially u16 read is entirely
unnecessary. This is not performance sensitive code.

> +}
> +
> +bool aq_hw_is_valid_ether_addr(const u8 *addr)
> +{
> +	return !is_multicast_ether_addr(addr) &&
> +	       !aq_hw_is_zero_ether_addr(addr);
> +}
> +
> +void aq_hw_eth_random_addr(u8 *mac)
> +{
> +	unsigned int rnd = 0;
> +	u32 h = 0U;
> +	u32 l = 0U;
> +
> +	get_random_bytes(&rnd, sizeof(unsigned int));
> +	l = 0xE300 0000U | (0xFFFFU & rnd) | (0x00 << 16);
> +	h = 0x8001300EU;
> +
> +	mac[5] = (u8)(0xFFU & l);
> +	l >>= 8;
> +	mac[4] = (u8)(0xFFU & l);
> +	l >>= 8;
> +	mac[3] = (u8)(0xFFU & l);
> +	l >>= 8;
> +	mac[2] = (u8)(0xFFU & l);
> +	mac[1] = (u8)(0xFFU & h);
> +	h >>= 8;
> +	mac[0] = (u8)(0xFFU & h);

This can be greatly simplified using helpers from etherdevice.h, if
it's really needed.

> +}