Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 7 Nov 2022 20:40:27 +0530
From:   Deepak R Varma <drv@mailo.com>
To:     Dan Carpenter <error27@gmail.com>
Cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        linux-staging@lists.linux.dev, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] staging: most: video: use min_t() for comparison and
 assignment
Message-ID: <Y2kf40kSbFWkWkLl@qemulion>
References: <Y2iHl5CuqyR2vEc8@qemulion>
 <Y2kGG3zuvvDLZKWq@kadam>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <Y2kGG3zuvvDLZKWq@kadam>
Precedence: bulk

On Mon, Nov 07, 2022 at 04:20:27PM +0300, Dan Carpenter wrote:
> On Mon, Nov 07, 2022 at 09:50:39AM +0530, Deepak R Varma wrote:
> > Simplify code by using min_t helper macro for logical evaluation
> > and value assignment. Use the _t variant of min macro since the
> > variable types are not same.
> > This issue is identified by coccicheck using the minmax.cocci file.
> >
> > Signed-off-by: Deepak R Varma <drv@mailo.com>
> > ---
> >
> > Changes in v2:
> >    1. Revise patch description. No functional change.
> >
> >  drivers/staging/most/video/video.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/staging/most/video/video.c b/drivers/staging/most/video/video.c
> > index ffa97ef21ea5..d5cc7eea3b52 100644
> > --- a/drivers/staging/most/video/video.c
> > +++ b/drivers/staging/most/video/video.c
> > @@ -173,7 +173,7 @@ static ssize_t comp_vdev_read(struct file *filp, char __user *buf,
> >  	while (count > 0 && data_ready(mdev)) {
> >  		struct mbo *const mbo = get_top_mbo(mdev);
> >  		int const rem = mbo->processed_length - fh->offs;
> > -		int const cnt = rem < count ? rem : count;
> > +		int const cnt = min_t(int, rem, count);
>
> TL;DR use size_t instead of int.

Hi Dan,
Thank you for reviewing the patch. Please see my queries inline.

>
> Using "int" here is wrong.  size_t is unsigned long meaning that it has
> 64 bits to use to represent positive values.  (Let's ignore 32 bit
> arches).  You have chopped it down to say that it now has 31 bits for
> positives and if BIT(31) is set then treat it as negative.  Everything
> which is larger than INT_MAX will be broken.

I did worry about the truncation int might cause to the size_t variable,
however, as the result is being assigned to an int, I decided to go for int to
be the typecast for min_t.

Also, won't size_t will force the int rem to be treated as unsigned value which
will impact the comparison when rem indeed is negative. If rem will never be
-ve, my worry will be void.

Can you please correct if my understanding is wrong?

Thanks,
./drv

>
> Fortunately, in this code the value of count will never go higher than
> "INT_MAX - PAGE_SIZE" because Linus understands that it's easy to
> introduce bugs like this.
>
> regards,
> dan carpenter
>
>