[PATCH 0/5] media: use int type to store negative error codes

[PATCH 0/5] media: use int type to store negative error codes

[Qianfeng Rong]

The 'ret' variable usually is used to store returns from some functions,
which return either zero on success or negative error codes on failure.

Storing the negative error codes in unsigned type, doesn't cause an issue
at runtime but it's ugly as pants.  Change "ret" from u8/u32/unsigned int
to int type.  No effect on runtime.

Qianfeng Rong (5):
  media: dvb: use int type to store negative error codes
  media: i2c: mt9v111: use int type to store negative error codes
  media: raspberrypi: use int type to store negative error codes
  media: stm32-dcmi: use int type to store negative error codes
  media: redrat3: use int type to store negative error codes

 drivers/media/dvb-frontends/cxd2841er.c           | 3 ++-
 drivers/media/dvb-frontends/lgdt330x.c            | 4 ++--
 drivers/media/i2c/mt9v111.c                       | 2 +-
 drivers/media/platform/raspberrypi/rp1-cfe/csi2.c | 2 +-
 drivers/media/platform/st/stm32/stm32-dcmi.c      | 4 ++--
 drivers/media/rc/redrat3.c                        | 2 +-
 6 files changed, 9 insertions(+), 8 deletions(-)

-- 
2.34.1

[PATCH 1/5] media: dvb: Use int type to store negative error codes

[Qianfeng Rong]

Change the 'ret' variable from u8/u32 to int to store zero or negative
error codes returned by other functions.

Storing the negative error codes in unsigned type, doesn't cause an issue
at runtime but it's ugly as pants.

No effect on runtime.

Signed-off-by: Qianfeng Rong <rongqianfeng@vivo.com>
---
 drivers/media/dvb-frontends/cxd2841er.c | 3 ++-
 drivers/media/dvb-frontends/lgdt330x.c  | 4 ++--
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/media/dvb-frontends/cxd2841er.c b/drivers/media/dvb-frontends/cxd2841er.c
index 415f1f91cc30..8fcb4417ba22 100644
--- a/drivers/media/dvb-frontends/cxd2841er.c
+++ b/drivers/media/dvb-frontends/cxd2841er.c
@@ -1936,7 +1936,8 @@ static void cxd2841er_read_ber(struct dvb_frontend *fe)
 {
 	struct dtv_frontend_properties *p = &fe->dtv_property_cache;
 	struct cxd2841er_priv *priv = fe->demodulator_priv;
-	u32 ret, bit_error = 0, bit_count = 0;
+	u32 bit_error = 0, bit_count = 0;
+	int ret;
 
 	dev_dbg(&priv->i2c->dev, "%s()\n", __func__);
 	switch (p->delivery_system) {
diff --git a/drivers/media/dvb-frontends/lgdt330x.c b/drivers/media/dvb-frontends/lgdt330x.c
index cab442a350a5..8c34a5b850bc 100644
--- a/drivers/media/dvb-frontends/lgdt330x.c
+++ b/drivers/media/dvb-frontends/lgdt330x.c
@@ -124,7 +124,6 @@ static int i2c_read_demod_bytes(struct lgdt330x_state *state,
 /* Software reset */
 static int lgdt3302_sw_reset(struct lgdt330x_state *state)
 {
-	u8 ret;
 	u8 reset[] = {
 		IRQ_MASK,
 		/*
@@ -133,6 +132,7 @@ static int lgdt3302_sw_reset(struct lgdt330x_state *state)
 		 */
 		0x00
 	};
+	int ret;
 
 	ret = i2c_write_demod_bytes(state,
 				    reset, sizeof(reset));
@@ -147,11 +147,11 @@ static int lgdt3302_sw_reset(struct lgdt330x_state *state)
 
 static int lgdt3303_sw_reset(struct lgdt330x_state *state)
 {
-	u8 ret;
 	u8 reset[] = {
 		0x02,
 		0x00 /* bit 0 is active low software reset */
 	};
+	int ret;
 
 	ret = i2c_write_demod_bytes(state,
 				    reset, sizeof(reset));
-- 
2.34.1

[PATCH 2/5] media: i2c: mt9v111: fix incorrect type for ret

[Qianfeng Rong]

Change "ret" from unsigned int to int type in mt9v111_calc_frame_rate()
to store negative error codes or zero returned by __mt9v111_hw_reset()
and other functions.

Storing the negative error codes in unsigned type, doesn't cause an issue
at runtime but it's ugly as pants.

No effect on runtime.

Signed-off-by: Qianfeng Rong <rongqianfeng@vivo.com>
---
 drivers/media/i2c/mt9v111.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/i2c/mt9v111.c b/drivers/media/i2c/mt9v111.c
index 6aa80b504168..9d724a7cd2f5 100644
--- a/drivers/media/i2c/mt9v111.c
+++ b/drivers/media/i2c/mt9v111.c
@@ -532,8 +532,8 @@ static int mt9v111_calc_frame_rate(struct mt9v111_dev *mt9v111,
 static int mt9v111_hw_config(struct mt9v111_dev *mt9v111)
 {
 	struct i2c_client *c = mt9v111->client;
-	unsigned int ret;
 	u16 outfmtctrl2;
+	int ret;
 
 	/* Force device reset. */
 	ret = __mt9v111_hw_reset(mt9v111);
-- 
2.34.1

[PATCH 3/5] media: raspberrypi: use int type to store negative error codes

[Qianfeng Rong]

Use int instead of unsigned int for the 'ret' variable in csi2_init() to
store negative error codes or zero returned by media_entity_pads_init().

No effect on runtime.

Signed-off-by: Qianfeng Rong <rongqianfeng@vivo.com>
---
 drivers/media/platform/raspberrypi/rp1-cfe/csi2.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/platform/raspberrypi/rp1-cfe/csi2.c b/drivers/media/platform/raspberrypi/rp1-cfe/csi2.c
index 35c2ab1e2cd4..2c5b4d24b4e6 100644
--- a/drivers/media/platform/raspberrypi/rp1-cfe/csi2.c
+++ b/drivers/media/platform/raspberrypi/rp1-cfe/csi2.c
@@ -525,7 +525,7 @@ static const struct v4l2_subdev_internal_ops csi2_internal_ops = {
 
 int csi2_init(struct csi2_device *csi2, struct dentry *debugfs)
 {
-	unsigned int ret;
+	int ret;
 
 	spin_lock_init(&csi2->errors_lock);
 
-- 
2.34.1

[PATCH 4/5] media: stm32-dcmi: use int type to store negative error codes

[Qianfeng Rong]

Change "ret" from unsigned int to int type in dcmi_framesizes_init()
and dcmi_graph_notify_bound() to store negative error codes or zero
returned by v4l2_subdev_call() and media_create_pad_link() - this
better aligns with the coding standards and maintains code consistency.

No effect on runtime.

Signed-off-by: Qianfeng Rong <rongqianfeng@vivo.com>
---
 drivers/media/platform/st/stm32/stm32-dcmi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/st/stm32/stm32-dcmi.c b/drivers/media/platform/st/stm32/stm32-dcmi.c
index d94c61b8569d..13762861b769 100644
--- a/drivers/media/platform/st/stm32/stm32-dcmi.c
+++ b/drivers/media/platform/st/stm32/stm32-dcmi.c
@@ -1701,8 +1701,8 @@ static int dcmi_framesizes_init(struct stm32_dcmi *dcmi)
 		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
 		.code = dcmi->sd_format->mbus_code,
 	};
-	unsigned int ret;
 	unsigned int i;
+	int ret;
 
 	/* Allocate discrete framesizes array */
 	while (!v4l2_subdev_call(subdev, pad, enum_frame_size,
@@ -1808,8 +1808,8 @@ static int dcmi_graph_notify_bound(struct v4l2_async_notifier *notifier,
 				   struct v4l2_async_connection *asd)
 {
 	struct stm32_dcmi *dcmi = notifier_to_dcmi(notifier);
-	unsigned int ret;
 	int src_pad;
+	int ret;
 
 	dev_dbg(dcmi->dev, "Subdev \"%s\" bound\n", subdev->name);
 
-- 
2.34.1

[PATCH 5/5] media: redrat3: use int type to store negative error codes

[Qianfeng Rong]

Change "ret" from u8 to int type in redrat3_enable_detector() to store
negative error codes or zero returned by redrat3_send_cmd() and
usb_submit_urb() - this better aligns with the coding standards and
maintains code consistency.

No effect on runtime.

Signed-off-by: Qianfeng Rong <rongqianfeng@vivo.com>
---
 drivers/media/rc/redrat3.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/rc/redrat3.c b/drivers/media/rc/redrat3.c
index d89a4cfe3c89..a49173f54a4d 100644
--- a/drivers/media/rc/redrat3.c
+++ b/drivers/media/rc/redrat3.c
@@ -422,7 +422,7 @@ static int redrat3_send_cmd(int cmd, struct redrat3_dev *rr3)
 static int redrat3_enable_detector(struct redrat3_dev *rr3)
 {
 	struct device *dev = rr3->dev;
-	u8 ret;
+	int ret;
 
 	ret = redrat3_send_cmd(RR3_RC_DET_ENABLE, rr3);
 	if (ret != 0)
-- 
2.34.1

Re: [PATCH 2/5] media: i2c: mt9v111: fix incorrect type for ret

[Jacopo Mondi]

Hi Qianfeng

On Wed, Aug 27, 2025 at 08:39:10PM +0800, Qianfeng Rong wrote:
> Change "ret" from unsigned int to int type in mt9v111_calc_frame_rate()
> to store negative error codes or zero returned by __mt9v111_hw_reset()
> and other functions.
>
> Storing the negative error codes in unsigned type, doesn't cause an issue
> at runtime but it's ugly as pants.
>
> No effect on runtime.

well, I'm not sure that's true.

The code goes as

	ret = __mt9v111_hw_reset(mt9v111);
	if (ret == -EINVAL)
		ret = __mt9v111_sw_reset(mt9v111);
	if (ret)
		return ret;

And if ret is unsigned the condition ret == -EINVAL will never be met.

I guess this actually fixes a bug, doesn't it ?

>
> Signed-off-by: Qianfeng Rong <rongqianfeng@vivo.com>

You can add:

Cc: stable@vger.kernel.org
Fixes: aab7ed1c3927 ("media: i2c: Add driver for Aptina MT9V111")
Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

Thanks
  j


> ---
>  drivers/media/i2c/mt9v111.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/media/i2c/mt9v111.c b/drivers/media/i2c/mt9v111.c
> index 6aa80b504168..9d724a7cd2f5 100644
> --- a/drivers/media/i2c/mt9v111.c
> +++ b/drivers/media/i2c/mt9v111.c
> @@ -532,8 +532,8 @@ static int mt9v111_calc_frame_rate(struct mt9v111_dev *mt9v111,
>  static int mt9v111_hw_config(struct mt9v111_dev *mt9v111)
>  {
>  	struct i2c_client *c = mt9v111->client;
> -	unsigned int ret;
>  	u16 outfmtctrl2;
> +	int ret;
>
>  	/* Force device reset. */
>  	ret = __mt9v111_hw_reset(mt9v111);
> --
> 2.34.1
>

Re: [PATCH 2/5] media: i2c: mt9v111: fix incorrect type for ret

[Qianfeng Rong]

在 2025/8/27 20:58, Jacopo Mondi 写道:
> [You don't often get email from jacopo.mondi@ideasonboard.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>
> Hi Qianfeng
>
> On Wed, Aug 27, 2025 at 08:39:10PM +0800, Qianfeng Rong wrote:
>> Change "ret" from unsigned int to int type in mt9v111_calc_frame_rate()
>> to store negative error codes or zero returned by __mt9v111_hw_reset()
>> and other functions.
>>
>> Storing the negative error codes in unsigned type, doesn't cause an issue
>> at runtime but it's ugly as pants.
>>
>> No effect on runtime.
> well, I'm not sure that's true.
>
> The code goes as
>
>          ret = __mt9v111_hw_reset(mt9v111);
>          if (ret == -EINVAL)
>                  ret = __mt9v111_sw_reset(mt9v111);
>          if (ret)
>                  return ret;
>
> And if ret is unsigned the condition ret == -EINVAL will never be met.
>
> I guess this actually fixes a bug, doesn't it ?
> You can add:
>
> Cc: stable@vger.kernel.org
> Fixes: aab7ed1c3927 ("media: i2c: Add driver for Aptina MT9V111")
> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
>
> Thanks
>    j

I have written a test program on the arm64 platform:

unsigned int ret = -ENOMEM;

if (ret == -ENOMEM)
     pr_info("unsigned int ret(%u) == -ENOMEM\n", ret);
else
     pr_info("unsigned int ret(%u) != -ENOMEM\n", ret);

Output log is: unsigned int ret(4294967284) == -ENOMEM

I suspect that -ENOMEM is forcibly converted to an unsigned type during the
comparison, but I am not sure if this behavior is consistent across all
platforms and compilers. Therefore, I agree that your suggestion and will
submit the v2 version.

Best regards,
Qianfeng

Re: [PATCH 2/5] media: i2c: mt9v111: fix incorrect type for ret

[Jacopo Mondi]

Hi Qianfeng

On Wed, Aug 27, 2025 at 11:41:26PM +0800, Qianfeng Rong wrote:
>
> 在 2025/8/27 20:58, Jacopo Mondi 写道:
> > [You don't often get email from jacopo.mondi@ideasonboard.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> >
> > Hi Qianfeng
> >
> > On Wed, Aug 27, 2025 at 08:39:10PM +0800, Qianfeng Rong wrote:
> > > Change "ret" from unsigned int to int type in mt9v111_calc_frame_rate()
> > > to store negative error codes or zero returned by __mt9v111_hw_reset()
> > > and other functions.
> > >
> > > Storing the negative error codes in unsigned type, doesn't cause an issue
> > > at runtime but it's ugly as pants.
> > >
> > > No effect on runtime.
> > well, I'm not sure that's true.
> >
> > The code goes as
> >
> >          ret = __mt9v111_hw_reset(mt9v111);
> >          if (ret == -EINVAL)
> >                  ret = __mt9v111_sw_reset(mt9v111);
> >          if (ret)
> >                  return ret;
> >
> > And if ret is unsigned the condition ret == -EINVAL will never be met.
> >
> > I guess this actually fixes a bug, doesn't it ?
> > You can add:
> >
> > Cc: stable@vger.kernel.org
> > Fixes: aab7ed1c3927 ("media: i2c: Add driver for Aptina MT9V111")
> > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> >
> > Thanks
> >    j
>
> I have written a test program on the arm64 platform:
>
> unsigned int ret = -ENOMEM;
>
> if (ret == -ENOMEM)
>     pr_info("unsigned int ret(%u) == -ENOMEM\n", ret);
> else
>     pr_info("unsigned int ret(%u) != -ENOMEM\n", ret);
>
> Output log is: unsigned int ret(4294967284) == -ENOMEM

Indeed, I was very wrong and ignoring the C integer promotion rules.

If I read right the "6.3.1.8 Usual arithmetic conversions" section of
the C11 specs I found freely available online (ISO/IEC 9899:201x), in
particular:

if the operand that has unsigned integer type has rank greater or
equal to the rank of the type of the other operand, then the operand with
signed integer type is converted to the type of the operand with unsigned
integer type.

Indeed the above doesn't introduce any functional change (as the
'rank' of an unsigned int is the same as the one of an int [1])

I would anyway consider it at least a "logical" fix, as comparing an
unsigned int to a negative error code is misleading for the reader at
the least.

Thanks anyway for testing.

[1] Section "6.3.1.1"
The rank of any unsigned integer type shall equal the rank of the corresponding
signed integer type, if any.

>
> I suspect that -ENOMEM is forcibly converted to an unsigned type during the
> comparison, but I am not sure if this behavior is consistent across all
> platforms and compilers. Therefore, I agree that your suggestion and will
> submit the v2 version.
>
> Best regards,
> Qianfeng
>

Re: [PATCH 2/5] media: i2c: mt9v111: fix incorrect type for ret

[Qianfeng Rong]

在 2025/8/28 0:24, Jacopo Mondi 写道:
> [You don't often get email from jacopo.mondi@ideasonboard.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>
> Hi Qianfeng
>
> On Wed, Aug 27, 2025 at 11:41:26PM +0800, Qianfeng Rong wrote:
>> 在 2025/8/27 20:58, Jacopo Mondi 写道:
>>> [You don't often get email from jacopo.mondi@ideasonboard.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>>>
>>> Hi Qianfeng
>>>
>>> On Wed, Aug 27, 2025 at 08:39:10PM +0800, Qianfeng Rong wrote:
>>>> Change "ret" from unsigned int to int type in mt9v111_calc_frame_rate()
>>>> to store negative error codes or zero returned by __mt9v111_hw_reset()
>>>> and other functions.
>>>>
>>>> Storing the negative error codes in unsigned type, doesn't cause an issue
>>>> at runtime but it's ugly as pants.
>>>>
>>>> No effect on runtime.
>>> well, I'm not sure that's true.
>>>
>>> The code goes as
>>>
>>>           ret = __mt9v111_hw_reset(mt9v111);
>>>           if (ret == -EINVAL)
>>>                   ret = __mt9v111_sw_reset(mt9v111);
>>>           if (ret)
>>>                   return ret;
>>>
>>> And if ret is unsigned the condition ret == -EINVAL will never be met.
>>>
>>> I guess this actually fixes a bug, doesn't it ?
>>> You can add:
>>>
>>> Cc: stable@vger.kernel.org
>>> Fixes: aab7ed1c3927 ("media: i2c: Add driver for Aptina MT9V111")
>>> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
>>>
>>> Thanks
>>>     j
>> I have written a test program on the arm64 platform:
>>
>> unsigned int ret = -ENOMEM;
>>
>> if (ret == -ENOMEM)
>>      pr_info("unsigned int ret(%u) == -ENOMEM\n", ret);
>> else
>>      pr_info("unsigned int ret(%u) != -ENOMEM\n", ret);
>>
>> Output log is: unsigned int ret(4294967284) == -ENOMEM
> Indeed, I was very wrong and ignoring the C integer promotion rules.
>
> If I read right the "6.3.1.8 Usual arithmetic conversions" section of
> the C11 specs I found freely available online (ISO/IEC 9899:201x), in
> particular:
>
> if the operand that has unsigned integer type has rank greater or
> equal to the rank of the type of the other operand, then the operand with
> signed integer type is converted to the type of the operand with unsigned
> integer type.
>
> Indeed the above doesn't introduce any functional change (as the
> 'rank' of an unsigned int is the same as the one of an int [1])
>
> I would anyway consider it at least a "logical" fix, as comparing an
> unsigned int to a negative error code is misleading for the reader at
> the least.
>
> Thanks anyway for testing.
>
> [1] Section "6.3.1.1"
> The rank of any unsigned integer type shall equal the rank of the corresponding
> signed integer type, if any.


Thank you for letting me know about this.  It's a great learning experience
through our discussions-cheers!

Do I still need to submit the v2 version with the following additions?

Cc: stable@vger.kernel.org
Fixes: aab7ed1c3927 ("media: i2c: Add driver for Aptina MT9V111")
Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

Best regards,
Qianfeng

>

Re: [PATCH 2/5] media: i2c: mt9v111: fix incorrect type for ret

[Jacopo Mondi]

Hi Qianfeng

On Thu, Aug 28, 2025 at 10:08:07AM +0800, Qianfeng Rong wrote:
>
> 在 2025/8/28 0:24, Jacopo Mondi 写道:
> > [You don't often get email from jacopo.mondi@ideasonboard.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> >
> > Hi Qianfeng
> >
> > On Wed, Aug 27, 2025 at 11:41:26PM +0800, Qianfeng Rong wrote:
> > > 在 2025/8/27 20:58, Jacopo Mondi 写道:
> > > > [You don't often get email from jacopo.mondi@ideasonboard.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> > > >
> > > > Hi Qianfeng
> > > >
> > > > On Wed, Aug 27, 2025 at 08:39:10PM +0800, Qianfeng Rong wrote:
> > > > > Change "ret" from unsigned int to int type in mt9v111_calc_frame_rate()
> > > > > to store negative error codes or zero returned by __mt9v111_hw_reset()
> > > > > and other functions.
> > > > >
> > > > > Storing the negative error codes in unsigned type, doesn't cause an issue
> > > > > at runtime but it's ugly as pants.
> > > > >
> > > > > No effect on runtime.
> > > > well, I'm not sure that's true.
> > > >
> > > > The code goes as
> > > >
> > > >           ret = __mt9v111_hw_reset(mt9v111);
> > > >           if (ret == -EINVAL)
> > > >                   ret = __mt9v111_sw_reset(mt9v111);
> > > >           if (ret)
> > > >                   return ret;
> > > >
> > > > And if ret is unsigned the condition ret == -EINVAL will never be met.
> > > >
> > > > I guess this actually fixes a bug, doesn't it ?
> > > > You can add:
> > > >
> > > > Cc: stable@vger.kernel.org
> > > > Fixes: aab7ed1c3927 ("media: i2c: Add driver for Aptina MT9V111")
> > > > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > > >
> > > > Thanks
> > > >     j
> > > I have written a test program on the arm64 platform:
> > >
> > > unsigned int ret = -ENOMEM;
> > >
> > > if (ret == -ENOMEM)
> > >      pr_info("unsigned int ret(%u) == -ENOMEM\n", ret);
> > > else
> > >      pr_info("unsigned int ret(%u) != -ENOMEM\n", ret);
> > >
> > > Output log is: unsigned int ret(4294967284) == -ENOMEM
> > Indeed, I was very wrong and ignoring the C integer promotion rules.
> >
> > If I read right the "6.3.1.8 Usual arithmetic conversions" section of
> > the C11 specs I found freely available online (ISO/IEC 9899:201x), in
> > particular:
> >
> > if the operand that has unsigned integer type has rank greater or
> > equal to the rank of the type of the other operand, then the operand with
> > signed integer type is converted to the type of the operand with unsigned
> > integer type.
> >
> > Indeed the above doesn't introduce any functional change (as the
> > 'rank' of an unsigned int is the same as the one of an int [1])
> >
> > I would anyway consider it at least a "logical" fix, as comparing an
> > unsigned int to a negative error code is misleading for the reader at
> > the least.
> >
> > Thanks anyway for testing.
> >
> > [1] Section "6.3.1.1"
> > The rank of any unsigned integer type shall equal the rank of the corresponding
> > signed integer type, if any.
>
>
> Thank you for letting me know about this.  It's a great learning experience
> through our discussions-cheers!
>
> Do I still need to submit the v2 version with the following additions?
>
> Cc: stable@vger.kernel.org
> Fixes: aab7ed1c3927 ("media: i2c: Add driver for Aptina MT9V111")
> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

I would do so anyway, yes.

For sure you can keep my tag ;)

Cheers!

>
> Best regards,
> Qianfeng
>
> >

Re: [PATCH 2/5] media: i2c: mt9v111: fix incorrect type for ret

[Sakari Ailus]

Hi Jacopo, Qianfeng,

On Thu, Aug 28, 2025 at 08:40:22AM +0200, Jacopo Mondi wrote:
> Hi Qianfeng
> 
> On Thu, Aug 28, 2025 at 10:08:07AM +0800, Qianfeng Rong wrote:
> >
> > 在 2025/8/28 0:24, Jacopo Mondi 写道:
> > > [You don't often get email from jacopo.mondi@ideasonboard.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> > >
> > > Hi Qianfeng
> > >
> > > On Wed, Aug 27, 2025 at 11:41:26PM +0800, Qianfeng Rong wrote:
> > > > 在 2025/8/27 20:58, Jacopo Mondi 写道:
> > > > > [You don't often get email from jacopo.mondi@ideasonboard.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> > > > >
> > > > > Hi Qianfeng
> > > > >
> > > > > On Wed, Aug 27, 2025 at 08:39:10PM +0800, Qianfeng Rong wrote:
> > > > > > Change "ret" from unsigned int to int type in mt9v111_calc_frame_rate()
> > > > > > to store negative error codes or zero returned by __mt9v111_hw_reset()
> > > > > > and other functions.
> > > > > >
> > > > > > Storing the negative error codes in unsigned type, doesn't cause an issue
> > > > > > at runtime but it's ugly as pants.
> > > > > >
> > > > > > No effect on runtime.
> > > > > well, I'm not sure that's true.
> > > > >
> > > > > The code goes as
> > > > >
> > > > >           ret = __mt9v111_hw_reset(mt9v111);
> > > > >           if (ret == -EINVAL)
> > > > >                   ret = __mt9v111_sw_reset(mt9v111);
> > > > >           if (ret)
> > > > >                   return ret;
> > > > >
> > > > > And if ret is unsigned the condition ret == -EINVAL will never be met.
> > > > >
> > > > > I guess this actually fixes a bug, doesn't it ?
> > > > > You can add:
> > > > >
> > > > > Cc: stable@vger.kernel.org
> > > > > Fixes: aab7ed1c3927 ("media: i2c: Add driver for Aptina MT9V111")
> > > > > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > > > >
> > > > > Thanks
> > > > >     j
> > > > I have written a test program on the arm64 platform:
> > > >
> > > > unsigned int ret = -ENOMEM;
> > > >
> > > > if (ret == -ENOMEM)
> > > >      pr_info("unsigned int ret(%u) == -ENOMEM\n", ret);
> > > > else
> > > >      pr_info("unsigned int ret(%u) != -ENOMEM\n", ret);
> > > >
> > > > Output log is: unsigned int ret(4294967284) == -ENOMEM
> > > Indeed, I was very wrong and ignoring the C integer promotion rules.
> > >
> > > If I read right the "6.3.1.8 Usual arithmetic conversions" section of
> > > the C11 specs I found freely available online (ISO/IEC 9899:201x), in
> > > particular:
> > >
> > > if the operand that has unsigned integer type has rank greater or
> > > equal to the rank of the type of the other operand, then the operand with
> > > signed integer type is converted to the type of the operand with unsigned
> > > integer type.
> > >
> > > Indeed the above doesn't introduce any functional change (as the
> > > 'rank' of an unsigned int is the same as the one of an int [1])
> > >
> > > I would anyway consider it at least a "logical" fix, as comparing an
> > > unsigned int to a negative error code is misleading for the reader at
> > > the least.
> > >
> > > Thanks anyway for testing.
> > >
> > > [1] Section "6.3.1.1"
> > > The rank of any unsigned integer type shall equal the rank of the corresponding
> > > signed integer type, if any.
> >
> >
> > Thank you for letting me know about this.  It's a great learning experience
> > through our discussions-cheers!
> >
> > Do I still need to submit the v2 version with the following additions?
> >
> > Cc: stable@vger.kernel.org
> > Fixes: aab7ed1c3927 ("media: i2c: Add driver for Aptina MT9V111")
> > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> 
> I would do so anyway, yes.
> 
> For sure you can keep my tag ;)

No need to resend on my behalf -- it'll be just some additional Patchwork
churn.

b4 apparently applied the rest of the tags except Cc:; I'll add that.

Thanks!

-- 
Kind regards,

Sakari Ailus

Re: [PATCH 2/5] media: i2c: mt9v111: fix incorrect type for ret

[Sakari Ailus]

On Thu, Aug 28, 2025 at 09:43:30AM +0300, Sakari Ailus wrote:
> Hi Jacopo, Qianfeng,
> 
> On Thu, Aug 28, 2025 at 08:40:22AM +0200, Jacopo Mondi wrote:
> > Hi Qianfeng
> > 
> > On Thu, Aug 28, 2025 at 10:08:07AM +0800, Qianfeng Rong wrote:
> > >
> > > 在 2025/8/28 0:24, Jacopo Mondi 写道:
> > > > [You don't often get email from jacopo.mondi@ideasonboard.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> > > >
> > > > Hi Qianfeng
> > > >
> > > > On Wed, Aug 27, 2025 at 11:41:26PM +0800, Qianfeng Rong wrote:
> > > > > 在 2025/8/27 20:58, Jacopo Mondi 写道:
> > > > > > [You don't often get email from jacopo.mondi@ideasonboard.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> > > > > >
> > > > > > Hi Qianfeng
> > > > > >
> > > > > > On Wed, Aug 27, 2025 at 08:39:10PM +0800, Qianfeng Rong wrote:
> > > > > > > Change "ret" from unsigned int to int type in mt9v111_calc_frame_rate()
> > > > > > > to store negative error codes or zero returned by __mt9v111_hw_reset()
> > > > > > > and other functions.
> > > > > > >
> > > > > > > Storing the negative error codes in unsigned type, doesn't cause an issue
> > > > > > > at runtime but it's ugly as pants.
> > > > > > >
> > > > > > > No effect on runtime.
> > > > > > well, I'm not sure that's true.
> > > > > >
> > > > > > The code goes as
> > > > > >
> > > > > >           ret = __mt9v111_hw_reset(mt9v111);
> > > > > >           if (ret == -EINVAL)
> > > > > >                   ret = __mt9v111_sw_reset(mt9v111);
> > > > > >           if (ret)
> > > > > >                   return ret;
> > > > > >
> > > > > > And if ret is unsigned the condition ret == -EINVAL will never be met.
> > > > > >
> > > > > > I guess this actually fixes a bug, doesn't it ?
> > > > > > You can add:
> > > > > >
> > > > > > Cc: stable@vger.kernel.org
> > > > > > Fixes: aab7ed1c3927 ("media: i2c: Add driver for Aptina MT9V111")
> > > > > > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > > > > >
> > > > > > Thanks
> > > > > >     j
> > > > > I have written a test program on the arm64 platform:
> > > > >
> > > > > unsigned int ret = -ENOMEM;
> > > > >
> > > > > if (ret == -ENOMEM)
> > > > >      pr_info("unsigned int ret(%u) == -ENOMEM\n", ret);
> > > > > else
> > > > >      pr_info("unsigned int ret(%u) != -ENOMEM\n", ret);
> > > > >
> > > > > Output log is: unsigned int ret(4294967284) == -ENOMEM
> > > > Indeed, I was very wrong and ignoring the C integer promotion rules.
> > > >
> > > > If I read right the "6.3.1.8 Usual arithmetic conversions" section of
> > > > the C11 specs I found freely available online (ISO/IEC 9899:201x), in
> > > > particular:
> > > >
> > > > if the operand that has unsigned integer type has rank greater or
> > > > equal to the rank of the type of the other operand, then the operand with
> > > > signed integer type is converted to the type of the operand with unsigned
> > > > integer type.
> > > >
> > > > Indeed the above doesn't introduce any functional change (as the
> > > > 'rank' of an unsigned int is the same as the one of an int [1])
> > > >
> > > > I would anyway consider it at least a "logical" fix, as comparing an
> > > > unsigned int to a negative error code is misleading for the reader at
> > > > the least.
> > > >
> > > > Thanks anyway for testing.
> > > >
> > > > [1] Section "6.3.1.1"
> > > > The rank of any unsigned integer type shall equal the rank of the corresponding
> > > > signed integer type, if any.
> > >
> > >
> > > Thank you for letting me know about this.  It's a great learning experience
> > > through our discussions-cheers!
> > >
> > > Do I still need to submit the v2 version with the following additions?
> > >
> > > Cc: stable@vger.kernel.org
> > > Fixes: aab7ed1c3927 ("media: i2c: Add driver for Aptina MT9V111")
> > > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > 
> > I would do so anyway, yes.
> > 
> > For sure you can keep my tag ;)
> 
> No need to resend on my behalf -- it'll be just some additional Patchwork
> churn.

I missed there were four other patches. If you intend to add Fixes: tags to
them, too, I'd resend the set. But if there was no actual bug, it should be
fine as-is.

-- 
Sakari Ailus