Re: [PATCH 10/13] media: adv7604 prevent underflow condition when reporting colorspace

[Mauro Carvalho Chehab <mchehab+huawei@kernel.org>]

Em Wed, 16 Oct 2024 12:57:53 +0200
Hans Verkuil <hverkuil@xs4all.nl> escreveu:

> On 16/10/2024 12:22, Mauro Carvalho Chehab wrote:
> > Currently, adv76xx_log_status() reads some date using
> > io_read() which may return negative values. The current logi
> > doesn't check such errors, causing colorspace to be reported
> > on a wrong way at adv76xx_log_status().
> > 
> > If I/O error happens there, print a different message, instead
> > of reporting bogus messages to userspace.
> > 
> > Fixes: 54450f591c99 ("[media] adv7604: driver for the Analog Devices ADV7604 video decoder")
> > Cc: stable@vger.kernel.org  
> 
> Not really a fix since this would just affect logging for debugging
> purposes. I would personally just drop the Fixes and Cc tag.

The issue is on a VIDIOC_ ioctl, so part of media API. Ok, this is
used only for debugging purposes and should, instead be implemented
via debugfs, etc, but, in summary: it is what it is: part of the V4L2
uAPI.

-

Now, the question about what should have Fixes: tag and what
shouldn't is a different matter. I've saw long discussions about
that at the kernel mailing lists. In the particular case of y2038,
I'm pretty sure I saw some of them with Fixes tag on it.

> 
> > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>  
> 
> Reviewed-by: Hans Verkuil <hverkuil@xs4all.nl>
> 
> Regards,
> 
> 	Hans
> 
> > ---
> >  drivers/media/i2c/adv7604.c | 26 +++++++++++++++++---------
> >  1 file changed, 17 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/media/i2c/adv7604.c b/drivers/media/i2c/adv7604.c
> > index 48230d5109f0..272945a878b3 100644
> > --- a/drivers/media/i2c/adv7604.c
> > +++ b/drivers/media/i2c/adv7604.c
> > @@ -2519,10 +2519,10 @@ static int adv76xx_log_status(struct v4l2_subdev *sd)
> >  	const struct adv76xx_chip_info *info = state->info;
> >  	struct v4l2_dv_timings timings;
> >  	struct stdi_readback stdi;
> > -	u8 reg_io_0x02 = io_read(sd, 0x02);
> > +	int ret;
> > +	u8 reg_io_0x02;
> >  	u8 edid_enabled;
> >  	u8 cable_det;
> > -
> >  	static const char * const csc_coeff_sel_rb[16] = {
> >  		"bypassed", "YPbPr601 -> RGB", "reserved", "YPbPr709 -> RGB",
> >  		"reserved", "RGB -> YPbPr601", "reserved", "RGB -> YPbPr709",
> > @@ -2621,13 +2621,21 @@ static int adv76xx_log_status(struct v4l2_subdev *sd)
> >  	v4l2_info(sd, "-----Color space-----\n");
> >  	v4l2_info(sd, "RGB quantization range ctrl: %s\n",
> >  			rgb_quantization_range_txt[state->rgb_quantization_range]);
> > -	v4l2_info(sd, "Input color space: %s\n",
> > -			input_color_space_txt[reg_io_0x02 >> 4]);
> > -	v4l2_info(sd, "Output color space: %s %s, alt-gamma %s\n",
> > -			(reg_io_0x02 & 0x02) ? "RGB" : "YCbCr",
> > -			(((reg_io_0x02 >> 2) & 0x01) ^ (reg_io_0x02 & 0x01)) ?
> > -				"(16-235)" : "(0-255)",
> > -			(reg_io_0x02 & 0x08) ? "enabled" : "disabled");
> > +
> > +	ret = io_read(sd, 0x02);
> > +	if (ret < 0) {
> > +		v4l2_info(sd, "Can't read Input/Output color space\n");
> > +	} else {
> > +		reg_io_0x02 = ret;
> > +
> > +		v4l2_info(sd, "Input color space: %s\n",
> > +				input_color_space_txt[reg_io_0x02 >> 4]);
> > +		v4l2_info(sd, "Output color space: %s %s, alt-gamma %s\n",
> > +				(reg_io_0x02 & 0x02) ? "RGB" : "YCbCr",
> > +				(((reg_io_0x02 >> 2) & 0x01) ^ (reg_io_0x02 & 0x01)) ?
> > +					"(16-235)" : "(0-255)",
> > +				(reg_io_0x02 & 0x08) ? "enabled" : "disabled");
> > +	}
> >  	v4l2_info(sd, "Color space conversion: %s\n",
> >  			csc_coeff_sel_rb[cp_read(sd, info->cp_csc) >> 4]);
> >    
> 



Thanks,
Mauro