From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755424AbcBIInc (ORCPT ); Tue, 9 Feb 2016 03:43:32 -0500 Received: from mailout4.w1.samsung.com ([210.118.77.14]:55527 "EHLO mailout4.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753075AbcBIIna (ORCPT ); Tue, 9 Feb 2016 03:43:30 -0500 MIME-version: 1.0 Content-type: text/plain; charset=windows-1252 X-AuditID: cbfec7f4-f79026d00000418a-ef-56b9a6af98a3 Content-transfer-encoding: 8BIT Subject: Re: [PATCH v3] err.h: allow IS_ERR_VALUE to handle properly more types To: Arnd Bergmann References: <20160202163350.f7d42f4b97f48756f3900e9a@linux-foundation.org> <2046663.fHIlWH1ph1@wuerfel> <56B855C3.5010006@samsung.com> <5549573.1i7GpRbvFd@wuerfel> Cc: Andrew Morton , Bartlomiej Zolnierkiewicz , Marek Szyprowski , open list , Bob Peterson , linux@rasmusvillemoes.dk From: Andrzej Hajda Message-id: <56B9A672.1080905@samsung.com> Date: Tue, 09 Feb 2016 09:42:26 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.5.1 In-reply-to: <5549573.1i7GpRbvFd@wuerfel> X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFrrLLMWRmVeSWpSXmKPExsVy+t/xy7rrl+0MM+i4rGoxZ/0aNou/k46x W2ycsZ7V4vKuOWwWj2fNY7NYe+Quu0XX7FY2B3aP378mMXqcmPGbxeNCV7bH+31X2Tz6tqxi 9Pi8SS6ALYrLJiU1J7MstUjfLoEr48T5/UwFDxQq1pzaxN7A+Fyyi5GTQ0LARGLmoRXsELaY xIV769m6GLk4hASWMkrcfr2QDSTBKyAo8WPyPZYuRg4OZgF5iSOXsiFMPYn7F7Ugyp8zSkxq v8oEEhcW8JdY8kgVpFNEQFFi6otnzBA1yxkllnX2MoI4zAINTBJHFn5gAqliE9CU+Lv5JtQu LYmWrzsZQWwWAVWJmW0Qx4kKREgc7uwCszmB6l8tusU2gVFgFpLzZiGcNwvhvAWMzKsYRVNL kwuKk9JzDfWKE3OLS/PS9ZLzczcxQgL9yw7GxcesDjEKcDAq8fAe+LwjTIg1say4MvcQowQH s5IIr+ecnWFCvCmJlVWpRfnxRaU5qcWHGKU5WJTEeefueh8iJJCeWJKanZpakFoEk2Xi4JRq YEzflshtnxZ9uShw7fJSxYtmaQ/5b//hUOXjmtj/36xD4IVAcK7srPdHNvzIquMzyZ3W6bJp 7ecf72bNaz5mKcemOGtO5IzP/Rqe/gF6T2adzg6+4a6SEXtgtYIPx7dFN7KrP7pZ3uacHbN5 fbhsQFl89Un+Q5436lgVG3+KCHVu1Sh/zSjMq8RSnJFoqMVcVJwIACduiahwAgAA Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org +cc Rasmus Villemoes, I forgot to add him earlier. On 02/08/2016 01:01 PM, Arnd Bergmann wrote: > On Monday 08 February 2016 09:45:55 Andrzej Hajda wrote: >> On 02/05/2016 11:52 AM, Arnd Bergmann wrote: >>> On Thursday 04 February 2016 10:59:31 Andrew Morton wrote: >> My version produces shortest code, Arnd's is the same as the old one. >> On the other side Rasmus proposition seems to be the most straightforward >> to me. Anyway I am not sure if the code length is the most important here. >> >> By the way .data segment size grows almost 4 times between gcc 4.4 and >> 4.8 :) >> Also numbers for arm64 looks interesting. >> >> Just for the record below all proposed implementations: >> #define IS_ERR_VALUE_old(x) unlikely((x) >= (unsigned long)-MAX_ERRNO) >> #define IS_ERR_VALUE_andrzej(x) ((typeof(x))(-1) <= 0 \ >> ? unlikely((x) <= -1) \ >> : unlikely((x) >= (typeof(x))-MAX_ERRNO)) >> #define IS_ERR_VALUE_arnd(x) (unlikely((unsigned long long)(x) >= >> (unsigned long long)(typeof(x))-MAX_ERRNO)) >> #define IS_ERR_VALUE_rasmus(x) ({\ >> typeof(x) _x = (x);\ >> unlikely(_x >= (typeof(x))-MAX_ERRNO && _x <= (typeof(x))-1);\ >> }) >> >>> Andrzej's version is a little shorter on ARM because in case of signed numbers >>> it only checks for negative values, rather than checking for values in the >>> [-MAX_ERRNO..-1] range. I think the original behavior is more logical >>> in this case, and my version restores it. >> As I looked at the usage of the macro in the kernel I have not found any >> code >> which could benefit from the original behavior, except some buggy code in >> staging which have already pending fix[1]. >> But maybe it would be better to use IS_ERR_VALUE to always check if err >> is in >> range [-MAX_ERRNO..-1] and just use simple 'err < 0' in typical case of >> signed types. > If we do that, should we also make it illegal to use an invalid type > for IS_ERR()? At least that could also catch any use of 'char' and 'unsigned > char' that are still broken. I meant rather to make such 'policy' for future code by adding some comment to the macro. Optionally adding compile time warning to encourage developers to change current usage, however I am not sure if it is not too harsh. This way it could be also good to use your version of the macro. It could be also good to add compiletime_assert to prevent char types as suggested by Rasmus. Finally it could look like: /* * Use IS_ERR_VALUE only on unsigned types of at least two bytes size. * For signed types use '< 0' comparison. */ #define IS_ERR_VALUE(x)\ ({\ compiletime_assert(sizeof(x) > 1, "IS_ERR_VALUE does not handle byte-size types");\ compiletime_assert_warning((typeof(x))(-1) > 0, "IS_ERR_VALUE should be called on unsigned types only, use '< 0' instead");\ (unlikely((unsigned long long)(x) >= (unsigned long long)(typeof(x))-MAX_ERRNO));\ }) Minor issue: there are no compile-time warning macros in kernel. Helper provided by gcc (warning attribute) is not so nice: optimizations removes the warning itself, preventing optimization influences final code. I have put my proposition of workaround below: #define compiletime_assert_warning(cond, msg) \ ({ \ __maybe_unused void const *p = (cond) ? 0 : 1; \ }) On older compilers it issues just warning: drivers/nvmem/core.c:1059: warning: initialization makes pointer from integer without a cast Since gcc 4.8 it is more verbose: drivers/nvmem/core.c: In function ‘nvmem_device_write’: include/linux/err.h:33:33: warning: initialization makes pointer from integer without a cast [enabled by default] __maybe_unused void const *p = (cond) ? 0 : 1; \ ^ include/linux/err.h:41:2: note: in expansion of macro ‘compiletime_assert_warning’ compiletime_assert_warning((typeof(x))(-1) > 0, "IS_ERR_VALUE should be called on unsigned types only, use '< 0' instead");\ ^ drivers/nvmem/core.c:1059:6: note: in expansion of macro ‘IS_ERR_VALUE’ if (IS_ERR_VALUE(rc)) Regards Andrzej