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(-)
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]);
--
2.47.0On 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
© 2016 - 2024 Red Hat, Inc.