| Message ID | 41d12c1afd6571f9cc56c1b920df6ba558d0b927.1729074076.git.mchehab+huawei@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Media: fix several issues on drivers | expand |
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. > 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]); >
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
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
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(-)