Re: [PATCH v3 2/6] media: i2c: mt9p031: Rewrite assignment to make smatch happy

Re: [PATCH v3 2/6] media: i2c: mt9p031: Rewrite assignment to make smatch happy

[Laurent Pinchart]

On Mon, May 04, 2026 at 06:54:05AM +0000, Ricardo Ribalda wrote:
> The current code makes smatch a bit uncomfortable:
> drivers/media/i2c/mt9p031.c:799 mt9p031_s_ctrl() warn: assigning (-1952) to unsigned variable 'data'
> 
> Probably because smatch is not clever enough (yet). Do a simple rewrite
> to make sure that smatch understands what we are doing here.
> 
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/media/i2c/mt9p031.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/i2c/mt9p031.c b/drivers/media/i2c/mt9p031.c
> index ea5d43d925ff..8dc57eeba606 100644
> --- a/drivers/media/i2c/mt9p031.c
> +++ b/drivers/media/i2c/mt9p031.c
> @@ -796,7 +796,8 @@ static int mt9p031_s_ctrl(struct v4l2_ctrl *ctrl)
>  			data = (1 << 6) | (ctrl->val >> 1);
>  		} else {
>  			ctrl->val &= ~7;
> -			data = ((ctrl->val - 64) << 5) | (1 << 6) | 32;
> +			data = ((ctrl->val - 64) >> 3) & 0x7f;
> +			data = (data << 8) | (1 << 6) | 32;
>  		}
>  
>  		return mt9p031_write(client, MT9P031_GLOBAL_GAIN, data);

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 1/6] media: v4l2-dev: Add range check for vdev->minor

[Laurent Pinchart]

On Mon, May 04, 2026 at 06:54:04AM +0000, Ricardo Ribalda wrote:
> If the fixed minor ranges are not properly set we could end up in a
> situation where the calculated minor is invalid. Add a check for this in
> the code to make it more robust.

If it was just for that, we could define the ranges in a way that could
not lead to future programmatic errors.

> This check also fixes the following false positive smatch warning:
> 
> drivers/media/v4l2-core/v4l2-dev.c:1036 __video_register_device() error: buffer overflow 'video_devices' 256 <= 288
> drivers/media/v4l2-core/v4l2-dev.c:1043 __video_register_device() error: buffer overflow 'video_devices' 256 <= 288
> drivers/media/v4l2-core/v4l2-dev.c:1101 __video_register_device() error: buffer overflow 'video_devices' 256 <= 288
> 
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/v4l2-core/v4l2-dev.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
> index 6ce623a1245a..5516b2bbb08f 100644
> --- a/drivers/media/v4l2-core/v4l2-dev.c
> +++ b/drivers/media/v4l2-core/v4l2-dev.c
> @@ -1032,6 +1032,11 @@ int __video_register_device(struct video_device *vdev,
>  	vdev->minor = i + minor_offset;
>  	vdev->num = nr;
>  
> +	if (WARN_ON(vdev->minor >= VIDEO_NUM_DEVICES)) {
> +		mutex_unlock(&videodev_lock);

I may get tempted to convert code to using scoped guards at some point.

> +		return -EINVAL;
> +	}
> +

I'm annoyed by the proliferation of workarounds for smatch false
positives that generate useless code :-/ This is in particular is not a
big deal though, so

Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

but I don't want to continue in this direction with every new kernel
release. We need a way to tell smatch that this is safe with incurring
an runtime cost.

>  	/* Should not happen since we thought this minor was free */
>  	if (WARN_ON(video_devices[vdev->minor])) {
>  		mutex_unlock(&videodev_lock);
> 

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 6/6] media: amlogic-c3: Add validations for ae and awb config

[Laurent Pinchart]

On Mon, May 04, 2026 at 09:19:30AM +0200, Jacopo Mondi wrote:
> On Mon, May 04, 2026 at 06:54:09AM +0000, Ricardo Ribalda wrote:
> > Avoid invalid memory access if the zones_num is bigger than
> > zone_weight.
> >
> > This patch fixes the following smatch errors:
> > drivers/media/platform/amlogic/c3/isp/c3-isp-params.c:111 c3_isp_params_awb_wt() error: buffer overflow 'cfg->zone_weight' 768 <= u32max
> > drivers/media/platform/amlogic/c3/isp/c3-isp-params.c:111 c3_isp_params_awb_wt() error: buffer overflow 'cfg->zone_weight' 768 <= u32max
> > drivers/media/platform/amlogic/c3/isp/c3-isp-params.c:227 c3_isp_params_ae_wt() error: buffer overflow 'cfg->zone_weight' 255 <= u32max
> > drivers/media/platform/amlogic/c3/isp/c3-isp-params.c:227 c3_isp_params_ae_wt() error: buffer overflow 'cfg->zone_weight' 255 <= u32max
> >
> > Cc: stable@vger.kernel.org
> > Fixes: fb2e135208f3 ("media: platform: Add C3 ISP driver")
> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > ---
> >  drivers/media/platform/amlogic/c3/isp/c3-isp-params.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/drivers/media/platform/amlogic/c3/isp/c3-isp-params.c b/drivers/media/platform/amlogic/c3/isp/c3-isp-params.c
> > index 6f9ca7a7dd88..aec3eed0e443 100644
> > --- a/drivers/media/platform/amlogic/c3/isp/c3-isp-params.c
> > +++ b/drivers/media/platform/amlogic/c3/isp/c3-isp-params.c
> > @@ -104,6 +104,8 @@ static void c3_isp_params_awb_wt(struct c3_isp_device *isp,
> >  	c3_isp_write(isp, ISP_AWB_BLK_WT_ADDR, 0);
> >
> >  	zones_num = cfg->horiz_zones_num * cfg->vert_zones_num;
> > +	if (zones_num > C3_ISP_AWB_MAX_ZONES)
> > +		zones_num = C3_ISP_AWB_MAX_ZONES;
> 
> Or
>         zones_num = min(cfg->horiz_zones_num * cfg->vert_zones_num,
>                         C3_ISP_AWB_MAX_ZONES);

I have a slight preference for that, but both options work for me.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> Whatever you prefer:
> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> 
> >
> >  	/* Need to write 8 weights at once */
> >  	for (i = 0; i < zones_num / 8; i++) {
> > @@ -220,6 +222,8 @@ static void c3_isp_params_ae_wt(struct c3_isp_device *isp,
> >  	c3_isp_write(isp, ISP_AE_BLK_WT_ADDR, 0);
> >
> >  	zones_num = cfg->horiz_zones_num * cfg->vert_zones_num;
> > +	if (zones_num > C3_ISP_AE_MAX_ZONES)
> > +		zones_num = C3_ISP_AE_MAX_ZONES;
> >
> >  	/* Need to write 8 weights at once */
> >  	for (i = 0; i < zones_num / 8; i++) {

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 1/6] media: v4l2-dev: Add range check for vdev->minor

[Ricardo Ribalda]

Hi Laurent

On Wed, 6 May 2026 at 01:12, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> On Mon, May 04, 2026 at 06:54:04AM +0000, Ricardo Ribalda wrote:
> > If the fixed minor ranges are not properly set we could end up in a
> > situation where the calculated minor is invalid. Add a check for this in
> > the code to make it more robust.
>
> If it was just for that, we could define the ranges in a way that could
> not lead to future programmatic errors.
>
> > This check also fixes the following false positive smatch warning:
> >
> > drivers/media/v4l2-core/v4l2-dev.c:1036 __video_register_device() error: buffer overflow 'video_devices' 256 <= 288
> > drivers/media/v4l2-core/v4l2-dev.c:1043 __video_register_device() error: buffer overflow 'video_devices' 256 <= 288
> > drivers/media/v4l2-core/v4l2-dev.c:1101 __video_register_device() error: buffer overflow 'video_devices' 256 <= 288
> >
> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > ---
> >  drivers/media/v4l2-core/v4l2-dev.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
> > index 6ce623a1245a..5516b2bbb08f 100644
> > --- a/drivers/media/v4l2-core/v4l2-dev.c
> > +++ b/drivers/media/v4l2-core/v4l2-dev.c
> > @@ -1032,6 +1032,11 @@ int __video_register_device(struct video_device *vdev,
> >       vdev->minor = i + minor_offset;
> >       vdev->num = nr;
> >
> > +     if (WARN_ON(vdev->minor >= VIDEO_NUM_DEVICES)) {
> > +             mutex_unlock(&videodev_lock);
>
> I may get tempted to convert code to using scoped guards at some point.
>
> > +             return -EINVAL;
> > +     }
> > +
>
> I'm annoyed by the proliferation of workarounds for smatch false
> positives that generate useless code :-/ This is in particular is not a
> big deal though, so
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
>
> but I don't want to continue in this direction with every new kernel
> release. We need a way to tell smatch that this is safe with incurring
> an runtime cost.

To provide some context, in this release alone we have found two "Fixes:".

Do you have any data regarding the runtime cost of these new checks?
Most of the time, the compiler simply optimizes them out, so the
impact is either zero or minimal.

The added checks make the code more robust for future refactoring and
serve as useful documentation. Furthermore, when false positives occur
in new code, they force the author to write more idiomatic code, which
improves maintainability. We have a deficit of maintainers, not
authors.

So, I DO want to continue in this direction. I am very grateful for
these static analysis tools; they truly make our code more
maintainable.

Regards!


>
> >       /* Should not happen since we thought this minor was free */
> >       if (WARN_ON(video_devices[vdev->minor])) {
> >               mutex_unlock(&videodev_lock);
> >
>
> --
> Regards,
>
> Laurent Pinchart



-- 
Ricardo Ribalda

Re: [PATCH v3 1/6] media: v4l2-dev: Add range check for vdev->minor

[Laurent Pinchart]

On Wed, May 06, 2026 at 08:48:06AM +0200, Ricardo Ribalda wrote:
> On Wed, 6 May 2026 at 01:12, Laurent Pinchart wrote:
> > On Mon, May 04, 2026 at 06:54:04AM +0000, Ricardo Ribalda wrote:
> > > If the fixed minor ranges are not properly set we could end up in a
> > > situation where the calculated minor is invalid. Add a check for this in
> > > the code to make it more robust.
> >
> > If it was just for that, we could define the ranges in a way that could
> > not lead to future programmatic errors.
> >
> > > This check also fixes the following false positive smatch warning:
> > >
> > > drivers/media/v4l2-core/v4l2-dev.c:1036 __video_register_device() error: buffer overflow 'video_devices' 256 <= 288
> > > drivers/media/v4l2-core/v4l2-dev.c:1043 __video_register_device() error: buffer overflow 'video_devices' 256 <= 288
> > > drivers/media/v4l2-core/v4l2-dev.c:1101 __video_register_device() error: buffer overflow 'video_devices' 256 <= 288
> > >
> > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > > ---
> > >  drivers/media/v4l2-core/v4l2-dev.c | 5 +++++
> > >  1 file changed, 5 insertions(+)
> > >
> > > diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
> > > index 6ce623a1245a..5516b2bbb08f 100644
> > > --- a/drivers/media/v4l2-core/v4l2-dev.c
> > > +++ b/drivers/media/v4l2-core/v4l2-dev.c
> > > @@ -1032,6 +1032,11 @@ int __video_register_device(struct video_device *vdev,
> > >       vdev->minor = i + minor_offset;
> > >       vdev->num = nr;
> > >
> > > +     if (WARN_ON(vdev->minor >= VIDEO_NUM_DEVICES)) {
> > > +             mutex_unlock(&videodev_lock);
> >
> > I may get tempted to convert code to using scoped guards at some point.
> >
> > > +             return -EINVAL;
> > > +     }
> > > +
> >
> > I'm annoyed by the proliferation of workarounds for smatch false
> > positives that generate useless code :-/ This is in particular is not a
> > big deal though, so
> >
> > Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> >
> > but I don't want to continue in this direction with every new kernel
> > release. We need a way to tell smatch that this is safe with incurring
> > an runtime cost.
> 
> To provide some context, in this release alone we have found two "Fixes:".
> 
> Do you have any data regarding the runtime cost of these new checks?
> Most of the time, the compiler simply optimizes them out, so the
> impact is either zero or minimal.

If the compiler can optimize them out, then I expect the static analysis
tools to also get the ability to avoid those false positives.

> The added checks make the code more robust for future refactoring and
> serve as useful documentation.

In this specific case, if we want to make the code more robust, I think
we should have an array of minor offsets and calculate the minor ranges
based on that. It would avoid offsets and ranges getting out of sync. I
don't think it's worth it though, because I don't foresee we will ever
change ranges for the fixed minors case.

> Furthermore, when false positives occur
> in new code, they force the author to write more idiomatic code, which
> improves maintainability.

Up to a point only. Sometimes guarantees are provided in a very remote
place. For instance, control values have ranges that are set when
creating the control, and enforced by the control framework. Adding
range checks to .s_ctrl() is redundant, but I don't expect static
analysis tools to be able to understand that the value is guaranteed to
be within the range (especially given that we support modifying ranges
at runtime). How can we avoid those redundant checks (or mask
operations, as in the mt9p031 patch in this series) ?

> We have a deficit of maintainers, not
> authors.
> 
> So, I DO want to continue in this direction. I am very grateful for
> these static analysis tools; they truly make our code more
> maintainable.

I do like smatch a lot, it points out real issues. My concern is that we
have code that offers guarantees in ways that are not visible to static
analysis tools (or to compilers), and having to add more runtime checks
everywhere to silence warning is not nice. Those checks can over time
become redundant (because tools improve, or because we remove the code),
and will likely not be removed because nobody notices.

There's a broader question here of how to connect pieces of code where
one piece offers a guarantee that the other piece depends on, in a way
that can be exposed to tools.

> > >       /* Should not happen since we thought this minor was free */
> > >       if (WARN_ON(video_devices[vdev->minor])) {
> > >               mutex_unlock(&videodev_lock);
> > >

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 1/6] media: v4l2-dev: Add range check for vdev->minor

[Ricardo Ribalda]

Hi Laurent

On Wed, 6 May 2026 at 13:18, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> On Wed, May 06, 2026 at 08:48:06AM +0200, Ricardo Ribalda wrote:
> > On Wed, 6 May 2026 at 01:12, Laurent Pinchart wrote:
> > > On Mon, May 04, 2026 at 06:54:04AM +0000, Ricardo Ribalda wrote:
> > > > If the fixed minor ranges are not properly set we could end up in a
> > > > situation where the calculated minor is invalid. Add a check for this in
> > > > the code to make it more robust.
> > >
> > > If it was just for that, we could define the ranges in a way that could
> > > not lead to future programmatic errors.
> > >
> > > > This check also fixes the following false positive smatch warning:
> > > >
> > > > drivers/media/v4l2-core/v4l2-dev.c:1036 __video_register_device() error: buffer overflow 'video_devices' 256 <= 288
> > > > drivers/media/v4l2-core/v4l2-dev.c:1043 __video_register_device() error: buffer overflow 'video_devices' 256 <= 288
> > > > drivers/media/v4l2-core/v4l2-dev.c:1101 __video_register_device() error: buffer overflow 'video_devices' 256 <= 288
> > > >
> > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > > > ---
> > > >  drivers/media/v4l2-core/v4l2-dev.c | 5 +++++
> > > >  1 file changed, 5 insertions(+)
> > > >
> > > > diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
> > > > index 6ce623a1245a..5516b2bbb08f 100644
> > > > --- a/drivers/media/v4l2-core/v4l2-dev.c
> > > > +++ b/drivers/media/v4l2-core/v4l2-dev.c
> > > > @@ -1032,6 +1032,11 @@ int __video_register_device(struct video_device *vdev,
> > > >       vdev->minor = i + minor_offset;
> > > >       vdev->num = nr;
> > > >
> > > > +     if (WARN_ON(vdev->minor >= VIDEO_NUM_DEVICES)) {
> > > > +             mutex_unlock(&videodev_lock);
> > >
> > > I may get tempted to convert code to using scoped guards at some point.
> > >
> > > > +             return -EINVAL;
> > > > +     }
> > > > +
> > >
> > > I'm annoyed by the proliferation of workarounds for smatch false
> > > positives that generate useless code :-/ This is in particular is not a
> > > big deal though, so
> > >
> > > Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > >
> > > but I don't want to continue in this direction with every new kernel
> > > release. We need a way to tell smatch that this is safe with incurring
> > > an runtime cost.
> >
> > To provide some context, in this release alone we have found two "Fixes:".
> >
> > Do you have any data regarding the runtime cost of these new checks?
> > Most of the time, the compiler simply optimizes them out, so the
> > impact is either zero or minimal.
>
> If the compiler can optimize them out, then I expect the static analysis
> tools to also get the ability to avoid those false positives.

Ideally yes, but LLVM/gcc has hundreds (maybe thousands?) of
contributors and sparse/smatch are maintained by a handful group of
heroic warriors.

>
> > The added checks make the code more robust for future refactoring and
> > serve as useful documentation.
>
> In this specific case, if we want to make the code more robust, I think
> we should have an array of minor offsets and calculate the minor ranges
> based on that. It would avoid offsets and ranges getting out of sync. I
> don't think it's worth it though, because I don't foresee we will ever
> change ranges for the fixed minors case.
>
> > Furthermore, when false positives occur
> > in new code, they force the author to write more idiomatic code, which
> > improves maintainability.
>
> Up to a point only. Sometimes guarantees are provided in a very remote
> place. For instance, control values have ranges that are set when
> creating the control, and enforced by the control framework. Adding
> range checks to .s_ctrl() is redundant, but I don't expect static
> analysis tools to be able to understand that the value is guaranteed to
> be within the range (especially given that we support modifying ranges
> at runtime). How can we avoid those redundant checks (or mask
> operations, as in the mt9p031 patch in this series) ?

It depends on the nature of the range check and the consequences of
failing it. We can decide case by case and worst case scenario we can
ignore specific errors.


The mt9p031 patch in this series is special, the issue is not the
range check, but that smatch did not properly handle a mask operation.
Dan is working to fix it.



>
> > We have a deficit of maintainers, not
> > authors.
> >
> > So, I DO want to continue in this direction. I am very grateful for
> > these static analysis tools; they truly make our code more
> > maintainable.
>
> I do like smatch a lot, it points out real issues. My concern is that we
> have code that offers guarantees in ways that are not visible to static
> analysis tools (or to compilers), and having to add more runtime checks
> everywhere to silence warning is not nice. Those checks can over time
> become redundant (because tools improve, or because we remove the code),
> and will likely not be removed because nobody notices.
>
> There's a broader question here of how to connect pieces of code where
> one piece offers a guarantee that the other piece depends on, in a way
> that can be exposed to tools.

I agree with the idea, but this might not the right forum. It would be
a great topic for the Kernel Summit, though.

The "rust people" would love to have some annotations about the
guarantees of the C code.

But until we have those annotations the best we can do is runtime
checks where they make sense and keep track of where they do not make
sense (allow lists). That will make future discussions more
productive.

>
> > > >       /* Should not happen since we thought this minor was free */
> > > >       if (WARN_ON(video_devices[vdev->minor])) {
> > > >               mutex_unlock(&videodev_lock);
> > > >
>
> --
> Regards,
>
> Laurent Pinchart



-- 
Ricardo Ribalda

Re: [PATCH v3 1/6] media: v4l2-dev: Add range check for vdev->minor

[Laurent Pinchart]

On Wed, May 06, 2026 at 02:12:47PM +0200, Ricardo Ribalda wrote:
> On Wed, 6 May 2026 at 13:18, Laurent Pinchart wrote:
> > On Wed, May 06, 2026 at 08:48:06AM +0200, Ricardo Ribalda wrote:
> > > On Wed, 6 May 2026 at 01:12, Laurent Pinchart wrote:
> > > > On Mon, May 04, 2026 at 06:54:04AM +0000, Ricardo Ribalda wrote:
> > > > > If the fixed minor ranges are not properly set we could end up in a
> > > > > situation where the calculated minor is invalid. Add a check for this in
> > > > > the code to make it more robust.
> > > >
> > > > If it was just for that, we could define the ranges in a way that could
> > > > not lead to future programmatic errors.
> > > >
> > > > > This check also fixes the following false positive smatch warning:
> > > > >
> > > > > drivers/media/v4l2-core/v4l2-dev.c:1036 __video_register_device() error: buffer overflow 'video_devices' 256 <= 288
> > > > > drivers/media/v4l2-core/v4l2-dev.c:1043 __video_register_device() error: buffer overflow 'video_devices' 256 <= 288
> > > > > drivers/media/v4l2-core/v4l2-dev.c:1101 __video_register_device() error: buffer overflow 'video_devices' 256 <= 288
> > > > >
> > > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > > > > ---
> > > > >  drivers/media/v4l2-core/v4l2-dev.c | 5 +++++
> > > > >  1 file changed, 5 insertions(+)
> > > > >
> > > > > diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
> > > > > index 6ce623a1245a..5516b2bbb08f 100644
> > > > > --- a/drivers/media/v4l2-core/v4l2-dev.c
> > > > > +++ b/drivers/media/v4l2-core/v4l2-dev.c
> > > > > @@ -1032,6 +1032,11 @@ int __video_register_device(struct video_device *vdev,
> > > > >       vdev->minor = i + minor_offset;
> > > > >       vdev->num = nr;
> > > > >
> > > > > +     if (WARN_ON(vdev->minor >= VIDEO_NUM_DEVICES)) {
> > > > > +             mutex_unlock(&videodev_lock);
> > > >
> > > > I may get tempted to convert code to using scoped guards at some point.
> > > >
> > > > > +             return -EINVAL;
> > > > > +     }
> > > > > +
> > > >
> > > > I'm annoyed by the proliferation of workarounds for smatch false
> > > > positives that generate useless code :-/ This is in particular is not a
> > > > big deal though, so
> > > >
> > > > Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > > >
> > > > but I don't want to continue in this direction with every new kernel
> > > > release. We need a way to tell smatch that this is safe with incurring
> > > > an runtime cost.
> > >
> > > To provide some context, in this release alone we have found two "Fixes:".
> > >
> > > Do you have any data regarding the runtime cost of these new checks?
> > > Most of the time, the compiler simply optimizes them out, so the
> > > impact is either zero or minimal.
> >
> > If the compiler can optimize them out, then I expect the static analysis
> > tools to also get the ability to avoid those false positives.
> 
> Ideally yes, but LLVM/gcc has hundreds (maybe thousands?) of
> contributors and sparse/smatch are maintained by a handful group of
> heroic warriors.

What I meant is that I don't think compilers will optimize this one out.

> > > The added checks make the code more robust for future refactoring and
> > > serve as useful documentation.
> >
> > In this specific case, if we want to make the code more robust, I think
> > we should have an array of minor offsets and calculate the minor ranges
> > based on that. It would avoid offsets and ranges getting out of sync. I
> > don't think it's worth it though, because I don't foresee we will ever
> > change ranges for the fixed minors case.
> >
> > > Furthermore, when false positives occur
> > > in new code, they force the author to write more idiomatic code, which
> > > improves maintainability.
> >
> > Up to a point only. Sometimes guarantees are provided in a very remote
> > place. For instance, control values have ranges that are set when
> > creating the control, and enforced by the control framework. Adding
> > range checks to .s_ctrl() is redundant, but I don't expect static
> > analysis tools to be able to understand that the value is guaranteed to
> > be within the range (especially given that we support modifying ranges
> > at runtime). How can we avoid those redundant checks (or mask
> > operations, as in the mt9p031 patch in this series) ?
> 
> It depends on the nature of the range check and the consequences of
> failing it. We can decide case by case and worst case scenario we can
> ignore specific errors.
> 
> The mt9p031 patch in this series is special, the issue is not the
> range check, but that smatch did not properly handle a mask operation.
> Dan is working to fix it.

In the mt9p031 case, we have

	ctrl->val &= ~7;
	data = ((ctrl->val - 64) << 5) | (1 << 6) | 32;

ctrl->val is a 32-bit value, data is a 16-bit value. We know that
ctrl->val will be at most 1024, because that's the maximum limit set
when creating the control, but I don't see how smatch or a compiler
would be able to see that. I'm not blaming smatch on this one.

> > > We have a deficit of maintainers, not
> > > authors.
> > >
> > > So, I DO want to continue in this direction. I am very grateful for
> > > these static analysis tools; they truly make our code more
> > > maintainable.
> >
> > I do like smatch a lot, it points out real issues. My concern is that we
> > have code that offers guarantees in ways that are not visible to static
> > analysis tools (or to compilers), and having to add more runtime checks
> > everywhere to silence warning is not nice. Those checks can over time
> > become redundant (because tools improve, or because we remove the code),
> > and will likely not be removed because nobody notices.
> >
> > There's a broader question here of how to connect pieces of code where
> > one piece offers a guarantee that the other piece depends on, in a way
> > that can be exposed to tools.
> 
> I agree with the idea, but this might not the right forum. It would be
> a great topic for the Kernel Summit, though.

I was thinking about the same, yes.

> The "rust people" would love to have some annotations about the
> guarantees of the C code.

I wonder if that could be used to offer similar guarantees (or rather a
subset thereof) of the rust type system. Note that I'd expect rust to
also struggle in many cases, as we really have guarantees that won't be
expressable through its type system.

> But until we have those annotations the best we can do is runtime
> checks where they make sense and keep track of where they do not make
> sense (allow lists). That will make future discussions more
> productive.

Should we add comments on top of all those redundant checks, with a
standardized formatting to make them easy to locate in the future ? For
instance

	/* false-positive: Should never happen due to ... */

> > > > >       /* Should not happen since we thought this minor was free */
> > > > >       if (WARN_ON(video_devices[vdev->minor])) {
> > > > >               mutex_unlock(&videodev_lock);
> > > > >

-- 
Regards,

Laurent Pinchart