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 2B5E3C05027 for ; Thu, 26 Jan 2023 21:34:04 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 18296856B9; Thu, 26 Jan 2023 22:34:01 +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="MQ5BFaeV"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 5B97F85559; Thu, 26 Jan 2023 22:33:59 +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 8D37B85650 for ; Thu, 26 Jan 2023 22:33:52 +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-qt1-x82d.google.com with SMTP id e8so2514936qts.1 for ; Thu, 26 Jan 2023 13:33:52 -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=YEPHPFZyLiwKdZ9EFDuWjeJTAfkJW9pTtuNTc/QwxPU=; b=MQ5BFaeVNQ6gxeXXCUmc474QELheMOFhXzSdwSm3DzJrqViFpWlI/wzUj6tb7Kzg6I 4OfbIJ6t8CQ17VDGKu1z9eUtHp1zfagGpsyMgVU1nikjjnVK3kHrFXUitMOf3LUhJ5nR rle1T6NCAY8p7EMvWolpVV5lBkSd89spfbVnI= 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=YEPHPFZyLiwKdZ9EFDuWjeJTAfkJW9pTtuNTc/QwxPU=; b=Nys2WDvV1GK9eLjGfisasH+7WI6o3uB9niDHE+cb1ls8XtsCYQ5c8pNLYTmeYZs3Md oo/XagbkEGgvVHW5qHh96Rb7KH01F1j4krkDC7c6NvoT9JCQvsY+CBFd4RAzBkDV6mtM +Z3x6Kky8xpzwCfu2W0oPsbaXNm5vYFM6wtg6s6V/5FAsjiVr4rctdF/G2HpQV7h2ejV mF48OJwQXxU4VXTFWUz+E7u3yMuUs8wPVTxBoIe8n7m3672LkVS7rL0lZ1TY1wAIRVO5 9plHnrBal5KhDPAtkuAou21Xd5k/Qjw3aMNMQTS4klLZWbVOWanOeCJUZV3vtGDLQNFk fv3w== X-Gm-Message-State: AFqh2kry35px0GxegjFoM7F5gFXkg3eLCaU3F0RDYek+W8ncVYLK5X7O 5EtiEdHcq6h8sjf8owoqlJCc1Q== X-Google-Smtp-Source: AMrXdXuCzPQKfZF0UY5RlGJXeoe1uHnOSpaJRT/jauPKDnJf0aOgo27nGQgOdDU4UVPR5Yzp0AafEA== X-Received: by 2002:ac8:7353:0:b0:3b6:8ac4:4a41 with SMTP id q19-20020ac87353000000b003b68ac44a41mr42425224qtp.32.1674768831224; Thu, 26 Jan 2023 13:33:51 -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 u12-20020a05620a454c00b006fcab4da037sm1670903qkp.39.2023.01.26.13.33.50 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 26 Jan 2023 13:33:50 -0800 (PST) Date: Thu, 26 Jan 2023 16:33:48 -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="7ikoPqfK4i+kRlq2" 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 --7ikoPqfK4i+kRlq2 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Jan 26, 2023 at 02:29:53PM -0700, Simon Glass wrote: > Hi Tom, >=20 > On Thu, 26 Jan 2023 at 11:16, Tom Rini wrote: > > > > On Thu, Jan 26, 2023 at 11:04:21AM -0700, Simon Glass wrote: > > > Hi Tom, > > > > > > 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) && mm= c->quirks & quirk) > > > > > > > > > > > > which prevents retries in SPL code, and was causing booting fro= m 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-zA-Z_]*)\)/\n\1\n/gp"| \ > > > > > > sed -n -r "s/CONFIG_IS_ENABLED\(([0-9a-zA-Z_]+)\)/\1/p" |sort -= u|xargs -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-z= A-Z_]*)\)/\n\1\n/gp"| \ > > > > > > sed -n -r "s/IS_ENABLED\(CONFIG_([0-9a-zA-Z_]+)\)/\1/p" |sort -= u|xargs -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 exist= s. > > > > > > > > > > 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.3= 540724-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 getti= ng > > > > 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. > > > > > > Firstly, we want to drop the use of #ifdef so what should we say inst= ead? > > > > 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 >=20 > So far in my prototype I have implemented >=20 > #if CONFIG(FOO) >=20 > and >=20 > if (CONFIG(FOO)) >=20 > which replaces direct use of CONFIG_FOO and also > CONFIG_IS_ENABLED(FOO). We could ban use of CONFIG_FOO easily enough. I'm not convinced #if CONFIG(FOO) is better, but OK. > > 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). >=20 > We have IF_ENABLED_INT() so can do things like that And could come up wit IF_ENABLED_STRING() I suppose. But I'm still not sure it'll read better / more clearly. > > > 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. >=20 > Well let's see what you think once I get this next version of the series = out. OK. > > 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. >=20 > Well I believe these are bugs, but don't know how many. Will probably > have some idea by early next week. There's certainly some bugs, but there's also some intentionally done ones, such as around SKIP_LOWLEVEL_INIT --=20 Tom --7ikoPqfK4i+kRlq2 Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQGzBAABCgAdFiEEGjx/cOCPqxcHgJu/FHw5/5Y0tywFAmPS8bkACgkQFHw5/5Y0 tyy8nwwAlLS4YpEWHxVuBWsscrukxkzap9+GmsZWylFzjzimnPrbjjIbt6B1wb2T NG74YMQKKVZWbjVPQw8XRNNl71ATX6cRJTQvUHNBsI7hmcihOzz+Ghk8K3OABi5Z pLlce0k+KVXG6h4k0SkjuZSIvn3Xb7yNez35zoUBu5oT33xdD7z5dG1hDY6HF+iK 52KyQdEckJpnl7T9pSBFHuiF7dMVXCqLUKdyef4B+rlRjvBz9v81xxP8YIRULf8+ 8HdFOC7farnupp4WGchaytkSGo44UhQYjRQ7Ym34cTIlki9g9PaZIz4J56b8GLn2 UVty9mkyrMVOF/2fq68YA7Xy7hYqdxaj11uJWyNiq9tdcdQicS8pK59W/+Q+PmKc /WmE/vM6HeBRXpCeAWAHdlZc0lXRjMztP4XvLCWal1qCAH+0Kqee/ZAffyayZkHl 1Cxr4XIegIY0aFsJc4euiVTp872fQLy9ECD/khEalVTjOONxPwGSpB01oPvElGIg lOoN3v5V =L1eX -----END PGP SIGNATURE----- --7ikoPqfK4i+kRlq2--