From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mgamail.intel.com (mgamail.intel.com [198.175.65.18]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 9E18B154C0D for ; Tue, 21 Jan 2025 08:03:08 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=198.175.65.18 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1737446591; cv=none; b=o5/RBm0Epn5DyuKX86Ke238L7OW9aBmEoOUBVdAqCGDxfAZIL4VMoe2KFKbZjokvyOAenx+k/KV/A4frlujxOmuikClneIRf7pjtD23WiFMnYc9p8YjorTr/DQR8iCwI2zKgNErz0yJmnW79aSRSJwhrB4poQYGB3EypSMfKSKM= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1737446591; c=relaxed/simple; bh=4IlwIyOiNVaUb2wT21B1H5m3caBcaE3w6fBDrOeTH8w=; h=From:To:Cc:Subject:In-Reply-To:References:Date:Message-ID: MIME-Version:Content-Type; b=hJjyhvAIVPyMthPPRxj4QqyHDbbVRfEsVRcx7DBXf+orXUsGM7eymxi0BpTQ2AQzWBuNJdZ9LXn8HvX2EkYElGUjDSLwMj7QXMI/E5FSlG99oZWPhydOzKt6wiaLNicvBGoRFbSca2JSPbmDsVMMF891YxE0EUJZ+MdQCTZ5bts= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.intel.com; spf=pass smtp.mailfrom=intel.com; dkim=pass (2048-bit key) header.d=intel.com header.i=@intel.com header.b=F1tQPv0S; arc=none smtp.client-ip=198.175.65.18 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.intel.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=intel.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=intel.com header.i=@intel.com header.b="F1tQPv0S" DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1737446589; x=1768982589; h=from:to:cc:subject:in-reply-to:references:date: message-id:mime-version; bh=4IlwIyOiNVaUb2wT21B1H5m3caBcaE3w6fBDrOeTH8w=; b=F1tQPv0SPIRDSUUxPKzj7VSqGTu5WMvyQW028ihta50ig7zYuAB0OWnv IEnrvHXHn1dvgwFqdk8lv8v91WnQPz2FezpQu21BLXXmM70SV6ZNpEqBe dmzQgdDCfNaI8teCAqcn3v9PEcKXUQ1/DpUdCaUp2bK6EfubYyXBxWcD7 lOBcFKq29ofciMwdo2pXO63NFMiH2aeCsiYQRmTZgOoVnDr95skZfECaj EZeJJZMdvBVH5AYfKwZdmbJLaVNwvdyr5hoZBMwHEQNEok4T+vbKADqUo gWIZQ5kz5fMIV8h+nZfGwi7J9srDw9DGiQljRpuA4LDSzzpsNmWnzqzfW w==; X-CSE-ConnectionGUID: qdJU944GTb+Ak8v/0G/jbA== X-CSE-MsgGUID: q63Kj5lTQGmu89HLcVtESg== X-IronPort-AV: E=McAfee;i="6700,10204,11314"; a="37955295" X-IronPort-AV: E=Sophos;i="6.12,310,1728975600"; d="scan'208";a="37955295" Received: from fmviesa008.fm.intel.com ([10.60.135.148]) by orvoesa110.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 21 Jan 2025 00:03:08 -0800 X-CSE-ConnectionGUID: Y5KUXnF8Qk6bo2tYHN/krA== X-CSE-MsgGUID: p57F4Wr1Qpq0+yr734Ra6A== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="6.13,221,1732608000"; d="scan'208";a="106853390" Received: from mklonows-mobl1.ger.corp.intel.com (HELO localhost) ([10.245.246.186]) by fmviesa008-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 21 Jan 2025 00:03:03 -0800 From: Jani Nikula To: Guenter Roeck Cc: Rodrigo Vivi , Joonas Lahtinen , Tvrtko Ursulin , David Airlie , Simona Vetter , intel-gfx@lists.freedesktop.org, intel-xe@lists.freedesktop.org, dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org, Guenter Roeck , Linus Torvalds , David Laight , Andy Shevchenko Subject: Re: [PATCH] drm/i915/backlight: Return immediately when scale() finds invalid parameters In-Reply-To: <20250121061746.2730572-1-linux@roeck-us.net> Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo References: <20250121061746.2730572-1-linux@roeck-us.net> Date: Tue, 21 Jan 2025 10:03:00 +0200 Message-ID: <87o700ppaj.fsf@intel.com> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain On Mon, 20 Jan 2025, Guenter Roeck wrote: > The scale() functions detects invalid parameters, but continues > its calculations anyway. This causes bad results if negative values > are used for unsigned operations. Worst case, a division by 0 error > will be seen if source_min == source_max. > > On top of that, after v6.13, the sequence of WARN_ON() followed by clamp() > may result in a build error with gcc 13.x. > > drivers/gpu/drm/i915/display/intel_backlight.c: In function 'scale': > include/linux/compiler_types.h:542:45: error: > call to '__compiletime_assert_415' declared with attribute error: > clamp() low limit source_min greater than high limit source_max > > This happens if the compiler decides to rearrange the code as follows. > > if (source_min > source_max) { > WARN(..); > /* Do the clamp() knowing that source_min > source_max */ > source_val = clamp(source_val, source_min, source_max); > } else { > /* Do the clamp knowing that source_min <= source_max */ > source_val = clamp(source_val, source_min, source_max); > } > > Fix the problem by evaluating the return values from WARN_ON and returning > immediately after a warning. While at it, fix divide by zero error seen > if source_min == source_max. Thanks for the effort in tracking this down. > Analyzed-by: Linus Torvalds > Suggested-by: Linus Torvalds > Suggested-by: David Laight > Cc: David Laight > Cc: Jani Nikula > Cc: Andy Shevchenko > Signed-off-by: Guenter Roeck > --- > drivers/gpu/drm/i915/display/intel_backlight.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_backlight.c b/drivers/gpu/drm/i915/display/intel_backlight.c > index 3f81a726cc7d..ad49bd4a1c12 100644 > --- a/drivers/gpu/drm/i915/display/intel_backlight.c > +++ b/drivers/gpu/drm/i915/display/intel_backlight.c > @@ -40,8 +40,11 @@ static u32 scale(u32 source_val, > { > u64 target_val; > > - WARN_ON(source_min > source_max); > - WARN_ON(target_min > target_max); > + if (WARN_ON(target_min > target_max)) > + return target_min; > + > + if (WARN_ON(source_min > source_max) || source_min == source_max) > + return target_min + (target_max - target_min) / 2; There's really no need to be this fancy, though. Years down the line someone's going to think that mean value calculation is something we need to preserve, but we don't. I'd just return target_max everywhere. And source_min == source_max could be a warn too. BR, Jani. > > /* defensive */ > source_val = clamp(source_val, source_min, source_max); -- Jani Nikula, Intel