| Message ID | 41d12c1afd6571f9cc56c1b920df6ba558d0b927.1729074076.git.mchehab+huawei@kernel.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Media: fix several issues on drivers | expand |
Em Wed, 16 Oct 2024 13:58:48 +0200 Hans Verkuil <hverkuil@xs4all.nl> escreveu: > 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. Sure, logs can change, but this is an user-visible bug. > 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. Agreed. Will drop cc stable. > > > > - > > > > 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 :-) We can't count on it. Civil infrastructure is now working with a 10 years SLTS: https://www.linuxfoundation.org/press/civil-infrastructure-platform-expands-slt-stable-kernel-program I heard somewhere that having a 15 years or 20 years stable Kernel is a need for certain usages. Even commercial distros have a minimum of 10 years as LTS. Suse is now working with a 13-years support. Both Canonical and Red Hat announced a 12-years ELTS support. As they usually takes the last year's LTS Kernel, it means support will end with a 14 years old Kernel (so, support will end in 2037 or 2038 if they release an LTS distro next year), and don't decide to extend it further. I also heard during LPC that there's an increased pressure from Linux customers from commercial distros to extend it even further. Thanks, Mauro
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]);
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 Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org> --- drivers/media/i2c/adv7604.c | 26 +++++++++++++++++--------- 1 file changed, 17 insertions(+), 9 deletions(-)