Message ID | 20231212213625.3653558-1-arnd@kernel.org (mailing list archive) |
---|---|
State | Changes Requested |
Delegated to: | Sakari Ailus |
Headers |
Received: from sy.mirrors.kernel.org ([147.75.48.161]) by www.linuxtv.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from <linux-media+bounces-2282-patchwork=linuxtv.org@vger.kernel.org>) id 1rDAQn-004xDr-Hf for patchwork@linuxtv.org; Tue, 12 Dec 2023 21:36:46 +0000 Received: from smtp.subspace.kernel.org (wormhole.subspace.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by sy.mirrors.kernel.org (Postfix) with ESMTPS id C24AAB20EC3 for <patchwork@linuxtv.org>; Tue, 12 Dec 2023 21:36:40 +0000 (UTC) Received: from localhost.localdomain (localhost.localdomain [127.0.0.1]) by smtp.subspace.kernel.org (Postfix) with ESMTP id 1D0D064152; Tue, 12 Dec 2023 21:36:35 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="jogJziO6" X-Original-To: linux-media@vger.kernel.org Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (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 9E5046413B; Tue, 12 Dec 2023 21:36:34 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id A13DFC433C7; Tue, 12 Dec 2023 21:36:29 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1702416993; bh=7eLZmT4c+fhFsyeFMw0JfCmjrU8I9lIkdJtLdS55xkI=; h=From:To:Cc:Subject:Date:From; b=jogJziO6KMjsDBdgTJ3ForpAxZ2uFB69WZGXd3/IfFyg5tI+o5RHURJQ5VRTxE6Fr C7Zbo3zE8fb2bRohJVE808bYOONGZLkZg+KBkwOZb/yHfUpsimO4JHhTJed8xjn6Od F2HFZr0skIQJQRqzTV/DbxXbxZG6TLjbK3B3fG+kEHSmDbz7rMlQSFjus3J1wPqEko RqOhl9hRq8kXc/4jUEJunbPBlWk+qDiAjxoZ9XlsuqqVrTkeEcADCMv/QhZvgLqX1e zZ85U+5FZbgw/vVOC6RlquCAdfIJMGzs6J0yNJuKc0qayPp5USq1o1vZsSITFzKmYm c8mZD3flToCpw== From: Arnd Bergmann <arnd@kernel.org> To: Mauro Carvalho Chehab <mchehab@kernel.org>, Nathan Chancellor <nathan@kernel.org>, Sakari Ailus <sakari.ailus@linux.intel.com>, Jacopo Mondi <jacopo.mondi@ideasonboard.com>, Hans Verkuil <hverkuil-cisco@xs4all.nl>, Laurent Pinchart <laurent.pinchart@ideasonboard.com> Cc: Arnd Bergmann <arnd@arndb.de>, Nick Desaulniers <ndesaulniers@google.com>, Bill Wendling <morbo@google.com>, Justin Stitt <justinstitt@google.com>, Hans de Goede <hdegoede@redhat.com>, Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>, linux-media@vger.kernel.org, linux-kernel@vger.kernel.org, llvm@lists.linux.dev Subject: [PATCH] media: i2c: mt9m114: add CONFIG_COMMON_CLK dependency Date: Tue, 12 Dec 2023 22:18:04 +0100 Message-Id: <20231212213625.3653558-1-arnd@kernel.org> X-Mailer: git-send-email 2.39.2 Precedence: bulk X-Mailing-List: linux-media@vger.kernel.org List-Id: <linux-media.vger.kernel.org> List-Subscribe: <mailto:linux-media+subscribe@vger.kernel.org> List-Unsubscribe: <mailto:linux-media+unsubscribe@vger.kernel.org> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-LSpam-Score: -5.4 (-----) X-LSpam-Report: No, score=-5.4 required=5.0 tests=BAYES_00=-1.9,DKIMWL_WL_HIGH=0.001,DKIM_SIGNED=0.1,DKIM_VALID=-0.1,DKIM_VALID_AU=-0.1,DKIM_VALID_EF=-0.1,MAILING_LIST_MULTI=-1,RCVD_IN_DNSWL_MED=-2.3 autolearn=ham autolearn_force=no |
Series |
media: i2c: mt9m114: add CONFIG_COMMON_CLK dependency
|
|
Commit Message
Arnd Bergmann
Dec. 12, 2023, 9:18 p.m. UTC
From: Arnd Bergmann <arnd@arndb.de> With clang-16, building without COMMON_CLK triggers a range check on udelay() because of a constant division-by-zero calculation: ld.lld: error: undefined symbol: __bad_udelay >>> referenced by mt9m114.c >>> drivers/media/i2c/mt9m114.o:(mt9m114_power_on) in archive vmlinux.a Avoid this by adding a Kconfig dependency that avoids the broken build. Fixes: 24d756e914fc ("media: i2c: Add driver for onsemi MT9M114 camera sensor") Signed-off-by: Arnd Bergmann <arnd@arndb.de> --- drivers/media/i2c/Kconfig | 1 + 1 file changed, 1 insertion(+)
Comments
Hi Arnd, On Tue, Dec 12, 2023 at 10:18:04PM +0100, Arnd Bergmann wrote: > From: Arnd Bergmann <arnd@arndb.de> > > With clang-16, building without COMMON_CLK triggers a range check on > udelay() because of a constant division-by-zero calculation: > > ld.lld: error: undefined symbol: __bad_udelay > >>> referenced by mt9m114.c > >>> drivers/media/i2c/mt9m114.o:(mt9m114_power_on) in archive vmlinux.a > > Avoid this by adding a Kconfig dependency that avoids the broken build. This sounds like an odd way to fix an issue with udelay(). On which arch does it happen? Wouldn't it be better to fix the udelay() problem in the source? A quick check reveals there are about 2400 files using udelay. > > Fixes: 24d756e914fc ("media: i2c: Add driver for onsemi MT9M114 camera sensor") > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > drivers/media/i2c/Kconfig | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig > index aae05142e191..b224c37bfd77 100644 > --- a/drivers/media/i2c/Kconfig > +++ b/drivers/media/i2c/Kconfig > @@ -228,6 +228,7 @@ config VIDEO_MT9M111 > > config VIDEO_MT9M114 > tristate "onsemi MT9M114 sensor support" > + depends on COMMON_CLK > select V4L2_CCI_I2C > help > This is a Video4Linux2 sensor-level driver for the onsemi MT9M114
On Wed, Dec 13, 2023 at 08:09:03AM +0000, Sakari Ailus wrote: > Hi Arnd, > > On Tue, Dec 12, 2023 at 10:18:04PM +0100, Arnd Bergmann wrote: > > From: Arnd Bergmann <arnd@arndb.de> > > > > With clang-16, building without COMMON_CLK triggers a range check on > > udelay() because of a constant division-by-zero calculation: > > > > ld.lld: error: undefined symbol: __bad_udelay > > >>> referenced by mt9m114.c > > >>> drivers/media/i2c/mt9m114.o:(mt9m114_power_on) in archive vmlinux.a > > > > Avoid this by adding a Kconfig dependency that avoids the broken build. > > This sounds like an odd way to fix an issue with udelay(). On which arch > does it happen? Wouldn't it be better to fix the udelay() problem in the > source? > > A quick check reveals there are about 2400 files using udelay. After discussing with Tomi, I think the driver could be improved, too, by adding checks for clock frequency and avoiding an obvious potential division by zero if clk_get_rate() happens to return 0. Switching to fsleep() would probably address the Clang 16 issue, but that doesn't seem to be the primary cause here anyway.
On Wed, Dec 13, 2023, at 09:09, Sakari Ailus wrote: > On Tue, Dec 12, 2023 at 10:18:04PM +0100, Arnd Bergmann wrote: >> From: Arnd Bergmann <arnd@arndb.de> >> >> With clang-16, building without COMMON_CLK triggers a range check on >> udelay() because of a constant division-by-zero calculation: >> >> ld.lld: error: undefined symbol: __bad_udelay >> >>> referenced by mt9m114.c >> >>> drivers/media/i2c/mt9m114.o:(mt9m114_power_on) in archive vmlinux.a >> >> Avoid this by adding a Kconfig dependency that avoids the broken build. > > This sounds like an odd way to fix an issue with udelay(). On which arch > does it happen? Wouldn't it be better to fix the udelay() problem in the > source? > > A quick check reveals there are about 2400 files using udelay. I observed this on arm, but same sanity check exists on arc, m68k, microblaze, nios2 and xtensa, all of which try to discourage overly large constant delays busy loops. On Arm that limit is any delay of over 2 miliseconds, at which time a driver should generally use either msleep() to avoid a busy-loop, or (in extreme cases) mdelay(). I first tried to rewrite the mt9m114_power_on() function to have an upper bound on the udelay, but that didn't avoid the link error because it still got into undefined C. Disabling the driver without COMMON_CLK seemed easier since it already fails to probe if mt9m114_clk_init() runs into a zero clk. Maybe we could just fall back to the soft reset when the clock rate is unknown? diff --git a/drivers/media/i2c/mt9m114.c b/drivers/media/i2c/mt9m114.c index 0a22f328981d..88a67568edf8 100644 --- a/drivers/media/i2c/mt9m114.c +++ b/drivers/media/i2c/mt9m114.c @@ -2092,6 +2092,7 @@ static void mt9m114_ifp_cleanup(struct mt9m114 *sensor) static int mt9m114_power_on(struct mt9m114 *sensor) { + long freq; int ret; /* Enable power and clocks. */ @@ -2104,9 +2105,10 @@ static int mt9m114_power_on(struct mt9m114 *sensor) if (ret < 0) goto error_regulator; + freq = clk_get_rate(sensor->clk); + /* Perform a hard reset if available, or a soft reset otherwise. */ - if (sensor->reset) { - long freq = clk_get_rate(sensor->clk); + if (sensor->reset && freq) { unsigned int duration; /* Arnd
Hi Arnd, On Wed, Dec 13, 2023 at 09:39:01AM +0100, Arnd Bergmann wrote: > On Wed, Dec 13, 2023, at 09:09, Sakari Ailus wrote: > > On Tue, Dec 12, 2023 at 10:18:04PM +0100, Arnd Bergmann wrote: > >> From: Arnd Bergmann <arnd@arndb.de> > >> > >> With clang-16, building without COMMON_CLK triggers a range check on > >> udelay() because of a constant division-by-zero calculation: > >> > >> ld.lld: error: undefined symbol: __bad_udelay > >> >>> referenced by mt9m114.c > >> >>> drivers/media/i2c/mt9m114.o:(mt9m114_power_on) in archive vmlinux.a > >> > >> Avoid this by adding a Kconfig dependency that avoids the broken build. > > > > This sounds like an odd way to fix an issue with udelay(). On which arch > > does it happen? Wouldn't it be better to fix the udelay() problem in the > > source? > > > > A quick check reveals there are about 2400 files using udelay. > > I observed this on arm, but same sanity check exists on arc, m68k, > microblaze, nios2 and xtensa, all of which try to discourage > overly large constant delays busy loops. On Arm that limit is > any delay of over 2 miliseconds, at which time a driver should > generally use either msleep() to avoid a busy-loop, or (in extreme > cases) mdelay(). > > I first tried to rewrite the mt9m114_power_on() function to > have an upper bound on the udelay, but that didn't avoid the > link error because it still got into undefined C. Disabling > the driver without COMMON_CLK seemed easier since it already > fails to probe if mt9m114_clk_init() runs into a zero clk. > > Maybe we could just fall back to the soft reset when the > clock rate is unknown? The datasheet says the input clock range is between 6 MHz and 54 MHz. We could check that, and return an error if it's not in the range. The rate is needed for other reasons, too, and the sensor is unlikely to work if it's (more than little) off. I wonder if this could address the Clang 16 issue, too? I'd replace udelay() with fsleep() in any case, and at the very least Clang should be happy with this. > > diff --git a/drivers/media/i2c/mt9m114.c b/drivers/media/i2c/mt9m114.c > index 0a22f328981d..88a67568edf8 100644 > --- a/drivers/media/i2c/mt9m114.c > +++ b/drivers/media/i2c/mt9m114.c > @@ -2092,6 +2092,7 @@ static void mt9m114_ifp_cleanup(struct mt9m114 *sensor) > > static int mt9m114_power_on(struct mt9m114 *sensor) > { > + long freq; > int ret; > > /* Enable power and clocks. */ > @@ -2104,9 +2105,10 @@ static int mt9m114_power_on(struct mt9m114 *sensor) > if (ret < 0) > goto error_regulator; > > + freq = clk_get_rate(sensor->clk); > + > /* Perform a hard reset if available, or a soft reset otherwise. */ > - if (sensor->reset) { > - long freq = clk_get_rate(sensor->clk); > + if (sensor->reset && freq) { > unsigned int duration; > > /* > > Arnd
On Wed, Dec 13, 2023 at 09:10:19AM +0000, Sakari Ailus wrote: > Hi Arnd, > > On Wed, Dec 13, 2023 at 09:39:01AM +0100, Arnd Bergmann wrote: > > On Wed, Dec 13, 2023, at 09:09, Sakari Ailus wrote: > > > On Tue, Dec 12, 2023 at 10:18:04PM +0100, Arnd Bergmann wrote: > > >> From: Arnd Bergmann <arnd@arndb.de> > > >> > > >> With clang-16, building without COMMON_CLK triggers a range check on > > >> udelay() because of a constant division-by-zero calculation: > > >> > > >> ld.lld: error: undefined symbol: __bad_udelay > > >> >>> referenced by mt9m114.c > > >> >>> drivers/media/i2c/mt9m114.o:(mt9m114_power_on) in archive vmlinux.a > > >> > > >> Avoid this by adding a Kconfig dependency that avoids the broken build. > > > > > > This sounds like an odd way to fix an issue with udelay(). On which arch > > > does it happen? Wouldn't it be better to fix the udelay() problem in the > > > source? > > > > > > A quick check reveals there are about 2400 files using udelay. > > > > I observed this on arm, but same sanity check exists on arc, m68k, > > microblaze, nios2 and xtensa, all of which try to discourage > > overly large constant delays busy loops. On Arm that limit is > > any delay of over 2 miliseconds, at which time a driver should > > generally use either msleep() to avoid a busy-loop, or (in extreme > > cases) mdelay(). > > > > I first tried to rewrite the mt9m114_power_on() function to > > have an upper bound on the udelay, but that didn't avoid the > > link error because it still got into undefined C. Disabling > > the driver without COMMON_CLK seemed easier since it already > > fails to probe if mt9m114_clk_init() runs into a zero clk. > > > > Maybe we could just fall back to the soft reset when the > > clock rate is unknown? > > The datasheet says the input clock range is between 6 MHz and 54 MHz. We > could check that, and return an error if it's not in the range. The rate is > needed for other reasons, too, and the sensor is unlikely to work if it's > (more than little) off. > > I wonder if this could address the Clang 16 issue, too? I'd replace > udelay() with fsleep() in any case, and at the very least Clang should be > happy with this. I'm fine with both of those changes. > > diff --git a/drivers/media/i2c/mt9m114.c b/drivers/media/i2c/mt9m114.c > > index 0a22f328981d..88a67568edf8 100644 > > --- a/drivers/media/i2c/mt9m114.c > > +++ b/drivers/media/i2c/mt9m114.c > > @@ -2092,6 +2092,7 @@ static void mt9m114_ifp_cleanup(struct mt9m114 *sensor) > > > > static int mt9m114_power_on(struct mt9m114 *sensor) > > { > > + long freq; > > int ret; > > > > /* Enable power and clocks. */ > > @@ -2104,9 +2105,10 @@ static int mt9m114_power_on(struct mt9m114 *sensor) > > if (ret < 0) > > goto error_regulator; > > > > + freq = clk_get_rate(sensor->clk); > > + > > /* Perform a hard reset if available, or a soft reset otherwise. */ > > - if (sensor->reset) { > > - long freq = clk_get_rate(sensor->clk); > > + if (sensor->reset && freq) { > > unsigned int duration; > > > > /*
On Wed, Dec 13, 2023, at 10:58, Laurent Pinchart wrote: > On Wed, Dec 13, 2023 at 09:10:19AM +0000, Sakari Ailus wrote: >> On Wed, Dec 13, 2023 at 09:39:01AM +0100, Arnd Bergmann wrote: >> > On Wed, Dec 13, 2023, at 09:09, Sakari Ailus wrote: >> >> The datasheet says the input clock range is between 6 MHz and 54 MHz. We >> could check that, and return an error if it's not in the range. The rate is >> needed for other reasons, too, and the sensor is unlikely to work if it's >> (more than little) off. >> >> I wonder if this could address the Clang 16 issue, too? I'd replace >> udelay() with fsleep() in any case, and at the very least Clang should be >> happy with this. > > I'm fine with both of those changes. I verified that the build failure is solved by either one. I would just do the fsleep() change in that case since that is a sensible change regardless of the undefined behavior. I'll send a v2. Arnd
diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig index aae05142e191..b224c37bfd77 100644 --- a/drivers/media/i2c/Kconfig +++ b/drivers/media/i2c/Kconfig @@ -228,6 +228,7 @@ config VIDEO_MT9M111 config VIDEO_MT9M114 tristate "onsemi MT9M114 sensor support" + depends on COMMON_CLK select V4L2_CCI_I2C help This is a Video4Linux2 sensor-level driver for the onsemi MT9M114