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

[Jacopo Mondi]

Hi Vincenzo
  thanks for the review

On Mon, Jun 29, 2026 at 08:52:09AM +0100, Vincenzo Frascino wrote:
> Hi Jacopo,
>
> thank you for your patch.
>
> On 27/06/2026 15:29, Jacopo Mondi wrote:
> > From: Jacopo Mondi <jacopo.mondi+renesas@xxxxxxxxxxxxxxxx>
> >
> > 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.
> >
> > Signed-off-by: Jacopo Mondi <jacopo.mondi+renesas@xxxxxxxxxxxxxxxx>
> >
>
> I have a couple of comments. With this:
>
> Reviewed-by: Vincenzo Frascino <vincenzo.frascino@xxxxxxx>
>
> > ---
> > v3:
> > - new patch
> > ---
> >  .../media/platform/arm/mali-c55/mali-c55-params.c  | 36 ++++++++++++++++++++++
> >  1 file changed, 36 insertions(+)
> >
> > diff --git a/drivers/media/platform/arm/mali-c55/mali-c55-params.c b/drivers/media/platform/arm/mali-c55/mali-c55-params.c
> > index 0da5215c52c3..333e66ee3923 100644
> > --- a/drivers/media/platform/arm/mali-c55/mali-c55-params.c
> > +++ b/drivers/media/platform/arm/mali-c55/mali-c55-params.c
> > @@ -477,6 +477,41 @@ static const mali_c55_params_handler mali_c55_params_handlers[] = {
> >  	[MALI_C55_PARAM_BLOCK_CCM] = &mali_c55_params_ccm,
> >  };
> >
> > +static int mali_c55_ccm_validate(struct device *dev,
> > +				 const struct v4l2_isp_block_header *block)
> > +{
> > +	const struct mali_c55_params_ccm *ccm =
> > +		(const struct mali_c55_params_ccm *)(block);
> > +
> > +	for (unsigned int i = 0; i < 3; i++) {
> > +
>
> Nit: extra blank line after the for opening brace, kernel style usually avoids this.
>

I know, checkpatch reports this as well, but as I have three
validation 'blocks' it felt nicer this way. I can drop the blank line
though

> > +		for (unsigned int j = 0; j < 3; j++) {
> > +			/* Coefficients are 13 bits signed Q4.8. */
> > +			if (ccm->coeffs[i][j] > 8191) {
>
> No magic numbers please :) Maybe use GENMASK(12, 0) or BIT(13) - 1 for
> readability and consistency with the commit message, and similarly GENMASK(11,
> 0) or BIT(12) - 1 for gains and offsets. This also avoids magic numbers in
> validation. The same could be applied to the other cases in this patch.
>

Nice suggestion. I used comments to explain the meaning of 8191 and
4095 but indeed using GENMASK() is certainly more explicit.

I'll do so.

> > +				dev_dbg(dev, "Invalid ccm coefficient %u\n",
> > +					ccm->coeffs[i][j]);
> > +				return -EINVAL;
> > +			}
> > +		}
> > +
> > +		/* Gains are 12 bits unsigned Q4.8. */
> > +		if (ccm->gains[i] > 4095) {
> > +			dev_dbg(dev, "Invalid ccm gain %u\n",
> > +				ccm->gains[i]);
> > +			return -EINVAL;
> > +		}
> > +
> > +		/* Offsets are 12 bits unsigned integers. */
> > +		if (ccm->offs[i] > 4095) {
> > +			dev_dbg(dev, "Invalid ccm offset %u\n",
> > +				ccm->offs[i]);
> > +			return -EINVAL;
> > +		}
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  static const struct v4l2_isp_params_block_type_info
> >  mali_c55_params_block_types_info[] = {
> >  	[MALI_C55_PARAM_BLOCK_SENSOR_OFFS] = {
> > @@ -514,6 +549,7 @@ mali_c55_params_block_types_info[] = {
> >  	},
> >  	[MALI_C55_PARAM_BLOCK_CCM] = {
> >  		.size = sizeof(struct mali_c55_params_ccm),
> > +		.block_validate = mali_c55_ccm_validate,
> >  	},
> >  };
> >
> >
>
> --
> Regards,
> Vincenzo
>