* [PATCH v2] media: i2c: imx214: Exit early on control init errors @ 2025-10-14 11:00 Ricardo Ribalda 2025-10-14 11:52 ` Hans Verkuil 2025-10-20 18:28 ` Sakari Ailus 0 siblings, 2 replies; 9+ messages in thread From: Ricardo Ribalda @ 2025-10-14 11:00 UTC (permalink / raw) To: Ricardo Ribalda, Hans Verkuil; +Cc: linux-media, linux-kernel, Ricardo Ribalda 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> ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] media: i2c: imx214: Exit early on control init errors 2025-10-14 11:00 [PATCH v2] media: i2c: imx214: Exit early on control init errors Ricardo Ribalda @ 2025-10-14 11:52 ` Hans Verkuil 2025-10-14 11:54 ` Ricardo Ribalda 2025-10-20 18:28 ` Sakari Ailus 1 sibling, 1 reply; 9+ messages in thread From: Hans Verkuil @ 2025-10-14 11:52 UTC (permalink / raw) To: Ricardo Ribalda, Ricardo Ribalda; +Cc: linux-media, linux-kernel 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, ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] media: i2c: imx214: Exit early on control init errors 2025-10-14 11:52 ` Hans Verkuil @ 2025-10-14 11:54 ` Ricardo Ribalda 2025-10-14 12:04 ` Hans Verkuil 0 siblings, 1 reply; 9+ messages in thread From: Ricardo Ribalda @ 2025-10-14 11:54 UTC (permalink / raw) To: Hans Verkuil; +Cc: Ricardo Ribalda, linux-media, linux-kernel 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] media: i2c: imx214: Exit early on control init errors 2025-10-14 11:54 ` Ricardo Ribalda @ 2025-10-14 12:04 ` Hans Verkuil 0 siblings, 0 replies; 9+ messages in thread From: Hans Verkuil @ 2025-10-14 12:04 UTC (permalink / raw) To: Ricardo Ribalda; +Cc: Ricardo Ribalda, linux-media, linux-kernel 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, >> > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] media: i2c: imx214: Exit early on control init errors 2025-10-14 11:00 [PATCH v2] media: i2c: imx214: Exit early on control init errors Ricardo Ribalda 2025-10-14 11:52 ` Hans Verkuil @ 2025-10-20 18:28 ` Sakari Ailus 2025-10-20 18:51 ` Ricardo Ribalda 1 sibling, 1 reply; 9+ messages in thread From: Sakari Ailus @ 2025-10-20 18:28 UTC (permalink / raw) To: Ricardo Ribalda; +Cc: Ricardo Ribalda, Hans Verkuil, linux-media, linux-kernel 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] media: i2c: imx214: Exit early on control init errors 2025-10-20 18:28 ` Sakari Ailus @ 2025-10-20 18:51 ` Ricardo Ribalda 2025-10-20 19:34 ` Sakari Ailus 0 siblings, 1 reply; 9+ messages in thread From: Ricardo Ribalda @ 2025-10-20 18:51 UTC (permalink / raw) To: Sakari Ailus, Dan Carpenter Cc: Ricardo Ribalda, Hans Verkuil, linux-media, linux-kernel 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] media: i2c: imx214: Exit early on control init errors 2025-10-20 18:51 ` Ricardo Ribalda @ 2025-10-20 19:34 ` Sakari Ailus 2025-10-20 19:58 ` Ricardo Ribalda 0 siblings, 1 reply; 9+ messages in thread From: Sakari Ailus @ 2025-10-20 19:34 UTC (permalink / raw) To: Ricardo Ribalda Cc: Dan Carpenter, Ricardo Ribalda, Hans Verkuil, linux-media, linux-kernel 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] media: i2c: imx214: Exit early on control init errors 2025-10-20 19:34 ` Sakari Ailus @ 2025-10-20 19:58 ` Ricardo Ribalda 2025-10-20 20:04 ` Sakari Ailus 0 siblings, 1 reply; 9+ messages in thread From: Ricardo Ribalda @ 2025-10-20 19:58 UTC (permalink / raw) To: Sakari Ailus Cc: Dan Carpenter, Ricardo Ribalda, Hans Verkuil, linux-media, linux-kernel 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] media: i2c: imx214: Exit early on control init errors 2025-10-20 19:58 ` Ricardo Ribalda @ 2025-10-20 20:04 ` Sakari Ailus 0 siblings, 0 replies; 9+ messages in thread From: Sakari Ailus @ 2025-10-20 20:04 UTC (permalink / raw) To: Ricardo Ribalda Cc: Dan Carpenter, Ricardo Ribalda, Hans Verkuil, linux-media, linux-kernel 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2025-10-20 20:04 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2025-10-14 11:00 [PATCH v2] media: i2c: imx214: Exit early on control init errors Ricardo Ribalda 2025-10-14 11:52 ` Hans Verkuil 2025-10-14 11:54 ` Ricardo Ribalda 2025-10-14 12:04 ` Hans Verkuil 2025-10-20 18:28 ` Sakari Ailus 2025-10-20 18:51 ` Ricardo Ribalda 2025-10-20 19:34 ` Sakari Ailus 2025-10-20 19:58 ` Ricardo Ribalda 2025-10-20 20:04 ` Sakari Ailus
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®