Message ID | 20240124050205.3646390-2-lucas.demarchi@intel.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel+bounces-36446-ouuuleilei=gmail.com@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a05:7300:2553:b0:103:945f:af90 with SMTP id p19csp781542dyi; Tue, 23 Jan 2024 21:03:17 -0800 (PST) X-Google-Smtp-Source: AGHT+IG4NpJbU1jLJErxIKIr5W2b5SkU4MPJfGXaxSmQN2dHKMODtGxcmxxpPzshm11+SrIN1L1I X-Received: by 2002:a05:620a:424d:b0:783:9be1:38f with SMTP id w13-20020a05620a424d00b007839be1038fmr5510413qko.29.1706072597721; Tue, 23 Jan 2024 21:03:17 -0800 (PST) ARC-Seal: i=2; a=rsa-sha256; t=1706072597; cv=pass; d=google.com; s=arc-20160816; b=OluLrKu6Lv5tYwQrHflnxJ8jhYqXjUKHuzA0dQjRi2narRDm+uIx0HGPPvYxWhtBWt PEd94ZPKm0h2vpa8v+bHJu5yMuwNAchnQEnS9+R1mTk2H9NZsau4WGNzNoutF0UL6i/k x4Bm6OsCUB9Li5ukdZjf1XeLs0nY25wqlqlzZJPKibXcRvj+XgUyIGqgwFJC442OODX8 Q7hH7Jdv+rkj2W+B01Ubm30DT7XtP+0c1FCAkeBTNf16hR5ISP4TaGQF6f3l5QIXlTc1 nx4g70W7vURC0eWWRRUOck2kEHUZ7Lsi7QXdhZ0BTgk5Lolj65106leXfm4Rdk6iGwNx xnNQ== ARC-Message-Signature: i=2; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=content-transfer-encoding:mime-version:list-unsubscribe :list-subscribe:list-id:precedence:references:in-reply-to:message-id :date:subject:cc:to:from:dkim-signature; bh=9frCqJNZ9Ebl8UrjoEJV5vlBplYaNAj2sCVd9+6gDP0=; fh=/FcLXTpjOid3TvFfEUjbYIuo/hdxOddxBi/MbWsARlw=; b=pblF3J+tq/LlpydYsNnsU+WeuorY0SJK9sDhfbAEv7d6c3Ar8NLHe+M61Va/5pb04y 1UxzEbtAaUZjeaVtkTniI5TbMoLeI7LSomlHy3y/Cw/+S6R2HDhdMybzBbfyyL687lfj GoLzqn58jKAKjkKdLjCAup2kD3I+XMgNkiDpzeQBI7Kp4LoYiApcXCj9sWc6MDHjUUpD uTZJXlfa+xWTLFA3zYcLODE3Qf5wog1UF8jXsmL3Bsj5t0mXmHGjvMEt3gu+dcAWirsa fAqm7adZEu5Blm48d4kC+f73rcqak+ip09IRg8ArVuY41lvEjo1oEgIzSO0LqXc4l3lH 2C/g== ARC-Authentication-Results: i=2; mx.google.com; dkim=pass header.i=@intel.com header.s=Intel header.b=MrQnhGrA; arc=pass (i=1 spf=pass spfdomain=intel.com dkim=pass dkdomain=intel.com dmarc=pass fromdomain=intel.com); spf=pass (google.com: domain of linux-kernel+bounces-36446-ouuuleilei=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) smtp.mailfrom="linux-kernel+bounces-36446-ouuuleilei=gmail.com@vger.kernel.org"; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=intel.com Received: from ny.mirrors.kernel.org (ny.mirrors.kernel.org. [2604:1380:45d1:ec00::1]) by mx.google.com with ESMTPS id v2-20020a05620a0a8200b00783966e534bsi8306021qkg.11.2024.01.23.21.03.17 for <ouuuleilei@gmail.com> (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 23 Jan 2024 21:03:17 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel+bounces-36446-ouuuleilei=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1; Authentication-Results: mx.google.com; dkim=pass header.i=@intel.com header.s=Intel header.b=MrQnhGrA; arc=pass (i=1 spf=pass spfdomain=intel.com dkim=pass dkdomain=intel.com dmarc=pass fromdomain=intel.com); spf=pass (google.com: domain of linux-kernel+bounces-36446-ouuuleilei=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) smtp.mailfrom="linux-kernel+bounces-36446-ouuuleilei=gmail.com@vger.kernel.org"; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=intel.com Received: from smtp.subspace.kernel.org (wormhole.subspace.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ny.mirrors.kernel.org (Postfix) with ESMTPS id 82EBA1C26350 for <ouuuleilei@gmail.com>; Wed, 24 Jan 2024 05:03:17 +0000 (UTC) Received: from localhost.localdomain (localhost.localdomain [127.0.0.1]) by smtp.subspace.kernel.org (Postfix) with ESMTP id 152F4134CD; Wed, 24 Jan 2024 05:02:32 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=intel.com header.i=@intel.com header.b="MrQnhGrA" Received: from mgamail.intel.com (mgamail.intel.com [192.55.52.115]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 9F002C8F1 for <linux-kernel@vger.kernel.org>; Wed, 24 Jan 2024 05:02:27 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=192.55.52.115 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1706072549; cv=none; b=Y3PY0QWGs6xBRpx8wRdvfTDCoswkgJFnnYJoDo7NNy2z8plvYQzEIv6yWh75gr4b0Yrm38L27l1S4/t953kGMiyQtXInwUd3C81b/ZPiZaC/JLPN5QC6W2KZLZZgRjAQr9fk/LyK41frczBhhsT3HbEzVpqquG8uYX7IRsbAhkA= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1706072549; c=relaxed/simple; bh=fGpX5d8msWVViBqm7sRAsYLfy5pDgJccFA8lspAl428=; h=From:To:Cc:Subject:Date:Message-ID:In-Reply-To:References: MIME-Version; b=uiocoVPPkilVTWn0nUry9KhbceSO61/LbkT4KG4oIcLgKtTYgncSVnaBNUwKWSglQCp1bgy2LksMYxXnucOqOfF7kP5pfYKPbKzTVyeKWHvRypWnJsyEgdRu7Zp2MJmlpNyHVBp0eAOxoWoIxPNQ4PYsmTRyhj+RUUsgulEghEY= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=intel.com; spf=pass smtp.mailfrom=intel.com; dkim=pass (2048-bit key) header.d=intel.com header.i=@intel.com header.b=MrQnhGrA; arc=none smtp.client-ip=192.55.52.115 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=intel.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=intel.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1706072547; x=1737608547; h=from:to:cc:subject:date:message-id:in-reply-to: references:mime-version:content-transfer-encoding; bh=fGpX5d8msWVViBqm7sRAsYLfy5pDgJccFA8lspAl428=; b=MrQnhGrA6wZIvmbGcsytF5q2nf2KbMhUGpwXbdolK5AlaNHuUqE9Ee6w 91z8AF/a79MuVhQH2x5sA+RzXanK7a6/PF7HjZrD4PmXNtbk5GovONj1S ZDJGLNrKDsaBOj11dkJgL+2JYrdoy5kGM3t0gaF+LUfpIpSZhUkEnKNdL faSa71kUMBaCztDcTuHHH0BbQ2KyX1mlcUP4Oitl7vSEzEx5VshMfegxE jhyYDSBdMWovfNH/clNry/q72YZuHmblNk6LpsJsStdX095CLRdF6gMnU vmVI3Dd30kOgtoqfIiDGdZAa4q+S9rIBwTqVbPwV75zpu05UP7jHfZ5Xy w==; X-IronPort-AV: E=McAfee;i="6600,9927,10962"; a="401399359" X-IronPort-AV: E=Sophos;i="6.05,216,1701158400"; d="scan'208";a="401399359" Received: from orsmga004.jf.intel.com ([10.7.209.38]) by fmsmga103.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 23 Jan 2024 21:02:24 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10962"; a="909551505" X-IronPort-AV: E=Sophos;i="6.05,216,1701158400"; d="scan'208";a="909551505" Received: from lucas-s2600cw.jf.intel.com ([10.165.21.196]) by orsmga004-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 23 Jan 2024 21:02:23 -0800 From: Lucas De Marchi <lucas.demarchi@intel.com> To: Yury Norov <yury.norov@gmail.com> Cc: linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, Andy Shevchenko <andriy.shevchenko@linux.intel.com>, Jani Nikula <jani.nikula@linux.intel.com>, intel-xe@lists.freedesktop.org, intel-gfx@lists.freedesktop.org Subject: [PATCH 1/3] bits: introduce fixed-type genmasks Date: Tue, 23 Jan 2024 21:02:03 -0800 Message-ID: <20240124050205.3646390-2-lucas.demarchi@intel.com> X-Mailer: git-send-email 2.43.0 In-Reply-To: <20240124050205.3646390-1-lucas.demarchi@intel.com> References: <20240124050205.3646390-1-lucas.demarchi@intel.com> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: <linux-kernel.vger.kernel.org> List-Subscribe: <mailto:linux-kernel+subscribe@vger.kernel.org> List-Unsubscribe: <mailto:linux-kernel+unsubscribe@vger.kernel.org> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1788946780163679639 X-GMAIL-MSGID: 1788946780163679639 |
Series |
Fixed-type GENMASK/BIT
|
|
Commit Message
Lucas De Marchi
Jan. 24, 2024, 5:02 a.m. UTC
From: Yury Norov <yury.norov@gmail.com> Generalize __GENMASK() to support different types, and implement fixed-types versions of GENMASK() based on it. The fixed-type version allows more strict checks to the min/max values accepted, which is useful for defining registers like implemented by i915 and xe drivers with their REG_GENMASK*() macros. Signed-off-by: Yury Norov <yury.norov@gmail.com> --- include/linux/bitops.h | 1 - include/linux/bits.h | 22 ++++++++++++---------- 2 files changed, 12 insertions(+), 11 deletions(-)
Comments
On Tue, 23 Jan 2024, Lucas De Marchi <lucas.demarchi@intel.com> wrote: > From: Yury Norov <yury.norov@gmail.com> > > Generalize __GENMASK() to support different types, and implement > fixed-types versions of GENMASK() based on it. The fixed-type version > allows more strict checks to the min/max values accepted, which is > useful for defining registers like implemented by i915 and xe drivers > with their REG_GENMASK*() macros. Mmh, the commit message says the fixed-type version allows more strict checks, but none are actually added. GENMASK_INPUT_CHECK() remains the same. Compared to the i915 and xe versions, this is more lax now. You could specify GENMASK_U32(63,32) without complaints. BR, Jani. > > Signed-off-by: Yury Norov <yury.norov@gmail.com> > --- > include/linux/bitops.h | 1 - > include/linux/bits.h | 22 ++++++++++++---------- > 2 files changed, 12 insertions(+), 11 deletions(-) > > diff --git a/include/linux/bitops.h b/include/linux/bitops.h > index 2ba557e067fe..1db50c69cfdb 100644 > --- a/include/linux/bitops.h > +++ b/include/linux/bitops.h > @@ -15,7 +15,6 @@ > # define aligned_byte_mask(n) (~0xffUL << (BITS_PER_LONG - 8 - 8*(n))) > #endif > > -#define BITS_PER_TYPE(type) (sizeof(type) * BITS_PER_BYTE) > #define BITS_TO_LONGS(nr) __KERNEL_DIV_ROUND_UP(nr, BITS_PER_TYPE(long)) > #define BITS_TO_U64(nr) __KERNEL_DIV_ROUND_UP(nr, BITS_PER_TYPE(u64)) > #define BITS_TO_U32(nr) __KERNEL_DIV_ROUND_UP(nr, BITS_PER_TYPE(u32)) > diff --git a/include/linux/bits.h b/include/linux/bits.h > index 7c0cf5031abe..cb94128171b2 100644 > --- a/include/linux/bits.h > +++ b/include/linux/bits.h > @@ -6,6 +6,8 @@ > #include <vdso/bits.h> > #include <asm/bitsperlong.h> > > +#define BITS_PER_TYPE(type) (sizeof(type) * BITS_PER_BYTE) > + > #define BIT_MASK(nr) (UL(1) << ((nr) % BITS_PER_LONG)) > #define BIT_WORD(nr) ((nr) / BITS_PER_LONG) > #define BIT_ULL_MASK(nr) (ULL(1) << ((nr) % BITS_PER_LONG_LONG)) > @@ -30,16 +32,16 @@ > #define GENMASK_INPUT_CHECK(h, l) 0 > #endif > > -#define __GENMASK(h, l) \ > - (((~UL(0)) - (UL(1) << (l)) + 1) & \ > - (~UL(0) >> (BITS_PER_LONG - 1 - (h)))) > -#define GENMASK(h, l) \ > - (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l)) > +#define __GENMASK(t, h, l) \ > + (GENMASK_INPUT_CHECK(h, l) + \ > + (((t)~0ULL - ((t)(1) << (l)) + 1) & \ > + ((t)~0ULL >> (BITS_PER_TYPE(t) - 1 - (h))))) > > -#define __GENMASK_ULL(h, l) \ > - (((~ULL(0)) - (ULL(1) << (l)) + 1) & \ > - (~ULL(0) >> (BITS_PER_LONG_LONG - 1 - (h)))) > -#define GENMASK_ULL(h, l) \ > - (GENMASK_INPUT_CHECK(h, l) + __GENMASK_ULL(h, l)) > +#define GENMASK(h, l) __GENMASK(unsigned long, h, l) > +#define GENMASK_ULL(h, l) __GENMASK(unsigned long long, h, l) > +#define GENMASK_U8(h, l) __GENMASK(u8, h, l) > +#define GENMASK_U16(h, l) __GENMASK(u16, h, l) > +#define GENMASK_U32(h, l) __GENMASK(u32, h, l) > +#define GENMASK_U64(h, l) __GENMASK(u64, h, l) > > #endif /* __LINUX_BITS_H */
On Wed, Jan 24, 2024 at 09:58:26AM +0200, Jani Nikula wrote: >On Tue, 23 Jan 2024, Lucas De Marchi <lucas.demarchi@intel.com> wrote: >> From: Yury Norov <yury.norov@gmail.com> >> >> Generalize __GENMASK() to support different types, and implement >> fixed-types versions of GENMASK() based on it. The fixed-type version >> allows more strict checks to the min/max values accepted, which is >> useful for defining registers like implemented by i915 and xe drivers >> with their REG_GENMASK*() macros. > >Mmh, the commit message says the fixed-type version allows more strict >checks, but none are actually added. GENMASK_INPUT_CHECK() remains the >same. > >Compared to the i915 and xe versions, this is more lax now. You could >specify GENMASK_U32(63,32) without complaints. Doing this on top of the this series: -#define XELPDP_PORT_M2P_COMMAND_TYPE_MASK REG_GENMASK(30, 27) +#define XELPDP_PORT_M2P_COMMAND_TYPE_MASK REG_GENMASK(62, 32) and I do get a build failure: ./drivers/gpu/drm/i915/display/intel_cx0_phy.c: In function ‘__intel_cx0_read_once’: ./include/linux/bits.h:41:31: error: left shift count >= width of type [-Werror=shift-count-overflow] 41 | (((t)~0ULL - ((t)(1) << (l)) + 1) & \ | ^~ I also tested them individually. All of these fail to build for me: 1) -#define XELPDP_PORT_M2P_COMMAND_TYPE_MASK REG_GENMASK(30, 27) +#define XELPDP_PORT_M2P_COMMAND_TYPE_MASK REG_GENMASK(32, 27) 2) -#define XELPDP_PORT_M2P_COMMAND_TYPE_MASK REG_GENMASK(30, 27) +#define XELPDP_PORT_M2P_COMMAND_TYPE_MASK REG_GENMASK(30, 31) 3) -#define XELPDP_PORT_M2P_COMMAND_TYPE_MASK REG_GENMASK(30, 27) +#define XELPDP_PORT_M2P_COMMAND_TYPE_MASK REG_GENMASK(30, -1) Lucas De Marchi
On Wed, Jan 24, 2024 at 08:03:53AM -0600, Lucas De Marchi wrote: > On Wed, Jan 24, 2024 at 09:58:26AM +0200, Jani Nikula wrote: > > On Tue, 23 Jan 2024, Lucas De Marchi <lucas.demarchi@intel.com> wrote: > > > From: Yury Norov <yury.norov@gmail.com> > > > > > > Generalize __GENMASK() to support different types, and implement > > > fixed-types versions of GENMASK() based on it. The fixed-type version > > > allows more strict checks to the min/max values accepted, which is > > > useful for defining registers like implemented by i915 and xe drivers > > > with their REG_GENMASK*() macros. > > > > Mmh, the commit message says the fixed-type version allows more strict > > checks, but none are actually added. GENMASK_INPUT_CHECK() remains the > > same. > > > > Compared to the i915 and xe versions, this is more lax now. You could > > specify GENMASK_U32(63,32) without complaints. > > Doing this on top of the this series: > > -#define XELPDP_PORT_M2P_COMMAND_TYPE_MASK REG_GENMASK(30, 27) > +#define XELPDP_PORT_M2P_COMMAND_TYPE_MASK REG_GENMASK(62, 32) > > and I do get a build failure: > > ../drivers/gpu/drm/i915/display/intel_cx0_phy.c: In function ‘__intel_cx0_read_once’: > ../include/linux/bits.h:41:31: error: left shift count >= width of type [-Werror=shift-count-overflow] > 41 | (((t)~0ULL - ((t)(1) << (l)) + 1) & \ > | ^~ I would better include this in commit message to avoid people's confusion. If it comes to v2, can you please do it and mention that this trick relies on shift-count-overflow compiler check? Thanks, Yury
Quoting Yury Norov (2024-01-24 12:27:58-03:00) >On Wed, Jan 24, 2024 at 08:03:53AM -0600, Lucas De Marchi wrote: >> On Wed, Jan 24, 2024 at 09:58:26AM +0200, Jani Nikula wrote: >> > On Tue, 23 Jan 2024, Lucas De Marchi <lucas.demarchi@intel.com> wrote: >> > > From: Yury Norov <yury.norov@gmail.com> >> > > >> > > Generalize __GENMASK() to support different types, and implement >> > > fixed-types versions of GENMASK() based on it. The fixed-type version >> > > allows more strict checks to the min/max values accepted, which is >> > > useful for defining registers like implemented by i915 and xe drivers >> > > with their REG_GENMASK*() macros. >> > >> > Mmh, the commit message says the fixed-type version allows more strict >> > checks, but none are actually added. GENMASK_INPUT_CHECK() remains the >> > same. >> > >> > Compared to the i915 and xe versions, this is more lax now. You could >> > specify GENMASK_U32(63,32) without complaints. >> >> Doing this on top of the this series: >> >> -#define XELPDP_PORT_M2P_COMMAND_TYPE_MASK REG_GENMASK(30, 27) >> +#define XELPDP_PORT_M2P_COMMAND_TYPE_MASK REG_GENMASK(62, 32) >> >> and I do get a build failure: >> >> ../drivers/gpu/drm/i915/display/intel_cx0_phy.c: In function ‘__intel_cx0_read_once’: >> ../include/linux/bits.h:41:31: error: left shift count >= width of type [-Werror=shift-count-overflow] >> 41 | (((t)~0ULL - ((t)(1) << (l)) + 1) & \ >> | ^~ > >I would better include this in commit message to avoid people's >confusion. If it comes to v2, can you please do it and mention that >this trick relies on shift-count-overflow compiler check? Wouldn't it be better to have explicit check that l and h are not out of bounds based on BITS_PER_TYPE() than relying on a compiler flag that could be turned off (maybe for some questionable reason, but even so)? -- Gustavo Sousa
On Wed, 24 Jan 2024, Gustavo Sousa <gustavo.sousa@intel.com> wrote: > Quoting Yury Norov (2024-01-24 12:27:58-03:00) >>On Wed, Jan 24, 2024 at 08:03:53AM -0600, Lucas De Marchi wrote: >>> On Wed, Jan 24, 2024 at 09:58:26AM +0200, Jani Nikula wrote: >>> > On Tue, 23 Jan 2024, Lucas De Marchi <lucas.demarchi@intel.com> wrote: >>> > > From: Yury Norov <yury.norov@gmail.com> >>> > > >>> > > Generalize __GENMASK() to support different types, and implement >>> > > fixed-types versions of GENMASK() based on it. The fixed-type version >>> > > allows more strict checks to the min/max values accepted, which is >>> > > useful for defining registers like implemented by i915 and xe drivers >>> > > with their REG_GENMASK*() macros. >>> > >>> > Mmh, the commit message says the fixed-type version allows more strict >>> > checks, but none are actually added. GENMASK_INPUT_CHECK() remains the >>> > same. >>> > >>> > Compared to the i915 and xe versions, this is more lax now. You could >>> > specify GENMASK_U32(63,32) without complaints. >>> >>> Doing this on top of the this series: >>> >>> -#define XELPDP_PORT_M2P_COMMAND_TYPE_MASK REG_GENMASK(30, 27) >>> +#define XELPDP_PORT_M2P_COMMAND_TYPE_MASK REG_GENMASK(62, 32) >>> >>> and I do get a build failure: >>> >>> ../drivers/gpu/drm/i915/display/intel_cx0_phy.c: In function ‘__intel_cx0_read_once’: >>> ../include/linux/bits.h:41:31: error: left shift count >= width of type [-Werror=shift-count-overflow] >>> 41 | (((t)~0ULL - ((t)(1) << (l)) + 1) & \ >>> | ^~ I stand corrected. >> >>I would better include this in commit message to avoid people's >>confusion. If it comes to v2, can you please do it and mention that >>this trick relies on shift-count-overflow compiler check? > > Wouldn't it be better to have explicit check that l and h are not out of bounds > based on BITS_PER_TYPE() than relying on a compiler flag that could be turned > off (maybe for some questionable reason, but even so)? My preference would be the explicit check, a comment in code, or an explanation in the commit message, in this order. Because honestly, none of this is obvious, and a future refactoring of GENMASK might just inadvertently thwart the whole check. Regardless, my main concern was moot, on the series, Acked-by: Jani Nikula <jani.nikula@intel.com>
On Wed, Jan 24, 2024 at 07:27:58AM -0800, Yury Norov wrote: >On Wed, Jan 24, 2024 at 08:03:53AM -0600, Lucas De Marchi wrote: >> On Wed, Jan 24, 2024 at 09:58:26AM +0200, Jani Nikula wrote: >> > On Tue, 23 Jan 2024, Lucas De Marchi <lucas.demarchi@intel.com> wrote: >> > > From: Yury Norov <yury.norov@gmail.com> >> > > >> > > Generalize __GENMASK() to support different types, and implement >> > > fixed-types versions of GENMASK() based on it. The fixed-type version >> > > allows more strict checks to the min/max values accepted, which is >> > > useful for defining registers like implemented by i915 and xe drivers >> > > with their REG_GENMASK*() macros. >> > >> > Mmh, the commit message says the fixed-type version allows more strict >> > checks, but none are actually added. GENMASK_INPUT_CHECK() remains the >> > same. >> > >> > Compared to the i915 and xe versions, this is more lax now. You could >> > specify GENMASK_U32(63,32) without complaints. >> >> Doing this on top of the this series: >> >> -#define XELPDP_PORT_M2P_COMMAND_TYPE_MASK REG_GENMASK(30, 27) >> +#define XELPDP_PORT_M2P_COMMAND_TYPE_MASK REG_GENMASK(62, 32) >> >> and I do get a build failure: >> >> ../drivers/gpu/drm/i915/display/intel_cx0_phy.c: In function ‘__intel_cx0_read_once’: >> ../include/linux/bits.h:41:31: error: left shift count >= width of type [-Werror=shift-count-overflow] >> 41 | (((t)~0ULL - ((t)(1) << (l)) + 1) & \ >> | ^~ > >I would better include this in commit message to avoid people's >confusion. If it comes to v2, can you please do it and mention that >this trick relies on shift-count-overflow compiler check? either that or an explicit check as it was suggested. What's your preference? Lucas De Marchi > >Thanks, >Yury
On Mon, Jan 29, 2024 at 08:49:35AM -0600, Lucas De Marchi wrote: > On Wed, Jan 24, 2024 at 07:27:58AM -0800, Yury Norov wrote: > > On Wed, Jan 24, 2024 at 08:03:53AM -0600, Lucas De Marchi wrote: > > > On Wed, Jan 24, 2024 at 09:58:26AM +0200, Jani Nikula wrote: > > > > On Tue, 23 Jan 2024, Lucas De Marchi <lucas.demarchi@intel.com> wrote: > > > > > From: Yury Norov <yury.norov@gmail.com> > > > > > > > > > > Generalize __GENMASK() to support different types, and implement > > > > > fixed-types versions of GENMASK() based on it. The fixed-type version > > > > > allows more strict checks to the min/max values accepted, which is > > > > > useful for defining registers like implemented by i915 and xe drivers > > > > > with their REG_GENMASK*() macros. > > > > > > > > Mmh, the commit message says the fixed-type version allows more strict > > > > checks, but none are actually added. GENMASK_INPUT_CHECK() remains the > > > > same. > > > > > > > > Compared to the i915 and xe versions, this is more lax now. You could > > > > specify GENMASK_U32(63,32) without complaints. > > > > > > Doing this on top of the this series: > > > > > > -#define XELPDP_PORT_M2P_COMMAND_TYPE_MASK REG_GENMASK(30, 27) > > > +#define XELPDP_PORT_M2P_COMMAND_TYPE_MASK REG_GENMASK(62, 32) > > > > > > and I do get a build failure: > > > > > > ../drivers/gpu/drm/i915/display/intel_cx0_phy.c: In function ‘__intel_cx0_read_once’: > > > ../include/linux/bits.h:41:31: error: left shift count >= width of type [-Werror=shift-count-overflow] > > > 41 | (((t)~0ULL - ((t)(1) << (l)) + 1) & \ > > > | ^~ > > > > I would better include this in commit message to avoid people's > > confusion. If it comes to v2, can you please do it and mention that > > this trick relies on shift-count-overflow compiler check? > > either that or an explicit check as it was suggested. What's your > preference? Let's put a comment in the code. An argument that shift-count-overflow may be disabled sounds more like a speculation unless we have a solid example of a build system where the error is disabled for a good sane reason, but possible GENMASK() overflow is still considered dangerous. GENMASK() is all about bit shifts, so shift-related error is something I'd expect when using GENMASK(). Also, the macro is widely used in the kernel: yury:linux$ git grep GENMASK | wc -l 26879 Explicit check would add pressure on the compiler for nothing. Thanks, Yury
diff --git a/include/linux/bitops.h b/include/linux/bitops.h index 2ba557e067fe..1db50c69cfdb 100644 --- a/include/linux/bitops.h +++ b/include/linux/bitops.h @@ -15,7 +15,6 @@ # define aligned_byte_mask(n) (~0xffUL << (BITS_PER_LONG - 8 - 8*(n))) #endif -#define BITS_PER_TYPE(type) (sizeof(type) * BITS_PER_BYTE) #define BITS_TO_LONGS(nr) __KERNEL_DIV_ROUND_UP(nr, BITS_PER_TYPE(long)) #define BITS_TO_U64(nr) __KERNEL_DIV_ROUND_UP(nr, BITS_PER_TYPE(u64)) #define BITS_TO_U32(nr) __KERNEL_DIV_ROUND_UP(nr, BITS_PER_TYPE(u32)) diff --git a/include/linux/bits.h b/include/linux/bits.h index 7c0cf5031abe..cb94128171b2 100644 --- a/include/linux/bits.h +++ b/include/linux/bits.h @@ -6,6 +6,8 @@ #include <vdso/bits.h> #include <asm/bitsperlong.h> +#define BITS_PER_TYPE(type) (sizeof(type) * BITS_PER_BYTE) + #define BIT_MASK(nr) (UL(1) << ((nr) % BITS_PER_LONG)) #define BIT_WORD(nr) ((nr) / BITS_PER_LONG) #define BIT_ULL_MASK(nr) (ULL(1) << ((nr) % BITS_PER_LONG_LONG)) @@ -30,16 +32,16 @@ #define GENMASK_INPUT_CHECK(h, l) 0 #endif -#define __GENMASK(h, l) \ - (((~UL(0)) - (UL(1) << (l)) + 1) & \ - (~UL(0) >> (BITS_PER_LONG - 1 - (h)))) -#define GENMASK(h, l) \ - (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l)) +#define __GENMASK(t, h, l) \ + (GENMASK_INPUT_CHECK(h, l) + \ + (((t)~0ULL - ((t)(1) << (l)) + 1) & \ + ((t)~0ULL >> (BITS_PER_TYPE(t) - 1 - (h))))) -#define __GENMASK_ULL(h, l) \ - (((~ULL(0)) - (ULL(1) << (l)) + 1) & \ - (~ULL(0) >> (BITS_PER_LONG_LONG - 1 - (h)))) -#define GENMASK_ULL(h, l) \ - (GENMASK_INPUT_CHECK(h, l) + __GENMASK_ULL(h, l)) +#define GENMASK(h, l) __GENMASK(unsigned long, h, l) +#define GENMASK_ULL(h, l) __GENMASK(unsigned long long, h, l) +#define GENMASK_U8(h, l) __GENMASK(u8, h, l) +#define GENMASK_U16(h, l) __GENMASK(u16, h, l) +#define GENMASK_U32(h, l) __GENMASK(u32, h, l) +#define GENMASK_U64(h, l) __GENMASK(u64, h, l) #endif /* __LINUX_BITS_H */