Re: [PATCH v3 2/4] media: mali-c55: Implement CCM block validation

[Vincenzo Frascino <vincenzo.frascino@arm.com>]

Hi Laurent,

Thank you for your answer.

On 29/06/2026 13:05, Laurent Pinchart wrote:
> On Mon, Jun 29, 2026 at 12:08:08PM +0100, Vincenzo Frascino wrote:
>> On 29/06/2026 10:57, Laurent Pinchart wrote:
>>> On Sat, Jun 27, 2026 at 04:29:14PM +0200, Jacopo Mondi wrote:
>>>> From: Jacopo Mondi <jacopo.mondi+renesas@ideasonboard.com>
>>>>
>>>> Implement validation of CCM block parameters.
>>>>
>>>> CCM coefficients are expressed as 13 bits signed Q4.8 format and their
>>>> raw value cannot be higher than 8191 (BIT(13) - 1).
>>>>
>>>> CCM gains are expressed as unsigned 12 bits Q4.8 format and their raw
>>>> value cannot be higher than 4095 (BIT(12) - 1).
>>>>
>>>> CCM offsets are 12 bits unsigned integers and their value cannot be
>>>> higher than 4095 (BIT(12) - 1).
>>>>
>>>> Validate the parameters provided by userspace using the .block_validate
>>>> callback of struct v4l2_isp_params_block_type_info.
>>> I don't think this is needed.
>>>
>>> We need to validate parameters that can cause the ISP to malfunction in
>>> ways that requires a system reset, or in ways that cause malfunction of
>>> other system components (e.g. buffer overflows, memory bus lock ups,
>>> ...). The rest doesn't need to be validated.
>>>
>>> If you want to be cautious, you can just mask the value when writing to
>>> registers, which I think you're doing in patch 1/4.
>>
>> According to me here is not a matter of being cautious, but of honouring the
>> contract with the userspace.
>>
>> If the userspace is doing something wrong it should be notified. The only
>> reasonable argument against this would be if this code is on a critical path and
>> the validations have a performance impact.
> 
> I don't agree with this. As long as it doesn't have an impact on other
> parts of the system, there's no need to notify userspace. It's purely a
> userspace issue, it's pointless to waste CPU cycles every frame.

I don't think it's only about protecting the hardware from invalid values.

The userspace API defines the valid range for these parameters. If userspace
provides values outside that range, returning an error makes the issue visible
immediately instead of silently changing the requested configuration by masking
or clamping the values. From userspace's perspective, silently accepting invalid
input can make debugging harder, as the configuration that gets applied is no
longer the one that was requested.

This also matches the documented behaviour of the V4L2 extended controls ioctl,
which specifies that EINVAL should be returned when the value of a control is
invalid:
https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/vidioc-g-ext-ctrls.html

The validation also isn't on the critical per-frame path. It's only performed
when userspace updates the parameters, so the CPU cost is negligible.

That said, if the expectation for the V4L2 ISP API is that drivers should only
validate values that could affect system stability or security, and silently
mask everything else, I'm happy to follow that approach for consistency.

> 
>> @Jacopo, can you please confirm if this is the case?
> 

-- 
Regards,
Vincenzo