From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wm1-f54.google.com (mail-wm1-f54.google.com [209.85.128.54]) (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 E4608347C6 for ; Fri, 10 Apr 2026 18:45:42 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.128.54 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1775846744; cv=none; b=I2L9rIqc7WZhmVHfZhTuGNvB2C1qeyRsDlMJytf/u4aDY2j90Emag7feWSIaP3ATCdGKACDDyoUbOgljE34rtHdpP9flLdPwJU/Qh4pez4D3T9hLxb7qvWgZwzHl0+jBPJrJeQmEHJ/kk6aD+IxB+xNvMX8CSdR85eNMMmrfap0= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1775846744; c=relaxed/simple; bh=obREVtnEXF2ex30wGp4AwtsbnB9TwULh6tC8lvy+Vy0=; h=Date:From:To:Cc:Subject:Message-ID:In-Reply-To:References: MIME-Version:Content-Type; b=MCmM2KInF8ggaGh+qcdmamhLO2zqeR9WhYu0HP7Mo9ZVPQivVlgQtNOe18MIQKngqKhXQ7OxQq9DL8yrD7NdapB5uE7arMRMOagD8QN1OCKOM5E45sG7E1VDPaC7bb0OwHCjPkWgEq9gNRtSG9Oq/pmnroBqt4kKHh2ljypZM8c= 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=RBfmtFmg; arc=none smtp.client-ip=209.85.128.54 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="RBfmtFmg" Received: by mail-wm1-f54.google.com with SMTP id 5b1f17b1804b1-488a041eae5so17017675e9.1 for ; Fri, 10 Apr 2026 11:45:42 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20251104; t=1775846741; x=1776451541; 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=uKMmMKoadEvaxs9k303oMR0jtDn6IiRciNDbjt6eSSQ=; b=RBfmtFmgVKM/XIeu1QlXElC3kkhmzgL5azXWbsk5rlqs98ikmnOc3Qag4HJcE78Jg3 mtJzFxh8q6etJ8L7/nJS2+DS/Y8dk/v3r/Y0pDZ/38quvYgg5pqXOy8YVYN2dN47/84t BuosQIiGcXGCQcMs8tpnxO9TA7SK303jCeT8abLIY25ouX0Sx8V2oKDbl1Q1HLJYCHWh 1lZ7mpXaqlaQxbVAYS1HS03Y3tj87ihaA1QcfYDZDo6D/ymKFXOxtuL9BhUVLggcrewc mq/wBWT33JnYnqHmipFkiJEI6JxmEzyGRDD1zXJt4hkAMMlswsxfX04JPYnoD9Y2AQH8 LpIw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1775846741; x=1776451541; 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=uKMmMKoadEvaxs9k303oMR0jtDn6IiRciNDbjt6eSSQ=; b=D3e3vohP75+zYBlQz5gkoVDoT+9kzyvcsTQmFxDK3o9x3sSwYmEWvDPWG5JCYf7nb5 fKCHOVUtda4tyIzKRJmzdz28RC0TXOyvalOJZNoWErmJAIIIjqETJ+uupmd39tq8kxOy O+xr12hYeY/6+fipnVwlrcH75tj8z4uznsLwARZlENaRSNpkc6r/y+AlFA52mGz067gV bHPoa1KchaOubIYL9IlvurIUskCljGzhjoNDOWH/CIg5FQOgpPAcohHVN2ybn6qkwqfU HKORv4Y1Wj95gr9w3iy/4+KcB5w9z2LtELGNnlEyNPZOohD3NKzrHg+DuHVTuOyFnUad RTWQ== X-Forwarded-Encrypted: i=1; AJvYcCUitKyxc8XAt11ByiAEErFD5u8GvtqphkQB/jZtUNy1pCZ8wVl9J0ejoe6wF5xCK/KGyXpnnK2HFKvjwuk=@vger.kernel.org X-Gm-Message-State: AOJu0YwMSHd51QFyOvovNZ2ZZFW7xZxP9A2zWO8lXum8qPMtjWSPEaZ4 E4TzHBkjeZMRfiefRk1HEGbs7Gcotr063E5bylmq4OxeEfmeaY1wbDkk X-Gm-Gg: AeBDievGmDS3KUj3nWCevY5yg9MGg8A4J6BxY6SwUFeQL0vKYd4AvMjEBBN0oU6dcE1 5NR6xY2w1TO3OQ0/aREZp75/DcW39xIArTy0UVnAuc2sUCs5hBCTJL2Q3coSJvy8Sl77/3UHdZG l0hwcSSTweY4P72CIKqPbPK5dBoIKIvjmEiJJfcQk+4gxP5MbDb4AEgQqpR6yDokrXZgdBKeeUQ RCX7ajMsHZ6AUda1tMZt9xSDeTjBXuJuhi1FHgDs1SKMntSxc4XVernKrnjwUcvFWvLN7VXNpQ5 96MgmCe8zNRVDCm4zb1ruIwXzIf65m/mymz8Ql+iz89s5RykaF50EjYqkVlMtV5F0xer7iD9VFr /Mzxrr8d6oHqqZLzFBwQMTtWWI5ZYBlBdbk81l6JlqVjuYaYeePjLsornmYHquL0T5Ms7cB26wi 5HVb4ZV5x96XaviPMsBQDggWDcwMdQwDPxMca+HsWB+vs0Tqxw0UBzjiCgMSPQAONkHwPzPOdnf Rg= X-Received: by 2002:a05:600c:a109:b0:488:b239:77ec with SMTP id 5b1f17b1804b1-488d683d510mr42314985e9.17.1775846740893; Fri, 10 Apr 2026 11:45:40 -0700 (PDT) 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-43d63e469ddsm9283383f8f.17.2026.04.10.11.45.40 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 10 Apr 2026 11:45:40 -0700 (PDT) Date: Fri, 10 Apr 2026 19:45:38 +0100 From: David Laight To: Yury Norov Cc: Geert Uytterhoeven , Alexandre Belloni , Jonathan Cameron , Crt Mori , Nuno =?UTF-8?B?U8Oh?= , Richard Genoud , Andy Shevchenko , Yury Norov , Rasmus Villemoes , Matt Coster , open list Subject: Re: [PATCH 1/1] bitfield.h: Ensure FIELD_PREP_CONST() is constant Message-ID: <20260410194539.560b5fdc@pumpkin> In-Reply-To: References: <20260410090927.60484-1-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-kernel@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 Fri, 10 Apr 2026 12:55:25 -0400 Yury Norov wrote: > + Matt Coster > > On Fri, Apr 10, 2026 at 10:09:27AM +0100, david.laight.linux@gmail.com wrote: > > From: David Laight > > > > Some versions of gcc report that some expressions involving > > __builtin_ffsll(1ull << 63) are not integer constant expressions. > > > > Rework FIELD_PREP_CONST() to avoid the issue. > > > > Signed-off-by: David Laight > > Thanks, David. > > So, the original patch that spotted the problem is: > > https://lore.kernel.org/all/20260409-field-prep-fix-v1-1-f0e9ae64f63c@imgtec.com/ That isn't the patch that spotted the problem. That is a suggested fix. I've not seen the code (or patch) that actually exhibited the issue. I think someone changed (drivers/gpu/drm/imagination/pvr_device.h): #define PVR_PACKED_BVNC(b, v, n, c) \ ((((u64)(b) & GENMASK_ULL(15, 0)) << 48) | \ (((u64)(v) & GENMASK_ULL(15, 0)) << 32) | \ (((u64)(n) & GENMASK_ULL(15, 0)) << 16) | \ (((u64)(c) & GENMASK_ULL(15, 0)) << 0)) to use FIELD_PREP_CONST(). The top line might be FIELD_PREP_CONST(GENMASK_ULL(63, 48), b)) which has the MSB set on the call to __builtin_ffsll() which seems to be necessary (but not sufficient) to fail. The expansion of that is used (pvr_device.c) in: static enum pvr_gpu_support_level pvr_gpu_support_level(const struct pvr_gpu_id *gpu_id) { switch (pvr_gpu_id_to_packed_bvnc(gpu_id)) { case PVR_PACKED_BVNC(33, 15, 11, 3): case PVR_PACKED_BVNC(36, 53, 104, 796): return PVR_GPU_SUPPORTED; case PVR_PACKED_BVNC(36, 52, 104, 182): return PVR_GPU_EXPERIMENTAL; default: return PVR_GPU_UNKNOWN; } } > > Let's keep Matt in the loop, at least. I thought I'd typed his address in - somewhere it got lost. > Please keep the details from the original patch. "Some versions of > some expressions" style is too uncertain. But that is pretty much what it is. I could give a failing test case, but it all reminds me of a bug in ash where: x=$(command args) could lose the last byte of output. Fiddling with the shell script would fix it (never mind changing the compiler options for the shell itself). (Basically is was scanning the on-stack temporary buffer when removing trailing '\n' and could access buffer[-1], if that happened to be 0xa then the length would be decremented. Only likely to happen on BE.) In this case minor tweaks to the expression change the behaviour. This one fails: int t : (__builtin_ffsll((1ull << 63)) < 3u) ? 1 : 2; change the 3u to 3 or the 63 to 62 and it is ok. Until someone supporting gcc actually identifies the underlying bug there really isn't much to say. > > > --- > > > > Note that when 'val' is a variable 'val << constant' is likely > > to execute faster than 'val * (1 << constant)'. > > So the normal FIELD_PREP() is best left alone. > > Do you have any numbers? I'd prefer to have the codebase consistent > when possible. I think the multiply instruction will have a higher latency than the shift. So you are talking about a very small number of clocks if the expression is in the critical register dependency path. However FIELD_GET() would need to use a divide - and that would be a lot worse. Having written that, ISTR that 'mask' is required to be a constant. So the compiler may use a shift anyway - if the divide is unsigned. But for non-constant mask you definitely want a 'shift right'. While you might think that it only makes sense to use unsigned values, I've found one piece of code (IIRC in the x86 fault handler) that passes a signed value to FIELD_GET() and needs the result sign extended. So, unless that is changed, FIELD_GET() must use an explicit right shift. (Of course, right shift of negative values is probably UB...) David > > Thanks, > Yury > > > include/linux/bitfield.h | 9 ++++----- > > 1 file changed, 4 insertions(+), 5 deletions(-) > > > > diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h > > index 54aeeef1f0ec..f21765851d5c 100644 > > --- a/include/linux/bitfield.h > > +++ b/include/linux/bitfield.h > > @@ -45,6 +45,7 @@ > > */ > > > > #define __bf_shf(x) (__builtin_ffsll(x) - 1) > > +#define __bf_low_bit(mask) ((mask) & (~(mask) + 1)) > > > > #define __scalar_type_to_unsigned_cases(type) \ > > unsigned type: (unsigned type)0, \ > > @@ -138,8 +139,6 @@ > > __FIELD_PREP(_mask, _val, "FIELD_PREP: "); \ > > }) > > > > -#define __BF_CHECK_POW2(n) BUILD_BUG_ON_ZERO(((n) & ((n) - 1)) != 0) > > - > > /** > > * FIELD_PREP_CONST() - prepare a constant bitfield element > > * @_mask: shifted mask defining the field's length and position > > @@ -157,11 +156,11 @@ > > /* mask must be non-zero */ \ > > BUILD_BUG_ON_ZERO((_mask) == 0) + \ > > /* check if value fits */ \ > > - BUILD_BUG_ON_ZERO(~((_mask) >> __bf_shf(_mask)) & (_val)) + \ > > + BUILD_BUG_ON_ZERO(~((_mask) / __bf_low_bit(_mask)) & (_val)) + \ > > /* check if mask is contiguous */ \ > > - __BF_CHECK_POW2((_mask) + (1ULL << __bf_shf(_mask))) + \ > > + BUILD_BUG_ON_ZERO((_mask) & ((_mask) + __bf_low_bit(_mask))) + \ > > /* and create the value */ \ > > - (((typeof(_mask))(_val) << __bf_shf(_mask)) & (_mask)) \ > > + (((_val) * __bf_low_bit(_mask)) & (_mask)) \ > > ) > > > > /** > > -- > > 2.39.5