linux-kernel - Re: [PATCH] media: i2c: mt9m114: add CONFIG_COMMON

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20231213095849.GA2191@pendragon.ideasonboard.com>
Date:   Wed, 13 Dec 2023 11:58:49 +0200
From:   Laurent Pinchart <laurent.pinchart@...asonboard.com>
To:     Sakari Ailus <sakari.ailus@...ux.intel.com>
Cc:     Arnd Bergmann <arnd@...db.de>, Arnd Bergmann <arnd@...nel.org>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Nathan Chancellor <nathan@...nel.org>,
        Jacopo Mondi <jacopo.mondi@...asonboard.com>,
        Hans Verkuil <hverkuil-cisco@...all.nl>,
        Nick Desaulniers <ndesaulniers@...gle.com>,
        Bill Wendling <morbo@...gle.com>,
        Justin Stitt <justinstitt@...gle.com>,
        Hans de Goede <hdegoede@...hat.com>,
        Tomi Valkeinen <tomi.valkeinen@...asonboard.com>,
        linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
        llvm@...ts.linux.dev
Subject: Re: [PATCH] media: i2c: mt9m114: add CONFIG_COMMON_CLK dependency

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@...db.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;
> >  
> >                 /*

-- 
Regards,

Laurent Pinchart