| Message ID | 20231212213625.3653558-1-arnd@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: i2c: mt9m114: add CONFIG_COMMON_CLK dependency | expand |
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