From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wm1-f45.google.com (mail-wm1-f45.google.com [209.85.128.45]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 40EFE303CB6 for ; Tue, 9 Dec 2025 19:11:54 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.128.45 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1765307516; cv=none; b=piMLtk8y4KpzG0ND1mDzWtgJPdAkBK/xzZNZaOaW/+jXgY/bhOGb/qqbCoHZwWvp3/L0Pb0srHeR6nD5oUevHdZ7Fh0jRJhE0iqxO6K7LTXJNNd0zFXwCHyH9aL/Z96UjerD0j9jnNqexYblSDdq+owqT/NAHrqS/lMR7BRGraY= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1765307516; c=relaxed/simple; bh=Vigss2VmDX7bEdaP1IC8CfMj88h+qnBHbPLRY6X9h5g=; h=Date:From:To:Cc:Subject:Message-ID:In-Reply-To:References: MIME-Version:Content-Type; b=BRbIT8ygzWhYBUh8H9hmmE469tdwCgPGHYOGA3gzeDwR0v9E1ED/uoAdhMGkJOJipRCOZN2WEqqwPxEMhWAgvApd5K0rmWCMAAsnLmsWnEPH4zSEUVBzPK16yxAF8pbeF047+eN5AUI4vYi6HAJkClbgnv3bXuETHPK1Ss6h88k= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com; spf=pass smtp.mailfrom=gmail.com; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b=V2x8JN5a; arc=none smtp.client-ip=209.85.128.45 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=gmail.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="V2x8JN5a" Received: by mail-wm1-f45.google.com with SMTP id 5b1f17b1804b1-4779cb0a33fso79698505e9.0 for ; Tue, 09 Dec 2025 11:11:53 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1765307512; x=1765912312; darn=vger.kernel.org; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:subject:cc:to:from:date:from:to:cc:subject:date :message-id:reply-to; bh=7VrojYWmad2/hXGEw6Ve6lcVoq3UDRgDrKieHhrBvuI=; b=V2x8JN5aduJWBdc8uCNsZyEklP+C0uNfe+QvXzXdSWGvPA5xiAIQL5Z0UmQMFUmDvy rl7dk1XxmNLpyMDbbo8ew3yoLhZPEC7VUrntT3JNTRqWzTEg22Ee4qtHJaiIRiVUDkhz Fw9t6SxoD2kuWNJEqbX93PUy3cB79ZgtEFh7dwI3SLcnxSy/RkbUH4BkQx/3Gl3VXd25 bxRY5grX8OSWpzIvyRnyJjxze080GeypAE5qKRiKqw8mZaijNarH1I4hh7U3kot3lCS2 3x+iPvKJemi5fbenOq/INQyYPzNKFOxfHPNoCfHXPGdwniPI4CqM8RAwUZRrDk1OOM9H vg+g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1765307512; x=1765912312; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:subject:cc:to:from:date:x-gm-gg:x-gm-message-state:from :to:cc:subject:date:message-id:reply-to; bh=7VrojYWmad2/hXGEw6Ve6lcVoq3UDRgDrKieHhrBvuI=; b=OiOJwsc0JM3+tiTDu/064xMu/4CcHK42TivhQPgcCQ0Bx1lw6c2rMyTpkaowfUSjxU RqBfl7zyzR3Vmn93bycde2O3taTTc6VGl/sK5Uu88NEQvqvz0rv1NCqp4iQEbS04iQRQ KxgiTBw5SqRRARMM4dYTMz+nI8quZtTWmeCfWe7UbQFp+qm1zfXXx1BhSQ9gKtN94z5f 1KsFN0a2h4JG5iqf0XCw24A2apR/DX6mAXcoWzO11IE5aMitpfZc/T0lJF3y/pik6Onx rWfIzVOVhLalGgLPtvv9i22rOAMBokNicU1gsI6obTIhXpjI7PzixX/5f55Vr0krGehS PpGA== X-Forwarded-Encrypted: i=1; AJvYcCW/LQJXgWQjBPyxBlx3eRJ9kfurubVKtHbB31G7Sv3G57UU42TXo0/kLOzHmLqav1aotZDgMuJuc0Q=@vger.kernel.org X-Gm-Message-State: AOJu0Yyw9tUq0bS7AoKaOAM8s75JFSQa84ngZVsYDd3BQT1wH2F6IiN0 QXSDiRuBt/mSivPtVEgGNAfz7mVSWGdpOCA1Hd8C8gVfCXQkX2U9KSHw X-Gm-Gg: ASbGncsBJ2w8SetjITCuMRmUSH92+5K1OdxHTBP6au3fmTVaKsm0OOzPNqiVEAfcTJG YpQqcjFKbx1KpUeNvrcBZr7UUQDWSv4wyOg7d67O/LQpCCgQs4XlAkCJuSW4dXHFKQ0b0L+yOWe oKAN+jXu/qKq3lOpPZv79oI84/hK8A6xcrkznocDFsptPmS8f4XtoG4kdiW5qGA+Mk1N17Dhbgt Vx9Hg7zp+uVx/NLFY7zklMtFPh2Wzu1flUpHRVHByWvZxXo9LEbQqwuRduoOZCtwsEeBs0vVdrx +FxWEj44jInU6jcobbH90qoL/aV7wT6sDlkIkKU8vKT3OCaXHTdSIwhsRHFfx1NxEKzGU06Rz4h yDyN/2srgdWGBMsbJlis4Mfz3HO6PR9gHucaMkWH449dIQVnBbgM8CZgn3V3hsupEDxZfWN74IE 9sMYRP5uCGumvqPmDHS0Fg186jmTHc+YNS4IZpANQKGJzHWEBuHoNL X-Google-Smtp-Source: AGHT+IF6wLKEoqKWHUIso/KMV2us/t6CixsT57DQtOhmkn/Y6ES7LiNIjvfalect3nhhJyAITGuBxw== X-Received: by 2002:a05:600c:37c3:b0:46e:35a0:3587 with SMTP id 5b1f17b1804b1-47939e37b60mr131029595e9.27.1765307512383; Tue, 09 Dec 2025 11:11:52 -0800 (PST) Received: from pumpkin (82-69-66-36.dsl.in-addr.zen.co.uk. [82.69.66.36]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-42f7d222478sm32697794f8f.20.2025.12.09.11.11.50 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 09 Dec 2025 11:11:52 -0800 (PST) Date: Tue, 9 Dec 2025 19:11:48 +0000 From: David Laight To: Andy Shevchenko Cc: Yury Norov , Rasmus Villemoes , linux-kernel@vger.kernel.org, linux-usb@vger.kernel.org, Geert Uytterhoeven , Alexandre Belloni , Jonathan Cameron , Crt Mori , Richard Genoud , Luo Jie , Peter Zijlstra , Jakub Kicinski , netdev@vger.kernel.org, "David S . Miller" , Simon Horman , Mika Westerberg , Andreas Noever , Yehezkel Bernat , Nicolas Frattaroli Subject: Re: [PATCH 4/9] bitfield: Copy #define parameters to locals Message-ID: <20251209191148.16b7fdee@pumpkin> In-Reply-To: References: <20251209100313.2867-1-david.laight.linux@gmail.com> <20251209100313.2867-5-david.laight.linux@gmail.com> X-Mailer: Claws Mail 4.1.1 (GTK 3.24.38; arm-unknown-linux-gnueabihf) Precedence: bulk X-Mailing-List: linux-usb@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit On Tue, 9 Dec 2025 17:51:48 +0200 Andy Shevchenko wrote: > On Tue, Dec 09, 2025 at 10:03:08AM +0000, david.laight.linux@gmail.com wrote: > > > Use __auto_type to take copies of parameters to both ensure they are > > evaluated only once and to avoid bloating the pre-processor output. > > In particular 'mask' is likely to be GENMASK() and the expension > > of FIELD_GET() is then about 18KB. > > > > Remove any extra (), update kerneldoc. > > > Consistently use xxx for #define formal parameters and _xxx for > > local variables. > > Okay, I commented below, and I think this is too huge to be in this commit. > Can we make it separate? I originally wrote a much longer patch set, then merged some to reduce the number of patches - you can't win really. > > > Rather than use (typeof(mask))(val) to ensure bits aren't lost when > > val is shifted left, use '__auto_type _val = 1 ? (val) : _mask;' > > relying on the ?: operator to generate a type that is large enough. > > > > Remove the (typeof(mask)) cast from __FIELD_GET(), it can only make > > a difference if 'reg' is larger than 'mask' and the caller cares about > > the actual type. > > Note that mask usually comes from GENMASK() and is then 'unsigned long'. > > > > Rename the internal defines __FIELD_PREP to __BF_FIELD_PREP and > > __FIELD_GET to __BF_FIELD_GET. > > > > Now that field_prep() and field_get() copy their parameters there is > > no need for the __field_prep() and __field_get() defines. > > But add a define to generate the required 'shift' to use in both defines. > > ... > > > -#define __BF_FIELD_CHECK_MASK(_mask, _val, _pfx) \ > > +#define __BF_FIELD_CHECK_MASK(mask, val, pfx) \ > > ({ \ > > - BUILD_BUG_ON_MSG(!__builtin_constant_p(_mask), \ > > - _pfx "mask is not constant"); \ > > - BUILD_BUG_ON_MSG((_mask) == 0, _pfx "mask is zero"); \ > > - BUILD_BUG_ON_MSG(__builtin_constant_p(_val) ? \ > > - ~((_mask) >> __bf_shf(_mask)) & \ > > - (0 + (_val)) : 0, \ > > - _pfx "value too large for the field"); \ > > - __BUILD_BUG_ON_NOT_POWER_OF_2((_mask) + \ > > - (1ULL << __bf_shf(_mask))); \ > > + BUILD_BUG_ON_MSG(!__builtin_constant_p(mask), \ > > + pfx "mask is not constant"); \ > > + BUILD_BUG_ON_MSG((mask) == 0, _pfx "mask is zero"); \ > > + BUILD_BUG_ON_MSG(__builtin_constant_p(val) ? \ > > + ~((mask) >> __bf_shf(mask)) & \ > > + (0 + (val)) : 0, \ > > + pfx "value too large for the field"); \ > > + __BUILD_BUG_ON_NOT_POWER_OF_2((mask) + \ > > + (1ULL << __bf_shf(mask))); \ > > }) > > I looks like renaming parameters without any benefit, actually the opposite > it's very hard to see if there is any interesting change here. Please, drop > this or make it clear to focus only on the things that needs to be changed. I'm pretty sure there are no other changes in that bit. (The entire define is pretty much re-written in a later patch and I did want to separate the changes.) I wanted to the file to be absolutely consistent with the parameter/variable names. Plausibly the scheme could be slightly different: 'user' parameters are 'xxx', '__auto_type' variables are '_xxx'. But internal defines that evaluate/expand parameters more than once are '_xxx' and must be 'copied' by an outer define. David