From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S941560AbcIHObU (ORCPT ); Thu, 8 Sep 2016 10:31:20 -0400 Received: from mga06.intel.com ([134.134.136.31]:56856 "EHLO mga06.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S934412AbcIHObR (ORCPT ); Thu, 8 Sep 2016 10:31:17 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.30,301,1470726000"; d="scan'208";a="6142142" Subject: Re: [Intel-gfx] [PATCH 1/1] drm/i915/dsi: silence a warning about uninitialized return value To: Nicolas Iooss References: <20160904185809.31916-1-nicolas.iooss_linux@m4x.org> <7d5589b2-d59c-8092-ba38-5e791ea7ca7d@intel.com> <0030b427-a704-e67c-3ca7-3887a8cbaa7f@m4x.org> <180b7dfa-268c-14c6-5e71-b5827b209e10@m4x.org> Cc: "Vetter, Daniel" , Jani Nikula , "intel-gfx@lists.freedesktop.org" , "dri-devel@lists.freedesktop.org" , "linux-kernel@vger.kernel.org" From: Dave Gordon Organization: Intel Corporation (UK) Ltd. - Co. Reg. #1134945 - Pipers Way, Swindon SN3 1RJ Message-ID: <8a1da06f-70f0-8903-0a8f-fbe544501069@intel.com> Date: Thu, 8 Sep 2016 15:31:13 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0 MIME-Version: 1.0 In-Reply-To: <180b7dfa-268c-14c6-5e71-b5827b209e10@m4x.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 08/09/16 00:02, Nicolas Iooss wrote: > On 07/09/16 18:03, Dave Gordon wrote: >> On 06/09/16 21:36, Nicolas Iooss wrote: >>> On 06/09/16 12:21, Dave Gordon wrote: >>>> On 04/09/16 19:58, Nicolas Iooss wrote: >>>>> When building the kernel with clang and some warning flags, the >>>>> compiler >>>>> reports that the return value of dcs_get_backlight() may be >>>>> uninitialized: >>>>> >>>>> drivers/gpu/drm/i915/intel_dsi_dcs_backlight.c:53:2: error: >>>>> variable >>>>> 'data' is used uninitialized whenever 'for' loop exits because its >>>>> condition is false [-Werror,-Wsometimes-uninitialized] >>>>> for_each_dsi_port(port, intel_dsi->dcs_backlight_ports) { >>>>> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >>>>> drivers/gpu/drm/i915/intel_dsi.h:126:49: note: expanded from macro >>>>> 'for_each_dsi_port' >>>>> #define for_each_dsi_port(__port, __ports_mask) >>>>> for_each_port_masked(__port, >>>>> __ports_mask) >>>>> >>>>> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >>>>> drivers/gpu/drm/i915/i915_drv.h:322:26: note: expanded from macro >>>>> 'for_each_port_masked' >>>>> for ((__port) = PORT_A; (__port) < I915_MAX_PORTS; >>>>> (__port)++) \ >>>>> ^~~~~~~~~~~~~~~~~~~~~~~~~ >>>>> drivers/gpu/drm/i915/intel_dsi_dcs_backlight.c:60:9: note: >>>>> uninitialized use occurs here >>>>> return data; >>>>> ^~~~ >>>>> >>>>> As intel_dsi->dcs_backlight_ports seems to be always initialized to a >>>>> non-null value, the content of the for loop is always executed and >>>>> there >>>>> is no bug in the current code. Nevertheless the compiler has no way of >>>>> knowing that assumption, so initialize variable 'data' to silence the >>>>> warning here. >>>>> >>>>> Signed-off-by: Nicolas Iooss >>>> >>>> Interesting ... there are two things that could lead to this (possibly) >>>> incorrect analysis. Either it thinks the loop could be executed zero >>>> times, which would be a deficiency in the compiler, as the initialiser >>>> and loop bound are both known (different) constants: >>>> >>>> enum port { >>>> PORT_A = 0, >>>> PORT_B, >>>> PORT_C, >>>> PORT_D, >>>> PORT_E, >>>> I915_MAX_PORTS >>>> }; >>>> >>>> or, it doesn't understand that because we've passed &data to another >>>> function, it can have been set by the callee. It may be extra confusing >>>> that the callee takes (void *); or it may be being ultra-sophisticated >>>> in its analysis and noted that in one error path data is *not* set (and >>>> we then discard the error and use data anyway). As an experiment, you >>>> could try: >>> >>> The code that the compiler sees is not a simple loop other enum 'port' >>> but "for_each_dsi_port(port, intel_dsi->dcs_backlight_ports) {", which >>> is expanded [1] to: >>> >>> for ((port) = PORT_A; (port) < I915_MAX_PORTS; (port)++) >>> if (!((intel_dsi->dcs_backlight_ports) & (1 << (port)))) {} else { >>> >>> This is why I spoke of intel_dsi->dcs_backlight_ports in my description: >>> if it is zero, the body of the loop is never run. >>> >>> As for the analyses of calls using &data, clang does not warn about the >>> variable being maybe uninitialized following a call. This is quite >>> expected as this would lead to too many false positives, even though it >>> may miss some bugs. >>> >>>> static u8 mipi_dsi_dcs_read1(struct mipi_dsi_device *dsi_device, u8 cmd) >>>> { >>>> u8 data = 0; >>>> >>>> mipi_dsi_dcs_read(dsi_device, cmd, &data, sizeof(data)); >>>> >>>> return data; >>>> } >>>> >>>> static u32 dcs_get_backlight(struct intel_connector *connector) >>>> { >>>> struct intel_encoder *encoder = connector->encoder; >>>> struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base); >>>> struct mipi_dsi_device *dsi_device; >>>> enum port port; >>>> u8 data; >>>> >>>> /* FIXME: Need to take care of 16 bit brightness level */ >>>> for_each_dsi_port(port, intel_dsi->dcs_backlight_ports) { >>>> dsi_device = intel_dsi->dsi_hosts[port]->device; >>>> data = mipi_dsi_dcs_read1(dsi_device, >>>> MIPI_DCS_GET_DISPLAY_BRIGHTNESS); >>>> break; >>>> } >>>> >>>> return data; >>>> } >>>> >>>> If it complains about that then it's a shortcoming in the loop analysis. >>> >>> It complains (in dcs_get_backlight), because for_each_dsi_port() still >>> hides an 'if' condition. >> >> So it does, In that case the complaint is really quite reasonable. >> >>>> If not you could try: >>>> >>>> static u8 mipi_dsi_dcs_read1(struct mipi_dsi_device *dsi_device, u8 cmd) >>>> { >>>> u8 data; >>>> ssize_t nbytes = sizeof(data); >>>> >>>> nbytes = mipi_dsi_dcs_read(dsi_device, cmd, &data, nbytes); >>>> return nbytes == sizeof(data) ? data : 0; >>>> } >>>> >>>> and if complains about that then it doesn't understand that passing >>>> &data allows it to be set. If it doesn't complain about this version, >>>> then the original error was actually correct, in the sense that data can >>>> indeed be used uninitialised if certain error paths can be taken. >>> >>> clang did not complain with this last case. >> >> It probably should have, since the (hidden) if() could still result in >> this function never being called. Oh well ... > > Sorry, my message was not clear enough. The compiler did not complain in > mipi_dsi_dcs_read1() in the last case, but the -Wsometimes-uninitialized > warning was still there for variable 'data' in dcs_get_backlight(), as > expected because of the "hidden if". > > Nicolas OK, thanks. BTW do you see any "may be used uninitialised" warnings in gen{6,8}_ggtt_insert_entries()? In particular for_each_sgt_dma(addr, sgt_iter, st) { gtt_entry = gen8_pte_encode(addr, level, true); gen8_set_pte(>t_entries[i++], gtt_entry); } [snip] if (i != 0) WARN_ON(readq(>t_entries[i-1]) != gtt_entry); Or maybe clang is smart enough to realise that the WARN_ON() is reachable only if the gen8_set_pte() has already been executed at least once? .Dave.