Received: by 10.223.176.5 with SMTP id f5csp504711wra; Tue, 6 Feb 2018 02:43:31 -0800 (PST) X-Google-Smtp-Source: AH8x227f8SmVi68sMfdHJBB5XYatIjkVDmZaIqkfmqCbvP3BAeaCd99jGDJ2A5OX2pD7s6Lfl6Tk X-Received: by 10.99.104.131 with SMTP id d125mr1628493pgc.125.1517913811729; Tue, 06 Feb 2018 02:43:31 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1517913811; cv=none; d=google.com; s=arc-20160816; b=uu+8CHEmTZvaxvjirqgYID2WYizsfkG10QsyjP9LeBJP9HApOYoQ1pbwQj+J0l2Avh tiUcHazfokaLLQZ4tqXaXypNkKfI1bxXOJVAm1cGoOYDy92GGzo1Iy3hiYOCS5FbdAVF hV6eZ4z5zImEn9Vd1VfsAnlolBe/WQz/0dQEHe877OVyOBrBRhb6sQGDl3tHgB73YfRO YgyAaqiDvFrKZo5chF+DWupjxlkigmP8jKA+5EodY5RRGrHe2snjxkZwJ6TWtSHASa+B voakuPjRZTwgqTAhuVxCuTA76nkH9U+C75H0PqQwBBhhxGvO79viHnDavawG6HScFsUr PS2w== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:content-transfer-encoding :content-language:in-reply-to:mime-version:user-agent:date :message-id:from:references:cc:to:subject:arc-authentication-results; bh=O0TG7SJgyUk0s3j7n1lqNz0TSpUNG3bawh5vTknPOUY=; b=QGsOn3N0bBba/keUVIMTQvuNkMg7D/PkttmRY2lWCVJXxF2sS92VtM+j0VS0DOyAkf arB+1aUgT2EwJgnJacPgCN+I3fu3at76YmMOJ7eHnHF2uxLpE9/gsNU3i6TxUwTIQALA jMOg8TuWMPDhfTQ20hYVr0UZk+KZHoqlwoLoWFvMXiLRAYvXg9v8U5/6M0wfGHL2x1kJ afb+0LIw/TcuPzyIo3hC+Ec6uh0iNeAO+5Y9MmKz0L5+3d41FITObiAC5HuYXHuffxQ3 yB6vXKm7rNkSlxgEJ0bd69kCA2DGbShy3+fhfFoHMUhvWDtvguAXaTNrWl8Dww8NrDkj LRWQ== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id f35-v6si1912044plh.424.2018.02.06.02.43.17; Tue, 06 Feb 2018 02:43:31 -0800 (PST) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752778AbeBFKmG (ORCPT + 99 others); Tue, 6 Feb 2018 05:42:06 -0500 Received: from lb1-smtp-cloud7.xs4all.net ([194.109.24.24]:42008 "EHLO lb1-smtp-cloud7.xs4all.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752246AbeBFKl7 (ORCPT ); Tue, 6 Feb 2018 05:41:59 -0500 Received: from [IPv6:2001:420:44c1:2579:2d7e:be56:ee2d:7ebf] ([IPv6:2001:420:44c1:2579:2d7e:be56:ee2d:7ebf]) by smtp-cloud7.xs4all.net with ESMTPA id j0hGezZck3A62j0hJeegwF; Tue, 06 Feb 2018 11:41:57 +0100 Subject: Re: [PATCH v2 8/8] platform: vivid-cec: use 64-bit arithmetic instead of 32-bit To: "Gustavo A. R. Silva" Cc: "Gustavo A. R. Silva" , Mauro Carvalho Chehab , linux-media@vger.kernel.org, linux-kernel@vger.kernel.org References: <20180205155419.Horde.WgpJoLkqF8wsBtPMp9n7V8U@gator4166.hostgator.com> From: Hans Verkuil Message-ID: <238647bc-106d-8dbf-569e-82e7968f887d@xs4all.nl> Date: Tue, 6 Feb 2018 11:41:54 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1 MIME-Version: 1.0 In-Reply-To: <20180205155419.Horde.WgpJoLkqF8wsBtPMp9n7V8U@gator4166.hostgator.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-CMAE-Envelope: MS4wfHcUCIh+7VPaJvAlId2uWiUNPJpYtGHnEj7smZ+OjSAeETLM4NGdobOV8rQsd9f1BYfrQIyykUW6Gr5XAEV8FBYBFiwWAklLvXVOiUHFTA43KiJd+YUl e8IATWcjcIaBqQU3JX8PKzu83fUppIagNg1GIWOT8Tx5y2nKamrxeJz7g8gS6npoVLihNH/3yass3I118C9rLzx0wo6xSRwyj60mGeKpL7NUgJ+V1Ht49tz9 Tns5VjkLT1NyibBBbonWWAwecWJYOxKxf4jlVLIwIkZKcwCIKNiYggCul5lgFfoGu+LT70rM8NhIZ93jwhIuAnSjzmd91Bh+8C8xv3mSCFHZdbXvVsS5XT5f kIdcNTARyTxVNcwI5gSWZwKi1o+7G6icWwokv/aC18QVZtTtCoyzO+c5s9sM/ut5NdtnGDm5 Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 02/05/18 22:54, Gustavo A. R. Silva wrote: > Hi Hans, > > Quoting Hans Verkuil : > >> On 02/05/2018 09:36 PM, Gustavo A. R. Silva wrote: >>> Add suffix ULL to constant 10 in order to give the compiler complete >>> information about the proper arithmetic to use. Notice that this >>> constant is used in a context that expects an expression of type >>> u64 (64 bits, unsigned). >>> >>> The expression len * 10 * CEC_TIM_DATA_BIT_TOTAL is currently being >>> evaluated using 32-bit arithmetic. >>> >>> Also, remove unnecessary parentheses and add a code comment to make it >>> clear what is the reason of the code change. >>> >>> Addresses-Coverity-ID: 1454996 >>> Signed-off-by: Gustavo A. R. Silva >>> --- >>> Changes in v2: >>> - Update subject and changelog to better reflect the proposed code changes. >>> - Add suffix ULL to constant instead of casting a variable. >>> - Remove unncessary parentheses. >> >> unncessary -> unnecessary >> > > Thanks for this. > >>> - Add code comment. >>> >>> drivers/media/platform/vivid/vivid-cec.c | 11 +++++++++-- >>> 1 file changed, 9 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/media/platform/vivid/vivid-cec.c >>> b/drivers/media/platform/vivid/vivid-cec.c >>> index b55d278..614787b 100644 >>> --- a/drivers/media/platform/vivid/vivid-cec.c >>> +++ b/drivers/media/platform/vivid/vivid-cec.c >>> @@ -82,8 +82,15 @@ static void vivid_cec_pin_adap_events(struct >>> cec_adapter *adap, ktime_t ts, >>> >>> if (adap == NULL) >>> return; >>> - ts = ktime_sub_us(ts, (CEC_TIM_START_BIT_TOTAL + >>> - len * 10 * CEC_TIM_DATA_BIT_TOTAL)); >>> + >>> + /* >>> + * Suffix ULL on constant 10 makes the expression >>> + * CEC_TIM_START_BIT_TOTAL + 10ULL * len * CEC_TIM_DATA_BIT_TOTAL >>> + * be evaluated using 64-bit unsigned arithmetic (u64), which >>> + * is what ktime_sub_us expects as second argument. >>> + */ >> >> That's not really the comment that I was looking for. It still doesn't >> explain *why* this is needed at all. How about something like this: >> > > In MHO the reason for the change is simply the discrepancy between the > arithmetic expected by > the function ktime_sub_us and the arithmetic in which the expression > is being evaluated. And this > has nothing to do with any particular tool. Hmm, you have a point. OK, I've looked at the other patches in this patch series as well, and the only thing I would like to see changed is the 'Addresses-Coverity-ID' line in the patches: patch 4 says: Addresses-Coverity-ID: 1324146 ("Unintentional integer overflow") but that's the only one that mentions the specific coverity error. It would be nice if that can be added to the other patches as well so we have a record of the actual coverity error. > >> /* >> * Add the ULL suffix to the constant 10 to work around a false Coverity >> * "Unintentional integer overflow" warning. Coverity isn't smart enough >> * to understand that len is always <= 16, so there is no chance of an >> * integer overflow. >> */ >> > > :P > > In my opinion it is not a good idea to tie the code to a particular tool. > There are only three appearances of the word 'Coverity' in the whole > code base, and, honestly I don't want to add more. > > So I think I will document this issue as a FP in the Coverity platform. FP? Regards, Hans