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 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 smtp.lore.kernel.org (Postfix) with ESMTPS id 6D1F1C61DA3 for ; Thu, 26 Jan 2023 18:16:38 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id C685A856AF; Thu, 26 Jan 2023 19:16:35 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=pass (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="WQBWoPye"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id E1EF8856C7; Thu, 26 Jan 2023 19:16:31 +0100 (CET) Received: from mail-vk1-xa34.google.com (mail-vk1-xa34.google.com [IPv6:2607:f8b0:4864:20::a34]) (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 5440385463 for ; Thu, 26 Jan 2023 19:16:26 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=konsulko.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=trini@konsulko.com Received: by mail-vk1-xa34.google.com with SMTP id 22so1297543vkn.2 for ; Thu, 26 Jan 2023 10:16:26 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=konsulko.com; s=google; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=9bd99zzTabj1VmQs1MJvQ90c5U2N4tZLyOYNtKD3UAI=; b=WQBWoPyeRr1b1ROyPeQdi9shYA4z5MS1m9Pz6tSHPVog8AyRXvdVYR7mBTrQcEc1II 0qu0xZAB6biJjZtjVkLtIiWGyyf3uSZRjFOgUVSnWOLMXFNp67ibmmsVAmSvqm//OutQ 6DZGbcfyL8I+xj78fyY0eP+SC/gE9QAyPVqVM= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=9bd99zzTabj1VmQs1MJvQ90c5U2N4tZLyOYNtKD3UAI=; b=IV9r9Tvac/Lp2ojXYPjbDMIT/ykbTs1wAByEzRQ0O9E/8LdIkS/a7W6T2M117aDY1z JFRvRJkye9pZs/UVyW4wzDCrNx/1Tn04Uae1d2kcFIUo204yJL4GijUbtDnM61cz94FZ PEzRA+ZoJ2Y5Aa1p8mE6zfbKTGnCpfBHkTuVePms8SePo2O1oSz/g3F29ADZ1miZfNZV we8xBHaBGd6RddDBfQY/ndDzwWaoNMGVWtjuYYujzqENr2uYM/UUCgTkq5Z6VgXUPIDw Bb5gvmmPKipBBUka0cP9mZUavpCyiyfLftxJa/0MHGAClNWJV2N8hhHuoIqhD6e1OL0D uX7Q== X-Gm-Message-State: AFqh2kpIUhnkBiGvBL+l/xOQUtWrpmF9L3ZXcVTmTsyyINcNlVXVIxfn dUL4MblMixmVXSvn3vi99LrbIQ== X-Google-Smtp-Source: AMrXdXs+Vll4+KG5OLmYQ84NpvOEPsN9Bl8f1kvJ9vJy3zYZaDvoWt8w7KN6o+0OrtpsK+xoZd69Fg== X-Received: by 2002:a05:6122:e35:b0:3db:c01:83 with SMTP id bk53-20020a0561220e3500b003db0c010083mr20215788vkb.9.1674756985000; Thu, 26 Jan 2023 10:16:25 -0800 (PST) Received: from bill-the-cat (2603-6081-7b00-6400-3a56-04fb-7eea-5810.res6.spectrum.com. [2603:6081:7b00:6400:3a56:4fb:7eea:5810]) by smtp.gmail.com with ESMTPSA id y140-20020a376492000000b00706b299d014sm1271094qkb.132.2023.01.26.10.16.24 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 26 Jan 2023 10:16:24 -0800 (PST) Date: Thu, 26 Jan 2023 13:16:22 -0500 From: Tom Rini To: Simon Glass Cc: Troy Kisky , "u-boot@lists.denx.de" , "sbabic@denx.de" , "festevam@gmail.com" , "marex@denx.de" Subject: Re: CONFIG_IS_ENABLED vs IS_ENABLED Message-ID: References: MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="nm5nl4STLgXeLVr4" Content-Disposition: inline In-Reply-To: X-Clacks-Overhead: GNU Terry Pratchett X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.39 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.6 at phobos.denx.de X-Virus-Status: Clean --nm5nl4STLgXeLVr4 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Jan 26, 2023 at 11:04:21AM -0700, Simon Glass wrote: > Hi Tom, >=20 > On Thu, 26 Jan 2023 at 10:21, Tom Rini wrote: > > > > On Tue, Jan 24, 2023 at 06:36:00PM -0700, Simon Glass wrote: > > > Hi Troy, > > > > > > On Tue, 24 Jan 2023 at 16:31, Troy Kisky wrote: > > > > > > > > > > > > > > > > ________________________________ > > > > From: Troy Kisky > > > > Sent: Tuesday, January 24, 2023 2:52 PM > > > > To: u-boot@lists.denx.de ; sbabic@denx.de ; trini@konsulko.com ; festevam@gmail.com = > > > > Cc: sjg@chromium.org ; marex@denx.de > > > > Subject: CONFIG_IS_ENABLED vs IS_ENABLED > > > > > > > > Hi Guys > > > > > > > > In a recent debugging session, I stumbled across this line > > > > drivers/mmc/mmc.c: if (CONFIG_IS_ENABLED(MMC_QUIRKS) && mmc->q= uirks & quirk) > > > > > > > > which prevents retries in SPL code, and was causing booting from an= SD card to fail. > > > > So I wrote a little script to print uses of > > > > CONFIG_IS_ENABLED(x) which might need to be > > > > IS_ENABLED(CONFIG_x) like the above one. > > > > > > > > Here it is if you want to try it out. > > > > > > > > git grep CONFIG_IS_ENABLED|sed -n -e "s/\(CONFIG_IS_ENABLED([0-9a-z= A-Z_]*)\)/\n\1\n/gp"| \ > > > > sed -n -r "s/CONFIG_IS_ENABLED\(([0-9a-zA-Z_]+)\)/\1/p" |sort -u|xa= rgs -I {} \ > > > > sh -c "git grep -E 'config [ST]PL_{}' | grep -q -E -w '[ST]PL_{}' |= | git grep 'CONFIG_IS_ENABLED({})'" > > > > > > > > It prints CONFIG_IS_ENABLED(x) uses where there is no SPL_x or TPL_= x. > > > > > > > > BR > > > > Troy > > > > > > > > _______ > > > > And here is the opposite check > > > > > > > > git grep -w IS_ENABLED|sed -n -e "s/\(IS_ENABLED(CONFIG_[0-9a-zA-Z_= ]*)\)/\n\1\n/gp"| \ > > > > sed -n -r "s/IS_ENABLED\(CONFIG_([0-9a-zA-Z_]+)\)/\1/p" |sort -u|xa= rgs -I {} \ > > > > sh -c "git grep -E 'config [ST]PL_{}' | grep -q -E -w '[ST]PL_{}' &= & git grep 'IS_ENABLED(CONFIG_{})'" > > > > > > > > > > > > It prints uses of IS_ENABLED(CONFIG_x) where CONFIG_SPL_x exists. > > > > > > Thank you for that. We definitely have quite a few of these. > > > > > > By a great coincidence I updated moveconfig.py to do something a > > > little like that:. > > > > > > https://patchwork.ozlabs.org/project/uboot/patch/20230123220031.35407= 24-2-sjg@chromium.org/ > > > > I think this also shows that we might really want to just drop the > > checkpatch.pl note about IS_ENABLED / CONFIG_IS_ENABLED, it's getting > > used in a lot of wrong places where it's not helpful. It's not the root > > cause here (where a compile time check that allows for the rest of the > > code to be statically checked still is OK), but it's part of the > > problem. >=20 > Firstly, we want to drop the use of #ifdef so what should we say instead? I'm not sure that dropping #ifdef in and of itself is a good goal. #if IS_ENABLED(CONFIG_FOO) does not read better #ifdef CONFIG_FOO And we have a lot of cases of the former that I'm not sure can logically or helpfully be replaced with if (IS_ENABLED(CONFIG_FOO)) either for functional / legibility reasons (ie it doesn't read better and just getting static analysis isn't a great reasons, more indent makes the code harder to follow) or isn't possible because things like: #ifdef CONFIG_FOO int i =3D CONFIG_BAZ; ... #endif Can't be replaced with if (IS_ENABLED(CONFIG_FOO)) { ... } when CONFIG_BAZ is only ever asked when CONFIG_FOO is enabled. And we can't define CONFIG_BAZ to 0 if not set (that leads back to introducing CONFIG things being defined). > Secondly, I think we should fix all this by splitting the config, > along the lines of my old series [1]. I hit similar problems to Troy > and have modified moveconfig.py to detect these (hence my recent > series [2]). I will see if I can get some sort of series out by > Monday. I had something pretty close(TM) but it failed on a few qemu > tests [3] I don't disagree with seeing what things look like with a "split" config again, but I think that fundamentally Yamada-san was right way back, and we really need to move towards separate config files for each stage and then combine them when applicable. That's also not easy, but I'm also not sure how to deal with the cases where we intentionally have CONFIG_FOO=3Dn CONFIG_SPL_FOO=3Dy without doing what we're doing today, at some level. > Troy, are you able to create patches for the problems you find, and > perhaps adding your checks to a Makefile rule or script so we can run > it in CI? What we're doing today does need to be analyzed and some good number of cases corrected I am sure. --=20 Tom --nm5nl4STLgXeLVr4 Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQGzBAABCgAdFiEEGjx/cOCPqxcHgJu/FHw5/5Y0tywFAmPSw3AACgkQFHw5/5Y0 tyxw0Qv/XsOFhSCp9cSjBWDwMJBEUOUfcAHItjsvK1CIy9lOXDdauvz8MwrNwcCO 1//3xmwfE1RlpM3wfnEfUn/L02oEog6pT9fl6KcQgyAnAd2uVVSzWQRw0IWkaMB+ oQ1nO7KkZkIHTWH9VTjHNnoVC6/jWRfpC3AeRliwLs+H6IDLlmJONcsiHBtzJ1lc cNZIAvOgQyzI5UxJFH2ly5thWBT3K8qUGRoRs1cmEwYRgXKrnDsVAa9+R/NdcBI8 ZQDZelmL4MLGartVAlaO5nOs8cuKoMNKEodZvs1Wn2gUzydCd/LAyTI67Q8+4Ew9 YT+Y9pNJ+Ziq04hS2m4Ys3IcJmtZUgH9Uo05tt2/4L7sFh3vB+7UXmY4zIf0B5/Z xDo+G+bkzyA0H7O8RJYmWJSQpfTERW/9BG8gP6HEIr9dAhZ1y8aefcoOSdzf0oLo AJJ3yTWidiq/8SEVWCEMbsOqwit1iDCvPiRi59pK98RZfkuXKBrW/HZteBRDvhXA VhlpIYml =/KrI -----END PGP SIGNATURE----- --nm5nl4STLgXeLVr4--