Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 9 May 2023 01:15:46 -0700
From:   Lucas De Marchi <lucas.demarchi@intel.com>
To:     Jani Nikula <jani.nikula@linux.intel.com>
CC:     <intel-gfx@lists.freedesktop.org>,
        <intel-xe@lists.freedesktop.org>,
        <dri-devel@lists.freedesktop.org>,
        Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        <linux-kernel@vger.kernel.org>,
        Masahiro Yamada <masahiroy@kernel.org>,
        Andrew Morton <akpm@linux-foundation.org>,
        Kevin Brodsky <kevin.brodsky@arm.com>,
        Alex Deucher <alexander.deucher@amd.com>,
        Christian =?utf-8?B?S8O2bmln?= <christian.koenig@amd.com>
Subject: Re: [PATCH 3/3] drm/i915: Temporary conversion to new GENMASK/BIT
 macros
Message-ID: <svwv5rvhjyrqz5fkmrmuurwib6wkyujvd27ydffwlw5py4kj77@s44hufd53etv>
References: <20230509051403.2748545-1-lucas.demarchi@intel.com>
 <20230509051403.2748545-4-lucas.demarchi@intel.com>
 <87cz3andi8.fsf@intel.com>
Content-Type: text/plain; charset="utf-8"; format=flowed
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <87cz3andi8.fsf@intel.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?d3NpZHhvNCtVYXg0UzZFeXdHd0dmSDd6Qy9iT0V5MnhxeUpOMGd2aVZNMXU3?=
 =?utf-8?B?ZERXdnFhMW9DNEsxYjlvQnhjSEFKZ0FFZnVNajE2U0JNNDQveVNjMlB3L0hL?=
 =?utf-8?B?ZGRsMG9sUUZ4Z0piSEUyK3NmeFZIQmFRZDRrV1J5NUtUdm5yYWxoVHBlbEZC?=
 =?utf-8?B?NEd2OEFvQ2R3RmRDRWdiYjMyRFB4OWkxSDNWSHNtRTRnSFlpWkR3ekdPN3R6?=
 =?utf-8?B?Q0VOWE5RUFJzUFdhQ1Y1Zjh4bnhKVnY1UkdWZXRZbFBwZ2l1UjZhKzhZZkRo?=
 =?utf-8?B?WHJTeWk5R3dJbkZhWmhERHJTby9RS1RHZEx1L255aUVTcjFUTThIMVFCOVpF?=
 =?utf-8?B?QnlmUmk3WGVuUnRFMTYwQWZ2bzg3Sk4vTmxydm0vZWU3dlNURkhsRWZvb3M5?=
 =?utf-8?B?Q0FmMll2dGJreUUza2lXN1FZVEduSEh4dVNiTnB5UzBENnNnTnlPSGIvNk84?=
 =?utf-8?B?Zm54ZjQyZ2NPTERVdzhxSkpGemd2UHhLRndnbm1SK1RtYmlHS1dmSkdDNVFv?=
 =?utf-8?B?cW13bHJxUkVnL3RKdHlOS2QzNm45MGdIZkNZMnpuTTFpM081VlNaU2xjcGh5?=
 =?utf-8?B?ZmFHdHY2bnVwWVN5ZlZ1UHhIWWZlakE5NGd5VHpqc09pUWsrQ2FuYjVNcG4w?=
 =?utf-8?B?NUZqRDlDc2ZnMlhnMlpYM3BZTjhSMm1JOWJ0UG95VjduRmE4TE5nSWR1RzEz?=
 =?utf-8?B?SEJlaUZjYzBGZXUwS2FuWUdaN1RXeWRvN21lbnJjSDh2WGFCSGt4YnR1Z0tW?=
 =?utf-8?B?bFR4NmgrS1RiUXAyaEhhRzg5ZmxoRlhvaUZQYno1aVhQNitoQ1Z4RXBORVNL?=
 =?utf-8?B?dWRVWXFTNkl0OG1xclVhRUREQXVzckdaNHo3bk5obG5iUXJsbUh2Y2Q4cWFl?=
 =?utf-8?B?enlJZ0RhWmpMbGFoZU1YZ1J4d2h1U0dMNnNSL2xBbFVHR0g5TzdaV2Z1UzNT?=
 =?utf-8?B?eExoRHQ3U3NSS1doMUVlRjk0RndFc0RPNUhVU3RER3lJdzI1amN0bzh2NHdL?=
 =?utf-8?B?d3RwQnJaZzM3ZkZkellnV05OTXJpcEZ2eGJBaU82M0M1aHgvZzM2cmdTdkZZ?=
 =?utf-8?B?WHpNNWM4L04zV2ZpZk5yUFQ4cmxMdWxGNXNEVGh6R3dLejB6MGpoVjhYK1ZQ?=
 =?utf-8?B?dWt0bk9GRk91bnl6dmNqV28yN204OEhMSDVhQXBMWGN3TXZpWm1iR0RBNzEx?=
 =?utf-8?B?djkzSWJrdjFxeHhRamJmVE1GT3lwUUtiTDY2aURRM2w1REE3Z1BYa0Fwa21J?=
 =?utf-8?B?c0Zhc1htdFhRcVdLM3Bqa2NRSWRJRENFUG12MC9NbTVLeVpQS2RFMnFGZFFm?=
 =?utf-8?B?bHRTNC9NOW5FTy9nYjJQQUI1a2RPaE12QjVKSXNCdmQrTEwyeVFEYWhkQS81?=
 =?utf-8?B?SFg3YmVZKzlDYlJHRGhxQ05mODVHckNZbUlXdFZNcDVzVCszTXhaajRLY0o0?=
 =?utf-8?B?RlltN1VIUC9EbG1ZUTJRU2F5VUlqYmlWT0hXSmtqdDRmem1DOGhZWnZydytK?=
 =?utf-8?B?K08xU0V5d3ltbFk1S254Mm95WHM2bllLdUVLM3lPQkMxMCt2NDVUVkc3TDAz?=
 =?utf-8?B?akJLbGtzMWdCc1VzU1gyUWxwMXhLSHNZMGkrRlVvT2VGTFNodno3eUFneEc3?=
 =?utf-8?B?NGp1VmNMdTNPaGVCck9nNkJDWHpLSlE3Y0w4NU9tb284clhnRWZsNjIvVFJQ?=
 =?utf-8?B?K1pwNTJpV1V0MDlOUGw1OXU5UVN0RnA3VzhKdk9NVjQ4cW9WNGNEMFdyaWk4?=
 =?utf-8?B?bU02cUpUK29oVVNweWlUNDRNWkJLQWJXMjZjOEFxTHQ1WENpVVdkczlobXc4?=
 =?utf-8?B?anBCdkp3VXdCd1hBeStlRDlUZUVrdWo5Y3U0cVllaGpIQzgxNlBLWjRIUGZz?=
 =?utf-8?B?b3p0ZEFvNll2RHM4dEVuQUtkTHdlcGZiNUl0UHhwK1h0VUFzdnpSK1VjelJR?=
 =?utf-8?B?dGVxSGloUkM0L0JhY09OaWVQVmxqR0hRTEZXTUZPSE9PRTBQaW4rNmt4TUR4?=
 =?utf-8?B?cU1wdXFNYVRRays0Q1F0Y2RMQ2ZXWjUxVjBuUUFuMnJqOGpxQVFOcCtnVmRv?=
 =?utf-8?B?N0NudExROTUreHEzYldGUEVJbVBiQVYxNFZQRFdYaSs2WGpwYlpGS2tlV3Y0?=
 =?utf-8?B?RUhHb2Y5Y2t2WnpkNTZLczQ5S1VSOEVQYzVPTFVUY1ZiOFBROVJzK0JZeDFw?=
 =?utf-8?B?SUE9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 36f5de79-9ee0-4936-6d3f-08db50659b7f
X-MS-Exchange-CrossTenant-AuthSource: CY5PR11MB6139.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 09 May 2023 08:15:53.6418
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: oZAS0Bdm5bK7Sg5S+2ZLW0ePvP+Lj8gTCGCjNrgnVkujEd8kE0de/iamfbMzNuHkUcQcHthgfu7O7n6m3t9KtWkHdr0HQWai1UdmXQg38Ho=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY8PR11MB7798
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.4 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,
        SPF_HELO_PASS,SPF_NONE,T_SCC_BODY_TEXT_LINE,URIBL_BLOCKED
        autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Tue, May 09, 2023 at 10:57:19AM +0300, Jani Nikula wrote:
>On Mon, 08 May 2023, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
>> Convert the REG_* macros from i915_reg_defs.h to use the new macros
>> defined in linux/bits.h. This is just to help on the implementation
>> of the new macros and not intended to be applied.
>
>This drops a number of build time input checks as well as casts to the
>specified types.

the explicit checks... but the checks are still there and the compiler
still gives me a warning or error for using invalid values. See test program in
the second patch. Example:

	static const u32 a2 = GENMASK_U32(32, 0);

	In file included from /tmp/genmask.c:2:                                                                                                                             
	include/linux/bits.h:47:19: warning: right shift count is negative [-Wshift-count-negative]                                                                         
	   47 |          (~U32(0) >> (32 - 1 - (h))))                                                                                                                       
	      |                   ^~                                                                                                                                        

It's a warning, not an error though.

Same warning for the other fixed-widths with numbers above the supported width.
For negative values:

	In file included from include/linux/bits.h:21,                                     
			 from /tmp/genmask.c:2:                                            
	include/linux/build_bug.h:16:51: error: negative width in bit-field ‘<anonymous>’  
	   16 | #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))     
	      |                                                   ^                        


The cast to the specified type we lose indeed. Could you give an example where
those are useful in the context they are used? I debated adding them, but couldn't find
a justified use of them.

Lucas De Marchi

>
>BR,
>Jani.
>
>>
>> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_reg_defs.h | 28 +++++-----------------------
>>  1 file changed, 5 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_reg_defs.h b/drivers/gpu/drm/i915/i915_reg_defs.h
>> index 622d603080f9..61fbb8d62b25 100644
>> --- a/drivers/gpu/drm/i915/i915_reg_defs.h
>> +++ b/drivers/gpu/drm/i915/i915_reg_defs.h
>> @@ -17,10 +17,7 @@
>>   *
>>   * @return: Value with bit @__n set.
>>   */
>> -#define REG_BIT(__n)							\
>> -	((u32)(BIT(__n) +						\
>> -	       BUILD_BUG_ON_ZERO(__is_constexpr(__n) &&		\
>> -				 ((__n) < 0 || (__n) > 31))))
>> +#define REG_BIT(__n) BIT_U32(__n)
>>
>>  /**
>>   * REG_BIT8() - Prepare a u8 bit value
>> @@ -30,10 +27,7 @@
>>   *
>>   * @return: Value with bit @__n set.
>>   */
>> -#define REG_BIT8(__n)                                                   \
>> -	((u8)(BIT(__n) +                                                \
>> -	       BUILD_BUG_ON_ZERO(__is_constexpr(__n) &&         \
>> -				 ((__n) < 0 || (__n) > 7))))
>> +#define REG_BIT8(__n) BIT_U8(__n)
>>
>>  /**
>>   * REG_GENMASK() - Prepare a continuous u32 bitmask
>> @@ -44,11 +38,7 @@
>>   *
>>   * @return: Continuous bitmask from @__high to @__low, inclusive.
>>   */
>> -#define REG_GENMASK(__high, __low)					\
>> -	((u32)(GENMASK(__high, __low) +					\
>> -	       BUILD_BUG_ON_ZERO(__is_constexpr(__high) &&	\
>> -				 __is_constexpr(__low) &&		\
>> -				 ((__low) < 0 || (__high) > 31 || (__low) > (__high)))))
>> +#define REG_GENMASK(__high, __low) GENMASK_U32(__high, __low)
>>
>>  /**
>>   * REG_GENMASK64() - Prepare a continuous u64 bitmask
>> @@ -59,11 +49,7 @@
>>   *
>>   * @return: Continuous bitmask from @__high to @__low, inclusive.
>>   */
>> -#define REG_GENMASK64(__high, __low)					\
>> -	((u64)(GENMASK_ULL(__high, __low) +				\
>> -	       BUILD_BUG_ON_ZERO(__is_constexpr(__high) &&		\
>> -				 __is_constexpr(__low) &&		\
>> -				 ((__low) < 0 || (__high) > 63 || (__low) > (__high)))))
>> +#define REG_GENMASK64(__high, __low) GENMASK_ULL(__high, __low)
>>
>>  /**
>>   * REG_GENMASK8() - Prepare a continuous u8 bitmask
>> @@ -74,11 +60,7 @@
>>   *
>>   * @return: Continuous bitmask from @__high to @__low, inclusive.
>>   */
>> -#define REG_GENMASK8(__high, __low)                                     \
>> -	((u8)(GENMASK(__high, __low) +                                  \
>> -	       BUILD_BUG_ON_ZERO(__is_constexpr(__high) &&      \
>> -				 __is_constexpr(__low) &&               \
>> -				 ((__low) < 0 || (__high) > 7 || (__low) > (__high)))))
>> +#define REG_GENMASK8(__high, __low) GENMASK_U8(__high, __low)
>>
>>  /*
>>   * Local integer constant expression version of is_power_of_2().
>
>-- 
>Jani Nikula, Intel Open Source Graphics Center