From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-2.9 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_PASS, URIBL_BLOCKED,USER_AGENT_NEOMUTT autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 05658ECDFAA for ; Wed, 18 Jul 2018 10:12:35 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id A66FE2075C for ; Wed, 18 Jul 2018 10:12:34 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=linaro.org header.i=@linaro.org header.b="IEPSyd9y" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org A66FE2075C Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=linaro.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726954AbeGRKtl (ORCPT ); Wed, 18 Jul 2018 06:49:41 -0400 Received: from mail-wm0-f65.google.com ([74.125.82.65]:38387 "EHLO mail-wm0-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726142AbeGRKtl (ORCPT ); Wed, 18 Jul 2018 06:49:41 -0400 Received: by mail-wm0-f65.google.com with SMTP id 69-v6so2243993wmf.3 for ; Wed, 18 Jul 2018 03:12:30 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:content-transfer-encoding:in-reply-to :user-agent; bh=imUCsgRDqHoDd4+CkueS+QavmBqEWNumZNHqCcUy4ow=; b=IEPSyd9yX7grDssxetECIbydKk/egxkabtNzxGHryfIz3e5qu2Top+1LpzEL5SwSLG 4R1GuD7LNQo4XQZ4g+oDeU1otYAWV8lqP7o291Wf6pZsIXYN1MfZCdkZtMgAYd4Y/htD OSj+r2gwMFx/AWpfYKYTzSd70eMrYZigU9yjI= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:content-transfer-encoding :in-reply-to:user-agent; bh=imUCsgRDqHoDd4+CkueS+QavmBqEWNumZNHqCcUy4ow=; b=U4FUNK+ZzC1rs1v9azJ1LMYYzVDAJo/aI5UbcqxuzbR9hbn6jZEDT4kk2gjGDrFVAi MiqBWD+AHXZ0nQy7yEdQtvwD+dfA4dbx/YdOof4XFQvIMyrWAJlDEUR5lwL88qJ+Lr0f 29FUhAhIVRutv32yMSpsHc0BYpNTY0LulDVVEW6lF3SJqCRgC7zy/S432Ij2k/mGKbuR Z83+N93vEqCxzxfg3RVSzYBUV4jsjMRAX+lRUb7K1bfxhJ8kur/57TIjurJstQGWT4xl TNFC85OMIgzKLxXwGJGyto6nyUI/Iy9pVTZMpK6jm+GdlWdkO2ZGS5lv6LUDzkn/ueF5 6yVQ== X-Gm-Message-State: AOUpUlF8lpd0GG3cWt0zLH9FTTdg+7IaWbKlpInoNYFff+kDEewZq1QR gDISBBXnYZi1qJwIInSGmb9vaQ== X-Google-Smtp-Source: AAOMgpd2lkcQbxX07JyCrgSeuYJ5y6FF9y+OJV/orK1JHorqXZI6+Nr3+aO+N+OBEpI3arZiL0q8HA== X-Received: by 2002:a1c:9a16:: with SMTP id c22-v6mr1252816wme.114.1531908749982; Wed, 18 Jul 2018 03:12:29 -0700 (PDT) Received: from holly.lan (cpc141214-aztw34-2-0-cust773.18-1.cable.virginm.net. [86.9.19.6]) by smtp.gmail.com with ESMTPSA id f23-v6sm1450600wmc.42.2018.07.18.03.12.28 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Wed, 18 Jul 2018 03:12:29 -0700 (PDT) Date: Wed, 18 Jul 2018 11:12:27 +0100 From: Daniel Thompson To: Lee Jones Cc: Marcel Ziswiler , "linux-kernel@vger.kernel.org" , "jingoohan1@gmail.com" , "linux-pwm@vger.kernel.org" , "linux-fbdev@vger.kernel.org" , "b.zolnierkie@samsung.com" , "thierry.reding@gmail.com" , "dri-devel@lists.freedesktop.org" , "patches@linaro.org" Subject: Re: [PATCH] backlight: pwm_bl: Fix uninitialized variable Message-ID: <20180718101227.shqf54wpt4kdrsj2@holly.lan> References: <20180716210241.9457-1-daniel.thompson@linaro.org> <20180718080913.GB4641@dell> <1531902119.16896.13.camel@toradex.com> <20180718095335.GD4641@dell> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20180718095335.GD4641@dell> User-Agent: NeoMutt/20180622 Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Jul 18, 2018 at 10:53:35AM +0100, Lee Jones wrote: > On Wed, 18 Jul 2018, Marcel Ziswiler wrote: > > > On Wed, 2018-07-18 at 09:09 +0100, Lee Jones wrote: > > > On Mon, 16 Jul 2018, Daniel Thompson wrote: > > > > > > > Currently, if the DT does not define num-interpolated-steps then > > > > num_steps is undefined and the interpolation code will deploy > > > > randomly. > > > > Fix this. > > > > > > > > Fixes: 573fe6d1c25c ("backlight: pwm_bl: Linear interpolation > > > > between > > > > brightness-levels") > > > > Reported-by: Marcel Ziswiler > > > > Signed-off-by: Daniel Thompson > > > > Signed-off-by: Marcel Ziswiler > > > > > > This line is confusing. Did you guys author this patch together? > > > > Yes, I reported it and we came to a conclusion together. > > It sounds like you need to have all of the tags (except this one). :) > > Reported-by: for reporting the issue > Suggested-by: for suggesting a resolution > Acked-by: for reviewing it > Tested-by: for testing it > > Signed-off-by usually means you either wrote a significant amount of > the diffstat or you were part of the submission path. He did [I don't object to but wouldn't have used the extra brackets you brought up ;-) ]. > > > > My guess is that this line should be dropped and the RB and TB tags > > > should remain? If it was reviewed too, perhaps an AB too? > > > > I'm OK either way and do not need any explicit authorship to be > > expressed for me. > > In this instance I suggest keeping Reported-by and Tested-by. > > > > > Tested-by: Marcel Ziswiler > > > > --- > > > > drivers/video/backlight/pwm_bl.c | 17 ++++++++--------- > > > > 1 file changed, 8 insertions(+), 9 deletions(-) > > > > > > > > diff --git a/drivers/video/backlight/pwm_bl.c > > > > b/drivers/video/backlight/pwm_bl.c > > > > index 9ee4c1b735b2..e3c22b79fbcd 100644 > > > > --- a/drivers/video/backlight/pwm_bl.c > > > > +++ b/drivers/video/backlight/pwm_bl.c > > > > @@ -299,15 +299,14 @@ static int pwm_backlight_parse_dt(struct > > > > device *dev, > > > > * interpolation between each of the values of > > > > brightness levels > > > > * and creates a new pre-computed table. > > > > */ > > > > - of_property_read_u32(node, "num-interpolated- > > > > steps", > > > > - &num_steps); > > > > - > > > > - /* > > > > - * Make sure that there is at least two entries in > > > > the > > > > - * brightness-levels table, otherwise we can't > > > > interpolate > > > > - * between two points. > > > > - */ > > > > - if (num_steps) { > > > > + if ((of_property_read_u32(node, "num-interpolated- > > > > steps", > > > > + &num_steps) == 0) && > > > > num_steps) { > > > > > > This is pretty ugly, and isn't it suffering from over-bracketing? My > > > suggestion would be to break out the invocation of > > > of_property_read_u32() from the if and test only the result. > > > > > > of_property_read_u32(node, "num-interpolated-steps", > > > &num_steps); > > > > you mean: > > > > ret = of_property_read_u32(node, "num-interpolated- > > steps", &num_steps); > > > > > if (!ret && num_steps) { > > > > > > I haven't checked the underling code, but is it even feasible for > > > of_property_read_u32() to not succeed AND for num_steps to be set? > > > > > > If not, the check for !ret if superfluous and you can drop it. > > > > No, then we are back to the initial issue of num_steps potentially not > > being initialised. We really want both of_property_read_u32() to > > succeed AND num_steps to actually be set. > > I also think num_steps should be pre-initialised. > > Then it will only be set if of_property_read_u32() succeeds. > > -- > Lee Jones [李琼斯] > Linaro Services Technical Lead > Linaro.org │ Open source software for ARM SoCs > Follow Linaro: Facebook | Twitter | Blog