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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aK_6kuoXr2Es209t@kekkonen.localdomain>
Date: Thu, 28 Aug 2025 09:43:30 +0300
From: Sakari Ailus <sakari.ailus@...ux.intel.com>
To: Jacopo Mondi <jacopo.mondi@...asonboard.com>
Cc: Qianfeng Rong <rongqianfeng@...o.com>, Jacopo Mondi <jacopo@...ndi.org>,
	Mauro Carvalho Chehab <mchehab@...nel.org>,
	"open list:MT9V111 APTINA CAMERA SENSOR" <linux-media@...r.kernel.org>,
	open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/5] media: i2c: mt9v111: fix incorrect type for ret

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@...asonboard.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@...asonboard.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@...r.kernel.org
> > > > > Fixes: aab7ed1c3927 ("media: i2c: Add driver for Aptina MT9V111")
> > > > > Reviewed-by: Jacopo Mondi <jacopo.mondi@...asonboard.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@...r.kernel.org
> > Fixes: aab7ed1c3927 ("media: i2c: Add driver for Aptina MT9V111")
> > Reviewed-by: Jacopo Mondi <jacopo.mondi@...asonboard.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