Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) client-ip=2620:137:e000::3:7;
Feedback-ID: i56a14606:Fastmail
User-Agent: Cyrus-JMAP/3.9.0-alpha0-1283-g327e3ec917-fm-20231207.002-g327e3ec9
MIME-Version: 1.0
Message-Id: <97a826ab-cd68-4494-884e-f7bd512a7bef@app.fastmail.com>
In-Reply-To: <ZXlmnmGepRnMyNfr@kekkonen.localdomain>
References: <20231212213625.3653558-1-arnd@kernel.org>
 <ZXlmnmGepRnMyNfr@kekkonen.localdomain>
Date:   Wed, 13 Dec 2023 09:39:01 +0100
From:   "Arnd Bergmann" <arnd@arndb.de>
To:     "Sakari Ailus" <sakari.ailus@linux.intel.com>,
        "Arnd Bergmann" <arnd@kernel.org>
Cc:     "Mauro Carvalho Chehab" <mchehab@kernel.org>,
        "Nathan Chancellor" <nathan@kernel.org>,
        "Jacopo Mondi" <jacopo.mondi@ideasonboard.com>,
        "Hans Verkuil" <hverkuil-cisco@xs4all.nl>,
        "laurent.pinchart" <laurent.pinchart@ideasonboard.com>,
        "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: Re: [PATCH] media: i2c: mt9m114: add CONFIG_COMMON_CLK dependency
Content-Type: text/plain
Precedence: bulk

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