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

[Sakari Ailus <sakari.ailus@linux.intel.com>]

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