On 16/10/2024 13:24, Mauro Carvalho Chehab wrote:

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.

The ioctl, yes, but what it logs to the kernel log isn't part of the ABI. That can change.

I think it is overkill to send this to stable for an old chip that almost nobody uses, and that requires an i2c read to go wrong for it to produce a wrong debug message. It seems an unnecessary waste of time.

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.

But patch 13/13 doesn't affect the operation either, again it is just an incorrect log message that can only go wrong if Pulse-Eight still sells that device in 2038 with a firmware build date >= 2038. And v6.12 is guaranteed to be EOL in 2038 :-)

Regards,

Hans

...

...

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