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

[Jacopo Mondi <jacopo.mondi@ideasonboard.com>]

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
>
> >