[PATCH] media: i2c: mt9m114: add CONFIG_COMMON_CLK dependency

[PATCH] media: i2c: mt9m114: add CONFIG_COMMON_CLK dependency

[Arnd Bergmann]

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(+)

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
-- 
2.39.2

Re: [PATCH] media: i2c: mt9m114: add CONFIG_COMMON_CLK dependency

[Sakari Ailus]

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

-- 
Regards,

Sakari Ailus

Re: [PATCH] media: i2c: mt9m114: add CONFIG_COMMON_CLK dependency

[Sakari Ailus]

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.

-- 
Sakari Ailus

Re: [PATCH] media: i2c: mt9m114: add CONFIG_COMMON_CLK dependency

[Arnd Bergmann]

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

Re: [PATCH] media: i2c: mt9m114: add CONFIG_COMMON_CLK dependency

[Sakari Ailus]

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

-- 
Regards,

Sakari Ailus

Re: [PATCH] media: i2c: mt9m114: add CONFIG_COMMON_CLK dependency

[Laurent Pinchart]

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;
> >  
> >                 /*

-- 
Regards,

Laurent Pinchart

Re: [PATCH] media: i2c: mt9m114: add CONFIG_COMMON_CLK dependency

[Arnd Bergmann]

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