From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 99166C433F5 for ; Wed, 17 Nov 2021 18:24:49 +0000 (UTC) Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id BE84761BC1 for ; Wed, 17 Nov 2021 18:24:48 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org BE84761BC1 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=konsulko.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=lists.denx.de Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 388C182A74; Wed, 17 Nov 2021 19:24:46 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=none (p=none dis=none) header.from=konsulko.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (1024-bit key; unprotected) header.d=konsulko.com header.i=@konsulko.com header.b="PA0bzWIR"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 52A2082B94; Wed, 17 Nov 2021 19:24:44 +0100 (CET) Received: from mail-qt1-x82d.google.com (mail-qt1-x82d.google.com [IPv6:2607:f8b0:4864:20::82d]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id 0771B8169D for ; Wed, 17 Nov 2021 19:24:40 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=none (p=none dis=none) header.from=konsulko.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=trini@konsulko.com Received: by mail-qt1-x82d.google.com with SMTP id v22so3517446qtx.8 for ; Wed, 17 Nov 2021 10:24:39 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=konsulko.com; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=WG3HBPpUGn9DDKKUpA5tfiQHZCqIZN6nfuk/xwEgU2E=; b=PA0bzWIR14A4dM+lmGJ2woNfco6zn+Rm3Ktv5t8IK05ujOxyafpPpayxkrfbXRPAh8 XLl6hK1hSJ41FPpZtZLG5PcG8vbd+bC4etX3RemcEy3gT+cu3Z1H0SGH1D6/DMaeLB/S a/ycZdNu6lQgbhDe13ZwaTNOaOWIWRPzRmVzc= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=WG3HBPpUGn9DDKKUpA5tfiQHZCqIZN6nfuk/xwEgU2E=; b=gdIcspRewGmcyp0xQK8iIAnCrHwWbXw/drN2SYgUM0glI6xyELvgTMZY3X9JIlRTmz rXbamwxF4Jp1jwgmmylX1akB4B2fRjvWkWCFhQrPz72KQqE+BYEDUgV2++XRxn9ZKXn7 LekBCUgtqT3cYwLZnz81xiJELI5MU1da4K7rcwKvDLMbpMWIA5HOZjTDfqhQY2EUabLc qU5j+S2UEybcUFH3CMWjBOEtnOz7O/iiN7wWicxFuNwTFAQ7I+cZHnbLByxusyyH1YZo 7Z9+sMPGM7pnF5I89qp1u2y6q75ypeZ0U7FbEv4+Sc0IItdUqewuxuyL9ENRaEptSANa dntA== X-Gm-Message-State: AOAM531fGjLdo5cl5Nbp8MeKr/U6oBgjI6zfivFcVU/sxBi1sL7Z0vih oB8FBUuURChXdAqvuxuJ0lb1diwrijUK3A== X-Google-Smtp-Source: ABdhPJyF3vq9td0F2iUkVktP4T4awjFkKmq67i+1kjkWMjW4vjMldjMQvPw8iYt/J/rBzj/GGTqjHg== X-Received: by 2002:a05:622a:198a:: with SMTP id u10mr19529653qtc.36.1637173478672; Wed, 17 Nov 2021 10:24:38 -0800 (PST) Received: from bill-the-cat (2603-6081-7b01-cbda-a8a6-c29d-3d45-57ea.res6.spectrum.com. [2603:6081:7b01:cbda:a8a6:c29d:3d45:57ea]) by smtp.gmail.com with ESMTPSA id o126sm300957qke.11.2021.11.17.10.24.36 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 17 Nov 2021 10:24:37 -0800 (PST) Date: Wed, 17 Nov 2021 13:24:35 -0500 From: Tom Rini To: Michael Walle Cc: u-boot@lists.denx.de Subject: Re: [PATCH 10/10] board: sl28: disable random MAC address generation Message-ID: <20211117182435.GC24579@bill-the-cat> References: <20211115224551.3549744-1-michael@walle.cc> <20211115224551.3549744-11-michael@walle.cc> <20211116211442.GS24579@bill-the-cat> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="wx5xIorIUvKY4Jz9" Content-Disposition: inline In-Reply-To: X-Clacks-Overhead: GNU Terry Pratchett X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.35 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.2 at phobos.denx.de X-Virus-Status: Clean --wx5xIorIUvKY4Jz9 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Nov 17, 2021 at 05:45:58PM +0100, Michael Walle wrote: > Am 2021-11-16 22:14, schrieb Tom Rini: > > On Mon, Nov 15, 2021 at 11:45:51PM +0100, Michael Walle wrote: > >=20 > > > Nowadays, u-boot (when CONFIG_NET_RANDOM_ETHADDR is set) will set > > > enetaddr to a random value if not set and then pass the randomly > > > generated MAC address to linux. > >=20 > > First, for clarity I'm not nak'ing this. I kind of would like to see a > > slight reword as I think some things aren't 100% correct, even if the > > "save random MAC to ethaddr environment variable" change goes in. For > > example, it's quite long standing that (dev|pdata)->enetaddr populates > > "mac-address" and "local-mac-address" and it seems in some older cases > > we only set the "local-mac-address" property. >=20 > fdt_fixup_memory() in common/fdt_support.c does a env_get(mac). Whoops, yes, I misrecalled this. > I'm not even sure, if there is a connection between what is fixed up > in the kernel DT and the corresponding device in u-boot if > CONFIG_DM_SEQ_ALIAS is not set. And then yes, the logic here can be further fragile at times with multiple interfaces. > > > This is bad for the following reasons: > > > (1) it makes it impossible for linux to detect this error > > > (2) linux won't trigger any fallback mechanism for the case where > > > it didn't find any valid MAC address > >=20 > > This feels in some ways to be a limitation of the binding: > > https://www.kernel.org/doc/Documentation/devicetree/bindings/net/ethern= et-controller.yaml >=20 > Agreed. But it doesn't render my argument invalid ;) "Bug" or "feature", but yes, OK. > > And it reads like we really must be populating "mac-address" with that > > random one and while providing a blank "local-mac-address" would be a > > way to say we don't know the true device one, it seems that wouldn't be > > used / noticed? >=20 > I guess it will just use the one populated in mac-address, i.e. the > random one. Btw, the binding says "last used" what if u-boot never > actually used that ethernet controller, should it then be empty or > populated with the random mac address. My gut feeling here is that we have an example of a binding that has been formalized around what exists in the wild, and was not strictly intentional and clear at all times. What we do today in the case of ethernet devices we haven't used is populate both, just like the one we have used, based on the environment. And popping over to current Linux kernel git, the handful of boards that hard-code a non-zero mac-address or local-mac-address in the dts just makes things even less-clear on what to do. Which probably leaves us with "populate mac-address with whatever you can". > > > (3) a saveenv will store this randomly generated MAC address in the > > > environment > > >=20 > > > Probably, the user will also be unaware that something is wrong. He > > > will > > > just get different MAC addresses on each reboot, asking himself why > > > this > > > is the case. > > >=20 > > > As this board usually have a serial port, the user can just fix this > > > by > > > setting the MAC address manually in the environment. Also disable the > > > netconsole just in case, because it cannot be guaranteed that it will > > > work in any case. After all, this was just a convenience option, > > > because > > > the bootloader - right now - doesn't have the ability to read the MAC > > > address, which is stored in the OTP. But it is far more important to > > > have a clear view of whats wrong with a board and that means we can no > > > longer use this Kconfig option. > > >=20 > > > Signed-off-by: Michael Walle > > > --- > > > configs/kontron_sl28_defconfig | 2 -- > > > 1 file changed, 2 deletions(-) > > >=20 > > > diff --git a/configs/kontron_sl28_defconfig > > > b/configs/kontron_sl28_defconfig > > > index af907175f1..7fb6bdbe82 100644 > > > --- a/configs/kontron_sl28_defconfig > > > +++ b/configs/kontron_sl28_defconfig > > > @@ -59,8 +59,6 @@ CONFIG_OF_LIST=3D"" > > > CONFIG_ENV_OVERWRITE=3Dy > > > CONFIG_ENV_IS_IN_SPI_FLASH=3Dy > > > CONFIG_SYS_REDUNDAND_ENVIRONMENT=3Dy > > > -CONFIG_NET_RANDOM_ETHADDR=3Dy > > > -CONFIG_NETCONSOLE=3Dy > > > CONFIG_SPL_DM_SEQ_ALIAS=3Dy > > > CONFIG_SCSI_AHCI=3Dy > > > CONFIG_SATA_CEVA=3Dy > >=20 > > Now, an alternate solution here would be to enable these two options > > still and write an ft_board_setup() with: > > ... if ethaddr is in the locally administered pool then > > fdt_delprop(... "mac-address"); > > fdt_delprop(... "local-mac-address"); >=20 > Which would also trigger if a user sets a "locally administered" > mac himself. Even if unlikely, I'm opposed to such magic and > unexpected behavior. Sooner or later it will bite you. A fair point, yes. > > And that should cause the kernel to fall through the cases to find out > > where to get the real MAC from. I'm not super happy with this at first, > > but I also don't see anything clever in the binding we can do. >=20 > If I'll need this one again, then I'll just add the random mac > generation in board_eth_init(), I think. OK. --=20 Tom --wx5xIorIUvKY4Jz9 Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQGzBAABCgAdFiEEGjx/cOCPqxcHgJu/FHw5/5Y0tywFAmGVSNwACgkQFHw5/5Y0 tywZsgv/WNvPWyuydo1AOFWnQMW0aBGmFHcfV6+oYCMlm5IXtmqqW3gK8RKd96Xp jI6rnTS36vX1VKYCe5g3gsubkX+2dqA6qg39I2qFomxmsNau7n3jZfZZowija8wi 24pKAA9k7zMY2er5NvNwLW0ADl2+wGmnp0hbT6djeDiOTguJIir+ZfVEfovgTDyO vGTb9WAOHE/vR34tMM+6j1cKM7xQKQ3BF3MteqqB5m0e19uiAMB3P6N7qhxCBJWa W6n0KbxW97r7p0AqXiZM6mIpEydaUntrsEfT0GEnVbFFh/3/O6+GjSZpR2MMUmsW jvGfljhiN8+kNFy70g1jfKBo20CwDbvY3BQCM48ow7j0uEXjRsbMCDsDQL8zDwMI F2EG0+yk6+kBgL7IU8WRMMpp878icFiXhxmN4038Cc+Prc7dTe/CcsRtnJNascJg 8sGQn+nrRr2cW60VZtgWunl5rJ8iDte/tVO63fN27pEQrrl6IHRrqAcO+pXebIxM U/X8Yg7m =iuQb -----END PGP SIGNATURE----- --wx5xIorIUvKY4Jz9--