drivers/media/i2c/imx214.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
Now we try to initialize all the controls and at the very end check
ctrl_hdlr->error to check if one of them has failed.
This confuses smatch, who do not know how to track the state of
imx214->link_freq.
drivers/media/i2c/imx214.c:1109 imx214_ctrls_init() error: we previously assumed 'imx214->link_freq' could be null (see line 1017)
Fix this by exiting early on control initialization errors.
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
Right now we are handling this with a quirk in media-ci, if Dan cannot
fix smatch in a kernel cycle we should merge this patch.
---
Changes in v2:
- Fix typo in commit message commit
- Move error tag where it belongs (Thanks Hans!)
- Link to v1: https://lore.kernel.org/r/20250829-imx214-smatch-v1-1-f3d1653b48e4@chromium.org
---
drivers/media/i2c/imx214.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c
index 94ebe625c9e6ee0fb67fe1d89b48b2f1bf58ffc6..c66f0e18726c3fc15df91c37888a797bcea82134 100644
--- a/drivers/media/i2c/imx214.c
+++ b/drivers/media/i2c/imx214.c
@@ -1014,8 +1014,10 @@ static int imx214_ctrls_init(struct imx214 *imx214)
V4L2_CID_LINK_FREQ,
imx214->bus_cfg.nr_of_link_frequencies - 1,
0, imx214->bus_cfg.link_frequencies);
- if (imx214->link_freq)
- imx214->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
+ if (!imx214->link_freq)
+ goto err_init_ctrl;
+
+ imx214->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
/*
* WARNING!
@@ -1099,6 +1101,7 @@ static int imx214_ctrls_init(struct imx214 *imx214)
v4l2_ctrl_new_fwnode_properties(ctrl_hdlr, &imx214_ctrl_ops, &props);
+err_init_ctrl:
ret = ctrl_hdlr->error;
if (ret) {
v4l2_ctrl_handler_free(ctrl_hdlr);
---
base-commit: 3a8660878839faadb4f1a6dd72c3179c1df56787
change-id: 20250829-imx214-smatch-c4d4d47428d5
Best regards,
--
Ricardo Ribalda <ribalda@chromium.org>Hi Ricardo,
On Tue, Oct 14, 2025 at 11:00:17AM +0000, Ricardo Ribalda wrote:
> Now we try to initialize all the controls and at the very end check
> ctrl_hdlr->error to check if one of them has failed.
>
> This confuses smatch, who do not know how to track the state of
> imx214->link_freq.
>
> drivers/media/i2c/imx214.c:1109 imx214_ctrls_init() error: we previously assumed 'imx214->link_freq' could be null (see line 1017)
>
> Fix this by exiting early on control initialization errors.
>
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
> Right now we are handling this with a quirk in media-ci, if Dan cannot
> fix smatch in a kernel cycle we should merge this patch.
> ---
> Changes in v2:
> - Fix typo in commit message commit
> - Move error tag where it belongs (Thanks Hans!)
> - Link to v1: https://lore.kernel.org/r/20250829-imx214-smatch-v1-1-f3d1653b48e4@chromium.org
> ---
> drivers/media/i2c/imx214.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c
> index 94ebe625c9e6ee0fb67fe1d89b48b2f1bf58ffc6..c66f0e18726c3fc15df91c37888a797bcea82134 100644
> --- a/drivers/media/i2c/imx214.c
> +++ b/drivers/media/i2c/imx214.c
> @@ -1014,8 +1014,10 @@ static int imx214_ctrls_init(struct imx214 *imx214)
> V4L2_CID_LINK_FREQ,
> imx214->bus_cfg.nr_of_link_frequencies - 1,
> 0, imx214->bus_cfg.link_frequencies);
> - if (imx214->link_freq)
> - imx214->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> + if (!imx214->link_freq)
> + goto err_init_ctrl;
> +
> + imx214->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
You could do this cleaner by simply moving the assignment after the handler
error check. Some drivers do that already.
I wonder why this seems to be a problem for smatch in the imx214 driver as
the pattern is widely used across the sensor drivers.
>
> /*
> * WARNING!
> @@ -1099,6 +1101,7 @@ static int imx214_ctrls_init(struct imx214 *imx214)
>
> v4l2_ctrl_new_fwnode_properties(ctrl_hdlr, &imx214_ctrl_ops, &props);
>
> +err_init_ctrl:
> ret = ctrl_hdlr->error;
> if (ret) {
> v4l2_ctrl_handler_free(ctrl_hdlr);
>
--
Kind regards,
Sakari Ailus
Hi Sakari
On Mon, 20 Oct 2025 at 20:28, Sakari Ailus <sakari.ailus@iki.fi> wrote:
>
> Hi Ricardo,
>
> On Tue, Oct 14, 2025 at 11:00:17AM +0000, Ricardo Ribalda wrote:
> > Now we try to initialize all the controls and at the very end check
> > ctrl_hdlr->error to check if one of them has failed.
> >
> > This confuses smatch, who do not know how to track the state of
> > imx214->link_freq.
> >
> > drivers/media/i2c/imx214.c:1109 imx214_ctrls_init() error: we previously assumed 'imx214->link_freq' could be null (see line 1017)
> >
> > Fix this by exiting early on control initialization errors.
> >
> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > ---
> > Right now we are handling this with a quirk in media-ci, if Dan cannot
> > fix smatch in a kernel cycle we should merge this patch.
> > ---
> > Changes in v2:
> > - Fix typo in commit message commit
> > - Move error tag where it belongs (Thanks Hans!)
> > - Link to v1: https://lore.kernel.org/r/20250829-imx214-smatch-v1-1-f3d1653b48e4@chromium.org
> > ---
> > drivers/media/i2c/imx214.c | 7 +++++--
> > 1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c
> > index 94ebe625c9e6ee0fb67fe1d89b48b2f1bf58ffc6..c66f0e18726c3fc15df91c37888a797bcea82134 100644
> > --- a/drivers/media/i2c/imx214.c
> > +++ b/drivers/media/i2c/imx214.c
> > @@ -1014,8 +1014,10 @@ static int imx214_ctrls_init(struct imx214 *imx214)
> > V4L2_CID_LINK_FREQ,
> > imx214->bus_cfg.nr_of_link_frequencies - 1,
> > 0, imx214->bus_cfg.link_frequencies);
> > - if (imx214->link_freq)
> > - imx214->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> > + if (!imx214->link_freq)
> > + goto err_init_ctrl;
> > +
> > + imx214->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
>
> You could do this cleaner by simply moving the assignment after the handler
> error check. Some drivers do that already.
>
> I wonder why this seems to be a problem for smatch in the imx214 driver as
> the pattern is widely used across the sensor drivers.
Smatch thinks that there could be case where
imx->link_freq = NULL, and imx214_pll_update returns 0.
That is not solved by moving the assignment `imx214->link_freq->flags
|=` after if (ret)
I believe Dan is already flagged about this, but I do not think that
it will be super simple to fix in his code.
If smatch can handle this case before rc5 I will delete this patch.
Regards!
>
> >
> > /*
> > * WARNING!
> > @@ -1099,6 +1101,7 @@ static int imx214_ctrls_init(struct imx214 *imx214)
> >
> > v4l2_ctrl_new_fwnode_properties(ctrl_hdlr, &imx214_ctrl_ops, &props);
> >
> > +err_init_ctrl:
> > ret = ctrl_hdlr->error;
> > if (ret) {
> > v4l2_ctrl_handler_free(ctrl_hdlr);
> >
>
> --
> Kind regards,
>
> Sakari Ailus
--
Ricardo Ribalda
Hi Ricardo, On Mon, Oct 20, 2025 at 08:51:44PM +0200, Ricardo Ribalda wrote: > Hi Sakari > > On Mon, 20 Oct 2025 at 20:28, Sakari Ailus <sakari.ailus@iki.fi> wrote: > > > > Hi Ricardo, > > > > On Tue, Oct 14, 2025 at 11:00:17AM +0000, Ricardo Ribalda wrote: > > > Now we try to initialize all the controls and at the very end check > > > ctrl_hdlr->error to check if one of them has failed. > > > > > > This confuses smatch, who do not know how to track the state of > > > imx214->link_freq. > > > > > > drivers/media/i2c/imx214.c:1109 imx214_ctrls_init() error: we previously assumed 'imx214->link_freq' could be null (see line 1017) > > > > > > Fix this by exiting early on control initialization errors. > > > > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > > > --- > > > Right now we are handling this with a quirk in media-ci, if Dan cannot > > > fix smatch in a kernel cycle we should merge this patch. > > > --- > > > Changes in v2: > > > - Fix typo in commit message commit > > > - Move error tag where it belongs (Thanks Hans!) > > > - Link to v1: https://lore.kernel.org/r/20250829-imx214-smatch-v1-1-f3d1653b48e4@chromium.org > > > --- > > > drivers/media/i2c/imx214.c | 7 +++++-- > > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c > > > index 94ebe625c9e6ee0fb67fe1d89b48b2f1bf58ffc6..c66f0e18726c3fc15df91c37888a797bcea82134 100644 > > > --- a/drivers/media/i2c/imx214.c > > > +++ b/drivers/media/i2c/imx214.c > > > @@ -1014,8 +1014,10 @@ static int imx214_ctrls_init(struct imx214 *imx214) > > > V4L2_CID_LINK_FREQ, > > > imx214->bus_cfg.nr_of_link_frequencies - 1, > > > 0, imx214->bus_cfg.link_frequencies); > > > - if (imx214->link_freq) > > > - imx214->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY; > > > + if (!imx214->link_freq) > > > + goto err_init_ctrl; > > > + > > > + imx214->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY; > > > > You could do this cleaner by simply moving the assignment after the handler > > error check. Some drivers do that already. > > > > I wonder why this seems to be a problem for smatch in the imx214 driver as > > the pattern is widely used across the sensor drivers. > > Smatch thinks that there could be case where > > imx->link_freq = NULL, and imx214_pll_update returns 0. > > That is not solved by moving the assignment `imx214->link_freq->flags > |=` after if (ret) Did you test this? The smatch message suggests otherwise (but of course this could just turn into a different smatch error). > > I believe Dan is already flagged about this, but I do not think that > it will be super simple to fix in his code. > > If smatch can handle this case before rc5 I will delete this patch. There are other options, too, such as storing the link frequency index (the driver won't even support setting it) or the frequency itself. -- Regards, Sakari Ailus
Hi Sakai
On Mon, 20 Oct 2025 at 21:34, Sakari Ailus <sakari.ailus@iki.fi> wrote:
>
> Hi Ricardo,
>
> On Mon, Oct 20, 2025 at 08:51:44PM +0200, Ricardo Ribalda wrote:
> > Hi Sakari
> >
> > On Mon, 20 Oct 2025 at 20:28, Sakari Ailus <sakari.ailus@iki.fi> wrote:
> > >
> > > Hi Ricardo,
> > >
> > > On Tue, Oct 14, 2025 at 11:00:17AM +0000, Ricardo Ribalda wrote:
> > > > Now we try to initialize all the controls and at the very end check
> > > > ctrl_hdlr->error to check if one of them has failed.
> > > >
> > > > This confuses smatch, who do not know how to track the state of
> > > > imx214->link_freq.
> > > >
> > > > drivers/media/i2c/imx214.c:1109 imx214_ctrls_init() error: we previously assumed 'imx214->link_freq' could be null (see line 1017)
> > > >
> > > > Fix this by exiting early on control initialization errors.
> > > >
> > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > > > ---
> > > > Right now we are handling this with a quirk in media-ci, if Dan cannot
> > > > fix smatch in a kernel cycle we should merge this patch.
> > > > ---
> > > > Changes in v2:
> > > > - Fix typo in commit message commit
> > > > - Move error tag where it belongs (Thanks Hans!)
> > > > - Link to v1: https://lore.kernel.org/r/20250829-imx214-smatch-v1-1-f3d1653b48e4@chromium.org
> > > > ---
> > > > drivers/media/i2c/imx214.c | 7 +++++--
> > > > 1 file changed, 5 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c
> > > > index 94ebe625c9e6ee0fb67fe1d89b48b2f1bf58ffc6..c66f0e18726c3fc15df91c37888a797bcea82134 100644
> > > > --- a/drivers/media/i2c/imx214.c
> > > > +++ b/drivers/media/i2c/imx214.c
> > > > @@ -1014,8 +1014,10 @@ static int imx214_ctrls_init(struct imx214 *imx214)
> > > > V4L2_CID_LINK_FREQ,
> > > > imx214->bus_cfg.nr_of_link_frequencies - 1,
> > > > 0, imx214->bus_cfg.link_frequencies);
> > > > - if (imx214->link_freq)
> > > > - imx214->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> > > > + if (!imx214->link_freq)
> > > > + goto err_init_ctrl;
> > > > +
> > > > + imx214->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> > >
> > > You could do this cleaner by simply moving the assignment after the handler
> > > error check. Some drivers do that already.
> > >
> > > I wonder why this seems to be a problem for smatch in the imx214 driver as
> > > the pattern is widely used across the sensor drivers.
> >
> > Smatch thinks that there could be case where
> >
> > imx->link_freq = NULL, and imx214_pll_update returns 0.
> >
> > That is not solved by moving the assignment `imx214->link_freq->flags
> > |=` after if (ret)
>
> Did you test this? The smatch message suggests otherwise (but of course
> this could just turn into a different smatch error).
Actually smatch do not hate it :)
ribalda@ribalda:~/work/linux$ make -i W=1 C=1
CHECK="../media-ci/third_party/smatch/smatch -p=kernel"
KCFLAGS="-Wmaybe-uninitialized" drivers/media/i2c/imx214.o
CC kernel/sched/rq-offsets.s
In file included from kernel/sched/rq-offsets.c:5:
kernel/sched/sched.h: In function ‘mm_cid_get’:
kernel/sched/sched.h:3743:25: error: variable ‘cpumask’ set but not
used [-Werror=unused-but-set-variable]
3743 | struct cpumask *cpumask;
| ^~~~~~~
cc1: all warnings being treated as errors
make[2]: [scripts/Makefile.build:182: kernel/sched/rq-offsets.s] Error
1 (ignored)
/bin/sh: line 1: kernel/sched/rq-offsets.s: No such file or directory
make[2]: [Kbuild:46: include/generated/rq-offsets.h] Error 1 (ignored)
CALL scripts/checksyscalls.sh
DESCEND objtool
INSTALL libsubcmd_headers
CC drivers/media/i2c/imx214.o
CHECK drivers/media/i2c/imx214.c
ribalda@ribalda:~/work/linux$ git diff
diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c
index 94ebe625c9e6..a21461b55923 100644
--- a/drivers/media/i2c/imx214.c
+++ b/drivers/media/i2c/imx214.c
@@ -1014,8 +1014,6 @@ static int imx214_ctrls_init(struct imx214 *imx214)
V4L2_CID_LINK_FREQ,
imx214->bus_cfg.nr_of_link_frequencies - 1,
0,
imx214->bus_cfg.link_frequencies);
- if (imx214->link_freq)
- imx214->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
/*
* WARNING!
@@ -1038,9 +1036,6 @@ static int imx214_ctrls_init(struct imx214 *imx214)
imx214->hblank = v4l2_ctrl_new_std(ctrl_hdlr, &imx214_ctrl_ops,
V4L2_CID_HBLANK, hblank, hblank,
1, hblank);
- if (imx214->hblank)
- imx214->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
-
exposure_max = mode->vts_def - IMX214_EXPOSURE_OFFSET;
exposure_def = min(exposure_max, IMX214_EXPOSURE_DEFAULT);
imx214->exposure = v4l2_ctrl_new_std(ctrl_hdlr, &imx214_ctrl_ops,
@@ -1060,13 +1055,9 @@ static int imx214_ctrls_init(struct imx214 *imx214)
imx214->hflip = v4l2_ctrl_new_std(ctrl_hdlr, &imx214_ctrl_ops,
V4L2_CID_HFLIP, 0, 1, 1, 0);
- if (imx214->hflip)
- imx214->hflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
imx214->vflip = v4l2_ctrl_new_std(ctrl_hdlr, &imx214_ctrl_ops,
V4L2_CID_VFLIP, 0, 1, 1, 0);
- if (imx214->vflip)
- imx214->vflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
v4l2_ctrl_cluster(2, &imx214->hflip);
@@ -1106,6 +1097,11 @@ static int imx214_ctrls_init(struct imx214 *imx214)
return ret;
}
+ imx214->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
+ imx214->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
+ imx214->hflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
+ imx214->vflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
+
ret = imx214_pll_update(imx214);
if (ret < 0) {
v4l2_ctrl_handler_free(ctrl_hdlr);
>
> >
> > I believe Dan is already flagged about this, but I do not think that
> > it will be super simple to fix in his code.
> >
> > If smatch can handle this case before rc5 I will delete this patch.
>
> There are other options, too, such as storing the link frequency index (the
> driver won't even support setting it) or the frequency itself.
There are plenty of options :) But I am still failing to see what is
wrong with this patch.
We exit early when there is an error instead of continuing doing work
that will be useless.
If you really prefer your way I can send a v3... but we have probably
more fun work to do :P
Regards!
>
> --
> Regards,
>
> Sakari Ailus
--
Ricardo Ribalda
Hi Ricardo,
On Mon, Oct 20, 2025 at 09:58:47PM +0200, Ricardo Ribalda wrote:
> Hi Sakai
>
> On Mon, 20 Oct 2025 at 21:34, Sakari Ailus <sakari.ailus@iki.fi> wrote:
> >
> > Hi Ricardo,
> >
> > On Mon, Oct 20, 2025 at 08:51:44PM +0200, Ricardo Ribalda wrote:
> > > Hi Sakari
> > >
> > > On Mon, 20 Oct 2025 at 20:28, Sakari Ailus <sakari.ailus@iki.fi> wrote:
> > > >
> > > > Hi Ricardo,
> > > >
> > > > On Tue, Oct 14, 2025 at 11:00:17AM +0000, Ricardo Ribalda wrote:
> > > > > Now we try to initialize all the controls and at the very end check
> > > > > ctrl_hdlr->error to check if one of them has failed.
> > > > >
> > > > > This confuses smatch, who do not know how to track the state of
> > > > > imx214->link_freq.
> > > > >
> > > > > drivers/media/i2c/imx214.c:1109 imx214_ctrls_init() error: we previously assumed 'imx214->link_freq' could be null (see line 1017)
> > > > >
> > > > > Fix this by exiting early on control initialization errors.
> > > > >
> > > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > > > > ---
> > > > > Right now we are handling this with a quirk in media-ci, if Dan cannot
> > > > > fix smatch in a kernel cycle we should merge this patch.
> > > > > ---
> > > > > Changes in v2:
> > > > > - Fix typo in commit message commit
> > > > > - Move error tag where it belongs (Thanks Hans!)
> > > > > - Link to v1: https://lore.kernel.org/r/20250829-imx214-smatch-v1-1-f3d1653b48e4@chromium.org
> > > > > ---
> > > > > drivers/media/i2c/imx214.c | 7 +++++--
> > > > > 1 file changed, 5 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c
> > > > > index 94ebe625c9e6ee0fb67fe1d89b48b2f1bf58ffc6..c66f0e18726c3fc15df91c37888a797bcea82134 100644
> > > > > --- a/drivers/media/i2c/imx214.c
> > > > > +++ b/drivers/media/i2c/imx214.c
> > > > > @@ -1014,8 +1014,10 @@ static int imx214_ctrls_init(struct imx214 *imx214)
> > > > > V4L2_CID_LINK_FREQ,
> > > > > imx214->bus_cfg.nr_of_link_frequencies - 1,
> > > > > 0, imx214->bus_cfg.link_frequencies);
> > > > > - if (imx214->link_freq)
> > > > > - imx214->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> > > > > + if (!imx214->link_freq)
> > > > > + goto err_init_ctrl;
> > > > > +
> > > > > + imx214->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> > > >
> > > > You could do this cleaner by simply moving the assignment after the handler
> > > > error check. Some drivers do that already.
> > > >
> > > > I wonder why this seems to be a problem for smatch in the imx214 driver as
> > > > the pattern is widely used across the sensor drivers.
> > >
> > > Smatch thinks that there could be case where
> > >
> > > imx->link_freq = NULL, and imx214_pll_update returns 0.
> > >
> > > That is not solved by moving the assignment `imx214->link_freq->flags
> > > |=` after if (ret)
> >
> > Did you test this? The smatch message suggests otherwise (but of course
> > this could just turn into a different smatch error).
>
> Actually smatch do not hate it :)
>
> ribalda@ribalda:~/work/linux$ make -i W=1 C=1
> CHECK="../media-ci/third_party/smatch/smatch -p=kernel"
> KCFLAGS="-Wmaybe-uninitialized" drivers/media/i2c/imx214.o
> CC kernel/sched/rq-offsets.s
> In file included from kernel/sched/rq-offsets.c:5:
> kernel/sched/sched.h: In function ‘mm_cid_get’:
> kernel/sched/sched.h:3743:25: error: variable ‘cpumask’ set but not
> used [-Werror=unused-but-set-variable]
> 3743 | struct cpumask *cpumask;
> | ^~~~~~~
> cc1: all warnings being treated as errors
> make[2]: [scripts/Makefile.build:182: kernel/sched/rq-offsets.s] Error
> 1 (ignored)
> /bin/sh: line 1: kernel/sched/rq-offsets.s: No such file or directory
> make[2]: [Kbuild:46: include/generated/rq-offsets.h] Error 1 (ignored)
> CALL scripts/checksyscalls.sh
> DESCEND objtool
> INSTALL libsubcmd_headers
> CC drivers/media/i2c/imx214.o
> CHECK drivers/media/i2c/imx214.c
>
>
> ribalda@ribalda:~/work/linux$ git diff
> diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c
> index 94ebe625c9e6..a21461b55923 100644
> --- a/drivers/media/i2c/imx214.c
> +++ b/drivers/media/i2c/imx214.c
> @@ -1014,8 +1014,6 @@ static int imx214_ctrls_init(struct imx214 *imx214)
> V4L2_CID_LINK_FREQ,
>
> imx214->bus_cfg.nr_of_link_frequencies - 1,
> 0,
> imx214->bus_cfg.link_frequencies);
> - if (imx214->link_freq)
> - imx214->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
>
> /*
> * WARNING!
> @@ -1038,9 +1036,6 @@ static int imx214_ctrls_init(struct imx214 *imx214)
> imx214->hblank = v4l2_ctrl_new_std(ctrl_hdlr, &imx214_ctrl_ops,
> V4L2_CID_HBLANK, hblank, hblank,
> 1, hblank);
> - if (imx214->hblank)
> - imx214->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> -
> exposure_max = mode->vts_def - IMX214_EXPOSURE_OFFSET;
> exposure_def = min(exposure_max, IMX214_EXPOSURE_DEFAULT);
> imx214->exposure = v4l2_ctrl_new_std(ctrl_hdlr, &imx214_ctrl_ops,
> @@ -1060,13 +1055,9 @@ static int imx214_ctrls_init(struct imx214 *imx214)
>
> imx214->hflip = v4l2_ctrl_new_std(ctrl_hdlr, &imx214_ctrl_ops,
> V4L2_CID_HFLIP, 0, 1, 1, 0);
> - if (imx214->hflip)
> - imx214->hflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
>
> imx214->vflip = v4l2_ctrl_new_std(ctrl_hdlr, &imx214_ctrl_ops,
> V4L2_CID_VFLIP, 0, 1, 1, 0);
> - if (imx214->vflip)
> - imx214->vflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
>
> v4l2_ctrl_cluster(2, &imx214->hflip);
>
> @@ -1106,6 +1097,11 @@ static int imx214_ctrls_init(struct imx214 *imx214)
> return ret;
> }
>
> + imx214->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> + imx214->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> + imx214->hflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
> + imx214->vflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
> +
> ret = imx214_pll_update(imx214);
> if (ret < 0) {
> v4l2_ctrl_handler_free(ctrl_hdlr);
>
>
> >
> > >
> > > I believe Dan is already flagged about this, but I do not think that
> > > it will be super simple to fix in his code.
> > >
> > > If smatch can handle this case before rc5 I will delete this patch.
> >
> > There are other options, too, such as storing the link frequency index (the
> > driver won't even support setting it) or the frequency itself.
>
> There are plenty of options :) But I am still failing to see what is
> wrong with this patch.
Error handling is often complicated and poorly tested. If you don't need to
introduce new labels to do it, just don't. The control framework does it
nicely to simplify drivers so let's rely on it.
>
> We exit early when there is an error instead of continuing doing work
> that will be useless.
>
> If you really prefer your way I can send a v3... but we have probably
> more fun work to do :P
Yes, but I don't want this pattern to spread. That tends to happen if you
introduce it in one driver.
--
Regards,
Sakari Ailus
On 14/10/2025 13:00, Ricardo Ribalda wrote:
> Now we try to initialize all the controls and at the very end check
> ctrl_hdlr->error to check if one of them has failed.
>
> This confuses smatch, who do not know how to track the state of
> imx214->link_freq.
>
> drivers/media/i2c/imx214.c:1109 imx214_ctrls_init() error: we previously assumed 'imx214->link_freq' could be null (see line 1017)
>
> Fix this by exiting early on control initialization errors.
>
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
> Right now we are handling this with a quirk in media-ci, if Dan cannot
> fix smatch in a kernel cycle we should merge this patch.
OK, will you keep track of this? This patch is delegated to me, so if you tell me when
it should be merged, then I can do that. And if it is fixed in smatch, then you can just
drop this patch in patchwork, of course.
Until then it just stays in my TODO list.
Regards,
Hans
> ---
> Changes in v2:
> - Fix typo in commit message commit
> - Move error tag where it belongs (Thanks Hans!)
> - Link to v1: https://lore.kernel.org/r/20250829-imx214-smatch-v1-1-f3d1653b48e4@chromium.org
> ---
> drivers/media/i2c/imx214.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c
> index 94ebe625c9e6ee0fb67fe1d89b48b2f1bf58ffc6..c66f0e18726c3fc15df91c37888a797bcea82134 100644
> --- a/drivers/media/i2c/imx214.c
> +++ b/drivers/media/i2c/imx214.c
> @@ -1014,8 +1014,10 @@ static int imx214_ctrls_init(struct imx214 *imx214)
> V4L2_CID_LINK_FREQ,
> imx214->bus_cfg.nr_of_link_frequencies - 1,
> 0, imx214->bus_cfg.link_frequencies);
> - if (imx214->link_freq)
> - imx214->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> + if (!imx214->link_freq)
> + goto err_init_ctrl;
> +
> + imx214->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
>
> /*
> * WARNING!
> @@ -1099,6 +1101,7 @@ static int imx214_ctrls_init(struct imx214 *imx214)
>
> v4l2_ctrl_new_fwnode_properties(ctrl_hdlr, &imx214_ctrl_ops, &props);
>
> +err_init_ctrl:
> ret = ctrl_hdlr->error;
> if (ret) {
> v4l2_ctrl_handler_free(ctrl_hdlr);
>
> ---
> base-commit: 3a8660878839faadb4f1a6dd72c3179c1df56787
> change-id: 20250829-imx214-smatch-c4d4d47428d5
>
> Best regards,
On Tue, 14 Oct 2025 at 13:52, Hans Verkuil <hverkuil+cisco@kernel.org> wrote:
>
> On 14/10/2025 13:00, Ricardo Ribalda wrote:
> > Now we try to initialize all the controls and at the very end check
> > ctrl_hdlr->error to check if one of them has failed.
> >
> > This confuses smatch, who do not know how to track the state of
> > imx214->link_freq.
> >
> > drivers/media/i2c/imx214.c:1109 imx214_ctrls_init() error: we previously assumed 'imx214->link_freq' could be null (see line 1017)
> >
> > Fix this by exiting early on control initialization errors.
> >
> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > ---
> > Right now we are handling this with a quirk in media-ci, if Dan cannot
> > fix smatch in a kernel cycle we should merge this patch.
>
> OK, will you keep track of this? This patch is delegated to me, so if you tell me when
> it should be merged, then I can do that. And if it is fixed in smatch, then you can just
> drop this patch in patchwork, of course.
Which is the latest rc that you will feel comfortable merging this? rc5? rc6?
I can ping you then if smatch is not ready by then.
Thanks :)
>
> Until then it just stays in my TODO list.
>
> Regards,
>
> Hans
>
> > ---
> > Changes in v2:
> > - Fix typo in commit message commit
> > - Move error tag where it belongs (Thanks Hans!)
> > - Link to v1: https://lore.kernel.org/r/20250829-imx214-smatch-v1-1-f3d1653b48e4@chromium.org
> > ---
> > drivers/media/i2c/imx214.c | 7 +++++--
> > 1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c
> > index 94ebe625c9e6ee0fb67fe1d89b48b2f1bf58ffc6..c66f0e18726c3fc15df91c37888a797bcea82134 100644
> > --- a/drivers/media/i2c/imx214.c
> > +++ b/drivers/media/i2c/imx214.c
> > @@ -1014,8 +1014,10 @@ static int imx214_ctrls_init(struct imx214 *imx214)
> > V4L2_CID_LINK_FREQ,
> > imx214->bus_cfg.nr_of_link_frequencies - 1,
> > 0, imx214->bus_cfg.link_frequencies);
> > - if (imx214->link_freq)
> > - imx214->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> > + if (!imx214->link_freq)
> > + goto err_init_ctrl;
> > +
> > + imx214->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> >
> > /*
> > * WARNING!
> > @@ -1099,6 +1101,7 @@ static int imx214_ctrls_init(struct imx214 *imx214)
> >
> > v4l2_ctrl_new_fwnode_properties(ctrl_hdlr, &imx214_ctrl_ops, &props);
> >
> > +err_init_ctrl:
> > ret = ctrl_hdlr->error;
> > if (ret) {
> > v4l2_ctrl_handler_free(ctrl_hdlr);
> >
> > ---
> > base-commit: 3a8660878839faadb4f1a6dd72c3179c1df56787
> > change-id: 20250829-imx214-smatch-c4d4d47428d5
> >
> > Best regards,
>
--
Ricardo Ribalda
On 14/10/2025 13:54, Ricardo Ribalda wrote:
> On Tue, 14 Oct 2025 at 13:52, Hans Verkuil <hverkuil+cisco@kernel.org> wrote:
>>
>> On 14/10/2025 13:00, Ricardo Ribalda wrote:
>>> Now we try to initialize all the controls and at the very end check
>>> ctrl_hdlr->error to check if one of them has failed.
>>>
>>> This confuses smatch, who do not know how to track the state of
>>> imx214->link_freq.
>>>
>>> drivers/media/i2c/imx214.c:1109 imx214_ctrls_init() error: we previously assumed 'imx214->link_freq' could be null (see line 1017)
>>>
>>> Fix this by exiting early on control initialization errors.
>>>
>>> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
>>> ---
>>> Right now we are handling this with a quirk in media-ci, if Dan cannot
>>> fix smatch in a kernel cycle we should merge this patch.
>>
>> OK, will you keep track of this? This patch is delegated to me, so if you tell me when
>> it should be merged, then I can do that. And if it is fixed in smatch, then you can just
>> drop this patch in patchwork, of course.
>
> Which is the latest rc that you will feel comfortable merging this? rc5? rc6?
Late rc5.
Regards,
Hans
>
> I can ping you then if smatch is not ready by then.
>
>
> Thanks :)
>
>>
>> Until then it just stays in my TODO list.
>>
>> Regards,
>>
>> Hans
>>
>>> ---
>>> Changes in v2:
>>> - Fix typo in commit message commit
>>> - Move error tag where it belongs (Thanks Hans!)
>>> - Link to v1: https://lore.kernel.org/r/20250829-imx214-smatch-v1-1-f3d1653b48e4@chromium.org
>>> ---
>>> drivers/media/i2c/imx214.c | 7 +++++--
>>> 1 file changed, 5 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c
>>> index 94ebe625c9e6ee0fb67fe1d89b48b2f1bf58ffc6..c66f0e18726c3fc15df91c37888a797bcea82134 100644
>>> --- a/drivers/media/i2c/imx214.c
>>> +++ b/drivers/media/i2c/imx214.c
>>> @@ -1014,8 +1014,10 @@ static int imx214_ctrls_init(struct imx214 *imx214)
>>> V4L2_CID_LINK_FREQ,
>>> imx214->bus_cfg.nr_of_link_frequencies - 1,
>>> 0, imx214->bus_cfg.link_frequencies);
>>> - if (imx214->link_freq)
>>> - imx214->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
>>> + if (!imx214->link_freq)
>>> + goto err_init_ctrl;
>>> +
>>> + imx214->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
>>>
>>> /*
>>> * WARNING!
>>> @@ -1099,6 +1101,7 @@ static int imx214_ctrls_init(struct imx214 *imx214)
>>>
>>> v4l2_ctrl_new_fwnode_properties(ctrl_hdlr, &imx214_ctrl_ops, &props);
>>>
>>> +err_init_ctrl:
>>> ret = ctrl_hdlr->error;
>>> if (ret) {
>>> v4l2_ctrl_handler_free(ctrl_hdlr);
>>>
>>> ---
>>> base-commit: 3a8660878839faadb4f1a6dd72c3179c1df56787
>>> change-id: 20250829-imx214-smatch-c4d4d47428d5
>>>
>>> Best regards,
>>
>
>
© 2016 - 2025 Red Hat, Inc.